diff mbox series

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

Message ID 20231213195125.212923-5-knaerzche@gmail.com
State New
Headers
Series Add HDMI support for RK3128 |

Commit Message

Alex Bee Dec. 13, 2023, 7:51 p.m. UTC 
  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@gmail.com>
---
 drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 8 deletions(-)

Comments

Maxime Ripard Dec. 14, 2023, 7:56 a.m. UTC | #1 
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@gmail.com>
> ---
>  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@kernel.org/

I would appreciate if you could test and merge them into your series.

In particular, there's no need to store the range here: enc_out_format
is always RGB, so you'll always end up calling
drm_hdmi_avi_infoframe_quant_range(), and you'll always have the same csc values.

Maxime
Alex Bee Dec. 14, 2023, 11:12 a.m. UTC | #2 
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@gmail.com>
>> ---
>>   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@kernel.org/
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.

> I would appreciate if you could test and merge them into your series.
>
> In particular, there's no need to store the range here: enc_out_format
rgb_limited_range is currently not only used for csc, but also for for 
infoframe creation. So it makes sense to have this stored  to avoid 
calling drm_default_rgb_quant_range twice.

> is always RGB, so you'll always end up calling
> drm_hdmi_avi_infoframe_quant_range(), and you'll always have the same csc values.

I don't think that's true. Non-VIC modes like 800x600 or 1024x764 want 
full range RGB and csc must not be done in those cases.

Alex

>
> Maxime
Heiko Stübner Dec. 14, 2023, 11:17 a.m. UTC | #3 
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@gmail.com>
> >> ---
> >>   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@kernel.org/
> 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.

So the sentence below suggests that Maxime wanted you to pick the
appropriate patches from there and incorporate them into your series
(as it looks like you developed a nice working knowledge of the inno-hdmi
driver). So there is no need to first drop and then reintroduce stuff, but
there may be other interesting cleanups there.

> > I would appreciate if you could test and merge them into your series.


Heiko

> > In particular, there's no need to store the range here: enc_out_format
> rgb_limited_range is currently not only used for csc, but also for for 
> infoframe creation. So it makes sense to have this stored  to avoid 
> calling drm_default_rgb_quant_range twice.
> 
> > is always RGB, so you'll always end up calling
> > drm_hdmi_avi_infoframe_quant_range(), and you'll always have the same csc values.
> 
> I don't think that's true. Non-VIC modes like 800x600 or 1024x764 want 
> full range RGB and csc must not be done in those cases.
Maxime Ripard Dec. 14, 2023, 11:33 a.m. UTC | #4 
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@gmail.com>
> > > ---
> > >   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@kernel.org/
>
> 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 would appreciate if you could test and merge them into your series.
> > 
> > In particular, there's no need to store the range here: enc_out_format
>
> rgb_limited_range is currently not only used for csc, but also for for
> infoframe creation. So it makes sense to have this stored  to avoid calling
> drm_default_rgb_quant_range twice.

You're right, I missed one. Still, it shouldn't be stored in the
hdmi_data_info structure, it's tied to the mode, and the mode is part of
the state, so it's not a property to a given device, but it's tied to
the connector state.

So if you want to do so, you should really create a custom state
structure and store the range there, just like vc4 is doing for example.

Maxime
Maxime Ripard Dec. 14, 2023, 11:36 a.m. UTC | #5 
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@gmail.com>
> > >> ---
> > >>   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@kernel.org/
> > 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 :)

Maxime
Andy Yan Dec. 14, 2023, 11:50 a.m. UTC | #6 
Hi：

在 2023-12-14 19:36:05，"Maxime Ripard" <mripard@kernel.org> 写道：
>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@gmail.com>
>> > >> ---
>> > >>   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@kernel.org/
>> > 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
Alex Bee Dec. 14, 2023, 2:05 p.m. UTC | #7 
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@gmail.com>
>>>> ---
>>>>    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@kernel.org/
>> 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.
>
>>> I would appreciate if you could test and merge them into your series.
>>>
>>> In particular, there's no need to store the range here: enc_out_format
>> rgb_limited_range is currently not only used for csc, but also for for
>> infoframe creation. So it makes sense to have this stored  to avoid calling
>> drm_default_rgb_quant_range twice.
> You're right, I missed one. Still, it shouldn't be stored in the
> hdmi_data_info structure, it's tied to the mode, and the mode is part of
> the state, so it's not a property to a given device, but it's tied to
> the connector state.
>
> So if you want to do so, you should really create a custom state
> structure and store the range there, just like vc4 is doing for example.

OK - I'll check.

Alex

>
> Maxime
Alex Bee Dec. 14, 2023, 4:34 p.m. UTC | #8 
Hi Heiko, Hi Maxime,

Am 14.12.23 um 12:36 schrieb Maxime Ripard:
> 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@gmail.com>
>>>>> ---
>>>>>    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@kernel.org/
>>> 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'm not exactly sure how to proceed here. Do you want me to:

- base my patches on top of Maxime's series and reintroduce csc things 
(of course only those which touch inno-hdmi w/o the framwork changes)

- only apply the patches that do not touch csc things and base my 
series  on top of that

- adapt Maxime's patches so that RGB full to RGB limited stays in there

- wait with resend until Maxime's series is merged and reintroduce csc 
after that

- something else

?

Please advise.

Thanks,

Alex

> Maxime
Maxime Ripard Dec. 15, 2023, 12:27 p.m. UTC | #9 
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@gmail.com>
> > > > > ---
> > > > >    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@kernel.org/
> > > 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
Maxime Ripard Dec. 15, 2023, 12:29 p.m. UTC | #10 
On Thu, Dec 14, 2023 at 05:34:59PM +0100, Alex Bee wrote:
> Hi Heiko, Hi Maxime,
> 
> Am 14.12.23 um 12:36 schrieb Maxime Ripard:
> > 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@gmail.com>
> > > > > > ---
> > > > > >    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@kernel.org/
> > > > 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'm not exactly sure how to proceed here. Do you want me to:
> 
> - base my patches on top of Maxime's series and reintroduce csc things (of
> course only those which touch inno-hdmi w/o the framwork changes)
> 
> - only apply the patches that do not touch csc things and base my series  on
> top of that
> 
> - adapt Maxime's patches so that RGB full to RGB limited stays in there
> 
> - wait with resend until Maxime's series is merged and reintroduce csc after
> that
> 
> - something else
> 
> ?

2 or 3, at your discretion

Maxime
diff mbox series

Patch

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;
+
 	/* Mute video and audio output */
 	hdmi_modb(hdmi, HDMI_AV_MUTE, m_AUDIO_MUTE | m_VIDEO_BLACK,
 		  v_AUDIO_MUTE(1) | v_VIDEO_MUTE(1));