[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e17db728-d91b-a2b3-08a9-1dd1fde9c727@linaro.org>
Date: Mon, 10 Jul 2023 23:11:32 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Jessica Zhang <quic_jesszhan@...cinc.com>,
Pekka Paalanen <ppaalanen@...il.com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>,
Daniel Vetter <daniel@...ll.ch>,
Rob Clark <robdclark@...il.com>, Sean Paul <sean@...rly.run>,
Marijn Suijten <marijn.suijten@...ainline.org>,
quic_abhinavk@...cinc.com, contact@...rsion.fr,
laurent.pinchart@...asonboard.com, sebastian.wick@...hat.com,
ville.syrjala@...ux.intel.com, dri-devel@...ts.freedesktop.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
freedreno@...ts.freedesktop.org,
wayland-devel@...ts.freedesktop.org
Subject: Re: [PATCH RFC v4 2/7] drm: Introduce pixel_source DRM plane property
On 10/07/2023 22:51, Jessica Zhang wrote:
>
>
> On 6/30/2023 1:27 AM, Pekka Paalanen wrote:
>> On Fri, 30 Jun 2023 03:42:28 +0300
>> Dmitry Baryshkov <dmitry.baryshkov@...aro.org> wrote:
>>
>>> On 30/06/2023 03:25, Jessica Zhang wrote:
>>>> Add support for pixel_source property to drm_plane and related
>>>> documentation.
>>>>
>>>> This enum property will allow user to specify a pixel source for the
>>>> plane. Possible pixel sources will be defined in the
>>>> drm_plane_pixel_source enum.
>>>>
>>>> The current possible pixel sources are DRM_PLANE_PIXEL_SOURCE_FB and
>>>> DRM_PLANE_PIXEL_SOURCE_COLOR. The default value is *_SOURCE_FB.
>>>
>>> I think, this should come before the solid fill property addition. First
>>> you tell that there is a possibility to define other pixel sources, then
>>> additional sources are defined.
>>
>> Hi,
>>
>> that would be logical indeed.
>
> Hi Dmitry and Pekka,
>
> Sorry for the delay in response, was out of office last week.
>
> Acked.
>
>>
>>>>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@...cinc.com>
>>>> ---
>>>> drivers/gpu/drm/drm_atomic_state_helper.c | 1 +
>>>> drivers/gpu/drm/drm_atomic_uapi.c | 4 ++
>>>> drivers/gpu/drm/drm_blend.c | 81
>>>> +++++++++++++++++++++++++++++++
>>>> include/drm/drm_blend.h | 2 +
>>>> include/drm/drm_plane.h | 21 ++++++++
>>>> 5 files changed, 109 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
>>>> b/drivers/gpu/drm/drm_atomic_state_helper.c
>>>> index fe14be2bd2b2..86fb876efbe6 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
>>>> @@ -252,6 +252,7 @@ void
>>>> __drm_atomic_helper_plane_state_reset(struct drm_plane_state
>>>> *plane_state,
>>>> plane_state->alpha = DRM_BLEND_ALPHA_OPAQUE;
>>>> plane_state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
>>>> + plane_state->pixel_source = DRM_PLANE_PIXEL_SOURCE_FB;
>>>> if (plane_state->solid_fill_blob) {
>>>> drm_property_blob_put(plane_state->solid_fill_blob);
>>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
>>>> b/drivers/gpu/drm/drm_atomic_uapi.c
>>>> index a28b4ee79444..6e59c21af66b 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>>>> @@ -596,6 +596,8 @@ static int drm_atomic_plane_set_property(struct
>>>> drm_plane *plane,
>>>> drm_property_blob_put(solid_fill);
>>>> return ret;
>>>> + } else if (property == plane->pixel_source_property) {
>>>> + state->pixel_source = val;
>>>> } else if (property == plane->alpha_property) {
>>>> state->alpha = val;
>>>> } else if (property == plane->blend_mode_property) {
>>>
>>> I think, it was pointed out in the discussion that drm_mode_setplane()
>>> (a pre-atomic IOCTL to turn the plane on and off) should also reset
>>> pixel_source to FB.
>
> I don't remember drm_mode_setplane() being mentioned in the pixel_source
> discussion... can you share where it was mentioned?
https://lore.kernel.org/dri-devel/20230627105849.004050b3@eldfell/
Let me quote it here:
"Legacy non-atomic UAPI wrappers can do whatever they want, and program
any (new) properties they want in order to implement the legacy
expectations, so that does not seem to be a problem."
>
> I'd prefer to avoid having driver change the pixel_source directly as it
> could cause some unexpected side effects. In general, I would like
> userspace to assign the value of pixel_source without driver doing
> anything "under the hood".
s/driver/drm core/
We have to remain compatible with old userspace, especially with the
non-atomic one. If the userspace calls ioctl(DRM_IOCTL_MODE_SETPLANE),
we have to display the specified FB, no matter what was the value of
PIXEL_SOURCE before this ioctl.
>
>>>
>>>> @@ -671,6 +673,8 @@ drm_atomic_plane_get_property(struct drm_plane
>>>> *plane,
>>>> } else if (property == plane->solid_fill_property) {
>>>> *val = state->solid_fill_blob ?
>>>> state->solid_fill_blob->base.id : 0;
>>>> + } else if (property == plane->pixel_source_property) {
>>>> + *val = state->pixel_source;
>>>> } else if (property == plane->alpha_property) {
>>>> *val = state->alpha;
>>>> } else if (property == plane->blend_mode_property) {
>>>> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
>>>> index 38c3c5d6453a..8c100a957ee2 100644
>>>> --- a/drivers/gpu/drm/drm_blend.c
>>>> +++ b/drivers/gpu/drm/drm_blend.c
>>>> @@ -189,6 +189,18 @@
>>>> * solid_fill is set up with
>>>> drm_plane_create_solid_fill_property(). It
>>>> * contains pixel data that drivers can use to fill a plane.
>>>> *
>>>> + * pixel_source:
>>>> + * pixel_source is set up with
>>>> drm_plane_create_pixel_source_property().
>>>> + * It is used to toggle the source of pixel data for the plane.
>>
>> Other sources than the selected one are ignored?
>
> Yep, the plane will only display the data from the set pixel_source.
>
> So if pixel_source == FB and solid_fill_blob is non-NULL,
> solid_fill_blob will be ignored and the plane will display the FB that
> is set.
correct.
>
> Will add a note about this in the comment docs.
>
>>
>>>> + *
>>>> + * Possible values:
>>
>> Wouldn't hurt to explicitly mention here that this is an enum.
>
> Acked.
>
>>
>>>> + *
>>>> + * "FB":
>>>> + * Framebuffer source
>>>> + *
>>>> + * "COLOR":
>>>> + * solid_fill source
>>
>> I think these two should be more explicit. Framebuffer source is the
>> usual source from the property "FB_ID". Solid fill source comes from
>> the property "solid_fill".
>
> Acked.
>
>>
>> Why "COLOR" and not, say, "SOLID_FILL"?
>
> Ah, that would make more sense :)
>
> I'll change this to "SOLID_FILL".
>
>>
>>>> + *
>>>> * Note that all the property extensions described here apply
>>>> either to the
>>>> * plane or the CRTC (e.g. for the background color, which
>>>> currently is not
>>>> * exposed and assumed to be black).
>>>> @@ -648,3 +660,72 @@ int drm_plane_create_solid_fill_property(struct
>>>> drm_plane *plane)
>>>> return 0;
>>>> }
>>>> EXPORT_SYMBOL(drm_plane_create_solid_fill_property);
>>>> +
>>>> +/**
>>>> + * drm_plane_create_pixel_source_property - create a new pixel
>>>> source property
>>>> + * @plane: drm plane
>>>> + * @supported_sources: bitmask of supported pixel_sources for the
>>>> driver (NOT
>>>> + * including DRM_PLANE_PIXEL_SOURCE_FB, as it
>>>> will be supported
>>>> + * by default).
>>>
>>> I'd say this is too strong. I'd suggest either renaming this to
>>> extra_sources (mentioning that FB is enabled for all the planes) or
>>> allowing any source bitmask (mentioning that FB should be enabled by the
>>> caller, unless there is a good reason not to do so).
>>
>> Right. I don't see any problem with having planes of type OVERLAY that
>> support only solid_fill and no FB. Planes of type PRIMARY and CURSOR I
>> would expect to always support at least FB.
>>
>> Atomic userspace is prepared to have an OVERLAY plane fail for any
>> arbitrary reason. Legacy userspace probably should not ever see a plane
>> that does not support FB.
>
> Got it... If we allow the possibility of FB sources not being supported,
> then should the default pixel_source per plane be decided by the driver
> too?
>
> I'd forced FB support so that I could set pixel_source to FB in
> __drm_atomic_helper_plane_state_reset(). If we allow more flexibility in
> the default pixel_source value, I guess we can also store a
> default_pixel_source value in the plane_state.
I'd say, the FB is a sane default. It the driver has other needs, it can
override the value in drm_plane_funcs::reset().
>
[skipped the rest]
--
With best wishes
Dmitry
Powered by blists - more mailing lists