| Message ID | 20211018132923.476242-11-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Mon, Oct 18, 2021 at 06:59:22PM +0530, Umang Jain wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > The camera HAL APIs requires that any acquire fence that hasn't been > waited on to be sent back to the framework as a release fence. The > CameraDevice already copies the acquire fence to the release fence when > signaling request completion, but the CameraStream incorrectly closes > the fence when a wait times out and sets it to -1. Fix it. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/android/camera_stream.cpp | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > index 9b5cd0c4..8e6ccb83 100644 > --- a/src/android/camera_stream.cpp > +++ b/src/android/camera_stream.cpp > @@ -147,16 +147,16 @@ int CameraStream::process(const FrameBuffer &source, > Camera3RequestDescriptor *request) > { > /* Handle waiting on fences on the destination buffer. */ > - int fence = dest.fence; > - if (fence != -1) { > - int ret = waitFence(fence); > - ::close(fence); > - dest.fence = -1; > + if (dest.fence != -1) { > + int ret = waitFence(dest.fence); > if (ret < 0) { If the only reason why a fence should not be closed is a timeout, shouldn't we inspect the return error code then ? > LOG(HAL, Error) << "Failed waiting for fence: " > - << fence << ": " << strerror(-ret); > + << dest.fence << ": " << strerror(-ret); > return ret; > } > + > + ::close(dest.fence); > + dest.fence = -1; > } > > if (!postProcessor_) > -- > 2.31.0 >
Hi Jacopo, On Mon, Oct 18, 2021 at 06:38:05PM +0200, Jacopo Mondi wrote: > On Mon, Oct 18, 2021 at 06:59:22PM +0530, Umang Jain wrote: > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > The camera HAL APIs requires that any acquire fence that hasn't been > > waited on to be sent back to the framework as a release fence. The > > CameraDevice already copies the acquire fence to the release fence when > > signaling request completion, but the CameraStream incorrectly closes > > the fence when a wait times out and sets it to -1. Fix it. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/android/camera_stream.cpp | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > > index 9b5cd0c4..8e6ccb83 100644 > > --- a/src/android/camera_stream.cpp > > +++ b/src/android/camera_stream.cpp > > @@ -147,16 +147,16 @@ int CameraStream::process(const FrameBuffer &source, > > Camera3RequestDescriptor *request) > > { > > /* Handle waiting on fences on the destination buffer. */ > > - int fence = dest.fence; > > - if (fence != -1) { > > - int ret = waitFence(fence); > > - ::close(fence); > > - dest.fence = -1; > > + if (dest.fence != -1) { > > + int ret = waitFence(dest.fence); > > if (ret < 0) { > > If the only reason why a fence should not be closed is a timeout, > shouldn't we inspect the return error code then ? Or we should update the commit message. If we don't successfully wait on a fence, I think it should be passed back to the camera service (I'm not sure what other conditions could arise though, it's probably theoretical only). s/times out/fails/ in the subject and commit message. > > LOG(HAL, Error) << "Failed waiting for fence: " > > - << fence << ": " << strerror(-ret); > > + << dest.fence << ": " << strerror(-ret); > > return ret; > > } > > + > > + ::close(dest.fence); > > + dest.fence = -1; > > } > > > > if (!postProcessor_)
Hello, On 10/18/21 6:59 PM, Umang Jain wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > The camera HAL APIs requires that any acquire fence that hasn't been > waited on to be sent back to the framework as a release fence. The > CameraDevice already copies the acquire fence to the release fence when > signaling request completion, but the CameraStream incorrectly closes > the fence when a wait times out and sets it to -1. Fix it. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Umang Jain<umang.jain@ideasonboard.com> > --- > src/android/camera_stream.cpp | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > index 9b5cd0c4..8e6ccb83 100644 > --- a/src/android/camera_stream.cpp > +++ b/src/android/camera_stream.cpp > @@ -147,16 +147,16 @@ int CameraStream::process(const FrameBuffer &source, > Camera3RequestDescriptor *request) > { > /* Handle waiting on fences on the destination buffer. */ > - int fence = dest.fence; > - if (fence != -1) { > - int ret = waitFence(fence); > - ::close(fence); > - dest.fence = -1; > + if (dest.fence != -1) { > + int ret = waitFence(dest.fence); > if (ret < 0) { > LOG(HAL, Error) << "Failed waiting for fence: " > - << fence << ": " << strerror(-ret); > + << dest.fence << ": " << strerror(-ret); > return ret; > } > + > + ::close(dest.fence); > + dest.fence = -1; > } > > if (!postProcessor_)
diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp index 9b5cd0c4..8e6ccb83 100644 --- a/src/android/camera_stream.cpp +++ b/src/android/camera_stream.cpp @@ -147,16 +147,16 @@ int CameraStream::process(const FrameBuffer &source, Camera3RequestDescriptor *request) { /* Handle waiting on fences on the destination buffer. */ - int fence = dest.fence; - if (fence != -1) { - int ret = waitFence(fence); - ::close(fence); - dest.fence = -1; + if (dest.fence != -1) { + int ret = waitFence(dest.fence); if (ret < 0) { LOG(HAL, Error) << "Failed waiting for fence: " - << fence << ": " << strerror(-ret); + << dest.fence << ": " << strerror(-ret); return ret; } + + ::close(dest.fence); + dest.fence = -1; } if (!postProcessor_)