[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <26585954-7c86-45fd-9190-f1109cbe9901@quicinc.com>
Date: Mon, 16 Oct 2023 17:40:53 -0700
From: Jessica Zhang <quic_jesszhan@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
Pekka Paalanen <ppaalanen@...il.com>
CC: <linux-arm-msm@...r.kernel.org>, <freedreno@...ts.freedesktop.org>,
<sebastian.wick@...hat.com>,
Thomas Zimmermann <tzimmermann@...e.de>,
"Sean Paul" <sean@...rly.run>, <dri-devel@...ts.freedesktop.org>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
<quic_abhinavk@...cinc.com>, "Maxime Ripard" <mripard@...nel.org>,
<linux-kernel@...r.kernel.org>, Rob Clark <robdclark@...il.com>,
<laurent.pinchart@...asonboard.com>,
Daniel Vetter <daniel@...ll.ch>, <contact@...rsion.fr>,
Marijn Suijten <marijn.suijten@...ainline.org>,
<wayland-devel@...ts.freedesktop.org>,
"David Airlie" <airlied@...il.com>, <ville.syrjala@...ux.intel.com>
Subject: Re: [Freedreno] [PATCH RFC v6 07/10] drm/atomic: Loosen FB atomic
checks
On 9/24/2023 3:23 AM, Dmitry Baryshkov wrote:
> On 22/09/2023 20:49, Jessica Zhang wrote:
>>
>>
>> On 8/29/2023 1:22 AM, Pekka Paalanen wrote:
>>> On Mon, 28 Aug 2023 17:05:13 -0700
>>> Jessica Zhang <quic_jesszhan@...cinc.com> wrote:
>>>
>>>> Loosen the requirements for atomic and legacy commit so that, in cases
>>>> where pixel_source != FB, the commit can still go through.
>>>>
>>>> This includes adding framebuffer NULL checks in other areas to
>>>> account for
>>>> FB being NULL when non-FB pixel sources are enabled.
>>>>
>>>> To disable a plane, the pixel_source must be NONE or the FB must be
>>>> NULL
>>>> if pixel_source == FB.
>>>>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@...cinc.com>
>>>> ---
>>>> drivers/gpu/drm/drm_atomic.c | 20 +++++++++++---------
>>>> drivers/gpu/drm/drm_atomic_helper.c | 36
>>>> ++++++++++++++++++++----------------
>>>> include/drm/drm_atomic_helper.h | 4 ++--
>>>> include/drm/drm_plane.h | 29
>>>> +++++++++++++++++++++++++++++
>>>> 4 files changed, 62 insertions(+), 27 deletions(-)
>>>
>>> ...
>>>
>>>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>>>> index a58f84b6bd5e..4c5b7bcdb25c 100644
>>>> --- a/include/drm/drm_plane.h
>>>> +++ b/include/drm/drm_plane.h
>>>> @@ -992,6 +992,35 @@ static inline struct drm_plane
>>>> *drm_plane_find(struct drm_device *dev,
>>>> #define drm_for_each_plane(plane, dev) \
>>>> list_for_each_entry(plane, &(dev)->mode_config.plane_list, head)
>>>> +/**
>>>> + * drm_plane_solid_fill_enabled - Check if solid fill is enabled on
>>>> plane
>>>> + * @state: plane state
>>>> + *
>>>> + * Returns:
>>>> + * Whether the plane has been assigned a solid_fill_blob
>>>> + */
>>>> +static inline bool drm_plane_solid_fill_enabled(struct
>>>> drm_plane_state *state)
>>>> +{
>>>> + if (!state)
>>>> + return false;
>>>> + return state->pixel_source == DRM_PLANE_PIXEL_SOURCE_SOLID_FILL
>>>> && state->solid_fill_blob;
>>>> +}
>>>> +
>>>> +static inline bool drm_plane_has_visible_data(const struct
>>>> drm_plane_state *state)
>>>> +{
>>>> + switch (state->pixel_source) {
>>>> + case DRM_PLANE_PIXEL_SOURCE_NONE:
>>>> + return false;
>>>> + case DRM_PLANE_PIXEL_SOURCE_SOLID_FILL:
>>>> + return state->solid_fill_blob != NULL;
>>>
>>> This reminds me, new UAPI docs did not say what the requirements are for
>>> choosing solid fill pixel source. Is the atomic commit rejected if
>>> pixel source is solid fill, but solid_fill property has no blob?
>>
>> Hi Pekka,
>>
>> Yes, if pixel_source is solid_fill and the solid_fill property blob
>> isn't set, the atomic commit should throw an error.
>>
>> Will document this in the UAPI.
>
> I don't see a corresponding error check in atomic_check() functions.
> Could you please check that there is one, as you are updating the uAPI.
Hi Dmitry,
Sorry for the late response.
drm_plane_has_visible_data() is being called from
drm_atomic_plane_check() which is called from drm_atomic_commit() (via
drm_atomic_check_only()).
It's also called within the atomic_check() callstack in
drm_atomic_helper_check_plane_state(), though that check will set
plane.visible to false and return 0.
Would it be better to have a more explicit `if (source == solid_fill &&
!plane->solid_fill_blob) then return -EINVAL` check in atomic_check()?
Thanks,
Jessica Zhang
>
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>> This should be doc'd.
>>>
>>>
>>> Thanks,
>>> pq
>>>
>>>> + case DRM_PLANE_PIXEL_SOURCE_FB:
>>>> + default:
>>>> + WARN_ON(state->pixel_source != DRM_PLANE_PIXEL_SOURCE_FB);
>>>> + }
>>>> +
>>>> + return state->fb != NULL;
>>>> +}
>>>> +
>>>> bool drm_any_plane_has_format(struct drm_device *dev,
>>>> u32 format, u64 modifier);
>>>>
>>>
>
> --
> With best wishes
> Dmitry
>
Powered by blists - more mailing lists