[libcamera-devel] [PATCH v5 08/19] libcamera: ipu3: Cache the camera sizes

[Laurent Pinchart]

Hi Jacopo,

On Tue, Apr 02, 2019 at 11:23:30AM +0200, Jacopo Mondi wrote:
> On Tue, Apr 02, 2019 at 12:11:59PM +0300, Laurent Pinchart wrote:
> > On Tue, Apr 02, 2019 at 10:26:07AM +0200, Jacopo Mondi wrote:
> >> On Tue, Apr 02, 2019 at 10:27:17AM +0300, Laurent Pinchart wrote:
> >>> On Tue, Mar 26, 2019 at 09:38:51AM +0100, Jacopo Mondi wrote:
> >>>> When creating a camera, make sure a the image sensor provides images in
> >>>> a format compatible with IPU3 CIO2 unit requirements and cache the
> >>>> minimum and maximum camera sizes.
> >>>>
> >>>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >>>> ---
> >>>>  src/libcamera/pipeline/ipu3/ipu3.cpp | 56 +++++++++++++++++++++++++++-
> >>>>  1 file changed, 54 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>>> index 55489c31df2d..d42c81273cc6 100644
> >>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>>> @@ -8,11 +8,14 @@
> >>>>  #include <memory>
> >>>>  #include <vector>
> >>>>
> >>>> +#include <linux/media-bus-format.h>
> >>>> +
> >>>>  #include <libcamera/camera.h>
> >>>>  #include <libcamera/request.h>
> >>>>  #include <libcamera/stream.h>
> >>>>
> >>>>  #include "device_enumerator.h"
> >>>> +#include "geometry.h"
> >>>>  #include "log.h"
> >>>>  #include "media_device.h"
> >>>>  #include "pipeline_handler.h"
> >>>> @@ -24,6 +27,22 @@ namespace libcamera {
> >>>>
> >>>>  LOG_DEFINE_CATEGORY(IPU3)
> >>>>
> >>>> +static int mediaBusToCIO2Format(unsigned int code)
> >>>> +{
> >>>> +	switch (code) {
> >>>> +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> >>>> +		return V4L2_PIX_FMT_IPU3_SBGGR10;
> >>>> +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> >>>> +		return V4L2_PIX_FMT_IPU3_SGBRG10;
> >>>> +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> >>>> +		return V4L2_PIX_FMT_IPU3_SGRBG10;
> >>>> +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> >>>> +		return V4L2_PIX_FMT_IPU3_SRGGB10;
> >>>> +	default:
> >>>> +		return -EINVAL;
> >>>> +	}
> >>>> +}
> >>>> +
> >>>>  class PipelineHandlerIPU3 : public PipelineHandler
> >>>>  {
> >>>>  public:
> >>>> @@ -70,6 +89,9 @@ private:
> >>>>  		V4L2Subdevice *sensor_;
> >>>>
> >>>>  		Stream stream_;
> >>>> +
> >>>> +		/* Maximum sizes and the mbus code used to produce them. */
> >>>> +		std::pair<unsigned int, SizeRange> maxSizes_;
> >>>
> >>> The use of std::pair here makes the code below use .first and .second,
> >>> which are not very readable. I think it would be better to store the
> >>> pixel format and max size in two separate fields, of unsigned int and
> >>> SizeRange types respectively. Or, now that I think about it, as you only
> >>> care about the maximum size, maybe adding a Size structure to geometry.h
> >>> would be a good idea.
> >>>
> >>
> >> I see... I'll ponder about it. Adding new stuff it's always a burden
> >> because of documentation and such, but I get your point...
> >
> > I know it can be a bit painful :-( A Size structure should hopefully be
> > simple though.
> >
> >>>>  	};
> >>>>
> >>>>  	IPU3CameraData *cameraData(const Camera *camera)
> >>>> @@ -404,18 +426,48 @@ void PipelineHandlerIPU3::registerCameras()
> >>>>  		if (ret)
> >>>>  			continue;
> >>>>
> >>>> -		data->cio2_->bufferReady.connect(data.get(), &IPU3CameraData::bufferReady);
> >>>> -
> >>>> +		/*
> >>>> +		 * Make sure the sensor produces at least one image format
> >>>> +		 * compatible with IPU3 CIO2 requirements and cache the camera
> >>>> +		 * maximum sizes.
> >>>> +		 */
> >>>>  		data->sensor_ = new V4L2Subdevice(sensor);
> >>>>  		ret = data->sensor_->open();
> >>>>  		if (ret)
> >>>>  			continue;
> >>>>
> >>>> +		for (auto it : data->sensor_->formats(0)) {
> >>>> +			int mbusCode = mediaBusToCIO2Format(it.first);
> >>>> +			if (mbusCode < 0)
> >>>> +				continue;
> >>>> +
> >>>> +			for (const SizeRange &size : it.second) {
> >>>> +				SizeRange &maxSize = data->maxSizes_.second;
> >>>> +
> >>>> +				if (maxSize.maxWidth < size.maxWidth &&
> >>>> +				    maxSize.maxHeight < size.maxHeight) {
> >>>> +					maxSize.maxWidth = size.maxWidth;
> >>>> +					maxSize.maxHeight = size.maxHeight;
> >>>> +					data->maxSizes_.first = mbusCode;
> >>>> +				}
> >>>> +			}
> >>>> +		}
> >>>
> >>> One blank line ?
> >>
> >> Not sure, as this is an error condition, I like it better without an
> >> empty line.
> >
> > There are generally empty lines after a for loop of if block, but it's
> > your code :-)
> >
> >>>> +		if (data->maxSizes_.second.maxWidth == 0) {
> >>>> +			LOG(IPU3, Info)
> >>>> +				<< "Sensor '" << data->sensor_->entityName()
> >>>> +				<< "' detected, but no supported image format "
> >>>> +				<< " found: skip camera creation";
> >>>> +			continue;
> >>>> +		}
> >>>> +
> >>>>  		data->csi2_ = new V4L2Subdevice(csi2);
> >>>>  		ret = data->csi2_->open();
> >>>>  		if (ret)
> >>>>  			continue;
> >>>>
> >>>> +		data->cio2_->bufferReady.connect(data.get(),
> >>>> +						 &IPU3CameraData::bufferReady);
> >>>> +
> >>>
> >>> This seems unrelated to this patch, is it needed ?
> >>
> >> please see:
> >> 
> >>>> -           data->cio2_->bufferReady.connect(data.get(), &IPU3CameraData::bufferReady);
> >>
> >> A few lines above here.
> >
> > I know, what I meant is that moving the code seems unrelated, as you
> > don't touch it (apart from wrapping the line).
> >
> 
> Ah, got it. I moved it here because I want to connect the signal only
> after we have successfully validated the sensor provided formats
> (introduced in this patch), so I don't think it's unrelated, isn't it?

My point is that there's no strict need to connect the signal only after
validating the sensor formats. It's no big deal though.

> >>>>  		registerCamera(std::move(camera), std::move(data));
> >>>>
> >>>>  		LOG(IPU3, Info)

-- 
Regards,

Laurent Pinchart