| Message ID | 20220120151625.594595-4-maxime@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vc4: hdmi: Yet Another Approach to HDMI YUV output | expand |
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
On Thu, 20 Jan 2022 16:16:12 +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. > > [...] Applied to drm/drm-misc (drm-misc-next). Thanks! Maxime
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.
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> --- 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(-)