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

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

[Nicholas Sielicki]

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

Re: [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@linux.intel.com>
> Signed-off-by: Nicholas Sielicki <nicholas.sielicki@gmail.com>

I guess we need Cc: stable@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

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

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

[Ernst Sjöstrand]

[-- Attachment #1.1: Type: text/plain, Size: 6348 bytes --]

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

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
>

[-- Attachment #1.2: Type: text/html, Size: 8568 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

On Tue, Jan 10, 2017 at 12:33:35PM +0100, Ernst Sjöstrand 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

I suppose that refers to the panel? Not sure.

> 
> 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
> >

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

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

[Jani Nikula]

On Tue, 10 Jan 2017, Ville Syrjälä <ville.syrjala@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?

>> 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.

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?

BR,
Jani.


>
> 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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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@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@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.
> 
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Alex Deucher]

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 can talk to our display team to
see what other OSes do.

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

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

[Ville Syrjälä]

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.

> 
> 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

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

[Alex Deucher]

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

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

[Ernst Sjöstrand]

[-- Attachment #1.1: Type: text/plain, Size: 9088 bytes --]

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?

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
>

[-- Attachment #1.2: Type: text/html, Size: 13822 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Alex Deucher]

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.

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

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

[Harry Wentland]

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.

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

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

[Harry Wentland]

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.

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

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

[Jani Nikula]

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



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

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

[Ville Syrjälä]

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

[PATCH] drm/i915: Reject HDMI 12bpc if the sink doesn't indicate support

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Check that the sink really declared 12bpc support before we enable it.
This should not actually never happen since it's mandatory for HDMI
sinks to support 12bpc if they support any deep color modes. But
reality disagrees with the theory and there are actually sinks in
the wild that violate the spec.

v2: Fix the output_types check
    Update commit message to state that these things are in fact real

Cc: stable@vger.kernel.org
Cc: Nicholas Sielicki <nicholas.sielicki@gmail.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99250
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index a580de80d2b5..2bf5915284aa 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1298,16 +1298,34 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
 
 static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)
 {
-	struct drm_device *dev = crtc_state->base.crtc->dev;
+	struct drm_i915_private *dev_priv =
+		to_i915(crtc_state->base.crtc->dev);
+	struct drm_atomic_state *state = crtc_state->base.state;
+	struct drm_connector_state *connector_state;
+	struct drm_connector *connector;
+	int i;
 
-	if (HAS_GMCH_DISPLAY(to_i915(dev)))
+	if (HAS_GMCH_DISPLAY(dev_priv))
 		return false;
 
 	/*
 	 * HDMI 12bpc affects the clocks, so it's only possible
 	 * when not cloning with other encoder types.
 	 */
-	return crtc_state->output_types == 1 << INTEL_OUTPUT_HDMI;
+	if (crtc_state->output_types != 1 << INTEL_OUTPUT_HDMI)
+		return false;
+
+	for_each_connector_in_state(state, connector, connector_state, i) {
+		const struct drm_display_info *info = &connector->display_info;
+
+		if (connector_state->crtc != crtc_state->base.crtc)
+			continue;
+
+		if ((info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36) == 0)
+			return false;
+	}
+
+	return true;
 }
 
 bool intel_hdmi_compute_config(struct intel_encoder *encoder,
-- 
2.10.2

✓ Fi.CI.BAT: success for drm/i915: Reject HDMI 12bpc if the sink doesn't indicate support

[Patchwork]

== Series Details ==

Series: drm/i915: Reject HDMI 12bpc if the sink doesn't indicate support
URL   : https://patchwork.freedesktop.org/series/19570/
State : success

== Summary ==

Series 19570v1 drm/i915: Reject HDMI 12bpc if the sink doesn't indicate support
https://patchwork.freedesktop.org/api/1.0/series/19570/revisions/1/mbox/

fi-bdw-5557u     total:252  pass:241  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:252  pass:213  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:252  pass:233  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:83   pass:70   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:252  pass:225  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:252  pass:221  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:252  pass:236  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:252  pass:202  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:252  pass:234  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:252  pass:235  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:252  pass:230  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:252  pass:242  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:252  pass:224  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:252  pass:223  dwarn:0   dfail:0   fail:0   skip:29 

58294e4063fcc710bd7bc97349c9753f1978d5c1 drm-tip: 2017y-02m-13d-11h-20m-27s UTC integration manifest
1cf085c drm/i915: Reject HDMI 12bpc if the sink doesn't indicate support

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3794/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: FW: [PATCH] drm/i915: Reject HDMI 12bpc if the sink doesn't indicate support

[Sharma, Shashank]

Regards

Shashank

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Check that the sink really declared 12bpc support before we enable it.
> This should not actually never happen since it's mandatory for HDMI sinks to support 12bpc if they support any deep color modes. But reality disagrees with the theory and there are actually sinks in the wild that violate the spec.
>
> v2: Fix the output_types check
>      Update commit message to state that these things are in fact real
>
> Cc: stable@vger.kernel.org
> Cc: Nicholas Sielicki <nicholas.sielicki@gmail.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99250
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_hdmi.c | 24 +++++++++++++++++++++---
>   1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index a580de80d2b5..2bf5915284aa 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1298,16 +1298,34 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
>   
>   static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)  {
> -	struct drm_device *dev = crtc_state->base.crtc->dev;
> +	struct drm_i915_private *dev_priv =
> +		to_i915(crtc_state->base.crtc->dev);
> +	struct drm_atomic_state *state = crtc_state->base.state;
> +	struct drm_connector_state *connector_state;
> +	struct drm_connector *connector;
> +	int i;
>   
> -	if (HAS_GMCH_DISPLAY(to_i915(dev)))
> +	if (HAS_GMCH_DISPLAY(dev_priv))
>   		return false;
>   
>   	/*
>   	 * HDMI 12bpc affects the clocks, so it's only possible
>   	 * when not cloning with other encoder types.
>   	 */
> -	return crtc_state->output_types == 1 << INTEL_OUTPUT_HDMI;
> +	if (crtc_state->output_types != 1 << INTEL_OUTPUT_HDMI)
> +		return false;
> +
This function is called from only one place ( for now), and that already 
has a pipe_config->has_hdmi_sink check.
Does it makes sense to have only one of the checks ? I can understand 
that this might be to comple
> +	for_each_connector_in_state(state, connector, connector_state, i) {
> +		const struct drm_display_info *info = &connector->display_info;
> +
> +		if (connector_state->crtc != crtc_state->base.crtc)
> +			continue;
> +
> +		if ((info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36) == 0)
> +			return false;
Do we want to have a info->bpc check too, or they are more or less same ?
- Shashank
> +	}
> +
> +	return true;
>   }
>   
>   bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> --
> 2.10.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: FW: [PATCH] drm/i915: Reject HDMI 12bpc if the sink doesn't indicate support

[Sharma, Shashank]

Regards

Shashank

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Check that the sink really declared 12bpc support before we enable it.
> This should not actually never happen since it's mandatory for HDMI sinks to support 12bpc if they support any deep color modes. But reality disagrees with the theory and there are actually sinks in the wild that violate the spec.
>
> v2: Fix the output_types check
>      Update commit message to state that these things are in fact real
>
> Cc: stable@vger.kernel.org
> Cc: Nicholas Sielicki <nicholas.sielicki@gmail.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99250
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_hdmi.c | 24 +++++++++++++++++++++---
>   1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index a580de80d2b5..2bf5915284aa 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1298,16 +1298,34 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
>   
>   static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)  {
> -	struct drm_device *dev = crtc_state->base.crtc->dev;
> +	struct drm_i915_private *dev_priv =
> +		to_i915(crtc_state->base.crtc->dev);
> +	struct drm_atomic_state *state = crtc_state->base.state;
> +	struct drm_connector_state *connector_state;
> +	struct drm_connector *connector;
> +	int i;
>   
> -	if (HAS_GMCH_DISPLAY(to_i915(dev)))
> +	if (HAS_GMCH_DISPLAY(dev_priv))
>   		return false;
>   
>   	/*
>   	 * HDMI 12bpc affects the clocks, so it's only possible
>   	 * when not cloning with other encoder types.
>   	 */
> -	return crtc_state->output_types == 1 << INTEL_OUTPUT_HDMI;
> +	if (crtc_state->output_types != 1 << INTEL_OUTPUT_HDMI)
> +		return false;
> +
This function is called from only one place ( for now), and that already 
has a pipe_config->has_hdmi_sink check.
Does it makes sense to have only one of the checks ? I can understand 
that this might be to comple
> +	for_each_connector_in_state(state, connector, connector_state, i) {
> +		const struct drm_display_info *info = &connector->display_info;
> +
> +		if (connector_state->crtc != crtc_state->base.crtc)
> +			continue;
> +
> +		if ((info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36) == 0)
> +			return false;
Do we want to have a info->bpc check too, or they are more or less same ?
- Shashank
> +	}
> +
> +	return true;
>   }
>   
>   bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> --
> 2.10.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: FW: [PATCH] drm/i915: Reject HDMI 12bpc if the sink doesn't indicate support

[Ville Syrjälä]

On Mon, Mar 13, 2017 at 12:22:53PM +0200, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> > From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >
> > Check that the sink really declared 12bpc support before we enable it.
> > This should not actually never happen since it's mandatory for HDMI sinks to support 12bpc if they support any deep color modes. But reality disagrees with the theory and there are actually sinks in the wild that violate the spec.
> >
> > v2: Fix the output_types check
> >      Update commit message to state that these things are in fact real
> >
> > Cc: stable@vger.kernel.org
> > Cc: Nicholas Sielicki <nicholas.sielicki@gmail.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99250
> > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_hdmi.c | 24 +++++++++++++++++++++---
> >   1 file changed, 21 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index a580de80d2b5..2bf5915284aa 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -1298,16 +1298,34 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
> >   
> >   static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)  {
> > -	struct drm_device *dev = crtc_state->base.crtc->dev;
> > +	struct drm_i915_private *dev_priv =
> > +		to_i915(crtc_state->base.crtc->dev);
> > +	struct drm_atomic_state *state = crtc_state->base.state;
> > +	struct drm_connector_state *connector_state;
> > +	struct drm_connector *connector;
> > +	int i;
> >   
> > -	if (HAS_GMCH_DISPLAY(to_i915(dev)))
> > +	if (HAS_GMCH_DISPLAY(dev_priv))
> >   		return false;
> >   
> >   	/*
> >   	 * HDMI 12bpc affects the clocks, so it's only possible
> >   	 * when not cloning with other encoder types.
> >   	 */
> > -	return crtc_state->output_types == 1 << INTEL_OUTPUT_HDMI;
> > +	if (crtc_state->output_types != 1 << INTEL_OUTPUT_HDMI)
> > +		return false;
> > +
> This function is called from only one place ( for now), and that already 
> has a pipe_config->has_hdmi_sink check.
> Does it makes sense to have only one of the checks ? I can understand 
> that this might be to comple

has_hdmi_sink is not the same thing. It just says we're talking to at
least one HDMI sink and thus can send infoframes/audio. output_types on 
the other hand lists all the different port types we're cloning with.
So you can do HDMI+VGA for instance and still the HDMI goes to a real
HDMI sink so we'll have has_hdmi_sink==true. But since VGA can't deal
with the 1.5x clock we can't do deep color when cloning.

> > +	for_each_connector_in_state(state, connector, connector_state, i) {
> > +		const struct drm_display_info *info = &connector->display_info;
> > +
> > +		if (connector_state->crtc != crtc_state->base.crtc)
> > +			continue;
> > +
> > +		if ((info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36) == 0)
> > +			return false;
> Do we want to have a info->bpc check too, or they are more or less same ?

That was already checked at the start of the state computation. So
state->pipe_bpp already accounts for that. However as info->bpc is just
the max bpc the sink can do it's not sufficient to guarantee it can doo
all lower bpc values.

> - Shashank
> > +	}
> > +
> > +	return true;
> >   }
> >   
> >   bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> > --
> > 2.10.2
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrj�l�
Intel OTC

Re: FW: [PATCH] drm/i915: Reject HDMI 12bpc if the sink doesn't indicate support

[Ville Syrjälä]

On Mon, Mar 13, 2017 at 12:22:53PM +0200, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Check that the sink really declared 12bpc support before we enable it.
> > This should not actually never happen since it's mandatory for HDMI sinks to support 12bpc if they support any deep color modes. But reality disagrees with the theory and there are actually sinks in the wild that violate the spec.
> >
> > v2: Fix the output_types check
> >      Update commit message to state that these things are in fact real
> >
> > Cc: stable@vger.kernel.org
> > Cc: Nicholas Sielicki <nicholas.sielicki@gmail.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99250
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_hdmi.c | 24 +++++++++++++++++++++---
> >   1 file changed, 21 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index a580de80d2b5..2bf5915284aa 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -1298,16 +1298,34 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
> >   
> >   static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)  {
> > -	struct drm_device *dev = crtc_state->base.crtc->dev;
> > +	struct drm_i915_private *dev_priv =
> > +		to_i915(crtc_state->base.crtc->dev);
> > +	struct drm_atomic_state *state = crtc_state->base.state;
> > +	struct drm_connector_state *connector_state;
> > +	struct drm_connector *connector;
> > +	int i;
> >   
> > -	if (HAS_GMCH_DISPLAY(to_i915(dev)))
> > +	if (HAS_GMCH_DISPLAY(dev_priv))
> >   		return false;
> >   
> >   	/*
> >   	 * HDMI 12bpc affects the clocks, so it's only possible
> >   	 * when not cloning with other encoder types.
> >   	 */
> > -	return crtc_state->output_types == 1 << INTEL_OUTPUT_HDMI;
> > +	if (crtc_state->output_types != 1 << INTEL_OUTPUT_HDMI)
> > +		return false;
> > +
> This function is called from only one place ( for now), and that already 
> has a pipe_config->has_hdmi_sink check.
> Does it makes sense to have only one of the checks ? I can understand 
> that this might be to comple

has_hdmi_sink is not the same thing. It just says we're talking to at
least one HDMI sink and thus can send infoframes/audio. output_types on 
the other hand lists all the different port types we're cloning with.
So you can do HDMI+VGA for instance and still the HDMI goes to a real
HDMI sink so we'll have has_hdmi_sink==true. But since VGA can't deal
with the 1.5x clock we can't do deep color when cloning.

> > +	for_each_connector_in_state(state, connector, connector_state, i) {
> > +		const struct drm_display_info *info = &connector->display_info;
> > +
> > +		if (connector_state->crtc != crtc_state->base.crtc)
> > +			continue;
> > +
> > +		if ((info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36) == 0)
> > +			return false;
> Do we want to have a info->bpc check too, or they are more or less same ?

That was already checked at the start of the state computation. So
state->pipe_bpp already accounts for that. However as info->bpc is just
the max bpc the sink can do it's not sufficient to guarantee it can doo
all lower bpc values.

> - Shashank
> > +	}
> > +
> > +	return true;
> >   }
> >   
> >   bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> > --
> > 2.10.2
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: FW: [PATCH] drm/i915: Reject HDMI 12bpc if the sink doesn't indicate support

