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

[Naushir Patuck]

Hi Kieran,


On Wed, 26 Feb 2020 at 14:24, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> 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;
> }
>

Initially, this is what I was doing as well, but seems it was not enough.

>
> 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;
> }
>

Yes, but I thought that mechanism was only to mark the buffers
FrameMetadata::FrameCancelled, and causes Request::complete() to set
the appropriate stats

void Request::complete()
{
   ASSERT(!hasPendingBuffers());
   status_ = cancelled_ ? RequestCancelled : RequestComplete;
}

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

I did not see where this might be happening.  From my brief
investigation, CameraData::queuedRequests_ maintains an internal list
of pending requests.  The only place this list gets cleared is in
PipelineHandler::completeRequest.  So I was under the impression that
my pipeline handler should also maintain a list of Requests queued,
and call PipelineHandler::completeReques() on all those elements
during a Stop().  By the way, I noticed the UVC pipeline handler does
not do this, so I could be completely wrong about this!

void PipelineHandler::completeRequest(Camera *camera, Request *request)
{
   request->complete();

   CameraData *data = cameraData(camera);

   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);
   }
}

If I called the above function on all the queued requests, the assert
would fire as the buffers in the request are not deleted via
Request::completeBuffer().

>
> > 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?
>

See above :)

>
> > 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

Thanks,
Naush