[libcamera-devel,v2,12/12] libcamera: pipeline: raspberrypi: Start IPA when starting camera
diff mbox series

Message ID 20200704005227.21782-13-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • Support passing custom data to IPA configure()
Related show

Commit Message

Laurent Pinchart July 4, 2020, 12:52 a.m. UTC 
The IPA is meant to be started when starting the camera, and stopped
when stopping it. It was so far start early in order to handle the
IPAInterface::processEvent() call related to lens shading table
allocation before IPAInterface::configure() to pass the table to the
IPA. Now that the lens shading table is passed through configure(),
starting the IPA early isn't needed anymore.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 31 ++++++++++---------
 1 file changed, 17 insertions(+), 14 deletions(-)

Comments

Jacopo Mondi July 6, 2020, 9:30 a.m. UTC | #1 
On Sat, Jul 04, 2020 at 03:52:27AM +0300, Laurent Pinchart wrote:
> The IPA is meant to be started when starting the camera, and stopped
> when stopping it. It was so far start early in order to handle the

s/start/started ?

> IPAInterface::processEvent() call related to lens shading table
> allocation before IPAInterface::configure() to pass the table to the
> IPA. Now that the lens shading table is passed through configure(),
> starting the IPA early isn't needed anymore.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 31 ++++++++++---------
>  1 file changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 9babf9f4a19c..c877820a6e1e 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -303,10 +303,6 @@ public:
>  			vcsm_.free(lsTable_);
>  			lsTable_ = nullptr;
>  		}
> -
> -		/* Stop the IPA proxy thread. */
> -		if (ipa_)
> -			ipa_->stop();
>  	}
>
>  	void frameStarted(uint32_t sequence);
> @@ -800,6 +796,16 @@ int PipelineHandlerRPi::start(Camera *camera)
>  	ret = queueAllBuffers(camera);
>  	if (ret) {
>  		LOG(RPI, Error) << "Failed to queue buffers";
> +		stop(camera);
> +		return ret;
> +	}
> +
> +	/* Start the IPA. */
> +	ret = data->ipa_->start();
> +	if (ret) {
> +		LOG(RPI, Error)
> +			<< "Failed to start IPA for " << camera->name();
> +		stop(camera);
>  		return ret;
>  	}
>
> @@ -810,8 +816,10 @@ int PipelineHandlerRPi::start(Camera *camera)
>  	V4L2DeviceFormat sensorFormat;
>  	data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat);
>  	ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);
> -	if (ret)
> +	if (ret) {
> +		stop(camera);

Do we want to stop the whole camera or just the IPA here ?
I think you meant data->ipa_->stop(), right ?

With this fixed
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>  		return ret;
> +	}
>
>  	/* Enable SOF event generation. */
>  	data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);
> @@ -855,6 +863,9 @@ void PipelineHandlerRPi::stop(Camera *camera)
>  	data->bayerQueue_ = std::queue<FrameBuffer *>{};
>  	data->embeddedQueue_ = std::queue<FrameBuffer *>{};
>
> +	/* Stop the IPA. */
> +	data->ipa_->stop();
> +
>  	freeBuffers(camera);
>  }
>
> @@ -1107,15 +1118,7 @@ int RPiCameraData::loadIPA()
>  		.configurationFile = ipa_->configurationFile(sensor_->model() + ".json")
>  	};
>
> -	ipa_->init(settings);
> -
> -	/*
> -	 * Startup the IPA thread now. Without this call, none of the IPA API
> -	 * functions will run.
> -	 *
> -	 * It only gets stopped in the class destructor.
> -	 */
> -	return ipa_->start();
> +	return ipa_->init(settings);
>  }
>
>  int RPiCameraData::configureIPA()
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart July 6, 2020, 11:08 a.m. UTC | #2 
Hi Jacopo,

On Mon, Jul 06, 2020 at 11:30:11AM +0200, Jacopo Mondi wrote:
> On Sat, Jul 04, 2020 at 03:52:27AM +0300, Laurent Pinchart wrote:
> > The IPA is meant to be started when starting the camera, and stopped
> > when stopping it. It was so far start early in order to handle the
> 
> s/start/started ?

Indeed.

> > IPAInterface::processEvent() call related to lens shading table
> > allocation before IPAInterface::configure() to pass the table to the
> > IPA. Now that the lens shading table is passed through configure(),
> > starting the IPA early isn't needed anymore.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 31 ++++++++++---------
> >  1 file changed, 17 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 9babf9f4a19c..c877820a6e1e 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -303,10 +303,6 @@ public:
> >  			vcsm_.free(lsTable_);
> >  			lsTable_ = nullptr;
> >  		}
> > -
> > -		/* Stop the IPA proxy thread. */
> > -		if (ipa_)
> > -			ipa_->stop();
> >  	}
> >
> >  	void frameStarted(uint32_t sequence);
> > @@ -800,6 +796,16 @@ int PipelineHandlerRPi::start(Camera *camera)
> >  	ret = queueAllBuffers(camera);
> >  	if (ret) {
> >  		LOG(RPI, Error) << "Failed to queue buffers";
> > +		stop(camera);
> > +		return ret;
> > +	}
> > +
> > +	/* Start the IPA. */
> > +	ret = data->ipa_->start();
> > +	if (ret) {
> > +		LOG(RPI, Error)
> > +			<< "Failed to start IPA for " << camera->name();
> > +		stop(camera);
> >  		return ret;
> >  	}
> >
> > @@ -810,8 +816,10 @@ int PipelineHandlerRPi::start(Camera *camera)
> >  	V4L2DeviceFormat sensorFormat;
> >  	data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat);
> >  	ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);
> > -	if (ret)
> > +	if (ret) {
> > +		stop(camera);
> 
> Do we want to stop the whole camera or just the IPA here ?
> I think you meant data->ipa_->stop(), right ?

The idea is that the PipelineHandlerRPi::stop() function should clean up
everything that start() may have done, and can be called in start() when
an error occurs. To make this easier I need IPAProxy::stop() to be safe
to call when the IPA is already stopped. I'll send a patch to do so.

> With this fixed
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> >  		return ret;
> > +	}
> >
> >  	/* Enable SOF event generation. */
> >  	data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);
> > @@ -855,6 +863,9 @@ void PipelineHandlerRPi::stop(Camera *camera)
> >  	data->bayerQueue_ = std::queue<FrameBuffer *>{};
> >  	data->embeddedQueue_ = std::queue<FrameBuffer *>{};
> >
> > +	/* Stop the IPA. */
> > +	data->ipa_->stop();
> > +
> >  	freeBuffers(camera);
> >  }
> >
> > @@ -1107,15 +1118,7 @@ int RPiCameraData::loadIPA()
> >  		.configurationFile = ipa_->configurationFile(sensor_->model() + ".json")
> >  	};
> >
> > -	ipa_->init(settings);
> > -
> > -	/*
> > -	 * Startup the IPA thread now. Without this call, none of the IPA API
> > -	 * functions will run.
> > -	 *
> > -	 * It only gets stopped in the class destructor.
> > -	 */
> > -	return ipa_->start();
> > +	return ipa_->init(settings);
> >  }
> >
> >  int RPiCameraData::configureIPA()
Jacopo Mondi July 6, 2020, 11:37 a.m. UTC | #3 
Hi Laurent,

