Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
* Re: [RFC] How to report the supported camera sensor resolutions (was "[libcamera-devel] [PATCH] libcamera: v4l2_subdevice: use G_FORMAT if ENUM_FRAME_SIZE isn't implemented")
       [not found]     ` <CAPY8ntCPJQdBzATpDRJcLyMH7bV56gO-WZjvmX2eGQ0NnJZBVA@mail.gmail.com>
@ 2019-11-26 16:44       ` Laurent Pinchart
  0 siblings, 0 replies; only message in thread
From: Laurent Pinchart @ 2019-11-26 16:44 UTC (permalink / raw)
  To: Dave Stevenson; +Cc: Andrey Konovalov, libcamera-devel, linux-media

Hello,

CC'ing the linux-media mailing list

On Wed, Nov 13, 2019 at 10:59:16AM +0000, Dave Stevenson wrote:
> On Tue, 12 Nov 2019 at 20:22, Andrey Konovalov wrote:
> > On 09.11.2019 13:50, Jacopo Mondi wrote:
> >> On Fri, Oct 25, 2019 at 04:32:08PM +0300, Andrey Konovalov wrote:
> >>> Many camera sensor drivers support only one frame size, and don't
> >>> implement VIDIOC_SUBDEV_ENUM_FRAME_SIZE ioctl. And libcamera fails
> >
> > The key point here is VIDIOC_SUBDEV_ENUM_FRAME_SIZE support (see below)
> >
> >> Is this true ? A single frame size?
> > Well, not quite
> >
> >> Do you see this often ?
> >
> > I do see this as one of the cameras I currently use is ov5647, and the mainline driver for it supports 640x480 only :)
> > (and this might explain why it doesn't implement VIDIOC_SUBDEV_ENUM_FRAME_SIZE).
> 
> Somehow I guessed it might be ov5647, aka Raspberry Pi v1.3 camera module.
> VGA raw8 only, even though the sensor is 5MPix and supports raw 10.
> 
> Then again that driver needs a bit of love as the mainline driver
> doesn't support the set_fmt or get_fmt in the pad ops either - I've
> got a patch to add those on the downstream Pi kernel which ought to be
> upstreamed [1].
> Register sets are available for other resolutions, so it'd be nice to extend it.
> 
> So I'll agree with Andrey's point that there are some drivers that
> don't currently support VIDIOC_SUBDEV_ENUM_FRAME_SIZE, but also with
> Jacopo that drivers really ought to implement a minimum set of subdev
> functions. The list of exactly which functions ought to be documented,
> and probably includes VIDIOC_SUBDEV_ENUM_FRAME_SIZE.
> I asked the question over mandatory subdev ops on linux-media a couple
> of years back [2]. I had a response from Sakari on the subject, but no
> conclusions.

I think we really should address the problem on the kernel side first
and decide whether VIDIOC_SUBDEV_ENUM_FRAME_SIZE should be mandatory. If
it should, we should fix the kernel side, and we could possibly do so
quite simply with a default implementation of the ioctl that would just
return a single size when the subdev driver doesn't implement the
corresponding operation. How exactly this should be done remains to be
investigated, one option is to default to get_fmt within the kernel,
another option would be to store the single size somewhere in the
v4l2_subdev structure. Other options may be possible.

If we decide that the ioctl shouldn't be mandatory, libcamera should
then default to a different way of querying the sensor capabilities.
This patch (or the new version proposed below) is one option, another
one would be to set a 1x1 TRY format and use what the driver returns.
Here again other option may be possible.

Please see below for additional comments.

> [1] https://github.com/raspberrypi/linux/commit/031525b8e40f342f7842bc69f18bc613d659b0fc
> [2] https://www.spinics.net/lists/linux-media/msg115550.html
> 
> > But there are quite a few sensor drivers which don't support VIDIOC_SUBDEV_ENUM_FRAME_SIZE even though they support several
> > frame sizes.
> > I put them into two groups:
> >
> > 1) Have a small set of fixed sizes:
> >    drivers/media/i2c/ov5647.c (only have 640x480) - 5MP
> >    drivers/media/i2c/imx274.c (3 fixed frame sizes)
> >    drivers/media/i2c/noon010pc30.c (3 fixed frame sizes)
> >    drivers/media/i2c/sr030pc30.c (3 fixed resolutions - 640x480 plus 320x240 and 160x120 (x2 and x4 subsampling))
> >    drivers/media/i2c/tc358743.c (HDMI to CSI-2 bridge; has [set|get]_fmt, but the resolution is "fixed" to what it is getting
> >      from HDMI if I get it correct)
> >
> > 2) Use cropping to set any resolution matching the limits and the restrictions
> > (e.g. height and width might have to be even numbers):
> >    drivers/media/i2c/mt9m001.c (any frame size from 48x32 to 1280x1024 with 2 pixels step) - 1.3 MP
> >    drivers/media/i2c/mt9m111.c (quite similar to the one above) - 1.3 MP
> >    drivers/media/i2c/mt9t112.c (similar to the one above) - 2MP
> >    drivers/media/i2c/mt9v011.c (uses cropping to fit to the resolution requested; doesn't use selection API)
> >    drivers/media/i2c/ov2640.c (uses cropping to set a set of standard resolutions; has get_selection, but not set_selection)
> >    drivers/media/i2c/ov6650.c (uses selection API to crop the image to fit the requested resolution;
> >                             support half scale (QCIF) and full scale (CIF))
> >    drivers/media/i2c/ov7640.c (empty v4l2_subdev_ops)
> >    drivers/media/i2c/ov772x.c (similar to ov2640.c)
> >    drivers/media/i2c/ov9640.c (has a set of 7 standard resolutions; has get_selection, but not set_selection) - 1.3 MP
> >    drivers/media/i2c/rj54n1cb0c.c (uses cropping, and selection API)
> >    drivers/media/i2c/s5k4ecgx.c (4 fixed "preview" - up to 720x480 - resolutions) - 5MP
> >    drivers/media/i2c/s5k6a3.c (?)
> >    drivers/media/i2c/vs6624.c (supports quite a few resolutions) - 1.3 MP
> >
> >>> to enumerate such cameras as it doesn't get the list of resolutions
> >>> the sensor driver supports.
> >
> > - here. libcamera ignores cameras which don't implement VIDIOC_SUBDEV_ENUM_FRAME_SIZE.
> > And there are quite a few of such sensors.
> >
> >>> Fix this by using VIDIOC_SUBDEV_G_FORMAT
> >>> to get the size of the sensor's active format when the ENUM_FRAME_SIZE
> >>> ioctl isn't implemented by the sensor driver.
> >>>
> >>
> >> As a general note, personally, I would prefer drivers to implement the
> >> proper ioctls, and I think we should not provide incentives to work
> >> this around. But if this fixes a real issue with several drivers I
> >> think we could consider this. What do others think here?
> >
> > Again, my real issue is that libcamera uses VIDIOC_SUBDEV_ENUM_FRAME_SIZE to get the list
> > of the resolutions supported by sensor. If sensor responds to this ioctl with -ENOTTY,
> > libcamera creates an empty list of supported resolutions for this sensor and refuses to use it.
> > Here is the call sequence to "enumerate all media bus codes and frame sizes supported by the
> > subdevice on a pad":
> > V4L2Subdevice::formats() -> V4L2Subdevice::enumPadSizes() -> ioctl(VIDIOC_SUBDEV_ENUM_FRAME_SIZE, &sizeEnum))
> >
> > I am not sure if implementing VIDIOC_SUBDEV_ENUM_FRAME_SIZE is a must for every camera sensor;
> > maybe the most from the 2) group above don't really need it.
> > But then it would be good for all the camera sensors to be recognized by libcamera - whether
> > they implement VIDIOC_SUBDEV_ENUM_FRAME_SIZE or not, correct?
> >
> > On a different matter:
> >
> > My current feeling about VIDIOC_SUBDEV_ENUM_FRAME_SIZE is that it might be useful to distinguish
> > between:
> > 1) the resolutions which use the full area of the sensor, have the same field of view, but
> > differ in pixel density (use binning, or subsampling),
> > and 2) the lower resolutions obtained by cropping when only a part of the sensor is used.
> > In this sense e.g. mt9t112.c supports single frame size of 2048x1536, and can use cropping to
> > provide any resolution below 2048x1536.
> >
> > In other words, VIDIOC_SUBDEV_ENUM_FRAME_SIZE could list maximum resolutions the sensor supports
> > (like "16:9 Hi Res", "4:3 Hi Res", "16:9 Low Res", etc - with the same field of view for the same
> > aspect ratio regardless of Hi or Low Res). Versus a (kind of continuous) range of resolutions
> > implemented by cropping which is rather a "digital zoom" in my understanding.

