[libcamera-devel] [PATCH v2 05/18] libcamera: internal: Document the SharedMemObject class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Feb 5 15:16:14 CET 2024
Hi Naush,
On Wed, Jan 24, 2024 at 11:07:35AM +0000, Naushir Patuck wrote:
> On Tue, 23 Jan 2024 at 14:43, Laurent Pinchart wrote:
> > On Tue, Jan 23, 2024 at 03:34:53PM +0200, Laurent Pinchart wrote:
> > > On Sat, Jan 13, 2024 at 10:45:14PM +0000, -dev wrote:
> > > > Quoting Hans de Goede via libcamera-devel (2024-01-13 14:22:05)
> > > > > From: Dennis Bonke <admin at dennisbonke.com>
> > > > >
> > > > > Document the SharedMemObject class.
> > > > >
> > > > > Signed-off-by: Dennis Bonke <admin at dennisbonke.com>
> > > > > Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> > > > > Tested-by: Bryan O'Donoghue <bryan.odonoghue at linaro.org> # sc8280xp Lenovo x13s
> > > > > Tested-by: Pavel Machek <pavel at ucw.cz>
> > > > > ---
> > > > > .../libcamera/internal/shared_mem_object.h | 53 +++++++++++++++++++
> > > > > 1 file changed, 53 insertions(+)
> > > > >
> > > > > diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h
> > > > > index bfb639ee..e862ce48 100644
> > > > > --- a/include/libcamera/internal/shared_mem_object.h
> > > > > +++ b/include/libcamera/internal/shared_mem_object.h
> > > > > @@ -19,10 +19,20 @@
> > > > >
> > > > > namespace libcamera {
> > > > >
> > > > > +/**
> > > > > + * \class SharedMemObject
> > > > > + * \brief Helper class for shared memory allocations.
> > > > > + *
> > > > > + * Takes a template T which is used to indicate the
> > > > > + * data type of the object stored.
> > > >
> > > > I'd wrap this to the usual 80 chars. Not critical though.
> > >
> > > Not critical, but no reason not to do so :-)
> > >
> > > > It might be nice to explain here what the class is doing, as the code
> > > > may not be visible to the reader.
> > > >
> > > > The only part I'd add is perhaps something like:
> > > >
> > > > """
> > > > Memory is allocated and exposed as a SharedFD for use across IPC
> > > > boundaries.
> > > > """
> > >
> > > I would like more documentation too, with an explanation of how this is
> > > supposed to be used.
> > >
> > > > But either way,
> > > >
> > > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > >
> > > Documentation goes to .cpp files. The documentation should also come in
> > > the same patch as the one that adds the class.
> >
> > Just to be clear, as there's no .cpp file yet, I think most of the
> > SharedMemObject constructor should go to a separate init(size_t size)
> > function moved to a .cpp file, as it doesn't otherwise depend on the
> > type T or arguments Args.
> >
> > The way we create a single memfd for every SharedMemObject isn't very
> > nice, as it could cause lots of fd allocations if callers are not aware
> > that they should create a single large object instead. This should be at
> > the very least clearly explained in the documentation. Ideally the
> > class should be refactored to make it possible to allocate multiple
> > objects within a single shared memory segment, but that's out of scope
> > for this series.
> >
> > As for squashing documentation with the patch that introduces the class,
> > I hadn't realized that the class had simply be moved from the rpi code.
> > I'm OK keeping the two patches separate.
>
> Admittedly I've not been following this series, so apologies if I get some of
> the details below wrong.
>
> The RPi Pi implementation of SharedMemObject is exactly what we want as per
> Laurent's description above, i.e. a single FD per object instance allocated and
> constructed through "placement new". This is why the SharedMemObject
> constructor takes the var-args list for the wrapped object constructor. There
> are only ever 2 SharedMemObject instances (backend and frontend) at runtime
> shared between the IPA and pipeline handler.
>
> I agree with Laurent that this same mechanism may not be entirely appropriate
> if there are a large number of objects being allocated in shared memory,
> resulting in a large number of FDs. In such cases, perhaps SharedMemObject()
> is not the right tool, and the underlying memfd allocations may need to be
> wrapped differently. Or maybe SharedMemObject can be used as-is, but the
> object it allocates is a superset of many smaller sub-objects...?
I agree with all that. We may rework the implementation, but from a
Raspberry Pi point of view, it will have to stay two objects with one fd
each, with an easy to use API.
> > > > > + */
> > > > > template<class T>
> > > > > class SharedMemObject
> > > > > {
> > > > > public:
> > > > > + /**
> > > > > + * \brief The size of the object that is going to be stored here.
> > > > > + */
> > > > > static constexpr std::size_t SIZE = sizeof(T);
> >
> > We don't use all uppercase constants. You can name this kSize or size.
> >
> > > > >
> > > > > SharedMemObject()
> > > > > @@ -30,6 +40,11 @@ public:
> > > > > {
> > > > > }
> > > > >
> > > > > + /**
> > > > > + * \brief Contstructor for the SharedMemObject.
> > > > > + * \param[in] name The requested name.
> > > > > + * \param[in] args Any additional args.
> > > > > + */
> >
> > You need to expand the documentation. Consider the point of view of a
> > reader who hasn't seen the code. They can't tell from the documentation
> > what the name and args are for.
> >
> > Also, I'd like unit tests.
> >
> > > > > template<class... Args>
> > > > > SharedMemObject(const std::string &name, Args &&...args)
> > > > > : name_(name), obj_(nullptr)
> > > > > @@ -57,6 +72,10 @@ public:
> > > > > obj_ = new (mem) T(std::forward<Args>(args)...);
> > > > > }
> > > > >
> > > > > + /**
> > > > > + * \brief Move constructor for SharedMemObject.
> > > > > + * \param[in] rhs The object to move.
> > > > > + */
> > > > > SharedMemObject(SharedMemObject<T> &&rhs)
> > > > > {
> > > > > this->name_ = std::move(rhs.name_);
> > > > > @@ -76,6 +95,10 @@ public:
> > > > > /* Make SharedMemObject non-copyable for now. */
> > > > > LIBCAMERA_DISABLE_COPY(SharedMemObject)
> > > > >
> > > > > + /**
> > > > > + * \brief Operator= for SharedMemObject.
> > > > > + * \param[in] rhs The SharedMemObject object to take the data from.
> > > > > + */
> > > > > SharedMemObject<T> &operator=(SharedMemObject<T> &&rhs)
> > > > > {
> > > > > this->name_ = std::move(rhs.name_);
> > > > > @@ -85,31 +108,61 @@ public:
> > > > > return *this;
> > > > > }
> > > > >
> > > > > + /**
> > > > > + * \brief Operator-> for SharedMemObject.
> > > > > + *
> > > > > + * \return the object.
> > > > > + */
> > > > > T *operator->()
> > > > > {
> > > > > return obj_;
> > > > > }
> > > > >
> > > > > + /**
> > > > > + * \brief Operator-> for SharedMemObject.
> > > > > + *
> > > > > + * \return the object.
> > > > > + */
> > > > > const T *operator->() const
> > > > > {
> > > > > return obj_;
> > > > > }
> > > > >
> > > > > + /**
> > > > > + * \brief Operator* for SharedMemObject.
> > > > > + *
> > > > > + * \return the object.
> > > > > + */
> > > > > T &operator*()
> > > > > {
> > > > > return *obj_;
> > > > > }
> > > > >
> > > > > + /**
> > > > > + * \brief Operator* for SharedMemObject.
> > > > > + *
> > > > > + * \return the object.
> > > > > + */
> > > > > const T &operator*() const
> > > > > {
> > > > > return *obj_;
> > > > > }
> > > > >
> > > > > + /**
> > > > > + * \brief Gets the file descriptor for the underlaying storage file.
> > > > > + *
> > > > > + * \return the file descriptor.
> > > > > + */
> > > > > const SharedFD &fd() const
> > > > > {
> > > > > return fd_;
> > > > > }
> > > > >
> > > > > + /**
> > > > > + * \brief Operator bool() for SharedMemObject.
> > > > > + *
> > > > > + * \return true if the object is not null, false otherwise.
> > > > > + */
> > > > > explicit operator bool() const
> > > > > {
> > > > > return !!obj_;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list