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

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

[-- Attachment #1.1: Type: text/plain, Size: 13777 bytes --]

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


[-- Attachment #1.2: Type: text/html, Size: 16735 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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