Re: [Patch 0/1]drm_irq: Introducing the irq_thread support
On Thu, Oct 11, 2012 at 3:19 PM, Rob Clark 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Patch 0/1]drm_irq: Introducing the irq_thread support
On Thu, Oct 11, 2012 at 7:07 AM, Laurent Pinchart wrote: > On Wednesday 05 September 2012 15:27:24 Daniel Vetter wrote: >> On Wed, Sep 05, 2012 at 01:53:44AM +, 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-de...@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Patch 0/1]drm_irq: Introducing the irq_thread support
On Wednesday 05 September 2012 15:27:24 Daniel Vetter wrote: > On Wed, Sep 05, 2012 at 01:53:44AM +, 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Patch 0/1]drm_irq: Introducing the irq_thread support
On Thu, Sep 06, 2012 at 12:42:05AM +, 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
> 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
> Signed-off-by: liu chuansheng
Nacked-by: Daniel Vetter
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-de...@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Mail: dan...@ffwll.ch
Mobile: +41 (0)79 365 57 48
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [Patch 0/1]drm_irq: Introducing the irq_thread support
> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [Patch 0/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. > Can not understand well, I found many GPU drivers are using drm_irq helpers' function, including ours:) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [Patch 0/1]drm_irq: Introducing the irq_thread support
> 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
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
Signed-off-by: liu chuansheng
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [Patch 0/1]drm_irq: Introducing the irq_thread support
On Wed, Sep 5, 2012 at 8:27 AM, Daniel Vetter wrote: > On Wed, Sep 05, 2012 at 01:53:44AM +, 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: dan...@ffwll.ch > Mobile: +41 (0)79 365 57 48 > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Patch 0/1]drm_irq: Introducing the irq_thread support
On Wed, Sep 05, 2012 at 03:12:39PM +, 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: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [Patch 0/1]drm_irq: Introducing the irq_thread support
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@intel.com Tel:88215666-4239 -Original Message- From: Daniel Vetter [mailto:daniel.vet...@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-de...@lists.freedesktop.org; alexander.deuc...@amd.com; airl...@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 +, 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: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Patch 0/1]drm_irq: Introducing the irq_thread support
On Wed, Sep 05, 2012 at 01:53:44AM +, 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: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Patch 0/1]drm_irq: Introducing the irq_thread support
On Wed, 5 Sep 2012 01:53:44 + "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. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[Patch 0/1]drm_irq: Introducing the irq_thread support
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
