[libcamera-devel,v2,05/18] libcamera: internal: Document the SharedMemObject class
diff mbox series

Message ID 20240113142218.28063-6-hdegoede@redhat.com
State Superseded
Headers show
Series
  • [libcamera-devel,v2,01/18] libcamera: pipeline: simple: fix size adjustment in validate()
Related show

Commit Message

Hans de Goede Jan. 13, 2024, 2:22 p.m. UTC
From: Dennis Bonke <admin@dennisbonke.com>

Document the SharedMemObject class.

Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
Tested-by: Pavel Machek <pavel@ucw.cz>
---
 .../libcamera/internal/shared_mem_object.h    | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)

Comments

Kieran Bingham Jan. 13, 2024, 10:45 p.m. UTC | #1
Quoting Hans de Goede via libcamera-devel (2024-01-13 14:22:05)
> From: Dennis Bonke <admin@dennisbonke.com>
> 
> Document the SharedMemObject class.
> 
> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel@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.

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.
"""

But either way,

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> + */
>  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);
>  
>         SharedMemObject()
> @@ -30,6 +40,11 @@ public:
>         {
>         }
>  
> +       /**
> +        * \brief Contstructor for the SharedMemObject.
> +        * \param[in] name The requested name.
> +        * \param[in] args Any additional args.
> +        */
>         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_;
> -- 
> 2.43.0
>
Milan Zamazal Jan. 23, 2024, 1:14 p.m. UTC | #2
Hans de Goede <hdegoede@redhat.com> writes:

> From: Dennis Bonke <admin@dennisbonke.com>
>
> Document the SharedMemObject class.
>
> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel@ucw.cz>
> ---

Besides what Kieran has already said:

>  .../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.
> + */
>  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);
>  
>  	SharedMemObject()
> @@ -30,6 +40,11 @@ public:
>  	{
>  	}
>  
> +	/**
> +	 * \brief Contstructor for the SharedMemObject.
                  ^^^^^^^^^^^^

Constructor

> +	 * \param[in] name The requested name.

Of what?  What's its purpose?  Is it unique in any sense?

> +	 * \param[in] args Any additional args.

To what?

> +	 */
>  	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_;
Laurent Pinchart Jan. 23, 2024, 1:34 p.m. UTC | #3
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@dennisbonke.com>
> > 
> > Document the SharedMemObject class.
> > 
> > Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> > Tested-by: Pavel Machek <pavel@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@ideasonboard.com>

Documentation goes to .cpp files. The documentation should also come in
the same patch as the one that adds the class.

> > + */
> >  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);
> >  
> >         SharedMemObject()
> > @@ -30,6 +40,11 @@ public:
> >         {
> >         }
> >  
> > +       /**
> > +        * \brief Contstructor for the SharedMemObject.
> > +        * \param[in] name The requested name.
> > +        * \param[in] args Any additional args.
> > +        */
> >         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_;
Laurent Pinchart Jan. 23, 2024, 2:43 p.m. UTC | #4
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@dennisbonke.com>
> > > 
> > > Document the SharedMemObject class.
> > > 
> > > Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> > > Tested-by: Pavel Machek <pavel@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@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.

> > > + */
> > >  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_;
Naushir Patuck Jan. 24, 2024, 11:07 a.m. UTC | #5
Hi all,

On Tue, 23 Jan 2024 at 14:43, Laurent Pinchart <
laurent.pinchart@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@dennisbonke.com>
> > > >
> > > > Document the SharedMemObject class.
> > > >
> > > > Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
> > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > > Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp
Lenovo x13s
> > > > Tested-by: Pavel Machek <pavel@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@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
Laurent Pinchart Feb. 5, 2024, 2:16 p.m. UTC | #6
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@dennisbonke.com>
> > > > >
> > > > > Document the SharedMemObject class.
> > > > >
> > > > > Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
> > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > > > Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> > > > > Tested-by: Pavel Machek <pavel@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@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_;

Patch
diff mbox series

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.
+ */
 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);
 
 	SharedMemObject()
@@ -30,6 +40,11 @@  public:
 	{
 	}
 
+	/**
+	 * \brief Contstructor for the SharedMemObject.
+	 * \param[in] name The requested name.
+	 * \param[in] args Any additional args.
+	 */
 	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_;