[libcamera-devel,3/3] pipeline: raspberrypi: Update lens shading control in-place

11320 diff mbox series

Add the dmabuf file descriptor to the lens shading control in-place
rather than taking a copy.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Hi Naush,

On 17/02/2021 10:08, Naushir Patuck wrote:
> Add the dmabuf file descriptor to the lens shading control in-place
> rather than taking a copy.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 9dd4d112a907..a60415d93705 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1338,13 +1338,14 @@ void RPiCameraData::embeddedComplete(uint32_t bufferId)
>  
>  void RPiCameraData::setIspControls(const ControlList &controls)
>  {
> -	ControlList ctrls = controls;
> +	ControlList ctrls = std::move(controls);

Oh, can we do that on a const ControlList reference ?
Doesn't that by definition modify the parameter that came in?


>  
>  	if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) {
> -		Span<const uint8_t> s =
> -			ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();
> -		const bcm2835_isp_lens_shading *ls =
> -			reinterpret_cast<const bcm2835_isp_lens_shading *>(s.data());
> +		ControlValue &value =
> +			const_cast<ControlValue &>(ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING));

Could you take a look at Laurent's V4L2 control series he posted recently?

 [PATCH/RFC 0/2] libcamera: Add Control instances for V4L2 controls

By putting the types into the Control, I think it helps simplify a lot
of code, but I'm curious how it would affect controls which map to
structure arrays like this...

> +		Span<uint8_t> s = value.data();
> +		bcm2835_isp_lens_shading *ls =
> +			reinterpret_cast<bcm2835_isp_lens_shading *>(s.data());
>  		ls->dmabuf = lsTable_.fd();
>  	}

Now that you've moved controls to the local ctrls, when it reaches the
end of this function, it would go out of scope and release the control
list won't it ?

Does that mean your now potentially pointing to a lens-shading table
that has been released? or is that not an issue?

(Or is it used later in this function which the mail has not provided
context for ?...)



>  
>

Hi Kieran,

On Thu, 18 Feb 2021 at 11:35, Kieran Bingham <
kieran.bingham@ideasonboard.com> wrote:

> Hi Naush,
>
> On 17/02/2021 10:08, Naushir Patuck wrote:
> > Add the dmabuf file descriptor to the lens shading control in-place
> > rather than taking a copy.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 9dd4d112a907..a60415d93705 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1338,13 +1338,14 @@ void RPiCameraData::embeddedComplete(uint32_t
> bufferId)
> >
> >  void RPiCameraData::setIspControls(const ControlList &controls)
> >  {
> > -     ControlList ctrls = controls;
> > +     ControlList ctrls = std::move(controls);
>
> Oh, can we do that on a const ControlList reference ?
> Doesn't that by definition modify the parameter that came in?
>

The compiler does not seem to complain, so I assume this works as we are
not (yet) modifying the elements of the map container.


>
>
> >
> >       if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) {
> > -             Span<const uint8_t> s =
> > -
>  ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();
> > -             const bcm2835_isp_lens_shading *ls =
> > -                     reinterpret_cast<const bcm2835_isp_lens_shading
> *>(s.data());
> > +             ControlValue &value =
> > +                     const_cast<ControlValue
> &>(ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING));
>
> Could you take a look at Laurent's V4L2 control series he posted recently?
>
>  [PATCH/RFC 0/2] libcamera: Add Control instances for V4L2 controls
>
> By putting the types into the Control, I think it helps simplify a lot
> of code, but I'm curious how it would affect controls which map to
> structure arrays like this...
>

Sure, will have a look at that.


>
> > +             Span<uint8_t> s = value.data();
> > +             bcm2835_isp_lens_shading *ls =
> > +                     reinterpret_cast<bcm2835_isp_lens_shading
> *>(s.data());
> >               ls->dmabuf = lsTable_.fd();
> >       }
>
> Now that you've moved controls to the local ctrls, when it reaches the
> end of this function, it would go out of scope and release the control
> list won't it ?
>

Yes, the local ctrls will be out of scope at that point, but the call to
setControls() would have copied the
bcm2835_isp_lens_shading struct internally, so that is not an issue.


>
> Does that mean your now potentially pointing to a lens-shading table
> that has been released? or is that not an issue?
>

The bcm2835_isp_lens_shading does not hold the table directly, rather a
handle to the dmabuf
where the table is actually stored.  So again, this is not an issue.


>
> (Or is it used later in this function which the mail has not provided
> context for ?...)
>

The setControls() call is the last line of the patch, so you may have
missed that :-)

However, if you do feel this is still wrong, please shout!


>
>
>
> >
> >
>
> --
> Regards
> --
> Kieran
>