<div dir="auto"><div>Hi Laurent,<br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, 21 Feb 2021, 7:52 pm Laurent Pinchart, <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Sun, Feb 21, 2021 at 07:39:38PM +0000, Naushir Patuck wrote:<br>
> On Sun, 21 Feb 2021 at 13:07, Laurent Pinchart wrote: <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" rel="noreferrer">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>
> <br>
> Understood, will put it on my todo list.<br>
> <br>
> Just curious though, what is the issue with the old method of getting the<br>
> IPA to fill in the file descriptor in the struct?<br>
> This would avoid the need of modifying the control list like this in the<br>
> pipeline handler. Is it to do with testing process<br>
> isolation between the pipeline handler and IPA?<br>
<br>
Yes, it's related to process isolation. When the IPA is run within the<br>
libcamera process, file descriptors are just integers, and they can be<br>
passed back and forth between the pipeline handler and IPA without any<br>
issue.<br>
<br>
When isolating an IPA in a process, the file descriptors have to be<br>
passed through the IPC pipe (the IPA IPC mechanism handles this<br>
transparently), and the receiving side will see the file descriptor as<br>
an integer local to the process, whose numerical value will usually be<br>
different than the one in the sending process. The lens shading table<br>
file descriptor can be used in the IPA to map the buffer, but if its<br>
numerical value is set in the V4L2_CID_USER_BCM2835_ISP_LENS_SHADING<br>
control, there will be no translation on the way back to the pipeline<br>
handler as the IPA IPC layer isn't aware that the V4L2 control contains<br>
a file descriptor. The control will be set by the libcamera process,<br>
using a file descriptor value that corresponds to the IPA process.<br>
<br>
One way to handle this would be to pass both the file descriptor and its<br>
value as an integer to the IPA, but it's a bit of a hack, the IPA<br>
shouldn't have to care about this. That's why it's patched in the<br>
pipeline handler.<br>
<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>
Do you think we should have such an extension ?<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Yes please :-)</div><div dir="auto"><br></div><div dir="auto">This would certainly simplify code like the above!</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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></div>