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

[Ville Syrjälä]

On Tue, Jan 10, 2017 at 03:39:42PM +0200, Jani Nikula wrote:
> On Tue, 10 Jan 2017, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> > 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.
> >>
> 
> I thought there was a related bugzilla? Where's the reference?

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99250

> 
> >> 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.
> 
> Hmmh.
> 
> So currently, the sink in question violates this, "All Deep Color modes
> are optional though if an HDMI Source or Sink supports any Deep Color
> mode, it shall support 36-bit mode."
> 
> But also currently, we violate this, "An HDMI Source shall not send any
> Deep Color mode to a Sink that does not indicate support for that mode."
> 
> Instead of fixing our violation, this patch points fingers at the
> violating sinks, and refuses to play ball with any deep colors.
> 
> Just to get the record straight, is that a fair assesment?

More or less. i915 can't violate the spec unless the sink violates the
spec as well. I did actually write a patch once to explicitly check the
DC_36 bit in i915 code, but I don't think I ever sent it out as I
figured it's an impossible scenario. But I should have known better and
assumed that some sink will eventually violate the spec.

-- 
Ville Syrjälä
Intel OTC