[PATCH 0/4] drm/bridge: dw-hdmi: Various updates

[PATCH 0/4] drm/bridge: dw-hdmi: Various updates

[Jernej Skrabec]

This series fixes multiple issues I found out.
Patch 1 fixes reporting colorimetry in AVI frame.
Patch 2 fixes color space conversion detection. At the moment it doesn't
change anything, but it would needlessly enable CSC unit when conversion
is not needed.
Patch 3 sets scan mode to underscan which is in line with most other hdmi
drivers.
Patch 4 aligns RGB quantization to CEA 861 standard.

Please take a look.

Best regards,
Jernej

Jernej Skrabec (3):
  drm/bridge: dw-hdmi: fix AVI frame colorimetry
  drm/bridge: dw-hdmi: Fix color space conversion detection
  drm/bridge: dw-hdmi: Add support for RGB limited range

Jonas Karlman (1):
  drm/bridge: dw-hdmi: do not force "none" scan mode

 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 87 ++++++++++++++++-------
 1 file changed, 62 insertions(+), 25 deletions(-)

-- 
2.25.1

[PATCH 1/4] drm/bridge: dw-hdmi: fix AVI frame colorimetry

[Jernej Skrabec]

CTA-861-F explicitly states that for RGB colorspace colorimetry should
be set to "none". Fix that.

Fixes: def23aa7e982 ("drm: bridge: dw-hdmi: Switch to V4L bus format and encodings")
Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 46 +++++++++++++----------
 1 file changed, 26 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 67fca439bbfb..24965e53d351 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -1624,28 +1624,34 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
 		frame.colorspace = HDMI_COLORSPACE_RGB;
 
 	/* Set up colorimetry */
-	switch (hdmi->hdmi_data.enc_out_encoding) {
-	case V4L2_YCBCR_ENC_601:
-		if (hdmi->hdmi_data.enc_in_encoding == V4L2_YCBCR_ENC_XV601)
-			frame.colorimetry = HDMI_COLORIMETRY_EXTENDED;
-		else
+	if (!hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format)) {
+		switch (hdmi->hdmi_data.enc_out_encoding) {
+		case V4L2_YCBCR_ENC_601:
+			if (hdmi->hdmi_data.enc_in_encoding == V4L2_YCBCR_ENC_XV601)
+				frame.colorimetry = HDMI_COLORIMETRY_EXTENDED;
+			else
+				frame.colorimetry = HDMI_COLORIMETRY_ITU_601;
+			frame.extended_colorimetry =
+					HDMI_EXTENDED_COLORIMETRY_XV_YCC_601;
+			break;
+		case V4L2_YCBCR_ENC_709:
+			if (hdmi->hdmi_data.enc_in_encoding == V4L2_YCBCR_ENC_XV709)
+				frame.colorimetry = HDMI_COLORIMETRY_EXTENDED;
+			else
+				frame.colorimetry = HDMI_COLORIMETRY_ITU_709;
+			frame.extended_colorimetry =
+					HDMI_EXTENDED_COLORIMETRY_XV_YCC_709;
+			break;
+		default: /* Carries no data */
 			frame.colorimetry = HDMI_COLORIMETRY_ITU_601;
+			frame.extended_colorimetry =
+					HDMI_EXTENDED_COLORIMETRY_XV_YCC_601;
+			break;
+		}
+	} else {
+		frame.colorimetry = HDMI_COLORIMETRY_NONE;
 		frame.extended_colorimetry =
-				HDMI_EXTENDED_COLORIMETRY_XV_YCC_601;
-		break;
-	case V4L2_YCBCR_ENC_709:
-		if (hdmi->hdmi_data.enc_in_encoding == V4L2_YCBCR_ENC_XV709)
-			frame.colorimetry = HDMI_COLORIMETRY_EXTENDED;
-		else
-			frame.colorimetry = HDMI_COLORIMETRY_ITU_709;
-		frame.extended_colorimetry =
-				HDMI_EXTENDED_COLORIMETRY_XV_YCC_709;
-		break;
-	default: /* Carries no data */
-		frame.colorimetry = HDMI_COLORIMETRY_ITU_601;
-		frame.extended_colorimetry =
-				HDMI_EXTENDED_COLORIMETRY_XV_YCC_601;
-		break;
+			HDMI_EXTENDED_COLORIMETRY_XV_YCC_601;
 	}
 
 	frame.scan_mode = HDMI_SCAN_MODE_NONE;
-- 
2.25.1

[PATCH 2/4] drm/bridge: dw-hdmi: Fix color space conversion detection

[Jernej Skrabec]

Currently, is_color_space_conversion() compares not only color spaces
but also formats. For example, function would return true if YCbCr 4:4:4
and YCbCr 4:2:2 would be set. Obviously in that case color spaces are
the same.

Fix that by comparing if both values represent RGB color space.

Fixes: b21f4b658df8 ("drm: imx: imx-hdmi: move imx-hdmi to bridge/dw_hdmi")
Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 24965e53d351..9d7bfb1cb213 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -956,7 +956,8 @@ static void hdmi_video_sample(struct dw_hdmi *hdmi)
 
 static int is_color_space_conversion(struct dw_hdmi *hdmi)
 {
-	return hdmi->hdmi_data.enc_in_bus_format != hdmi->hdmi_data.enc_out_bus_format;
+	return hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) !=
+		hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format);
 }
 
 static int is_color_space_decimation(struct dw_hdmi *hdmi)
-- 
2.25.1

[PATCH 3/4] drm/bridge: dw-hdmi: do not force "none" scan mode

[Jernej Skrabec]

From: Jonas Karlman <jonas@kwiboo.se>

Setting scan mode to "none" confuses some TVs like LG B8, which randomly
change overscan procentage over time. Digital outputs like HDMI and DVI,
handled by this controller, don't really need overscan, so we can always
set scan mode to underscan. Actually, this is exactly what
drm_hdmi_avi_infoframe_from_display_mode() already does, so we can just
remove offending line.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
[updated commit message]
Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 9d7bfb1cb213..3d6021119942 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -1655,8 +1655,6 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
 			HDMI_EXTENDED_COLORIMETRY_XV_YCC_601;
 	}
 
-	frame.scan_mode = HDMI_SCAN_MODE_NONE;
-
 	/*
 	 * The Designware IP uses a different byte format from standard
 	 * AVI info frames, though generally the bits are in the correct
-- 
2.25.1

[PATCH 4/4] drm/bridge: dw-hdmi: Add support for RGB limited range

[Jernej Skrabec]

CEA 861 standard request that RGB quantization range is "limited" for
CEA modes. Support that by adding CSC matrix which downscales values.

This allows to proper color reproduction on TV and PC monitor at the
same time. In future, override property can be added, like "Broadcast
RGB" in i915 driver.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 36 +++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 3d6021119942..101c90156fa0 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -92,6 +92,12 @@ static const u16 csc_coeff_rgb_in_eitu709[3][4] = {
 	{ 0x6756, 0x78ab, 0x2000, 0x0200 }
 };
 
+static const u16 csc_coeff_rgb_limited[3][4] = {
+	{ 0x1B7C, 0x0000, 0x0000, 0x0020 },
+	{ 0x0000, 0x1B7C, 0x0000, 0x0020 },
+	{ 0x0000, 0x0000, 0x1B7C, 0x0020 }
+};
+
 struct hdmi_vmode {
 	bool mdataenablepolarity;
 
@@ -109,6 +115,7 @@ struct hdmi_data_info {
 	unsigned int pix_repet_factor;
 	unsigned int hdcp_enable;
 	struct hdmi_vmode video_mode;
+	bool rgb_limited_range;
 };
 
 struct dw_hdmi_i2c {
@@ -960,6 +967,13 @@ static int is_color_space_conversion(struct dw_hdmi *hdmi)
 		hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format);
 }
 
+static int is_rgb_downscale_needed(struct dw_hdmi *hdmi)
+{
+	return  hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) &&
+		hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format) &&
+		hdmi->hdmi_data.rgb_limited_range;
+}
+
 static int is_color_space_decimation(struct dw_hdmi *hdmi)
 {
 	if (!hdmi_bus_fmt_is_yuv422(hdmi->hdmi_data.enc_out_bus_format))
@@ -1006,6 +1020,8 @@ static void dw_hdmi_update_csc_coeffs(struct dw_hdmi *hdmi)
 				csc_coeff = &csc_coeff_rgb_in_eitu709;
 			csc_scale = 0;
 		}
+	} else if (is_rgb_downscale_needed(hdmi)) {
+		csc_coeff = &csc_coeff_rgb_limited;
 	}
 
 	/* The CSC registers are sequential, alternating MSB then LSB */
@@ -1615,6 +1631,18 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
 	drm_hdmi_avi_infoframe_from_display_mode(&frame,
 						 &hdmi->connector, mode);
 
+	if (hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format)) {
+		drm_hdmi_avi_infoframe_quant_range(&frame, &hdmi->connector,
+						   mode,
+						   hdmi->hdmi_data.rgb_limited_range ?
+						   HDMI_QUANTIZATION_RANGE_LIMITED :
+						   HDMI_QUANTIZATION_RANGE_FULL);
+	} else {
+		frame.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
+		frame.ycc_quantization_range =
+			HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
+	}
+
 	if (hdmi_bus_fmt_is_yuv444(hdmi->hdmi_data.enc_out_bus_format))
 		frame.colorspace = HDMI_COLORSPACE_YUV444;
 	else if (hdmi_bus_fmt_is_yuv422(hdmi->hdmi_data.enc_out_bus_format))
@@ -1990,13 +2018,13 @@ static void dw_hdmi_enable_video_path(struct dw_hdmi *hdmi)
 	hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS);
 
 	/* Enable csc path */
-	if (is_color_space_conversion(hdmi)) {
+	if (is_color_space_conversion(hdmi) || is_rgb_downscale_needed(hdmi)) {
 		hdmi->mc_clkdis &= ~HDMI_MC_CLKDIS_CSCCLK_DISABLE;
 		hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS);
 	}
 
 	/* Enable color space conversion if needed */
-	if (is_color_space_conversion(hdmi))
+	if (is_color_space_conversion(hdmi) || is_rgb_downscale_needed(hdmi))
 		hdmi_writeb(hdmi, HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_IN_PATH,
 			    HDMI_MC_FLOWCTRL);
 	else
@@ -2100,6 +2128,10 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
 	/* TOFIX: Default to RGB888 output format */
 	hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
 
+	hdmi->hdmi_data.rgb_limited_range = hdmi->sink_is_hdmi &&
+		drm_default_rgb_quant_range(mode) ==
+		HDMI_QUANTIZATION_RANGE_LIMITED;
+
 	hdmi->hdmi_data.pix_repet_factor = 0;
 	hdmi->hdmi_data.hdcp_enable = 0;
 	hdmi->hdmi_data.video_mode.mdataenablepolarity = true;
-- 
2.25.1

Re: [PATCH 1/4] drm/bridge: dw-hdmi: fix AVI frame colorimetry

[Laurent Pinchart]

Hi Jernej,

Thank you for the patch.

On Sat, Feb 29, 2020 at 05:30:40PM +0100, Jernej Skrabec wrote:
> CTA-861-F explicitly states that for RGB colorspace colorimetry should
> be set to "none". Fix that.
> 
> Fixes: def23aa7e982 ("drm: bridge: dw-hdmi: Switch to V4L bus format and encodings")
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 46 +++++++++++++----------
>  1 file changed, 26 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 67fca439bbfb..24965e53d351 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -1624,28 +1624,34 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>  		frame.colorspace = HDMI_COLORSPACE_RGB;
>  
>  	/* Set up colorimetry */
> -	switch (hdmi->hdmi_data.enc_out_encoding) {
> -	case V4L2_YCBCR_ENC_601:
> -		if (hdmi->hdmi_data.enc_in_encoding == V4L2_YCBCR_ENC_XV601)
> -			frame.colorimetry = HDMI_COLORIMETRY_EXTENDED;
> -		else
> +	if (!hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format)) {
> +		switch (hdmi->hdmi_data.enc_out_encoding) {
> +		case V4L2_YCBCR_ENC_601:
> +			if (hdmi->hdmi_data.enc_in_encoding == V4L2_YCBCR_ENC_XV601)
> +				frame.colorimetry = HDMI_COLORIMETRY_EXTENDED;
> +			else
> +				frame.colorimetry = HDMI_COLORIMETRY_ITU_601;
> +			frame.extended_colorimetry =
> +					HDMI_EXTENDED_COLORIMETRY_XV_YCC_601;
> +			break;
> +		case V4L2_YCBCR_ENC_709:
> +			if (hdmi->hdmi_data.enc_in_encoding == V4L2_YCBCR_ENC_XV709)
> +				frame.colorimetry = HDMI_COLORIMETRY_EXTENDED;
> +			else
> +				frame.colorimetry = HDMI_COLORIMETRY_ITU_709;
> +			frame.extended_colorimetry =
> +					HDMI_EXTENDED_COLORIMETRY_XV_YCC_709;
> +			break;
> +		default: /* Carries no data */
>  			frame.colorimetry = HDMI_COLORIMETRY_ITU_601;
> +			frame.extended_colorimetry =
> +					HDMI_EXTENDED_COLORIMETRY_XV_YCC_601;
> +			break;
> +		}
> +	} else {
> +		frame.colorimetry = HDMI_COLORIMETRY_NONE;
>  		frame.extended_colorimetry =
> -				HDMI_EXTENDED_COLORIMETRY_XV_YCC_601;
> -		break;
> -	case V4L2_YCBCR_ENC_709:
> -		if (hdmi->hdmi_data.enc_in_encoding == V4L2_YCBCR_ENC_XV709)
> -			frame.colorimetry = HDMI_COLORIMETRY_EXTENDED;
> -		else
> -			frame.colorimetry = HDMI_COLORIMETRY_ITU_709;
> -		frame.extended_colorimetry =
> -				HDMI_EXTENDED_COLORIMETRY_XV_YCC_709;
> -		break;
> -	default: /* Carries no data */
> -		frame.colorimetry = HDMI_COLORIMETRY_ITU_601;
> -		frame.extended_colorimetry =
> -				HDMI_EXTENDED_COLORIMETRY_XV_YCC_601;
> -		break;
> +			HDMI_EXTENDED_COLORIMETRY_XV_YCC_601;
>  	}
>  
>  	frame.scan_mode = HDMI_SCAN_MODE_NONE;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/4] drm/bridge: dw-hdmi: Fix color space conversion detection

[Neil Armstrong]

Hi Jernej,

On 29/02/2020 17:30, Jernej Skrabec wrote:
> Currently, is_color_space_conversion() compares not only color spaces
> but also formats. For example, function would return true if YCbCr 4:4:4
> and YCbCr 4:2:2 would be set. Obviously in that case color spaces are
> the same.
> 
> Fix that by comparing if both values represent RGB color space.
> 
> Fixes: b21f4b658df8 ("drm: imx: imx-hdmi: move imx-hdmi to bridge/dw_hdmi")
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 24965e53d351..9d7bfb1cb213 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -956,7 +956,8 @@ static void hdmi_video_sample(struct dw_hdmi *hdmi)
>  
>  static int is_color_space_conversion(struct dw_hdmi *hdmi)
>  {
> -	return hdmi->hdmi_data.enc_in_bus_format != hdmi->hdmi_data.enc_out_bus_format;
> +	return hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) !=
> +		hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format);
>  }
>  
>  static int is_color_space_decimation(struct dw_hdmi *hdmi)
> 

