Re: [PATCH] omapfb vsync support

Mathias Agopian
2006-02-27T23:27:36Z

Hello,

I'm the one to blame for the omapfb vsync patch :-)

> ["Woodruff, Richard" <r-woodruff2@ti.com>]
>> How come there is no disable of the interrupt?
>>
>> Seems like at ioctrl the caller needs to be tracked so when he does a
>> close the interrupt can be disabled.

I decided not to bother too much about tracking the clients because
the omapfb driver is not thread-safe in a lot of ways, which means it
cannot really support more than one client anyway (or in very basic
situations). In other words, I assumed there would only always be one
client.

Nevertheless, we wouldn't really need to track the clients because I
think it would be fine to turn the IRQ off when any client closes the
driver (provided that no other client is currently waiting on it),
since it is re-enabled when a client calls the FBIO_WAITFORSYNC ioctl.

>> This will cause a VST enabled kernel to wake up for no good reason if
>> there is no application requesting it anymore. Someone pointed
>> out this
>> fact to me recently while doing power measurements. If you have a
>> low
>> power screen saver mode this can mess up expected power savings.
>
> Hmm. I wonder if there's some happy medium between disabling the
> interrupt every time the ioctl returns and having to close and reopen
> the driver when going into and returning from low power mode.
>
> Brian

Maybe we could simply disable the IRQ from the IRQ handler after a
second or so if no client has requested a FBIO_WAITFORSYNC ioctl?

That, combined with disabling the interrupt when a client closes the
driver should take care of most cases.

Alternatively, we could disable the IRQ each time, if it's not a too
heavy operation?

On Feb 27, 2006, at 3:10 PM, Woodruff, Richard wrote:

> Two bits, one is I don't see the disable being called in the code when
> you wake the process up. I see you have the function, at interrupt
> wake
> up time it should likely be disabled.

Yes, I added the function for completeness, I thought it might be
useful at some point (I thought the issue of not disabling it each
time would come up ;-) ).

Mathias