[PATCH v2 0/4] drm/vc4: Improve alpha format plane support

[Stefan Schake]

This series improves the handling of alpha formats with the VC4 HVS
compositor. Alpha formats are marked as premultiplied as is standard
for DRM. Further fix a display corruption issue when planes with
per-pixel alpha try blending from the (nonexistent) background by
selectively enabling a black background color fill.

This series follows the changes suggested by Eric Anholt in a previous
patch discussion:

https://patchwork.freedesktop.org/patch/207667/

A simple test program for the display corruption issue is available:

https://github.com/stschake/vc4-alpha-test

v2 of the series fixes the has_alpha confusion and moves needs_bg_fill
into the plane state. This unfortunately necessitated moving the plane
state to a header where it can be referenced from vc4_crtc.

Stefan Schake (4):
drm/vc4: Set premultiplied for alpha formats
drm/vc4: Check if plane requires background fill
drm/vc4: Move plane state to header
drm/vc4: Enable background color fill when necessary

drivers/gpu/drm/vc4/vc4_crtc.c | 25 +++++++++++++++
drivers/gpu/drm/vc4/vc4_drv.h | 60 ++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/vc4/vc4_plane.c | 68 ++++++++---------------------------------
drivers/gpu/drm/vc4/vc4_regs.h | 1 +
4 files changed, 99 insertions(+), 55 deletions(-)

[Stefan Schake]

Alpha formats in DRM are assumed to be premultiplied, so we should be
setting the PREMULT bit in the plane configuration for HVS.

Changes from v1:
- Use correct has_alpha

Signed-off-by: Stefan Schake <***@gmail.com>
---
drivers/gpu/drm/vc4/vc4_plane.c | 3 ++-
drivers/gpu/drm/vc4/vc4_regs.h | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index c4c7af1..831e195 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -618,13 +618,14 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
SCALER_POS1_SCL_HEIGHT));
}

- /* Position Word 2: Source Image Size, Alpha Mode */
+ /* Position Word 2: Source Image Size, Alpha */
vc4_state->pos2_offset = vc4_state->dlist_count;
vc4_dlist_write(vc4_state,
VC4_SET_FIELD(fb->format->has_alpha ?
SCALER_POS2_ALPHA_MODE_PIPELINE :
SCALER_POS2_ALPHA_MODE_FIXED,
SCALER_POS2_ALPHA_MODE) |
+ (fb->format->has_alpha ? SCALER_POS2_ALPHA_PREMULT : 0) |
VC4_SET_FIELD(vc4_state->src_w[0], SCALER_POS2_WIDTH) |
VC4_SET_FIELD(vc4_state->src_h[0], SCALER_POS2_HEIGHT));

diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h
index b9749cb..a141496 100644
--- a/drivers/gpu/drm/vc4/vc4_regs.h
+++ b/drivers/gpu/drm/vc4/vc4_regs.h
@@ -848,6 +848,7 @@ enum hvs_pixel_format {
#define SCALER_POS2_ALPHA_MODE_FIXED 1
#define SCALER_POS2_ALPHA_MODE_FIXED_NONZERO 2
#define SCALER_POS2_ALPHA_MODE_FIXED_OVER_0x07 3
+#define SCALER_POS2_ALPHA_PREMULT BIT(29)

#define SCALER_POS2_HEIGHT_MASK VC4_MASK(27, 16)
#define SCALER_POS2_HEIGHT_SHIFT 16

[Stefan Schake]

Considering a single plane only, we have to enable background color
when the plane has an alpha format and could be blending from the
background or when it doesn't cover the entire screen.

Changes from v1:
- Drop unrelated change
- Move needs_bg_fill to plane state

Signed-off-by: Stefan Schake <***@gmail.com>
---
drivers/gpu/drm/vc4/vc4_plane.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 831e195..8be9a87 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -73,6 +73,12 @@ struct vc4_plane_state {

/* Our allocation in LBM for temporary storage during scaling. */
struct drm_mm_node lbm;
+
+ /* Set when the plane has per-pixel alpha content or does not cover
+ * the entire screen. This is a hint to the CRTC that it might need
+ * to enable background color fill.
+ */
+ bool needs_bg_fill;
};

static inline struct vc4_plane_state *
@@ -521,6 +527,7 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
u32 ctl0_offset = vc4_state->dlist_count;
const struct hvs_format *format = vc4_get_hvs_format(fb->format->format);
int num_planes = drm_format_num_planes(format->drm);
+ bool covers_screen;
u32 scl0, scl1, pitch0;
u32 lbm_size, tiling;
unsigned long irqflags;
@@ -701,6 +708,16 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
vc4_state->dlist[ctl0_offset] |=
VC4_SET_FIELD(vc4_state->dlist_count, SCALER_CTL0_SIZE);

+ /* crtc_* are already clipped coordinates. */
+ covers_screen = vc4_state->crtc_x == 0 && vc4_state->crtc_y == 0 &&
+ vc4_state->crtc_w == state->crtc->mode.hdisplay &&
+ vc4_state->crtc_h == state->crtc->mode.vdisplay;
+ /* Background fill might be necessary when the plane has per-pixel
+ * alpha content and blends from the background or does not cover
+ * the entire screen.
+ */
+ vc4_state->needs_bg_fill = fb->format->has_alpha || !covers_screen;
+
return 0;
}

