| Message ID | 20200314235728.15495-8-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your work. On 2020-03-15 01:57:26 +0200, Laurent Pinchart wrote: > Use the V4L2 buffer orphaning feature, exposed through > V4L2VideoDevice::exportBuffers(), to decouple buffer import and export. > The PipelineHandler::importFrameBuffers() function is now called for all > streams regardless of whether exportFrameBuffers() has been called or > not. This simplifies the Camera implementation slightly, and opens the > door to additional simplifications. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > include/libcamera/camera.h | 1 - > src/libcamera/camera.cpp | 21 +------------------- > src/libcamera/framebuffer_allocator.cpp | 9 --------- > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- > src/libcamera/pipeline/uvcvideo.cpp | 2 +- > src/libcamera/pipeline/vimc.cpp | 2 +- > src/libcamera/pipeline_handler.cpp | 25 +++++------------------- > 8 files changed, 10 insertions(+), 54 deletions(-) > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > index c453b95228a7..a5e2b49f0f25 100644 > --- a/include/libcamera/camera.h > +++ b/include/libcamera/camera.h > @@ -113,7 +113,6 @@ private: > friend class FrameBufferAllocator; > int exportFrameBuffers(Stream *stream, > std::vector<std::unique_ptr<FrameBuffer>> *buffers); > - int freeFrameBuffers(Stream *stream); > /* \todo Remove allocator_ from the exposed API */ > FrameBufferAllocator *allocator_; > }; > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 9c432adb1d97..3192dfb42d01 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -554,18 +554,6 @@ int Camera::exportFrameBuffers(Stream *stream, > buffers); > } > > -int Camera::freeFrameBuffers(Stream *stream) > -{ > - int ret = p_->isAccessAllowed(Private::CameraConfigured, true); > - if (ret < 0) > - return ret; > - > - p_->pipe_->invokeMethod(&PipelineHandler::freeFrameBuffers, > - ConnectionTypeBlocking, this, stream); > - > - return 0; > -} > - > /** > * \brief Acquire the camera device for exclusive access > * > @@ -928,9 +916,6 @@ int Camera::start() > LOG(Camera, Debug) << "Starting capture"; > > for (Stream *stream : p_->activeStreams_) { > - if (allocator_ && !allocator_->buffers(stream).empty()) > - continue; > - > ret = p_->pipe_->invokeMethod(&PipelineHandler::importFrameBuffers, > ConnectionTypeDirect, this, stream); > if (ret < 0) > @@ -974,13 +959,9 @@ int Camera::stop() > p_->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking, > this); > > - for (Stream *stream : p_->activeStreams_) { > - if (allocator_ && !allocator_->buffers(stream).empty()) > - continue; > - > + for (Stream *stream : p_->activeStreams_) > p_->pipe_->invokeMethod(&PipelineHandler::freeFrameBuffers, > ConnectionTypeBlocking, this, stream); > - } > > return 0; > } > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp > index e79f4a8f65c8..6f7a2e90b08a 100644 > --- a/src/libcamera/framebuffer_allocator.cpp > +++ b/src/libcamera/framebuffer_allocator.cpp > @@ -87,11 +87,6 @@ FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr<Camera> camera) > > FrameBufferAllocator::~FrameBufferAllocator() > { > - for (auto &value : buffers_) { > - Stream *stream = value.first; > - camera_->freeFrameBuffers(stream); > - } > - > buffers_.clear(); > > camera_->allocator_ = nullptr; > @@ -148,10 +143,6 @@ int FrameBufferAllocator::free(Stream *stream) > if (iter == buffers_.end()) > return -EINVAL; > > - int ret = camera_->freeFrameBuffers(stream); > - if (ret < 0) > - return ret; > - > std::vector<std::unique_ptr<FrameBuffer>> &buffers = iter->second; > buffers.clear(); > buffers_.erase(iter); > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 10a2698bad09..b6db8d567ea4 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -622,7 +622,7 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream, > V4L2VideoDevice *video = ipu3stream->device_->dev; > unsigned int count = stream->configuration().bufferCount; > > - return video->allocateBuffers(count, buffers); > + return video->exportBuffers(count, buffers); > } > > int PipelineHandlerIPU3::importFrameBuffers(Camera *camera, Stream *stream) > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index f6934324c5a3..1ad53fbde112 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -665,7 +665,7 @@ int PipelineHandlerRkISP1::exportFrameBuffers(Camera *camera, Stream *stream, > std::vector<std::unique_ptr<FrameBuffer>> *buffers) > { > unsigned int count = stream->configuration().bufferCount; > - return video_->allocateBuffers(count, buffers); > + return video_->exportBuffers(count, buffers); > } > > int PipelineHandlerRkISP1::importFrameBuffers(Camera *camera, Stream *stream) > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > index d81627c224ea..29afb121aa46 100644 > --- a/src/libcamera/pipeline/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo.cpp > @@ -199,7 +199,7 @@ int PipelineHandlerUVC::exportFrameBuffers(Camera *camera, Stream *stream, > UVCCameraData *data = cameraData(camera); > unsigned int count = stream->configuration().bufferCount; > > - return data->video_->allocateBuffers(count, buffers); > + return data->video_->exportBuffers(count, buffers); > } > > int PipelineHandlerUVC::importFrameBuffers(Camera *camera, Stream *stream) > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > index bb94ef7fd38d..5d3d12fef30b 100644 > --- a/src/libcamera/pipeline/vimc.cpp > +++ b/src/libcamera/pipeline/vimc.cpp > @@ -265,7 +265,7 @@ int PipelineHandlerVimc::exportFrameBuffers(Camera *camera, Stream *stream, > VimcCameraData *data = cameraData(camera); > unsigned int count = stream->configuration().bufferCount; > > - return data->video_->allocateBuffers(count, buffers); > + return data->video_->exportBuffers(count, buffers); > } > > int PipelineHandlerVimc::importFrameBuffers(Camera *camera, Stream *stream) > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index 8d623f5431fa..e5034c54e2fb 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -337,16 +337,7 @@ const ControlList &PipelineHandler::properties(Camera *camera) > * > * The method may only be called after the Camera has been configured and before > * it gets started, or after it gets stopped. It shall be called only for > - * streams that are part of the active camera configuration, and at most once > - * per stream until buffers for the stream are freed with freeFrameBuffers(). > - * > - * exportFrameBuffers() shall also allocate all other resources required by > - * the pipeline handler for the stream to prepare for starting the Camera. This > - * responsibility is shared with importFrameBuffers(), and one and only one of > - * those two methods shall be called for each stream until the buffers are > - * freed. The pipeline handler shall support all combinations of > - * exportFrameBuffers() and importFrameBuffers() for the streams contained in > - * any camera configuration. > + * streams that are part of the active camera configuration. > * > * The only intended caller is Camera::exportFrameBuffers(). > * > @@ -371,12 +362,7 @@ const ControlList &PipelineHandler::properties(Camera *camera) > * per stream until buffers for the stream are freed with freeFrameBuffers(). > * > * importFrameBuffers() shall also allocate all other resources required by the > - * pipeline handler for the stream to prepare for starting the Camera. This > - * responsibility is shared with exportFrameBuffers(), and one and only one of > - * those two methods shall be called for each stream until the buffers are > - * freed. The pipeline handler shall support all combinations of > - * exportFrameBuffers() and importFrameBuffers() for the streams contained in > - * any camera configuration. > + * pipeline handler for the stream to prepare for starting the Camera. > * > * The only intended caller is Camera::start(). > * > @@ -391,10 +377,9 @@ const ControlList &PipelineHandler::properties(Camera *camera) > * \param[in] camera The camera > * \param[in] stream The stream to free buffers for > * > - * This method shall free all buffers and all other resources allocated for the > - * \a stream by exportFrameBuffers() or importFrameBuffers(). It shall be > - * called only after a successful call to either of these two methods, and only > - * once per stream. > + * This method shall release all resources allocated for the \a stream by > + * importFrameBuffers(). It shall be called only after a successful call that > + * method, and only once per stream. > * > * The only intended callers are Camera::stop() and Camera::freeFrameBuffers(). > * > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h index c453b95228a7..a5e2b49f0f25 100644 --- a/include/libcamera/camera.h +++ b/include/libcamera/camera.h @@ -113,7 +113,6 @@ private: friend class FrameBufferAllocator; int exportFrameBuffers(Stream *stream, std::vector<std::unique_ptr<FrameBuffer>> *buffers); - int freeFrameBuffers(Stream *stream); /* \todo Remove allocator_ from the exposed API */ FrameBufferAllocator *allocator_; }; diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 9c432adb1d97..3192dfb42d01 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -554,18 +554,6 @@ int Camera::exportFrameBuffers(Stream *stream, buffers); } -int Camera::freeFrameBuffers(Stream *stream) -{ - int ret = p_->isAccessAllowed(Private::CameraConfigured, true); - if (ret < 0) - return ret; - - p_->pipe_->invokeMethod(&PipelineHandler::freeFrameBuffers, - ConnectionTypeBlocking, this, stream); - - return 0; -} - /** * \brief Acquire the camera device for exclusive access * @@ -928,9 +916,6 @@ int Camera::start() LOG(Camera, Debug) << "Starting capture"; for (Stream *stream : p_->activeStreams_) { - if (allocator_ && !allocator_->buffers(stream).empty()) - continue; - ret = p_->pipe_->invokeMethod(&PipelineHandler::importFrameBuffers, ConnectionTypeDirect, this, stream); if (ret < 0) @@ -974,13 +959,9 @@ int Camera::stop() p_->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking, this); - for (Stream *stream : p_->activeStreams_) { - if (allocator_ && !allocator_->buffers(stream).empty()) - continue; - + for (Stream *stream : p_->activeStreams_) p_->pipe_->invokeMethod(&PipelineHandler::freeFrameBuffers, ConnectionTypeBlocking, this, stream); - } return 0; } diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp index e79f4a8f65c8..6f7a2e90b08a 100644 --- a/src/libcamera/framebuffer_allocator.cpp +++ b/src/libcamera/framebuffer_allocator.cpp @@ -87,11 +87,6 @@ FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr<Camera> camera) FrameBufferAllocator::~FrameBufferAllocator() { - for (auto &value : buffers_) { - Stream *stream = value.first; - camera_->freeFrameBuffers(stream); - } - buffers_.clear(); camera_->allocator_ = nullptr; @@ -148,10 +143,6 @@ int FrameBufferAllocator::free(Stream *stream) if (iter == buffers_.end()) return -EINVAL; - int ret = camera_->freeFrameBuffers(stream); - if (ret < 0) - return ret; - std::vector<std::unique_ptr<FrameBuffer>> &buffers = iter->second; buffers.clear(); buffers_.erase(iter); diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 10a2698bad09..b6db8d567ea4 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -622,7 +622,7 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream, V4L2VideoDevice *video = ipu3stream->device_->dev; unsigned int count = stream->configuration().bufferCount; - return video->allocateBuffers(count, buffers); + return video->exportBuffers(count, buffers); } int PipelineHandlerIPU3::importFrameBuffers(Camera *camera, Stream *stream) diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index f6934324c5a3..1ad53fbde112 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -665,7 +665,7 @@ int PipelineHandlerRkISP1::exportFrameBuffers(Camera *camera, Stream *stream, std::vector<std::unique_ptr<FrameBuffer>> *buffers) { unsigned int count = stream->configuration().bufferCount; - return video_->allocateBuffers(count, buffers); + return video_->exportBuffers(count, buffers); } int PipelineHandlerRkISP1::importFrameBuffers(Camera *camera, Stream *stream) diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp index d81627c224ea..29afb121aa46 100644 --- a/src/libcamera/pipeline/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo.cpp @@ -199,7 +199,7 @@ int PipelineHandlerUVC::exportFrameBuffers(Camera *camera, Stream *stream, UVCCameraData *data = cameraData(camera); unsigned int count = stream->configuration().bufferCount; - return data->video_->allocateBuffers(count, buffers); + return data->video_->exportBuffers(count, buffers); } int PipelineHandlerUVC::importFrameBuffers(Camera *camera, Stream *stream) diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp index bb94ef7fd38d..5d3d12fef30b 100644 --- a/src/libcamera/pipeline/vimc.cpp +++ b/src/libcamera/pipeline/vimc.cpp @@ -265,7 +265,7 @@ int PipelineHandlerVimc::exportFrameBuffers(Camera *camera, Stream *stream, VimcCameraData *data = cameraData(camera); unsigned int count = stream->configuration().bufferCount; - return data->video_->allocateBuffers(count, buffers); + return data->video_->exportBuffers(count, buffers); } int PipelineHandlerVimc::importFrameBuffers(Camera *camera, Stream *stream) diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index 8d623f5431fa..e5034c54e2fb 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -337,16 +337,7 @@ const ControlList &PipelineHandler::properties(Camera *camera) * * The method may only be called after the Camera has been configured and before * it gets started, or after it gets stopped. It shall be called only for - * streams that are part of the active camera configuration, and at most once - * per stream until buffers for the stream are freed with freeFrameBuffers(). - * - * exportFrameBuffers() shall also allocate all other resources required by - * the pipeline handler for the stream to prepare for starting the Camera. This - * responsibility is shared with importFrameBuffers(), and one and only one of - * those two methods shall be called for each stream until the buffers are - * freed. The pipeline handler shall support all combinations of - * exportFrameBuffers() and importFrameBuffers() for the streams contained in - * any camera configuration. + * streams that are part of the active camera configuration. * * The only intended caller is Camera::exportFrameBuffers(). * @@ -371,12 +362,7 @@ const ControlList &PipelineHandler::properties(Camera *camera) * per stream until buffers for the stream are freed with freeFrameBuffers(). * * importFrameBuffers() shall also allocate all other resources required by the - * pipeline handler for the stream to prepare for starting the Camera. This - * responsibility is shared with exportFrameBuffers(), and one and only one of - * those two methods shall be called for each stream until the buffers are - * freed. The pipeline handler shall support all combinations of - * exportFrameBuffers() and importFrameBuffers() for the streams contained in - * any camera configuration. + * pipeline handler for the stream to prepare for starting the Camera. * * The only intended caller is Camera::start(). * @@ -391,10 +377,9 @@ const ControlList &PipelineHandler::properties(Camera *camera) * \param[in] camera The camera * \param[in] stream The stream to free buffers for * - * This method shall free all buffers and all other resources allocated for the - * \a stream by exportFrameBuffers() or importFrameBuffers(). It shall be - * called only after a successful call to either of these two methods, and only - * once per stream. + * This method shall release all resources allocated for the \a stream by + * importFrameBuffers(). It shall be called only after a successful call that + * method, and only once per stream. * * The only intended callers are Camera::stop() and Camera::freeFrameBuffers(). *
Use the V4L2 buffer orphaning feature, exposed through V4L2VideoDevice::exportBuffers(), to decouple buffer import and export. The PipelineHandler::importFrameBuffers() function is now called for all streams regardless of whether exportFrameBuffers() has been called or not. This simplifies the Camera implementation slightly, and opens the door to additional simplifications. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/camera.h | 1 - src/libcamera/camera.cpp | 21 +------------------- src/libcamera/framebuffer_allocator.cpp | 9 --------- src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- src/libcamera/pipeline/uvcvideo.cpp | 2 +- src/libcamera/pipeline/vimc.cpp | 2 +- src/libcamera/pipeline_handler.cpp | 25 +++++------------------- 8 files changed, 10 insertions(+), 54 deletions(-)