All of lore.kernel.org
 help / color / mirror / Atom feed
* [RESEND PATCH V3] drm/drm_vblank: Change EINVAL by the correct errno
@ 2019-06-13  2:10 ` Rodrigo Siqueira
  0 siblings, 0 replies; 21+ 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: dri-devel, linux-kernel, kernel-janitors, intel-gfx

[-- 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] 21+ messages in thread

* [RESEND PATCH V3] drm/drm_vblank: Change EINVAL by the correct errno
@ 2019-06-13  2:10 ` Rodrigo Siqueira
  0 siblings, 0 replies; 21+ 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] 21+ messages in thread

* [RESEND PATCH V3] drm/drm_vblank: Change EINVAL by the correct errno
@ 2019-06-13  2:10 ` Rodrigo Siqueira
  0 siblings, 0 replies; 21+ 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.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 #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* ✓ Fi.CI.BAT: success for drm/drm_vblank: Change EINVAL by the correct errno (rev2)
  2019-06-13  2:10 ` Rodrigo Siqueira
  (?)
  (?)
@ 2019-06-13  3:43 ` Patchwork
  -1 siblings, 0 replies; 21+ messages in thread
From: Patchwork @ 2019-06-13  3:43 UTC (permalink / raw)
  To: Brian Starkey; +Cc: intel-gfx

== Series Details ==

Series: drm/drm_vblank: Change EINVAL by the correct errno (rev2)
URL   : https://patchwork.freedesktop.org/series/51147/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6253 -> Patchwork_13262
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/

Known issues
------------

  Here are the changes found in Patchwork_13262 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_flink_basic@flink-lifetime:
    - fi-icl-u3:          [PASS][1] -> [DMESG-WARN][2] ([fdo#107724])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-icl-u3/igt@gem_flink_basic@flink-lifetime.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/fi-icl-u3/igt@gem_flink_basic@flink-lifetime.html

  * igt@i915_selftest@live_hangcheck:
    - fi-icl-u3:          [PASS][3] -> [INCOMPLETE][4] ([fdo#107713] / [fdo#108569])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-icl-u3/igt@i915_selftest@live_hangcheck.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/fi-icl-u3/igt@i915_selftest@live_hangcheck.html

  
#### Possible fixes ####

  * igt@gem_busy@busy-all:
    - fi-icl-guc:         [INCOMPLETE][5] ([fdo#107713]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-icl-guc/igt@gem_busy@busy-all.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/fi-icl-guc/igt@gem_busy@busy-all.html

  * igt@gem_ctx_create@basic-files:
    - fi-icl-u2:          [INCOMPLETE][7] ([fdo#107713] / [fdo#109100]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-icl-u2/igt@gem_ctx_create@basic-files.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/fi-icl-u2/igt@gem_ctx_create@basic-files.html

  
#### Warnings ####

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - fi-kbl-guc:         [SKIP][9] ([fdo#109271]) -> [FAIL][10] ([fdo#110829])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-kbl-guc/igt@i915_pm_rpm@basic-pci-d3-state.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/fi-kbl-guc/igt@i915_pm_rpm@basic-pci-d3-state.html

  
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#110829]: https://bugs.freedesktop.org/show_bug.cgi?id=110829


Participating hosts (53 -> 47)
------------------------------

  Additional (1): fi-icl-dsi 
  Missing    (7): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * Linux: CI_DRM_6253 -> Patchwork_13262

  CI_DRM_6253: 83fdc69645c5c6b511e36e171f1c75a6132f007c @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5054: 7a295df596fdf71e5c28ecb1fbfec002060e9293 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13262: 00e5d8655d9def26db86da13e2730d682658c0e8 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

00e5d8655d9d drm/drm_vblank: Change EINVAL by the correct errno

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Drop use of DRM_WAIT_ON() [Was: drm/drm_vblank: Change EINVAL by the correct errno]
  2019-06-13  2:10 ` Rodrigo Siqueira
  (?)
@ 2019-06-13  5:04   ` Sam Ravnborg
  -1 siblings, 0 replies; 21+ messages in thread
From: Sam Ravnborg @ 2019-06-13  5:04 UTC (permalink / raw)
  To: Rodrigo Siqueira
  Cc: Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie,
	Daniel Vetter, intel-gfx, kernel-janitors, linux-kernel,
	dri-devel

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] 21+ messages in thread

* Drop use of DRM_WAIT_ON() [Was: drm/drm_vblank: Change EINVAL by the correct errno]
@ 2019-06-13  5:04   ` Sam Ravnborg
  0 siblings, 0 replies; 21+ 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] 21+ messages in thread

