<html> <head> <meta content="text/html; charset=windows-1252" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> Regards Shashank <div class="moz-cite-prefix">On 7/15/2017 12:32 AM, Ville Syrjälä wrote: </div> <blockquote cite="mid:20170714190222.GR12629@intel.com" type="cite"> <pre wrap="">On Thu, Jul 13, 2017 at 09:03:06PM +0530, Shashank Sharma wrote: </pre> <blockquote type="cite"> <pre wrap="">Following YCBCR 4:4:4 and 4:2:2, YCBCR 4:2:0 is a new output format, which is currently supported on HDMI 2.0 sources/sinks. Due to lower chroma sub-sampling rate, YCBCR 4:2:0 can drive the video modes at half the pixel clock than YCBCR 4:4:4 or RGB 8:8:8 outputs. For example, a CEA 4K@60, RGB 8:8:8 mode needs a clock of appx 594Mhz, but it can be driven at 297Mhz using YCBCR 4:2:0 output. Of course, the lower rate of chroma subsampling, causes the quality of YCBCR 4:2:0 to be lower than YCBCR 4:4:4 or RGB 8:8:8. This patch series adds support for YCBCR 4:2:0 output in DRM layer. - First 2 patches, complete the CEA mode-db in drm driver, by adding new 4k modes. Current CEA mode-db contains 64 modes only (VIC 1-64), whereas CEA-861-F defined VICs up to 107, including 4k modes, from VIC range 93-107. First patch makes sure that inclusion of these modes doesn't break existing HDMI 1.4 monitors, across various drivers. - Next 5 patches focus on parsing new YCBCR 4:2:0 EDID blocks, and adding YCBCR 4:2:0 modes in connector. They also contain a prune function, which will cleanup the YCBCR 4:2:0 modes from list, if the connector doesn't declare them supported. - Next 2 patches add helper functions for identifing YCBCR 4:2:0 modes and setup the color space in AVI infoframes. - Next 6 patches add code for I915 layer handling of YCBCR 4:2:0 output. This patch series was initially published as a complete framework to handle all YCBCR outputs (4:4:4, 4:2:2, 4:2:0), but based on the code reviews, now its been published as YCBCR 4:2:0 handling series only. The previous discussion and reviews can be found here: V5: <a class="moz-txt-link-freetext" href="https://patchwork.freedesktop.org/series/26815/">https://patchwork.freedesktop.org/series/26815/</a> V1-V4: <a class="moz-txt-link-freetext" href="https://patchwork.freedesktop.org/series/22683/">https://patchwork.freedesktop.org/series/22683/</a> Now re-publishing this patch series as YCBCR 4:2:0 handling series here. V2: Addressed review comments from Ville This series dropped one patch in V2(patch 8 from V1), so 14 patches in V2 This series has been tested with drm-tip code with following setup: Source: Intel Geminilake device. Sink: ACER S277HK HDMI 2.0 monitor. Protocol testing: Astro VA-1844A HDMI analyzer. Shashank Sharma (14): drm: handle HDMI 2.0 VICs in AVI info-frames drm/edid: complete CEA modedb(VIC 1-107) drm/edid: parse sink information before CEA blocks drm/edid: cleanup patch for CEA extended-tag macro drm: add helper to validate YCBCR420 modes drm/edid: parse YCBCR420 videomodes from EDID drm/edid: parse ycbcr 420 deep color information drm: add helper functions for YCBCR420 handling </pre> </blockquote> <pre wrap=""> I just finished pushing the core bits into drm-misc-next. But I'm not super happy how that went because there were still quite a few trivial checkpatch warnings and indentation issues I had to fix up. All of that should have been dealt with before even submitting the patches to the list. Review bandwidth is a scarce resource so we don't want to squander it on this sort of stuff. </pre> </blockquote> The whole patch series was checked with checkpatch every time it was sent for review. It had only 1 checkpatch warning, for which I had added an explanation in patch's header like: For patch 1: <pre class="content" style="box-sizing: border-box; overflow: auto; font-family: Menlo, Monaco, Consolas, "Courier New", monospace; font-size: 13px; display: block; padding: 9.5px; margin: 0px 0px 10px; line-height: 14.3px; color: rgb(51, 51, 51); word-break: break-all; word-wrap: break-word; background-color: white; border: 0px; border-radius: 0px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration-style: initial; text-decoration-color: initial;">PS: This patch touches a few lines in few files, which were already above 80 char, so checkpatch gives 80 char warning again. - gpu/drm/omapdrm/omap_encoder.c - gpu/drm/i915/intel_sdvo.c </pre> Apart from this, here is the checkpatch output: ../scripts/checkpatch.pl *.patch --------------------------------------------------------- v8-0001-drm-handle-HDMI-2.0-VICs-in-AVI-info-frames.patch --------------------------------------------------------- WARNING: line over 80 characters #278: FILE: drivers/gpu/drm/i915/intel_sdvo.c:999: + &pipe_config->base.adjusted_mode, WARNING: line over 80 characters #332: FILE: drivers/gpu/drm/omapdrm/omap_encoder.c:88: + r = drm_hdmi_avi_infoframe_from_display_mode(&avi, adjusted_mode, total: 0 errors, 2 warnings, 247 lines checked v8-0001-drm-handle-HDMI-2.0-VICs-in-AVI-info-frames.patch has style problems, please review. ---------------------------------------------------- v8-0002-drm-edid-complete-CEA-modedb-VIC-1-107.patch ---------------------------------------------------- total: 0 errors, 0 warnings, 245 lines checked v8-0002-drm-edid-complete-CEA-modedb-VIC-1-107.patch has no obvious style problems and is ready for submission. --------------------------------------------------------------- v8-0003-drm-edid-parse-sink-information-before-CEA-blocks.patch --------------------------------------------------------------- total: 0 errors, 0 warnings, 21 lines checked v8-0003-drm-edid-parse-sink-information-before-CEA-blocks.patch has no obvious style problems and is ready for submission. --------------------------------------------------------------- v8-0004-drm-edid-cleanup-patch-for-CEA-extended-tag-macro.patch --------------------------------------------------------------- total: 0 errors, 0 warnings, 33 lines checked v8-0004-drm-edid-cleanup-patch-for-CEA-extended-tag-macro.patch has no obvious style problems and is ready for submission. ------------------------------------------------------- v8-0005-drm-add-helper-to-validate-YCBCR420-modes.patch ------------------------------------------------------- total: 0 errors, 0 warnings, 110 lines checked v8-0005-drm-add-helper-to-validate-YCBCR420-modes.patch has no obvious style problems and is ready for submission. ---------------------------------------------------------- v8-0006-drm-edid-parse-YCBCR420-videomodes-from-EDID.patch ---------------------------------------------------------- total: 0 errors, 0 warnings, 228 lines checked v8-0006-drm-edid-parse-YCBCR420-videomodes-from-EDID.patch has no obvious style problems and is ready for submission. ------------------------------------------------------------- v8-0007-drm-edid-parse-ycbcr-420-deep-color-information.patch ------------------------------------------------------------- total: 0 errors, 0 warnings, 47 lines checked v8-0007-drm-edid-parse-ycbcr-420-deep-color-information.patch has no obvious style problems and is ready for submission. ------------------------------------------------------------ v8-0008-drm-add-helper-functions-for-YCBCR420-handling.patch ------------------------------------------------------------ total: 0 errors, 0 warnings, 73 lines checked v8-0008-drm-add-helper-functions-for-YCBCR420-handling.patch has no obvious style problems and is ready for submission. --------------------------------------------------------------- v8-0009-drm-i915-add-config-function-for-YCBCR420-outputs.patch --------------------------------------------------------------- total: 0 errors, 0 warnings, 89 lines checked v8-0009-drm-i915-add-config-function-for-YCBCR420-outputs.patch has no obvious style problems and is ready for submission. ---------------------------------------------------------- v8-0010-drm-i915-prepare-scaler-for-YCBCR420-modeset.patch ---------------------------------------------------------- total: 0 errors, 0 warnings, 58 lines checked v8-0010-drm-i915-prepare-scaler-for-YCBCR420-modeset.patch has no obvious style problems and is ready for submission. ------------------------------------------------------- v8-0011-drm-i915-prepare-pipe-for-YCBCR420-output.patch ------------------------------------------------------- total: 0 errors, 0 warnings, 28 lines checked v8-0011-drm-i915-prepare-pipe-for-YCBCR420-output.patch has no obvious style problems and is ready for submission. ----------------------------------------------------------- v8-0012-drm-i915-prepare-csc-unit-for-YCBCR420-output.patch ----------------------------------------------------------- total: 0 errors, 0 warnings, 85 lines checked v8-0012-drm-i915-prepare-csc-unit-for-YCBCR420-output.patch has no obvious style problems and is ready for submission. ---------------------------------------------------------- v8-0013-drm-i915-set-colorspace-for-YCBCR420-outputs.patch ---------------------------------------------------------- total: 0 errors, 0 warnings, 18 lines checked v8-0013-drm-i915-set-colorspace-for-YCBCR420-outputs.patch has no obvious style problems and is ready for submission. -------------------------------------------------- v8-0014-drm-i915-glk-set-HDMI-2.0-identifier.patch -------------------------------------------------- total: 0 errors, 0 warnings, 9 lines checked v8-0014-drm-i915-glk-set-HDMI-2.0-identifier.patch has no obvious style problems and is ready for submission. <blockquote cite="mid:20170714190222.GR12629@intel.com" type="cite"> <pre wrap=""> Another issue I ran into when I tried to actually run the code. As soon as I loaded the driver a warning and the accompanying backtrace from the state checker flew past my terminal. That too should have been caught before even sending the patches to the list. </pre> </blockquote> Yes, I had seen that, and I knew why is that happening, but I needed a discussion on what is the right way to fix it. It was about readouts of the state and pixel clock mismatch (as YCBCR420 clock = half). But at the same time, I am at fault, I should have mentioned this somewhere in the cover letter or I should have mentioned it to you on IRC. Sorry about that, My bad :( ! <blockquote cite="mid:20170714190222.GR12629@intel.com" type="cite"> <pre wrap=""> So I think the next time someone asks me to review something I will start by asking these basic questions: - Did you configure your editor so that it automagically formats code correctly? - Did you check for new compiler/sparse/checkpatch warnings? - Did you check for new runtime errors/warnings from the driver? If the answer to any of those is "no", then I won't even bother reviewing anything. OK, that's enough ranting. Now for the more positive feedback. The code does work for me, for the mose part. There is some kind of initial problem though. When I first load the driver I get some kind of weird shifted grey screen. So I guess either we're misconfiguring something in the source, or we're not properly telling the sink what we're sending it. After another modeset the problem goes away and everything appears to work correctly. So congrats on making that happen \o/</pre> </blockquote> Thanks, for the detailed review, and the time you invested on this. As such I had tested this whole series with this combination: - GLK system & KBL NUC system - 2 different HDMI 2.0 monitors ACER/Samsung - 1 HDMI 1.4 monitor (Dell) - 1 HDMI analyzer (Astro VA1844) - Mint desktop GUI - IGT (testdisplay) and I dint see any greying issue even once, but May be I will try to test with the way you are testing. <blockquote cite="mid:20170714190222.GR12629@intel.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap=""> drm/i915: add config function for YCBCR420 outputs drm/i915: prepare scaler for YCBCR420 modeset drm/i915: prepare pipe for YCBCR420 output drm/i915: prepare csc unit for YCBCR420 output drm/i915: set colorspace for YCBCR420 outputs drm/i915/glk: set HDMI 2.0 identifier </pre> </blockquote> <pre wrap=""> These remaining i915 patches still have a few issues, the lack of state readout and the grey screen issue are the major ones. I've left more detailed feedback on the individual patches, but right now I don't have a clear idea what's causing the grey screen. Assuming the remaining issues I've highlighted get fixed, and the remaining patches won't get massively altered in the process, you can slap Reviewed-by: Ville Syrjälä <a class="moz-txt-link-rfc2396E" href="mailto:ville.syrjala@linux.intel.com"><ville.syrjala@linux.intel.com></a> onto the patches.</pre> </blockquote> Thanks ! - Shashank <blockquote cite="mid:20170714190222.GR12629@intel.com" type="cite"> <pre wrap=""> </pre> <blockquote type="cite"> <pre wrap=""> drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 2 +- drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 2 +- drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 2 +- drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 +- drivers/gpu/drm/bridge/analogix-anx78xx.c | 3 +- drivers/gpu/drm/bridge/sii902x.c | 2 +- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 +- drivers/gpu/drm/drm_edid.c | 438 +++++++++++++++++++++++++++++- drivers/gpu/drm/drm_modes.c | 87 ++++++ drivers/gpu/drm/drm_probe_helper.c | 4 + drivers/gpu/drm/exynos/exynos_hdmi.c | 2 +- drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- drivers/gpu/drm/i915/i915_reg.h | 3 + drivers/gpu/drm/i915/intel_color.c | 46 +++- drivers/gpu/drm/i915/intel_display.c | 26 ++ drivers/gpu/drm/i915/intel_drv.h | 7 +- drivers/gpu/drm/i915/intel_hdmi.c | 69 ++++- drivers/gpu/drm/i915/intel_panel.c | 3 +- drivers/gpu/drm/i915/intel_sdvo.c | 3 +- drivers/gpu/drm/mediatek/mtk_hdmi.c | 2 +- drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 2 +- drivers/gpu/drm/nouveau/nv50_display.c | 3 +- drivers/gpu/drm/omapdrm/omap_encoder.c | 3 +- drivers/gpu/drm/radeon/radeon_audio.c | 2 +- drivers/gpu/drm/rockchip/inno_hdmi.c | 2 +- drivers/gpu/drm/sti/sti_hdmi.c | 2 +- drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 2 +- drivers/gpu/drm/tegra/hdmi.c | 2 +- drivers/gpu/drm/tegra/sor.c | 2 +- drivers/gpu/drm/vc4/vc4_hdmi.c | 2 +- drivers/gpu/drm/zte/zx_hdmi.c | 2 +- include/drm/drm_connector.h | 32 +++ include/drm/drm_edid.h | 11 +- include/drm/drm_modes.h | 11 + 34 files changed, 746 insertions(+), 39 deletions(-) -- 2.7.4 _______________________________________________ dri-devel mailing list <a class="moz-txt-link-abbreviated" href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a> <a class="moz-txt-link-freetext" href="https://lists.freedesktop.org/mailman/listinfo/dri-devel">https://lists.freedesktop.org/mailman/listinfo/dri-devel</a> </pre> </blockquote> <pre wrap=""> </pre> </blockquote> </body> </html>