Hi Julien,
On Jun 22, 2023 at 11:18:28 +0200, Julien Massot wrote:
> Hi Vaishnav,
>
> On 14/03/2023 13:55, Vaishnav Achath wrote:
> > Hi,
> >
> > This series adds support for CSI2 capture on J721E. It includes some
> > fixes to the Cadence CSI2RX driver, and adds the TI CSI2RX wrapper
> > driver.
> We are testing this patch series and experienced some strange behaviour,
> with the same sequence of 5-10 frames repeated over and over.
> (Almost the same sequence since frames have different md5sum)
>
> To solve this issue we had to forward port some functions from the TI BSP
> Kernel[1] such as ti_csi2rx_restart_dma, and ti_csi2rx_drain_dma.
>
> Can you consider this issue for the next patchset version?
While we haven't seen this particular issue (of repeating frames) due to
a lack of draining, I agree that it is a useful fix.
Will include it in v8 of this series along with some other features and
fixes around stream stop sequence.
Thanks,
>
> Thank you,
> Julien
>
>
> Here are the modifications we made for information only.
>
> ---
> .../platform/ti/j721e-csi2rx/j721e-csi2rx.c | 138 +++++++++++++++---
> 1 file changed, 117 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> index 1af7b0b09cfc..e8579dbf07b4 100644
> --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
> @@ -46,6 +46,8 @@
> #define MAX_WIDTH_BYTES SZ_16K
> #define MAX_HEIGHT_LINES SZ_16K
>
> +#define DRAIN_TIMEOUT_MS 50
> +
> struct ti_csi2rx_fmt {
> u32 fourcc; /* Four character code. */
> u32 code; /* Mbus code. */
> @@ -498,6 +500,59 @@ static void ti_csi2rx_setup_shim(struct ti_csi2rx_dev
> *csi)
> writel(reg, csi->shim + SHIM_PSI_CFG0);
> }
>
> +static void ti_csi2rx_drain_callback(void *param)
> +{
> + struct completion *drain_complete = param;
> +
> + complete(drain_complete);
> +}
> +
> +static int ti_csi2rx_drain_dma(struct ti_csi2rx_dev *csi)
> +{
> + struct dma_async_tx_descriptor *desc;
> + struct device *dev = csi->dma.chan->device->dev;
> + struct completion drain_complete;
> + void *buf;
> + size_t len = csi->v_fmt.fmt.pix.sizeimage;
> + dma_addr_t addr;
> + dma_cookie_t cookie;
> + int ret;
> +
> + init_completion(&drain_complete);
> +
> + buf = dma_alloc_coherent(dev, len, &addr, GFP_KERNEL | GFP_ATOMIC);
> + if (!buf)
> + return -ENOMEM;
> +
> + desc = dmaengine_prep_slave_single(csi->dma.chan, addr, len,
> + DMA_DEV_TO_MEM,
> + DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> + if (!desc) {
> + ret = -EIO;
> + goto out;
> + }
> +
> + desc->callback = ti_csi2rx_drain_callback;
> + desc->callback_param = &drain_complete;
> +
> + cookie = dmaengine_submit(desc);
> + ret = dma_submit_error(cookie);
> + if (ret)
> + goto out;
> +
> + dma_async_issue_pending(csi->dma.chan);
> +
> + if (!wait_for_completion_timeout(&drain_complete,
> + msecs_to_jiffies(DRAIN_TIMEOUT_MS))) {
> + dmaengine_terminate_sync(csi->dma.chan);
> + ret = -ETIMEDOUT;
> + goto out;
> + }
> +out:
> + dma_free_coherent(dev, len, buf, addr);
> + return ret;
> +}
> +
> static void ti_csi2rx_dma_callback(void *param)
> {
> struct ti_csi2rx_buffer *buf = param;
> @@ -564,24 +619,61 @@ static int ti_csi2rx_start_dma(struct ti_csi2rx_dev
> *csi,
> }
>
> static void ti_csi2rx_cleanup_buffers(struct ti_csi2rx_dev *csi,
> - enum vb2_buffer_state state)
> + enum vb2_buffer_state buf_state)
> {
> struct ti_csi2rx_dma *dma = &csi->dma;
> - struct ti_csi2rx_buffer *buf = NULL, *tmp;
> + struct ti_csi2rx_buffer *buf = NULL, *tmp, *curr;;
> + enum ti_csi2rx_dma_state state;
> unsigned long flags;
> + int ret;
>
> spin_lock_irqsave(&dma->lock, flags);
> list_for_each_entry_safe(buf, tmp, &csi->dma.queue, list) {
> list_del(&buf->list);
> - vb2_buffer_done(&buf->vb.vb2_buf, state);
> + vb2_buffer_done(&buf->vb.vb2_buf, buf_state);
> }
>
> - if (dma->curr)
> - vb2_buffer_done(&dma->curr->vb.vb2_buf, state);
> -
> + curr = csi->dma.curr;
> + state = csi->dma.state;
> dma->curr = NULL;
> dma->state = TI_CSI2RX_DMA_STOPPED;
> spin_unlock_irqrestore(&dma->lock, flags);
> +
> + if (state != TI_CSI2RX_DMA_STOPPED) {
> + ret = ti_csi2rx_drain_dma(csi);
> + if (ret)
> + dev_dbg(csi->dev,
> + "Failed to drain DMA. Next frame might be bogus\n");
> + dmaengine_terminate_sync(csi->dma.chan);
> + }
> +
> + if (curr)
> + vb2_buffer_done(&curr->vb.vb2_buf, buf_state);
> +}
> +
> +static int ti_csi2rx_restart_dma(struct ti_csi2rx_dev *csi,
> + struct ti_csi2rx_buffer *buf)
> +{
> + struct ti_csi2rx_dma *dma = &csi->dma;
> + unsigned long flags = 0;
> + int ret = 0;
> +
> + ret = ti_csi2rx_drain_dma(csi);
> + if (ret)
> + dev_warn(csi->dev,
> + "Failed to drain DMA. Next frame might be bogus\n");
> +
> + ret = ti_csi2rx_start_dma(csi, buf);
> + if (ret) {
> + dev_err(csi->dev, "Failed to start DMA: %d\n", ret);
> + spin_lock_irqsave(&dma->lock, flags);
> + vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> + dma->curr = NULL;
> + dma->state = TI_CSI2RX_DMA_IDLE;
> + spin_unlock_irqrestore(&dma->lock, flags);
> + }
> +
> + return ret;
> }
>
> static int ti_csi2rx_queue_setup(struct vb2_queue *q, unsigned int
> *nbuffers,
> @@ -622,6 +714,7 @@ static void ti_csi2rx_buffer_queue(struct vb2_buffer
> *vb)
> struct ti_csi2rx_dev *csi = vb2_get_drv_priv(vb->vb2_queue);
> struct ti_csi2rx_buffer *buf;
> struct ti_csi2rx_dma *dma = &csi->dma;
> + bool restart_dma = false;
> unsigned long flags = 0;
> int ret;
>
> @@ -634,21 +727,30 @@ static void ti_csi2rx_buffer_queue(struct vb2_buffer
> *vb)
> * But if DMA has stalled due to lack of buffers, restart it now.
> */
> if (dma->state == TI_CSI2RX_DMA_IDLE) {
> - ret = ti_csi2rx_start_dma(csi, buf);
> - if (ret) {
> - dev_err(csi->dev, "Failed to start DMA: %d\n", ret);
> - vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_QUEUED);
> - goto unlock;
> - }
> -
> + /*
> + * Do not restart DMA with the lock held because
> + * ti_csi2rx_drain_dma() might block when allocating a buffer.
> + * There won't be a race on queueing DMA anyway since the
> + * callback is not being fired.
> + */
> + restart_dma = true;
> dma->curr = buf;
> dma->state = TI_CSI2RX_DMA_ACTIVE;
> } else {
> list_add_tail(&buf->list, &dma->queue);
> }
> -
> -unlock:
> 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
> + * the application and will only confuse it. Issue a DMA
> + * transaction to drain that up.
> + */
> + ti_csi2rx_restart_dma(csi, buf);
> + }
> }
>
> static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int
> count)
> @@ -718,12 +820,6 @@ static void ti_csi2rx_stop_streaming(struct vb2_queue
> *vq)
>
> writel(0, csi->shim + SHIM_CNTL);
>
> - ret = dmaengine_terminate_sync(csi->dma.chan);
> - if (ret)
> - dev_err(csi->dev, "Failed to stop DMA: %d\n", ret);
> -
> - writel(0, csi->shim + SHIM_DMACNTX);
> -
> ti_csi2rx_cleanup_buffers(csi, VB2_BUF_STATE_ERROR);
> }
>
> --
> 2.41.0
>
>
> [1] TI BSP kernel : https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/tree/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c?h=ti-linux-6.1.y-cicd
>
> --
> Julien Massot
> Senior Software Engineer
> Collabora Ltd.
> Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
> Registered in England & Wales, no. 5513718
--
Jai
GPG Fingerprint: 4DE0 D818 E5D5 75E8 D45A AFC5 43DE 91F9 249A 7145
Attachment:
signature.asc
Description: PGP signature