[Stefan Schake]

We need to reference it from the CRTC to make a decision for enabling
background color fill.

Signed-off-by: Stefan Schake <***@gmail.com>
---
drivers/gpu/drm/vc4/vc4_drv.h | 60 +++++++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/vc4/vc4_plane.c | 60 -----------------------------------------
2 files changed, 60 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index fefa166..1b4cd1f 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -310,6 +310,66 @@ to_vc4_plane(struct drm_plane *plane)
return (struct vc4_plane *)plane;
}

+enum vc4_scaling_mode {
+ VC4_SCALING_NONE,
+ VC4_SCALING_TPZ,
+ VC4_SCALING_PPF,
+};
+
+struct vc4_plane_state {
+ struct drm_plane_state base;
+ /* System memory copy of the display list for this element, computed
+ * at atomic_check time.
+ */
+ u32 *dlist;
+ u32 dlist_size; /* Number of dwords allocated for the display list */
+ u32 dlist_count; /* Number of used dwords in the display list. */
+
+ /* Offset in the dlist to various words, for pageflip or
+ * cursor updates.
+ */
+ u32 pos0_offset;
+ u32 pos2_offset;
+ u32 ptr0_offset;
+
+ /* Offset where the plane's dlist was last stored in the
+ * hardware at vc4_crtc_atomic_flush() time.
+ */
+ u32 __iomem *hw_dlist;
+
+ /* Clipped coordinates of the plane on the display. */
+ int crtc_x, crtc_y, crtc_w, crtc_h;
+ /* Clipped area being scanned from in the FB. */
+ u32 src_x, src_y;
+
+ u32 src_w[2], src_h[2];
+
+ /* Scaling selection for the RGB/Y plane and the Cb/Cr planes. */
+ enum vc4_scaling_mode x_scaling[2], y_scaling[2];
+ bool is_unity;
+ bool is_yuv;
+
+ /* Offset to start scanning out from the start of the plane's
+ * BO.
+ */
+ u32 offsets[3];
+
+ /* Our allocation in LBM for temporary storage during scaling. */
+ struct drm_mm_node lbm;
+
+ /* Set when the plane has per-pixel alpha content or does not cover
+ * the entire screen. This is a hint to the CRTC that it might need
+ * to enable background color fill.
+ */
+ bool needs_bg_fill;
+};
+
+static inline struct vc4_plane_state *
+to_vc4_plane_state(struct drm_plane_state *state)
+{
+ return (struct vc4_plane_state *)state;
+}
+
enum vc4_encoder_type {
VC4_ENCODER_TYPE_NONE,
VC4_ENCODER_TYPE_HDMI,
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 8be9a87..ce39390 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -27,66 +27,6 @@
#include "vc4_drv.h"
#include "vc4_regs.h"

-enum vc4_scaling_mode {
- VC4_SCALING_NONE,
- VC4_SCALING_TPZ,
- VC4_SCALING_PPF,
-};
-
-struct vc4_plane_state {
- struct drm_plane_state base;
- /* System memory copy of the display list for this element, computed
- * at atomic_check time.
- */
- u32 *dlist;
- u32 dlist_size; /* Number of dwords allocated for the display list */
- u32 dlist_count; /* Number of used dwords in the display list. */
-
- /* Offset in the dlist to various words, for pageflip or
- * cursor updates.
- */
- u32 pos0_offset;
- u32 pos2_offset;
- u32 ptr0_offset;
-
- /* Offset where the plane's dlist was last stored in the
- * hardware at vc4_crtc_atomic_flush() time.
- */
- u32 __iomem *hw_dlist;
-
- /* Clipped coordinates of the plane on the display. */
- int crtc_x, crtc_y, crtc_w, crtc_h;
- /* Clipped area being scanned from in the FB. */
- u32 src_x, src_y;
-
- u32 src_w[2], src_h[2];
-
- /* Scaling selection for the RGB/Y plane and the Cb/Cr planes. */
- enum vc4_scaling_mode x_scaling[2], y_scaling[2];
- bool is_unity;
- bool is_yuv;
-
- /* Offset to start scanning out from the start of the plane's
- * BO.
- */
- u32 offsets[3];
-
- /* Our allocation in LBM for temporary storage during scaling. */
- struct drm_mm_node lbm;
-
- /* Set when the plane has per-pixel alpha content or does not cover
- * the entire screen. This is a hint to the CRTC that it might need
- * to enable background color fill.
- */
- bool needs_bg_fill;
-};
-
-static inline struct vc4_plane_state *
-to_vc4_plane_state(struct drm_plane_state *state)
-{
- return (struct vc4_plane_state *)state;
-}
-
static const struct hvs_format {
u32 drm; /* DRM_FORMAT_* */
u32 hvs; /* HVS_FORMAT_* */

[Stefan Schake]

Using the hint from the plane state, we turn on the background color
to avoid display corruption from planes blending with the background.

Changes from v1:
- Use needs_bg_fill from plane state

Signed-off-by: Stefan Schake <***@gmail.com>
---
drivers/gpu/drm/vc4/vc4_crtc.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index ce1e3b9..bf46674 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -643,9 +643,12 @@ static void vc4_crtc_atomic_flush(struct drm_crtc *crtc,
{
struct drm_device *dev = crtc->dev;
struct vc4_dev *vc4 = to_vc4_dev(dev);
+ struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state);
struct drm_plane *plane;
+ struct vc4_plane_state *vc4_plane_state;
bool debug_dump_regs = false;
+ bool enable_bg_fill = false;
u32 __iomem *dlist_start = vc4->hvs->dlist + vc4_state->mm.start;
u32 __iomem *dlist_next = dlist_start;

@@ -656,6 +659,20 @@ static void vc4_crtc_atomic_flush(struct drm_crtc *crtc,

/* Copy all the active planes' dlist contents to the hardware dlist. */
drm_atomic_crtc_for_each_plane(plane, crtc) {
+ /* Is this the first active plane? */
+ if (dlist_next == dlist_start) {
+ /* We need to enable background fill when a plane
+ * could be alpha blending from the background, i.e.
+ * where no other plane is underneath. It suffices to
+ * consider the first active plane here since we set
+ * needs_bg_fill such that either the first plane
+ * already needs it or all planes on top blend from
+ * the first or a lower plane.
+ */
+ vc4_plane_state = to_vc4_plane_state(plane->state);
+ enable_bg_fill = vc4_plane_state->needs_bg_fill;
+ }
+
dlist_next += vc4_plane_write_dlist(plane, dlist_next);
}

@@ -664,6 +681,14 @@ static void vc4_crtc_atomic_flush(struct drm_crtc *crtc,

WARN_ON_ONCE(dlist_next - dlist_start != vc4_state->mm.size);

+ if (enable_bg_fill)
+ /* This sets a black background color fill, as is the case
+ * with other DRM drivers.
+ */
+ HVS_WRITE(SCALER_DISPBKGNDX(vc4_crtc->channel),
+ HVS_READ(SCALER_DISPBKGNDX(vc4_crtc->channel)) |
+ SCALER_DISPBKGND_FILL);
+
/* Only update DISPLIST if the CRTC was already running and is not
* being disabled.
* vc4_crtc_enable() takes care of updating the dlist just after

[Eric Anholt]

Post by Stefan Schake
This series improves the handling of alpha formats with the VC4 HVS
compositor. Alpha formats are marked as premultiplied as is standard
for DRM. Further fix a display corruption issue when planes with
per-pixel alpha try blending from the (nonexistent) background by
selectively enabling a black background color fill.

Reviewed and applied to drm-misc-next. Thanks!