Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Message-ID: <26585954-7c86-45fd-9190-f1109cbe9901@quicinc.com>
Date:   Mon, 16 Oct 2023 17:40:53 -0700
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [Freedreno] [PATCH RFC v6 07/10] drm/atomic: Loosen FB atomic
 checks
To:     Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Pekka Paalanen <ppaalanen@gmail.com>
CC:     <linux-arm-msm@vger.kernel.org>, <freedreno@lists.freedesktop.org>,
        <sebastian.wick@redhat.com>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        "Sean Paul" <sean@poorly.run>, <dri-devel@lists.freedesktop.org>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        <quic_abhinavk@quicinc.com>, "Maxime Ripard" <mripard@kernel.org>,
        <linux-kernel@vger.kernel.org>, Rob Clark <robdclark@gmail.com>,
        <laurent.pinchart@ideasonboard.com>,
        Daniel Vetter <daniel@ffwll.ch>, <contact@emersion.fr>,
        Marijn Suijten <marijn.suijten@somainline.org>,
        <wayland-devel@lists.freedesktop.org>,
        "David Airlie" <airlied@gmail.com>, <ville.syrjala@linux.intel.com>
References: <20230828-solid-fill-v6-0-a820efcce852@quicinc.com>
 <20230828-solid-fill-v6-7-a820efcce852@quicinc.com>
 <20230829112230.7106a8bf@eldfell>
 <752176d8-23f4-4689-8bf4-db27f153fd39@quicinc.com>
 <6851b864-447f-453f-8b34-1fbb6e97eefe@linaro.org>
Content-Language: en-US
From:   Jessica Zhang <quic_jesszhan@quicinc.com>
In-Reply-To: <6851b864-447f-453f-8b34-1fbb6e97eefe@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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