| Message ID | 20241121-add-mtk-isp-3-0-support-v7-0-b04dc9610619@baylibre.com |
|---|---|
| Headers | show |
| Series | Add Mediatek ISP3.0 | expand |
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 void mtk_cam_cmos_vf_enable(struct mtk_cam_dev *cam_dev, > + bool enable, bool pak_en) > +{ > + if (enable) > + cam_dev->hw_functions->mtk_cam_cmos_vf_hw_enable(cam_dev); Directly call mtk_camsv30_cmos_vf_hw_enable(). This has discussed in previous version [1]. [1] https://patchwork.kernel.org/project/linux-mediatek/patch/20240729-add-mtk-isp-3-0-support-v6-4-c374c9e0c672@baylibre.com/#25966327 Regards, CK > + else > + cam_dev->hw_functions->mtk_cam_cmos_vf_hw_disable(cam_dev); > +} > + >
Le ven. 22 nov. 2024 à 08:54, 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 void mtk_cam_cmos_vf_enable(struct mtk_cam_dev *cam_dev, > > + bool enable, bool pak_en) > > +{ > > + if (enable) > > + cam_dev->hw_functions->mtk_cam_cmos_vf_hw_enable(cam_dev); > > Directly call mtk_camsv30_cmos_vf_hw_enable(). > This has discussed in previous version [1]. > > [1] https://patchwork.kernel.org/project/linux-mediatek/patch/20240729-add-mtk-isp-3-0-support-v6-4-c374c9e0c672@baylibre.com/#25966327 > Hi CK, I forgot about that discussion sorry :/ I guess you want me to completely remove the mtk_cam_hw_functions struct? In that case, what do you prefer: - keep mtk_camsv30_hw.c and put signatures in mtkcamsv30_hw.h and include mtk_camsv30_hw.h in mtk_camsv_video.c - rename mtk_camsv30_hw.c to mtk_camsv_hw.c (and all functions) and put signatures in mtk_camsv_hw.h Cheers Julien > Regards, > CK > > > + else > > + cam_dev->hw_functions->mtk_cam_cmos_vf_hw_disable(cam_dev); > > +} > > + > > > > > ************* 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!
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 */ 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!
Hi, Julien: On Fri, 2024-11-22 at 10:16 +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 à 08:54, 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 void mtk_cam_cmos_vf_enable(struct mtk_cam_dev *cam_dev, > > > + bool enable, bool pak_en) > > > +{ > > > + if (enable) > > > + cam_dev->hw_functions->mtk_cam_cmos_vf_hw_enable(cam_dev); > > > > Directly call mtk_camsv30_cmos_vf_hw_enable(). > > This has discussed in previous version [1]. > > > > [1] https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/patch/20240729-add-mtk-isp-3-0-support-v6-4-c374c9e0c672@baylibre.com/*25966327__;Iw!!CTRNKA9wMg0ARbw!lydgLydtAuzr-BC5qArz3AEzOM0iSSr6TXifwab1kPvWVJLy0k7rUiasR_goMAF_6XYmPIpErGF6CdLkPQ$ > > > > Hi CK, > > I forgot about that discussion sorry :/ > I guess you want me to completely remove the mtk_cam_hw_functions struct? > In that case, what do you prefer: > - keep mtk_camsv30_hw.c and put signatures in mtkcamsv30_hw.h and > include mtk_camsv30_hw.h in mtk_camsv_video.c > - rename mtk_camsv30_hw.c to mtk_camsv_hw.c (and all functions) and > put signatures in mtk_camsv_hw.h I prefer the second one. Regards, CK > > Cheers > Julien > > > Regards, > > CK > > > > > + else > > > + cam_dev->hw_functions->mtk_cam_cmos_vf_hw_disable(cam_dev); > > > +} > > > + > > > > > > > ************* 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!
Le ven. 22 nov. 2024 à 10:19, 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 void mtk_cam_vb2_buf_queue(struct vb2_buffer *vb) > > +{ > > + struct mtk_cam_dev *cam = vb2_get_drv_priv(vb->vb2_queue); > > + struct mtk_cam_dev_buffer *buf = to_mtk_cam_dev_buffer(vb); > > + unsigned long flags; > > + > > + /* Add the buffer into the tracking list */ > > + spin_lock_irqsave(&cam->buf_list_lock, flags); > > + if (list_empty(&cam->buf_list)) > > + (*cam->hw_functions->mtk_cam_update_buffers_add)(cam, buf); > > + > > + list_add_tail(&buf->list, &cam->buf_list); > > + (*cam->hw_functions->mtk_cam_fbc_inc)(cam); > > I think fbc_inc should together with update_buffers_add. > After update_buffers_add then fbc_inc. > So squash fbc_inc into update_buffers_add and drop fbc_inc function. > No, this is not true. mtk_cam_update_buffers_add is used to indicate which buffer should be used for dma write. This is the first entry in the buf list. mtk_cam_fbc_inc is used to increase the number of available user space buffers. If the buffer list is not empty and user space calls buf_queue again, we need to call mtk_cam_fbc_inc to increase the number of available user buffers, but we don't want to change the buffer for DMA write. mtk_camsv30_update_buffers_add is called on irq to update the address to the next buffer (if available). Maybe the name mtk_camsv30_update_buffers_add is confusing then? What do you think about: - mtk_camsv30_update_buffers_add -> mtk_camsv30_update_buffers_address - mtk_cam_fbc_inc -> mtk_camsv30_buffer_add Cheers Julien > Regards, > CK > > > + spin_unlock_irqrestore(&cam->buf_list_lock, flags); > > +} > > + > > > > > ************* 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!
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. 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!
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. > 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!
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! > >
Le ven. 22 nov. 2024 à 11:01, CK Hu (胡俊光) <ck.hu@mediatek.com> a écrit : > > 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. > OK, sorry for confusion! Did you notice my previous reply about fbc_inc? Cheers Julien > 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! > > > > > > ************* 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!
On Fri, Nov 22, 2024 at 09:28:53AM +0000, CK Hu (胡俊光) wrote: > On Fri, 2024-11-22 at 10:16 +0100, Julien Stephan wrote: > > Le ven. 22 nov. 2024 à 08:54, CK Hu (胡俊光) a écrit : > > > 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 void mtk_cam_cmos_vf_enable(struct mtk_cam_dev *cam_dev, > > > > + bool enable, bool pak_en) > > > > +{ > > > > + if (enable) > > > > + cam_dev->hw_functions->mtk_cam_cmos_vf_hw_enable(cam_dev); > > > > > > Directly call mtk_camsv30_cmos_vf_hw_enable(). > > > This has discussed in previous version [1]. > > > > > > [1] https://patchwork.kernel.org/project/linux-mediatek/patch/20240729-add-mtk-isp-3-0-support-v6-4-c374c9e0c672@baylibre.com/ > > > > Hi CK, > > > > I forgot about that discussion sorry :/ > > I guess you want me to completely remove the mtk_cam_hw_functions struct? > > In that case, what do you prefer: > > - keep mtk_camsv30_hw.c and put signatures in mtkcamsv30_hw.h and > > include mtk_camsv30_hw.h in mtk_camsv_video.c > > - rename mtk_camsv30_hw.c to mtk_camsv_hw.c (and all functions) and > > put signatures in mtk_camsv_hw.h > > I prefer the second one. If we drop the indirection (which I think is a good idea until we get a second hardware generation supported by the same driver) I would also go for the latter. > > > > + else > > > > + cam_dev->hw_functions->mtk_cam_cmos_vf_hw_disable(cam_dev); > > > > +} > > > > + > > > >
On Fri, 2024-11-22 at 10:44 +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:19, 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 void mtk_cam_vb2_buf_queue(struct vb2_buffer *vb) > > > +{ > > > + struct mtk_cam_dev *cam = vb2_get_drv_priv(vb->vb2_queue); > > > + struct mtk_cam_dev_buffer *buf = to_mtk_cam_dev_buffer(vb); > > > + unsigned long flags; > > > + > > > + /* Add the buffer into the tracking list */ > > > + spin_lock_irqsave(&cam->buf_list_lock, flags); > > > + if (list_empty(&cam->buf_list)) > > > + (*cam->hw_functions->mtk_cam_update_buffers_add)(cam, buf); > > > + > > > + list_add_tail(&buf->list, &cam->buf_list); > > > + (*cam->hw_functions->mtk_cam_fbc_inc)(cam); > > > > I think fbc_inc should together with update_buffers_add. > > After update_buffers_add then fbc_inc. > > So squash fbc_inc into update_buffers_add and drop fbc_inc function. > > > > No, this is not true. > mtk_cam_update_buffers_add is used to indicate which buffer should be > used for dma write. This is the first entry in the buf list. > mtk_cam_fbc_inc is used to increase the number of available user space buffers. > > If the buffer list is not empty and user space calls buf_queue again, > we need to call mtk_cam_fbc_inc to increase the number of available > user buffers, but we don't want to change the buffer for DMA write. > > mtk_camsv30_update_buffers_add is called on irq to update the address > to the next buffer (if available). I think current design is OK. I just think it could have more flexible design. For example, reduce frame rate from 30 fps to 15 fps. And this is a time sequence to reduce frame rate: t = -5 ms, set buf1 address and fbc increase. (hardware index = 0, software index = 1) t = 0 ms, t = 33 ms, buf1 is done, do not set next buffer. (hardware index = 1, software index = 1) t = 67 ms, set buf2 addres and fbc increase. (hardware index = 1, software index = 2) t = 100 ms, buf2 is done, (hardware index = 2, software idex = 2) If want to keep the 30 fps, when t = 33ms, set buf2 address and fbc increase. If you want to keep current design, that's OK for me because now has no requirement for such advanced control. But I have a new question, in the time sequence I list, does it has interrupt when t = 0 (the first vblank) ? If t = 0 has no interrupt, I think t = 67 also has no interrupt and my design would not work. It t = 0 has interrupt, I think software should think this buffer is not done. > > Maybe the name mtk_camsv30_update_buffers_add is confusing then? > What do you think about: > - mtk_camsv30_update_buffers_add -> mtk_camsv30_update_buffers_address mtk_camsv30_update_buffers_address is better. > - mtk_cam_fbc_inc -> mtk_camsv30_buffer_add mtk_cam_fbc_inc is better, it show counter increase. Regards, CK > > Cheers > Julien > > > Regards, > > CK > > > > > + spin_unlock_irqrestore(&cam->buf_list_lock, flags); > > > +} > > > + > > > > > > > ************* 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!
On Fri, 2024-11-22 at 09:28 +0000, CK Hu (胡俊光) wrote: > Hi, Julien: > > On Fri, 2024-11-22 at 10:16 +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 à 08:54, 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 void mtk_cam_cmos_vf_enable(struct mtk_cam_dev *cam_dev, > > > > + bool enable, bool pak_en) > > > > +{ > > > > + if (enable) > > > > + cam_dev->hw_functions->mtk_cam_cmos_vf_hw_enable(cam_dev); > > > > > > Directly call mtk_camsv30_cmos_vf_hw_enable(). > > > This has discussed in previous version [1]. > > > > > > [1] https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/patch/20240729-add-mtk-isp-3-0-support-v6-4-c374c9e0c672@baylibre.com/*25966327__;Iw!!CTRNKA9wMg0ARbw!lydgLydtAuzr-BC5qArz3AEzOM0iSSr6TXifwab1kPvWVJLy0k7rUiasR_goMAF_6XYmPIpErGF6CdLkPQ$ > > > > > > > Hi CK, > > > > I forgot about that discussion sorry :/ > > I guess you want me to completely remove the mtk_cam_hw_functions struct? > > In that case, what do you prefer: > > - keep mtk_camsv30_hw.c and put signatures in mtkcamsv30_hw.h and > > include mtk_camsv30_hw.h in mtk_camsv_video.c > > - rename mtk_camsv30_hw.c to mtk_camsv_hw.c (and all functions) and > > put signatures in mtk_camsv_hw.h > > I prefer the second one. In addition, I think mtk_cam_cmos_vf_enable() could be removed. Caller of this function could directly call mtk_camsv30_cmos_vf_hw_enable() and mtk_camsv30_cmos_vf_hw_disable(). Regards, CK > > Regards, > CK > > > > > Cheers > > Julien > > > > > Regards, > > > CK > > > > > > > + else > > > > + cam_dev->hw_functions->mtk_cam_cmos_vf_hw_disable(cam_dev); > > > > +} > > > > + > > > > > > > > > > ************* 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!
This series adds the support of the Mediatek ISP3.0 found on some Mediatek SoCs such as the mt8365. The driver is divided into 2 parts: * SENINF: the sensor interface * CAMSV: 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 or camera sensor directly. The SENINF driver is based on previous work done by Louis Kuo available as an RFC here: https://lore.kernel.org/all/20200708104023.3225-1-louis.kuo@mediatek.com/ Changes in v7: - fix several comments from Laurent Pinchart and CK about style issues, such as: sort Kconfig and Makefile alphabetically, remove unneeded headers, use 80 char limits ... - add back I/O accessors around readl/writel - use enable_streams/disable_streams instead of s_stream - use v4l2_subdev_init_finalize and don't store active format - remove mtk_camsv30_regs.h file to merge it inside mtk_camsv30_hw.c - adding reviewed-by tag from robh and angelo - implement .has_pad_interdep callback to fix multistream error - fix mtk_seninf_get_clk_divider to give the correct pad number. This caused an issue for multi camera - use hardware FBC (framce buffer control) instead of dummy buffer to deal with underrruns - simplify directory architecture and remove isp_30, camsv and seninf directories - Link to v6: https://lore.kernel.org/r/20240729-add-mtk-isp-3-0-support-v6-0-c374c9e0c672@baylibre.com Changes in v6: - remove unneeded "link" tag from commits bindings: - remove labels from example node - remove complexity for phy and phy-name properties driver: - fix some comments from CK : - remove unneeded variables - rename irqlock to buf_list_lock for clarity - remove unneeded lock/unlock around hw_enable/hw_disable - Link to v5: https://lore.kernel.org/r/20240704-add-mtk-isp-3-0-support-v5-0-bfccccc5ec21@baylibre.com Changes on v5: drivers: - rebase on 6.10-rc1 - fix various comments from all reviews (mostly style issues and minor code refactor) - add a function to calculate the clock divider for the master sensor clock: NOTE: setting this register seems to have no effect at all, currently checking with mediatek apps engineer (OOO until 17/04) bindings: - camsv: update description - seninf: fix phy definition and example indentation - use generic name for node example dts: - sort nodes by addresses - use lower case for hexadecimal Changes in v4: - fix suspend/resume deadlock - fix various locking issues reported by Laurent Pinchart on v3 - run LOCKDEP - add missing include reported by kernel-test-robot for non mediatek arch and COMPILE_TEST=y - use atomic poll inside mtk_camsv30_setup - drop second lane support as it was not used - remove useless members in structs - fix media entity initialization - initialize correct pad for camsv video device - add isp support in mt8365.dtsi - rebase on 6.7 Changes in v3: - fix a lot of formatting issues/coding style issues found in camsv/seninf reported by Angelo on v2 - fix camsv/seninf binding file error reported by Rob Changes in v2: - renamed clock `cam_seninf` to `camsys` - renamed clock `top_mux_seninf` to `top_mux` - moved phy properties from port nodes to top level - remove patternProperties - specify power management dependency in the cover letter description to fix missing include in dt-binding example - change '$ref' properties on some endpoint nodes from '$ref: video-interfaces.yaml#' to '$ref: /schemas/graph.yaml#/$defs/endpoint-base' where applicable Best Julien Stephan Signed-off-by: Julien Stephan <jstephan@baylibre.com> --- Julien Stephan (1): arm64: dts: mediatek: mt8365: Add support for camera Louis Kuo (2): dt-bindings: media: add mediatek ISP3.0 sensor interface media: platform: mediatek: isp: add mediatek ISP3.0 sensor interface Phi-bang Nguyen (2): dt-bindings: media: add mediatek ISP3.0 camsv media: platform: mediatek: isp: add mediatek ISP3.0 camsv .../bindings/media/mediatek,mt8365-camsv.yaml | 109 ++ .../bindings/media/mediatek,mt8365-seninf.yaml | 259 ++++ MAINTAINERS | 9 + arch/arm64/boot/dts/mediatek/mt8365.dtsi | 125 ++ drivers/media/platform/mediatek/Kconfig | 1 + drivers/media/platform/mediatek/Makefile | 1 + drivers/media/platform/mediatek/isp/Kconfig | 35 + drivers/media/platform/mediatek/isp/Makefile | 9 + drivers/media/platform/mediatek/isp/mtk_camsv.c | 275 ++++ drivers/media/platform/mediatek/isp/mtk_camsv.h | 170 ++ .../media/platform/mediatek/isp/mtk_camsv30_hw.c | 539 +++++++ .../media/platform/mediatek/isp/mtk_camsv_video.c | 701 +++++++++ drivers/media/platform/mediatek/isp/mtk_seninf.c | 1636 ++++++++++++++++++++ .../media/platform/mediatek/isp/mtk_seninf_reg.h | 114 ++ 14 files changed, 3983 insertions(+) --- base-commit: 00873e6fe91b77e9bbc82012fe6103080066fbfc change-id: 20240704-add-mtk-isp-3-0-support-a08a978cac36 Best regards,