Re: [PATCH] drm: Improve manual IRQ installation documentation

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jun 20, 2013 at 01:00:36PM +0200, Thierry Reding wrote:
> On Thu, Jun 20, 2013 at 12:46:21PM +0200, Laurent Pinchart wrote:
> > Hi Thierry,
> > 
> > On Thursday 20 June 2013 12:40:26 Thierry Reding wrote:
> > > On Thu, Jun 20, 2013 at 12:17:25PM +0200, Laurent Pinchart wrote:
> > > > On Thursday 20 June 2013 12:10:47 Thierry Reding wrote:
> > > > > On Wed, Jun 19, 2013 at 02:00:45PM +0200, Laurent Pinchart wrote:
> > > > > > Signed-off-by: Laurent Pinchart
> > > > > > <laurent.pinchart+renesas@ideasonboard.com>
> > > > > > ---
> > > > > > 
> > > > > >  Documentation/DocBook/drm.tmpl | 14 ++++++++------
> > > > > >  1 file changed, 8 insertions(+), 6 deletions(-)
> > > > > > 
> > > > > > diff --git a/Documentation/DocBook/drm.tmpl
> > > > > > b/Documentation/DocBook/drm.tmpl index f9df3b8..738b727 100644
> > > > > > --- a/Documentation/DocBook/drm.tmpl
> > > > > > +++ b/Documentation/DocBook/drm.tmpl
> > > > > > @@ -186,11 +186,12 @@
> > > > > > 
> > > > > >            <varlistentry>
> > > > > >            
> > > > > >              <term>DRIVER_HAVE_IRQ</term><term>DRIVER_IRQ_SHARED</term
> > > > > >              >
> > > > > >              <listitem><para>
> > > > > > 
> > > > > > -              DRIVER_HAVE_IRQ indicates whether the driver has an IRQ
> > > > > > handler. The -              DRM core will automatically register an
> > > > > > interrupt handler when the -              flag is set.
> > > > > > DRIVER_IRQ_SHARED
> > > > > > indicates whether the device &amp; -              handler support
> > > > > > shared
> > > > > > IRQs (note that this is required of PCI -              drivers).
> > > > > > +              DRIVER_HAVE_IRQ indicates whether the driver has an IRQ
> > > > > > handler +              managed by the DRM Core. The core will support
> > > > > > simple IRQ handler +              installation when the flag is set.
> > > > > > The
> > > > > > installation process is +              described in <xref
> > > > > > linkend="drm-irq-registration"/>.</para> +
> > > > > > <para>DRIVER_IRQ_SHARED indicates whether the device &amp; handler +
> > > > > > 
> > > > > >         support shared IRQs (note that this is required of PCI 
> > > > > >         drivers).>
> > > > > >         
> > > > > >              </para></listitem>
> > > > > >            
> > > > > >            </varlistentry>
> > > > > >            <varlistentry>
> > > > > > 
> > > > > > @@ -344,7 +345,8 @@ char *date;</synopsis>
> > > > > > 
> > > > > >            The DRM core tries to facilitate IRQ handler registration
> > > > > >            and
> > > > > >            unregistration by providing
> > > > > >            <function>drm_irq_install</function> and
> > > > > >            <function>drm_irq_uninstall</function> functions. Those
> > > > > >            functions only
> > > > > > 
> > > > > > -          support a single interrupt per device.
> > > > > > +          support a single interrupt per device, devices that use
> > > > > > more
> > > > > > than one +          IRQs need to be handled manually.
> > > > > 
> > > > > Perhaps this should mention that if you handle IRQ installation manually
> > > > > you also need to manually set drm->irq_enabled = 1, as otherwise things
> > > > > like DRM_IOCTL_WAIT_VBLANK won't work properly.
> > > > 
> > > > That's only needed if DRIVER_HAVE_IRQ is set, otherwise the
> > > > drm_wait_vblank() function skips the irq_enabled check.
> > > 
> > > Not quite. There's another check for dev->irq_enabled in the
> > > DRM_WAIT_ON() so it'll never actually block.
> > 
> > Indeed.
> > 
> > > Arguably this could be solved by making the DRM_WAIT_ON() condition drop the
> > > !dev->irq_enabled in case DRIVER_HAVE_IRQ isn't set.
> > 
> > Or we could force drivers to set drm->irq_enabled, I'm fine with that as well. 
> > I can fix the documentation if that would be the preferred solution.
> 
> Thinking about it some more, the latter seems more appropriate. Consider
> some driver that doesn't set DRIVER_HAVE_IRQ but also doesn't initialize
> interrupts manually. If so it'll cause DRM_IOCTL_WAIT_VBLANK to block
> indefinitely.
> 
> On the other hand perhaps the check at the beginning of drm_wait_vblank
> should be improved. Maybe make it something like this:
> 
> 	if (drm_core_check_feature(dev, DRIVER_HAVE_IRQ))
> 		if (!drm_dev_to_irq(dev))
> 			return -EINVAL;
> 
> 	if (!dev->irq_enabled)
> 		return -EINVAL;

I think the vblank subsystem is ripe for rework, and I have a few plans
for that. But till that happens I guess we could just roll forward with
whatever hacks we currently have ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch