Drop use of DRM_WAIT_ON() [Was: drm/drm_vblank: Change EINVAL by the correct errno]

Sam Ravnborg
2019-06-13T05:04:03Z

Hi Rodrigo.

On Wed, Jun 12, 2019 at 11:10:54PM -0300, Rodrigo Siqueira wrote:
> For historical reason, the function drm_wait_vblank_ioctl always return
> -EINVAL if something gets wrong. This scenario limits the flexibility
> for the userspace make detailed verification of the problem and take
> some action. In particular, the validation of “if (!dev->irq_enabled)”
> in the drm_wait_vblank_ioctl is responsible for checking if the driver
> support vblank or not. If the driver does not support VBlank, the
> function drm_wait_vblank_ioctl returns EINVAL which does not represent
> the real issue; this patch changes this behavior by return EOPNOTSUPP.
> Additionally, some operations are unsupported by this function, and
> returns EINVAL; this patch also changes the return value to EOPNOTSUPP
> in this case. Lastly, the function drm_wait_vblank_ioctl is invoked by
> libdrm, which is used by many compositors; because of this, it is
> important to check if this change breaks any compositor. In this sense,
> the following projects were examined:
>
> * Drm-hwcomposer
> * Kwin
> * Sway
> * Wlroots
> * Wayland-core
> * Weston
> * Xorg (67 different drivers)
>
> For each repository the verification happened in three steps:
>
> * Update the main branch
> * Look for any occurrence "drmWaitVBlank" with the command:
> git grep -n "drmWaitVBlank"
> * Look in the git history of the project with the command:
> git log -SdrmWaitVBlank
>
> Finally, none of the above projects validate the use of EINVAL which
> make safe, at least for these projects, to change the return values.
>
> Change since V2:
> Daniel Vetter and Chris Wilson
> - Replace ENOTTY by EOPNOTSUPP
> - Return EINVAL if the parameters are wrong
>
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> ---
> Update:
> Now IGT has a way to validate if a driver has vblank support or not.
> See: https://gitlab.freedesktop.org/drm/igt-gpu-tools/commit/2d244aed69165753f3adbbd6468db073dc1acf9A
>
> drivers/gpu/drm/drm_vblank.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 0d704bddb1a6..d76a783a7d4b 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -1578,10 +1578,10 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
> unsigned int flags, pipe, high_pipe;
>
> if (!dev->irq_enabled)
> - return -EINVAL;
> + return -EOPNOTSUPP;
>
> if (vblwait->request.type & _DRM_VBLANK_SIGNAL)
> - return -EINVAL;
> + return -EOPNOTSUPP;
>
> if (vblwait->request.type &
> ~(_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK |

When touching this function, could I ask you to take a look at
eliminating the use of DRM_WAIT_ON()?
It comes from the deprecated drm_os_linux.h header, and it is only of
the few remaining users of DRM_WAIT_ON().

Below you can find my untested first try - where I did an attempt not to
change behaviour.

Sam

commit 17b119b02467356198b57bca9949b146082bcaa1
Author: Sam Ravnborg <sam@ravnborg.org>
Date: Thu May 30 09:38:47 2019 +0200

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.

The timeout value of 3 * HZ was translated to 30 msec

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>
Cc: Sean Paul <sean@poorly.run>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 0d704bddb1a6..51fc6b106333 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"
@@ -1668,18 +1667,27 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
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));
+ ret = wait_event_interruptible_timeout(vblank->queue,
+ vblank_passed(drm_vblank_count(dev, pipe), req_seq) ||
+ !READ_ONCE(vblank->enabled),
+ msecs_to_jiffies(30));
}

- if (ret != -EINTR) {
+ switch (ret) {
+ case 1:
+ ret = 0;
drm_wait_vblank_reply(dev, pipe, &vblwait->reply);
-
DRM_DEBUG("crtc %d returning %u to client\n",
pipe, vblwait->reply.sequence);
- } else {
+ break;
+ case 0:
+ ret = -EBUSY;
+ drm_wait_vblank_reply(dev, pipe, &vblwait->reply);
+ DRM_DEBUG("timeout waiting for vblank. crtc %d returning %u to client\n",
+ pipe, vblwait->reply.sequence);
+ break;
+ default:
+ ret = -EINTR;
DRM_DEBUG("crtc %d vblank wait interrupted by signal\n", pipe);
}