| Message ID | 20181015170529.dgzpbm37hbuvqatc@smtp.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/drm_vblank: Change EINVAL by the correct errno | expand |
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
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
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 >
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.
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
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
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
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
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
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
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 >
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 |
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(-)