[libcamera-devel,v2,10/12] android: camera_stream: Don't close fence if wait fails

14184 diff mbox series

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 fails 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(-)

Hi Umang,

On Tue, Oct 19, 2021 at 05:18:00PM +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 fails and sets it to -1. Fix it.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Umang Jain<umang.jain@ideasonboard.com>

Good catch!
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> ---
>  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_)
> --
> 2.31.0
>

Hi Laurent and Umang, thank you for the patch.

On Tue, Oct 19, 2021 at 9:12 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Umang,
>
> On Tue, Oct 19, 2021 at 05:18:00PM +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 fails and sets it to -1. Fix it.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Umang Jain<umang.jain@ideasonboard.com>
>

Reviewed-by: Hirokazu Honda <hiroh@chromium.org>

> Good catch!
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks
>   j
>
> > ---
> >  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_)
> > --
> > 2.31.0
> >