Hi Tomi,
On Fri, Mar 13, 2020 at 04:18:13PM +0200, Tomi Valkeinen wrote:
On 13/03/2020 16:03, Laurent Pinchart wrote:
- /* wait for stream and dma to finish */
- dma_act = true;
- timeout = jiffies + msecs_to_jiffies(500);
- while (dma_act && time_before(jiffies, timeout)) {
msleep(50);spin_lock_irqsave(&ctx->slock, flags);dma_act = ctx->dma_act;spin_unlock_irqrestore(&ctx->slock, flags);- }
Waiting for the transfer to complete seems to be a good idea, but how about using a wait queue instead of such a loop ? That would allow better usage of CPU time and faster reaction time, and shouldn't be difficult to implement. You may also want to replace dma_act with a state if needed (in case you need to express running/stopping/stopped states), and I would rename it to running if you just need a boolean.
Maybe, but I wasn't sure how to implement it safely.
So, when we call csi2_ppi_disable() (just above the wait code above), the HW will stop the DMA after the next frame has ended.
But there's no way to know in the irq handler if the DMA transfer that just ended was the last one or not. And I don't see how I could set a "disabling" flag before calling csi2_ppi_disable(), as I think that would always be racy with the irq handler.
So I went with a safe way: call csi2_ppi_disable(), then wait a bit so that we are sure that either
- the last frame is on going 2) the last frame has finished (instead of the previous-to-last frame
is on going or finished). Then see if the DMA is active. If yes, we loop for it to end.
I think the loop could be replaced with a wait queue, but we still need the initial sleep to ensure we don't end the wait when the previous-to-last frame DMA has been finished.
I think you can solve this by introducing a new enum state field with RUNNING, STOPPING and STOPPED values, protected by a spinlock. Here's what I have in the VSP1 driver for instance:
bool vsp1_pipeline_stopped(struct vsp1_pipeline *pipe) { unsigned long flags; bool stopped;
spin_lock_irqsave(&pipe->irqlock, flags); stopped = pipe->state == VSP1_PIPELINE_STOPPED; spin_unlock_irqrestore(&pipe->irqlock, flags);
return stopped; }
int vsp1_pipeline_stop(struct vsp1_pipeline *pipe) { ... spin_lock_irqsave(&pipe->irqlock, flags); if (pipe->state == VSP1_PIPELINE_RUNNING) pipe->state = VSP1_PIPELINE_STOPPING; spin_unlock_irqrestore(&pipe->irqlock, flags);
ret = wait_event_timeout(pipe->wq, vsp1_pipeline_stopped(pipe), msecs_to_jiffies(500)); ret = ret == 0 ? -ETIMEDOUT : 0; ... }
and in the interrupt handler:
state = pipe->state; pipe->state = VSP1_PIPELINE_STOPPED;
/* * If a stop has been requested, mark the pipeline as stopped and * return. Otherwise restart the pipeline if ready. */ if (state == VSP1_PIPELINE_STOPPING) wake_up(&pipe->wq); else if (vsp1_pipeline_ready(pipe)) vsp1_video_pipeline_run(pipe);