<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, 21 Feb 2021 at 13:07, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">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">Hi Naush,<br>
<br>
Thank you for the patch.<br>
<br>
On Thu, Feb 18, 2021 at 12:48:24PM +0000, Naushir Patuck wrote:<br>
> Only update the lens shading control if it is present in the<br>
> ControlList.<br>
> <br>
> Add the dmabuf file descriptor to the lens shading control in-place<br>
> rather than taking a copy.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 21 +++++++++----------<br>
>  1 file changed, 10 insertions(+), 11 deletions(-)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index acf2d56cddb2..a60415d93705 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -1338,17 +1338,16 @@ void RPiCameraData::embeddedComplete(uint32_t bufferId)<br>
>  <br>
>  void RPiCameraData::setIspControls(const ControlList &controls)<br>
>  {<br>
> -     ControlList ctrls = controls;<br>
> -<br>
> -     Span<const uint8_t> s =<br>
> -             ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();<br>
> -     bcm2835_isp_lens_shading ls =<br>
> -             *reinterpret_cast<const bcm2835_isp_lens_shading *>(s.data());<br>
> -     ls.dmabuf = lsTable_.fd();<br>
> -<br>
> -     ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ls),<br>
> -                                         sizeof(ls) });<br>
> -     ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);<br>
> +     ControlList ctrls = std::move(controls);<br>
<br>
std::move() won't have the expected effect here. controls is a const<br>
reference, so ctrls will be a copy. It's a bit annoying that the<br>
compiler doesn't warn us :-/<br>
<br>
The code will run fine, but I think avoiding std::move() would be more<br>
explicit as it's easy to overlook the fact that a copy is created<br>
otherwise. No need to change this now, but if you get the chance at some<br>
point, it would be nice.<br></blockquote><div><br></div><div>Understood, will put it on my todo list.</div><div><br></div><div>Just curious though, what is the issue with the old method of getting the IPA to fill in the file descriptor in the struct?</div><div>This would avoid the need of modifying the control list like this in the pipeline handler.  Is it to do with testing process</div><div>isolation between the pipeline handler and IPA?</div><div><br></div><div>Thanks,</div><div>Naush</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>
> +<br>
> +     if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) {<br>
> +             ControlValue &value =<br>
> +                     const_cast<ControlValue &>(ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING));<br>
<br>
The const_cast is not nice. If we want to support this kind of usage, we<br>
should extend the ControlList API to return a non-const ControlValue.<br>
<br>
> +             Span<uint8_t> s = value.data();<br>
> +             bcm2835_isp_lens_shading *ls =<br>
> +                     reinterpret_cast<bcm2835_isp_lens_shading *>(s.data());<br>
> +             ls->dmabuf = lsTable_.fd();<br>
> +     }<br>
>  <br>
>       isp_[Isp::Input].dev()->setControls(&ctrls);<br>
>       handleState();<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>