| Message ID | 20210907145949.270824-1-umang.jain@ideasonboard.com |
|---|---|
| State | Accepted |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, thank you for the patch. On Tue, Sep 7, 2021 at 11:59 PM Umang Jain <umang.jain@ideasonboard.com> wrote: > > Failure can still happen by CameraBufferManager during Unlock() and/or > Deregister() of camera3Buffer handles. We should be logging those > errors as well. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > --- > Changes in v2: > - Remove a debug log that creeped in by mistake > > I have been able to spot one of the failure which is happening on my > in-developement async post-processing. It is due a failure in > Deregister(). It is intermittent and non-fatal as far as I can see. > > Having a failure log in HAL, apart from https://paste.debian.net/1210728/ > helps to know the exact place from where the error is originating from > libcamera HAL. > --- > src/android/mm/cros_camera_buffer.cpp | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp > index ec45e04c..86770135 100644 > --- a/src/android/mm/cros_camera_buffer.cpp > +++ b/src/android/mm/cros_camera_buffer.cpp > @@ -73,10 +73,20 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer, > > CameraBuffer::Private::~Private() > { > - if (mapped_) > - bufferManager_->Unlock(handle_); > - if (registered_) > - bufferManager_->Deregister(handle_); > + int ret; > + if (mapped_) { > + ret = bufferManager_->Unlock(handle_); > + if (ret != 0) > + LOG(HAL, Error) << "Failed to unlock buffer: " > + << strerror(-ret); > + } > + > + if (registered_) { > + ret = bufferManager_->Deregister(handle_); > + if (ret != 0) > + LOG(HAL, Error) << "Failed to deregister buffer: " > + << strerror(-ret); > + } > } > > unsigned int CameraBuffer::Private::numPlanes() const > -- > 2.31.0 >
On 07/09/2021 15:59, Umang Jain wrote: > Failure can still happen by CameraBufferManager during Unlock() and/or > Deregister() of camera3Buffer handles. We should be logging those > errors as well. > I am suspicious that the reason you've seen these errors is that you are double-free'ing the CameraBuffer (which is why you get an invalid handle). I think having the error prints are better than not having them though, as it should there was a failure that was otherwise silent. I'm surprised there wasn't a segfault or such in fact. But there's no direct issue or fault with this patch - It makes me go back to wanting to put a double-free protection/assertion on the Extensible class though.. But for here: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > Changes in v2: > - Remove a debug log that creeped in by mistake > > I have been able to spot one of the failure which is happening on my > in-developement async post-processing. It is due a failure in > Deregister(). It is intermittent and non-fatal as far as I can see. > > Having a failure log in HAL, apart from https://paste.debian.net/1210728/ > helps to know the exact place from where the error is originating from > libcamera HAL. > --- > src/android/mm/cros_camera_buffer.cpp | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp > index ec45e04c..86770135 100644 > --- a/src/android/mm/cros_camera_buffer.cpp > +++ b/src/android/mm/cros_camera_buffer.cpp > @@ -73,10 +73,20 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer, > > CameraBuffer::Private::~Private() > { > - if (mapped_) > - bufferManager_->Unlock(handle_); > - if (registered_) > - bufferManager_->Deregister(handle_); > + int ret; > + if (mapped_) { > + ret = bufferManager_->Unlock(handle_); > + if (ret != 0) > + LOG(HAL, Error) << "Failed to unlock buffer: " > + << strerror(-ret); > + } > + > + if (registered_) { > + ret = bufferManager_->Deregister(handle_); > + if (ret != 0) > + LOG(HAL, Error) << "Failed to deregister buffer: " > + << strerror(-ret); > + } > } > > unsigned int CameraBuffer::Private::numPlanes() const >
diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp index ec45e04c..86770135 100644 --- a/src/android/mm/cros_camera_buffer.cpp +++ b/src/android/mm/cros_camera_buffer.cpp @@ -73,10 +73,20 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer, CameraBuffer::Private::~Private() { - if (mapped_) - bufferManager_->Unlock(handle_); - if (registered_) - bufferManager_->Deregister(handle_); + int ret; + if (mapped_) { + ret = bufferManager_->Unlock(handle_); + if (ret != 0) + LOG(HAL, Error) << "Failed to unlock buffer: " + << strerror(-ret); + } + + if (registered_) { + ret = bufferManager_->Deregister(handle_); + if (ret != 0) + LOG(HAL, Error) << "Failed to deregister buffer: " + << strerror(-ret); + } } unsigned int CameraBuffer::Private::numPlanes() const
Failure can still happen by CameraBufferManager during Unlock() and/or Deregister() of camera3Buffer handles. We should be logging those errors as well. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- Changes in v2: - Remove a debug log that creeped in by mistake I have been able to spot one of the failure which is happening on my in-developement async post-processing. It is due a failure in Deregister(). It is intermittent and non-fatal as far as I can see. Having a failure log in HAL, apart from https://paste.debian.net/1210728/ helps to know the exact place from where the error is originating from libcamera HAL. --- src/android/mm/cros_camera_buffer.cpp | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)