[PATCH] drm: disable deep color when EDID violates spec
Ernst Sjöstrand
ernstp at gmail.com
Tue Jan 10 11:33:35 UTC 2017
Isn't 10bpc very common among monitors, and 12bpc very rare? Or maybe I'm
confusing the transport layer with the presentation capabilities...?
Here are 201 monitors that claim 10bpc:
http://pricespy.co.uk/category.php?l=s300859434&o=eg_401#prodlista
Regards
//Ernst
2017-01-10 11:52 GMT+01:00 Ville Syrjälä <ville.syrjala at linux.intel.com>:
> On Thu, Jan 05, 2017 at 05:45:23PM -0600, Nicholas Sielicki wrote:
> > As per the HDMI 1.3 and 1.4 spec, "deep color modes" are color depths
> > greater than 24 bits per pixel. The spec explicitly states, "All Deep
> > Color modes are optional though if an HDMI Source or Sink supports any
> > Deep Color mode, it shall support 36-bit mode." (6.2.4 Color Depth
> > Requirements).
> >
> > I came across a monitor (Acer X233H) that supplies an illegal EDID where
> > DC_30bit is set and DC_36bit is not set. The end result is badly garbled
> > output because the driver is sending 36BPP when the monitor can't handle
> > it.
> >
> > Much of the intel hardware is incapable of operating at any
> > bit-per-component setting outside of 8 or 12, and the spec seems to
> > imply that if any deep color support is found, then it is a safe
> > assumption to operate at 12.
> >
> > This patch ensures that the EDID is within the spec (specifically, that
> > DC_36bit is set) before committing to going forward with any deep
> > colors. There was already a check for this EDID inconsistency, but it
> > resulted only in a warning and did not fall-back to safer settings.
> >
> > CC: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Signed-off-by: Nicholas Sielicki <nicholas.sielicki at gmail.com>
> > ---
> > drivers/gpu/drm/drm_edid.c | 35 +++++++++++++++++++++++------------
> > 1 file changed, 23 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 336be31ff3de..42ce3f54d2dc 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -3772,30 +3772,34 @@ static void drm_parse_hdmi_deep_color_info(struct
> drm_connector *connector,
> > {
> > struct drm_display_info *info = &connector->display_info;
> > unsigned int dc_bpc = 0;
> > + u8 modes = 0;
> >
> > /* HDMI supports at least 8 bpc */
> > info->bpc = 8;
> >
> > + /* Ensure all DC modes are unset if we return early */
> > + info->edid_hdmi_dc_modes = 0;
>
> Clearing this in drm_add_display_info() should be sufficient since
> this guy doesn't get called from anywhere else. So this part could
> be droppped.
>
> Otherwise this feels like a decent way to handle the problem. We
> could of course try to use the 10bpc (or whatever) deep color modes
> the sink claims to support, but given that the people designing the
> thing didn't bother reading the spec, it seems safer to just disable
> deep color support entirely.
>
> Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
> > +
> > if (cea_db_payload_len(hdmi) < 6)
> > return;
> >
> > if (hdmi[6] & DRM_EDID_HDMI_DC_30) {
> > dc_bpc = 10;
> > - info->edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_30;
> > + 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;
> > + 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;
> > + modes |= DRM_EDID_HDMI_DC_48;
> > DRM_DEBUG("%s: HDMI sink does deep color 48.\n",
> > connector->name);
> > }
> > @@ -3806,9 +3810,24 @@ static void drm_parse_hdmi_deep_color_info(struct
> drm_connector *connector,
> > return;
> > }
> >
> > + /*
> > + * All deep color modes are optional, but if a sink supports any
> deep
> > + * color mode, it must support 36-bit mode. If this is found not
> > + * to be the case, sink is in violation of HDMI 1.3 / 1.4 spec and
> it
> > + * is prudent to disable all deep color modes. Return here before
> > + * committing bpc and edid_hdmi_dc_modes.
> > + */
> > + if (!(modes & DRM_EDID_HDMI_DC_36)) {
> > + DRM_DEBUG("%s: HDMI sink should do DC_36, but does not!\n",
> > + connector->name);
> > + return;
> > + }
> > +
> > +
> > DRM_DEBUG("%s: Assigning HDMI sink color depth as %d bpc.\n",
> > connector->name, dc_bpc);
> > info->bpc = dc_bpc;
> > + info->edid_hdmi_dc_modes = modes;
> >
> > /*
> > * Deep color support mandates RGB444 support for all video
> > @@ -3823,15 +3842,6 @@ static void drm_parse_hdmi_deep_color_info(struct
> drm_connector *connector,
> > DRM_DEBUG("%s: HDMI sink does YCRCB444 in deep color.\n",
> > connector->name);
> > }
> > -
> > - /*
> > - * Spec says that if any deep color mode is supported at all,
> > - * then deep color 36 bit must be supported.
> > - */
> > - if (!(hdmi[6] & DRM_EDID_HDMI_DC_36)) {
> > - DRM_DEBUG("%s: HDMI sink should do DC_36, but does not!\n",
> > - connector->name);
> > - }
> > }
> >
> > static void
> > @@ -3895,6 +3905,7 @@ static void drm_add_display_info(struct
> drm_connector *connector,
> > /* driver figures it out in this case */
> > info->bpc = 0;
> > info->color_formats = 0;
> > + info->edid_hdmi_dc_modes = 0;
> > info->cea_rev = 0;
> > info->max_tmds_clock = 0;
> > info->dvi_dual = false;
> > --
> > 2.11.0
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20170110/22794c37/attachment-0001.html>
More information about the dri-devel
mailing list