[PATCH v2] libcamera: software_isp: Clean up pending requests on stop
Milan Zamazal
mzamazal at redhat.com
Wed Nov 6 21:38:39 CET 2024
Hi Laurent,
Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
> Hi Milan,
>
> Discussing this fix today made me realized I forgot to reply to this
> e-mail. Sorry about that.
>
> On Fri, Oct 11, 2024 at 04:09:30PM +0200, Milan Zamazal wrote:
>> Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
>> > On Wed, Oct 09, 2024 at 09:43:42PM +0200, Milan Zamazal wrote:
>> >> Laurent Pinchart writes:
>> >> > On Wed, Oct 09, 2024 at 06:51:36PM +0200, Milan Zamazal wrote:
>> >> >> Laurent Pinchart writes:
>> >> >> > On Wed, Oct 09, 2024 at 10:35:56AM +0200, Milan Zamazal wrote:
>> >> >> >> PipelineHandler::stop() calls stopDevice() method to perform pipeline
>> >> >> >> specific cleanup and then completes waiting requests. If any queued
>> >> >> >> requests remain, an assertion error is raised.
>> >> >> >>
>> >> >> >> Software ISP stores request buffers in
>> >> >> >> SimpleCameraData::conversionQueue_ and queues them as V4L2 signals
>> >> >> >> bufferReady. stopDevice() cleanup forgets to clean up the buffers and
>> >> >> >> their requests from conversionQueue_, possibly resulting in the
>> >> >> >> assertion error.
>> >> >> >>
>> >> >> >> This patch fixes the omission. The request must be also canceled, which
>> >> >> >> requires introducing a little PipelineHandler::cancelRequest helper in
>> >> >> >> order to be able to access the private cancel() method.
>> >> >> >>
>> >> >> >> The problem wasn't very visible when
>> >> >> >> SimplePipelineHandler::kNumInternalBuffers (the number of buffers
>> >> >> >> allocated in V4L2) was equal to the number of buffers exported from
>> >> >> >> software ISP. But when the number of the exported buffers was increased
>> >> >> >> by one in commit abe2ec64f9e4e97bbdfe3a50372611bd7b5315c2, the assertion
>> >> >> >> error started pop up in some environments. Increasing the number of the
>> >> >> >> buffers much more, e.g. to 9, makes the problem very reproducible.
>> >> >> >>
>> >> >> >> Each pipeline uses its own mechanism to track the requests to clean up
>> >> >> >> and it can't be excluded that similar omissions are present in other
>> >> >> >> places. But there is no obvious way to make a common cleanup for all
>> >> >> >> the pipelines (except for doing it instead of raising the assertion
>> >> >> >> error, which is probably undesirable, in order not to hide incomplete
>> >> >> >> pipeline specific cleanups).
>> >> >> >>
>> >> >> >> Bug: https://bugs.libcamera.org/show_bug.cgi?id=234
>> >> >> >> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> >> >> >> ---
>> >> >> >> include/libcamera/internal/pipeline_handler.h | 1 +
>> >> >> >> src/libcamera/pipeline/simple/simple.cpp | 14 +++++++++++
>> >> >> >> src/libcamera/pipeline_handler.cpp | 23 +++++++++++++------
>> >> >> >> 3 files changed, 31 insertions(+), 7 deletions(-)
>> >> >> >>
>> >> >> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
>> >> >> >> index 0d3808036..fb28a18d0 100644
>> >> >> >> --- a/include/libcamera/internal/pipeline_handler.h
>> >> >> >> +++ b/include/libcamera/internal/pipeline_handler.h
>> >> >> >> @@ -60,6 +60,7 @@ public:
>> >> >> >>
>> >> >> >> bool completeBuffer(Request *request, FrameBuffer *buffer);
>> >> >> >> void completeRequest(Request *request);
>> >> >> >> + void cancelRequest(Request *request);
>> >> >> >>
>> >> >> >> std::string configurationFile(const std::string &subdir,
>> >> >> >> const std::string &name) const;
>> >> >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> >> >> >> index 3ddce71d3..e862ef00f 100644
>> >> >> >> --- a/src/libcamera/pipeline/simple/simple.cpp
>> >> >> >> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> >> >> >> @@ -284,6 +284,7 @@ public:
>> >> >> >> std::vector<std::unique_ptr<FrameBuffer>> conversionBuffers_;
>> >> >> >> std::queue<std::map<const Stream *, FrameBuffer *>> conversionQueue_;
>> >> >> >> bool useConversion_;
>> >> >> >> + void clearIncompleteRequests();
>> >> >> >>
>> >> >> >> std::unique_ptr<Converter> converter_;
>> >> >> >> std::unique_ptr<SoftwareIsp> swIsp_;
>> >> >> >> @@ -897,6 +898,18 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)
>> >> >> >> pipe->completeRequest(request);
>> >> >> >> }
>> >> >> >>
>> >> >> >> +void SimpleCameraData::clearIncompleteRequests()
>> >> >> >> +{
>> >> >> >> + while (!conversionQueue_.empty()) {
>> >> >> >> + for (auto &item : conversionQueue_.front()) {
>> >> >> >> + FrameBuffer *outputBuffer = item.second;
>> >> >> >> + Request *request = outputBuffer->request();
>> >> >> >> + pipe()->cancelRequest(request);
>> >> >> >
>> >> >> > Aren't you cancelling the same request multiple times ?
>> >> >>
>> >> >> Possibly yes. I'll add a check for RequestPending status.
>> >> >
>> >> > How about modifying conversionQueue_ to store instances of a structure
>> >> > that contain a Request pointer in addition to the std::map ?
>> >>
>> >> I prefer keeping data structures simple. What would be the advantage
>> >> worth of maintaining the extra pointer? I'd be inclined to have it if
>> >> it served more purposes, but is it worth just for the cleanup?
>> >
>> > As far as I understand, the conversionQueue_ contains a map of streams
>> > to output buffers for one request. If you look at
>> > SimpleCameraData::bufferReady(), you'll see, in the
>> > !FrameMetadata::FrameSuccess path at the top of the function,
>> >
>> > Request *request = nullptr;
>> > for (auto &item : conversionQueue_.front()) {
>> > FrameBuffer *outputBuffer = item.second;
>> > request = outputBuffer->request();
>> > pipe->completeBuffer(request, outputBuffer);
>> > }
>> > conversionQueue_.pop();
>> >
>> > if (request)
>> > pipe->completeRequest(request);
>> >
>> > All buffers need to be completed individually, but the request needs to
>> > be completed once only.
>>
>> But it is completed only if:
>>
>> - There is at least one output buffer
>> - AND the buffer refers to the given request.
>>
>> Those look like reasonable assumptions but I'm not sure there is nothing
>> subtle behind. You authored the code structure above when adding
>> support for multiple streams so can you confirm that something like
>>
>> std::queue<std::pair<Request *, std::map<const Stream *, FrameBuffer *>>> conversionQueue_;
>>
>> ...
>>
>> Request *request = conversionQueue_.front().first;
>> for (auto &item : conversionQueue_.front().second)
>> pipe->completeBuffer(request, item.second);
>> pipe->completeRequest(request);
>> conversionQueue_.pop();
>>
>> would be equivalent under the right assumptions?
>
> My assumption (please tell me if I'm wrong) is that an entry in the
> conversion queue corresponds to one request, and holds a map of streams
> to frame buffers for that particular request.
OK.
> We have code that needs to deal with those buffers, and also code that
> needs to deal with the request. Currently, the request is retrieved
> from the buffer. In the case of SimpleCameraData::bufferReady(), we
> use the request from the last buffer in the map, but any buffer would
> do, as they all belong to the same request.
>
> I find this code confusing, and I believe we should explicitly store the
> request pointer in the conversion queue entry, and retrieve it from
> there, instead of retrieving it from the buffer. It would make the code
> clearer. Your code snippet above looks fine, althouh I would probably
> create a new structure to hold the request pointer and map:
>
> struct ConversionJob {
> Request *request;
> std::map<const Stream *, FrameBuffer *> buffers;
> };
>
> std::queue<ConversionJob> conversionQueue_;
>
> (we can bikeshed the ConversionJob name). The code could then look like
>
> Request *request = nullptr;
> const ConversionJob &job = conversionQueue_.front();
>
> for (auto &[stream, outputBuffer] : job)
> pipe->completeBuffer(request, outputBuffer);
>
> pipe->completeRequest(job.request);
> conversionQueue_.pop();
>
> which I think is much more readable.
I think the eventual code in v6 is basically the same, minus naming and
omitting `request' variable. And since it's based on a snippet you
wrote, I suppose it should be OK for you :-) but tell me in case any
further adjustments are needed.
>> > All the output buffers in the map relate to the same request, so I think
>> > it makes sense to store the request pointer separately in the queue for
>> > easy access. Alternatively, you could have a code construct similar to
>> > the above, getting the request pointer from the buffer, and calling
>> > completeRequest() once only. It would be nice if we could share more
>> > code, replacing the above construct with something shared by the cancel
>> > path.
>> >
>> >> >> >> + }
>> >> >> >> + conversionQueue_.pop();
>> >> >> >> + }
>> >> >> >> +}
>> >> >> >> +
>> >> >> >> void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
>> >> >> >> {
>> >> >> >> swIsp_->processStats(frame, bufferId,
>> >> >> >> @@ -1407,6 +1420,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>> >> >> >> video->bufferReady.disconnect(data, &SimpleCameraData::bufferReady);
>> >> >> >>
>> >> >> >> data->conversionBuffers_.clear();
>> >> >> >> + data->clearIncompleteRequests();
>> >> >> >>
>> >> >> >> releasePipeline(data);
>> >> >> >> }
>> >> >> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
>> >> >> >> index e59404691..c9cb11f0f 100644
>> >> >> >> --- a/src/libcamera/pipeline_handler.cpp
>> >> >> >> +++ b/src/libcamera/pipeline_handler.cpp
>> >> >> >> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)
>> >> >> >> while (!waitingRequests_.empty()) {
>> >> >> >> Request *request = waitingRequests_.front();
>> >> >> >> waitingRequests_.pop();
>> >> >> >> -
>> >> >> >> - request->_d()->cancel();
>> >> >> >> - completeRequest(request);
>> >> >> >> + cancelRequest(request);
>> >> >> >> }
>> >> >> >>
>> >> >> >> /* Make sure no requests are pending. */
>> >> >> >> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)
>> >> >> >> }
>> >> >> >>
>> >> >> >> int ret = queueRequestDevice(camera, request);
>> >> >> >> - if (ret) {
>> >> >> >> - request->_d()->cancel();
>> >> >> >> - completeRequest(request);
>> >> >> >> - }
>> >> >> >> + if (ret)
>> >> >> >> + cancelRequest(request);
>> >> >> >> }
>> >> >> >>
>> >> >> >> /**
>> >> >> >> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)
>> >> >> >> }
>> >> >> >> }
>> >> >> >>
>> >> >> >> +/**
>> >> >> >> + * \brief Cancel request and signal its completion
>> >> >> >> + * \param[in] request The request to cancel
>> >> >> >> + *
>> >> >> >> + * This function cancels the request in addition to its completion. The same
>> >> >> >> + * rules as for completeRequest() apply.
>> >> >> >> + */
>> >> >> >> +void PipelineHandler::cancelRequest(Request *request)
>> >> >> >> +{
>> >> >> >> + request->_d()->cancel();
>> >> >> >> + completeRequest(request);
>> >> >> >> +}
>> >> >> >> +
>> >> >> >> /**
>> >> >> >> * \brief Retrieve the absolute path to a platform configuration file
>> >> >> >> * \param[in] subdir The pipeline handler specific subdirectory name
More information about the libcamera-devel
mailing list