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@xxxxxxxxx> > --- > 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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> Cc: Maxime Ripard <maxime.ripard@xxxxxxxxxxx> Cc: Sean Paul <sean@xxxxxxxxxx> Cc: David Airlie <airlied@xxxxxxxx> Cc: Daniel Vetter <daniel@xxxxxxxx> 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); } _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
