[libcamera-devel] [PATCH v2 4/8] libcamera: camera: Validate requests are completed in Running state
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Mar 24 13:49:38 CET 2021
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
More information about the libcamera-devel
mailing list