I think in this case you should also fix the CEC enablement to:
if (is_color_space_conversion(hdmi) || is_color_space_decimation(hdmi))

in dw_hdmi_enable_video_path() otherwise CSC won't be enabled and will be bypassed
in decimation case only.

Neil

Re: [PATCH 3/4] drm/bridge: dw-hdmi: do not force "none" scan mode

[Neil Armstrong]

On 29/02/2020 17:30, Jernej Skrabec wrote:
> From: Jonas Karlman <jonas@kwiboo.se>
> 
> Setting scan mode to "none" confuses some TVs like LG B8, which randomly
> change overscan procentage over time. Digital outputs like HDMI and DVI,
> handled by this controller, don't really need overscan, so we can always
> set scan mode to underscan. Actually, this is exactly what
> drm_hdmi_avi_infoframe_from_display_mode() already does, so we can just
> remove offending line.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> [updated commit message]
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 9d7bfb1cb213..3d6021119942 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -1655,8 +1655,6 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>  			HDMI_EXTENDED_COLORIMETRY_XV_YCC_601;
>  	}
>  
> -	frame.scan_mode = HDMI_SCAN_MODE_NONE;
> -
>  	/*
>  	 * The Designware IP uses a different byte format from standard
>  	 * AVI info frames, though generally the bits are in the correct
> 

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>

Re: [PATCH 2/4] drm/bridge: dw-hdmi: Fix color space conversion detection

[Laurent Pinchart]

Hi Jernej,

Thank you for the patch.

On Sat, Feb 29, 2020 at 05:30:41PM +0100, Jernej Skrabec wrote:
> Currently, is_color_space_conversion() compares not only color spaces
> but also formats. For example, function would return true if YCbCr 4:4:4
> and YCbCr 4:2:2 would be set. Obviously in that case color spaces are
> the same.
> 
> Fix that by comparing if both values represent RGB color space.
> 
> Fixes: b21f4b658df8 ("drm: imx: imx-hdmi: move imx-hdmi to bridge/dw_hdmi")
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

This isn't implemented today, but could the CSC be used to convert
between different YCbCr encodings ?

In any case the patch is correct based on the current implementation, so

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 24965e53d351..9d7bfb1cb213 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -956,7 +956,8 @@ static void hdmi_video_sample(struct dw_hdmi *hdmi)
>  
>  static int is_color_space_conversion(struct dw_hdmi *hdmi)
>  {
> -	return hdmi->hdmi_data.enc_in_bus_format != hdmi->hdmi_data.enc_out_bus_format;
> +	return hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) !=
> +		hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format);
>  }
>  
>  static int is_color_space_decimation(struct dw_hdmi *hdmi)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] drm/bridge: dw-hdmi: do not force "none" scan mode

[Laurent Pinchart]

Hi Jernej and Jonas,

Thank you for the patch.

On Sat, Feb 29, 2020 at 05:30:42PM +0100, Jernej Skrabec wrote:
> From: Jonas Karlman <jonas@kwiboo.se>
> 
> Setting scan mode to "none" confuses some TVs like LG B8, which randomly
> change overscan procentage over time. Digital outputs like HDMI and DVI,

s/procentage/percentage/ ?

> handled by this controller, don't really need overscan, so we can always
> set scan mode to underscan. Actually, this is exactly what
> drm_hdmi_avi_infoframe_from_display_mode() already does, so we can just
> remove offending line.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> [updated commit message]
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 9d7bfb1cb213..3d6021119942 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -1655,8 +1655,6 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>  			HDMI_EXTENDED_COLORIMETRY_XV_YCC_601;
>  	}
>  
> -	frame.scan_mode = HDMI_SCAN_MODE_NONE;
> -
>  	/*
>  	 * The Designware IP uses a different byte format from standard
>  	 * AVI info frames, though generally the bits are in the correct

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/4] drm/bridge: dw-hdmi: Fix color space conversion detection

[Jernej Škrabec]

Hi Neil!

Dne ponedeljek, 02. marec 2020 ob 10:26:09 CET je Neil Armstrong napisal(a):
> Hi Jernej,
> 
> On 29/02/2020 17:30, Jernej Skrabec wrote:
> > Currently, is_color_space_conversion() compares not only color spaces
> > but also formats. For example, function would return true if YCbCr 4:4:4
> > and YCbCr 4:2:2 would be set. Obviously in that case color spaces are
> > the same.
> > 
> > Fix that by comparing if both values represent RGB color space.
> > 
> > Fixes: b21f4b658df8 ("drm: imx: imx-hdmi: move imx-hdmi to
> > bridge/dw_hdmi")
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> > 
> >  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index
> > 24965e53d351..9d7bfb1cb213 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > @@ -956,7 +956,8 @@ static void hdmi_video_sample(struct dw_hdmi *hdmi)
> > 
> >  static int is_color_space_conversion(struct dw_hdmi *hdmi)
> >  {
> > 
> > -	return hdmi->hdmi_data.enc_in_bus_format !=
> > hdmi->hdmi_data.enc_out_bus_format; +	return
> > hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) !=
> > +		hdmi_bus_fmt_is_rgb(hdmi-
>hdmi_data.enc_out_bus_format);
> > 
> >  }
> >  
> >  static int is_color_space_decimation(struct dw_hdmi *hdmi)
> 
> I think in this case you should also fix the CEC enablement to:

you mean CSC, right?

> if (is_color_space_conversion(hdmi) || is_color_space_decimation(hdmi))
> 
> in dw_hdmi_enable_video_path() otherwise CSC won't be enabled and will be
> bypassed in decimation case only.
> 

Missed that one. I'll fix in v2.

Best regards,
Jernej

Re: [PATCH 4/4] drm/bridge: dw-hdmi: Add support for RGB limited range

[Laurent Pinchart]

Hi Jernej,

Thank you for the patch.

On Sat, Feb 29, 2020 at 05:30:43PM +0100, Jernej Skrabec wrote:
> CEA 861 standard request that RGB quantization range is "limited" for

s/request/requests/

> CEA modes. Support that by adding CSC matrix which downscales values.

Interesting, that's related to what I asked in the review of 2/4 :-)

> This allows to proper color reproduction on TV and PC monitor at the
> same time. In future, override property can be added, like "Broadcast
> RGB" in i915 driver.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 36 +++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 3d6021119942..101c90156fa0 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -92,6 +92,12 @@ static const u16 csc_coeff_rgb_in_eitu709[3][4] = {
>  	{ 0x6756, 0x78ab, 0x2000, 0x0200 }
>  };
>  
> +static const u16 csc_coeff_rgb_limited[3][4] = {

Could you name this to make it explicit that we're converting from RGB
to RGB ? Maybe csc_coeff_rgb_full_to_rgb_limited ?

> +	{ 0x1B7C, 0x0000, 0x0000, 0x0020 },
> +	{ 0x0000, 0x1B7C, 0x0000, 0x0020 },
> +	{ 0x0000, 0x0000, 0x1B7C, 0x0020 }

Lowercase hex constants please.

> +};
> +
>  struct hdmi_vmode {
>  	bool mdataenablepolarity;
>  
> @@ -109,6 +115,7 @@ struct hdmi_data_info {
>  	unsigned int pix_repet_factor;
>  	unsigned int hdcp_enable;
>  	struct hdmi_vmode video_mode;
> +	bool rgb_limited_range;
>  };
>  
>  struct dw_hdmi_i2c {
> @@ -960,6 +967,13 @@ static int is_color_space_conversion(struct dw_hdmi *hdmi)
>  		hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format);
>  }
>  
> +static int is_rgb_downscale_needed(struct dw_hdmi *hdmi)
> +{
> +	return  hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) &&
> +		hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format) &&
> +		hdmi->hdmi_data.rgb_limited_range;
> +}
> +
>  static int is_color_space_decimation(struct dw_hdmi *hdmi)
>  {
>  	if (!hdmi_bus_fmt_is_yuv422(hdmi->hdmi_data.enc_out_bus_format))
> @@ -1006,6 +1020,8 @@ static void dw_hdmi_update_csc_coeffs(struct dw_hdmi *hdmi)
>  				csc_coeff = &csc_coeff_rgb_in_eitu709;
>  			csc_scale = 0;
>  		}
> +	} else if (is_rgb_downscale_needed(hdmi)) {
> +		csc_coeff = &csc_coeff_rgb_limited;
>  	}
>  
>  	/* The CSC registers are sequential, alternating MSB then LSB */
> @@ -1615,6 +1631,18 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>  	drm_hdmi_avi_infoframe_from_display_mode(&frame,
>  						 &hdmi->connector, mode);
>  
> +	if (hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format)) {
> +		drm_hdmi_avi_infoframe_quant_range(&frame, &hdmi->connector,
> +						   mode,
> +						   hdmi->hdmi_data.rgb_limited_range ?
> +						   HDMI_QUANTIZATION_RANGE_LIMITED :
> +						   HDMI_QUANTIZATION_RANGE_FULL);
> +	} else {
> +		frame.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
> +		frame.ycc_quantization_range =
> +			HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
> +	}
> +
>  	if (hdmi_bus_fmt_is_yuv444(hdmi->hdmi_data.enc_out_bus_format))
>  		frame.colorspace = HDMI_COLORSPACE_YUV444;
>  	else if (hdmi_bus_fmt_is_yuv422(hdmi->hdmi_data.enc_out_bus_format))
> @@ -1990,13 +2018,13 @@ static void dw_hdmi_enable_video_path(struct dw_hdmi *hdmi)
>  	hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS);
>  
>  	/* Enable csc path */
> -	if (is_color_space_conversion(hdmi)) {
> +	if (is_color_space_conversion(hdmi) || is_rgb_downscale_needed(hdmi)) {

I would fold this change in is_color_space_conversion(), and modify
dw_hdmi_update_csc_coeffs() accordingly with something like

	if (!hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) &&
	    hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format)) {
		if (hdmi->hdmi_data.enc_out_encoding == V4L2_YCBCR_ENC_601)
			csc_coeff = &csc_coeff_rgb_out_eitu601;
		else
			csc_coeff = &csc_coeff_rgb_out_eitu709;
	} else if (hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) &&
		   !hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format)) {
		if (hdmi->hdmi_data.enc_out_encoding == V4L2_YCBCR_ENC_601)
			csc_coeff = &csc_coeff_rgb_in_eitu601;
		else
			csc_coeff = &csc_coeff_rgb_in_eitu709;
		csc_scale = 0;
	} else if (is_rgb_downscale_needed(hdmi)) {
		csc_coeff = &csc_coeff_rgb_limited;
	}

