Re: [Freedreno] [PATCH RFC v4 1/7] drm: Introduce solid fill DRM plane property

[Pekka Paalanen]

On Mon, 10 Jul 2023 16:12:06 -0700
Jessica Zhang <quic_jesszhan@xxxxxxxxxxx> wrote:

> On 6/30/2023 1:27 AM, Pekka Paalanen wrote:
> > On Thu, 29 Jun 2023 17:25:00 -0700
> > Jessica Zhang <quic_jesszhan@xxxxxxxxxxx> wrote:
> >   
> >> Document and add support for solid_fill property to drm_plane. In
> >> addition, add support for setting and getting the values for solid_fill.
> >>
> >> To enable solid fill planes, userspace must assign a property blob to
> >> the "solid_fill" plane property containing the following information:
> >>
> >> struct drm_solid_fill_info {
> >> 	u8 version;
> >> 	u32 r, g, b;
> >> };
> >>
> >> Signed-off-by: Jessica Zhang <quic_jesszhan@xxxxxxxxxxx>  
> > 
> > Hi Jessica,
> > 
> > I've left some general UAPI related comments here.
> >   
> >> ---
> >>   drivers/gpu/drm/drm_atomic_state_helper.c |  9 +++++
> >>   drivers/gpu/drm/drm_atomic_uapi.c         | 55 +++++++++++++++++++++++++++++++
> >>   drivers/gpu/drm/drm_blend.c               | 33 +++++++++++++++++++
> >>   include/drm/drm_blend.h                   |  1 +
> >>   include/drm/drm_plane.h                   | 43 ++++++++++++++++++++++++
> >>   5 files changed, 141 insertions(+)

...

> >> diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
> >> index 88bdfec3bd88..0338a860b9c8 100644
> >> --- a/include/drm/drm_blend.h
> >> +++ b/include/drm/drm_blend.h
> >> @@ -58,4 +58,5 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
> >>   			      struct drm_atomic_state *state);
> >>   int drm_plane_create_blend_mode_property(struct drm_plane *plane,
> >>   					 unsigned int supported_modes);
> >> +int drm_plane_create_solid_fill_property(struct drm_plane *plane);
> >>   #endif
> >> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> >> index 51291983ea44..f6ab313cb83e 100644
> >> --- a/include/drm/drm_plane.h
> >> +++ b/include/drm/drm_plane.h
> >> @@ -40,6 +40,25 @@ enum drm_scaling_filter {
> >>   	DRM_SCALING_FILTER_NEAREST_NEIGHBOR,
> >>   };
> >>   
> >> +/**
> >> + * struct drm_solid_fill_info - User info for solid fill planes
> >> + */
> >> +struct drm_solid_fill_info {
> >> +	__u8 version;
> >> +	__u32 r, g, b;
> >> +};  
> > 
> > Shouldn't UAPI structs be in UAPI headers?  
> 
> Acked, will move this to uapi/drm_mode.h
> 
> > 
> > Shouldn't UAPI structs use explicit padding to not leave any gaps when
> > it's unavoidable? And the kernel to check that the gaps are indeed
> > zeroed?  
> 
> I don't believe so... From my understanding, padding will be taken care 
> of by the compiler by default. Looking at the drm_mode_modeinfo UAPI 
> struct [1], it also doesn't seem to do explicit padding. And the 
> corresponding set_property() code doesn't seem check the gaps either.
> 
> That being said, it's possible that I'm missing something here, so 
> please let me know if that's the case.
> 
> [1] 
> https://elixir.bootlin.com/linux/v6.5-rc1/source/include/uapi/drm/drm_mode.h#L242

I suspect that drm_mode_modeinfo predates the lessons learnt about
"botching up ioctls" by many years:
https://www.kernel.org/doc/Documentation/ioctl/botching-up-ioctls.rst

drm_mode_modeinfo goes all the way back to

commit f453ba0460742ad027ae0c4c7d61e62817b3e7ef
Date:   Fri Nov 7 14:05:41 2008 -0800

    DRM: add mode setting support

and

commit e0c8463a8b00b467611607df0ff369d062528875
Date:   Fri Dec 19 14:50:50 2008 +1000

    drm: sanitise drm modesetting API + remove unused hotplug

and it got the proper types later in

commit 1d7f83d5ad6c30b385ba549c1c3a287cc872b7ae
Date:   Thu Feb 26 00:51:42 2009 +0100

    make drm headers use strict integer types


My personal feeling is that if you cannot avoid padding in a struct,
convert it into explicit fields anyway and require them to be zero.
That way if you ever need to extend or modify the struct, you already
have an "unused" field that old userspace guarantees to be zero, so you
can re-purpose it when it's not zero.

A struct for blob contents is maybe needing slightly less forward
planning than ioctl struct, because KMS properties are cheap compared
to ioctl numbers, I believe.

Maybe eliminating compiler induced padding in structs is not strictly
necessary, but it seems like a good idea to me, because compilers are
allowed to leave the padding bits undefined. If a struct was filled in
by the kernel and delivered to userspace, undefined padding could even
be a security leak, theoretically.

Anyway, don't take my word for it. Maybe kernel devs have a different
style.


Thanks,
pq

> > 
> > It also needs more UAPI doc, like a link to the property doc that uses
> > this and an explanation of what the values mean.  
> 
> Acked.
> 
> Thanks,
> 
> Jessica Zhang
> 
> > 
> > 
> > Thanks,
> > pq
> >   
> >> +
> >> +/**
> >> + * struct solid_fill_property - RGB values for solid fill plane
> >> + *
> >> + * Note: This is the V1 for this feature
> >> + */
> >> +struct drm_solid_fill {
> >> +	uint32_t r;
> >> +	uint32_t g;
> >> +	uint32_t b;
> >> +};
> >> +
> >>   /**
> >>    * struct drm_plane_state - mutable plane state
> >>    *
> >> @@ -116,6 +135,23 @@ struct drm_plane_state {
> >>   	/** @src_h: height of visible portion of plane (in 16.16) */
> >>   	uint32_t src_h, src_w;
> >>   
> >> +	/**
> >> +	 * @solid_fill_blob:
> >> +	 *
> >> +	 * Blob containing relevant information for a solid fill plane
> >> +	 * including pixel format and data. See
> >> +	 * drm_plane_create_solid_fill_property() for more details.
> >> +	 */
> >> +	struct drm_property_blob *solid_fill_blob;
> >> +
> >> +	/**
> >> +	 * @solid_fill:
> >> +	 *
> >> +	 * Pixel data for solid fill planes. See
> >> +	 * drm_plane_create_solid_fill_property() for more details.
> >> +	 */
> >> +	struct drm_solid_fill solid_fill;
> >> +
> >>   	/**
> >>   	 * @alpha:
> >>   	 * Opacity of the plane with 0 as completely transparent and 0xffff as
> >> @@ -699,6 +735,13 @@ struct drm_plane {
> >>   	 */
> >>   	struct drm_plane_state *state;
> >>   
> >> +	/*
> >> +	 * @solid_fill_property:
> >> +	 * Optional solid_fill property for this plane. See
> >> +	 * drm_plane_create_solid_fill_property().
> >> +	 */
> >> +	struct drm_property *solid_fill_property;
> >> +
> >>   	/**
> >>   	 * @alpha_property:
> >>   	 * Optional alpha property for this plane. See
> >>  
> >

Attachment: pgpxrOU5MTwt7.pgp
Description: OpenPGP digital signature