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

[Laurent Pinchart]

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://libcamera.org/contributing.html#submitting-patches

> >>            }
> >>
> >> +       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.

-- 
Regards,

Laurent Pinchart