[libcamera-devel,v4,5/8] libcamera: v4l2_videodevice: Better tracking of the device state
diff mbox series

Message ID 20220322092257.2713521-6-naush@raspberrypi.com
State Accepted
Headers show
Series
  • Raspberry Pi: Efficient start/stop/start sequences
Related show

Commit Message

Naushir Patuck March 22, 2022, 9:22 a.m. UTC 
Replace the existing streaming_ state variable with an enum to track the
following three state: Streaming, Stopping, and Stopped. The alternate states
will be used in a subsequent commit.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 include/libcamera/internal/v4l2_videodevice.h |  3 ++-
 src/libcamera/v4l2_videodevice.cpp            | 10 ++++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

Comments

Laurent Pinchart March 22, 2022, 8:08 p.m. UTC | #1 
Hi Naush,

Thank you for the patch.

On Tue, Mar 22, 2022 at 09:22:54AM +0000, Naushir Patuck via libcamera-devel wrote:
> Replace the existing streaming_ state variable with an enum to track the
> following three state: Streaming, Stopping, and Stopped. The alternate states
> will be used in a subsequent commit.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  include/libcamera/internal/v4l2_videodevice.h |  3 ++-
>  src/libcamera/v4l2_videodevice.cpp            | 10 ++++++----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index 2d2ccc477c91..32e022543385 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -258,7 +258,8 @@ private:
>  
>  	EventNotifier *fdBufferNotifier_;
>  
> -	bool streaming_;
> +	enum class State { Streaming, Stopping, Stopped };

	enum class State {
		Streaming,
		Stopping,
		Stopped,
	};

and this should go right after LIBCAMERA_DISABLE_COPY() as we declare
types before functions and variables.

