Re: [PATCH] drm: disable deep color when EDID violates spec

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jan 10, 2017 at 6:33 AM, Ernst Sjöstrand <ernstp@xxxxxxxxx> 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 can talk to our display team to
see what other OSes do.

Alex

> Regards
> //Ernst
>
> 2017-01-10 11:52 GMT+01:00 Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>:
>>
>> 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@xxxxxxxxxxxxxxx>
>> > Signed-off-by: Nicholas Sielicki <nicholas.sielicki@xxxxxxxxx>
>> > ---
>> >  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@xxxxxxxxxxxxxxx>
>>
>> > +
>> >       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@xxxxxxxxxxxxxxxxxxxxx
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux