[libcamera-devel] [PATCH] android: Re-order out-of-order completion path

[Umang Jain]

Hello,

On 10/15/21 7:32 AM, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Oct 14, 2021 at 12:37:57PM +0200, Jacopo Mondi wrote:
>> When the camera HAL detects an out-of-order completion of a request, it
>> sends to the camera framework a CAMERA3_MSG_ERROR_DEVICE error.
>>
>> Such error not only forces the service to close the camera as prescribed
>> by the camera3 specification, but in some implementation (specifically
>> the ChromeOS one) it causes the camera service to abort and exit.
>>
>> This prevents any error messages to be printed by libcamera, as the
> s/to be printed/from being printed/
>
>> library gets terminated before getting to that point, and also hides the
>> printout of error messages that lead to out-of-order completion, making
>> it impossible to get from the output log what happened.
>>
>> Move the call to notifyError() at the end of the error path and demote
>> the error message to LogLevels::Error from Fatal to let the service
>> implementation decide how to handle CAMERA3_MSG_ERROR_DEVICE errors.
>>
>> Before this patch, when waiting on a fence fails and the capture
>> request is not queued to the Camera, we get an out-of-order completion
>> but no backtrace. With this patch applied the error path is visible:
>>
>> ERROR HAL camera_worker.cpp:122 Failed waiting for fence: 82: Timer expired
>> ERROR HAL camera_device.cpp:1110 '\_SB_.PCI0.I2C2.CAM0': Out-of-order completion for request 0x00007e6de4004c70
>>
>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>> ---
>>   src/android/camera_device.cpp | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index 901867105085..f60297e13c8b 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -1104,16 +1104,17 @@ void CameraDevice::requestComplete(Request *request)
>>   	if (descriptor->request_->cookie() != request->cookie()) {
>>   		/*
>>   		 * \todo Clarify if the Camera has to be closed on
>> -		 * ERROR_DEVICE and possibly demote the Fatal to simple
>> -		 * Error.
>> +		 * ERROR_DEVICE.
>>   		 */
>> -		notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);
>> -		LOG(HAL, Fatal)
>> +		LOG(HAL, Error)
>>   			<< "Out-of-order completion for request "
>>   			<< utils::hex(request->cookie());
>>   
>>   		MutexLocker descriptorsLock(descriptorsMutex_);
>>   		descriptors_.pop();
>> +
>> +		notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);
> Given that we've removed the descriptor at the head of the queue, which
> doesn't correspond to the request, from this point onwards all hell will
> likely break loose, possibly even in the close() path. It's probably not


Can we use this as an error message, " All hell broke loose, Run!!!" ;-)

> worth than a Fatal though, so
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>


Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>

>
>> +
>>   		return;
>>   	}
>>