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

[Naushir Patuck]

Hi Laurent,

On Sun, 21 Feb 2021 at 13:07, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> 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?

Thanks,
Naush


>
> > +
> > +     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.
>
> > +             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/f51a09e2/attachment.htm>