[libcamera-devel,v2,2/8] libcamera: request: Add Request::cancel()

12264 diff mbox series

Add a cancel() function to the Request class that allows to forcefully
complete the request and its associated buffers in error state.

Only pending requests can be forcefully cancelled. Enforce that
by asserting the request state to be RequestPending.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/request.h |  1 +
 src/libcamera/request.cpp   | 30 ++++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

Hi Jacopo, thank you for the patch.

On Thu, May 13, 2021 at 6:22 PM Jacopo Mondi <jacopo@jmondi.org> wrote:

> Add a cancel() function to the Request class that allows to forcefully
> complete the request and its associated buffers in error state.
>
> Only pending requests can be forcefully cancelled. Enforce that
> by asserting the request state to be RequestPending.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>

Reviewed-by: Hirokazu Honda <hiroh@chromium.org>


> ---
>  include/libcamera/request.h |  1 +
>  src/libcamera/request.cpp   | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+)
>
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 4cf5ff3f7d3b..5596901ddd8e 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -65,6 +65,7 @@ private:
>         friend class PipelineHandler;
>
>         void complete();
> +       void cancel();
>
>         bool completeBuffer(FrameBuffer *buffer);
>
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index ce2dd7b17f10..fc5e25199112 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -292,6 +292,36 @@ void Request::complete()
>         LIBCAMERA_TRACEPOINT(request_complete, this);
>  }
>
> +/**
> + * \brief Cancel a queued request
> + *
> + * Mark the request and its associated buffers as cancelled and complete
> it.
> + *
> + * Set the status of each buffer in the request to the frame cancelled
> state and
> + * remove them from the pending buffer queue before completing the
> request with
> + * error.
> + */
> +void Request::cancel()
> +{
> +       LIBCAMERA_TRACEPOINT(request_cancel, this);
> +
> +       ASSERT(status_ == RequestPending);
> +
> +       /*
> +        * We can't simply loop and call completeBuffer() as erase()
> invalidates
> +        * pointers and iterators, so we have to manually cancel the
> buffer and
> +        * erase it from the pending buffers list.
> +        */
> +       for (auto buffer = pending_.begin(); buffer != pending_.end();) {
> +               (*buffer)->cancel();
> +               (*buffer)->setRequest(nullptr);
> +               buffer = pending_.erase(buffer);
> +       }
> +
> +       cancelled_ = true;
> +       complete();
> +}
> +
>  /**
>   * \brief Complete a buffer for the request
>   * \param[in] buffer The buffer that has completed
> --
> 2.31.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>

Hi Jacopo,

On 13/05/2021 10:22, Jacopo Mondi wrote:
> Add a cancel() function to the Request class that allows to forcefully
> complete the request and its associated buffers in error state.
> 
> Only pending requests can be forcefully cancelled. Enforce that
> by asserting the request state to be RequestPending.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  include/libcamera/request.h |  1 +
>  src/libcamera/request.cpp   | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 4cf5ff3f7d3b..5596901ddd8e 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -65,6 +65,7 @@ private:
>  	friend class PipelineHandler;
>  
>  	void complete();
> +	void cancel();
>  
>  	bool completeBuffer(FrameBuffer *buffer);
>  
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index ce2dd7b17f10..fc5e25199112 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -292,6 +292,36 @@ void Request::complete()
>  	LIBCAMERA_TRACEPOINT(request_complete, this);
>  }
>  
> +/**
> + * \brief Cancel a queued request
> + *
> + * Mark the request and its associated buffers as cancelled and complete it.
> + *
> + * Set the status of each buffer in the request to the frame cancelled state and
> + * remove them from the pending buffer queue before completing the request with
> + * error.
> + */
> +void Request::cancel()
> +{
> +	LIBCAMERA_TRACEPOINT(request_cancel, this);

Have you checked that this is sufficient?

Doesn't this require an addition in
   include/libcamera/internal/tracepoints/request.tp


> +
> +	ASSERT(status_ == RequestPending);
> +
> +	/*
> +	 * We can't simply loop and call completeBuffer() as erase() invalidates
> +	 * pointers and iterators, so we have to manually cancel the buffer and
> +	 * erase it from the pending buffers list.
> +	 */
> +	for (auto buffer = pending_.begin(); buffer != pending_.end();) {
> +		(*buffer)->cancel();

Do buffers need to notify a completion event if they're cancelled? I
don't think they do - but I'm curious if there is a use case to be
notified in a buffer event handler of a buffer being cancelled...



> +		(*buffer)->setRequest(nullptr);

Even if the buffer is cancelled, it's still associated with this request
isn't it?

Does it need to be set to null?


> +		buffer = pending_.erase(buffer);
> +	}
> +
> +	cancelled_ = true;
> +	complete();
> +}
> +
>  /**
>   * \brief Complete a buffer for the request
>   * \param[in] buffer The buffer that has completed
>

Hi Kieran,

On Mon, May 17, 2021 at 12:53:54PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 13/05/2021 10:22, Jacopo Mondi wrote:
> > Add a cancel() function to the Request class that allows to forcefully
> > complete the request and its associated buffers in error state.
> >
> > Only pending requests can be forcefully cancelled. Enforce that
> > by asserting the request state to be RequestPending.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  include/libcamera/request.h |  1 +
> >  src/libcamera/request.cpp   | 30 ++++++++++++++++++++++++++++++
> >  2 files changed, 31 insertions(+)
> >
> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> > index 4cf5ff3f7d3b..5596901ddd8e 100644
> > --- a/include/libcamera/request.h
> > +++ b/include/libcamera/request.h
> > @@ -65,6 +65,7 @@ private:
> >  	friend class PipelineHandler;
> >
> >  	void complete();
> > +	void cancel();
> >
> >  	bool completeBuffer(FrameBuffer *buffer);
> >
> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > index ce2dd7b17f10..fc5e25199112 100644
> > --- a/src/libcamera/request.cpp
> > +++ b/src/libcamera/request.cpp
> > @@ -292,6 +292,36 @@ void Request::complete()
> >  	LIBCAMERA_TRACEPOINT(request_complete, this);
> >  }
> >
> > +/**
> > + * \brief Cancel a queued request
> > + *
> > + * Mark the request and its associated buffers as cancelled and complete it.
> > + *
> > + * Set the status of each buffer in the request to the frame cancelled state and
> > + * remove them from the pending buffer queue before completing the request with
> > + * error.
> > + */
> > +void Request::cancel()
> > +{
> > +	LIBCAMERA_TRACEPOINT(request_cancel, this);
>
> Have you checked that this is sufficient?
>
> Doesn't this require an addition in
>    include/libcamera/internal/tracepoints/request.tp

AH! I had no idea...

It would be nice to be able to catch this automatically, thanks for
spotting!

>
>
> > +
> > +	ASSERT(status_ == RequestPending);
> > +
> > +	/*
> > +	 * We can't simply loop and call completeBuffer() as erase() invalidates
> > +	 * pointers and iterators, so we have to manually cancel the buffer and
> > +	 * erase it from the pending buffers list.
> > +	 */
> > +	for (auto buffer = pending_.begin(); buffer != pending_.end();) {
> > +		(*buffer)->cancel();
>
> Do buffers need to notify a completion event if they're cancelled? I
> don't think they do - but I'm curious if there is a use case to be
> notified in a buffer event handler of a buffer being cancelled...

We discussed buffer completion in relation to cancel here
https://patchwork.libcamera.org/patch/12243/

Although my position at the time was that buffers should be completed
by the caller of Request::cancel() as they know better and for some
use cases it is better to let the caller handle that. However it is
also true that we're iterating on the pending_ buffers, so my argument
on partial request completion is defintely moot....

Hiro suggested to emit buffer completion in Request::cancel() would
you be in favour of that as well ? I'm not too at ease in emitting a
signal on the camera from the Request class though.

>
>
>
> > +		(*buffer)->setRequest(nullptr);
>
> Even if the buffer is cancelled, it's still associated with this request
> isn't it?
>
> Does it need to be set to null?

Nope, especially if the completion signal is emitted later. The
handler would receive a buffer with no Request set, which is
dangerous.

I'll change this function to

        for (buf : pending_) {
                buf->cancel();
                /* Is this a layer violation ? */
                camera->bufferCompleted.emit(this, buf);
                buf = pending_.erase(buffer);
        }

        cancelled_ = true;
        complete();

What do you think ?

>
>
> > +		buffer = pending_.erase(buffer);
> > +	}
> > +
> > +	cancelled_ = true;
> > +	complete();
> > +}
> > +
> >  /**
> >   * \brief Complete a buffer for the request
> >   * \param[in] buffer The buffer that has completed
> >
>
> --
> Regards
> --
> Kieran