>  		hdmi->mc_clkdis &= ~HDMI_MC_CLKDIS_CSCCLK_DISABLE;
>  		hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS);
>  	}
>  
>  	/* Enable color space conversion if needed */
> -	if (is_color_space_conversion(hdmi))
> +	if (is_color_space_conversion(hdmi) || is_rgb_downscale_needed(hdmi))
>  		hdmi_writeb(hdmi, HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_IN_PATH,
>  			    HDMI_MC_FLOWCTRL);
>  	else
> @@ -2100,6 +2128,10 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
>  	/* TOFIX: Default to RGB888 output format */
>  	hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>  
> +	hdmi->hdmi_data.rgb_limited_range = hdmi->sink_is_hdmi &&
> +		drm_default_rgb_quant_range(mode) ==
> +		HDMI_QUANTIZATION_RANGE_LIMITED;
> +
>  	hdmi->hdmi_data.pix_repet_factor = 0;
>  	hdmi->hdmi_data.hdcp_enable = 0;
>  	hdmi->hdmi_data.video_mode.mdataenablepolarity = true;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/4] drm/bridge: dw-hdmi: Fix color space conversion detection

[Jernej Škrabec]

Dne ponedeljek, 02. marec 2020 ob 10:27:48 CET je Laurent Pinchart napisal(a):
> Hi Jernej,
> 
> Thank you for the patch.
> 
> On Sat, Feb 29, 2020 at 05:30:41PM +0100, Jernej Skrabec wrote:
> > Currently, is_color_space_conversion() compares not only color spaces
> > but also formats. For example, function would return true if YCbCr 4:4:4
> > and YCbCr 4:2:2 would be set. Obviously in that case color spaces are
> > the same.
> > 
> > Fix that by comparing if both values represent RGB color space.
> > 
> > Fixes: b21f4b658df8 ("drm: imx: imx-hdmi: move imx-hdmi to
> > bridge/dw_hdmi")
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> 
> This isn't implemented today, but could the CSC be used to convert
> between different YCbCr encodings ?

Yes, CSC offers great flexibility, but unfortunately that also means that you 
have as much CSC matrices as there is possible conversions. This explodes 
quickly, especially if you convert from one YCbCr encoding to another (BT.601, 
BT.709, BT.2020) and also considering range (full, limited). If you don't mind 
doing some calculations in code, this becames much simpler, but doing fixed 
point arithmetic isn't fun. Is floating point arithmetic allowed in kernel?

I wrote a simple program to produce all those CSC matrices for sun4i-drm 
driver: http://ix.io/2dak Note that it's for RGB <-> YUV conversion, but DW 
HDMI has a bit different order. I believe it's GRB, but I'm not 100% sure.

You can also do various color adjustements, like brigthness, but that would 
also mean that you have to multiply all matrices to get final one which you can 
then write into registers.

Best regards,
Jernej

> 
> In any case the patch is correct based on the current implementation, so
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > ---
> > 
> >  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index
> > 24965e53d351..9d7bfb1cb213 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > @@ -956,7 +956,8 @@ static void hdmi_video_sample(struct dw_hdmi *hdmi)
> > 
> >  static int is_color_space_conversion(struct dw_hdmi *hdmi)
> >  {
> > 
> > -	return hdmi->hdmi_data.enc_in_bus_format !=
> > hdmi->hdmi_data.enc_out_bus_format; +	return
> > hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) !=
> > +		hdmi_bus_fmt_is_rgb(hdmi-
>hdmi_data.enc_out_bus_format);
> > 
> >  }
> >  
> >  static int is_color_space_decimation(struct dw_hdmi *hdmi)

