[libcamera-devel] [PATCH v4 03/31] libcamera: ipu3: Make sure sensor provides a compatible format

[Jacopo Mondi]

Hi Laurent,

On Sat, Mar 23, 2019 at 02:58:18PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Thu, Mar 21, 2019 at 03:50:16PM +0100, Jacopo Mondi wrote:
> > On Thu, Mar 21, 2019 at 10:50:05AM +0200, Laurent Pinchart wrote:
> > > On Wed, Mar 20, 2019 at 05:30:27PM +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.
> > >>
> > >> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > >> ---
> > >>  src/libcamera/pipeline/ipu3/ipu3.cpp | 43 ++++++++++++++++++++++++++--
> > >>  1 file changed, 41 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > >> index 55489c31df2d..2602f89617a3 100644
> > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > >> @@ -8,6 +8,8 @@
> > >>  #include <memory>
> > >>  #include <vector>
> > >>
> > >> +#include <linux/media-bus-format.h>
> > >> +
> > >>  #include <libcamera/camera.h>
> > >>  #include <libcamera/request.h>
> > >>  #include <libcamera/stream.h>
> > >> @@ -78,6 +80,8 @@ private:
> > >>  			PipelineHandler::cameraData(camera));
> > >>  	}
> > >>
> > >> +	int mediaBusToCIO2Format(unsigned int code);
> > >> +
> > >>  	void registerCameras();
> > >>
> > >>  	std::shared_ptr<MediaDevice> cio2_;
> > >> @@ -327,6 +331,22 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> > >>  	return true;
> > >>  }
> > >>
> > >> +int PipelineHandlerIPU3::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;
> > >> +	}
> > >> +}
> > >> +
> > >>  /*
> > >>   * Cameras are created associating an image sensor (represented by a
> > >>   * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four
> > >> @@ -404,18 +424,37 @@ 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.
> > >> +		 */
> > >>  		data->sensor_ = new V4L2Subdevice(sensor);
> > >>  		ret = data->sensor_->open();
> > >>  		if (ret)
> > >>  			continue;
> > >>
> > >> +		const FormatEnum formats = data->sensor_->formats(0);
> > >> +		auto it = formats.begin();
> > >> +		for (; it != formats.end(); ++it) {
> > >
> > > How about
> > >
> > > 		for (auto it : formats)
> > >
> >
> > Here and in other patches in the series I need to check the value of
> > it after the for loop, so I cannot use a variable with a scope local
> > to the loop only.
> >
> > You suggested alternatives offline, but this cose seems quite parsable
> > to me.
> >
> > >> +			if (mediaBusToCIO2Format(it->first) != -EINVAL)
> > >> +				break;
> > >> +		}
> > >> +		if (it == formats.end()) {
> > >> +			LOG(IPU3, Info)
> > >> +				<< "Sensor '" << data->sensor_->deviceName()
> > >
> > > How about printing the entity name instead ?
> >
> > For V4L2Subdevice deviceName() prints the entity name.
> > Confusing?
>
> I suppose so, given that I got confused :-)

V5 will contain a patch which changes this (and makes the
V4L2Subdevice return a const & to a string in its deviceNode() and
deviceName() methods, which they embarassing enough copy back at
the moment)

Thanks
  j
>
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > Thanks, I'll also consider caching the camera mix and min sizes when
> > registering it, as you suggested in a comment to the next patch.
> >
> > >> +				<< "' 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);
> > >> +
> > >>  		registerCamera(std::move(camera), std::move(data));
> > >>
> > >>  		LOG(IPU3, Info)
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190323/59a628ed/attachment.sig>