Re: [PATCH v4 12/12] media: ti: j721e-csi2rx: Change the drain architecture for multistream

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

On 25/06/25 07:03, Jai Luthra wrote:
> Hi Rishikesh,


Hi Jai,

Thank you for the review !


>
> Thanks for the patch.
>
> Quoting Rishikesh Donadkar (2025-05-14 04:25:27)
>> On buffer starvation the DMA is marked IDLE, and the stale data in the
>> internal FIFOs gets drained only on the next VIDIOC_QBUF call from the
>> userspace. This approach works fine for a single stream case.
>>
>> But in multistream scenarios, buffer starvation for one stream i.e. one
>> virtual channel, can block the shared HW FIFO of the CSI2RX IP. This can
>> stall the pipeline for all other virtual channels, even if buffers are
>> available for them.
>>
>> This patch introduces a new architecture, that continuously drains data
>> from the shared HW FIFO into a small (32KiB) buffer if no buffers are made
>> available to the driver from the userspace. This ensures independence
>> between different streams, where a slower downstream element for one
>> camera does not block streaming for other cameras.
>>
>> Signed-off-by: Rishikesh Donadkar <r-donadkar@ti.com>
>> ---
>>   .../platform/ti/j721e-csi2rx/j721e-csi2rx.c   | 96 +++++++------------
>>   1 file changed, 33 insertions(+), 63 deletions(-)
>>
>> diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
>> index ba2a30bfed37d..3b046d3cf7e5a 100644
>> --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
>> +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
>> @@ -57,7 +57,6 @@
>>   #define TI_CSI2RX_MAX_SOURCE_PADS      TI_CSI2RX_MAX_CTX
>>   #define TI_CSI2RX_MAX_PADS             (1 + TI_CSI2RX_MAX_SOURCE_PADS)
>>   
>> -#define DRAIN_TIMEOUT_MS               50
>>   #define DRAIN_BUFFER_SIZE              SZ_32K
>>   
>>   struct ti_csi2rx_fmt {
>> @@ -77,7 +76,6 @@ struct ti_csi2rx_buffer {
>>   
>>   enum ti_csi2rx_dma_state {
>>          TI_CSI2RX_DMA_STOPPED,  /* Streaming not started yet. */
>> -       TI_CSI2RX_DMA_IDLE,     /* Streaming but no pending DMA operation. */
>>          TI_CSI2RX_DMA_ACTIVE,   /* Streaming and pending DMA operation. */
>>   };
>>   
>> @@ -245,6 +243,10 @@ static const struct ti_csi2rx_fmt ti_csi2rx_formats[] = {
>>   static int ti_csi2rx_start_dma(struct ti_csi2rx_ctx *ctx,
>>                                 struct ti_csi2rx_buffer *buf);
>>   
>> +/* Forward declarations needed by ti_csi2rx_drain_callback. */
>> +static int ti_csi2rx_drain_dma(struct ti_csi2rx_ctx *ctx);
>> +static int ti_csi2rx_dma_submit_pending(struct ti_csi2rx_ctx *ctx);
>> +
>>   static const struct ti_csi2rx_fmt *find_format_by_fourcc(u32 pixelformat)
>>   {
>>          unsigned int i;
>> @@ -596,9 +598,28 @@ static void ti_csi2rx_setup_shim(struct ti_csi2rx_ctx *ctx)
>>   
>>   static void ti_csi2rx_drain_callback(void *param)
>>   {
>> -       struct completion *drain_complete = param;
>> +       struct ti_csi2rx_ctx *ctx = param;
>> +       struct ti_csi2rx_dma *dma = &ctx->dma;
>> +       unsigned long flags;
>> +
>> +       spin_lock_irqsave(&dma->lock, flags);
>>   
>> -       complete(drain_complete);
>> +       if (dma->state == TI_CSI2RX_DMA_STOPPED) {
>> +               spin_unlock_irqrestore(&dma->lock, flags);
>> +               return;
>> +       }
>> +
>> +       /*
>> +        * If dma->queue is empty, it signals no buffer has arrived from
>> +        * user space, so, queue more transaction to drain dma
>> +        */
>> +       if (list_empty(&dma->queue)) {
>> +               if (ti_csi2rx_drain_dma(ctx))
>> +                       dev_warn(ctx->csi->dev, "DMA drain failed\n");
>> +       } else {
>> +               ti_csi2rx_dma_submit_pending(ctx);
>> +       }
>> +       spin_unlock_irqrestore(&dma->lock, flags);
>>   }
>>   
>>   /*
>> @@ -616,12 +637,9 @@ static int ti_csi2rx_drain_dma(struct ti_csi2rx_ctx *ctx)
>>   {
>>          struct ti_csi2rx_dev *csi = ctx->csi;
>>          struct dma_async_tx_descriptor *desc;
>> -       struct completion drain_complete;
>>          dma_cookie_t cookie;
>>          int ret;
>>   
>> -       init_completion(&drain_complete);
>> -
>>          desc = dmaengine_prep_slave_single(ctx->dma.chan, csi->drain.paddr,
>>                                             csi->drain.len, DMA_DEV_TO_MEM,
>>                                             DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>> @@ -631,7 +649,7 @@ static int ti_csi2rx_drain_dma(struct ti_csi2rx_ctx *ctx)
>>          }
>>   
>>          desc->callback = ti_csi2rx_drain_callback;
>> -       desc->callback_param = &drain_complete;
>> +       desc->callback_param = ctx;
>>   
>>          cookie = dmaengine_submit(desc);
>>          ret = dma_submit_error(cookie);
>> @@ -640,13 +658,6 @@ static int ti_csi2rx_drain_dma(struct ti_csi2rx_ctx *ctx)
>>   
>>          dma_async_issue_pending(ctx->dma.chan);
>>   
>> -       if (!wait_for_completion_timeout(&drain_complete,
>> -                                        msecs_to_jiffies(DRAIN_TIMEOUT_MS))) {
>> -               dmaengine_terminate_sync(ctx->dma.chan);
>> -               dev_dbg(csi->dev, "DMA transfer timed out for drain buffer\n");
>> -               ret = -ETIMEDOUT;
>> -               goto out;
>> -       }
>>   out:
>>          return ret;
>>   }
>> @@ -694,9 +705,11 @@ static void ti_csi2rx_dma_callback(void *param)
>>   
>>          ti_csi2rx_dma_submit_pending(ctx);
>>   
>> -       if (list_empty(&dma->submitted))
>> -               dma->state = TI_CSI2RX_DMA_IDLE;
>> -
>> +       if (list_empty(&dma->submitted)) {
>> +               if (ti_csi2rx_drain_dma(ctx))
>> +                       dev_warn(ctx->csi->dev,
>> +                                "DMA drain failed on one of the transactions\n");
>> +       }
>>          spin_unlock_irqrestore(&dma->lock, flags);
>>   }
>>   
>> @@ -749,7 +762,7 @@ static void ti_csi2rx_stop_dma(struct ti_csi2rx_ctx *ctx)
>>                   * enforced before terminating DMA.
>>                   */
>>                  ret = ti_csi2rx_drain_dma(ctx);
>> -               if (ret && ret != -ETIMEDOUT)
>> +               if (ret)
>>                          dev_warn(ctx->csi->dev,
>>                                   "Failed to drain DMA. Next frame might be bogus\n");
>>          }
>> @@ -816,57 +829,14 @@ static void ti_csi2rx_buffer_queue(struct vb2_buffer *vb)
>>          struct ti_csi2rx_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
>>          struct ti_csi2rx_buffer *buf;
>>          struct ti_csi2rx_dma *dma = &ctx->dma;
>> -       bool restart_dma = false;
>>          unsigned long flags = 0;
>> -       int ret;
>>   
>>          buf = container_of(vb, struct ti_csi2rx_buffer, vb.vb2_buf);
>>          buf->ctx = ctx;
>>   
>>          spin_lock_irqsave(&dma->lock, flags);
>> -       /*
>> -        * Usually the DMA callback takes care of queueing the pending buffers.
>> -        * But if DMA has stalled due to lack of buffers, restart it now.
>> -        */
>> -       if (dma->state == TI_CSI2RX_DMA_IDLE) {
>> -               /*
>> -                * Do not restart DMA with the lock held because
>> -                * ti_csi2rx_drain_dma() might block for completion.
>> -                * There won't be a race on queueing DMA anyway since the
>> -                * callback is not being fired.
>> -                */
>> -               restart_dma = true;
>> -               dma->state = TI_CSI2RX_DMA_ACTIVE;
>> -       } else {
>> -               list_add_tail(&buf->list, &dma->queue);
>> -       }
>> +       list_add_tail(&buf->list, &dma->queue);
>>          spin_unlock_irqrestore(&dma->lock, flags);
>> -
>> -       if (restart_dma) {
>> -               /*
>> -                * Once frames start dropping, some data gets stuck in the DMA
>> -                * pipeline somewhere. So the first DMA transfer after frame
>> -                * drops gives a partial frame. This is obviously not useful to
> I think it would be good to retain a similar comment in the code, and also
> in the commit message, as there is a possibility of returning a partial frame
> to the user post drain.


Noted, Will add this info in the next revision of this series


Regards,

Rishikesh

>
> With that change,
>
> Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com>
>
>> -                * the application and will only confuse it. Issue a DMA
>> -                * transaction to drain that up.
>> -                */
>> -               ret = ti_csi2rx_drain_dma(ctx);
>> -               if (ret && ret != -ETIMEDOUT)
>> -                       dev_warn(ctx->csi->dev,
>> -                                "Failed to drain DMA. Next frame might be bogus\n");
>> -
>> -               spin_lock_irqsave(&dma->lock, flags);
>> -               ret = ti_csi2rx_start_dma(ctx, buf);
>> -               if (ret) {
>> -                       vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
>> -                       dma->state = TI_CSI2RX_DMA_IDLE;
>> -                       spin_unlock_irqrestore(&dma->lock, flags);
>> -                       dev_err(ctx->csi->dev, "Failed to start DMA: %d\n", ret);
>> -               } else {
>> -                       list_add_tail(&buf->list, &dma->submitted);
>> -                       spin_unlock_irqrestore(&dma->lock, flags);
>> -               }
>> -       }
>>   }
>>   
>>   static int ti_csi2rx_get_route(struct ti_csi2rx_ctx *ctx)
>> -- 
>> 2.34.1
> >