| Message ID | 20191126233620.1695316-30-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. On Wed, Nov 27, 2019 at 12:36:19AM +0100, Niklas Söderlund wrote: > With FrameBuffer interface in place there is no need for the Camera to s/FrameBuffer/the FrameBuffer/ > call into the specific pipelines allocation and freeing of buffers as it > no longer needs to be synced with buffer allocation by the application. s/synced/synchronized/ > Remove the function prototypes in the pipeline handler base class and > fold the functionality in the pipelines start() and stop() functions > where needed. A follow up patch will remove the now no-op > Camera::allocateBuffers() and Camera::freeBuffers(). > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/camera.cpp | 8 +------ > src/libcamera/include/pipeline_handler.h | 5 ---- > src/libcamera/pipeline/ipu3/ipu3.cpp | 21 +++++++++-------- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 21 ++++++++++------- > src/libcamera/pipeline/uvcvideo.cpp | 17 -------------- > src/libcamera/pipeline/vimc.cpp | 17 -------------- > src/libcamera/pipeline_handler.cpp | 29 ------------------------ > 7 files changed, 26 insertions(+), 92 deletions(-) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 05fdcaab8f918afa..12878a1c2e6076d3 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -709,12 +709,6 @@ int Camera::allocateBuffers() > return -EINVAL; > } > > - int ret = pipe_->allocateBuffers(this, activeStreams_); > - if (ret) { > - LOG(Camera, Error) << "Failed to allocate buffers"; > - return ret; > - } > - > state_ = CameraPrepared; > > return 0; > @@ -735,7 +729,7 @@ int Camera::freeBuffers() > > state_ = CameraConfigured; > > - return pipe_->freeBuffers(this, activeStreams_); > + return 0; > } > > /** > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h > index e4588f10bac0228c..9e5dca894aff45ae 100644 > --- a/src/libcamera/include/pipeline_handler.h > +++ b/src/libcamera/include/pipeline_handler.h > @@ -69,11 +69,6 @@ public: > const StreamRoles &roles) = 0; > virtual int configure(Camera *camera, CameraConfiguration *config) = 0; > > - virtual int allocateBuffers(Camera *camera, > - const std::set<Stream *> &streams) = 0; > - virtual int freeBuffers(Camera *camera, > - const std::set<Stream *> &streams) = 0; > - > virtual int start(Camera *camera) = 0; > virtual void stop(Camera *camera) = 0; > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 8d9420aea3eb0b21..6d6ecb511777ec49 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -215,10 +215,8 @@ public: > const StreamRoles &roles) override; > int configure(Camera *camera, CameraConfiguration *config) override; > > - int allocateBuffers(Camera *camera, > - const std::set<Stream *> &streams) override; > - int freeBuffers(Camera *camera, > - const std::set<Stream *> &streams) override; > + int allocateBuffers(Camera *camera); > + int freeBuffers(Camera *camera); Shouldn't those methods be made private ? > > int start(Camera *camera) override; > void stop(Camera *camera) override; > @@ -629,8 +627,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > * In order to be able to start the 'viewfinder' and 'stat' nodes, we need > * memory to be reserved. > */ > -int PipelineHandlerIPU3::allocateBuffers(Camera *camera, > - const std::set<Stream *> &streams) > +int PipelineHandlerIPU3::allocateBuffers(Camera *camera) > { > IPU3CameraData *data = cameraData(camera); > unsigned int bufferCount; > @@ -670,13 +667,12 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, > > return 0; > error: > - freeBuffers(camera, streams); > + freeBuffers(camera); > > return ret; > } > > -int PipelineHandlerIPU3::freeBuffers(Camera *camera, > - const std::set<Stream *> &streams) > +int PipelineHandlerIPU3::freeBuffers(Camera *camera) > { > IPU3CameraData *data = cameraData(camera); > > @@ -693,6 +689,10 @@ int PipelineHandlerIPU3::start(Camera *camera) > ImgUDevice *imgu = data->imgu_; > int ret; > > + ret = allocateBuffers(camera); > + if (ret) > + return ret; > + > /* > * Start the ImgU video devices, buffers will be queued to the > * ImgU output and viewfinder when requests will be queued. > @@ -711,6 +711,7 @@ int PipelineHandlerIPU3::start(Camera *camera) > return 0; > > error: > + freeBuffers(camera); > LOG(IPU3, Error) << "Failed to start camera " << camera->name(); > > return ret; > @@ -726,6 +727,8 @@ void PipelineHandlerIPU3::stop(Camera *camera) > if (ret) > LOG(IPU3, Warning) << "Failed to stop camera " > << camera->name(); > + > + freeBuffers(camera); > } > > int PipelineHandlerIPU3::queueRequestHardware(Camera *camera, Request *request) > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 96e863f0208fa748..a56c7022f57ce771 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -174,10 +174,8 @@ public: > const StreamRoles &roles) override; > int configure(Camera *camera, CameraConfiguration *config) override; > > - int allocateBuffers(Camera *camera, > - const std::set<Stream *> &streams) override; > - int freeBuffers(Camera *camera, > - const std::set<Stream *> &streams) override; > + int allocateBuffers(Camera *camera); > + int freeBuffers(Camera *camera); Same here, shouldn't these methods be made private ? With this fixed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > int start(Camera *camera) override; > void stop(Camera *camera) override; > @@ -651,8 +649,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > return 0; > } > > -int PipelineHandlerRkISP1::allocateBuffers(Camera *camera, > - const std::set<Stream *> &streams) > +int PipelineHandlerRkISP1::allocateBuffers(Camera *camera) > { > RkISP1CameraData *data = cameraData(camera); > std::vector<FrameBuffer *> paramBuffers, statBuffers; > @@ -695,8 +692,7 @@ err: > return ret; > } > > -int PipelineHandlerRkISP1::freeBuffers(Camera *camera, > - const std::set<Stream *> &streams) > +int PipelineHandlerRkISP1::freeBuffers(Camera *camera) > { > RkISP1CameraData *data = cameraData(camera); > > @@ -731,10 +727,15 @@ int PipelineHandlerRkISP1::start(Camera *camera) > RkISP1CameraData *data = cameraData(camera); > int ret; > > + ret = allocateBuffers(camera); > + if (ret) > + return ret; > + > data->frame_ = 0; > > ret = param_->streamOn(); > if (ret) { > + freeBuffers(camera); > LOG(RkISP1, Error) > << "Failed to start parameters " << camera->name(); > return ret; > @@ -743,6 +744,7 @@ int PipelineHandlerRkISP1::start(Camera *camera) > ret = stat_->streamOn(); > if (ret) { > param_->streamOff(); > + freeBuffers(camera); > LOG(RkISP1, Error) > << "Failed to start statistics " << camera->name(); > return ret; > @@ -752,6 +754,7 @@ int PipelineHandlerRkISP1::start(Camera *camera) > if (ret) { > param_->streamOff(); > stat_->streamOff(); > + freeBuffers(camera); > > LOG(RkISP1, Error) > << "Failed to start camera " << camera->name(); > @@ -796,6 +799,8 @@ void PipelineHandlerRkISP1::stop(Camera *camera) > > data->timeline_.reset(); > > + freeBuffers(camera); > + > activeCamera_ = nullptr; > } > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > index c275b9f0f75429bd..2be7864ef59e932b 100644 > --- a/src/libcamera/pipeline/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo.cpp > @@ -65,11 +65,6 @@ public: > const StreamRoles &roles) override; > int configure(Camera *camera, CameraConfiguration *config) override; > > - int allocateBuffers(Camera *camera, > - const std::set<Stream *> &streams) override; > - int freeBuffers(Camera *camera, > - const std::set<Stream *> &streams) override; > - > int start(Camera *camera) override; > void stop(Camera *camera) override; > > @@ -193,18 +188,6 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config) > return 0; > } > > -int PipelineHandlerUVC::allocateBuffers(Camera *camera, > - const std::set<Stream *> &streams) > -{ > - return 0; > -} > - > -int PipelineHandlerUVC::freeBuffers(Camera *camera, > - const std::set<Stream *> &streams) > -{ > - return 0; > -} > - > int PipelineHandlerUVC::start(Camera *camera) > { > UVCCameraData *data = cameraData(camera); > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > index 64793788c752c791..cacf8bfe3ff9cc45 100644 > --- a/src/libcamera/pipeline/vimc.cpp > +++ b/src/libcamera/pipeline/vimc.cpp > @@ -84,11 +84,6 @@ public: > const StreamRoles &roles) override; > int configure(Camera *camera, CameraConfiguration *config) override; > > - int allocateBuffers(Camera *camera, > - const std::set<Stream *> &streams) override; > - int freeBuffers(Camera *camera, > - const std::set<Stream *> &streams) override; > - > int start(Camera *camera) override; > void stop(Camera *camera) override; > > @@ -261,18 +256,6 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config) > return 0; > } > > -int PipelineHandlerVimc::allocateBuffers(Camera *camera, > - const std::set<Stream *> &streams) > -{ > - return 0; > -} > - > -int PipelineHandlerVimc::freeBuffers(Camera *camera, > - const std::set<Stream *> &streams) > -{ > - return 0; > -} > - > int PipelineHandlerVimc::start(Camera *camera) > { > VimcCameraData *data = cameraData(camera); > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index b107e23258dee692..67758f3f1ebc39e5 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -287,35 +287,6 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera) > * \return 0 on success or a negative error code otherwise > */ > > -/** > - * \fn PipelineHandler::allocateBuffers() > - * \brief Allocate buffers for a stream > - * \param[in] camera The camera the \a stream belongs to > - * \param[in] streams The set of streams to allocate buffers for > - * > - * This method allocates buffers internally in the pipeline handler for each > - * stream in the \a streams buffer set, and associates them with the stream's > - * buffer pool. > - * > - * The intended caller of this method is the Camera class. > - * > - * \return 0 on success or a negative error code otherwise > - */ > - > -/** > - * \fn PipelineHandler::freeBuffers() > - * \brief Free all buffers associated with a stream > - * \param[in] camera The camera the \a stream belongs to > - * \param[in] streams The set of streams to free buffers from > - * > - * After a capture session has been stopped all buffers associated with each > - * stream shall be freed. > - * > - * The intended caller of this method is the Camera class. > - * > - * \return 0 on success or a negative error code otherwise > - */ > - > /** > * \fn PipelineHandler::start() > * \brief Start capturing from a group of streams
Hi Niklas, One additional comment. On Sun, Dec 15, 2019 at 11:28:42AM +0200, Laurent Pinchart wrote: > On Wed, Nov 27, 2019 at 12:36:19AM +0100, Niklas Söderlund wrote: > > With FrameBuffer interface in place there is no need for the Camera to > > s/FrameBuffer/the FrameBuffer/ > > > call into the specific pipelines allocation and freeing of buffers as it > > no longer needs to be synced with buffer allocation by the application. > > s/synced/synchronized/ > > > Remove the function prototypes in the pipeline handler base class and > > fold the functionality in the pipelines start() and stop() functions > > where needed. A follow up patch will remove the now no-op > > Camera::allocateBuffers() and Camera::freeBuffers(). > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/libcamera/camera.cpp | 8 +------ > > src/libcamera/include/pipeline_handler.h | 5 ---- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 21 +++++++++-------- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 21 ++++++++++------- > > src/libcamera/pipeline/uvcvideo.cpp | 17 -------------- > > src/libcamera/pipeline/vimc.cpp | 17 -------------- > > src/libcamera/pipeline_handler.cpp | 29 ------------------------ > > 7 files changed, 26 insertions(+), 92 deletions(-) > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index 05fdcaab8f918afa..12878a1c2e6076d3 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -709,12 +709,6 @@ int Camera::allocateBuffers() > > return -EINVAL; > > } > > > > - int ret = pipe_->allocateBuffers(this, activeStreams_); > > - if (ret) { > > - LOG(Camera, Error) << "Failed to allocate buffers"; > > - return ret; > > - } > > - > > state_ = CameraPrepared; > > > > return 0; > > @@ -735,7 +729,7 @@ int Camera::freeBuffers() > > > > state_ = CameraConfigured; > > > > - return pipe_->freeBuffers(this, activeStreams_); > > + return 0; > > } > > > > /** > > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h > > index e4588f10bac0228c..9e5dca894aff45ae 100644 > > --- a/src/libcamera/include/pipeline_handler.h > > +++ b/src/libcamera/include/pipeline_handler.h > > @@ -69,11 +69,6 @@ public: > > const StreamRoles &roles) = 0; > > virtual int configure(Camera *camera, CameraConfiguration *config) = 0; > > > > - virtual int allocateBuffers(Camera *camera, > > - const std::set<Stream *> &streams) = 0; > > - virtual int freeBuffers(Camera *camera, > > - const std::set<Stream *> &streams) = 0; > > - > > virtual int start(Camera *camera) = 0; > > virtual void stop(Camera *camera) = 0; > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 8d9420aea3eb0b21..6d6ecb511777ec49 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -215,10 +215,8 @@ public: > > const StreamRoles &roles) override; > > int configure(Camera *camera, CameraConfiguration *config) override; > > > > - int allocateBuffers(Camera *camera, > > - const std::set<Stream *> &streams) override; > > - int freeBuffers(Camera *camera, > > - const std::set<Stream *> &streams) override; > > + int allocateBuffers(Camera *camera); > > + int freeBuffers(Camera *camera); > > Shouldn't those methods be made private ? > > > > > int start(Camera *camera) override; > > void stop(Camera *camera) override; > > @@ -629,8 +627,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > > * In order to be able to start the 'viewfinder' and 'stat' nodes, we need > > * memory to be reserved. > > */ > > -int PipelineHandlerIPU3::allocateBuffers(Camera *camera, > > - const std::set<Stream *> &streams) > > +int PipelineHandlerIPU3::allocateBuffers(Camera *camera) > > { > > IPU3CameraData *data = cameraData(camera); > > unsigned int bufferCount; > > @@ -670,13 +667,12 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, > > > > return 0; > > error: > > - freeBuffers(camera, streams); > > + freeBuffers(camera); > > > > return ret; > > } > > > > -int PipelineHandlerIPU3::freeBuffers(Camera *camera, > > - const std::set<Stream *> &streams) > > +int PipelineHandlerIPU3::freeBuffers(Camera *camera) > > { > > IPU3CameraData *data = cameraData(camera); > > > > @@ -693,6 +689,10 @@ int PipelineHandlerIPU3::start(Camera *camera) > > ImgUDevice *imgu = data->imgu_; > > int ret; > > I think we need a comment here to explain why buffers need to be allocated here, and what buffers are being allocated. This code will be used as a reference implementation, so it should have detailed comments. Same for the RkISP1 pipeline handler. > > + ret = allocateBuffers(camera); > > + if (ret) > > + return ret; > > + > > /* > > * Start the ImgU video devices, buffers will be queued to the > > * ImgU output and viewfinder when requests will be queued. > > @@ -711,6 +711,7 @@ int PipelineHandlerIPU3::start(Camera *camera) > > return 0; > > > > error: > > + freeBuffers(camera); > > LOG(IPU3, Error) << "Failed to start camera " << camera->name(); > > > > return ret; > > @@ -726,6 +727,8 @@ void PipelineHandlerIPU3::stop(Camera *camera) > > if (ret) > > LOG(IPU3, Warning) << "Failed to stop camera " > > << camera->name(); > > + > > + freeBuffers(camera); > > } > > > > int PipelineHandlerIPU3::queueRequestHardware(Camera *camera, Request *request) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 96e863f0208fa748..a56c7022f57ce771 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -174,10 +174,8 @@ public: > > const StreamRoles &roles) override; > > int configure(Camera *camera, CameraConfiguration *config) override; > > > > - int allocateBuffers(Camera *camera, > > - const std::set<Stream *> &streams) override; > > - int freeBuffers(Camera *camera, > > - const std::set<Stream *> &streams) override; > > + int allocateBuffers(Camera *camera); > > + int freeBuffers(Camera *camera); > > Same here, shouldn't these methods be made private ? > > With this fixed, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > int start(Camera *camera) override; > > void stop(Camera *camera) override; > > @@ -651,8 +649,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > > return 0; > > } > > > > -int PipelineHandlerRkISP1::allocateBuffers(Camera *camera, > > - const std::set<Stream *> &streams) > > +int PipelineHandlerRkISP1::allocateBuffers(Camera *camera) > > { > > RkISP1CameraData *data = cameraData(camera); > > std::vector<FrameBuffer *> paramBuffers, statBuffers; > > @@ -695,8 +692,7 @@ err: > > return ret; > > } > > > > -int PipelineHandlerRkISP1::freeBuffers(Camera *camera, > > - const std::set<Stream *> &streams) > > +int PipelineHandlerRkISP1::freeBuffers(Camera *camera) > > { > > RkISP1CameraData *data = cameraData(camera); > > > > @@ -731,10 +727,15 @@ int PipelineHandlerRkISP1::start(Camera *camera) > > RkISP1CameraData *data = cameraData(camera); > > int ret; > > > > + ret = allocateBuffers(camera); > > + if (ret) > > + return ret; > > + > > data->frame_ = 0; > > > > ret = param_->streamOn(); > > if (ret) { > > + freeBuffers(camera); > > LOG(RkISP1, Error) > > << "Failed to start parameters " << camera->name(); > > return ret; > > @@ -743,6 +744,7 @@ int PipelineHandlerRkISP1::start(Camera *camera) > > ret = stat_->streamOn(); > > if (ret) { > > param_->streamOff(); > > + freeBuffers(camera); > > LOG(RkISP1, Error) > > << "Failed to start statistics " << camera->name(); > > return ret; > > @@ -752,6 +754,7 @@ int PipelineHandlerRkISP1::start(Camera *camera) > > if (ret) { > > param_->streamOff(); > > stat_->streamOff(); > > + freeBuffers(camera); > > > > LOG(RkISP1, Error) > > << "Failed to start camera " << camera->name(); > > @@ -796,6 +799,8 @@ void PipelineHandlerRkISP1::stop(Camera *camera) > > > > data->timeline_.reset(); > > > > + freeBuffers(camera); > > + > > activeCamera_ = nullptr; > > } > > > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > > index c275b9f0f75429bd..2be7864ef59e932b 100644 > > --- a/src/libcamera/pipeline/uvcvideo.cpp > > +++ b/src/libcamera/pipeline/uvcvideo.cpp > > @@ -65,11 +65,6 @@ public: > > const StreamRoles &roles) override; > > int configure(Camera *camera, CameraConfiguration *config) override; > > > > - int allocateBuffers(Camera *camera, > > - const std::set<Stream *> &streams) override; > > - int freeBuffers(Camera *camera, > > - const std::set<Stream *> &streams) override; > > - > > int start(Camera *camera) override; > > void stop(Camera *camera) override; > > > > @@ -193,18 +188,6 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config) > > return 0; > > } > > > > -int PipelineHandlerUVC::allocateBuffers(Camera *camera, > > - const std::set<Stream *> &streams) > > -{ > > - return 0; > > -} > > - > > -int PipelineHandlerUVC::freeBuffers(Camera *camera, > > - const std::set<Stream *> &streams) > > -{ > > - return 0; > > -} > > - > > int PipelineHandlerUVC::start(Camera *camera) > > { > > UVCCameraData *data = cameraData(camera); > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > > index 64793788c752c791..cacf8bfe3ff9cc45 100644 > > --- a/src/libcamera/pipeline/vimc.cpp > > +++ b/src/libcamera/pipeline/vimc.cpp > > @@ -84,11 +84,6 @@ public: > > const StreamRoles &roles) override; > > int configure(Camera *camera, CameraConfiguration *config) override; > > > > - int allocateBuffers(Camera *camera, > > - const std::set<Stream *> &streams) override; > > - int freeBuffers(Camera *camera, > > - const std::set<Stream *> &streams) override; > > - > > int start(Camera *camera) override; > > void stop(Camera *camera) override; > > > > @@ -261,18 +256,6 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config) > > return 0; > > } > > > > -int PipelineHandlerVimc::allocateBuffers(Camera *camera, > > - const std::set<Stream *> &streams) > > -{ > > - return 0; > > -} > > - > > -int PipelineHandlerVimc::freeBuffers(Camera *camera, > > - const std::set<Stream *> &streams) > > -{ > > - return 0; > > -} > > - > > int PipelineHandlerVimc::start(Camera *camera) > > { > > VimcCameraData *data = cameraData(camera); > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > index b107e23258dee692..67758f3f1ebc39e5 100644 > > --- a/src/libcamera/pipeline_handler.cpp > > +++ b/src/libcamera/pipeline_handler.cpp > > @@ -287,35 +287,6 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera) > > * \return 0 on success or a negative error code otherwise > > */ > > > > -/** > > - * \fn PipelineHandler::allocateBuffers() > > - * \brief Allocate buffers for a stream > > - * \param[in] camera The camera the \a stream belongs to > > - * \param[in] streams The set of streams to allocate buffers for > > - * > > - * This method allocates buffers internally in the pipeline handler for each > > - * stream in the \a streams buffer set, and associates them with the stream's > > - * buffer pool. > > - * > > - * The intended caller of this method is the Camera class. > > - * > > - * \return 0 on success or a negative error code otherwise > > - */ > > - > > -/** > > - * \fn PipelineHandler::freeBuffers() > > - * \brief Free all buffers associated with a stream > > - * \param[in] camera The camera the \a stream belongs to > > - * \param[in] streams The set of streams to free buffers from > > - * > > - * After a capture session has been stopped all buffers associated with each > > - * stream shall be freed. > > - * > > - * The intended caller of this method is the Camera class. > > - * > > - * \return 0 on success or a negative error code otherwise > > - */ > > - > > /** > > * \fn PipelineHandler::start() > > * \brief Start capturing from a group of streams
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 05fdcaab8f918afa..12878a1c2e6076d3 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -709,12 +709,6 @@ int Camera::allocateBuffers() return -EINVAL; } - int ret = pipe_->allocateBuffers(this, activeStreams_); - if (ret) { - LOG(Camera, Error) << "Failed to allocate buffers"; - return ret; - } - state_ = CameraPrepared; return 0; @@ -735,7 +729,7 @@ int Camera::freeBuffers() state_ = CameraConfigured; - return pipe_->freeBuffers(this, activeStreams_); + return 0; } /** diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h index e4588f10bac0228c..9e5dca894aff45ae 100644 --- a/src/libcamera/include/pipeline_handler.h +++ b/src/libcamera/include/pipeline_handler.h @@ -69,11 +69,6 @@ public: const StreamRoles &roles) = 0; virtual int configure(Camera *camera, CameraConfiguration *config) = 0; - virtual int allocateBuffers(Camera *camera, - const std::set<Stream *> &streams) = 0; - virtual int freeBuffers(Camera *camera, - const std::set<Stream *> &streams) = 0; - virtual int start(Camera *camera) = 0; virtual void stop(Camera *camera) = 0; diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 8d9420aea3eb0b21..6d6ecb511777ec49 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -215,10 +215,8 @@ public: const StreamRoles &roles) override; int configure(Camera *camera, CameraConfiguration *config) override; - int allocateBuffers(Camera *camera, - const std::set<Stream *> &streams) override; - int freeBuffers(Camera *camera, - const std::set<Stream *> &streams) override; + int allocateBuffers(Camera *camera); + int freeBuffers(Camera *camera); int start(Camera *camera) override; void stop(Camera *camera) override; @@ -629,8 +627,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) * In order to be able to start the 'viewfinder' and 'stat' nodes, we need * memory to be reserved. */ -int PipelineHandlerIPU3::allocateBuffers(Camera *camera, - const std::set<Stream *> &streams) +int PipelineHandlerIPU3::allocateBuffers(Camera *camera) { IPU3CameraData *data = cameraData(camera); unsigned int bufferCount; @@ -670,13 +667,12 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera, return 0; error: - freeBuffers(camera, streams); + freeBuffers(camera); return ret; } -int PipelineHandlerIPU3::freeBuffers(Camera *camera, - const std::set<Stream *> &streams) +int PipelineHandlerIPU3::freeBuffers(Camera *camera) { IPU3CameraData *data = cameraData(camera); @@ -693,6 +689,10 @@ int PipelineHandlerIPU3::start(Camera *camera) ImgUDevice *imgu = data->imgu_; int ret; + ret = allocateBuffers(camera); + if (ret) + return ret; + /* * Start the ImgU video devices, buffers will be queued to the * ImgU output and viewfinder when requests will be queued. @@ -711,6 +711,7 @@ int PipelineHandlerIPU3::start(Camera *camera) return 0; error: + freeBuffers(camera); LOG(IPU3, Error) << "Failed to start camera " << camera->name(); return ret; @@ -726,6 +727,8 @@ void PipelineHandlerIPU3::stop(Camera *camera) if (ret) LOG(IPU3, Warning) << "Failed to stop camera " << camera->name(); + + freeBuffers(camera); } int PipelineHandlerIPU3::queueRequestHardware(Camera *camera, Request *request) diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 96e863f0208fa748..a56c7022f57ce771 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -174,10 +174,8 @@ public: const StreamRoles &roles) override; int configure(Camera *camera, CameraConfiguration *config) override; - int allocateBuffers(Camera *camera, - const std::set<Stream *> &streams) override; - int freeBuffers(Camera *camera, - const std::set<Stream *> &streams) override; + int allocateBuffers(Camera *camera); + int freeBuffers(Camera *camera); int start(Camera *camera) override; void stop(Camera *camera) override; @@ -651,8 +649,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) return 0; } -int PipelineHandlerRkISP1::allocateBuffers(Camera *camera, - const std::set<Stream *> &streams) +int PipelineHandlerRkISP1::allocateBuffers(Camera *camera) { RkISP1CameraData *data = cameraData(camera); std::vector<FrameBuffer *> paramBuffers, statBuffers; @@ -695,8 +692,7 @@ err: return ret; } -int PipelineHandlerRkISP1::freeBuffers(Camera *camera, - const std::set<Stream *> &streams) +int PipelineHandlerRkISP1::freeBuffers(Camera *camera) { RkISP1CameraData *data = cameraData(camera); @@ -731,10 +727,15 @@ int PipelineHandlerRkISP1::start(Camera *camera) RkISP1CameraData *data = cameraData(camera); int ret; + ret = allocateBuffers(camera); + if (ret) + return ret; + data->frame_ = 0; ret = param_->streamOn(); if (ret) { + freeBuffers(camera); LOG(RkISP1, Error) << "Failed to start parameters " << camera->name(); return ret; @@ -743,6 +744,7 @@ int PipelineHandlerRkISP1::start(Camera *camera) ret = stat_->streamOn(); if (ret) { param_->streamOff(); + freeBuffers(camera); LOG(RkISP1, Error) << "Failed to start statistics " << camera->name(); return ret; @@ -752,6 +754,7 @@ int PipelineHandlerRkISP1::start(Camera *camera) if (ret) { param_->streamOff(); stat_->streamOff(); + freeBuffers(camera); LOG(RkISP1, Error) << "Failed to start camera " << camera->name(); @@ -796,6 +799,8 @@ void PipelineHandlerRkISP1::stop(Camera *camera) data->timeline_.reset(); + freeBuffers(camera); + activeCamera_ = nullptr; } diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp index c275b9f0f75429bd..2be7864ef59e932b 100644 --- a/src/libcamera/pipeline/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo.cpp @@ -65,11 +65,6 @@ public: const StreamRoles &roles) override; int configure(Camera *camera, CameraConfiguration *config) override; - int allocateBuffers(Camera *camera, - const std::set<Stream *> &streams) override; - int freeBuffers(Camera *camera, - const std::set<Stream *> &streams) override; - int start(Camera *camera) override; void stop(Camera *camera) override; @@ -193,18 +188,6 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config) return 0; } -int PipelineHandlerUVC::allocateBuffers(Camera *camera, - const std::set<Stream *> &streams) -{ - return 0; -} - -int PipelineHandlerUVC::freeBuffers(Camera *camera, - const std::set<Stream *> &streams) -{ - return 0; -} - int PipelineHandlerUVC::start(Camera *camera) { UVCCameraData *data = cameraData(camera); diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp index 64793788c752c791..cacf8bfe3ff9cc45 100644 --- a/src/libcamera/pipeline/vimc.cpp +++ b/src/libcamera/pipeline/vimc.cpp @@ -84,11 +84,6 @@ public: const StreamRoles &roles) override; int configure(Camera *camera, CameraConfiguration *config) override; - int allocateBuffers(Camera *camera, - const std::set<Stream *> &streams) override; - int freeBuffers(Camera *camera, - const std::set<Stream *> &streams) override; - int start(Camera *camera) override; void stop(Camera *camera) override; @@ -261,18 +256,6 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config) return 0; } -int PipelineHandlerVimc::allocateBuffers(Camera *camera, - const std::set<Stream *> &streams) -{ - return 0; -} - -int PipelineHandlerVimc::freeBuffers(Camera *camera, - const std::set<Stream *> &streams) -{ - return 0; -} - int PipelineHandlerVimc::start(Camera *camera) { VimcCameraData *data = cameraData(camera); diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index b107e23258dee692..67758f3f1ebc39e5 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -287,35 +287,6 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera) * \return 0 on success or a negative error code otherwise */ -/** - * \fn PipelineHandler::allocateBuffers() - * \brief Allocate buffers for a stream - * \param[in] camera The camera the \a stream belongs to - * \param[in] streams The set of streams to allocate buffers for - * - * This method allocates buffers internally in the pipeline handler for each - * stream in the \a streams buffer set, and associates them with the stream's - * buffer pool. - * - * The intended caller of this method is the Camera class. - * - * \return 0 on success or a negative error code otherwise - */ - -/** - * \fn PipelineHandler::freeBuffers() - * \brief Free all buffers associated with a stream - * \param[in] camera The camera the \a stream belongs to - * \param[in] streams The set of streams to free buffers from - * - * After a capture session has been stopped all buffers associated with each - * stream shall be freed. - * - * The intended caller of this method is the Camera class. - * - * \return 0 on success or a negative error code otherwise - */ - /** * \fn PipelineHandler::start() * \brief Start capturing from a group of streams
With FrameBuffer interface in place there is no need for the Camera to call into the specific pipelines allocation and freeing of buffers as it no longer needs to be synced with buffer allocation by the application. Remove the function prototypes in the pipeline handler base class and fold the functionality in the pipelines start() and stop() functions where needed. A follow up patch will remove the now no-op Camera::allocateBuffers() and Camera::freeBuffers(). Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/camera.cpp | 8 +------ src/libcamera/include/pipeline_handler.h | 5 ---- src/libcamera/pipeline/ipu3/ipu3.cpp | 21 +++++++++-------- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 21 ++++++++++------- src/libcamera/pipeline/uvcvideo.cpp | 17 -------------- src/libcamera/pipeline/vimc.cpp | 17 -------------- src/libcamera/pipeline_handler.cpp | 29 ------------------------ 7 files changed, 26 insertions(+), 92 deletions(-)