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

Hi Julien,

Another comment.

On Mon, Aug 07, 2023 at 11:48:13AM +0200, Julien Stephan wrote:

From: Phi-bang Nguyen <redacted>

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 <redacted>
Signed-off-by: Florian Sylvestre <redacted>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Julien Stephan <jstephan@baylibre.com>
---
 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]

[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]

[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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel