[Intel-gfx] [PATCH v2 00/14] YCBCR 4:2:0 handling in DRM layer
Sharma, Shashank
shashank.sharma at intel.com
Sat Jul 15 05:03:20 UTC 2017
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 at 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 at 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20170715/545f927d/attachment-0001.html>
More information about the Intel-gfx
mailing list