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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jan 11, 2017 at 12:04:09PM +0200, Jani Nikula wrote:
> On Tue, 10 Jan 2017, Harry Wentland <harry.wentland@amd.com> wrote:
> > On 2017-01-10 03:41 PM, Harry Wentland wrote:
> >> On 2017-01-10 03:10 PM, Alex Deucher wrote:
> >>> On Tue, Jan 10, 2017 at 3:02 PM, Ernst Sjöstrand <ernstp@gmail.com>
> >>> wrote:
> >>>> 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?
> >>>
> >>> We see 10 bpc only over HDMI a lot apparently.  I guess a lot of
> >>> monitor vendors do the minimum necessary for deep color.
> >>>
> >>
> >> Yes, apparently there are a bunch of HDMI displays that we drive in
> >> 10bpc that might or might not report 12bpc support. From talking to our
> >> HDMI guys it sounds like this is a slightly ambiguous area in the spec,
> >> despite what the HDMI spec quote mentioned by Nicholas said. I'll see if
> >> I can get more info.
> >>
> >
> > Adding Ernst, Nicholas, Ville, Alex again.
> >
> > So apparently the spec is pretty clear and there's a sink compliance 
> > test that should cover this. I don't really think it makes sense for the 
> > source device to babysit the sink's behavior, though.
> >
> > In general we might want to try for a solution that gives the best user 
> > experience and highest interoperability, so check what sink supports, 
> > check what source supports, check what cable can do, and do an 
> > intersection of all to give the best user experience.
> >
> > I suggest to block 10-bit on driver's that can't handle this.
> 
> Agreed. Pretty much boils down to what I wrote in [1].
> 
> BR,
> Jani.
> 
> [1] https://lists.freedesktop.org/archives/dri-devel/2017-January/129374.html

After a bit of digging around I found my patch to check the DC_36 bit
in i915 code:
git://github.com/vsyrjala/linux.git hdmi_sink_tmds_limit_2

Nicholas, you want to give that go?

> 
> 
> 
> >
> > Harry
> >
> >
> >> I'm not sure it makes sense to block all deep color modes in this case
> >> for all drivers. Other HW (e.g. AMD's) is perfectly capable of driving
> >> 10-bit. Would it make sense to just block this on the i915 side as Ville
> >> alluded to on another thread?
> >>
> >> Harry
> >>
> >>> Alex
> >>>
> >>>>
> >>>> Regards
> >>>> //Ernst
> >>>>
> >>>> 2017-01-10 20:54 GMT+01:00 Alex Deucher <alexdeucher@gmail.com>:
> >>>>>
> >>>>> On Tue, Jan 10, 2017 at 12:46 PM, Ville Syrjälä
> >>>>> <ville.syrjala@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@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@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@linux.intel.com>
> >>>>>>>>>> Signed-off-by: Nicholas Sielicki <nicholas.sielicki@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@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@lists.freedesktop.org
> >>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> _______________________________________________
> >>>>>>>> dri-devel mailing list
> >>>>>>>> dri-devel@lists.freedesktop.org
> >>>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>>>>>>>
> >>>>>>
> >>>>>> --
> >>>>>> Ville Syrjälä
> >>>>>> Intel OTC
> >>>>
> >>>>
> >>> _______________________________________________
> >>> dri-devel mailing list
> >>> dri-devel@lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>>
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel