Re: [PATCH v7 4/5] media: platform: mediatek: isp: add mediatek ISP3.0 camsv

On Fri, 2024-11-22 at 10:25 +0100, Julien Stephan wrote:

External email : Please do not click links or open attachments until you have verified the sender or the content.


Le ven. 22 nov. 2024 à 09:41, CK Hu (胡俊光) [off-list ref] a écrit :

quoted

Hi, Julien:

On Thu, 2024-11-21 at 09:53 +0100, Julien Stephan wrote:

quoted

External email : Please do not click links or open attachments until you have verified the sender or the content.


From: Phi-bang Nguyen <redacted>

This driver provides a path to bypass the SoC ISP so that image data
coming from the SENINF can go directly into memory without any image
processing. This allows the use of an external ISP.

Signed-off-by: Phi-bang Nguyen <redacted>
Signed-off-by: Florian Sylvestre <redacted>
[Paul Elder fix irq locking]
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Co-developed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Co-developed-by: Julien Stephan <jstephan@baylibre.com>
Signed-off-by: Julien Stephan <jstephan@baylibre.com>
---

[snip]

quoted

+static irqreturn_t isp_irq_camsv30(int irq, void *data)
+{
+       struct mtk_cam_dev *cam_dev = (struct mtk_cam_dev *)data;
+       struct mtk_cam_dev_buffer *buf;
+       unsigned int irq_status;
+
+       spin_lock(&cam_dev->buf_list_lock);
+
+       irq_status = mtk_camsv30_read(cam_dev, CAMSV_INT_STATUS);
+
+       if (irq_status & INT_ST_MASK_CAMSV_ERR)
+               dev_err(cam_dev->dev, "irq error 0x%lx\n",
+                       irq_status & INT_ST_MASK_CAMSV_ERR);
+
+       /* De-queue frame */
+       if (irq_status & CAMSV_IRQ_PASS1_DON) {
+               cam_dev->sequence++;
+
+               buf = list_first_entry_or_null(&cam_dev->buf_list,
+                                              struct mtk_cam_dev_buffer,
+                                              list);
+               if (buf) {
+                       buf->v4l2_buf.sequence = cam_dev->sequence;
+                       buf->v4l2_buf.vb2_buf.timestamp =
+                               ktime_get_ns();
+                       vb2_buffer_done(&buf->v4l2_buf.vb2_buf,
+                                       VB2_BUF_STATE_DONE);
+                       list_del(&buf->list);
+               }
+
+               buf = list_first_entry_or_null(&cam_dev->buf_list,
+                                              struct mtk_cam_dev_buffer,
+                                              list);
+               if (buf)
+                       mtk_camsv30_update_buffers_add(cam_dev, buf);

If buf == NULL, so hardware would automatically stop DMA?
I don't know how this hardware work.
Below is my imagine about this hardware.

1. Software use CAMSV_IMGO_FBC_RCNT_INC to increase software buffer index.
2. Hardware has a hardware buffer index. After hardware finish one frame, hardware buffer index increase.
3. After software buffer index increase, hardware start DMA.
4. When hardware buffer index is equal to software buffer index, hardware automatically stop DMA.

Does the hardware work as my imagine?
If hardware could automatically stop DMA, add comment to describe.
If hardware could not automatically stop DMA, software should do something to stop DMA when buf == NULL.

You are right except that dma is not stopped but frames are
automatically dropped by hardware until a new buffer is enqueued and
software uses CAMSV_IMGO_FBC_RCNT_INC to increase the software buffer
index.

What about adding the following comment:

/*
* If there is no user buffer available, hardware will drop automatically
* frames until buf_queue is called
*/

You say DMA is not stopped. Do you mean hardware still write data into latest buffer which would be dequeued to user space?
I think hardware should not write data into the buffer which has been take away by user space.
I think software should do something to stop DMA. Maybe use mtk_camsv30_cmos_vf_hw_disable() to stop DMA.

Regards,
CK

Let me know if that works for you

Cheers
Julien

quoted

Regards,
CK

quoted

+       }
+
+       spin_unlock(&cam_dev->buf_list_lock);
+
+       return IRQ_HANDLED;
+}
+

************* MEDIATEK Confidentiality Notice ********************
The information contained in this e-mail message (including any
attachments) may be confidential, proprietary, privileged, or otherwise
exempt from disclosure under applicable laws. It is intended to be
conveyed only to the designated recipient(s). Any use, dissemination,
distribution, printing, retaining or copying of this e-mail (including its
attachments) by unintended recipient(s) is strictly prohibited and may
be unlawful. If you are not an intended recipient of this e-mail, or believe
that you have received this e-mail in error, please notify the sender
immediately (by replying to this e-mail), delete any and all copies of
this e-mail (including any attachments) from your system, and do not
disclose the content of this e-mail to any other person. Thank you!