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

["CK Hu (胡俊光)" <ck.hu@mediatek.com>]

On Fri, 2024-11-22 at 10:50 +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 à 10:49, CK Hu (胡俊光) <ck.hu@mediatek.com> a écrit :
> > 
> > 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 (胡俊光) <ck.hu@mediatek.com> a écrit :
> > > > 
> > > > Hi, Julien:
> > > > 
> > > > On Thu, 2024-11-21 at 09:53 +0100, Julien Stephan wrote:
> > > > > External email : Please do not click links or open attachments until you have verified the sender or the content.
> > > > > 
> > > > > 
> > > > > From: Phi-bang Nguyen <pnguyen@baylibre.com>
> > > > > 
> > > > > 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 <pnguyen@baylibre.com>
> > > > > Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
> > > > > [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]
> > > > 
> > > > > +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.
> > 
> 
> No, I said frame is dropped.. It does not write any data.

OK, for me, DMA mean memory access. Not writing any data is equal to stop DMA for me.
The comment is OK for me now. But it may change after we discuss fbc_inc.

Regards,
CK

> 
> > Regards,
> > CK
> > 
> > > 
> > > Let me know if that works for you
> > > 
> > > Cheers
> > > Julien
> > > 
> > > > Regards,
> > > > CK
> > > > 
> > > > > +       }
> > > > > +
> > > > > +       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!
> > 
> > ************* 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!
> 
>