| Message ID | 20170404165221.28240-1-daniels@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Daniel, Zero objections against the patch, just a couple of small notes. On 4 April 2017 at 17:52, Daniel Stone <daniels@collabora.com> wrote: > From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > > When using the atomic API, one request can span multiple CRTCs, however > one event is generated per CRTC. As we cannot disambiguate the CRTC with > user data (since we only have one piece of user data to pass in), newer > kernels can include the CRTC ID in the page flip event. > > Add a new vfunc to dispatch vblank events carrying a CRTC ID to clients > who negotiate a higher interface version. > > [daniels: Rebased, include new cap, call page_flip_handler if it is set > but page_flip_handler2 isn't even on newer contexts, write a > commit message.] > > 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> > --- > include/drm/drm.h | 3 ++- > xf86drm.h | 9 ++++++++- > xf86drmMode.c | 24 ++++++++++++++++-------- > 3 files changed, 26 insertions(+), 10 deletions(-) > > diff --git a/include/drm/drm.h b/include/drm/drm.h > index 09d4262f..c4492372 100644 > --- a/include/drm/drm.h > +++ b/include/drm/drm.h > @@ -641,6 +641,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 { > @@ -846,7 +847,7 @@ struct drm_event_vblank { > __u32 tv_sec; > __u32 tv_usec; > __u32 sequence; > - __u32 reserved; > + __u32 crtc_id; > }; > Please split these to a separate commit as mentioned in include/drm/README "When and how to update these files" ? > /* typedef area */ > diff --git a/xf86drm.h b/xf86drm.h > index 0d927018..d75ca8ce 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 > We really want to fix weston to _not_ use this define, but to set the version it actually supports/implements. I'll send a patch for that in a minute. -Emil
Hi Emil, On Tue, 2017-04-04 at 18:12 +0100, Emil Velikov wrote: > On 4 April 2017 at 17:52, Daniel Stone <daniels@collabora.com> wrote: > > @@ -846,7 +847,7 @@ struct drm_event_vblank { > > __u32 tv_sec; > > __u32 tv_usec; > > __u32 sequence; > > - __u32 reserved; > > + __u32 crtc_id; > > }; > > > > Please split these to a separate commit as mentioned in > include/drm/README "When and how to update these files" ? OK, will do. > > @@ -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 > > > > We really want to fix weston to _not_ use this define, but to set the > version it actually supports/implements. > I'll send a patch for that in a minute. One step ahead of you: - evctx.version = DRM_EVENT_CONTEXT_VERSION; - evctx.page_flip_handler = page_flip_handler; + evctx.version = 3; https://lists.freedesktop.org/archives/wayland-devel/2017-April/033747. html Cheers, Daniel
On Tue, 4 Apr 2017 17:52:20 +0100 Daniel Stone <daniels@collabora.com> wrote: > From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > > When using the atomic API, one request can span multiple CRTCs, however > one event is generated per CRTC. As we cannot disambiguate the CRTC with > user data (since we only have one piece of user data to pass in), newer > kernels can include the CRTC ID in the page flip event. > > Add a new vfunc to dispatch vblank events carrying a CRTC ID to clients > who negotiate a higher interface version. > > [daniels: Rebased, include new cap, call page_flip_handler if it is set > but page_flip_handler2 isn't even on newer contexts, write a > commit message.] > > 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> > --- > include/drm/drm.h | 3 ++- > xf86drm.h | 9 ++++++++- > xf86drmMode.c | 24 ++++++++++++++++-------- > 3 files changed, 26 insertions(+), 10 deletions(-) > Hi, I'm replying to this patch to keep the scope rather than the split ones which are needed together and have unrelated changes. > diff --git a/include/drm/drm.h b/include/drm/drm.h > index 09d4262f..c4492372 100644 > --- a/include/drm/drm.h > +++ b/include/drm/drm.h > @@ -641,6 +641,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 { > @@ -846,7 +847,7 @@ struct drm_event_vblank { > __u32 tv_sec; > __u32 tv_usec; > __u32 sequence; > - __u32 reserved; > + __u32 crtc_id; > }; > > /* typedef area */ > diff --git a/xf86drm.h b/xf86drm.h > index 0d927018..d75ca8ce 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,13 @@ typedef struct _drmEventContext { > unsigned int tv_usec, > void *user_data); > > + void (*page_flip_handler2)(int fd, > + unsigned int sequence, > + unsigned int tv_sec, > + unsigned int tv_usec, > + unsigned int crtc_id, > + void *user_data); > + > } drmEventContext, *drmEventContextPtr; > > extern int drmHandleEvent(int fd, drmEventContextPtr evctx); > diff --git a/xf86drmMode.c b/xf86drmMode.c > index fb47fa8b..b71520aa 100644 > --- a/xf86drmMode.c > +++ b/xf86drmMode.c > @@ -889,6 +889,7 @@ int drmHandleEvent(int fd, drmEventContextPtr evctx) > int len, i; > struct drm_event *e; > struct drm_event_vblank *vblank; > + void *user_data; > > /* The DRM read semantics guarantees that we always get only > * complete events. */ > @@ -915,15 +916,22 @@ 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) > - break; > vblank = (struct drm_event_vblank *) e; > - evctx->page_flip_handler(fd, > - vblank->sequence, > - vblank->tv_sec, > - vblank->tv_usec, > - U642VOID (vblank->user_data)); > + user_data = U642VOID (vblank->user_data); > + > + if (evctx->version >= 3 && evctx->page_flip_handler2) > + evctx->page_flip_handler2(fd, > + vblank->sequence, > + vblank->tv_sec, > + vblank->tv_usec, > + vblank->crtc_id, > + user_data); > + else if (evctx->version >= 2 && evctx->page_flip_handler) > + evctx->page_flip_handler(fd, > + vblank->sequence, > + vblank->tv_sec, > + vblank->tv_usec, > + user_data); > break; > default: > break; This is excellent. I have browsed through Weston's DRM backend as of Daniel's atomic series v9, and the end result looks good. It depends on the CRTC ID in the pageflip events. There Weston runs each CRTC independently, but at opportunity will coalesce updates to several CRTCs into the same atomic commit. As the repaint state machines are per CRTC in Weston, it is necessary to know which CRTC completed the pageflip at a time to avoid stalling everything until the last one or not being able to coalesce across CRTCs at all. Being able to coalesce across CRTCs is important particularly for modesets where programming all CRTCs in one go improves the chances of the transition succeeding in hardware. I also cannot think of needing anything more in the event. For instance, if DRM flip queue depth was grown beyond the current of one, we can use the user data pointer to identify which atomic commit is completing. This patch gets all thumbs up from me on behalf of Weston. Thanks, pq
diff --git a/include/drm/drm.h b/include/drm/drm.h index 09d4262f..c4492372 100644 --- a/include/drm/drm.h +++ b/include/drm/drm.h @@ -641,6 +641,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 { @@ -846,7 +847,7 @@ struct drm_event_vblank { __u32 tv_sec; __u32 tv_usec; __u32 sequence; - __u32 reserved; + __u32 crtc_id; }; /* typedef area */ diff --git a/xf86drm.h b/xf86drm.h index 0d927018..d75ca8ce 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,13 @@ typedef struct _drmEventContext { unsigned int tv_usec, void *user_data); + void (*page_flip_handler2)(int fd, + unsigned int sequence, + unsigned int tv_sec, + unsigned int tv_usec, + unsigned int crtc_id, + void *user_data); + } drmEventContext, *drmEventContextPtr; extern int drmHandleEvent(int fd, drmEventContextPtr evctx); diff --git a/xf86drmMode.c b/xf86drmMode.c index fb47fa8b..b71520aa 100644 --- a/xf86drmMode.c +++ b/xf86drmMode.c @@ -889,6 +889,7 @@ int drmHandleEvent(int fd, drmEventContextPtr evctx) int len, i; struct drm_event *e; struct drm_event_vblank *vblank; + void *user_data; /* The DRM read semantics guarantees that we always get only * complete events. */ @@ -915,15 +916,22 @@ 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) - break; vblank = (struct drm_event_vblank *) e; - evctx->page_flip_handler(fd, - vblank->sequence, - vblank->tv_sec, - vblank->tv_usec, - U642VOID (vblank->user_data)); + user_data = U642VOID (vblank->user_data); + + if (evctx->version >= 3 && evctx->page_flip_handler2) + evctx->page_flip_handler2(fd, + vblank->sequence, + vblank->tv_sec, + vblank->tv_usec, + vblank->crtc_id, + user_data); + else if (evctx->version >= 2 && evctx->page_flip_handler) + evctx->page_flip_handler(fd, + vblank->sequence, + vblank->tv_sec, + vblank->tv_usec, + user_data); break; default: break;