[libcamera-devel] [PATCH 03/10] libcamera: ipu3: Initialize and link ImgU devices
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Mar 2 22:41:29 CET 2019
Hi Jacopo,
Thank you for the patch.
On Thu, Feb 28, 2019 at 09:04:03PM +0100, Jacopo Mondi wrote:
> Create video devices and subdevices associated with an ImgU unit, and
> link the entities in the media graph to prepare the device for capture
> operations at stream configuration time.
>
> As we support a single stream at the moment, always select imgu0.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> src/libcamera/pipeline/ipu3/ipu3.cpp | 219 +++++++++++++++++++++++++--
> 1 file changed, 207 insertions(+), 12 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 4f1ab72debf8..9fa59c1bc97e 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -24,6 +24,28 @@ namespace libcamera {
>
> LOG_DEFINE_CATEGORY(IPU3)
>
> +struct ImguDevice {
s/ImguDevice/ImgUDevice/ ?
> + ImguDevice()
> + : imgu(nullptr), input(nullptr), output(nullptr),
> + viewfinder(nullptr), stat(nullptr) {}
{
}
> +
> + ~ImguDevice()
> + {
> + delete imgu;
> + delete input;
> + delete output;
> + delete viewfinder;
> + delete stat;
> + }
Unlike for the CIO2Device, this could be made a class, as I expect it
will have more code associated with it, and the ImgU instances will be
shared between the multiple camera instances. The linkImgu function
would be a good candidate.
> +
> + V4L2Subdevice *imgu;
> + V4L2Device *input;
> + V4L2Device *output;
> + V4L2Device *viewfinder;
> + V4L2Device *stat;
> + /* TODO: add param video device for 3A tuning */
\todo for consistency, even if this isn't a doxygen comment ?
> +};
> +
> struct Cio2Device {
> Cio2Device()
> : output(nullptr), csi2(nullptr), sensor(nullptr) {}
> @@ -44,6 +66,7 @@ class IPU3CameraData : public CameraData
> {
> public:
> Cio2Device cio2;
> + ImguDevice *imgu;
>
> Stream stream_;
> };
> @@ -71,6 +94,10 @@ public:
> bool match(DeviceEnumerator *enumerator);
>
> private:
> + 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;
> static constexpr unsigned int IPU3_BUF_NUM = 4;
I'd move that to the ImgUDevice struct/class. You can then remove the
IMGU_ prefix.
>
> IPU3CameraData *cameraData(const Camera *camera)
> @@ -79,9 +106,17 @@ private:
> PipelineHandler::cameraData(camera));
> }
>
> + int linkImgu(ImguDevice *imgu);
> +
> + V4L2Device *openDevice(MediaDevice *media, std::string &name);
> + V4L2Subdevice *openSubdevice(MediaDevice *media, std::string &name);
> + int initImgu(ImguDevice *imgu);
> int initCio2(unsigned int index, Cio2Device *cio2);
> void registerCameras();
>
> + ImguDevice imgu0_;
> + ImguDevice imgu1_;
Maybe an array with two entries ?
> +
> std::shared_ptr<MediaDevice> cio2MediaDev_;
> std::shared_ptr<MediaDevice> imguMediaDev_;
> };
> @@ -159,6 +194,15 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
> V4L2DeviceFormat devFormat = {};
> int ret;
>
> + /*
> + * TODO: dynamically assign ImgU devices; as of now, with a single
> + * stream supported, always use 'imgu0'.
/**
* \todo
?
> + */
> + data->imgu = &imgu0_;
How about moving this to camera creation time, and assigned imgu0 to the
first sensor and imgu1 to the second one ?
> + ret = linkImgu(data->imgu);
> + if (ret)
> + return ret;
> +
> /*
> * FIXME: as of now, the format gets applied to the sensor and is
> * propagated along the pipeline. It should instead be applied on the
> @@ -334,17 +378,29 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> if (cio2MediaDev_->open())
> goto error_release_mdev;
>
> + if (imguMediaDev_->open())
> + goto error_close_mdev;
> +
> if (cio2MediaDev_->disableLinks())
> - goto error_close_cio2;
> + goto error_close_mdev;
> +
> + if (initImgu(&imgu0_))
> + goto error_close_mdev;
> +
> + if (initImgu(&imgu1_))
> + goto error_close_mdev;
> +
>
> registerCameras();
>
> cio2MediaDev_->close();
> + imguMediaDev_->close();
>
> return true;
>
> -error_close_cio2:
> +error_close_mdev:
> cio2MediaDev_->close();
> + imguMediaDev_->close();
Error handling will be simplified when you'll rebase. I'd go as far as
calling close() in the PipelineHandlerIPU3 destructor and remove the
error path here.
>
> error_release_mdev:
> cio2MediaDev_->release();
> @@ -353,6 +409,153 @@ error_release_mdev:
> return false;
> }
>
> +/* Link entities in the ImgU unit to prepare for capture operations. */
> +int PipelineHandlerIPU3::linkImgu(ImguDevice *imguDevice)
> +{
> + MediaLink *link;
> + int ret;
> +
> + unsigned int index = imguDevice == &imgu0_ ? 0 : 1;
> + std::string imguName = "ipu3-imgu " + std::to_string(index);
> + std::string inputName = imguName + " input";
> + std::string outputName = imguName + " output";
> + std::string viewfinderName = imguName + " viewfinder";
> + std::string statName = imguName + " 3a stat";
> +
> + ret = imguMediaDev_->open();
> + if (ret)
> + return ret;
> +
> + ret = imguMediaDev_->disableLinks();
This isn't a good idea, as you will interfere with the other ImgU. You
should enable/disable links selectively based on your needs.
> + if (ret) {
> + imguMediaDev_->close();
> + return ret;
> + }
> +
> + /* Link entities to configure the IMGU unit for capture. */
> + link = imguMediaDev_->link(inputName, 0, imguName, IMGU_PAD_INPUT);
> + if (!link) {
> + LOG(IPU3, Error)
> + << "Failed to get link '" << inputName << "':0 -> '"
> + << imguName << "':0";
Ideally you should use the IMGU_PAD_* constants instead of hardcoding
them in error messages.
> + ret = -ENODEV;
> + goto error_close_mediadev;
> + }
> + link->setEnabled(true);
> +
> + link = imguMediaDev_->link(imguName, IMGU_PAD_OUTPUT, outputName, 0);
> + if (!link) {
> + LOG(IPU3, Error)
> + << "Failed to get link '" << imguName << "':2 -> '"
> + << outputName << "':0";
> + ret = -ENODEV;
> + goto error_close_mediadev;
> + }
> + link->setEnabled(true);
> +
> + link = imguMediaDev_->link(imguName, IMGU_PAD_VF, viewfinderName, 0);
> + if (!link) {
> + LOG(IPU3, Error)
> + << "Failed to get link '" << imguName << "':3 -> '"
> + << viewfinderName << "':0";
> + ret = -ENODEV;
> + goto error_close_mediadev;
> + }
> + link->setEnabled(true);
We have a single stream, why do you enable both output and viewfinder ?
> +
> + link = imguMediaDev_->link(imguName, IMGU_PAD_STAT, statName, 0);
> + if (!link) {
> + LOG(IPU3, Error)
> + << "Failed to get link '" << imguName << "':4 -> '"
> + << statName << "':0";
> + ret = -ENODEV;
> + goto error_close_mediadev;
> + }
> + link->setEnabled(true);
Same here, we don't make use of stats yes, there's no need to enable
this link.
> +
> + imguMediaDev_->close();
> +
> + return 0;
> +
> +error_close_mediadev:
> + imguMediaDev_->close();
We really really need to think about how to handle open/close and write
down a set of rules. Please make sure this is captured in a \todo.
> +
> + return ret;
> +
> +}
> +
> +V4L2Device *PipelineHandlerIPU3::openDevice(MediaDevice *media,
> + std::string &name)
const std::string &
Same below.
> +{
> + V4L2Device *dev;
You can move this line down to declare and initialize the variable at
the same time.
> +
> + MediaEntity *entity = media->getEntityByName(name);
> + if (!entity) {
> + LOG(IPU3, Error)
> + << "Failed to get entity '" << name << "'";
> + return nullptr;
> + }
> +
> + dev = new V4L2Device(entity);
> + if (dev->open())
> + return nullptr;
You'll leak dev, a delete is needed.
> +
> + return dev;
> +}
> +
> +V4L2Subdevice *PipelineHandlerIPU3::openSubdevice(MediaDevice *media,
> + std::string &name)
> +{
> + V4L2Subdevice *dev;
> +
> + MediaEntity *entity = media->getEntityByName(name);
> + if (!entity) {
> + LOG(IPU3, Error)
> + << "Failed to get entity '" << name << "'";
> + return nullptr;
> + }
> +
> + dev = new V4L2Subdevice(entity);
> + if (dev->open())
Leak.
> + return nullptr;
> +
> + return dev;
> +}
These two functions may be candidates for future generic helpers. Could
you document them and add a \todo to mention this ?
> +
> +/* Create video devices and subdevices for the ImgU instance. */
> +int PipelineHandlerIPU3::initImgu(ImguDevice *imgu)
> +{
> + unsigned int index = imgu == &imgu0_ ? 0 : 1;
A bit ugly, how about adding an index field to the ImguDevice structure
?
> + std::string imguName = "ipu3-imgu " + std::to_string(index);
> + std::string devName;
> +
> + imgu->imgu = openSubdevice(imguMediaDev_.get(), imguName);
> + if (!imgu->imgu)
> + return -ENODEV;
> +
> + devName = imguName + " input";
> + imgu->input = openDevice(imguMediaDev_.get(), devName);
> + if (!imgu->input)
> + return -ENODEV;
> +
> + devName = imguName + " output";
> + imgu->output = openDevice(imguMediaDev_.get(), devName);
> + if (!imgu->output)
> + return -ENODEV;
> +
> + devName = imguName + " viewfinder";
> + imgu->viewfinder = openDevice(imguMediaDev_.get(), devName);
> + if (!imgu->viewfinder)
> + return -ENODEV;
> +
> + devName = imguName + " 3a stat";
> + imgu->stat = openDevice(imguMediaDev_.get(), devName);
You could drop devName and call
imgu->state = openDevice(imguMediaDev_.get(), imguName + " 3a stat");
Up to you.
> + if (!imgu->stat)
> + return -ENODEV;
> +
> + return 0;
> +}
> +
> int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2)
> {
> int ret;
> @@ -400,16 +603,8 @@ int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2)
> return ret;
>
> std::string cio2Name = "ipu3-cio2 " + std::to_string(index);
> - entity = cio2MediaDev_->getEntityByName(cio2Name);
> - if (!entity) {
> - LOG(IPU3, Error)
> - << "Failed to get entity '" << cio2Name << "'";
> - return -EINVAL;
> - }
> -
> - cio2->output = new V4L2Device(entity);
> - ret = cio2->output->open();
> - if (ret)
> + cio2->output = openDevice(cio2MediaDev_.get(), cio2Name);
> + if (!cio2->output)
> return ret;
>
> return 0;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list