| Message ID | 20240618030024.500532-3-andrealmeid@igalia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm: Support per-plane async flip configuration | expand |
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)
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
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. */
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. > */
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 --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)
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(+)