Re: [PATCH RFC v3 4/4] drm/atomic: hook atomic_async_{check,update} with PAGE_FLIP_ASYNC flag

From: Helen Koike
Date: Fri Apr 12 2019 - 09:40:05 EST

Next message: Heikki Krogerus: "[PATCH v3 01/13] software node: Allow node creation without properties"
Previous message: Miroslav Benes: "Re: [PATCH] selftests: livepatch use TEST_PROGS for test shell scripts"
In reply to: Helen Koike: "[PATCH RFC v3 4/4] drm/atomic: hook atomic_async_{check,update} with PAGE_FLIP_ASYNC flag"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 4/12/19 9:58 AM, Helen Koike wrote:
> Add atomic_async_{check,update} hooks in drm_plane_helper_funcs.
> These hooks are called when userspace requests asyncronous page flip in
> the atomic api through the flag DRM_MODE_PAGE_FLIP_ASYNC.
>
> Update those hooks in the drivers that implement async functions, except
> for amdgpu who handles the flag in the normal path, and rockchip who
> doesn't support async page flip.
>
> Signed-off-by: Helen Koike <helen.koike@xxxxxxxxxxxxx>
>
> ---
> Hi,
>
> This patch is an attempt to expose the implementation that already exist
> for true async page flips updates through atomic api when the
> DRM_MODE_PAGE_FLIP_ASYNC is used.
>
> In this commit I'm re-introducing the atomic_async_{check,update} hooks.
> I know it is a bit weird to remove them first and them re-add them, but
> I did this in the last commit to avoid any state of inconsistency
> between commits, as rockchip doesn't support async page flip and they were
> being used as amend.
> So I reverted to amend first and then re-introced async again
> tied to the DRM_MODE_PAGE_FLIP_ASYNC flag (I also think this is easier
> to read).
>
> To use async, it is required to have
> dev->mode_config.async_page_flip = true;
> but I see that only amdgpu and vc4 have this, so this patch won't take
> effect on mdp5.
> Nouveau and radeon also have this flag, but they don't implement the
> async hooks yet.
>
> Please let me know what you think.
>
> Thanks
> Helen
>
> Changes in v3: None
> Changes in v2: None
> Changes in v1: None
>
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++
> drivers/gpu/drm/drm_atomic_helper.c | 31 ++++++++++++----
> drivers/gpu/drm/drm_atomic_uapi.c | 3 +-
> drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 2 +
> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 4 ++
> drivers/gpu/drm/vc4/vc4_plane.c | 2 +
> include/drm/drm_atomic.h | 2 +
> include/drm/drm_atomic_helper.h | 9 +++--
> include/drm/drm_modeset_helper_vtables.h | 37 +++++++++++++++++++
> 9 files changed, 83 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 711e7715e112..bb8a5f1997d6 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3785,6 +3785,11 @@ static const struct drm_plane_helper_funcs dm_plane_helper_funcs = {
> */
> .atomic_amend_check = dm_plane_atomic_async_check,
> .atomic_amend_update = dm_plane_atomic_async_update
> + /*
> + * Note: amdgpu takes care of DRM_MODE_PAGE_FLIP_ASYNC flag in the
> + * normal commit path, thus .atomic_async_check and .atomic_async_update
> + * are not provided here.
> + */
> };
>
> /*
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 9b0df08836c3..bfcf88359de5 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -947,16 +947,31 @@ int drm_atomic_helper_check(struct drm_device *dev,
> if (ret)
> return ret;
>
> + /*
> + * If async page flip was explicitly requested, but it is not possible,
> + * return error instead of falling back to a normal commit.
> + * If async_amend_check returns -EOPNOTSUPP, it means
> + * ->atomic_async_update hook doesn't exist, so fallback to normal
> + * commit and let the driver handle DRM_MODE_PAGE_FLIP_ASYNC flag
> + * through normal commit code path.
> + */
> + if (state->async_update) {
> + ret = drm_atomic_helper_async_amend_check(dev, state, false);
> + state->async_update = !ret;
> + return !ret || ret == -EOPNOTSUPP ? 0 : ret;
> + }
> +
> /*
> * If amend was explicitly requested, but it is not possible,
> * return error instead of falling back to a normal commit.
> */
> if (state->amend_update)
> - return drm_atomic_helper_amend_check(dev, state);
> + return drm_atomic_helper_async_amend_check(dev, state, true);
>
> /* Legacy mode falls back to a normal commit if amend isn't possible. */
> if (state->legacy_cursor_update)
> - state->amend_update = !drm_atomic_helper_amend_check(dev, state);
> + state->amend_update =
> + !drm_atomic_helper_async_amend_check(dev, state, true);
>
> return 0;
> }
> @@ -1651,8 +1666,9 @@ static void commit_work(struct work_struct *work)
> * if not. Note that error just mean it can't be committed in amend mode, if it
> * fails the commit should be treated like a normal commit.
> */
> -int drm_atomic_helper_amend_check(struct drm_device *dev,
> - struct drm_atomic_state *state)
> +int drm_atomic_helper_async_amend_check(struct drm_device *dev,
> + struct drm_atomic_state *state,
> + bool amend)
> {
> struct drm_crtc *crtc;
> struct drm_crtc_state *crtc_state;
> @@ -1695,7 +1711,7 @@ int drm_atomic_helper_amend_check(struct drm_device *dev,
> return -EINVAL;
>

Sorry, I forgot a modif here:

- if (!funcs->atomic_amend_update)
- return -EINVAL;
+ if ((amend && !funcs->atomic_amend_update) ||
+ !funcs->atomic_async_update)
+ return -EOPNOTSUPP;

I need to return -EOPNOTSUPP so I can know if async should fallback to
the normal async path, this is required for amdgpu as it handles the
PAGE_FLIP_ASYNC flag it self.

I'll correct this in the next version after your comments.

You can also see the series on
https://gitlab.collabora.com/koike/linux/commits/drm/amend/uapi

Thanks

> /* Only allow amend update for cursor type planes. */
> - if (plane->type != DRM_PLANE_TYPE_CURSOR)
> + if (amend && plane->type != DRM_PLANE_TYPE_CURSOR)
> return -EINVAL;
>
> /*
> @@ -1707,9 +1723,10 @@ int drm_atomic_helper_amend_check(struct drm_device *dev,
> !try_wait_for_completion(&old_plane_state->commit->hw_done))
> return -EBUSY;
>
> - return funcs->atomic_amend_check(plane, new_plane_state);
> + return amend ? funcs->atomic_amend_check(plane, new_plane_state) :
> + funcs->atomic_async_check(plane, new_plane_state);
> }
> -EXPORT_SYMBOL(drm_atomic_helper_amend_check);
> +EXPORT_SYMBOL(drm_atomic_helper_async_amend_check);
>
> /**
> * drm_atomic_helper_amend_commit - commit state in amend mode
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index d1962cdea602..1d9a6142218e 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -1312,8 +1312,9 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
> state->acquire_ctx = &ctx;
> state->allow_modeset = !!(arg->flags & DRM_MODE_ATOMIC_ALLOW_MODESET);
> + state->async_update = !!(arg->flags & DRM_MODE_PAGE_FLIP_ASYNC);
> /* async takes precedence over amend */
> - state->amend_update = arg->flags & DRM_MODE_PAGE_FLIP_ASYNC ? 0 :
> + state->amend_update = state->async_update ? 0 :
> !!(arg->flags & DRM_MODE_ATOMIC_AMEND);
>
> retry:
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> index 814e8230cec6..e3b2a2c74852 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c
> @@ -531,6 +531,8 @@ static const struct drm_plane_helper_funcs mdp5_plane_helper_funcs = {
> .cleanup_fb = mdp5_plane_cleanup_fb,
> .atomic_check = mdp5_plane_atomic_check,
> .atomic_update = mdp5_plane_atomic_update,
> + .atomic_async_check = mdp5_plane_atomic_async_check,
> + .atomic_async_update = mdp5_plane_atomic_async_update,
> /*
> * FIXME: ideally, instead of hooking async updates to amend,
> * we could avoid tearing by modifying the pending commit.
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 216ad76118dc..c2f7201e52a9 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -954,6 +954,10 @@ static const struct drm_plane_helper_funcs plane_helper_funcs = {
> .atomic_disable = vop_plane_atomic_disable,
> .atomic_amend_check = vop_plane_atomic_amend_check,
> .atomic_amend_update = vop_plane_atomic_amend_update,
> + /*
> + * Note: rockchip doesn't support async page flip, thus
> + * .atomic_async_update and .atomic_async_check are not provided.
> + */
> .prepare_fb = drm_gem_fb_prepare_fb,
> };
>
> diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> index ea560000222d..24a9befe89e6 100644
> --- a/drivers/gpu/drm/vc4/vc4_plane.c
> +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> @@ -1175,6 +1175,8 @@ static const struct drm_plane_helper_funcs vc4_plane_helper_funcs = {
> */
> .atomic_amend_check = vc4_plane_atomic_async_check,
> .atomic_amend_update = vc4_plane_atomic_async_update,
> + .atomic_async_check = vc4_plane_atomic_async_check,
> + .atomic_async_update = vc4_plane_atomic_async_update,
> };
>
> static void vc4_plane_destroy(struct drm_plane *plane)
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index b1ced069ffc1..05756a09e762 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -300,6 +300,7 @@ struct __drm_private_objs_state {
> * @ref: count of all references to this state (will not be freed until zero)
> * @dev: parent DRM device
> * @legacy_cursor_update: hint to enforce legacy cursor IOCTL semantics
> + * @async_update: hint for asyncronous page flip update
> * @amend_update: hint for amend plane update
> * @planes: pointer to array of structures with per-plane data
> * @crtcs: pointer to array of CRTC pointers
> @@ -328,6 +329,7 @@ struct drm_atomic_state {
> */
> bool allow_modeset : 1;
> bool legacy_cursor_update : 1;
> + bool async_update : 1;
> bool amend_update : 1;
> /**
> * @duplicated:
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 8ce0594ccfb9..39e57d559a30 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -55,10 +55,11 @@ void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *state);
> int drm_atomic_helper_commit(struct drm_device *dev,
> struct drm_atomic_state *state,
> bool nonblock);
> -int drm_atomic_helper_amend_check(struct drm_device *dev,
> - struct drm_atomic_state *state);
> -void drm_atomic_helper_amend_commit(struct drm_device *dev,
> - struct drm_atomic_state *state);
> +int drm_atomic_helper_async_amend_check(struct drm_device *dev,
> + struct drm_atomic_state *state,
> + bool amend);
> +void drm_atomic_helper_async_amend_commit(struct drm_device *dev,
> + struct drm_atomic_state *state);
>
> int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
> struct drm_atomic_state *state,
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index d92e62dd76c4..c2863d62f160 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -1135,6 +1135,43 @@ struct drm_plane_helper_funcs {
> void (*atomic_disable)(struct drm_plane *plane,
> struct drm_plane_state *old_state);
>
> + /**
> + * @atomic_async_check:
> + *
> + * Drivers should set this function pointer to check if a page flip can
> + * be performed asynchronously, i.e., immediately without waiting for a
> + * vblank.
> + *
> + * This hook is called by drm_atomic_async_check() to establish if a
> + * given update can be committed in async mode, that is, if it can
> + * jump ahead of the state currently queued for update.
> + *
> + * RETURNS:
> + *
> + * Return 0 on success and any error returned indicates that the update
> + * can not be applied in asynd mode.
> + */
> + int (*atomic_async_check)(struct drm_plane *plane,
> + struct drm_plane_state *state);
> +
> + /**
> + * @atomic_async_update:
> + *
> + * Drivers should set this function pointer to perform asynchronous page
> + * flips through the atomic api, i.e. not vblank synchronized.
> + *
> + * Note that unlike &drm_plane_helper_funcs.atomic_update this hook
> + * takes the new &drm_plane_state as parameter. When doing async_update
> + * drivers shouldn't replace the &drm_plane_state but update the
> + * current one with the new plane configurations in the new
> + * plane_state.
> + *
> + * FIXME:
> + * - It only works for single plane updates
> + */
> + void (*atomic_async_update)(struct drm_plane *plane,
> + struct drm_plane_state *new_state);
> +
> /**
> * @atomic_amend_check:
> *
>

Next message: Heikki Krogerus: "[PATCH v3 01/13] software node: Allow node creation without properties"
Previous message: Miroslav Benes: "Re: [PATCH] selftests: livepatch use TEST_PROGS for test shell scripts"
In reply to: Helen Koike: "[PATCH RFC v3 4/4] drm/atomic: hook atomic_async_{check,update} with PAGE_FLIP_ASYNC flag"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]