[libcamera-devel] [RFCv2 6/7] libcamera: pipeline: vimc: Call IPA start() and stop()

[Laurent Pinchart]

Hi Niklas,

On Thu, Mar 26, 2020 at 10:43:18PM +0100, Niklas Söderlund wrote:
> On 2020-03-26 23:29:23 +0200, Laurent Pinchart wrote:
> > On Thu, Mar 26, 2020 at 05:08:18PM +0100, Niklas Söderlund wrote:
> > > Call the IPA start()/stop() functions before/after the camera is
> > > started. This makes sure the IPA functions properly once thread
> > > management is moved into the start/stop interface.
> > > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > ---
> > >  src/libcamera/pipeline/vimc/vimc.cpp | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > > index b04a9726efa5ade6..c5eea3a01b0e9446 100644
> > > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > > @@ -274,8 +274,15 @@ int PipelineHandlerVimc::start(Camera *camera)
> > >  	if (ret < 0)
> > >  		return ret;
> > >  
> > > +	ret = data->ipa_->start();
> > > +	if (ret) {
> > > +		data->video_->releaseBuffers();
> > > +		return ret;
> > > +	}
> > > +
> > >  	ret = data->video_->streamOn();
> > >  	if (ret < 0) {
> > > +		data->ipa_->stop();
> > >  		data->video_->releaseBuffers();
> > >  		return ret;
> > >  	}
> > > @@ -287,6 +294,7 @@ void PipelineHandlerVimc::stop(Camera *camera)
> > >  {
> > >  	VimcCameraData *data = cameraData(camera);
> > >  	data->video_->streamOff();
> > > +	data->ipa_->stop();
> > 
> > Conceptually speaking I think the IPA should be stopped first (and
> > started last), as I see it "depending" on the operation of the pipeline
> > handler, but that makes absolutely no difference in practice here. It
> > should however be consistent between the rkisp1 and vimc pipeline
> > handlers.
> 
> I wrestled a bit with this when writing the IPA. As the ipa_ is stored 
> in the base class CameraData shall we move the ipa_-{start,stop}() calls 
> to the camera class start/stop to force them being called uniformly for 
> all pipelines?

I think there's room for standardization, but I don't think it belongs
to the Camera class, as the IPA is for the pipeline handler to control.
CameraData is a bit of a misnomer, it should be PipelineCameraData or
something similar, but that's longer.

I'd like, when the IPAs will stabilize a bit, to see how we could have
helpers in the base PipelineHandler class, but I don't think now is the
right time.

> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > 
> > >  	data->video_->releaseBuffers();
> > >  }
> > >  

-- 
Regards,

Laurent Pinchart