| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2da35b0a-de39-4f15-9b7d-4cc2e3a5237b@ti.com>
Date: Mon, 4 Aug 2025 15:56:50 +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 v4 12/12] media: ti: j721e-csi2rx: Change the drain
architecture for multistream
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@...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@...asonboard.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
> >
Powered by blists - more mailing lists