| Message ID | 20191022160110.19419-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Tue, Oct 22, 2019 at 05:01:10PM +0100, Kieran Bingham wrote: > Track the state of streamon/streamoff calls to simplify error paths. > > Ensuring that streamOff() can be called on non-streaming streams facilitates simpler > error code paths, where a set of devices can all call streamOff regardless of their > initialisation state. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > > This is a patch I have while developing the RPi ISP handler. > It means I can do the following for error paths: > > data->isp_.stats_->streamOff(); > data->isp_.capture1_->streamOff(); > data->isp_.capture0_->streamOff(); > data->isp_.output_->streamOff(); > data->unicam_->streamOff(); > > which essentially means I can just call stop(camera) for cleanup, without > having to track which ones were successfully started or not. > > So - Reasonable or not ? Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I hope this answers your question :-) > Patch will not apply directly to master, as it's currently based on my dev > branch, hence RFC > > src/libcamera/include/v4l2_videodevice.h | 2 ++ > src/libcamera/v4l2_videodevice.cpp | 9 ++++++++- > 2 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h > index 60fca01670c6..d6e0c0e7bf02 100644 > --- a/src/libcamera/include/v4l2_videodevice.h > +++ b/src/libcamera/include/v4l2_videodevice.h > @@ -188,6 +188,8 @@ private: > std::map<unsigned int, Buffer *> queuedBuffers_; > > EventNotifier *fdEvent_; > + > + bool streaming_; > }; > > class V4L2M2MDevice > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index 71c32766fc2d..01e11597cfb0 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -273,7 +273,7 @@ const std::string V4L2DeviceFormat::toString() const > */ > V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode) > : V4L2Device(deviceNode), multiPlanar_(false), bufferPool_(nullptr), > - fdEvent_(nullptr) > + fdEvent_(nullptr), streaming_(false) > { > /* > * We default to an MMAP based CAPTURE video device, however this will > @@ -1197,6 +1197,8 @@ int V4L2VideoDevice::streamOn() > return ret; > } > > + streaming_ = true; > + > return 0; > } > > @@ -1214,6 +1216,9 @@ int V4L2VideoDevice::streamOff() > { > int ret; > > + if (!streaming_) > + return 0; > + > ret = ioctl(VIDIOC_STREAMOFF, &bufferType_); > if (ret < 0) { > LOG(V4L2, Error) > @@ -1234,6 +1239,8 @@ int V4L2VideoDevice::streamOff() > queuedBuffers_.clear(); > fdEvent_->setEnabled(false); > > + streaming_ = false; > + > return 0; > } >
diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h index 60fca01670c6..d6e0c0e7bf02 100644 --- a/src/libcamera/include/v4l2_videodevice.h +++ b/src/libcamera/include/v4l2_videodevice.h @@ -188,6 +188,8 @@ private: std::map<unsigned int, Buffer *> queuedBuffers_; EventNotifier *fdEvent_; + + bool streaming_; }; class V4L2M2MDevice diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index 71c32766fc2d..01e11597cfb0 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -273,7 +273,7 @@ const std::string V4L2DeviceFormat::toString() const */ V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode) : V4L2Device(deviceNode), multiPlanar_(false), bufferPool_(nullptr), - fdEvent_(nullptr) + fdEvent_(nullptr), streaming_(false) { /* * We default to an MMAP based CAPTURE video device, however this will @@ -1197,6 +1197,8 @@ int V4L2VideoDevice::streamOn() return ret; } + streaming_ = true; + return 0; } @@ -1214,6 +1216,9 @@ int V4L2VideoDevice::streamOff() { int ret; + if (!streaming_) + return 0; + ret = ioctl(VIDIOC_STREAMOFF, &bufferType_); if (ret < 0) { LOG(V4L2, Error) @@ -1234,6 +1239,8 @@ int V4L2VideoDevice::streamOff() queuedBuffers_.clear(); fdEvent_->setEnabled(false); + streaming_ = false; + return 0; }
Track the state of streamon/streamoff calls to simplify error paths. Ensuring that streamOff() can be called on non-streaming streams facilitates simpler error code paths, where a set of devices can all call streamOff regardless of their initialisation state. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- This is a patch I have while developing the RPi ISP handler. It means I can do the following for error paths: data->isp_.stats_->streamOff(); data->isp_.capture1_->streamOff(); data->isp_.capture0_->streamOff(); data->isp_.output_->streamOff(); data->unicam_->streamOff(); which essentially means I can just call stop(camera) for cleanup, without having to track which ones were successfully started or not. So - Reasonable or not ? Patch will not apply directly to master, as it's currently based on my dev branch, hence RFC src/libcamera/include/v4l2_videodevice.h | 2 ++ src/libcamera/v4l2_videodevice.cpp | 9 ++++++++- 2 files changed, 10 insertions(+), 1 deletion(-)