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

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

[Rodrigo Siqueira]

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 V1:
 Daniel Vetter and Chris Wilson
 - Replace ENOTTY by EOPNOTSUPP
 - Return EINVAL if the parameters are wrong

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 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 98e091175921..80f5a3bb427e 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -1533,10 +1533,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.19.1

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

[Daniel Vetter]

On Mon, Oct 15, 2018 at 02:05:29PM -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 V1:
>  Daniel Vetter and Chris Wilson
>  - Replace ENOTTY by EOPNOTSUPP
>  - Return EINVAL if the parameters are wrong
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Can you pls also let intel-gfx-ci test this patch? You just need to
include intel-gfx@lists.freedesktop.org in your recipient list.

For merging, since you plan to stick around doing kms stuff a bit: Want
commit rights for drm-misc?

https://drm.pages.freedesktop.org/maintainer-tools/getting-started.html

Cheers, Daniel

> ---
>  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 98e091175921..80f5a3bb427e 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -1533,10 +1533,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.19.1

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Rodrigo Siqueira]

Hi,

First of all, thanks to all for the reviewers and feedbacks.

On 10/16, Daniel Vetter wrote:
> On Mon, Oct 15, 2018 at 02:05:29PM -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 V1:
> >  Daniel Vetter and Chris Wilson
> >  - Replace ENOTTY by EOPNOTSUPP
> >  - Return EINVAL if the parameters are wrong
> > 
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Can you pls also let intel-gfx-ci test this patch? You just need to
> include intel-gfx@lists.freedesktop.org in your recipient list.

I did know about intel-gfx-ci, really nice:)

Should I CC this mailing list if I send patches to the DRM core, amdgpu,
i915, vc4, vgem, and virtio-gpu? I suppose that IGT is running on the
CI, right?

Another question, do I need to send a V3 with intel-gfx-ci?
 
> For merging, since you plan to stick around doing kms stuff a bit: Want
> commit rights for drm-misc?
> 
> https://drm.pages.freedesktop.org/maintainer-tools/getting-started.html

Yes, I want :)
I will need some guidance, in the beginning, to get confident about the
processes. If you can help me with this, I will be glad to play around
with 'dim' and the merging tasks.

Best Regards

> Cheers, Daniel
> 
> > ---
> >  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 98e091175921..80f5a3bb427e 100644
> > --- a/drivers/gpu/drm/drm_vblank.c
> > +++ b/drivers/gpu/drm/drm_vblank.c
> > @@ -1533,10 +1533,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.19.1
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Rodrigo Siqueira
https://siqueira.tech
https://twitter.com/siqueirajordao
Graduate Student
Department of Computer Science
University of São Paulo

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

[Daniel Vetter]

On Wed, Oct 17, 2018 at 2:43 PM Rodrigo Siqueira
<rodrigosiqueiramelo@gmail.com> wrote:
>
> Hi,
>
> First of all, thanks to all for the reviewers and feedbacks.
>
> On 10/16, Daniel Vetter wrote:
> > On Mon, Oct 15, 2018 at 02:05:29PM -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 V1:
> > >  Daniel Vetter and Chris Wilson
> > >  - Replace ENOTTY by EOPNOTSUPP
> > >  - Return EINVAL if the parameters are wrong
> > >
> > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > Can you pls also let intel-gfx-ci test this patch? You just need to
> > include intel-gfx@lists.freedesktop.org in your recipient list.
>
> I did know about intel-gfx-ci, really nice:)
>
> Should I CC this mailing list if I send patches to the DRM core, amdgpu,
> i915, vc4, vgem, and virtio-gpu? I suppose that IGT is running on the
> CI, right?

It's only intel gpus (well and one special amd one) running igt on a
_lot_ of different machines.

> Another question, do I need to send a V3 with intel-gfx-ci?

If the patch-set is unchanged people use the "FOR CI" subject prefix
when resending, instead of incrementing the version number.

> > For merging, since you plan to stick around doing kms stuff a bit: Want
> > commit rights for drm-misc?
> >
> > https://drm.pages.freedesktop.org/maintainer-tools/getting-started.html
>
> Yes, I want :)
> I will need some guidance, in the beginning, to get confident about the
> processes. If you can help me with this, I will be glad to play around
> with 'dim' and the merging tasks.

Sure, just ping me on irc. First you need a freedesktop.org ssh account:

https://www.freedesktop.org/wiki/AccountRequests/

You need to request access to the drm-misc group. Once you have the
bugzilla, pls ping me so I can ack it.

Thanks, Daniel

>
> Best Regards
>
> > Cheers, Daniel
> >
> > > ---
> > >  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 98e091175921..80f5a3bb427e 100644
> > > --- a/drivers/gpu/drm/drm_vblank.c
> > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > @@ -1533,10 +1533,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.19.1
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
> --
> Rodrigo Siqueira
> https://siqueira.tech
> https://twitter.com/siqueirajordao
> Graduate Student
> Department of Computer Science
> University of São Paulo
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Rodrigo Siqueira]

On 10/17, Daniel Vetter wrote:
> On Wed, Oct 17, 2018 at 2:43 PM Rodrigo Siqueira
> <rodrigosiqueiramelo@gmail.com> wrote:
> >
> > Hi,
> >
> > First of all, thanks to all for the reviewers and feedbacks.
> >
> > On 10/16, Daniel Vetter wrote:
> > > On Mon, Oct 15, 2018 at 02:05:29PM -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 V1:
> > > >  Daniel Vetter and Chris Wilson
> > > >  - Replace ENOTTY by EOPNOTSUPP
> > > >  - Return EINVAL if the parameters are wrong
> > > >
> > > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > >
> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > >
> > > Can you pls also let intel-gfx-ci test this patch? You just need to
> > > include intel-gfx@lists.freedesktop.org in your recipient list.
> >
> > I did know about intel-gfx-ci, really nice:)
> >
> > Should I CC this mailing list if I send patches to the DRM core, amdgpu,
> > i915, vc4, vgem, and virtio-gpu? I suppose that IGT is running on the
> > CI, right?
> 
> It's only intel gpus (well and one special amd one) running igt on a
> _lot_ of different machines.
> 
> > Another question, do I need to send a V3 with intel-gfx-ci?
> 
> If the patch-set is unchanged people use the "FOR CI" subject prefix
> when resending, instead of incrementing the version number.

Ok, I will do it.
 
> > > For merging, since you plan to stick around doing kms stuff a bit: Want
> > > commit rights for drm-misc?
> > >
> > > https://drm.pages.freedesktop.org/maintainer-tools/getting-started.html
> >
> > Yes, I want :)
> > I will need some guidance, in the beginning, to get confident about the
> > processes. If you can help me with this, I will be glad to play around
> > with 'dim' and the merging tasks.
> 
> Sure, just ping me on irc. First you need a freedesktop.org ssh account:
> 
> https://www.freedesktop.org/wiki/AccountRequests/
> 
> You need to request access to the drm-misc group. Once you have the
> bugzilla, pls ping me so I can ack it.

Ok, first I will carefully read the documentation. Next, I will
request the access. I will ping you on IRC after I get the account.

Thanks

> Thanks, Daniel
> 
> >
> > Best Regards
> >
> > > Cheers, Daniel
> > >
> > > > ---
> > > >  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 98e091175921..80f5a3bb427e 100644
> > > > --- a/drivers/gpu/drm/drm_vblank.c
> > > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > > @@ -1533,10 +1533,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.19.1
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> >
> > --
> > Rodrigo Siqueira
> > https://siqueira.tech
> > https://twitter.com/siqueirajordao
> > Graduate Student
> > Department of Computer Science
> > University of São Paulo
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Rodrigo Siqueira
https://siqueira.tech
https://twitter.com/siqueirajordao
Graduate Student
Department of Computer Science
University of São Paulo

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

[Rodrigo Siqueira]

Hi,

