[PATCH v7 2/9] drm: Support per-plane async flip configuration

[André Almeida]

Em 18/06/2024 14:43, Dmitry Baryshkov escreveu:
> On Tue, Jun 18, 2024 at 01:18:10PM GMT, André Almeida wrote:
>> Em 18/06/2024 07:07, Dmitry Baryshkov escreveu:
>>> On Tue, 18 Jun 2024 at 12:38, Jani Nikula <jani.nikula at linux.intel.com> wrote:
>>>>
>>>> On Tue, 18 Jun 2024, André Almeida <andrealmeid at igalia.com> wrote:
>>>>> Drivers have different capabilities on what plane types they can or
>>>>> cannot perform async flips. Create a plane::async_flip field so each
>>>>> driver can choose which planes they allow doing async flips.
>>>>>
>>>>> Signed-off-by: André Almeida <andrealmeid at igalia.com>
>>>>> ---
>>>>>    include/drm/drm_plane.h | 5 +++++
>>>>>    1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>>>>> index 9507542121fa..0bebc72af5c3 100644
>>>>> --- a/include/drm/drm_plane.h
>>>>> +++ b/include/drm/drm_plane.h
>>>>> @@ -786,6 +786,11 @@ struct drm_plane {
>>>>>          * @kmsg_panic: Used to register a panic notifier for this plane
>>>>>          */
>>>>>         struct kmsg_dumper kmsg_panic;
>>>>> +
>>>>> +     /**
>>>>> +      * @async_flip: indicates if a plane can do async flips
>>>>> +      */
>>>>
>>>> When is it okay to set or change the value of this member?
>>>>
>>>> If you don't document it, people will find creative uses for this.
>>>
>>> Maybe it's better to have a callback instead of a static field? This
>>> way it becomes clear that it's only relevant at the time of the
>>> atomic_check().
>>>
>>
>> So we would have something like bool (*async_flip) for struct
>> drm_plane_funcs I suppose. Then each driver will implement this function and
>> check on runtime if it should flip or not, right?
>>
>> I agree that it makes more clear, but as far as I can see this is not
>> something that is subject to being changed at runtime at all, so it seems a
>> bit overkill to me to encapsulate a static information like that. I prefer
>> to improve the documentation on the struct member to see if this solves the
>> problem. What do you think of the following comment:
> 
> It looks like I keep on mixing async_flips as handled via the
> DRM_MODE_PAGE_FLIP_ASYNC and the plane flips that are governed by
> .atomic_async_check / .atomic_async_update / drm_atomic_helper_check()
> and which end up being used just for legacy cursor updates.
> 
> So, yes, those are two different code paths, but with your changes I
> think it becomes even easier to get confused between
> atomic_async_check() and .async_flip member.
> 

I see, now that I read about atomic_async_check(), it got me confused as 
well :)

I see that drivers define atomic_async_check() to tell DRM whether or 
not such plane is able to do async flips... just like I'm trying to do 
here. amdgpu implementation for that function is almost the opposite of 
the restrictions that I've implemented in this patchset:

int amdgpu_dm_plane_atomic_async_check(...) {
	/* Only support async updates on cursor planes. */
	if (plane->type != DRM_PLANE_TYPE_CURSOR)
		return -EINVAL;

	return 0;
}

Anyway, I'll try to see if the legacy cursor path might be incorporated 
somehow in the DRM_MODE_PAGE_FLIP_ASYNC path, or to come up with 
something that makes them more distinguishable.

Thanks!

> 
>> /**
>>   * @async_flip: indicates if a plane can perform async flips. The
>>   * driver should set this true only for planes that the hardware
>>   * supports flipping asynchronously. It may not be changed during
>>   * runtime. This field is checked inside drm_mode_atomic_ioctl() to
>>   * allow only the correct planes to go with DRM_MODE_PAGE_FLIP_ASYNC.
>>   */
>