Re: [PATCH v7 6/6] media: starfive: camss: Add VIN driver

Hi Jack,

On Fri, Aug 04, 2023 at 07:14:22PM +0800, Jack Zhu wrote:

On 2023/8/4 6:18, Laurent Pinchart wrote:

quoted

On Thu, Aug 03, 2023 at 10:44:50AM +0800, Jack Zhu wrote:

quoted

On 2023/8/2 18:38, Laurent Pinchart wrote:

quoted

On Wed, Aug 02, 2023 at 05:58:26PM +0800, Jack Zhu wrote:

quoted

On 2023/7/28 4:49, Laurent Pinchart wrote:

quoted

On Mon, Jun 19, 2023 at 07:28:38PM +0800, Jack Zhu wrote:

quoted

Add Video In Controller driver for StarFive Camera Subsystem.

I haven't reviewed this patch in details, as I have a high-level
question: why do you need VIN subdevs ? They don't seem to represent any
particular piece of hardware, their input and output formats are always
identical, and they're not used to configure registers. The contents of
this patch seems to belong to the video device, I think you can drop the
VIN subdevs.

The VIN module corresponds to a hardware module, which is mainly responsible
for data routing and partial interrupt management (when the image data does
not pass through the isp but directly goes to the ddr), the relevant registers
need to be configured.

That's fine, but I don't think you need a subdev for it. As far as I
understand, the VIn modules are (more or less) DMA engines. You can just
model them as video devices, connected directly to the CSI-2 RX and ISP
source pads.

The VIN hardware can also route input data, it can decide whether DVP sensor
or MIPI sensor is used as input data.

quoted

Does the "vin0_wr" have the ability to capture raw data from the DVP
interface as well, or only from the CSI-2 RX ?

Yes, the "vin0_wr" has the ability to capture raw data from the DVP
interface.

Then I would recommend something similar to the following media graph:

digraph board {
        rankdir=TB
        imx219 [label="{{} | imx219 6-0010\n/dev/v4l-subdev0 | {<port0> 0}}", shape=Mrecord, style=filled, fillcolor=green]
        imx219:port0 -> csi2rx:port0 [style=bold]
        csi2rx [label="{{<port0> 0} | cdns_csi2rx.19800000.csi-bridge\n | {<port1> 1 | <port2> 2 | <port3> 3 | <port4> 4}}", shape=Mrecord, style=filled, fillcolor=green]
        csi2rx:port1 -> vin:port0 [style=bold]
        ov5640 [label="{{} | ov5640 6-0020\n/dev/v4l-subdev1 | {<port0> 0}}", shape=Mrecord, style=filled, fillcolor=green]
        ov5640:port0 -> vin:port1 [style=bold]
        vin [label="{{<port0> 0 | <port1> 1} | stf_vin0\n/dev/v4l-subdev2 | {<port2> 2}}", shape=Mrecord, style=filled, fillcolor=green]
        vin:port2 -> raw_capture [style=dashed]
        vin:port2 -> isp:port0 [style=dashed]
        raw_capture [label="stf_vin0_wr_video0\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
        isp [label="{{<port0> 0} | stf_isp0\n/dev/v4l-subdev3 | {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green]
        isp:port1 -> yuv_capture [style=bold]
        yuv_capture [label="stf_vin0_isp0_video1\n/dev/video1", shape=box, style=filled, fillcolor=yellow]
}

Here, the stf_vin0 subdev is used to route either the CSI-2 input or the
DVP input to the raw capture video device and the ISP.

Does this match the hardware architecture ?

Yes, but I have a question to ask, is there any way for stf_vin0 subdev
to distinguish whether the current scene is 'raw_capture' or 'yuv_capture'?

Sorry about the late reply, I had missed your e-mail.

I'm not sure to follow you here. Do you mean differentiating between
capturing raw frames from stf_vin0_wr_video0 and YUV frames from
stf_vin0_isp0_video1 ? Are those mutually exclusive, or is it possible
to capture both raw frames and YUV frames at the same time ?

quoted

What are ports 2, 3 and 4 for in the CSI-2 RX ?

port2-4 are useless, they should be created by 'cdns-csi2rx.c' by default.
 

quoted

quoted

quoted

quoted

quoted

quoted

Signed-off-by: Jack Zhu <jack.zhu@starfivetech.com>
---
 .../media/platform/starfive/camss/Makefile    |    4 +-
 .../media/platform/starfive/camss/stf_camss.c |   42 +-
 .../media/platform/starfive/camss/stf_camss.h |    2 +
 .../media/platform/starfive/camss/stf_vin.c   | 1069 +++++++++++++++++
 .../media/platform/starfive/camss/stf_vin.h   |  173 +++
 .../platform/starfive/camss/stf_vin_hw_ops.c  |  192 +++
 6 files changed, 1478 insertions(+), 4 deletions(-)
 create mode 100644 drivers/media/platform/starfive/camss/stf_vin.c
 create mode 100644 drivers/media/platform/starfive/camss/stf_vin.h
 create mode 100644 drivers/media/platform/starfive/camss/stf_vin_hw_ops.c

diff --git a/drivers/media/platform/starfive/camss/Makefile b/drivers/media/platform/starfive/camss/Makefile
index cdf57e8c9546..ef574e01ca47 100644
--- a/drivers/media/platform/starfive/camss/Makefile
+++ b/drivers/media/platform/starfive/camss/Makefile

@@ -7,6 +7,8 @@ starfive-camss-objs += \
 		stf_camss.o \
 		stf_isp.o \
 		stf_isp_hw_ops.o \
-		stf_video.o
+		stf_video.o \
+		stf_vin.o \
+		stf_vin_hw_ops.o
 
 obj-$(CONFIG_VIDEO_STARFIVE_CAMSS) += starfive-camss.o

diff --git a/drivers/media/platform/starfive/camss/stf_camss.c b/drivers/media/platform/starfive/camss/stf_camss.c
index 6f56b45f57db..834ea63eb833 100644
--- a/drivers/media/platform/starfive/camss/stf_camss.c
+++ b/drivers/media/platform/starfive/camss/stf_camss.c

@@ -131,27 +131,61 @@ static int stfcamss_init_subdevices(struct stfcamss *stfcamss)
 		return ret;
 	}
 

-- 
Regards,

Laurent Pinchart