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

[Rishikesh Donadkar <r-donadkar@ti.com>]

On 01/07/25 13:39, Sjoerd Simons wrote:
> Hey,


Hi Sjoerd,

>
> On Wed, 2025-05-14 at 16:55 +0530, Rishikesh Donadkar wrote:
>> From: Jai Luthra <j-luthra@ti.com>
>>
>> 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@ti.com>
>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> Signed-off-by: Rishikesh Donadkar <r-donadkar@ti.com>
>> ---
>>   .../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@collabora.com/


Thank you for the pointer !

I will make these changes in the next revision


Regards,

Rishikesh

>
>
>
>> +		}
>> +		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));