[libcamera-devel] [PATCH v2 05/18] libcamera: internal: Document the SharedMemObject class
Naushir Patuck
naush at raspberrypi.com
Wed Jan 24 12:07:35 CET 2024
Hi all,
On Tue, 23 Jan 2024 at 14:43, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> 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...?
Regards,
Naush
>
> > > > + */
> > > > 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240124/3d62d944/attachment.htm>
More information about the libcamera-devel
mailing list