Re: [PATCH] media: ti-vpe: cal: fix DMA memory corruption

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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