| Message ID | 20190718194709.GA11103@ravnborg.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] drm/vblank: drop use of DRM_WAIT_ON() | expand |
On Thu, Jul 18, 2019 at 09:47:09PM +0200, Sam Ravnborg wrote: > From e6f70cb90e0c3c90d45017a8257353652b7e0dcc Mon Sep 17 00:00:00 2001 > From: Sam Ravnborg <sam@ravnborg.org> > Date: Thu, 30 May 2019 09:38:47 +0200 > Subject: [PATCH] drm/vblank: drop use of DRM_WAIT_ON() > > DRM_WAIT_ON() is from the deprecated drm_os_linux header and > the modern replacement is the wait_event_*. > > The return values differ, so a conversion is needed to > keep the original interface towards userspace. > Introduced a switch/case to make code obvious and to allow > different debug prints depending on the result. > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > Reviewed-by: Sean Paul <sean@poorly.run> A bit bold to keep the r-b for this substantial bugfix to the patch. Also not per-patch changelog here. Plus doesn't seem to be on dri-devel somehow. > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <maxime.ripard@bootlin.com> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@ffwll.ch> > --- > > To verify if it works with the fix. > (Added wat variable to handle the situation where we never wait) > > Sam > > drivers/gpu/drm/drm_vblank.c | 32 ++++++++++++++++++++++---------- > 1 file changed, 22 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > index 603ab105125d..4e83f1dfd446 100644 > --- a/drivers/gpu/drm/drm_vblank.c > +++ b/drivers/gpu/drm/drm_vblank.c > @@ -31,7 +31,6 @@ > #include <drm/drm_drv.h> > #include <drm/drm_framebuffer.h> > #include <drm/drm_print.h> > -#include <drm/drm_os_linux.h> > #include <drm/drm_vblank.h> > > #include "drm_internal.h" > @@ -1576,7 +1575,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data, > struct drm_crtc *crtc; > struct drm_vblank_crtc *vblank; > union drm_wait_vblank *vblwait = data; > - int ret; > + int ret, wait; > u64 req_seq, seq; > unsigned int pipe_index; > unsigned int flags, pipe, high_pipe; > @@ -1669,22 +1668,35 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data, > return drm_queue_vblank_event(dev, pipe, req_seq, vblwait, file_priv); > } > > + wait = 1; Instead of this hack I think better to remap the ret value in the if condition. This here is really hard to read. -Daniel > if (req_seq != seq) { > DRM_DEBUG("waiting on vblank count %llu, crtc %u\n", > req_seq, pipe); > - DRM_WAIT_ON(ret, vblank->queue, 3 * HZ, > - vblank_passed(drm_vblank_count(dev, pipe), > - req_seq) || > - !READ_ONCE(vblank->enabled)); > + wait = wait_event_interruptible_timeout(vblank->queue, > + vblank_passed(drm_vblank_count(dev, pipe), req_seq) || > + !READ_ONCE(vblank->enabled), > + msecs_to_jiffies(3000)); > } > > - if (ret != -EINTR) { > + switch (wait) { > + case 0: > + /* timeout */ > + ret = -EBUSY; > drm_wait_vblank_reply(dev, pipe, &vblwait->reply); > - > - DRM_DEBUG("crtc %d returning %u to client\n", > + DRM_DEBUG("timeout waiting for vblank. crtc %d returning %u to client\n", > pipe, vblwait->reply.sequence); > - } else { > + break; > + case -ERESTARTSYS: > + /* interrupted by signal */ > + ret = -EINTR; > DRM_DEBUG("crtc %d vblank wait interrupted by signal\n", pipe); > + break; > + default: > + ret = 0; > + drm_wait_vblank_reply(dev, pipe, &vblwait->reply); > + DRM_DEBUG("crtc %d returning %u to client\n", > + pipe, vblwait->reply.sequence); > + break; > } > > done: > -- > 2.20.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi Daniel. On Fri, Jul 19, 2019 at 11:39:26AM +0200, Daniel Vetter wrote: > On Thu, Jul 18, 2019 at 09:47:09PM +0200, Sam Ravnborg wrote: > > From e6f70cb90e0c3c90d45017a8257353652b7e0dcc Mon Sep 17 00:00:00 2001 > > From: Sam Ravnborg <sam@ravnborg.org> > > Date: Thu, 30 May 2019 09:38:47 +0200 > > Subject: [PATCH] drm/vblank: drop use of DRM_WAIT_ON() > > > > DRM_WAIT_ON() is from the deprecated drm_os_linux header and > > the modern replacement is the wait_event_*. > > > > The return values differ, so a conversion is needed to > > keep the original interface towards userspace. > > Introduced a switch/case to make code obvious and to allow > > different debug prints depending on the result. > > > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > > Reviewed-by: Sean Paul <sean@poorly.run> > > A bit bold to keep the r-b for this substantial bugfix to the patch. > > Also not per-patch changelog here. Plus doesn't seem to be on dri-devel > somehow. It was a quick hack that I just throw after IGT for testing and did not really consider that there are subscribers on the mailing list too. Should have dealt with this like any public patch. The patch in question handle the situation where req_seq equals seq. This was not correct before. I looked at the IGT test results while they were running and concluded that the run was bad - was suprised to see it passed this morning. Anyway, as pointed out on irc and in your other mail, there is a significant difference in polling or just waiting with some explicit wakeups. So in v3 I have just opencoded DRM_WAIT_ON() - but I will post this only after I have done some more local testing. Hopefully tonight if the weather is not too good. If, on the other hand, you think I should try to move forward with wait_event_* then I will address your feedback below. It needs to be simpler and more clear what is going on. Sam > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Maxime Ripard <maxime.ripard@bootlin.com> > > Cc: David Airlie <airlied@linux.ie> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > --- > > > > To verify if it works with the fix. > > (Added wat variable to handle the situation where we never wait) > > > > Sam > > > > drivers/gpu/drm/drm_vblank.c | 32 ++++++++++++++++++++++---------- > > 1 file changed, 22 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > > index 603ab105125d..4e83f1dfd446 100644 > > --- a/drivers/gpu/drm/drm_vblank.c > > +++ b/drivers/gpu/drm/drm_vblank.c > > @@ -31,7 +31,6 @@ > > #include <drm/drm_drv.h> > > #include <drm/drm_framebuffer.h> > > #include <drm/drm_print.h> > > -#include <drm/drm_os_linux.h> > > #include <drm/drm_vblank.h> > > > > #include "drm_internal.h" > > @@ -1576,7 +1575,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data, > > struct drm_crtc *crtc; > > struct drm_vblank_crtc *vblank; > > union drm_wait_vblank *vblwait = data; > > - int ret; > > + int ret, wait; > > u64 req_seq, seq; > > unsigned int pipe_index; > > unsigned int flags, pipe, high_pipe; > > @@ -1669,22 +1668,35 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data, > > return drm_queue_vblank_event(dev, pipe, req_seq, vblwait, file_priv); > > } > > > > + wait = 1; > > Instead of this hack I think better to remap the ret value in the if > condition. This here is really hard to read. > -Daniel > > > if (req_seq != seq) { > > DRM_DEBUG("waiting on vblank count %llu, crtc %u\n", > > req_seq, pipe); > > - DRM_WAIT_ON(ret, vblank->queue, 3 * HZ, > > - vblank_passed(drm_vblank_count(dev, pipe), > > - req_seq) || > > - !READ_ONCE(vblank->enabled)); > > + wait = wait_event_interruptible_timeout(vblank->queue, > > + vblank_passed(drm_vblank_count(dev, pipe), req_seq) || > > + !READ_ONCE(vblank->enabled), > > + msecs_to_jiffies(3000)); > > } > > > > - if (ret != -EINTR) { > > + switch (wait) { > > + case 0: > > + /* timeout */ > > + ret = -EBUSY; > > drm_wait_vblank_reply(dev, pipe, &vblwait->reply); > > - > > - DRM_DEBUG("crtc %d returning %u to client\n", > > + DRM_DEBUG("timeout waiting for vblank. crtc %d returning %u to client\n", > > pipe, vblwait->reply.sequence); > > - } else { > > + break; > > + case -ERESTARTSYS: > > + /* interrupted by signal */ > > + ret = -EINTR; > > DRM_DEBUG("crtc %d vblank wait interrupted by signal\n", pipe); > > + break; > > + default: > > + ret = 0; > > + drm_wait_vblank_reply(dev, pipe, &vblwait->reply); > > + DRM_DEBUG("crtc %d returning %u to client\n", > > + pipe, vblwait->reply.sequence); > > + break; > > } > > > > done: > > -- > > 2.20.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index 603ab105125d..4e83f1dfd446 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -31,7 +31,6 @@ #include <drm/drm_drv.h> #include <drm/drm_framebuffer.h> #include <drm/drm_print.h> -#include <drm/drm_os_linux.h> #include <drm/drm_vblank.h> #include "drm_internal.h" @@ -1576,7 +1575,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data, struct drm_crtc *crtc; struct drm_vblank_crtc *vblank; union drm_wait_vblank *vblwait = data; - int ret; + int ret, wait; u64 req_seq, seq; unsigned int pipe_index; unsigned int flags, pipe, high_pipe; @@ -1669,22 +1668,35 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data, return drm_queue_vblank_event(dev, pipe, req_seq, vblwait, file_priv); } + wait = 1; if (req_seq != seq) { DRM_DEBUG("waiting on vblank count %llu, crtc %u\n", req_seq, pipe); - DRM_WAIT_ON(ret, vblank->queue, 3 * HZ, - vblank_passed(drm_vblank_count(dev, pipe), - req_seq) || - !READ_ONCE(vblank->enabled)); + wait = wait_event_interruptible_timeout(vblank->queue, + vblank_passed(drm_vblank_count(dev, pipe), req_seq) || + !READ_ONCE(vblank->enabled), + msecs_to_jiffies(3000)); } - if (ret != -EINTR) { + switch (wait) { + case 0: + /* timeout */ + ret = -EBUSY; drm_wait_vblank_reply(dev, pipe, &vblwait->reply); - - DRM_DEBUG("crtc %d returning %u to client\n", + DRM_DEBUG("timeout waiting for vblank. crtc %d returning %u to client\n", pipe, vblwait->reply.sequence); - } else { + break; + case -ERESTARTSYS: + /* interrupted by signal */ + ret = -EINTR; DRM_DEBUG("crtc %d vblank wait interrupted by signal\n", pipe); + break; + default: + ret = 0; + drm_wait_vblank_reply(dev, pipe, &vblwait->reply); + DRM_DEBUG("crtc %d returning %u to client\n", + pipe, vblwait->reply.sequence); + break; } done: