[PATCH] pipeline: rkisp1: clear pending requests when stopping camera device

[Javier Carrasco]

On 27/10/2024 19:19, Laurent Pinchart wrote:
> Hello Javier,
> 
> On Fri, Oct 25, 2024 at 01:11:56PM +0200, Javier Carrasco wrote:
>> On 25/10/2024 11:46, Umang Jain wrote:
>>> On 25/10/24 2:08 pm, Javier Carrasco wrote:
>>>> If the camera device is stopped, there may still be requests within
>>>> 'data->queuedRequests_' that are mistakenly never popped. If this is the
>>>> case, libcamera terminates with the assertion
>>>> 'ASSERT(data->queuedRequests_.empty())'.
>>>>
>>>> Walk over the pending queued requests, destroy the info frames and
>>>> complete the requests to ensure that no requests are left behind.
>>>>
>>>> Co-developed-by: Lukas Riezler <lukas.riezler at wolfvision.net>
>>>> Signed-off-by: Lukas Riezler <lukas.riezler at wolfvision.net>
>>>> Signed-off-by: Javier Carrasco <javier.carrasco at wolfvision.net>
>>>>
>>>> ---
>>>>    src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +++++++
>>>>    1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>>> b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>>> index c02c7cf3..efa7dbd5 100644
>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>>> @@ -997,6 +997,13 @@ void PipelineHandlerRkISP1::stopDevice(Camera
>>>> *camera)
>>>>                                    << "Failed to stop parameters for " <<
>>>> camera->id();
> 
> Could you please send patches using plain text e-mails ? HTML message up
> the formatting. I recommend using git-send-email, see
> https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flibcamera.org%2Fcontributing.html%23submitting-patches&data=05%7C02%7Cjavier.carrasco%40wolfvision.net%7Ce74f35d7b2d74c4ba36d08dcf6b3d802%7Ce94ec9da9183471e83b351baa8eb804f%7C1%7C0%7C638656499513440175%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=YQQ3a3CC2krhQ9APaZuZa%2FB8TKKQm4Cv2dfNrg5ma9s%3D&reserved=0
> 
>>>>            }
>>>>
>>>> +       while (!data->queuedRequests_.empty()) {
>>>> +               Request *req = data->queuedRequests_.front();
>>>> +               RkISP1FrameInfo *info = data->frameInfo_.find(req);
>>>> +               data->frameInfo_.destroy(info->frame);
>>>> +               completeRequest(req);
>>>> +       }
>>>> +
>>>
>>> We should never do this - if we need to this, there is clearly a bug
>>> somewhere. stop() makes sure all requests are completed (even with
>>> status = cancelled)
>>>
>>> Refer to PipelineHandler::stop() documentation
>>>
>>>>            ASSERT(data->queuedRequests_.empty());
>>>
>>> What I think is, this is a redundant assert and should be dropped from
>>> RkISP1PipelineHandler::stopDevice()
>>>
>>> We are already in stopDevice() which is called by
>>> PipelineHandler::stop() - which already has this ASSERT
>>> https://eur04.safelinks.protection.outlook.com/?url=
>>> https%3A%2F%2Fgit.libcamera.org%2Flibcamera%2Flibcamera.git%2Ftree%2Fsrc%2Flibcamera%2Fpipeline_handler.cpp%23n377&data=
>>> 05%7C02%7Cjavier.carrasco%40wolfvision.net%7Cceed53d3ea3e474198f908dcf4d9f125%7Ce94ec9da9183471e83b351baa8eb804f%7C1%7C0%7C638654464149558114%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C
>>> &sdata=7tKyOubPVOy6Avz3VQpA7lnUcU%2F%2FGSUWhqAOwYXBz80%3D&reserved=0
> 
> Disabling this would be nice too.
> 
>>> So, we should the drop the ASSERT from rkisp1 pipeline handler and let
>>> the base pipeline handler class handle the ASSERT (which it already does).
>>>
>>> It's redundant and called in wrong order currently. That should solve
>>> the issue!
>>
>> Hello Umang, thank you for your prompt reply.
>>
>> Our very first quick test was removing the ASSERT() to see what happens
>> then, and we hit the second one you refer. They might be redundant, but
>> removing one is not enough, at least in our case.
>>
>> The problem seems to be that a request stays in the pending state and it
>> never changes. That does not happen every time we stop the camera by the
>> way, only sporadically. That's why we think that it might be some
>> sync-issue that could have something to do with the moment when stop()
>> is triggered.
>>
>> When it actually happens, we can see that one request stays in the queue
>> and it is never set to completed or cancelled. The rest of the requests
>> are set to one of those states via tryCompleteRequest(), which in turn
>> calls completeRequest().
>>
>> The completed/cancelled requests are popped out as soon as they reach
>> the front of the queue, but eventually the pending request reaches the
>> front, and the pipeline is never emptied:
>>
>> void PipelineHandler::completeRequest(Request *request)
>> {
>>         Camera *camera = request->_d()->camera();
>>
>>         request->_d()->complete();
>>
>>         Camera::Private *data = camera->_d();
>>
>>         while (!data->queuedRequests_.empty()) {
>>                 Request *req = data->queuedRequests_.front();
>>                 if (req->status() == Request::RequestPending)
>>                         break;
>>
>>                 ASSERT(!req->hasPendingBuffers());
>>                 data->queuedRequests_.pop_front();
>>                 camera->requestComplete(req);
>>         }
>> }
>>
>> which lead us to think that the completeRequest() call for that pending
>> request was never being triggered, and we forced it to happen with the
>> proposed patch in a similar fashion as in the patch we mentioned, maybe
>> naively.
> 
> If you hit this case then we certainly want to fix it, but probably not
> like done in this patch. stopDevice() stops all the V4L2VideoDevice
> instances, and that should complete all buffers (in the cancelled
> state), leading to completion of all requests. There may be a race
> condition somewhere, which needs to be diagnosed. Based on the findings,
> the appropriate patch can be implemented.
> 

Hello Laurent,

Sorry for the annoying signature and HTML style, that was automatically
added by the company before the email left our infrastructure. I have
been told that it should not happen again, but just in case I will reply
with a private account to save everyone from that again. I hope this
reply is cleaner!

Thanks for your feedback, we will delve into the underlying race
condition then.

Best regards,
Javier Carrasco