[6/6] drm: Add DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags
diff mbox

Message ID 1470281981-18172-7-git-send-email-michel@daenzer.net
State New
Headers show

Commit Message

Michel Dänzer Aug. 4, 2016, 3:39 a.m. UTC
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(-)

Comments

Ville Syrjälä Aug. 4, 2016, 7:42 a.m. UTC | #1
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.

I like the idea. Atomic could use this too, although there we'd need to
potentially pass in a target vblank for each crtc taking part of the
operation, or I suppose you could pass it only for, say, a single crtc
in case you only want ot sync to that one.

One thing I've pondered in the past is the OML_sync_control modulo stuff.
Looks like what you have here won't free up userspace from doing the
wait_vblank+flip sequence if it wants to implement the modulo behaviour.
And of course it's still not guaranteed to honor the modulo if, for
instance, the flip ioctl itself gets blocked on a mutex for a wee bit too
long and misses the target. So should we just implement the modulo stuff
in the kernel instead?

> 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;
>  	__u64 user_data;
>  };
>  
> -- 
> 2.8.1
Daniel Vetter Aug. 4, 2016, 10:56 a.m. UTC | #2
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
Michel Dänzer Aug. 8, 2016, 6:53 a.m. UTC | #3
On 04.08.2016 19:56, Daniel Vetter wrote:
> On Thu, Aug 04, 2016 at 12:39:41PM +0900, Michel Dänzer wrote:
>> From: Michel Dänzer <michel.daenzer@amd.com>
>>
>> @@ -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.

It doesn't break ABI. I guess you mean there might be userspace code
referencing the reserved field? Such code would have to be open-coding
drmModePageFlip instead of using it. And it turns out you're right, e.g.
SNA actually does that and would fail to compile... Will be fixed in v2.


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

Thanks! Will it be okay to merge this via Alex's tree?
Michel Dänzer Aug. 8, 2016, 7:06 a.m. UTC | #4
On 04.08.2016 16:42, Ville Syrjälä wrote:
> 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.
> 
> I like the idea. Atomic could use this too, although there we'd need to
> potentially pass in a target vblank for each crtc taking part of the
> operation, or I suppose you could pass it only for, say, a single crtc
> in case you only want ot sync to that one.

Yes, userspace tends to just synchronize to one CRTC and doesn't
particularly care about any others performing the same flip.


> One thing I've pondered in the past is the OML_sync_control modulo stuff.
> Looks like what you have here won't free up userspace from doing the
> wait_vblank+flip sequence if it wants to implement the modulo behaviour.
> And of course it's still not guaranteed to honor the modulo if, for
> instance, the flip ioctl itself gets blocked on a mutex for a wee bit too
> long and misses the target. So should we just implement the modulo stuff
> in the kernel instead?

That might be nice, but I'm afraid it would be rather more complex than
this patch for various reasons. E.g., in no particular order:

The modulo can't be passed to the ioctl without extending the struct
used by the ioctl, which could be tricky to get right in all corner cases.

The current driver interface of the Present extension code in xserver
wouldn't allow making use of it, the xserver code already handles the
modulo before calling into the driver.

The kernel driver interface would have to change significantly as well,
it would be basically the driver's responsibility to handle the modulo.
It would also be rather tricky to make it work reliably with our
hardware, because we can't reliably determine in advance when a flip
will take effect in the hardware.


Overall, it seems like too much effort for too little gain with the
legacy KMS API. Might be worth tackling for atomic though.

Patch
diff mbox

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