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

[CK Hu (胡俊光)]

On Mon, 2024-07-29 at 15:40 +0200, Julien Stephan wrote:
>  	 
> External email : Please do not click links or open attachments until you have verified the sender or the content.
>  Le jeu. 18 juil. 2024 à 04:54, CK Hu (胡俊光) <ck.hu@xxxxxxxxxxxx> a écrit :
> >
> > Hi, Julien:
> >
> > On Thu, 2024-07-04 at 15:36 +0200, 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@xxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxx>
> > > Signed-off-by: Florian Sylvestre <fsylvestre@xxxxxxxxxxxx>
> > > [Paul Elder fix irq locking]
> > > Signed-off-by: Paul Elder <paul.elder@xxxxxxxxxxxxxxxx>
> > > Co-developed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > > Co-developed-by: Julien Stephan <jstephan@xxxxxxxxxxxx>
> > > Signed-off-by: Julien Stephan <jstephan@xxxxxxxxxxxx>
> > > ---
> >
> > [snip]
> >
> > > +static int mtk_cam_vb2_start_streaming(struct vb2_queue *vq,
> > > +       unsigned int count)
> > > +{
> > > +struct mtk_cam_dev *cam = vb2_get_drv_priv(vq);
> > > +struct mtk_cam_dev_buffer *buf;
> > > +struct mtk_cam_video_device *vdev =
> > > +vb2_queue_to_mtk_cam_video_device(vq);
> > > +struct device *dev = cam->dev;
> > > +const struct v4l2_pix_format_mplane *fmt = &vdev->format;
> > > +int ret;
> > > +unsigned long flags;
> > > +
> > > +if (pm_runtime_get_sync(dev) < 0) {
> > > +dev_err(dev, "failed to get pm_runtime\n");
> > > +pm_runtime_put_autosuspend(dev);
> > > +return -1;
> > > +}
> > > +
> > > +(*cam->hw_functions->mtk_cam_setup)(cam, fmt->width, fmt->height,
> > > +fmt->plane_fmt[0].bytesperline, vdev->fmtinfo->code);
> > > +
> > > +
> > > +/* Enable CMOS and VF */
> > > +mtk_cam_cmos_vf_enable(cam, true, true);
> > > +
> > > +mutex_lock(&cam->op_lock);
> > > +
> > > +ret = mtk_cam_verify_format(cam);
> > > +if (ret < 0)
> > > +goto fail_unlock;
> > > +
> > > +/* Start streaming of the whole pipeline now*/
> > > +if (!cam->pipeline.start_count) {
> > > +ret = media_pipeline_start(vdev->vdev.entity.pads,
> > > +   &cam->pipeline);
> > > +if (ret) {
> > > +dev_err(dev, "failed to start pipeline:%d\n", ret);
> > > +goto fail_unlock;
> > > +}
> > > +}
> > > +
> > > +/* Media links are fixed after media_pipeline_start */
> > > +cam->stream_count++;
> > > +
> > > +cam->sequence = (unsigned int)-1;
> > > +
> > > +/* Stream on the sub-device */
> > > +ret = v4l2_subdev_call(&cam->subdev, video, s_stream, 1);
> > > +if (ret)
> > > +goto fail_no_stream;
> > > +
> > > +mutex_unlock(&cam->op_lock);
> > > +
> > > +/* Create dummy buffer */
> > > +cam->dummy_size = fmt->plane_fmt[0].sizeimage;
> > > +
> > > +cam->dummy.vaddr = dma_alloc_coherent(cam->dev, cam->dummy_size,
> > > +      &cam->dummy.daddr, GFP_KERNEL);
> >
> > Dummy buffer cost much in DRAM footprint. I think we can get rid of
> > this dummy buffer. If no buffer is queued from user space, call
> > mtk_camsv30_cmos_vf_hw_disable() to stop write data into DRAM. After
> > buffer is queued from user space, call mtk_camsv30_cmos_vf_hw_enable()
> > to start write data into DRAM.
> >
> 
> Hi CK,
> 
> IMHO it does not cost that much. A long time ago, we tried to remove
> it, but we faced an issue (can't remember what :/).

5376 * 4032 * 2 = 43352064 (bytes) is a large size for me.

If this is a software issue, fix it.
If this is a hardware limitation, add comment to describe how hardware work so need this dummy buffer.
If next SoC has no this hardware limitation, we could drop dummy buffer in next SoC.

> Moreover, some other driver already uses the dummy buffer
> implementation, if I am not wrong.

I think every driver has different reason to use dummy buffer.
So we could not directly infer that this driver need dummy buffer.

Regards,
CK

> 
> Cheers
> Julien
> 
> > Regards,
> > CK
> >
> > > +if (!cam->dummy.vaddr) {
> > > +ret = -ENOMEM;
> > > +goto fail_no_buffer;
> > > +}
> > > +
> > > +/* update first buffer address */
> > > +
> > > +/* added the buffer into the tracking list */
> > > +spin_lock_irqsave(&cam->irqlock, flags);
> > > +if (list_empty(&cam->buf_list)) {
> > > +(*cam->hw_functions->mtk_cam_update_buffers_add)(cam, &cam->dummy);
> > > +cam->is_dummy_used = true;
> > > +} else {
> > > +buf = list_first_entry_or_null(&cam->buf_list,
> > > +       struct mtk_cam_dev_buffer,
> > > +       list);
> > > +(*cam->hw_functions->mtk_cam_update_buffers_add)(cam, buf);
> > > +cam->is_dummy_used = false;
> > > +}
> > > +spin_unlock_irqrestore(&cam->irqlock, flags);
> > > +
> > > +return 0;
> > > +
> > > +fail_no_buffer:
> > > +mutex_lock(&cam->op_lock);
> > > +v4l2_subdev_call(&cam->subdev, video, s_stream, 0);
> > > +fail_no_stream:
> > > +cam->stream_count--;
> > > +if (cam->stream_count == 0)
> > > +media_pipeline_stop(vdev->vdev.entity.pads);
> > > +fail_unlock:
> > > +mutex_unlock(&cam->op_lock);
> > > +mtk_cam_vb2_return_all_buffers(cam, VB2_BUF_STATE_QUEUED);
> > > +
> > > +return ret;
> > > +}
> > > +
> >
> > ************* 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!
>