[libcamera-devel] [PATCH v2 07/12] libcamera: pipeline: raspberrypi: Don't call handleState() in stop state

[Laurent Pinchart]

Hi Jacopo,

On Mon, Jul 06, 2020 at 11:00:10AM +0200, Jacopo Mondi wrote:
> On Sat, Jul 04, 2020 at 03:52:22AM +0300, Laurent Pinchart wrote:
> > The handleState() function is a no-op when the state is State::Stopped.
> > Don't call it in that case, which simplifies the caller.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index a05fc68b98fa..74bee6895dcd 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1188,7 +1188,7 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> >  		ControlList controls = action.controls[0];
> >  		if (!staggeredCtrl_.set(controls))
> >  			LOG(RPI, Error) << "V4L2 staggered set failed";
> > -		goto done;
> > +		return;
> >  	}
> >
> >  	case RPI_IPA_ACTION_SET_SENSOR_CONFIG: {
> > @@ -1206,18 +1206,18 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> >  		/* Set the sensor orientation here as well. */
> >  		ControlList controls = action.controls[0];
> >  		unicam_[Unicam::Image].dev()->setControls(&controls);
> > -		goto done;
> > +		return;
> >  	}
> >
> >  	case RPI_IPA_ACTION_V4L2_SET_ISP: {
> >  		ControlList controls = action.controls[0];
> >  		isp_[Isp::Input].dev()->setControls(&controls);
> > -		goto done;
> > +		return;
> 
> I'm sure I don't fully understand the IPA protocol for this device,
> but the comment says this operation *can* be handled with the IPA in
> stopped state, not that the stop state is required. Furthermore, the
> next switch block says the there handled operation *must not* be handled
> in stopped state, which makes me think these ones can but don't
> require to.

You're absolutely right. As this patch will then become a one-liner I
think I'll just drop it.

> >  	}
> >  	}
> >
> >  	if (state_ == State::Stopped)
> > -		goto done;
> > +		return;
> 
> This, instead, is crystal clear :)
> 
> >
> >  	/*
> >  	 * The following actions must not be handled when the pipeline handler
> > @@ -1261,7 +1261,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> >  		break;
> >  	}
> >
> > -done:
> >  	handleState();
> >  }
> >

-- 
Regards,

Laurent Pinchart