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

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

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