[libcamera-devel,06/17] libcamera: v4l2_subdevice: Breakout mbus code enumeration
diff mbox series

Message ID 20190527001543.13593-7-niklas.soderlund@ragnatech.se
State Superseded
Headers show
Series
  • libcamera: Add support for format information and validation
Related show

Commit Message

Niklas Söderlund May 27, 2019, 12:15 a.m. UTC 
Simplify frame size enumeration by breaking out mbus code enumeration in
a helper. Making the code easier to read while also preparing for
enhancing the frame size enumeration. There is no functional change.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/include/v4l2_subdevice.h |  1 +
 src/libcamera/v4l2_subdevice.cpp       | 60 +++++++++++++++-----------
 2 files changed, 35 insertions(+), 26 deletions(-)

Comments

Jacopo Mondi May 27, 2019, 6:55 p.m. UTC | #1 
Hi Niklas,

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
> enhancing the frame size enumeration. There is no functional change.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@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++) {
> +		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)

Thanks
   j

> +		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)
>  {
> --
> 2.21.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart June 9, 2019, 12:36 p.m. UTC | #2 
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@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)
> >  {

Patch
diff mbox series 

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++) {
+		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) {
+		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)
 {