Re: [PATCH v4 11/12] media: ti: j721e-csi2rx: Submit all available buffers

[Sjoerd Simons]

Hey,

On Wed, 2025-05-14 at 16:55 +0530, Rishikesh Donadkar wrote:
> From: Jai Luthra <j-luthra@xxxxxx>
> 
> We already make sure to submit all available buffers to DMA in each DMA
> completion callback.
> 
> Move that logic in a separate function, and use it during stream start
> as well, as most application queue all their buffers before stream on.
> 
> Signed-off-by: Jai Luthra <j-luthra@xxxxxx>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> Signed-off-by: Rishikesh Donadkar <r-donadkar@xxxxxx>
> ---
>  .../platform/ti/j721e-csi2rx/j721e-csi2rx.c   | 43 +++++++++++--------
>  1 file changed, 24 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> index 7986f96c5e11b..ba2a30bfed37d 100644
> --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> @@ -651,6 +651,27 @@ static int ti_csi2rx_drain_dma(struct ti_csi2rx_ctx *ctx)
>  	return ret;
>  }
>  
> +static int ti_csi2rx_dma_submit_pending(struct ti_csi2rx_ctx *ctx)
> +{
> +	struct ti_csi2rx_dma *dma = &ctx->dma;
> +	struct ti_csi2rx_buffer *buf;
> +	int ret = 0;
> +
> +	/* If there are more buffers to process then start their transfer. */
> +	while (!list_empty(&dma->queue)) {
> +		buf = list_entry(dma->queue.next, struct ti_csi2rx_buffer,
> list);
> +		ret = ti_csi2rx_start_dma(ctx, buf);
> +		if (ret) {
> +			dev_err(ctx->csi->dev,
> +				"Failed to queue the next buffer for DMA\n");
> +			vb2_buffer_done(&buf->vb.vb2_buf,
> VB2_BUF_STATE_ERROR);
> +			break;

The break here seems wrong and does change the previous logic; It means once *a*
buffer fails to start DMA, you'll no longer try to submit the other (queued)
buffers. If this was called from the DMA callback of the last submitted buffer
and userspace doesn't re-queue the error buffer, then capturing will stop, even
if there were still queued up buffers from a userspace pov.


For a potential next iteration you probably also want to wrap in the changes
from to fix list_del corruption:
https://lore.kernel.org/all/20250630-j721e-dma-fixup-v1-1-591e378ab3a8@xxxxxxxxxxxxx/



> +		}
> +		list_move_tail(&buf->list, &dma->submitted);
> +	}
> +	return ret;
> +}
> +
>  static void ti_csi2rx_dma_callback(void *param)
>  {
>  	struct ti_csi2rx_buffer *buf = param;
> @@ -671,18 +692,7 @@ static void ti_csi2rx_dma_callback(void *param)
>  	vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
>  	list_del(&buf->list);
>  
> -	/* If there are more buffers to process then start their transfer. */
> -	while (!list_empty(&dma->queue)) {
> -		buf = list_entry(dma->queue.next, struct ti_csi2rx_buffer,
> list);
> -
> -		if (ti_csi2rx_start_dma(ctx, buf)) {
> -			dev_err(ctx->csi->dev,
> -				"Failed to queue the next buffer for DMA\n");
> -			vb2_buffer_done(&buf->vb.vb2_buf,
> VB2_BUF_STATE_ERROR);
> -		} else {
> -			list_move_tail(&buf->list, &dma->submitted);
> -		}
> -	}
> +	ti_csi2rx_dma_submit_pending(ctx);
>  
>  	if (list_empty(&dma->submitted))
>  		dma->state = TI_CSI2RX_DMA_IDLE;
> @@ -941,7 +951,6 @@ static int ti_csi2rx_start_streaming(struct vb2_queue *vq,
> unsigned int count)
>  	struct ti_csi2rx_ctx *ctx = vb2_get_drv_priv(vq);
>  	struct ti_csi2rx_dev *csi = ctx->csi;
>  	struct ti_csi2rx_dma *dma = &ctx->dma;
> -	struct ti_csi2rx_buffer *buf;
>  	unsigned long flags;
>  	int ret = 0;
>  
> @@ -980,16 +989,13 @@ static int ti_csi2rx_start_streaming(struct vb2_queue
> *vq, unsigned int count)
>  	ctx->sequence = 0;
>  
>  	spin_lock_irqsave(&dma->lock, flags);
> -	buf = list_entry(dma->queue.next, struct ti_csi2rx_buffer, list);
>  
> -	ret = ti_csi2rx_start_dma(ctx, buf);
> +	ret = ti_csi2rx_dma_submit_pending(ctx);
>  	if (ret) {
> -		dev_err(csi->dev, "Failed to start DMA: %d\n", ret);
>  		spin_unlock_irqrestore(&dma->lock, flags);
> -		goto err_pipeline;
> +		goto err_dma;
>  	}
>  
> -	list_move_tail(&buf->list, &dma->submitted);
>  	dma->state = TI_CSI2RX_DMA_ACTIVE;
>  	spin_unlock_irqrestore(&dma->lock, flags);
>  
> @@ -1004,7 +1010,6 @@ static int ti_csi2rx_start_streaming(struct vb2_queue
> *vq, unsigned int count)
>  
>  err_dma:
>  	ti_csi2rx_stop_dma(ctx);
> -err_pipeline:
>  	video_device_pipeline_stop(&ctx->vdev);
>  	writel(0, csi->shim + SHIM_CNTL);
>  	writel(0, csi->shim + SHIM_DMACNTX(ctx->idx));