| Message ID | 20200627030043.2088585-13-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thanks for your feedback. On 2020-06-27 12:16:30 +0200, Jacopo Mondi wrote: > 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 ? We had ideas of providing more then 2 streams per camera IIRC. This might still happen but would likely be quiet an big change to the pipeline so I see no reason to keep this small part in preparation for this. Likely this would change in any case if we ever go down that route. > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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? Good point I will remove it, thanks. > > > - 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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, Thank you for the patch. On Sat, Jun 27, 2020 at 12:43:33PM +0200, Niklas Söderlund wrote: > On 2020-06-27 12:16:30 +0200, Jacopo Mondi wrote: > > 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 ? > > We had ideas of providing more then 2 streams per camera IIRC. This > might still happen but would likely be quiet an big change to the > pipeline so I see no reason to keep this small part in preparation for > this. Likely this would change in any case if we ever go down that > route. Correct, we thought we could use two ImgU instances for a single camera (and we likely will need to do so in the future). I also agree that more changes will be needed. > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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_; I'm not sure to like this much though. For all it's worth, I would have gone the other way around, make cio2_ a pointer too. There are 4 CIO2 devices, and we could thus support more than two cameras, provided they're not all used at the same time. It would make sense to instantiate 4 CIO2 devices in PipelineHandlerIPU3, and point to the corresponding CIO2 in IPU3CameraData. That's most likely what we will end up doing, and if we embed both the CIO2 and IMGU in IPU3CameraData we will then make more code changes that will rely on that design, making future cleanups more difficult. I also don't really see this patch performing any change that allows further simplifications in the rest of the series (as far as I can tell, 13/13 doesn't depend on it). I'd thus rather keep the current design, and if you want to unify the cio2_ and imgu_ field, replace the patch with one that would turn cio2_ into a pointer. > > > 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? > > Good point I will remove it, thanks. > > > > - 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);
Hi Laurent, Thanks for your feedback. On 2020-06-27 19:48:34 +0300, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Sat, Jun 27, 2020 at 12:43:33PM +0200, Niklas Söderlund wrote: > > On 2020-06-27 12:16:30 +0200, Jacopo Mondi wrote: > > > 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 ? > > > > We had ideas of providing more then 2 streams per camera IIRC. This > > might still happen but would likely be quiet an big change to the > > pipeline so I see no reason to keep this small part in preparation for > > this. Likely this would change in any case if we ever go down that > > route. > > Correct, we thought we could use two ImgU instances for a single camera > (and we likely will need to do so in the future). I also agree that more > changes will be needed. > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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_; > > I'm not sure to like this much though. For all it's worth, I would have > gone the other way around, make cio2_ a pointer too. There are 4 CIO2 > devices, and we could thus support more than two cameras, provided > they're not all used at the same time. It would make sense to > instantiate 4 CIO2 devices in PipelineHandlerIPU3, and point to the > corresponding CIO2 in IPU3CameraData. That's most likely what we will > end up doing, and if we embed both the CIO2 and IMGU in IPU3CameraData > we will then make more code changes that will rely on that design, > making future cleanups more difficult. I also don't really see this > patch performing any change that allows further simplifications in the > rest of the series (as far as I can tell, 13/13 doesn't depend on it). > I'd thus rather keep the current design, and if you want to unify the > cio2_ and imgu_ field, replace the patch with one that would turn cio2_ > into a pointer. You are correct that 13/13 does not depend on this. This was something I spotted while working with the series which felt unnatural that the CIO2 and ImgU where handled differently. I find it unlikely that we will extend the IPU3 pipeline with support for more then two cameras before we have at least added an basic IPA and and sorted out a lot of other things. Likewise I think it will be a while before we attempt to have a singe camera use more then one ImgU, if ever. I think code should reflect the current usage and take into account future planed work which are schedule to happen and not theoretical use-cases that are not on the horizon :-) I fear this is a bikesheeding issue and the patch itself does not add or remove much value from the real work in this series so I will drop it for v2. > > > > > 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? > > > > Good point I will remove it, thanks. > > > > > > - 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); > > -- > Regards, > > Laurent Pinchart
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. */ - 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);
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. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 48 ++++++++++++---------------- 1 file changed, 20 insertions(+), 28 deletions(-)