Message-ID: <79eb29b5-f018-d92c-b514-5ae0c954ff46@quicinc.com>
Date:   Tue, 11 Jul 2023 15:42:28 -0700
From:   Abhinav Kumar <quic_abhinavk@...cinc.com>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        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>,
        <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 7/11/2023 3:19 PM, Dmitry Baryshkov wrote:
> On 12/07/2023 01:07, Jessica Zhang wrote:
>>
>>
>> On 7/10/2023 1:11 PM, Dmitry Baryshkov wrote:
>>> 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.
>>
>>
>> Got it, thanks the clarification -- I see your point.
>>
>> I'm already setting plane_state->pixel_source to FB in 
>> __drm_atomic_helper_plane_reset() and it seems to me that all drivers 
>> are calling that within their respective plane_funcs->reset().
>>
>> Since (as far as I know) reset() is being called for both the atomic 
>> and non-atomic paths, shouldn't that be enough to default pixel_source 
>> to FB for old userspace?
> 
> No, this will not clean up the state between userspace apps. Currently 
> the rule is simple: call DRM_IOCTL_MODE_SETPLANE, get the image from FB 
> displayed. We should keep it so.
> 

Ok, so you are considering a use-case where we bootup with a userspace 
(which is aware of pixel_source), that one uses the pixel_source to 
switch the property to solid_color and then we kill this userspace and 
bootup one which is unaware of this property and uses 
DRM_IOCTL_MODE_SETPLANE, then we should default back to FB.

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