On Thu, Apr 17, 2014 at 12:02:07AM +0200, Laurent Pinchart wrote:
> And another comment...
>
> On Friday 11 April 2014 23:36:07 Daniel Vetter wrote:
> > To get rid of the dev->bus->get_irq callback we need to pass in the
> > desired irq explicitly into drm_irq_install. To avoid having to do the
> > same for drm_irq_unistall just track it internally. That leaves
> > drivers with less room to botch things up.
> >
> > v2: Add the hunk lost in an earlier patch to this one (Thierry).
> >
> > Cc: Thierry Reding <thierry.reding at gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > ---
> > drivers/gpu/drm/drm_irq.c | 18 +++++++++++-------
> > include/drm/drmP.h | 2 ++
> > 2 files changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 330e85b19115..1c3b6229363d 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -249,14 +249,16 @@ static inline int drm_dev_to_irq(struct drm_device
> > *dev) */
> > int drm_irq_install(struct drm_device *dev)
> > {
> > - int ret;
> > + int ret, irq;
> > unsigned long sh_flags = 0;
> > char *irqname;
> >
> > + irq = drm_dev_to_irq(dev);
> > +
> > if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
> > return -EINVAL;
> >
> > - if (drm_dev_to_irq(dev) == 0)
> > + if (irq == 0)
>
> Isn't 0 a valid IRQ number ? Shouldn't you check for irq < 0 instead ? At
> least platform_get_irq() returns a negative error value on failure.
tbh I'm not really clear on how this works. If we want to change this then
I think that should be a separate patch. Also it might be better to
extract this check into drm_control, which is the only function that
really needs it since it is called by userspace.
For all other places it is a simple driver bug and I'm not sure how much
we should care - request_irq should catch any abuse already.
In any case this should be a separate patch.
-Daniel
>
> > return -EINVAL;
> >
> > /* Driver must have been initialized */
> > @@ -267,7 +269,7 @@ int drm_irq_install(struct drm_device *dev)
> > return -EBUSY;
> > dev->irq_enabled = true;
> >
> > - DRM_DEBUG("irq=%d\n", drm_dev_to_irq(dev));
> > + DRM_DEBUG("irq=%d\n", dev->irq);
> >
> > /* Before installing handler */
> > if (dev->driver->irq_preinstall)
> > @@ -282,7 +284,7 @@ int drm_irq_install(struct drm_device *dev)
> > else
> > irqname = dev->driver->name;
> >
> > - ret = request_irq(drm_dev_to_irq(dev), dev->driver->irq_handler,
> > + ret = request_irq(dev->irq, dev->driver->irq_handler,
> > sh_flags, irqname, dev);
> >
> > if (ret < 0) {
> > @@ -301,7 +303,9 @@ int drm_irq_install(struct drm_device *dev)
> > dev->irq_enabled = false;
> > if (!drm_core_check_feature(dev, DRIVER_MODESET))
> > vga_client_register(dev->pdev, NULL, NULL, NULL);
> > - free_irq(drm_dev_to_irq(dev), dev);
> > + free_irq(dev->irq, dev);
> > + } else {
> > + dev->irq = irq;
> > }
> >
> > return ret;
> > @@ -344,7 +348,7 @@ int drm_irq_uninstall(struct drm_device *dev)
> > if (!irq_enabled)
> > return -EINVAL;
> >
> > - DRM_DEBUG("irq=%d\n", drm_dev_to_irq(dev));
> > + DRM_DEBUG("irq=%d\n", dev->irq);
> >
> > if (!drm_core_check_feature(dev, DRIVER_MODESET))
> > vga_client_register(dev->pdev, NULL, NULL, NULL);
> > @@ -352,7 +356,7 @@ int drm_irq_uninstall(struct drm_device *dev)
> > if (dev->driver->irq_uninstall)
> > dev->driver->irq_uninstall(dev);
> >
> > - free_irq(drm_dev_to_irq(dev), dev);
> > + free_irq(dev->irq, dev);
> >
> > return 0;
> > }
> > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > index 8b23a34a103e..6f512cd97cd5 100644
> > --- a/include/drm/drmP.h
> > +++ b/include/drm/drmP.h
> > @@ -1107,6 +1107,8 @@ struct drm_device {
> > /** \name Context support */
> > /*@{ */
> > bool irq_enabled; /**< True if irq handler is enabled */
> > + int irq;
> > +
> > __volatile__ long context_flag; /**< Context swapping flag */
> > int last_context; /**< Last current context */
> > /*@} */
>
> --
> Regards,
>
> Laurent Pinchart
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
