| Summary: | [SKL,IVB] HDMI and DP: EDID-supplied BPP is not always properly clamped. | ||
|---|---|---|---|
| Product: | DRI | Reporter: | Nicholas Sielicki <sielicki> |
| Component: | DRM/Intel | Assignee: | Intel GFX Bugs mailing list <intel-gfx-bugs> |
| Status: | CLOSED FIXED | QA Contact: | Intel GFX Bugs mailing list <intel-gfx-bugs> |
| Severity: | normal | ||
| Priority: | medium | CC: | intel-gfx-bugs |
| Version: | DRI git | ||
| Hardware: | All | ||
| OS: | Linux (All) | ||
| Whiteboard: | |||
| i915 platform: | IVB, SKL | i915 features: | display/HDMI |
| Attachments: | |||
|
Description
Nicholas Sielicki
2017-01-02 21:09:57 UTC
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.