Hi Ryan,
sorry for very late review, but here we go...
Dne nedelja, 16. februar 2025 ob 19:31:59 Srednjeevropski standardni čas je Ryan Walklin napisal(a):
> Hi All,
>
> v7 of this patch adding support for the Allwinner DE33 display engine, used in the H616 family of SoCs. Apologies for the short interval between versions but a compile error due to a missed helper function in patch 11 snuck by me. v7 only differs from v6 in adding this back.
>
> v6 includes some small fixes to the device tree documentation, improves naming of an enum type, moves colorspace configuration from the sunxi engine object to the mixer object, and a handful of very small style and whitespace changes. All comments/tags from previous versions addressed. No functional change from v5.
>
> A v1 patch to enable LCD output for the Anbernic RGnnXX family of devices which use this SoC with an RGB LCD will be submitted shortly.
>
> Thanks to those who have reviewed and tested previous versions, and to Jernej for the initial patch.
>
> Original blurb below:
>
> There is existing mainline support for the DE2 and DE3 AllWinner display pipeline IP blocks, used in the A64 and H6 among others, however the H700 (as well as the H616/H618 and the T507 automotive SoC) have a newer version of the Display Engine (v3.3/DE33) which adds additional high-resolution support as well as YUV colour formats and AFBC compression support.
>
> This patch set adds DE33 support, following up from the previous RFC [1], with significant rework to break down the previous relatively complex set into more logical steps, detailed below.
>
> 1. Refactor the existing DE2/DE3 code in readiness to support YUV colour formats in the DE3 engine (patches 1-4).
> 2. Add YUV420 colour format support in the DE3 driver (patches 5-13).
> 3. Replace the is_de3 mixer flag with an enum to support multiple DE versions (patch 14).
> 4. Refactor the mixer, vi_scaler and some register code to merge common init code and more easily support multiple DE versions (patches 15-18).
> 5. Add Arm Frame Buffer Compression (AFBC) compressed buffer support to the DE3 driver. This is currently only supported for VI layers (for HW-decoded video output) but is well integrated into these changes and a subsequent patchset to enable the Video Engine is planned. (patch 19).
> 6. Add DT bindings for the DE33 engine. (patches 20-22).
> 7. Extend the DE2/3 driver for the DE33, comprising clock, mixer, vi_scaler, fmt and csc module support (patches 23-27).
This patchset actually introduces 3 disticnt features, which should IMO be separated
and thus making reviewing patches easier.
1. native 10-bit YUV420 output (without YUV->RGB->YUV conversions) - this is used on
HDMI for proper 10-bit 4K@60 HDR output
2. Display Engine 3.3 (DE33) support
3. AFBC modifier/format support (used for more efficient GPU or VPU output rendering)
If I'm not mistaken, your goal is only DE33 support. There is are two reasons why
I've never sent these patches myself:
1. scaling YUV images doesn't work
Not a problem for any user who doesn't use video player with DRM plane rendering,
which I assume is most of them. We can get around of this issue to deny scaling
YUV buffers until the issue is sorted out.
2. plane allocations are hackish
DE33 for the first time introduced option to move planes between the mixers. DRM
has also support for this, but for time being static allocation is acceptable and tbh,
even dynamic support need appropriate initial setting.
As you might guessed this is done in DE33 clock driver using magic values. Proper
way would be to introduce some kind of connection between mixer/plane and clock
driver, so initial configuration can be moved to appropriate module instead of
being thrown into clock driver. I need to check where to put it and how to properly
connect DT nodes. Maybe syscon phandle? I'll do some research.
There is one glaring issue (when you work on driver and test every aspect of it).
DE33 introduced RCQ, which is basically DMA, which copies shadowed registers
from memory buffer directly to hw registers. IIRC it does that at vblank time. This
tells you that current concept of writing register values at any time userspace feels
to do commit is wrong and we've been lucky that it works as well as it does. So,
in order to fix this, driver would need quite a big reorganization. Introducing
shadow buffers would solve most of the issues, most likely also with YUV scaling.
Implementing RCQ would be then relatively simple and even improve timings.
Note that even DE3 has occasional issue with YUV scaler and also read-modify-read
access to some of its register can produce bogus value and thus corrupt image
until next commit.
This is not to say that these issues must be solved with this effort, I'm just stating
them to make people aware.
Best regards,
Jernej
>
> Further patchsets are planned to support HDMI and the LCD timing controller present in these SoCs.
>
> Regards,
>
> Ryan
>
> Jernej Skrabec (21):
> drm: sun4i: de2/de3: Change CSC argument
> drm: sun4i: de2/de3: Merge CSC functions into one
> drm: sun4i: de2/de3: call csc setup also for UI layer
> drm: sun4i: de2: Initialize layer fields earlier
> drm: sun4i: de3: Add YUV formatter module
> drm: sun4i: de3: add format enumeration function to engine
> drm: sun4i: de3: add formatter flag to mixer config
> drm: sun4i: de3: add YUV support to the DE3 mixer
> drm: sun4i: de3: pass mixer reference to ccsc setup function
> drm: sun4i: de3: add YUV support to the color space correction module
> drm: sun4i: de3: add YUV support to the TCON
> drm: sun4i: support YUV formats in VI scaler
> drm: sun4i: de2/de3: add mixer version enum
> drm: sun4i: de2/de3: refactor mixer initialisation
> drm: sun4i: vi_scaler refactor vi_scaler enablement
> drm: sun4i: de2/de3: add generic blender register reference function
> drm: sun4i: de2/de3: use generic register reference function for layer
> configuration
> drm: sun4i: de3: Implement AFBC support
> drm: sun4i: de33: mixer: add Display Engine 3.3 (DE33) support
> drm: sun4i: de33: vi_scaler: add Display Engine 3.3 (DE33) support
> drm: sun4i: de33: fmt: add Display Engine 3.3 (DE33) support
>
> Ryan Walklin (6):
> drm: sun4i: de3: refactor YUV formatter module setup
> dt-bindings: allwinner: add H616 DE33 bus binding
> dt-bindings: allwinner: add H616 DE33 clock binding
> dt-bindings: allwinner: add H616 DE33 mixer binding
> clk: sunxi-ng: ccu: add Display Engine 3.3 (DE33) support
> drm: sun4i: de33: csc: add Display Engine 3.3 (DE33) support
>
> .../bus/allwinner,sun50i-a64-de2.yaml | 7 +-
> .../clock/allwinner,sun8i-a83t-de2-clk.yaml | 1 +
> .../allwinner,sun8i-a83t-de2-mixer.yaml | 21 +-
> drivers/clk/sunxi-ng/ccu-sun8i-de2.c | 25 ++
> drivers/gpu/drm/sun4i/Makefile | 3 +-
> drivers/gpu/drm/sun4i/sun4i_tcon.c | 28 +-
> drivers/gpu/drm/sun4i/sun50i_afbc.c | 250 +++++++++++++
> drivers/gpu/drm/sun4i/sun50i_afbc.h | 87 +++++
> drivers/gpu/drm/sun4i/sun50i_fmt.c | 100 +++++
> drivers/gpu/drm/sun4i/sun50i_fmt.h | 32 ++
> drivers/gpu/drm/sun4i/sun8i_csc.c | 345 +++++++++++++++---
> drivers/gpu/drm/sun4i/sun8i_csc.h | 20 +-
> drivers/gpu/drm/sun4i/sun8i_mixer.c | 226 +++++++++---
> drivers/gpu/drm/sun4i/sun8i_mixer.h | 53 ++-
> drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 41 ++-
> drivers/gpu/drm/sun4i/sun8i_ui_scaler.c | 2 +-
> drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 133 +++++--
> drivers/gpu/drm/sun4i/sun8i_vi_scaler.c | 115 ++++--
> drivers/gpu/drm/sun4i/sun8i_vi_scaler.h | 2 +-
> drivers/gpu/drm/sun4i/sunxi_engine.h | 29 ++
> 20 files changed, 1306 insertions(+), 214 deletions(-)
> create mode 100644 drivers/gpu/drm/sun4i/sun50i_afbc.c
> create mode 100644 drivers/gpu/drm/sun4i/sun50i_afbc.h
> create mode 100644 drivers/gpu/drm/sun4i/sun50i_fmt.c
> create mode 100644 drivers/gpu/drm/sun4i/sun50i_fmt.h
>
>
