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@xxxxxxxxxxxxxxx>: > > > On Thu, Jan 05, 2017 at 05:45:23PM -0600, Nicholas Sielicki wrote: > > > As per the HDMI 1.3 and 1.4 spec, "deep color modes" are color depths > > > greater than 24 bits per pixel. The spec explicitly states, "All Deep > > > Color modes are optional though if an HDMI Source or Sink supports any > > > Deep Color mode, it shall support 36-bit mode." (6.2.4 Color Depth > > > Requirements). > > > > > > I came across a monitor (Acer X233H) that supplies an illegal EDID where > > > DC_30bit is set and DC_36bit is not set. The end result is badly garbled > > > output because the driver is sending 36BPP when the monitor can't handle > > > it. > > > > > > Much of the intel hardware is incapable of operating at any > > > bit-per-component setting outside of 8 or 12, and the spec seems to > > > imply that if any deep color support is found, then it is a safe > > > assumption to operate at 12. > > > > > > This patch ensures that the EDID is within the spec (specifically, that > > > DC_36bit is set) before committing to going forward with any deep > > > colors. There was already a check for this EDID inconsistency, but it > > > resulted only in a warning and did not fall-back to safer settings. > > > > > > CC: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > Signed-off-by: Nicholas Sielicki <nicholas.sielicki@xxxxxxxxx> > > > --- > > > drivers/gpu/drm/drm_edid.c | 35 +++++++++++++++++++++++------------ > > > 1 file changed, 23 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > > > index 336be31ff3de..42ce3f54d2dc 100644 > > > --- a/drivers/gpu/drm/drm_edid.c > > > +++ b/drivers/gpu/drm/drm_edid.c > > > @@ -3772,30 +3772,34 @@ static void drm_parse_hdmi_deep_color_info(struct > > drm_connector *connector, > > > { > > > struct drm_display_info *info = &connector->display_info; > > > unsigned int dc_bpc = 0; > > > + u8 modes = 0; > > > > > > /* HDMI supports at least 8 bpc */ > > > info->bpc = 8; > > > > > > + /* Ensure all DC modes are unset if we return early */ > > > + info->edid_hdmi_dc_modes = 0; > > > > Clearing this in drm_add_display_info() should be sufficient since > > this guy doesn't get called from anywhere else. So this part could > > be droppped. > > > > Otherwise this feels like a decent way to handle the problem. We > > could of course try to use the 10bpc (or whatever) deep color modes > > the sink claims to support, but given that the people designing the > > thing didn't bother reading the spec, it seems safer to just disable > > deep color support entirely. > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > > + > > > if (cea_db_payload_len(hdmi) < 6) > > > return; > > > > > > if (hdmi[6] & DRM_EDID_HDMI_DC_30) { > > > dc_bpc = 10; > > > - info->edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_30; > > > + modes |= DRM_EDID_HDMI_DC_30; > > > DRM_DEBUG("%s: HDMI sink does deep color 30.\n", > > > connector->name); > > > } > > > > > > if (hdmi[6] & DRM_EDID_HDMI_DC_36) { > > > dc_bpc = 12; > > > - info->edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_36; > > > + modes |= DRM_EDID_HDMI_DC_36; > > > DRM_DEBUG("%s: HDMI sink does deep color 36.\n", > > > connector->name); > > > } > > > > > > if (hdmi[6] & DRM_EDID_HDMI_DC_48) { > > > dc_bpc = 16; > > > - info->edid_hdmi_dc_modes |= DRM_EDID_HDMI_DC_48; > > > + modes |= DRM_EDID_HDMI_DC_48; > > > DRM_DEBUG("%s: HDMI sink does deep color 48.\n", > > > connector->name); > > > } > > > @@ -3806,9 +3810,24 @@ static void drm_parse_hdmi_deep_color_info(struct > > drm_connector *connector, > > > return; > > > } > > > > > > + /* > > > + * All deep color modes are optional, but if a sink supports any > > deep > > > + * color mode, it must support 36-bit mode. If this is found not > > > + * to be the case, sink is in violation of HDMI 1.3 / 1.4 spec and > > it > > > + * is prudent to disable all deep color modes. Return here before > > > + * committing bpc and edid_hdmi_dc_modes. > > > + */ > > > + if (!(modes & DRM_EDID_HDMI_DC_36)) { > > > + DRM_DEBUG("%s: HDMI sink should do DC_36, but does not!\n", > > > + connector->name); > > > + return; > > > + } > > > + > > > + > > > DRM_DEBUG("%s: Assigning HDMI sink color depth as %d bpc.\n", > > > connector->name, dc_bpc); > > > info->bpc = dc_bpc; > > > + info->edid_hdmi_dc_modes = modes; > > > > > > /* > > > * Deep color support mandates RGB444 support for all video > > > @@ -3823,15 +3842,6 @@ static void drm_parse_hdmi_deep_color_info(struct > > drm_connector *connector, > > > DRM_DEBUG("%s: HDMI sink does YCRCB444 in deep color.\n", > > > connector->name); > > > } > > > - > > > - /* > > > - * Spec says that if any deep color mode is supported at all, > > > - * then deep color 36 bit must be supported. > > > - */ > > > - if (!(hdmi[6] & DRM_EDID_HDMI_DC_36)) { > > > - DRM_DEBUG("%s: HDMI sink should do DC_36, but does not!\n", > > > - connector->name); > > > - } > > > } > > > > > > static void > > > @@ -3895,6 +3905,7 @@ static void drm_add_display_info(struct > > drm_connector *connector, > > > /* driver figures it out in this case */ > > > info->bpc = 0; > > > info->color_formats = 0; > > > + info->edid_hdmi_dc_modes = 0; > > > info->cea_rev = 0; > > > info->max_tmds_clock = 0; > > > info->dvi_dual = false; > > > -- > > > 2.11.0 > > > > -- > > Ville Syrjälä > > Intel OTC > > _______________________________________________ > > dri-devel mailing list > > dri-devel@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
