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

[Julien Stephan]

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 :/).
Moreover, some other driver already uses the dummy buffer
implementation, if I am not wrong.

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!