[PATCH 0/5] Driver for Apple ISP and cameras.

[PATCH 0/5] Driver for Apple ISP and cameras.

[Sasha Finkelstein via B4 Relay]

Hi.

This series adds support for the camera and ISP system present
on Apple devices using M-series chips. This is a "simple" camera
and does not need any special userspace handling, everything
is handled by the firmware running on an ASC coprocessor.

Patches 1 and 2 add support for special handling neccesary for the
ISP power domains. The rest add the driver itself.

Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
---
Asahi Lina (1):
      pmdomain: apple: Add force-disable/force-reset

Eileen Yoon (2):
      media: apple: Add Apple ISP driver
      arm64: dts: apple: Add ISP nodes

Sasha Finkelstein (2):
      dt-bindings: power: apple,pmgr-pwrstate: Add force-{disable,reset} properties
      media: dt-bindings: Add Apple ISP

 .../devicetree/bindings/media/apple,isp.yaml       | 151 ++++
 .../bindings/power/apple,pmgr-pwrstate.yaml        |  10 +
 MAINTAINERS                                        |   2 +
 arch/arm64/boot/dts/apple/isp-common.dtsi          |  45 ++
 arch/arm64/boot/dts/apple/isp-imx248.dtsi          |  62 ++
 arch/arm64/boot/dts/apple/isp-imx364.dtsi          |  78 ++
 arch/arm64/boot/dts/apple/isp-imx558-cfg0.dtsi     | 101 +++
 arch/arm64/boot/dts/apple/isp-imx558.dtsi          | 102 +++
 arch/arm64/boot/dts/apple/t600x-die0.dtsi          |  48 ++
 arch/arm64/boot/dts/apple/t600x-j314-j316.dtsi     |   6 +
 arch/arm64/boot/dts/apple/t600x-pmgr.dtsi          |  81 ++
 arch/arm64/boot/dts/apple/t8103-j293.dts           |   6 +
 arch/arm64/boot/dts/apple/t8103-j313.dts           |   6 +
 arch/arm64/boot/dts/apple/t8103-j456.dts           |   6 +
 arch/arm64/boot/dts/apple/t8103-j457.dts           |   6 +
 arch/arm64/boot/dts/apple/t8103-pmgr.dtsi          | 118 +++
 arch/arm64/boot/dts/apple/t8103.dtsi               |  50 ++
 arch/arm64/boot/dts/apple/t8112-j413.dts           |   7 +
 arch/arm64/boot/dts/apple/t8112-j493.dts           |   6 +
 arch/arm64/boot/dts/apple/t8112-pmgr.dtsi          | 118 +++
 arch/arm64/boot/dts/apple/t8112.dtsi               |  50 ++
 drivers/media/platform/Kconfig                     |   1 +
 drivers/media/platform/Makefile                    |   1 +
 drivers/media/platform/apple/Kconfig               |   5 +
 drivers/media/platform/apple/Makefile              |   3 +
 drivers/media/platform/apple/isp/Kconfig           |  16 +
 drivers/media/platform/apple/isp/Makefile          |   3 +
 drivers/media/platform/apple/isp/isp-cam.c         | 414 ++++++++++
 drivers/media/platform/apple/isp/isp-cam.h         |  21 +
 drivers/media/platform/apple/isp/isp-cmd.c         | 635 +++++++++++++++
 drivers/media/platform/apple/isp/isp-cmd.h         | 692 ++++++++++++++++
 drivers/media/platform/apple/isp/isp-drv.c         | 586 ++++++++++++++
 drivers/media/platform/apple/isp/isp-drv.h         | 284 +++++++
 drivers/media/platform/apple/isp/isp-fw.c          | 770 ++++++++++++++++++
 drivers/media/platform/apple/isp/isp-fw.h          |  24 +
 drivers/media/platform/apple/isp/isp-iommu.c       | 251 ++++++
 drivers/media/platform/apple/isp/isp-iommu.h       |  20 +
 drivers/media/platform/apple/isp/isp-ipc.c         | 258 ++++++
 drivers/media/platform/apple/isp/isp-ipc.h         |  25 +
 drivers/media/platform/apple/isp/isp-regs.h        |  56 ++
 drivers/media/platform/apple/isp/isp-v4l2.c        | 900 +++++++++++++++++++++
 drivers/media/platform/apple/isp/isp-v4l2.h        |  16 +
 drivers/pmdomain/apple/pmgr-pwrstate.c             |  43 +-
 43 files changed, 6077 insertions(+), 6 deletions(-)
---
base-commit: 2408a807bfc3f738850ef5ad5e3fd59d66168996
change-id: 20250218-isp-5da1dbc7d472

[PATCH 1/5] dt-bindings: power: apple,pmgr-pwrstate: Add force-{disable,reset} properties

[Sasha Finkelstein via B4 Relay]

From: Sasha Finkelstein <fnkl.kernel@gmail.com>

Add properties to set disable/reset bits when powering down
certain domains

Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
---
 .../devicetree/bindings/power/apple,pmgr-pwrstate.yaml         | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml b/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml
index 59a6af735a2167b7edd9e0491da238f21effe316..9c8f5385bee5aa1bac8c3e44963713299160521e 100644
--- a/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml
+++ b/Documentation/devicetree/bindings/power/apple,pmgr-pwrstate.yaml
@@ -62,6 +62,16 @@ properties:
       Forces this power domain to always be powered up.
     type: boolean
 
+  apple,force-disable:
+    description:
+      Use the disable bit when turning the power off
+    type: boolean
+
+  apple,force-reset:
+    description:
+      Use the reset bit when turning the power off
+    type: boolean
+
   apple,min-state:
     description:
       Specifies the minimum power state for auto-PM.

-- 
2.48.1

[PATCH 2/5] pmdomain: apple: Add force-disable/force-reset

[Sasha Finkelstein via B4 Relay]

From: Asahi Lina <lina@asahilina.net>

