Hi Laurent Sorry for my late reply here - I've been off sick for the last week. On Tue, 12 Apr 2022 at 10:20, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > > Hi Dave, > > (CC'ing Sakari) > > Sorry for the late reply. This is the kind of topic where I always hope > someone will beat me to it and I won't have to reply :-) > > On Tue, Apr 05, 2022 at 12:37:56PM +0100, Dave Stevenson via libcamera-devel wrote: > > On Thu, 17 Mar 2022 at 17:47, Dave Stevenson wrote: > > > > > > Hi All > > > > > > I'm trying to tally the selection API documentation for image sensors > > > with implementing it in practice, specifically over arbitrary cropping > > > on the sensor. > > > > > > I've had a downstream PR for IMX219 that adds support for the > > > selection API to allow arbitrary cropping and selection of binning > > > mode [1]. > > > > > > The docs for "Writing camera sensor drivers" [2] lists the two options > > > as either freely configurable via multiple subdevices, or register > > > based. It doesn't apparently cover just cropping (there is no scaler > > > on IMX219), but there is the IMX274 driver that implements setting the > > > sensor cropping via the selection API [3]. > > > > > > The current IMX219 register-based modes are > > > - 3280x2464 up to 15fps > > > - 1920x1080 up to 30fps as a crop of the 3280x2464 mode > > > - 1640x1232 up to 40fps, 2x2 binned > > > - 640x480 up to 200fps, "special" 2x2 binning and cropped. > > > > > > The main issue is that implementing the selection API reduces the > > > number of modes that can be selected directly via set_fmt to the base > > > 3280x2464 and 1640x1232. Surely that constitutes a regression as use > > > cases that did work now don't, and therefore it is not acceptable. > > > 3280x2464 can't run at 30fps, therefore we can't easily get a 1080p30 > > > source without additional knowledge of modes and crop settings. > > > > > > So how should the selection API be implemented without introducing regressions? > > > Is it permitted to enumerate the extra modes as before and have them > > > update the crop rectangle? The docs [4] say not: > > > "Drivers shall set the active crop rectangle to the default when the > > > driver is first loaded, but not later." > > > which leaves a bit of a quandry. > > > > > > If we do drop the existing modes it just pushes the problem of which > > > modes to select onto the client. Most likely you end up with an > > > extended sensor specific helper in libcamera with a list of modes and > > > the framerates that each can achieve, pretty much identical to the > > > list of modes in the kernel at present. > > > Any other clients are forced to jump through similar hoops (unlikely > > > to happen), or we rename it to Video 4 Libcamera 2 ;-) > > > > > > Making that shift also means that selecting the special binning mode > > > has to be done via some other heuristics. AIUI it's optimised for high > > > frame rates so that's possible (but not nice). > > > > > > Guidance sought please. > > Good description of a complex problem. I'm afraid I have no ready-made > answer that will solve all issues. If we were to forget about history > and API compatibility, I'm sure we could design a decent API to expose > the capabilities of camera sensors to userspace. Unfortunately, we > can't, but none of the options I can think of will preserve the API. > > I'm tempted to design something decent going forward, and move older > drivers to the new API over time as needed, with backward-compatibility > handled as a module option or even by forking and duplicating drivers. > It's messy, but keeping the status quo is even worse, we have lots of > drivers that expose the same features in many different ways. It's > becoming impossible to handle in any decent way in userspace. That seems like a longer term proposition. It sounds like converting to use the selection API largely isn't possible at present, so I'll keep these as downstream patches, and allow S_FMT to update the crop. Thanks Dave > > A gentle ping on this. > > > > At present I'm going to ignore the spec and allow setting the format > > to update the crop. That way we retain the modes and avoid regression, > > but allow those who wish to set an arbitrary selection to do so. > > > > Dave > > > > > Thanks > > > Dave > > > > > > [1] https://github.com/raspberrypi/linux/pull/4935 > > > [2] https://www.kernel.org/doc/html/latest/driver-api/media/camera-sensor.html#frame-size > > > [3] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/imx274.c > > > [4] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/selection-api-configuration.html#configuration-of-video-capture > > -- > Regards, > > Laurent Pinchart
