[libcamera-devel] [PATCH 01/10] libcamera: ipu3: Group CIO2 devices
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Mar 2 22:04:44 CET 2019
Hi Jacopo,
Thank you for the patch.
On Thu, Feb 28, 2019 at 09:04:01PM +0100, Jacopo Mondi wrote:
> Group CIO2 devices (cio2, csi2 and image sensor) in a structure
> associated with the CameraData, to ease management of the CIO2 devices.
>
> Update the IPU3 pipeline handler implementation to avoid name clashes
> and break-out IPU3CameraData from the pipeline handler class for
> clarity.
I'm not sure that really brings clarity :-) I would have kept the
IPU3CameraData structure where it was, but preferences differ of course.
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> src/libcamera/pipeline/ipu3/ipu3.cpp | 251 ++++++++++++++-------------
> 1 file changed, 133 insertions(+), 118 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 9694d0ce51ab..d3f1d9a95f81 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -24,6 +24,30 @@ namespace libcamera {
>
> LOG_DEFINE_CATEGORY(IPU3)
>
> +struct Cio2Device {
Should this be called CIO2Device ?
> + Cio2Device()
> + : output(nullptr), csi2(nullptr), sensor(nullptr) {}
I'd open the {} on the next two lines for consistency, up to you.
> +
> + ~Cio2Device()
> + {
> + delete output;
> + delete csi2;
> + delete sensor;
> + }
I'm not very fond on having part of the logic in the PipelineHandlerIPU3
class (the initCio2() function - I know I asked for it to be moved
there) part here in the destructor :-/ Splitting it in two doesn't feel
really right, I'd move these three delete statements to the
IPU3CameraData destructor. What do you think ?
> +
> + V4L2Device *output;
> + V4L2Subdevice *csi2;
> + V4L2Subdevice *sensor;
> +};
> +
> +class IPU3CameraData : public CameraData
> +{
> +public:
> + Cio2Device cio2;
> +
> + Stream stream_;
> +};
> +
> class PipelineHandlerIPU3 : public PipelineHandler
> {
> public:
> @@ -47,65 +71,47 @@ public:
> bool match(DeviceEnumerator *enumerator);
>
> private:
> - class IPU3CameraData : public CameraData
> - {
> - public:
> - IPU3CameraData()
> - : cio2_(nullptr), csi2_(nullptr), sensor_(nullptr) {}
> -
> - ~IPU3CameraData()
> - {
> - delete cio2_;
> - delete csi2_;
> - delete sensor_;
> - }
> -
> - V4L2Device *cio2_;
> - V4L2Subdevice *csi2_;
> - V4L2Subdevice *sensor_;
> -
> - Stream stream_;
> - };
> -
> IPU3CameraData *cameraData(const Camera *camera)
> {
> return static_cast<IPU3CameraData *>(
> PipelineHandler::cameraData(camera));
> }
>
> + int initCio2(unsigned int index, Cio2Device *cio2);
initCIO2() ?
> void registerCameras();
>
> - std::shared_ptr<MediaDevice> cio2_;
> - std::shared_ptr<MediaDevice> imgu_;
> + std::shared_ptr<MediaDevice> cio2MediaDev_;
> + std::shared_ptr<MediaDevice> imguMediaDev_;
Are these renames really needed ?
> };
>
> PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)
> - : PipelineHandler(manager), cio2_(nullptr), imgu_(nullptr)
> + : PipelineHandler(manager), cio2MediaDev_(nullptr), imguMediaDev_(nullptr)
> {
> }
>
> PipelineHandlerIPU3::~PipelineHandlerIPU3()
> {
> - if (cio2_)
> - cio2_->release();
> + if (cio2MediaDev_)
> + cio2MediaDev_->release();
>
> - if (imgu_)
> - imgu_->release();
> + if (imguMediaDev_)
> + imguMediaDev_->release();
> }
>
> std::map<Stream *, StreamConfiguration>
> PipelineHandlerIPU3::streamConfiguration(Camera *camera,
> std::vector<Stream *> &streams)
> {
> - IPU3CameraData *data = cameraData(camera);
> std::map<Stream *, StreamConfiguration> configs;
> + IPU3CameraData *data = cameraData(camera);
> + V4L2Subdevice *sensor = data->cio2.sensor;
> V4L2SubdeviceFormat format = {};
>
> /*
> * FIXME: As of now, return the image format reported by the sensor.
> * In future good defaults should be provided for each stream.
> */
> - if (data->sensor_->getFormat(0, &format)) {
> + if (sensor->getFormat(0, &format)) {
> LOG(IPU3, Error) << "Failed to create stream configurations";
> return configs;
> }
> @@ -126,9 +132,9 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
> {
> IPU3CameraData *data = cameraData(camera);
> StreamConfiguration *cfg = &config[&data->stream_];
> - V4L2Subdevice *sensor = data->sensor_;
> - V4L2Subdevice *csi2 = data->csi2_;
> - V4L2Device *cio2 = data->cio2_;
> + V4L2Subdevice *sensor = data->cio2.sensor;
> + V4L2Subdevice *csi2 = data->cio2.csi2;
> + V4L2Device *cio2 = data->cio2.output;
> V4L2SubdeviceFormat subdevFormat = {};
> V4L2DeviceFormat devFormat = {};
> int ret;
> @@ -185,13 +191,14 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
>
> int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)
> {
> - IPU3CameraData *data = cameraData(camera);
> const StreamConfiguration &cfg = stream->configuration();
> + IPU3CameraData *data = cameraData(camera);
> + V4L2Device *cio2 = data->cio2.output;
>
> if (!cfg.bufferCount)
> return -EINVAL;
>
> - int ret = data->cio2_->exportBuffers(&stream->bufferPool());
> + int ret = cio2->exportBuffers(&stream->bufferPool());
> if (ret) {
> LOG(IPU3, Error) << "Failed to request memory";
> return ret;
> @@ -203,8 +210,9 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, Stream *stream)
> int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)
> {
> IPU3CameraData *data = cameraData(camera);
> + V4L2Device *cio2 = data->cio2.output;
>
> - int ret = data->cio2_->releaseBuffers();
> + int ret = cio2->releaseBuffers();
> if (ret) {
> LOG(IPU3, Error) << "Failed to release memory";
> return ret;
> @@ -216,9 +224,10 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)
> int PipelineHandlerIPU3::start(const Camera *camera)
> {
> IPU3CameraData *data = cameraData(camera);
> + V4L2Device *cio2 = data->cio2.output;
> int ret;
>
> - ret = data->cio2_->streamOn();
> + ret = cio2->streamOn();
> if (ret) {
> LOG(IPU3, Info) << "Failed to start camera " << camera->name();
> return ret;
> @@ -230,14 +239,16 @@ int PipelineHandlerIPU3::start(const Camera *camera)
> void PipelineHandlerIPU3::stop(const Camera *camera)
> {
> IPU3CameraData *data = cameraData(camera);
> + V4L2Device *cio2 = data->cio2.output;
>
> - if (data->cio2_->streamOff())
> + if (cio2->streamOff())
> LOG(IPU3, Info) << "Failed to stop camera " << camera->name();
> }
>
> int PipelineHandlerIPU3::queueRequest(const Camera *camera, Request *request)
> {
> IPU3CameraData *data = cameraData(camera);
> + V4L2Device *cio2 = data->cio2.output;
> Stream *stream = &data->stream_;
>
> Buffer *buffer = request->findBuffer(stream);
> @@ -247,7 +258,7 @@ int PipelineHandlerIPU3::queueRequest(const Camera *camera, Request *request)
> return -ENOENT;
> }
>
> - data->cio2_->queueBuffer(buffer);
> + cio2->queueBuffer(buffer);
>
> return 0;
> }
> @@ -278,20 +289,20 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> imgu_dm.add("ipu3-imgu 1 viewfinder");
> imgu_dm.add("ipu3-imgu 1 3a stat");
>
> - cio2_ = enumerator->search(cio2_dm);
> - if (!cio2_)
> + cio2MediaDev_ = enumerator->search(cio2_dm);
> + if (!cio2MediaDev_)
> return false;
>
> - imgu_ = enumerator->search(imgu_dm);
> - if (!imgu_)
> + imguMediaDev_ = enumerator->search(imgu_dm);
> + if (!imguMediaDev_)
> return false;
>
> /*
> * It is safe to acquire both media devices at this point as
> * DeviceEnumerator::search() skips the busy ones for us.
> */
> - cio2_->acquire();
> - imgu_->acquire();
> + cio2MediaDev_->acquire();
> + imguMediaDev_->acquire();
>
> /*
> * Disable all links that are enabled by default on CIO2, as camera
> @@ -300,28 +311,90 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> * Close the CIO2 media device after, as links are enabled and should
> * not need to be changed after.
> */
> - if (cio2_->open())
> + if (cio2MediaDev_->open())
> goto error_release_mdev;
>
> - if (cio2_->disableLinks())
> + if (cio2MediaDev_->disableLinks())
> goto error_close_cio2;
>
> registerCameras();
>
> - cio2_->close();
> + cio2MediaDev_->close();
>
> return true;
>
> error_close_cio2:
> - cio2_->close();
> + cio2MediaDev_->close();
>
> error_release_mdev:
> - cio2_->release();
> - imgu_->release();
> + cio2MediaDev_->release();
> + imguMediaDev_->release();
>
> return false;
> }
>
This pipeline handler will likely be used as a reference implementation
in the future, could you add more documentation, even for private
functions ?
> +int PipelineHandlerIPU3::initCio2(unsigned int index, Cio2Device *cio2)
> +{
> + int ret;
> +
> + /* Verify a sensor subdevice is connected to this CIO2 instance. */
> + std::string csi2Name = "ipu3-csi2 " + std::to_string(index);
> + MediaEntity *entity = cio2MediaDev_->getEntityByName(csi2Name);
> + if (!entity) {
> + LOG(IPU3, Error)
> + << "Failed to get entity '" << csi2Name << "'";
> + return -ENODEV;
> + }
> +
> + const std::vector<MediaPad *> &pads = entity->pads();
> + if (pads.empty())
> + return -EINVAL;
> +
> + /* IPU3 CSI-2 receivers have a single sink pad at index 0. */
> + MediaPad *sink = pads[0];
> + const std::vector<MediaLink *> &links = sink->links();
> + if (links.empty())
> + return -EINVAL;
> +
> + MediaLink *link = links[0];
> + MediaEntity *sensorEntity = link->source()->entity();
> + if (sensorEntity->function() != MEDIA_ENT_F_CAM_SENSOR)
> + return -ENODEV;
> +
> + ret = link->setEnabled(true);
> + if (ret)
> + return ret;
> +
> + /*
> + * Now that we're sure a sensor subdevice is connected, create and open
> + * video devices and subdevices associated with this CIO2 unit.
> + */
> + cio2->sensor = new V4L2Subdevice(sensorEntity);
> + ret = cio2->sensor->open();
> + if (ret)
> + return ret;
> +
> + cio2->csi2 = new V4L2Subdevice(entity);
> + ret = cio2->csi2->open();
> + if (ret)
> + 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();
Note for later, we shouldn't keep device nodes open when they're not
needed. Depending on the kernel drivers, it may consume more power.
Could you add a \todo ?
> + if (ret)
> + 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
> @@ -329,85 +402,27 @@ error_release_mdev:
> */
> void PipelineHandlerIPU3::registerCameras()
> {
> + int ret;
> +
> /*
> * For each CSI-2 receiver on the IPU3, create a Camera if an
> * image sensor is connected to it.
> */
> unsigned int numCameras = 0;
> for (unsigned int id = 0; id < 4; ++id) {
> - std::string csi2Name = "ipu3-csi2 " + std::to_string(id);
> - MediaEntity *csi2 = cio2_->getEntityByName(csi2Name);
> - int ret;
You could keep ret within the loop.
> -
> - /*
> - * This shall not happen, as the device enumerator matched
> - * all entities described in the cio2_dm DeviceMatch.
> - *
> - * As this check is basically free, better stay safe than sorry.
> - */
> - if (!csi2)
> - continue;
> -
> - const std::vector<MediaPad *> &pads = csi2->pads();
> - if (pads.empty())
> - continue;
> -
> - /* IPU3 CSI-2 receivers have a single sink pad at index 0. */
> - MediaPad *sink = pads[0];
> - const std::vector<MediaLink *> &links = sink->links();
> - if (links.empty())
> - continue;
> -
> - /*
> - * Verify that the receiver is connected to a sensor, enable
> - * the media link between the two, and create a Camera with
> - * a unique name.
> - */
> - MediaLink *link = links[0];
> - MediaEntity *sensor = link->source()->entity();
> - if (sensor->function() != MEDIA_ENT_F_CAM_SENSOR)
> - continue;
> -
> - if (link->setEnabled(true))
> - continue;
> -
> - std::unique_ptr<IPU3CameraData> data = utils::make_unique<IPU3CameraData>();
> -
> - std::string cameraName = sensor->name() + " " + std::to_string(id);
> + std::unique_ptr<IPU3CameraData> data =
> + utils::make_unique<IPU3CameraData>();
> std::vector<Stream *> streams{ &data->stream_ };
> - std::shared_ptr<Camera> camera = Camera::create(this, cameraName, streams);
> -
> - /*
> - * Create and open video devices and subdevices associated with
> - * the camera.
> - *
> - * If any of these operations fails, the Camera instance won't
> - * be registered. The 'camera' shared pointer and the 'data'
> - * unique pointers go out of scope and delete the objects they
> - * manage.
> - */
> - std::string cio2Name = "ipu3-cio2 " + std::to_string(id);
> - MediaEntity *cio2 = cio2_->getEntityByName(cio2Name);
> - if (!cio2) {
> - LOG(IPU3, Error)
> - << "Failed to get entity '" << cio2Name << "'";
> - continue;
> - }
> -
> - data->cio2_ = new V4L2Device(cio2);
> - ret = data->cio2_->open();
> - if (ret)
> - continue;
> + Cio2Device *cio2 = &data->cio2;
>
No need for a blank line here.
> - data->sensor_ = new V4L2Subdevice(sensor);
> - ret = data->sensor_->open();
> + ret = initCio2(id, cio2);
> if (ret)
> continue;
>
> - data->csi2_ = new V4L2Subdevice(csi2);
> - ret = data->csi2_->open();
> - if (ret)
> - continue;
> + std::string cameraName = cio2->sensor->deviceName() + " "
> + + std::to_string(id);
> + std::shared_ptr<Camera> camera =
> + Camera::create(this, cameraName, streams);
>
> setCameraData(camera.get(), std::move(data));
> registerCamera(std::move(camera));
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list