[libcamera-devel] [PATCH v2 4/8] libcamera: camera: Validate requests are completed in Running state

[Kieran Bingham]

Hi Laurent,

On 13/03/2021 23:00, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Sat, Mar 13, 2021 at 10:35:49AM +0100, Jacopo Mondi wrote:
>> On Fri, Mar 12, 2021 at 05:47:23AM +0000, Kieran Bingham wrote:
>>> All requests must have completed before the Camera has fully stopped.
>>>
>>> Requests completing when the camera is not running represent an internal
>>> pipeline handler bug.
>>>
>>> Trap this event with a fatal error.
>>
>> Am I wrong or this is actually the other way around ? Marking the
>> Camera as Configured -after- the pipeline's stop() has been invoked
>> makes sure the requests that complete with error due to stop() are
>> refused.
>>
>> IOW stop() is invoked on the pipeline thread, which might complete
>> requests with error asynchronously from the camera state being
>> moved to 'CameraConfigured'. After the Camera state has changed,
>> all the requests completed with error will trigger a Fatal.
>> Am I missing something obvious ?
> 
> The call to PipelineHandler::stop() is blocking, which means that the
> state will only be set back to CameraConfigured once the pipeline
> handler's stop() function returns. Pipeline handlers are required to
> complete all pending requests before returning from stop(), so the
> assertion shouldn't be triggered. Am I missing something ? :-)
> 
>>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>> ---
>>>  src/libcamera/camera.cpp | 10 ++++++++--
>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
>>> index 84edbb8f494d..19fb9eb415b0 100644
>>> --- a/src/libcamera/camera.cpp
>>> +++ b/src/libcamera/camera.cpp
>>> @@ -1062,11 +1062,11 @@ int Camera::stop()
>>>
>>>  	LOG(Camera, Debug) << "Stopping capture";
>>>
>>> -	d->setState(Private::CameraConfigured);
>>> -
>>>  	d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,
>>>  			       this);
>>>
>>> +	d->setState(Private::CameraConfigured);
>>> +
>>>  	return 0;
>>>  }
>>>
>>> @@ -1079,6 +1079,12 @@ int Camera::stop()
>>>   */
>>>  void Camera::requestComplete(Request *request)
>>>  {
>>> +	Private *const d = LIBCAMERA_D_PTR();
>>> +
>>> +	int ret = d->isAccessAllowed(Private::CameraRunning);
>>> +	if (ret < 0)
>>> +		LOG(Camera, Fatal) << "Trying to complete a request when Stopped";
> 
> Nitpicking, as there's no Stopped state, I'd write "stopped".
> 
> And you could also write
> 
> 	if (d->isAccessAllowed(Private::CameraRunning) < 0)
> 		LOG(Camera, Fatal) << "Trying to complete a request when Stopped";

As we don't otherwise need this ret value I agree, until ...

> I also wonder what then happens when a camera is unplugged. Could you
> try unplugging a UVC camera while streaming ?

Hrm ... indeed this is a race probably.

When a camera is disconnected, it should set disconnected_, which will
then causes isAccessAllowed() to return -ENODEV, which would be entirely
permittable here to still complete the request and pass it back to the
application.

How about:

/* Disconnected cameras are still able to complete requests. */
ret = d->isAccessAllowed(Private::CameraRunning);
if (ret < 0 && ret != -ENODEV)
   LOG(Camera, Fatal) << "Trying to complete a request when stopped";


>>> +
>>>  	requestCompleted.emit(request);
>>>  }
>>>
> 

-- 
Regards
--
Kieran