Created attachment 128711 [details] [review] Patch -- Display connector: (such as HDMI, DP, eDP, ...) HDMI Seen with: * Dell E6430 (Ivybridge i5-3340M) * Skylake NUC6I7KYK * Acer X233H 24-bit monitor with ability for 30-bit FRC. # Steps to reproduce the issue. With male-to-male HDMI cable, plug monitor into either of these devices. Output will appear, but it is corrupt. # How often does the steps listed above trigger the issue? Always, with or without X, seen on framebuffer. Does not occur when using HDMI to DVI adapter. # What's going on? With Ivybridge, HDMI can only ever operate in 8BPC or 12BPC. * ( page 78: https://01.org/sites/default/files/documentation/snb_ihd_os_vol3_part3.pdf ) With HDMI 1.3, I believe it's legal for the monitor EDID to claim a maximum BPP of 30. See the following chunk of code: > drivers/gpu/drm/i915/intel_hdmi.c:1308 > > bool intel_hdmi_compute_config(struct intel_encoder *encoder, > struct intel_crtc_state *pipe_config, > struct drm_connector_state *conn_state) > { > ... SNIP ... > > /* > * HDMI is either 12 or 8, so if the display lets 10bpc sneak > * through, clamp it down. Note that g4x/vlv don't support 12bpc hdmi > * outputs. We also need to check that the higher clock still fits > * within limits. > */ > if (pipe_config->pipe_bpp > 8*3 && pipe_config->has_hdmi_sink && > hdmi_port_clock_valid(intel_hdmi, clock_12bpc, true) == MODE_OK && > hdmi_12bpc_possible(pipe_config)) { > DRM_DEBUG_KMS("picking bpc to 12 for HDMI output\n"); > desired_bpp = 12*3; > > /* Need to adjust the port link by 1.5x for 12bpc. */ > pipe_config->port_clock = clock_12bpc; > } else { > DRM_DEBUG_KMS("picking bpc to 8 for HDMI output\n"); > desired_bpp = 8*3; > > pipe_config->port_clock = clock_8bpc; > } > } That check for (pipe_bpp > 24) is not right. It needs to check that BPP is sufficient for 12bpc (eg: >= 36) rather than check that bpp is more than sufficient for 8bpc. Attached patch 0001-Fix-BPP-clamping-on-HDMI.patch does that, and it fixes the problem with this monitor on the Ivybridge laptop. Corrupt output is no longer seen. It does not fix the problem with the NUC6I7KYK, though. I will open another bug for that and link to it in here.
Created attachment 128712 [details] EDID of monitor in question
HDMI 1.3A spec says the following > 6.2.4 Color Depth Requirements > > HDMI Sources and Sinks may support color depths of 24, 30, 36 and/or 48 bits > per pixel. All HDMI Sources and Sinks shall support 24 bits per pixel. Color > depths greater than 24 bits are defined to be “Deep Color” modes. All Deep > Color modes are optional though if an HDMI Source or Sink supports any Deep > Color mode, it shall support 36-bit mode. > > For each supported Deep Color mode, RGB 4:4:4 shall be supported and optionally > YCBCR 4:4:4 may be supported. > > YCBCR 4:2:2 is not permitted for any Deep Color mode. > > An HDMI Sink shall support all EDID-indicated Deep Color modes on all > EDID-indicated video formats except if that combination exceeds the > Max_TMDS_Clock indication. > > An HDMI Source shall not send any Deep Color mode to a Sink that does not > indicate support for that mode. The take away: it is illegal for DC_30bit to be set without DC_36bit also being set. The EDID claims the following: > CEA extension block > Extension version: 3 > 30 bytes of CEA data > <...> > Vendor-specific data block, OUI 000c03 (HDMI) > Source physical address 1.0.0.0 > DC_30bit Note the lack of DC_36bit. While i915 is technically sending DC_36bit (a deep color mode) to a sink that does not indicate support for that mode, the monitor clearly violated the spec first and in a worse way. Had the monitor not violated the spec, i915 would not have violated the spec. So I think this can be chalked up as a bad monitor operating outside the spec. I'm going to close this bug. (Thank you to everyone on #intel-gfx for the help.)
Let's not be so hasty closing this one. Real world trumps any spec, so I think we should make i915 handle this case more gracefully. I think what we want to do is explicitly check for the DC_36bit thing. I've even written that exact patch once but decided not to send it out since I'd not seen any monitor that would violate the spec in this manner.
(In reply to Ville Syrjala from comment #3) > Let's not be so hasty closing this one. Real world trumps any spec, so I > think we should make i915 handle this case more gracefully. > > I think what we want to do is explicitly check for the DC_36bit thing. I've > even written that exact patch once but decided not to send it out since I'd > not seen any monitor that would violate the spec in this manner. Any problems with this patch? It isn't specific for i915. Tested on the ivybridge laptop, does what it's supposed to. There's already a check for this EDID inconsistency in drm_edid.c, but it only results in a warning. This patch just moves that check up and returns before setting anything if the EDID is bad, resulting in 8BPC. Looking at a quick `grep -r HDMI_DC drivers/gpu/drm`, I have a hunch that if I plugged this monitor into an AMD card, that they would actually be able to successfully send it 10BPC, and this patch would make them fall back to 8BPC. So I can understand if you'd want to move this fix into i915/ instead of here. That being said, depending on how you/they interpret the spec, falling back to 8BPC when the monitor doesn't send DC_36 might technically be more correct, anyway. Thanks again for the help.
Created attachment 128761 [details] [review] Patch to disable all deep colors when DC_36bit not set
Created attachment 128762 [details] [review] Patch to disable all deep colors when DC_36bit not set (fixed) There was a problem with the way I checked the bits in the prior patch.
(In reply to Nicholas Sielicki from comment #6) > Created attachment 128762 [details] [review] [review] > Patch to disable all deep colors when DC_36bit not set (fixed) > > There was a problem with the way I checked the bits in the prior patch. The patch looks pretty good. Care to send to dri-devel@lists.freedesktop.org ? Oh and while looking at the code I also noticed that we don't seem to reset edid_hdmi_dc_modes back to zero at the start of drm_add_display_info(). Wanna cook up another patch to fix that up as well?
Created attachment 128785 [details] [review] drm: disable deep color when EDID violates spec > The patch looks pretty good. Care to send to dri-devel@lists.freedesktop.org ? I will do that. > Oh and while looking at the code I also noticed that we don't seem to reset > edid_hdmi_dc_modes back to zero at the start of drm_add_display_info(). Wanna > cook up another patch to fix that up as well? Attached is another patch with an edited commit message, the fix you mention, and some cleanup. Thanks again for the help.
Ping. I'm wondering if you've had a chance to try out my alternate fix: https://patchwork.freedesktop.org/patch/138653/
commit c750bdd3e7e204cc88b32806c3864487a03cd84b Author: Ville Syrjälä <ville.syrjala@linux.intel.com> Date: Mon Feb 13 19:58:18 2017 +0200 drm/i915: Reject HDMI 12bpc if the sink doesn't indicate support
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.