| Message ID | 1371643245-8325-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 & > - 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. Thierry
Hi Thierry, 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 & - 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.
On Thu, Jun 20, 2013 at 12:17:25PM +0200, Laurent Pinchart wrote: > Hi Thierry, > > 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 & - 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. 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. I'll see if I can find the time to come up with a patch. Thierry
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 & - 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. > I'll see if I can find the time to come up with a patch.
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 & - 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; Thierry
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 & - 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. But till that happens I guess we could just roll forward with whatever hacks we currently have ... -Daniel
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@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
On Sat, Jun 22, 2013 at 12:56:46PM +0200, Thierry Reding wrote: > 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@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? Oh, not yet solid enough to be called plans, but just a few goals: - Shift from using int pipe to struct drm_crtc *crtc for modeset drivers. This will be a bit of fun since we have to keep the old ums vblank interrupt going. Probably ends up just duplicating almost all the vblank code in core drm for the modesetting case. - Once that's done we can add solid locking (i.e. grabbing crtc->mutex at relevant places). Atm vblank handling at least in i915 is simply racy. Not a that big issue as long as userspace doesn't grab a vblank ref while doing modesets, but still. - Rework the high-precision timestamping stuff into something more clearly resembling a helper layer. Modern intel hw has a set of nice vblank/pageflip timestamp registers which could replace the current code completely and in a race-free manner. This way we could drop the vblank irq enabling hystersis on those platforms completely. We probably also want to push down the vblank handling for pageflips into drivers (since on the same new platforms we don't need to enable interrupts at all for pageflip timestamps). - Icing on the cake (and not sure whether really worth it): vblank interrupt handler and work item support. At least in i915 we have a few use cases (and more are planned) that need generic support to do stuff at/after the next n vblanks. We need both support for tight timing constrains (so probably run from the interrupt handler directly) and process context (e.g. for unpinnning after a pageflip completes). > > 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? Yeah, forcing drivers to set dev->irq_enabled is fine with me as an interim solution. -Daniel
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. </para> <!--!Fdrivers/char/drm/drm_irq.c drm_irq_install--> <para>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- Documentation/DocBook/drm.tmpl | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)