On Thu, Jun 20, 2013 at 04:55:03PM +0200, Daniel Vetter wrote: > 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 at 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 & - 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 & > > > > > > > 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.
Would you mind sharing those plans so that maybe others can help out? > But till that happens I guess we could just roll forward with > whatever hacks we currently have ... So that means the above sounds like a reasonable interim solution? Thierry -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 836 bytes Desc: not available URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130622/0a0dc7ba/attachment.pgp>
