* [RESEND PATCH V3] drm/drm_vblank: Change EINVAL by the correct errno
@ 2019-06-13 2:10 Rodrigo Siqueira
2019-06-13 5:04 ` Drop use of DRM_WAIT_ON() [Was: drm/drm_vblank: Change EINVAL by the correct errno] Sam Ravnborg
2019-06-13 8:21 ` [RESEND PATCH V3] drm/drm_vblank: Change EINVAL by the correct errno Daniel Vetter
0 siblings, 2 replies; 6+ messages in thread
From: Rodrigo Siqueira @ 2019-06-13 2:10 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie, Daniel Vetter
Cc: intel-gfx, kernel-janitors, linux-kernel, dri-devel
[-- Attachment #1: Type: text/plain, Size: 2558 bytes --]
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 |
--
2.21.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Drop use of DRM_WAIT_ON() [Was: drm/drm_vblank: Change EINVAL by the correct errno]
2019-06-13 2:10 [RESEND PATCH V3] drm/drm_vblank: Change EINVAL by the correct errno Rodrigo Siqueira
@ 2019-06-13 5:04 ` Sam Ravnborg
2019-06-13 18:44 ` Sam Ravnborg
` (2 more replies)
2019-06-13 8:21 ` [RESEND PATCH V3] drm/drm_vblank: Change EINVAL by the correct errno Daniel Vetter
1 sibling, 3 replies; 6+ messages in thread
From: Sam Ravnborg @ 2019-06-13 5:04 UTC (permalink / raw)
To: Rodrigo Siqueira
Cc: Maxime Ripard, kernel-janitors, intel-gfx, linux-kernel,
dri-devel, David Airlie
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);
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RESEND PATCH V3] drm/drm_vblank: Change EINVAL by the correct errno
2019-06-13 2:10 [RESEND PATCH V3] drm/drm_vblank: Change EINVAL by the correct errno Rodrigo Siqueira
2019-06-13 5:04 ` Drop use of DRM_WAIT_ON() [Was: drm/drm_vblank: Change EINVAL by the correct errno] Sam Ravnborg
@ 2019-06-13 8:21 ` Daniel Vetter
1 sibling, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2019-06-13 8:21 UTC (permalink / raw)
To: Rodrigo Siqueira
Cc: Maxime Ripard, kernel-janitors, intel-gfx, linux-kernel,
dri-devel, David Airlie, Sean Paul
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;
Not sure we want EINVAL here, that's kinda a "parameters are wrong"
version too. With that changed:
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> if (vblwait->request.type &
> ~(_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK |
> --
> 2.21.0
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Drop use of DRM_WAIT_ON() [Was: drm/drm_vblank: Change EINVAL by the correct errno]
2019-06-13 5:04 ` Drop use of DRM_WAIT_ON() [Was: drm/drm_vblank: Change EINVAL by the correct errno] Sam Ravnborg
@ 2019-06-13 18:44 ` Sam Ravnborg
2019-06-13 18:55 ` Rodrigo Siqueira
2019-06-14 17:20 ` Ville Syrjälä
2 siblings, 0 replies; 6+ messages in thread
From: Sam Ravnborg @ 2019-06-13 18:44 UTC (permalink / raw)
To: Rodrigo Siqueira
Cc: Maxime Ripard, kernel-janitors, intel-gfx, linux-kernel,
dri-devel, David Airlie, Sean Paul
Hi Rodrigo et al.
On Thu, Jun 13, 2019 at 07:04:03AM +0200, Sam Ravnborg 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@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.
intel-gfx did not like the patch - so no need to spend time looking at
the patch until I have that fixed.
Sam
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Drop use of DRM_WAIT_ON() [Was: drm/drm_vblank: Change EINVAL by the correct errno]
2019-06-13 5:04 ` Drop use of DRM_WAIT_ON() [Was: drm/drm_vblank: Change EINVAL by the correct errno] Sam Ravnborg
2019-06-13 18:44 ` Sam Ravnborg
@ 2019-06-13 18:55 ` Rodrigo Siqueira
2019-06-14 17:20 ` Ville Syrjälä
2 siblings, 0 replies; 6+ messages in thread
From: Rodrigo Siqueira @ 2019-06-13 18:55 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Maxime Ripard, kernel-janitors, Intel GFX ML,
Linux Kernel Mailing List, dri-devel, David Airlie, Sean Paul
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@ravnborg.org> 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@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));
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Drop use of DRM_WAIT_ON() [Was: drm/drm_vblank: Change EINVAL by the correct errno]
2019-06-13 5:04 ` Drop use of DRM_WAIT_ON() [Was: drm/drm_vblank: Change EINVAL by the correct errno] Sam Ravnborg
2019-06-13 18:44 ` Sam Ravnborg
2019-06-13 18:55 ` Rodrigo Siqueira
@ 2019-06-14 17:20 ` Ville Syrjälä
2 siblings, 0 replies; 6+ messages in thread
From: Ville Syrjälä @ 2019-06-14 17:20 UTC (permalink / raw)
To: Sam Ravnborg
Cc: Rodrigo Siqueira, Maxime Ripard, intel-gfx, kernel-janitors,
linux-kernel, dri-devel, David Airlie, Sean Paul
On Thu, Jun 13, 2019 at 07:04:03AM +0200, Sam Ravnborg 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@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().
IIRC all previous attempts at removing that ended up with
regressions. I think there are some dragons lurking inside that
macro.
--
Ville Syrjälä
Intel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-14 17:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-13 2:10 [RESEND PATCH V3] drm/drm_vblank: Change EINVAL by the correct errno Rodrigo Siqueira
2019-06-13 5:04 ` Drop use of DRM_WAIT_ON() [Was: drm/drm_vblank: Change EINVAL by the correct errno] Sam Ravnborg
2019-06-13 18:44 ` Sam Ravnborg
2019-06-13 18:55 ` Rodrigo Siqueira
2019-06-14 17:20 ` Ville Syrjälä
2019-06-13 8:21 ` [RESEND PATCH V3] drm/drm_vblank: Change EINVAL by the correct errno Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).