[libcamera-devel] Request and Pipeline::Stop() behavior

[Kieran Bingham]

Hi Naush,

On 26/02/2020 09:03, Naushir Patuck wrote:
> Hi,
> 
> Could you help with my understanding on how Requests are handled in
> vendor pipeline handlers please?
> 
> Given that multiple requests can be queued, I assume the pipeline
> handlers must maintain a queue of Request types.  Can we use
> CameraData::queuedRequests_ for this, or is that a libcamera internal
> queue and we must duplicate?
> 
> On a PipelineHandler::stop(), I assume we must go through the Request
> queue and call PipelineHandler::completeRequest() on each Request.
> However, this will fail if the Request has any outstanding buffers
> pending for which we need to call PipelineHandler::completeBuffer().

There's another way to look at it. The framework is already in place to
report cancelled buffers up to complete the request (which gets reported
as cancelled).

The RkISP1 handles this issue by simply calling streamOff() on all of
the applicable devices :


void PipelineHandlerRkISP1::stop(Camera *camera)
{
	RkISP1CameraData *data = cameraData(camera);
	int ret;

	ret = video_->streamOff();
	if (ret)
		LOG(RkISP1, Warning)
			<< "Failed to stop camera " << camera->name();

	ret = stat_->streamOff();
	if (ret)
		LOG(RkISP1, Warning)
			<< "Failed to stop statistics " << camera->name();

	ret = param_->streamOff();
	if (ret)
		LOG(RkISP1, Warning)
			<< "Failed to stop parameters " << camera->name();

	data->timeline_.reset();

	freeBuffers(camera);

	activeCamera_ = nullptr;
}


This has the effect that all buffers queued will be marked as
'cancelled' and returned as part of V4L2VideoDevice through the
bufferReady signal:


int V4L2VideoDevice::streamOff()
{
	int ret;

	ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);
	if (ret < 0) {
		LOG(V4L2, Error)
			<< "Failed to stop streaming: " << strerror(-ret);
		return ret;
	}

	/* Send back all queued buffers. */
	for (auto it : queuedBuffers_) {
		FrameBuffer *buffer = it.second;

		buffer->metadata_.status = FrameMetadata::FrameCancelled;
		bufferReady.emit(buffer);
	}

	queuedBuffers_.clear();
	fdEvent_->setEnabled(false);

	return 0;
}


And once all buffers have completed (with their status as cancelled) the
Request will (I think) automatically be cancelled too.


> This in-turn means that each pipeline handler must have a mechanism to
> go through all Requests and call PipelineHandler::completeBuffer() on
> all streams.  If we call PipelineHandler::completeBuffer() on a stream
> that has already previously called PipelineHandler::completeBuffer()
> for an in-flight Request, we hit an assert.  So the pipeline handlers
> must maintain state of what streams/buffers in the Request is
> completed to clean up correctly in these situations.
> 
> Now this is not difficult to maintain state in the pipeline handers,
> but seems like it would be duplicated code for all vendors.  Given
> that there is already a notion of RequestCancelled (in
> Request::status_), would it make state to have a
> PipelineHandler::cancelRequest() method that explicitly does a
> completeBuffer() on all outstanding streams in the Request and markes
> status_ as RequestCancelled.  This would avoid any code duplication in
> the various vendor pipeline handlers.

If we do indeed need anything generic to handle this in the other
direction - then I would agree we should offer a cancel() operation on
the Request which would perform things in the reverse direction, and
would thus mark all buffers as cancelled (and ideally wait/ensure that
they are released from hardware) before returning.

But it's the 'release from hardware' part that makes me think
maintaining the current method of stopping the hardware is appropriate
in this case.

If in the future we discover we need to stop a single request without
stopping the pipeline - that might cause a rethink here...


Do you have a use case where just stopping the V4L2VideoDevice is not
enough to cancel all of the queued Requests?


> Apologies if I have misunderstood and this is not how any of this is
> meant to work, please do let me know :)

No apologies required :-) I suspect that as we are lacking a "Pipeline
Implementers Guidebook" - all questions are valid :-)

-- 
Regards
--
Kieran