* Drop use of DRM_WAIT_ON() [Was: drm/drm_vblank: Change EINVAL by the correct errno]
@ 2019-06-13  5:04   ` Sam Ravnborg
  0 siblings, 0 replies; 21+ 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);
 	}
 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* ✗ Fi.CI.CHECKPATCH: warning for drm/drm_vblank: Change EINVAL by the correct errno (rev3)
  2019-06-13  2:10 ` Rodrigo Siqueira
                   ` (3 preceding siblings ...)
  (?)
@ 2019-06-13  5:17 ` Patchwork
  -1 siblings, 0 replies; 21+ messages in thread
From: Patchwork @ 2019-06-13  5:17 UTC (permalink / raw)
  To: Sam Ravnborg; +Cc: intel-gfx

== Series Details ==

Series: drm/drm_vblank: Change EINVAL by the correct errno (rev3)
URL   : https://patchwork.freedesktop.org/series/51147/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
8534a60ab43b Drop use of DRM_WAIT_ON() [Was: drm/drm_vblank: Change EINVAL by the correct errno]
-:16: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#16: 
> some action. In particular, the validation of “if (!dev->irq_enabled)”

-:89: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit fatal: bad o ("a9949b146082bcaa1")'
#89: 
commit 17b119b02467356198b57bca9949b146082bcaa1

-:105: WARNING:BAD_SIGN_OFF: Do not use whitespace before Signed-off-by:
#105: 
    Signed-off-by: Sam Ravnborg <sam@ravnborg.org>

-:106: WARNING:BAD_SIGN_OFF: Do not use whitespace before Cc:
#106: 
    Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

-:107: WARNING:BAD_SIGN_OFF: Do not use whitespace before Cc:
#107: 
    Cc: Maxime Ripard <maxime.ripard@bootlin.com>

-:108: WARNING:BAD_SIGN_OFF: Do not use whitespace before Cc:
#108: 
    Cc: Sean Paul <sean@poorly.run>

-:109: WARNING:BAD_SIGN_OFF: Do not use whitespace before Cc:
#109: 
    Cc: David Airlie <airlied@linux.ie>

-:110: WARNING:BAD_SIGN_OFF: Do not use whitespace before Cc:
#110: 
    Cc: Daniel Vetter <daniel@ffwll.ch>

