<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 9 May 2024 at 12:31, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Thu, May 09, 2024 at 12:22:56PM +0100, Kieran Bingham wrote:<br>
> Quoting Laurent Pinchart (2024-05-09 12:16:15)<br>
> > On Thu, May 09, 2024 at 11:49:41AM +0200, Jacopo Mondi wrote:<br>
> > > On Wed, May 08, 2024 at 09:04:01AM GMT, Naushir Patuck wrote:<br>
> > > > The shared_mem_object may be used to construct complex classes, so<br>
> > > > remove the standard layout type restriction.<br>
> > > ><br>
> > > > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > > <br>
> > > This looks fine to me, but maybe better check on why this was<br>
> > > introduced in first place.<br>
> > <br>
> > It was added there because sharing objects containing pointers between<br>
> > different processes isn't safe. Naush, what object do you need to store<br>
> > in shared memory that has a non-standard layout, but is still safe to<br>
> > share ?<br>
> <br>
> Oh, interesting point. I wonder if that's why we've seen people get<br>
> segfaults if the RPi IPA runs in isolation (as then these buffers are<br>
> shared, and could potentially contain invalid pointers?)<br>
<br>
On Pi4 the only shared buffer I'm aware of contains the LSC table, so<br>
that should be fine. I haven't checked Pi5.<br></blockquote><div><br></div><div>The Pi 4 LS tables use a dmabuf and FD between the pipeline handler and IPA, so that's ok.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
The std::is_standard_layout() check doesn't catch pointers<br>
unfortunately, but it catches C++ objects that can contain pointers<br>
(such as containers from the C++ standard library). It may also catch<br>
objects that are fine, if that's the case here, we can try to relax the<br>
check.<br></blockquote><div><br></div><div>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...</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> > > Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>><br>
> > > <br>
> > > > ---<br>
> > > >  include/libcamera/internal/shared_mem_object.h | 2 +-<br>
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)<br>
> > > ><br>
> > > > diff --git a/include/libcamera/internal/shared_mem_object.h b/include/libcamera/internal/shared_mem_object.h<br>
> > > > index 9b1d939302a8..c9c0482062bd 100644<br>
> > > > --- a/include/libcamera/internal/shared_mem_object.h<br>
> > > > +++ b/include/libcamera/internal/shared_mem_object.h<br>
> > > > @@ -56,7 +56,7 @@ private:<br>
> > > >     Span<uint8_t> mem_;<br>
> > > >  };<br>
> > > ><br>
> > > > -template<class T, typename = std::enable_if_t<std::is_standard_layout<T>::value>><br>
> > > > +template<class T><br>
> > > >  class SharedMemObject : public SharedMem<br>
> > > >  {<br>
> > > >  public:<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>