It seems some ISP power states should have their force disable device
access flag set when powered down (which may avoid this problem, but
we're still figuring that out), and on some bit 12 is also explicitly
set before shutdown. Add two properties to handle this case.

Signed-off-by: Asahi Lina <lina@asahilina.net>
Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
---
 drivers/pmdomain/apple/pmgr-pwrstate.c | 43 +++++++++++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/drivers/pmdomain/apple/pmgr-pwrstate.c b/drivers/pmdomain/apple/pmgr-pwrstate.c
index 9467235110f4654e00ab96c25e160e125ef0f3e5..3236d854265c9489cca7affadf4448e8444dd839 100644
--- a/drivers/pmdomain/apple/pmgr-pwrstate.c
+++ b/drivers/pmdomain/apple/pmgr-pwrstate.c
@@ -21,7 +21,8 @@
 #define APPLE_PMGR_AUTO_ENABLE  BIT(28)
 #define APPLE_PMGR_PS_AUTO      GENMASK(27, 24)
 #define APPLE_PMGR_PS_MIN       GENMASK(19, 16)
-#define APPLE_PMGR_PARENT_OFF   BIT(11)
+#define APPLE_PMGR_PS_RESET     BIT(12)
+#define APPLE_PMGR_BUSY         BIT(11)
 #define APPLE_PMGR_DEV_DISABLE  BIT(10)
 #define APPLE_PMGR_WAS_CLKGATED BIT(9)
 #define APPLE_PMGR_WAS_PWRGATED BIT(8)
@@ -44,6 +45,8 @@ struct apple_pmgr_ps {
 	struct regmap *regmap;
 	u32 offset;
 	u32 min_state;
+	bool force_disable;
+	bool force_reset;
 };
 
 #define genpd_to_apple_pmgr_ps(_genpd) container_of(_genpd, struct apple_pmgr_ps, genpd)
@@ -53,7 +56,7 @@ static int apple_pmgr_ps_set(struct generic_pm_domain *genpd, u32 pstate, bool a
 {
 	int ret;
 	struct apple_pmgr_ps *ps = genpd_to_apple_pmgr_ps(genpd);
-	u32 reg;
+	u32 reg, cur;
 
 	ret = regmap_read(ps->regmap, ps->offset, &reg);
 	if (ret < 0)
@@ -64,7 +67,29 @@ static int apple_pmgr_ps_set(struct generic_pm_domain *genpd, u32 pstate, bool a
 		dev_err(ps->dev, "PS %s: powering off with RESET active\n",
 			genpd->name);
 
-	reg &= ~(APPLE_PMGR_AUTO_ENABLE | APPLE_PMGR_FLAGS | APPLE_PMGR_PS_TARGET);
+	if (pstate != APPLE_PMGR_PS_ACTIVE && (ps->force_disable || ps->force_reset)) {
+		u32 reg_pre = reg & ~(APPLE_PMGR_AUTO_ENABLE | APPLE_PMGR_FLAGS);
+
+		if (ps->force_disable)
+			reg_pre |= APPLE_PMGR_DEV_DISABLE;
+		if (ps->force_reset)
+			reg_pre |= APPLE_PMGR_PS_RESET;
+
+		regmap_write(ps->regmap, ps->offset, reg_pre);
+
+		ret = regmap_read_poll_timeout_atomic(
+			ps->regmap, ps->offset, cur,
+			(cur & (APPLE_PMGR_DEV_DISABLE | APPLE_PMGR_PS_RESET)) ==
+			(reg_pre & (APPLE_PMGR_DEV_DISABLE | APPLE_PMGR_PS_RESET)), 1,
+			APPLE_PMGR_PS_SET_TIMEOUT);
+
+		if (ret < 0)
+			dev_err(ps->dev, "PS %s: Failed to set reset/disable bits (now: 0x%x)\n",
+				genpd->name, reg);
+	}
+
+	reg &= ~(APPLE_PMGR_DEV_DISABLE | APPLE_PMGR_PS_RESET |
+		 APPLE_PMGR_AUTO_ENABLE | APPLE_PMGR_FLAGS | APPLE_PMGR_PS_TARGET);
 	reg |= FIELD_PREP(APPLE_PMGR_PS_TARGET, pstate);
 
 	dev_dbg(ps->dev, "PS %s: pwrstate = 0x%x: 0x%x\n", genpd->name, pstate, reg);
@@ -72,16 +97,16 @@ static int apple_pmgr_ps_set(struct generic_pm_domain *genpd, u32 pstate, bool a
 	regmap_write(ps->regmap, ps->offset, reg);
 
 	ret = regmap_read_poll_timeout_atomic(
-		ps->regmap, ps->offset, reg,
-		(FIELD_GET(APPLE_PMGR_PS_ACTUAL, reg) == pstate), 1,
+		ps->regmap, ps->offset, cur,
+		FIELD_GET(APPLE_PMGR_PS_ACTUAL, cur) == pstate, 1,
 		APPLE_PMGR_PS_SET_TIMEOUT);
+
 	if (ret < 0)
 		dev_err(ps->dev, "PS %s: Failed to reach power state 0x%x (now: 0x%x)\n",
 			genpd->name, pstate, reg);
 
 	if (auto_enable) {
 		/* Not all devices implement this; this is a no-op where not implemented. */
-		reg &= ~APPLE_PMGR_FLAGS;
 		reg |= APPLE_PMGR_AUTO_ENABLE;
 		regmap_write(ps->regmap, ps->offset, reg);
 	}
@@ -244,6 +269,12 @@ static int apple_pmgr_ps_probe(struct platform_device *pdev)
 		}
 	}
 
+	if (of_property_read_bool(node, "apple,force-disable"))
+		ps->force_disable = true;
+
+	if (of_property_read_bool(node, "apple,force-reset"))
+		ps->force_reset = true;
+
 	/* Turn on auto-PM if the domain is already on */
 	if (active)
 		regmap_update_bits(regmap, ps->offset, APPLE_PMGR_FLAGS | APPLE_PMGR_AUTO_ENABLE,

-- 
2.48.1

[PATCH 3/5] media: dt-bindings: Add Apple ISP

[Sasha Finkelstein via B4 Relay]

From: Sasha Finkelstein <fnkl.kernel@gmail.com>

Add bindings for the ISP used with the webcam in Apple
ARM laptops.

Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
---
 .../devicetree/bindings/media/apple,isp.yaml       | 151 +++++++++++++++++++++
 MAINTAINERS                                        |   1 +
 2 files changed, 152 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/apple,isp.yaml b/Documentation/devicetree/bindings/media/apple,isp.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..67d536b61851af30fcc5bc452a761138876c6b18
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/apple,isp.yaml
@@ -0,0 +1,151 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/apple,isp.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: An ISP block used in Apple products
+
+maintainers:
+  - Sasha Finkelstein <fnkl.kernel@gmail.com>
+
+description:
+  The ISP in charge of webcams on ARM Apple laptops
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - apple,t8103-isp
+          - apple,t8112-isp
+          - apple,t6000-isp
+          - apple,t6020-isp
+      - const: apple,isp
+
+  reg:
+    items:
+      - description: ASC coprocessor control
+      - description: Peripheral to host mailbox
+      - description: General-purpose ASC IO registers
+      - description: Host to peripheral mailbox
+
+  reg-names:
+    items:
+      - const: coproc
+      - const: mbox
+      - const: gpio
+      - const: mbox2
+
+  iommus:
+    description: All 3 must be kept in sync
+    minItems: 3
+    maxItems: 3
+
+  interrupts:
+    maxItems: 1
+
+  power-domains:
+    minItems: 1
+    maxItems: 20
+    description: All necessary power domains. Driver will enable them in order
+
+  memory-region:
+    maxItems: 1
+
+  apple,dart-vm-size:
+    description: Supported device memory range
+    $ref: /schemas/types.yaml#/definitions/uint64
+
+  apple,platform-id:
+    description: Platform id for firmware
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  apple,temporal-filter:
+    description: Whether temporal filter should be enabled in firmware
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  sensor-presets:
+    additionalProperties: false
+
+    patternProperties:
+      '^preset[0-9]+$':
+        type: object
+
+        additionalProperties: false
+
+        properties:
+          apple,config-index:
+            description: Firmware config index
+            $ref: /schemas/types.yaml#/definitions/uint32
+
+          apple,input-size:
+            $ref: /schemas/types.yaml#/definitions/uint32-array
+            minItems: 2
+            maxItems: 2
+            description: Raw sensor size
+
+          apple,output-size:
+            $ref: /schemas/types.yaml#/definitions/uint32-array
+            minItems: 2
+            maxItems: 2
+            description: Cropped and scaled image size
+
+          apple,crop:
+            $ref: /schemas/types.yaml#/definitions/uint32-array
+            minItems: 4
+            maxItems: 4
+            description: Area to crop
+
+        required:
+          - apple,config-index
+          - apple,input-size
+          - apple,output-size
+          - apple,crop
+
+required:
+  - compatible
+  - reg
+  - iommus
+  - interrupts
+  - power-domains
+  - memory-region
+  - apple,dart-vm-size
+  - apple,platform-id
+  - apple,temporal-filter
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/apple-aic.h>
+    isp: isp@2a000000 {
+        compatible = "apple,t8103-isp", "apple,isp";
+        reg = <0x2a000000 0x2000000>,
+              <0x2c104000 0x100>,
+              <0x2c104170 0x100>,
+              <0x2c1043f0 0x100>;
+        reg-names = "coproc", "mbox", "gpio", "mbox2";
+        iommus = <&isp_dart0 0>, <&isp_dart1 0>, <&isp_dart2 0>;
+        interrupt-parent = <&aic>;
+        interrupts = <AIC_IRQ 246 IRQ_TYPE_LEVEL_HIGH>;
+        power-domains = <&ps_isp_sys>, <&ps_isp_set0>,
+                        <&ps_isp_set1>, <&ps_isp_set2>, <&ps_isp_fe>,
+                        <&ps_isp_set4>, <&ps_isp_set5>, <&ps_isp_set6>,
+                        <&ps_isp_set7>, <&ps_isp_set8>, <&ps_isp_set9>,
+                        <&ps_isp_set10>, <&ps_isp_set11>,
+                        <&ps_isp_set12>;
+        memory-region = <&isp_heap>;
+        apple,dart-vm-size = <0x0 0xa0000000>;
+        apple,platform-id = <1>;
+        apple,temporal-filter = <0>;
+
+        sensor-presets {
+            preset0 {
+                apple,config-index = <0>;
+                apple,input-size = <1296 736>;
+                apple,output-size = <1280 720>;
+                apple,crop = <8 8 1280 720>;
+            };
+        };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index efee40ea589f70bc5e4a390072a4543234616743..dea7239ee0f5464b31efed5a2e0e5e602bcb6439 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2228,6 +2228,7 @@ F:	Documentation/devicetree/bindings/interrupt-controller/apple,*
 F:	Documentation/devicetree/bindings/iommu/apple,dart.yaml
 F:	Documentation/devicetree/bindings/iommu/apple,sart.yaml
 F:	Documentation/devicetree/bindings/mailbox/apple,mailbox.yaml
+F:	Documentation/devicetree/bindings/media/apple,isp.yaml
 F:	Documentation/devicetree/bindings/net/bluetooth/brcm,bcm4377-bluetooth.yaml
 F:	Documentation/devicetree/bindings/nvme/apple,nvme-ans.yaml
 F:	Documentation/devicetree/bindings/nvmem/apple,efuses.yaml

-- 
2.48.1

[PATCH 5/5] arm64: dts: apple: Add ISP nodes

[Sasha Finkelstein via B4 Relay]

From: Eileen Yoon <eyn@gmx.com>

Adds device tree entries for the ISP and camera sensors

Signed-off-by: Eileen Yoon <eyn@gmx.com>
Co-developed-by: Janne Grunau <j@jannau.net>
Signed-off-by: Janne Grunau <j@jannau.net>
Co-developed-by: Hector Martin <marcan@marcan.st>
Signed-off-by: Hector Martin <marcan@marcan.st>
Co-developed-by: Asahi Lina <lina@asahilina.net>
Signed-off-by: Asahi Lina <lina@asahilina.net>
Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
---
 arch/arm64/boot/dts/apple/isp-common.dtsi      |  45 ++++++++++
 arch/arm64/boot/dts/apple/isp-imx248.dtsi      |  62 +++++++++++++
 arch/arm64/boot/dts/apple/isp-imx364.dtsi      |  78 ++++++++++++++++
 arch/arm64/boot/dts/apple/isp-imx558-cfg0.dtsi | 101 +++++++++++++++++++++
 arch/arm64/boot/dts/apple/isp-imx558.dtsi      | 102 +++++++++++++++++++++
 arch/arm64/boot/dts/apple/t600x-die0.dtsi      |  48 ++++++++++
 arch/arm64/boot/dts/apple/t600x-j314-j316.dtsi |   6 ++
 arch/arm64/boot/dts/apple/t600x-pmgr.dtsi      |  81 +++++++++++++++++
 arch/arm64/boot/dts/apple/t8103-j293.dts       |   6 ++
 arch/arm64/boot/dts/apple/t8103-j313.dts       |   6 ++
 arch/arm64/boot/dts/apple/t8103-j456.dts       |   6 ++
 arch/arm64/boot/dts/apple/t8103-j457.dts       |   6 ++
 arch/arm64/boot/dts/apple/t8103-pmgr.dtsi      | 118 +++++++++++++++++++++++++
 arch/arm64/boot/dts/apple/t8103.dtsi           |  50 +++++++++++
 arch/arm64/boot/dts/apple/t8112-j413.dts       |   7 ++
 arch/arm64/boot/dts/apple/t8112-j493.dts       |   6 ++
 arch/arm64/boot/dts/apple/t8112-pmgr.dtsi      | 118 +++++++++++++++++++++++++
 arch/arm64/boot/dts/apple/t8112.dtsi           |  50 +++++++++++
 18 files changed, 896 insertions(+)

diff --git a/arch/arm64/boot/dts/apple/isp-common.dtsi b/arch/arm64/boot/dts/apple/isp-common.dtsi
new file mode 100644
index 0000000000000000000000000000000000000000..beaaab950bef70d7521666ba093f10a57cf1b910
--- /dev/null
+++ b/arch/arm64/boot/dts/apple/isp-common.dtsi
@@ -0,0 +1,45 @@
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+/*
+ * Common ISP configuration for Apple silicon platforms.
+ *
+ * Copyright The Asahi Linux Contributors
+ */
+
+/ {
+	aliases {
+		isp = &isp;
+	};
+
+	reserved-memory {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		isp_heap: isp-heap {
+			/* Filled in by bootloder */
+			reg = <0 0 0 0>;
+			no-map;
+		};
+	};
+};
+
+&isp {
+	memory-region = <&isp_heap>;
+	status = "okay";
+};
+
+&isp_dart0 {
+	status = "okay";
+};
+
+&isp_dart1 {
+	status = "okay";
+};
+
+&isp_dart2 {
+	status = "okay";
+};
+
+&ps_isp_sys {
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/apple/isp-imx248.dtsi b/arch/arm64/boot/dts/apple/isp-imx248.dtsi
new file mode 100644
index 0000000000000000000000000000000000000000..1d8a87b8058cfc8d63ccef5874aa7c2c318f9654
--- /dev/null
+++ b/arch/arm64/boot/dts/apple/isp-imx248.dtsi
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+/*
+ * ISP configuration for platforms with IMX248 sensor.
+ *
+ * Copyright The Asahi Linux Contributors
+ */
+
+#include "isp-common.dtsi"
+
+&isp {
+	apple,temporal-filter = <0>;
+
+	sensor-presets {
+		/* 1280x720 */
+		preset0 {
+			apple,config-index = <0>;
+			apple,input-size = <1296 736>;
+			apple,output-size = <1280 720>;
+			apple,crop = <8 8 1280 720>;
+		};
+
+		/* 960x720 (4:3) */
+		preset1 {
+			apple,config-index = <0>;
+			apple,input-size = <1296 736>;
+			apple,output-size = <960 720>;
+			apple,crop = <168 8 960 720>;
+		};
+
+		/* 960x540 (16:9) */
+		preset2 {
+			apple,config-index = <0>;
+			apple,input-size = <1296 736>;
+			apple,output-size = <960 540>;
+			apple,crop = <8 8 1280 720>;
+		};
+
+		/* 640x480 (4:3) */
+		preset3 {
+			apple,config-index = <0>;
+			apple,input-size = <1296 736>;
+			apple,output-size = <640 480>;
+			apple,crop = <168 8 960 720>;
+		};
+
+		/* 640x360 (16:9) */
+		preset4 {
+			apple,config-index = <0>;
+			apple,input-size = <1296 736>;
+			apple,output-size = <640 360>;
+			apple,crop = <8 8 1280 720>;
+		};
+
+		/* 320x180 (16:9) */
+		preset5 {
+			apple,config-index = <0>;
+			apple,input-size = <1296 736>;
+			apple,output-size = <320 180>;
+			apple,crop = <8 8 1280 720>;
+		};
+	};
+};
diff --git a/arch/arm64/boot/dts/apple/isp-imx364.dtsi b/arch/arm64/boot/dts/apple/isp-imx364.dtsi
new file mode 100644
index 0000000000000000000000000000000000000000..dc52fb22126ffb7370fb4bb9e9a9e04ee3c1b9ae
--- /dev/null
+++ b/arch/arm64/boot/dts/apple/isp-imx364.dtsi
@@ -0,0 +1,78 @@
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+/*
+ * ISP configuration for platforms with IMX364 sensor.
+ *
+ * Copyright The Asahi Linux Contributors
+ */
+
+#include "isp-common.dtsi"
+
+&isp {
+	apple,temporal-filter = <0>;
+
+	sensor-presets {
+		/* 1920x1080 */
+		preset0 {
+			apple,config-index = <0>;
+			apple,input-size = <1920 1080>;
+			apple,output-size = <1920 1080>;
+			apple,crop = <0 0 1920 1080>;
+		};
+
+		/* 1440x720 (4:3) */
+		preset1 {
+			apple,config-index = <0>;
+			apple,input-size = <1920 1080>;
+			apple,output-size = <1440 1080>;
+			apple,crop = <240 0 1440 1080>;
+		};
+
+		/* 1280x720 (16:9) */
+		preset2 {
+			apple,config-index = <0>;
+			apple,input-size = <1920 1080>;
+			apple,output-size = <1280 720>;
+			apple,crop = <0 0 1920 1080>;
+		};
+
+		/* 960x720 (4:3) */
+		preset3{
+			apple,config-index = <0>;
+			apple,input-size = <1920 1080>;
+			apple,output-size = <960 720>;
+			apple,crop = <240 0 1440 1080>;
+		};
+
+		/* 960x540 (16:9) */
+		preset4 {
+			apple,config-index = <0>;
+			apple,input-size = <1920 1080>;
+			apple,output-size = <960 540>;
+			apple,crop = <0 0 1920 1080>;
+		};
+
+		/* 640x480 (4:3) */
+		preset5 {
+			apple,config-index = <0>;
+			apple,input-size = <1920 1080>;
+			apple,output-size = <640 480>;
+			apple,crop = <240 0 1440 1080>;
+		};
+
+		/* 640x360 (16:9) */
+		preset6 {
+			apple,config-index = <0>;
+			apple,input-size = <1920 1080>;
+			apple,output-size = <640 360>;
+			apple,crop = <0 0 1920 1080>;
+		};
+
+		/* 320x180 (16:9) */
+		preset7 {
+			apple,config-index = <0>;
+			apple,input-size = <1920 1080>;
+			apple,output-size = <320 180>;
+			apple,crop = <0 0 1920 1080>;
+		};
+	};
+};
diff --git a/arch/arm64/boot/dts/apple/isp-imx558-cfg0.dtsi b/arch/arm64/boot/dts/apple/isp-imx558-cfg0.dtsi
new file mode 100644
index 0000000000000000000000000000000000000000..83a86a937f41a4589419b051d33f3fae728ffe6e
--- /dev/null
+++ b/arch/arm64/boot/dts/apple/isp-imx558-cfg0.dtsi
@@ -0,0 +1,101 @@
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+/*
+ * ISP configuration for platforms with IMX558 sensor in
+ * config #0 mode.
+ *
+ * These platforms enable MLVNR for all configs except
+ * #0, which we don't support. Config #0 is an uncropped
+ * square 1920x1920 sensor, with dark corners.
+ * Therefore, we synthesize common resolutions by using
+ * crop/scale while always choosing config #0.
+ *
+ * Copyright The Asahi Linux Contributors
+ */
+
+#include "isp-common.dtsi"
+
+&isp {
+	apple,temporal-filter = <0>;
+
+	sensor-presets {
+		/* 1920x1080 */
+		preset0 {
+			apple,config-index = <0>;
+			apple,input-size = <1920 1920>;
+			apple,output-size = <1920 1080>;
+			apple,crop = <0 420 1920 1080>;
+		};
+
+		/* 1080x1920 */
+		preset1 {
+			apple,config-index = <0>;
+			apple,input-size = <1920 1920>;
+			apple,output-size = <1080 1920>;
+			apple,crop = <420 0 1080 1920>;
+		};
+
+		/* 1920x1440 */
+		preset2 {
+			apple,config-index = <0>;
+			apple,input-size = <1920 1920>;
+			apple,output-size = <1920 1440>;
+			apple,crop = <0 240 1920 1440>;
+		};
+
+		/* 1440x1920 */
+		preset3 {
+			apple,config-index = <0>;
+			apple,input-size = <1920 1920>;
+			apple,output-size = <1440 1920>;
+			apple,crop = <240 0 1440 1920>;
+		};
+
+		/* 1280x720 */
+		preset4 {
+			apple,config-index = <0>;
+			apple,input-size = <1920 1920>;
+			apple,output-size = <1280 720>;
+			apple,crop = <0 420 1920 1080>;
+		};
+
+		/* 720x1280 */
+		preset5 {
+			apple,config-index = <0>;
+			apple,input-size = <1920 1920>;
+			apple,output-size = <720 1280>;
+			apple,crop = <420 0 1080 1920>;
+		};
+
+		/* 1280x960 */
+		preset6 {
+			apple,config-index = <0>;
+			apple,input-size = <1920 1920>;
+			apple,output-size = <1280 960>;
+			apple,crop = <0 240 1920 1440>;
+		};
+
+		/* 960x1280 */
+		preset7 {
+			apple,config-index = <0>;
+			apple,input-size = <1920 1920>;
+			apple,output-size = <960 1280>;
+			apple,crop = <240 0 1440 1920>;
+		};
+
+		/* 640x480 */
+		preset8 {
+			apple,config-index = <0>;
+			apple,input-size = <1920 1920>;
+			apple,output-size = <640 480>;
+			apple,crop = <0 240 1920 1440>;
+		};
+
+		/* 480x640 */
+		preset9 {
+			apple,config-index = <0>;
+			apple,input-size = <1920 1920>;
+			apple,output-size = <480 640>;
+			apple,crop = <240 0 1440 1920>;
+		};
+	};
+};
diff --git a/arch/arm64/boot/dts/apple/isp-imx558.dtsi b/arch/arm64/boot/dts/apple/isp-imx558.dtsi
new file mode 100644
index 0000000000000000000000000000000000000000..6eae34a3c2ee4a5dc02c7871462c4cebfc0a9be2
--- /dev/null
+++ b/arch/arm64/boot/dts/apple/isp-imx558.dtsi
@@ -0,0 +1,102 @@
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+/*
+ * ISP configuration for platforms with IMX558 sensor.
+ *
+ * Copyright The Asahi Linux Contributors
+ */
+
+#include "isp-common.dtsi"
+
+&isp {
+	apple,temporal-filter = <0>;
+
+	sensor-presets {
+		/* 1920x1080 */
+		preset0 {
+			apple,config-index = <1>;
+			apple,input-size = <1920 1080>;
+			apple,output-size = <1920 1080>;
+			apple,crop = <0 0 1920 1080>;
+		};
+
+		/* 1080x1920 */
+		preset1 {
+			apple,config-index = <2>;
+			apple,input-size = <1080 1920>;
+			apple,output-size = <1080 1920>;
+			apple,crop = <0 0 1080 1920>;
+		};
+
+		/* 1760x1328 */
+		preset2 {
+			apple,config-index = <3>;
+			apple,input-size = <1760 1328>;
+			apple,output-size = <1760 1328>;
+			apple,crop = <0 0 1760 1328>;
+		};
+
+		/* 1328x1760 */
+		preset3 {
+			apple,config-index = <4>;
+			apple,input-size = <1328 1760>;
+			apple,output-size = < 1328 1760>;
+			apple,crop = <0 0 1328 1760>;
+		};
+
+		/* 1152x1152 */
+		preset4 {
+			apple,config-index = <5>;
+			apple,input-size = <1152 1152>;
+			apple,output-size = <1152 1152>;
+			apple,crop = <0 0 1152 1152>;
+		};
+
+		/* 1280x720 */
+		preset5 {
+			apple,config-index = <1>;
+			apple,input-size = <1920 1080>;
+			apple,output-size = <1280 720>;
+			apple,crop = <0 0 1920 1080>;
+		};
+
+		/* 720x1280 */
+		preset6 {
+			apple,config-index = <2>;
+			apple,input-size = <1080 1920>;
+			apple,output-size = <720 1280>;
+			apple,crop = <0 0 1080 1920>;
+		};
+
+		/* 1280x960 */
+		preset7 {
+			apple,config-index = <3>;
+			apple,input-size = <1760 1328>;
+			apple,output-size = <1280 960>;
+			apple,crop = <0 4 1760 1320>;
+		};
+
+		/* 960x1280 */
+		preset8 {
+			apple,config-index = <4>;
+			apple,input-size = <1328 1760>;
+			apple,output-size = <960 1280>;
+			apple,crop = <4 0 1320 1760>;
+		};
+
+		/* 640x480 */
+		preset9 {
+			apple,config-index = <3>;
+			apple,input-size = <1760 1328>;
+			apple,output-size = <640 480>;
+			apple,crop = <0 4 1760 1320>;
+		};
+
+		/* 480x640 */
+		preset10 {
+			apple,config-index = <4>;
+			apple,input-size = <1328 1760>;
+			apple,output-size = <480 640>;
+			apple,crop = <4 0 1320 1760>;
+		};
+	};
+};
diff --git a/arch/arm64/boot/dts/apple/t600x-die0.dtsi b/arch/arm64/boot/dts/apple/t600x-die0.dtsi
index b1c875e692c8fb9c0af46a23568a7b0cd720141b..019484360804ecdd0313638a0694a61693a5729a 100644
--- a/arch/arm64/boot/dts/apple/t600x-die0.dtsi
+++ b/arch/arm64/boot/dts/apple/t600x-die0.dtsi
@@ -53,6 +53,54 @@ wdt: watchdog@2922b0000 {
 		interrupts = <AIC_IRQ 0 631 IRQ_TYPE_LEVEL_HIGH>;
 	};
 
+	isp: isp@384000000 {
+		compatible = "apple,t6000-isp", "apple,isp";
+		reg = <0x3 0x84000000 0x0 0x2000000>,
+			<0x3 0x86104000 0x0 0x100>,
+			<0x3 0x86104170 0x0 0x100>,
+			<0x3 0x861043f0 0x0 0x100>;
+		reg-names = "coproc", "mbox", "gpio", "mbox2";
+		iommus = <&isp_dart0 0>, <&isp_dart1 0>, <&isp_dart2 0>;
+		interrupt-parent = <&aic>;
+		interrupts = <AIC_IRQ 0 538 IRQ_TYPE_LEVEL_HIGH>;
+		power-domains = <&ps_isp_sys>, <&ps_isp_set0>,
+			<&ps_isp_set1>, <&ps_isp_fe>, <&ps_isp_set3>,
+			<&ps_isp_set4>, <&ps_isp_set5>, <&ps_isp_set6>,
+			<&ps_isp_set7>, <&ps_isp_set8>;
+		apple,dart-vm-size = <0x0 0xa0000000>;
+		status = "disabled";
+	};
+
+	isp_dart0: iommu@3860e8000 {
+		compatible = "apple,t6000-dart";
+		reg = <0x3 0x860e8000 0x0 0x4000>;
+		interrupt-parent = <&aic>;
+		interrupts = <AIC_IRQ 0 543 IRQ_TYPE_LEVEL_HIGH>;
+		#iommu-cells = <1>;
+		power-domains = <&ps_isp_sys>;
+		status = "disabled";
+	};
+
+	isp_dart1: iommu@3860f4000 {
+		compatible = "apple,t6000-dart";
+		reg = <0x3 0x860f4000 0x0 0x4000>;
+		interrupt-parent = <&aic>;
+		interrupts = <AIC_IRQ 0 543 IRQ_TYPE_LEVEL_HIGH>;
+		#iommu-cells = <1>;
+		power-domains = <&ps_isp_sys>;
+		status = "disabled";
+	};
+
+	isp_dart2: iommu@3860fc000 {
+		compatible = "apple,t6000-dart";
+		reg = <0x3 0x860fc000 0x0 0x4000>;
+		interrupt-parent = <&aic>;
+		interrupts = <AIC_IRQ 0 543 IRQ_TYPE_LEVEL_HIGH>;
+		#iommu-cells = <1>;
+		power-domains = <&ps_isp_sys>;
+		status = "disabled";
+	};
+
 	sio_dart_0: iommu@39b004000 {
 		compatible = "apple,t6000-dart";
 		reg = <0x3 0x9b004000 0x0 0x4000>;
diff --git a/arch/arm64/boot/dts/apple/t600x-j314-j316.dtsi b/arch/arm64/boot/dts/apple/t600x-j314-j316.dtsi
index 2e471dfe43cf885c1234d36bf0e0acfdc4904621..416e6ec213c8238f0d3c15298d5e44cb13abaabf 100644
--- a/arch/arm64/boot/dts/apple/t600x-j314-j316.dtsi
+++ b/arch/arm64/boot/dts/apple/t600x-j314-j316.dtsi
@@ -119,3 +119,9 @@ sdhci0: mmc@0,0 {
 &fpwm0 {
 	status = "okay";
 };
+
+#include "isp-imx558.dtsi"
+
+&isp {
+	apple,platform-id = <3>;
+};
diff --git a/arch/arm64/boot/dts/apple/t600x-pmgr.dtsi b/arch/arm64/boot/dts/apple/t600x-pmgr.dtsi
index 0bd44753b76a0c111dc12e6e900bf2f3c07a07ff..1a24e0d20c040fc6225326cd665cf1d1902b7813 100644
--- a/arch/arm64/boot/dts/apple/t600x-pmgr.dtsi
+++ b/arch/arm64/boot/dts/apple/t600x-pmgr.dtsi
@@ -1368,6 +1368,7 @@ DIE_NODE(ps_isp_sys): power-controller@3a8 {
 		#reset-cells = <0>;
 		label = DIE_LABEL(isp_sys);
 		power-domains = <&DIE_NODE(ps_afnc2_lw1)>;
+		status = "disabled";
 	};
 
 	DIE_NODE(ps_venc_sys): power-controller@3b0 {
@@ -1456,6 +1457,86 @@ DIE_NODE(ps_venc_me1): power-controller@8020 {
 		label = DIE_LABEL(venc_me1);
 		power-domains = <&DIE_NODE(ps_venc_me0)>;
 	};
+
+	/* There is a dependency tree involved with these PDs,
+	 * but we do not express it here since the ISP driver
+	 * is supposed to sequence them in the right order anyway
+	 * (and we do not know the exact tree structure).
+	 *
+	 * This also works around spurious parent PD activation
+	 * on machines with ISP disabled (desktops).
+	 */
+	DIE_NODE(ps_isp_set0): power-controller@4000 {
+		compatible = "apple,t6000-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4000 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set0";
+	};
+
+	DIE_NODE(ps_isp_set1): power-controller@4010 {
+		compatible = "apple,t6000-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4010 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set1";
+	};
+
+	DIE_NODE(ps_isp_fe): power-controller@4008 {
+		compatible = "apple,t6000-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4008 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set2";
+	};
+
+	DIE_NODE(ps_isp_set3): power-controller@4028 {
+		compatible = "apple,t6000-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4028 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set3";
+	};
+
+	DIE_NODE(ps_isp_set4): power-controller@4020 {
+		compatible = "apple,t6000-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4020 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set4";
+	};
+
+	DIE_NODE(ps_isp_set5): power-controller@4030 {
+		compatible = "apple,t6000-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4030 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set5";
+	};
+
+	DIE_NODE(ps_isp_set6): power-controller@4018 {
+		compatible = "apple,t6000-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4018 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set6";
+	};
+
+	DIE_NODE(ps_isp_set7): power-controller@4038 {
+		compatible = "apple,t6000-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4038 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set7";
+	};
+
+	DIE_NODE(ps_isp_set8): power-controller@4040 {
+		compatible = "apple,t6000-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4040 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set8";
+	};
 };
 
 &DIE_NODE(pmgr_south) {
diff --git a/arch/arm64/boot/dts/apple/t8103-j293.dts b/arch/arm64/boot/dts/apple/t8103-j293.dts
index 56b0c67bfcda321b60c621de092643017693ff91..cef284f347363d416d375a68dacc7221739f2d75 100644
--- a/arch/arm64/boot/dts/apple/t8103-j293.dts
+++ b/arch/arm64/boot/dts/apple/t8103-j293.dts
@@ -49,3 +49,9 @@ &i2c4 {
 &fpwm1 {
 	status = "okay";
 };
+
+#include "isp-imx248.dtsi"
+
+&isp {
+	apple,platform-id = <1>;
+};
diff --git a/arch/arm64/boot/dts/apple/t8103-j313.dts b/arch/arm64/boot/dts/apple/t8103-j313.dts
index 97a4344d8dca685708aff136af92a1b316f3c3dd..b6899af595f47eaae3e5ff24ba18490dc81377c8 100644
--- a/arch/arm64/boot/dts/apple/t8103-j313.dts
+++ b/arch/arm64/boot/dts/apple/t8103-j313.dts
@@ -41,3 +41,9 @@ &wifi0 {
 &fpwm1 {
 	status = "okay";
 };
+
+#include "isp-imx248.dtsi"
+
+&isp {
+	apple,platform-id = <1>;
+};
diff --git a/arch/arm64/boot/dts/apple/t8103-j456.dts b/arch/arm64/boot/dts/apple/t8103-j456.dts
index 58c8e43789b4861544e20c717124ede3327be010..e54393b4e83c12a233067683c03e9ada2d1fba70 100644
--- a/arch/arm64/boot/dts/apple/t8103-j456.dts
+++ b/arch/arm64/boot/dts/apple/t8103-j456.dts
@@ -75,3 +75,9 @@ &pcie0_dart_1 {
 &pcie0_dart_2 {
 	status = "okay";
 };
+
+#include "isp-imx364.dtsi"
+
+&isp {
+	apple,platform-id = <2>;
+};
diff --git a/arch/arm64/boot/dts/apple/t8103-j457.dts b/arch/arm64/boot/dts/apple/t8103-j457.dts
index 152f95fd49a2118093396838fbd8b6bd1b518f81..7a825eee01f5600fe5eb9a11d59e64d3939edac6 100644
--- a/arch/arm64/boot/dts/apple/t8103-j457.dts
+++ b/arch/arm64/boot/dts/apple/t8103-j457.dts
@@ -48,3 +48,9 @@ ethernet0: ethernet@0,0 {
 &pcie0_dart_2 {
 	status = "okay";
 };
+
+#include "isp-imx364.dtsi"
+
+&isp {
+	apple,platform-id = <2>;
+};
diff --git a/arch/arm64/boot/dts/apple/t8103-pmgr.dtsi b/arch/arm64/boot/dts/apple/t8103-pmgr.dtsi
index 9645861a858c1a7c46c25a614c2cc4b03083bf46..5761cb260b7f1c1a0657aa99137e451892610ec3 100644
--- a/arch/arm64/boot/dts/apple/t8103-pmgr.dtsi
+++ b/arch/arm64/boot/dts/apple/t8103-pmgr.dtsi
@@ -805,6 +805,7 @@ ps_isp_sys: power-controller@400 {
 		#reset-cells = <0>;
 		label = "isp_sys";
 		power-domains = <&ps_rmx>;
+		status = "disabled";
 	};
 
 	ps_venc_sys: power-controller@408 {
@@ -1003,6 +1004,123 @@ ps_disp0_cpu0: power-controller@10018 {
 		apple,always-on; /* TODO: figure out if we can enable PM here */
 		apple,min-state = <4>;
 	};
+
+	/* There is a dependency tree involved with these PDs,
+	 * but we do not express it here since the ISP driver
+	 * is supposed to sequence them in the right order anyway
+	 * (and we do not know the exact tree structure).
+	 *
+	 * This also works around spurious parent PD activation
+	 * on machines with ISP disabled (desktops).
+	 */
+	ps_isp_set0: power-controller@4000 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4000 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set0";
+		apple,force-disable;
+	};
+
+	ps_isp_set1: power-controller@4008 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4008 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set1";
+		apple,force-disable;
+		apple,force-reset;
+	};
+
+	ps_isp_set2: power-controller@4010 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4010 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set2";
+		apple,force-disable;
+		apple,force-reset;
+	};
+
+	ps_isp_fe: power-controller@4018 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4018 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_fe";
+	};
+
+	ps_isp_set4: power-controller@4020 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4020 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set4";
+	};
+
+	ps_isp_set5: power-controller@4028 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4028 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set5";
+	};
+
+	ps_isp_set6: power-controller@4030 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4030 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set6";
+	};
+
+	ps_isp_set7: power-controller@4038 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4038 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set7";
+	};
+
+	ps_isp_set8: power-controller@4040 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4040 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set8";
+	};
+
+	ps_isp_set9: power-controller@4048 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4048 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set9";
+	};
+
+	ps_isp_set10: power-controller@4050 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4050 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set10";
+	};
+
+	ps_isp_set11: power-controller@4058 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4058 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set11";
+	};
+
+	ps_isp_set12: power-controller@4060 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4060 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set12";
+	};
 };
 
 &pmgr_mini {
diff --git a/arch/arm64/boot/dts/apple/t8103.dtsi b/arch/arm64/boot/dts/apple/t8103.dtsi
index 9b0dad6b618444ac6b1c9735c50cccfc3965f947..420fda4a8dba70992c7a55c9ef56142ed39ad68f 100644
--- a/arch/arm64/boot/dts/apple/t8103.dtsi
+++ b/arch/arm64/boot/dts/apple/t8103.dtsi
@@ -356,6 +356,56 @@ cpufreq_p: performance-controller@211e20000 {
 			#performance-domain-cells = <0>;
 		};
 
+		isp: isp@22a000000 {
+			compatible = "apple,t8103-isp", "apple,isp";
+			reg = <0x2 0x2a000000 0x0 0x2000000>,
+				<0x2 0x2c104000 0x0 0x100>,
+				<0x2 0x2c104170 0x0 0x100>,
+				<0x2 0x2c1043f0 0x0 0x100>;
+			reg-names = "coproc", "mbox", "gpio", "mbox2";
+			iommus = <&isp_dart0 0>, <&isp_dart1 0>, <&isp_dart2 0>;
+			interrupt-parent = <&aic>;
+			interrupts = <AIC_IRQ 246 IRQ_TYPE_LEVEL_HIGH>;
+			power-domains = <&ps_isp_sys>, <&ps_isp_set0>,
+				<&ps_isp_set1>, <&ps_isp_set2>, <&ps_isp_fe>,
+				<&ps_isp_set4>, <&ps_isp_set5>, <&ps_isp_set6>,
+				<&ps_isp_set7>, <&ps_isp_set8>, <&ps_isp_set9>,
+				<&ps_isp_set10>, <&ps_isp_set11>,
+				<&ps_isp_set12>;
+			apple,dart-vm-size = <0x0 0xa0000000>;
+			status = "disabled";
+		};
+
+		isp_dart0: iommu@22c0e8000 {
+			compatible = "apple,t8103-dart";
+			reg = <0x2 0x2c0e8000 0x0 0x4000>;
+			interrupt-parent = <&aic>;
+			interrupts = <AIC_IRQ 251 IRQ_TYPE_LEVEL_HIGH>;
+			power-domains = <&ps_isp_sys>;
+			#iommu-cells = <1>;
+			status = "disabled";
+		};
+
+		isp_dart1: iommu@22c0f4000 {
+			compatible = "apple,t8103-dart";
+			reg = <0x2 0x2c0f4000 0x0 0x4000>;
+			interrupt-parent = <&aic>;
+			interrupts = <AIC_IRQ 251 IRQ_TYPE_LEVEL_HIGH>;
+			#iommu-cells = <1>;
+			power-domains = <&ps_isp_sys>;
+			status = "disabled";
+		};
+
+		isp_dart2: iommu@22c0fc000 {
+			compatible = "apple,t8103-dart";
+			reg = <0x2 0x2c0fc000 0x0 0x4000>;
+			interrupt-parent = <&aic>;
+			interrupts = <AIC_IRQ 251 IRQ_TYPE_LEVEL_HIGH>;
+			#iommu-cells = <1>;
+			power-domains = <&ps_isp_sys>;
+			status = "disabled";
+		};
+
 		sio_dart: iommu@235004000 {
 			compatible = "apple,t8103-dart";
 			reg = <0x2 0x35004000 0x0 0x4000>;
diff --git a/arch/arm64/boot/dts/apple/t8112-j413.dts b/arch/arm64/boot/dts/apple/t8112-j413.dts
index 6f69658623bf89ce73e3486bce504f1f5f8003f3..94bf097ff2a343cd6446b97e3391f9faee2fc5f4 100644
--- a/arch/arm64/boot/dts/apple/t8112-j413.dts
+++ b/arch/arm64/boot/dts/apple/t8112-j413.dts
@@ -78,3 +78,10 @@ &i2c4 {
 &fpwm1 {
 	status = "okay";
 };
+
+#include "isp-imx558-cfg0.dtsi"
+
+&isp {
+	apple,platform-id = <14>;
+	apple,temporal-filter = <1>;
+};
diff --git a/arch/arm64/boot/dts/apple/t8112-j493.dts b/arch/arm64/boot/dts/apple/t8112-j493.dts
index 0ad908349f55406783942735a2e9dad54cda00ec..5632bfb939ef8ff4f746c8e61ae0a87b82c646cb 100644
--- a/arch/arm64/boot/dts/apple/t8112-j493.dts
+++ b/arch/arm64/boot/dts/apple/t8112-j493.dts
@@ -67,3 +67,9 @@ &i2c4 {
 &fpwm1 {
 	status = "okay";
 };
+
+#include "isp-imx248.dtsi"
+
+&isp {
+	apple,platform-id = <6>;
+};
diff --git a/arch/arm64/boot/dts/apple/t8112-pmgr.dtsi b/arch/arm64/boot/dts/apple/t8112-pmgr.dtsi
index 7c050c6f2707a1d8128c09b4f58076a1c035adac..7154615b757215a2294f8f1559657ade951aab6c 100644
--- a/arch/arm64/boot/dts/apple/t8112-pmgr.dtsi
+++ b/arch/arm64/boot/dts/apple/t8112-pmgr.dtsi
@@ -818,6 +818,7 @@ ps_isp_sys: power-controller@438 {
 		#reset-cells = <0>;
 		label = "isp_sys";
 		power-domains = <&ps_rmx1>;
+		status = "disabled";
 	};
 
 	ps_venc_sys: power-controller@440 {
@@ -964,6 +965,123 @@ ps_sep: power-controller@c00 {
 		apple,always-on;
 	};
 
+	/* There is a dependency tree involved with these PDs,
+	 * but we do not express it here since the ISP driver
+	 * is supposed to sequence them in the right order anyway
+	 * (and we do not know the exact tree structure).
+	 *
+	 * This also works around spurious parent PD activation
+	 * on machines with ISP disabled (desktops).
+	 */
+	ps_isp_set0: power-controller@4000 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4000 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set0";
+		apple,force-disable;
+	};
+
+	ps_isp_set1: power-controller@4008 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4008 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set1";
+		apple,force-disable;
+		apple,force-reset;
+	};
+
+	ps_isp_set2: power-controller@4010 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4010 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set2";
+		apple,force-disable;
+		apple,force-reset;
+	};
+
+	ps_isp_fe: power-controller@4018 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4018 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_fe";
+	};
+
+	ps_isp_set4: power-controller@4020 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4020 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set4";
+	};
+
+	ps_isp_set5: power-controller@4028 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4028 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set5";
+	};
+
+	ps_isp_set6: power-controller@4030 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4030 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set6";
+	};
+
+	ps_isp_set7: power-controller@4038 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4038 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set7";
+	};
+
+	ps_isp_set8: power-controller@4040 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4040 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set8";
+	};
+
+	ps_isp_set9: power-controller@4048 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4048 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set9";
+	};
+
+	ps_isp_set12: power-controller@4050 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4050 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set10";
+	};
+
+	ps_isp_set10: power-controller@4058 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4058 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set11";
+	};
+
+	ps_isp_set11: power-controller@4060 {
+		compatible = "apple,t8103-pmgr-pwrstate", "apple,pmgr-pwrstate";
+		reg = <0x4060 4>;
+		#power-domain-cells = <0>;
+		#reset-cells = <0>;
+		label = "isp_set12";
+	};
+
 	ps_venc_dma: power-controller@8000 {
 		compatible = "apple,t8112-pmgr-pwrstate", "apple,pmgr-pwrstate";
 		reg = <0x8000 4>;
diff --git a/arch/arm64/boot/dts/apple/t8112.dtsi b/arch/arm64/boot/dts/apple/t8112.dtsi
index 1666e6ab250bc0be9b8318e3c8fc903ccd3f3760..ed1852ac346b0f74d84619147feb7e624e355630 100644
--- a/arch/arm64/boot/dts/apple/t8112.dtsi
+++ b/arch/arm64/boot/dts/apple/t8112.dtsi
@@ -379,6 +379,56 @@ cpufreq_p: cpufreq@211e20000 {
 			#performance-domain-cells = <0>;
 		};
 
+		isp: isp@22a000000 {
+			compatible = "apple,t8112-isp", "apple,isp";
+			reg = <0x2 0x2a000000 0x0 0x2000000>,
+				<0x2 0x2c4c4000 0x0 0x100>,
+				<0x2 0x2c4c41b0 0x0 0x100>,
+				<0x2 0x2c4c4430 0x0 0x100>;
+			reg-names = "coproc", "mbox", "gpio", "mbox2";
+			iommus = <&isp_dart0 0>, <&isp_dart1 0>, <&isp_dart2 0>;
+			interrupt-parent = <&aic>;
+			interrupts = <AIC_IRQ 269 IRQ_TYPE_LEVEL_HIGH>;
+			power-domains = <&ps_isp_sys>, <&ps_isp_set0>,
+				<&ps_isp_set1>, <&ps_isp_set2>, <&ps_isp_fe>,
+				<&ps_isp_set4>, <&ps_isp_set5>, <&ps_isp_set6>,
+				<&ps_isp_set7>, <&ps_isp_set8>, <&ps_isp_set9>,
+				<&ps_isp_set10>, <&ps_isp_set11>,
+				<&ps_isp_set12>;
+			apple,dart-vm-size = <0x0 0xa0000000>;
+			status = "disabled";
+		};
+
+		isp_dart0: iommu@22c4a8000 {
+			compatible = "apple,t8110-dart";
+			reg = <0x2 0x2c4a8000 0x0 0x4000>;
+			interrupt-parent = <&aic>;
+			interrupts = <AIC_IRQ 274 IRQ_TYPE_LEVEL_HIGH>;
+			power-domains = <&ps_isp_sys>;
+			#iommu-cells = <1>;
+			status = "disabled";
+		};
+
+		isp_dart1: iommu@22c4b4000 {
+			compatible = "apple,t8110-dart";
+			reg = <0x2 0x2c4b4000 0x0 0x4000>;
+			interrupt-parent = <&aic>;
+			interrupts = <AIC_IRQ 274 IRQ_TYPE_LEVEL_HIGH>;
+			power-domains = <&ps_isp_sys>;
+			#iommu-cells = <1>;
+			status = "disabled";
+		};
+
+		isp_dart2: iommu@22c4bc000 {
+			compatible = "apple,t8110-dart";
+			reg = <0x2 0x2c4bc000 0x0 0x4000>;
+			interrupt-parent = <&aic>;
+			interrupts = <AIC_IRQ 274 IRQ_TYPE_LEVEL_HIGH>;
+			power-domains = <&ps_isp_sys>;
+			#iommu-cells = <1>;
+			status = "disabled";
+		};
+
 		sio_dart: iommu@235004000 {
 			compatible = "apple,t8110-dart";
 			reg = <0x2 0x35004000 0x0 0x4000>;

-- 
2.48.1

Re: [PATCH 1/5] dt-bindings: power: apple,pmgr-pwrstate: Add force-{disable,reset} properties

[Krzysztof Kozlowski]

On 19/02/2025 10:26, Sasha Finkelstein via B4 Relay wrote:
> From: Sasha Finkelstein <fnkl.kernel@gmail.com>
> 
> Add properties to set disable/reset bits when powering down
> certain domains


Please explain why and what problem are you solving. This looks too
close to SW policy or really arbitrary choice. Background would be useful.

Best regards,
Krzysztof

Re: [PATCH 3/5] media: dt-bindings: Add Apple ISP

[Krzysztof Kozlowski]

On 19/02/2025 10:26, Sasha Finkelstein via B4 Relay wrote:
> +  reg-names:
> +    items:
> +      - const: coproc
> +      - const: mbox
> +      - const: gpio
> +      - const: mbox2
> +
> +  iommus:
> +    description: All 3 must be kept in sync
> +    minItems: 3


Drop minItems

> +    maxItems: 3
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  power-domains:
> +    minItems: 1
> +    maxItems: 20
> +    description: All necessary power domains. Driver will enable them in order
> +
> +  memory-region:
> +    maxItems: 1
> +
> +  apple,dart-vm-size:
> +    description: Supported device memory range
> +    $ref: /schemas/types.yaml#/definitions/uint64


That's deducible from comaptible.

> +
> +  apple,platform-id:
> +    description: Platform id for firmware
> +    $ref: /schemas/types.yaml#/definitions/uint32


No, use firmware-name.

> +
> +  apple,temporal-filter:
> +    description: Whether temporal filter should be enabled in firmware
> +    $ref: /schemas/types.yaml#/definitions/uint32

And why is this not enabled always? Why this is board specific?


You miss here ports or port. ISP usually gets signal from some camera or
other block.


> +
> +  sensor-presets:
> +    additionalProperties: false
> +
> +    patternProperties:
> +      '^preset[0-9]+$':
> +        type: object
> +
> +        additionalProperties: false
> +
> +        properties:
> +          apple,config-index:
> +            description: Firmware config index
> +            $ref: /schemas/types.yaml#/definitions/uint32


No duplicated indices. You have reg for this, assuming this is index.

> +
> +          apple,input-size:
> +            $ref: /schemas/types.yaml#/definitions/uint32-array
> +            minItems: 2
> +            maxItems: 2
> +            description: Raw sensor size
> +
> +          apple,output-size:
> +            $ref: /schemas/types.yaml#/definitions/uint32-array
> +            minItems: 2
> +            maxItems: 2
> +            description: Cropped and scaled image size
> +
> +          apple,crop:
> +            $ref: /schemas/types.yaml#/definitions/uint32-array
> +            minItems: 4
> +            maxItems: 4
> +            description: Area to crop


All these do not look like hardware properties but rather configuration
of sensor which should be done runtime by OS, not by DT.

Best regards,
Krzysztof

Re: [PATCH 1/5] dt-bindings: power: apple,pmgr-pwrstate: Add force-{disable,reset} properties

[Sasha Finkelstein]

On Wed, 19 Feb 2025 at 10:34, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On 19/02/2025 10:26, Sasha Finkelstein via B4 Relay wrote:
> > From: Sasha Finkelstein <fnkl.kernel@gmail.com>
> >
> > Add properties to set disable/reset bits when powering down
> > certain domains
>
>
> Please explain why and what problem are you solving. This looks too
> close to SW policy or really arbitrary choice. Background would be useful.

The ISP block has some weird requirements where some of it's power domains
will not power down correctly without using the "force disable" or "force reset"
pmgr feature. Basically a hardware quirk.

Re: [PATCH 3/5] media: dt-bindings: Add Apple ISP

[Sasha Finkelstein]

On Wed, 19 Feb 2025 at 10:37, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > +
> > +  apple,platform-id:
> > +    description: Platform id for firmware
> > +    $ref: /schemas/types.yaml#/definitions/uint32
>
>
> No, use firmware-name.

Not sure how is firmware-name an appropriate field, fw-name is a string
that references a firmware file, while this field is an id that is sent to the
coprocessor firmware in order to identify the platform.

> > +  apple,temporal-filter:
> > +    description: Whether temporal filter should be enabled in firmware
> > +    $ref: /schemas/types.yaml#/definitions/uint32
>
> And why is this not enabled always? Why this is board specific?

Not every board has support for this feature.

> You miss here ports or port. ISP usually gets signal from some camera or
> other block.

For complex cameras - yes, but this is closer to a UVC camera connected
via a bespoke protocol. We do not need to deal with the sensor access,
all of it is managed by the coprocessor firmware.

> > +        properties:
> > +          apple,config-index:
> > +            description: Firmware config index
> > +            $ref: /schemas/types.yaml#/definitions/uint32
>
>
> No duplicated indices. You have reg for this, assuming this is index.

There are duplicated indices, see isp-imx248.dtsi in patch 5 for an example.

> All these do not look like hardware properties but rather configuration
> of sensor which should be done runtime by OS, not by DT.

Those are board-specific and not discoverable via the ISP protocol.

Re: [PATCH 3/5] media: dt-bindings: Add Apple ISP

[Laurent Pinchart]

On Wed, Feb 19, 2025 at 10:54:31AM +0100, Sasha Finkelstein wrote:
> On Wed, 19 Feb 2025 at 10:37, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > +
> > > +  apple,platform-id:
> > > +    description: Platform id for firmware
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> >
> >
> > No, use firmware-name.
> 
> Not sure how is firmware-name an appropriate field, fw-name is a string
> that references a firmware file, while this field is an id that is sent to the
> coprocessor firmware in order to identify the platform.
> 
> > > +  apple,temporal-filter:
> > > +    description: Whether temporal filter should be enabled in firmware
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> >
> > And why is this not enabled always? Why this is board specific?
> 
> Not every board has support for this feature.
> 
> > You miss here ports or port. ISP usually gets signal from some camera or
> > other block.
> 
> For complex cameras - yes, but this is closer to a UVC camera connected
> via a bespoke protocol. We do not need to deal with the sensor access,
> all of it is managed by the coprocessor firmware.
> 
> > > +        properties:
> > > +          apple,config-index:
> > > +            description: Firmware config index
> > > +            $ref: /schemas/types.yaml#/definitions/uint32
> >
> >
> > No duplicated indices. You have reg for this, assuming this is index.
> 
> There are duplicated indices, see isp-imx248.dtsi in patch 5 for an example.
> 
> > All these do not look like hardware properties but rather configuration
> > of sensor which should be done runtime by OS, not by DT.
> 
> Those are board-specific and not discoverable via the ISP protocol.

But they are settable through the ISP protocol, aren't they ? For
instance, looking at isp-imx248.dtsi, the first four entries are

	/* 1280x720 */
	preset0 {
		apple,config-index = <0>;
		apple,input-size = <1296 736>;
		apple,output-size = <1280 720>;
		apple,crop = <8 8 1280 720>;
	};

	/* 960x720 (4:3) */
	preset1 {
		apple,config-index = <0>;
		apple,input-size = <1296 736>;
		apple,output-size = <960 720>;
		apple,crop = <168 8 960 720>;
	};

	/* 960x540 (16:9) */
	preset2 {
		apple,config-index = <0>;
		apple,input-size = <1296 736>;
		apple,output-size = <960 540>;
		apple,crop = <8 8 1280 720>;
	};

	/* 640x480 (4:3) */
	preset3 {
		apple,config-index = <0>;
		apple,input-size = <1296 736>;
		apple,output-size = <640 480>;
		apple,crop = <168 8 960 720>;
	};

But I may be interested in capturing a 640x480 frame with cropping only
and without scaling, with

input-size = 1296x736
output-size = 640x480
crop = (328,128)/640x480

Or I may want my cropped frame to be located in the upper-left corner:

input-size = 1296x736
output-size = 640x480
crop = (8,8)/640x480

If I set those parameters through the ISP protocol, won't it work ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/5] media: dt-bindings: Add Apple ISP

[Sasha Finkelstein]

On Wed, 19 Feb 2025 at 11:53, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> >
> > Those are board-specific and not discoverable via the ISP protocol.
>
> But they are settable through the ISP protocol, aren't they ? For
> instance, looking at isp-imx248.dtsi, the first four entries are
>
>         /* 1280x720 */
>         preset0 {
>                 apple,config-index = <0>;
>                 apple,input-size = <1296 736>;
>                 apple,output-size = <1280 720>;
>                 apple,crop = <8 8 1280 720>;
>         };
>
>         /* 960x720 (4:3) */
>         preset1 {
>                 apple,config-index = <0>;
>                 apple,input-size = <1296 736>;
>                 apple,output-size = <960 720>;
>                 apple,crop = <168 8 960 720>;
>         };
>
>         /* 960x540 (16:9) */
>         preset2 {
>                 apple,config-index = <0>;
>                 apple,input-size = <1296 736>;
>                 apple,output-size = <960 540>;
>                 apple,crop = <8 8 1280 720>;
>         };
>
>         /* 640x480 (4:3) */
>         preset3 {
>                 apple,config-index = <0>;
>                 apple,input-size = <1296 736>;
>                 apple,output-size = <640 480>;
>                 apple,crop = <168 8 960 720>;
>         };
>
> But I may be interested in capturing a 640x480 frame with cropping only
> and without scaling, with
>
> input-size = 1296x736
> output-size = 640x480
> crop = (328,128)/640x480
>
> Or I may want my cropped frame to be located in the upper-left corner:
>
> input-size = 1296x736
> output-size = 640x480
> crop = (8,8)/640x480
>
> If I set those parameters through the ISP protocol, won't it work ?
>
> --
> Regards,
>
> Laurent Pinchart

For cropping - you do not want to change those parameters, the sensor
is partially occluded, and the crop area is specified in such a way
to not expose those pixels. As for scaling - we can expose only the 1:1
scale and let userspace deal with it, but it appears that it expects
the other common output sizes to exist.

Re: [PATCH 4/5] media: apple: Add Apple ISP driver

[Janne Grunau]

Hej,

On Wed, Feb 19, 2025 at 10:27:00AM +0100, Sasha Finkelstein via B4 Relay wrote:
> From: Eileen Yoon <eyn@gmx.com>
> 
> This is the ISP and camera module present on certain Apple laptops
> 
> Signed-off-by: Eileen Yoon <eyn@gmx.com>
> Co-developed-by: Hector Martin <marcan@marcan.st>
> Signed-off-by: Hector Martin <marcan@marcan.st>
> Co-developed-by: Asahi Lina <lina@asahilina.net>
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> Co-developed-by: Janne Grunau <j@jannau.net>
> Signed-off-by: Janne Grunau <j@jannau.net>
> Co-developed-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
> Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
> ---
>  MAINTAINERS                                  |   1 +
>  drivers/media/platform/Kconfig               |   1 +
>  drivers/media/platform/Makefile              |   1 +
>  drivers/media/platform/apple/Kconfig         |   5 +
>  drivers/media/platform/apple/Makefile        |   3 +
>  drivers/media/platform/apple/isp/Kconfig     |  16 +
>  drivers/media/platform/apple/isp/Makefile    |   3 +
>  drivers/media/platform/apple/isp/isp-cam.c   | 414 ++++++++++++
>  drivers/media/platform/apple/isp/isp-cam.h   |  21 +
>  drivers/media/platform/apple/isp/isp-cmd.c   | 635 +++++++++++++++++++
>  drivers/media/platform/apple/isp/isp-cmd.h   | 692 ++++++++++++++++++++
>  drivers/media/platform/apple/isp/isp-drv.c   | 586 +++++++++++++++++
>  drivers/media/platform/apple/isp/isp-drv.h   | 284 +++++++++
>  drivers/media/platform/apple/isp/isp-fw.c    | 770 +++++++++++++++++++++++
>  drivers/media/platform/apple/isp/isp-fw.h    |  24 +
>  drivers/media/platform/apple/isp/isp-iommu.c | 251 ++++++++
>  drivers/media/platform/apple/isp/isp-iommu.h |  20 +
>  drivers/media/platform/apple/isp/isp-ipc.c   | 258 ++++++++
>  drivers/media/platform/apple/isp/isp-ipc.h   |  25 +
>  drivers/media/platform/apple/isp/isp-regs.h  |  56 ++
>  drivers/media/platform/apple/isp/isp-v4l2.c  | 900 +++++++++++++++++++++++++++
>  drivers/media/platform/apple/isp/isp-v4l2.h  |  16 +
>  22 files changed, 4982 insertions(+)

quick partial review

> diff --git a/MAINTAINERS b/MAINTAINERS
> index dea7239ee0f5464b31efed5a2e0e5e602bcb6439..60517f7dcee14fc942dd3f77ed5d58eae394f7fa 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2248,6 +2248,7 @@ F:	drivers/i2c/busses/i2c-pasemi-platform.c
>  F:	drivers/iommu/apple-dart.c
>  F:	drivers/iommu/io-pgtable-dart.c
>  F:	drivers/irqchip/irq-apple-aic.c
> +F:	drivers/media/platform/apple/*
>  F:	drivers/nvme/host/apple.c
>  F:	drivers/nvmem/apple-efuses.c
>  F:	drivers/pinctrl/pinctrl-apple-gpio.c
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index 85d2627776b6a424fbd392187669535c4159ec97..ba75cfdb57f710cca086136e4524d3e1bc1910ac 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -65,6 +65,7 @@ config VIDEO_MUX
>  source "drivers/media/platform/allegro-dvt/Kconfig"
>  source "drivers/media/platform/amlogic/Kconfig"
>  source "drivers/media/platform/amphion/Kconfig"
> +source "drivers/media/platform/apple/Kconfig"
>  source "drivers/media/platform/aspeed/Kconfig"
>  source "drivers/media/platform/atmel/Kconfig"
>  source "drivers/media/platform/broadcom/Kconfig"
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index ace4e34483ddce6c3361479989086145dd495f29..e59e4259064bf04b718ea8d128031af859a13d2e 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -8,6 +8,7 @@
>  obj-y += allegro-dvt/
>  obj-y += amlogic/
>  obj-y += amphion/
> +obj-y += apple/
>  obj-y += aspeed/
>  obj-y += atmel/
>  obj-y += broadcom/
> diff --git a/drivers/media/platform/apple/Kconfig b/drivers/media/platform/apple/Kconfig
> new file mode 100644
> index 0000000000000000000000000000000000000000..f16508bff5242a0bc433bf8a1d8e3f29737d20d1
> --- /dev/null
> +++ b/drivers/media/platform/apple/Kconfig
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +comment "Apple media platform drivers"
> +
> +source "drivers/media/platform/apple/isp/Kconfig"
> diff --git a/drivers/media/platform/apple/Makefile b/drivers/media/platform/apple/Makefile
> new file mode 100644
> index 0000000000000000000000000000000000000000..d8fe985b0e6c377de6c77d30a3a796c40f3da116
> --- /dev/null
> +++ b/drivers/media/platform/apple/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +obj-y += isp/
> diff --git a/drivers/media/platform/apple/isp/Kconfig b/drivers/media/platform/apple/isp/Kconfig
> new file mode 100644
> index 0000000000000000000000000000000000000000..8e94962990031304d51cdd7cd6190b05b05b40bb
> --- /dev/null
> +++ b/drivers/media/platform/apple/isp/Kconfig
> @@ -0,0 +1,16 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +config VIDEO_APPLE_ISP
> +	tristate "Apple Silicon Image Signal Processor driver"
> +	select VIDEOBUF2_CORE
> +	select VIDEOBUF2_V4L2
> +	select VIDEOBUF2_DMA_SG
> +	depends on ARCH_APPLE || COMPILE_TEST
> +	depends on V4L_PLATFORM_DRIVERS
> +	depends on VIDEO_DEV

missing dependency on DRM for drm_mm. I don't remember why iova's top
down allocation was a problem but if we can avoid drm_mm that would be
an option as well.

> +	help
> +	  Say Y here to enable support for the ISP and cameras persent
> +	  in Apple ARM laptops.
> +
> +	  To compile this driver as a module, choose M here. The module will be
> +	  called apple_isp
> diff --git a/drivers/media/platform/apple/isp/Makefile b/drivers/media/platform/apple/isp/Makefile
> new file mode 100644
> index 0000000000000000000000000000000000000000..4649f32987f025a639945a37d774d4ecdc83b02a
> --- /dev/null
> +++ b/drivers/media/platform/apple/isp/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +apple-isp-y := isp-cam.o isp-cmd.o isp-drv.o isp-fw.o isp-iommu.o isp-ipc.o isp-v4l2.o
> +obj-$(CONFIG_VIDEO_APPLE_ISP) += apple-isp.o

...

> diff --git a/drivers/media/platform/apple/isp/isp-drv.c b/drivers/media/platform/apple/isp/isp-drv.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..b0c73b4f43d73f4ee29093fe62ed1d39ccfa33dd
> --- /dev/null
> +++ b/drivers/media/platform/apple/isp/isp-drv.c
> @@ -0,0 +1,586 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Apple Image Signal Processor driver
> + *
> + * Copyright (C) 2023 The Asahi Linux Contributors
> + */
> +
> +#include <linux/iommu.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/workqueue.h>
> +
> +#include "isp-cam.h"
> +#include "isp-fw.h"
> +#include "isp-iommu.h"
> +#include "isp-v4l2.h"
> +

...

> +static int apple_isp_init_iommu(struct apple_isp *isp)
> +{
> +	struct device *dev = isp->dev;
> +	phys_addr_t heap_base;
> +	size_t heap_size;
> +	u64 vm_size;
> +	int err;
> +	int size;
> +	struct device_node *mem_node;
> +	const __be32 *maps, *end;
> +
> +	isp->domain = iommu_get_domain_for_dev(isp->dev);
> +	if (!isp->domain)
> +		return -ENODEV;
> +	isp->shift = __ffs(isp->domain->pgsize_bitmap);
> +
> +	mem_node = of_parse_phandle(dev->of_node, "memory-region", 0);
> +	if (!mem_node) {
> +		dev_err(dev, "No memory-region found for heap\n");
> +		return -ENODEV;
> +	}
> +
> +	maps = of_get_property(mem_node, "iommu-addresses", &size);
> +	if (!maps || !size) {
> +		dev_err(dev, "No valid iommu-addresses found for heap\n");
> +		return -ENODEV;
> +	}
> +
> +	end = maps + size / sizeof(__be32);
> +
> +	while (maps < end) {
> +		maps++;
> +		maps = of_translate_dma_region(dev->of_node, maps, &heap_base,
> +					       &heap_size);
> +	}

The hand-rolled reserved memory parsing looks like it can be replaced
with of_iommu_get_resv_region();
> +

> +	isp->fw.heap_top = heap_base + heap_size;
> +
> +	err = of_property_read_u64(dev->of_node, "apple,dart-vm-size",
> +				   &vm_size);

This is not necessary and can be inferred from
isp->domain->geometry.aperture_{start,end}.

> +	if (err) {
> +		dev_err(dev, "failed to read 'apple,dart-vm-size': %d\n", err);
> +		return err;
> +	}
> +
> +	drm_mm_init(&isp->iovad, isp->fw.heap_top,
> +		    vm_size - (heap_base & 0xffffffff));

drm_mm probably should be replaced with something else. As I wrote above
I don't understand / remember what the problem was with relying on the
DMA api and iova. I can't imagine that the firmware cares about top-down
vs. bottom-up allocation. I could image it was related to
"apple,dart-vm-size" and iova's top down allocation only allocated
outside of the device's aperture. That should be solved by our current
downstream handling of the "vm-base" and "vm-size" properties from
Apple's devicetree.

ciao Janne

Re: [PATCH 3/5] media: dt-bindings: Add Apple ISP

[Janne Grunau]

On Wed, Feb 19, 2025 at 12:53:26PM +0200, Laurent Pinchart wrote:
> On Wed, Feb 19, 2025 at 10:54:31AM +0100, Sasha Finkelstein wrote:
> > On Wed, 19 Feb 2025 at 10:37, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > > +
> > > > +  apple,platform-id:
> > > > +    description: Platform id for firmware
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > >
> > >
> > > No, use firmware-name.
> > 
> > Not sure how is firmware-name an appropriate field, fw-name is a string
> > that references a firmware file, while this field is an id that is sent to the
> > coprocessor firmware in order to identify the platform.
> > 
> > > > +  apple,temporal-filter:
> > > > +    description: Whether temporal filter should be enabled in firmware
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > >
> > > And why is this not enabled always? Why this is board specific?
> > 
> > Not every board has support for this feature.
> > 
> > > You miss here ports or port. ISP usually gets signal from some camera or
> > > other block.
> > 
> > For complex cameras - yes, but this is closer to a UVC camera connected
> > via a bespoke protocol. We do not need to deal with the sensor access,
> > all of it is managed by the coprocessor firmware.
> > 
> > > > +        properties:
> > > > +          apple,config-index:
> > > > +            description: Firmware config index
> > > > +            $ref: /schemas/types.yaml#/definitions/uint32
> > >
> > >
> > > No duplicated indices. You have reg for this, assuming this is index.
> > 
> > There are duplicated indices, see isp-imx248.dtsi in patch 5 for an example.
> > 
> > > All these do not look like hardware properties but rather configuration
> > > of sensor which should be done runtime by OS, not by DT.
> > 
> > Those are board-specific and not discoverable via the ISP protocol.
> 
> But they are settable through the ISP protocol, aren't they ? For
> instance, looking at isp-imx248.dtsi, the first four entries are
> 
> 	/* 1280x720 */
> 	preset0 {
> 		apple,config-index = <0>;
> 		apple,input-size = <1296 736>;
> 		apple,output-size = <1280 720>;
> 		apple,crop = <8 8 1280 720>;
> 	};
> 
> 	/* 960x720 (4:3) */
> 	preset1 {
> 		apple,config-index = <0>;
> 		apple,input-size = <1296 736>;
> 		apple,output-size = <960 720>;
> 		apple,crop = <168 8 960 720>;
> 	};
> 
> 	/* 960x540 (16:9) */
> 	preset2 {
> 		apple,config-index = <0>;
> 		apple,input-size = <1296 736>;
> 		apple,output-size = <960 540>;
> 		apple,crop = <8 8 1280 720>;
> 	};
> 
> 	/* 640x480 (4:3) */
> 	preset3 {
> 		apple,config-index = <0>;
> 		apple,input-size = <1296 736>;
> 		apple,output-size = <640 480>;
> 		apple,crop = <168 8 960 720>;
> 	};
> 
> But I may be interested in capturing a 640x480 frame with cropping only
> and without scaling, with
> 
> input-size = 1296x736
> output-size = 640x480
> crop = (328,128)/640x480
> 
> Or I may want my cropped frame to be located in the upper-left corner:
> 
> input-size = 1296x736
> output-size = 640x480
> crop = (8,8)/640x480
> 
> If I set those parameters through the ISP protocol, won't it work ?

If my memory serves me right the presets wre added as workaround for
userspace not handling V4L2_FRMSIZE_TYPE_STEPWISE well (or at all) and
the added complexity of handling the qadratic sensor with partially
occluded or outside of the usable lens diameter corners.

It is a simplified description of the hardware to make it useable for
most software which is expected simple uvc cameras.

There are still two common issues in user space software related to this
driver:
- software expects width == linesize
- resolution selection is based frame height, i.e. it prefers 1080x1920
  over 1920x1080 on devices with quadratic sensor.

ciao Janne

Re: [PATCH 3/5] media: dt-bindings: Add Apple ISP

[Laurent Pinchart]

On Wed, Feb 19, 2025 at 12:05:29PM +0100, Sasha Finkelstein wrote:
> On Wed, 19 Feb 2025 at 11:53, Laurent Pinchart wrote:
> > >
> > > Those are board-specific and not discoverable via the ISP protocol.
> >
> > But they are settable through the ISP protocol, aren't they ? For
> > instance, looking at isp-imx248.dtsi, the first four entries are
> >
> >         /* 1280x720 */
> >         preset0 {
> >                 apple,config-index = <0>;
> >                 apple,input-size = <1296 736>;
> >                 apple,output-size = <1280 720>;
> >                 apple,crop = <8 8 1280 720>;
> >         };
> >
> >         /* 960x720 (4:3) */
> >         preset1 {
> >                 apple,config-index = <0>;
> >                 apple,input-size = <1296 736>;
> >                 apple,output-size = <960 720>;
> >                 apple,crop = <168 8 960 720>;
> >         };
> >
> >         /* 960x540 (16:9) */
> >         preset2 {
> >                 apple,config-index = <0>;
> >                 apple,input-size = <1296 736>;
> >                 apple,output-size = <960 540>;
> >                 apple,crop = <8 8 1280 720>;
> >         };
> >
> >         /* 640x480 (4:3) */
> >         preset3 {
> >                 apple,config-index = <0>;
> >                 apple,input-size = <1296 736>;
> >                 apple,output-size = <640 480>;
> >                 apple,crop = <168 8 960 720>;
> >         };
> >
> > But I may be interested in capturing a 640x480 frame with cropping only
> > and without scaling, with
> >
> > input-size = 1296x736
> > output-size = 640x480
> > crop = (328,128)/640x480
> >
> > Or I may want my cropped frame to be located in the upper-left corner:
> >
> > input-size = 1296x736
> > output-size = 640x480
> > crop = (8,8)/640x480
> >
> > If I set those parameters through the ISP protocol, won't it work ?
> 
> For cropping - you do not want to change those parameters, the sensor
> is partially occluded, and the crop area is specified in such a way
> to not expose those pixels.

Surely cropping *more* wouldn't be an issue from that point of view ?
The visible area is a device-specific property, so that could be
specified in DT, instead of presets. The problem isn't limited to this
device, so ideally the DT binding for this feature should be
standardized.

In general, I'd say that even occluded pixels should be exposed to
userspace, should an application want to capture them. If you think of
fisheye lenses, for instance, it's common to have a round image captured
from a rectangular sensor, and use a dewarping engine (it could just be
GPU shaders) to un-distord it. Restricting capture to rectangles of
fully visible pixels would result in loss of information. For this
particular device I don't think that would really be a use case, so I'm
not opposed to the driver setting restrictions on crop rectangles.

> As for scaling - we can expose only the 1:1
> scale and let userspace deal with it, but it appears that it expects
> the other common output sizes to exist.

Can the driver synthesize the list of scaled-down resolutions, instead
of specifying them in DT ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/5] media: dt-bindings: Add Apple ISP

[Laurent Pinchart]

On Wed, Feb 19, 2025 at 12:57:37PM +0100, Janne Grunau wrote:
> On Wed, Feb 19, 2025 at 12:53:26PM +0200, Laurent Pinchart wrote:
> > On Wed, Feb 19, 2025 at 10:54:31AM +0100, Sasha Finkelstein wrote:
> > > On Wed, 19 Feb 2025 at 10:37, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > > > +
> > > > > +  apple,platform-id:
> > > > > +    description: Platform id for firmware
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > >
> > > >
> > > > No, use firmware-name.
> > > 
> > > Not sure how is firmware-name an appropriate field, fw-name is a string
> > > that references a firmware file, while this field is an id that is sent to the
> > > coprocessor firmware in order to identify the platform.
> > > 
> > > > > +  apple,temporal-filter:
> > > > > +    description: Whether temporal filter should be enabled in firmware
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > >
> > > > And why is this not enabled always? Why this is board specific?
> > > 
> > > Not every board has support for this feature.
> > > 
> > > > You miss here ports or port. ISP usually gets signal from some camera or
> > > > other block.
> > > 
> > > For complex cameras - yes, but this is closer to a UVC camera connected
> > > via a bespoke protocol. We do not need to deal with the sensor access,
> > > all of it is managed by the coprocessor firmware.
> > > 
> > > > > +        properties:
> > > > > +          apple,config-index:
> > > > > +            description: Firmware config index
> > > > > +            $ref: /schemas/types.yaml#/definitions/uint32
> > > >
> > > >
> > > > No duplicated indices. You have reg for this, assuming this is index.
> > > 
> > > There are duplicated indices, see isp-imx248.dtsi in patch 5 for an example.
> > > 
> > > > All these do not look like hardware properties but rather configuration
> > > > of sensor which should be done runtime by OS, not by DT.
> > > 
> > > Those are board-specific and not discoverable via the ISP protocol.
> > 
> > But they are settable through the ISP protocol, aren't they ? For
> > instance, looking at isp-imx248.dtsi, the first four entries are
> > 
> > 	/* 1280x720 */
> > 	preset0 {
> > 		apple,config-index = <0>;
> > 		apple,input-size = <1296 736>;
> > 		apple,output-size = <1280 720>;
> > 		apple,crop = <8 8 1280 720>;
> > 	};
> > 
> > 	/* 960x720 (4:3) */
> > 	preset1 {
> > 		apple,config-index = <0>;
> > 		apple,input-size = <1296 736>;
> > 		apple,output-size = <960 720>;
> > 		apple,crop = <168 8 960 720>;
> > 	};
> > 
> > 	/* 960x540 (16:9) */
> > 	preset2 {
> > 		apple,config-index = <0>;
> > 		apple,input-size = <1296 736>;
> > 		apple,output-size = <960 540>;
> > 		apple,crop = <8 8 1280 720>;
> > 	};
> > 
> > 	/* 640x480 (4:3) */
> > 	preset3 {
> > 		apple,config-index = <0>;
> > 		apple,input-size = <1296 736>;
> > 		apple,output-size = <640 480>;
> > 		apple,crop = <168 8 960 720>;
> > 	};
> > 
> > But I may be interested in capturing a 640x480 frame with cropping only
> > and without scaling, with
> > 
> > input-size = 1296x736
> > output-size = 640x480
> > crop = (328,128)/640x480
> > 
> > Or I may want my cropped frame to be located in the upper-left corner:
> > 
> > input-size = 1296x736
> > output-size = 640x480
> > crop = (8,8)/640x480
> > 
> > If I set those parameters through the ISP protocol, won't it work ?
> 
> If my memory serves me right the presets wre added as workaround for
> userspace not handling V4L2_FRMSIZE_TYPE_STEPWISE well (or at all) and
> the added complexity of handling the qadratic sensor with partially
> occluded or outside of the usable lens diameter corners.
> 
> It is a simplified description of the hardware to make it useable for
> most software which is expected simple uvc cameras.

I understand that. Ideally userspace should be fixed, but in the
meantime, I'm fine with the driver exposing a set of presets. They
should however not be listed in DT, but computed by the driver based on
properties that describe the device (such as, for instance, the full
sensor resolution, or the visible area). Those properties could come
from DT, or be hardcoded in the driver on a per-compatible basis.

> There are still two common issues in user space software related to this
> driver:
> - software expects width == linesize
> - resolution selection is based frame height, i.e. it prefers 1080x1920
>   over 1920x1080 on devices with quadratic sensor.

I share your pain, having to fix userspace when doing kernel work isn't
always fun :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/5] media: dt-bindings: Add Apple ISP

[Asahi Lina]

On February 19, 2025 12:05:29 PM GMT+01:00, Sasha Finkelstein <fnkl.kernel@gmail.com> wrote:
>On Wed, 19 Feb 2025 at 11:53, Laurent Pinchart
><laurent.pinchart@ideasonboard.com> wrote:
>> >
>> > Those are board-specific and not discoverable via the ISP protocol.
>>
>> But they are settable through the ISP protocol, aren't they ? For
>> instance, looking at isp-imx248.dtsi, the first four entries are
>>
>>         /* 1280x720 */
>>         preset0 {
>>                 apple,config-index = <0>;
>>                 apple,input-size = <1296 736>;
>>                 apple,output-size = <1280 720>;
>>                 apple,crop = <8 8 1280 720>;
>>         };
>>
>>         /* 960x720 (4:3) */
>>         preset1 {
>>                 apple,config-index = <0>;
>>                 apple,input-size = <1296 736>;
>>                 apple,output-size = <960 720>;
>>                 apple,crop = <168 8 960 720>;
>>         };
>>
>>         /* 960x540 (16:9) */
>>         preset2 {
>>                 apple,config-index = <0>;
>>                 apple,input-size = <1296 736>;
>>                 apple,output-size = <960 540>;
>>                 apple,crop = <8 8 1280 720>;
>>         };
>>
>>         /* 640x480 (4:3) */
>>         preset3 {
>>                 apple,config-index = <0>;
>>                 apple,input-size = <1296 736>;
>>                 apple,output-size = <640 480>;
>>                 apple,crop = <168 8 960 720>;
>>         };
>>
>> But I may be interested in capturing a 640x480 frame with cropping only
>> and without scaling, with
>>
>> input-size = 1296x736
>> output-size = 640x480
>> crop = (328,128)/640x480
>>
>> Or I may want my cropped frame to be located in the upper-left corner:
>>
>> input-size = 1296x736
>> output-size = 640x480
>> crop = (8,8)/640x480
>>
>> If I set those parameters through the ISP protocol, won't it work ?
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
>
>For cropping - you do not want to change those parameters, the sensor
>is partially occluded, and the crop area is specified in such a way
>to not expose those pixels. As for scaling - we can expose only the 1:1
>scale and let userspace deal with it, but it appears that it expects
>the other common output sizes to exist.
>
>

The square sensor with the hack is the IMX558. The configs there are actually a workaround for missing ANE support.

What the driver is supposed to be doing is using the higher numbered presets, which crop internally and exclude the occluded area. But those on machines with that sensor end up requiring ANE integration for denoising and ISP crashes without it.

Config #0 doesn't use ANE, so the crops are re-creations of the other configs so it behaves the same minus the denoising.

Re: [PATCH 1/5] dt-bindings: power: apple,pmgr-pwrstate: Add force-{disable,reset} properties

[Rob Herring]

On Wed, Feb 19, 2025 at 10:43:17AM +0100, Sasha Finkelstein wrote:
> On Wed, 19 Feb 2025 at 10:34, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > On 19/02/2025 10:26, Sasha Finkelstein via B4 Relay wrote:
> > > From: Sasha Finkelstein <fnkl.kernel@gmail.com>
> > >
> > > Add properties to set disable/reset bits when powering down
> > > certain domains
> >
> >
> > Please explain why and what problem are you solving. This looks too
> > close to SW policy or really arbitrary choice. Background would be useful.
> 
> The ISP block has some weird requirements where some of it's power domains
> will not power down correctly without using the "force disable" or "force reset"
> pmgr feature. Basically a hardware quirk.

Add that to the commit message.

Rob

Re: [PATCH 4/5] media: apple: Add Apple ISP driver

[Sasha Finkelstein]

On Wed, 19 Feb 2025 at 12:34, Janne Grunau <j@jannau.net> wrote:
> > +     while (maps < end) {
> > +             maps++;
> > +             maps = of_translate_dma_region(dev->of_node, maps, &heap_base,
> > +                                            &heap_size);
> > +     }
>
> The hand-rolled reserved memory parsing looks like it can be replaced
> with of_iommu_get_resv_region();

I have looked into it, and `of_iommu_get_resv_region` does the wrong
thing. We fill out `reg`, and it grabs that instead of `iommu-addresses`.

Re: [PATCH 3/5] media: dt-bindings: Add Apple ISP

[Krzysztof Kozlowski]

On 19/02/2025 10:54, Sasha Finkelstein wrote:
> On Wed, 19 Feb 2025 at 10:37, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>> +
>>> +  apple,platform-id:
>>> +    description: Platform id for firmware
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>
>>
>> No, use firmware-name.
> 
> Not sure how is firmware-name an appropriate field, fw-name is a string
> that references a firmware file, while this field is an id that is sent to the
> coprocessor firmware in order to identify the platform.

You get advice how you explain the hardware. Why do you need to identify
the platform? Compatible already tells that. Who identifies the
platform? For what purpose?


> 
>>> +  apple,temporal-filter:
>>> +    description: Whether temporal filter should be enabled in firmware
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>
>> And why is this not enabled always? Why this is board specific?
> 
> Not every board has support for this feature.

Board for the same SoC, right?  It's fine then, but you should explain
all this in binding description.


> 
>> You miss here ports or port. ISP usually gets signal from some camera or
>> other block.
> 
> For complex cameras - yes, but this is closer to a UVC camera connected
> via a bespoke protocol. We do not need to deal with the sensor access,
> all of it is managed by the coprocessor firmware.

And there is nothing more in this pipeline, like some other processing
units? It's fine then, but you should explain all this in binding
description.

> 
>>> +        properties:
>>> +          apple,config-index:
>>> +            description: Firmware config index
>>> +            $ref: /schemas/types.yaml#/definitions/uint32
>>
>>
>> No duplicated indices. You have reg for this, assuming this is index.
> 
> There are duplicated indices, see isp-imx248.dtsi in patch 5 for an example.

I still do no understand their point and description does not help me much.

> 
>> All these do not look like hardware properties but rather configuration
>> of sensor which should be done runtime by OS, not by DT.
> 
> Those are board-specific and not discoverable via the ISP protocol.

No, I rather suggest these should be runtime or deduced from the
compatible of the sensor. You know embed the sensor characteristics into
DT... or if not sensor then some user choice?


Best regards,
Krzysztof

Re: [PATCH 4/5] media: apple: Add Apple ISP driver

[Janne Grunau]

On Sun, Feb 23, 2025 at 09:48:30AM +0100, Sasha Finkelstein wrote:
> On Wed, 19 Feb 2025 at 12:34, Janne Grunau <j@jannau.net> wrote:
> > > +     while (maps < end) {
> > > +             maps++;
> > > +             maps = of_translate_dma_region(dev->of_node, maps, &heap_base,
> > > +                                            &heap_size);
> > > +     }
> >
> > The hand-rolled reserved memory parsing looks like it can be replaced
> > with of_iommu_get_resv_region();
> 
> I have looked into it, and `of_iommu_get_resv_region` does the wrong
> thing. We fill out `reg`, and it grabs that instead of `iommu-addresses`.

Downstream commit 704ace01cd3c4 [0] ("iommu: Add IOMMU_RESV_TRANSLATED
for non 1:1 mapped reserved regions") adds device virtual addresses to
struct iommu_resv_region. Sorry for the added dependency but it is
required anyway for the isp to work.

ciao Janne

[0] https://github.com/AsahiLinux/linux/commit/704ace01cd3c423b1e2492f0777d9c4c0f3404d8