[libcamera-devel] [PATCH 5/5] libcamera: ipu3: Add IMGUDevice class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Feb 22 01:28:38 CET 2019
Hi Jacopo,
Thank you for the patch.
On Wed, Feb 20, 2019 at 02:17:57PM +0100, Jacopo Mondi wrote:
> Add an IMGUDevice class that represents all devices associated with the
> IPU3 imgu unit.
s/imgu/ImgU/
>
> Imgu units will be freely assigned to cameras, depending on the number of
s/Imgu/ImgU/
> streams that are required. Add two imgu instances to the ipu3 pipeline,
Same here and everywhere else, and s/ipu3/IPU3/
> and provide methods to configure the format on the imgu internal
> components.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> src/libcamera/pipeline/ipu3/imgu.cpp | 338 ++++++++++++++++++++++++
> src/libcamera/pipeline/ipu3/ipu3.cpp | 71 +++++
> src/libcamera/pipeline/ipu3/ipu3.h | 34 +++
> src/libcamera/pipeline/ipu3/meson.build | 1 +
> 4 files changed, 444 insertions(+)
> create mode 100644 src/libcamera/pipeline/ipu3/imgu.cpp
>
> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> new file mode 100644
> index 000000000000..7945764eb989
> --- /dev/null
> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> @@ -0,0 +1,338 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * imgu.cpp - IPU3 IMGU unit
> + */
> +
> +#include <string>
> +#include <vector>
> +
> +#include <linux/media-bus-format.h>
> +
> +#include "geometry.h"
> +#include "ipu3.h"
> +#include "media_device.h"
> +#include "v4l2_device.h"
> +#include "v4l2_subdevice.h"
> +
> +/*
> + * IMGUDevice represents one of the two 'IMGU' units the Intel IPU3 ISP
How about naming the class ImgUDevice ?
> + * provides.
> + *
> + * IMGU units are fed with image data captured from a CIO2 instance and with
> + * configuration parameters for 3A tuning.
> + *
> + * Each IMGU handles 2 video pipes at the time, and each pipe provide 2 image
Did you mean "at the same time" ?
s/provide/provides/
> + * outputs ('output' and 'viewfinder') and one statistics metadata output.
> + *
> + * TODO: support concurrent pipes
> + * TODO: support multiple video capture streams (output + viewfinder)
> + */
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(IPU3)
> +
> +IMGUDevice::IMGUDevice()
> + : owner_(nullptr), imgu_(nullptr), input_(nullptr),
> + output_(nullptr), viewfinder_(nullptr), stat_(nullptr)
> +{
> +}
> +
> +IMGUDevice::~IMGUDevice()
> +{
> + delete imgu_;
> + delete input_;
> + delete output_;
> + delete viewfinder_;
> + delete stat_;
> +}
> +
> +int IMGUDevice::open(unsigned int index, MediaDevice *imguMediaDev)
Same comment as for CIO2Device, here and through this function.
> +{
> + int ret;
> +
> + std::string imguName = "ipu3-imgu " + std::to_string(index);
> + MediaEntity *entity = imguMediaDev->getEntityByName(imguName);
> + if (!entity) {
> + LOG(IPU3, Error) << "Failed to get entity '" << imguName << "'";
> + return -ENODEV;
> + }
> +
> + imgu_ = new V4L2Subdevice(entity);
> + ret = imgu_->open();
> + if (ret)
> + return ret;
> +
> + std::string inputName = imguName + " input";
> + entity = imguMediaDev->getEntityByName(inputName);
> + if (!entity) {
> + LOG(IPU3, Error) << "Failed to get entity '" << inputName << "'";
> + return -ENODEV;
> + }
> +
> + input_ = new V4L2Device(entity);
> + ret = input_->open();
> + if (ret)
> + return ret;
> +
> + std::string outputName = imguName + " output";
> + entity = imguMediaDev->getEntityByName(outputName);
> + if (!entity) {
> + LOG(IPU3, Error) << "Failed to get entity '" << outputName << "'";
> + return -ENODEV;
> + }
> +
> + output_ = new V4L2Device(entity);
> + ret = output_->open();
> + if (ret)
> + return ret;
> +
> + std::string viewfinderName = imguName + " viewfinder";
> + entity = imguMediaDev->getEntityByName(viewfinderName);
> + if (!entity) {
> + LOG(IPU3, Error) << "Failed to get entity '"
> + << viewfinderName << "'";
> + return -ENODEV;
> + }
> +
> + viewfinder_ = new V4L2Device(entity);
> + ret = viewfinder_->open();
> + if (ret)
> + return ret;
> +
> + std::string statName = imguName + " 3a stat";
> + entity = imguMediaDev->getEntityByName(statName);
> + if (!entity) {
> + LOG(IPU3, Error) << "Failed to get entity '"
> + << statName << "'";
> + return -ENODEV;
> + }
> +
> + stat_ = new V4L2Device(entity);
> + ret = stat_->open();
> + if (ret)
> + return ret;
> +
> + /* Link entities to configure the IMGU unit for capture. */
> + MediaLink *link = imguMediaDev->link(inputName, 0, imguName, 0);
Shouldn't you use the constants you defined for pad numbers ?
> + if (!link) {
> + LOG(IPU3, Error)
> + << "Failed to get link '" << inputName << "':0 -> '"
> + << imguName << "':0";
> + return -EINVAL;
> + }
> + link->setEnabled(true);
> +
> + link = imguMediaDev->link(imguName, 2, outputName, 0);
> + if (!link) {
> + LOG(IPU3, Error)
> + << "Failed to get link '" << imguName << "':2 -> '"
> + << outputName << "':0";
> + return -EINVAL;
> + }
> + link->setEnabled(true);
> +
> + link = imguMediaDev->link(imguName, 3, viewfinderName, 0);
> + if (!link) {
> + LOG(IPU3, Error)
> + << "Failed to get link '" << imguName << "':3 -> '"
> + << viewfinderName << "':0";
> + return -EINVAL;
> + }
> + link->setEnabled(true);
> +
> + link = imguMediaDev->link(imguName, 4, statName, 0);
> + if (!link) {
> + LOG(IPU3, Error)
> + << "Failed to get link '" << imguName << "':4 -> '"
> + << statName << "':0";
> + return -EINVAL;
> + }
> + link->setEnabled(true);
> +
> + return 0;
> +}
> +
> +int IMGUDevice::configure(const IPU3DeviceFormat &inputFormat,
> + const IPU3DeviceFormat &outputFormat)
> +{
> + int ret;
> +
> + Rectangle selectionRectangle = {};
Maybe just rectangle or rect ? Let's not use long names needlessly.
> + selectionRectangle.x = 0;
> + selectionRectangle.y = 0;
> + selectionRectangle.w = inputFormat.width;
> + selectionRectangle.h = inputFormat.height;
Can't you initialize the rectangle when declaring it ?
> +
> + ret = imgu_->setCrop(IMGU_PAD_INPUT, selectionRectangle);
> + if (ret)
> + return ret;
> +
> + LOG(IPU3, Debug)
> + << "'" << imgu_->deviceName() << "':" << IMGU_PAD_INPUT << " = "
> + << "crop: (0,0)/" << selectionRectangle.w << "x"
> + << selectionRectangle.h;
> +
> + ret = imgu_->setCompose(IMGU_PAD_INPUT, selectionRectangle);
> + if (ret)
> + return ret;
> +
> + LOG(IPU3, Debug)
> + << "'" << imgu_->deviceName() << "':" << IMGU_PAD_INPUT << " = "
> + << "compose: (0,0)/" << selectionRectangle.w << "x"
> + << selectionRectangle.h;
> +
> + V4L2SubdeviceFormat subdevFormat = {};
> + ret = imgu_->getFormat(IMGU_PAD_INPUT, &subdevFormat);
> + if (ret)
> + return ret;
> +
> + LOG(IPU3, Debug)
> + << "'" << imgu_->deviceName() << "':" << IMGU_PAD_INPUT << " = "
> + << subdevFormat.width << "x" << subdevFormat.height << " - "
> + << std::hex << subdevFormat.mbus_code;
> +
> + /*
> + * Apply image format to the 'imgu' unit subdevices.
> + *
> + * 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.
> + */
> + subdevFormat = {};
> + subdevFormat.width = outputFormat.width;
> + subdevFormat.height = outputFormat.height;
> + subdevFormat.mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10;
> +
> + ret = imgu_->setFormat(IMGU_PAD_INPUT, &subdevFormat);
> + if (ret)
> + return ret;
> +
> + LOG(IPU3, Debug)
> + << "'" << imgu_->deviceName() << "':" << IMGU_PAD_INPUT << " = "
> + << subdevFormat.width << "x" << subdevFormat.height << " - "
> + << std::hex << subdevFormat.mbus_code;
> +
> + /*
> + * Configure the 'imgu output' and 'imgu 3a stat' pads with the
> + * desired output image sizes.
> + */
> + subdevFormat = {};
> + subdevFormat.width = outputFormat.width;;
> + subdevFormat.height = outputFormat.height;
> + subdevFormat.mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10;
> +
> + ret = imgu_->setFormat(IMGU_PAD_OUTPUT, &subdevFormat);
> + if (ret)
> + return ret;
> +
> + LOG(IPU3, Debug)
> + << "'" << imgu_->deviceName() << "':" << IMGU_PAD_OUTPUT << " = "
> + << subdevFormat.width << "x" << subdevFormat.height << " - "
> + << std::hex << subdevFormat.mbus_code;
> +
> + subdevFormat = {};
> + subdevFormat.width = outputFormat.width;;
> + subdevFormat.height = outputFormat.height;
> + subdevFormat.mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10;
> +
> + ret = imgu_->setFormat(IMGU_PAD_STAT, &subdevFormat);
> + if (ret)
> + return ret;
> +
> + LOG(IPU3, Debug)
> + << "'" << imgu_->deviceName() << "':" << IMGU_PAD_STAT << " = "
> + << subdevFormat.width << "x" << subdevFormat.height << " - "
> + << std::hex << subdevFormat.mbus_code;
> +
> + /*
> + * FIXME: Set the 'imgu viewfinder' to hardcoded size
> + * It shall be configured as secondary stream when support for
> + * multiple streams is added.
> + */
> + subdevFormat = {};
> + subdevFormat.width = outputFormat.width / 2;
> + subdevFormat.height = outputFormat.height / 2;
> + subdevFormat.mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10;
> +
> + ret = imgu_->setFormat(IMGU_PAD_VF, &subdevFormat);
> + if (ret)
> + return ret;
> +
> + LOG(IPU3, Debug)
> + << "'" << imgu_->deviceName() << "':" << IMGU_PAD_VF << " = "
> + << subdevFormat.width << "x" << subdevFormat.height << " - "
> + << std::hex << subdevFormat.mbus_code;
> +
> + /*
> + * Apply the CIO2 provided 'inputFormat' to the 'imgu input' video
> + * device, and the requested 'outputFormat' to the to the 'imgu output'
> + * 'imgu viewfinder' and 'imgu 3a stat' nodes.
> + *
> + * FIXME: Keep the viewfinder size reduced.
> + */
> + V4L2DeviceFormat devFormat = {};
> + devFormat.width = inputFormat.width;
> + devFormat.height = inputFormat.height;
> + devFormat.fourcc = inputFormat.fourcc;
> +
> + ret = input_->setFormat(&devFormat);
> + if (ret)
> + return ret;
> +
> + LOG(IPU3, Debug)
> + << "'" << input_->driverName() << "': "
> + << devFormat.width << "x" << devFormat.height
> + << "- 0x" << std::hex << devFormat.fourcc << " planes: "
> + << devFormat.planesCount;
> +
> + devFormat = {};
> + devFormat.width = outputFormat.width;
> + devFormat.height = outputFormat.height;
> + devFormat.fourcc = outputFormat.fourcc;
> + V4L2DeviceFormat tmpFormat = devFormat;
> +
> + ret = output_->setFormat(&tmpFormat);
> + if (ret)
> + return ret;
> +
> + LOG(IPU3, Debug)
> + << "'" << output_->driverName() << "': "
> + << tmpFormat.width << "x" << tmpFormat.height
> + << "- 0x" << std::hex << tmpFormat.fourcc << " planes: "
> + << tmpFormat.planesCount;
> +
> + tmpFormat = devFormat;
> + ret = stat_->setFormat(&tmpFormat);
> + if (ret)
> + return ret;
> +
> + LOG(IPU3, Debug)
> + << "'" << stat_->driverName() << "': "
> + << tmpFormat.width << "x" << tmpFormat.height
> + << "- 0x" << std::hex << tmpFormat.fourcc << " planes: "
> + << tmpFormat.planesCount;
> +
> + tmpFormat = devFormat;
> + tmpFormat.width = devFormat.width / 2;
> + tmpFormat.height = devFormat.height / 2;
> +
> + ret = viewfinder_->setFormat(&tmpFormat);
> + if (ret)
> + return ret;
> +
> + LOG(IPU3, Debug)
> + << "'" << viewfinder_->driverName() << "': "
> + << tmpFormat.width << "x" << tmpFormat.height
> + << "- 0x" << std::hex << tmpFormat.fourcc << " planes: "
> + << tmpFormat.planesCount;
> +
> + return 0;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 5fcdd6335db6..8b4c88048f6c 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -64,6 +64,7 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
> Stream *stream = &cameraData(camera)->stream_;
> StreamConfiguration *cfg = &config[stream];
> IPU3DeviceFormat outputFormat = {};
> + IMGUDevice *imgu;
> int ret;
>
> /* Validate the requested image format and resolution. */
> @@ -75,10 +76,46 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
> outputFormat.height = cfg->height;
> outputFormat.fourcc = cfg->pixelFormat;
>
> + /*
> + * FIXME: as of now, a single stream is supported, so we can claim
> + * the first available imgu unit, if any. When multiple streams per
> + * camera will be supported, a single camera could use both the imgu
s/both the imgu units/both ImgUs/ (U stands for unit)
> + * units and prevent other cameras to be used.
s/to be used/from being used/
> + */
> + if (imgu0_.available())
> + imgu = &imgu0_;
> + else if (imgu1_.available())
> + imgu = &imgu1_;
> + else
> + return -EBUSY;
You can just use imgu0_ unconditionally for now.
> + /*
> + * FIXME: the imgu unit shall be released when streams gets released.
> + * How to deal with cross-camera dependencies ?
> + */
> + imgu->acquire(camera);
> +
> + /*
> + * Configure the CIO2 unit to provide images in a resolution that
> + * satisfies the desired output format.
> + */
> ret = cio2->configure(outputFormat);
> if (ret)
> return ret;
>
> + /*
> + * Get the CIO2 output format and apply it to the IMGU input and
> + * apply the output format to the IMGU output.
> + *
> + * Hardcode the fourcc code output from CIO2 to IMGU.
> + */
> + IPU3DeviceFormat inputFormat = cio2->format();
> + inputFormat.fourcc = V4L2_PIX_FMT_IPU3_SGRBG10;
Should cio2->format() provide the right fourcc already ?
> +
> + ret = imgu->configure(inputFormat, outputFormat);
> + if (ret)
> + return ret;
> +
> return 0;
> }
>
> @@ -205,12 +242,22 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> if (cio2_->disableLinks())
> goto error_close_cio2;
>
> + if (imgu_->open())
> + goto error_close_cio2;
> +
> registerCameras();
>
> + if (registerImgus())
> + goto error_close_imgu;
> +
> cio2_->close();
> + imgu_->close();
>
> return true;
>
> +error_close_imgu:
> + imgu_->close();
> +
> error_close_cio2:
> cio2_->close();
You can close an unopened device, so let's go for a single error label,
it makes code simpler.
>
> @@ -221,6 +268,30 @@ error_release_mdev:
> return false;
> }
>
> +/*
> + * IPU3 has two 'imgu' units which can be freely assigned to cameras;
s/has two 'imgu' units which/two ImgUs that/
s/;/./
> + *
> + * Create instances for both the units, creating video devices and subdevices
s/both the units/both units/
> + * associated with an instance.
You open the instances, you don't create them.
> + */
> +
> +int PipelineHandlerIPU3::registerImgus()
The ImgUs are not registered anywhere, this function should be renamed.
> +{
> + int ret = imgu0_.open(0, imgu_.get());
> + if (ret) {
> + LOG(IPU3, Error) << "Failed to open 'imgu0' unit";
> + return ret;
> + }
> +
> + ret = imgu1_.open(1, imgu_.get());
> + if (ret) {
> + LOG(IPU3, Error) << "Failed to open 'imgu1' unit";
> + return ret;
> + }
> +
> + 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
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.h b/src/libcamera/pipeline/ipu3/ipu3.h
> index 2a8b6f13b1c7..83d6e7cf27db 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.h
> +++ b/src/libcamera/pipeline/ipu3/ipu3.h
> @@ -57,6 +57,36 @@ public:
> IPU3DeviceFormat cio2Format_;
> };
>
> +class IMGUDevice
> +{
> +public:
> + static constexpr unsigned int IMGU_PAD_INPUT = 0;
> + static constexpr unsigned int IMGU_PAD_OUTPUT = 2;
> + static constexpr unsigned int IMGU_PAD_VF = 3;
> + static constexpr unsigned int IMGU_PAD_STAT = 4;
I wonder if static is needed here.
> +
> + IMGUDevice();
> + IMGUDevice(const IMGUDevice &) = delete;
Should you delete the copy assignement operator too ?
> + ~IMGUDevice();
> +
> + Camera *owner_;
> + bool available() const { return owner_ == nullptr; }
> + void acquire(Camera *camera) { owner_ = camera; }
> + void release() { owner_ = nullptr; }
We don't know yet how we will deal with ownership, so for now I'd drop
this.
> +
> + int open(unsigned int index, MediaDevice *imguMediaDev);
Missing close().
> +
> + int configure(const IPU3DeviceFormat &cio2Format,
> + const IPU3DeviceFormat &imguFormat);
> +
> + V4L2Subdevice *imgu_;
> + V4L2Device *input_;
> + V4L2Device *output_;
> + V4L2Device *viewfinder_;
> + V4L2Device *stat_;
> + /* TODO: add param video device for 3A tuning */
> +};
> +
> class PipelineHandlerIPU3 : public PipelineHandler
> {
> public:
> @@ -95,10 +125,14 @@ private:
> PipelineHandler::cameraData(camera));
> }
>
> + int registerImgus();
> void registerCameras();
>
> std::shared_ptr<MediaDevice> cio2_;
> std::shared_ptr<MediaDevice> imgu_;
> +
> + IMGUDevice imgu0_;
> + IMGUDevice imgu1_;
> };
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/ipu3/meson.build b/src/libcamera/pipeline/ipu3/meson.build
> index fcb2d319d517..67f1a5e656d9 100644
> --- a/src/libcamera/pipeline/ipu3/meson.build
> +++ b/src/libcamera/pipeline/ipu3/meson.build
> @@ -1,4 +1,5 @@
> libcamera_sources += files([
> 'cio2.cpp',
> + 'imgu.cpp',
> 'ipu3.cpp',
> ])
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list