-:133: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#133: FILE: drivers/gpu/drm/drm_vblank.c:1671:
+		ret = wait_event_interruptible_timeout(vblank->queue,
+			vblank_passed(drm_vblank_count(dev, pipe), req_seq) ||

total: 1 errors, 7 warnings, 1 checks, 41 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 21+ messages in thread

* ✗ Fi.CI.BAT: failure for drm/drm_vblank: Change EINVAL by the correct errno (rev3)
  2019-06-13  2:10 ` Rodrigo Siqueira
                   ` (4 preceding siblings ...)
  (?)
@ 2019-06-13  5:39 ` Patchwork
  -1 siblings, 0 replies; 21+ messages in thread
From: Patchwork @ 2019-06-13  5:39 UTC (permalink / raw)
  To: Sam Ravnborg; +Cc: intel-gfx

== Series Details ==

Series: drm/drm_vblank: Change EINVAL by the correct errno (rev3)
URL   : https://patchwork.freedesktop.org/series/51147/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_6253 -> Patchwork_13263
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_13263 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_13263, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_13263:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - fi-icl-y:           [PASS][1] -> [FAIL][2] +9 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-icl-y/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-icl-y/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
    - fi-byt-j1900:       [PASS][3] -> [FAIL][4] +9 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-byt-j1900/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-byt-j1900/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - fi-hsw-4770r:       [PASS][5] -> [FAIL][6] +9 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-hsw-4770r/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-hsw-4770r/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
    - fi-kbl-x1275:       [PASS][7] -> [FAIL][8] +9 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-kbl-x1275/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-kbl-x1275/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
    - fi-hsw-peppy:       [PASS][9] -> [FAIL][10] +9 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-hsw-peppy/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-hsw-peppy/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
    - fi-cfl-8700k:       [PASS][11] -> [FAIL][12] +9 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-cfl-8700k/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-cfl-8700k/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
    - fi-apl-guc:         [PASS][13] -> [FAIL][14] +9 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-apl-guc/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-apl-guc/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
    - fi-bdw-5557u:       [PASS][15] -> [FAIL][16] +9 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-bdw-5557u/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-bdw-5557u/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_cursor_legacy@basic-flip-after-cursor-atomic:
    - fi-icl-u3:          [PASS][17] -> [FAIL][18] +9 similar issues
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-icl-u3/igt@kms_cursor_legacy@basic-flip-after-cursor-atomic.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-icl-u3/igt@kms_cursor_legacy@basic-flip-after-cursor-atomic.html
    - fi-cml-u:           [PASS][19] -> [FAIL][20] +9 similar issues
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-cml-u/igt@kms_cursor_legacy@basic-flip-after-cursor-atomic.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-cml-u/igt@kms_cursor_legacy@basic-flip-after-cursor-atomic.html
    - fi-bxt-dsi:         [PASS][21] -> [FAIL][22] +9 similar issues
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-bxt-dsi/igt@kms_cursor_legacy@basic-flip-after-cursor-atomic.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-bxt-dsi/igt@kms_cursor_legacy@basic-flip-after-cursor-atomic.html

  * igt@kms_cursor_legacy@basic-flip-after-cursor-legacy:
    - fi-ivb-3770:        [PASS][23] -> [FAIL][24] +9 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-ivb-3770/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-ivb-3770/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html
    - fi-bxt-j4205:       [PASS][25] -> [FAIL][26] +9 similar issues
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-bxt-j4205/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-bxt-j4205/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html
    - fi-blb-e6850:       [PASS][27] -> [FAIL][28] +6 similar issues
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-blb-e6850/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-blb-e6850/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html

  * igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size:
    - fi-kbl-7567u:       [PASS][29] -> [FAIL][30] +9 similar issues
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-kbl-7567u/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-kbl-7567u/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html
    - fi-whl-u:           [PASS][31] -> [FAIL][32] +9 similar issues
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-whl-u/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-whl-u/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html
    - fi-snb-2600:        [PASS][33] -> [FAIL][34] +9 similar issues
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-snb-2600/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-snb-2600/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html
    - fi-bwr-2160:        [PASS][35] -> [FAIL][36] +6 similar issues
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-bwr-2160/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-bwr-2160/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html

  * igt@kms_cursor_legacy@basic-flip-before-cursor-atomic:
    - fi-snb-2520m:       [PASS][37] -> [FAIL][38] +9 similar issues
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-snb-2520m/igt@kms_cursor_legacy@basic-flip-before-cursor-atomic.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-snb-2520m/igt@kms_cursor_legacy@basic-flip-before-cursor-atomic.html

  * igt@kms_cursor_legacy@basic-flip-before-cursor-legacy:
    - fi-cml-u2:          [PASS][39] -> [FAIL][40] +9 similar issues
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-cml-u2/igt@kms_cursor_legacy@basic-flip-before-cursor-legacy.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-cml-u2/igt@kms_cursor_legacy@basic-flip-before-cursor-legacy.html

  * igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size:
    - fi-hsw-4770:        [PASS][41] -> [FAIL][42] +9 similar issues
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-hsw-4770/igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-hsw-4770/igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size.html
    - fi-cfl-guc:         [PASS][43] -> [FAIL][44] +9 similar issues
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-cfl-guc/igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-cfl-guc/igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size.html
    - fi-bdw-gvtdvm:      [PASS][45] -> [FAIL][46] +9 similar issues
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-bdw-gvtdvm/igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-bdw-gvtdvm/igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size.html
    - fi-kbl-7500u:       [PASS][47] -> [FAIL][48] +9 similar issues
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-kbl-7500u/igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-kbl-7500u/igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a:
    - fi-glk-dsi:         [PASS][49] -> [FAIL][50] +1 similar issue
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-glk-dsi/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-glk-dsi/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a.html
    - fi-skl-guc:         [PASS][51] -> [FAIL][52] +1 similar issue
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-skl-guc/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-skl-guc/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a.html
    - fi-skl-6600u:       [PASS][53] -> [FAIL][54] +1 similar issue
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-skl-6600u/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-skl-6600u/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a.html
    - fi-skl-6770hq:      [PASS][55] -> [FAIL][56] +1 similar issue
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-skl-6770hq/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-skl-6770hq/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a.html
    - fi-skl-gvtdvm:      [PASS][57] -> [FAIL][58] +1 similar issue
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-skl-gvtdvm/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-skl-gvtdvm/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a.html
    - fi-skl-lmem:        [PASS][59] -> [FAIL][60] +1 similar issue
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-skl-lmem/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-skl-lmem/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a.html
    - fi-skl-6260u:       [PASS][61] -> [FAIL][62] +1 similar issue
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-skl-6260u/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-skl-6260u/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a.html
    - fi-bsw-kefka:       [PASS][63] -> [FAIL][64] +1 similar issue
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-bsw-kefka/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-bsw-kefka/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
    - fi-ilk-650:         [PASS][65] -> [FAIL][66] +9 similar issues
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-ilk-650/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-ilk-650/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html
    - fi-elk-e7500:       [PASS][67] -> [FAIL][68] +6 similar issues
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-elk-e7500/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-elk-e7500/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html
    - fi-byt-n2820:       [PASS][69] -> [FAIL][70] +9 similar issues
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-byt-n2820/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-byt-n2820/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html
    - fi-icl-u2:          NOTRUN -> [FAIL][71] +9 similar issues
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-icl-u2/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html
    - fi-pnv-d510:        [PASS][72] -> [FAIL][73] +1 similar issue
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-pnv-d510/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-pnv-d510/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html
    - fi-cfl-8109u:       [PASS][74] -> [FAIL][75] +9 similar issues
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-cfl-8109u/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-cfl-8109u/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html
    - fi-gdg-551:         [PASS][76] -> [FAIL][77] +6 similar issues
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-gdg-551/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-gdg-551/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html
    - fi-skl-iommu:       [PASS][78] -> [FAIL][79] +1 similar issue
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-skl-iommu/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-skl-iommu/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html
    - fi-skl-6700k2:      [PASS][80] -> [FAIL][81] +1 similar issue
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-skl-6700k2/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-skl-6700k2/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence.html

  
Known issues
------------

  Here are the changes found in Patchwork_13263 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - fi-pnv-d510:        [PASS][82] -> [FAIL][83] ([fdo#106081]) +4 similar issues
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-pnv-d510/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-pnv-d510/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
    - fi-skl-6600u:       [PASS][84] -> [FAIL][85] ([fdo#106081]) +7 similar issues
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-skl-6600u/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-skl-6600u/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_cursor_legacy@basic-flip-after-cursor-legacy:
    - fi-skl-6770hq:      [PASS][86] -> [FAIL][87] ([fdo#106081]) +7 similar issues
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-skl-6770hq/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-skl-6770hq/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html
    - fi-skl-lmem:        [PASS][88] -> [FAIL][89] ([fdo#106081]) +7 similar issues
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-skl-lmem/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-skl-lmem/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html
    - fi-skl-gvtdvm:      [PASS][90] -> [FAIL][91] ([fdo#106081]) +7 similar issues
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-skl-gvtdvm/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html
   [91]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-skl-gvtdvm/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html

  * igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size:
    - fi-skl-guc:         [PASS][92] -> [FAIL][93] ([fdo#106081]) +7 similar issues
   [92]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-skl-guc/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html
   [93]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-skl-guc/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html
    - fi-skl-6260u:       [PASS][94] -> [FAIL][95] ([fdo#106081]) +7 similar issues
   [94]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-skl-6260u/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html
   [95]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-skl-6260u/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html

  * igt@kms_cursor_legacy@basic-flip-before-cursor-legacy:
    - fi-bsw-kefka:       [PASS][96] -> [FAIL][97] ([fdo#106081]) +7 similar issues
   [96]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-bsw-kefka/igt@kms_cursor_legacy@basic-flip-before-cursor-legacy.html
   [97]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-bsw-kefka/igt@kms_cursor_legacy@basic-flip-before-cursor-legacy.html

  * igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size:
    - fi-bsw-n3050:       [PASS][98] -> [FAIL][99] ([fdo#106081]) +7 similar issues
   [98]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-bsw-n3050/igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size.html
   [99]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-bsw-n3050/igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size.html
    - fi-skl-6700k2:      [PASS][100] -> [FAIL][101] ([fdo#106081]) +7 similar issues
   [100]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-skl-6700k2/igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size.html
   [101]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-skl-6700k2/igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size.html
    - fi-skl-iommu:       [PASS][102] -> [FAIL][103] ([fdo#106081]) +7 similar issues
   [102]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-skl-iommu/igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size.html
   [103]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-skl-iommu/igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size.html
    - fi-glk-dsi:         [PASS][104] -> [FAIL][105] ([fdo#106081]) +7 similar issues
   [104]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/fi-glk-dsi/igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size.html
   [105]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/fi-glk-dsi/igt@kms_curs

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13263/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [RESEND PATCH V3] drm/drm_vblank: Change EINVAL by the correct errno
  2019-06-13  2:10 ` Rodrigo Siqueira
  (?)
@ 2019-06-13  8:21   ` Daniel Vetter
  -1 siblings, 0 replies; 21+ messages in thread
From: Daniel Vetter @ 2019-06-13  8:21 UTC (permalink / raw)
  To: Rodrigo Siqueira
  Cc: Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie,
	Daniel Vetter, dri-devel, linux-kernel, kernel-janitors,
	intel-gfx

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] 21+ messages in thread

* Re: [RESEND PATCH V3] drm/drm_vblank: Change EINVAL by the correct errno
@ 2019-06-13  8:21   ` Daniel Vetter
  0 siblings, 0 replies; 21+ 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] 21+ messages in thread

* Re: [RESEND PATCH V3] drm/drm_vblank: Change EINVAL by the correct errno
@ 2019-06-13  8:21   ` Daniel Vetter
  0 siblings, 0 replies; 21+ 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 21+ 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   ` Sam Ravnborg
@ 2019-06-13 18:44     ` Sam Ravnborg
  -1 siblings, 0 replies; 21+ 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] 21+ messages in thread

* Re: Drop use of DRM_WAIT_ON() [Was: drm/drm_vblank: Change EINVAL by the correct errno]
@ 2019-06-13 18:44     ` Sam Ravnborg
  0 siblings, 0 replies; 21+ 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] 21+ 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   ` Sam Ravnborg
  (?)
@ 2019-06-13 18:55     ` Rodrigo Siqueira
  -1 siblings, 0 replies; 21+ messages in thread
From: Rodrigo Siqueira @ 2019-06-13 18:55 UTC (permalink / raw)
  To: Sam Ravnborg
  Cc: Maarten Lankhorst, Maxime Ripard, Sean Paul, David Airlie,
	Daniel Vetter, Intel GFX ML, kernel-janitors,
	Linux Kernel Mailing List, dri-devel

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] 21+ messages in thread

* Re: Drop use of DRM_WAIT_ON() [Was: drm/drm_vblank: Change EINVAL by the correct errno]
@ 2019-06-13 18:55     ` Rodrigo Siqueira
  0 siblings, 0 replies; 21+ 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] 21+ messages in thread

* Re: Drop use of DRM_WAIT_ON() [Was: drm/drm_vblank: Change EINVAL by the correct errno]
@ 2019-06-13 18:55     ` Rodrigo Siqueira
  0 siblings, 0 replies; 21+ 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 21+ messages in thread

* ✗ Fi.CI.IGT: failure for drm/drm_vblank: Change EINVAL by the correct errno (rev2)
  2019-06-13  2:10 ` Rodrigo Siqueira
                   ` (6 preceding siblings ...)
  (?)
@ 2019-06-14 16:54 ` Patchwork
  -1 siblings, 0 replies; 21+ messages in thread
From: Patchwork @ 2019-06-14 16:54 UTC (permalink / raw)
  To: Sam Ravnborg; +Cc: intel-gfx

== Series Details ==

Series: drm/drm_vblank: Change EINVAL by the correct errno (rev2)
URL   : https://patchwork.freedesktop.org/series/51147/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_6253_full -> Patchwork_13262_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_13262_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_13262_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_13262_full:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_vblank@invalid:
    - shard-kbl:          [PASS][1] -> [FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-kbl1/igt@kms_vblank@invalid.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-kbl4/igt@kms_vblank@invalid.html
    - shard-apl:          [PASS][3] -> [FAIL][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-apl1/igt@kms_vblank@invalid.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-apl4/igt@kms_vblank@invalid.html
    - shard-glk:          [PASS][5] -> [FAIL][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-glk1/igt@kms_vblank@invalid.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-glk5/igt@kms_vblank@invalid.html
    - shard-skl:          [PASS][7] -> [FAIL][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-skl9/igt@kms_vblank@invalid.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-skl1/igt@kms_vblank@invalid.html
    - shard-snb:          [PASS][9] -> [FAIL][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-snb1/igt@kms_vblank@invalid.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-snb5/igt@kms_vblank@invalid.html
    - shard-hsw:          [PASS][11] -> [FAIL][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-hsw1/igt@kms_vblank@invalid.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-hsw7/igt@kms_vblank@invalid.html

  
Known issues
------------

  Here are the changes found in Patchwork_13262_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_eio@in-flight-contexts-1us:
    - shard-glk:          [PASS][13] -> [DMESG-WARN][14] ([fdo#110913 ]) +1 similar issue
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-glk6/igt@gem_eio@in-flight-contexts-1us.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-glk9/igt@gem_eio@in-flight-contexts-1us.html

  * igt@gem_eio@in-flight-suspend:
    - shard-glk:          [PASS][15] -> [FAIL][16] ([fdo#110667])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-glk5/igt@gem_eio@in-flight-suspend.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-glk6/igt@gem_eio@in-flight-suspend.html

  * igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing:
    - shard-apl:          [PASS][17] -> [DMESG-WARN][18] ([fdo#110913 ]) +1 similar issue
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-apl6/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-apl8/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing.html
    - shard-skl:          [PASS][19] -> [DMESG-WARN][20] ([fdo#110913 ]) +2 similar issues
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-skl1/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-skl6/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing.html
    - shard-hsw:          [PASS][21] -> [DMESG-WARN][22] ([fdo#110789] / [fdo#110913 ])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-hsw4/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-hsw5/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing.html

  * igt@gem_persistent_relocs@forked-interruptible-thrashing:
    - shard-snb:          [PASS][23] -> [DMESG-WARN][24] ([fdo#110789] / [fdo#110913 ]) +1 similar issue
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-snb1/igt@gem_persistent_relocs@forked-interruptible-thrashing.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-snb5/igt@gem_persistent_relocs@forked-interruptible-thrashing.html

  * igt@gem_userptr_blits@map-fixed-invalidate-busy-gup:
    - shard-kbl:          [PASS][25] -> [DMESG-WARN][26] ([fdo#110913 ])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-kbl4/igt@gem_userptr_blits@map-fixed-invalidate-busy-gup.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-kbl1/igt@gem_userptr_blits@map-fixed-invalidate-busy-gup.html

  * igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy-gup:
    - shard-iclb:         [PASS][27] -> [DMESG-WARN][28] ([fdo#110913 ])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-iclb5/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy-gup.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-iclb2/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy-gup.html

  * igt@i915_selftest@mock_fence:
    - shard-iclb:         [PASS][29] -> [INCOMPLETE][30] ([fdo#107713])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-iclb7/igt@i915_selftest@mock_fence.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-iclb7/igt@i915_selftest@mock_fence.html

  * igt@kms_cursor_crc@pipe-b-cursor-suspend:
    - shard-apl:          [PASS][31] -> [DMESG-WARN][32] ([fdo#108566]) +1 similar issue
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-apl6/igt@kms_cursor_crc@pipe-b-cursor-suspend.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-apl8/igt@kms_cursor_crc@pipe-b-cursor-suspend.html

  * igt@kms_cursor_crc@pipe-c-cursor-128x42-sliding:
    - shard-kbl:          [PASS][33] -> [FAIL][34] ([fdo#103232])
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-kbl2/igt@kms_cursor_crc@pipe-c-cursor-128x42-sliding.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-kbl2/igt@kms_cursor_crc@pipe-c-cursor-128x42-sliding.html

  * igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
    - shard-glk:          [PASS][35] -> [FAIL][36] ([fdo#105363])
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-glk4/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-glk9/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible.html

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-skl:          [PASS][37] -> [FAIL][38] ([fdo#105363])
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-skl2/igt@kms_flip@flip-vs-expired-vblank.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-skl4/igt@kms_flip@flip-vs-expired-vblank.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render:
    - shard-iclb:         [PASS][39] -> [FAIL][40] ([fdo#103167]) +8 similar issues
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-iclb6/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-iclb5/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render.html

  * igt@kms_frontbuffer_tracking@fbc-suspend:
    - shard-skl:          [PASS][41] -> [INCOMPLETE][42] ([fdo#104108]) +1 similar issue
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-skl4/igt@kms_frontbuffer_tracking@fbc-suspend.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-skl2/igt@kms_frontbuffer_tracking@fbc-suspend.html

  * igt@kms_psr2_su@page_flip:
    - shard-iclb:         [PASS][43] -> [SKIP][44] ([fdo#109642])
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-iclb2/igt@kms_psr2_su@page_flip.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-iclb4/igt@kms_psr2_su@page_flip.html

  * igt@kms_psr@psr2_sprite_mmap_gtt:
    - shard-iclb:         [PASS][45] -> [SKIP][46] ([fdo#109441]) +1 similar issue
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-iclb2/igt@kms_psr@psr2_sprite_mmap_gtt.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-iclb5/igt@kms_psr@psr2_sprite_mmap_gtt.html

  * igt@kms_setmode@basic:
    - shard-kbl:          [PASS][47] -> [FAIL][48] ([fdo#99912])
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-kbl1/igt@kms_setmode@basic.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-kbl7/igt@kms_setmode@basic.html

  * igt@kms_sysfs_edid_timing:
    - shard-iclb:         [PASS][49] -> [FAIL][50] ([fdo#100047])
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-iclb5/igt@kms_sysfs_edid_timing.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-iclb3/igt@kms_sysfs_edid_timing.html

  * igt@kms_vblank@pipe-c-ts-continuation-suspend:
    - shard-kbl:          [PASS][51] -> [DMESG-WARN][52] ([fdo#108566]) +1 similar issue
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-kbl6/igt@kms_vblank@pipe-c-ts-continuation-suspend.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-kbl3/igt@kms_vblank@pipe-c-ts-continuation-suspend.html

  * igt@perf@polling:
    - shard-skl:          [PASS][53] -> [FAIL][54] ([fdo#110728])
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-skl10/igt@perf@polling.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-skl9/igt@perf@polling.html

  * igt@perf_pmu@rc6:
    - shard-kbl:          [PASS][55] -> [SKIP][56] ([fdo#109271])
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-kbl1/igt@perf_pmu@rc6.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-kbl7/igt@perf_pmu@rc6.html

  
#### Possible fixes ####

  * igt@gem_persistent_relocs@forked-faulting-reloc-thrashing:
    - shard-skl:          [DMESG-WARN][57] ([fdo#110913 ]) -> [PASS][58] +2 similar issues
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-skl6/igt@gem_persistent_relocs@forked-faulting-reloc-thrashing.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-skl6/igt@gem_persistent_relocs@forked-faulting-reloc-thrashing.html
    - shard-hsw:          [DMESG-WARN][59] ([fdo#110789] / [fdo#110913 ]) -> [PASS][60] +1 similar issue
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-hsw2/igt@gem_persistent_relocs@forked-faulting-reloc-thrashing.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-hsw4/igt@gem_persistent_relocs@forked-faulting-reloc-thrashing.html

  * igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing:
    - shard-snb:          [DMESG-WARN][61] ([fdo#110789] / [fdo#110913 ]) -> [PASS][62] +1 similar issue
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-snb4/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-snb1/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing.html
    - shard-kbl:          [DMESG-WARN][63] ([fdo#110913 ]) -> [PASS][64]
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-kbl4/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-kbl1/igt@gem_persistent_relocs@forked-interruptible-faulting-reloc-thrashing.html

  * igt@gem_persistent_relocs@forked-interruptible-thrashing:
    - shard-glk:          [DMESG-WARN][65] ([fdo#110913 ]) -> [PASS][66] +1 similar issue
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-glk6/igt@gem_persistent_relocs@forked-interruptible-thrashing.html
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-glk9/igt@gem_persistent_relocs@forked-interruptible-thrashing.html

  * igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy:
    - shard-iclb:         [DMESG-WARN][67] ([fdo#110913 ]) -> [PASS][68]
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-iclb5/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy.html
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-iclb2/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy.html
    - shard-hsw:          [DMESG-WARN][69] ([fdo#110913 ]) -> [PASS][70]
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-hsw6/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy.html
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-hsw7/igt@gem_userptr_blits@map-fixed-invalidate-overlap-busy.html

  * igt@gem_workarounds@suspend-resume-context:
    - shard-apl:          [DMESG-WARN][71] ([fdo#108566]) -> [PASS][72] +3 similar issues
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-apl7/igt@gem_workarounds@suspend-resume-context.html
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-apl8/igt@gem_workarounds@suspend-resume-context.html

  * igt@i915_suspend@fence-restore-untiled:
    - shard-skl:          [INCOMPLETE][73] ([fdo#104108]) -> [PASS][74]
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-skl6/igt@i915_suspend@fence-restore-untiled.html
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-skl4/igt@i915_suspend@fence-restore-untiled.html

  * igt@kms_cursor_crc@pipe-b-cursor-64x21-random:
    - shard-apl:          [FAIL][75] ([fdo#103232]) -> [PASS][76]
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-apl2/igt@kms_cursor_crc@pipe-b-cursor-64x21-random.html
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-apl2/igt@kms_cursor_crc@pipe-b-cursor-64x21-random.html
    - shard-skl:          [FAIL][77] ([fdo#103232]) -> [PASS][78]
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-skl7/igt@kms_cursor_crc@pipe-b-cursor-64x21-random.html
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-skl9/igt@kms_cursor_crc@pipe-b-cursor-64x21-random.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-blt:
    - shard-iclb:         [FAIL][79] ([fdo#103167]) -> [PASS][80] +3 similar issues
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-iclb7/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-blt.html
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-iclb4/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-blt.html

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-draw-pwrite:
    - shard-hsw:          [SKIP][81] ([fdo#109271]) -> [PASS][82] +28 similar issues
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-hsw1/igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-draw-pwrite.html
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-hsw2/igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-draw-pwrite.html

  * igt@kms_plane_lowres@pipe-a-tiling-x:
    - shard-iclb:         [FAIL][83] ([fdo#103166]) -> [PASS][84]
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-iclb7/igt@kms_plane_lowres@pipe-a-tiling-x.html
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-iclb4/igt@kms_plane_lowres@pipe-a-tiling-x.html

  * igt@kms_psr@no_drrs:
    - shard-iclb:         [FAIL][85] ([fdo#108341]) -> [PASS][86]
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-iclb1/igt@kms_psr@no_drrs.html
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-iclb3/igt@kms_psr@no_drrs.html

  * igt@kms_psr@psr2_primary_page_flip:
    - shard-iclb:         [SKIP][87] ([fdo#109441]) -> [PASS][88] +3 similar issues
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-iclb5/igt@kms_psr@psr2_primary_page_flip.html
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-iclb2/igt@kms_psr@psr2_primary_page_flip.html

  * igt@kms_setmode@basic:
    - shard-skl:          [FAIL][89] ([fdo#99912]) -> [PASS][90]
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-skl3/igt@kms_setmode@basic.html
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-skl7/igt@kms_setmode@basic.html

  
#### Warnings ####

  * igt@kms_frontbuffer_tracking@psr-2p-primscrn-shrfb-pgflip-blt:
    - shard-apl:          [INCOMPLETE][91] ([fdo#103927]) -> [SKIP][92] ([fdo#109271])
   [91]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6253/shard-apl2/igt@kms_frontbuffer_tracking@psr-2p-primscrn-shrfb-pgflip-blt.html
   [92]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/shard-apl7/igt@kms_frontbuffer_tracking@psr-2p-primscrn-shrfb-pgflip-blt.html

  
  [fdo#100047]: https://bugs.freedesktop.org/show_bug.cgi?id=100047
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#108341]: https://bugs.freedesktop.org/show_bug.cgi?id=108341
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#110667]: https://bugs.freedesktop.org/show_bug.cgi?id=110667
  [fdo#110728]: https://bugs.freedesktop.org/show_bug.cgi?id=110728
  [fdo#110789]: https://bugs.freedesktop.org/show_bug.cgi?id=110789
  [fdo#110913 ]: https://bugs.freedesktop.org/show_bug.cgi?id=110913 
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


Build changes
-------------

  * Linux: CI_DRM_6253 -> Patchwork_13262

  CI_DRM_6253: 83fdc69645c5c6b511e36e171f1c75a6132f007c @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5054: 7a295df596fdf71e5c28ecb1fbfec002060e9293 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13262: 00e5d8655d9def26db86da13e2730d682658c0e8 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13262/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 21+ 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   ` Sam Ravnborg
  (?)
@ 2019-06-14 17:20     ` Ville Syrjälä
  -1 siblings, 0 replies; 21+ messages in thread
From: Ville Syrjälä @ 2019-06-14 17:20 UTC (permalink / raw)
  To: Sam Ravnborg
  Cc: Rodrigo Siqueira, Maxime Ripard, kernel-janitors, intel-gfx,
	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] 21+ messages in thread

* Re: Drop use of DRM_WAIT_ON() [Was: drm/drm_vblank: Change EINVAL by the correct errno]
@ 2019-06-14 17:20     ` Ville Syrjälä
  0 siblings, 0 replies; 21+ 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] 21+ messages in thread

* Re: Drop use of DRM_WAIT_ON() [Was: drm/drm_vblank: Change EINVAL by the correct errno]
@ 2019-06-14 17:20     ` Ville Syrjälä
  0 siblings, 0 replies; 21+ 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2019-06-14 17:20 UTC | newest]

Thread overview: 21+ 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  2:10 ` Rodrigo Siqueira
2019-06-13  2:10 ` Rodrigo Siqueira
2019-06-13  3:43 ` ✓ Fi.CI.BAT: success for drm/drm_vblank: Change EINVAL by the correct errno (rev2) Patchwork
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  5:04   ` Sam Ravnborg
2019-06-13  5:04   ` Sam Ravnborg
2019-06-13 18:44   ` Sam Ravnborg
2019-06-13 18:44     ` Sam Ravnborg
2019-06-13 18:55   ` Rodrigo Siqueira
2019-06-13 18:55     ` Rodrigo Siqueira
2019-06-13 18:55     ` Rodrigo Siqueira
2019-06-14 17:20   ` Ville Syrjälä
2019-06-14 17:20     ` Ville Syrjälä
2019-06-14 17:20     ` Ville Syrjälä
2019-06-13  5:17 ` ✗ Fi.CI.CHECKPATCH: warning for drm/drm_vblank: Change EINVAL by the correct errno (rev3) Patchwork
2019-06-13  5:39 ` ✗ Fi.CI.BAT: failure " Patchwork
2019-06-13  8:21 ` [RESEND PATCH V3] drm/drm_vblank: Change EINVAL by the correct errno Daniel Vetter
2019-06-13  8:21   ` Daniel Vetter
2019-06-13  8:21   ` Daniel Vetter
2019-06-14 16:54 ` ✗ Fi.CI.IGT: failure for drm/drm_vblank: Change EINVAL by the correct errno (rev2) Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.