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

Message ID 20231129093113.255161-3-paul.elder@ideasonboard.com (mailing list archive)
State Not Applicable
Headers
Series arm64: dts: imx8mp: Add support for the ISPs |

Commit Message

Paul Elder Nov. 29, 2023, 9:31 a.m. UTC
  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.

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
  

Comments

Alexander Stein Nov. 29, 2023, 10:20 a.m. UTC | #1
Hi Paul,

thanks for the patch.

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.

> 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?

> +
> +	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.

Best regards,
Alexander

[1] https://lore.kernel.org/all/
CAMuHMdWu4KZbBkvEofUV2wuA1g2S=XHHM3RUN1cNrcZBkhsPZA@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>;
> +};
  
Laurent Pinchart Nov. 29, 2023, 3:16 p.m. UTC | #2
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.

> > 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.

> > +
> > +	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 :-)

> [1] https://lore.kernel.org/all/CAMuHMdWu4KZbBkvEofUV2wuA1g2S=XHHM3RUN1cNrcZBkhsPZA@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>;
> > +};
  
Alexander Stein Nov. 30, 2023, 9:51 a.m. UTC | #3
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 :-)
> 
> > [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>;
> > > +};
  
Adam Ford Nov. 30, 2023, 1:48 p.m. UTC | #4
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.

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/
>
>
>
  
Kieran Bingham Nov. 30, 2023, 2:02 p.m. UTC | #5
+ 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/
> >
> >
> >
  
Adam Ford Nov. 30, 2023, 2:09 p.m. UTC | #6
On Thu, Nov 30, 2023 at 8:02 AM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> + 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.
>

 That makes sense, I didn't think of it that way.

>
> 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/
> > >
> > >
> > >
  
Laurent Pinchart Nov. 30, 2023, 2:20 p.m. UTC | #7
Hi Alexander,

On Thu, Nov 30, 2023 at 10:51:22AM +0100, Alexander Stein wrote:
> Am Mittwoch, 29. November 2023, 16:16:37 CET schrieb Laurent Pinchart:
> > 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.

Any sensor could be used with either the ISI or the ISP, so I don't
think the camera module overlay would be the best place for this. Unless
you want to duplicate all camera module overlays, with an ISI version
and an ISP version :-)

> > > > 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?

Correct, that's the hard part.

> > > > +
> > > > +	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 :-)
> > 
> > > [1] https://lore.kernel.org/all/CAMuHMdWu4KZbBkvEofUV2wuA1g2S=XHHM3RUN1cNrcZBkhsPZA@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>;
> > > > +};
  
Alexander Stein Nov. 30, 2023, 2:22 p.m. UTC | #8
Hi Kieran,

Am Donnerstag, 30. November 2023, 15:02:36 CET schrieb Kieran Bingham:
> + 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.

Maybe I am missing something, but how can the intermediate overlay reduce this 
amount of overlays? At least the remote-endpoint of the sensor needs the CSI 
endpoint, which is different for CSI1/CSI2, and vice versa.
I do not have rockchip hardware, but I would not assume the CSI endpoint to 
not have the same label, so the phandle reference is different there as well.

> 
> 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.

You still would have the naming problem. How do you identify a connector? It's 
similar to label naming on device tree nodes.

Best regards,
Alexander

> 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.anto
> niou@konsulko.com/
> 
> --
> Kieran
> 
> > adam
> > 
> > > > > [1]
> > > > > https://lore.kernel.org/all/CAMuHMdWu4KZbBkvEofUV2wuA1g2S=XHHM3RUN1c
> > > > > NrcZB
> > > > > 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/
  
Alexander Stein Nov. 30, 2023, 3:34 p.m. UTC | #9
Hi Laurent,

Am Donnerstag, 30. November 2023, 15:20:48 CET schrieb Laurent Pinchart:
> Hi Alexander,
> 
> On Thu, Nov 30, 2023 at 10:51:22AM +0100, Alexander Stein wrote:
> > Am Mittwoch, 29. November 2023, 16:16:37 CET schrieb Laurent Pinchart:
> > > 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.
> 
> Any sensor could be used with either the ISI or the ISP, so I don't
> think the camera module overlay would be the best place for this. Unless
> you want to duplicate all camera module overlays, with an ISI version
> and an ISP version :-)

