[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <5f830f6e-f48d-4150-b705-0cad04ea5267@ti.com>
Date: Mon, 8 Sep 2025 15:21:19 +0530
From: Rishikesh Donadkar <r-donadkar@...com>
To: Jai Luthra <jai.luthra@...asonboard.com>, <jai.luthra@...ux.dev>,
<laurent.pinchart@...asonboard.com>, <mripard@...nel.org>
CC: <y-abhilashchandra@...com>, <devarsht@...com>, <vaishnav.a@...com>,
<s-jain1@...com>, <vigneshr@...com>, <mchehab@...nel.org>,
<robh@...nel.org>, <krzk+dt@...nel.org>, <conor+dt@...nel.org>,
<sakari.ailus@...ux.intel.com>, <hverkuil-cisco@...all.nl>,
<tomi.valkeinen@...asonboard.com>, <changhuang.liang@...rfivetech.com>,
<jack.zhu@...rfivetech.com>, <linux-kernel@...r.kernel.org>,
<linux-media@...r.kernel.org>, <devicetree@...r.kernel.org>
Subject: Re: [PATCH v5 14/14] media: ti: j721e-csi2rx: Wait for the last drain
completion
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@...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
Powered by blists - more mailing lists