diff mbox

[kernel,1/2] drm: Pass CRTC ID in userspace vblank events

Message ID 20170404165221.28240-2-daniels@collabora.com (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Stone April 4, 2017, 4:52 p.m. UTC 
From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

With the atomic API, it is possible that a single commit affects
multiple crtcs. If the user requests an event with that commit, one
event will be sent for each CRTC, but it is not possible to distinguish
which crtc an event is for in user space. To solve this, the reserved
field in struct drm_vblank_event is repurposed to include the crtc_id
which the event is for.

The DRM_CAP_CRTC_IN_VBLANK_EVENT is added to allow userspace to query if
the crtc field will be set properly.

[daniels: Rebased, using Maarten's forward-port.]

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Signed-off-by: Daniel Stone <daniels@collabora.com>
Cc: Maarten Lankhorst <maarten.lankhorst@intel.com>
---
 drivers/gpu/drm/drm_ioctl.c | 3 +++
 drivers/gpu/drm/drm_irq.c   | 2 ++
 include/uapi/drm/drm.h      | 3 ++-
 3 files changed, 7 insertions(+), 1 deletion(-)

Comments

Sean Paul April 4, 2017, 5:58 p.m. UTC | #1 
On Tue, Apr 04, 2017 at 05:52:21PM +0100, Daniel Stone wrote:
> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> 
> With the atomic API, it is possible that a single commit affects
> multiple crtcs. If the user requests an event with that commit, one
> event will be sent for each CRTC, but it is not possible to distinguish
> which crtc an event is for in user space. To solve this, the reserved
> field in struct drm_vblank_event is repurposed to include the crtc_id
> which the event is for.
> 
> The DRM_CAP_CRTC_IN_VBLANK_EVENT is added to allow userspace to query if
> the crtc field will be set properly.
> 
> [daniels: Rebased, using Maarten's forward-port.]
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> Signed-off-by: Daniel Stone <daniels@collabora.com>

Yeah, this seems like good use of the reserved field to me.

Reviewed-by: Sean Paul <seanpaul@chromium.org>


> Cc: Maarten Lankhorst <maarten.lankhorst@intel.com>
> ---
>  drivers/gpu/drm/drm_ioctl.c | 3 +++
>  drivers/gpu/drm/drm_irq.c   | 2 ++
>  include/uapi/drm/drm.h      | 3 ++-
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 7d6deaa91281..0793e2da9d21 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -286,6 +286,9 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
>  	case DRM_CAP_ADDFB2_MODIFIERS:
>  		req->value = dev->mode_config.allow_fb_modifiers;
>  		break;
> +	case DRM_CAP_CRTC_IN_VBLANK_EVENT:
> +		req->value = 1;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index dac1b2593cb1..8c866cac62dd 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1026,6 +1026,7 @@ void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
>  
>  	e->pipe = pipe;
>  	e->event.sequence = drm_vblank_count(dev, pipe);
> +	e->event.crtc_id = crtc->base.id;
>  	list_add_tail(&e->base.link, &dev->vblank_event_list);
>  }
>  EXPORT_SYMBOL(drm_crtc_arm_vblank_event);
> @@ -1056,6 +1057,7 @@ void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>  		now = get_drm_timestamp();
>  	}
>  	e->pipe = pipe;
> +	e->event.crtc_id = crtc->base.id;
>  	send_vblank_event(dev, e, seq, &now);
>  }
>  EXPORT_SYMBOL(drm_crtc_send_vblank_event);
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index b2c52843bc70..42d9f64ce416 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -647,6 +647,7 @@ struct drm_gem_open {
>  #define DRM_CAP_CURSOR_HEIGHT		0x9
>  #define DRM_CAP_ADDFB2_MODIFIERS	0x10
>  #define DRM_CAP_PAGE_FLIP_TARGET	0x11
> +#define DRM_CAP_CRTC_IN_VBLANK_EVENT	0x12
>  
>  /** DRM_IOCTL_GET_CAP ioctl argument type */
>  struct drm_get_cap {
> @@ -851,7 +852,7 @@ struct drm_event_vblank {
>  	__u32 tv_sec;
>  	__u32 tv_usec;
>  	__u32 sequence;
> -	__u32 reserved;
> +	__u32 crtc_id; /* 0 on older kernels that do not support this */
>  };
>  
>  /* typedef area */
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Stone April 4, 2017, 8:53 p.m. UTC | #2 
Hi,

On 4 April 2017 at 18:58, Sean Paul <seanpaul@chromium.org> wrote:
> On Tue, Apr 04, 2017 at 05:52:21PM +0100, Daniel Stone wrote:
>> With the atomic API, it is possible that a single commit affects
>> multiple crtcs. If the user requests an event with that commit, one
>> event will be sent for each CRTC, but it is not possible to distinguish
>> which crtc an event is for in user space. To solve this, the reserved
>> field in struct drm_vblank_event is repurposed to include the crtc_id
>> which the event is for.
>>
>> The DRM_CAP_CRTC_IN_VBLANK_EVENT is added to allow userspace to query if
>> the crtc field will be set properly.
>>
>> [daniels: Rebased, using Maarten's forward-port.]
>
> Yeah, this seems like good use of the reserved field to me.
>
> Reviewed-by: Sean Paul <seanpaul@chromium.org>

Thanks Sean for the review! I've pushed this to drm-misc-next ...
without your R-b tag, as I botched my first dim. Oops.

libdrm v2 has been sent, and Weston atomic will be revved to use this
with its real version number when that gets merged and released. FWIW,
the Weston side was also acked by Pekka Paalanen, though I'll let him
add his thoughts in his own words tomorrow.

Cheers,
Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 7d6deaa91281..0793e2da9d21 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -286,6 +286,9 @@  static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
 	case DRM_CAP_ADDFB2_MODIFIERS:
 		req->value = dev->mode_config.allow_fb_modifiers;
 		break;
+	case DRM_CAP_CRTC_IN_VBLANK_EVENT:
+		req->value = 1;
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index dac1b2593cb1..8c866cac62dd 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1026,6 +1026,7 @@  void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
 
 	e->pipe = pipe;
 	e->event.sequence = drm_vblank_count(dev, pipe);
+	e->event.crtc_id = crtc->base.id;
 	list_add_tail(&e->base.link, &dev->vblank_event_list);
 }
 EXPORT_SYMBOL(drm_crtc_arm_vblank_event);
@@ -1056,6 +1057,7 @@  void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
 		now = get_drm_timestamp();
 	}
 	e->pipe = pipe;
+	e->event.crtc_id = crtc->base.id;
 	send_vblank_event(dev, e, seq, &now);
 }
 EXPORT_SYMBOL(drm_crtc_send_vblank_event);
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index b2c52843bc70..42d9f64ce416 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -647,6 +647,7 @@  struct drm_gem_open {
 #define DRM_CAP_CURSOR_HEIGHT		0x9
 #define DRM_CAP_ADDFB2_MODIFIERS	0x10
 #define DRM_CAP_PAGE_FLIP_TARGET	0x11
+#define DRM_CAP_CRTC_IN_VBLANK_EVENT	0x12
 
 /** DRM_IOCTL_GET_CAP ioctl argument type */
 struct drm_get_cap {
@@ -851,7 +852,7 @@  struct drm_event_vblank {
 	__u32 tv_sec;
 	__u32 tv_usec;
 	__u32 sequence;
-	__u32 reserved;
+	__u32 crtc_id; /* 0 on older kernels that do not support this */
 };
 
 /* typedef area */