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
Stefan Schake (3): drm/vc4: Set premultiplied for alpha formats drm/vc4: Check if plane requires background fill drm/vc4: Enable background color fill when necessary
drivers/gpu/drm/vc4/vc4_crtc.c | 22 ++++++++++++++++++++++ drivers/gpu/drm/vc4/vc4_drv.h | 6 ++++++ drivers/gpu/drm/vc4/vc4_plane.c | 16 +++++++++++++++- drivers/gpu/drm/vc4/vc4_regs.h | 1 + 4 files changed, 44 insertions(+), 1 deletion(-)
Alpha formats in DRM are assumed to be premultiplied, so we should be setting the PREMULT bit in the plane configuration for HVS.
Signed-off-by: Stefan Schake stschake@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..3d0c8a2 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) | + (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 stschake@gmail.com writes:
Alpha formats in DRM are assumed to be premultiplied, so we should be setting the PREMULT bit in the plane configuration for HVS.
Signed-off-by: Stefan Schake stschake@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..3d0c8a2 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) |
(format->has_alpha ? SCALER_POS2_ALPHA_PREMULT : 0) |
Looks like you meant fb->format->has_alpha here. I can fix that up when applying -- everything else looks good to me. I'll let this sit on the list for a day or two in case anyone else has feedback.
On Tue, Mar 6, 2018 at 8:35 PM, Eric Anholt eric@anholt.net wrote:
Stefan Schake stschake@gmail.com writes:
Alpha formats in DRM are assumed to be premultiplied, so we should be setting the PREMULT bit in the plane configuration for HVS.
Signed-off-by: Stefan Schake stschake@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..3d0c8a2 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) |(format->has_alpha ? SCALER_POS2_ALPHA_PREMULT : 0) |Looks like you meant fb->format->has_alpha here. I can fix that up when applying -- everything else looks good to me. I'll let this sit on the list for a day or two in case anyone else has feedback.
I remember fixing that up, and I did - but in 2/3, where of course it doesn't really belong. Sorry, I must have gotten my rebase mixed up.
Since that makes two patches that need fixups, would you prefer I send a v2 instead?
Thanks, Stefan
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.
Signed-off-by: Stefan Schake stschake@gmail.com --- drivers/gpu/drm/vc4/vc4_drv.h | 6 ++++++ drivers/gpu/drm/vc4/vc4_plane.c | 15 ++++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index fefa166..7cc6390 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -302,6 +302,12 @@ struct vc4_hvs {
struct vc4_plane { struct drm_plane base; + + /* 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 * diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 3d0c8a2..c299e29 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -517,10 +517,12 @@ static int vc4_plane_mode_set(struct drm_plane *plane, { struct vc4_dev *vc4 = to_vc4_dev(plane->dev); struct vc4_plane_state *vc4_state = to_vc4_plane_state(state); + struct vc4_plane *vc4_plane = to_vc4_plane(plane); struct drm_framebuffer *fb = state->fb; 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; @@ -625,7 +627,7 @@ static int vc4_plane_mode_set(struct drm_plane *plane, SCALER_POS2_ALPHA_MODE_PIPELINE : SCALER_POS2_ALPHA_MODE_FIXED, SCALER_POS2_ALPHA_MODE) | - (format->has_alpha ? SCALER_POS2_ALPHA_PREMULT : 0) | + (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));
@@ -701,6 +703,17 @@ 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_plane->needs_bg_fill = fb->format->has_alpha || !covers_screen; + + return 0; }
On Tue, Mar 06, 2018 at 02:48:38AM +0100, Stefan Schake wrote:
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.
Signed-off-by: Stefan Schake stschake@gmail.com
drivers/gpu/drm/vc4/vc4_drv.h | 6 ++++++ drivers/gpu/drm/vc4/vc4_plane.c | 15 ++++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index fefa166..7cc6390 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -302,6 +302,12 @@ struct vc4_hvs {
struct vc4_plane { struct drm_plane base;
- /* 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;
Looks to me like that should really be a bitmask (or something similar) in the crtc state.
};
static inline struct vc4_plane * diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 3d0c8a2..c299e29 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -517,10 +517,12 @@ static int vc4_plane_mode_set(struct drm_plane *plane, { struct vc4_dev *vc4 = to_vc4_dev(plane->dev); struct vc4_plane_state *vc4_state = to_vc4_plane_state(state);
- struct vc4_plane *vc4_plane = to_vc4_plane(plane); struct drm_framebuffer *fb = state->fb; 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;
@@ -625,7 +627,7 @@ static int vc4_plane_mode_set(struct drm_plane *plane, SCALER_POS2_ALPHA_MODE_PIPELINE : SCALER_POS2_ALPHA_MODE_FIXED, SCALER_POS2_ALPHA_MODE) |
(format->has_alpha ? SCALER_POS2_ALPHA_PREMULT : 0) |
(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));@@ -701,6 +703,17 @@ 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_plane->needs_bg_fill = fb->format->has_alpha || !covers_screen;
- return 0;
}
-- 2.7.4
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Ville Syrjälä ville.syrjala@linux.intel.com writes:
On Tue, Mar 06, 2018 at 02:48:38AM +0100, Stefan Schake wrote:
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.
Signed-off-by: Stefan Schake stschake@gmail.com
drivers/gpu/drm/vc4/vc4_drv.h | 6 ++++++ drivers/gpu/drm/vc4/vc4_plane.c | 15 ++++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index fefa166..7cc6390 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -302,6 +302,12 @@ struct vc4_hvs {
struct vc4_plane { struct drm_plane base;
- /* 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;
Looks to me like that should really be a bitmask (or something similar) in the crtc state.
Why?
In particular, VC4 really doesn't have a fixed number of planes, and the fact that we're exposing just a handful so far is probably a bug.
On Tue, Mar 06, 2018 at 04:10:33PM -0800, Eric Anholt wrote:
Ville Syrjälä ville.syrjala@linux.intel.com writes:
On Tue, Mar 06, 2018 at 02:48:38AM +0100, Stefan Schake wrote:
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.
Signed-off-by: Stefan Schake stschake@gmail.com
drivers/gpu/drm/vc4/vc4_drv.h | 6 ++++++ drivers/gpu/drm/vc4/vc4_plane.c | 15 ++++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index fefa166..7cc6390 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -302,6 +302,12 @@ struct vc4_hvs {
struct vc4_plane { struct drm_plane base;
- /* 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;
Looks to me like that should really be a bitmask (or something similar) in the crtc state.
Why?
In particular, VC4 really doesn't have a fixed number of planes, and the fact that we're exposing just a handful so far is probably a bug.
The problem is that this seems to clobber the device state from the .atomic_check() hook. So if you do a CHECK_ONLY atomic ioctl (or some later check simply fails and the operation is aborted) you've already modified the state of the device, and some later operation may then end up doing the wrong thing.
I guess you could track this in the plane struct like here, but as with the actual hardware state that shouldn't get modified until we're sure the checked state is really meant to be commited to the hardware.
Ville Syrjälä ville.syrjala@linux.intel.com writes:
On Tue, Mar 06, 2018 at 04:10:33PM -0800, Eric Anholt wrote:
Ville Syrjälä ville.syrjala@linux.intel.com writes:
On Tue, Mar 06, 2018 at 02:48:38AM +0100, Stefan Schake wrote:
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.
Signed-off-by: Stefan Schake stschake@gmail.com
drivers/gpu/drm/vc4/vc4_drv.h | 6 ++++++ drivers/gpu/drm/vc4/vc4_plane.c | 15 ++++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index fefa166..7cc6390 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -302,6 +302,12 @@ struct vc4_hvs {
struct vc4_plane { struct drm_plane base;
- /* 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;
Looks to me like that should really be a bitmask (or something similar) in the crtc state.
Why?
In particular, VC4 really doesn't have a fixed number of planes, and the fact that we're exposing just a handful so far is probably a bug.
The problem is that this seems to clobber the device state from the .atomic_check() hook. So if you do a CHECK_ONLY atomic ioctl (or some later check simply fails and the operation is aborted) you've already modified the state of the device, and some later operation may then end up doing the wrong thing.
I guess you could track this in the plane struct like here, but as with the actual hardware state that shouldn't get modified until we're sure the checked state is really meant to be commited to the hardware.
Oh, I hadn't noticed it was in vc4_plane, not vc4_plane_state. Yeah, it should be in the plane state.
Using the hint from the plane state, we turn on the background color to avoid display corruption from planes blending with the background.
Signed-off-by: Stefan Schake stschake@gmail.com --- drivers/gpu/drm/vc4/vc4_crtc.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index ce1e3b9..728845f 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -643,9 +643,11 @@ 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; 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 +658,18 @@ 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. + */ + enable_bg_fill = to_vc4_plane(plane)->needs_bg_fill; + dlist_next += vc4_plane_write_dlist(plane, dlist_next); }
@@ -664,6 +678,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
dri-devel@lists.freedesktop.org
-
Eric Anholt -
Stefan Schake -
Ville Syrjälä