[libcamera-devel,03/20] pipeline: rpi: Add SharedMemObject class
diff mbox series

Message ID 20231006132000.23504-4-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi: Preliminary PiSP support
Related show

Commit Message

Naushir Patuck Oct. 6, 2023, 1:19 p.m. UTC 
Add new SharedMemObject class that wraps a memfd memory allocation and
constructs a templated object in the memory. With appropriate locking,
this object can then be shared across different processes using the
associated allocation file handle.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
---
 .../pipeline/rpi/common/shared_mem_object.h   | 118 ++++++++++++++++++
 1 file changed, 118 insertions(+)
 create mode 100644 src/libcamera/pipeline/rpi/common/shared_mem_object.h

Comments

Jacopo Mondi Oct. 12, 2023, 8:08 a.m. UTC | #1 
Cc-ing Andrey, Bryan and Hans for the SoftISP work

On Fri, Oct 06, 2023 at 02:19:43PM +0100, Naushir Patuck via libcamera-devel wrote:
> Add new SharedMemObject class that wraps a memfd memory allocation and
> constructs a templated object in the memory. With appropriate locking,
> this object can then be shared across different processes using the
> associated allocation file handle.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  .../pipeline/rpi/common/shared_mem_object.h   | 118 ++++++++++++++++++
>  1 file changed, 118 insertions(+)
>  create mode 100644 src/libcamera/pipeline/rpi/common/shared_mem_object.h
>
> diff --git a/src/libcamera/pipeline/rpi/common/shared_mem_object.h b/src/libcamera/pipeline/rpi/common/shared_mem_object.h
> new file mode 100644
> index 000000000000..69051ecc3f52
> --- /dev/null
> +++ b/src/libcamera/pipeline/rpi/common/shared_mem_object.h
> @@ -0,0 +1,118 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Raspberry Pi Ltd
> + *
> + * shared_mem_object.h - Helper class for shared memory allocations
> + */
> +#pragma once
> +
> +#include <fcntl.h>
> +#include <string>
> +#include <sys/mman.h>
> +#include <sys/stat.h>
> +#include <unistd.h>

#include <utility> for std::forward

> +
> +#include <libcamera/base/class.h>
> +#include <libcamera/base/shared_fd.h>
> +
> +namespace libcamera {
> +
> +namespace RPi {
> +
> +template<class T>
> +class SharedMemObject
> +{
> +public:
> +	static constexpr std::size_t SIZE = sizeof(T);

and <cstddef> for std::size_t ?

> +
> +	SharedMemObject()
> +		: obj_(nullptr)
> +	{
> +	}
> +
> +	template<class... Args>
> +	SharedMemObject(const std::string &name, Args &&...args)
> +		: name_(name), 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)
> +			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)...);
> +	}
> +
> +	~SharedMemObject()
> +	{
> +		if (obj_) {
> +			obj_->~T();
> +			munmap(obj_, SIZE);
> +		}
> +	}
> +
> +	/* Make SharedMemObject non-copyable for now. */
> +	LIBCAMERA_DISABLE_COPY(SharedMemObject)
> +
> +	SharedMemObject<T> &operator=(SharedMemObject<T> &&rhs)

Do you need a move constructor as well, since you have a move
assignment operator ?

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


> +		this->name_ = std::move(rhs.name_);
> +		this->fd_ = std::move(rhs.fd_);
> +		this->obj_ = rhs.obj_;
> +		rhs.obj_ = nullptr;
> +		return *this;
> +	}
> +
> +	T *operator->()
> +	{
> +		return obj_;
> +	}
> +
> +	const T *operator->() const
> +	{
> +		return obj_;
> +	}
> +
> +	T &operator*()
> +	{
> +		return *obj_;
> +	}
> +
> +	const T &operator*() const
> +	{
> +		return *obj_;
> +	}
> +
> +	const SharedFD &fd() const
> +	{
> +		return fd_;
> +	}
> +
> +	explicit operator bool() const
> +	{
> +		return !!obj_;
> +	}
> +
> +private:
> +	std::string name_;
> +	SharedFD fd_;
> +	T *obj_;
> +};

The rest looks good.. making this a library component on top shouldn't be
hard..

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> +
> +} /* namespace RPi */
> +
> +} /* namespace libcamera */
> --
> 2.34.1
>
Naushir Patuck Oct. 12, 2023, 8:25 a.m. UTC | #2 
Hi Jacopo,

Thank you for the feedback.

