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

[Helen Koike <helen.koike@collabora.com>]

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@collabora.com>
> 
> ---
> 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:
>  	 *
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel