Re: Locking in the clk API

[Nicolas Pitre]

On Fri, 21 Jan 2011, Dima Zavin wrote:

> On Thu, Jan 20, 2011 at 8:12 PM, Saravana Kannan <skannan@xxxxxxxxxxxxxx> wrote:
> > On 01/20/2011 06:06 PM, Dima Zavin wrote:
> >> Here's a better one.
> >>
> >> Many devices use serial display panels sitting on either MDDI or MIPI
> >> links. The interface clocks need to be on, but they stay in low-power
> >> mode while the display is on. The display controller however does not
> >> need to be on since the serial panels typically have a local
> >> framebuffer that does the idle panel refresh on it's own. When a new
> >> frame comes in to be displayed, you need to clock on the display
> >> controller, DMA the data to the panel, and when it's done turn the
> >> controller off. The clk_enable may or may not happen at irq context,
> >> depending on whether or not you are starting the DMA from a
> >> vsync/tear-effect irq or simply from the screen_update() function. The
> >> clk_disable will most certainly happen from the DMA_DONE irq.
> >
> > Why do we need to turn on the clock in the IRQ? Why not defer it to a
> > workqueue (or whatever is the method of the day to defer work from an IRQ)?
> > The advantage of doing the clk_enable in the IRQ should be negligible
> > compared to the time it takes to do the DMA.
> 
> Because in an interactive system running at 60fps, you only have 16ms
> budget per frame. During the blanking interval when you receive the
> IRQ you need to immediately start the DMA. If you defer to a workqueue
> and schedule you are practically guaranteeing to never run at 60fps
> (more like 30 if you are consistently late, which you would with that
> kind of timing and even a mildly busy system).

And... is there really some advantage to turn the clock off in between 
frames when you're otherwise busy generating them anyway?

I think the fundamental problem with this clock API is that it tries to 
consolidate completely different usages behind the same set of 
interfaces, and that obviously fails.

Some people have clocks that are derived from slow-stabilizing PLLs and 
in that case you do want to use a sleeping API.  Obviously such clocks 
just can't be turned off and on between video frames while in interrupt 
context, hence for such clocks, the hypothetical clk_disable() call must 
do nothing.

So I think that the API must be augmented with more methods, such as:

clk_slow_enable():
  - may sleep
  - may be a no-op if the clk_fast_enable() is supported

clk_fast_enable():
  - may not sleep, used in atomic context
  - may be a no-op if controlling the clock takes time, in which case 
    clk_slow_enable() must have set the clock up entirely

... and similar for clk_slow_disable() and clk_fast_disable().

Then the driver must call clk_slow_enable() when in sleepable context, 
at worst after a successful probe, or better within the open method, or 
even better right before some processing request if possible.  The 
driver must also call clk_fast_enable() right before the hardware is 
kicked into motion.

Then, as soon as the IRQ from the hardware to indicate that the job is 
done is received, you call clk_fast_disable().  At that point you might 
awake a sleeping thread waiting for the resulting data.  In that thread, 
you then call clk_slow_disable() if possible, or from the driver's 
release() method otherwise.

So if you happen to have a fast-enabling clock, you make 
clk_slow_enable() a no op, and do everything in clk_fast_enable().  If 
your clock control sits on some I2C bus then you must do everything from 
clk_slow_enable() and make clk_fast_enable() a no op.  If you do have 
both a PLL and the ability to gate clock signals, then you just 
configure the PLL from clk_slow_enable() keeping the clock signal gated, 
and ungate it only from clk_fast_enable(), to gate it (and only 
gate it) with clk_fast_disable() so it can be quickly be ungated again 
with clk_fast_enable().

This way, the API has a chance of making drivers look generic, and the 
clock providers have more flexibility to hook into the API and optimize 
to the best they can do.

Of course that means that drivers must call both the slow and the fast 
methods eventually, each in their best place.  If a driver has no use 
for clk_fast_enable() then it should simply call it right after 
clk_slow_enable() returns (and a convenience macro could be provided for 
that case).


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/