[Freedreno] [PATCH RFC v4 1/7] drm: Introduce solid fill DRM plane property
Jessica Zhang
quic_jesszhan at quicinc.com
Tue Jul 11 21:47:55 UTC 2023
On 7/11/2023 12:42 AM, Pekka Paalanen wrote:
> On Mon, 10 Jul 2023 16:12:06 -0700
> Jessica Zhang <quic_jesszhan at 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 at 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 at 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
>>>>
>>>
>
More information about the dri-devel
mailing list