[Sharma, Shashank]

Regards

Shashank


On 3/13/2017 12:53 PM, Ville Syrj�l� wrote:
> On Mon, Mar 13, 2017 at 12:22:53PM +0200, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
>>>
>>> Check that the sink really declared 12bpc support before we enable it.
>>> This should not actually never happen since it's mandatory for HDMI sinks to support 12bpc if they support any deep color modes. But reality disagrees with the theory and there are actually sinks in the wild that violate the spec.
>>>
>>> v2: Fix the output_types check
>>>       Update commit message to state that these things are in fact real
>>>
>>> Cc: stable@vger.kernel.org
>>> Cc: Nicholas Sielicki <nicholas.sielicki@gmail.com>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99250
>>> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/intel_hdmi.c | 24 +++++++++++++++++++++---
>>>    1 file changed, 21 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>>> index a580de80d2b5..2bf5915284aa 100644
>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>>> @@ -1298,16 +1298,34 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
>>>    
>>>    static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)  {
>>> -	struct drm_device *dev = crtc_state->base.crtc->dev;
>>> +	struct drm_i915_private *dev_priv =
>>> +		to_i915(crtc_state->base.crtc->dev);
>>> +	struct drm_atomic_state *state = crtc_state->base.state;
>>> +	struct drm_connector_state *connector_state;
>>> +	struct drm_connector *connector;
>>> +	int i;
>>>    
>>> -	if (HAS_GMCH_DISPLAY(to_i915(dev)))
>>> +	if (HAS_GMCH_DISPLAY(dev_priv))
>>>    		return false;
>>>    
>>>    	/*
>>>    	 * HDMI 12bpc affects the clocks, so it's only possible
>>>    	 * when not cloning with other encoder types.
>>>    	 */
>>> -	return crtc_state->output_types == 1 << INTEL_OUTPUT_HDMI;
>>> +	if (crtc_state->output_types != 1 << INTEL_OUTPUT_HDMI)
>>> +		return false;
>>> +
>> This function is called from only one place ( for now), and that already
>> has a pipe_config->has_hdmi_sink check.
>> Does it makes sense to have only one of the checks ? I can understand
>> that this might be to comple
> has_hdmi_sink is not the same thing. It just says we're talking to at
> least one HDMI sink and thus can send infoframes/audio. output_types on
> the other hand lists all the different port types we're cloning with.
> So you can do HDMI+VGA for instance and still the HDMI goes to a real
> HDMI sink so we'll have has_hdmi_sink==true. But since VGA can't deal
> with the 1.5x clock we can't do deep color when cloning.
Makes sense :-).
>>> +	for_each_connector_in_state(state, connector, connector_state, i) {
>>> +		const struct drm_display_info *info = &connector->display_info;
>>> +
>>> +		if (connector_state->crtc != crtc_state->base.crtc)
>>> +			continue;
>>> +
>>> +		if ((info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36) == 0)
>>> +			return false;
>> Do we want to have a info->bpc check too, or they are more or less same ?
> That was already checked at the start of the state computation. So
> state->pipe_bpp already accounts for that. However as info->bpc is just
> the max bpc the sink can do it's not sufficient to guarantee it can doo
> all lower bpc values.
I guess just in this scenario, the max bpc value(12) and the value of 
our interest is same, so it was making
sense, but I agree, would not be true for all cases like max 16bpc.

Please feel free to use:
Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>

- Shashank
>
>> - Shashank
>>> +	}
>>> +
>>> +	return true;
>>>    }
>>>    
>>>    bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>>> --
>>> 2.10.2
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: FW: [PATCH] drm/i915: Reject HDMI 12bpc if the sink doesn't indicate support

