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 <s...@ravnborg.org>
> Cc: "Michel Dänzer" <mic...@daenzer.net>
> Cc: Sean Paul <s...@poorly.run>
> Cc: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> Cc: Maxime Ripard <maxime.rip...@bootlin.com>
> Cc: David Airlie <airl...@linux.ie>
> Cc: Daniel Vetter <dan...@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-...@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Reply via email to