Re: [PATCH 1/6] drm: Add page_flip_target CRTC hook

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Aug 04, 2016 at 12:39:36PM +0900, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
> 
> Mostly the same as the existing page_flip hook, but takes an additional
> parameter specifying the target vertical blank period when the flip
> should take effect.
> 
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 23 +++++++++++++++++++----
>  include/drm/drm_crtc.h     | 14 ++++++++++++++
>  2 files changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 9d3f80e..15ad7e2 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -5421,6 +5421,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	struct drm_crtc *crtc;
>  	struct drm_framebuffer *fb = NULL;
>  	struct drm_pending_vblank_event *e = NULL;
> +	u32 target_vblank = 0;
>  	int ret = -EINVAL;
>  
>  	if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS ||
> @@ -5434,6 +5435,18 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	if (!crtc)
>  		return -ENOENT;
>  
> +	if (crtc->funcs->page_flip_target) {
> +		int r;
> +
> +		r = drm_crtc_vblank_get(crtc);

This leaks when e.g framebuffer_lookup fails.
-Daniel

> +		if (r)
> +			return r;
> +
> +		target_vblank = drm_crtc_vblank_count(crtc) +
> +			!(page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC);
> +	} else if (crtc->funcs->page_flip == NULL)
> +		return -EINVAL;
> +
>  	drm_modeset_lock_crtc(crtc, crtc->primary);
>  	if (crtc->primary->fb == NULL) {
>  		/* The framebuffer is currently unbound, presumably
> @@ -5444,9 +5457,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  		goto out;
>  	}
>  
> -	if (crtc->funcs->page_flip == NULL)
> -		goto out;
> -
>  	fb = drm_framebuffer_lookup(dev, page_flip->fb_id);
>  	if (!fb) {
>  		ret = -ENOENT;
> @@ -5487,7 +5497,12 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	}
>  
>  	crtc->primary->old_fb = crtc->primary->fb;
> -	ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags);
> +	if (crtc->funcs->page_flip_target)
> +		ret = crtc->funcs->page_flip_target(crtc, fb, e,
> +						    page_flip->flags,
> +						    target_vblank);
> +	else
> +		ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags);
>  	if (ret) {
>  		if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT)
>  			drm_event_cancel_free(dev, &e->base);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 9e6ab4a..ae1d9b6 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -579,6 +579,20 @@ struct drm_crtc_funcs {
>  			 uint32_t flags);
>  
>  	/**
> +	 * @page_flip_target:
> +	 *
> +	 * Same as @page_flip but with an additional parameter specifying the
> +	 * target vertical blank period when the flip should take effect.
> +	 *
> +	 * Note that the core code calls drm_crtc_vblank_get before this entry
> +	 * point.

I think you should add "Drivers must drop that reference again by calling
drm_crtc_vblank_put()."

It's a bit icky that we have this difference (both compared to old page
flip, but also atomic doesn't grab a vblank reference either). But really
can't be avoided, at least if we implement the relative mode.
-Daniel

> +	 */
> +	int (*page_flip_target)(struct drm_crtc *crtc,
> +				struct drm_framebuffer *fb,
> +				struct drm_pending_vblank_event *event,
> +				uint32_t flags, uint32_t target);
> +
> +	/**
>  	 * @set_property:
>  	 *
>  	 * This is the legacy entry point to update a property attached to the
> -- 
> 2.8.1
> 
> _______________________________________________
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel