Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 11 Jul 2023 14:47:55 -0700 | Subject | Re: [Freedreno] [PATCH RFC v4 1/7] drm: Introduce solid fill DRM plane property | From | Jessica Zhang <> |
| |
On 7/11/2023 12:42 AM, Pekka Paalanen wrote: > On Mon, 10 Jul 2023 16:12:06 -0700 > Jessica Zhang <quic_jesszhan@quicinc.com> wrote: > >> On 6/30/2023 1:27 AM, Pekka Paalanen wrote: >>> On Thu, 29 Jun 2023 17:25:00 -0700 >>> Jessica Zhang <quic_jesszhan@quicinc.com> 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@quicinc.com> >>> >>> 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.
Ah, got it. Thanks for the info! Looking over more recent implementations of blob properties, I do see that there's a precedent for explicit padding [1].
I think I could also just make `version` a __u32 instead. Either way, that seems to be how other structs declare `version`.
Thanks,
Jessica Zhang
[1] https://elixir.bootlin.com/linux/latest/source/include/uapi/drm/virtgpu_drm.h#L178
> > > 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 >>>> >>> >
| ![\](/images/icornerr.gif) |