[PATCH] drm: disable deep color when EDID violates spec
Ernst Sjöstrand
ernstp at gmail.com
Tue Jan 10 20:02:49 UTC 2017
I kindof assume DP is the default connection these days and with Freesync
you use
DP or course, but this question was specifically for HDMI.
I guess this patch doesn't affect deep color over DP?
Anyway, only 17 of those monitors have FreeSync but almost all have DP, so
perhaps they only support
10 bpc when connected with DP?
Regards
//Ernst
2017-01-10 20:54 GMT+01:00 Alex Deucher <alexdeucher at gmail.com>:
> On Tue, Jan 10, 2017 at 12:46 PM, Ville Syrjälä
> <ville.syrjala at linux.intel.com> wrote:
> > On Tue, Jan 10, 2017 at 12:27:45PM -0500, Alex Deucher wrote:
> >> On Tue, Jan 10, 2017 at 6:33 AM, Ernst Sjöstrand <ernstp at gmail.com>
> wrote:
> >> > 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
> >> >
> >>
> >> FWIW, I've almost never seen a monitor that supports 12 bpc, but I've
> >> see quite a few 10 bpc monitors.
> >
> > I've seen plenty of monitors that do 10bpc over DP, but I've never seen
> > anything that does 10bpc over HDMI. 12bpc over HDMI is pretty common
> > in "proper" TVs in my experience.
> >
> >> I can talk to our display team to
> >> see what other OSes do.
> >
> > Thanks. That should give us some idea if anyone ever tried 10bpc
> > over HDMI on these things.
>
> We apparently see this pretty commonly, especially with freesync
> monitors, and we support it. It seems to be pretty prevalent because
> you can support a higher refresh rate with a lower bpc.
>
> Alex
>
> >
> >>
> >> Alex
> >>
> >> > 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
> >> >
> >> >
> >> >
> >> > _______________________________________________
> >> > dri-devel mailing list
> >> > dri-devel at lists.freedesktop.org
> >> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >> >
> >
> > --
> > Ville Syrjälä
> > Intel OTC
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20170110/24b33a5e/attachment-0001.html>
More information about the dri-devel
mailing list