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

[Rodrigo Siqueira]

Hi Sam,

I tested it by using VKMS and kms_flip, and tests related to âvblankâ
fails (e.g., basic-flip-vs-wf_vblank, blocking-absolute-wf_vblank,
flip-vs-absolute-wf_vblank, etc). I tried to dig into this issue, and
you can see my comments inline:

On Thu, Jun 13, 2019 at 2:04 AM Sam Ravnborg <sam@xxxxxxxxxxxx> wrote:
>
> 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));

Maybe Iâm wrong, but msecs_to_jiffies(30) is much smaller than 3 * HZ. Right?

>         }
>
> -       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;

After applying your patch, I noticed that drm_wait_vblank_ioctl()
consistently returns EBUSY, which is the cause of the errors in the
userspace. After that, I decided to take a look at DRM_WAIT_ON; See
below the code and my comments:

#define DRM_WAIT_ON( ret, queue, timeout, condition )        \
do {                                \
    DECLARE_WAITQUEUE(entry, current);            \
    unsigned long end = jiffies + (timeout);        \
    add_wait_queue(&(queue), &entry);            \
                                \
    for (;;) {                        \
        __set_current_state(TASK_INTERRUPTIBLE);    \
        if (condition)                    \
            break;                    \
        if (time_after_eq(jiffies, end)) {        \
            ret = -EBUSY;                \
            break;                    \
        }                        \

I think that your code does not handle this condition for EBUSY in the
same way of DRM_WAIT_ON(), or did I miss something?

Best regards

> +               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);
>         }
>


-- 

Rodrigo Siqueira
https://siqueira.tech