On Mon, Jul 06, 2020 at 02:08:57PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Jul 06, 2020 at 11:30:11AM +0200, Jacopo Mondi wrote:
> > On Sat, Jul 04, 2020 at 03:52:27AM +0300, Laurent Pinchart wrote:
> > > The IPA is meant to be started when starting the camera, and stopped
> > > when stopping it. It was so far start early in order to handle the
> >
> > s/start/started ?
>
> Indeed.
>
> > > IPAInterface::processEvent() call related to lens shading table
> > > allocation before IPAInterface::configure() to pass the table to the
> > > IPA. Now that the lens shading table is passed through configure(),
> > > starting the IPA early isn't needed anymore.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > ---
> > >  .../pipeline/raspberrypi/raspberrypi.cpp      | 31 ++++++++++---------
> > >  1 file changed, 17 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 9babf9f4a19c..c877820a6e1e 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -303,10 +303,6 @@ public:
> > >  			vcsm_.free(lsTable_);
> > >  			lsTable_ = nullptr;
> > >  		}
> > > -
> > > -		/* Stop the IPA proxy thread. */
> > > -		if (ipa_)
> > > -			ipa_->stop();
> > >  	}
> > >
> > >  	void frameStarted(uint32_t sequence);
> > > @@ -800,6 +796,16 @@ int PipelineHandlerRPi::start(Camera *camera)
> > >  	ret = queueAllBuffers(camera);
> > >  	if (ret) {
> > >  		LOG(RPI, Error) << "Failed to queue buffers";
> > > +		stop(camera);
> > > +		return ret;
> > > +	}
> > > +
> > > +	/* Start the IPA. */
> > > +	ret = data->ipa_->start();
> > > +	if (ret) {
> > > +		LOG(RPI, Error)
> > > +			<< "Failed to start IPA for " << camera->name();
> > > +		stop(camera);
> > >  		return ret;
> > >  	}
> > >
> > > @@ -810,8 +816,10 @@ int PipelineHandlerRPi::start(Camera *camera)
> > >  	V4L2DeviceFormat sensorFormat;
> > >  	data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat);
> > >  	ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);
> > > -	if (ret)
> > > +	if (ret) {
> > > +		stop(camera);
> >
> > Do we want to stop the whole camera or just the IPA here ?
> > I think you meant data->ipa_->stop(), right ?
>
> The idea is that the PipelineHandlerRPi::stop() function should clean up
> everything that start() may have done, and can be called in start() when
> an error occurs. To make this easier I need IPAProxy::stop() to be safe
> to call when the IPA is already stopped. I'll send a patch to do so.
>

Does this mean all the error paths in the start() function should call
stop() ?

> > With this fixed
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >

Please reain my tag here!

Thanks
  j

> > >  		return ret;
> > > +	}
> > >
> > >  	/* Enable SOF event generation. */
> > >  	data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);
> > > @@ -855,6 +863,9 @@ void PipelineHandlerRPi::stop(Camera *camera)
> > >  	data->bayerQueue_ = std::queue<FrameBuffer *>{};
> > >  	data->embeddedQueue_ = std::queue<FrameBuffer *>{};
> > >
> > > +	/* Stop the IPA. */
> > > +	data->ipa_->stop();
> > > +
> > >  	freeBuffers(camera);
> > >  }
> > >
> > > @@ -1107,15 +1118,7 @@ int RPiCameraData::loadIPA()
> > >  		.configurationFile = ipa_->configurationFile(sensor_->model() + ".json")
> > >  	};
> > >
> > > -	ipa_->init(settings);
> > > -
> > > -	/*
> > > -	 * Startup the IPA thread now. Without this call, none of the IPA API
> > > -	 * functions will run.
> > > -	 *
> > > -	 * It only gets stopped in the class destructor.
> > > -	 */
> > > -	return ipa_->start();
> > > +	return ipa_->init(settings);
> > >  }
> > >
> > >  int RPiCameraData::configureIPA()
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart July 6, 2020, 11:43 a.m. UTC | #4 
Hi Jacopo,

On Mon, Jul 06, 2020 at 01:37:56PM +0200, Jacopo Mondi wrote:
> On Mon, Jul 06, 2020 at 02:08:57PM +0300, Laurent Pinchart wrote:
> > On Mon, Jul 06, 2020 at 11:30:11AM +0200, Jacopo Mondi wrote:
> >> On Sat, Jul 04, 2020 at 03:52:27AM +0300, Laurent Pinchart wrote:
> >>> The IPA is meant to be started when starting the camera, and stopped
> >>> when stopping it. It was so far start early in order to handle the
> >>
> >> s/start/started ?
> >
> > Indeed.
> >
> >>> IPAInterface::processEvent() call related to lens shading table
> >>> allocation before IPAInterface::configure() to pass the table to the
> >>> IPA. Now that the lens shading table is passed through configure(),
> >>> starting the IPA early isn't needed anymore.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >>> ---
> >>>  .../pipeline/raspberrypi/raspberrypi.cpp      | 31 ++++++++++---------
> >>>  1 file changed, 17 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> index 9babf9f4a19c..c877820a6e1e 100644
> >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> @@ -303,10 +303,6 @@ public:
> >>>  			vcsm_.free(lsTable_);
> >>>  			lsTable_ = nullptr;
> >>>  		}
> >>> -
> >>> -		/* Stop the IPA proxy thread. */
> >>> -		if (ipa_)
> >>> -			ipa_->stop();
> >>>  	}
> >>>
> >>>  	void frameStarted(uint32_t sequence);
> >>> @@ -800,6 +796,16 @@ int PipelineHandlerRPi::start(Camera *camera)
> >>>  	ret = queueAllBuffers(camera);
> >>>  	if (ret) {
> >>>  		LOG(RPI, Error) << "Failed to queue buffers";
> >>> +		stop(camera);
> >>> +		return ret;
> >>> +	}
> >>> +
> >>> +	/* Start the IPA. */
> >>> +	ret = data->ipa_->start();
> >>> +	if (ret) {
> >>> +		LOG(RPI, Error)
> >>> +			<< "Failed to start IPA for " << camera->name();
> >>> +		stop(camera);
> >>>  		return ret;
> >>>  	}
> >>>
> >>> @@ -810,8 +816,10 @@ int PipelineHandlerRPi::start(Camera *camera)
> >>>  	V4L2DeviceFormat sensorFormat;
> >>>  	data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat);
> >>>  	ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);
> >>> -	if (ret)
> >>> +	if (ret) {
> >>> +		stop(camera);
> >>
> >> Do we want to stop the whole camera or just the IPA here ?
> >> I think you meant data->ipa_->stop(), right ?
> >
> > The idea is that the PipelineHandlerRPi::stop() function should clean up
> > everything that start() may have done, and can be called in start() when
> > an error occurs. To make this easier I need IPAProxy::stop() to be safe
> > to call when the IPA is already stopped. I'll send a patch to do so.
> 
> Does this mean all the error paths in the start() function should call
> stop() ?

Yes, that's the idea.

> >> With this fixed
> >> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> Please reain my tag here!
> 
> >>>  		return ret;
> >>> +	}
> >>>
> >>>  	/* Enable SOF event generation. */
> >>>  	data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);
> >>> @@ -855,6 +863,9 @@ void PipelineHandlerRPi::stop(Camera *camera)
> >>>  	data->bayerQueue_ = std::queue<FrameBuffer *>{};
> >>>  	data->embeddedQueue_ = std::queue<FrameBuffer *>{};
> >>>
> >>> +	/* Stop the IPA. */
> >>> +	data->ipa_->stop();
> >>> +
> >>>  	freeBuffers(camera);
> >>>  }
> >>>
> >>> @@ -1107,15 +1118,7 @@ int RPiCameraData::loadIPA()
> >>>  		.configurationFile = ipa_->configurationFile(sensor_->model() + ".json")
> >>>  	};
> >>>
> >>> -	ipa_->init(settings);
> >>> -
> >>> -	/*
> >>> -	 * Startup the IPA thread now. Without this call, none of the IPA API
> >>> -	 * functions will run.
> >>> -	 *
> >>> -	 * It only gets stopped in the class destructor.
> >>> -	 */
> >>> -	return ipa_->start();
> >>> +	return ipa_->init(settings);
> >>>  }
> >>>
> >>>  int RPiCameraData::configureIPA()
Naushir Patuck July 8, 2020, 11:12 a.m. UTC | #5 
Hi Laurent,

Thank you for the patch.

On Sat, 4 Jul 2020 at 01:52, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> The IPA is meant to be started when starting the camera, and stopped
> when stopping it. It was so far start early in order to handle the
> IPAInterface::processEvent() call related to lens shading table
> allocation before IPAInterface::configure() to pass the table to the
> IPA. Now that the lens shading table is passed through configure(),
> starting the IPA early isn't needed anymore.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

One question, what is the overhead of ipa_->start() and ipa_->stop().
If we have to do this on a mode switch preparing for a capture (where
we call pipeline_handler::stop() and pipeline_handler::start()), it
might increase capture latency.  Perhaps this does not matter, or we
can address if it is indeed a problem.  Otherwise,

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>

> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 31 ++++++++++---------
>  1 file changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 9babf9f4a19c..c877820a6e1e 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -303,10 +303,6 @@ public:
>                         vcsm_.free(lsTable_);
>                         lsTable_ = nullptr;
>                 }
> -
> -               /* Stop the IPA proxy thread. */
> -               if (ipa_)
> -                       ipa_->stop();
>         }
>
>         void frameStarted(uint32_t sequence);
> @@ -800,6 +796,16 @@ int PipelineHandlerRPi::start(Camera *camera)
>         ret = queueAllBuffers(camera);
>         if (ret) {
>                 LOG(RPI, Error) << "Failed to queue buffers";
> +               stop(camera);
> +               return ret;
> +       }
> +
> +       /* Start the IPA. */
> +       ret = data->ipa_->start();
> +       if (ret) {
> +               LOG(RPI, Error)
> +                       << "Failed to start IPA for " << camera->name();
> +               stop(camera);
>                 return ret;
>         }
>
> @@ -810,8 +816,10 @@ int PipelineHandlerRPi::start(Camera *camera)
>         V4L2DeviceFormat sensorFormat;
>         data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat);
>         ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);
> -       if (ret)
> +       if (ret) {
> +               stop(camera);
>                 return ret;
> +       }
>
>         /* Enable SOF event generation. */
>         data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);
> @@ -855,6 +863,9 @@ void PipelineHandlerRPi::stop(Camera *camera)
>         data->bayerQueue_ = std::queue<FrameBuffer *>{};
>         data->embeddedQueue_ = std::queue<FrameBuffer *>{};
>
> +       /* Stop the IPA. */
> +       data->ipa_->stop();
> +
>         freeBuffers(camera);
>  }
>
> @@ -1107,15 +1118,7 @@ int RPiCameraData::loadIPA()
>                 .configurationFile = ipa_->configurationFile(sensor_->model() + ".json")
>         };
>
> -       ipa_->init(settings);
> -
> -       /*
> -        * Startup the IPA thread now. Without this call, none of the IPA API
> -        * functions will run.
> -        *
> -        * It only gets stopped in the class destructor.
> -        */
> -       return ipa_->start();
> +       return ipa_->init(settings);
>  }
>
>  int RPiCameraData::configureIPA()
> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart July 8, 2020, 8:46 p.m. UTC | #6 
Hi Naush,

On Wed, Jul 08, 2020 at 12:12:58PM +0100, Naushir Patuck wrote:
> On Sat, 4 Jul 2020 at 01:52, Laurent Pinchart wrote:
> >
> > The IPA is meant to be started when starting the camera, and stopped
> > when stopping it. It was so far start early in order to handle the
> > IPAInterface::processEvent() call related to lens shading table
> > allocation before IPAInterface::configure() to pass the table to the
> > IPA. Now that the lens shading table is passed through configure(),
> > starting the IPA early isn't needed anymore.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> 
> One question, what is the overhead of ipa_->start() and ipa_->stop().
> If we have to do this on a mode switch preparing for a capture (where
> we call pipeline_handler::stop() and pipeline_handler::start()), it
> might increase capture latency.  Perhaps this does not matter, or we
> can address if it is indeed a problem.  Otherwise,

There's a small overhead, but I think it's offset by
IPAInterface::configure() becoming a direct call instead of a
cross-thread call. It may be more of a problem for closed IPA modules
isolated in a separate process though, it's a good point. I could
reconsider that, but I'd rather first align all implementations on the
current model, and then rework start/stop (possibly with the addition of
prepare/finish to handle the expensive operations that are not related
to starting and stopping the camera itself) on top.

> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> 
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 31 ++++++++++---------
> >  1 file changed, 17 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 9babf9f4a19c..c877820a6e1e 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -303,10 +303,6 @@ public:
> >                         vcsm_.free(lsTable_);
> >                         lsTable_ = nullptr;
> >                 }
> > -
> > -               /* Stop the IPA proxy thread. */
> > -               if (ipa_)
> > -                       ipa_->stop();
> >         }
> >
> >         void frameStarted(uint32_t sequence);
> > @@ -800,6 +796,16 @@ int PipelineHandlerRPi::start(Camera *camera)
> >         ret = queueAllBuffers(camera);
> >         if (ret) {
> >                 LOG(RPI, Error) << "Failed to queue buffers";
> > +               stop(camera);
> > +               return ret;
> > +       }
> > +
> > +       /* Start the IPA. */
> > +       ret = data->ipa_->start();
> > +       if (ret) {
> > +               LOG(RPI, Error)
> > +                       << "Failed to start IPA for " << camera->name();
> > +               stop(camera);
> >                 return ret;
> >         }
> >
> > @@ -810,8 +816,10 @@ int PipelineHandlerRPi::start(Camera *camera)
> >         V4L2DeviceFormat sensorFormat;
> >         data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat);
> >         ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);
> > -       if (ret)
> > +       if (ret) {
> > +               stop(camera);
> >                 return ret;
> > +       }
> >
> >         /* Enable SOF event generation. */
> >         data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);
> > @@ -855,6 +863,9 @@ void PipelineHandlerRPi::stop(Camera *camera)
> >         data->bayerQueue_ = std::queue<FrameBuffer *>{};
> >         data->embeddedQueue_ = std::queue<FrameBuffer *>{};
> >
> > +       /* Stop the IPA. */
> > +       data->ipa_->stop();
> > +
> >         freeBuffers(camera);
> >  }
> >
> > @@ -1107,15 +1118,7 @@ int RPiCameraData::loadIPA()
> >                 .configurationFile = ipa_->configurationFile(sensor_->model() + ".json")
> >         };
> >
> > -       ipa_->init(settings);
> > -
> > -       /*
> > -        * Startup the IPA thread now. Without this call, none of the IPA API
> > -        * functions will run.
> > -        *
> > -        * It only gets stopped in the class destructor.
> > -        */
> > -       return ipa_->start();
> > +       return ipa_->init(settings);
> >  }
> >
> >  int RPiCameraData::configureIPA()

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index 9babf9f4a19c..c877820a6e1e 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -303,10 +303,6 @@  public:
 			vcsm_.free(lsTable_);
 			lsTable_ = nullptr;
 		}
-
-		/* Stop the IPA proxy thread. */
-		if (ipa_)
-			ipa_->stop();
 	}
 
 	void frameStarted(uint32_t sequence);
@@ -800,6 +796,16 @@  int PipelineHandlerRPi::start(Camera *camera)
 	ret = queueAllBuffers(camera);
 	if (ret) {
 		LOG(RPI, Error) << "Failed to queue buffers";
+		stop(camera);
+		return ret;
+	}
+
+	/* Start the IPA. */
+	ret = data->ipa_->start();
+	if (ret) {
+		LOG(RPI, Error)
+			<< "Failed to start IPA for " << camera->name();
+		stop(camera);
 		return ret;
 	}
 
@@ -810,8 +816,10 @@  int PipelineHandlerRPi::start(Camera *camera)
 	V4L2DeviceFormat sensorFormat;
 	data->unicam_[Unicam::Image].dev()->getFormat(&sensorFormat);
 	ret = data->isp_[Isp::Input].dev()->setFormat(&sensorFormat);
-	if (ret)
+	if (ret) {
+		stop(camera);
 		return ret;
+	}
 
 	/* Enable SOF event generation. */
 	data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);
@@ -855,6 +863,9 @@  void PipelineHandlerRPi::stop(Camera *camera)
 	data->bayerQueue_ = std::queue<FrameBuffer *>{};
 	data->embeddedQueue_ = std::queue<FrameBuffer *>{};
 
+	/* Stop the IPA. */
+	data->ipa_->stop();
+
 	freeBuffers(camera);
 }
 
@@ -1107,15 +1118,7 @@  int RPiCameraData::loadIPA()
 		.configurationFile = ipa_->configurationFile(sensor_->model() + ".json")
 	};
 
-	ipa_->init(settings);
-
-	/*
-	 * Startup the IPA thread now. Without this call, none of the IPA API
-	 * functions will run.
-	 *
-	 * It only gets stopped in the class destructor.
-	 */
-	return ipa_->start();
+	return ipa_->init(settings);
 }
 
 int RPiCameraData::configureIPA()