Dmitry Baryshkov
2024-Jun-18 17:43 UTC
[PATCH v7 2/9] drm: Support per-plane async flip configuration
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.> /** > * @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. > */-- With best wishes Dmitry