[libcamera-devel] [PATCH v1 9/9] libcamera: pipeline: raspberrypi: Don't handle any actions in stop state

[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 at 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();
> >  }
> >

-- 
Regards,

Laurent Pinchart