From: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> To: Daniel Vetter <daniel@ffwll.ch> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <maxime.ripard@bootlin.com>, Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>, dri-devel <dri-devel@lists.freedesktop.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, kernel-janitors@vger.kernel.org, intel-gfx <intel-gfx@lists.freedesktop.org> Subject: Re: [Intel-gfx] [PATCH V4] drm/drm_vblank: Change EINVAL by the correct errno Date: Wed, 26 Jun 2019 10:37:11 -0300 [thread overview] Message-ID: <CADKXj+5+QHr1a0aiVZ1cSiPbtZhUAjmqiTmoQHGyEhodbcA2WQ@mail.gmail.com> (raw) In-Reply-To: <CAKMK7uEd71XTeuZeu1Km8Vq1K1VJJbgANyaZNWm4v18Qh-OqVw@mail.gmail.com> On Wed, Jun 26, 2019 at 4:53 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Wed, Jun 26, 2019 at 4:00 AM Rodrigo Siqueira > <rodrigosiqueiramelo@gmail.com> wrote: > > > > On 06/19, Daniel Vetter wrote: > > > On Wed, Jun 19, 2019 at 09:48:56AM +0200, Daniel Vetter wrote: > > > > On Tue, Jun 18, 2019 at 11:07:50PM -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 V3: > > > > > - Return EINVAL for _DRM_VBLANK_SIGNAL (Daniel) > > > > > > > > > > Change since V2: > > > > > Daniel Vetter and Chris Wilson > > > > > - Replace ENOTTY by EOPNOTSUPP > > > > > - Return EINVAL if the parameters are wrong > > > > > > > > > > > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > > > > > Apologies for the confusion on the last time around. btw if someone tells > > > > you "r-b (or a-b) with these changes", then just apply the r-b/a-b tag > > > > next time around. Otherwise people will re-review the same thing over and > > > > over again. > > > > > > btw when resending patches it's good practice to add anyone who commented > > > on it (or who commented on the igt test for the same patch and other way > > > round) onto the explicit Cc: list of the patch. That way it's easier for > > > them to follow the patch evolution and do a quick r-b once they're happy. > > > > Thanks for these valuable tips. > > Do you think that is a good idea to resend this patch CC's everybody? Or > > is it ok if I just apply it? > > Hm I thought I answered that on irc ... but just today I realized that > we missed 2 ioctls. There's also drm_crtc_get_sequence_ioctl and > drm_crtc_queue_sequence_ioctl which have the same dev->irq_enabled > check and I think should be treated the same. Hi, I reexamined all the composers described in the commit message (latest versions) to check if any project use and validate the return value from drm_crtc_get_sequence_ioctl and drm_crtc_queue_sequence_ioctl. I noticed that mesa and xserver use them. FWIU replace EINVAL by EOPNOTSUPP is harmless for mesa project, however it is not the same for xserver. Take a look at line 189 and 238 of hw/xfree86/drivers/modesetting/vblank.c * https://gitlab.freedesktop.org/xorg/xserver/blob/master/hw/xfree86/drivers/modesetting/vblank.c#L238 * https://gitlab.freedesktop.org/xorg/xserver/blob/master/hw/xfree86/drivers/modesetting/vblank.c#L189 A little bit below the above lines, you can see a validation like that: if (ret != -1 || (errno != ENOTTY && errno != EINVAL)) In other words, if we change the EINVAL by EOPNOTSUPP drm_crtc_[get|queue]_sequence_ioctl we could break xserver. I noticed that Keith Packard introduced these ioctls to the kernel and also to the xserver, I will prepare a new version and CC Keith. Should I do another thing to notify xserver developers? Thanks > Can you pls resend with those addressed too? Then you can also resend > with the cc's all added. > -Daniel > > > > > > If you don't do that then much bigger chances your patch gets ignored. > > > -Daniel > > > > > > > > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > > > > > --- > > > > > drivers/gpu/drm/drm_vblank.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > > > > > index 603ab105125d..bed233361614 100644 > > > > > --- a/drivers/gpu/drm/drm_vblank.c > > > > > +++ b/drivers/gpu/drm/drm_vblank.c > > > > > @@ -1582,7 +1582,7 @@ 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; > > > > > -- > > > > > 2.21.0 > > > > > > > > -- > > > > Daniel Vetter > > > > Software Engineer, Intel Corporation > > > > http://blog.ffwll.ch > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > > > -- > > Rodrigo Siqueira > > https://siqueira.tech > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Rodrigo Siqueira https://siqueira.tech
WARNING: multiple messages have this Message-ID (diff)
From: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> To: Daniel Vetter <daniel@ffwll.ch> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <maxime.ripard@bootlin.com>, Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>, dri-devel <dri-devel@lists.freedesktop.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, kernel-janitors@vger.kernel.org, intel-gfx <intel-gfx@lists.freedesktop.org> Subject: Re: [Intel-gfx] [PATCH V4] drm/drm_vblank: Change EINVAL by the correct errno Date: Wed, 26 Jun 2019 13:37:11 +0000 [thread overview] Message-ID: <CADKXj+5+QHr1a0aiVZ1cSiPbtZhUAjmqiTmoQHGyEhodbcA2WQ@mail.gmail.com> (raw) In-Reply-To: <CAKMK7uEd71XTeuZeu1Km8Vq1K1VJJbgANyaZNWm4v18Qh-OqVw@mail.gmail.com> On Wed, Jun 26, 2019 at 4:53 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Wed, Jun 26, 2019 at 4:00 AM Rodrigo Siqueira > <rodrigosiqueiramelo@gmail.com> wrote: > > > > On 06/19, Daniel Vetter wrote: > > > On Wed, Jun 19, 2019 at 09:48:56AM +0200, Daniel Vetter wrote: > > > > On Tue, Jun 18, 2019 at 11:07:50PM -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 V3: > > > > > - Return EINVAL for _DRM_VBLANK_SIGNAL (Daniel) > > > > > > > > > > Change since V2: > > > > > Daniel Vetter and Chris Wilson > > > > > - Replace ENOTTY by EOPNOTSUPP > > > > > - Return EINVAL if the parameters are wrong > > > > > > > > > > > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > > > > > Apologies for the confusion on the last time around. btw if someone tells > > > > you "r-b (or a-b) with these changes", then just apply the r-b/a-b tag > > > > next time around. Otherwise people will re-review the same thing over and > > > > over again. > > > > > > btw when resending patches it's good practice to add anyone who commented > > > on it (or who commented on the igt test for the same patch and other way > > > round) onto the explicit Cc: list of the patch. That way it's easier for > > > them to follow the patch evolution and do a quick r-b once they're happy. > > > > Thanks for these valuable tips. > > Do you think that is a good idea to resend this patch CC's everybody? Or > > is it ok if I just apply it? > > Hm I thought I answered that on irc ... but just today I realized that > we missed 2 ioctls. There's also drm_crtc_get_sequence_ioctl and > drm_crtc_queue_sequence_ioctl which have the same dev->irq_enabled > check and I think should be treated the same. Hi, I reexamined all the composers described in the commit message (latest versions) to check if any project use and validate the return value from drm_crtc_get_sequence_ioctl and drm_crtc_queue_sequence_ioctl. I noticed that mesa and xserver use them. FWIU replace EINVAL by EOPNOTSUPP is harmless for mesa project, however it is not the same for xserver. Take a look at line 189 and 238 of hw/xfree86/drivers/modesetting/vblank.c * https://gitlab.freedesktop.org/xorg/xserver/blob/master/hw/xfree86/drivers/modesetting/vblank.c#L238 * https://gitlab.freedesktop.org/xorg/xserver/blob/master/hw/xfree86/drivers/modesetting/vblank.c#L189 A little bit below the above lines, you can see a validation like that: if (ret != -1 || (errno != ENOTTY && errno != EINVAL)) In other words, if we change the EINVAL by EOPNOTSUPP drm_crtc_[get|queue]_sequence_ioctl we could break xserver. I noticed that Keith Packard introduced these ioctls to the kernel and also to the xserver, I will prepare a new version and CC Keith. Should I do another thing to notify xserver developers? Thanks > Can you pls resend with those addressed too? Then you can also resend > with the cc's all added. > -Daniel > > > > > > If you don't do that then much bigger chances your patch gets ignored. > > > -Daniel > > > > > > > > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > > > > > --- > > > > > drivers/gpu/drm/drm_vblank.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > > > > > index 603ab105125d..bed233361614 100644 > > > > > --- a/drivers/gpu/drm/drm_vblank.c > > > > > +++ b/drivers/gpu/drm/drm_vblank.c > > > > > @@ -1582,7 +1582,7 @@ 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; > > > > > -- > > > > > 2.21.0 > > > > > > > > -- > > > > Daniel Vetter > > > > Software Engineer, Intel Corporation > > > > http://blog.ffwll.ch > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > > > -- > > Rodrigo Siqueira > > https://siqueira.tech > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Rodrigo Siqueira https://siqueira.tech
next prev parent reply other threads:[~2019-06-26 13:37 UTC|newest] Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-06-19 2:07 [PATCH V4] drm/drm_vblank: Change EINVAL by the correct errno Rodrigo Siqueira 2019-06-19 2:07 ` Rodrigo Siqueira 2019-06-19 2:07 ` Rodrigo Siqueira 2019-06-19 2:45 ` ✓ Fi.CI.BAT: success for drm/drm_vblank: Change EINVAL by the correct errno (rev4) Patchwork 2019-06-19 7:48 ` [PATCH V4] drm/drm_vblank: Change EINVAL by the correct errno Daniel Vetter 2019-06-19 7:48 ` Daniel Vetter 2019-06-19 7:48 ` Daniel Vetter 2019-06-19 7:50 ` Daniel Vetter 2019-06-19 7:50 ` Daniel Vetter 2019-06-26 2:00 ` Rodrigo Siqueira 2019-06-26 2:00 ` Rodrigo Siqueira 2019-06-26 2:00 ` Rodrigo Siqueira 2019-06-26 7:52 ` [Intel-gfx] " Daniel Vetter 2019-06-26 7:52 ` Daniel Vetter 2019-06-26 7:52 ` [Intel-gfx] " Daniel Vetter 2019-06-26 13:37 ` Rodrigo Siqueira [this message] 2019-06-26 13:37 ` Rodrigo Siqueira 2019-06-26 14:26 ` Daniel Vetter 2019-06-26 14:26 ` Daniel Vetter 2019-06-19 18:43 ` ✓ Fi.CI.IGT: success for drm/drm_vblank: Change EINVAL by the correct errno (rev4) Patchwork
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CADKXj+5+QHr1a0aiVZ1cSiPbtZhUAjmqiTmoQHGyEhodbcA2WQ@mail.gmail.com \ --to=rodrigosiqueiramelo@gmail.com \ --cc=airlied@linux.ie \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=kernel-janitors@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=maarten.lankhorst@linux.intel.com \ --cc=maxime.ripard@bootlin.com \ --cc=sean@poorly.run \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.