[libcamera-devel] [PATCH 12/13] libcamera: ipu3: Align how CIO2 and ImgU are stored in CameraData
Jacopo Mondi
jacopo at jmondi.org
Sat Jun 27 12:16:30 CEST 2020
On Sat, Jun 27, 2020 at 05:00:42AM +0200, Niklas Söderlund wrote:
> One is a pointer and the other is not. It is unintuitive to interact
> with and with our current design of one ImgU for each camera there is no
> advantage of having it as a pointer. Our current design is unlikely to
> change as the system really only has one ImgU. Align the two to make the
> pipeline more consistent.
Do you recall why we wanted to assign ImgU devices dynamically ?
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> src/libcamera/pipeline/ipu3/ipu3.cpp | 48 ++++++++++++----------------
> 1 file changed, 20 insertions(+), 28 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 0ebd762982142471..6ae4728b470f9487 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -43,7 +43,7 @@ public:
> void cio2BufferReady(FrameBuffer *buffer);
>
> CIO2Device cio2_;
> - ImgUDevice *imgu_;
> + ImgUDevice imgu_;
>
> Stream outStream_;
> Stream vfStream_;
> @@ -117,8 +117,6 @@ private:
> int allocateBuffers(Camera *camera);
> int freeBuffers(Camera *camera);
>
> - ImgUDevice imgu0_;
> - ImgUDevice imgu1_;
> MediaDevice *cio2MediaDev_;
> MediaDevice *imguMediaDev_;
> };
> @@ -414,7 +412,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> Stream *outStream = &data->outStream_;
> Stream *vfStream = &data->vfStream_;
> CIO2Device *cio2 = &data->cio2_;
> - ImgUDevice *imgu = data->imgu_;
> + ImgUDevice *imgu = &data->imgu_;
> V4L2DeviceFormat outputFormat;
> int ret;
>
> @@ -456,7 +454,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> * stream which is for raw capture, in which case no buffers will
> * ever be queued to the ImgU.
> */
> - ret = data->imgu_->enableLinks(true);
> + ret = data->imgu_.enableLinks(true);
> if (ret)
> return ret;
>
> @@ -563,9 +561,9 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
> unsigned int count = stream->configuration().bufferCount;
>
> if (stream == &data->outStream_)
> - return data->imgu_->output_.dev->exportBuffers(count, buffers);
> + return data->imgu_.output_.dev->exportBuffers(count, buffers);
> else if (stream == &data->vfStream_)
> - return data->imgu_->viewfinder_.dev->exportBuffers(count, buffers);
> + return data->imgu_.viewfinder_.dev->exportBuffers(count, buffers);
> else if (stream == &data->rawStream_)
> return data->cio2_.exportBuffers(count, buffers);
>
> @@ -583,7 +581,7 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
> int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
> {
> IPU3CameraData *data = cameraData(camera);
> - ImgUDevice *imgu = data->imgu_;
> + ImgUDevice *imgu = &data->imgu_;
> unsigned int bufferCount;
> int ret;
>
> @@ -604,7 +602,7 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)
> {
> IPU3CameraData *data = cameraData(camera);
>
> - data->imgu_->freeBuffers();
> + data->imgu_.freeBuffers();
>
> return 0;
> }
> @@ -613,7 +611,7 @@ int PipelineHandlerIPU3::start(Camera *camera)
> {
> IPU3CameraData *data = cameraData(camera);
> CIO2Device *cio2 = &data->cio2_;
> - ImgUDevice *imgu = data->imgu_;
> + ImgUDevice *imgu = &data->imgu_;
> int ret;
>
> /* Allocate buffers for internal pipeline usage. */
> @@ -650,7 +648,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> IPU3CameraData *data = cameraData(camera);
> int ret = 0;
>
> - ret |= data->imgu_->stop();
> + ret |= data->imgu_.stop();
> ret |= data->cio2_.stop();
> if (ret)
> LOG(IPU3, Warning) << "Failed to stop camera "
> @@ -680,9 +678,9 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
> int ret;
>
> if (stream == &data->outStream_)
> - ret = data->imgu_->output_.dev->queueBuffer(buffer);
> + ret = data->imgu_.output_.dev->queueBuffer(buffer);
> else if (stream == &data->vfStream_)
> - ret = data->imgu_->viewfinder_.dev->queueBuffer(buffer);
> + ret = data->imgu_.viewfinder_.dev->queueBuffer(buffer);
> else
> continue;
>
> @@ -757,16 +755,6 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
> */
> int PipelineHandlerIPU3::registerCameras()
> {
> - int ret;
> -
> - ret = imgu0_.init(imguMediaDev_, 0);
> - if (ret)
> - return ret;
> -
> - ret = imgu1_.init(imguMediaDev_, 1);
> - if (ret)
> - return ret;
> -
> /*
> * For each CSI-2 receiver on the IPU3, create a Camera if an
> * image sensor is connected to it and the sensor can produce images
> @@ -774,6 +762,8 @@ int PipelineHandlerIPU3::registerCameras()
> */
> unsigned int numCameras = 0;
> for (unsigned int id = 0; id < 4 && numCameras < 2; ++id) {
> + int ret;
> +
> std::unique_ptr<IPU3CameraData> data =
> std::make_unique<IPU3CameraData>(this);
> std::set<Stream *> streams = {
> @@ -796,7 +786,9 @@ int PipelineHandlerIPU3::registerCameras()
> * only, and assign imgu0 to the first one and imgu1 to the
> * second.
> */
/**
* \todo Dynamically assign ImgU and output devices to each
* stream and camera; as of now, limit support to two cameras
* only, and assign imgu0 to the first one and imgu1 to the
* second.
*/
Is this a leftover ? Should you remove it as well?
> - data->imgu_ = numCameras ? &imgu1_ : &imgu0_;
> + ret = data->imgu_.init(imguMediaDev_, numCameras);
> + if (ret)
> + return ret;
>
> /*
> * Connect video devices' 'bufferReady' signals to their
> @@ -808,11 +800,11 @@ int PipelineHandlerIPU3::registerCameras()
> */
> data->cio2_.bufferReady.connect(data.get(),
> &IPU3CameraData::cio2BufferReady);
> - data->imgu_->input_->bufferReady.connect(&data->cio2_,
> + data->imgu_.input_->bufferReady.connect(&data->cio2_,
> &CIO2Device::tryReturnBuffer);
> - data->imgu_->output_.dev->bufferReady.connect(data.get(),
> + data->imgu_.output_.dev->bufferReady.connect(data.get(),
> &IPU3CameraData::imguOutputBufferReady);
> - data->imgu_->viewfinder_.dev->bufferReady.connect(data.get(),
> + data->imgu_.viewfinder_.dev->bufferReady.connect(data.get(),
> &IPU3CameraData::imguOutputBufferReady);
>
> /* Create and register the Camera instance. */
> @@ -881,7 +873,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
> return;
> }
>
> - imgu_->input_->queueBuffer(buffer);
> + imgu_.input_->queueBuffer(buffer);
> }
>
> REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list