2012-10-11 12:06:29

by Laurent Pinchart

[permalink] [raw]
Subject: 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 +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


2012-10-11 13:19:22

by Rob Clark

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

On Thu, Oct 11, 2012 at 7:07 AM, Laurent Pinchart
<[email protected]> 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
> [email protected]
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

2012-10-11 15:18:06

by Daniel Vetter

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

On Thu, Oct 11, 2012 at 3:19 PM, Rob Clark <[email protected]> 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