[PATCH 0/2] Add support for passing crtc_id to page flip events.

[PATCH 0/2] Add support for passing crtc_id to page flip events.

[Maarten Lankhorst]

When generating events for a atomic commit involving multiple crtc's
it's not possible to distinguish which crtc belongs to which page flip event.
Solve this by putting crtc_id in the reserved member of page_flip_event.

I've checked weston, and the following ddx: modesetting (xserver), i915,
nouveau, ati, vmwgfx.

None of them depend in any way on the reserved member, so setting it to
non-zero should be safe. A lot of them use libdrm, which hides the page
flip event struct and only passes its members.

This is why I also changed libdrm to pass crtc_id in page_flip_handler2.
Any compositor that uses drmHandleEvent will be able to get information
about the crtc_id too, if available.

Maarten Lankhorst (2):
  (kernel) drm/core: Reuse the reserved member in drm_event_vblank for crtc_id.
  (libdrm) Add support for crtc_id in page flip events

Kernel:
 drivers/gpu/drm/drm_irq.c | 2 ++
 include/uapi/drm/drm.h    | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

Libdrm:
 include/drm/drm.h         |  2 +-
 tests/modetest/modetest.c | 20 ++++++++++++++++++--
 xf86drm.h                 | 16 +++++++++++++++-
 xf86drmMode.c             | 23 ++++++++++++++++-------
 4 files changed, 50 insertions(+), 11 deletions(-)
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm/core: Reuse the reserved member in drm_event_vblank for crtc_id.

[Maarten Lankhorst]

When doing a atomic commit affecting multiple crtc's, multiple events
are generated. The user_data member does not allow you to distinguish,
because they all have the same pointer.

I've chosen to use crtc_id, because using pipe would create ambiguity
when pipe = 0. A test for != 0 is easier to implement, and crtc_id
will never be 0.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Stone <daniels@collabora.com>
---
 drivers/gpu/drm/drm_irq.c | 2 ++
 include/uapi/drm/drm.h    | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 97c70642dbe1..f01bb0eea31c 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1021,6 +1021,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);
@@ -1048,6 +1049,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 452675fb55d9..dced4d4517d6 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -850,7 +850,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.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[LIBDRM PATCH 2/2] Add support for crtc_id in page flip events

[Maarten Lankhorst]

Add a page_flip_handler2 member to drmEventContext and bump
DRM_EVENT_CONTEXT.

To make sure that the new api works as intended, modetest is
changed to use it.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Stone <daniels@collabora.com>
---
 include/drm/drm.h         |  2 +-
 tests/modetest/modetest.c | 20 ++++++++++++++++++--
 xf86drm.h                 | 16 +++++++++++++++-
 xf86drmMode.c             | 23 ++++++++++++++++-------
 4 files changed, 50 insertions(+), 11 deletions(-)

diff --git a/include/drm/drm.h b/include/drm/drm.h
index d7874c6f0d47..6da7f2a1f40b 100644
--- a/include/drm/drm.h
+++ b/include/drm/drm.h
@@ -844,7 +844,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 */
diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
index 21d543842c95..56ee7f2adc83 100644
--- a/tests/modetest/modetest.c
+++ b/tests/modetest/modetest.c
@@ -913,8 +913,8 @@ static void set_property(struct device *dev, struct property_arg *p)
 /* -------------------------------------------------------------------------- */
 
 static void
-page_flip_handler(int fd, unsigned int frame,
-		  unsigned int sec, unsigned int usec, void *data)
+page_flip_handler2(int fd, unsigned int crtc_id, unsigned int frame,
+		   unsigned int sec, unsigned int usec, void *data)
 {
 	struct pipe_arg *pipe;
 	unsigned int new_fb_id;
@@ -927,6 +927,8 @@ page_flip_handler(int fd, unsigned int frame,
 	else
 		new_fb_id = pipe->fb_id[0];
 
+	assert(crtc_id == pipe->crtc->crtc->crtc_id);
+
 	drmModePageFlip(fd, pipe->crtc->crtc->crtc_id, new_fb_id,
 			DRM_MODE_PAGE_FLIP_EVENT, pipe);
 	pipe->current_fb_id = new_fb_id;
@@ -941,6 +943,19 @@ page_flip_handler(int fd, unsigned int frame,
 	}
 }
 
+static void
+page_flip_handler(int fd, unsigned int frame,
+		  unsigned int sec, unsigned int usec, void *data)
+{
+	struct pipe_arg *pipe = data;
+	static int once = 0;
+
+	if (!once++)
+		fprintf(stderr, "kernel doesn't pass crtc_id, or using older libdrm\n");
+
+	page_flip_handler2(fd, pipe->crtc->crtc->crtc_id, frame, sec, usec, data);
+}
+
 static bool format_support(const drmModePlanePtr ovr, uint32_t fmt)
 {
 	unsigned int i;
@@ -1233,6 +1248,7 @@ static void test_page_flip(struct device *dev, struct pipe_arg *pipes, unsigned
 	evctx.version = DRM_EVENT_CONTEXT_VERSION;
 	evctx.vblank_handler = NULL;
 	evctx.page_flip_handler = page_flip_handler;
+	evctx.page_flip_handler2 = page_flip_handler2;
 
 	while (1) {
 #if 0
diff --git a/xf86drm.h b/xf86drm.h
index 481d882aa017..556390cdec47 100644
--- a/xf86drm.h
+++ b/xf86drm.h
@@ -728,7 +728,7 @@ extern void drmMsg(const char *format, ...) DRM_PRINTFLIKE(1, 2);
 extern int drmSetMaster(int fd);
 extern int drmDropMaster(int fd);
 
-#define DRM_EVENT_CONTEXT_VERSION 2
+#define DRM_EVENT_CONTEXT_VERSION 3
 
 typedef struct _drmEventContext {
 
@@ -748,6 +748,20 @@ typedef struct _drmEventContext {
 				  unsigned int tv_usec,
 				  void *user_data);
 
+	/*
+	 * Page flip handler used when kernel supports passing crtc_id
+	 * to userspace.
+	 *
+	 * If the kernel doesn't support passing crtc_id, the old
+	 * page_flip_handler() member will be called if set,
+	 * if it's NULL, this function will be called with crtc_id set to 0.
+	 */
+	void (*page_flip_handler2)(int fd,
+				   unsigned int crtc_id,
+				   unsigned int sequence,
+				   unsigned int tv_sec,
+				   unsigned int tv_usec,
+				   void *user_data);
 } drmEventContext, *drmEventContextPtr;
 
 extern int drmHandleEvent(int fd, drmEventContextPtr evctx);
diff --git a/xf86drmMode.c b/xf86drmMode.c
index f7b59484153e..62bfd69b1d14 100644
--- a/xf86drmMode.c
+++ b/xf86drmMode.c
@@ -902,15 +902,24 @@ int drmHandleEvent(int fd, drmEventContextPtr evctx)
 					      U642VOID (vblank->user_data));
 			break;
 		case DRM_EVENT_FLIP_COMPLETE:
-			if (evctx->version < 2 ||
-			    evctx->page_flip_handler == NULL)
+			if (evctx->version < 2)
 				break;
 			vblank = (struct drm_event_vblank *) e;
-			evctx->page_flip_handler(fd,
-						 vblank->sequence,
-						 vblank->tv_sec,
-						 vblank->tv_usec,
-						 U642VOID (vblank->user_data));
+
+			if (evctx->version >= 3 && evctx->page_flip_handler2 &&
+			    (vblank->crtc_id || !evctx->page_flip_handler))
+				evctx->page_flip_handler2(fd,
+							  vblank->crtc_id,
+							  vblank->sequence,
+							  vblank->tv_sec,
+							  vblank->tv_usec,
+							  U642VOID (vblank->user_data));
+			else if (evctx->page_flip_handler)
+				evctx->page_flip_handler(fd,
+							 vblank->sequence,
+							 vblank->tv_sec,
+							 vblank->tv_usec,
+							 U642VOID (vblank->user_data));
 			break;
 		default:
 			break;
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/core: Reuse the reserved member in drm_event_vblank for crtc_id.

[Daniel Vetter]

On Wed, Aug 10, 2016 at 12:46:23PM +0200, Maarten Lankhorst wrote:
> When doing a atomic commit affecting multiple crtc's, multiple events
> are generated. The user_data member does not allow you to distinguish,
> because they all have the same pointer.
> 
> I've chosen to use crtc_id, because using pipe would create ambiguity
> when pipe = 0. A test for != 0 is easier to implement, and crtc_id
> will never be 0.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Stone <daniels@collabora.com>
> ---
>  drivers/gpu/drm/drm_irq.c | 2 ++
>  include/uapi/drm/drm.h    | 2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 97c70642dbe1..f01bb0eea31c 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1021,6 +1021,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);
> @@ -1048,6 +1049,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 452675fb55d9..dced4d4517d6 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -850,7 +850,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 */

Same comment as in reply to Michel's patch: Is there really no existing
userspace which would fail to compile now because we renamed this? Simply
creating a new drm_event_vblank2 struct would fix that.
-Daniel

>  };
>  
>  /* typedef area */
> -- 
> 2.7.4
> 
> _______________________________________________
> 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

Re: [PATCH 1/2] drm/core: Reuse the reserved member in drm_event_vblank for crtc_id.

[Maarten Lankhorst]

Op 10-08-16 om 16:14 schreef Daniel Vetter:
> On Wed, Aug 10, 2016 at 12:46:23PM +0200, Maarten Lankhorst wrote:
>> When doing a atomic commit affecting multiple crtc's, multiple events
>> are generated. The user_data member does not allow you to distinguish,
>> because they all have the same pointer.
>>
>> I've chosen to use crtc_id, because using pipe would create ambiguity
>> when pipe = 0. A test for != 0 is easier to implement, and crtc_id
>> will never be 0.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: David Airlie <airlied@linux.ie>
>> Cc: Daniel Stone <daniels@collabora.com>
>> ---
>>  drivers/gpu/drm/drm_irq.c | 2 ++
>>  include/uapi/drm/drm.h    | 2 +-
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
>> index 97c70642dbe1..f01bb0eea31c 100644
>> --- a/drivers/gpu/drm/drm_irq.c
>> +++ b/drivers/gpu/drm/drm_irq.c
>> @@ -1021,6 +1021,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);
>> @@ -1048,6 +1049,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 452675fb55d9..dced4d4517d6 100644
>> --- a/include/uapi/drm/drm.h
>> +++ b/include/uapi/drm/drm.h
>> @@ -850,7 +850,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 */
> Same comment as in reply to Michel's patch: Is there really no existing
> userspace which would fail to compile now because we renamed this? Simply
> creating a new drm_event_vblank2 struct would fix that.
Hey,

This comment must have fallen through, I haven't found any case in which this happened. All of them use a memset or allocate it zero'd.

~Maarten
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel