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

[Daniel Vetter]

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>

I guess we need Cc: stable at vger.kernel.org on this too? Any bugzilla links
to reference?

Ville, if this makes sense for you too, can you pls push it to
drm-misc-fixes?

Thanks, Daniel

> ---
>  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;
> +
>  	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
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch