| Message ID | 20191025133208.22637-1-andrey.konovalov@linaro.org |
|---|---|
| State | Rejected |
| Headers | show |
| Series |
|
| Related | show |
Hello Andrey, thanks for the patch 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 Is this true ? A single frame size? Do you see this often ? > to enumerate such cameras as it doesn't get the list of resolutions > the sensor driver supports. 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? > 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. Thanks j > + } > + } > + > + if (ret < 0 && ret != -EINVAL) { > LOG(V4L2, Error) > << "Unable to enumerate sizes on pad " << pad > << ": " << strerror(-ret); > -- > 2.17.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On 09.11.2019 13:50, Jacopo Mondi wrote: > Hello Andrey, > thanks for the patch > > 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). 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. 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)? >> 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<----- Thanks, Andrey > Thanks > j > >> + } >> + } >> + >> + if (ret < 0 && ret != -EINVAL) { >> LOG(V4L2, Error) >> << "Unable to enumerate sizes on pad " << pad >> << ": " << strerror(-ret); >> -- >> 2.17.1 >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel
Hi Andrey On Tue, 12 Nov 2019 at 20:22, Andrey Konovalov <andrey.konovalov@linaro.org> wrote: > > Hi Jacopo, > > On 09.11.2019 13:50, Jacopo Mondi wrote: > > Hello Andrey, > > thanks for the patch > > > > 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. Dave [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. > > 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)? > > >> 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<----- > > Thanks, > Andrey > > > Thanks > > j > > > >> + } > >> + } > >> + > >> + if (ret < 0 && ret != -EINVAL) { > >> LOG(V4L2, Error) > >> << "Unable to enumerate sizes on pad " << pad > >> << ": " << strerror(-ret); > >> -- > >> 2.17.1 > >> > >> _______________________________________________ > >> libcamera-devel mailing list > >> libcamera-devel@lists.libcamera.org > >> https://lists.libcamera.org/listinfo/libcamera-devel > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Andrey, On Tue, Nov 12, 2019 at 11:22:04PM +0300, Andrey Konovalov wrote: > Hi Jacopo, > > On 09.11.2019 13:50, Jacopo Mondi wrote: > > Hello Andrey, > > thanks for the patch > > > > 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). > > 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 > Thanks for checking > > > 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. I would say the opposite. Most drivers in 2) shold report the min and max resolutions, and from there you could obtain the desired one by cropping/scaling, if I'm not mistaken. > 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? > As I've said, I'm not entirely sure about this, as we by using get_format as you proposed doesn't really give you back the same information as ENUM_FRAME_SIZE does. > > 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. what you're here hitting is limitation in the v4l2 specification and in the average sensor driver implementation, which relies on discrete resolutions as it can often just use a set of pre-defined register values provided by the vendors 'known to work' for that specific mode. Ideally, ENUM_FRAME_SIZE should report the full pixel array available for a media bus format, and you could then get all the intermediate resolutions from there. This does very seldom happens though. > > 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. I feel like you should get the first one from ENUM_FRAME_SIZE and the possible cropping/scaling rectangles through the selection APIs. I'm not sure how many drivers actually do so. > > 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)? > There's people way more expert then me that could answer that, personally I would say that you currently can't and it really depends on the driver implementation to decide that. I would suggest you to turn to linux-media and Cc Sakari, who surely knows better than me on this topic :) Back on libcamera releated issues: I'm not sure we can actually work around a missing ENUM_FRAME_SIZE, and I'm not sure it could be done with G_FMT. My suggestion at the moment would be: patch the faulty driver, it's a simple additional subdev op that report a single value, it shouldn't be hard. Maybe others have different opinions though (maybe let's wait for Laurent to come back and give his opinion too). Thanks j > > > 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<----- > > Thanks, > Andrey > > > Thanks > > j > > > > > + } > > > + } > > > + > > > + if (ret < 0 && ret != -EINVAL) { > > > LOG(V4L2, Error) > > > << "Unable to enumerate sizes on pad " << pad > > > << ": " << strerror(-ret); > > > -- > > > 2.17.1 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel
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);
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); + } + } + + if (ret < 0 && ret != -EINVAL) { LOG(V4L2, Error) << "Unable to enumerate sizes on pad " << pad << ": " << strerror(-ret);
Many camera sensor drivers support only one frame size, and don't implement VIDIOC_SUBDEV_ENUM_FRAME_SIZE ioctl. And libcamera fails to enumerate such cameras as it doesn't get the list of resolutions the sensor driver supports. 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. Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org> --- src/libcamera/v4l2_subdevice.cpp | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)