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@xxxxxxxxxxxxxxx> wrote:
On Tue, 18 Jun 2024, André Almeida <andrealmeid@xxxxxxxxxx> 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@xxxxxxxxxx>
---
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.
*/
