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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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