VIDIOC_SUBDEV_ENUM_FRAME_SIZE has been implemented to mimic the related
VIDIOC_ENUM_FRAME_SIZE, which was itself specified to support UVC
devices that expose a set of resolutions without exposing explicing
cropping or scaling. I'd go as far as saying that it was a bit of a
historical mistake for sensors, as we really want drivers that support
explicit binning, skipping and cropping instead of a limited set of
hardcoded resolutions. Of course the majority of sensor drivers just
have a hardcoded list of register settings corresponding to one or a few
modes, and that's what we have to live with :-(

A sensor driver that supports explicit binning/skipping and cropping
should in my opinion report its full size, either through
VIDIOC_SUBDEV_ENUM_FRAME_SIZE (if we decice that the ioctl should be
mandatory), or through the selection API (I think Jacopo has posted a
patch not too long ago to clarify the selection rectangles in that
respect). Cropping and binning/skipping should then be applied on top.
For sensor drivers that support a fixed list of modes,
VIDIOC_SUBDEV_ENUM_FRAME_SIZE should be mandatory.

There's an additional question of how sensors should report their
skipping/binning capabilities. Some sensor drivers currently do so
through VIDIOC_SUBDEV_ENUM_FRAME_SIZE by reporting all the sizes
resulting from skipping/binning the full frame, with cropping being
applied separately. We could decide this is a good API and generalize
it, or pick a different approach.