On Thu, 12 Oct 2023 at 09:08, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Cc-ing Andrey, Bryan and Hans for the SoftISP work
>
> On Fri, Oct 06, 2023 at 02:19:43PM +0100, Naushir Patuck via libcamera-devel wrote:
> > Add new SharedMemObject class that wraps a memfd memory allocation and
> > constructs a templated object in the memory. With appropriate locking,
> > this object can then be shared across different processes using the
> > associated allocation file handle.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  .../pipeline/rpi/common/shared_mem_object.h   | 118 ++++++++++++++++++
> >  1 file changed, 118 insertions(+)
> >  create mode 100644 src/libcamera/pipeline/rpi/common/shared_mem_object.h
> >
> > diff --git a/src/libcamera/pipeline/rpi/common/shared_mem_object.h b/src/libcamera/pipeline/rpi/common/shared_mem_object.h
> > new file mode 100644
> > index 000000000000..69051ecc3f52
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/rpi/common/shared_mem_object.h
> > @@ -0,0 +1,118 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2023, Raspberry Pi Ltd
> > + *
> > + * shared_mem_object.h - Helper class for shared memory allocations
> > + */
> > +#pragma once
> > +
> > +#include <fcntl.h>
> > +#include <string>
> > +#include <sys/mman.h>
> > +#include <sys/stat.h>
> > +#include <unistd.h>
>
> #include <utility> for std::forward
>
> > +
> > +#include <libcamera/base/class.h>
> > +#include <libcamera/base/shared_fd.h>
> > +
> > +namespace libcamera {
> > +
> > +namespace RPi {
> > +
> > +template<class T>
> > +class SharedMemObject
> > +{
> > +public:
> > +     static constexpr std::size_t SIZE = sizeof(T);
>
> and <cstddef> for std::size_t ?

Ack on both.

>
> > +
> > +     SharedMemObject()
> > +             : obj_(nullptr)
> > +     {
> > +     }
> > +
> > +     template<class... Args>
> > +     SharedMemObject(const std::string &name, Args &&...args)
> > +             : name_(name), 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)
> > +                     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)...);
> > +     }
> > +
> > +     ~SharedMemObject()
> > +     {
> > +             if (obj_) {
> > +                     obj_->~T();
> > +                     munmap(obj_, SIZE);
> > +             }
> > +     }
> > +
> > +     /* Make SharedMemObject non-copyable for now. */
> > +     LIBCAMERA_DISABLE_COPY(SharedMemObject)
> > +
> > +     SharedMemObject<T> &operator=(SharedMemObject<T> &&rhs)
>
> Do you need a move constructor as well, since you have a move
> assignment operator ?
>
>         SharedMemObject(SharedMemObject<T> &&rhs)
>         {
>                 this->name_ = std::move(rhs.name_);
>                 this->fd_ = std::move(rhs.fd_);
>                 this->obj_ = rhs.obj_;
>                 rhs.obj_ = nullptr;
>         }

Yes, it seems reasonable to add this.

>
>
> > +             this->name_ = std::move(rhs.name_);
> > +             this->fd_ = std::move(rhs.fd_);
> > +             this->obj_ = rhs.obj_;
> > +             rhs.obj_ = nullptr;
> > +             return *this;
> > +     }
> > +
> > +     T *operator->()
> > +     {
> > +             return obj_;
> > +     }
> > +
> > +     const T *operator->() const
> > +     {
> > +             return obj_;
> > +     }
> > +
> > +     T &operator*()
> > +     {
> > +             return *obj_;
> > +     }
> > +
> > +     const T &operator*() const
> > +     {
> > +             return *obj_;
> > +     }
> > +
> > +     const SharedFD &fd() const
> > +     {
> > +             return fd_;
> > +     }
> > +
> > +     explicit operator bool() const
> > +     {
> > +             return !!obj_;
> > +     }
> > +
> > +private:
> > +     std::string name_;
> > +     SharedFD fd_;
> > +     T *obj_;
> > +};
>
> The rest looks good.. making this a library component on top shouldn't be
> hard..
>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks!
I'll post an update once I've collected all the feedback.

Regards,
Naush

>
> > +
> > +} /* namespace RPi */
> > +
> > +} /* namespace libcamera */
> > --
> > 2.34.1
> >

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rpi/common/shared_mem_object.h b/src/libcamera/pipeline/rpi/common/shared_mem_object.h
new file mode 100644
index 000000000000..69051ecc3f52
--- /dev/null
+++ b/src/libcamera/pipeline/rpi/common/shared_mem_object.h
@@ -0,0 +1,118 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2023, Raspberry Pi Ltd
+ *
+ * shared_mem_object.h - Helper class for shared memory allocations
+ */
+#pragma once
+
+#include <fcntl.h>
+#include <string>
+#include <sys/mman.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+#include <libcamera/base/class.h>
+#include <libcamera/base/shared_fd.h>
+
+namespace libcamera {
+
+namespace RPi {
+
+template<class T>
+class SharedMemObject
+{
+public:
+	static constexpr std::size_t SIZE = sizeof(T);
+
+	SharedMemObject()
+		: obj_(nullptr)
+	{
+	}
+
+	template<class... Args>
+	SharedMemObject(const std::string &name, Args &&...args)
+		: name_(name), 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)
+			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)...);
+	}
+
+	~SharedMemObject()
+	{
+		if (obj_) {
+			obj_->~T();
+			munmap(obj_, SIZE);
+		}
+	}
+
+	/* Make SharedMemObject non-copyable for now. */
+	LIBCAMERA_DISABLE_COPY(SharedMemObject)
+
+	SharedMemObject<T> &operator=(SharedMemObject<T> &&rhs)
+	{
+		this->name_ = std::move(rhs.name_);
+		this->fd_ = std::move(rhs.fd_);
+		this->obj_ = rhs.obj_;
+		rhs.obj_ = nullptr;
+		return *this;
+	}
+
+	T *operator->()
+	{
+		return obj_;
+	}
+
+	const T *operator->() const
+	{
+		return obj_;
+	}
+
+	T &operator*()
+	{
+		return *obj_;
+	}
+
+	const T &operator*() const
+	{
+		return *obj_;
+	}
+
+	const SharedFD &fd() const
+	{
+		return fd_;
+	}
+
+	explicit operator bool() const
+	{
+		return !!obj_;
+	}
+
+private:
+	std::string name_;
+	SharedFD fd_;
+	T *obj_;
+};
+
+} /* namespace RPi */
+
+} /* namespace libcamera */