diff mbox series

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

Message ID 20240618030024.500532-3-andrealmeid@igalia.com (mailing list archive)
State New, archived
Headers show
Series drm: Support per-plane async flip configuration | expand

Commit Message

André Almeida June 18, 2024, 3 a.m. UTC
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@igalia.com>
---
 include/drm/drm_plane.h | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Jani Nikula June 18, 2024, 9:38 a.m. UTC | #1
On Tue, 18 Jun 2024, André Almeida <andrealmeid@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@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.

BR,
Jani.


> +	bool async_flip;
>  };
>  
>  #define obj_to_plane(x) container_of(x, struct drm_plane, base)
Dmitry Baryshkov June 18, 2024, 10:07 a.m. UTC | #2
On Tue, 18 Jun 2024 at 12:38, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Tue, 18 Jun 2024, André Almeida <andrealmeid@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@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().

> > +     bool async_flip;
> >  };
> >
> >  #define obj_to_plane(x) container_of(x, struct drm_plane, base)
>
> --
> Jani Nikula, Intel
André Almeida June 18, 2024, 4:18 p.m. UTC | #3
Em 18/06/2024 07:07, Dmitry Baryshkov escreveu:
> On Tue, 18 Jun 2024 at 12:38, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>
>> On Tue, 18 Jun 2024, André Almeida <andrealmeid@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@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:

/**
  * @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.
  */
Dmitry Baryshkov June 18, 2024, 5:43 p.m. UTC | #4
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@linux.intel.com> wrote:
> > > 
> > > On Tue, 18 Jun 2024, André Almeida <andrealmeid@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@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.


> /**
>  * @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.
>  */
André Almeida June 18, 2024, 8:45 p.m. UTC | #5
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@linux.intel.com> wrote:
>>>>
>>>> On Tue, 18 Jun 2024, André Almeida <andrealmeid@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@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.
>>   */
>
diff mbox series

Patch

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
+	 */
+	bool async_flip;
 };
 
 #define obj_to_plane(x) container_of(x, struct drm_plane, base)