Re: [PATCH 6/6] drm: Add DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Aug 04, 2016 at 12:39:41PM +0900, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
> 
> These flags allow userspace to explicitly specify the target vertical
> blank period when a flip should take effect.
> 
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> ---
> 
> Note that the previous patches in this series can avoid delaying page
> flips in some cases even without this patch or any corresponding
> userspace changes. Here are examples of how userspace can take advantage
> of this patch to avoid delaying page flips in even more cases:
> 
> https://cgit.freedesktop.org/~daenzer/xf86-video-ati/commit/?id=fc884a8af25345c32bd4104c864ecfeb9bb3db9b
> 
> https://cgit.freedesktop.org/~daenzer/xf86-video-amdgpu/commit/?id=b8631a9ba49c0d0ebe5dcd1dbfb68fcfe907296f
> 
>  drivers/gpu/drm/drm_crtc.c  | 46 +++++++++++++++++++++++++++++++++++++++------
>  drivers/gpu/drm/drm_ioctl.c |  8 ++++++++
>  include/uapi/drm/drm.h      |  1 +
>  include/uapi/drm/drm_mode.h | 24 +++++++++++++++++++----
>  4 files changed, 69 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 15ad7e2..b2fd860 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -5421,11 +5421,19 @@ 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;
> +	u32 target_vblank = page_flip->sequence;
>  	int ret = -EINVAL;
>  
> -	if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS ||
> -	    page_flip->reserved != 0)
> +	if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS)
> +		return -EINVAL;
> +
> +	if (page_flip->sequence != 0 && !(page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET))
> +		return -EINVAL;
> +
> +	/* Only one of the DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags
> +	 * can be specified
> +	 */
> +	if ((page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET) == DRM_MODE_PAGE_FLIP_TARGET)
>  		return -EINVAL;
>  
>  	if ((page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC) && !dev->mode_config.async_page_flip)
> @@ -5436,15 +5444,41 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  		return -ENOENT;
>  
>  	if (crtc->funcs->page_flip_target) {
> +		u32 current_vblank;
>  		int r;
>  
>  		r = drm_crtc_vblank_get(crtc);
>  		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)
> +		current_vblank = drm_crtc_vblank_count(crtc);
> +
> +		switch (page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET) {
> +		case DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE:
> +			if ((int)(target_vblank - current_vblank) > 1) {
> +				DRM_DEBUG("Invalid absolute flip target %u, "
> +					  "must be <= %u\n", target_vblank,
> +					  current_vblank + 1);
> +				drm_crtc_vblank_put(crtc);
> +				return -EINVAL;
> +			}
> +			break;
> +		case DRM_MODE_PAGE_FLIP_TARGET_RELATIVE:
> +			if (target_vblank != 0 && target_vblank != 1) {
> +				DRM_DEBUG("Invalid relative flip target %u, "
> +					  "must be 0 or 1\n", target_vblank);
> +				drm_crtc_vblank_put(crtc);
> +				return -EINVAL;
> +			}
> +			target_vblank += current_vblank;
> +			break;
> +		default:
> +			target_vblank = current_vblank +
> +				!(page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC);
> +			break;
> +		}
> +	} else if (crtc->funcs->page_flip == NULL ||
> +		   (page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET))
>  		return -EINVAL;
>  
>  	drm_modeset_lock_crtc(crtc, crtc->primary);
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 33af4a5..0099d2a 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -228,6 +228,7 @@ static int drm_getstats(struct drm_device *dev, void *data,
>  static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
>  {
>  	struct drm_get_cap *req = data;
> +	struct drm_crtc *crtc;
>  
>  	req->value = 0;
>  	switch (req->capability) {
> @@ -254,6 +255,13 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
>  	case DRM_CAP_ASYNC_PAGE_FLIP:
>  		req->value = dev->mode_config.async_page_flip;
>  		break;
> +	case DRM_CAP_PAGE_FLIP_TARGET:
> +		req->value = 1;
> +		drm_for_each_crtc(crtc, dev) {
> +			if (!crtc->funcs->page_flip_target)
> +				req->value = 0;
> +		}
> +		break;
>  	case DRM_CAP_CURSOR_WIDTH:
>  		if (dev->mode_config.cursor_width)
>  			req->value = dev->mode_config.cursor_width;
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 452675f..b2c5284 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -646,6 +646,7 @@ struct drm_gem_open {
>  #define DRM_CAP_CURSOR_WIDTH		0x8
>  #define DRM_CAP_CURSOR_HEIGHT		0x9
>  #define DRM_CAP_ADDFB2_MODIFIERS	0x10
> +#define DRM_CAP_PAGE_FLIP_TARGET	0x11
>  
>  /** DRM_IOCTL_GET_CAP ioctl argument type */
>  struct drm_get_cap {
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 49a7265..175aa1f 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -520,7 +520,13 @@ struct drm_color_lut {
>  
>  #define DRM_MODE_PAGE_FLIP_EVENT 0x01
>  #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
> -#define DRM_MODE_PAGE_FLIP_FLAGS (DRM_MODE_PAGE_FLIP_EVENT|DRM_MODE_PAGE_FLIP_ASYNC)
> +#define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
> +#define DRM_MODE_PAGE_FLIP_TARGET_RELATIVE 0x8
> +#define DRM_MODE_PAGE_FLIP_TARGET (DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE | \
> +				   DRM_MODE_PAGE_FLIP_TARGET_RELATIVE)
> +#define DRM_MODE_PAGE_FLIP_FLAGS (DRM_MODE_PAGE_FLIP_EVENT | \
> +				  DRM_MODE_PAGE_FLIP_ASYNC | \
> +				  DRM_MODE_PAGE_FLIP_TARGET)
>  
>  /*
>   * Request a page flip on the specified crtc.
> @@ -543,15 +549,25 @@ struct drm_color_lut {
>   * 'as soon as possible', meaning that it not delay waiting for vblank.
>   * This may cause tearing on the screen.
>   *
> - * The reserved field must be zero until we figure out something
> - * clever to use it for.
> + * The sequence field must be zero unless either of the
> + * DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags is specified. When
> + * the ABSOLUTE flag is specified, the sequence field denotes the absolute
> + * vblank sequence when the flip should take effect. When the RELATIVE
> + * flag is specified, the sequence field denotes the relative (to the
> + * current one when the ioctl is called) vblank sequence when the flip
> + * should take effect. NOTE: DRM_IOCTL_WAIT_VBLANK must still be used to
> + * make sure the vblank sequence before the target one has passed before
> + * calling this ioctl. The purpose of the
> + * DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags is merely to clarify
> + * the target for when code dealing with a page flip runs during a
> + * vertical blank period.
>   */
>  
>  struct drm_mode_crtc_page_flip {
>  	__u32 crtc_id;
>  	__u32 fb_id;
>  	__u32 flags;
> -	__u32 reserved;
> +	__u32 sequence;

Might break abi somewhere. I think it'd be better to create a
struct drm_mode_crtc_page_flip2 with the renamed field. Otherwise this
looks great, and the only other quibble I have is that we extend the old
page_flip path here instead of atomic. But given all the fun we have
trying to port i915 to atomic I fully understand that you can't simply
first port amdgpu over ;-)

With or without the above (but strongingly in support of a new struct):

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  	__u64 user_data;
>  };
>  
> -- 
> 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