True, that's a really good argument for having these small overlays.
But how to deal with dtc warnings?
> imx8mp-isp1.dtbo: Warning (graph_port): /fragment@2: graph port node name 
should be 'port'
> imx8mp-isp1.dtso:34.17-36.3: Warning (graph_endpoint): /fragment@2/
__overlay__: graph endpoint node name should be 'endpoint'
> imx8mp-isp1.dtso:34.17-36.3: Warning (graph_endpoint): /fragment@2/
__overlay__: graph connection to node '/fragment@1/__overlay__/ports/port@1/
endpoint' is not bidirectional

But for the small overlay itself:
Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com>

> > > > > 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?
> 
> Correct, that's the hard part.
> 
> > > > > +
> > > > > +	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 :-)
> > > 
> > > > [1]
> > > > https://lore.kernel.org/all/CAMuHMdWu4KZbBkvEofUV2wuA1g2S=XHHM3RUN1cN
> > > > rcZBkhsPZA@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>;
> > > > > +};
  
Laurent Pinchart Nov. 30, 2023, 3:54 p.m. UTC | #10
On Thu, Nov 30, 2023 at 04:34:11PM +0100, Alexander Stein wrote:
> Am Donnerstag, 30. November 2023, 15:20:48 CET schrieb Laurent Pinchart:
> > On Thu, Nov 30, 2023 at 10:51:22AM +0100, Alexander Stein wrote:
> > > Am Mittwoch, 29. November 2023, 16:16:37 CET schrieb Laurent Pinchart:
> > > > 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.
> > 
> > Any sensor could be used with either the ISI or the ISP, so I don't
> > think the camera module overlay would be the best place for this. Unless
> > you want to duplicate all camera module overlays, with an ISI version
> > and an ISP version :-)
> 
> True, that's a really good argument for having these small overlays.
> But how to deal with dtc warnings?
> > imx8mp-isp1.dtbo: Warning (graph_port): /fragment@2: graph port node name 
> should be 'port'
> > imx8mp-isp1.dtso:34.17-36.3: Warning (graph_endpoint): /fragment@2/
> __overlay__: graph endpoint node name should be 'endpoint'
> > imx8mp-isp1.dtso:34.17-36.3: Warning (graph_endpoint): /fragment@2/
> __overlay__: graph connection to node '/fragment@1/__overlay__/ports/port@1/
> endpoint' is not bidirectional

See below :-)

> But for the small overlay itself:
> Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> 
> > > > > > 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

Overlays need to be validated in the context of a base DT on which they
apply. For instance, in the same Makefile, we have

--------
dtb-$(CONFIG_ARCH_MXC) += imx8mm-tqma8mqml-mba8mx.dtb
...
imx8mm-tqma8mqml-mba8mx-lvds-tm070jvhg33-dtbs += imx8mm-tqma8mqml-mba8mx.dtb imx8mm-tqma8mqml-mba8mx-lvds-tm070jvhg33.dtbo
dtb-$(CONFIG_ARCH_MXC) += imx8mm-tqma8mqml-mba8mx-lvds-tm070jvhg33.dtb
--------

imx8mm-tqma8mqml-mba8mx.dts is the base board DT, and
imx8mm-tqma8mqml-mba8mx-lvds-tm070jvhg33.dtso the overlay. As far as I
understand, when compiling dtbs, the build system will compile
imx8mm-tqma8mqml-mba8mx.dtb and
imx8mm-tqma8mqml-mba8mx-lvds-tm070jvhg33-dtb. To create the latter, it
will compile imx8mm-tqma8mqml-mba8mx-lvds-tm070jvhg33.dtbo and apply it
to imx8mm-tqma8mqml-mba8mx.dtb. Then, it will validate the DTBs
specified as part of dtb-$(CONFIG_ARCH_MXC), which are
imx8mm-tqma8mqml-mba8mx.dtb standlone, and through
imx8mm-tqma8mqml-mba8mx-lvds-tm070jvhg33-dtbs
imx8mm-tqma8mqml-mba8mx.dtb with the overlay applied.

TL;DR: a v2 of this patch should fix the Makefile, and be compile-tested
with make dtbs.

Rob, Conor or Krzysztof can correct me if I'm wrong.

> > > > > > 
> > > > > >  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?
> > 
> > Correct, that's the hard part.
> > 
> > > > > > +
> > > > > > +	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 :-)
> > > > 
> > > > > [1] https://lore.kernel.org/all/CAMuHMdWu4KZbBkvEofUV2wuA1g2S=XHHM3RUN1cNrcZBkhsPZA@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>;
> > > > > > +};
  

Patch

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";
+
+	ports {
+		port@0 {
+			/delete-node/ endpoint;
+		};
+	};
+};
+
+&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>;
+};