Re: [PATCH] drm/atomic-helper: implement ->page_flip

[Daniel Thompson <daniel.thompson@linaro.org>]

On 04/11/14 22:09, Daniel Vetter wrote:
> Currently there is no way to implement async flips using atomic, that
> essentially requires us to be able to cancel pending requests
> mid-flight.
> 
> To be able to do that (and I guess we want this since vblank synced
> updates whic opportunistically cancel still pending updates seem to be
> wanted) we'd need to add a mandatory cancellation mode. Depending upon
> the exact semantics we decide upon that could mean that userspace will
> not get completion events, or will get them all stacked up.
> 
> So reject async updates for now. Also async updates usually means not
> vblank synced at all, and I guess for drivers which want to support
> this they should simply add a special pageflip handler (since usually
> you need a special flip cmd to achieve this). That kind of async flip
> is pretty much exclusively just used for games and benchmarks where
> dropping just one frame means you'll get a headshot or something bad
> like that ... And so slight amounts of tearing is acceptable.
> 
> v2: Fixup kerneldoc, reported by Paulo.
> 
> v3: Use the set_crtc_for_plane function to assign the crtc, since
> otherwise the book-keeping is off.
> 
> v4: Update crtc->primary->fb since ->page_flip is the only driver
> callback where the core won't do this itself. We might want to fix
> this inconsistency eventually.
> 
> v5: Use set_crtc_for_connector as suggested by Sean.
> 
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: Paulo Zanoni <przanoni@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 92 +++++++++++++++++++++++++++++++++++++
>  include/drm/drm_atomic_helper.h     |  5 ++
>  2 files changed, 97 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 790edc18b2cb..36ccc42d43db 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1633,3 +1633,95 @@ backoff:
>  	goto retry;
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_connector_set_property);
> +
> +/**
> + * 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 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.
> + */
> +int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
> +				struct drm_framebuffer *fb,
> +				struct drm_pending_vblank_event *event,
> +				uint32_t flags)
> +{
> +	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)
> +		return -EINVAL;
> +
> +	state = drm_atomic_state_alloc(plane->dev);
> +	if (!state)
> +		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);
> +		if (ret == -EDEADLK)
> +			goto backoff;
> +		else
> +			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);
> +		if (ret == -EDEADLK)
> +			goto backoff;
> +		else
> +			goto fail;
> +	}
> +
> +	ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
> +	if (ret == -EDEADLK)
> +		goto backoff;

The kerneldoc for this function states that it can return -ENOMEM too
and, even if it didn't, this approach to error handling is potentially
fragile as the kernel changes.


> +	plane_state->fb = fb;
> +
> +	ret = drm_atomic_async_commit(state);
> +	if (ret == -EDEADLK)
> +		goto backoff;

A fussy point but this would be easier to read if it followed the same
pattern as the above error checks with a "goto fail". The the success
path below can be outdented and made more obvious.


> +
> +	/* Driver takes ownership of state on successful async commit. */
> +	if (ret == 0) {
> +		/* TODO: ->page_flip is the only driver callback where the core
> +		 * doesn't update plane->fb. For now patch it up here. */
> +		plane->fb = plane->state->fb;
> +
> +		return 0;
> +	}
> +
> +fail:
> +	drm_atomic_state_free(state);
> +
> +	return ret;
> +backoff:
> +	drm_atomic_legacy_backoff(state);
> +	drm_atomic_state_clear(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;
> +
> +	goto retry;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_page_flip);
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 8cd6fe7a48e5..28a2f3a815fd 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -69,5 +69,10 @@ int drm_atomic_helper_plane_set_property(struct drm_plane *plane,
>  int drm_atomic_helper_connector_set_property(struct drm_connector *connector,
>  					struct drm_property *property,
>  					uint64_t val);
> +int drm_atomic_helper_page_flip(struct drm_crtc *crtc,
> +				struct drm_framebuffer *fb,
> +				struct drm_pending_vblank_event *event,
> +				uint32_t flags);
> +
>  
>  #endif /* DRM_ATOMIC_HELPER_H_ */
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx