[libcamera-devel] [PATCH 03/20] pipeline: rpi: Add SharedMemObject class
Naushir Patuck
naush at raspberrypi.com
Thu Oct 12 10:25:13 CEST 2023
Hi Jacopo,
Thank you for the feedback.
On Thu, 12 Oct 2023 at 09:08, Jacopo Mondi
<jacopo.mondi at 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 at raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman at 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 at ideasonboard.com>
Thanks!
I'll post an update once I've collected all the feedback.
Regards,
Naush
>
> > +
> > +} /* namespace RPi */
> > +
> > +} /* namespace libcamera */
> > --
> > 2.34.1
> >
More information about the libcamera-devel
mailing list