lkml.org 
[lkml]   [2018]   [Aug]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] drm/atomic: add ATOMIC_AMEND flag to the Atomic IOCTL.
On Mon, Aug 06, 2018 at 06:01:02PM +0200, Enric Balletbo i Serra wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
>
> This flag tells core to jump ahead the queued update if the conditions
> in drm_atomic_async_check() are met. That means we are only able to do an
> async update if no modeset is pending and update for the same plane is
> not queued.
>
> It uses the already in place infrastructure for async updates.
>
> It is useful for cursor updates and async PageFlips over the atomic
> ioctl, otherwise in some cases updates may be delayed to the point the
> user will notice it. Note that for now it's only enabled for cursor
> planes.
>
> DRM_MODE_ATOMIC_AMEND should be passed to the Atomic IOCTL to use this
> feature.
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> Hi,
>
> This is an attempt to introduce the new ATOMIC_AMEND flag for atomic
> operations, see the commit message for a more detailed description.
>
> This was tested using a small program that exercises the uAPI for easy
> sanity testing. The program created by Alexandros can be found here
> [2]. To test, just build the program and use the --atomic flag to use the
> cursor plane in normal (blocking mode), and --atomic-async to use the cursor
> plane with the ATOMIC_AMEND flag. E.g.
>
> drm_cursor --atomic
>
> or
>
> drm_cursor --atomic-async

Would be really lovely to bake these into igt-based regression tests. i915
doesn't yet have async update support, but I hope we'll eventually gain
that, and then our CI could make sure this keeps working.

> The test worked on a Samsung Chromebook Plus on top of mainline plus
> the patch to update cursors asynchronously through atomic for the
> drm/rockchip driver.
>
> Alexandros also did a proof-of-concept to use this flag and draw cursors
> using atomic if possible on ozone [1].
>
> Best regards,
> Enric
>
> [1] https://chromium-review.googlesource.com/c/chromium/src/+/1092711
> [2] https://gitlab.collabora.com/alf/drm-cursor
>
>
> Changes in v1:
> - Only enable it if userspace requests it.
> - Only allow async update for cursor type planes.
> - Rename ASYNC_UPDATE for ATOMIC_AMEND.
>
> drivers/gpu/drm/drm_atomic.c | 5 +++++
> drivers/gpu/drm/drm_atomic_helper.c | 10 +++++++++-
> include/uapi/drm/drm_mode.h | 4 +++-
> 3 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 895741e9cd7d..7b3e4aa2874d 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -2338,6 +2338,10 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> (arg->flags & DRM_MODE_PAGE_FLIP_EVENT))
> return -EINVAL;
>
> + if ((arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET) &&
> + (arg->flags & DRM_MODE_ATOMIC_AMEND))
> + return -EINVAL;
> +
> drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
>
> state = drm_atomic_state_alloc(dev);
> @@ -2346,6 +2350,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>
> state->acquire_ctx = &ctx;
> state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET);
> + state->async_update = !!(arg->flags & DRM_MODE_ATOMIC_AMEND);
>
> retry:
> plane_mask = 0;
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 81e32199d3ef..59495f61c583 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -902,7 +902,7 @@ int drm_atomic_helper_check(struct drm_device *dev,
> if (ret)
> return ret;
>
> - if (state->legacy_cursor_update)
> + if (state->async_update || state->legacy_cursor_update)

Hm, it would be really cool if we could rip out the legacy_cursor_update
hack and also use async_update in the cursor compat helpers. At least if
available.

> state->async_update = !drm_atomic_helper_async_check(dev, state);
>
> return ret;
> @@ -1539,6 +1539,14 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
> if (new_plane_state->fence)
> return -EINVAL;
>
> + /* Only allow async update for cursor type planes. */
> + if (plane->type != DRM_PLANE_TYPE_CURSOR)
> + return -EINVAL;

Why limit to cursor planes? The async_update hooks are per-plane.

> +
> + /* Don't do an async update if there isn't a pending commit. */
> + if (!old_plane_state->commit)
> + return -EINVAL;

Hm, why this? Won't this break legacy cursor updates now for drivers which
implement this using async_update?

If userspace wants to avoid this, they can do so by properly tracking
their non-AMEND updates and making sure they've waited until those all
completed.

Also, how is userspace supposed to figure out async_update exists/works?
Usual approach is a getparam.

Another semantics questions: Should this fail if we fall back to a
non-async update? I think that would be best, since userspace can the
decide on its own whether it wants to fall back to a normal commit or do
something else. And the cursor compat code could do the same, i.e. try
first with AMEND, then a normal commit?

Cheers, Daniel

> +
> /*
> * Don't do an async update if there is an outstanding commit modifying
> * the plane. This prevents our async update's changes from getting
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 4b3a1bb58e68..6dae18428123 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -724,13 +724,15 @@ struct drm_mode_destroy_dumb {
> #define DRM_MODE_ATOMIC_TEST_ONLY 0x0100
> #define DRM_MODE_ATOMIC_NONBLOCK 0x0200
> #define DRM_MODE_ATOMIC_ALLOW_MODESET 0x0400
> +#define DRM_MODE_ATOMIC_AMEND 0x0800
>
> #define DRM_MODE_ATOMIC_FLAGS (\
> DRM_MODE_PAGE_FLIP_EVENT |\
> DRM_MODE_PAGE_FLIP_ASYNC |\
> DRM_MODE_ATOMIC_TEST_ONLY |\
> DRM_MODE_ATOMIC_NONBLOCK |\
> - DRM_MODE_ATOMIC_ALLOW_MODESET)
> + DRM_MODE_ATOMIC_ALLOW_MODESET |\
> + DRM_MODE_ATOMIC_AMEND)
>
> struct drm_mode_atomic {
> __u32 flags;
> --
> 2.18.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

\
 
 \ /
  Last update: 2018-08-07 18:43    [W:0.099 / U:0.188 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site