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.syrj...@linux.intel.com>
> Fixes: d0c94692e0a3 ("drm/edid: Parse and handle HDMI deep color modes.")
> Signed-off-by: Maxime Ripard <max...@cerno.tech>
Reviewed-by: Ville Syrjälä <ville.syrj...@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
