Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <53ca10d5-c1e0-285a-30b9-4e9a2a1b70c9@quicinc.com>
Date:   Tue, 11 Jul 2023 15:07:19 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
From:   Jessica Zhang <quic_jesszhan@quicinc.com>
Subject: Re: [PATCH RFC v4 2/7] drm: Introduce pixel_source DRM plane property
To:     Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Pekka Paalanen <ppaalanen@gmail.com>
CC:     Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Rob Clark <robdclark@gmail.com>, Sean Paul <sean@poorly.run>,
        Marijn Suijten <marijn.suijten@somainline.org>,
        <quic_abhinavk@quicinc.com>, <contact@emersion.fr>,
        <laurent.pinchart@ideasonboard.com>, <sebastian.wick@redhat.com>,
        <ville.syrjala@linux.intel.com>, <dri-devel@lists.freedesktop.org>,
        <linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <freedreno@lists.freedesktop.org>,
        <wayland-devel@lists.freedesktop.org>
References: <20230404-solid-fill-v4-0-f4ec0caa742d@quicinc.com>
 <20230404-solid-fill-v4-2-f4ec0caa742d@quicinc.com>
 <6e3eec49-f798-ff91-8b4d-417d31089296@linaro.org>
 <20230630112708.4d3a08a7@eldfell>
 <eb78b4d6-6da2-1cb5-5fab-01d7bf233111@quicinc.com>
 <e17db728-d91b-a2b3-08a9-1dd1fde9c727@linaro.org>
Content-Language: en-US
In-Reply-To: <e17db728-d91b-a2b3-08a9-1dd1fde9c727@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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@linaro.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@quicinc.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?

Thanks,

Jessica Zhang

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