Re: [PATCH v5 14/14] media: ti: j721e-csi2rx: Wait for the last drain completion

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

On 05/09/25 16:41, Jai Luthra wrote:
> Quoting Rishikesh Donadkar (2025-08-25 19:55:22)

Hi Jai,

Thank you for the comments.

>> dmaengine_terminate_sync() causes all activity for the DMA channel to be
>> stopped, and may discard data in the DMA FIFO which hasn't been fully
>> transferred. No callback functions will be called for any
>> incomplete transfers[1].
>>
>> In multistream use case, calling dmaengine_terminate_sync() immediately
>> after issuing the last drain transaction will result in no callback
>> for the last drain cycle.
>>
>> Implement complete callback for the last drain cycle to make sure that
>> the last drain has completed properly, this will ensure that stale data
>> is not left out in the HW FIFO.
>>
>> [1] : https://docs.kernel.org/driver-api/dmaengine/client.html
>>
>> Signed-off-by: Rishikesh Donadkar <r-donadkar@ti.com>
>> ---
>>   drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
>> index 4ac6a76b9409..520ee05eb5b4 100644
>> --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
>> +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
>> @@ -62,6 +62,7 @@
>>   #define TI_CSI2RX_MAX_PADS             (1 + TI_CSI2RX_MAX_SOURCE_PADS)
>>   
>>   #define DRAIN_BUFFER_SIZE              SZ_32K
>> +#define DRAIN_TIMEOUT_MS               50
> This was dropped in the previous patch, and now reintroduce.
>
> IIUC this patch is fixing a bug introduced by the previous one, so it's
> better to squash them together, and have a combined commit description that
> goes over this end-of-stream case, as well as why continuous drain was
> needed for mid-stream scenario.

Okay

>
>>   
>>   #define CSI2RX_BRIDGE_SOURCE_PAD       1
>>   
>> @@ -137,6 +138,7 @@ struct ti_csi2rx_dev {
>>                  size_t                  len;
>>          } drain;
>>          bool                            vc_cached;
>> +       struct completion drain_complete;
> Why is the struct completion shared amongst all contexts in the
> ti_csi2rx_dev structure?
>
> What happens when two streams are stopped together?
Right, this struct completion must be per ctx.
>
>>   };
>>   
>>   static inline struct ti_csi2rx_dev *to_csi2rx_dev(struct v4l2_subdev *sd)
>> @@ -624,12 +626,14 @@ static void ti_csi2rx_setup_shim(struct ti_csi2rx_ctx *ctx)
>>   static void ti_csi2rx_drain_callback(void *param)
>>   {
>>          struct ti_csi2rx_ctx *ctx = param;
>> +       struct ti_csi2rx_dev *csi = ctx->csi;
>>          struct ti_csi2rx_dma *dma = &ctx->dma;
>>          unsigned long flags;
>>   
>>          spin_lock_irqsave(&dma->lock, flags);
>>   
>>          if (dma->state == TI_CSI2RX_DMA_STOPPED) {
>> +               complete(&csi->drain_complete);
> Please also add comment above this if case explaining why we need to wait
> for the drain to complete when dma->state == STOPPED, which is set by the
> driver elsewhere when streamoff was requested, and no more data will be
> coming in from the source.
Sure, I believe it would make more sense to add this comment above the 
wait_for_completion_timeout() call.
>
>>                  spin_unlock_irqrestore(&dma->lock, flags);
>>                  return;
>>          }
>> @@ -774,6 +778,7 @@ static int ti_csi2rx_start_dma(struct ti_csi2rx_ctx *ctx,
>>   static void ti_csi2rx_stop_dma(struct ti_csi2rx_ctx *ctx)
>>   {
>>          struct ti_csi2rx_dma *dma = &ctx->dma;
>> +       struct ti_csi2rx_dev *csi = ctx->csi;
>>          enum ti_csi2rx_dma_state state;
>>          unsigned long flags;
>>          int ret;
>> @@ -783,6 +788,8 @@ static void ti_csi2rx_stop_dma(struct ti_csi2rx_ctx *ctx)
>>          dma->state = TI_CSI2RX_DMA_STOPPED;
>>          spin_unlock_irqrestore(&dma->lock, flags);
>>   
>> +       init_completion(&csi->drain_complete);
>> +
>>          if (state != TI_CSI2RX_DMA_STOPPED) {
>>                  /*
>>                   * Normal DMA termination does not clean up pending data on
>> @@ -796,6 +803,10 @@ static void ti_csi2rx_stop_dma(struct ti_csi2rx_ctx *ctx)
>>                                   "Failed to drain DMA. Next frame might be bogus\n");
>>          }
>>   
>> +       if (!wait_for_completion_timeout(&csi->drain_complete,
>> +                                        msecs_to_jiffies(DRAIN_TIMEOUT_MS)))
>> +               dev_dbg(csi->dev, "DMA transfer timed out for drain buffer\n");
>> +
>>          ret = dmaengine_terminate_sync(ctx->dma.chan);
>>          if (ret)
>>                  dev_err(ctx->csi->dev, "Failed to stop DMA: %d\n", ret);
>> -- 
>> 2.34.1
>>
> Thanks,
>      Jai

Regards,

Rishikesh