[PATCH] drm: Don't test for IRQ support in VBLANK ioctls

[Daniel Vetter]

On Tue, Jun 8, 2021 at 11:18 AM Daniel Vetter <daniel at ffwll.ch> wrote:
>
> On Tue, Jun 08, 2021 at 11:03:01AM +0200, Thomas Zimmermann wrote:
> > Replace the IRQ check in VBLANK ioctls with a check for vblank
> > support. IRQs might be enabled wthout vblanking being supported.
>
> Nah, or if they are, that's a broken driver. irq_enabled here really only
> means vblank_irq_enabled (maybe we should rename it). I'd like to
> understand the motivation here a bit better to make sure we'r not just
> papering over a driver bug.
>
> Also as-is this breaks legacy drivers, which do enable/disable irqs at
> runtime with the legacy IRQ_CONTROL ioctl, so we can't just throw this
> out. Hence this cleanup here is only ok for non-legacy drivers.
>
> Finally if you do this cleanup I think we should go through drivers and
> drop the irq_enabled = true settings that are littered around. For that
> cleanup I think this patch makes sense.

I forgot to add: We already do this check that you're adding here
because later on we validate the provided crtc index against
dev->num_crtcs. vblank support is confusing because it lives both in a
kms and legacy driver world.
-Daniel

> > This change also removes the DRM framework's only dependency on
> > IRQ state for non-legacy drivers.
> >
> > Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> > ---
> >  drivers/gpu/drm/drm_irq.c    | 10 +++-------
> >  drivers/gpu/drm/drm_vblank.c |  6 +++---
> >  2 files changed, 6 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index c3bd664ea733..1d7785721323 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -74,10 +74,8 @@
> >   * only supports devices with a single interrupt on the main device stored in
> >   * &drm_device.dev and set as the device paramter in drm_dev_alloc().
> >   *
> > - * These IRQ helpers are strictly optional. Drivers which roll their own only
> > - * need to set &drm_device.irq_enabled to signal the DRM core that vblank
> > - * interrupts are working. Since these helpers don't automatically clean up the
> > - * requested interrupt like e.g. devm_request_irq() they're not really
> > + * These IRQ helpers are strictly optional. Since these helpers don't automatically
> > + * clean up the requested interrupt like e.g. devm_request_irq() they're not really
> >   * recommended.
> >   */
> >
> > @@ -91,9 +89,7 @@
> >   * and after the installation.
> >   *
> >   * This is the simplified helper interface provided for drivers with no special
> > - * needs. Drivers which need to install interrupt handlers for multiple
> > - * interrupts must instead set &drm_device.irq_enabled to signal the DRM core
> > - * that vblank interrupts are available.
> > + * needs.
> >   *
> >   * @irq must match the interrupt number that would be passed to request_irq(),
> >   * if called directly instead of using this helper function.
> > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > index 3417e1ac7918..165286fef478 100644
> > --- a/drivers/gpu/drm/drm_vblank.c
> > +++ b/drivers/gpu/drm/drm_vblank.c
> > @@ -1748,7 +1748,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
> >       unsigned int pipe_index;
> >       unsigned int flags, pipe, high_pipe;
> >
> > -     if (!dev->irq_enabled)
> > +     if (!drm_dev_has_vblank(dev))
> >               return -EOPNOTSUPP;
> >
> >       if (vblwait->request.type & _DRM_VBLANK_SIGNAL)
> > @@ -2023,7 +2023,7 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
> >       if (!drm_core_check_feature(dev, DRIVER_MODESET))
> >               return -EOPNOTSUPP;
> >
> > -     if (!dev->irq_enabled)
> > +     if (!drm_dev_has_vblank(dev))
> >               return -EOPNOTSUPP;
> >
> >       crtc = drm_crtc_find(dev, file_priv, get_seq->crtc_id);
> > @@ -2082,7 +2082,7 @@ int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data,
> >       if (!drm_core_check_feature(dev, DRIVER_MODESET))
> >               return -EOPNOTSUPP;
> >
> > -     if (!dev->irq_enabled)
> > +     if (!drm_dev_has_vblank(dev))
> >               return -EOPNOTSUPP;
> >
> >       crtc = drm_crtc_find(dev, file_priv, queue_seq->crtc_id);
> > --
> > 2.31.1
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



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