I resend this patch for CI via “intel-gfx@lists.freedesktop.org” as
Daniel suggested, and I got a feedback that reported an issue as can be
seen here:

   https://patchwork.freedesktop.org/series/51147/

After a careful analysis of what happened, I concluded that the problem
is related to the function “igt_wait_for_vblank_count()” in “igt_kms.c”.
This function has the following assert:

   igt_assert(drmWaitVBlank(drm_fd, &wait_vbl) == 0)

This function only checks if everything went well with the
drmWaitVBlank() operation and does not make any other validation. IMHO
the patch is correct, and the problem pointed out by CI is not related
to this change.

Make sense?

Best Regards

On 10/17, Daniel Vetter wrote:
> On Wed, Oct 17, 2018 at 2:43 PM Rodrigo Siqueira
> <rodrigosiqueiramelo@gmail.com> wrote:
> >
> > Hi,
> >
> > First of all, thanks to all for the reviewers and feedbacks.
> >
> > On 10/16, Daniel Vetter wrote:
> > > On Mon, Oct 15, 2018 at 02:05:29PM -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 V1:
> > > >  Daniel Vetter and Chris Wilson
> > > >  - Replace ENOTTY by EOPNOTSUPP
> > > >  - Return EINVAL if the parameters are wrong
> > > >
> > > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > >
> > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > >
> > > Can you pls also let intel-gfx-ci test this patch? You just need to
> > > include intel-gfx@lists.freedesktop.org in your recipient list.
> >
> > I did know about intel-gfx-ci, really nice:)
> >
> > Should I CC this mailing list if I send patches to the DRM core, amdgpu,
> > i915, vc4, vgem, and virtio-gpu? I suppose that IGT is running on the
> > CI, right?
> 
> It's only intel gpus (well and one special amd one) running igt on a
> _lot_ of different machines.
> 
> > Another question, do I need to send a V3 with intel-gfx-ci?
> 
> If the patch-set is unchanged people use the "FOR CI" subject prefix
> when resending, instead of incrementing the version number.
> 
> > > For merging, since you plan to stick around doing kms stuff a bit: Want
> > > commit rights for drm-misc?
> > >
> > > https://drm.pages.freedesktop.org/maintainer-tools/getting-started.html
> >
> > Yes, I want :)
> > I will need some guidance, in the beginning, to get confident about the
> > processes. If you can help me with this, I will be glad to play around
> > with 'dim' and the merging tasks.
> 
> Sure, just ping me on irc. First you need a freedesktop.org ssh account:
> 
> https://www.freedesktop.org/wiki/AccountRequests/
> 
> You need to request access to the drm-misc group. Once you have the
> bugzilla, pls ping me so I can ack it.
> 
> Thanks, Daniel
> 
> >
> > Best Regards
> >
> > > Cheers, Daniel
> > >
> > > > ---
> > > >  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 98e091175921..80f5a3bb427e 100644
> > > > --- a/drivers/gpu/drm/drm_vblank.c
> > > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > > @@ -1533,10 +1533,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.19.1
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> >
> > --
> > Rodrigo Siqueira
> > https://siqueira.tech
> > https://twitter.com/siqueirajordao
> > Graduate Student
> > Department of Computer Science
> > University of São Paulo
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Maarten Lankhorst]

Op 13-01-2019 om 21:23 schreef Rodrigo Siqueira:
> Hi,
>
> I resend this patch for CI via “intel-gfx@lists.freedesktop.org” as
> Daniel suggested, and I got a feedback that reported an issue as can be
> seen here:
>
>    https://patchwork.freedesktop.org/series/51147/
>
> After a careful analysis of what happened, I concluded that the problem
> is related to the function “igt_wait_for_vblank_count()” in “igt_kms.c”.
> This function has the following assert:
>
>    igt_assert(drmWaitVBlank(drm_fd, &wait_vbl) == 0)
>
> This function only checks if everything went well with the
> drmWaitVBlank() operation and does not make any other validation. IMHO
> the patch is correct, and the problem pointed out by CI is not related
> to this change.

Hey,

Thanks for finding the root cause. Before upstreaming can you send a fix for i-g-t so we don't lose CI coverage after changing the behavior?

~Maarten

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

[Rodrigo Siqueira]

On 01/14, Maarten Lankhorst wrote:
> Op 13-01-2019 om 21:23 schreef Rodrigo Siqueira:
> > Hi,
> >
> > I resend this patch for CI via “intel-gfx@lists.freedesktop.org” as
> > Daniel suggested, and I got a feedback that reported an issue as can be
> > seen here:
> >
> >    https://patchwork.freedesktop.org/series/51147/
> >
> > After a careful analysis of what happened, I concluded that the problem
> > is related to the function “igt_wait_for_vblank_count()” in “igt_kms.c”.
> > This function has the following assert:
> >
> >    igt_assert(drmWaitVBlank(drm_fd, &wait_vbl) == 0)
> >
> > This function only checks if everything went well with the
> > drmWaitVBlank() operation and does not make any other validation. IMHO
> > the patch is correct, and the problem pointed out by CI is not related
> > to this change.
> 
> Hey,

Hi,

Thanks for the feedback :)

> Thanks for finding the root cause. Before upstreaming can you send a fix for i-g-t so we don't lose CI coverage after changing the behavior?

I'm just confused on my next step, should I fix the IGT test and then
resend the patch? Additionally, I noticed that tests related to vblank
wait have others issues as I pointed out here (see my last message):

	https://patchwork.freedesktop.org/patch/245784/

Is it enough if I handling EINVAL and EOPNOTSUPP in the tests?  I'm
afraid, that the tests will still fail if I consider these two case;
however, I suppose that handling only EOPNOTSUPP can fix the problem,
but I'm not sure if it is the best solution.

Best Regards
 
> ~Maarten
> 

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

[Maarten Lankhorst]

Op 15-10-18 om 19:05 schreef Rodrigo Siqueira:
> 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 V1:
>  Daniel Vetter and Chris Wilson
>  - Replace ENOTTY by EOPNOTSUPP
>  - Return EINVAL if the parameters are wrong
>
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> ---
>  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 98e091175921..80f5a3bb427e 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -1533,10 +1533,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;
Change to -EIO?

If userspace would ever print this out, it would print the following
confusing message to userspace:
"Operation not supported on transport endpoint"
>  
>  	if (vblwait->request.type & _DRM_VBLANK_SIGNAL)
> -		return -EINVAL;
> +		return -EOPNOTSUPP;
I would keep this -EINVAL, tbh and making it part of the below if statement..
>  	if (vblwait->request.type &
>  	    ~(_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK |

Cheers,

Maarten

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

[Daniel Vetter]

On Tue, Oct 16, 2018 at 03:36:20PM +0200, Maarten Lankhorst wrote:
> Op 15-10-18 om 19:05 schreef Rodrigo Siqueira:
> > 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 V1:
> >  Daniel Vetter and Chris Wilson
> >  - Replace ENOTTY by EOPNOTSUPP
> >  - Return EINVAL if the parameters are wrong
> >
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > ---
> >  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 98e091175921..80f5a3bb427e 100644
> > --- a/drivers/gpu/drm/drm_vblank.c
> > +++ b/drivers/gpu/drm/drm_vblank.c
> > @@ -1533,10 +1533,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;
> Change to -EIO?
> 
> If userspace would ever print this out, it would print the following
> confusing message to userspace:
> "Operation not supported on transport endpoint"

You're a bit late, EOPNOTSUPP is not established already in upstream for
this. And -EIO is taken already for "the gpu is dead".

> >  
> >  	if (vblwait->request.type & _DRM_VBLANK_SIGNAL)
> > -		return -EINVAL;
> > +		return -EOPNOTSUPP;
> I would keep this -EINVAL, tbh and making it part of the below if statement..

We discussed this, it's different: This here is an ioctl flag that's no
longer supported, the below is just an invalid request. Hence different
errno.

I think you missed a bit with your bikeshed :-)
-Daniel

> >  	if (vblwait->request.type &
> >  	    ~(_DRM_VBLANK_TYPES_MASK | _DRM_VBLANK_FLAGS_MASK |
> 
> Cheers,
> 
> Maarten
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Ville Syrjälä]

On Tue, Oct 16, 2018 at 06:38:31PM +0200, Daniel Vetter wrote:
> On Tue, Oct 16, 2018 at 03:36:20PM +0200, Maarten Lankhorst wrote:
> > Op 15-10-18 om 19:05 schreef Rodrigo Siqueira:
> > > 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 V1:
> > >  Daniel Vetter and Chris Wilson
> > >  - Replace ENOTTY by EOPNOTSUPP
> > >  - Return EINVAL if the parameters are wrong
> > >
> > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > ---
> > >  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 98e091175921..80f5a3bb427e 100644
> > > --- a/drivers/gpu/drm/drm_vblank.c
> > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > @@ -1533,10 +1533,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;
> > Change to -EIO?
> > 
> > If userspace would ever print this out, it would print the following
> > confusing message to userspace:
> > "Operation not supported on transport endpoint"
> 
> You're a bit late, EOPNOTSUPP is not established already in upstream for
> this. And -EIO is taken already for "the gpu is dead".
> 
> > >  
> > >  	if (vblwait->request.type & _DRM_VBLANK_SIGNAL)
> > > -		return -EINVAL;
> > > +		return -EOPNOTSUPP;
> > I would keep this -EINVAL, tbh and making it part of the below if statement..
> 
> We discussed this, it's different: This here is an ioctl flag that's no
> longer supported, the below is just an invalid request. Hence different
> errno.
> 
> I think you missed a bit with your bikeshed :-)

I think I too agree with the -EINVAL here as this flag is never
supported, whereas -EOPNOTSUPP seems to mean "this flag is still
valid, but not supported by your current hardware/driver
configuration".

Also drm_invalid_op() uses -EINVAL for deprecated features as well.

-- 
Ville Syrjälä
Intel

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

[Daniel Vetter]

On Tue, Oct 16, 2018 at 7:28 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Tue, Oct 16, 2018 at 06:38:31PM +0200, Daniel Vetter wrote:
> > On Tue, Oct 16, 2018 at 03:36:20PM +0200, Maarten Lankhorst wrote:
> > > Op 15-10-18 om 19:05 schreef Rodrigo Siqueira:
> > > > 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 V1:
> > > >  Daniel Vetter and Chris Wilson
> > > >  - Replace ENOTTY by EOPNOTSUPP
> > > >  - Return EINVAL if the parameters are wrong
> > > >
> > > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > > > ---
> > > >  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 98e091175921..80f5a3bb427e 100644
> > > > --- a/drivers/gpu/drm/drm_vblank.c
> > > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > > @@ -1533,10 +1533,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;
> > > Change to -EIO?
> > >
> > > If userspace would ever print this out, it would print the following
> > > confusing message to userspace:
> > > "Operation not supported on transport endpoint"
> >
> > You're a bit late, EOPNOTSUPP is not established already in upstream for
> > this. And -EIO is taken already for "the gpu is dead".
> >
> > > >
> > > >   if (vblwait->request.type & _DRM_VBLANK_SIGNAL)
> > > > -         return -EINVAL;
> > > > +         return -EOPNOTSUPP;
> > > I would keep this -EINVAL, tbh and making it part of the below if statement..
> >
> > We discussed this, it's different: This here is an ioctl flag that's no
> > longer supported, the below is just an invalid request. Hence different
> > errno.
> >
> > I think you missed a bit with your bikeshed :-)
>
> I think I too agree with the -EINVAL here as this flag is never
> supported, whereas -EOPNOTSUPP seems to mean "this flag is still
> valid, but not supported by your current hardware/driver
> configuration".

Michel Dänzer claims this was support way back in 2.6.29 or so :-)

> Also drm_invalid_op() uses -EINVAL for deprecated features as well.

Should probably adjust that too. The entire EOPNOTSUPP color choice is
very brand new ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch