| Message ID | 20200623020930.1781469-9-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On Tue, Jun 23, 2020 at 04:09:28AM +0200, Niklas Söderlund wrote: > In order to make the CIO2 easier to extend with new features make the > V4L2 devices (sensor, CIO2 and video device) private members. This > requires a few helper functions to be added to allow for the IPU3 driver > to still be able to interact with all parts of the CIO2. These helper > functions will later be extended to add new features to the IPU3 > pipeline. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> Much better than accessing parts of the CIO2 device from the other components. > --- > * Changes since v2 > - Style changes. > > * Changes since v1 > - Drop sensor access helpers and replace with a sensor() call to get > hold of the CameraSensor pointer directly. > --- > src/libcamera/pipeline/ipu3/cio2.cpp | 20 +++++++++++++++++++- > src/libcamera/pipeline/ipu3/cio2.h | 17 ++++++++++++++--- > src/libcamera/pipeline/ipu3/ipu3.cpp | 19 ++++++++----------- > 3 files changed, 41 insertions(+), 15 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > index d6bab896706dd60e..3d7348782b0fa6cb 100644 > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > @@ -38,7 +38,7 @@ static const std::map<uint32_t, MbusInfo> mbusCodesToInfo = { > } /* namespace */ > > CIO2Device::CIO2Device() > - : output_(nullptr), csi2_(nullptr), sensor_(nullptr) > + : sensor_(nullptr), csi2_(nullptr), output_(nullptr) > { > } > > @@ -126,6 +126,8 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > if (ret) > return ret; > > + output_->bufferReady.connect(this, &CIO2Device::cio2BufferReady); There's a bit of signal ping-pong here... Could this be avoided by providing a registerBufferReady() method that the pipielinehandler can call and provide a slot to connect to the video device signal ? I can't actually tell how bad this is tbh > + > return 0; > } > > @@ -226,6 +228,12 @@ int CIO2Device::allocateBuffers() > return ret; > } > > +int CIO2Device::exportBuffers(unsigned int count, > + std::vector<std::unique_ptr<FrameBuffer>> *buffers) > +{ > + return output_->exportBuffers(count, buffers); > +} > + > void CIO2Device::freeBuffers() > { > /* The default std::queue constructor is explicit with gcc 5 and 6. */ > @@ -266,4 +274,14 @@ int CIO2Device::stop() > return output_->streamOff(); > } > > +int CIO2Device::queueBuffer(FrameBuffer *buffer) > +{ > + return output_->queueBuffer(buffer); > +} > + > +void CIO2Device::cio2BufferReady(FrameBuffer *buffer) > +{ > + bufferReady.emit(buffer); > +} > + > } /* namespace libcamera */ > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h > index 6276573f2b585b25..cc898f13fd73f865 100644 > --- a/src/libcamera/pipeline/ipu3/cio2.h > +++ b/src/libcamera/pipeline/ipu3/cio2.h > @@ -11,6 +11,8 @@ > #include <queue> > #include <vector> > > +#include <libcamera/signal.h> > + > namespace libcamera { > > class CameraSensor; > @@ -36,6 +38,8 @@ public: > StreamConfiguration generateConfiguration(const Size &desiredSize) const; > > int allocateBuffers(); > + int exportBuffers(unsigned int count, > + std::vector<std::unique_ptr<FrameBuffer>> *buffers); > void freeBuffers(); > > FrameBuffer *getBuffer(); > @@ -44,11 +48,18 @@ public: > int start(); > int stop(); > > - V4L2VideoDevice *output_; > - V4L2Subdevice *csi2_; > - CameraSensor *sensor_; > + CameraSensor *sensor() { return sensor_; } > + > + int queueBuffer(FrameBuffer *buffer); > + Signal<FrameBuffer *> bufferReady; > > private: > + void cio2BufferReady(FrameBuffer *buffer); > + > + CameraSensor *sensor_; > + V4L2Subdevice *csi2_; > + V4L2VideoDevice *output_; > + > std::vector<std::unique_ptr<FrameBuffer>> buffers_; > std::queue<FrameBuffer *> availableBuffers_; > }; > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index c0e727e592f46883..2d1ec707ea4b08fe 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -431,7 +431,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, > > stream = &data->rawStream_; > > - cfg.size = data->cio2_.sensor_->resolution(); > + cfg.size = data->cio2_.sensor()->resolution(); > > cfg = data->cio2_.generateConfiguration(cfg.size); > break; > @@ -460,7 +460,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, > * available sensor resolution and to the IPU3 > * alignment constraints. > */ > - const Size &res = data->cio2_.sensor_->resolution(); > + const Size &res = data->cio2_.sensor()->resolution(); This could actually be Cio2Device::resolution(). Up to you. Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > unsigned int width = std::min(1280U, res.width); > unsigned int height = std::min(720U, res.height); > cfg.size = { width & ~7, height & ~3 }; > @@ -640,14 +640,11 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream, > IPU3CameraData *data = cameraData(camera); > IPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream); > unsigned int count = stream->configuration().bufferCount; > - V4L2VideoDevice *video; > > if (ipu3stream->raw_) > - video = data->cio2_.output_; > - else > - video = ipu3stream->device_->dev; > + return data->cio2_.exportBuffers(count, buffers); > > - return video->exportBuffers(count, buffers); > + return ipu3stream->device_->dev->exportBuffers(count, buffers); > } > > /** > @@ -757,7 +754,7 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request) > return -EINVAL; > > buffer->setRequest(request); > - data->cio2_.output_->queueBuffer(buffer); > + data->cio2_.queueBuffer(buffer); > > for (auto it : request->buffers()) { > IPU3Stream *stream = static_cast<IPU3Stream *>(it.first); > @@ -870,7 +867,7 @@ int PipelineHandlerIPU3::registerCameras() > continue; > > /* Initialize the camera properties. */ > - data->properties_ = cio2->sensor_->properties(); > + data->properties_ = cio2->sensor()->properties(); > > /** > * \todo Dynamically assign ImgU and output devices to each > @@ -894,7 +891,7 @@ int PipelineHandlerIPU3::registerCameras() > * associated ImgU input where they get processed and > * returned through the ImgU main and secondary outputs. > */ > - data->cio2_.output_->bufferReady.connect(data.get(), > + data->cio2_.bufferReady.connect(data.get(), > &IPU3CameraData::cio2BufferReady); > data->imgu_->input_->bufferReady.connect(data.get(), > &IPU3CameraData::imguInputBufferReady); > @@ -904,7 +901,7 @@ int PipelineHandlerIPU3::registerCameras() > &IPU3CameraData::imguOutputBufferReady); > > /* Create and register the Camera instance. */ > - std::string cameraName = cio2->sensor_->entity()->name(); > + std::string cameraName = cio2->sensor()->entity()->name(); > std::shared_ptr<Camera> camera = Camera::create(this, > cameraName, > streams); > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo and Niklas, On Wed, Jun 24, 2020 at 12:31:10PM +0200, Jacopo Mondi wrote: > On Tue, Jun 23, 2020 at 04:09:28AM +0200, Niklas Söderlund wrote: > > In order to make the CIO2 easier to extend with new features make the > > V4L2 devices (sensor, CIO2 and video device) private members. This > > requires a few helper functions to be added to allow for the IPU3 driver > > to still be able to interact with all parts of the CIO2. These helper > > functions will later be extended to add new features to the IPU3 > > pipeline. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > Much better than accessing parts of the CIO2 device from the other > components. > > > --- > > * Changes since v2 > > - Style changes. > > > > * Changes since v1 > > - Drop sensor access helpers and replace with a sensor() call to get > > hold of the CameraSensor pointer directly. > > --- > > src/libcamera/pipeline/ipu3/cio2.cpp | 20 +++++++++++++++++++- > > src/libcamera/pipeline/ipu3/cio2.h | 17 ++++++++++++++--- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 19 ++++++++----------- > > 3 files changed, 41 insertions(+), 15 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > > index d6bab896706dd60e..3d7348782b0fa6cb 100644 > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > > @@ -38,7 +38,7 @@ static const std::map<uint32_t, MbusInfo> mbusCodesToInfo = { > > } /* namespace */ > > > > CIO2Device::CIO2Device() > > - : output_(nullptr), csi2_(nullptr), sensor_(nullptr) > > + : sensor_(nullptr), csi2_(nullptr), output_(nullptr) > > { > > } > > > > @@ -126,6 +126,8 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > > if (ret) > > return ret; > > > > + output_->bufferReady.connect(this, &CIO2Device::cio2BufferReady); > > There's a bit of signal ping-pong here... > Could this be avoided by providing a registerBufferReady() method that > the pipielinehandler can call and provide a slot to connect to the > video device signal ? I can't actually tell how bad this is tbh How about - Signal<FrameBuffer *> bufferReady; + Signal<FrameBuffer *> &bufferReady() { return output_->bufferReady; } ? That would avoid proxying the signal while still not exposing the V4L2VideoDevice. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > + > > return 0; > > } > > > > @@ -226,6 +228,12 @@ int CIO2Device::allocateBuffers() > > return ret; > > } > > > > +int CIO2Device::exportBuffers(unsigned int count, > > + std::vector<std::unique_ptr<FrameBuffer>> *buffers) > > +{ > > + return output_->exportBuffers(count, buffers); > > +} > > + > > void CIO2Device::freeBuffers() > > { > > /* The default std::queue constructor is explicit with gcc 5 and 6. */ > > @@ -266,4 +274,14 @@ int CIO2Device::stop() > > return output_->streamOff(); > > } > > > > +int CIO2Device::queueBuffer(FrameBuffer *buffer) > > +{ > > + return output_->queueBuffer(buffer); > > +} > > + > > +void CIO2Device::cio2BufferReady(FrameBuffer *buffer) > > +{ > > + bufferReady.emit(buffer); > > +} > > + > > } /* namespace libcamera */ > > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h > > index 6276573f2b585b25..cc898f13fd73f865 100644 > > --- a/src/libcamera/pipeline/ipu3/cio2.h > > +++ b/src/libcamera/pipeline/ipu3/cio2.h > > @@ -11,6 +11,8 @@ > > #include <queue> > > #include <vector> > > > > +#include <libcamera/signal.h> > > + > > namespace libcamera { > > > > class CameraSensor; > > @@ -36,6 +38,8 @@ public: > > StreamConfiguration generateConfiguration(const Size &desiredSize) const; > > > > int allocateBuffers(); > > + int exportBuffers(unsigned int count, > > + std::vector<std::unique_ptr<FrameBuffer>> *buffers); > > void freeBuffers(); > > > > FrameBuffer *getBuffer(); > > @@ -44,11 +48,18 @@ public: > > int start(); > > int stop(); > > > > - V4L2VideoDevice *output_; > > - V4L2Subdevice *csi2_; > > - CameraSensor *sensor_; > > + CameraSensor *sensor() { return sensor_; } > > + > > + int queueBuffer(FrameBuffer *buffer); > > + Signal<FrameBuffer *> bufferReady; > > > > private: > > + void cio2BufferReady(FrameBuffer *buffer); > > + > > + CameraSensor *sensor_; > > + V4L2Subdevice *csi2_; > > + V4L2VideoDevice *output_; > > + > > std::vector<std::unique_ptr<FrameBuffer>> buffers_; > > std::queue<FrameBuffer *> availableBuffers_; > > }; > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index c0e727e592f46883..2d1ec707ea4b08fe 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -431,7 +431,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, > > > > stream = &data->rawStream_; > > > > - cfg.size = data->cio2_.sensor_->resolution(); > > + cfg.size = data->cio2_.sensor()->resolution(); > > > > cfg = data->cio2_.generateConfiguration(cfg.size); > > break; > > @@ -460,7 +460,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, > > * available sensor resolution and to the IPU3 > > * alignment constraints. > > */ > > - const Size &res = data->cio2_.sensor_->resolution(); > > + const Size &res = data->cio2_.sensor()->resolution(); > > This could actually be Cio2Device::resolution(). > Up to you. > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > unsigned int width = std::min(1280U, res.width); > > unsigned int height = std::min(720U, res.height); > > cfg.size = { width & ~7, height & ~3 }; > > @@ -640,14 +640,11 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream, > > IPU3CameraData *data = cameraData(camera); > > IPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream); > > unsigned int count = stream->configuration().bufferCount; > > - V4L2VideoDevice *video; > > > > if (ipu3stream->raw_) > > - video = data->cio2_.output_; > > - else > > - video = ipu3stream->device_->dev; > > + return data->cio2_.exportBuffers(count, buffers); > > > > - return video->exportBuffers(count, buffers); > > + return ipu3stream->device_->dev->exportBuffers(count, buffers); > > } > > > > /** > > @@ -757,7 +754,7 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request) > > return -EINVAL; > > > > buffer->setRequest(request); > > - data->cio2_.output_->queueBuffer(buffer); > > + data->cio2_.queueBuffer(buffer); > > > > for (auto it : request->buffers()) { > > IPU3Stream *stream = static_cast<IPU3Stream *>(it.first); > > @@ -870,7 +867,7 @@ int PipelineHandlerIPU3::registerCameras() > > continue; > > > > /* Initialize the camera properties. */ > > - data->properties_ = cio2->sensor_->properties(); > > + data->properties_ = cio2->sensor()->properties(); > > > > /** > > * \todo Dynamically assign ImgU and output devices to each > > @@ -894,7 +891,7 @@ int PipelineHandlerIPU3::registerCameras() > > * associated ImgU input where they get processed and > > * returned through the ImgU main and secondary outputs. > > */ > > - data->cio2_.output_->bufferReady.connect(data.get(), > > + data->cio2_.bufferReady.connect(data.get(), > > &IPU3CameraData::cio2BufferReady); > > data->imgu_->input_->bufferReady.connect(data.get(), > > &IPU3CameraData::imguInputBufferReady); > > @@ -904,7 +901,7 @@ int PipelineHandlerIPU3::registerCameras() > > &IPU3CameraData::imguOutputBufferReady); > > > > /* Create and register the Camera instance. */ > > - std::string cameraName = cio2->sensor_->entity()->name(); > > + std::string cameraName = cio2->sensor()->entity()->name(); > > std::shared_ptr<Camera> camera = Camera::create(this, > > cameraName, > > streams);
Hi Laurent and Jacopo, Thanks for your feedback. On 2020-06-25 04:30:53 +0300, Laurent Pinchart wrote: > Hi Jacopo and Niklas, > > On Wed, Jun 24, 2020 at 12:31:10PM +0200, Jacopo Mondi wrote: > > On Tue, Jun 23, 2020 at 04:09:28AM +0200, Niklas Söderlund wrote: > > > In order to make the CIO2 easier to extend with new features make the > > > V4L2 devices (sensor, CIO2 and video device) private members. This > > > requires a few helper functions to be added to allow for the IPU3 driver > > > to still be able to interact with all parts of the CIO2. These helper > > > functions will later be extended to add new features to the IPU3 > > > pipeline. > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > Much better than accessing parts of the CIO2 device from the other > > components. > > > > > --- > > > * Changes since v2 > > > - Style changes. > > > > > > * Changes since v1 > > > - Drop sensor access helpers and replace with a sensor() call to get > > > hold of the CameraSensor pointer directly. > > > --- > > > src/libcamera/pipeline/ipu3/cio2.cpp | 20 +++++++++++++++++++- > > > src/libcamera/pipeline/ipu3/cio2.h | 17 ++++++++++++++--- > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 19 ++++++++----------- > > > 3 files changed, 41 insertions(+), 15 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > > > index d6bab896706dd60e..3d7348782b0fa6cb 100644 > > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > > > @@ -38,7 +38,7 @@ static const std::map<uint32_t, MbusInfo> mbusCodesToInfo = { > > > } /* namespace */ > > > > > > CIO2Device::CIO2Device() > > > - : output_(nullptr), csi2_(nullptr), sensor_(nullptr) > > > + : sensor_(nullptr), csi2_(nullptr), output_(nullptr) > > > { > > > } > > > > > > @@ -126,6 +126,8 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > > > if (ret) > > > return ret; > > > > > > + output_->bufferReady.connect(this, &CIO2Device::cio2BufferReady); > > > > There's a bit of signal ping-pong here... > > Could this be avoided by providing a registerBufferReady() method that > > the pipielinehandler can call and provide a slot to connect to the > > video device signal ? I can't actually tell how bad this is tbh > > How about > > - Signal<FrameBuffer *> bufferReady; > + Signal<FrameBuffer *> &bufferReady() { return output_->bufferReady; } > > ? That would avoid proxying the signal while still not exposing the > V4L2VideoDevice. That would work in this pathc but 10/10 I inject code into CIO2Device::cio2BufferReady() which require the signal to be proxied. So taking this in here I fear would only be busy work :-) > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > + > > > return 0; > > > } > > > > > > @@ -226,6 +228,12 @@ int CIO2Device::allocateBuffers() > > > return ret; > > > } > > > > > > +int CIO2Device::exportBuffers(unsigned int count, > > > + std::vector<std::unique_ptr<FrameBuffer>> *buffers) > > > +{ > > > + return output_->exportBuffers(count, buffers); > > > +} > > > + > > > void CIO2Device::freeBuffers() > > > { > > > /* The default std::queue constructor is explicit with gcc 5 and 6. */ > > > @@ -266,4 +274,14 @@ int CIO2Device::stop() > > > return output_->streamOff(); > > > } > > > > > > +int CIO2Device::queueBuffer(FrameBuffer *buffer) > > > +{ > > > + return output_->queueBuffer(buffer); > > > +} > > > + > > > +void CIO2Device::cio2BufferReady(FrameBuffer *buffer) > > > +{ > > > + bufferReady.emit(buffer); > > > +} > > > + > > > } /* namespace libcamera */ > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h > > > index 6276573f2b585b25..cc898f13fd73f865 100644 > > > --- a/src/libcamera/pipeline/ipu3/cio2.h > > > +++ b/src/libcamera/pipeline/ipu3/cio2.h > > > @@ -11,6 +11,8 @@ > > > #include <queue> > > > #include <vector> > > > > > > +#include <libcamera/signal.h> > > > + > > > namespace libcamera { > > > > > > class CameraSensor; > > > @@ -36,6 +38,8 @@ public: > > > StreamConfiguration generateConfiguration(const Size &desiredSize) const; > > > > > > int allocateBuffers(); > > > + int exportBuffers(unsigned int count, > > > + std::vector<std::unique_ptr<FrameBuffer>> *buffers); > > > void freeBuffers(); > > > > > > FrameBuffer *getBuffer(); > > > @@ -44,11 +48,18 @@ public: > > > int start(); > > > int stop(); > > > > > > - V4L2VideoDevice *output_; > > > - V4L2Subdevice *csi2_; > > > - CameraSensor *sensor_; > > > + CameraSensor *sensor() { return sensor_; } > > > + > > > + int queueBuffer(FrameBuffer *buffer); > > > + Signal<FrameBuffer *> bufferReady; > > > > > > private: > > > + void cio2BufferReady(FrameBuffer *buffer); > > > + > > > + CameraSensor *sensor_; > > > + V4L2Subdevice *csi2_; > > > + V4L2VideoDevice *output_; > > > + > > > std::vector<std::unique_ptr<FrameBuffer>> buffers_; > > > std::queue<FrameBuffer *> availableBuffers_; > > > }; > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > index c0e727e592f46883..2d1ec707ea4b08fe 100644 > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > @@ -431,7 +431,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, > > > > > > stream = &data->rawStream_; > > > > > > - cfg.size = data->cio2_.sensor_->resolution(); > > > + cfg.size = data->cio2_.sensor()->resolution(); > > > > > > cfg = data->cio2_.generateConfiguration(cfg.size); > > > break; > > > @@ -460,7 +460,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, > > > * available sensor resolution and to the IPU3 > > > * alignment constraints. > > > */ > > > - const Size &res = data->cio2_.sensor_->resolution(); > > > + const Size &res = data->cio2_.sensor()->resolution(); > > > > This could actually be Cio2Device::resolution(). > > Up to you. > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > unsigned int width = std::min(1280U, res.width); > > > unsigned int height = std::min(720U, res.height); > > > cfg.size = { width & ~7, height & ~3 }; > > > @@ -640,14 +640,11 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream, > > > IPU3CameraData *data = cameraData(camera); > > > IPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream); > > > unsigned int count = stream->configuration().bufferCount; > > > - V4L2VideoDevice *video; > > > > > > if (ipu3stream->raw_) > > > - video = data->cio2_.output_; > > > - else > > > - video = ipu3stream->device_->dev; > > > + return data->cio2_.exportBuffers(count, buffers); > > > > > > - return video->exportBuffers(count, buffers); > > > + return ipu3stream->device_->dev->exportBuffers(count, buffers); > > > } > > > > > > /** > > > @@ -757,7 +754,7 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request) > > > return -EINVAL; > > > > > > buffer->setRequest(request); > > > - data->cio2_.output_->queueBuffer(buffer); > > > + data->cio2_.queueBuffer(buffer); > > > > > > for (auto it : request->buffers()) { > > > IPU3Stream *stream = static_cast<IPU3Stream *>(it.first); > > > @@ -870,7 +867,7 @@ int PipelineHandlerIPU3::registerCameras() > > > continue; > > > > > > /* Initialize the camera properties. */ > > > - data->properties_ = cio2->sensor_->properties(); > > > + data->properties_ = cio2->sensor()->properties(); > > > > > > /** > > > * \todo Dynamically assign ImgU and output devices to each > > > @@ -894,7 +891,7 @@ int PipelineHandlerIPU3::registerCameras() > > > * associated ImgU input where they get processed and > > > * returned through the ImgU main and secondary outputs. > > > */ > > > - data->cio2_.output_->bufferReady.connect(data.get(), > > > + data->cio2_.bufferReady.connect(data.get(), > > > &IPU3CameraData::cio2BufferReady); > > > data->imgu_->input_->bufferReady.connect(data.get(), > > > &IPU3CameraData::imguInputBufferReady); > > > @@ -904,7 +901,7 @@ int PipelineHandlerIPU3::registerCameras() > > > &IPU3CameraData::imguOutputBufferReady); > > > > > > /* Create and register the Camera instance. */ > > > - std::string cameraName = cio2->sensor_->entity()->name(); > > > + std::string cameraName = cio2->sensor()->entity()->name(); > > > std::shared_ptr<Camera> camera = Camera::create(this, > > > cameraName, > > > streams); > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp index d6bab896706dd60e..3d7348782b0fa6cb 100644 --- a/src/libcamera/pipeline/ipu3/cio2.cpp +++ b/src/libcamera/pipeline/ipu3/cio2.cpp @@ -38,7 +38,7 @@ static const std::map<uint32_t, MbusInfo> mbusCodesToInfo = { } /* namespace */ CIO2Device::CIO2Device() - : output_(nullptr), csi2_(nullptr), sensor_(nullptr) + : sensor_(nullptr), csi2_(nullptr), output_(nullptr) { } @@ -126,6 +126,8 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) if (ret) return ret; + output_->bufferReady.connect(this, &CIO2Device::cio2BufferReady); + return 0; } @@ -226,6 +228,12 @@ int CIO2Device::allocateBuffers() return ret; } +int CIO2Device::exportBuffers(unsigned int count, + std::vector<std::unique_ptr<FrameBuffer>> *buffers) +{ + return output_->exportBuffers(count, buffers); +} + void CIO2Device::freeBuffers() { /* The default std::queue constructor is explicit with gcc 5 and 6. */ @@ -266,4 +274,14 @@ int CIO2Device::stop() return output_->streamOff(); } +int CIO2Device::queueBuffer(FrameBuffer *buffer) +{ + return output_->queueBuffer(buffer); +} + +void CIO2Device::cio2BufferReady(FrameBuffer *buffer) +{ + bufferReady.emit(buffer); +} + } /* namespace libcamera */ diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h index 6276573f2b585b25..cc898f13fd73f865 100644 --- a/src/libcamera/pipeline/ipu3/cio2.h +++ b/src/libcamera/pipeline/ipu3/cio2.h @@ -11,6 +11,8 @@ #include <queue> #include <vector> +#include <libcamera/signal.h> + namespace libcamera { class CameraSensor; @@ -36,6 +38,8 @@ public: StreamConfiguration generateConfiguration(const Size &desiredSize) const; int allocateBuffers(); + int exportBuffers(unsigned int count, + std::vector<std::unique_ptr<FrameBuffer>> *buffers); void freeBuffers(); FrameBuffer *getBuffer(); @@ -44,11 +48,18 @@ public: int start(); int stop(); - V4L2VideoDevice *output_; - V4L2Subdevice *csi2_; - CameraSensor *sensor_; + CameraSensor *sensor() { return sensor_; } + + int queueBuffer(FrameBuffer *buffer); + Signal<FrameBuffer *> bufferReady; private: + void cio2BufferReady(FrameBuffer *buffer); + + CameraSensor *sensor_; + V4L2Subdevice *csi2_; + V4L2VideoDevice *output_; + std::vector<std::unique_ptr<FrameBuffer>> buffers_; std::queue<FrameBuffer *> availableBuffers_; }; diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index c0e727e592f46883..2d1ec707ea4b08fe 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -431,7 +431,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, stream = &data->rawStream_; - cfg.size = data->cio2_.sensor_->resolution(); + cfg.size = data->cio2_.sensor()->resolution(); cfg = data->cio2_.generateConfiguration(cfg.size); break; @@ -460,7 +460,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, * available sensor resolution and to the IPU3 * alignment constraints. */ - const Size &res = data->cio2_.sensor_->resolution(); + const Size &res = data->cio2_.sensor()->resolution(); unsigned int width = std::min(1280U, res.width); unsigned int height = std::min(720U, res.height); cfg.size = { width & ~7, height & ~3 }; @@ -640,14 +640,11 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream, IPU3CameraData *data = cameraData(camera); IPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream); unsigned int count = stream->configuration().bufferCount; - V4L2VideoDevice *video; if (ipu3stream->raw_) - video = data->cio2_.output_; - else - video = ipu3stream->device_->dev; + return data->cio2_.exportBuffers(count, buffers); - return video->exportBuffers(count, buffers); + return ipu3stream->device_->dev->exportBuffers(count, buffers); } /** @@ -757,7 +754,7 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request) return -EINVAL; buffer->setRequest(request); - data->cio2_.output_->queueBuffer(buffer); + data->cio2_.queueBuffer(buffer); for (auto it : request->buffers()) { IPU3Stream *stream = static_cast<IPU3Stream *>(it.first); @@ -870,7 +867,7 @@ int PipelineHandlerIPU3::registerCameras() continue; /* Initialize the camera properties. */ - data->properties_ = cio2->sensor_->properties(); + data->properties_ = cio2->sensor()->properties(); /** * \todo Dynamically assign ImgU and output devices to each @@ -894,7 +891,7 @@ int PipelineHandlerIPU3::registerCameras() * associated ImgU input where they get processed and * returned through the ImgU main and secondary outputs. */ - data->cio2_.output_->bufferReady.connect(data.get(), + data->cio2_.bufferReady.connect(data.get(), &IPU3CameraData::cio2BufferReady); data->imgu_->input_->bufferReady.connect(data.get(), &IPU3CameraData::imguInputBufferReady); @@ -904,7 +901,7 @@ int PipelineHandlerIPU3::registerCameras() &IPU3CameraData::imguOutputBufferReady); /* Create and register the Camera instance. */ - std::string cameraName = cio2->sensor_->entity()->name(); + std::string cameraName = cio2->sensor()->entity()->name(); std::shared_ptr<Camera> camera = Camera::create(this, cameraName, streams);
In order to make the CIO2 easier to extend with new features make the V4L2 devices (sensor, CIO2 and video device) private members. This requires a few helper functions to be added to allow for the IPU3 driver to still be able to interact with all parts of the CIO2. These helper functions will later be extended to add new features to the IPU3 pipeline. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- * Changes since v2 - Style changes. * Changes since v1 - Drop sensor access helpers and replace with a sensor() call to get hold of the CameraSensor pointer directly. --- src/libcamera/pipeline/ipu3/cio2.cpp | 20 +++++++++++++++++++- src/libcamera/pipeline/ipu3/cio2.h | 17 ++++++++++++++--- src/libcamera/pipeline/ipu3/ipu3.cpp | 19 ++++++++----------- 3 files changed, 41 insertions(+), 15 deletions(-)