> > If a sensor has the resolution of e.g. 640x480, the driver and the sensor support both 2x binning and
> > cropping, and the application requests the resolution of 320x240 - how the sensor driver could deduce
> > if the application asks to turn the binning on, or to use the 1/4 of the sensor area (both cases are
> > valid, but are different requests)?

If userspace issues a set_fmt(320x240), the sensor should use binning.
If userspace issues a set_fmt(640x480) and a set_selection(160,120,
320x240) then the sensor should crop to a 320x240 rectangle in the
center of the field of view without binning.

> >>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> >>> ---
> >>>   src/libcamera/v4l2_subdevice.cpp | 20 +++++++++++++++++++-
> >>>   1 file changed, 19 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> >>> index f2bcd7f..fac7586 100644
> >>> --- a/src/libcamera/v4l2_subdevice.cpp
> >>> +++ b/src/libcamera/v4l2_subdevice.cpp
> >>> @@ -317,7 +317,25 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
> >>>                                 sizeEnum.max_width, sizeEnum.max_height);
> >>>      }
> >>>
> >>> -    if (ret < 0 && ret != -EINVAL && ret != -ENOTTY) {
> >>> +    if (ret == -ENOTTY) {
> >>> +            struct v4l2_subdev_format format = {};
> >>> +
> >>> +            LOG(V4L2, Debug)
> >>> +                    << "VIDIOC_SUBDEV_ENUM_FRAME_SIZE not implemented "
> >>> +                    << "- trying VIDIOC_SUBDEV_G_FMT";
> >>> +            format.pad = pad;
> >>> +            format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >>> +
> >>> +            ret = ioctl(VIDIOC_SUBDEV_G_FMT, &format);
> >>> +            if (ret == 0) {
> >>> +                    sizes.emplace_back(format.format.width,
> >>> +                                       format.format.height,
> >>> +                                       format.format.width,
> >>> +                                       format.format.height);
> >>
> >> However, I don't think this works... ENUM_FRAME_SIZE has a .code field
> >> in struct v4l2_subdev_frame_size_enum, and enumerates sizes for that
> >> specific mbus code. As you can see the 'sizes' vector this operations
> >> returns is associated with said mbus code in the caller's 'formats'
> >> map.
> >>
> >> G_FMT returns the current format regardless of the mbus code, so if we
> >> want to work around faulty drivers using this approach, I would do
> >> that in the caller, and adjust the 'formats' map, not only the here
> >> enumerated sizes.
> >
> > Good point!
> > I'll try something like that in v2 of this patch:
> >
> > -----8<-----
> > --- a/src/libcamera/v4l2_subdevice.cpp
> > +++ b/src/libcamera/v4l2_subdevice.cpp
> > @@ -166,8 +166,13 @@ ImageFormats V4L2Subdevice::formats(unsigned int pad)
> >
> >          for (unsigned int code : enumPadCodes(pad)) {
> >                  std::vector<SizeRange> sizes = enumPadSizes(pad, code);
> > -               if (sizes.empty())
> > -                       return {};
> > +               if (sizes.empty()) {
> > +                       /* try S_FMT(which = V4L2_SUBDEV_FORMAT_TRY,
> > +                        *           format.width and format.height big enough,
> > +                        *           format.code = code)
> > +                        * if it succeeds, add the returned data to sizes
> > +                        * else return {}; */
> > +               }
> >
> >                  if (formats.addFormat(code, sizes)) {
> > -----8<-----
> >
> >>> +            }
> >>> +    }
> >>> +
> >>> +    if (ret < 0 && ret != -EINVAL) {
> >>>              LOG(V4L2, Error)
> >>>                      << "Unable to enumerate sizes on pad " << pad
> >>>                      << ": " << strerror(-ret);

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] only message in thread

only message in thread, back to index

Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20191025133208.22637-1-andrey.konovalov@linaro.org>
     [not found] ` <20191109105008.zezdel3b2ivv4uwd@uno.localdomain>
     [not found]   ` <863c5bd8-2110-b753-b43f-bf885f403a9b@linaro.org>
     [not found]     ` <CAPY8ntCPJQdBzATpDRJcLyMH7bV56gO-WZjvmX2eGQ0NnJZBVA@mail.gmail.com>
2019-11-26 16:44       ` [RFC] How to report the supported camera sensor resolutions (was "[libcamera-devel] [PATCH] libcamera: v4l2_subdevice: use G_FORMAT if ENUM_FRAME_SIZE isn't implemented") Laurent Pinchart

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git