| Message ID | 20200628231934.29025-10-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your work. On 2020-06-29 02:19:34 +0300, Laurent Pinchart wrote: > The RPI_IPA_ACTION_V4L2_SET_ISP is handled regardless of the pipeline > handler state, but is only generated by the IPA when the pipeline is > running. Don't handle it in the stopped state, which simplifies the > handling of actions. > > Additionally, handleState() is a no-op when the state is State::Stopped, > so don't call it in that case. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 25 +++++++------------ > 1 file changed, 9 insertions(+), 16 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 137e07379cf5..2a5d27fefe0c 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1190,24 +1190,12 @@ int RPiCameraData::configureIPA() > void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData &action) > { > /* > - * The following actions can be handled when the pipeline handler is in > - * a stopped state. > + * Actions are only handled when the pipeline handler is not in a > + * stopped state. > */ > - switch (action.operation) { > - case RPI_IPA_ACTION_V4L2_SET_ISP: { > - ControlList controls = action.controls[0]; > - isp_[Isp::Input].dev()->setControls(&controls); > - goto done; > - } > - } > - > if (state_ == State::Stopped) > - goto done; > + return; > > - /* > - * The following actions must not be handled when the pipeline handler > - * is in a stopped state. > - */ > switch (action.operation) { > case RPI_IPA_ACTION_V4L2_SET_STAGGERED: { > const ControlList &controls = action.controls[0]; > @@ -1216,6 +1204,12 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData > break; > } > > + case RPI_IPA_ACTION_V4L2_SET_ISP: { > + ControlList controls = action.controls[0]; > + isp_[Isp::Input].dev()->setControls(&controls); > + break; > + } > + > case RPI_IPA_ACTION_STATS_METADATA_COMPLETE: { > unsigned int bufferId = action.data[0]; > FrameBuffer *buffer = isp_[Isp::Stats].getBuffers()->at(bufferId).get(); > @@ -1253,7 +1247,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 Laurent, Thank you for the patch. On Mon, 29 Jun 2020 at 00:19, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > The RPI_IPA_ACTION_V4L2_SET_ISP is handled regardless of the pipeline > handler state, but is only generated by the IPA when the pipeline is > running. Don't handle it in the stopped state, which simplifies the > handling of actions. > > Additionally, handleState() is a no-op when the state is State::Stopped, > so don't call it in that case. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 25 +++++++------------ > 1 file changed, 9 insertions(+), 16 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 137e07379cf5..2a5d27fefe0c 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1190,24 +1190,12 @@ int RPiCameraData::configureIPA() > void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData &action) > { > /* > - * The following actions can be handled when the pipeline handler is in > - * a stopped state. > + * Actions are only handled when the pipeline handler is not in a > + * stopped state. > */ > - switch (action.operation) { > - case RPI_IPA_ACTION_V4L2_SET_ISP: { > - ControlList controls = action.controls[0]; > - isp_[Isp::Input].dev()->setControls(&controls); > - goto done; > - } > - } > - > if (state_ == State::Stopped) > - goto done; > + return; Not sure about this one. In a separate discussion, we are considering passing in a Request before the camera starts. In these cases, we do want to allow the IPA to program the ISP and sensor with parameters while still in a stopped state, correct? I need to think this one through a bit more. Regards, Naush > > - /* > - * The following actions must not be handled when the pipeline handler > - * is in a stopped state. > - */ > switch (action.operation) { > case RPI_IPA_ACTION_V4L2_SET_STAGGERED: { > const ControlList &controls = action.controls[0]; > @@ -1216,6 +1204,12 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData > break; > } > > + case RPI_IPA_ACTION_V4L2_SET_ISP: { > + ControlList controls = action.controls[0]; > + isp_[Isp::Input].dev()->setControls(&controls); > + break; > + } > + > case RPI_IPA_ACTION_STATS_METADATA_COMPLETE: { > unsigned int bufferId = action.data[0]; > FrameBuffer *buffer = isp_[Isp::Stats].getBuffers()->at(bufferId).get(); > @@ -1253,7 +1247,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData > break; > } > > -done: > handleState(); > } > > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Mon, Jun 29, 2020 at 06:08:21PM +0100, Naushir Patuck wrote: > On Mon, 29 Jun 2020 at 00:19, Laurent Pinchart wrote: > > > > The RPI_IPA_ACTION_V4L2_SET_ISP is handled regardless of the pipeline > > handler state, but is only generated by the IPA when the pipeline is > > running. Don't handle it in the stopped state, which simplifies the > > handling of actions. > > > > Additionally, handleState() is a no-op when the state is State::Stopped, > > so don't call it in that case. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 25 +++++++------------ > > 1 file changed, 9 insertions(+), 16 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 137e07379cf5..2a5d27fefe0c 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -1190,24 +1190,12 @@ int RPiCameraData::configureIPA() > > void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData &action) > > { > > /* > > - * The following actions can be handled when the pipeline handler is in > > - * a stopped state. > > + * Actions are only handled when the pipeline handler is not in a > > + * stopped state. > > */ > > - switch (action.operation) { > > - case RPI_IPA_ACTION_V4L2_SET_ISP: { > > - ControlList controls = action.controls[0]; > > - isp_[Isp::Input].dev()->setControls(&controls); > > - goto done; > > - } > > - } > > - > > if (state_ == State::Stopped) > > - goto done; > > + return; > > Not sure about this one. In a separate discussion, we are considering > passing in a Request before the camera starts. In these cases, we do > want to allow the IPA to program the ISP and sensor with parameters > while still in a stopped state, correct? I need to think this one > through a bit more. You're right. Let's delay this patch until that discussion comes to a conclusion, there's no urgency. The s/goto done/return/ could probably still be applied already, but I don't think it deserves a patch to fix it now. > > - /* > > - * The following actions must not be handled when the pipeline handler > > - * is in a stopped state. > > - */ > > switch (action.operation) { > > case RPI_IPA_ACTION_V4L2_SET_STAGGERED: { > > const ControlList &controls = action.controls[0]; > > @@ -1216,6 +1204,12 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData > > break; > > } > > > > + case RPI_IPA_ACTION_V4L2_SET_ISP: { > > + ControlList controls = action.controls[0]; > > + isp_[Isp::Input].dev()->setControls(&controls); > > + break; > > + } > > + > > case RPI_IPA_ACTION_STATS_METADATA_COMPLETE: { > > unsigned int bufferId = action.data[0]; > > FrameBuffer *buffer = isp_[Isp::Stats].getBuffers()->at(bufferId).get(); > > @@ -1253,7 +1247,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 137e07379cf5..2a5d27fefe0c 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -1190,24 +1190,12 @@ int RPiCameraData::configureIPA() void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData &action) { /* - * The following actions can be handled when the pipeline handler is in - * a stopped state. + * Actions are only handled when the pipeline handler is not in a + * stopped state. */ - switch (action.operation) { - case RPI_IPA_ACTION_V4L2_SET_ISP: { - ControlList controls = action.controls[0]; - isp_[Isp::Input].dev()->setControls(&controls); - goto done; - } - } - if (state_ == State::Stopped) - goto done; + return; - /* - * The following actions must not be handled when the pipeline handler - * is in a stopped state. - */ switch (action.operation) { case RPI_IPA_ACTION_V4L2_SET_STAGGERED: { const ControlList &controls = action.controls[0]; @@ -1216,6 +1204,12 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData break; } + case RPI_IPA_ACTION_V4L2_SET_ISP: { + ControlList controls = action.controls[0]; + isp_[Isp::Input].dev()->setControls(&controls); + break; + } + case RPI_IPA_ACTION_STATS_METADATA_COMPLETE: { unsigned int bufferId = action.data[0]; FrameBuffer *buffer = isp_[Isp::Stats].getBuffers()->at(bufferId).get(); @@ -1253,7 +1247,6 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData break; } -done: handleState(); }
The RPI_IPA_ACTION_V4L2_SET_ISP is handled regardless of the pipeline handler state, but is only generated by the IPA when the pipeline is running. Don't handle it in the stopped state, which simplifies the handling of actions. Additionally, handleState() is a no-op when the state is State::Stopped, so don't call it in that case. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- .../pipeline/raspberrypi/raspberrypi.cpp | 25 +++++++------------ 1 file changed, 9 insertions(+), 16 deletions(-)