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

["Sharma, Shashank" <shashank.sharma@xxxxxxxxx>]

Regards

Shashank

On 7/15/2017 12:32 AM, Ville Syrjälä wrote:

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.

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:

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

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.

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.

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 :( !

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/

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.

  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@xxxxxxxxxxxxxxx>
onto the patches.

Thanks !
- Shashank

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

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel