[Patch 0/1]drm_irq: Introducing the irq_thread support

[Patch 0/1]drm_irq: Introducing the irq_thread support

[Liu, Chuansheng]

This patch is for introducing the irq thread support in drm_irq.

Why we need irq thread in drm_irq code?
In our GPU system, the gpu interrupt handler need some time even > 1ms to finish,
in that case, the whole system will stay in irq disable status. One case is:
when audio is playing, it sometimes effects the audio quality.

So we have to introduce the irq thread in drm_irq, it can help us move some heavy work into irq thread
and other irq interrupts can be handled in time. Also the IRQF_ONESHOT is helpful for irq thread.

Include one patch:
[PATCH 01/1] drm_irq-Introducing-the-irq_thread-support

Re: [Patch 0/1]drm_irq: Introducing the irq_thread support

[Alan Cox]

On Wed, 5 Sep 2012 01:53:44 +0000
"Liu, Chuansheng" <chuansheng.liu@intel.com> wrote:

> This patch is for introducing the irq thread support in drm_irq.
> 
> Why we need irq thread in drm_irq code?
> In our GPU system, the gpu interrupt handler need some time even > 1ms to finish,
> in that case, the whole system will stay in irq disable status. One case is:
> when audio is playing, it sometimes effects the audio quality.

This possibly ought to be submitted in parallel with the code that uses it
so that the whole proposal can be evaluated as one thing ?

Alan

Re: [Patch 0/1]drm_irq: Introducing the irq_thread support

[Daniel Vetter]

On Wed, Sep 05, 2012 at 01:53:44AM +0000, Liu, Chuansheng wrote:
> This patch is for introducing the irq thread support in drm_irq.
> 
> Why we need irq thread in drm_irq code?
> In our GPU system, the gpu interrupt handler need some time even > 1ms to finish,
> in that case, the whole system will stay in irq disable status. One case is:
> when audio is playing, it sometimes effects the audio quality.
> 
> So we have to introduce the irq thread in drm_irq, it can help us move some heavy work into irq thread
> and other irq interrupts can be handled in time. Also the IRQF_ONESHOT is helpful for irq thread.
> 
> Include one patch:
> [PATCH 01/1] drm_irq-Introducing-the-irq_thread-support

For a kms drm driver (and tbh, doing a non-kms driver today is not a great
idea), there's no reason to use the drm_irq_install/_unistall helpers.

So if you driver has special needs wrt irq handling that don't neatly fit
what the drm_irq stuff provides, simply don't use it - all the generic
code that's there is just to keep non-kms userspace going.

Yours, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

RE: [Patch 0/1]drm_irq: Introducing the irq_thread support

[Shi, Yang A]

Hi Vetter:

    Do you mean we can just not use drm_irq_install, and make request_irq in our kernel driver pre-install or post-install interface?






Best Regards.
Yang Shi

PSI,System Integration, SH
E-mail:yang.a.shi@intel.com
Tel:88215666-4239


-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Wednesday, September 05, 2012 9:27 PM
To: Liu, Chuansheng
Cc: 'linux-kernel@vger.kernel.org' (linux-kernel@vger.kernel.org); dri-devel@lists.freedesktop.org; alexander.deucher@amd.com; airlied@redhat.com; Shi, Yang A
Subject: Re: [Patch 0/1]drm_irq: Introducing the irq_thread support

On Wed, Sep 05, 2012 at 01:53:44AM +0000, Liu, Chuansheng wrote:
> This patch is for introducing the irq thread support in drm_irq.
> 
> Why we need irq thread in drm_irq code?
> In our GPU system, the gpu interrupt handler need some time even > 1ms 
> to finish, in that case, the whole system will stay in irq disable status. One case is:
> when audio is playing, it sometimes effects the audio quality.
> 
> So we have to introduce the irq thread in drm_irq, it can help us move 
> some heavy work into irq thread and other irq interrupts can be handled in time. Also the IRQF_ONESHOT is helpful for irq thread.
> 
> Include one patch:
> [PATCH 01/1] drm_irq-Introducing-the-irq_thread-support

For a kms drm driver (and tbh, doing a non-kms driver today is not a great idea), there's no reason to use the drm_irq_install/_unistall helpers.

So if you driver has special needs wrt irq handling that don't neatly fit what the drm_irq stuff provides, simply don't use it - all the generic code that's there is just to keep non-kms userspace going.

Yours, Daniel
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [Patch 0/1]drm_irq: Introducing the irq_thread support

[Daniel Vetter]

On Wed, Sep 05, 2012 at 03:12:39PM +0000, Shi, Yang A wrote:
> Hi Vetter:
> 
>     Do you mean we can just not use drm_irq_install, and make
>     request_irq in our kernel driver pre-install or post-install
>     interface?

