[libcamera-devel] [RFC PATCH v3 3/3] libcamera: ipu3: Cancel pending requests correctly

[Hirokazu Honda]

Hi Laurent and Kieran,

On Wed, Apr 21, 2021 at 5:44 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Kieran,
>
> On Tue, Apr 20, 2021 at 11:48:16AM +0100, Kieran Bingham wrote:
> > On 20/04/2021 03:51, Laurent Pinchart wrote:
> > > On Thu, Apr 08, 2021 at 05:51:01PM +0900, Hirokazu Honda wrote:
> > >> IPU3CameraData stores requests that are not queued yet to a
> > >> camera node. They should be reported as cancel upon
> > >> PipelineHandlerIPU3::stop().
> > >>
> > >> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > >> ---
> > >>  include/libcamera/buffer.h           |  3 +++
> > >>  src/libcamera/pipeline/ipu3/ipu3.cpp | 19 ++++++++++++++++++-
> > >>  2 files changed, 21 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h
> > >> index 620f8a66..72c62144 100644
> > >> --- a/include/libcamera/buffer.h
> > >> +++ b/include/libcamera/buffer.h
> > >> @@ -58,6 +58,9 @@ private:
> > >>
> > >>    friend class V4L2VideoDevice; /* Needed to update metadata_. */
> > >>
> > >> +  /*! HACK! */
> > >> +  friend class IPU3CameraData; /* Needed to update metadata_. */
> > >
> > > Looks like we need a way to handle this cleanly :-) It may relate to the
> > > discussion we had earlier, about the rework of the buffer and requests
> > > state handling. I think a redesign of that mechanism, with a cleaner API
> > > for completion, would be nice. Making the Request class inherit from
> > > Extensible, with Request::Private being defined in
> > > include/libcamera/internal/request.h and offering public functions for
> > > the pipeline handlers may be part of the solution. The "Private" name in
> > > the our Extensible design pattern (which implements the p-impl design
> > > pattern) means private to applications, in the public API, but it can be
> > > accessed within libcamera.
> >
> > I have an as-yet-unpublished patch to make Request use Extensible, and I
> > also have a patch to provide a 'cancel' operation on Buffers.
> >
> > Perhaps I should send those out ...
>
> That would be a good way to move the discussion forward.
>

I got it.
I will submit the next patch series on top of Kieran's patch then.
-Hiro

> > > The rest of the patch looks good.
> > >
> > >> +
> > >>    std::vector<Plane> planes_;
> > >>
> > >>    Request *request_;
> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > >> index 462a0d9b..f89b8f3f 100644
> > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > >> @@ -66,6 +66,7 @@ public:
> > >>    void cio2BufferReady(FrameBuffer *buffer);
> > >>    void paramBufferReady(FrameBuffer *buffer);
> > >>    void statBufferReady(FrameBuffer *buffer);
> > >> +  void cancelPendingRequests();
> > >>    int queuePendingRequests();
> > >>
> > >>    CIO2Device cio2_;
> > >> @@ -768,7 +769,7 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> > >>    IPU3CameraData *data = cameraData(camera);
> > >>    int ret = 0;
> > >>
> > >> -  data->pendingRequests_ = {};
> > >> +  data->cancelPendingRequests();
> > >>
> > >>    data->ipa_->stop();
> > >>
> > >> @@ -780,6 +781,22 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> > >>    freeBuffers(camera);
> > >>  }
> > >>
> > >> +void IPU3CameraData::cancelPendingRequests()>> +{
> > >> +  while (!pendingRequests_.empty()) {
> > >> +          Request *request = pendingRequests_.front();
> > >> +
> > >> +          for (auto it : request->buffers()) {
> > >> +                  FrameBuffer *buffer = it.second;
> > >> +                  buffer->metadata_.status = FrameMetadata::FrameCancelled;
> > >> +                  pipe_->completeBuffer(request, buffer);
> > >> +          }
> > >> +
> > >> +          pipe_->completeRequest(request);
> > >> +          pendingRequests_.pop();
> > >> +  }
> > >> +}
> > >> +
> > >>  int IPU3CameraData::queuePendingRequests()
> > >>  {
> > >>    while (!pendingRequests_.empty()) {
>
> --
> Regards,
>
> Laurent Pinchart