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
#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;
};
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);
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.1Quoting Rishikesh Donadkar (2025-08-25 19:55:22)
> 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.
>
> #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?
> };
>
> 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.
> 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
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
On 25/08/25 19:55, Rishikesh Donadkar wrote:
> 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>
> ---
Reviewed-by: Yemike Abhilash Chandra <y-abhilashchandra@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
>
> #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;
> };
>
> 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);
> 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);
© 2016 - 2025 Red Hat, Inc.