> +	State state_;
>  };
>  
>  class V4L2M2MDevice
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 5f36ee20710d..9cea6a608660 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -507,7 +507,7 @@ const std::string V4L2DeviceFormat::toString() const
>   */
>  V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
>  	: V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr),
> -	  fdBufferNotifier_(nullptr), streaming_(false)
> +	  fdBufferNotifier_(nullptr), state_(State::Stopped)
>  {
>  	/*
>  	 * We default to an MMAP based CAPTURE video device, however this will
> @@ -1796,7 +1796,7 @@ int V4L2VideoDevice::streamOn()
>  		return ret;
>  	}
>  
> -	streaming_ = true;
> +	state_ = State::Streaming;
>  
>  	return 0;
>  }
> @@ -1818,7 +1818,7 @@ int V4L2VideoDevice::streamOff()
>  {
>  	int ret;
>  
> -	if (!streaming_ && queuedBuffers_.empty())
> +	if (state_ != State::Streaming && queuedBuffers_.empty())
>  		return 0;
>  
>  	ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);
> @@ -1828,6 +1828,8 @@ int V4L2VideoDevice::streamOff()
>  		return ret;
>  	}
>  
> +	state_ = State::Stopping;
> +
>  	/* Send back all queued buffers. */
>  	for (auto it : queuedBuffers_) {
>  		FrameBuffer *buffer = it.second;
> @@ -1838,7 +1840,7 @@ int V4L2VideoDevice::streamOff()
>  
>  	queuedBuffers_.clear();
>  	fdBufferNotifier_->setEnabled(false);
> -	streaming_ = false;
> +	state_ = State::Stopped;

This looks fine, but may depend on how it's then used by the next patch.
Provided that's fine too, with the above change,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  	return 0;
>  }
Kieran Bingham March 23, 2022, 9:59 a.m. UTC | #2 
Quoting Naushir Patuck via libcamera-devel (2022-03-22 09:22:54)
> Replace the existing streaming_ state variable with an enum to track the
> following three state: Streaming, Stopping, and Stopped. The alternate states
> will be used in a subsequent commit.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  include/libcamera/internal/v4l2_videodevice.h |  3 ++-
>  src/libcamera/v4l2_videodevice.cpp            | 10 ++++++----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index 2d2ccc477c91..32e022543385 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -258,7 +258,8 @@ private:
>  
>         EventNotifier *fdBufferNotifier_;
>  
> -       bool streaming_;
> +       enum class State { Streaming, Stopping, Stopped };
> +       State state_;
>  };
>  
>  class V4L2M2MDevice
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 5f36ee20710d..9cea6a608660 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -507,7 +507,7 @@ const std::string V4L2DeviceFormat::toString() const
>   */
>  V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
>         : V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr),
> -         fdBufferNotifier_(nullptr), streaming_(false)
> +         fdBufferNotifier_(nullptr), state_(State::Stopped)
>  {
>         /*
>          * We default to an MMAP based CAPTURE video device, however this will
> @@ -1796,7 +1796,7 @@ int V4L2VideoDevice::streamOn()
>                 return ret;
>         }
>  
> -       streaming_ = true;
> +       state_ = State::Streaming;
>  
>         return 0;
>  }
> @@ -1818,7 +1818,7 @@ int V4L2VideoDevice::streamOff()
>  {
>         int ret;
>  
> -       if (!streaming_ && queuedBuffers_.empty())
> +       if (state_ != State::Streaming && queuedBuffers_.empty())
>                 return 0;
>  
>         ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);
> @@ -1828,6 +1828,8 @@ int V4L2VideoDevice::streamOff()
>                 return ret;
>         }
>  
> +       state_ = State::Stopping;
> +

Should this be before the call for VIDIOC_STREAMOFF?
It may not make a difference in practice, or it might cause
V4L2VideoDevice to reject a buffer (/request) that is queued in parallel
... but I think we already know the request can be queued while calling
streamOff ... so I 'think' it's safe to change the state before calling
the ioctl...

Which ever way you're happier with, or consensus goes to:

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

>         /* Send back all queued buffers. */
>         for (auto it : queuedBuffers_) {
>                 FrameBuffer *buffer = it.second;
> @@ -1838,7 +1840,7 @@ int V4L2VideoDevice::streamOff()
>  
>         queuedBuffers_.clear();
>         fdBufferNotifier_->setEnabled(false);
> -       streaming_ = false;
> +       state_ = State::Stopped;
>  
>         return 0;
>  }
> -- 
> 2.25.1
>
Laurent Pinchart March 23, 2022, 10:05 a.m. UTC | #3 
On Wed, Mar 23, 2022 at 09:59:02AM +0000, Kieran Bingham via libcamera-devel wrote:
> Quoting Naushir Patuck via libcamera-devel (2022-03-22 09:22:54)
> > Replace the existing streaming_ state variable with an enum to track the
> > following three state: Streaming, Stopping, and Stopped. The alternate states
> > will be used in a subsequent commit.
> > 
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  include/libcamera/internal/v4l2_videodevice.h |  3 ++-
> >  src/libcamera/v4l2_videodevice.cpp            | 10 ++++++----
> >  2 files changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> > index 2d2ccc477c91..32e022543385 100644
> > --- a/include/libcamera/internal/v4l2_videodevice.h
> > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > @@ -258,7 +258,8 @@ private:
> >  
> >         EventNotifier *fdBufferNotifier_;
> >  
> > -       bool streaming_;
> > +       enum class State { Streaming, Stopping, Stopped };
> > +       State state_;
> >  };
> >  
> >  class V4L2M2MDevice
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 5f36ee20710d..9cea6a608660 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -507,7 +507,7 @@ const std::string V4L2DeviceFormat::toString() const
> >   */
> >  V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
> >         : V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr),
> > -         fdBufferNotifier_(nullptr), streaming_(false)
> > +         fdBufferNotifier_(nullptr), state_(State::Stopped)
> >  {
> >         /*
> >          * We default to an MMAP based CAPTURE video device, however this will
> > @@ -1796,7 +1796,7 @@ int V4L2VideoDevice::streamOn()
> >                 return ret;
> >         }
> >  
> > -       streaming_ = true;
> > +       state_ = State::Streaming;
> >  
> >         return 0;
> >  }
> > @@ -1818,7 +1818,7 @@ int V4L2VideoDevice::streamOff()
> >  {
> >         int ret;
> >  
> > -       if (!streaming_ && queuedBuffers_.empty())
> > +       if (state_ != State::Streaming && queuedBuffers_.empty())
> >                 return 0;
> >  
> >         ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);
> > @@ -1828,6 +1828,8 @@ int V4L2VideoDevice::streamOff()
> >                 return ret;
> >         }
> >  
> > +       state_ = State::Stopping;
> > +
> 
> Should this be before the call for VIDIOC_STREAMOFF?
> It may not make a difference in practice, or it might cause
> V4L2VideoDevice to reject a buffer (/request) that is queued in parallel
> ... but I think we already know the request can be queued while calling
> streamOff ... so I 'think' it's safe to change the state before calling
> the ioctl...

As the V4L2VideoDevice class isn't thread-safe, it will indeed make no
difference. I'd rather keep it here, or the state will stay Stopping
forever if VIDIOC_STREAMOFF fails.

> Which ever way you're happier with, or consensus goes to:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> >         /* Send back all queued buffers. */
> >         for (auto it : queuedBuffers_) {
> >                 FrameBuffer *buffer = it.second;
> > @@ -1838,7 +1840,7 @@ int V4L2VideoDevice::streamOff()
> >  
> >         queuedBuffers_.clear();
> >         fdBufferNotifier_->setEnabled(false);
> > -       streaming_ = false;
> > +       state_ = State::Stopped;
> >  
> >         return 0;
> >  }
Kieran Bingham March 23, 2022, 10:29 a.m. UTC | #4 
Quoting Laurent Pinchart (2022-03-23 10:05:02)
> On Wed, Mar 23, 2022 at 09:59:02AM +0000, Kieran Bingham via libcamera-devel wrote:
> > Quoting Naushir Patuck via libcamera-devel (2022-03-22 09:22:54)
> > > Replace the existing streaming_ state variable with an enum to track the
> > > following three state: Streaming, Stopping, and Stopped. The alternate states
> > > will be used in a subsequent commit.
> > > 
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > ---
> > >  include/libcamera/internal/v4l2_videodevice.h |  3 ++-
> > >  src/libcamera/v4l2_videodevice.cpp            | 10 ++++++----
> > >  2 files changed, 8 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> > > index 2d2ccc477c91..32e022543385 100644
> > > --- a/include/libcamera/internal/v4l2_videodevice.h
> > > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > > @@ -258,7 +258,8 @@ private:
> > >  
> > >         EventNotifier *fdBufferNotifier_;
> > >  
> > > -       bool streaming_;
> > > +       enum class State { Streaming, Stopping, Stopped };
> > > +       State state_;
> > >  };
> > >  
> > >  class V4L2M2MDevice
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index 5f36ee20710d..9cea6a608660 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -507,7 +507,7 @@ const std::string V4L2DeviceFormat::toString() const
> > >   */
> > >  V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
> > >         : V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr),
> > > -         fdBufferNotifier_(nullptr), streaming_(false)
> > > +         fdBufferNotifier_(nullptr), state_(State::Stopped)
> > >  {
> > >         /*
> > >          * We default to an MMAP based CAPTURE video device, however this will
> > > @@ -1796,7 +1796,7 @@ int V4L2VideoDevice::streamOn()
> > >                 return ret;
> > >         }
> > >  
> > > -       streaming_ = true;
> > > +       state_ = State::Streaming;
> > >  
> > >         return 0;
> > >  }
> > > @@ -1818,7 +1818,7 @@ int V4L2VideoDevice::streamOff()
> > >  {
> > >         int ret;
> > >  
> > > -       if (!streaming_ && queuedBuffers_.empty())
> > > +       if (state_ != State::Streaming && queuedBuffers_.empty())
> > >                 return 0;
> > >  
> > >         ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);
> > > @@ -1828,6 +1828,8 @@ int V4L2VideoDevice::streamOff()
> > >                 return ret;
> > >         }
> > >  
> > > +       state_ = State::Stopping;
> > > +
> > 
> > Should this be before the call for VIDIOC_STREAMOFF?
> > It may not make a difference in practice, or it might cause
> > V4L2VideoDevice to reject a buffer (/request) that is queued in parallel
> > ... but I think we already know the request can be queued while calling
> > streamOff ... so I 'think' it's safe to change the state before calling
> > the ioctl...
> 
> As the V4L2VideoDevice class isn't thread-safe, it will indeed make no
> difference. I'd rather keep it here, or the state will stay Stopping
> forever if VIDIOC_STREAMOFF fails.

That's a good enough reason to keep it where it is indeed ;-)
--
Kieran


> 
> > Which ever way you're happier with, or consensus goes to:
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >

Patch
diff mbox series 

diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
index 2d2ccc477c91..32e022543385 100644
--- a/include/libcamera/internal/v4l2_videodevice.h
+++ b/include/libcamera/internal/v4l2_videodevice.h
@@ -258,7 +258,8 @@  private:
 
 	EventNotifier *fdBufferNotifier_;
 
-	bool streaming_;
+	enum class State { Streaming, Stopping, Stopped };
+	State state_;
 };
 
 class V4L2M2MDevice
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
index 5f36ee20710d..9cea6a608660 100644
--- a/src/libcamera/v4l2_videodevice.cpp
+++ b/src/libcamera/v4l2_videodevice.cpp
@@ -507,7 +507,7 @@  const std::string V4L2DeviceFormat::toString() const
  */
 V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
 	: V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr),
