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
> 1) 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);
--
Regards,
Laurent Pinchart
