[libcamera-devel] [PATCH 06/17] libcamera: v4l2_subdevice: Breakout mbus code enumeration

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, May 27, 2019 at 08:55:11PM +0200, Jacopo Mondi wrote:
> On Mon, May 27, 2019 at 02:15:32AM +0200, Niklas Söderlund wrote:
> > Simplify frame size enumeration by breaking out mbus code enumeration in
> > a helper. Making the code easier to read while also preparing for

s/. Making/, making/

> > enhancing the frame size enumeration. There is no functional change.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/libcamera/include/v4l2_subdevice.h |  1 +
> >  src/libcamera/v4l2_subdevice.cpp       | 60 +++++++++++++++-----------
> >  2 files changed, 35 insertions(+), 26 deletions(-)
> >
> > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> > index 3e4e5107aebeee06..e714e2575022c04d 100644
> > --- a/src/libcamera/include/v4l2_subdevice.h
> > +++ b/src/libcamera/include/v4l2_subdevice.h
> > @@ -57,6 +57,7 @@ protected:
> >  	std::string logPrefix() const;
> >
> >  private:
> > +	std::vector<unsigned int> enumPadCodes(unsigned int pad);
> >  	int enumPadSizes(unsigned int pad, unsigned int code,
> >  			 std::vector<SizeRange> *size);
> >
> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > index fceee33156e94212..51b0ffaafb3e668e 100644
> > --- a/src/libcamera/v4l2_subdevice.cpp
> > +++ b/src/libcamera/v4l2_subdevice.cpp
> > @@ -203,37 +203,15 @@ int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect)
> >  FormatEnum V4L2Subdevice::formats(unsigned int pad)
> >  {
> >  	FormatEnum formatMap = {};
> > -	struct v4l2_subdev_mbus_code_enum mbusEnum = {};
> > -	int ret;
> >
> >  	if (pad >= entity_->pads().size()) {
> >  		LOG(V4L2Subdev, Error) << "Invalid pad: " << pad;
> > -		return formatMap;
> > +		return {};
> >  	}
> >
> > -	mbusEnum.pad = pad;
> > -	mbusEnum.index = 0;
> > -	mbusEnum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > -	while (true) {
> > -		ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);
> > -		if (ret)
> > -			break;
> > -
> > -		ret = enumPadSizes(pad, mbusEnum.code,
> > -				   &formatMap[mbusEnum.code]);
> > -		if (ret)
> > -			break;
> > -
> > -		mbusEnum.index++;
> > -	}
> > -
> > -	if (ret && (errno != EINVAL && errno != ENOTTY)) {
> > -		ret = -errno;
> > -		LOG(V4L2Subdev, Error)
> > -			<< "Unable to enumerate formats on pad " << pad
> > -			<< ": " << strerror(-ret);
> > -		formatMap.clear();
> > -	}
> > +	for (unsigned int code : enumPadCodes(pad))
> > +		if (enumPadSizes(pad, code, &formatMap[code]))
> > +			return {};
> >
> >  	return formatMap;
> >  }
> > @@ -328,6 +306,36 @@ std::string V4L2Subdevice::logPrefix() const
> >  	return "'" + entity_->name() + "'";
> >  }
> >
> > +std::vector<unsigned int> V4L2Subdevice::enumPadCodes(unsigned int pad)
> > +{
> > +	std::vector<unsigned int> codes;
> > +	int ret;
> > +
> > +	for (unsigned int index = 0;; index++) {

s/;;/; ;/

> > +		struct v4l2_subdev_mbus_code_enum mbusEnum = {
> > +			.pad = pad,
> > +			.index = index,
> > +			.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > +		};
> > +
> > +		ret = ioctl(fd_, VIDIOC_SUBDEV_ENUM_MBUS_CODE, &mbusEnum);
> > +		if (ret)
> > +			break;
> > +
> > +		codes.push_back(mbusEnum.code);
> > +	}
> > +
> > +	if (ret && errno != EINVAL) {
> 
> I think you should handle -ENOTTY, otherwise devices not supporting
> frame enumeration would fail (which is maybe intended, don't know)

That's a failure, so we should return an empty vector, but we could
possibly skip the error message.

> > +		ret = errno;
> > +		LOG(V4L2Subdev, Error)
> > +			<< "Unable to enumerate formats on pad " << pad
> > +			<< ": " << strerror(ret);
> > +		return {};
> > +	}
> > +
> > +	return codes;
> > +}
> > +
> >  int V4L2Subdevice::enumPadSizes(unsigned int pad,unsigned int code,
> >  				std::vector<SizeRange> *sizes)
> >  {

-- 
Regards,

Laurent Pinchart