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@xxxxxxxxxxxxxxx> > Fixes: d0c94692e0a3 ("drm/edid: Parse and handle HDMI deep color modes.") > Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx> Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> 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