[libcamera-devel] [PATCH v4 07/31] libcamera: ipu3: Propagate image format
Jacopo Mondi
jacopo at jmondi.org
Thu Mar 21 20:31:12 CET 2019
Hi Laurent,
On Thu, Mar 21, 2019 at 11:54:18AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Mar 20, 2019 at 05:30:31PM +0100, Jacopo Mondi wrote:
> > Apply the requested image format to the sensor device, and apply the
> > adjusted one to the CIO2 device, the ImgU subdevice and its input and
> > output video devices.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 220 +++++++++++++++++++++++----
> > 1 file changed, 189 insertions(+), 31 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 26fc56a76eb1..2975c218f6c9 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -16,6 +16,7 @@
> > #include <libcamera/stream.h>
> >
> > #include "device_enumerator.h"
> > +#include "geometry.h"
> > #include "log.h"
> > #include "media_device.h"
> > #include "pipeline_handler.h"
> > @@ -30,6 +31,11 @@ LOG_DEFINE_CATEGORY(IPU3)
> > class ImgUDevice
> > {
> > public:
> > + static constexpr unsigned int PAD_INPUT = 0;
> > + static constexpr unsigned int PAD_OUTPUT = 2;
> > + static constexpr unsigned int PAD_VF = 3;
> > + static constexpr unsigned int PAD_STAT = 4;
> > +
> > ImgUDevice()
> > : imgu(nullptr), input(nullptr), output(nullptr),
> > viewfinder(nullptr), stat(nullptr)
> > @@ -135,6 +141,13 @@ private:
> >
> > int initImgU(ImgUDevice *imgu);
> >
> > + int setImguFormat(ImgUDevice *imguDevice,
> > + const StreamConfiguration &config,
> > + Rectangle *rect);
>
> This should be moved to the ImgU class.
>
> > + int setCIO2Format(CIO2Device *cio2Device,
> > + const StreamConfiguration &config,
> > + V4L2SubdeviceFormat *format);
and this to the CIO2 class too?
> > +
> > void registerCameras();
> >
> > ImgUDevice imgus_[IPU3_IMGU_COUNT];
> > @@ -202,60 +215,86 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
> > std::map<Stream *, StreamConfiguration> &config)
> > {
> > IPU3CameraData *data = cameraData(camera);
> > - StreamConfiguration *cfg = &config[&data->stream_];
> > - V4L2Subdevice *sensor = data->cio2.sensor;
> > - V4L2Subdevice *csi2 = data->cio2.csi2;
> > + const StreamConfiguration &cfg = config[&data->stream_];
> > + V4L2Device *viewfinder = data->imgu->viewfinder;
> > + V4L2Device *output = data->imgu->output;
> > + V4L2Device *input = data->imgu->input;
> > V4L2Device *cio2 = data->cio2.output;
> > - V4L2SubdeviceFormat subdevFormat = {};
> > - V4L2DeviceFormat devFormat = {};
> > int ret;
> >
> > + LOG(IPU3, Info)
> > + << "Requested image format: " << cfg.width << "x"
> > + << cfg.height << " - " << std::hex << std::setw(8)
> > + << cfg.pixelFormat << " on camera:'" << camera->name() << "'";
> > +
> > /*
> > - * FIXME: as of now, the format gets applied to the sensor and is
> > - * propagated along the pipeline. It should instead be applied on the
> > - * capture device and the sensor format calculated accordingly.
> > + * Verify that the requested size respects the IPU3 alignement
> > + * requirements: the image width shall be a multiple of 8 pixels and
> > + * its height a multiple of 4 pixels.
> > + *
> > + * \todo: consider the BDS scaling factor requirements:
> > + * "the downscaling factor must be an integer value multiple of 1/32"
> > */
> > + if (cfg.width % 8 || cfg.height % 4) {
> > + LOG(IPU3, Error) << "Stream format not support: bad alignement";
> > + return -EINVAL;
> > + }
> >
> > - ret = sensor->getFormat(0, &subdevFormat);
> > + /*
> > + * Pass the requested output image size to the sensor and get back the
> > + * adjusted one to be propagated to the CIO2 device and to the ImgU
> > + * input.
> > + */
> > + V4L2SubdeviceFormat sensorFormat = {};
> > + ret = setCIO2Format(&data->cio2, cfg, &sensorFormat);
> > if (ret)
> > return ret;
> >
> > - subdevFormat.width = cfg->width;
> > - subdevFormat.height = cfg->height;
> > - ret = sensor->setFormat(0, &subdevFormat);
> > + /* Apply the CIO2 image format to the CIO2 output and ImgU input. */
> > + V4L2DeviceFormat cio2Format = {};
> > + cio2Format.width = sensorFormat.width;
> > + cio2Format.height = sensorFormat.height;
> > + cio2Format.fourcc = mediaBusToCIO2Format(sensorFormat.mbus_code);
> > + cio2Format.planesCount = 1;
> > + ret = cio2->setFormat(&cio2Format);
>
> Shouldn't this be part of setCIO2Format() ?
Moved there. I wanted to expose the fact the CIO2 output and the ImgU
input are configured with the same format.
>
> > if (ret)
> > return ret;
> >
> > - /* Return error if the requested format cannot be applied to sensor. */
> > - if (subdevFormat.width != cfg->width ||
> > - subdevFormat.height != cfg->height) {
> > - LOG(IPU3, Error)
> > - << "Failed to apply image format "
> > - << subdevFormat.width << "x" << subdevFormat.height
> > - << " - got: " << cfg->width << "x" << cfg->height;
> > - return -EINVAL;
> > - }
> > + LOG(IPU3, Debug)
> > + << "CIO2 output format = " << cio2Format.toString();
> >
> > - ret = csi2->setFormat(0, &subdevFormat);
> > + ret = input->setFormat(&cio2Format);
> > if (ret)
> > return ret;
> >
> > - ret = cio2->getFormat(&devFormat);
> > + /* Apply pad formats and crop/compose rectangle to the ImgU. */
> > + Rectangle rect = {
> > + .x = 0,
> > + .y = 0,
> > + .w = cio2Format.width,
> > + .h = cio2Format.height,
> > + };
> > + ret = setImguFormat(data->imgu, cfg, &rect);
> > if (ret)
> > return ret;
> >
> > - devFormat.width = subdevFormat.width;
> > - devFormat.height = subdevFormat.height;
> > - devFormat.fourcc = cfg->pixelFormat;
> > + /* Apply the format to the ImgU output and viewfinder devices. */
> > + V4L2DeviceFormat outputFormat = {};
> > + outputFormat.width = cfg.width;
> > + outputFormat.height = cfg.height;
> > + outputFormat.fourcc = V4L2_PIX_FMT_NV12;
> > + outputFormat.planesCount = 2;
> >
> > - ret = cio2->setFormat(&devFormat);
> > + ret = output->setFormat(&outputFormat);
> > if (ret)
> > return ret;
> >
> > - LOG(IPU3, Info) << cio2->driverName() << ": "
> > - << devFormat.width << "x" << devFormat.height
> > - << "- 0x" << std::hex << devFormat.fourcc << " planes: "
> > - << devFormat.planes;
> > + LOG(IPU3, Debug)
> > + << "ImgU output format = " << outputFormat.toString();
> > +
> > + ret = viewfinder->setFormat(&outputFormat);
> > + if (ret)
> > + return ret;
> >
> > return 0;
> > }
> > @@ -666,6 +705,125 @@ void PipelineHandlerIPU3::deleteCIO2(CIO2Device *cio2)
> > delete cio2->sensor;
> > }
> >
> > +int PipelineHandlerIPU3::setImguFormat(ImgUDevice *imguDevice,
> > + const StreamConfiguration &config,
> > + Rectangle *rect)
> > +{
> > + V4L2Subdevice *imgu = imguDevice->imgu;
> > + int ret;
> > +
> > + /*
> > + * Configure the 'imgu' subdevice with the requested sizes.
>
> s/'imgu'/ImgU/ ?
>
> > + *
> > + * FIXME: the IPU3 driver implementation shall be changed to use the
> > + * actual input sizes as 'imgu input' subdevice sizes, and use the
> > + * desired output sizes to configure the crop/compose rectangles. The
> > + * current implementation uses output sizes as 'imgu input' sizes, and
> > + * uses the input dimension to configure the crop/compose rectangles,
> > + * which contradicts the V4L2 specifications.
>
> s/specifications/specification/
>
> > + */
> > + ret = imgu->setCrop(ImgUDevice::PAD_INPUT, rect);
> > + if (ret)
> > + return ret;
> > +
> > + ret = imgu->setCompose(ImgUDevice::PAD_INPUT, rect);
> > + if (ret)
> > + return ret;
> > +
> > + LOG(IPU3, Debug)
> > + << "ImgU input feeder and BDS rectangle = (0,0)/"
> > + << rect->w << "x" << rect->h;
>
> toString() for our Rectangle class ?
>
I'll add a patch
> > +
> > + V4L2SubdeviceFormat imguFormat = {};
> > + imguFormat.width = config.width;
> > + imguFormat.height = config.height;
> > + imguFormat.mbus_code = MEDIA_BUS_FMT_FIXED;
> > +
> > + ret = imgu->setFormat(ImgUDevice::PAD_INPUT, &imguFormat);
> > + if (ret)
> > + return ret;
> > +
> > + ret = imgu->setFormat(ImgUDevice::PAD_OUTPUT, &imguFormat);
> > + if (ret)
> > + return ret;
> > +
> > + LOG(IPU3, Debug)
> > + << "ImgU GDC format = " << imguFormat.toString();
> > +
> > + ret = imgu->setFormat(ImgUDevice::PAD_VF, &imguFormat);
> > + if (ret)
> > + return ret;
> > +
> > + ret = imgu->setFormat(ImgUDevice::PAD_STAT, &imguFormat);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +int PipelineHandlerIPU3::setCIO2Format(CIO2Device *cio2Device,
> > + const StreamConfiguration &config,
> > + V4L2SubdeviceFormat *format)
> > +{
>
> We're starting to get quite some code associated with the CIO2Device. I
> know I requested for CIO2Device to be a struct without methods, but I
> wonder if that was a mistake :-(
>
> I would also rename the function, maybe to configure(). setFormat()
> hints that it sets the format passed as an argument, which is not the
> case here, the format parameter is an output.
>
> > + unsigned int imageSize = config.width * config.height;
> > + V4L2Subdevice *sensor = cio2Device->sensor;
> > + V4L2Subdevice *csi2 = cio2Device->csi2;
> > + unsigned int best = ~0;
> > + bool found = false;
> > + int ret;
> > +
> > + const FormatEnum formats = sensor->formats(0);
> > + for (auto it = formats.begin(); it != formats.end(); ++it) {
> > + /* Only consider formats consumable by the CIO2 unit. */
> > + int cio2Code = mediaBusToCIO2Format(it->first);
> > + if (cio2Code == -EINVAL)
>
> You could write
>
> if (mediaBusToCIO2Format(it->first) == -EINVAL)
>
> (and I would check for < 0 instead of == -EINVAL to avoid surprises if
> we later change that method)
>
> > + continue;
> > +
> > + for (const SizeRange &size : it->second) {
> > + /*
> > + * Only select formats bigger than the requested sizes
> > + * as the IPU3 cannot up-scale.
> > + */
> > + if (size.maxWidth < config.width ||
> > + size.maxHeight < config.height)
> > + continue;
> > +
> > + unsigned int diff = size.maxWidth * size.maxHeight
> > + - imageSize;
> > + if (diff >= best)
> > + continue;
> > +
> > + best = diff;
> > + found = true;
> > +
> > + format->width = size.maxWidth;
> > + format->height = size.maxHeight;
> > + format->mbus_code = it->first;
> > + }
> > + }
>
> Open question, should we do this, or always pick the largest size from
> the sensor and scale in the ImgU ?
>
Good question... I cannot tell the pros and cons actually... why would
you think it is better?
Thanks
j
> > + if (!found) {
> > + LOG(IPU3, Error)
> > + << "Unable to find image format suitable to produce: "
> > + << config.width << "x" << config.height
> > + << "- 0x" << std::hex << std::setfill('0')
>
> Spaces before and after dash, or none at all.
>
> > + << std::setw(8) << config.pixelFormat;
>
> Again a shame we can't use toString(). Do you use that helper at all ?
> :-)
>
> > + return -EINVAL;
> > + }
> > +
> > + /* Apply the selected format to the sensor and the CSI-2 receiver. */
> > + ret = sensor->setFormat(0, format);
> > + if (ret)
> > + return ret;
> > +
> > + ret = csi2->setFormat(0, format);
> > + if (ret)
> > + return ret;
> > +
> > + LOG(IPU3, Debug) << "CIO2 Image format: " << format->toString();
>
> Ah yes you use it here.
>
> > +
> > + return 0;
> > +}
> > +
> > /*
> > * Cameras are created associating an image sensor (represented by a
> > * media entity with function MEDIA_ENT_F_CAM_SENSOR) to one of the four
>
> --
> 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/20190321/09898c4e/attachment.sig>
More information about the libcamera-devel
mailing list