Re: [PATCH 2/4] drm/bridge: dw-hdmi: Fix color space conversion detection

[Jernej Škrabec]

Dne ponedeljek, 02. marec 2020 ob 10:26:09 CET je Neil Armstrong napisal(a):
> Hi Jernej,
> 
> On 29/02/2020 17:30, Jernej Skrabec wrote:
> > Currently, is_color_space_conversion() compares not only color spaces
> > but also formats. For example, function would return true if YCbCr 4:4:4
> > and YCbCr 4:2:2 would be set. Obviously in that case color spaces are
> > the same.
> > 
> > Fix that by comparing if both values represent RGB color space.
> > 
> > Fixes: b21f4b658df8 ("drm: imx: imx-hdmi: move imx-hdmi to
> > bridge/dw_hdmi")
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> > 
> >  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index
> > 24965e53d351..9d7bfb1cb213 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > @@ -956,7 +956,8 @@ static void hdmi_video_sample(struct dw_hdmi *hdmi)
> > 
> >  static int is_color_space_conversion(struct dw_hdmi *hdmi)
> >  {
> > 
> > -	return hdmi->hdmi_data.enc_in_bus_format !=
> > hdmi->hdmi_data.enc_out_bus_format; +	return
> > hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) !=
> > +		hdmi_bus_fmt_is_rgb(hdmi-
>hdmi_data.enc_out_bus_format);
> > 
> >  }
> >  
> >  static int is_color_space_decimation(struct dw_hdmi *hdmi)
> 
> I think in this case you should also fix the CEC enablement to:
> if (is_color_space_conversion(hdmi) || is_color_space_decimation(hdmi))
> 
> in dw_hdmi_enable_video_path() otherwise CSC won't be enabled and will be
> bypassed in decimation case only.

On second thought, I think original implementation is correct, just misnamed. 
Laurent, Neil, do you agree if I replace this patch with patch which renames 
is_color_space_conversion() to is_conversion_needed() ?

Best regards,
Jernej

Re: [PATCH 4/4] drm/bridge: dw-hdmi: Add support for RGB limited range

[Jernej Škrabec]

Hi Laurent!

Dne ponedeljek, 02. marec 2020 ob 10:53:54 CET je Laurent Pinchart napisal(a):
> Hi Jernej,
> 
> Thank you for the patch.
> 
> On Sat, Feb 29, 2020 at 05:30:43PM +0100, Jernej Skrabec wrote:
> > CEA 861 standard request that RGB quantization range is "limited" for
> 
> s/request/requests/
> 
> > CEA modes. Support that by adding CSC matrix which downscales values.
> 
> Interesting, that's related to what I asked in the review of 2/4 :-)
> 
> > This allows to proper color reproduction on TV and PC monitor at the
> > same time. In future, override property can be added, like "Broadcast
> > RGB" in i915 driver.
> > 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> > 
> >  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 36 +++++++++++++++++++++--
> >  1 file changed, 34 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index
> > 3d6021119942..101c90156fa0 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > @@ -92,6 +92,12 @@ static const u16 csc_coeff_rgb_in_eitu709[3][4] = {
> > 
> >  	{ 0x6756, 0x78ab, 0x2000, 0x0200 }
> >  
> >  };
> > 
> > +static const u16 csc_coeff_rgb_limited[3][4] = {
> 
> Could you name this to make it explicit that we're converting from RGB
> to RGB ? Maybe csc_coeff_rgb_full_to_rgb_limited ?

Ok.

> 
> > +	{ 0x1B7C, 0x0000, 0x0000, 0x0020 },
> > +	{ 0x0000, 0x1B7C, 0x0000, 0x0020 },
> > +	{ 0x0000, 0x0000, 0x1B7C, 0x0020 }
> 
> Lowercase hex constants please.

Ok.

> 
> > +};
> > +
> > 
> >  struct hdmi_vmode {
> >  
> >  	bool mdataenablepolarity;
> > 
> > @@ -109,6 +115,7 @@ struct hdmi_data_info {
> > 
> >  	unsigned int pix_repet_factor;
> >  	unsigned int hdcp_enable;
> >  	struct hdmi_vmode video_mode;
> > 
> > +	bool rgb_limited_range;
> > 
> >  };
> >  
> >  struct dw_hdmi_i2c {
> > 
> > @@ -960,6 +967,13 @@ static int is_color_space_conversion(struct dw_hdmi
> > *hdmi)> 
> >  		hdmi_bus_fmt_is_rgb(hdmi-
>hdmi_data.enc_out_bus_format);
> >  
> >  }
> > 
> > +static int is_rgb_downscale_needed(struct dw_hdmi *hdmi)
> > +{
> > +	return  hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) &&
> > +		hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format) 
&&
> > +		hdmi->hdmi_data.rgb_limited_range;
> > +}
> > +
> > 
> >  static int is_color_space_decimation(struct dw_hdmi *hdmi)
> >  {
> >  
> >  	if (!hdmi_bus_fmt_is_yuv422(hdmi->hdmi_data.enc_out_bus_format))
> > 
> > @@ -1006,6 +1020,8 @@ static void dw_hdmi_update_csc_coeffs(struct dw_hdmi
> > *hdmi)> 
> >  				csc_coeff = 
&csc_coeff_rgb_in_eitu709;
> >  			
> >  			csc_scale = 0;
> >  		
> >  		}
> > 
> > +	} else if (is_rgb_downscale_needed(hdmi)) {
> > +		csc_coeff = &csc_coeff_rgb_limited;
> > 
> >  	}
> >  	
> >  	/* The CSC registers are sequential, alternating MSB then LSB */
> > 
> > @@ -1615,6 +1631,18 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi,
> > struct drm_display_mode *mode)> 
> >  	drm_hdmi_avi_infoframe_from_display_mode(&frame,
> >  	
> >  						 &hdmi-
>connector, mode);
> > 
> > +	if (hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format)) {
> > +		drm_hdmi_avi_infoframe_quant_range(&frame, &hdmi-
>connector,
> > +						   mode,
> > +						   hdmi-
>hdmi_data.rgb_limited_range ?
> > +						   
HDMI_QUANTIZATION_RANGE_LIMITED :
> > +						   
HDMI_QUANTIZATION_RANGE_FULL);
> > +	} else {
> > +		frame.quantization_range = 
HDMI_QUANTIZATION_RANGE_DEFAULT;
> > +		frame.ycc_quantization_range =
> > +			HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
> > +	}
> > +
> > 
> >  	if (hdmi_bus_fmt_is_yuv444(hdmi->hdmi_data.enc_out_bus_format))
> >  	
> >  		frame.colorspace = HDMI_COLORSPACE_YUV444;
> >  	
> >  	else if (hdmi_bus_fmt_is_yuv422(hdmi-
>hdmi_data.enc_out_bus_format))
> > 
> > @@ -1990,13 +2018,13 @@ static void dw_hdmi_enable_video_path(struct
> > dw_hdmi *hdmi)> 
> >  	hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS);
> >  	
> >  	/* Enable csc path */
> > 
> > -	if (is_color_space_conversion(hdmi)) {
> > +	if (is_color_space_conversion(hdmi) || 
is_rgb_downscale_needed(hdmi)) {
> 
> I would fold this change in is_color_space_conversion(), and modify
> dw_hdmi_update_csc_coeffs() accordingly with something like
> 
> 	if (!hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) &&
> 	    hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format)) {
> 		if (hdmi->hdmi_data.enc_out_encoding == 
V4L2_YCBCR_ENC_601)
> 			csc_coeff = &csc_coeff_rgb_out_eitu601;
> 		else
> 			csc_coeff = &csc_coeff_rgb_out_eitu709;
> 	} else if (hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) 
&&
> 		   !hdmi_bus_fmt_is_rgb(hdmi-
>hdmi_data.enc_out_bus_format)) {
> 		if (hdmi->hdmi_data.enc_out_encoding == 
V4L2_YCBCR_ENC_601)
> 			csc_coeff = &csc_coeff_rgb_in_eitu601;
> 		else
> 			csc_coeff = &csc_coeff_rgb_in_eitu709;
> 		csc_scale = 0;
> 	} else if (is_rgb_downscale_needed(hdmi)) {
> 		csc_coeff = &csc_coeff_rgb_limited;
> 	}

