[PATCH v3 1/1] drm/vblank: drop use of DRM_WAIT_ON()

[PATCH v3 1/1] drm/vblank: drop use of DRM_WAIT_ON()

[Sam Ravnborg]

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.

Analysis from Michel Dänzer:

The waiting condition rely on all relevant places where vblank_count
is modified calls wake_up(&vblank->queue).

drm_handle_vblank():
- Calls wake_up(&vblank->queue)

drm_vblank_enable():
- There is no need here because there can be no sleeping waiters
  in the queue, because vblank->enabled == false immediately
  terminates any waits.

drm_crtc_accurate_vblank_count():
- This is called from interrupt handlers, at least from
  amdgpu_dm.c:dm_pflip_high_irq(). Not sure it needs to wake up
  the queue though, the driver should call
  drm_(crtc_)_handle_vblank anyway.

drm_vblank_disable_and_save():
- It can be called from an interrupt, via drm_handle_vblank ->
  vblank_disable_fn. However, the only place where
  drm_vblank_disable_and_save can be called with sleeping waiters
  in the queue is in drm_crtc_vblank_off, which wakes up the queue
  afterwards (which terminates all waits, because
  vblank->enabled == false at this point).

v3:
- Added analysis to changelog from Michel Dänzer
- Moved return result handling inside if (req_seq != seq) (Daniel V)
- Reused more of the former logic - resulting in simpler code
- Dropped Reviewed-by from Sean Paul as this is a new implmentation

v2:
- Fix so the case where req_seq equals seq was handled properly
- quick hack to check if IGT became happy
- Only sent to igt, not to dri-devel

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: "Michel Dänzer" <michel@daenzer.net>
Cc: 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>
---

This patch survives my testing here.
vbltest spits out the same value as before, and I can see something on
the screen.
Added intel-gfx@lists.freedesktop.org, as IGT have identified troubles
with this in v1.

	Sam

 drivers/gpu/drm/drm_vblank.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 603ab105125d..fd1fbc77871f 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"
@@ -1670,12 +1669,28 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
 	}
 
 	if (req_seq != seq) {
+		int wait;
+
 		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));
+
+		switch (wait) {
+		case 0:
+			/* timeout */
+			ret = -EBUSY;
+			break;
+		case -ERESTARTSYS:
+			/* interrupted by signal */
+			ret = -EINTR;
+			break;
+		default:
+			ret = 0;
+			break;
+		}
 	}
 
 	if (ret != -EINTR) {
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/1] drm/vblank: drop use of DRM_WAIT_ON()

[Michel Dänzer]

On 2019-07-26 11:06 p.m., Sam Ravnborg wrote:
> 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.
> 
> Analysis from Michel Dänzer:
> 
> The waiting condition rely on all relevant places where vblank_count
> is modified calls wake_up(&vblank->queue).
> 
> drm_handle_vblank():
> - Calls wake_up(&vblank->queue)
> 
> drm_vblank_enable():
> - There is no need here because there can be no sleeping waiters
>   in the queue, because vblank->enabled == false immediately
>   terminates any waits.
> 
> drm_crtc_accurate_vblank_count():
> - This is called from interrupt handlers, at least from
>   amdgpu_dm.c:dm_pflip_high_irq(). Not sure it needs to wake up
>   the queue though, the driver should call
>   drm_(crtc_)_handle_vblank anyway.
> 
> drm_vblank_disable_and_save():
> - It can be called from an interrupt, via drm_handle_vblank ->
>   vblank_disable_fn. However, the only place where
>   drm_vblank_disable_and_save can be called with sleeping waiters
>   in the queue is in drm_crtc_vblank_off, which wakes up the queue
>   afterwards (which terminates all waits, because
>   vblank->enabled == false at this point).
> 
> v3:
> - Added analysis to changelog from Michel Dänzer
> - Moved return result handling inside if (req_seq != seq) (Daniel V)
> - Reused more of the former logic - resulting in simpler code
> - Dropped Reviewed-by from Sean Paul as this is a new implmentation
> 
> v2:
> - Fix so the case where req_seq equals seq was handled properly
> - quick hack to check if IGT became happy
> - Only sent to igt, not to dri-devel
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: "Michel Dänzer" <michel@daenzer.net>
> Cc: 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>

Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>


-- 
Earthling Michel Dänzer               |              https://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/1] drm/vblank: drop use of DRM_WAIT_ON()

[Sam Ravnborg]

Hi all.

On Fri, Jul 26, 2019 at 11:06:58PM +0200, Sam Ravnborg wrote:
> 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.

Patch passes igt testing, and Michel was happy with it.
It is now applied to drm-misc-next and pushed out.

With this we have one less user of drm_os_linux.h
(only very few remains)

And one less cryptic macro use.

	Sam

> 
> Analysis from Michel Dänzer:
> 
> The waiting condition rely on all relevant places where vblank_count
> is modified calls wake_up(&vblank->queue).
> 
> drm_handle_vblank():
> - Calls wake_up(&vblank->queue)
> 
> drm_vblank_enable():
> - There is no need here because there can be no sleeping waiters
>   in the queue, because vblank->enabled == false immediately
>   terminates any waits.
> 
> drm_crtc_accurate_vblank_count():
> - This is called from interrupt handlers, at least from
>   amdgpu_dm.c:dm_pflip_high_irq(). Not sure it needs to wake up
>   the queue though, the driver should call
>   drm_(crtc_)_handle_vblank anyway.
> 
> drm_vblank_disable_and_save():
> - It can be called from an interrupt, via drm_handle_vblank ->
>   vblank_disable_fn. However, the only place where
>   drm_vblank_disable_and_save can be called with sleeping waiters
>   in the queue is in drm_crtc_vblank_off, which wakes up the queue
>   afterwards (which terminates all waits, because
>   vblank->enabled == false at this point).
> 
> v3:
> - Added analysis to changelog from Michel Dänzer
> - Moved return result handling inside if (req_seq != seq) (Daniel V)
> - Reused more of the former logic - resulting in simpler code
> - Dropped Reviewed-by from Sean Paul as this is a new implmentation
> 
> v2:
> - Fix so the case where req_seq equals seq was handled properly
> - quick hack to check if IGT became happy
> - Only sent to igt, not to dri-devel
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: "Michel Dänzer" <michel@daenzer.net>
> Cc: 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>
> ---
> 
> This patch survives my testing here.
> vbltest spits out the same value as before, and I can see something on
> the screen.
> Added intel-gfx@lists.freedesktop.org, as IGT have identified troubles
> with this in v1.
> 
> 	Sam
> 
>  drivers/gpu/drm/drm_vblank.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 603ab105125d..fd1fbc77871f 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"
> @@ -1670,12 +1669,28 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
>  	}
>  
>  	if (req_seq != seq) {
> +		int wait;
> +
>  		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));
> +
> +		switch (wait) {
> +		case 0:
> +			/* timeout */
> +			ret = -EBUSY;
> +			break;
> +		case -ERESTARTSYS:
> +			/* interrupted by signal */
> +			ret = -EINTR;
> +			break;
> +		default:
> +			ret = 0;
> +			break;
> +		}
>  	}
>  
>  	if (ret != -EINTR) {
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx