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

[Maxime Ripard]

On Thu, Dec 14, 2023 at 03:05:59PM +0100, Alex Bee wrote:
> 
> Am 14.12.23 um 12:33 schrieb Maxime Ripard:
> > On Thu, Dec 14, 2023 at 12:12:08PM +0100, Alex Bee wrote:
> > > 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.
> > Sorry, I missed that part.
> > 
> > > Your series is not merged yet and it didn't get much feedback so far.
> > > What is the status there?
> > It didn't have much reviews, but I'll hope to change that. For the
> > patches 22 to 38 though, it doesn't really matter. Those changes are
> > self-contained and can be applied as is outside of the series.
> > 
> > > Especially because you are removing things from inno-hdmi driver (which
> > > aren't really required to remove there) which will I have to reintroduce.
> > I'm not entirely sure which part I remove that are actually going to be
> > used here.
>
> I'm refering to [PATCH v5 33/44] which completly removes csc coeffs but this
> series needs the CSC_RGB_0_255_TO_RGB_16_235_8BIT  coeffs and  [PATCH v5
> 29/44] which removes writing csc_coeffs to the hardware.

Oh, right, feel free to drop those

Maxime

Attachment: signature.asc
Description: PGP signature