[libcamera-devel,v1,1/2] pipeline: raspberrypi: Add an error state

17372 diff mbox series

Add an error state used internally in the Raspberry Pi pipeline handler.
Currently this state is never set, but will be in a subsequent commit when a
device timeout has been signalled.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 .../pipeline/raspberrypi/raspberrypi.cpp       | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

Hi Naush,

Quoting Naushir Patuck via libcamera-devel (2022-09-16 11:05:16)
> Add an error state used internally in the Raspberry Pi pipeline handler.
> Currently this state is never set, but will be in a subsequent commit when a
> device timeout has been signalled.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp       | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index b4094898ca6c..d429cb444d58 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -253,7 +253,7 @@ public:
>          * thread. So, we do not need to have any mutex to protect access to any
>          * of the variables below.
>          */
> -       enum class State { Stopped, Idle, Busy, IpaComplete };
> +       enum class State { Stopped, Idle, Busy, IpaComplete, Error };
>         State state_;
>  
>         struct BayerFrame {
> @@ -1109,7 +1109,8 @@ int PipelineHandlerRPi::queueRequestDevice(Camera *camera, Request *request)
>  {
>         RPiCameraData *data = cameraData(camera);
>  
> -       if (data->state_ == RPiCameraData::State::Stopped)
> +       if (data->state_ == RPiCameraData::State::Stopped ||
> +           data->state_ == RPiCameraData::State::Error)

This is fine I think, but is it worth an 'isRunning()' state helper? 

Does the state match the state machine of the Camera object? (I.e.
should we expose the states/helpers more conveniently to the pipeline
handlers so that you don't have to manage it separately?


As this updates the existing state machine though, I think this is
fine...


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>                 return -EINVAL;
>  
>         LOG(RPI, Debug) << "queueRequestDevice: New request.";
> @@ -1708,7 +1709,7 @@ void RPiCameraData::enumerateVideoDevices(MediaLink *link)
>  
>  void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &controls)
>  {
> -       if (state_ == State::Stopped)
> +       if (state_ == State::Stopped || state_ == State::Error)
>                 return;
>  
>         FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
> @@ -1744,7 +1745,7 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &
>  
>  void RPiCameraData::runIsp(uint32_t bufferId)
>  {
> -       if (state_ == State::Stopped)
> +       if (state_ == State::Stopped || state_ == State::Error)
>                 return;
>  
>         FrameBuffer *buffer = unicam_[Unicam::Image].getBuffers().at(bufferId);
> @@ -1759,7 +1760,7 @@ void RPiCameraData::runIsp(uint32_t bufferId)
>  
>  void RPiCameraData::embeddedComplete(uint32_t bufferId)
>  {
> -       if (state_ == State::Stopped)
> +       if (state_ == State::Stopped || state_ == State::Error)
>                 return;
>  
>         FrameBuffer *buffer = unicam_[Unicam::Embedded].getBuffers().at(bufferId);
> @@ -1825,7 +1826,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
>         RPi::Stream *stream = nullptr;
>         int index;
>  
> -       if (state_ == State::Stopped)
> +       if (state_ == State::Stopped || state_ == State::Error)
>                 return;
>  
>         for (RPi::Stream &s : unicam_) {
> @@ -1864,7 +1865,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
>  
>  void RPiCameraData::ispInputDequeue(FrameBuffer *buffer)
>  {
> -       if (state_ == State::Stopped)
> +       if (state_ == State::Stopped || state_ == State::Error)
>                 return;
>  
>         LOG(RPI, Debug) << "Stream ISP Input buffer complete"
> @@ -1881,7 +1882,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)
>         RPi::Stream *stream = nullptr;
>         int index;
>  
> -       if (state_ == State::Stopped)
> +       if (state_ == State::Stopped || state_ == State::Error)
>                 return;
>  
>         for (RPi::Stream &s : isp_) {
> @@ -1991,6 +1992,7 @@ void RPiCameraData::handleState()
>         switch (state_) {
>         case State::Stopped:
>         case State::Busy:
> +       case State::Error:
>                 break;
>  
>         case State::IpaComplete:
> -- 
> 2.25.1
>

Hi Kieran,

Thanks for the review!

On Fri, 16 Sept 2022 at 14:31, Kieran Bingham <
kieran.bingham@ideasonboard.com> wrote:

> Hi Naush,
>
> Quoting Naushir Patuck via libcamera-devel (2022-09-16 11:05:16)
> > Add an error state used internally in the Raspberry Pi pipeline handler.
> > Currently this state is never set, but will be in a subsequent commit
> when a
> > device timeout has been signalled.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp       | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index b4094898ca6c..d429cb444d58 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -253,7 +253,7 @@ public:
> >          * thread. So, we do not need to have any mutex to protect
> access to any
> >          * of the variables below.
> >          */
> > -       enum class State { Stopped, Idle, Busy, IpaComplete };
> > +       enum class State { Stopped, Idle, Busy, IpaComplete, Error };
> >         State state_;
> >
> >         struct BayerFrame {
> > @@ -1109,7 +1109,8 @@ int PipelineHandlerRPi::queueRequestDevice(Camera
> *camera, Request *request)
> >  {
> >         RPiCameraData *data = cameraData(camera);
> >
> > -       if (data->state_ == RPiCameraData::State::Stopped)
> > +       if (data->state_ == RPiCameraData::State::Stopped ||
> > +           data->state_ == RPiCameraData::State::Error)
>
> This is fine I think, but is it worth an 'isRunning()' state helper?


I can do that for v2, it would probably look cleaner!


>
>
> Does the state match the state machine of the Camera object? (I.e.
> should we expose the states/helpers more conveniently to the pipeline
> handlers so that you don't have to manage it separately?
>

This state is internal the the rpi pipeline handler really, so not the same
as the Camera object - although the common start/stop states would
probably match. As such, it's not going to help me here if the Camera object
state was exposed to us.

Regards,
Naush


>
> As this updates the existing state machine though, I think this is
> fine...
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> >                 return -EINVAL;
> >
> >         LOG(RPI, Debug) << "queueRequestDevice: New request.";
> > @@ -1708,7 +1709,7 @@ void
> RPiCameraData::enumerateVideoDevices(MediaLink *link)
> >
> >  void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const
> ControlList &controls)
> >  {
> > -       if (state_ == State::Stopped)
> > +       if (state_ == State::Stopped || state_ == State::Error)
> >                 return;
> >
> >         FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);
> > @@ -1744,7 +1745,7 @@ void RPiCameraData::statsMetadataComplete(uint32_t
> bufferId, const ControlList &
> >
> >  void RPiCameraData::runIsp(uint32_t bufferId)
> >  {
> > -       if (state_ == State::Stopped)
> > +       if (state_ == State::Stopped || state_ == State::Error)
> >                 return;
> >
> >         FrameBuffer *buffer =
> unicam_[Unicam::Image].getBuffers().at(bufferId);
> > @@ -1759,7 +1760,7 @@ void RPiCameraData::runIsp(uint32_t bufferId)
> >
> >  void RPiCameraData::embeddedComplete(uint32_t bufferId)
> >  {
> > -       if (state_ == State::Stopped)
> > +       if (state_ == State::Stopped || state_ == State::Error)
> >                 return;
> >
> >         FrameBuffer *buffer =
> unicam_[Unicam::Embedded].getBuffers().at(bufferId);
> > @@ -1825,7 +1826,7 @@ void
> RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> >         RPi::Stream *stream = nullptr;
> >         int index;
> >
> > -       if (state_ == State::Stopped)
> > +       if (state_ == State::Stopped || state_ == State::Error)
> >                 return;
> >
> >         for (RPi::Stream &s : unicam_) {
> > @@ -1864,7 +1865,7 @@ void
> RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> >
> >  void RPiCameraData::ispInputDequeue(FrameBuffer *buffer)
> >  {
> > -       if (state_ == State::Stopped)
> > +       if (state_ == State::Stopped || state_ == State::Error)
> >                 return;
> >
> >         LOG(RPI, Debug) << "Stream ISP Input buffer complete"
> > @@ -1881,7 +1882,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer
> *buffer)
> >         RPi::Stream *stream = nullptr;
> >         int index;
> >
> > -       if (state_ == State::Stopped)
> > +       if (state_ == State::Stopped || state_ == State::Error)
> >                 return;
> >
> >         for (RPi::Stream &s : isp_) {
> > @@ -1991,6 +1992,7 @@ void RPiCameraData::handleState()
> >         switch (state_) {
> >         case State::Stopped:
> >         case State::Busy:
> > +       case State::Error:
> >                 break;
> >
> >         case State::IpaComplete:
> > --
> > 2.25.1
> >
>