[Sharma, Shashank]

Regards

Shashank


On 3/13/2017 12:53 PM, Ville Syrjälä wrote:
> On Mon, Mar 13, 2017 at 12:22:53PM +0200, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> Check that the sink really declared 12bpc support before we enable it.
>>> This should not actually never happen since it's mandatory for HDMI sinks to support 12bpc if they support any deep color modes. But reality disagrees with the theory and there are actually sinks in the wild that violate the spec.
>>>
>>> v2: Fix the output_types check
>>>       Update commit message to state that these things are in fact real
>>>
>>> Cc: stable@vger.kernel.org
>>> Cc: Nicholas Sielicki <nicholas.sielicki@gmail.com>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99250
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/intel_hdmi.c | 24 +++++++++++++++++++++---
>>>    1 file changed, 21 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>>> index a580de80d2b5..2bf5915284aa 100644
>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>>> @@ -1298,16 +1298,34 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
>>>    
>>>    static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)  {
>>> -	struct drm_device *dev = crtc_state->base.crtc->dev;
>>> +	struct drm_i915_private *dev_priv =
>>> +		to_i915(crtc_state->base.crtc->dev);
>>> +	struct drm_atomic_state *state = crtc_state->base.state;
>>> +	struct drm_connector_state *connector_state;
>>> +	struct drm_connector *connector;
>>> +	int i;
>>>    
>>> -	if (HAS_GMCH_DISPLAY(to_i915(dev)))
>>> +	if (HAS_GMCH_DISPLAY(dev_priv))
>>>    		return false;
>>>    
>>>    	/*
>>>    	 * HDMI 12bpc affects the clocks, so it's only possible
>>>    	 * when not cloning with other encoder types.
>>>    	 */
>>> -	return crtc_state->output_types == 1 << INTEL_OUTPUT_HDMI;
>>> +	if (crtc_state->output_types != 1 << INTEL_OUTPUT_HDMI)
>>> +		return false;
>>> +
>> This function is called from only one place ( for now), and that already
>> has a pipe_config->has_hdmi_sink check.
>> Does it makes sense to have only one of the checks ? I can understand
>> that this might be to comple
> has_hdmi_sink is not the same thing. It just says we're talking to at
> least one HDMI sink and thus can send infoframes/audio. output_types on
> the other hand lists all the different port types we're cloning with.
> So you can do HDMI+VGA for instance and still the HDMI goes to a real
> HDMI sink so we'll have has_hdmi_sink==true. But since VGA can't deal
> with the 1.5x clock we can't do deep color when cloning.
Makes sense :-).
>>> +	for_each_connector_in_state(state, connector, connector_state, i) {
>>> +		const struct drm_display_info *info = &connector->display_info;
>>> +
>>> +		if (connector_state->crtc != crtc_state->base.crtc)
>>> +			continue;
>>> +
>>> +		if ((info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36) == 0)
>>> +			return false;
>> Do we want to have a info->bpc check too, or they are more or less same ?
> That was already checked at the start of the state computation. So
> state->pipe_bpp already accounts for that. However as info->bpc is just
> the max bpc the sink can do it's not sufficient to guarantee it can doo
> all lower bpc values.
I guess just in this scenario, the max bpc value(12) and the value of 
our interest is same, so it was making
sense, but I agree, would not be true for all cases like max 16bpc.

Please feel free to use:
Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>

- Shashank
>
>> - Shashank
>>> +	}
>>> +
>>> +	return true;
>>>    }
>>>    
>>>    bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>>> --
>>> 2.10.2
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: FW: [PATCH] drm/i915: Reject HDMI 12bpc if the sink doesn't indicate support