Right, but for clarity reasons I'll introduce is_input_rgb and is_output_rgb 
variables.

Thanks for review.

Best regards,
Jernej

> 
> >  		hdmi->mc_clkdis &= ~HDMI_MC_CLKDIS_CSCCLK_DISABLE;
> >  		hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS);
> >  	
> >  	}
> >  	
> >  	/* Enable color space conversion if needed */
> > 
> > -	if (is_color_space_conversion(hdmi))
> > +	if (is_color_space_conversion(hdmi) || 
is_rgb_downscale_needed(hdmi))
> > 
> >  		hdmi_writeb(hdmi, 
HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_IN_PATH,
> >  		
> >  			    HDMI_MC_FLOWCTRL);
> >  	
> >  	else
> > 
> > @@ -2100,6 +2128,10 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi,
> > struct drm_display_mode *mode)> 
> >  	/* TOFIX: Default to RGB888 output format */
> >  	hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> > 
> > +	hdmi->hdmi_data.rgb_limited_range = hdmi->sink_is_hdmi &&
> > +		drm_default_rgb_quant_range(mode) ==
> > +		HDMI_QUANTIZATION_RANGE_LIMITED;
> > +
> > 
> >  	hdmi->hdmi_data.pix_repet_factor = 0;
> >  	hdmi->hdmi_data.hdcp_enable = 0;
> >  	hdmi->hdmi_data.video_mode.mdataenablepolarity = true;

Re: [PATCH 2/4] drm/bridge: dw-hdmi: Fix color space conversion detection

[Neil Armstrong]

On 02/03/2020 17:42, Jernej Škrabec wrote:
> Dne ponedeljek, 02. marec 2020 ob 10:26:09 CET je Neil Armstrong napisal(a):
>> Hi Jernej,
>>
>> On 29/02/2020 17:30, Jernej Skrabec wrote:
>>> Currently, is_color_space_conversion() compares not only color spaces
>>> but also formats. For example, function would return true if YCbCr 4:4:4
>>> and YCbCr 4:2:2 would be set. Obviously in that case color spaces are
>>> the same.
>>>
>>> Fix that by comparing if both values represent RGB color space.
>>>
>>> Fixes: b21f4b658df8 ("drm: imx: imx-hdmi: move imx-hdmi to
>>> bridge/dw_hdmi")
>>> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
>>> ---
>>>
>>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index
>>> 24965e53d351..9d7bfb1cb213 100644
>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> @@ -956,7 +956,8 @@ static void hdmi_video_sample(struct dw_hdmi *hdmi)
>>>
>>>  static int is_color_space_conversion(struct dw_hdmi *hdmi)
>>>  {
>>>
>>> -	return hdmi->hdmi_data.enc_in_bus_format !=
>>> hdmi->hdmi_data.enc_out_bus_format; +	return
>>> hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) !=
>>> +		hdmi_bus_fmt_is_rgb(hdmi-
>> hdmi_data.enc_out_bus_format);
>>>
>>>  }
>>>  
>>>  static int is_color_space_decimation(struct dw_hdmi *hdmi)
>>
>> I think in this case you should also fix the CEC enablement to:
>> if (is_color_space_conversion(hdmi) || is_color_space_decimation(hdmi))
>>
>> in dw_hdmi_enable_video_path() otherwise CSC won't be enabled and will be
>> bypassed in decimation case only.
> 
> On second thought, I think original implementation is correct, just misnamed. 
> Laurent, Neil, do you agree if I replace this patch with patch which renames 
> is_color_space_conversion() to is_conversion_needed() ?

Sure,
Neil

> 
> Best regards,
> Jernej
> 
> 
>