[PATCH v7 00/27] drm: sun4i: add Display Engine 3.3 (DE33) support

[PATCH v7 00/27] drm: sun4i: add Display Engine 3.3 (DE33) support

[Ryan Walklin]

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).

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

-- 
2.48.1

[PATCH v7 01/27] drm: sun4i: de2/de3: Change CSC argument

[Ryan Walklin]

From: Jernej Skrabec <jernej.skrabec@gmail.com>

Currently, CSC module takes care only for converting YUV to RGB.
However, DE3 is more suited to work in YUV color space. Change CSC mode
argument to format type to be more neutral. New argument only tells
layer format type and doesn't imply output type.

This commit doesn't make any functional change.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
Signed-off-by: Ryan Walklin <ryan@testtoast.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>

--
Changelog v5..v6:
- Rename format enum from format_type to sun8i_format_type
---
 drivers/gpu/drm/sun4i/sun8i_csc.c      | 22 +++++++++++-----------
 drivers/gpu/drm/sun4i/sun8i_csc.h      | 10 +++++-----
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 16 ++++++++--------
 3 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun8i_csc.c b/drivers/gpu/drm/sun4i/sun8i_csc.c
index 58480d8e4f704..a96de701c3304 100644
--- a/drivers/gpu/drm/sun4i/sun8i_csc.c
+++ b/drivers/gpu/drm/sun4i/sun8i_csc.c
@@ -108,7 +108,7 @@ static const u32 yuv2rgb_de3[2][3][12] = {
 };
 
 static void sun8i_csc_set_coefficients(struct regmap *map, u32 base,
-				       enum sun8i_csc_mode mode,
+				       enum sun8i_format_type fmt_type,
 				       enum drm_color_encoding encoding,
 				       enum drm_color_range range)
 {
@@ -118,12 +118,12 @@ static void sun8i_csc_set_coefficients(struct regmap *map, u32 base,
 
 	table = yuv2rgb[range][encoding];
 
-	switch (mode) {
-	case SUN8I_CSC_MODE_YUV2RGB:
+	switch (fmt_type) {
+	case FORMAT_TYPE_YUV:
 		base_reg = SUN8I_CSC_COEFF(base, 0);
 		regmap_bulk_write(map, base_reg, table, 12);
 		break;
-	case SUN8I_CSC_MODE_YVU2RGB:
+	case FORMAT_TYPE_YVU:
 		for (i = 0; i < 12; i++) {
 			if ((i & 3) == 1)
 				base_reg = SUN8I_CSC_COEFF(base, i + 1);
@@ -141,7 +141,7 @@ static void sun8i_csc_set_coefficients(struct regmap *map, u32 base,
 }
 
 static void sun8i_de3_ccsc_set_coefficients(struct regmap *map, int layer,
-					    enum sun8i_csc_mode mode,
+					    enum sun8i_format_type fmt_type,
 					    enum drm_color_encoding encoding,
 					    enum drm_color_range range)
 {
@@ -151,12 +151,12 @@ static void sun8i_de3_ccsc_set_coefficients(struct regmap *map, int layer,
 
 	table = yuv2rgb_de3[range][encoding];
 
-	switch (mode) {
-	case SUN8I_CSC_MODE_YUV2RGB:
+	switch (fmt_type) {
+	case FORMAT_TYPE_YUV:
 		addr = SUN50I_MIXER_BLEND_CSC_COEFF(DE3_BLD_BASE, layer, 0);
 		regmap_bulk_write(map, addr, table, 12);
 		break;
-	case SUN8I_CSC_MODE_YVU2RGB:
+	case FORMAT_TYPE_YVU:
 		for (i = 0; i < 12; i++) {
 			if ((i & 3) == 1)
 				addr = SUN50I_MIXER_BLEND_CSC_COEFF(DE3_BLD_BASE,
@@ -206,7 +206,7 @@ static void sun8i_de3_ccsc_enable(struct regmap *map, int layer, bool enable)
 }
 
 void sun8i_csc_set_ccsc_coefficients(struct sun8i_mixer *mixer, int layer,
-				     enum sun8i_csc_mode mode,
+				     enum sun8i_format_type fmt_type,
 				     enum drm_color_encoding encoding,
 				     enum drm_color_range range)
 {
@@ -214,14 +214,14 @@ void sun8i_csc_set_ccsc_coefficients(struct sun8i_mixer *mixer, int layer,
 
 	if (mixer->cfg->is_de3) {
 		sun8i_de3_ccsc_set_coefficients(mixer->engine.regs, layer,
-						mode, encoding, range);
+						fmt_type, encoding, range);
 		return;
 	}
 
 	base = ccsc_base[mixer->cfg->ccsc][layer];
 
 	sun8i_csc_set_coefficients(mixer->engine.regs, base,
-				   mode, encoding, range);
+				   fmt_type, encoding, range);
 }
 
 void sun8i_csc_enable_ccsc(struct sun8i_mixer *mixer, int layer, bool enable)
diff --git a/drivers/gpu/drm/sun4i/sun8i_csc.h b/drivers/gpu/drm/sun4i/sun8i_csc.h
index 828b86fd0cabb..e35e0ac951022 100644
--- a/drivers/gpu/drm/sun4i/sun8i_csc.h
+++ b/drivers/gpu/drm/sun4i/sun8i_csc.h
@@ -22,14 +22,14 @@ struct sun8i_mixer;
 
 #define SUN8I_CSC_CTRL_EN		BIT(0)
 
-enum sun8i_csc_mode {
-	SUN8I_CSC_MODE_OFF,
-	SUN8I_CSC_MODE_YUV2RGB,
-	SUN8I_CSC_MODE_YVU2RGB,
+enum sun8i_format_type {
+	FORMAT_TYPE_RGB,
+	FORMAT_TYPE_YUV,
+	FORMAT_TYPE_YVU,
 };
 
 void sun8i_csc_set_ccsc_coefficients(struct sun8i_mixer *mixer, int layer,
-				     enum sun8i_csc_mode mode,
+				     enum sun8i_format_type fmt_type,
 				     enum drm_color_encoding encoding,
 				     enum drm_color_range range);
 void sun8i_csc_enable_ccsc(struct sun8i_mixer *mixer, int layer, bool enable);
diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
index 9c09d9c08496d..8a80934e928fe 100644
--- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
@@ -193,19 +193,19 @@ static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int channel,
 	return 0;
 }
 
-static u32 sun8i_vi_layer_get_csc_mode(const struct drm_format_info *format)
+static u32 sun8i_vi_layer_get_format_type(const struct drm_format_info *format)
 {
 	if (!format->is_yuv)
-		return SUN8I_CSC_MODE_OFF;
+		return FORMAT_TYPE_RGB;
 
 	switch (format->format) {
 	case DRM_FORMAT_YVU411:
 	case DRM_FORMAT_YVU420:
 	case DRM_FORMAT_YVU422:
 	case DRM_FORMAT_YVU444:
-		return SUN8I_CSC_MODE_YVU2RGB;
+		return FORMAT_TYPE_YVU;
 	default:
-		return SUN8I_CSC_MODE_YUV2RGB;
+		return FORMAT_TYPE_YUV;
 	}
 }
 
@@ -213,7 +213,7 @@ static int sun8i_vi_layer_update_formats(struct sun8i_mixer *mixer, int channel,
 					 int overlay, struct drm_plane *plane)
 {
 	struct drm_plane_state *state = plane->state;
-	u32 val, ch_base, csc_mode, hw_fmt;
+	u32 val, ch_base, fmt_type, hw_fmt;
 	const struct drm_format_info *fmt;
 	int ret;
 
@@ -231,9 +231,9 @@ static int sun8i_vi_layer_update_formats(struct sun8i_mixer *mixer, int channel,
 			   SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base, overlay),
 			   SUN8I_MIXER_CHAN_VI_LAYER_ATTR_FBFMT_MASK, val);
 
-	csc_mode = sun8i_vi_layer_get_csc_mode(fmt);
-	if (csc_mode != SUN8I_CSC_MODE_OFF) {
-		sun8i_csc_set_ccsc_coefficients(mixer, channel, csc_mode,
+	fmt_type = sun8i_vi_layer_get_format_type(fmt);
+	if (fmt_type != FORMAT_TYPE_RGB) {
+		sun8i_csc_set_ccsc_coefficients(mixer, channel, fmt_type,
 						state->color_encoding,
 						state->color_range);
 		sun8i_csc_enable_ccsc(mixer, channel, true);
-- 
2.48.1

Re: [PATCH v7 00/27] drm: sun4i: add Display Engine 3.3 (DE33) support

[Jernej Škrabec]

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
> 
> 

Re: [PATCH v7 00/27] drm: sun4i: add Display Engine 3.3 (DE33) support

[Ryan Walklin]

On Sat, 22 Feb 2025, at 7:57 AM, Jernej Škrabec wrote:
> Hi Ryan,
>
> sorry for very late review, but here we go...

No problem, thanks for the review!

> 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. 

This is my initial goal, but I would still like good HDMI and video support (including HW decode). 

It took some refactoring and resequencing of (your) existing driver work to get to this series, and I have left out the HDMI and separated the TCON code for the same reason, that initially I am intending to just support RGB operation to an LCD. I do intend however to use the HDMI code either in or out of tree but think that will be a much bigger/more complex change to mainline given the current HDMI code is more invasive to the generic driver.

The YUV and AFBC changes seemed more self-contained and seemed reasonable approaches given that they were both features of the DE3 and up. I am happy either to split these into either 4 or 5 separate patches ie:

1a. refactor CSC code to prepare for YUV
1b. add YUV for DE3
2. refactor mixer enumeration and regmaps to support DE3+
3. add DE33
4. Add AFBC

My only reluctance is that that adds more work to manage multiple patches which are logically grouped and in terms of ease of review, all these 4 steps are in the current set in that order (apart from AFBC which is completely standalone), and I don't think submitting them separately reduces complexity. It however will reduce reviewer burden as you suggest, but this has been a slow process already.

I am happy to accept either process but this has been some time already now with lots of stylistic feedback but not much on the correctness of the approach and code, and I am keen to get this landed.

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.

Good to know, I hadn't appreciated that. Mostly relevant for video as you say and it would be good to land YUV support without scaling, then extend subsequently, possibly when we get to the video engine?

> 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.

Thanks, I was not aware of this either.

> 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.

Thanks, again I wasn't aware. All my testing has shown remarkable stability and there are some downstream users out-of-tree with good feedback, but would be happy to support an effort to improve this.

Regards,

Ryan

Re: [PATCH v7 00/27] drm: sun4i: add Display Engine 3.3 (DE33) support

[Jernej Škrabec]

Dne sobota, 22. februar 2025 ob 03:48:01 Srednjeevropski standardni čas je Ryan Walklin napisal(a):
> On Sat, 22 Feb 2025, at 7:57 AM, Jernej Škrabec wrote:
> > Hi Ryan,
> >
> > sorry for very late review, but here we go...
> 
> No problem, thanks for the review!
> 
> > 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. 
> 
> This is my initial goal, but I would still like good HDMI and video support (including HW decode). 
> 
> It took some refactoring and resequencing of (your) existing driver work to get to this series, and I have left out the HDMI and separated the TCON code for the same reason, that initially I am intending to just support RGB operation to an LCD. I do intend however to use the HDMI code either in or out of tree but think that will be a much bigger/more complex change to mainline given the current HDMI code is more invasive to the generic driver.
> 
> The YUV and AFBC changes seemed more self-contained and seemed reasonable approaches given that they were both features of the DE3 and up. I am happy either to split these into either 4 or 5 separate patches ie:
> 
> 1a. refactor CSC code to prepare for YUV
> 1b. add YUV for DE3
> 2. refactor mixer enumeration and regmaps to support DE3+
> 3. add DE33
> 4. Add AFBC
> 
> My only reluctance is that that adds more work to manage multiple patches which are logically grouped and in terms of ease of review, all these 4 steps are in the current set in that order (apart from AFBC which is completely standalone), and I don't think submitting them separately reduces complexity. It however will reduce reviewer burden as you suggest, but this has been a slow process already.

Sorry, completely forgot. YUV420 HDMI code relies on my previous work, with which
Maxime wasn't happy with:

https://lore.kernel.org/linux-sunxi/20230924192604.3262187-1-jernej.skrabec@gmail.com/

So unless switching HDMI to bridge ops is implemented, which also brings format,
YUV formatter and some other patches just add unused code, which isn't ideal,
especially if we decide to rework driver before that code can be put in acceptable
state for all involved.

From quick look, patches 5-13, 26 should be dropped for now. Not sure about 1-4.

I'm fine with AFBC support going in, it's just one patch.

> 
> I am happy to accept either process but this has been some time already now with lots of stylistic feedback but not much on the correctness of the approach and code, and I am keen to get this landed.
> 
> 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.
> 
> Good to know, I hadn't appreciated that. Mostly relevant for video as you say and it would be good to land YUV support without scaling, then extend subsequently, possibly when we get to the video engine?

Correct.

Just be aware of one thing. Even if YUV buffer is rendered in 1:1 scale, vi scaler
still needs to be configured. That's because U and V planes are subsampled and
need to be scaled to Y plane size. For unknown reason, that works just fine, but
if Y scale is bigger than 1, it all falls apart.

This should be implemented in atomic check.

> 
> > 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.
> 
> Thanks, I was not aware of this either.

Here you have some pointers how this values are actually set:
https://github.com/orangepi-xunlong/linux-orangepi/blob/orange-pi-5.15-sun55iw3/bsp/drivers/video/sunxi/disp2/disp/de/lowlevel_v33x/de330/de_top.c#L232-L260

I think this is the biggest issue of this code. As soon as we solve it properly, we
have an ability to implement all remaining features at a later date.

> 
> > 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.
> 
> Thanks, again I wasn't aware. All my testing has shown remarkable stability and there are some downstream users out-of-tree with good feedback, but would be happy to support an effort to improve this.

Let's land DE33 support first since it's long overdue and then I'm happy to discuss plans for moving forward.

Best regards,
Jernej

> 
> Regards,
> 
> Ryan
> 

Re: [PATCH v7 00/27] drm: sun4i: add Display Engine 3.3 (DE33) support

[Ryan Walklin]

On Sat, 22 Feb 2025, at 10:28 PM, Jernej Škrabec wrote:
> Sorry, completely forgot. YUV420 HDMI code relies on my previous work, 
> with which
> Maxime wasn't happy with:
>
> https://lore.kernel.org/linux-sunxi/20230924192604.3262187-1-jernej.skrabec@gmail.com/
>
> So unless switching HDMI to bridge ops is implemented, which also 
> brings format,
> YUV formatter and some other patches just add unused code, which isn't 
> ideal,
> especially if we decide to rework driver before that code can be put in 
> acceptable
> state for all involved.
>
> From quick look, patches 5-13, 26 should be dropped for now. Not sure about 1-4.
>
> I'm fine with AFBC support going in, it's just one patch.

Makes sense, ok I will prepare a pure DE33 patch with AFBC and RGB support, removing the YUV code. I think the CSC changes (1-4) are good to stay as it is a change that makes sense regardless and makes the YUV work a bit easier.

Thanks for the other clarifications and vendor code links, will do a bit of reading.

Regards,

Ryan

Re: [PATCH v7 00/27] drm: sun4i: add Display Engine 3.3 (DE33) support

[Jernej Škrabec]

Hi Maxime,

I'd like to pick your brain for the issue below.

Dne sobota, 22. februar 2025 ob 10:28:51 Srednjeevropski standardni čas je Jernej Škrabec napisal(a):
> Dne sobota, 22. februar 2025 ob 03:48:01 Srednjeevropski standardni čas je Ryan Walklin napisal(a):
> > On Sat, 22 Feb 2025, at 7:57 AM, Jernej Škrabec wrote:
> > > Hi Ryan,
> > >
> > > sorry for very late review, but here we go...
> > 
> > No problem, thanks for the review!
> > 
> > > 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. 
> > 
> > This is my initial goal, but I would still like good HDMI and video support (including HW decode). 
> > 
> > It took some refactoring and resequencing of (your) existing driver work to get to this series, and I have left out the HDMI and separated the TCON code for the same reason, that initially I am intending to just support RGB operation to an LCD. I do intend however to use the HDMI code either in or out of tree but think that will be a much bigger/more complex change to mainline given the current HDMI code is more invasive to the generic driver.
> > 
> > The YUV and AFBC changes seemed more self-contained and seemed reasonable approaches given that they were both features of the DE3 and up. I am happy either to split these into either 4 or 5 separate patches ie:
> > 
> > 1a. refactor CSC code to prepare for YUV
> > 1b. add YUV for DE3
> > 2. refactor mixer enumeration and regmaps to support DE3+
> > 3. add DE33
> > 4. Add AFBC
> > 
> > My only reluctance is that that adds more work to manage multiple patches which are logically grouped and in terms of ease of review, all these 4 steps are in the current set in that order (apart from AFBC which is completely standalone), and I don't think submitting them separately reduces complexity. It however will reduce reviewer burden as you suggest, but this has been a slow process already.
> 
> Sorry, completely forgot. YUV420 HDMI code relies on my previous work, with which
> Maxime wasn't happy with:
> 
> https://lore.kernel.org/linux-sunxi/20230924192604.3262187-1-jernej.skrabec@gmail.com/
> 
> So unless switching HDMI to bridge ops is implemented, which also brings format,
> YUV formatter and some other patches just add unused code, which isn't ideal,
> especially if we decide to rework driver before that code can be put in acceptable
> state for all involved.
> 
> From quick look, patches 5-13, 26 should be dropped for now. Not sure about 1-4.
> 
> I'm fine with AFBC support going in, it's just one patch.
> 
> > 
> > I am happy to accept either process but this has been some time already now with lots of stylistic feedback but not much on the correctness of the approach and code, and I am keen to get this landed.
> > 
> > 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.
> > 
> > Good to know, I hadn't appreciated that. Mostly relevant for video as you say and it would be good to land YUV support without scaling, then extend subsequently, possibly when we get to the video engine?
> 
> Correct.
> 
> Just be aware of one thing. Even if YUV buffer is rendered in 1:1 scale, vi scaler
> still needs to be configured. That's because U and V planes are subsampled and
> need to be scaled to Y plane size. For unknown reason, that works just fine, but
> if Y scale is bigger than 1, it all falls apart.
> 
> This should be implemented in atomic check.
> 
> > 
> > > 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.
> > 
> > Thanks, I was not aware of this either.
> 
> Here you have some pointers how this values are actually set:
> https://github.com/orangepi-xunlong/linux-orangepi/blob/orange-pi-5.15-sun55iw3/bsp/drivers/video/sunxi/disp2/disp/de/lowlevel_v33x/de330/de_top.c#L232-L260
> 
> I think this is the biggest issue of this code. As soon as we solve it properly, we
> have an ability to implement all remaining features at a later date.
> 

DE33 has introduced "shared" planes, which can be allocated to any mixer. They
have now distinct memory region and are not included in mixer regions anymore
as it was the case with DE2 and DE3. This patchset maps whole planes region to
mixer0 which was a hack to get quick result and to push problem to a later time,
which is obviously now.

I see two solutions:
1. All mixers map same region for planes. Not sure if this is acceptable, but it's
    far the easiest to implement (already done).
2. Implement separate plane driver. Each mixer would have phandle to plane node
    and could call plane management functions there, like switch plane crtc. This
    to me sounds like a better solution.

What do you think?

Best regards,
Jernej

> > 
> > > 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.
> > 
> > Thanks, again I wasn't aware. All my testing has shown remarkable stability and there are some downstream users out-of-tree with good feedback, but would be happy to support an effort to improve this.
> 
> Let's land DE33 support first since it's long overdue and then I'm happy to discuss plans for moving forward.
> 
> Best regards,
> Jernej
> 
> > 
> > Regards,
> > 
> > Ryan
> > 
> 
> 
> 
> 
>