[PATCH v2 6/6] libcamera: shared_mem_object: Remove is_standard_layout restriction

[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 at 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 at 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