[libcamera-devel,1/3] pipeline: ipa: raspberrypi: Various fixups after IPAInterface changes
diff mbox series

Message ID 20210217100852.1542397-1-naush@raspberrypi.com
State Superseded
Headers show
Series
  • [libcamera-devel,1/3] pipeline: ipa: raspberrypi: Various fixups after IPAInterface changes
Related show

Commit Message

Naushir Patuck Feb. 17, 2021, 10:08 a.m. UTC 
This commit addresses a few fixes and tidy-ups after the IPAInterface
rework:
- Rename ConfigStaggeredWrite -> ConfigSensorParams
- Rename setIsp -> setIspControls
- Switch to camel case for ISPConfig::embeddedbufferId and
ISPConfig::bayerbufferId.
- Signal handlers statsMetadataComplete(), runISP(), embeddedComplete()
should only run when state != Stopped. This matches the behavior of
the original code. Without this, start/stop cycles may cause errors.
- In setIspControls(), only update the LS config (with the fd) when a LS
control is present.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Fixes: e201cb4f5450 ("libcamera: IPAInterface: Replace C API with the new C++-only API")
---
 include/libcamera/ipa/raspberrypi.mojom       |  8 ++---
 src/ipa/raspberrypi/raspberrypi.cpp           |  8 ++---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 36 ++++++++++---------
 3 files changed, 27 insertions(+), 25 deletions(-)

Comments

Paul Elder Feb. 17, 2021, 10:18 a.m. UTC | #1 
Hi Naush,

On Wed, Feb 17, 2021 at 10:08:50AM +0000, Naushir Patuck wrote:
> This commit addresses a few fixes and tidy-ups after the IPAInterface
> rework:
> - Rename ConfigStaggeredWrite -> ConfigSensorParams
> - Rename setIsp -> setIspControls
> - Switch to camel case for ISPConfig::embeddedbufferId and
> ISPConfig::bayerbufferId.
> - Signal handlers statsMetadataComplete(), runISP(), embeddedComplete()
> should only run when state != Stopped. This matches the behavior of
> the original code. Without this, start/stop cycles may cause errors.
> - In setIspControls(), only update the LS config (with the fd) when a LS
> control is present.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Fixes: e201cb4f5450 ("libcamera: IPAInterface: Replace C API with the new C++-only API")
> ---
>  include/libcamera/ipa/raspberrypi.mojom       |  8 ++---
>  src/ipa/raspberrypi/raspberrypi.cpp           |  8 ++---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 36 ++++++++++---------
>  3 files changed, 27 insertions(+), 25 deletions(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index bab19a946e18..9c05cc68cceb 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -16,7 +16,7 @@ enum BufferMask {
>  const uint32 MaxLsGridSize = 0x8000;
>  
>  enum ConfigOutputParameters {
> -	ConfigStaggeredWrite = 0x01,
> +	ConfigSensorParams = 0x01,
>  };
>  
>  struct SensorConfig {
> @@ -27,8 +27,8 @@ struct SensorConfig {
>  };
>  
>  struct ISPConfig {
> -	uint32 embeddedbufferId;
> -	uint32 bayerbufferId;
> +	uint32 embeddedBufferId;
> +	uint32 bayerBufferId;
>  };
>  
>  struct ConfigInput {
> @@ -126,6 +126,6 @@ interface IPARPiEventInterface {
>  	statsMetadataComplete(uint32 bufferId, ControlList controls);
>  	runIsp(uint32 bufferId);
>  	embeddedComplete(uint32 bufferId);
> -	setIsp(ControlList controls);
> +	setIspControls(ControlList controls);
>  	setDelayedControls(ControlList controls);
>  };
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 81a3195c82e9..974f4ec63058 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -344,7 +344,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		helper_->GetDelays(exposureDelay, gainDelay);
>  		sensorMetadata = helper_->SensorEmbeddedDataPresent();
>  
> -		result->params |= ipa::rpi::ConfigStaggeredWrite;
> +		result->params |= ipa::rpi::ConfigSensorParams;
>  		result->sensorConfig.gainDelay = gainDelay;
>  		result->sensorConfig.exposureDelay = exposureDelay;
>  		result->sensorConfig.vblank = exposureDelay;
> @@ -447,11 +447,11 @@ void IPARPi::signalIspPrepare(const ipa::rpi::ISPConfig &data)
>  	 * avoid running the control algos for a few frames in case
>  	 * they are "unreliable".
>  	 */
> -	prepareISP(data.embeddedbufferId);
> +	prepareISP(data.embeddedBufferId);
>  	frameCount_++;
>  
>  	/* Ready to push the input buffer into the ISP. */
> -	runIsp.emit(data.bayerbufferId & ipa::rpi::MaskID);
> +	runIsp.emit(data.bayerBufferId & ipa::rpi::MaskID);
>  }
>  
>  void IPARPi::reportMetadata()
> @@ -968,7 +968,7 @@ void IPARPi::prepareISP(unsigned int bufferId)
>  			applyDPC(dpcStatus, ctrls);
>  
>  		if (!ctrls.empty())
> -			setIsp.emit(ctrls);
> +			setIspControls.emit(ctrls);
>  	}
>  }
>  
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 15aa600ed581..8770ae66a21a 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -152,7 +152,7 @@ public:
>  	void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);
>  	void runIsp(uint32_t bufferId);
>  	void embeddedComplete(uint32_t bufferId);
> -	void setIsp(const ControlList &controls);
> +	void setIspControls(const ControlList &controls);
>  	void setDelayedControls(const ControlList &controls);
>  
>  	/* bufferComplete signal handlers. */
> @@ -1172,7 +1172,7 @@ int RPiCameraData::loadIPA()
>  	ipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete);
>  	ipa_->runIsp.connect(this, &RPiCameraData::runIsp);
>  	ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);
> -	ipa_->setIsp.connect(this, &RPiCameraData::setIsp);
> +	ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);
>  	ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);
>  
>  	IPASettings settings(ipa_->configurationFile(sensor_->model() + ".json"));
> @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  		return -EPIPE;
>  	}
>  
> -	if (result.params & ipa::rpi::ConfigStaggeredWrite) {
> +	if (result.params & ipa::rpi::ConfigSensorParams) {
>  		/*
>  		 * Setup our delayed control writer with the sensor default
>  		 * gain and exposure delays.
> @@ -1281,7 +1281,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)
>  {
>  	if (state_ == State::Stopped)
> -		handleState();
> +		return;

I thought handleState() still needs to be called in this (and below) case?


Paul

>  
>  	FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
>  
> @@ -1314,7 +1314,7 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &
>  void RPiCameraData::runIsp(uint32_t bufferId)
>  {
>  	if (state_ == State::Stopped)
> -		handleState();
> +		return;
>  
>  	FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);
>  
> @@ -1329,26 +1329,28 @@ void RPiCameraData::runIsp(uint32_t bufferId)
>  void RPiCameraData::embeddedComplete(uint32_t bufferId)
>  {
>  	if (state_ == State::Stopped)
> -		handleState();
> +		return;
>  
>  	FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);
>  	handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
>  	handleState();
>  }
>  
> -void RPiCameraData::setIsp(const ControlList &controls)
> +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();
> +	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();
> +		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);
> +		ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ls),
> +						    sizeof(ls) });
> +		ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);
> +	}
>  
>  	isp_[Isp::Input].dev()->setControls(&ctrls);
>  	handleState();
> @@ -1692,8 +1694,8 @@ void RPiCameraData::tryRunPipeline()
>  			<< " Embedded buffer id: " << embeddedId;
>  
>  	ipa::rpi::ISPConfig ispPrepare;
> -	ispPrepare.embeddedbufferId = ipa::rpi::MaskEmbeddedData | embeddedId;
> -	ispPrepare.bayerbufferId = ipa::rpi::MaskBayerData | bayerId;
> +	ispPrepare.embeddedBufferId = ipa::rpi::MaskEmbeddedData | embeddedId;
> +	ispPrepare.bayerBufferId = ipa::rpi::MaskBayerData | bayerId;
>  	ipa_->signalIspPrepare(ispPrepare);
>  }
>  
> -- 
> 2.25.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Naushir Patuck Feb. 17, 2021, 10:25 a.m. UTC | #2 
Hi Paul,


On Wed, 17 Feb 2021 at 10:18, <paul.elder@ideasonboard.com> wrote:

> Hi Naush,
>
> On Wed, Feb 17, 2021 at 10:08:50AM +0000, Naushir Patuck wrote:
> > This commit addresses a few fixes and tidy-ups after the IPAInterface
> > rework:
> > - Rename ConfigStaggeredWrite -> ConfigSensorParams
> > - Rename setIsp -> setIspControls
> > - Switch to camel case for ISPConfig::embeddedbufferId and
> > ISPConfig::bayerbufferId.
> > - Signal handlers statsMetadataComplete(), runISP(), embeddedComplete()
> > should only run when state != Stopped. This matches the behavior of
> > the original code. Without this, start/stop cycles may cause errors.
> > - In setIspControls(), only update the LS config (with the fd) when a LS
> > control is present.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Fixes: e201cb4f5450 ("libcamera: IPAInterface: Replace C API with the
> new C++-only API")
> > ---
> >  include/libcamera/ipa/raspberrypi.mojom       |  8 ++---
> >  src/ipa/raspberrypi/raspberrypi.cpp           |  8 ++---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 36 ++++++++++---------
> >  3 files changed, 27 insertions(+), 25 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.mojom
> b/include/libcamera/ipa/raspberrypi.mojom
> > index bab19a946e18..9c05cc68cceb 100644
> > --- a/include/libcamera/ipa/raspberrypi.mojom
> > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > @@ -16,7 +16,7 @@ enum BufferMask {
> >  const uint32 MaxLsGridSize = 0x8000;
> >
> >  enum ConfigOutputParameters {
> > -     ConfigStaggeredWrite = 0x01,
> > +     ConfigSensorParams = 0x01,
> >  };
> >
> >  struct SensorConfig {
> > @@ -27,8 +27,8 @@ struct SensorConfig {
> >  };
> >
> >  struct ISPConfig {
> > -     uint32 embeddedbufferId;
> > -     uint32 bayerbufferId;
> > +     uint32 embeddedBufferId;
> > +     uint32 bayerBufferId;
> >  };
> >
> >  struct ConfigInput {
> > @@ -126,6 +126,6 @@ interface IPARPiEventInterface {
> >       statsMetadataComplete(uint32 bufferId, ControlList controls);
> >       runIsp(uint32 bufferId);
> >       embeddedComplete(uint32 bufferId);
> > -     setIsp(ControlList controls);
> > +     setIspControls(ControlList controls);
> >       setDelayedControls(ControlList controls);
> >  };
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 81a3195c82e9..974f4ec63058 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -344,7 +344,7 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> >               helper_->GetDelays(exposureDelay, gainDelay);
> >               sensorMetadata = helper_->SensorEmbeddedDataPresent();
> >
> > -             result->params |= ipa::rpi::ConfigStaggeredWrite;
> > +             result->params |= ipa::rpi::ConfigSensorParams;
> >               result->sensorConfig.gainDelay = gainDelay;
> >               result->sensorConfig.exposureDelay = exposureDelay;
> >               result->sensorConfig.vblank = exposureDelay;
> > @@ -447,11 +447,11 @@ void IPARPi::signalIspPrepare(const
> ipa::rpi::ISPConfig &data)
> >        * avoid running the control algos for a few frames in case
> >        * they are "unreliable".
> >        */
> > -     prepareISP(data.embeddedbufferId);
> > +     prepareISP(data.embeddedBufferId);
> >       frameCount_++;
> >
> >       /* Ready to push the input buffer into the ISP. */
> > -     runIsp.emit(data.bayerbufferId & ipa::rpi::MaskID);
> > +     runIsp.emit(data.bayerBufferId & ipa::rpi::MaskID);
> >  }
> >
> >  void IPARPi::reportMetadata()
> > @@ -968,7 +968,7 @@ void IPARPi::prepareISP(unsigned int bufferId)
> >                       applyDPC(dpcStatus, ctrls);
> >
> >               if (!ctrls.empty())
> > -                     setIsp.emit(ctrls);
> > +                     setIspControls.emit(ctrls);
> >       }
> >  }
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 15aa600ed581..8770ae66a21a 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -152,7 +152,7 @@ public:
> >       void statsMetadataComplete(uint32_t bufferId, const ControlList
> &controls);
> >       void runIsp(uint32_t bufferId);
> >       void embeddedComplete(uint32_t bufferId);
> > -     void setIsp(const ControlList &controls);
> > +     void setIspControls(const ControlList &controls);
> >       void setDelayedControls(const ControlList &controls);
> >
> >       /* bufferComplete signal handlers. */
> > @@ -1172,7 +1172,7 @@ int RPiCameraData::loadIPA()
> >       ipa_->statsMetadataComplete.connect(this,
> &RPiCameraData::statsMetadataComplete);
> >       ipa_->runIsp.connect(this, &RPiCameraData::runIsp);
> >       ipa_->embeddedComplete.connect(this,
> &RPiCameraData::embeddedComplete);
> > -     ipa_->setIsp.connect(this, &RPiCameraData::setIsp);
> > +     ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);
> >       ipa_->setDelayedControls.connect(this,
> &RPiCameraData::setDelayedControls);
> >
> >       IPASettings settings(ipa_->configurationFile(sensor_->model() +
> ".json"));
> > @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
> >               return -EPIPE;
> >       }
> >
> > -     if (result.params & ipa::rpi::ConfigStaggeredWrite) {
> > +     if (result.params & ipa::rpi::ConfigSensorParams) {
> >               /*
> >                * Setup our delayed control writer with the sensor default
> >                * gain and exposure delays.
> > @@ -1281,7 +1281,7 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
> >  void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const
> ControlList &controls)
> >  {
> >       if (state_ == State::Stopped)
> > -             handleState();
> > +             return;
>
> I thought handleState() still needs to be called in this (and below) case?
>

I thought so too :)

But if you have a look at handleState(), in the case State::Stopped, it
simply breaks and returns doing nothing.
So, replacing by a single return is sufficient.  Clearly it did do
something in the past, but we should be ok without
calling handleState now.

Regards,
Naush



>
>
> Paul
>
> >
> >       FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
> >
> > @@ -1314,7 +1314,7 @@ void RPiCameraData::statsMetadataComplete(uint32_t
> bufferId, const ControlList &
> >  void RPiCameraData::runIsp(uint32_t bufferId)
> >  {
> >       if (state_ == State::Stopped)
> > -             handleState();
> > +             return;
> >
> >       FrameBuffer *buffer =
> unicam_[Unicam::Image].getBuffers().at(bufferId);
> >
> > @@ -1329,26 +1329,28 @@ void RPiCameraData::runIsp(uint32_t bufferId)
> >  void RPiCameraData::embeddedComplete(uint32_t bufferId)
> >  {
> >       if (state_ == State::Stopped)
> > -             handleState();
> > +             return;
> >
> >       FrameBuffer *buffer =
> unicam_[Unicam::Embedded].getBuffers().at(bufferId);
> >       handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> >       handleState();
> >  }
> >
> > -void RPiCameraData::setIsp(const ControlList &controls)
> > +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();
> > +     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();
> > +             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);
> > +             ControlValue c(Span<const uint8_t>{
> reinterpret_cast<uint8_t *>(&ls),
> > +                                                 sizeof(ls) });
> > +             ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);
> > +     }
> >
> >       isp_[Isp::Input].dev()->setControls(&ctrls);
> >       handleState();
> > @@ -1692,8 +1694,8 @@ void RPiCameraData::tryRunPipeline()
> >                       << " Embedded buffer id: " << embeddedId;
> >
> >       ipa::rpi::ISPConfig ispPrepare;
> > -     ispPrepare.embeddedbufferId = ipa::rpi::MaskEmbeddedData |
> embeddedId;
> > -     ispPrepare.bayerbufferId = ipa::rpi::MaskBayerData | bayerId;
> > +     ispPrepare.embeddedBufferId = ipa::rpi::MaskEmbeddedData |
> embeddedId;
> > +     ispPrepare.bayerBufferId = ipa::rpi::MaskBayerData | bayerId;
> >       ipa_->signalIspPrepare(ispPrepare);
> >  }
> >
> > --
> > 2.25.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
Paul Elder Feb. 17, 2021, 10:28 a.m. UTC | #3 
Hi Naush,

On Wed, Feb 17, 2021 at 10:25:29AM +0000, Naushir Patuck wrote:
> Hi Paul,
> 
> 
> On Wed, 17 Feb 2021 at 10:18, <paul.elder@ideasonboard.com> wrote:
> 
>     Hi Naush,
> 
>     On Wed, Feb 17, 2021 at 10:08:50AM +0000, Naushir Patuck wrote:
>     > This commit addresses a few fixes and tidy-ups after the IPAInterface
>     > rework:
>     > - Rename ConfigStaggeredWrite -> ConfigSensorParams
>     > - Rename setIsp -> setIspControls
>     > - Switch to camel case for ISPConfig::embeddedbufferId and
>     > ISPConfig::bayerbufferId.
>     > - Signal handlers statsMetadataComplete(), runISP(), embeddedComplete()
>     > should only run when state != Stopped. This matches the behavior of
>     > the original code. Without this, start/stop cycles may cause errors.
>     > - In setIspControls(), only update the LS config (with the fd) when a LS
>     > control is present.
>     >
>     > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
>     > Fixes: e201cb4f5450 ("libcamera: IPAInterface: Replace C API with the new
>     C++-only API")
>     > ---
>     >  include/libcamera/ipa/raspberrypi.mojom       |  8 ++---
>     >  src/ipa/raspberrypi/raspberrypi.cpp           |  8 ++---
>     >  .../pipeline/raspberrypi/raspberrypi.cpp      | 36 ++++++++++---------
>     >  3 files changed, 27 insertions(+), 25 deletions(-)
>     >
>     > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/
>     ipa/raspberrypi.mojom
>     > index bab19a946e18..9c05cc68cceb 100644
>     > --- a/include/libcamera/ipa/raspberrypi.mojom
>     > +++ b/include/libcamera/ipa/raspberrypi.mojom
>     > @@ -16,7 +16,7 @@ enum BufferMask {
>     >  const uint32 MaxLsGridSize = 0x8000;
>     > 
>     >  enum ConfigOutputParameters {
>     > -     ConfigStaggeredWrite = 0x01,
>     > +     ConfigSensorParams = 0x01,
>     >  };
>     > 
>     >  struct SensorConfig {
>     > @@ -27,8 +27,8 @@ struct SensorConfig {
>     >  };
>     > 
>     >  struct ISPConfig {
>     > -     uint32 embeddedbufferId;
>     > -     uint32 bayerbufferId;
>     > +     uint32 embeddedBufferId;
>     > +     uint32 bayerBufferId;
>     >  };
>     > 
>     >  struct ConfigInput {
>     > @@ -126,6 +126,6 @@ interface IPARPiEventInterface {
>     >       statsMetadataComplete(uint32 bufferId, ControlList controls);
>     >       runIsp(uint32 bufferId);
>     >       embeddedComplete(uint32 bufferId);
>     > -     setIsp(ControlList controls);
>     > +     setIspControls(ControlList controls);
>     >       setDelayedControls(ControlList controls);
>     >  };
>     > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/
>     raspberrypi.cpp
>     > index 81a3195c82e9..974f4ec63058 100644
>     > --- a/src/ipa/raspberrypi/raspberrypi.cpp
>     > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>     > @@ -344,7 +344,7 @@ void IPARPi::configure(const CameraSensorInfo &
>     sensorInfo,
>     >               helper_->GetDelays(exposureDelay, gainDelay);
>     >               sensorMetadata = helper_->SensorEmbeddedDataPresent();
>     > 
>     > -             result->params |= ipa::rpi::ConfigStaggeredWrite;
>     > +             result->params |= ipa::rpi::ConfigSensorParams;
>     >               result->sensorConfig.gainDelay = gainDelay;
>     >               result->sensorConfig.exposureDelay = exposureDelay;
>     >               result->sensorConfig.vblank = exposureDelay;
>     > @@ -447,11 +447,11 @@ void IPARPi::signalIspPrepare(const
>     ipa::rpi::ISPConfig &data)
>     >        * avoid running the control algos for a few frames in case
>     >        * they are "unreliable".
>     >        */
>     > -     prepareISP(data.embeddedbufferId);
>     > +     prepareISP(data.embeddedBufferId);
>     >       frameCount_++;
>     > 
>     >       /* Ready to push the input buffer into the ISP. */
>     > -     runIsp.emit(data.bayerbufferId & ipa::rpi::MaskID);
>     > +     runIsp.emit(data.bayerBufferId & ipa::rpi::MaskID);
>     >  }
>     > 
>     >  void IPARPi::reportMetadata()
>     > @@ -968,7 +968,7 @@ void IPARPi::prepareISP(unsigned int bufferId)
>     >                       applyDPC(dpcStatus, ctrls);
>     > 
>     >               if (!ctrls.empty())
>     > -                     setIsp.emit(ctrls);
>     > +                     setIspControls.emit(ctrls);
>     >       }
>     >  }
>     > 
>     > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/
>     libcamera/pipeline/raspberrypi/raspberrypi.cpp
>     > index 15aa600ed581..8770ae66a21a 100644
>     > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>     > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>     > @@ -152,7 +152,7 @@ public:
>     >       void statsMetadataComplete(uint32_t bufferId, const ControlList &
>     controls);
>     >       void runIsp(uint32_t bufferId);
>     >       void embeddedComplete(uint32_t bufferId);
>     > -     void setIsp(const ControlList &controls);
>     > +     void setIspControls(const ControlList &controls);
>     >       void setDelayedControls(const ControlList &controls);
>     > 
>     >       /* bufferComplete signal handlers. */
>     > @@ -1172,7 +1172,7 @@ int RPiCameraData::loadIPA()
>     >       ipa_->statsMetadataComplete.connect(this, &
>     RPiCameraData::statsMetadataComplete);
>     >       ipa_->runIsp.connect(this, &RPiCameraData::runIsp);
>     >       ipa_->embeddedComplete.connect(this, &
>     RPiCameraData::embeddedComplete);
>     > -     ipa_->setIsp.connect(this, &RPiCameraData::setIsp);
>     > +     ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);
>     >       ipa_->setDelayedControls.connect(this, &
>     RPiCameraData::setDelayedControls);
>     > 
>     >       IPASettings settings(ipa_->configurationFile(sensor_->model() +
>     ".json"));
>     > @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const
>     CameraConfiguration *config)
>     >               return -EPIPE;
>     >       }
>     > 
>     > -     if (result.params & ipa::rpi::ConfigStaggeredWrite) {
>     > +     if (result.params & ipa::rpi::ConfigSensorParams) {
>     >               /*
>     >                * Setup our delayed control writer with the sensor default
>     >                * gain and exposure delays.
>     > @@ -1281,7 +1281,7 @@ int RPiCameraData::configureIPA(const
>     CameraConfiguration *config)
>     >  void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const
>     ControlList &controls)
>     >  {
>     >       if (state_ == State::Stopped)
>     > -             handleState();
>     > +             return;
> 
>     I thought handleState() still needs to be called in this (and below) case?
> 
> 
> I thought so too :)
> 
> But if you have a look at handleState(), in the case State::Stopped, it simply
> breaks and returns doing nothing.
> So, replacing by a single return is sufficient.  Clearly it did do something in
> the past, but we should be ok without
> calling handleState now.

Ah, I see. I just saw your reply to v1 too :)

Well that's nice, it makes everything cleaner.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>


> 
>     > 
>     >       FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
>     > 
>     > @@ -1314,7 +1314,7 @@ void RPiCameraData::statsMetadataComplete(uint32_t
>     bufferId, const ControlList &
>     >  void RPiCameraData::runIsp(uint32_t bufferId)
>     >  {
>     >       if (state_ == State::Stopped)
>     > -             handleState();
>     > +             return;
>     > 
>     >       FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at
>     (bufferId);
>     > 
>     > @@ -1329,26 +1329,28 @@ void RPiCameraData::runIsp(uint32_t bufferId)
>     >  void RPiCameraData::embeddedComplete(uint32_t bufferId)
>     >  {
>     >       if (state_ == State::Stopped)
>     > -             handleState();
>     > +             return;
>     > 
>     >       FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at
>     (bufferId);
>     >       handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
>     >       handleState();
>     >  }
>     > 
>     > -void RPiCameraData::setIsp(const ControlList &controls)
>     > +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();
>     > +     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();
>     > +             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);
>     > +             ControlValue c(Span<const uint8_t>{ reinterpret_cast
>     <uint8_t *>(&ls),
>     > +                                                 sizeof(ls) });
>     > +             ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);
>     > +     }
>     > 
>     >       isp_[Isp::Input].dev()->setControls(&ctrls);
>     >       handleState();
>     > @@ -1692,8 +1694,8 @@ void RPiCameraData::tryRunPipeline()
>     >                       << " Embedded buffer id: " << embeddedId;
>     > 
>     >       ipa::rpi::ISPConfig ispPrepare;
>     > -     ispPrepare.embeddedbufferId = ipa::rpi::MaskEmbeddedData |
>     embeddedId;
>     > -     ispPrepare.bayerbufferId = ipa::rpi::MaskBayerData | bayerId;
>     > +     ispPrepare.embeddedBufferId = ipa::rpi::MaskEmbeddedData |
>     embeddedId;
>     > +     ispPrepare.bayerBufferId = ipa::rpi::MaskBayerData | bayerId;
>     >       ipa_->signalIspPrepare(ispPrepare);
>     >  }
>     > 
>     > --
>     > 2.25.1
>     >
>     > _______________________________________________
>     > libcamera-devel mailing list
>     > libcamera-devel@lists.libcamera.org
>     > https://lists.libcamera.org/listinfo/libcamera-devel
>
David Plowman Feb. 17, 2021, 11:18 a.m. UTC | #4 
Hi Naush

Thanks for these patches. They fix an intermittent crash I was
experiencing when trying to stop libcamera (in fact exactly as
predicted in the commit message in relation to the signal handlers),
thus:

Tested-by: David Plowman <david.plowman@raspberrypi.com>

Best regards
David

On Wed, 17 Feb 2021 at 10:29, <paul.elder@ideasonboard.com> wrote:
>
> Hi Naush,
>
> On Wed, Feb 17, 2021 at 10:25:29AM +0000, Naushir Patuck wrote:
> > Hi Paul,
> >
> >
> > On Wed, 17 Feb 2021 at 10:18, <paul.elder@ideasonboard.com> wrote:
> >
> >     Hi Naush,
> >
> >     On Wed, Feb 17, 2021 at 10:08:50AM +0000, Naushir Patuck wrote:
> >     > This commit addresses a few fixes and tidy-ups after the IPAInterface
> >     > rework:
> >     > - Rename ConfigStaggeredWrite -> ConfigSensorParams
> >     > - Rename setIsp -> setIspControls
> >     > - Switch to camel case for ISPConfig::embeddedbufferId and
> >     > ISPConfig::bayerbufferId.
> >     > - Signal handlers statsMetadataComplete(), runISP(), embeddedComplete()
> >     > should only run when state != Stopped. This matches the behavior of
> >     > the original code. Without this, start/stop cycles may cause errors.
> >     > - In setIspControls(), only update the LS config (with the fd) when a LS
> >     > control is present.
> >     >
> >     > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> >     > Fixes: e201cb4f5450 ("libcamera: IPAInterface: Replace C API with the new
> >     C++-only API")
> >     > ---
> >     >  include/libcamera/ipa/raspberrypi.mojom       |  8 ++---
> >     >  src/ipa/raspberrypi/raspberrypi.cpp           |  8 ++---
> >     >  .../pipeline/raspberrypi/raspberrypi.cpp      | 36 ++++++++++---------
> >     >  3 files changed, 27 insertions(+), 25 deletions(-)
> >     >
> >     > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/
> >     ipa/raspberrypi.mojom
> >     > index bab19a946e18..9c05cc68cceb 100644
> >     > --- a/include/libcamera/ipa/raspberrypi.mojom
> >     > +++ b/include/libcamera/ipa/raspberrypi.mojom
> >     > @@ -16,7 +16,7 @@ enum BufferMask {
> >     >  const uint32 MaxLsGridSize = 0x8000;
> >     >
> >     >  enum ConfigOutputParameters {
> >     > -     ConfigStaggeredWrite = 0x01,
> >     > +     ConfigSensorParams = 0x01,
> >     >  };
> >     >
> >     >  struct SensorConfig {
> >     > @@ -27,8 +27,8 @@ struct SensorConfig {
> >     >  };
> >     >
> >     >  struct ISPConfig {
> >     > -     uint32 embeddedbufferId;
> >     > -     uint32 bayerbufferId;
> >     > +     uint32 embeddedBufferId;
> >     > +     uint32 bayerBufferId;
> >     >  };
> >     >
> >     >  struct ConfigInput {
> >     > @@ -126,6 +126,6 @@ interface IPARPiEventInterface {
> >     >       statsMetadataComplete(uint32 bufferId, ControlList controls);
> >     >       runIsp(uint32 bufferId);
> >     >       embeddedComplete(uint32 bufferId);
> >     > -     setIsp(ControlList controls);
> >     > +     setIspControls(ControlList controls);
> >     >       setDelayedControls(ControlList controls);
> >     >  };
> >     > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/
> >     raspberrypi.cpp
> >     > index 81a3195c82e9..974f4ec63058 100644
> >     > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> >     > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> >     > @@ -344,7 +344,7 @@ void IPARPi::configure(const CameraSensorInfo &
> >     sensorInfo,
> >     >               helper_->GetDelays(exposureDelay, gainDelay);
> >     >               sensorMetadata = helper_->SensorEmbeddedDataPresent();
> >     >
> >     > -             result->params |= ipa::rpi::ConfigStaggeredWrite;
> >     > +             result->params |= ipa::rpi::ConfigSensorParams;
> >     >               result->sensorConfig.gainDelay = gainDelay;
> >     >               result->sensorConfig.exposureDelay = exposureDelay;
> >     >               result->sensorConfig.vblank = exposureDelay;
> >     > @@ -447,11 +447,11 @@ void IPARPi::signalIspPrepare(const
> >     ipa::rpi::ISPConfig &data)
> >     >        * avoid running the control algos for a few frames in case
> >     >        * they are "unreliable".
> >     >        */
> >     > -     prepareISP(data.embeddedbufferId);
> >     > +     prepareISP(data.embeddedBufferId);
> >     >       frameCount_++;
> >     >
> >     >       /* Ready to push the input buffer into the ISP. */
> >     > -     runIsp.emit(data.bayerbufferId & ipa::rpi::MaskID);
> >     > +     runIsp.emit(data.bayerBufferId & ipa::rpi::MaskID);
> >     >  }
> >     >
> >     >  void IPARPi::reportMetadata()
> >     > @@ -968,7 +968,7 @@ void IPARPi::prepareISP(unsigned int bufferId)
> >     >                       applyDPC(dpcStatus, ctrls);
> >     >
> >     >               if (!ctrls.empty())
> >     > -                     setIsp.emit(ctrls);
> >     > +                     setIspControls.emit(ctrls);
> >     >       }
> >     >  }
> >     >
> >     > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/
> >     libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >     > index 15aa600ed581..8770ae66a21a 100644
> >     > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >     > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >     > @@ -152,7 +152,7 @@ public:
> >     >       void statsMetadataComplete(uint32_t bufferId, const ControlList &
> >     controls);
> >     >       void runIsp(uint32_t bufferId);
> >     >       void embeddedComplete(uint32_t bufferId);
> >     > -     void setIsp(const ControlList &controls);
> >     > +     void setIspControls(const ControlList &controls);
> >     >       void setDelayedControls(const ControlList &controls);
> >     >
> >     >       /* bufferComplete signal handlers. */
> >     > @@ -1172,7 +1172,7 @@ int RPiCameraData::loadIPA()
> >     >       ipa_->statsMetadataComplete.connect(this, &
> >     RPiCameraData::statsMetadataComplete);
> >     >       ipa_->runIsp.connect(this, &RPiCameraData::runIsp);
> >     >       ipa_->embeddedComplete.connect(this, &
> >     RPiCameraData::embeddedComplete);
> >     > -     ipa_->setIsp.connect(this, &RPiCameraData::setIsp);
> >     > +     ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);
> >     >       ipa_->setDelayedControls.connect(this, &
> >     RPiCameraData::setDelayedControls);
> >     >
> >     >       IPASettings settings(ipa_->configurationFile(sensor_->model() +
> >     ".json"));
> >     > @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const
> >     CameraConfiguration *config)
> >     >               return -EPIPE;
> >     >       }
> >     >
> >     > -     if (result.params & ipa::rpi::ConfigStaggeredWrite) {
> >     > +     if (result.params & ipa::rpi::ConfigSensorParams) {
> >     >               /*
> >     >                * Setup our delayed control writer with the sensor default
> >     >                * gain and exposure delays.
> >     > @@ -1281,7 +1281,7 @@ int RPiCameraData::configureIPA(const
> >     CameraConfiguration *config)
> >     >  void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const
> >     ControlList &controls)
> >     >  {
> >     >       if (state_ == State::Stopped)
> >     > -             handleState();
> >     > +             return;
> >
> >     I thought handleState() still needs to be called in this (and below) case?
> >
> >
> > I thought so too :)
> >
> > But if you have a look at handleState(), in the case State::Stopped, it simply
> > breaks and returns doing nothing.
> > So, replacing by a single return is sufficient.  Clearly it did do something in
> > the past, but we should be ok without
> > calling handleState now.
>
> Ah, I see. I just saw your reply to v1 too :)
>
> Well that's nice, it makes everything cleaner.
>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
>
>
> >
> >     >
> >     >       FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
> >     >
> >     > @@ -1314,7 +1314,7 @@ void RPiCameraData::statsMetadataComplete(uint32_t
> >     bufferId, const ControlList &
> >     >  void RPiCameraData::runIsp(uint32_t bufferId)
> >     >  {
> >     >       if (state_ == State::Stopped)
> >     > -             handleState();
> >     > +             return;
> >     >
> >     >       FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at
> >     (bufferId);
> >     >
> >     > @@ -1329,26 +1329,28 @@ void RPiCameraData::runIsp(uint32_t bufferId)
> >     >  void RPiCameraData::embeddedComplete(uint32_t bufferId)
> >     >  {
> >     >       if (state_ == State::Stopped)
> >     > -             handleState();
> >     > +             return;
> >     >
> >     >       FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at
> >     (bufferId);
> >     >       handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> >     >       handleState();
> >     >  }
> >     >
> >     > -void RPiCameraData::setIsp(const ControlList &controls)
> >     > +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();
> >     > +     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();
> >     > +             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);
> >     > +             ControlValue c(Span<const uint8_t>{ reinterpret_cast
> >     <uint8_t *>(&ls),
> >     > +                                                 sizeof(ls) });
> >     > +             ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);
> >     > +     }
> >     >
> >     >       isp_[Isp::Input].dev()->setControls(&ctrls);
> >     >       handleState();
> >     > @@ -1692,8 +1694,8 @@ void RPiCameraData::tryRunPipeline()
> >     >                       << " Embedded buffer id: " << embeddedId;
> >     >
> >     >       ipa::rpi::ISPConfig ispPrepare;
> >     > -     ispPrepare.embeddedbufferId = ipa::rpi::MaskEmbeddedData |
> >     embeddedId;
> >     > -     ispPrepare.bayerbufferId = ipa::rpi::MaskBayerData | bayerId;
> >     > +     ispPrepare.embeddedBufferId = ipa::rpi::MaskEmbeddedData |
> >     embeddedId;
> >     > +     ispPrepare.bayerBufferId = ipa::rpi::MaskBayerData | bayerId;
> >     >       ipa_->signalIspPrepare(ispPrepare);
> >     >  }
> >     >
> >     > --
> >     > 2.25.1
> >     >
> >     > _______________________________________________
> >     > libcamera-devel mailing list
> >     > libcamera-devel@lists.libcamera.org
> >     > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Naushir Patuck Feb. 18, 2021, 10:19 a.m. UTC | #5 
Hi all,

Would I be able to get another review on this commit please?  Without this
commit, the code on top-of-tree can cause crashes at runtime.

Many thanks,
Naush


On Wed, 17 Feb 2021 at 10:08, Naushir Patuck <naush@raspberrypi.com> wrote:

> This commit addresses a few fixes and tidy-ups after the IPAInterface
> rework:
> - Rename ConfigStaggeredWrite -> ConfigSensorParams
> - Rename setIsp -> setIspControls
> - Switch to camel case for ISPConfig::embeddedbufferId and
> ISPConfig::bayerbufferId.
> - Signal handlers statsMetadataComplete(), runISP(), embeddedComplete()
> should only run when state != Stopped. This matches the behavior of
> the original code. Without this, start/stop cycles may cause errors.
> - In setIspControls(), only update the LS config (with the fd) when a LS
> control is present.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Fixes: e201cb4f5450 ("libcamera: IPAInterface: Replace C API with the new
> C++-only API")
> ---
>  include/libcamera/ipa/raspberrypi.mojom       |  8 ++---
>  src/ipa/raspberrypi/raspberrypi.cpp           |  8 ++---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 36 ++++++++++---------
>  3 files changed, 27 insertions(+), 25 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.mojom
> b/include/libcamera/ipa/raspberrypi.mojom
> index bab19a946e18..9c05cc68cceb 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -16,7 +16,7 @@ enum BufferMask {
>  const uint32 MaxLsGridSize = 0x8000;
>
>  enum ConfigOutputParameters {
> -       ConfigStaggeredWrite = 0x01,
> +       ConfigSensorParams = 0x01,
>  };
>
>  struct SensorConfig {
> @@ -27,8 +27,8 @@ struct SensorConfig {
>  };
>
>  struct ISPConfig {
> -       uint32 embeddedbufferId;
> -       uint32 bayerbufferId;
> +       uint32 embeddedBufferId;
> +       uint32 bayerBufferId;
>  };
>
>  struct ConfigInput {
> @@ -126,6 +126,6 @@ interface IPARPiEventInterface {
>         statsMetadataComplete(uint32 bufferId, ControlList controls);
>         runIsp(uint32 bufferId);
>         embeddedComplete(uint32 bufferId);
> -       setIsp(ControlList controls);
> +       setIspControls(ControlList controls);
>         setDelayedControls(ControlList controls);
>  };
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> index 81a3195c82e9..974f4ec63058 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -344,7 +344,7 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
>                 helper_->GetDelays(exposureDelay, gainDelay);
>                 sensorMetadata = helper_->SensorEmbeddedDataPresent();
>
> -               result->params |= ipa::rpi::ConfigStaggeredWrite;
> +               result->params |= ipa::rpi::ConfigSensorParams;
>                 result->sensorConfig.gainDelay = gainDelay;
>                 result->sensorConfig.exposureDelay = exposureDelay;
>                 result->sensorConfig.vblank = exposureDelay;
> @@ -447,11 +447,11 @@ void IPARPi::signalIspPrepare(const
> ipa::rpi::ISPConfig &data)
>          * avoid running the control algos for a few frames in case
>          * they are "unreliable".
>          */
> -       prepareISP(data.embeddedbufferId);
> +       prepareISP(data.embeddedBufferId);
>         frameCount_++;
>
>         /* Ready to push the input buffer into the ISP. */
> -       runIsp.emit(data.bayerbufferId & ipa::rpi::MaskID);
> +       runIsp.emit(data.bayerBufferId & ipa::rpi::MaskID);
>  }
>
>  void IPARPi::reportMetadata()
> @@ -968,7 +968,7 @@ void IPARPi::prepareISP(unsigned int bufferId)
>                         applyDPC(dpcStatus, ctrls);
>
>                 if (!ctrls.empty())
> -                       setIsp.emit(ctrls);
> +                       setIspControls.emit(ctrls);
>         }
>  }
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 15aa600ed581..8770ae66a21a 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -152,7 +152,7 @@ public:
>         void statsMetadataComplete(uint32_t bufferId, const ControlList
> &controls);
>         void runIsp(uint32_t bufferId);
>         void embeddedComplete(uint32_t bufferId);
> -       void setIsp(const ControlList &controls);
> +       void setIspControls(const ControlList &controls);
>         void setDelayedControls(const ControlList &controls);
>
>         /* bufferComplete signal handlers. */
> @@ -1172,7 +1172,7 @@ int RPiCameraData::loadIPA()
>         ipa_->statsMetadataComplete.connect(this,
> &RPiCameraData::statsMetadataComplete);
>         ipa_->runIsp.connect(this, &RPiCameraData::runIsp);
>         ipa_->embeddedComplete.connect(this,
> &RPiCameraData::embeddedComplete);
> -       ipa_->setIsp.connect(this, &RPiCameraData::setIsp);
> +       ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);
>         ipa_->setDelayedControls.connect(this,
> &RPiCameraData::setDelayedControls);
>
>         IPASettings settings(ipa_->configurationFile(sensor_->model() +
> ".json"));
> @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
>                 return -EPIPE;
>         }
>
> -       if (result.params & ipa::rpi::ConfigStaggeredWrite) {
> +       if (result.params & ipa::rpi::ConfigSensorParams) {
>                 /*
>                  * Setup our delayed control writer with the sensor default
>                  * gain and exposure delays.
> @@ -1281,7 +1281,7 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
>  void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const
> ControlList &controls)
>  {
>         if (state_ == State::Stopped)
> -               handleState();
> +               return;
>
>         FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
>
> @@ -1314,7 +1314,7 @@ void RPiCameraData::statsMetadataComplete(uint32_t
> bufferId, const ControlList &
>  void RPiCameraData::runIsp(uint32_t bufferId)
>  {
>         if (state_ == State::Stopped)
> -               handleState();
> +               return;
>
>         FrameBuffer *buffer =
> unicam_[Unicam::Image].getBuffers().at(bufferId);
>
> @@ -1329,26 +1329,28 @@ void RPiCameraData::runIsp(uint32_t bufferId)
>  void RPiCameraData::embeddedComplete(uint32_t bufferId)
>  {
>         if (state_ == State::Stopped)
> -               handleState();
> +               return;
>
>         FrameBuffer *buffer =
> unicam_[Unicam::Embedded].getBuffers().at(bufferId);
>         handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
>         handleState();
>  }
>
> -void RPiCameraData::setIsp(const ControlList &controls)
> +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();
> +       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();
> +               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);
> +               ControlValue c(Span<const uint8_t>{
> reinterpret_cast<uint8_t *>(&ls),
> +                                                   sizeof(ls) });
> +               ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);
> +       }
>
>         isp_[Isp::Input].dev()->setControls(&ctrls);
>         handleState();
> @@ -1692,8 +1694,8 @@ void RPiCameraData::tryRunPipeline()
>                         << " Embedded buffer id: " << embeddedId;
>
>         ipa::rpi::ISPConfig ispPrepare;
> -       ispPrepare.embeddedbufferId = ipa::rpi::MaskEmbeddedData |
> embeddedId;
> -       ispPrepare.bayerbufferId = ipa::rpi::MaskBayerData | bayerId;
> +       ispPrepare.embeddedBufferId = ipa::rpi::MaskEmbeddedData |
> embeddedId;
> +       ispPrepare.bayerBufferId = ipa::rpi::MaskBayerData | bayerId;
>         ipa_->signalIspPrepare(ispPrepare);
>  }
>
> --
> 2.25.1
>
>
Kieran Bingham Feb. 18, 2021, 11:25 a.m. UTC | #6 
Hi Naush,

On 17/02/2021 10:08, Naushir Patuck wrote:
> This commit addresses a few fixes and tidy-ups after the IPAInterface
> rework:
> - Rename ConfigStaggeredWrite -> ConfigSensorParams
> - Rename setIsp -> setIspControls
> - Switch to camel case for ISPConfig::embeddedbufferId and
> ISPConfig::bayerbufferId.
> - Signal handlers statsMetadataComplete(), runISP(), embeddedComplete()
> should only run when state != Stopped. This matches the behavior of
> the original code. Without this, start/stop cycles may cause errors.
> - In setIspControls(), only update the LS config (with the fd) when a LS
> control is present.


That's quite a lot of change for a single patch, but I don't think
there's a point for splitting this out now, especially as it's required
to fix a crash.


> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Fixes: e201cb4f5450 ("libcamera: IPAInterface: Replace C API with the new C++-only API")
> ---
>  include/libcamera/ipa/raspberrypi.mojom       |  8 ++---
>  src/ipa/raspberrypi/raspberrypi.cpp           |  8 ++---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 36 ++++++++++---------
>  3 files changed, 27 insertions(+), 25 deletions(-)
> 
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index bab19a946e18..9c05cc68cceb 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -16,7 +16,7 @@ enum BufferMask {
>  const uint32 MaxLsGridSize = 0x8000;
>  
>  enum ConfigOutputParameters {
> -	ConfigStaggeredWrite = 0x01,
> +	ConfigSensorParams = 0x01,
>  };
>  
>  struct SensorConfig {
> @@ -27,8 +27,8 @@ struct SensorConfig {
>  };
>  
>  struct ISPConfig {
> -	uint32 embeddedbufferId;
> -	uint32 bayerbufferId;
> +	uint32 embeddedBufferId;
> +	uint32 bayerBufferId;
>  };
>  
>  struct ConfigInput {
> @@ -126,6 +126,6 @@ interface IPARPiEventInterface {
>  	statsMetadataComplete(uint32 bufferId, ControlList controls);
>  	runIsp(uint32 bufferId);
>  	embeddedComplete(uint32 bufferId);
> -	setIsp(ControlList controls);
> +	setIspControls(ControlList controls);
>  	setDelayedControls(ControlList controls);
>  };
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 81a3195c82e9..974f4ec63058 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -344,7 +344,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		helper_->GetDelays(exposureDelay, gainDelay);
>  		sensorMetadata = helper_->SensorEmbeddedDataPresent();
>  
> -		result->params |= ipa::rpi::ConfigStaggeredWrite;
> +		result->params |= ipa::rpi::ConfigSensorParams;
>  		result->sensorConfig.gainDelay = gainDelay;
>  		result->sensorConfig.exposureDelay = exposureDelay;
>  		result->sensorConfig.vblank = exposureDelay;
> @@ -447,11 +447,11 @@ void IPARPi::signalIspPrepare(const ipa::rpi::ISPConfig &data)
>  	 * avoid running the control algos for a few frames in case
>  	 * they are "unreliable".
>  	 */
> -	prepareISP(data.embeddedbufferId);
> +	prepareISP(data.embeddedBufferId);
>  	frameCount_++;
>  
>  	/* Ready to push the input buffer into the ISP. */
> -	runIsp.emit(data.bayerbufferId & ipa::rpi::MaskID);
> +	runIsp.emit(data.bayerBufferId & ipa::rpi::MaskID);
>  }
>  
>  void IPARPi::reportMetadata()
> @@ -968,7 +968,7 @@ void IPARPi::prepareISP(unsigned int bufferId)
>  			applyDPC(dpcStatus, ctrls);
>  
>  		if (!ctrls.empty())
> -			setIsp.emit(ctrls);
> +			setIspControls.emit(ctrls);
>  	}
>  }
>  
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 15aa600ed581..8770ae66a21a 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -152,7 +152,7 @@ public:
>  	void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);
>  	void runIsp(uint32_t bufferId);
>  	void embeddedComplete(uint32_t bufferId);
> -	void setIsp(const ControlList &controls);
> +	void setIspControls(const ControlList &controls);
>  	void setDelayedControls(const ControlList &controls);
>  
>  	/* bufferComplete signal handlers. */
> @@ -1172,7 +1172,7 @@ int RPiCameraData::loadIPA()
>  	ipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete);
>  	ipa_->runIsp.connect(this, &RPiCameraData::runIsp);
>  	ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);
> -	ipa_->setIsp.connect(this, &RPiCameraData::setIsp);
> +	ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);
>  	ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);
>  
>  	IPASettings settings(ipa_->configurationFile(sensor_->model() + ".json"));
> @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  		return -EPIPE;
>  	}
>  
> -	if (result.params & ipa::rpi::ConfigStaggeredWrite) {
> +	if (result.params & ipa::rpi::ConfigSensorParams) {
>  		/*
>  		 * Setup our delayed control writer with the sensor default
>  		 * gain and exposure delays.
> @@ -1281,7 +1281,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)
>  {
>  	if (state_ == State::Stopped)
> -		handleState();
> +		return;
>  
>  	FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
>  
> @@ -1314,7 +1314,7 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &
>  void RPiCameraData::runIsp(uint32_t bufferId)
>  {
>  	if (state_ == State::Stopped)
> -		handleState();
> +		return;
>  
>  	FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);
>  
> @@ -1329,26 +1329,28 @@ void RPiCameraData::runIsp(uint32_t bufferId)
>  void RPiCameraData::embeddedComplete(uint32_t bufferId)
>  {
>  	if (state_ == State::Stopped)
> -		handleState();
> +		return;
>  
>  	FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);
>  	handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
>  	handleState();
>  }
>  
> -void RPiCameraData::setIsp(const ControlList &controls)
> +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();
> +	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();
> +		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);
> +		ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ls),
> +						    sizeof(ls) });
> +		ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);
> +	}

I assume it's this hunk which fixes the bug?
Everything else seems to be a rename or the handleState changes perhaps
are a no-effect change?

As this is a bug fix, it would have been nice to see that split so the
reader can understand what code change actually causes the fix.

The renames are trivial, so they're fine, and this seesm to make sense
as it protects against accessing a control which isn't set.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>



>  
>  	isp_[Isp::Input].dev()->setControls(&ctrls);
>  	handleState();
> @@ -1692,8 +1694,8 @@ void RPiCameraData::tryRunPipeline()
>  			<< " Embedded buffer id: " << embeddedId;
>  
>  	ipa::rpi::ISPConfig ispPrepare;
> -	ispPrepare.embeddedbufferId = ipa::rpi::MaskEmbeddedData | embeddedId;
> -	ispPrepare.bayerbufferId = ipa::rpi::MaskBayerData | bayerId;
> +	ispPrepare.embeddedBufferId = ipa::rpi::MaskEmbeddedData | embeddedId;
> +	ispPrepare.bayerBufferId = ipa::rpi::MaskBayerData | bayerId;
>  	ipa_->signalIspPrepare(ispPrepare);
>  }
>  
>
Naushir Patuck Feb. 18, 2021, 11:35 a.m. UTC | #7 
Hi Kieran,

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

> Hi Naush,
>
> On 17/02/2021 10:08, Naushir Patuck wrote:
> > This commit addresses a few fixes and tidy-ups after the IPAInterface
> > rework:
> > - Rename ConfigStaggeredWrite -> ConfigSensorParams
> > - Rename setIsp -> setIspControls
> > - Switch to camel case for ISPConfig::embeddedbufferId and
> > ISPConfig::bayerbufferId.
> > - Signal handlers statsMetadataComplete(), runISP(), embeddedComplete()
> > should only run when state != Stopped. This matches the behavior of
> > the original code. Without this, start/stop cycles may cause errors.
> > - In setIspControls(), only update the LS config (with the fd) when a LS
> > control is present.
>
>
> That's quite a lot of change for a single patch, but I don't think
> there's a point for splitting this out now, especially as it's required
> to fix a crash.
>

I do agree.  If you prefer, I could split this into code changes (for
fixing the crash) and the other cosmetic changes.



>
>
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Fixes: e201cb4f5450 ("libcamera: IPAInterface: Replace C API with the
> new C++-only API")
> > ---
> >  include/libcamera/ipa/raspberrypi.mojom       |  8 ++---
> >  src/ipa/raspberrypi/raspberrypi.cpp           |  8 ++---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 36 ++++++++++---------
> >  3 files changed, 27 insertions(+), 25 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.mojom
> b/include/libcamera/ipa/raspberrypi.mojom
> > index bab19a946e18..9c05cc68cceb 100644
> > --- a/include/libcamera/ipa/raspberrypi.mojom
> > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > @@ -16,7 +16,7 @@ enum BufferMask {
> >  const uint32 MaxLsGridSize = 0x8000;
> >
> >  enum ConfigOutputParameters {
> > -     ConfigStaggeredWrite = 0x01,
> > +     ConfigSensorParams = 0x01,
> >  };
> >
> >  struct SensorConfig {
> > @@ -27,8 +27,8 @@ struct SensorConfig {
> >  };
> >
> >  struct ISPConfig {
> > -     uint32 embeddedbufferId;
> > -     uint32 bayerbufferId;
> > +     uint32 embeddedBufferId;
> > +     uint32 bayerBufferId;
> >  };
> >
> >  struct ConfigInput {
> > @@ -126,6 +126,6 @@ interface IPARPiEventInterface {
> >       statsMetadataComplete(uint32 bufferId, ControlList controls);
> >       runIsp(uint32 bufferId);
> >       embeddedComplete(uint32 bufferId);
> > -     setIsp(ControlList controls);
> > +     setIspControls(ControlList controls);
> >       setDelayedControls(ControlList controls);
> >  };
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 81a3195c82e9..974f4ec63058 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -344,7 +344,7 @@ void IPARPi::configure(const CameraSensorInfo
> &sensorInfo,
> >               helper_->GetDelays(exposureDelay, gainDelay);
> >               sensorMetadata = helper_->SensorEmbeddedDataPresent();
> >
> > -             result->params |= ipa::rpi::ConfigStaggeredWrite;
> > +             result->params |= ipa::rpi::ConfigSensorParams;
> >               result->sensorConfig.gainDelay = gainDelay;
> >               result->sensorConfig.exposureDelay = exposureDelay;
> >               result->sensorConfig.vblank = exposureDelay;
> > @@ -447,11 +447,11 @@ void IPARPi::signalIspPrepare(const
> ipa::rpi::ISPConfig &data)
> >        * avoid running the control algos for a few frames in case
> >        * they are "unreliable".
> >        */
> > -     prepareISP(data.embeddedbufferId);
> > +     prepareISP(data.embeddedBufferId);
> >       frameCount_++;
> >
> >       /* Ready to push the input buffer into the ISP. */
> > -     runIsp.emit(data.bayerbufferId & ipa::rpi::MaskID);
> > +     runIsp.emit(data.bayerBufferId & ipa::rpi::MaskID);
> >  }
> >
> >  void IPARPi::reportMetadata()
> > @@ -968,7 +968,7 @@ void IPARPi::prepareISP(unsigned int bufferId)
> >                       applyDPC(dpcStatus, ctrls);
> >
> >               if (!ctrls.empty())
> > -                     setIsp.emit(ctrls);
> > +                     setIspControls.emit(ctrls);
> >       }
> >  }
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 15aa600ed581..8770ae66a21a 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -152,7 +152,7 @@ public:
> >       void statsMetadataComplete(uint32_t bufferId, const ControlList
> &controls);
> >       void runIsp(uint32_t bufferId);
> >       void embeddedComplete(uint32_t bufferId);
> > -     void setIsp(const ControlList &controls);
> > +     void setIspControls(const ControlList &controls);
> >       void setDelayedControls(const ControlList &controls);
> >
> >       /* bufferComplete signal handlers. */
> > @@ -1172,7 +1172,7 @@ int RPiCameraData::loadIPA()
> >       ipa_->statsMetadataComplete.connect(this,
> &RPiCameraData::statsMetadataComplete);
> >       ipa_->runIsp.connect(this, &RPiCameraData::runIsp);
> >       ipa_->embeddedComplete.connect(this,
> &RPiCameraData::embeddedComplete);
> > -     ipa_->setIsp.connect(this, &RPiCameraData::setIsp);
> > +     ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);
> >       ipa_->setDelayedControls.connect(this,
> &RPiCameraData::setDelayedControls);
> >
> >       IPASettings settings(ipa_->configurationFile(sensor_->model() +
> ".json"));
> > @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
> >               return -EPIPE;
> >       }
> >
> > -     if (result.params & ipa::rpi::ConfigStaggeredWrite) {
> > +     if (result.params & ipa::rpi::ConfigSensorParams) {
> >               /*
> >                * Setup our delayed control writer with the sensor default
> >                * gain and exposure delays.
> > @@ -1281,7 +1281,7 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
> >  void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const
> ControlList &controls)
> >  {
> >       if (state_ == State::Stopped)
> > -             handleState();
> > +             return;
> >
> >       FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
> >
> > @@ -1314,7 +1314,7 @@ void RPiCameraData::statsMetadataComplete(uint32_t
> bufferId, const ControlList &
> >  void RPiCameraData::runIsp(uint32_t bufferId)
> >  {
> >       if (state_ == State::Stopped)
> > -             handleState();
> > +             return;
> >
> >       FrameBuffer *buffer =
> unicam_[Unicam::Image].getBuffers().at(bufferId);
> >
> > @@ -1329,26 +1329,28 @@ void RPiCameraData::runIsp(uint32_t bufferId)
> >  void RPiCameraData::embeddedComplete(uint32_t bufferId)
> >  {
> >       if (state_ == State::Stopped)
> > -             handleState();
> > +             return;
> >
> >       FrameBuffer *buffer =
> unicam_[Unicam::Embedded].getBuffers().at(bufferId);
> >       handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
> >       handleState();
> >  }
> >
> > -void RPiCameraData::setIsp(const ControlList &controls)
> > +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();
> > +     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();
> > +             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);
> > +             ControlValue c(Span<const uint8_t>{
> reinterpret_cast<uint8_t *>(&ls),
> > +                                                 sizeof(ls) });
> > +             ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);
> > +     }
>
> I assume it's this hunk which fixes the bug?
> Everything else seems to be a rename or the handleState changes perhaps
> are a no-effect change?
>

Actually not this one, but the hunks just above that fix the bug.  Let me
split this change up and I will resubmit a v3.  If it's ok, I will add your
R-b tags to them.

Regards,
Naush



>
> As this is a bug fix, it would have been nice to see that split so the
> reader can understand what code change actually causes the fix.
>
> The renames are trivial, so they're fine, and this seesm to make sense
> as it protects against accessing a control which isn't set.
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>
>
> >
> >       isp_[Isp::Input].dev()->setControls(&ctrls);
> >       handleState();
> > @@ -1692,8 +1694,8 @@ void RPiCameraData::tryRunPipeline()
> >                       << " Embedded buffer id: " << embeddedId;
> >
> >       ipa::rpi::ISPConfig ispPrepare;
> > -     ispPrepare.embeddedbufferId = ipa::rpi::MaskEmbeddedData |
> embeddedId;
> > -     ispPrepare.bayerbufferId = ipa::rpi::MaskBayerData | bayerId;
> > +     ispPrepare.embeddedBufferId = ipa::rpi::MaskEmbeddedData |
> embeddedId;
> > +     ispPrepare.bayerBufferId = ipa::rpi::MaskBayerData | bayerId;
> >       ipa_->signalIspPrepare(ispPrepare);
> >  }
> >
> >
>
> --
> Regards
> --
> Kieran
>
Kieran Bingham Feb. 18, 2021, 11:38 a.m. UTC | #8 
On 18/02/2021 11:35, Naushir Patuck wrote:
> Hi Kieran,
> 
> On Thu, 18 Feb 2021 at 11:25, Kieran Bingham
> <kieran.bingham@ideasonboard.com
> <mailto:kieran.bingham@ideasonboard.com>> wrote:
> 
>     Hi Naush,
> 
>     On 17/02/2021 10:08, Naushir Patuck wrote:
>     > This commit addresses a few fixes and tidy-ups after the IPAInterface
>     > rework:
>     > - Rename ConfigStaggeredWrite -> ConfigSensorParams
>     > - Rename setIsp -> setIspControls
>     > - Switch to camel case for ISPConfig::embeddedbufferId and
>     > ISPConfig::bayerbufferId.
>     > - Signal handlers statsMetadataComplete(), runISP(),
>     embeddedComplete()
>     > should only run when state != Stopped. This matches the behavior of
>     > the original code. Without this, start/stop cycles may cause errors.
>     > - In setIspControls(), only update the LS config (with the fd)
>     when a LS
>     > control is present.
> 
> 
>     That's quite a lot of change for a single patch, but I don't think
>     there's a point for splitting this out now, especially as it's required
>     to fix a crash.
> 
> 
> I do agree.  If you prefer, I could split this into code changes (for
> fixing the crash) and the other cosmetic changes.
> 
>  
> 
> 
> 
>     > Signed-off-by: Naushir Patuck <naush@raspberrypi.com
>     <mailto:naush@raspberrypi.com>>
>     > Fixes: e201cb4f5450 ("libcamera: IPAInterface: Replace C API with
>     the new C++-only API")
>     > ---
>     >  include/libcamera/ipa/raspberrypi.mojom       |  8 ++---
>     >  src/ipa/raspberrypi/raspberrypi.cpp           |  8 ++---
>     >  .../pipeline/raspberrypi/raspberrypi.cpp      | 36
>     ++++++++++---------
>     >  3 files changed, 27 insertions(+), 25 deletions(-)
>     >
>     > diff --git a/include/libcamera/ipa/raspberrypi.mojom
>     b/include/libcamera/ipa/raspberrypi.mojom
>     > index bab19a946e18..9c05cc68cceb 100644
>     > --- a/include/libcamera/ipa/raspberrypi.mojom
>     > +++ b/include/libcamera/ipa/raspberrypi.mojom
>     > @@ -16,7 +16,7 @@ enum BufferMask {
>     >  const uint32 MaxLsGridSize = 0x8000;
>     > 
>     >  enum ConfigOutputParameters {
>     > -     ConfigStaggeredWrite = 0x01,
>     > +     ConfigSensorParams = 0x01,
>     >  };
>     > 
>     >  struct SensorConfig {
>     > @@ -27,8 +27,8 @@ struct SensorConfig {
>     >  };
>     > 
>     >  struct ISPConfig {
>     > -     uint32 embeddedbufferId;
>     > -     uint32 bayerbufferId;
>     > +     uint32 embeddedBufferId;
>     > +     uint32 bayerBufferId;
>     >  };
>     > 
>     >  struct ConfigInput {
>     > @@ -126,6 +126,6 @@ interface IPARPiEventInterface {
>     >       statsMetadataComplete(uint32 bufferId, ControlList controls);
>     >       runIsp(uint32 bufferId);
>     >       embeddedComplete(uint32 bufferId);
>     > -     setIsp(ControlList controls);
>     > +     setIspControls(ControlList controls);
>     >       setDelayedControls(ControlList controls);
>     >  };
>     > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
>     b/src/ipa/raspberrypi/raspberrypi.cpp
>     > index 81a3195c82e9..974f4ec63058 100644
>     > --- a/src/ipa/raspberrypi/raspberrypi.cpp
>     > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>     > @@ -344,7 +344,7 @@ void IPARPi::configure(const CameraSensorInfo
>     &sensorInfo,
>     >               helper_->GetDelays(exposureDelay, gainDelay);
>     >               sensorMetadata = helper_->SensorEmbeddedDataPresent();
>     > 
>     > -             result->params |= ipa::rpi::ConfigStaggeredWrite;
>     > +             result->params |= ipa::rpi::ConfigSensorParams;
>     >               result->sensorConfig.gainDelay = gainDelay;
>     >               result->sensorConfig.exposureDelay = exposureDelay;
>     >               result->sensorConfig.vblank = exposureDelay;
>     > @@ -447,11 +447,11 @@ void IPARPi::signalIspPrepare(const
>     ipa::rpi::ISPConfig &data)
>     >        * avoid running the control algos for a few frames in case
>     >        * they are "unreliable".
>     >        */
>     > -     prepareISP(data.embeddedbufferId);
>     > +     prepareISP(data.embeddedBufferId);
>     >       frameCount_++;
>     > 
>     >       /* Ready to push the input buffer into the ISP. */
>     > -     runIsp.emit(data.bayerbufferId & ipa::rpi::MaskID);
>     > +     runIsp.emit(data.bayerBufferId & ipa::rpi::MaskID);
>     >  }
>     > 
>     >  void IPARPi::reportMetadata()
>     > @@ -968,7 +968,7 @@ void IPARPi::prepareISP(unsigned int bufferId)
>     >                       applyDPC(dpcStatus, ctrls);
>     > 
>     >               if (!ctrls.empty())
>     > -                     setIsp.emit(ctrls);
>     > +                     setIspControls.emit(ctrls);
>     >       }
>     >  }
>     > 
>     > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>     b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>     > index 15aa600ed581..8770ae66a21a 100644
>     > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>     > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>     > @@ -152,7 +152,7 @@ public:
>     >       void statsMetadataComplete(uint32_t bufferId, const
>     ControlList &controls);
>     >       void runIsp(uint32_t bufferId);
>     >       void embeddedComplete(uint32_t bufferId);
>     > -     void setIsp(const ControlList &controls);
>     > +     void setIspControls(const ControlList &controls);
>     >       void setDelayedControls(const ControlList &controls);
>     > 
>     >       /* bufferComplete signal handlers. */
>     > @@ -1172,7 +1172,7 @@ int RPiCameraData::loadIPA()
>     >       ipa_->statsMetadataComplete.connect(this,
>     &RPiCameraData::statsMetadataComplete);
>     >       ipa_->runIsp.connect(this, &RPiCameraData::runIsp);
>     >       ipa_->embeddedComplete.connect(this,
>     &RPiCameraData::embeddedComplete);
>     > -     ipa_->setIsp.connect(this, &RPiCameraData::setIsp);
>     > +     ipa_->setIspControls.connect(this,
>     &RPiCameraData::setIspControls);
>     >       ipa_->setDelayedControls.connect(this,
>     &RPiCameraData::setDelayedControls);
>     > 
>     >       IPASettings
>     settings(ipa_->configurationFile(sensor_->model() + ".json"));
>     > @@ -1241,7 +1241,7 @@ int RPiCameraData::configureIPA(const
>     CameraConfiguration *config)
>     >               return -EPIPE;
>     >       }
>     > 
>     > -     if (result.params & ipa::rpi::ConfigStaggeredWrite) {
>     > +     if (result.params & ipa::rpi::ConfigSensorParams) {
>     >               /*
>     >                * Setup our delayed control writer with the sensor
>     default
>     >                * gain and exposure delays.
>     > @@ -1281,7 +1281,7 @@ int RPiCameraData::configureIPA(const
>     CameraConfiguration *config)
>     >  void RPiCameraData::statsMetadataComplete(uint32_t bufferId,
>     const ControlList &controls)
>     >  {
>     >       if (state_ == State::Stopped)
>     > -             handleState();
>     > +             return;
>     > 
>     >       FrameBuffer *buffer =
>     isp_[Isp::Stats].getBuffers().at(bufferId);
>     > 
>     > @@ -1314,7 +1314,7 @@ void
>     RPiCameraData::statsMetadataComplete(uint32_t bufferId, const
>     ControlList &
>     >  void RPiCameraData::runIsp(uint32_t bufferId)
>     >  {
>     >       if (state_ == State::Stopped)
>     > -             handleState();
>     > +             return;
>     > 
>     >       FrameBuffer *buffer =
>     unicam_[Unicam::Image].getBuffers().at(bufferId);
>     > 
>     > @@ -1329,26 +1329,28 @@ void RPiCameraData::runIsp(uint32_t bufferId)
>     >  void RPiCameraData::embeddedComplete(uint32_t bufferId)
>     >  {
>     >       if (state_ == State::Stopped)
>     > -             handleState();
>     > +             return;
>     > 
>     >       FrameBuffer *buffer =
>     unicam_[Unicam::Embedded].getBuffers().at(bufferId);
>     >       handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
>     >       handleState();
>     >  }
>     > 
>     > -void RPiCameraData::setIsp(const ControlList &controls)
>     > +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();
>     > +     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();
>     > +             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);
>     > +             ControlValue c(Span<const uint8_t>{
>     reinterpret_cast<uint8_t *>(&ls),
>     > +                                                 sizeof(ls) });
>     > +             ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);
>     > +     }
> 
>     I assume it's this hunk which fixes the bug?
>     Everything else seems to be a rename or the handleState changes perhaps
>     are a no-effect change?
> 
> 
> Actually not this one, but the hunks just above that fix the bug.  Let
> me split this change up and I will resubmit a v3.  If it's ok, I will
> add your R-b tags to them.
> 

Ahaha, great, I messed up, so yes - splitting at least functional
changes from cosmetic would be helpful.

Yes, please keep the tag though.

--
Kieran


> Regards,
> Naush
> 
>  
> 
> 
>     As this is a bug fix, it would have been nice to see that split so the
>     reader can understand what code change actually causes the fix.
> 
>     The renames are trivial, so they're fine, and this seesm to make sense
>     as it protects against accessing a control which isn't set.
> 
>     Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com
>     <mailto:kieran.bingham@ideasonboard.com>>
> 
> 
> 
>     > 
>     >       isp_[Isp::Input].dev()->setControls(&ctrls);
>     >       handleState();
>     > @@ -1692,8 +1694,8 @@ void RPiCameraData::tryRunPipeline()
>     >                       << " Embedded buffer id: " << embeddedId;
>     > 
>     >       ipa::rpi::ISPConfig ispPrepare;
>     > -     ispPrepare.embeddedbufferId = ipa::rpi::MaskEmbeddedData |
>     embeddedId;
>     > -     ispPrepare.bayerbufferId = ipa::rpi::MaskBayerData | bayerId;
>     > +     ispPrepare.embeddedBufferId = ipa::rpi::MaskEmbeddedData |
>     embeddedId;
>     > +     ispPrepare.bayerBufferId = ipa::rpi::MaskBayerData | bayerId;
>     >       ipa_->signalIspPrepare(ispPrepare);
>     >  }
>     > 
>     >
> 
>     -- 
>     Regards
>     --
>     Kieran
>

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
index bab19a946e18..9c05cc68cceb 100644
--- a/include/libcamera/ipa/raspberrypi.mojom
+++ b/include/libcamera/ipa/raspberrypi.mojom
@@ -16,7 +16,7 @@  enum BufferMask {
 const uint32 MaxLsGridSize = 0x8000;
 
 enum ConfigOutputParameters {
-	ConfigStaggeredWrite = 0x01,
+	ConfigSensorParams = 0x01,
 };
 
 struct SensorConfig {
@@ -27,8 +27,8 @@  struct SensorConfig {
 };
 
 struct ISPConfig {
-	uint32 embeddedbufferId;
-	uint32 bayerbufferId;
+	uint32 embeddedBufferId;
+	uint32 bayerBufferId;
 };
 
 struct ConfigInput {
@@ -126,6 +126,6 @@  interface IPARPiEventInterface {
 	statsMetadataComplete(uint32 bufferId, ControlList controls);
 	runIsp(uint32 bufferId);
 	embeddedComplete(uint32 bufferId);
-	setIsp(ControlList controls);
+	setIspControls(ControlList controls);
 	setDelayedControls(ControlList controls);
 };
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 81a3195c82e9..974f4ec63058 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -344,7 +344,7 @@  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
 		helper_->GetDelays(exposureDelay, gainDelay);
 		sensorMetadata = helper_->SensorEmbeddedDataPresent();
 
-		result->params |= ipa::rpi::ConfigStaggeredWrite;
+		result->params |= ipa::rpi::ConfigSensorParams;
 		result->sensorConfig.gainDelay = gainDelay;
 		result->sensorConfig.exposureDelay = exposureDelay;
 		result->sensorConfig.vblank = exposureDelay;
@@ -447,11 +447,11 @@  void IPARPi::signalIspPrepare(const ipa::rpi::ISPConfig &data)
 	 * avoid running the control algos for a few frames in case
 	 * they are "unreliable".
 	 */
-	prepareISP(data.embeddedbufferId);
+	prepareISP(data.embeddedBufferId);
 	frameCount_++;
 
 	/* Ready to push the input buffer into the ISP. */
-	runIsp.emit(data.bayerbufferId & ipa::rpi::MaskID);
+	runIsp.emit(data.bayerBufferId & ipa::rpi::MaskID);
 }
 
 void IPARPi::reportMetadata()
@@ -968,7 +968,7 @@  void IPARPi::prepareISP(unsigned int bufferId)
 			applyDPC(dpcStatus, ctrls);
 
 		if (!ctrls.empty())
-			setIsp.emit(ctrls);
+			setIspControls.emit(ctrls);
 	}
 }
 
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index 15aa600ed581..8770ae66a21a 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -152,7 +152,7 @@  public:
 	void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);
 	void runIsp(uint32_t bufferId);
 	void embeddedComplete(uint32_t bufferId);
-	void setIsp(const ControlList &controls);
+	void setIspControls(const ControlList &controls);
 	void setDelayedControls(const ControlList &controls);
 
 	/* bufferComplete signal handlers. */
@@ -1172,7 +1172,7 @@  int RPiCameraData::loadIPA()
 	ipa_->statsMetadataComplete.connect(this, &RPiCameraData::statsMetadataComplete);
 	ipa_->runIsp.connect(this, &RPiCameraData::runIsp);
 	ipa_->embeddedComplete.connect(this, &RPiCameraData::embeddedComplete);
-	ipa_->setIsp.connect(this, &RPiCameraData::setIsp);
+	ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);
 	ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);
 
 	IPASettings settings(ipa_->configurationFile(sensor_->model() + ".json"));
@@ -1241,7 +1241,7 @@  int RPiCameraData::configureIPA(const CameraConfiguration *config)
 		return -EPIPE;
 	}
 
-	if (result.params & ipa::rpi::ConfigStaggeredWrite) {
+	if (result.params & ipa::rpi::ConfigSensorParams) {
 		/*
 		 * Setup our delayed control writer with the sensor default
 		 * gain and exposure delays.
@@ -1281,7 +1281,7 @@  int RPiCameraData::configureIPA(const CameraConfiguration *config)
 void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)
 {
 	if (state_ == State::Stopped)
-		handleState();
+		return;
 
 	FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
 
@@ -1314,7 +1314,7 @@  void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &
 void RPiCameraData::runIsp(uint32_t bufferId)
 {
 	if (state_ == State::Stopped)
-		handleState();
+		return;
 
 	FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);
 
@@ -1329,26 +1329,28 @@  void RPiCameraData::runIsp(uint32_t bufferId)
 void RPiCameraData::embeddedComplete(uint32_t bufferId)
 {
 	if (state_ == State::Stopped)
-		handleState();
+		return;
 
 	FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);
 	handleStreamBuffer(buffer, &unicam_[Unicam::Embedded]);
 	handleState();
 }
 
-void RPiCameraData::setIsp(const ControlList &controls)
+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();
+	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();
+		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);
+		ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ls),
+						    sizeof(ls) });
+		ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);
+	}
 
 	isp_[Isp::Input].dev()->setControls(&ctrls);
 	handleState();
@@ -1692,8 +1694,8 @@  void RPiCameraData::tryRunPipeline()
 			<< " Embedded buffer id: " << embeddedId;
 
 	ipa::rpi::ISPConfig ispPrepare;
-	ispPrepare.embeddedbufferId = ipa::rpi::MaskEmbeddedData | embeddedId;
-	ispPrepare.bayerbufferId = ipa::rpi::MaskBayerData | bayerId;
+	ispPrepare.embeddedBufferId = ipa::rpi::MaskEmbeddedData | embeddedId;
+	ispPrepare.bayerBufferId = ipa::rpi::MaskBayerData | bayerId;
 	ipa_->signalIspPrepare(ispPrepare);
 }