Re: [PATCH v4 03/16] drm/edid: Split deep color modes between RGB and YUV444

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Jan 20, 2022 at 04:16:12PM +0100, Maxime Ripard wrote:
> The current code assumes that the RGB444 and YUV444 formats are the
> same, but the HDMI 2.0 specification states that:
> 
>    The three DC_XXbit bits above only indicate support for RGB 4:4:4 at
>    that pixel size. Support for YCBCR 4:4:4 in Deep Color modes is
>    indicated with the DC_Y444 bit. If DC_Y444 is set, then YCBCR 4:4:4
>    is supported for all modes indicated by the DC_XXbit flags.
> 
> So if we have YUV444 support and any DC_XXbit flag set but the DC_Y444
> flag isn't, we'll assume that we support that deep colour mode for
> YUV444 which breaks the specification.
> 
> In order to fix this, let's split the edid_hdmi_dc_modes field in struct
> drm_display_info into two fields, one for RGB444 and one for YUV444.
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Fixes: d0c94692e0a3 ("drm/edid: Parse and handle HDMI deep color modes.")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Though we seem to be missing clearing of *dc_modes in
drm_reset_display_info()...

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c |  2 +-
>  drivers/gpu/drm/drm_edid.c                     |  7 ++++---
>  drivers/gpu/drm/i915/display/intel_hdmi.c      |  4 ++--
>  drivers/gpu/drm/radeon/radeon_connectors.c     |  2 +-
>  include/drm/drm_connector.h                    | 12 +++++++++---
>  5 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> index c16a2704ced6..f3160b951df3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
> @@ -175,7 +175,7 @@ int amdgpu_connector_get_monitor_bpc(struct drm_connector *connector)
>  
>  			/* Check if bpc is within clock limit. Try to degrade gracefully otherwise */
>  			if ((bpc == 12) && (mode_clock * 3/2 > max_tmds_clock)) {
> -				if ((connector->display_info.edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_30) &&
> +				if ((connector->display_info.edid_hdmi_rgb444_dc_modes & DRM_EDID_HDMI_DC_30) &&
>  				    (mode_clock * 5/4 <= max_tmds_clock))
>  					bpc = 10;
>  				else
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 5085ef08c22d..471b577dca79 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -5075,21 +5075,21 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
>  
>  	if (hdmi[6] & DRM_EDID_HDMI_DC_30) {
>  		dc_bpc = 10;
> -		info->edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_30;
> +		info->edid_hdmi_rgb444_dc_modes |= DRM_EDID_HDMI_DC_30;
>  		DRM_DEBUG("%s: HDMI sink does deep color 30.\n",
>  			  connector->name);
>  	}
>  
>  	if (hdmi[6] & DRM_EDID_HDMI_DC_36) {
>  		dc_bpc = 12;
> -		info->edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_36;
> +		info->edid_hdmi_rgb444_dc_modes |= DRM_EDID_HDMI_DC_36;
>  		DRM_DEBUG("%s: HDMI sink does deep color 36.\n",
>  			  connector->name);
>  	}
>  
>  	if (hdmi[6] & DRM_EDID_HDMI_DC_48) {
>  		dc_bpc = 16;
> -		info->edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_48;
> +		info->edid_hdmi_rgb444_dc_modes |= DRM_EDID_HDMI_DC_48;
>  		DRM_DEBUG("%s: HDMI sink does deep color 48.\n",
>  			  connector->name);
>  	}
> @@ -5106,6 +5106,7 @@ static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
>  
>  	/* YCRCB444 is optional according to spec. */
>  	if (hdmi[6] & DRM_EDID_HDMI_DC_Y444) {
> +		info->edid_hdmi_ycbcr444_dc_modes = info->edid_hdmi_rgb444_dc_modes;
>  		DRM_DEBUG("%s: HDMI sink does YCRCB444 in deep color.\n",
>  			  connector->name);
>  	}
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index 96e508ddc4af..52f6dc248453 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -1912,7 +1912,7 @@ static bool intel_hdmi_sink_bpc_possible(struct drm_connector *connector,
>  		if (ycbcr420_output)
>  			return hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_36;
>  		else
> -			return info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36;
> +			return info->edid_hdmi_rgb444_dc_modes & DRM_EDID_HDMI_DC_36;
>  	case 10:
>  		if (!has_hdmi_sink)
>  			return false;
> @@ -1920,7 +1920,7 @@ static bool intel_hdmi_sink_bpc_possible(struct drm_connector *connector,
>  		if (ycbcr420_output)
>  			return hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_30;
>  		else
> -			return info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_30;
> +			return info->edid_hdmi_rgb444_dc_modes & DRM_EDID_HDMI_DC_30;
>  	case 8:
>  		return true;
>  	default:
> diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
> index 607ad5620bd9..1546abcadacf 100644
> --- a/drivers/gpu/drm/radeon/radeon_connectors.c
> +++ b/drivers/gpu/drm/radeon/radeon_connectors.c
> @@ -204,7 +204,7 @@ int radeon_get_monitor_bpc(struct drm_connector *connector)
>  
>  			/* Check if bpc is within clock limit. Try to degrade gracefully otherwise */
>  			if ((bpc == 12) && (mode_clock * 3/2 > max_tmds_clock)) {
> -				if ((connector->display_info.edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_30) &&
> +				if ((connector->display_info.edid_hdmi_rgb444_dc_modes & DRM_EDID_HDMI_DC_30) &&
>  					(mode_clock * 5/4 <= max_tmds_clock))
>  					bpc = 10;
>  				else
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index b501d0badaea..eaf0ef5f1843 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -592,10 +592,16 @@ struct drm_display_info {
>  	bool rgb_quant_range_selectable;
>  
>  	/**
> -	 * @edid_hdmi_dc_modes: Mask of supported hdmi deep color modes. Even
> -	 * more stuff redundant with @bus_formats.
> +	 * @edid_hdmi_dc_rgb444_modes: Mask of supported hdmi deep color modes
> +	 * in RGB 4:4:4. Even more stuff redundant with @bus_formats.
>  	 */
> -	u8 edid_hdmi_dc_modes;
> +	u8 edid_hdmi_rgb444_dc_modes;
> +
> +	/**
> +	 * @edid_hdmi_dc_ycbcr444_modes: Mask of supported hdmi deep color
> +	 * modes in YCbCr 4:4:4. Even more stuff redundant with @bus_formats.
> +	 */
> +	u8 edid_hdmi_ycbcr444_dc_modes;
>  
>  	/**
>  	 * @cea_rev: CEA revision of the HDMI sink.
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel