| Message ID | 1519950760-68447-1-git-send-email-stschake@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 02, 2018 at 01:32:40AM +0100, Stefan Schake wrote: > 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 <stschake@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 ? 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? > SCALER_POS2_ALPHA_MODE_PIPELINE : > SCALER_POS2_ALPHA_MODE_FIXED, > SCALER_POS2_ALPHA_MODE) | > -- > 2.7.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Mar 02, 2018 at 04:39:22PM +0200, Ville Syrjälä wrote: > On Fri, Mar 02, 2018 at 01:32:40AM +0100, Stefan Schake wrote: > > 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 <stschake@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 ? > > 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?
Hey Ville, On Fri, Mar 2, 2018 at 3:43 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, Mar 02, 2018 at 04:39:22PM +0200, Ville Syrjälä wrote: >> 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
On Fri, Mar 02, 2018 at 04:06:58PM +0100, Stefan Schake wrote: > Hey Ville, > > On Fri, Mar 2, 2018 at 3:43 PM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > On Fri, Mar 02, 2018 at 04:39:22PM +0200, Ville Syrjälä wrote: > >> 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? > 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. > 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
On Fri, Mar 2, 2018 at 4:21 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, Mar 02, 2018 at 04:06:58PM +0100, Stefan Schake wrote: >> Hey Ville, >> >> On Fri, Mar 2, 2018 at 3:43 PM, Ville Syrjälä >> <ville.syrjala@linux.intel.com> wrote: >> > On Fri, Mar 02, 2018 at 04:39:22PM +0200, Ville Syrjälä wrote: >> >> 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. >> 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
On Fri, Mar 02, 2018 at 04:48:03PM +0100, Stefan Schake wrote: > On Fri, Mar 2, 2018 at 4:21 PM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > On Fri, Mar 02, 2018 at 04:06:58PM +0100, Stefan Schake wrote: > >> Hey Ville, > >> > >> On Fri, Mar 2, 2018 at 3:43 PM, Ville Syrjälä > >> <ville.syrjala@linux.intel.com> wrote: > >> > On Fri, Mar 02, 2018 at 04:39:22PM +0200, Ville Syrjälä wrote: > >> >> 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. > > >> 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ä <ville.syrjala@linux.intel.com> writes: > On Fri, Mar 02, 2018 at 04:06:58PM +0100, Stefan Schake wrote: >> Hey Ville, >> >> On Fri, Mar 2, 2018 at 3:43 PM, Ville Syrjälä >> <ville.syrjala@linux.intel.com> wrote: >> > On Fri, Mar 02, 2018 at 04:39:22PM +0200, Ville Syrjälä wrote: >> >> 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? > >> 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), then blending against black is the same as not doing any blending at all.
On Fri, Mar 02, 2018 at 09:13:14AM -0800, Eric Anholt wrote: > Ville Syrjälä <ville.syrjala@linux.intel.com> writes: > > > On Fri, Mar 02, 2018 at 04:06:58PM +0100, Stefan Schake wrote: > >> Hey Ville, > >> > >> On Fri, Mar 2, 2018 at 3:43 PM, Ville Syrjälä > >> <ville.syrjala@linux.intel.com> wrote: > >> > On Fri, Mar 02, 2018 at 04:39:22PM +0200, Ville Syrjälä wrote: > >> >> 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? > > > >> 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.
Stefan Schake <stschake@gmail.com> writes: > 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 <stschake@gmail.com> > --- > 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).
On Mon, Mar 5, 2018 at 10:15 PM, Eric Anholt <eric@anholt.net> wrote: > Stefan Schake <stschake@gmail.com> writes: > >> 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 <stschake@gmail.com> >> --- >> 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). 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
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) |
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 <stschake@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(-)