[libcamera-devel] [PATCH] libcamera: v4l2_subdevice: Fix formats() returning only sizes for last mbus code

[Jacopo Mondi]

Hi Marian,

On Thu, Mar 11, 2021 at 12:15:48PM +0100, Marian Cichy wrote:
>
>
> On 3/11/21 9:16 AM, Jacopo Mondi wrote:
> > Hello Marian,
> >
> > On Tue, Mar 09, 2021 at 03:44:57PM +0100, Marian Cichy wrote:
> > > V4L2Subdevice::formats() wants to enumerate all media bus codes and
> > > frame sizes on a pad, however, the current implementation always
> > > overwrites the SizeRange of the last mbus code, effectively returning
> > > only this SizeRange.
> > >
> > > This can be even fatal, as camera_sensor is using this function to
> > > enumerate all codes and sizes. If the last mbus code has no SizeRange
> > > for whatever reason, the camera_sensor will error out, claiming that it
> > > didn't found any image formats at all.
> > What is the use case for a driver returning an mbus code without any
> > image size associated ? Smells like a driver bug to me.
>
> The driver I am using has a code which is only used by the hardware-internal
> test pattern generator and it doesn't return a size for this code. It might
> still very well a driver bug though.
>

I see. It might be a valid use case, even if test patterns have a size,
doesn't they ?

> > > Signed-off-by: Marian Cichy <m.cichy at pengutronix.de>
> > > ---
> > >   src/libcamera/v4l2_subdevice.cpp | 6 +++---
> > >   1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > > index 9af1c0ab..37bafb9b 100644
> > > --- a/src/libcamera/v4l2_subdevice.cpp
> > > +++ b/src/libcamera/v4l2_subdevice.cpp
> > > @@ -354,10 +354,10 @@ V4L2Subdevice::Formats V4L2Subdevice::formats(unsigned int pad)
> > >   		return {};
> > >   	}
> > >
> > > +	std::vector<SizeRange> sizes = {};
> > >   	for (unsigned int code : enumPadCodes(pad)) {
> > > -		std::vector<SizeRange> sizes = enumPadSizes(pad, code);
> > > -		if (sizes.empty())
> > > -			return {};
> > > +		std::vector<SizeRange> codeSizeRange = enumPadSizes(pad, code);
> > > +		sizes.insert(sizes.end(), codeSizeRange.begin(), codeSizeRange.end());
> > Also, this builds 'sizes' incrementally, associating to the newly
> > enumerated 'code' the sizes previously enumerated for other codes
> > if I'm not mistaken.
> >
> > Is this intentional ?
>
> Right, this merges all sizes and all codes together and is probably not
> wanted. The previous implementation is actually correct, expect that I fall
> into the empty sizes case.

One thing you could do is to simply 'continue' if no size is
associated with a media bus code. I presume we bailed-out to be able
to catch errors returned by enumPadSizes(), which in case you decide
to 'continue' would be missed.

Alternatively, the enumPad[Code|Size] could be changed to accept a
vector of [unsigned int|SizeRange] respectively as output parameter
and return an error code to be able to distinguish an error condition
from an empty list of sizes.

Anyway, I would first make sure if the driver behaviour is intended or
not.

Thanks
   j

>
> >
> > Thanks
> >     j
> >
> > >   		const auto inserted = formats.insert({ code, sizes });
> > >   		if (!inserted.second) {
> > > --
> > > 2.29.2
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
>