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

On Wed, Feb 19, 2025 at 10:54:31AM +0100, Sasha Finkelstein wrote:

On Wed, 19 Feb 2025 at 10:37, Krzysztof Kozlowski [off-list ref] wrote:

quoted

quoted

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

quoted

quoted

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

quoted

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.

quoted

quoted

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

quoted

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