Re: [PATCH 2/3] drm/vc4: Check if plane requires background fill

[Eric Anholt <eric@anholt.net>]

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

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.

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