On Tue, Jan 10, 2017 at 3:02 PM, Ernst Sjöstrand <ernstp@xxxxxxxxx> 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@xxxxxxxxx>: >> >> On Tue, Jan 10, 2017 at 12:46 PM, Ville Syrjälä >> <ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxx> >> >> 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@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 >> >> > >> >> > >> >> > >> >> > _______________________________________________ >> >> > 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
