Re: [PATCH v2 00/14] YCBCR 4:2:0 handling in DRM layer

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Jul 13, 2017 at 09:03:06PM +0530, Shashank Sharma wrote:
> 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: https://patchwork.freedesktop.org/series/26815/
>     V1-V4: https://patchwork.freedesktop.org/series/22683/
> 
> 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

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.

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.

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/

>   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

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ä <ville.syrjala@linux.intel.com>
onto the patches.

> 
>  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
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel