[Freedreno] [PATCH RFC v6 07/10] drm/atomic: Loosen FB atomic checks

[Jessica Zhang]

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