| Message ID | 20190213151027.6376-4-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Wed, Feb 13, 2019 at 03:10:22PM +0000, Kieran Bingham wrote: > We must ensure that the stream is disabled before releasing buffers. It will > not hurt to call streamOff() even if it is already off before releasing any > buffers back to the device. I'm not sure about this one. Doesn't it indicate a problem in the caller instead ? > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/v4l2_device.cpp | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 23c0da295905..83073c28b817 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -642,6 +642,8 @@ int V4L2Device::releaseBuffers() > { > LOG(V4L2, Debug) << "Releasing bufferPool"; > > + streamOff(); > + > requestBuffers(0); > bufferPool_ = nullptr; >
Hi Laurent, On 13/02/2019 15:41, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Wed, Feb 13, 2019 at 03:10:22PM +0000, Kieran Bingham wrote: >> We must ensure that the stream is disabled before releasing buffers. It will >> not hurt to call streamOff() even if it is already off before releasing any >> buffers back to the device. > > I'm not sure about this one. Doesn't it indicate a problem in the caller > instead ? Perhaps. It means (in the cases that I've hit) that the error paths have not called streamOff(). Is it more preferable to leave buffers 'un-released'? Perhaps would you prefer to see releaseBuffers return an error if it fails? As this is likely to be on cleanup paths, and destruction - what would you expect the application to do in the event of failure to releaseBuffers? (I guess - fix their code...) Either way - I'll just drop this patch for now. -- Kieran >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> src/libcamera/v4l2_device.cpp | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp >> index 23c0da295905..83073c28b817 100644 >> --- a/src/libcamera/v4l2_device.cpp >> +++ b/src/libcamera/v4l2_device.cpp >> @@ -642,6 +642,8 @@ int V4L2Device::releaseBuffers() >> { >> LOG(V4L2, Debug) << "Releasing bufferPool"; >> >> + streamOff(); >> + >> requestBuffers(0); >> bufferPool_ = nullptr; >> >
On Wed, Feb 13, 2019 at 04:30:36PM +0000, Kieran Bingham wrote: > Hi Laurent, > > On 13/02/2019 15:41, Laurent Pinchart wrote: > > Hi Kieran, > > > > Thank you for the patch. > > > > On Wed, Feb 13, 2019 at 03:10:22PM +0000, Kieran Bingham wrote: > >> We must ensure that the stream is disabled before releasing buffers. It will > >> not hurt to call streamOff() even if it is already off before releasing any > >> buffers back to the device. > > > > I'm not sure about this one. Doesn't it indicate a problem in the caller > > instead ? > > Perhaps. It means (in the cases that I've hit) that the error paths have > not called streamOff(). > > Is it more preferable to leave buffers 'un-released'? I think that would be better than trying to hide the issue, yes. > Perhaps would you prefer to see releaseBuffers return an error if it > fails? Sounds good to me, with an error message logged. > As this is likely to be on cleanup paths, and destruction - what > would you expect the application to do in the event of failure to > releaseBuffers? (I guess - fix their code...) Yes, fix their code :-) Buffers can also be released at runtime when switching resolutions, in which case proper cleanup is mandatory, and errors in the code flow should be logged instead of hidden. > Either way - I'll just drop this patch for now. > > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- > >> src/libcamera/v4l2_device.cpp | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > >> index 23c0da295905..83073c28b817 100644 > >> --- a/src/libcamera/v4l2_device.cpp > >> +++ b/src/libcamera/v4l2_device.cpp > >> @@ -642,6 +642,8 @@ int V4L2Device::releaseBuffers() > >> { > >> LOG(V4L2, Debug) << "Releasing bufferPool"; > >> > >> + streamOff(); > >> + > >> requestBuffers(0); > >> bufferPool_ = nullptr; > >>
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index 23c0da295905..83073c28b817 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -642,6 +642,8 @@ int V4L2Device::releaseBuffers() { LOG(V4L2, Debug) << "Releasing bufferPool"; + streamOff(); + requestBuffers(0); bufferPool_ = nullptr;
We must ensure that the stream is disabled before releasing buffers. It will not hurt to call streamOff() even if it is already off before releasing any buffers back to the device. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/v4l2_device.cpp | 2 ++ 1 file changed, 2 insertions(+)