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(-)
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 */
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
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
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;
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Maarten Lankhorst