[libcamera-devel,RFC,4/5] libcamera: pipeline: uvcvideo: Allocate metadata buffers

18941 diff mbox series

Perform the allocation and mapping of metadata buffers into
libcamera's virtual address space.  UVC metadata buffers cannot be
exported as DMA buffer file descriptors, so use the MappedFrameBuffer
class to map them into memory directly.  This will give the UVC
pipeline access to buffer data to extract timestamp information.

Metadata buffers are internal to the UVC pipeline, so buffer memory
should not be exposed to the user.

Signed-off-by: Gabby George <gabbymg94@gmail.com>
---
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 97 +++++++++++++++++++-
 1 file changed, 95 insertions(+), 2 deletions(-)

Quoting Gabby George (2023-08-14 12:28:48)
> Perform the allocation and mapping of metadata buffers into
> libcamera's virtual address space.  UVC metadata buffers cannot be
> exported as DMA buffer file descriptors, so use the MappedFrameBuffer
> class to map them into memory directly.  This will give the UVC
> pipeline access to buffer data to extract timestamp information.
> 
> Metadata buffers are internal to the UVC pipeline, so buffer memory
> should not be exposed to the user.

Sounds reasonable.


> 
> Signed-off-by: Gabby George <gabbymg94@gmail.com>
> ---
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 97 +++++++++++++++++++-
>  1 file changed, 95 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 4470d8a2..51f30187 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -24,6 +24,7 @@
>  
>  #include "libcamera/internal/camera.h"
>  #include "libcamera/internal/device_enumerator.h"
> +#include "libcamera/internal/mapped_framebuffer.h"
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/pipeline_handler.h"
>  #include "libcamera/internal/sysfs.h"
> @@ -51,11 +52,17 @@ public:
>         std::unique_ptr<V4L2VideoDevice> video_;
>         std::unique_ptr<V4L2VideoDevice> metadata_;
>         Stream stream_;
> +       std::vector<std::unique_ptr<FrameBuffer>> metadataBuffers_;
> +       std::map<unsigned int, MappedFrameBuffer> mappedMetadataBuffers_;
> +       bool useMetadataStream_;

I wonder if we'll need this bool. Just from reading these 6 lines, I
could probably assume we could replace anything that has 
	if (useMoetadataStream_)
with
	if (metadata_)



> +
>         std::map<PixelFormat, std::vector<SizeRange>> formats_;
>  
>  private:
>         int initMetadata(MediaDevice *media);
>  
> +       const unsigned int minLengthHeaderBuf_ = 10;
> +
>         bool generateId();
>  
>         std::string id_;
> @@ -96,6 +103,10 @@ private:
>                            const ControlValue &value);
>         int processControls(UVCCameraData *data, Request *request);
>  
> +       int createMetadataBuffers(Camera *camera, unsigned int count);
> +       int cleanupMetadataBuffers(Camera *camera);
> +       int cleanup(Camera *camera);
> +
>         UVCCameraData *cameraData(Camera *camera)
>         {
>                 return static_cast<UVCCameraData *>(camera->_d());
> @@ -225,10 +236,66 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)
>                 return -EINVAL;
>  
>         cfg.setStream(&data->stream_);
> +       return 0;
> +}
>  
> +int PipelineHandlerUVC::cleanupMetadataBuffers(Camera *camera)
> +{
> +       int ret = 0;
> +       UVCCameraData *data = cameraData(camera);
> +
> +       ret = data->metadata_->releaseBuffers();
> +       data->metadataBuffers_.clear(); //call the destructor for the frame buffers

I think we can remove the comment on that line.

> +       data->mappedMetadataBuffers_.clear();
> +       data->useMetadataStream_ = false;
> +
> +       return ret;
> +}
> +
> +int PipelineHandlerUVC::cleanup(Camera *camera)
> +{
> +       UVCCameraData *data = cameraData(camera);
> +       cleanupMetadataBuffers(camera);
> +       data->video_->releaseBuffers();
>         return 0;
>  }
>  
> +/*

Open a block comment with
 	/**

if it's a documentation comment for doxygen.


> + * UVC Metadata stream does not support exporting buffers via EXPBUF,
> + * so it is necessary to create and store mmap-ed addresses.
> + * Metadata buffers are internal to libcamera. They are not, and
> + * cannot be, exposed to the user.
> + *
> + * Returns the number of buffers allocated and mapped.
> + *
> + * \return The number of buffers allocated, or a negative error code if
> + * the number of buffers allocated was not equal to "count"
> + * \retval -EINVAL if "count" buffers were not successfully allocated.
> + * \retval -ENOMEM if mmap failed.

Hrm ... Have you checked if this is what MappedFrameBuffer.error() will
return? Maybe this should refer to saying 'MappedFrameBuffer::error()'
if the Frame could not be mapped successfully ...

> + */
> +int PipelineHandlerUVC::createMetadataBuffers(Camera *camera, unsigned int count)
> +{
> +       UVCCameraData *data = cameraData(camera);
> +       int ret = data->metadata_->allocateBuffers(count, &data->metadataBuffers_);
> +       if (ret < 0)
> +               return -EINVAL;
> +
> +       for (unsigned int i = 0; i < count; i++) {
> +               MappedFrameBuffer mappedBuffer(data->metadataBuffers_[i].get(),
> +                                              MappedFrameBuffer::MapFlag::Read, true);
> +               if (!mappedBuffer.isValid()) {
> +                       LOG(UVC, Error)
> +                               << "Failed to mmap metadata buffer: "
> +                               << strerror(mappedBuffer.error());
> +                       return mappedBuffer.error();
> +               }
> +
> +               data->mappedMetadataBuffers_.emplace(i, std::move(mappedBuffer));
> +               data->metadataBuffers_[i]->setCookie(i);
> +       }
> +       return ret;
> +}
> +
>  int PipelineHandlerUVC::exportFrameBuffers(Camera *camera, Stream *stream,
>                                            std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>  {
> @@ -247,20 +314,46 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList
>         if (ret < 0)
>                 return ret;
>  
> +       if (data->useMetadataStream_) {
> +               if (createMetadataBuffers(camera, count) < 0) {
> +                       LOG(UVC, Error) << "Unable to allocate buffers for UVC metadata stream.";
> +                       data->useMetadataStream_ = false;
> +               }
> +       }
> +
>         ret = data->video_->streamOn();
>         if (ret < 0) {
> -               data->video_->releaseBuffers();
> +               cleanup(camera);
>                 return ret;
>         }
>  
> +       if (data->useMetadataStream_) {
> +               ret = data->metadata_->streamOn();
> +               if (ret) {
> +                       LOG(UVC, Error) << "Failed to start metadata stream";
> +                       return ret;
> +               }
> +
> +               for (std::unique_ptr<FrameBuffer> &buf : data->metadataBuffers_) {
> +                       ret = data->metadata_->queueBuffer(buf.get());
> +                       if (ret < 0) {
> +                               cleanupMetadataBuffers(camera);
> +                               return ret;
> +                       }
> +               }

Can all of this be done before video_->streamOn() to avoid having two
conditionals that check if we useMetadataStream_ ?


> +       }
>         return 0;
>  }
>  
>  void PipelineHandlerUVC::stopDevice(Camera *camera)
>  {
>         UVCCameraData *data = cameraData(camera);
> +
>         data->video_->streamOff();
> -       data->video_->releaseBuffers();
> +
> +       data->metadata_->streamOff();
> +
> +       cleanup(camera);
>  }
>  
>  int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> -- 
> 2.34.1
>