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

[Naushir Patuck]

Hi Laurent,

On Sun, 21 Feb 2021, 7:52 pm Laurent Pinchart, <
laurent.pinchart at ideasonboard.com> wrote:

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

Yes please :-)

This would certainly simplify code like the above!


> > > > +             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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210221/4244cc77/attachment-0001.htm>