[libcamera-devel] [PATCH v5 08/19] libcamera: ipu3: Cache the camera sizes
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Apr 2 11:11:59 CEST 2019
Hi Jacopo,
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).
> >> registerCamera(std::move(camera), std::move(data));
> >>
> >> LOG(IPU3, Info)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list