[PATCH] drm: disable deep color when EDID violates spec
Alex Deucher
alexdeucher at gmail.com
Tue Jan 10 20:10:33 UTC 2017
On Tue, Jan 10, 2017 at 3:02 PM, Ernst Sjöstrand <ernstp at 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.
Alex
>
> 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
>
>
More information about the dri-devel
mailing list