[v3,05/16] libcamera: shared_mem_object: reorganize the code and document the SharedMemObject class
diff mbox series

Message ID 20240214170122.60754-6-hdegoede@redhat.com
State Superseded
Headers show
Series
  • libcamera: introduce Software ISP and Software IPA
Related show

Commit Message

Hans de Goede Feb. 14, 2024, 5:01 p.m. UTC
From: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>

Split the parts which doesn't otherwise depend on the type T or
arguments Args out of the SharedMemObject class into a new
SharedMem class.

Doxygen documentation by Dennis Bonke and Andrei Konovalov.

Reviewed-by: Pavel Machek <pavel@ucw.cz>
Co-developed-by: Dennis Bonke <admin@dennisbonke.com>
Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
Signed-off-by: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>
---
 .../libcamera/internal/shared_mem_object.h    |  92 ++++++---
 src/libcamera/meson.build                     |   1 +
 src/libcamera/shared_mem_object.cpp           | 191 ++++++++++++++++++
 3 files changed, 253 insertions(+), 31 deletions(-)
 create mode 100644 src/libcamera/shared_mem_object.cpp

Comments

Milan Zamazal Feb. 15, 2024, 1:30 p.m. UTC | #1
Hans de Goede <hdegoede@redhat.com> writes:

> From: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>
>
> Split the parts which doesn't otherwise depend on the type T or
> arguments Args out of the SharedMemObject class into a new
> SharedMem class.
>
> Doxygen documentation by Dennis Bonke and Andrei Konovalov.
>
> Reviewed-by: Pavel Machek <pavel@ucw.cz>
> Co-developed-by: Dennis Bonke <admin@dennisbonke.com>
> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
> Signed-off-by: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>
> ---
>  .../libcamera/internal/shared_mem_object.h    |  92 ++++++---
>  src/libcamera/meson.build                     |   1 +
>  src/libcamera/shared_mem_object.cpp           | 191 ++++++++++++++++++
>  3 files changed, 253 insertions(+), 31 deletions(-)
>  create mode 100644 src/libcamera/shared_mem_object.cpp
>
> diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h
> index bfb639ee..a4de6500 100644
> --- a/include/libcamera/internal/shared_mem_object.h
> +++ b/include/libcamera/internal/shared_mem_object.h
> @@ -7,11 +7,8 @@
>  #pragma once
>  
>  #include <cstddef>
> -#include <fcntl.h>
>  #include <string>
>  #include <sys/mman.h>
> -#include <sys/stat.h>
> -#include <unistd.h>
>  #include <utility>
>  
>  #include <libcamera/base/class.h>
> @@ -19,6 +16,59 @@
>  
>  namespace libcamera {
>  
> +class SharedMem
> +{
> +public:
> +	SharedMem()
> +		: mem_(nullptr)
> +	{
> +	}
> +
> +	SharedMem(const std::string &name, std::size_t size);
> +
> +	SharedMem(SharedMem &&rhs)
> +	{
> +		this->name_ = std::move(rhs.name_);
> +		this->fd_ = std::move(rhs.fd_);
> +		this->mem_ = rhs.mem_;
> +		rhs.mem_ = nullptr;
> +	}
> +
> +	~SharedMem()
> +	{
> +		if (mem_)
> +			munmap(mem_, size_);
> +	}
> +
> +	/* Make SharedMem non-copyable for now. */
> +	LIBCAMERA_DISABLE_COPY(SharedMem)
> +
> +	SharedMem &operator=(SharedMem &&rhs)
> +	{
> +		this->name_ = std::move(rhs.name_);
> +		this->fd_ = std::move(rhs.fd_);
> +		this->mem_ = rhs.mem_;
> +		rhs.mem_ = nullptr;
> +		return *this;
> +	}
> +
> +	const SharedFD &fd() const
> +	{
> +		return fd_;
> +	}
> +
> +	void *mem() const
> +	{
> +		return mem_;
> +	}
> +
> +private:
> +	std::string name_;
> +	SharedFD fd_;
> +	size_t size_;
> +	void *mem_;
> +};
> +
>  template<class T>
>  class SharedMemObject
>  {
> @@ -32,26 +82,11 @@ public:
>  
>  	template<class... Args>
>  	SharedMemObject(const std::string &name, Args &&...args)
> -		: name_(name), obj_(nullptr)
> +		: shMem_(name, SIZE), obj_(nullptr)
>  	{
> -		void *mem;
> -		int ret;
> +		void *mem = shMem_.mem();
>  
> -		ret = memfd_create(name_.c_str(), MFD_CLOEXEC);
> -		if (ret < 0)
> -			return;
> -
> -		fd_ = SharedFD(std::move(ret));
> -		if (!fd_.isValid())
> -			return;
> -
> -		ret = ftruncate(fd_.get(), SIZE);
> -		if (ret < 0)
> -			return;
> -
> -		mem = mmap(nullptr, SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
> -			   fd_.get(), 0);
> -		if (mem == MAP_FAILED)
> +		if (mem == nullptr)
>  			return;
>  
>  		obj_ = new (mem) T(std::forward<Args>(args)...);
> @@ -59,18 +94,15 @@ public:
>  
>  	SharedMemObject(SharedMemObject<T> &&rhs)
>  	{
> -		this->name_ = std::move(rhs.name_);
> -		this->fd_ = std::move(rhs.fd_);
> +		this->shMem_ = std::move(rhs.shMem_);
>  		this->obj_ = rhs.obj_;
>  		rhs.obj_ = nullptr;
>  	}
>  
>  	~SharedMemObject()
>  	{
> -		if (obj_) {
> +		if (obj_)
>  			obj_->~T();
> -			munmap(obj_, SIZE);
> -		}
>  	}
>  
>  	/* Make SharedMemObject non-copyable for now. */
> @@ -78,8 +110,7 @@ public:
>  
>  	SharedMemObject<T> &operator=(SharedMemObject<T> &&rhs)
>  	{
> -		this->name_ = std::move(rhs.name_);
> -		this->fd_ = std::move(rhs.fd_);
> +		this->shMem_ = std::move(rhs.shMem_);
>  		this->obj_ = rhs.obj_;
>  		rhs.obj_ = nullptr;
>  		return *this;
> @@ -107,7 +138,7 @@ public:
>  
>  	const SharedFD &fd() const
>  	{
> -		return fd_;
> +		return shMem_.fd();
>  	}
>  
>  	explicit operator bool() const
> @@ -116,8 +147,7 @@ public:
>  	}
>  
>  private:
> -	std::string name_;
> -	SharedFD fd_;
> +	SharedMem shMem_;
>  	T *obj_;
>  };
>  
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 3c5e43df..94a95ae3 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -41,6 +41,7 @@ libcamera_sources = files([
>      'process.cpp',
>      'pub_key.cpp',
>      'request.cpp',
> +    'shared_mem_object.cpp',
>      'source_paths.cpp',
>      'stream.cpp',
>      'sysfs.cpp',
> diff --git a/src/libcamera/shared_mem_object.cpp b/src/libcamera/shared_mem_object.cpp
> new file mode 100644
> index 00000000..06bbee38
> --- /dev/null
> +++ b/src/libcamera/shared_mem_object.cpp
> @@ -0,0 +1,191 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Raspberry Pi Ltd
> + *
> + * shared_mem_object.cpp - Helper class for shared memory allocations
> + */
> +
> +#include "libcamera/internal/shared_mem_object.h"
> +
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +/**
> + * \file shared_mem_object.cpp
> + * \brief Helper class for shared memory allocations
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class SharedMem
> + * \brief Helper class for allocating shared memory
> + *
> + * Memory is allocated and exposed as a SharedFD for use across IPC boundaries.
> + *
> + * SharedMem allocates the shared memory of the given size and maps it.
> + * To check that the shared memory was allocated and mapped successfully, one
> + * needs to verify that the pointer to the shared memory returned by SharedMem::mem()
> + * is not nullptr.
> + *
> + * To access the shared memory from another process the SharedFD should be passed
> + * to that process, and then the shared memory should be mapped into that process
> + * address space by calling mmap().
> + *
> + * A single memfd is created for every SharedMem. If there is a need to allocate
> + * a large number of objects in shared memory, these objects should be grouped
> + * together and use the shared memory allocated by a single SharedMem object if
> + * possible. This will help to minimize the number of created memfd's.
> + */
> +
> +/**
> + * \fn SharedMem::SharedMem(const std::string &name, std::size_t size)
> + * \brief Contstructor for the SharedMem
> + * \param[in] name Name of the SharedMem
> + * \param[in] size Size of the shared memory to allocate and map
> + */
> +
> +/**
> + * \fn SharedMem::SharedMem(SharedMem &&rhs)
> + * \brief Move constructor for SharedMem
> + * \param[in] rhs The object to move
> + */
> +
> +/**
> + * \fn SharedMem::~SharedMem()
> + * \brief SharedMem destructor
> + *
> + * Unmaps the allocated shared memory. Decrements the shared memory descriptor use
> + * count.
> + */
> +
> +/**
> + * \fn SharedMem &SharedMem::operator=(SharedMem &&rhs)
> + * \brief Move constructor for SharedMem
> + * \param[in] rhs The object to move
> + */
> +
> +/**
> + * \fn const SharedFD &SharedMem::fd() const
> + * \brief Gets the file descriptor for the underlaying shared memory
> + * \return The file descriptor
> + */
> +
> +/**
> + * \fn void *SharedMem::mem() const
> + * \brief Gets the pointer to the underlaying shared memory
> + * \return The pointer to the shared memory
> + */
> +
> +SharedMem::SharedMem(const std::string &name, std::size_t size)
> +	: name_(name), size_(size), mem_(nullptr)
> +{
> +	int fd = memfd_create(name_.c_str(), MFD_CLOEXEC);
> +	if (fd < 0)
> +		return;
> +
> +	fd_ = SharedFD(std::move(fd));
> +	if (!fd_.isValid())
> +		return;
> +
> +	if (ftruncate(fd_.get(), size_) < 0)
> +		return;
> +
> +	mem_ = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED,
> +		    fd_.get(), 0);
> +	if (mem_ == MAP_FAILED)
> +		mem_ = nullptr;
> +}
> +
> +/**
> + * \class SharedMemObject
> + * \brief Helper class for allocating objects in shared memory
> + *
> + * Memory is allocated and exposed as a SharedFD for use across IPC boundaries.
> + *
> + * Given the type of the object to be created in shared memory and the arguments
> + * to pass to this object's constructor, SharedMemObject allocates the shared memory
> + * of the size of the object and constructs the object in this memory. To check that
> + * the SharedMemObject was created successfully, one needs to verify that the
> + * underlying SharedFD (the reference to it is returned by SharedMemObject::fd() member
> + * function) is valid. The object created in the shared memory can be accessed using
> + * the SharedMemObject::operator*() indirection operator. Its members can be accessed
> + * with the SharedMemObject::operator->() member of pointer operator.

But it can still happen that SharedMemObject::fd() is all right but the object
returned from the operators is nullptr, right?  It would be good to clarify this
and refer to the bool operator here.

> 
> + *
> + * To access the object from another process the SharedFD should be passed to that
> + * process, and the shared memory should be mapped by calling mmap().
> + *
> + * A single memfd is created for every SharedMemObject. If there is a need to allocate
> + * a large number of objects in shared memory, these objects should be grouped into a
> + * single large object to keep the number of created memfd's reasonably small.
> + */
> +
> +/**
> + * \var SharedMemObject::SIZE
> + * \brief The size of the object that is going to be stored here
> + */
> +
> +/**
> + * \fn SharedMemObject< T >::SharedMemObject(const std::string &name, Args &&...args)
> + * \brief Contstructor for the SharedMemObject
             ^^^^^^^^^^^^

Constructor

> + * \param[in] name Name of the SharedMemObject
> + * \param[in] args Args to pass to the constructor of the object in shared memory
> + */
> +
> +/**
> + * \fn SharedMemObject::SharedMemObject(SharedMemObject<T> &&rhs)
> + * \brief Move constructor for SharedMemObject
> + * \param[in] rhs The object to move
> + */
> +
> +/**
> + * \fn SharedMemObject::~SharedMemObject()
> + * \brief SharedMemObject destructor
> + *
> + * Destroys the object created in the shared memory and then unmaps the shared memory.
> + * Decrements the shared memory descriptor use count.
> + */
> +
> +/**
> + * \fn SharedMemObject::operator=(SharedMemObject<T> &&rhs)
> + * \brief Operator= for SharedMemObject
> + * \param[in] rhs The SharedMemObject object to take the data from
> + */
> +
> +/**
> + * \fn SharedMemObject::operator->()
> + * \brief Operator-> for SharedMemObject
> + * \return The pointer to the object
> + */
> +
> +/**
> + * \fn const T *SharedMemObject::operator->() const
> + * \brief Operator-> for SharedMemObject
> + * \return The pointer to the const object
> + */
> +
> +/**
> + * \fn SharedMemObject::operator*()
> + * \brief Operator* for SharedMemObject
> + * \return The reference to the object
> + */
> +
> +/**
> + * \fn const T &SharedMemObject::operator*() const
> + * \brief Operator* for SharedMemObject
> + * \return Const reference to the object
> + */
> +
> +/**
> + * \fn SharedMemObject::fd()
> + * \brief Gets the file descriptor for the underlaying storage file
> + * \return The shared memory file descriptor (as SharedFD)
> + */
> +
> +/**
> + * \fn SharedMemObject::operator bool()
> + * \brief Operator bool() for SharedMemObject
> + * \return True if the object was created OK in the shared memory, false otherwise
> + */
> +
> +} // namespace libcamera
Naushir Patuck Feb. 15, 2024, 1:39 p.m. UTC | #2
Hi Hans,

On Wed, 14 Feb 2024 at 17:01, Hans de Goede <hdegoede@redhat.com> wrote:
>
> From: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>
>
> Split the parts which doesn't otherwise depend on the type T or
> arguments Args out of the SharedMemObject class into a new
> SharedMem class.
>
> Doxygen documentation by Dennis Bonke and Andrei Konovalov.
>
> Reviewed-by: Pavel Machek <pavel@ucw.cz>
> Co-developed-by: Dennis Bonke <admin@dennisbonke.com>
> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
> Signed-off-by: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>
> ---
>  .../libcamera/internal/shared_mem_object.h    |  92 ++++++---
>  src/libcamera/meson.build                     |   1 +
>  src/libcamera/shared_mem_object.cpp           | 191 ++++++++++++++++++
>  3 files changed, 253 insertions(+), 31 deletions(-)
>  create mode 100644 src/libcamera/shared_mem_object.cpp
>
> diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h
> index bfb639ee..a4de6500 100644
> --- a/include/libcamera/internal/shared_mem_object.h
> +++ b/include/libcamera/internal/shared_mem_object.h
> @@ -7,11 +7,8 @@
>  #pragma once
>
>  #include <cstddef>
> -#include <fcntl.h>
>  #include <string>
>  #include <sys/mman.h>
> -#include <sys/stat.h>
> -#include <unistd.h>
>  #include <utility>
>
>  #include <libcamera/base/class.h>
> @@ -19,6 +16,59 @@
>
>  namespace libcamera {
>
> +class SharedMem
> +{
> +public:
> +       SharedMem()
> +               : mem_(nullptr)
> +       {
> +       }
> +
> +       SharedMem(const std::string &name, std::size_t size);
> +
> +       SharedMem(SharedMem &&rhs)
> +       {
> +               this->name_ = std::move(rhs.name_);
> +               this->fd_ = std::move(rhs.fd_);
> +               this->mem_ = rhs.mem_;
> +               rhs.mem_ = nullptr;
> +       }
> +
> +       ~SharedMem()
> +       {
> +               if (mem_)
> +                       munmap(mem_, size_);
> +       }
> +
> +       /* Make SharedMem non-copyable for now. */
> +       LIBCAMERA_DISABLE_COPY(SharedMem)
> +
> +       SharedMem &operator=(SharedMem &&rhs)
> +       {
> +               this->name_ = std::move(rhs.name_);
> +               this->fd_ = std::move(rhs.fd_);
> +               this->mem_ = rhs.mem_;
> +               rhs.mem_ = nullptr;
> +               return *this;
> +       }
> +
> +       const SharedFD &fd() const
> +       {
> +               return fd_;
> +       }
> +
> +       void *mem() const
> +       {
> +               return mem_;
> +       }
> +
> +private:
> +       std::string name_;
> +       SharedFD fd_;
> +       size_t size_;
> +       void *mem_;
> +};
> +
>  template<class T>
>  class SharedMemObject

Would it make sense to have SharedMemObject derived from SharedMem?
Then you would only need a constructor and destructor implemented for
SharedMemObject, and all other accessors can be through the base
class... i think?

>  {
> @@ -32,26 +82,11 @@ public:
>
>         template<class... Args>
>         SharedMemObject(const std::string &name, Args &&...args)
> -               : name_(name), obj_(nullptr)
> +               : shMem_(name, SIZE), obj_(nullptr)
>         {
> -               void *mem;
> -               int ret;
> +               void *mem = shMem_.mem();
>
> -               ret = memfd_create(name_.c_str(), MFD_CLOEXEC);
> -               if (ret < 0)
> -                       return;
> -
> -               fd_ = SharedFD(std::move(ret));
> -               if (!fd_.isValid())
> -                       return;
> -
> -               ret = ftruncate(fd_.get(), SIZE);
> -               if (ret < 0)
> -                       return;
> -
> -               mem = mmap(nullptr, SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
> -                          fd_.get(), 0);
> -               if (mem == MAP_FAILED)
> +               if (mem == nullptr)
>                         return;
>
>                 obj_ = new (mem) T(std::forward<Args>(args)...);
> @@ -59,18 +94,15 @@ public:
>
>         SharedMemObject(SharedMemObject<T> &&rhs)
>         {
> -               this->name_ = std::move(rhs.name_);
> -               this->fd_ = std::move(rhs.fd_);
> +               this->shMem_ = std::move(rhs.shMem_);
>                 this->obj_ = rhs.obj_;
>                 rhs.obj_ = nullptr;
>         }
>
>         ~SharedMemObject()
>         {
> -               if (obj_) {
> +               if (obj_)
>                         obj_->~T();
> -                       munmap(obj_, SIZE);
> -               }
>         }
>
>         /* Make SharedMemObject non-copyable for now. */
> @@ -78,8 +110,7 @@ public:
>
>         SharedMemObject<T> &operator=(SharedMemObject<T> &&rhs)
>         {
> -               this->name_ = std::move(rhs.name_);
> -               this->fd_ = std::move(rhs.fd_);
> +               this->shMem_ = std::move(rhs.shMem_);
>                 this->obj_ = rhs.obj_;
>                 rhs.obj_ = nullptr;
>                 return *this;
> @@ -107,7 +138,7 @@ public:
>
>         const SharedFD &fd() const
>         {
> -               return fd_;
> +               return shMem_.fd();
>         }
>
>         explicit operator bool() const
> @@ -116,8 +147,7 @@ public:
>         }
>
>  private:
> -       std::string name_;
> -       SharedFD fd_;
> +       SharedMem shMem_;
>         T *obj_;
>  };
>
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 3c5e43df..94a95ae3 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -41,6 +41,7 @@ libcamera_sources = files([
>      'process.cpp',
>      'pub_key.cpp',
>      'request.cpp',
> +    'shared_mem_object.cpp',
>      'source_paths.cpp',
>      'stream.cpp',
>      'sysfs.cpp',
> diff --git a/src/libcamera/shared_mem_object.cpp b/src/libcamera/shared_mem_object.cpp
> new file mode 100644
> index 00000000..06bbee38
> --- /dev/null
> +++ b/src/libcamera/shared_mem_object.cpp
> @@ -0,0 +1,191 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Raspberry Pi Ltd
> + *
> + * shared_mem_object.cpp - Helper class for shared memory allocations
> + */
> +
> +#include "libcamera/internal/shared_mem_object.h"
> +
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +/**
> + * \file shared_mem_object.cpp
> + * \brief Helper class for shared memory allocations
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class SharedMem
> + * \brief Helper class for allocating shared memory
> + *
> + * Memory is allocated and exposed as a SharedFD for use across IPC boundaries.
> + *
> + * SharedMem allocates the shared memory of the given size and maps it.
> + * To check that the shared memory was allocated and mapped successfully, one
> + * needs to verify that the pointer to the shared memory returned by SharedMem::mem()
> + * is not nullptr.
> + *
> + * To access the shared memory from another process the SharedFD should be passed
> + * to that process, and then the shared memory should be mapped into that process
> + * address space by calling mmap().
> + *
> + * A single memfd is created for every SharedMem. If there is a need to allocate
> + * a large number of objects in shared memory, these objects should be grouped
> + * together and use the shared memory allocated by a single SharedMem object if
> + * possible. This will help to minimize the number of created memfd's.
> + */
> +
> +/**
> + * \fn SharedMem::SharedMem(const std::string &name, std::size_t size)
> + * \brief Contstructor for the SharedMem

s/Contstructor/Constructor/

> + * \param[in] name Name of the SharedMem
> + * \param[in] size Size of the shared memory to allocate and map
> + */
> +
> +/**
> + * \fn SharedMem::SharedMem(SharedMem &&rhs)
> + * \brief Move constructor for SharedMem
> + * \param[in] rhs The object to move
> + */
> +
> +/**
> + * \fn SharedMem::~SharedMem()
> + * \brief SharedMem destructor
> + *
> + * Unmaps the allocated shared memory. Decrements the shared memory descriptor use
> + * count.
> + */
> +
> +/**
> + * \fn SharedMem &SharedMem::operator=(SharedMem &&rhs)
> + * \brief Move constructor for SharedMem
> + * \param[in] rhs The object to move
> + */
> +
> +/**
> + * \fn const SharedFD &SharedMem::fd() const
> + * \brief Gets the file descriptor for the underlaying shared memory

s/underlaying/underlying/?

> + * \return The file descriptor
> + */
> +
> +/**
> + * \fn void *SharedMem::mem() const
> + * \brief Gets the pointer to the underlaying shared memory

s/underlaying/underlying/?

> + * \return The pointer to the shared memory
> + */
> +
> +SharedMem::SharedMem(const std::string &name, std::size_t size)
> +       : name_(name), size_(size), mem_(nullptr)
> +{
> +       int fd = memfd_create(name_.c_str(), MFD_CLOEXEC);
> +       if (fd < 0)
> +               return;
> +
> +       fd_ = SharedFD(std::move(fd));
> +       if (!fd_.isValid())
> +               return;
> +
> +       if (ftruncate(fd_.get(), size_) < 0)
> +               return;
> +
> +       mem_ = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED,
> +                   fd_.get(), 0);
> +       if (mem_ == MAP_FAILED)
> +               mem_ = nullptr;
> +}
> +
> +/**
> + * \class SharedMemObject
> + * \brief Helper class for allocating objects in shared memory
> + *
> + * Memory is allocated and exposed as a SharedFD for use across IPC boundaries.
> + *
> + * Given the type of the object to be created in shared memory and the arguments
> + * to pass to this object's constructor, SharedMemObject allocates the shared memory
> + * of the size of the object and constructs the object in this memory. To check that
> + * the SharedMemObject was created successfully, one needs to verify that the
> + * underlying SharedFD (the reference to it is returned by SharedMemObject::fd() member
> + * function) is valid. The object created in the shared memory can be accessed using
> + * the SharedMemObject::operator*() indirection operator. Its members can be accessed
> + * with the SharedMemObject::operator->() member of pointer operator.
> + *
> + * To access the object from another process the SharedFD should be passed to that
> + * process, and the shared memory should be mapped by calling mmap().
> + *
> + * A single memfd is created for every SharedMemObject. If there is a need to allocate
> + * a large number of objects in shared memory, these objects should be grouped into a
> + * single large object to keep the number of created memfd's reasonably small.
> + */
> +
> +/**
> + * \var SharedMemObject::SIZE
> + * \brief The size of the object that is going to be stored here
> + */
> +
> +/**
> + * \fn SharedMemObject< T >::SharedMemObject(const std::string &name, Args &&...args)
> + * \brief Contstructor for the SharedMemObject

s/Contstructor/Constructor/

> + * \param[in] name Name of the SharedMemObject
> + * \param[in] args Args to pass to the constructor of the object in shared memory
> + */
> +
> +/**
> + * \fn SharedMemObject::SharedMemObject(SharedMemObject<T> &&rhs)
> + * \brief Move constructor for SharedMemObject
> + * \param[in] rhs The object to move
> + */
> +
> +/**
> + * \fn SharedMemObject::~SharedMemObject()
> + * \brief SharedMemObject destructor
> + *
> + * Destroys the object created in the shared memory and then unmaps the shared memory.
> + * Decrements the shared memory descriptor use count.
> + */
> +
> +/**
> + * \fn SharedMemObject::operator=(SharedMemObject<T> &&rhs)
> + * \brief Operator= for SharedMemObject
> + * \param[in] rhs The SharedMemObject object to take the data from
> + */
> +
> +/**
> + * \fn SharedMemObject::operator->()
> + * \brief Operator-> for SharedMemObject
> + * \return The pointer to the object
> + */
> +
> +/**
> + * \fn const T *SharedMemObject::operator->() const
> + * \brief Operator-> for SharedMemObject
> + * \return The pointer to the const object
> + */
> +
> +/**
> + * \fn SharedMemObject::operator*()
> + * \brief Operator* for SharedMemObject
> + * \return The reference to the object
> + */
> +
> +/**
> + * \fn const T &SharedMemObject::operator*() const
> + * \brief Operator* for SharedMemObject
> + * \return Const reference to the object
> + */
> +
> +/**
> + * \fn SharedMemObject::fd()
> + * \brief Gets the file descriptor for the underlaying storage file

s/underlaying/underlying/

Thanks,
Naush


> + * \return The shared memory file descriptor (as SharedFD)
> + */
> +
> +/**
> + * \fn SharedMemObject::operator bool()
> + * \brief Operator bool() for SharedMemObject
> + * \return True if the object was created OK in the shared memory, false otherwise
> + */
> +
> +} // namespace libcamera
> --
> 2.43.0
>
Andrei Konovalov Feb. 18, 2024, 7:22 p.m. UTC | #3
Hi Milan,

Thank you for the review!

On 15.02.2024 16:30, Milan Zamazal wrote:
> Hans de Goede <hdegoede@redhat.com> writes:
> 
>> From: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>
>>
>> Split the parts which doesn't otherwise depend on the type T or
>> arguments Args out of the SharedMemObject class into a new
>> SharedMem class.
>>
>> Doxygen documentation by Dennis Bonke and Andrei Konovalov.
>>
>> Reviewed-by: Pavel Machek <pavel@ucw.cz>
>> Co-developed-by: Dennis Bonke <admin@dennisbonke.com>
>> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
>> Signed-off-by: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>
>> ---
>>   .../libcamera/internal/shared_mem_object.h    |  92 ++++++---
>>   src/libcamera/meson.build                     |   1 +
>>   src/libcamera/shared_mem_object.cpp           | 191 ++++++++++++++++++
>>   3 files changed, 253 insertions(+), 31 deletions(-)
>>   create mode 100644 src/libcamera/shared_mem_object.cpp
>>
>> diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h
>> index bfb639ee..a4de6500 100644
>> --- a/include/libcamera/internal/shared_mem_object.h
>> +++ b/include/libcamera/internal/shared_mem_object.h
>> @@ -7,11 +7,8 @@
>>   #pragma once
>>   
>>   #include <cstddef>
>> -#include <fcntl.h>
>>   #include <string>
>>   #include <sys/mman.h>
>> -#include <sys/stat.h>
>> -#include <unistd.h>
>>   #include <utility>
>>   
>>   #include <libcamera/base/class.h>
>> @@ -19,6 +16,59 @@
>>   
>>   namespace libcamera {
>>   
>> +class SharedMem
>> +{
>> +public:
>> +	SharedMem()
>> +		: mem_(nullptr)
>> +	{
>> +	}
>> +
>> +	SharedMem(const std::string &name, std::size_t size);
>> +
>> +	SharedMem(SharedMem &&rhs)
>> +	{
>> +		this->name_ = std::move(rhs.name_);
>> +		this->fd_ = std::move(rhs.fd_);
>> +		this->mem_ = rhs.mem_;
>> +		rhs.mem_ = nullptr;
>> +	}
>> +
>> +	~SharedMem()
>> +	{
>> +		if (mem_)
>> +			munmap(mem_, size_);
>> +	}
>> +
>> +	/* Make SharedMem non-copyable for now. */
>> +	LIBCAMERA_DISABLE_COPY(SharedMem)
>> +
>> +	SharedMem &operator=(SharedMem &&rhs)
>> +	{
>> +		this->name_ = std::move(rhs.name_);
>> +		this->fd_ = std::move(rhs.fd_);
>> +		this->mem_ = rhs.mem_;
>> +		rhs.mem_ = nullptr;
>> +		return *this;
>> +	}
>> +
>> +	const SharedFD &fd() const
>> +	{
>> +		return fd_;
>> +	}
>> +
>> +	void *mem() const
>> +	{
>> +		return mem_;
>> +	}
>> +
>> +private:
>> +	std::string name_;
>> +	SharedFD fd_;
>> +	size_t size_;
>> +	void *mem_;
>> +};
>> +
>>   template<class T>
>>   class SharedMemObject
>>   {
>> @@ -32,26 +82,11 @@ public:
>>   
>>   	template<class... Args>
>>   	SharedMemObject(const std::string &name, Args &&...args)
>> -		: name_(name), obj_(nullptr)
>> +		: shMem_(name, SIZE), obj_(nullptr)
>>   	{
>> -		void *mem;
>> -		int ret;
>> +		void *mem = shMem_.mem();
>>   
>> -		ret = memfd_create(name_.c_str(), MFD_CLOEXEC);
>> -		if (ret < 0)
>> -			return;
>> -
>> -		fd_ = SharedFD(std::move(ret));
>> -		if (!fd_.isValid())
>> -			return;
>> -
>> -		ret = ftruncate(fd_.get(), SIZE);
>> -		if (ret < 0)
>> -			return;
>> -
>> -		mem = mmap(nullptr, SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
>> -			   fd_.get(), 0);
>> -		if (mem == MAP_FAILED)
>> +		if (mem == nullptr)
>>   			return;
>>   
>>   		obj_ = new (mem) T(std::forward<Args>(args)...);
>> @@ -59,18 +94,15 @@ public:
>>   
>>   	SharedMemObject(SharedMemObject<T> &&rhs)
>>   	{
>> -		this->name_ = std::move(rhs.name_);
>> -		this->fd_ = std::move(rhs.fd_);
>> +		this->shMem_ = std::move(rhs.shMem_);
>>   		this->obj_ = rhs.obj_;
>>   		rhs.obj_ = nullptr;
>>   	}
>>   
>>   	~SharedMemObject()
>>   	{
>> -		if (obj_) {
>> +		if (obj_)
>>   			obj_->~T();
>> -			munmap(obj_, SIZE);
>> -		}
>>   	}
>>   
>>   	/* Make SharedMemObject non-copyable for now. */
>> @@ -78,8 +110,7 @@ public:
>>   
>>   	SharedMemObject<T> &operator=(SharedMemObject<T> &&rhs)
>>   	{
>> -		this->name_ = std::move(rhs.name_);
>> -		this->fd_ = std::move(rhs.fd_);
>> +		this->shMem_ = std::move(rhs.shMem_);
>>   		this->obj_ = rhs.obj_;
>>   		rhs.obj_ = nullptr;
>>   		return *this;
>> @@ -107,7 +138,7 @@ public:
>>   
>>   	const SharedFD &fd() const
>>   	{
>> -		return fd_;
>> +		return shMem_.fd();
>>   	}
>>   
>>   	explicit operator bool() const
>> @@ -116,8 +147,7 @@ public:
>>   	}
>>   
>>   private:
>> -	std::string name_;
>> -	SharedFD fd_;
>> +	SharedMem shMem_;
>>   	T *obj_;
>>   };
>>   
>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>> index 3c5e43df..94a95ae3 100644
>> --- a/src/libcamera/meson.build
>> +++ b/src/libcamera/meson.build
>> @@ -41,6 +41,7 @@ libcamera_sources = files([
>>       'process.cpp',
>>       'pub_key.cpp',
>>       'request.cpp',
>> +    'shared_mem_object.cpp',
>>       'source_paths.cpp',
>>       'stream.cpp',
>>       'sysfs.cpp',
>> diff --git a/src/libcamera/shared_mem_object.cpp b/src/libcamera/shared_mem_object.cpp
>> new file mode 100644
>> index 00000000..06bbee38
>> --- /dev/null
>> +++ b/src/libcamera/shared_mem_object.cpp
>> @@ -0,0 +1,191 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2023, Raspberry Pi Ltd
>> + *
>> + * shared_mem_object.cpp - Helper class for shared memory allocations
>> + */
>> +
>> +#include "libcamera/internal/shared_mem_object.h"
>> +
>> +#include <sys/types.h>
>> +#include <unistd.h>
>> +
>> +/**
>> + * \file shared_mem_object.cpp
>> + * \brief Helper class for shared memory allocations
>> + */
>> +
>> +namespace libcamera {
>> +
>> +/**
>> + * \class SharedMem
>> + * \brief Helper class for allocating shared memory
>> + *
>> + * Memory is allocated and exposed as a SharedFD for use across IPC boundaries.
>> + *
>> + * SharedMem allocates the shared memory of the given size and maps it.
>> + * To check that the shared memory was allocated and mapped successfully, one
>> + * needs to verify that the pointer to the shared memory returned by SharedMem::mem()
>> + * is not nullptr.
>> + *
>> + * To access the shared memory from another process the SharedFD should be passed
>> + * to that process, and then the shared memory should be mapped into that process
>> + * address space by calling mmap().
>> + *
>> + * A single memfd is created for every SharedMem. If there is a need to allocate
>> + * a large number of objects in shared memory, these objects should be grouped
>> + * together and use the shared memory allocated by a single SharedMem object if
>> + * possible. This will help to minimize the number of created memfd's.
>> + */
>> +
>> +/**
>> + * \fn SharedMem::SharedMem(const std::string &name, std::size_t size)
>> + * \brief Contstructor for the SharedMem
>> + * \param[in] name Name of the SharedMem
>> + * \param[in] size Size of the shared memory to allocate and map
>> + */
>> +
>> +/**
>> + * \fn SharedMem::SharedMem(SharedMem &&rhs)
>> + * \brief Move constructor for SharedMem
>> + * \param[in] rhs The object to move
>> + */
>> +
>> +/**
>> + * \fn SharedMem::~SharedMem()
>> + * \brief SharedMem destructor
>> + *
>> + * Unmaps the allocated shared memory. Decrements the shared memory descriptor use
>> + * count.
>> + */
>> +
>> +/**
>> + * \fn SharedMem &SharedMem::operator=(SharedMem &&rhs)
>> + * \brief Move constructor for SharedMem
>> + * \param[in] rhs The object to move
>> + */
>> +
>> +/**
>> + * \fn const SharedFD &SharedMem::fd() const
>> + * \brief Gets the file descriptor for the underlaying shared memory
>> + * \return The file descriptor
>> + */
>> +
>> +/**
>> + * \fn void *SharedMem::mem() const
>> + * \brief Gets the pointer to the underlaying shared memory
>> + * \return The pointer to the shared memory
>> + */
>> +
>> +SharedMem::SharedMem(const std::string &name, std::size_t size)
>> +	: name_(name), size_(size), mem_(nullptr)
>> +{
>> +	int fd = memfd_create(name_.c_str(), MFD_CLOEXEC);
>> +	if (fd < 0)
>> +		return;
>> +
>> +	fd_ = SharedFD(std::move(fd));
>> +	if (!fd_.isValid())
>> +		return;
>> +
>> +	if (ftruncate(fd_.get(), size_) < 0)
>> +		return;
>> +
>> +	mem_ = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED,
>> +		    fd_.get(), 0);
>> +	if (mem_ == MAP_FAILED)
>> +		mem_ = nullptr;
>> +}
>> +
>> +/**
>> + * \class SharedMemObject
>> + * \brief Helper class for allocating objects in shared memory
>> + *
>> + * Memory is allocated and exposed as a SharedFD for use across IPC boundaries.
>> + *
>> + * Given the type of the object to be created in shared memory and the arguments
>> + * to pass to this object's constructor, SharedMemObject allocates the shared memory
>> + * of the size of the object and constructs the object in this memory. To check that
>> + * the SharedMemObject was created successfully, one needs to verify that the
>> + * underlying SharedFD (the reference to it is returned by SharedMemObject::fd() member
>> + * function) is valid. The object created in the shared memory can be accessed using
>> + * the SharedMemObject::operator*() indirection operator. Its members can be accessed
>> + * with the SharedMemObject::operator->() member of pointer operator.
> 
> But it can still happen that SharedMemObject::fd() is all right but the object
> returned from the operators is nullptr, right?

Right (if the memory is allocated and truncated OK, but mmap fails).

> It would be good to clarify this and refer to the bool operator here.

I will.
I'll also modify the SoftwareIsp constructor to use 'if (!sharedParams_)' instead of
'if (!sharedParams_.fd().isValid())'.

>> + *
>> + * To access the object from another process the SharedFD should be passed to that
>> + * process, and the shared memory should be mapped by calling mmap().
>> + *
>> + * A single memfd is created for every SharedMemObject. If there is a need to allocate
>> + * a large number of objects in shared memory, these objects should be grouped into a
>> + * single large object to keep the number of created memfd's reasonably small.
>> + */
>> +
>> +/**
>> + * \var SharedMemObject::SIZE
>> + * \brief The size of the object that is going to be stored here
>> + */
>> +
>> +/**
>> + * \fn SharedMemObject< T >::SharedMemObject(const std::string &name, Args &&...args)
>> + * \brief Contstructor for the SharedMemObject
>               ^^^^^^^^^^^^
> 
> Constructor

OK, I'll fix these misprints (they are two).

Thanks,
Andrei

>> + * \param[in] name Name of the SharedMemObject
>> + * \param[in] args Args to pass to the constructor of the object in shared memory
>> + */
>> +
>> +/**
>> + * \fn SharedMemObject::SharedMemObject(SharedMemObject<T> &&rhs)
>> + * \brief Move constructor for SharedMemObject
>> + * \param[in] rhs The object to move
>> + */
>> +
>> +/**
>> + * \fn SharedMemObject::~SharedMemObject()
>> + * \brief SharedMemObject destructor
>> + *
>> + * Destroys the object created in the shared memory and then unmaps the shared memory.
>> + * Decrements the shared memory descriptor use count.
>> + */
>> +
>> +/**
>> + * \fn SharedMemObject::operator=(SharedMemObject<T> &&rhs)
>> + * \brief Operator= for SharedMemObject
>> + * \param[in] rhs The SharedMemObject object to take the data from
>> + */
>> +
>> +/**
>> + * \fn SharedMemObject::operator->()
>> + * \brief Operator-> for SharedMemObject
>> + * \return The pointer to the object
>> + */
>> +
>> +/**
>> + * \fn const T *SharedMemObject::operator->() const
>> + * \brief Operator-> for SharedMemObject
>> + * \return The pointer to the const object
>> + */
>> +
>> +/**
>> + * \fn SharedMemObject::operator*()
>> + * \brief Operator* for SharedMemObject
>> + * \return The reference to the object
>> + */
>> +
>> +/**
>> + * \fn const T &SharedMemObject::operator*() const
>> + * \brief Operator* for SharedMemObject
>> + * \return Const reference to the object
>> + */
>> +
>> +/**
>> + * \fn SharedMemObject::fd()
>> + * \brief Gets the file descriptor for the underlaying storage file
>> + * \return The shared memory file descriptor (as SharedFD)
>> + */
>> +
>> +/**
>> + * \fn SharedMemObject::operator bool()
>> + * \brief Operator bool() for SharedMemObject
>> + * \return True if the object was created OK in the shared memory, false otherwise
>> + */
>> +
>> +} // namespace libcamera
>
Andrei Konovalov Feb. 19, 2024, 12:18 p.m. UTC | #4
Hi Naushir,

On 15.02.2024 16:39, Naushir Patuck wrote:
> Hi Hans,
> 
> On Wed, 14 Feb 2024 at 17:01, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> From: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>
>>
>> Split the parts which doesn't otherwise depend on the type T or
>> arguments Args out of the SharedMemObject class into a new
>> SharedMem class.
>>
>> Doxygen documentation by Dennis Bonke and Andrei Konovalov.
>>
>> Reviewed-by: Pavel Machek <pavel@ucw.cz>
>> Co-developed-by: Dennis Bonke <admin@dennisbonke.com>
>> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
>> Signed-off-by: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>
>> ---
>>   .../libcamera/internal/shared_mem_object.h    |  92 ++++++---
>>   src/libcamera/meson.build                     |   1 +
>>   src/libcamera/shared_mem_object.cpp           | 191 ++++++++++++++++++
>>   3 files changed, 253 insertions(+), 31 deletions(-)
>>   create mode 100644 src/libcamera/shared_mem_object.cpp
>>
>> diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h
>> index bfb639ee..a4de6500 100644
>> --- a/include/libcamera/internal/shared_mem_object.h
>> +++ b/include/libcamera/internal/shared_mem_object.h
>> @@ -7,11 +7,8 @@
>>   #pragma once
>>
>>   #include <cstddef>
>> -#include <fcntl.h>
>>   #include <string>
>>   #include <sys/mman.h>
>> -#include <sys/stat.h>
>> -#include <unistd.h>
>>   #include <utility>
>>
>>   #include <libcamera/base/class.h>
>> @@ -19,6 +16,59 @@
>>
>>   namespace libcamera {
>>
>> +class SharedMem
>> +{
>> +public:
>> +       SharedMem()
>> +               : mem_(nullptr)
>> +       {
>> +       }
>> +
>> +       SharedMem(const std::string &name, std::size_t size);
>> +
>> +       SharedMem(SharedMem &&rhs)
>> +       {
>> +               this->name_ = std::move(rhs.name_);
>> +               this->fd_ = std::move(rhs.fd_);
>> +               this->mem_ = rhs.mem_;
>> +               rhs.mem_ = nullptr;
>> +       }
>> +
>> +       ~SharedMem()
>> +       {
>> +               if (mem_)
>> +                       munmap(mem_, size_);
>> +       }
>> +
>> +       /* Make SharedMem non-copyable for now. */
>> +       LIBCAMERA_DISABLE_COPY(SharedMem)
>> +
>> +       SharedMem &operator=(SharedMem &&rhs)
>> +       {
>> +               this->name_ = std::move(rhs.name_);
>> +               this->fd_ = std::move(rhs.fd_);
>> +               this->mem_ = rhs.mem_;
>> +               rhs.mem_ = nullptr;
>> +               return *this;
>> +       }
>> +
>> +       const SharedFD &fd() const
>> +       {
>> +               return fd_;
>> +       }
>> +
>> +       void *mem() const
>> +       {
>> +               return mem_;
>> +       }
>> +
>> +private:
>> +       std::string name_;
>> +       SharedFD fd_;
>> +       size_t size_;
>> +       void *mem_;
>> +};
>> +
>>   template<class T>
>>   class SharedMemObject
> 
> Would it make sense to have SharedMemObject derived from SharedMem?
> Then you would only need a constructor and destructor implemented for
> SharedMemObject, and all other accessors can be through the base
> class... i think?

Does this updated patch (attached) looks OK for you?

Thanks,
Andrei
Andrei Konovalov Feb. 19, 2024, 12:25 p.m. UTC | #5
On 19.02.2024 15:18, Andrei Konovalov wrote:
> Hi Naushir,
> 
> On 15.02.2024 16:39, Naushir Patuck wrote:
>> Hi Hans,
>>
>> On Wed, 14 Feb 2024 at 17:01, Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> From: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>
>>>
>>> Split the parts which doesn't otherwise depend on the type T or
>>> arguments Args out of the SharedMemObject class into a new
>>> SharedMem class.
>>>
>>> Doxygen documentation by Dennis Bonke and Andrei Konovalov.
>>>
>>> Reviewed-by: Pavel Machek <pavel@ucw.cz>
>>> Co-developed-by: Dennis Bonke <admin@dennisbonke.com>
>>> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
>>> Signed-off-by: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>
>>> ---
>>>   .../libcamera/internal/shared_mem_object.h    |  92 ++++++---
>>>   src/libcamera/meson.build                     |   1 +
>>>   src/libcamera/shared_mem_object.cpp           | 191 ++++++++++++++++++
>>>   3 files changed, 253 insertions(+), 31 deletions(-)
>>>   create mode 100644 src/libcamera/shared_mem_object.cpp
>>>
>>> diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h
<snip>
>> Would it make sense to have SharedMemObject derived from SharedMem?
>> Then you would only need a constructor and destructor implemented for
>> SharedMemObject, and all other accessors can be through the base
>> class... i think?
> 
> Does this updated patch (attached) looks OK for you?

Oops.. The attachment wasn't inlined. Resending the patch in an easier to comment on
form.

Thanks,
Andrei


From: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>
Date: Wed, 31 Jan 2024 02:17:34 +0300
Subject: [PATCH 05/16] libcamera: shared_mem_object: reorganize the code and
  document the SharedMemObject class

Split the parts which doesn't otherwise depend on the type T or
arguments Args out of the SharedMemObject class into a new
SharedMem class.

Doxygen documentation by Dennis Bonke and Andrei Konovalov.

Reviewed-by: Pavel Machek <pavel@ucw.cz>
Co-developed-by: Dennis Bonke <admin@dennisbonke.com>
Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
Signed-off-by: Andrei Konovalov <andrey.konovalov.ynk@gmail.com>
---
  .../libcamera/internal/shared_mem_object.h    | 101 ++++++----
  src/libcamera/meson.build                     |   1 +
  src/libcamera/shared_mem_object.cpp           | 189 ++++++++++++++++++
  3 files changed, 252 insertions(+), 39 deletions(-)
  create mode 100644 src/libcamera/shared_mem_object.cpp

diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h
index bfb639ee..bb9cddcd 100644
--- a/include/libcamera/internal/shared_mem_object.h
+++ b/include/libcamera/internal/shared_mem_object.h
@@ -7,11 +7,8 @@
  #pragma once

  #include <cstddef>
-#include <fcntl.h>
  #include <string>
  #include <sys/mman.h>
-#include <sys/stat.h>
-#include <unistd.h>
  #include <utility>

  #include <libcamera/base/class.h>
@@ -19,58 +16,92 @@

  namespace libcamera {

+class SharedMem
+{
+public:
+	SharedMem()
+		: mem_(nullptr)
+	{
+	}
+
+	SharedMem(const std::string &name, std::size_t size);
+
+	SharedMem(SharedMem &&rhs)
+	{
+		this->name_ = std::move(rhs.name_);
+		this->fd_ = std::move(rhs.fd_);
+		this->mem_ = rhs.mem_;
+		rhs.mem_ = nullptr;
+	}
+
+	virtual ~SharedMem()
+	{
+		if (mem_)
+			munmap(mem_, size_);
+	}
+
+	/* Make SharedMem non-copyable for now. */
+	LIBCAMERA_DISABLE_COPY(SharedMem)
+
+	SharedMem &operator=(SharedMem &&rhs)
+	{
+		this->name_ = std::move(rhs.name_);
+		this->fd_ = std::move(rhs.fd_);
+		this->mem_ = rhs.mem_;
+		rhs.mem_ = nullptr;
+		return *this;
+	}
+
+	const SharedFD &fd() const
+	{
+		return fd_;
+	}
+
+	void *mem() const
+	{
+		return mem_;
+	}
+
+private:
+	std::string name_;
+	SharedFD fd_;
+	size_t size_;
+protected:
+	void *mem_;
+};
+
  template<class T>
-class SharedMemObject
+class SharedMemObject : public SharedMem
  {
  public:
  	static constexpr std::size_t SIZE = sizeof(T);

  	SharedMemObject()
-		: obj_(nullptr)
+		: SharedMem(), obj_(nullptr)
  	{
  	}

  	template<class... Args>
  	SharedMemObject(const std::string &name, Args &&...args)
-		: name_(name), obj_(nullptr)
+		: SharedMem(name, SIZE), obj_(nullptr)
  	{
-		void *mem;
-		int ret;
-
-		ret = memfd_create(name_.c_str(), MFD_CLOEXEC);
-		if (ret < 0)
-			return;
-
-		fd_ = SharedFD(std::move(ret));
-		if (!fd_.isValid())
-			return;
-
-		ret = ftruncate(fd_.get(), SIZE);
-		if (ret < 0)
+		if (mem_ == nullptr)
  			return;

-		mem = mmap(nullptr, SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
-			   fd_.get(), 0);
-		if (mem == MAP_FAILED)
-			return;
-
-		obj_ = new (mem) T(std::forward<Args>(args)...);
+		obj_ = new (mem_) T(std::forward<Args>(args)...);
  	}

  	SharedMemObject(SharedMemObject<T> &&rhs)
+		: SharedMem(std::move(rhs))
  	{
-		this->name_ = std::move(rhs.name_);
-		this->fd_ = std::move(rhs.fd_);
  		this->obj_ = rhs.obj_;
  		rhs.obj_ = nullptr;
  	}

  	~SharedMemObject()
  	{
-		if (obj_) {
+		if (obj_)
  			obj_->~T();
-			munmap(obj_, SIZE);
-		}
  	}

  	/* Make SharedMemObject non-copyable for now. */
@@ -78,8 +109,7 @@ public:

  	SharedMemObject<T> &operator=(SharedMemObject<T> &&rhs)
  	{
-		this->name_ = std::move(rhs.name_);
-		this->fd_ = std::move(rhs.fd_);
+		SharedMem::operator=(std::move(rhs));
  		this->obj_ = rhs.obj_;
  		rhs.obj_ = nullptr;
  		return *this;
@@ -105,19 +135,12 @@ public:
  		return *obj_;
  	}

-	const SharedFD &fd() const
-	{
-		return fd_;
-	}
-
  	explicit operator bool() const
  	{
  		return !!obj_;
  	}

  private:
-	std::string name_;
-	SharedFD fd_;
  	T *obj_;
  };

diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
index 3c5e43df..94a95ae3 100644
--- a/src/libcamera/meson.build
+++ b/src/libcamera/meson.build
@@ -41,6 +41,7 @@ libcamera_sources = files([
      'process.cpp',
      'pub_key.cpp',
      'request.cpp',
+    'shared_mem_object.cpp',
      'source_paths.cpp',
      'stream.cpp',
      'sysfs.cpp',
diff --git a/src/libcamera/shared_mem_object.cpp b/src/libcamera/shared_mem_object.cpp
new file mode 100644
index 00000000..dbaa7351
--- /dev/null
+++ b/src/libcamera/shared_mem_object.cpp
@@ -0,0 +1,189 @@
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2023, Raspberry Pi Ltd
+ *
+ * shared_mem_object.cpp - Helper class for shared memory allocations
+ */
+
+#include "libcamera/internal/shared_mem_object.h"
+
+#include <sys/types.h>
+#include <unistd.h>
+
+/**
+ * \file shared_mem_object.cpp
+ * \brief Helper class for shared memory allocations
+ */
+
+namespace libcamera {
+
+/**
+ * \class SharedMem
+ * \brief Helper class for allocating shared memory
+ *
+ * Memory is allocated and exposed as a SharedFD for use across IPC boundaries.
+ *
+ * SharedMem allocates the shared memory of the given size and maps it.
+ * To ensure that the shared memory was allocated and mapped successfully, one
+ * needs to verify that the overloaded bool() operator returns true.
+ *
+ * To access the shared memory from another process the SharedFD should be passed
+ * to that process, and then the shared memory should be mapped into that process
+ * address space by calling mmap().
+ *
+ * A single memfd is created for every SharedMem. If there is a need to allocate
+ * a large number of objects in shared memory, these objects should be grouped
+ * together and use the shared memory allocated by a single SharedMem object if
+ * possible. This will help to minimize the number of created memfd's.
+ */
+
+/**
+ * \fn SharedMem::SharedMem(const std::string &name, std::size_t size)
+ * \brief Constructor for the SharedMem
+ * \param[in] name Name of the SharedMem
+ * \param[in] size Size of the shared memory to allocate and map
+ */
+
+/**
+ * \fn SharedMem::SharedMem(SharedMem &&rhs)
+ * \brief Move constructor for SharedMem
+ * \param[in] rhs The object to move
+ */
+
+/**
+ * \fn SharedMem::~SharedMem()
+ * \brief SharedMem destructor
+ *
+ * Unmaps the allocated shared memory. Decrements the shared memory descriptor use
+ * count.
+ */
+
+/**
+ * \fn SharedMem &SharedMem::operator=(SharedMem &&rhs)
+ * \brief Move constructor for SharedMem
+ * \param[in] rhs The object to move
+ */
+
+/**
+ * \fn const SharedFD &SharedMem::fd() const
+ * \brief Gets the file descriptor for the underlying shared memory
+ * \return The file descriptor
+ */
+
+/**
+ * \fn void *SharedMem::mem() const
+ * \brief Gets the pointer to the underlying shared memory
+ * \return The pointer to the shared memory
+ */
+
+SharedMem::SharedMem(const std::string &name, std::size_t size)
+	: name_(name), size_(size), mem_(nullptr)
+{
+	int fd = memfd_create(name_.c_str(), MFD_CLOEXEC);
+	if (fd < 0)
+		return;
+
+	fd_ = SharedFD(std::move(fd));
+	if (!fd_.isValid())
+		return;
+
+	if (ftruncate(fd_.get(), size_) < 0)
+		return;
+
+	mem_ = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED,
+		    fd_.get(), 0);
+	if (mem_ == MAP_FAILED)
+		mem_ = nullptr;
+}
+
+/**
+ * \var SharedMem::mem_
+ * \brief Pointer to the shared memory allocated
+ */
+
+/**
+ * \class SharedMemObject
+ * \brief Helper class for allocating objects in shared memory
+ *
+ * Memory is allocated and exposed as a SharedFD for use across IPC boundaries.
+ *
+ * Given the type of the object to be created in shared memory and the arguments
+ * to pass to this object's constructor, SharedMemObject allocates the shared memory
+ * of the size of the object and constructs the object in this memory. To check that
+ * the SharedMemObject was created successfully, one needs to verify that the
+ * underlying SharedFD (the reference to it is returned by SharedMemObject::fd() member
+ * function) is valid. The object created in the shared memory can be accessed using
+ * the SharedMemObject::operator*() indirection operator. Its members can be accessed
+ * with the SharedMemObject::operator->() member of pointer operator.
+ *
+ * To access the object from another process the SharedFD should be passed to that
+ * process, and the shared memory should be mapped by calling mmap().
+ *
+ * A single memfd is created for every SharedMemObject. If there is a need to allocate
+ * a large number of objects in shared memory, these objects should be grouped into a
+ * single large object to keep the number of created memfd's reasonably small.
+ */
+
+/**
+ * \var SharedMemObject::SIZE
+ * \brief The size of the object that is going to be stored here
+ */
+
+/**
+ * \fn SharedMemObject< T >::SharedMemObject(const std::string &name, Args &&...args)
+ * \brief Constructor for the SharedMemObject
+ * \param[in] name Name of the SharedMemObject
+ * \param[in] args Args to pass to the constructor of the object in shared memory
+ */
+
+/**
+ * \fn SharedMemObject::SharedMemObject(SharedMemObject<T> &&rhs)
+ * \brief Move constructor for SharedMemObject
+ * \param[in] rhs The object to move
+ */
+
+/**
+ * \fn SharedMemObject::~SharedMemObject()
+ * \brief SharedMemObject destructor
+ *
+ * Destroys the object created in the shared memory and then unmaps the shared memory.
+ * Decrements the shared memory descriptor use count.
+ */
+
+/**
+ * \fn SharedMemObject::operator=(SharedMemObject<T> &&rhs)
+ * \brief Operator= for SharedMemObject
+ * \param[in] rhs The SharedMemObject object to take the data from
+ */
+
+/**
+ * \fn SharedMemObject::operator->()
+ * \brief Operator-> for SharedMemObject
+ * \return The pointer to the object
+ */
+
+/**
+ * \fn const T *SharedMemObject::operator->() const
+ * \brief Operator-> for SharedMemObject
+ * \return The pointer to the const object
+ */
+
+/**
+ * \fn SharedMemObject::operator*()
+ * \brief Operator* for SharedMemObject
+ * \return The reference to the object
+ */
+
+/**
+ * \fn const T &SharedMemObject::operator*() const
+ * \brief Operator* for SharedMemObject
+ * \return Const reference to the object
+ */
+
+/**
+ * \fn SharedMemObject::operator bool()
+ * \brief Operator bool() for SharedMemObject
+ * \return True if the object was created OK in the shared memory, false otherwise
+ */
+
+} // namespace libcamera

Patch
diff mbox series

diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h
index bfb639ee..a4de6500 100644
--- a/include/libcamera/internal/shared_mem_object.h
+++ b/include/libcamera/internal/shared_mem_object.h
@@ -7,11 +7,8 @@ 
 #pragma once
 
 #include <cstddef>
-#include <fcntl.h>
 #include <string>
 #include <sys/mman.h>
-#include <sys/stat.h>
-#include <unistd.h>
 #include <utility>
 
 #include <libcamera/base/class.h>
@@ -19,6 +16,59 @@ 
 
 namespace libcamera {
 
+class SharedMem
+{
+public:
+	SharedMem()
+		: mem_(nullptr)
+	{
+	}
+
+	SharedMem(const std::string &name, std::size_t size);
+
+	SharedMem(SharedMem &&rhs)
+	{
+		this->name_ = std::move(rhs.name_);
+		this->fd_ = std::move(rhs.fd_);
+		this->mem_ = rhs.mem_;
+		rhs.mem_ = nullptr;
+	}
+
+	~SharedMem()
+	{
+		if (mem_)
+			munmap(mem_, size_);
+	}
+
+	/* Make SharedMem non-copyable for now. */
+	LIBCAMERA_DISABLE_COPY(SharedMem)
+
+	SharedMem &operator=(SharedMem &&rhs)
+	{
+		this->name_ = std::move(rhs.name_);
+		this->fd_ = std::move(rhs.fd_);
+		this->mem_ = rhs.mem_;
+		rhs.mem_ = nullptr;
+		return *this;
+	}
+
+	const SharedFD &fd() const
+	{
+		return fd_;
+	}
+
+	void *mem() const
+	{
+		return mem_;
+	}
+
+private:
+	std::string name_;
+	SharedFD fd_;
+	size_t size_;
+	void *mem_;
+};
+
 template<class T>
 class SharedMemObject
 {
@@ -32,26 +82,11 @@  public:
 
 	template<class... Args>
 	SharedMemObject(const std::string &name, Args &&...args)
-		: name_(name), obj_(nullptr)
+		: shMem_(name, SIZE), obj_(nullptr)
 	{
-		void *mem;
-		int ret;
+		void *mem = shMem_.mem();
 
-		ret = memfd_create(name_.c_str(), MFD_CLOEXEC);
-		if (ret < 0)
-			return;
-
-		fd_ = SharedFD(std::move(ret));
-		if (!fd_.isValid())
-			return;
-
-		ret = ftruncate(fd_.get(), SIZE);
-		if (ret < 0)
-			return;
-
-		mem = mmap(nullptr, SIZE, PROT_READ | PROT_WRITE, MAP_SHARED,
-			   fd_.get(), 0);
-		if (mem == MAP_FAILED)
+		if (mem == nullptr)
 			return;
 
 		obj_ = new (mem) T(std::forward<Args>(args)...);
@@ -59,18 +94,15 @@  public:
 
 	SharedMemObject(SharedMemObject<T> &&rhs)
 	{
-		this->name_ = std::move(rhs.name_);
-		this->fd_ = std::move(rhs.fd_);
+		this->shMem_ = std::move(rhs.shMem_);
 		this->obj_ = rhs.obj_;
 		rhs.obj_ = nullptr;
 	}
 
 	~SharedMemObject()
 	{
-		if (obj_) {
+		if (obj_)
 			obj_->~T();
-			munmap(obj_, SIZE);
-		}
 	}
 
 	/* Make SharedMemObject non-copyable for now. */
@@ -78,8 +110,7 @@  public:
 
 	SharedMemObject<T> &operator=(SharedMemObject<T> &&rhs)
 	{
-		this->name_ = std::move(rhs.name_);
-		this->fd_ = std::move(rhs.fd_);
+		this->shMem_ = std::move(rhs.shMem_);
 		this->obj_ = rhs.obj_;
 		rhs.obj_ = nullptr;
 		return *this;
@@ -107,7 +138,7 @@  public:
 
 	const SharedFD &fd() const
 	{
-		return fd_;
+		return shMem_.fd();
 	}
 
 	explicit operator bool() const
@@ -116,8 +147,7 @@  public:
 	}
 
 private:
-	std::string name_;
-	SharedFD fd_;
+	SharedMem shMem_;
 	T *obj_;
 };
 
diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
index 3c5e43df..94a95ae3 100644
--- a/src/libcamera/meson.build
+++ b/src/libcamera/meson.build
@@ -41,6 +41,7 @@  libcamera_sources = files([
     'process.cpp',
     'pub_key.cpp',
     'request.cpp',
+    'shared_mem_object.cpp',
     'source_paths.cpp',
     'stream.cpp',
     'sysfs.cpp',
diff --git a/src/libcamera/shared_mem_object.cpp b/src/libcamera/shared_mem_object.cpp
new file mode 100644
index 00000000..06bbee38
--- /dev/null
+++ b/src/libcamera/shared_mem_object.cpp
@@ -0,0 +1,191 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2023, Raspberry Pi Ltd
+ *
+ * shared_mem_object.cpp - Helper class for shared memory allocations
+ */
+
+#include "libcamera/internal/shared_mem_object.h"
+
+#include <sys/types.h>
+#include <unistd.h>
+
+/**
+ * \file shared_mem_object.cpp
+ * \brief Helper class for shared memory allocations
+ */
+
+namespace libcamera {
+
+/**
+ * \class SharedMem
+ * \brief Helper class for allocating shared memory
+ *
+ * Memory is allocated and exposed as a SharedFD for use across IPC boundaries.
+ *
+ * SharedMem allocates the shared memory of the given size and maps it.
+ * To check that the shared memory was allocated and mapped successfully, one
+ * needs to verify that the pointer to the shared memory returned by SharedMem::mem()
+ * is not nullptr.
+ *
+ * To access the shared memory from another process the SharedFD should be passed
+ * to that process, and then the shared memory should be mapped into that process
+ * address space by calling mmap().
+ *
+ * A single memfd is created for every SharedMem. If there is a need to allocate
+ * a large number of objects in shared memory, these objects should be grouped
+ * together and use the shared memory allocated by a single SharedMem object if
+ * possible. This will help to minimize the number of created memfd's.
+ */
+
+/**
+ * \fn SharedMem::SharedMem(const std::string &name, std::size_t size)
+ * \brief Contstructor for the SharedMem
+ * \param[in] name Name of the SharedMem
+ * \param[in] size Size of the shared memory to allocate and map
+ */
+
+/**
+ * \fn SharedMem::SharedMem(SharedMem &&rhs)
+ * \brief Move constructor for SharedMem
+ * \param[in] rhs The object to move
+ */
+
+/**
+ * \fn SharedMem::~SharedMem()
+ * \brief SharedMem destructor
+ *
+ * Unmaps the allocated shared memory. Decrements the shared memory descriptor use
+ * count.
+ */
+
+/**
+ * \fn SharedMem &SharedMem::operator=(SharedMem &&rhs)
+ * \brief Move constructor for SharedMem
+ * \param[in] rhs The object to move
+ */
+
+/**
+ * \fn const SharedFD &SharedMem::fd() const
+ * \brief Gets the file descriptor for the underlaying shared memory
+ * \return The file descriptor
+ */
+
+/**
+ * \fn void *SharedMem::mem() const
+ * \brief Gets the pointer to the underlaying shared memory
+ * \return The pointer to the shared memory
+ */
+
+SharedMem::SharedMem(const std::string &name, std::size_t size)
+	: name_(name), size_(size), mem_(nullptr)
+{
+	int fd = memfd_create(name_.c_str(), MFD_CLOEXEC);
+	if (fd < 0)
+		return;
+
+	fd_ = SharedFD(std::move(fd));
+	if (!fd_.isValid())
+		return;
+
+	if (ftruncate(fd_.get(), size_) < 0)
+		return;
+
+	mem_ = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED,
+		    fd_.get(), 0);
+	if (mem_ == MAP_FAILED)
+		mem_ = nullptr;
+}
+
+/**
+ * \class SharedMemObject
+ * \brief Helper class for allocating objects in shared memory
+ *
+ * Memory is allocated and exposed as a SharedFD for use across IPC boundaries.
+ *
+ * Given the type of the object to be created in shared memory and the arguments
+ * to pass to this object's constructor, SharedMemObject allocates the shared memory
+ * of the size of the object and constructs the object in this memory. To check that
+ * the SharedMemObject was created successfully, one needs to verify that the
+ * underlying SharedFD (the reference to it is returned by SharedMemObject::fd() member
+ * function) is valid. The object created in the shared memory can be accessed using
+ * the SharedMemObject::operator*() indirection operator. Its members can be accessed
+ * with the SharedMemObject::operator->() member of pointer operator.
+ *
+ * To access the object from another process the SharedFD should be passed to that
+ * process, and the shared memory should be mapped by calling mmap().
+ *
+ * A single memfd is created for every SharedMemObject. If there is a need to allocate
+ * a large number of objects in shared memory, these objects should be grouped into a
+ * single large object to keep the number of created memfd's reasonably small.
+ */
+
+/**
+ * \var SharedMemObject::SIZE
+ * \brief The size of the object that is going to be stored here
+ */
+
+/**
+ * \fn SharedMemObject< T >::SharedMemObject(const std::string &name, Args &&...args)
+ * \brief Contstructor for the SharedMemObject
+ * \param[in] name Name of the SharedMemObject
+ * \param[in] args Args to pass to the constructor of the object in shared memory
+ */
+
+/**
+ * \fn SharedMemObject::SharedMemObject(SharedMemObject<T> &&rhs)
+ * \brief Move constructor for SharedMemObject
+ * \param[in] rhs The object to move
+ */
+
+/**
+ * \fn SharedMemObject::~SharedMemObject()
+ * \brief SharedMemObject destructor
+ *
+ * Destroys the object created in the shared memory and then unmaps the shared memory.
+ * Decrements the shared memory descriptor use count.
+ */
+
+/**
+ * \fn SharedMemObject::operator=(SharedMemObject<T> &&rhs)
+ * \brief Operator= for SharedMemObject
+ * \param[in] rhs The SharedMemObject object to take the data from
+ */
+
+/**
+ * \fn SharedMemObject::operator->()
+ * \brief Operator-> for SharedMemObject
+ * \return The pointer to the object
+ */
+
+/**
+ * \fn const T *SharedMemObject::operator->() const
+ * \brief Operator-> for SharedMemObject
+ * \return The pointer to the const object
+ */
+
+/**
+ * \fn SharedMemObject::operator*()
+ * \brief Operator* for SharedMemObject
+ * \return The reference to the object
+ */
+
+/**
+ * \fn const T &SharedMemObject::operator*() const
+ * \brief Operator* for SharedMemObject
+ * \return Const reference to the object
+ */
+
+/**
+ * \fn SharedMemObject::fd()
+ * \brief Gets the file descriptor for the underlaying storage file
+ * \return The shared memory file descriptor (as SharedFD)
+ */
+
+/**
+ * \fn SharedMemObject::operator bool()
+ * \brief Operator bool() for SharedMemObject
+ * \return True if the object was created OK in the shared memory, false otherwise
+ */
+
+} // namespace libcamera