| 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: <9fa2e0ab-fed1-4d19-a735-e5458014f549@ti.com>
Date: Thu, 21 Aug 2025 11:51:57 +0530
From: Rishikesh Donadkar <r-donadkar@...com>
To: Sjoerd Simons <sjoerd@...labora.com>, <jai.luthra@...ux.dev>,
<laurent.pinchart@...asonboard.com>, <mripard@...nel.org>,
Julien Massot
<jmassot@...labora.com>
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>, <jai.luthra@...asonboard.com>,
<changhuang.liang@...rfivetech.com>, <jack.zhu@...rfivetech.com>,
<linux-kernel@...r.kernel.org>, <linux-media@...r.kernel.org>,
<devicetree@...r.kernel.org>, "Liu (EP), Bin"
<b-liu@...com>
Subject: Re: [PATCH v4 11/12] media: ti: j721e-csi2rx: Submit all available
buffers
On 01/07/25 13:39, Sjoerd Simons wrote:
> Hey,
Hi Sjoerd,
>
> On Wed, 2025-05-14 at 16:55 +0530, Rishikesh Donadkar wrote:
>> From: Jai Luthra <j-luthra@...com>
>>
>> We already make sure to submit all available buffers to DMA in each DMA
>> completion callback.
>>
>> Move that logic in a separate function, and use it during stream start
>> as well, as most application queue all their buffers before stream on.
>>
>> Signed-off-by: Jai Luthra <j-luthra@...com>
>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
>> Signed-off-by: Rishikesh Donadkar <r-donadkar@...com>
>> ---
>> .../platform/ti/j721e-csi2rx/j721e-csi2rx.c | 43 +++++++++++--------
>> 1 file changed, 24 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
>> b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
>> index 7986f96c5e11b..ba2a30bfed37d 100644
>> --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
>> +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
>> @@ -651,6 +651,27 @@ static int ti_csi2rx_drain_dma(struct ti_csi2rx_ctx *ctx)
>> return ret;
>> }
>>
>> +static int ti_csi2rx_dma_submit_pending(struct ti_csi2rx_ctx *ctx)
>> +{
>> + struct ti_csi2rx_dma *dma = &ctx->dma;
>> + struct ti_csi2rx_buffer *buf;
>> + int ret = 0;
>> +
>> + /* If there are more buffers to process then start their transfer. */
>> + while (!list_empty(&dma->queue)) {
>> + buf = list_entry(dma->queue.next, struct ti_csi2rx_buffer,
>> list);
>> + ret = ti_csi2rx_start_dma(ctx, buf);
>> + if (ret) {
>> + dev_err(ctx->csi->dev,
>> + "Failed to queue the next buffer for DMA\n");
>> + vb2_buffer_done(&buf->vb.vb2_buf,
>> VB2_BUF_STATE_ERROR);
>> + break;
> The break here seems wrong and does change the previous logic; It means once *a*
> buffer fails to start DMA, you'll no longer try to submit the other (queued)
> buffers. If this was called from the DMA callback of the last submitted buffer
> and userspace doesn't re-queue the error buffer, then capturing will stop, even
> if there were still queued up buffers from a userspace pov.
>
>
> For a potential next iteration you probably also want to wrap in the changes
> from to fix list_del corruption:
> https://lore.kernel.org/all/20250630-j721e-dma-fixup-v1-1-591e378ab3a8@collabora.com/
Thank you for the pointer !
I will make these changes in the next revision
Regards,
Rishikesh
>
>
>
>> + }
>> + list_move_tail(&buf->list, &dma->submitted);
>> + }
>> + return ret;
>> +}
>> +
>> static void ti_csi2rx_dma_callback(void *param)
>> {
>> struct ti_csi2rx_buffer *buf = param;
>> @@ -671,18 +692,7 @@ static void ti_csi2rx_dma_callback(void *param)
>> vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_DONE);
>> list_del(&buf->list);
>>
>> - /* If there are more buffers to process then start their transfer. */
>> - while (!list_empty(&dma->queue)) {
>> - buf = list_entry(dma->queue.next, struct ti_csi2rx_buffer,
>> list);
>> -
>> - if (ti_csi2rx_start_dma(ctx, buf)) {
>> - dev_err(ctx->csi->dev,
>> - "Failed to queue the next buffer for DMA\n");
>> - vb2_buffer_done(&buf->vb.vb2_buf,
>> VB2_BUF_STATE_ERROR);
>> - } else {
>> - list_move_tail(&buf->list, &dma->submitted);
>> - }
>> - }
>> + ti_csi2rx_dma_submit_pending(ctx);
>>
>> if (list_empty(&dma->submitted))
>> dma->state = TI_CSI2RX_DMA_IDLE;
>> @@ -941,7 +951,6 @@ static int ti_csi2rx_start_streaming(struct vb2_queue *vq,
>> unsigned int count)
>> struct ti_csi2rx_ctx *ctx = vb2_get_drv_priv(vq);
>> struct ti_csi2rx_dev *csi = ctx->csi;
>> struct ti_csi2rx_dma *dma = &ctx->dma;
>> - struct ti_csi2rx_buffer *buf;
>> unsigned long flags;
>> int ret = 0;
>>
>> @@ -980,16 +989,13 @@ static int ti_csi2rx_start_streaming(struct vb2_queue
>> *vq, unsigned int count)
>> ctx->sequence = 0;
>>
>> spin_lock_irqsave(&dma->lock, flags);
>> - buf = list_entry(dma->queue.next, struct ti_csi2rx_buffer, list);
>>
>> - ret = ti_csi2rx_start_dma(ctx, buf);
>> + ret = ti_csi2rx_dma_submit_pending(ctx);
>> if (ret) {
>> - dev_err(csi->dev, "Failed to start DMA: %d\n", ret);
>> spin_unlock_irqrestore(&dma->lock, flags);
>> - goto err_pipeline;
>> + goto err_dma;
>> }
>>
>> - list_move_tail(&buf->list, &dma->submitted);
>> dma->state = TI_CSI2RX_DMA_ACTIVE;
>> spin_unlock_irqrestore(&dma->lock, flags);
>>
>> @@ -1004,7 +1010,6 @@ static int ti_csi2rx_start_streaming(struct vb2_queue
>> *vq, unsigned int count)
>>
>> err_dma:
>> ti_csi2rx_stop_dma(ctx);
>> -err_pipeline:
>> video_device_pipeline_stop(&ctx->vdev);
>> writel(0, csi->shim + SHIM_CNTL);
>> writel(0, csi->shim + SHIM_DMACNTX(ctx->idx));
Powered by blists - more mailing lists