* [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.