Discussion:
[PATCH] drm/vc4: Ignore alpha on primary plane
(too old to reply)
Stefan Schake
2018-03-02 00:32:40 UTC
Permalink
We allow alpha formats on the primary plane but a partially transparent
framebuffer will cause a corrupted display. With this change black pixels
are output instead, in line with the behavior for other DRM drivers.

Signed-off-by: Stefan Schake <***@gmail.com>
---
Test program is available at https://github.com/stschake/vc4-alpha-test

drivers/gpu/drm/vc4/vc4_plane.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 61ad955..8c829fa 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -521,6 +521,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 primary_plane = state->crtc->primary == plane;
u32 scl0, scl1, pitch0;
u32 lbm_size, tiling;
unsigned long irqflags;
@@ -620,8 +621,12 @@ static int vc4_plane_mode_set(struct drm_plane *plane,

/* Position Word 2: Source Image Size, Alpha Mode */
vc4_state->pos2_offset = vc4_state->dlist_count;
+ /* We do not enable the HVS background color fill so the primary plane
+ * must be opaque to avoid display artifacts. Achieve this by always
+ * using fixed alpha (initialized to 0xff above) on the primary plane.
+ */
vc4_dlist_write(vc4_state,
- VC4_SET_FIELD(fb->format->has_alpha ?
+ VC4_SET_FIELD(fb->format->has_alpha && !primary_plane ?
SCALER_POS2_ALPHA_MODE_PIPELINE :
SCALER_POS2_ALPHA_MODE_FIXED,
SCALER_POS2_ALPHA_MODE) |
--
2.7.4
Ville Syrjälä
2018-03-02 14:39:22 UTC
Permalink
Post by Stefan Schake
We allow alpha formats on the primary plane but a partially transparent
framebuffer will cause a corrupted display. With this change black pixels
are output instead, in line with the behavior for other DRM drivers.
---
Test program is available at https://github.com/stschake/vc4-alpha-test
drivers/gpu/drm/vc4/vc4_plane.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 61ad955..8c829fa 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -521,6 +521,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 primary_plane = state->crtc->primary == plane;
u32 scl0, scl1, pitch0;
u32 lbm_size, tiling;
unsigned long irqflags;
@@ -620,8 +621,12 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
/* Position Word 2: Source Image Size, Alpha Mode */
vc4_state->pos2_offset = vc4_state->dlist_count;
+ /* We do not enable the HVS background color fill so the primary plane
+ * must be opaque to avoid display artifacts. Achieve this by always
+ * using fixed alpha (initialized to 0xff above) on the primary plane.
+ */
vc4_dlist_write(vc4_state,
- VC4_SET_FIELD(fb->format->has_alpha ?
+ VC4_SET_FIELD(fb->format->has_alpha && !primary_plane ?
If you want the plane to always be opaque you shouldn't expose any
formats with alpha.

Also what happens if one disables the primary plane? Or does the driver
not allow that?
Post by Stefan Schake
SCALER_POS2_ALPHA_MODE_FIXED,
SCALER_POS2_ALPHA_MODE) |
--
2.7.4
_______________________________________________
dri-devel mailing list
https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
Ville Syrjälä
2018-03-02 14:43:52 UTC
Permalink
Post by Ville Syrjälä
Post by Stefan Schake
We allow alpha formats on the primary plane but a partially transparent
framebuffer will cause a corrupted display. With this change black pixels
are output instead, in line with the behavior for other DRM drivers.
---
Test program is available at https://github.com/stschake/vc4-alpha-test
drivers/gpu/drm/vc4/vc4_plane.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 61ad955..8c829fa 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -521,6 +521,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 primary_plane = state->crtc->primary == plane;
u32 scl0, scl1, pitch0;
u32 lbm_size, tiling;
unsigned long irqflags;
@@ -620,8 +621,12 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
/* Position Word 2: Source Image Size, Alpha Mode */
vc4_state->pos2_offset = vc4_state->dlist_count;
+ /* We do not enable the HVS background color fill so the primary plane
+ * must be opaque to avoid display artifacts. Achieve this by always
+ * using fixed alpha (initialized to 0xff above) on the primary plane.
+ */
vc4_dlist_write(vc4_state,
- VC4_SET_FIELD(fb->format->has_alpha ?
+ VC4_SET_FIELD(fb->format->has_alpha && !primary_plane ?
If you want the plane to always be opaque you shouldn't expose any
formats with alpha.
Also what happens if one disables the primary plane? Or does the driver
not allow that?
Or just makes the plane not cover the entire screen?
--
Ville Syrjälä
Intel OTC
Stefan Schake
2018-03-02 15:06:58 UTC
Permalink
Hey Ville,

On Fri, Mar 2, 2018 at 3:43 PM, Ville Syrjälä
Post by Ville Syrjälä
Post by Ville Syrjälä
If you want the plane to always be opaque you shouldn't expose any
formats with alpha.
Also what happens if one disables the primary plane? Or does the driver
not allow that?
Or just makes the plane not cover the entire screen?
We've exposed alpha formats in the past so disabling that now would break
userspace, certainly Android that chooses alpha-everything. The VC4 HVS
has no fixed planes so I'll acknowledge that the concept of a primary plane
is somewhat dubious and userspace could disable it or make it not cover the
entire screen, making this ineffective.

But then ultimately there doesn't seem to be a standard for what the display
is supposed to be if you have transparent pixels with no plane to blend to
below. This helps in the common case, the belts&braces fix would be to
enable the VC4 HVS background color fill incurring a fixed overhead that
in most cases is wasted because the composition ends up being opaque.

Thanks,
Stefan
Ville Syrjälä
2018-03-02 15:21:17 UTC
Permalink
Post by Stefan Schake
Hey Ville,
On Fri, Mar 2, 2018 at 3:43 PM, Ville Syrjälä
Post by Ville Syrjälä
Post by Ville Syrjälä
If you want the plane to always be opaque you shouldn't expose any
formats with alpha.
Also what happens if one disables the primary plane? Or does the driver
not allow that?
Or just makes the plane not cover the entire screen?
We've exposed alpha formats in the past so disabling that now would break
userspace, certainly Android that chooses alpha-everything.
So it refuses to even run on hardware that can't do per-pixel alpha on
the primary plane?
Post by Stefan Schake
The VC4 HVS
has no fixed planes so I'll acknowledge that the concept of a primary plane
is somewhat dubious and userspace could disable it or make it not cover the
entire screen, making this ineffective.
But then ultimately there doesn't seem to be a standard for what the display
is supposed to be if you have transparent pixels with no plane to blend to
below.
The standard is black. IMO it's a driver bug if it fails to do that.
Post by Stefan Schake
This helps in the common case, the belts&braces fix would be to
enable the VC4 HVS background color fill incurring a fixed overhead that
in most cases is wasted because the composition ends up being opaque.
Thanks,
Stefan
--
Ville Syrjälä
Intel OTC
Stefan Schake
2018-03-02 15:48:03 UTC
Permalink
On Fri, Mar 2, 2018 at 4:21 PM, Ville Syrjälä
Post by Ville Syrjälä
Post by Stefan Schake
Hey Ville,
On Fri, Mar 2, 2018 at 3:43 PM, Ville Syrjälä
Post by Ville Syrjälä
Post by Ville Syrjälä
If you want the plane to always be opaque you shouldn't expose any
formats with alpha.
Also what happens if one disables the primary plane? Or does the driver
not allow that?
Or just makes the plane not cover the entire screen?
We've exposed alpha formats in the past so disabling that now would break
userspace, certainly Android that chooses alpha-everything.
So it refuses to even run on hardware that can't do per-pixel alpha on
the primary plane?
Well since we have no real primary plane we'd have to remove support from
every plane or add elaborate logic to atomic check that detects and rejects
a configuration that has pixels blending from nothing, which presumably is
what triggers the display artifacts.
Post by Ville Syrjälä
Post by Stefan Schake
The VC4 HVS
has no fixed planes so I'll acknowledge that the concept of a primary plane
is somewhat dubious and userspace could disable it or make it not cover the
entire screen, making this ineffective.
But then ultimately there doesn't seem to be a standard for what the display
is supposed to be if you have transparent pixels with no plane to blend to
below.
The standard is black. IMO it's a driver bug if it fails to do that.
Then to be sure we should always enable the background fill. Maybe Eric can
clarify what the cost for this is, all I have to go on there is the comment
on the register set.

Thanks,
Stefan
Ville Syrjälä
2018-03-02 16:04:43 UTC
Permalink
Post by Stefan Schake
On Fri, Mar 2, 2018 at 4:21 PM, Ville Syrjälä
Post by Ville Syrjälä
Post by Stefan Schake
Hey Ville,
On Fri, Mar 2, 2018 at 3:43 PM, Ville Syrjälä
Post by Ville Syrjälä
Post by Ville Syrjälä
If you want the plane to always be opaque you shouldn't expose any
formats with alpha.
Also what happens if one disables the primary plane? Or does the driver
not allow that?
Or just makes the plane not cover the entire screen?
We've exposed alpha formats in the past so disabling that now would break
userspace, certainly Android that chooses alpha-everything.
So it refuses to even run on hardware that can't do per-pixel alpha on
the primary plane?
Well since we have no real primary plane we'd have to remove support from
every plane or add elaborate logic to atomic check that detects and rejects
a configuration that has pixels blending from nothing, which presumably is
what triggers the display artifacts.
Post by Ville Syrjälä
Post by Stefan Schake
The VC4 HVS
has no fixed planes so I'll acknowledge that the concept of a primary plane
is somewhat dubious and userspace could disable it or make it not cover the
entire screen, making this ineffective.
But then ultimately there doesn't seem to be a standard for what the display
is supposed to be if you have transparent pixels with no plane to blend to
below.
The standard is black. IMO it's a driver bug if it fails to do that.
Then to be sure we should always enable the background fill. Maybe Eric can
clarify what the cost for this is, all I have to go on there is the comment
on the register set.
Yeah, that sounds like the correct thing to do.
--
Ville Syrjälä
Intel OTC
Ville Syrjälä
2018-03-02 17:58:18 UTC
Permalink
Post by Ville Syrjälä
Post by Stefan Schake
Hey Ville,
On Fri, Mar 2, 2018 at 3:43 PM, Ville Syrjälä
Post by Ville Syrjälä
Post by Ville Syrjälä
If you want the plane to always be opaque you shouldn't expose any
formats with alpha.
Also what happens if one disables the primary plane? Or does the driver
not allow that?
Or just makes the plane not cover the entire screen?
We've exposed alpha formats in the past so disabling that now would break
userspace, certainly Android that chooses alpha-everything.
So it refuses to even run on hardware that can't do per-pixel alpha on
the primary plane?
Post by Stefan Schake
The VC4 HVS
has no fixed planes so I'll acknowledge that the concept of a primary plane
is somewhat dubious and userspace could disable it or make it not cover the
entire screen, making this ineffective.
But then ultimately there doesn't seem to be a standard for what the display
is supposed to be if you have transparent pixels with no plane to blend to
below.
The standard is black. IMO it's a driver bug if it fails to do that.
If the plane is premultiplied (isn't that what DRM planes are supposed
to be? I can't find any docs),
Yeah I think pre-multiplied is what was agreed on because that was
what everyone's cursor planes were doing already. But I guess we didn't
actually document that anywhere :(

Should we add something like
+/*
+ * Note that using any format with alpha (A) implies pre-multiplied
+ * alpha blending: Dc = Sc + (1.0 - Sa) * Dc, where Sa is source alpha,
+ * Sc is source color, and Dc is destination color.
+ */
to drm_fourcc.h perhaps?

Not sure if that's the best place, nor am I sure the wording is good
enough. Opinions welcome.

igt could really use some alpha blending tests as well...
then blending against black is the same
as not doing any blending at all.
Ah, yes indeeed. Hmm. And with my i915 hat on that actually means that we
should in theory be able to expose alpha formats on i915 primary planes
because there can never be anything but black underneath them on account
of the fixed zorder. But we can't make that promise for the sprite planes,
so probably not worth the hassle for primary planes either. Sufficiently
smart userspace should be able figure this fact out on its own I
suppose.
--
Ville Syrjälä
Intel OTC
Eric Anholt
2018-03-05 21:15:39 UTC
Permalink
Post by Stefan Schake
We allow alpha formats on the primary plane but a partially transparent
framebuffer will cause a corrupted display. With this change black pixels
are output instead, in line with the behavior for other DRM drivers.
---
Test program is available at https://github.com/stschake/vc4-alpha-test
How about this as a suggestion for a patch series:

vc4_plane_mode_set() sets ALPHA_PREMULT (POS2 bit 29) if alpha is
enabled.

vc4_plane_mode_set() sets a new vc4_plane->needs_bg_fill boolean to
(format->has_alpha || !covers_screen) where covers_screenis the
can_position logic from drm_atomic_helper.c

vc4_crtc_atomic_flush() updates DISPBKGND to enable background fill
(before vc4_crtc_update_dlist()) if the first plane has needs_bg_fill
set.

vc4_plane_mode_set() strips off the alpha blend bits if
!vc4_plane->needs_bg_fill.

This lets us keep avoiding the background fill cost in the normal case,
and fixes the case where the "primary" plane doesn't cover the screen.
It doesn't get the background fill turned back off if you transition
away from primary not covering the screen, but that seems unlikely and
harder to handle (since you would need to wait for the flip to be done
before disabling).
Stefan Schake
2018-03-06 01:55:30 UTC
Permalink
Post by Eric Anholt
Post by Stefan Schake
We allow alpha formats on the primary plane but a partially transparent
framebuffer will cause a corrupted display. With this change black pixels
are output instead, in line with the behavior for other DRM drivers.
---
Test program is available at https://github.com/stschake/vc4-alpha-test
vc4_plane_mode_set() sets ALPHA_PREMULT (POS2 bit 29) if alpha is
enabled.
vc4_plane_mode_set() sets a new vc4_plane->needs_bg_fill boolean to
(format->has_alpha || !covers_screen) where covers_screenis the
can_position logic from drm_atomic_helper.c
vc4_crtc_atomic_flush() updates DISPBKGND to enable background fill
(before vc4_crtc_update_dlist()) if the first plane has needs_bg_fill
set.
vc4_plane_mode_set() strips off the alpha blend bits if
!vc4_plane->needs_bg_fill.
This lets us keep avoiding the background fill cost in the normal case,
and fixes the case where the "primary" plane doesn't cover the screen.
It doesn't get the background fill turned back off if you transition
away from primary not covering the screen, but that seems unlikely and
harder to handle (since you would need to wait for the flip to be done
before disabling).
I've sent out the series:

https://patchwork.freedesktop.org/series/39411/

I don't think we need the final change since

!needs_bg_fill <=> !(has_alpha||!covers_screen) <=> covers_screen && !has_alpha

so we shouldn't be setting the alpha blend bits in the first place.

Thanks,
Stefan

Loading...