Well, you cant use the pre_install/post_install hooks the drm_irq code
provides, but yes, just do the request_irq in your driver code at the
right time, with the right parameters. Much easier than adding code to a
part of the drm core fraught with backwards-compat stuff no one really
wants to touch ... All the additional stuff besides calling request_irq
and the driver hooks that drm_irq_install does is really just to support
old dri1 userspace.

Yours, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [Patch 0/1]drm_irq: Introducing the irq_thread support

[Rob Clark]

On Wed, Sep 5, 2012 at 8:27 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Sep 05, 2012 at 01:53:44AM +0000, Liu, Chuansheng wrote:
>> This patch is for introducing the irq thread support in drm_irq.
>>
>> Why we need irq thread in drm_irq code?
>> In our GPU system, the gpu interrupt handler need some time even > 1ms to finish,
>> in that case, the whole system will stay in irq disable status. One case is:
>> when audio is playing, it sometimes effects the audio quality.
>>
>> So we have to introduce the irq thread in drm_irq, it can help us move some heavy work into irq thread
>> and other irq interrupts can be handled in time. Also the IRQF_ONESHOT is helpful for irq thread.
>>
>> Include one patch:
>> [PATCH 01/1] drm_irq-Introducing-the-irq_thread-support
>
> For a kms drm driver (and tbh, doing a non-kms driver today is not a great
> idea), there's no reason to use the drm_irq_install/_unistall helpers.
>
> So if you driver has special needs wrt irq handling that don't neatly fit
> what the drm_irq stuff provides, simply don't use it - all the generic
> code that's there is just to keep non-kms userspace going.

perhaps an easy thing would just be to allow the driver to provide
it's own request_irq?  That might be an easier way for devices that
need to register multiple irq's, etc?

Or is it better to just bypass and dev->irq_enabled=1?  That seemed a
bit like a hack to me, but the current irq code is more framework-ish,
and less helper-ish..

BR,
-R

> Yours, Daniel
> --
> Daniel Vetter
> Mail: daniel@ffwll.ch
> Mobile: +41 (0)79 365 57 48
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [Patch 0/1]drm_irq: Introducing the irq_thread support

[Liu, Chuansheng]

> This possibly ought to be submitted in parallel with the code that uses it so that
> the whole proposal can be evaluated as one thing ?
> 
> Alan

Patch is here, thanks.

From: liu chuansheng <chuansheng.liu@intel.com>
Subject: [PATCH] drm_irq: Introducing the irq_thread support

For some GPUs, the irq handler need >1ms to handle the irq action.
And it will delay the whole system irq handler.

This patch is adding the irq thread support, it will make the drm_irq
interface more flexible.

The changes include:
1/ Change the request_irq to request_thread_irq, and add new callback
   irq_handler_t;
2/ Normally we need IRQF_ONESHOT flag support for irq thread, so add
   this option in drm_irq;

Cc: Shi Yang <yang.a.shi@intel.com>
Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
---
 drivers/gpu/drm/drm_irq.c |    8 ++++++--
 include/drm/drmP.h        |    2 ++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 03f16f3..bc105fe 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -345,13 +345,17 @@ int drm_irq_install(struct drm_device *dev)
        if (drm_core_check_feature(dev, DRIVER_IRQ_SHARED))
                sh_flags = IRQF_SHARED;
 
+       if (drm_core_check_feature(dev, DRIVER_IRQ_ONESHOT))
+               sh_flags |= IRQF_ONESHOT;
+
        if (dev->devname)
                irqname = dev->devname;
        else
                irqname = dev->driver->name;
 
-       ret = request_irq(drm_dev_to_irq(dev), dev->driver->irq_handler,
-                         sh_flags, irqname, dev);
+       ret = request_threaded_irq(drm_dev_to_irq(dev),
+               dev->driver->irq_handler, dev->driver->irq_handler_t,
+               sh_flags, irqname, dev);
 
        if (ret < 0) {
                mutex_lock(&dev->struct_mutex);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index d6b67bb..b28be4b 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -152,6 +152,7 @@ int drm_err(const char *func, const char *format, ...);
 #define DRIVER_GEM         0x1000
 #define DRIVER_MODESET     0x2000
 #define DRIVER_PRIME       0x4000
+#define DRIVER_IRQ_ONESHOT 0x8000
 
 #define DRIVER_BUS_PCI 0x1
 #define DRIVER_BUS_PLATFORM 0x2
@@ -872,6 +873,7 @@ struct drm_driver {
        /* these have to be filled in */
 
        irqreturn_t(*irq_handler) (DRM_IRQ_ARGS);
+       irqreturn_t(*irq_handler_t) (DRM_IRQ_ARGS);
        void (*irq_preinstall) (struct drm_device *dev);
        int (*irq_postinstall) (struct drm_device *dev);
        void (*irq_uninstall) (struct drm_device *dev);
-- 
1.7.0.4

RE: [Patch 0/1]drm_irq: Introducing the irq_thread support

[Liu, Chuansheng]

> For a kms drm driver (and tbh, doing a non-kms driver today is not a great idea),
> there's no reason to use the drm_irq_install/_unistall helpers.
> 
Can not understand well, I found many GPU drivers are using drm_irq helpers' function, including ours:)

RE: [Patch 0/1]drm_irq: Introducing the irq_thread support

[Liu, Chuansheng]

> Well, you cant use the pre_install/post_install hooks the drm_irq code provides,
> but yes, just do the request_irq in your driver code at the right time, with the
> right parameters. Much easier than adding code to a part of the drm core
> fraught with backwards-compat stuff no one really wants to touch ... All the
> additional stuff besides calling request_irq and the driver hooks that
> drm_irq_install does is really just to support old dri1 userspace.
> 
Please have a look for the patch, I just added the callback of irq thread handler, default is NULL without set,
So it should be no impact with others.

In case irq threadler func is NULL, it equals to request_irq, thanks.

Re: [Patch 0/1]drm_irq: Introducing the irq_thread support

[Daniel Vetter]

On Thu, Sep 06, 2012 at 12:42:05AM +0000, Liu, Chuansheng wrote:
> > This possibly ought to be submitted in parallel with the code that uses it so that
> > the whole proposal can be evaluated as one thing ?
> > 
> > Alan
> 
> Patch is here, thanks.
> 
> From: liu chuansheng <chuansheng.liu@intel.com>
> Subject: [PATCH] drm_irq: Introducing the irq_thread support
> 
> For some GPUs, the irq handler need >1ms to handle the irq action.
> And it will delay the whole system irq handler.
> 
> This patch is adding the irq thread support, it will make the drm_irq
> interface more flexible.
> 
> The changes include:
> 1/ Change the request_irq to request_thread_irq, and add new callback
>    irq_handler_t;
> 2/ Normally we need IRQF_ONESHOT flag support for irq thread, so add
>    this option in drm_irq;
> 
> Cc: Shi Yang <yang.a.shi@intel.com>
> Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>

Nacked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I _really_ hate when we add random special cases for random strange
drivers to core code - the usual end result is that in a few years we'll
have a maze of special-cases only used by one driver each. And nope,
cleaning that up isn't _that_ much fun ...

So just do all this in your own driver's code (and maybe set
dev->irq_enabled ourselve so that wait_vblank still works).

Yours, Daniel

> ---
>  drivers/gpu/drm/drm_irq.c |    8 ++++++--
>  include/drm/drmP.h        |    2 ++
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 03f16f3..bc105fe 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -345,13 +345,17 @@ int drm_irq_install(struct drm_device *dev)
>         if (drm_core_check_feature(dev, DRIVER_IRQ_SHARED))
>                 sh_flags = IRQF_SHARED;
>  
> +       if (drm_core_check_feature(dev, DRIVER_IRQ_ONESHOT))
> +               sh_flags |= IRQF_ONESHOT;
> +
>         if (dev->devname)
>                 irqname = dev->devname;
>         else
>                 irqname = dev->driver->name;
>  
> -       ret = request_irq(drm_dev_to_irq(dev), dev->driver->irq_handler,
> -                         sh_flags, irqname, dev);
> +       ret = request_threaded_irq(drm_dev_to_irq(dev),
> +               dev->driver->irq_handler, dev->driver->irq_handler_t,
> +               sh_flags, irqname, dev);
>  
>         if (ret < 0) {
>                 mutex_lock(&dev->struct_mutex);
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index d6b67bb..b28be4b 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -152,6 +152,7 @@ int drm_err(const char *func, const char *format, ...);
>  #define DRIVER_GEM         0x1000
>  #define DRIVER_MODESET     0x2000
>  #define DRIVER_PRIME       0x4000
> +#define DRIVER_IRQ_ONESHOT 0x8000
>  
>  #define DRIVER_BUS_PCI 0x1
>  #define DRIVER_BUS_PLATFORM 0x2
> @@ -872,6 +873,7 @@ struct drm_driver {
>         /* these have to be filled in */
>  
>         irqreturn_t(*irq_handler) (DRM_IRQ_ARGS);
> +       irqreturn_t(*irq_handler_t) (DRM_IRQ_ARGS);
>         void (*irq_preinstall) (struct drm_device *dev);
>         int (*irq_postinstall) (struct drm_device *dev);
>         void (*irq_uninstall) (struct drm_device *dev);
> -- 
> 1.7.0.4
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [Patch 0/1]drm_irq: Introducing the irq_thread support

[Laurent Pinchart]

On Wednesday 05 September 2012 15:27:24 Daniel Vetter wrote:
> On Wed, Sep 05, 2012 at 01:53:44AM +0000, Liu, Chuansheng wrote:
> > This patch is for introducing the irq thread support in drm_irq.
> > 
> > Why we need irq thread in drm_irq code?
> > In our GPU system, the gpu interrupt handler need some time even > 1ms to
> > finish, in that case, the whole system will stay in irq disable status.
> > One case is: when audio is playing, it sometimes effects the audio
> > quality.
> > 
> > So we have to introduce the irq thread in drm_irq, it can help us move
> > some heavy work into irq thread and other irq interrupts can be handled
> > in time. Also the IRQF_ONESHOT is helpful for irq thread.
> > 
> > Include one patch:
> > [PATCH 01/1] drm_irq-Introducing-the-irq_thread-support
> 
> For a kms drm driver (and tbh, doing a non-kms driver today is not a great
> idea), there's no reason to use the drm_irq_install/_unistall helpers.

Should the documenation be updated to mark those functions as deprecated for 
new drivers and explain how to handle IRQ (un)registration manually ?

> So if you driver has special needs wrt irq handling that don't neatly fit
> what the drm_irq stuff provides, simply don't use it - all the generic
> code that's there is just to keep non-kms userspace going.

-- 
Regards,

Laurent Pinchart

Re: [Patch 0/1]drm_irq: Introducing the irq_thread support

[Rob Clark]

On Thu, Oct 11, 2012 at 7:07 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wednesday 05 September 2012 15:27:24 Daniel Vetter wrote:
>> On Wed, Sep 05, 2012 at 01:53:44AM +0000, Liu, Chuansheng wrote:
>> > This patch is for introducing the irq thread support in drm_irq.
>> >
>> > Why we need irq thread in drm_irq code?
>> > In our GPU system, the gpu interrupt handler need some time even > 1ms to
>> > finish, in that case, the whole system will stay in irq disable status.
>> > One case is: when audio is playing, it sometimes effects the audio
>> > quality.
>> >
>> > So we have to introduce the irq thread in drm_irq, it can help us move
>> > some heavy work into irq thread and other irq interrupts can be handled
>> > in time. Also the IRQF_ONESHOT is helpful for irq thread.
>> >
>> > Include one patch:
>> > [PATCH 01/1] drm_irq-Introducing-the-irq_thread-support
>>
>> For a kms drm driver (and tbh, doing a non-kms driver today is not a great
>> idea), there's no reason to use the drm_irq_install/_unistall helpers.
>
> Should the documenation be updated to mark those functions as deprecated for
> new drivers and explain how to handle IRQ (un)registration manually ?

It might be nice to provide the driver an option to give it's own
install/uninstall irq fxn ptrs.. this way we can keep
drm_irq_install/uninstall().  In particular, the uninstall fxn still
does some useful cleanup like wake up anyone waiting for vblank events
which would still be needed by drivers registering irq in their own
special way.  And the irq pre/post-install stuff is still a bit nice
to keep.

BR,
-R

>> So if you driver has special needs wrt irq handling that don't neatly fit
>> what the drm_irq stuff provides, simply don't use it - all the generic
>> code that's there is just to keep non-kms userspace going.
>
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Patch 0/1]drm_irq: Introducing the irq_thread support

[Daniel Vetter]

On Thu, Oct 11, 2012 at 3:19 PM, Rob Clark <robdclark@gmail.com> wrote:
>> Should the documenation be updated to mark those functions as deprecated for
>> new drivers and explain how to handle IRQ (un)registration manually ?

They're not deprecated, since for most drivers they're good enough.
Maybe just make it clear that they're optional (and whoever's the
first might need to do some refactoring to make things simpler for
fancy irq handling).

> It might be nice to provide the driver an option to give it's own
> install/uninstall irq fxn ptrs.. this way we can keep
> drm_irq_install/uninstall().  In particular, the uninstall fxn still
> does some useful cleanup like wake up anyone waiting for vblank events
> which would still be needed by drivers registering irq in their own
> special way.  And the irq pre/post-install stuff is still a bit nice
> to keep.

If a driver needs its own irq setup/teardown magic, I'd prefer if we
simply extract the useful parts of the common code into a little
helper that drivers can call, and don't add new&fancy callbacks and
interface. At least not without really good reasons.

/me has seen enough midlayer awesomeness in drm land already

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch