[v3,6/7] android: notify CAMERA3_MSG_ERROR_REQUEST out of order
diff mbox series

Message ID 20241204164137.3938891-7-chenghaoyang@chromium.org
State New
Headers show
Series
  • Refactor Android HAL before supporting partial result
Related show

Commit Message

Cheng-Hao Yang Dec. 4, 2024, 4:36 p.m. UTC 
When a request hasn't done any processing, CAMERA3_MSG_ERROR_REQUEST and
the following process_capture_result don't need to wait for the previous
requests' completion. This patch avoids pushing the aborted requests
into the request queue.

Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
---
 src/android/camera_device.cpp | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

Comments

Jacopo Mondi Dec. 5, 2024, 4:46 p.m. UTC | #1 
On Wed, Dec 04, 2024 at 04:36:31PM +0000, Harvey Yang wrote:
> When a request hasn't done any processing, CAMERA3_MSG_ERROR_REQUEST and
> the following process_capture_result don't need to wait for the previous
> requests' completion. This patch avoids pushing the aborted requests
> into the request queue.
>
> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> ---
>  src/android/camera_device.cpp | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index dd2c603e0..18e974f56 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -867,6 +867,8 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const
>  		buffer.status = StreamBuffer::Status::Error;
>
>  	descriptor->status_ = Camera3RequestDescriptor::Status::Error;
> +
> +	sendCaptureResult(descriptor);
>  }
>
>  bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const
> @@ -1136,14 +1138,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  	MutexLocker stateLock(stateMutex_);
>
>  	if (state_ == State::Flushing) {
> -		Camera3RequestDescriptor *rawDescriptor = descriptor.get();
> -		{
> -			MutexLocker descriptorsLock(descriptorsMutex_);
> -			descriptors_.push(std::move(descriptor));
> -		}
> -		abortRequest(rawDescriptor);
> -		completeDescriptor(rawDescriptor);
> -
> +		abortRequest(descriptor.get());

Right, if we don't have to complete in order this indeed seems
correct.

However, we fall in this case, right ?

     * 2. For pending requests that have not done any processing, the HAL must call notify
     *    CAMERA3_MSG_ERROR_REQUEST, and return all the output buffers with
     *    process_capture_result in the error state (CAMERA3_BUFFER_STATUS_ERROR).
     *    The HAL must not place the release fence into an error state, instead,
     *    the release fences must be set to the acquire fences passed by the framework,
     *    or -1 if they have been waited on by the HAL already. This is also the path
     *    to follow for any captures for which the HAL already called notify() with
     *    CAMERA3_MSG_SHUTTER but won't be producing any metadata/valid buffers for.
     *    After CAMERA3_MSG_ERROR_REQUEST, for a given frame, only process_capture_results with
     *    buffers in CAMERA3_BUFFER_STATUS_ERROR are allowed. No further notifys or
     *    process_capture_result with non-null metadata is allowed.

Apparently we're not handling fences here. Not your fault, seems like
it wasn't there already. While at it could you add a \todo or, if you
feel like it's worth fixing it, pile up a patch ?

The patch itself seems good to me
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

>  		return 0;
>  	}
>
> --
> 2.47.0.338.g60cca15819-goog
>
Cheng-Hao Yang Dec. 10, 2024, 5:40 a.m. UTC | #2 
Hi Jacopo,

On Fri, Dec 6, 2024 at 12:46 AM Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> On Wed, Dec 04, 2024 at 04:36:31PM +0000, Harvey Yang wrote:
> > When a request hasn't done any processing, CAMERA3_MSG_ERROR_REQUEST and
> > the following process_capture_result don't need to wait for the previous
> > requests' completion. This patch avoids pushing the aborted requests
> > into the request queue.
> >
> > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > ---
> >  src/android/camera_device.cpp | 11 +++--------
> >  1 file changed, 3 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index dd2c603e0..18e974f56 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -867,6 +867,8 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const
> >               buffer.status = StreamBuffer::Status::Error;
> >
> >       descriptor->status_ = Camera3RequestDescriptor::Status::Error;
> > +
> > +     sendCaptureResult(descriptor);
> >  }
> >
> >  bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const
> > @@ -1136,14 +1138,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >       MutexLocker stateLock(stateMutex_);
> >
> >       if (state_ == State::Flushing) {
> > -             Camera3RequestDescriptor *rawDescriptor = descriptor.get();
> > -             {
> > -                     MutexLocker descriptorsLock(descriptorsMutex_);
> > -                     descriptors_.push(std::move(descriptor));
> > -             }
> > -             abortRequest(rawDescriptor);
> > -             completeDescriptor(rawDescriptor);
> > -
> > +             abortRequest(descriptor.get());
>
> Right, if we don't have to complete in order this indeed seems
> correct.
>
> However, we fall in this case, right ?
>
>      * 2. For pending requests that have not done any processing, the HAL must call notify
>      *    CAMERA3_MSG_ERROR_REQUEST, and return all the output buffers with
>      *    process_capture_result in the error state (CAMERA3_BUFFER_STATUS_ERROR).
>      *    The HAL must not place the release fence into an error state, instead,
>      *    the release fences must be set to the acquire fences passed by the framework,
>      *    or -1 if they have been waited on by the HAL already. This is also the path
>      *    to follow for any captures for which the HAL already called notify() with
>      *    CAMERA3_MSG_SHUTTER but won't be producing any metadata/valid buffers for.
>      *    After CAMERA3_MSG_ERROR_REQUEST, for a given frame, only process_capture_results with
>      *    buffers in CAMERA3_BUFFER_STATUS_ERROR are allowed. No further notifys or
>      *    process_capture_result with non-null metadata is allowed.
>
> Apparently we're not handling fences here. Not your fault, seems like
> it wasn't there already. While at it could you add a \todo or, if you
> feel like it's worth fixing it, pile up a patch ?

Yes we fall into this case, while I think the original logic already
handles the fence as described?
In `CameraDevice::sendCaptureResults()` (or
`CameraDevice::sendCaptureResult()` in my patches),
`buffer.fence.release()` is put in the `release_fence` field of
`camera3_steram_buffer_t`.

Please check if I missed anything.

BR,
Harvey


>
> The patch itself seems good to me
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>
> >               return 0;
> >       }
> >
> > --
> > 2.47.0.338.g60cca15819-goog
> >
Jacopo Mondi Dec. 10, 2024, 8:36 a.m. UTC | #3 
Hi Harvey

