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

[Laurent Pinchart]

Hi Julien,

Another comment.

On Mon, Aug 07, 2023 at 11:48:13AM +0200, Julien Stephan wrote:
> 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>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Signed-off-by: Julien Stephan <jstephan@xxxxxxxxxxxx>
> ---
>  MAINTAINERS                                   |   1 +
>  .../platform/mediatek/isp/isp_30/Kconfig      |  19 +
>  .../platform/mediatek/isp/isp_30/Makefile     |   1 +
>  .../mediatek/isp/isp_30/camsv/Makefile        |   7 +
>  .../mediatek/isp/isp_30/camsv/mtk_camsv.c     | 328 ++++++++
>  .../mediatek/isp/isp_30/camsv/mtk_camsv.h     | 196 +++++
>  .../isp/isp_30/camsv/mtk_camsv30_hw.c         | 432 ++++++++++
>  .../isp/isp_30/camsv/mtk_camsv30_regs.h       |  60 ++
>  .../isp/isp_30/camsv/mtk_camsv_video.c        | 781 ++++++++++++++++++
>  9 files changed, 1825 insertions(+)
>  create mode 100644 drivers/media/platform/mediatek/isp/isp_30/camsv/Makefile
>  create mode 100644 drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv.c
>  create mode 100644 drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv.h
>  create mode 100644 drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv30_hw.c
>  create mode 100644 drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv30_regs.h
>  create mode 100644 drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv_video.c

[snip]

> diff --git a/drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv.h b/drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv.h
> new file mode 100644
> index 000000000000..902f2a391064
> --- /dev/null
> +++ b/drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv.h
> @@ -0,0 +1,196 @@

[snip]

> +struct mtk_cam_dev_buffer {
> +	struct vb2_v4l2_buffer v4l2_buf;
> +	struct list_head list;
> +	dma_addr_t daddr;
> +	dma_addr_t fhaddr;
> +};

fhaddr is a dma_addr_t.

[snip]

> diff --git a/drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv_video.c b/drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv_video.c
> new file mode 100644
> index 000000000000..f879726eacd8
> --- /dev/null
> +++ b/drivers/media/platform/mediatek/isp/isp_30/camsv/mtk_camsv_video.c
> @@ -0,0 +1,781 @@

[snip]

> +static int mtk_cam_vb2_buf_prepare(struct vb2_buffer *vb)
> +{
> +	struct mtk_cam_video_device *vdev =
> +		vb2_queue_to_mtk_cam_video_device(vb->vb2_queue);
> +	struct mtk_cam_dev *cam = vb2_get_drv_priv(vb->vb2_queue);
> +	struct mtk_cam_dev_buffer *buf = to_mtk_cam_dev_buffer(vb);
> +	const struct v4l2_pix_format_mplane *fmt = &vdev->format;
> +	u32 size;
> +	int i;
> +
> +	for (i = 0; i < vb->num_planes; i++) {
> +		size = fmt->plane_fmt[i].sizeimage;
> +		if (vb2_plane_size(vb, i) < size) {
> +			dev_err(cam->dev, "plane size is too small:%lu<%u\n",
> +				vb2_plane_size(vb, i), size);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	buf->v4l2_buf.field = V4L2_FIELD_NONE;
> +
> +	for (i = 0; i < vb->num_planes; i++) {
> +		size = fmt->plane_fmt[i].sizeimage;
> +		vb2_set_plane_payload(vb, i, size);
> +	}
> +
> +	if (buf->daddr == 0ULL) {
> +		buf->daddr = vb2_dma_contig_plane_dma_addr(vb, 0);
> +		if (cam->conf->frm_hdr_en)
> +			buf->fhaddr = vb2_dma_contig_plane_dma_addr(vb, 1);

Here you store the address of the second plane in fhaddr. This is
correct, but that address is then never used anywhere.

> +	}
> +
> +	return 0;
> +}

[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 = 0;
> +
> +	if (pm_runtime_get_sync(dev) < 0) {
> +		dev_err(dev, "failed to get pm_runtime\n");
> +		pm_runtime_put_autosuspend(dev);
> +		return -1;
> +	}
> +
> +	/* Enable CMOS and VF */
> +	mtk_cam_cmos_vf_enable(cam, true, vdev->fmtinfo->packed);
> +
> +	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.fhaddr = (dma_addr_t)dma_alloc_coherent(cam->dev,
> +					       cam->dummy_size,
> +					       &cam->dummy.daddr, GFP_KERNEL);

And here, for the dummy buffer only, you use fhaddr to store a CPU
address. The cast to dma_addr_t is here only to silence the compiler
telling you you've made a mistake.

I recommend dropping the handling of the second plane (which should then
prompt you to review the usage of the frm_hdr_en flag), turning fhaddr
into a void *, and renaming to vaddr. You should also document the
mtk_cam_dev_buffer structure with kerneldoc to explain what the
different fields are about. It could be useful to document other
structures too.

> +	if (!cam->dummy.fhaddr) {
> +		dev_err(cam->dev, "can't allocate dummy buffer\n");
> +		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;
> +}

[snip]

-- 
Regards,

Laurent Pinchart