[Ville Syrjälä]

On Mon, Mar 13, 2017 at 01:09:10PM +0200, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 3/13/2017 12:53 PM, Ville Syrj�l� wrote:
> > On Mon, Mar 13, 2017 at 12:22:53PM +0200, Sharma, Shashank wrote:
> >> Regards
> >>
> >> Shashank
> >>
> >>> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >>>
> >>> Check that the sink really declared 12bpc support before we enable it.
> >>> This should not actually never happen since it's mandatory for HDMI sinks to support 12bpc if they support any deep color modes. But reality disagrees with the theory and there are actually sinks in the wild that violate the spec.
> >>>
> >>> v2: Fix the output_types check
> >>>       Update commit message to state that these things are in fact real
> >>>
> >>> Cc: stable@vger.kernel.org
> >>> Cc: Nicholas Sielicki <nicholas.sielicki@gmail.com>
> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99250
> >>> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/intel_hdmi.c | 24 +++++++++++++++++++++---
> >>>    1 file changed, 21 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> >>> index a580de80d2b5..2bf5915284aa 100644
> >>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> >>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> >>> @@ -1298,16 +1298,34 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
> >>>    
> >>>    static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)  {
> >>> -	struct drm_device *dev = crtc_state->base.crtc->dev;
> >>> +	struct drm_i915_private *dev_priv =
> >>> +		to_i915(crtc_state->base.crtc->dev);
> >>> +	struct drm_atomic_state *state = crtc_state->base.state;
> >>> +	struct drm_connector_state *connector_state;
> >>> +	struct drm_connector *connector;
> >>> +	int i;
> >>>    
> >>> -	if (HAS_GMCH_DISPLAY(to_i915(dev)))
> >>> +	if (HAS_GMCH_DISPLAY(dev_priv))
> >>>    		return false;
> >>>    
> >>>    	/*
> >>>    	 * HDMI 12bpc affects the clocks, so it's only possible
> >>>    	 * when not cloning with other encoder types.
> >>>    	 */
> >>> -	return crtc_state->output_types == 1 << INTEL_OUTPUT_HDMI;
> >>> +	if (crtc_state->output_types != 1 << INTEL_OUTPUT_HDMI)
> >>> +		return false;
> >>> +
> >> This function is called from only one place ( for now), and that already
> >> has a pipe_config->has_hdmi_sink check.
> >> Does it makes sense to have only one of the checks ? I can understand
> >> that this might be to comple
> > has_hdmi_sink is not the same thing. It just says we're talking to at
> > least one HDMI sink and thus can send infoframes/audio. output_types on
> > the other hand lists all the different port types we're cloning with.
> > So you can do HDMI+VGA for instance and still the HDMI goes to a real
> > HDMI sink so we'll have has_hdmi_sink==true. But since VGA can't deal
> > with the 1.5x clock we can't do deep color when cloning.
> Makes sense :-).
> >>> +	for_each_connector_in_state(state, connector, connector_state, i) {
> >>> +		const struct drm_display_info *info = &connector->display_info;
> >>> +
> >>> +		if (connector_state->crtc != crtc_state->base.crtc)
> >>> +			continue;
> >>> +
> >>> +		if ((info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36) == 0)
> >>> +			return false;
> >> Do we want to have a info->bpc check too, or they are more or less same ?
> > That was already checked at the start of the state computation. So
> > state->pipe_bpp already accounts for that. However as info->bpc is just
> > the max bpc the sink can do it's not sufficient to guarantee it can doo
> > all lower bpc values.
> I guess just in this scenario, the max bpc value(12) and the value of 
> our interest is same, so it was making
> sense, but I agree, would not be true for all cases like max 16bpc.
> 
> Please feel free to use:
> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>

Thanks. Patch pushed to dinq.

> 
> - Shashank
> >
> >> - Shashank
> >>> +	}
> >>> +
> >>> +	return true;
> >>>    }
> >>>    
> >>>    bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> >>> --
> >>> 2.10.2
> >>>
> >>> _______________________________________________
> >>> dri-devel mailing list
> >>> dri-devel@lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrj�l�
Intel OTC

Re: FW: [PATCH] drm/i915: Reject HDMI 12bpc if the sink doesn't indicate support

[Ville Syrjälä]

On Mon, Mar 13, 2017 at 01:09:10PM +0200, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 3/13/2017 12:53 PM, Ville Syrjälä wrote:
> > On Mon, Mar 13, 2017 at 12:22:53PM +0200, Sharma, Shashank wrote:
> >> Regards
> >>
> >> Shashank
> >>
> >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>> Check that the sink really declared 12bpc support before we enable it.
> >>> This should not actually never happen since it's mandatory for HDMI sinks to support 12bpc if they support any deep color modes. But reality disagrees with the theory and there are actually sinks in the wild that violate the spec.
> >>>
> >>> v2: Fix the output_types check
> >>>       Update commit message to state that these things are in fact real
> >>>
> >>> Cc: stable@vger.kernel.org
> >>> Cc: Nicholas Sielicki <nicholas.sielicki@gmail.com>
> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99250
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/intel_hdmi.c | 24 +++++++++++++++++++++---
> >>>    1 file changed, 21 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> >>> index a580de80d2b5..2bf5915284aa 100644
> >>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> >>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> >>> @@ -1298,16 +1298,34 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
> >>>    
> >>>    static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)  {
> >>> -	struct drm_device *dev = crtc_state->base.crtc->dev;
> >>> +	struct drm_i915_private *dev_priv =
> >>> +		to_i915(crtc_state->base.crtc->dev);
> >>> +	struct drm_atomic_state *state = crtc_state->base.state;
> >>> +	struct drm_connector_state *connector_state;
> >>> +	struct drm_connector *connector;
> >>> +	int i;
> >>>    
> >>> -	if (HAS_GMCH_DISPLAY(to_i915(dev)))
> >>> +	if (HAS_GMCH_DISPLAY(dev_priv))
> >>>    		return false;
> >>>    
> >>>    	/*
> >>>    	 * HDMI 12bpc affects the clocks, so it's only possible
> >>>    	 * when not cloning with other encoder types.
> >>>    	 */
> >>> -	return crtc_state->output_types == 1 << INTEL_OUTPUT_HDMI;
> >>> +	if (crtc_state->output_types != 1 << INTEL_OUTPUT_HDMI)
> >>> +		return false;
> >>> +
> >> This function is called from only one place ( for now), and that already
> >> has a pipe_config->has_hdmi_sink check.
> >> Does it makes sense to have only one of the checks ? I can understand
> >> that this might be to comple
> > has_hdmi_sink is not the same thing. It just says we're talking to at
> > least one HDMI sink and thus can send infoframes/audio. output_types on
> > the other hand lists all the different port types we're cloning with.
> > So you can do HDMI+VGA for instance and still the HDMI goes to a real
> > HDMI sink so we'll have has_hdmi_sink==true. But since VGA can't deal
> > with the 1.5x clock we can't do deep color when cloning.
> Makes sense :-).
> >>> +	for_each_connector_in_state(state, connector, connector_state, i) {
> >>> +		const struct drm_display_info *info = &connector->display_info;
> >>> +
> >>> +		if (connector_state->crtc != crtc_state->base.crtc)
> >>> +			continue;
> >>> +
> >>> +		if ((info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36) == 0)
> >>> +			return false;
> >> Do we want to have a info->bpc check too, or they are more or less same ?
> > That was already checked at the start of the state computation. So
> > state->pipe_bpp already accounts for that. However as info->bpc is just
> > the max bpc the sink can do it's not sufficient to guarantee it can doo
> > all lower bpc values.
> I guess just in this scenario, the max bpc value(12) and the value of 
> our interest is same, so it was making
> sense, but I agree, would not be true for all cases like max 16bpc.
> 
> Please feel free to use:
> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>

Thanks. Patch pushed to dinq.

> 
> - Shashank
> >
> >> - Shashank
> >>> +	}
> >>> +
> >>> +	return true;
> >>>    }
> >>>    
> >>>    bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> >>> --
> >>> 2.10.2
> >>>
> >>> _______________________________________________
> >>> dri-devel mailing list
> >>> dri-devel@lists.freedesktop.org
> >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC