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
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
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);
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
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
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
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
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.
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
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.
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
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
dri-devel@lists.freedesktop.org
-
Alan Cox -
Daniel Vetter -
Laurent Pinchart -
Liu, Chuansheng -
Rob Clark -
Rob Clark -
Shi, Yang A