Hi Jack,
On Thu, Aug 03, 2023 at 10:44:50AM +0800, Jack Zhu wrote:
> On 2023/8/2 18:38, Laurent Pinchart wrote:
> > On Wed, Aug 02, 2023 at 05:58:26PM +0800, Jack Zhu wrote:
> >> On 2023/7/28 4:49, Laurent Pinchart wrote:
> >> > On Mon, Jun 19, 2023 at 07:28:38PM +0800, Jack Zhu wrote:
> >> >> 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.
>
> > 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 ?
What are ports 2, 3 and 4 for in the CSI-2 RX ?
> >> >> Signed-off-by: Jack Zhu <jack.zhu@xxxxxxxxxxxxxxxx>
> >> >> ---
> >> >> .../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
