[libcamera-devel] [PATCH v3 4/4] pipeline: raspberrypi: Update the lens shading control in-place

[Laurent Pinchart]

On Sun, Feb 21, 2021 at 07:39:38PM +0000, Naushir Patuck wrote:
> On Sun, 21 Feb 2021 at 13:07, Laurent Pinchart wrote: 
> > On Thu, Feb 18, 2021 at 12:48:24PM +0000, Naushir Patuck wrote:
> > > Only update the lens shading control if it is present in the
> > > ControlList.
> > >
> > > Add the dmabuf file descriptor to the lens shading control in-place
> > > rather than taking a copy.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > ---
> > >  .../pipeline/raspberrypi/raspberrypi.cpp      | 21 +++++++++----------
> > >  1 file changed, 10 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index acf2d56cddb2..a60415d93705 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -1338,17 +1338,16 @@ void RPiCameraData::embeddedComplete(uint32_t bufferId)
> > >
> > >  void RPiCameraData::setIspControls(const ControlList &controls)
> > >  {
> > > -     ControlList ctrls = controls;
> > > -
> > > -     Span<const uint8_t> s =
> > > -             ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();
> > > -     bcm2835_isp_lens_shading ls =
> > > -             *reinterpret_cast<const bcm2835_isp_lens_shading *>(s.data());
> > > -     ls.dmabuf = lsTable_.fd();
> > > -
> > > -     ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ls),
> > > -                                         sizeof(ls) });
> > > -     ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);
> > > +     ControlList ctrls = std::move(controls);
> >
> > std::move() won't have the expected effect here. controls is a const
> > reference, so ctrls will be a copy. It's a bit annoying that the
> > compiler doesn't warn us :-/
> >
> > The code will run fine, but I think avoiding std::move() would be more
> > explicit as it's easy to overlook the fact that a copy is created
> > otherwise. No need to change this now, but if you get the chance at some
> > point, it would be nice.
> 
> Understood, will put it on my todo list.
> 
> Just curious though, what is the issue with the old method of getting the
> IPA to fill in the file descriptor in the struct?
> This would avoid the need of modifying the control list like this in the
> pipeline handler.  Is it to do with testing process
> isolation between the pipeline handler and IPA?

Yes, it's related to process isolation. When the IPA is run within the
libcamera process, file descriptors are just integers, and they can be
passed back and forth between the pipeline handler and IPA without any
issue.

When isolating an IPA in a process, the file descriptors have to be
passed through the IPC pipe (the IPA IPC mechanism handles this
transparently), and the receiving side will see the file descriptor as
an integer local to the process, whose numerical value will usually be
different than the one in the sending process. The lens shading table
file descriptor can be used in the IPA to map the buffer, but if its
numerical value is set in the V4L2_CID_USER_BCM2835_ISP_LENS_SHADING
control, there will be no translation on the way back to the pipeline
handler as the IPA IPC layer isn't aware that the V4L2 control contains
a file descriptor. The control will be set by the libcamera process,
using a file descriptor value that corresponds to the IPA process.

One way to handle this would be to pass both the file descriptor and its
value as an integer to the IPA, but it's a bit of a hack, the IPA
shouldn't have to care about this. That's why it's patched in the
pipeline handler.

> > > +
> > > +     if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) {
> > > +             ControlValue &value =
> > > +                     const_cast<ControlValue &>(ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING));
> >
> > The const_cast is not nice. If we want to support this kind of usage, we
> > should extend the ControlList API to return a non-const ControlValue.

Do you think we should have such an extension ?

> > > +             Span<uint8_t> s = value.data();
> > > +             bcm2835_isp_lens_shading *ls =
> > > +                     reinterpret_cast<bcm2835_isp_lens_shading *>(s.data());
> > > +             ls->dmabuf = lsTable_.fd();
> > > +     }
> > >
> > >       isp_[Isp::Input].dev()->setControls(&ctrls);
> > >       handleState();

-- 
Regards,

Laurent Pinchart