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

[Jessica Zhang <quic_jesszhan@quicinc.com>]

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