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

[Naushir Patuck]

On Thu, 9 May 2024 at 13:26, Laurent Pinchart
<laurent.pinchart at 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 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.


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