-	  fdBufferNotifier_(nullptr), streaming_(false)
+	  fdBufferNotifier_(nullptr), state_(State::Stopped)
 {
 	/*
 	 * We default to an MMAP based CAPTURE video device, however this will
@@ -1796,7 +1796,7 @@  int V4L2VideoDevice::streamOn()
 		return ret;
 	}
 
-	streaming_ = true;
+	state_ = State::Streaming;
 
 	return 0;
 }
@@ -1818,7 +1818,7 @@  int V4L2VideoDevice::streamOff()
 {
 	int ret;
 
-	if (!streaming_ && queuedBuffers_.empty())
+	if (state_ != State::Streaming && queuedBuffers_.empty())
 		return 0;
 
 	ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);
@@ -1828,6 +1828,8 @@  int V4L2VideoDevice::streamOff()
 		return ret;
 	}
 
+	state_ = State::Stopping;
+
 	/* Send back all queued buffers. */
 	for (auto it : queuedBuffers_) {
 		FrameBuffer *buffer = it.second;
@@ -1838,7 +1840,7 @@  int V4L2VideoDevice::streamOff()
 
 	queuedBuffers_.clear();
 	fdBufferNotifier_->setEnabled(false);
-	streaming_ = false;
+	state_ = State::Stopped;
 
 	return 0;
 }