Re:Re: [PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

[Andy Yan]

Hi：

在 2023-12-14 19:36:05，"Maxime Ripard" <mripard@xxxxxxxxxx> 写道：
>On Thu, Dec 14, 2023 at 12:17:34PM +0100, Heiko Stübner wrote:
>> Am Donnerstag, 14. Dezember 2023, 12:12:08 CET schrieb Alex Bee:
>> > Hi Maxime
>> > 
>> > Am 14.12.23 um 08:56 schrieb Maxime Ripard:
>> > > Hi Alex,
>> > >
>> > > Thanks for working on this!
>> > >
>> > > On Wed, Dec 13, 2023 at 08:51:18PM +0100, Alex Bee wrote:
>> > >> The display controller will always give full range RGB regardless of the
>> > >> mode set, but HDMI requires certain modes to be transmitted in limited
>> > >> range RGB. This is especially required for HDMI sinks which do not support
>> > >> non-standard quantization ranges.
>> > >>
>> > >> This enables color space conversion for those modes and sets the
>> > >> quantization range accordingly in the AVI infoframe.
>> > >>
>> > >> Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
>> > >> Signed-off-by: Alex Bee <knaerzche@xxxxxxxxx>
>> > >> ---
>> > >>   drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------
>> > >>   1 file changed, 32 insertions(+), 8 deletions(-)
>> > >>
>> > >> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
>> > >> index 345253e033c5..32626a75723c 100644
>> > >> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
>> > >> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
>> > >> @@ -33,6 +33,7 @@ struct hdmi_data_info {
>> > >>   	unsigned int enc_in_format;
>> > >>   	unsigned int enc_out_format;
>> > >>   	unsigned int colorimetry;
>> > >> +	bool rgb_limited_range;
>> > >>   };
>> > >>   
>> > >>   struct inno_hdmi_i2c {
>> > >> @@ -308,6 +309,18 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
>> > >>   	else
>> > >>   		frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>> > >>   
>> > >> +	if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) {
>> > >> +		drm_hdmi_avi_infoframe_quant_range(&frame.avi,
>> > >> +						   &hdmi->connector, mode,
>> > >> +						   hdmi->hdmi_data.rgb_limited_range ?
>> > >> +						   HDMI_QUANTIZATION_RANGE_LIMITED :
>> > >> +						   HDMI_QUANTIZATION_RANGE_FULL);
>> > >> +	} else {
>> > >> +		frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
>> > >> +		frame.avi.ycc_quantization_range =
>> > >> +			HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
>> > >> +	}
>> > >> +
>> > >>   	return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_AVI, 0, 0, 0);
>> > >>   }
>> > >>   
>> > >> @@ -334,14 +347,22 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
>> > >>   	if (data->enc_in_format == data->enc_out_format) {
>> > >>   		if ((data->enc_in_format == HDMI_COLORSPACE_RGB) ||
>> > >>   		    (data->enc_in_format >= HDMI_COLORSPACE_YUV444)) {
>> > >> -			value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
>> > >> -			hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
>> > >> -
>> > >> -			hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
>> > >> -				  m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
>> > >> -				  v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
>> > >> -				  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
>> > >> -			return 0;
>> > >> +			if (data->enc_in_format == HDMI_COLORSPACE_RGB &&
>> > >> +			    data->enc_out_format == HDMI_COLORSPACE_RGB &&
>> > >> +			    hdmi->hdmi_data.rgb_limited_range) {
>> > >> +				csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
>> > >> +				auto_csc = AUTO_CSC_DISABLE;
>> > >> +				c0_c2_change = C0_C2_CHANGE_DISABLE;
>> > >> +				csc_enable = v_CSC_ENABLE;
>> > >> +			} else {
>> > >> +				value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
>> > >> +				hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
>> > >> +				hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
>> > >> +					  m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
>> > >> +					  v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
>> > >> +					  v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
>> > >> +				return 0;
>> > >> +			}
>> > >>   		}
>> > >>   	}
>> > >>   
>> > >> @@ -458,6 +479,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
>> > >>   	else
>> > >>   		hdmi->hdmi_data.colorimetry = HDMI_COLORIMETRY_ITU_709;
>> > >>   
>> > >> +	hdmi->hdmi_data.rgb_limited_range =
>> > >> +		drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
>> > >> +
>> > > This patch conflicts heavily with my inno_hdmi patches here (patches 22 to 38):
>> > > https://lore.kernel.org/dri-devel/20231207-kms-hdmi-connector-state-v5-0-6538e19d634d@xxxxxxxxxx/
>> > I'm aware of that and I mentioned it in the cover letter. Your series is 
>> > not merged yet and it didn't get much feedback so far. What is the 
>> > status there? Especially because you are removing things from inno-hdmi 
>> > driver (which aren't really required to remove there) which will I have 
>> > to reintroduce.
>> 
>> Sadly I haven't found the time to look closer at Maxime's series so far,
>> but I got the impression that it separates into multiple cleanup steps
>> for a number of controllers.
>
>Yeah, one of the previous version comment was to support more
>controllers than vc4, which is fair. So I ended up reworking and
>converting multiple controllers, but most of the clean up changes can be
>applied outside of that series just fine.
>
>I just didn't find someone to test / review them yet :)

I will try to bring up my rk3036 kylin board whith mainline kernel this weekend， then I can do some tests.

>
>Maxime