[PATCH] drm/atomic: Add target_vblank support in atomic helpers.

[Daniel Vetter]

On Tue, Jan 03, 2017 at 10:06:38AM -0500, Andrey Grodzovsky wrote:
> Allows usage of the new page_flip_target hook for
> drivers implementing the atomic path.
> Provides default atomic helper for the new hook.
> 
> Signed-off-by: Andrey Grodzovsky <Andrey.Grodzovsky at amd.com>

Please keep a per-patch changelog so that it's easier for everyone to
follow the evolution of this patch.

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 146 ++++++++++++++++++++++++++++--------
>  include/drm/drm_atomic_helper.h     |   6 ++
>  include/drm/drm_crtc.h              |   3 +
>  3 files changed, 125 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 583f47f..5e76f90 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2733,6 +2733,59 @@ int drm_atomic_helper_resume(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);
>  
> +static int drm_atomic_helper_page_flip_create_state(

I'd just call this page_flip_common without the long namespace prefix
since it's static. And without the create_state name since this function
also does some basic checks.

> +				struct drm_atomic_state *state,
> +				struct drm_crtc *crtc,
> +				struct drm_framebuffer *fb,
> +				struct drm_pending_vblank_event *event)
> +{
> +	struct drm_plane *plane = crtc->primary;
> +	struct drm_plane_state *plane_state;
> +	struct drm_crtc_state *crtc_state;
> +	int ret = 0;
> +
> +	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +	if (IS_ERR(crtc_state))
> +		return PTR_ERR(crtc_state);
> +
> +	crtc_state->event = event;
> +
> +	plane_state = drm_atomic_get_plane_state(state, plane);
> +	if (IS_ERR(plane_state))
> +		return PTR_ERR(plane_state);
> +
> +
> +	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
> +	if (ret != 0)
> +		return ret;
> +	drm_atomic_set_fb_for_plane(plane_state, fb);
> +
> +	/* Make sure we don't accidentally do a full modeset. */
> +	state->allow_modeset = false;
> +	if (!crtc_state->active) {
> +		DRM_DEBUG_ATOMIC("[CRTC:%d] disabled, rejecting legacy flip\n",
> +				 crtc->base.id);
> +		return -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +static void drm_atomic_helper_page_flip_prepare_retry(
> +				struct drm_atomic_state *state,
> +				struct drm_plane *plane)
> +{
> +	drm_atomic_state_clear(state);
> +	drm_atomic_legacy_backoff(state);
> +
> +	/*
> +	 * Someone might have exchanged the framebuffer while we dropped locks
> +	 * in the backoff code. We need to fix up the fb refcount tracking the
> +	 * core does for us.
> +	 */
> +	plane->old_fb = plane->fb;

There's a lot more places where we do this trick, please either refactor
them all, or none. Personally I'd go with none since the function call is
more verbose than the assignement, and you're hiding this rather important
comment behind a function name that doesn't say what games are being
played here.

> +}
> +
>  /**
>   * drm_atomic_helper_page_flip - execute a legacy page flip
>   * @crtc: DRM crtc
> @@ -2756,8 +2809,6 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
>  {
>  	struct drm_plane *plane = crtc->primary;
>  	struct drm_atomic_state *state;
> -	struct drm_plane_state *plane_state;
> -	struct drm_crtc_state *crtc_state;
>  	int ret = 0;
>  
>  	if (flags & DRM_MODE_PAGE_FLIP_ASYNC)
> @@ -2768,35 +2819,79 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
>  		return -ENOMEM;
>  
>  	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
> +
>  retry:
> -	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> -	if (IS_ERR(crtc_state)) {
> -		ret = PTR_ERR(crtc_state);
> +	ret = drm_atomic_helper_page_flip_create_state(state, crtc, fb, event);
> +	if (ret != 0)
>  		goto fail;
> -	}
> -	crtc_state->event = event;
>  
> -	plane_state = drm_atomic_get_plane_state(state, plane);
> -	if (IS_ERR(plane_state)) {
> -		ret = PTR_ERR(plane_state);
> -		goto fail;
> -	}
> +	ret = drm_atomic_nonblocking_commit(state);
>  
> -	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
> +fail:
> +	if (ret == -EDEADLK)
> +		goto backoff;
> +
> +	drm_atomic_state_put(state);
> +	return ret;
> +
> +backoff:
> +	drm_atomic_helper_page_flip_prepare_retry(state, plane);
> +	goto retry;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_page_flip);
> +
> +/**
> + * drm_atomic_helper_page_flip - execute a legacy page flip
> + * @crtc: DRM crtc
> + * @fb: DRM framebuffer
> + * @event: optional DRM event to signal upon completion
> + * @flags: flip flags for non-vblank sync'ed updates
> + *
> + * Provides a default page flip on specific vblank implementation using
> + * the atomic driver interface.
> + *
> + * Note that for now so called async page flips (i.e. updates which are not
> + * synchronized to vblank) are not supported, since the atomic interfaces have
> + * no provisions for this yet.
> + *
> + * Returns:
> + * Returns 0 on success, negative errno numbers on failure.

Kerneldoc isn't updated, just verbatim copypaste. Please also linke these
2 functions. And if you have time, would be good to sprinkle links to the
vfunc hooks for each of these default helpers (they're unfortunately
missing ...), e.g. here &drm_crtc_funcs.page_flip_target.

And please build the docs per http://blog.ffwll.ch/2016/12/howto-docs.html
and make sure it all looks pretty.

> + */
> +int drm_atomic_helper_page_flip_on_target_vblank(

Please name this helper to match the vhook it's supposed to slot into,
i.e. drm_atomic_helper_page_flip_target.

> +				struct drm_crtc *crtc,
> +				struct drm_framebuffer *fb,
> +				struct drm_pending_vblank_event *event,
> +				uint32_t flags,
> +				uint32_t target)
> +{
> +	struct drm_plane *plane = crtc->primary;
> +	struct drm_atomic_state *state;
> +	struct drm_crtc_state *crtc_state;
> +	int ret = 0;
> +
> +	if (flags & DRM_MODE_PAGE_FLIP_ASYNC)
> +		return -EINVAL;
> +
> +	state = drm_atomic_state_alloc(plane->dev);
> +	if (!state)
> +		return -ENOMEM;
> +
> +	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
> +
> +retry:
> +	ret = drm_atomic_helper_page_flip_create_state(state, crtc, fb, event);
>  	if (ret != 0)
>  		goto fail;
> -	drm_atomic_set_fb_for_plane(plane_state, fb);
>  
> -	/* Make sure we don't accidentally do a full modeset. */
> -	state->allow_modeset = false;
> -	if (!crtc_state->active) {
> -		DRM_DEBUG_ATOMIC("[CRTC:%d] disabled, rejecting legacy flip\n",
> -				 crtc->base.id);
> +	crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
> +	if (WARN_ON(!crtc_state)) {
>  		ret = -EINVAL;
>  		goto fail;
>  	}
> +	crtc_state->target_vblank = target;
>  
>  	ret = drm_atomic_nonblocking_commit(state);
> +
>  fail:
>  	if (ret == -EDEADLK)
>  		goto backoff;
> @@ -2805,19 +2900,10 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
>  	return ret;
>  
>  backoff:
> -	drm_atomic_state_clear(state);
> -	drm_atomic_legacy_backoff(state);
> -
> -	/*
> -	 * Someone might have exchanged the framebuffer while we dropped locks
> -	 * in the backoff code. We need to fix up the fb refcount tracking the
> -	 * core does for us.
> -	 */
> -	plane->old_fb = plane->fb;
> -
> +	drm_atomic_helper_page_flip_prepare_retry(state, plane);
>  	goto retry;
>  }
> -EXPORT_SYMBOL(drm_atomic_helper_page_flip);
> +EXPORT_SYMBOL(drm_atomic_helper_page_flip_on_target_vblank);
>  
>  /**
>   * drm_atomic_helper_connector_dpms() - connector dpms helper implementation
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 7ff92b0..e8cb6b7 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -124,6 +124,12 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
>  				struct drm_framebuffer *fb,
>  				struct drm_pending_vblank_event *event,
>  				uint32_t flags);
> +int drm_atomic_helper_page_flip_on_target_vblank(
> +				struct drm_crtc *crtc,
> +				struct drm_framebuffer *fb,
> +				struct drm_pending_vblank_event *event,
> +				uint32_t flags,
> +				uint32_t target);
>  int drm_atomic_helper_connector_dpms(struct drm_connector *connector,
>  				     int mode);
>  struct drm_encoder *
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 946672f..cc926dc 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -103,6 +103,7 @@ static inline uint64_t I642U64(int64_t val)
>   * @ctm: Transformation matrix
>   * @gamma_lut: Lookup table for converting pixel data after the
>   *	conversion matrix
> + * @target_vblank: Target vblank count to flip
>   * @state: backpointer to global drm_atomic_state
>   *
>   * Note that the distinction between @enable and @active is rather subtile:
> @@ -156,6 +157,8 @@ struct drm_crtc_state {
>  	struct drm_property_blob *ctm;
>  	struct drm_property_blob *gamma_lut;
>  
> +	u32 target_vblank;

The in-line style is preferred for structures, since it allows to group
comments with each member. There's also still the question of how drivers
opt-in into target_vblank (only amdgpu, and then only in your private
branch supports it). I think that should be documented in the kernel-doc.

Thanks, Daniel
> +
>  	/**
>  	 * @event:
>  	 *
> -- 
> 1.9.1
> 

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