| Message ID | 20200704005227.21782-8-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Rejected |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, 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@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. > } > } > > 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 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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(); > > } > >
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; } } if (state_ == State::Stopped) - goto done; + return; /* * 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(); }
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@ideasonboard.com> --- src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)