Message-ID: <6380144e-f0c4-4415-89d5-36ed3d5a4205@wolfvision.net>
Date: Fri, 10 Jan 2025 09:48:04 +0100
From: Michael Riesch <michael.riesch@...fvision.net>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Mehdi Djait <mehdi.djait@...ux.intel.com>,
 Maxime Chevallier <maxime.chevallier@...tlin.com>,
 Théo Lebrun <theo.lebrun@...tlin.com>,
 Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring
 <robh+dt@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
 Kever Yang <kever.yang@...k-chips.com>,
 Nicolas Dufresne <nicolas@...fresne.ca>,
 Sebastian Fricke <sebastian.fricke@...labora.com>,
 Alexander Shiyan <eagle.alexander923@...il.com>,
 Val Packett <val@...kett.cool>, Rob Herring <robh@...nel.org>,
 Philipp Zabel <p.zabel@...gutronix.de>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>, linux-media@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org,
 Mehdi Djait <mehdi.djait@...tlin.com>,
 Gerald Loacker <gerald.loacker@...fvision.net>
Subject: Re: [PATCH v2 0/6] media: rockchip: add a driver for the rockchip
 camera interface (cif)

Hi Laurent,

On 1/9/25 18:12, Laurent Pinchart wrote:
> Hi Michael,
> 
> On Tue, Dec 17, 2024 at 04:55:12PM +0100, Michael Riesch wrote:
>> [...]
>> and refactor the whole thing. The resulting v2 of the series now adds a
>> basic media controller centric V4L2 driver for the Rockchip CIF with
>>  - support for the PX30 VIP (not tested, though, due to the lack of HW)
>>  - support for the RK3568 VICAP DVP
>>  - abstraction for the ping-pong scheme to allow for future extensions 
>>
>> However, several features are not yet addressed, such as
>>  - support for the RK3568 MIPI CSI-2 receiver
> 
> We've discussed this previously on IRC, but I don't think the issue has
> been raised on the list.
> 
> I'm puzzled by how this will work. As far as I understand, the RK3568
> has a CSI-2 receiver with 4 data lanes and 2 clock lanes. I assume this
> is used to support both 2x2 lanes and 1x4 lanes. Both the VICAP and ISP
> sections of the TRM list CSI2 RX registers, but it's not clear how the
> components are all connected. Does the ISP need to be part of the same
> media graph ?

Well you are not the only one to be puzzled by this HW :-) I started to
bring up the MIPI CSI-2 part and can describe the situation as I
understand it. No claim for correctness or completeness, though.

Indeed, the RK3568 MIPI CSI-2 PHY (TRM Chapter 28) has 4 data lanes and
2 clock lanes. And indeed it is possible to attach one device (1x4) or
two devices (2x2) to it. In the latter case the PHY is operated in split
mode (and 1x4 would be the default full mode, then). The mode can be set
in a GRF register.

The RK3568 features a MIPI CSI-2 host controller (TRM Chapter 27) that
(apparently) is connected to the RK3568 VICAP but has its registers in a
separate memory region. Right now I am trying to clean up the downstream
V4L2 subdevice driver for this host controller, which shall be linked to
the PHY using the "phys" DT property, and linked to the VICAP using DT
endpoints. In the end, the media graph could look like

  CC1 (V4L2 subdev) --> RK3568 VICAP DVP (V4L2 device)

  CC2 (V4L2 subdev) --> RK3568 MIPI CSI-2 HOST (V4L2 subdev) -->
  RK3568 VICAP MIPI (V4L2 device)

where CC denotes a companion chip, such as an image sensor or a video
decoder. Note that there are two disjoint subgraphs in this topology.

As you already pointed out, the RK3568 ISP features its integrated MIPI
CSI-2 host controller (as e.g. the RK3399 ISP does). This host
controller is represented by a V4L2 subdevice as well, and the
connection to the (same) MIPI PHY is established similarly. The
difference may be that this controller has its registers within the ISP
register block and is thus tightly coupled to the ISP (and the rkisp1
driver).

Now I guess that in split mode the ISP MIPI path works completely
independent and could be represented either by a separate media graph or
by another disjoint subgraph in the same media graph.
In split mode we need to define which host controller should handle
which set of lanes (there are two sets, clock 1 + data 1 + data 2 as
well as clock 2 + data 3 + data 4). Right now I am not sure how this
should be accomplished, but then the split mode can also wait a bit.

However, as you pointed out in our IRC discussion, it is mentioned in
TRM Chapter 12 that the RK3568 ISP can also process the DVP input. This
still needs some verification, the rest of the paragraph is pure
speculation. But maybe on simply needs to set the correct bits in
CTRL_0000_VI_DPCL. Now this would mean that there should be one large
media graph the includes the RK3568 VICAP as well as the RK3568 ISP,
i.e., something along the lines

  CC0 (V4L2 subdev) --> RK3568 ISP MIPI CSI-2 HOST (V4L2 subdev)
                            |
                            v
                     RK3568 ISP MUX (V4L2 subdev) --> RK3568 ISP...
                            ^
                            |
  CC1 (V4L2 subdev) --> MAGIC V4L2 subdev? -->
  RK3568 VICAP DVP (V4L2 device)

  CC2 (V4L2 subdev) --> RK3568 MIPI CSI-2 HOST (V4L2 subdev) -->
  RK3568 VICAP MIPI (V4L2 device)

where the MAGIC V4L2 subdev should be a fan out of some sort?!

Now to answer your question: I guess it is going to be one media graph
for RK3568 VICAP and RK3588 ISP. This shall be in perfect alignment with
the RK3588, where VICAP and ISPs are clearly connected.

Is this something we need to take into account right now, though? Or can
we complete the work on the VICAP first and then start adding the ISP?

Hope that helps!

Regards,
Michael

> 
>>  - support for the RK3588 variant
>>  - support for the scaling/cropping units that can be found in some
>>    variants
>>  - support for capturing different virtual channels (up to four IDs
>>    possible)
>> This needs to be in the scope of future work.
>>
>> Finally, please forgive me if I forgot to address reviewer comments from
>> the previous iterations. Between v1 and v13 they have seen significant
>> feedback including renaming the complete driver twice (from rkcif to vip
>> and back to cif) and I am pretty sure that I was not able to gather
>> everything.
>>
>> Looking forward to your comments!
>>
>> [0] https://lore.kernel.org/linux-media/20201229161724.511102-1-maxime.chevallier@bootlin.com/
>> [1] https://lore.kernel.org/linux-media/cover.1707677804.git.mehdi.djait.k@gmail.com/
>> [2] https://lore.kernel.org/all/20240220-v6-8-topic-rk3568-vicap-v1-0-2680a1fa640b@wolfvision.net/
>>
>> To: Mehdi Djait <mehdi.djait@...ux.intel.com>
>> To: Maxime Chevallier <maxime.chevallier@...tlin.com>
>> To: Théo Lebrun <theo.lebrun@...tlin.com>
>> To: Thomas Petazzoni <thomas.petazzoni@...tlin.com>
>> To: Sakari Ailus <sakari.ailus@....fi>
>> To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
>> To: Mauro Carvalho Chehab <mchehab@...nel.org>
>> To: Rob Herring <robh+dt@...nel.org>
>> To: Krzysztof Kozlowski <krzk+dt@...nel.org>
>> To: Conor Dooley <conor+dt@...nel.org>
>> To: Heiko Stuebner <heiko@...ech.de>
>> To: Kever Yang <kever.yang@...k-chips.com>
>> To: Nicolas Dufresne <nicolas@...fresne.ca>
>> To: Sebastian Fricke <sebastian.fricke@...labora.com>
>> To: Alexander Shiyan <eagle.alexander923@...il.com>
>> To: Val Packett <val@...kett.cool>
>> To: Rob Herring <robh@...nel.org>
>> To: Philipp Zabel <p.zabel@...gutronix.de>
>> Cc: linux-media@...r.kernel.org
>> Cc: devicetree@...r.kernel.org
>> Cc: linux-kernel@...r.kernel.org
>> Cc: linux-arm-kernel@...ts.infradead.org
>> Cc: linux-rockchip@...ts.infradead.org
>> Signed-off-by: Michael Riesch <michael.riesch@...fvision.net>
>>
>> Changes in v2:
>> - merged with Mehdi's v13
>> - refactored the complete driver towards a media controller centric driver
>> - abstracted the generic ping-pong stream (can be used for DVP as well as for CSI-2)
>> - switched to MPLANE API
>> - added support for notifications
>> - Link to v1: https://lore.kernel.org/r/20240220-v6-8-topic-rk3568-vicap-v1-0-2680a1fa640b@wolfvision.net
>>
>> ---
>> Mehdi Djait (2):
>>       media: dt-bindings: media: add bindings for rockchip px30 vip
>>       arm64: dts: rockchip: add the vip node to px30
>>
>> Michael Riesch (4):
>>       media: dt-bindings: media: video-interfaces: add defines for sampling modes
>>       media: dt-bindings: media: add bindings for rockchip rk3568 vicap
>>       media: rockchip: add a driver for the rockchip camera interface (cif)
>>       arm64: dts: rockchip: add vicap node to rk356x
>>
>>  .../bindings/media/rockchip,px30-vip.yaml          | 123 ++++
>>  .../bindings/media/rockchip,rk3568-vicap.yaml      | 168 +++++
>>  MAINTAINERS                                        |   9 +
>>  arch/arm64/boot/dts/rockchip/px30.dtsi             |  12 +
>>  arch/arm64/boot/dts/rockchip/rk356x-base.dtsi      |  44 ++
>>  drivers/media/platform/rockchip/Kconfig            |   1 +
>>  drivers/media/platform/rockchip/Makefile           |   1 +
>>  drivers/media/platform/rockchip/cif/Kconfig        |  15 +
>>  drivers/media/platform/rockchip/cif/Makefile       |   3 +
>>  .../media/platform/rockchip/cif/cif-capture-dvp.c  | 794 +++++++++++++++++++++
>>  .../media/platform/rockchip/cif/cif-capture-dvp.h  |  24 +
>>  drivers/media/platform/rockchip/cif/cif-common.h   | 163 +++++
>>  drivers/media/platform/rockchip/cif/cif-dev.c      | 405 +++++++++++
>>  drivers/media/platform/rockchip/cif/cif-regs.h     | 132 ++++
>>  drivers/media/platform/rockchip/cif/cif-stream.c   | 676 ++++++++++++++++++
>>  drivers/media/platform/rockchip/cif/cif-stream.h   |  24 +
>>  include/dt-bindings/media/video-interfaces.h       |   4 +
>>  17 files changed, 2598 insertions(+)
>> ---
>> base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
>> change-id: 20240220-v6-8-topic-rk3568-vicap-b9b3f9925f44
> 

Powered by blists - more mailing lists