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

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

 



On Tue, 10 Jan 2017, Harry Wentland <harry.wentland@xxxxxxx> 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@xxxxxxxxx>
>>> 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



>
> 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@xxxxxxxxx>:
>>>>>
>>>>> On Tue, Jan 10, 2017 at 12:46 PM, Ville Syrjälä
>>>>> <ville.syrjala@xxxxxxxxxxxxxxx> 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@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'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@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
>>>>>>>>
>>>>>>
>>>>>> --
>>>>>> 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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
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