On Wed, Jan 11, 2017 at 12:04:09PM +0200, Jani Nikula wrote: > On Tue, 10 Jan 2017, Harry Wentland <harry.wentland@xxxxxxx> 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@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. > >>> > >> > >> 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@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 > >>> > > _______________________________________________ > > dri-devel mailing list > > dri-devel@xxxxxxxxxxxxxxxxxxxxx > > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
