* [PATCH] drm: Improve manual IRQ installation documentation @ 2013-06-19 12:00 Laurent Pinchart 2013-06-20 10:10 ` Thierry Reding 0 siblings, 1 reply; 9+ messages in thread From: Laurent Pinchart @ 2013-06-19 12:00 UTC (permalink / raw) To: dri-devel 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. </para> <!--!Fdrivers/char/drm/drm_irq.c drm_irq_install--> <para> -- Regards, Laurent Pinchart ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: Improve manual IRQ installation documentation 2013-06-19 12:00 [PATCH] drm: Improve manual IRQ installation documentation Laurent Pinchart @ 2013-06-20 10:10 ` Thierry Reding 2013-06-20 10:17 ` Laurent Pinchart 0 siblings, 1 reply; 9+ messages in thread From: Thierry Reding @ 2013-06-20 10:10 UTC (permalink / raw) To: Laurent Pinchart; +Cc: dri-devel [-- Attachment #1.1: Type: text/plain, Size: 2256 bytes --] 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 [-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: Improve manual IRQ installation documentation 2013-06-20 10:10 ` Thierry Reding @ 2013-06-20 10:17 ` Laurent Pinchart 2013-06-20 10:40 ` Thierry Reding 0 siblings, 1 reply; 9+ messages in thread From: Laurent Pinchart @ 2013-06-20 10:17 UTC (permalink / raw) To: Thierry Reding; +Cc: Laurent Pinchart, dri-devel [-- Attachment #1.1: Type: text/plain, Size: 2605 bytes --] 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. -- Regards, Laurent Pinchart [-- Attachment #1.2: This is a digitally signed message part. --] [-- Type: application/pgp-signature, Size: 490 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: Improve manual IRQ installation documentation 2013-06-20 10:17 ` Laurent Pinchart @ 2013-06-20 10:40 ` Thierry Reding 2013-06-20 10:46 ` Laurent Pinchart 0 siblings, 1 reply; 9+ messages in thread From: Thierry Reding @ 2013-06-20 10:40 UTC (permalink / raw) To: Laurent Pinchart; +Cc: Laurent Pinchart, dri-devel [-- Attachment #1.1: Type: text/plain, Size: 3124 bytes --] 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 [-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: Improve manual IRQ installation documentation 2013-06-20 10:40 ` Thierry Reding @ 2013-06-20 10:46 ` Laurent Pinchart 2013-06-20 11:00 ` Thierry Reding 0 siblings, 1 reply; 9+ messages in thread From: Laurent Pinchart @ 2013-06-20 10:46 UTC (permalink / raw) To: Thierry Reding; +Cc: Laurent Pinchart, dri-devel [-- Attachment #1.1: Type: text/plain, Size: 3511 bytes --] 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. -- Regards, Laurent Pinchart [-- Attachment #1.2: This is a digitally signed message part. --] [-- Type: application/pgp-signature, Size: 490 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: Improve manual IRQ installation documentation 2013-06-20 10:46 ` Laurent Pinchart @ 2013-06-20 11:00 ` Thierry Reding 2013-06-20 14:55 ` Daniel Vetter 0 siblings, 1 reply; 9+ messages in thread From: Thierry Reding @ 2013-06-20 11:00 UTC (permalink / raw) To: Laurent Pinchart; +Cc: Laurent Pinchart, dri-devel [-- Attachment #1.1: Type: text/plain, Size: 4250 bytes --] 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 [-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: Improve manual IRQ installation documentation 2013-06-20 11:00 ` Thierry Reding @ 2013-06-20 14:55 ` Daniel Vetter 2013-06-22 10:56 ` Thierry Reding 0 siblings, 1 reply; 9+ messages in thread From: Daniel Vetter @ 2013-06-20 14:55 UTC (permalink / raw) To: Thierry Reding; +Cc: Laurent Pinchart, Laurent Pinchart, dri-devel 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: Improve manual IRQ installation documentation 2013-06-20 14:55 ` Daniel Vetter @ 2013-06-22 10:56 ` Thierry Reding 2013-06-24 7:19 ` Daniel Vetter 0 siblings, 1 reply; 9+ messages in thread From: Thierry Reding @ 2013-06-22 10:56 UTC (permalink / raw) To: Daniel Vetter; +Cc: Laurent Pinchart, Laurent Pinchart, dri-devel [-- Attachment #1.1: Type: text/plain, Size: 5103 bytes --] 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 [-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm: Improve manual IRQ installation documentation 2013-06-22 10:56 ` Thierry Reding @ 2013-06-24 7:19 ` Daniel Vetter 0 siblings, 0 replies; 9+ messages in thread From: Daniel Vetter @ 2013-06-24 7:19 UTC (permalink / raw) To: Thierry Reding; +Cc: dri-devel, Laurent Pinchart, Laurent Pinchart 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-06-24 7:19 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-06-19 12:00 [PATCH] drm: Improve manual IRQ installation documentation Laurent Pinchart 2013-06-20 10:10 ` Thierry Reding 2013-06-20 10:17 ` Laurent Pinchart 2013-06-20 10:40 ` Thierry Reding 2013-06-20 10:46 ` Laurent Pinchart 2013-06-20 11:00 ` Thierry Reding 2013-06-20 14:55 ` Daniel Vetter 2013-06-22 10:56 ` Thierry Reding 2013-06-24 7:19 ` Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.