[1/1] drm/vblank: drop use of DRM_WAIT_ON()
diff mbox series

Message ID 20190718194709.GA11103@ravnborg.org
State New
Headers show
Series
  • [1/1] drm/vblank: drop use of DRM_WAIT_ON()
Related show

Commit Message

Sam Ravnborg July 18, 2019, 7:47 p.m. UTC
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>
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(-)

Comments

Daniel Vetter July 19, 2019, 9:39 a.m. UTC | #1
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
Sam Ravnborg July 19, 2019, 11:14 a.m. UTC | #2
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

Patch
diff mbox series

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: