[RESEND PATCH V3] drm/drm_vblank: Change EINVAL by the correct errno

[RESEND PATCH V3] drm/drm_vblank: Change EINVAL by the correct errno

[Rodrigo Siqueira]

[-- 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 --]

[RESEND PATCH V3] drm/drm_vblank: Change EINVAL by the correct errno

[Rodrigo Siqueira]

[-- 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

[RESEND PATCH V3] drm/drm_vblank: Change EINVAL by the correct errno

[Rodrigo Siqueira]

[-- 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 --]

✓ Fi.CI.BAT: success for drm/drm_vblank: Change EINVAL by the correct errno (rev2)

[Patchwork]

== 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

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

[Sam Ravnborg]

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

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

[Sam Ravnborg]

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

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

[Sam Ravnborg]

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

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

[Sam Ravnborg]

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

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

[Sam Ravnborg]

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

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

[Rodrigo Siqueira]

Hi Sam,

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

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

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

[Rodrigo Siqueira]

Hi Sam,

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

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

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

[Rodrigo Siqueira]

Hi Sam,

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

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

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

[Ville Syrjälä]

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

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

[Ville Syrjälä]

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

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

[Ville Syrjälä]

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

✗ Fi.CI.CHECKPATCH: warning for drm/drm_vblank: Change EINVAL by the correct errno (rev3)

[Patchwork]

== 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

✗ Fi.CI.BAT: failure for drm/drm_vblank: Change EINVAL by the correct errno (rev3)

[Patchwork]

== 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

Re: [RESEND PATCH V3] drm/drm_vblank: Change EINVAL by the correct errno

[Daniel Vetter]

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

Re: [RESEND PATCH V3] drm/drm_vblank: Change EINVAL by the correct errno

[Daniel Vetter]

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

Re: [RESEND PATCH V3] drm/drm_vblank: Change EINVAL by the correct errno

[Daniel Vetter]

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

✗ Fi.CI.IGT: failure for drm/drm_vblank: Change EINVAL by the correct errno (rev2)

[Patchwork]

== 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