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

[Laurent Pinchart]

Hello,

On Thu, Mar 11, 2021 at 12:28:05PM +0100, Jacopo Mondi wrote:
> On Thu, Mar 11, 2021 at 12:15:48PM +0100, Marian Cichy wrote:
> > On 3/11/21 9:16 AM, Jacopo Mondi wrote:
> > > 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 ?

A media bus code without sizes seems a bit weird. Marian, on which
subdev/pad is this, and what's the code ?

> > > > 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.
> 
> > > >   		const auto inserted = formats.insert({ code, sizes });
> > > >   		if (!inserted.second) {

-- 
Regards,

Laurent Pinchart