| Message ID | 20240113142218.28063-6-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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 >
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_;
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_;
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_;
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
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_;
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_;