On Tue, Dec 10, 2024 at 01:40:14PM +0800, Cheng-Hao Yang wrote:
> Hi Jacopo,
>
> On Fri, Dec 6, 2024 at 12:46 AM Jacopo Mondi
> <jacopo.mondi@ideasonboard.com> wrote:
> >
> > On Wed, Dec 04, 2024 at 04:36:31PM +0000, Harvey Yang wrote:
> > > When a request hasn't done any processing, CAMERA3_MSG_ERROR_REQUEST and
> > > the following process_capture_result don't need to wait for the previous
> > > requests' completion. This patch avoids pushing the aborted requests
> > > into the request queue.
> > >
> > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > > ---
> > >  src/android/camera_device.cpp | 11 +++--------
> > >  1 file changed, 3 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index dd2c603e0..18e974f56 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -867,6 +867,8 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const
> > >               buffer.status = StreamBuffer::Status::Error;
> > >
> > >       descriptor->status_ = Camera3RequestDescriptor::Status::Error;
> > > +
> > > +     sendCaptureResult(descriptor);
> > >  }
> > >
> > >  bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const
> > > @@ -1136,14 +1138,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > >       MutexLocker stateLock(stateMutex_);
> > >
> > >       if (state_ == State::Flushing) {
> > > -             Camera3RequestDescriptor *rawDescriptor = descriptor.get();
> > > -             {
> > > -                     MutexLocker descriptorsLock(descriptorsMutex_);
> > > -                     descriptors_.push(std::move(descriptor));
> > > -             }
> > > -             abortRequest(rawDescriptor);
> > > -             completeDescriptor(rawDescriptor);
> > > -
> > > +             abortRequest(descriptor.get());
> >
> > Right, if we don't have to complete in order this indeed seems
> > correct.
> >
> > However, we fall in this case, right ?
> >
> >      * 2. For pending requests that have not done any processing, the HAL must call notify
> >      *    CAMERA3_MSG_ERROR_REQUEST, and return all the output buffers with
> >      *    process_capture_result in the error state (CAMERA3_BUFFER_STATUS_ERROR).
> >      *    The HAL must not place the release fence into an error state, instead,
> >      *    the release fences must be set to the acquire fences passed by the framework,
> >      *    or -1 if they have been waited on by the HAL already. This is also the path
> >      *    to follow for any captures for which the HAL already called notify() with
> >      *    CAMERA3_MSG_SHUTTER but won't be producing any metadata/valid buffers for.
> >      *    After CAMERA3_MSG_ERROR_REQUEST, for a given frame, only process_capture_results with
> >      *    buffers in CAMERA3_BUFFER_STATUS_ERROR are allowed. No further notifys or
> >      *    process_capture_result with non-null metadata is allowed.
> >
> > Apparently we're not handling fences here. Not your fault, seems like
> > it wasn't there already. While at it could you add a \todo or, if you
> > feel like it's worth fixing it, pile up a patch ?
>
> Yes we fall into this case, while I think the original logic already
> handles the fence as described?
> In `CameraDevice::sendCaptureResults()` (or
> `CameraDevice::sendCaptureResult()` in my patches),
> `buffer.fence.release()` is put in the `release_fence` field of
> `camera3_steram_buffer_t`.
>
> Please check if I missed anything.

Yes, you're right there even is a comment here

        /*
         * Pass the buffer fence back to the camera framework as
         * a release fence. This instructs the framework to wait
         * on the acquire fence in case we haven't done so
         * ourselves for any reason.
         */

And I have checked the call stack and confirmed that once a Fence is
waited on by the library, it is reset to -1 so the Android specs
should be respected here.

>
> BR,
> Harvey
>
>
> >
> > The patch itself seems good to me
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Still holds indeed

Thanks
  j

> >
> > >               return 0;
> > >       }
> > >
> > > --
> > > 2.47.0.338.g60cca15819-goog
> > >

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index dd2c603e0..18e974f56 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -867,6 +867,8 @@  void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const
 		buffer.status = StreamBuffer::Status::Error;
 
 	descriptor->status_ = Camera3RequestDescriptor::Status::Error;
+
+	sendCaptureResult(descriptor);
 }
 
 bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const
@@ -1136,14 +1138,7 @@  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 	MutexLocker stateLock(stateMutex_);
 
 	if (state_ == State::Flushing) {
-		Camera3RequestDescriptor *rawDescriptor = descriptor.get();
-		{
-			MutexLocker descriptorsLock(descriptorsMutex_);
-			descriptors_.push(std::move(descriptor));
-		}
-		abortRequest(rawDescriptor);
-		completeDescriptor(rawDescriptor);
-
+		abortRequest(descriptor.get());
 		return 0;
 	}