Re: [PATCH 2/2] arm64: dts: imx8mp: Add overlays for ISP instances

[Kieran Bingham <kieran.bingham@ideasonboard.com>]

+ Pantellis

Quoting Adam Ford (2023-11-30 13:48:58)
> On Thu, Nov 30, 2023 at 3:51 AM Alexander Stein
> <alexander.stein@ew.tq-group.com> wrote:
> >
> > Hi Laurent,
> >
> > Am Mittwoch, 29. November 2023, 16:16:37 CET schrieb Laurent Pinchart:
> > > Hi Alexander,
> > >
> > > On Wed, Nov 29, 2023 at 11:20:07AM +0100, Alexander Stein wrote:
> > > > Am Mittwoch, 29. November 2023, 10:31:13 CET schrieb Paul Elder:
> > > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > >
> > > > > Add two overlay to enable each ISP instance. The ISP is wired directly
> > > > > to the CSIS for now, bypassing the ISI completely.
> > > >
> > > > I'm not sure if this is worth adding in a separate overlay.
> > >
> > > The trouble is that, at this point, selection between the ISP and the
> > > ISI can only be performed through DT :-S That's why this is implemented
> > > as an overlay.
> >
> > I feel a better place would be the overlay which actually adds the sensor.
> > This knows best whether ISI or ISP should be used.
> >
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > > ---
> > > > >
> > > > >  arch/arm64/boot/dts/freescale/Makefile        |  2 ++
> > > > >  .../arm64/boot/dts/freescale/imx8mp-isp1.dtso | 36 +++++++++++++++++++
> > > > >  .../arm64/boot/dts/freescale/imx8mp-isp2.dtso | 36 +++++++++++++++++++
> > > > >  3 files changed, 74 insertions(+)
> > > > >  create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-isp1.dtso
> > > > >  create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-isp2.dtso
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/freescale/Makefile
> > > > > b/arch/arm64/boot/dts/freescale/Makefile index
> > > > > 300049037eb0..f97dfac11189
> > > > > 100644
> > > > > --- a/arch/arm64/boot/dts/freescale/Makefile
> > > > > +++ b/arch/arm64/boot/dts/freescale/Makefile
> > > > > @@ -113,6 +113,8 @@ dtb-$(CONFIG_ARCH_MXC) += imx8mp-dhcom-pdk2.dtb
> > > > >
> > > > >  dtb-$(CONFIG_ARCH_MXC) += imx8mp-dhcom-pdk3.dtb
> > > > >  dtb-$(CONFIG_ARCH_MXC) += imx8mp-evk.dtb
> > > > >  dtb-$(CONFIG_ARCH_MXC) += imx8mp-icore-mx8mp-edimm2.2.dtb
> > > > >
> > > > > +dtb-$(CONFIG_ARCH_MXC) += imx8mp-isp1.dtbo
> > > > > +dtb-$(CONFIG_ARCH_MXC) += imx8mp-isp2.dtbo
> > > > >
> > > > >  dtb-$(CONFIG_ARCH_MXC) += imx8mp-msc-sm2s-ep1.dtb
> > > > >  dtb-$(CONFIG_ARCH_MXC) += imx8mp-phyboard-pollux-rdk.dtb
> > > > >  dtb-$(CONFIG_ARCH_MXC) += imx8mp-tqma8mpql-mba8mpxl.dtb
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp-isp1.dtso
> > > > > b/arch/arm64/boot/dts/freescale/imx8mp-isp1.dtso new file mode 100644
> > > > > index 000000000000..cf394ed224ab
> > > > > --- /dev/null
> > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mp-isp1.dtso
> > > > > @@ -0,0 +1,36 @@
> > > > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > > > +/*
> > > > > + * Copyright 2022 Ideas on Board Oy
> > > > > + */
> > > > > +
> > > > > +/dts-v1/;
> > > > > +/plugin/;
> > > > > +
> > > > > +#include <dt-bindings/media/video-interfaces.h>
> > > > > +
> > > > > +&isi_0 {
> > > > > + status = "disabled";
> > > >
> > > > ISI is disabled by default. What is your intention here?
> > >
> > > It could be enabled by an overlay for a camera module. Ideally we want
> > > to be able to enable both the ISI and ISP at runtime, but that's not
> > > possible yet and will require a very large amount of work.
> >
> > Again IMHO this is part of sensor setup, in a very specific overlay. To put it
> > into different words: I barely see the gain of this small overlay.
> >
> > Runtime switching would require a combined media controller including both ISI
> > and ISP, no?
> >
> > Best regards,
> > Alexander
> >
> > > > > +
> > > > > + ports {
> > > > > +         port@0 {
> > > > > +                 /delete-node/ endpoint;
> > > >
> > > > This doesn't work in overlays. See [1]. Otherwise the OF graph connections
> > > > look fine to me. I'm using the same in my local overlay.
> > >
> > > Interesting, I wasn't aware of that. Maybe we should fix it :-)
> 
> When I did my camera implementation, I thought it was simpler to:
> 
> /delete-node/ &isi_in_0;
> 
> it's a one-line change.
> 
> I would suggest we just drop the overlay and make users who have the
> cameras integrate the cameras and the isp routing into their
> respective overlays.
> 

I use these to factor out common parts between multiple cameras that can
be connected to multiple ports.

I can connect any of (Physically available to me right now)
 IMX219, IMX477, IMX708, GC2145, OV5640(7?) IMX335, IMX283, IMX519, Arducam64MP

to either of:

Debix-SOM-A Port CSI-1
Debix-SOM-A Port CSI-2

And I can connect those same cameras to two ports of a Pi5. So now
that's 27 overlays to manage the 9 cameras I have /on my desk/ to
connect to this board.

Uh Oh - sorry I can also connect them to a Debix Model A ... oh no ... I
need to stop thinking about what I can connect them to. I have rockchip
boards they'll work on too!

This explosion of overlays could be ... hard to manage. With /a lot/ of
repetition of the same data.


I'm not opposed to dropping these intermediate helper overlays, but I'd
be interested to know if anyone has ideas on how we could define
'connectors' and then abstract the cameras / overlays that can be moved
between different compatible ports.

The [RFC 0/3] Portable Device Tree Connector [0] might be interesting to
resurrect. Did that go anywhere?

[0] https://lore.kernel.org/all/1464986273-12039-1-git-send-email-pantelis.antoniou@konsulko.com/

--
Kieran

> adam
> > >
> > > > [1]
> > > > https://lore.kernel.org/all/CAMuHMdWu4KZbBkvEofUV2wuA1g2S=XHHM3RUN1cNrcZB
> > > > khsPZA@mail.gmail.com/>
> > > > > +         };
> > > > > + };
> > > > > +};
> > > > > +
> > > > > +&isp_0 {
> > > > > + status = "okay";
> > > > > +
> > > > > + ports {
> > > > > +         port@1 {
> > > > > +                 isp0_in: endpoint {
> > > > > +                         bus-type = <MEDIA_BUS_TYPE_PARALLEL>;
> > > > > +                         remote-endpoint = <&mipi_csi_0_out>;
> > > > > +                 };
> > > > > +         };
> > > > > + };
> > > > > +};
> > > > > +
> > > > > +&mipi_csi_0_out {
> > > > > + remote-endpoint = <&isp0_in>;
> > > > > +};
> > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp-isp2.dtso
> > > > > b/arch/arm64/boot/dts/freescale/imx8mp-isp2.dtso new file mode 100644
> > > > > index 000000000000..14e2e7b2617f
> > > > > --- /dev/null
> > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mp-isp2.dtso
> > > > > @@ -0,0 +1,36 @@
> > > > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > > > +/*
> > > > > + * Copyright 2022 Ideas on Board Oy
> > > > > + */
> > > > > +
> > > > > +/dts-v1/;
> > > > > +/plugin/;
> > > > > +
> > > > > +#include <dt-bindings/media/video-interfaces.h>
> > > > > +
> > > > > +&isi_0 {
> > > > > + status = "disabled";
> > > > > +
> > > > > + ports {
> > > > > +         port@1 {
> > > > > +                 /delete-node/ endpoint;
> > > > > +         };
> > > > > + };
> > > > > +};
> > > > > +
> > > > > +&isp_1 {
> > > > > + status = "okay";
> > > > > +
> > > > > + ports {
> > > > > +         port@1 {
> > > > > +                 isp1_in: endpoint {
> > > > > +                         bus-type = <MEDIA_BUS_TYPE_PARALLEL>;
> > > > > +                         remote-endpoint = <&mipi_csi_1_out>;
> > > > > +                 };
> > > > > +         };
> > > > > + };
> > > > > +};
> > > > > +
> > > > > +&mipi_csi_1_out {
> > > > > + remote-endpoint = <&isp1_in>;
> > > > > +};
> >
> >
> > --
> > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > Amtsgericht München, HRB 105018
> > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> > http://www.tq-group.com/
> >
> >
> >