| Message ID | 20240508080401.14850-7-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush On Wed, May 08, 2024 at 09:04:01AM GMT, Naushir Patuck wrote: > The shared_mem_object may be used to construct complex classes, so > remove the standard layout type restriction. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> This looks fine to me, but maybe better check on why this was introduced in first place. Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > --- > include/libcamera/internal/shared_mem_object.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h > index 9b1d939302a8..c9c0482062bd 100644 > --- a/include/libcamera/internal/shared_mem_object.h > +++ b/include/libcamera/internal/shared_mem_object.h > @@ -56,7 +56,7 @@ private: > Span<uint8_t> mem_; > }; > > -template<class T, typename = std::enable_if_t<std::is_standard_layout<T>::value>> > +template<class T> > class SharedMemObject : public SharedMem > { > public: > -- > 2.34.1 >
Quoting Jacopo Mondi (2024-05-09 10:49:41) > Hi Naush > > On Wed, May 08, 2024 at 09:04:01AM GMT, Naushir Patuck wrote: > > The shared_mem_object may be used to construct complex classes, so > > remove the standard layout type restriction. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > This looks fine to me, but maybe better check on why this was > introduced in first place. > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Thanks > j > > > --- > > include/libcamera/internal/shared_mem_object.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h > > index 9b1d939302a8..c9c0482062bd 100644 > > --- a/include/libcamera/internal/shared_mem_object.h > > +++ b/include/libcamera/internal/shared_mem_object.h > > @@ -56,7 +56,7 @@ private: > > Span<uint8_t> mem_; > > }; > > > > -template<class T, typename = std::enable_if_t<std::is_standard_layout<T>::value>> > > +template<class T> I think I'm fine with this as long as the CI builds succeed, so we're about to find out ;-) Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > class SharedMemObject : public SharedMem > > { > > public: > > -- > > 2.34.1 > >
On Thu, May 09, 2024 at 11:49:41AM +0200, Jacopo Mondi wrote: > On Wed, May 08, 2024 at 09:04:01AM GMT, Naushir Patuck wrote: > > The shared_mem_object may be used to construct complex classes, so > > remove the standard layout type restriction. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > This looks fine to me, but maybe better check on why this was > introduced in first place. It was added there because sharing objects containing pointers between different processes isn't safe. Naush, what object do you need to store in shared memory that has a non-standard layout, but is still safe to share ? > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > --- > > include/libcamera/internal/shared_mem_object.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h > > index 9b1d939302a8..c9c0482062bd 100644 > > --- a/include/libcamera/internal/shared_mem_object.h > > +++ b/include/libcamera/internal/shared_mem_object.h > > @@ -56,7 +56,7 @@ private: > > Span<uint8_t> mem_; > > }; > > > > -template<class T, typename = std::enable_if_t<std::is_standard_layout<T>::value>> > > +template<class T> > > class SharedMemObject : public SharedMem > > { > > public:
Quoting Laurent Pinchart (2024-05-09 12:16:15) > On Thu, May 09, 2024 at 11:49:41AM +0200, Jacopo Mondi wrote: > > On Wed, May 08, 2024 at 09:04:01AM GMT, Naushir Patuck wrote: > > > The shared_mem_object may be used to construct complex classes, so > > > remove the standard layout type restriction. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > This looks fine to me, but maybe better check on why this was > > introduced in first place. > > It was added there because sharing objects containing pointers between > different processes isn't safe. Naush, what object do you need to store > in shared memory that has a non-standard layout, but is still safe to > share ? Oh, interesting point. I wonder if that's why we've seen people get segfaults if the RPi IPA runs in isolation (as then these buffers are shared, and could potentially contain invalid pointers?) -- Kieran > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > --- > > > include/libcamera/internal/shared_mem_object.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h > > > index 9b1d939302a8..c9c0482062bd 100644 > > > --- a/include/libcamera/internal/shared_mem_object.h > > > +++ b/include/libcamera/internal/shared_mem_object.h > > > @@ -56,7 +56,7 @@ private: > > > Span<uint8_t> mem_; > > > }; > > > > > > -template<class T, typename = std::enable_if_t<std::is_standard_layout<T>::value>> > > > +template<class T> > > > class SharedMemObject : public SharedMem > > > { > > > public: > > -- > Regards, > > Laurent Pinchart
On Thu, May 09, 2024 at 12:22:56PM +0100, Kieran Bingham wrote: > Quoting Laurent Pinchart (2024-05-09 12:16:15) > > On Thu, May 09, 2024 at 11:49:41AM +0200, Jacopo Mondi wrote: > > > On Wed, May 08, 2024 at 09:04:01AM GMT, Naushir Patuck wrote: > > > > The shared_mem_object may be used to construct complex classes, so > > > > remove the standard layout type restriction. > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > This looks fine to me, but maybe better check on why this was > > > introduced in first place. > > > > It was added there because sharing objects containing pointers between > > different processes isn't safe. Naush, what object do you need to store > > in shared memory that has a non-standard layout, but is still safe to > > share ? > > Oh, interesting point. I wonder if that's why we've seen people get > segfaults if the RPi IPA runs in isolation (as then these buffers are > shared, and could potentially contain invalid pointers?) On Pi4 the only shared buffer I'm aware of contains the LSC table, so that should be fine. I haven't checked Pi5. The std::is_standard_layout() check doesn't catch pointers unfortunately, but it catches C++ objects that can contain pointers (such as containers from the C++ standard library). It may also catch objects that are fine, if that's the case here, we can try to relax the check. > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > > > --- > > > > include/libcamera/internal/shared_mem_object.h | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h > > > > index 9b1d939302a8..c9c0482062bd 100644 > > > > --- a/include/libcamera/internal/shared_mem_object.h > > > > +++ b/include/libcamera/internal/shared_mem_object.h > > > > @@ -56,7 +56,7 @@ private: > > > > Span<uint8_t> mem_; > > > > }; > > > > > > > > -template<class T, typename = std::enable_if_t<std::is_standard_layout<T>::value>> > > > > +template<class T> > > > > class SharedMemObject : public SharedMem > > > > { > > > > public:
On Thu, 9 May 2024 at 12:31, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > On Thu, May 09, 2024 at 12:22:56PM +0100, Kieran Bingham wrote: > > Quoting Laurent Pinchart (2024-05-09 12:16:15) > > > On Thu, May 09, 2024 at 11:49:41AM +0200, Jacopo Mondi wrote: > > > > On Wed, May 08, 2024 at 09:04:01AM GMT, Naushir Patuck wrote: > > > > > The shared_mem_object may be used to construct complex classes, so > > > > > remove the standard layout type restriction. > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > > > This looks fine to me, but maybe better check on why this was > > > > introduced in first place. > > > > > > It was added there because sharing objects containing pointers between > > > different processes isn't safe. Naush, what object do you need to store > > > in shared memory that has a non-standard layout, but is still safe to > > > share ? > > > > Oh, interesting point. I wonder if that's why we've seen people get > > segfaults if the RPi IPA runs in isolation (as then these buffers are > > shared, and could potentially contain invalid pointers?) > > On Pi4 the only shared buffer I'm aware of contains the LSC table, so > that should be fine. I haven't checked Pi5. > The Pi 4 LS tables use a dmabuf and FD between the pipeline handler and IPA, so that's ok. > > The std::is_standard_layout() check doesn't catch pointers > unfortunately, but it catches C++ objects that can contain pointers > (such as containers from the C++ standard library). It may also catch > objects that are fine, if that's the case here, we can try to relax the > check. > Eek, this can be nasty. Frontend is safe, but Backend is not. We don't use any pointers but have a couple of std::vectors, these can easily be removed. It does have a few std::map types that are problematic. We only ever access these maps and vectors from the pipeline handler side, so it "works", but potentially fragile. I don't know what to do here... > > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > > > > > --- > > > > > include/libcamera/internal/shared_mem_object.h | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/include/libcamera/internal/shared_mem_object.h > b/include/libcamera/internal/shared_mem_object.h > > > > > index 9b1d939302a8..c9c0482062bd 100644 > > > > > --- a/include/libcamera/internal/shared_mem_object.h > > > > > +++ b/include/libcamera/internal/shared_mem_object.h > > > > > @@ -56,7 +56,7 @@ private: > > > > > Span<uint8_t> mem_; > > > > > }; > > > > > > > > > > -template<class T, typename = > std::enable_if_t<std::is_standard_layout<T>::value>> > > > > > +template<class T> > > > > > class SharedMemObject : public SharedMem > > > > > { > > > > > public: > > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Thu, May 09, 2024 at 12:55:20PM +0100, Naushir Patuck wrote: > On Thu, 9 May 2024 at 12:31, Laurent Pinchart wrote: > > On Thu, May 09, 2024 at 12:22:56PM +0100, Kieran Bingham wrote: > > > Quoting Laurent Pinchart (2024-05-09 12:16:15) > > > > On Thu, May 09, 2024 at 11:49:41AM +0200, Jacopo Mondi wrote: > > > > > On Wed, May 08, 2024 at 09:04:01AM GMT, Naushir Patuck wrote: > > > > > > The shared_mem_object may be used to construct complex classes, so > > > > > > remove the standard layout type restriction. > > > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > > > > > This looks fine to me, but maybe better check on why this was > > > > > introduced in first place. > > > > > > > > It was added there because sharing objects containing pointers between > > > > different processes isn't safe. Naush, what object do you need to store > > > > in shared memory that has a non-standard layout, but is still safe to > > > > share ? > > > > > > Oh, interesting point. I wonder if that's why we've seen people get > > > segfaults if the RPi IPA runs in isolation (as then these buffers are > > > shared, and could potentially contain invalid pointers?) > > > > On Pi4 the only shared buffer I'm aware of contains the LSC table, so > > that should be fine. I haven't checked Pi5. > > The Pi 4 LS tables use a dmabuf and FD between the pipeline handler and > IPA, so that's ok. Ah yes, I forgot for a moment it was using dmabuf. > > The std::is_standard_layout() check doesn't catch pointers > > unfortunately, but it catches C++ objects that can contain pointers > > (such as containers from the C++ standard library). It may also catch > > objects that are fine, if that's the case here, we can try to relax the > > check. > > Eek, this can be nasty. Frontend is safe, but Backend is not. We don't > use any pointers but have a couple of std::vectors, these can easily be > removed. It does have a few std::map types that are problematic. We only > ever access these maps and vectors from the pipeline handler side, so > it "works", but potentially fragile. I don't know what to do here... Can we discuss this by looking at the problematic code, and the layout of the object you want to share between the pipeline handler and IPA side ? A short explanation regarding what parts of the data are read and written on each side will be useful. > > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > > > > > > > --- > > > > > > include/libcamera/internal/shared_mem_object.h | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h > > > > > > index 9b1d939302a8..c9c0482062bd 100644 > > > > > > --- a/include/libcamera/internal/shared_mem_object.h > > > > > > +++ b/include/libcamera/internal/shared_mem_object.h > > > > > > @@ -56,7 +56,7 @@ private: > > > > > > Span<uint8_t> mem_; > > > > > > }; > > > > > > > > > > > > -template<class T, typename = std::enable_if_t<std::is_standard_layout<T>::value>> > > > > > > +template<class T> > > > > > > class SharedMemObject : public SharedMem > > > > > > { > > > > > > public:
On Thu, 9 May 2024 at 13:26, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > On Thu, May 09, 2024 at 12:55:20PM +0100, Naushir Patuck wrote: > > On Thu, 9 May 2024 at 12:31, Laurent Pinchart wrote: > > > On Thu, May 09, 2024 at 12:22:56PM +0100, Kieran Bingham wrote: > > > > Quoting Laurent Pinchart (2024-05-09 12:16:15) > > > > > On Thu, May 09, 2024 at 11:49:41AM +0200, Jacopo Mondi wrote: > > > > > > On Wed, May 08, 2024 at 09:04:01AM GMT, Naushir Patuck wrote: > > > > > > > The shared_mem_object may be used to construct complex classes, so > > > > > > > remove the standard layout type restriction. > > > > > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > > > > > > > This looks fine to me, but maybe better check on why this was > > > > > > introduced in first place. > > > > > > > > > > It was added there because sharing objects containing pointers between > > > > > different processes isn't safe. Naush, what object do you need to store > > > > > in shared memory that has a non-standard layout, but is still safe to > > > > > share ? > > > > > > > > Oh, interesting point. I wonder if that's why we've seen people get > > > > segfaults if the RPi IPA runs in isolation (as then these buffers are > > > > shared, and could potentially contain invalid pointers?) > > > > > > On Pi4 the only shared buffer I'm aware of contains the LSC table, so > > > that should be fine. I haven't checked Pi5. > > > > The Pi 4 LS tables use a dmabuf and FD between the pipeline handler and > > IPA, so that's ok. > > Ah yes, I forgot for a moment it was using dmabuf. > > > > The std::is_standard_layout() check doesn't catch pointers > > > unfortunately, but it catches C++ objects that can contain pointers > > > (such as containers from the C++ standard library). It may also catch > > > objects that are fine, if that's the case here, we can try to relax the > > > check. > > > > Eek, this can be nasty. Frontend is safe, but Backend is not. We don't > > use any pointers but have a couple of std::vectors, these can easily be > > removed. It does have a few std::map types that are problematic. We only > > ever access these maps and vectors from the pipeline handler side, so > > it "works", but potentially fragile. I don't know what to do here... > > Can we discuss this by looking at the problematic code, and the layout > of the object you want to share between the pipeline handler and IPA > side ? A short explanation regarding what parts of the data are read and > written on each side will be useful. Ok, this turned out to be a lot less painful than I expected. I've managed to make classes FrontEnd and BackEnd standard layout types by getting rid of a const reference and the std::maps. We can remove this patch from the series. I'll post an update to libpisp with the changes shortly for you to have a look at the needed changes. Naush > > > > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > > > > > > > > > --- > > > > > > > include/libcamera/internal/shared_mem_object.h | 2 +- > > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h > > > > > > > index 9b1d939302a8..c9c0482062bd 100644 > > > > > > > --- a/include/libcamera/internal/shared_mem_object.h > > > > > > > +++ b/include/libcamera/internal/shared_mem_object.h > > > > > > > @@ -56,7 +56,7 @@ private: > > > > > > > Span<uint8_t> mem_; > > > > > > > }; > > > > > > > > > > > > > > -template<class T, typename = std::enable_if_t<std::is_standard_layout<T>::value>> > > > > > > > +template<class T> > > > > > > > class SharedMemObject : public SharedMem > > > > > > > { > > > > > > > public: > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h index 9b1d939302a8..c9c0482062bd 100644 --- a/include/libcamera/internal/shared_mem_object.h +++ b/include/libcamera/internal/shared_mem_object.h @@ -56,7 +56,7 @@ private: Span<uint8_t> mem_; }; -template<class T, typename = std::enable_if_t<std::is_standard_layout<T>::value>> +template<class T> class SharedMemObject : public SharedMem { public:
The shared_mem_object may be used to construct complex classes, so remove the standard layout type restriction. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- include/libcamera/internal/shared_mem_object.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)