[PATCH] drm/vc4: Ignore alpha on primary plane

[PATCH] drm/vc4: Ignore alpha on primary plane

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

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

Re: [PATCH] drm/vc4: Ignore alpha on primary plane

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vc4: Ignore alpha on primary plane

[Ville Syrjälä]

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?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vc4: Ignore alpha on primary plane

[Stefan Schake]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vc4: Ignore alpha on primary plane

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vc4: Ignore alpha on primary plane

[Stefan Schake]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vc4: Ignore alpha on primary plane

[Ville Syrjälä]

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ä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vc4: Ignore alpha on primary plane

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 1488 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vc4: Ignore alpha on primary plane

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vc4: Ignore alpha on primary plane

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 1306 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vc4: Ignore alpha on primary plane

[Stefan Schake]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel