| Message ID | 20210510105235.28319-3-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2021-05-10 12:52:29 +0200, 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); > + > + 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); > + } I wonder how this works if a buffer have been queued to hardware but not yet completed? Do we need a new Request status RequestProcessing(?) to deal with such a corner case or maybe it's not a problem? > + > + 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, thank you for the patch. On Tue, May 11, 2021 at 5:25 AM Niklas Söderlund < niklas.soderlund@ragnatech.se> wrote: > Hi Jacopo, > > Thanks for your work. > > On 2021-05-10 12:52:29 +0200, 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); > > + > > + 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); > Shall we do (*buffer)->metadata().status = FrameMetadata::FrameCancelled? > > + buffer = pending_.erase(buffer); > > + } > > I wonder how this works if a buffer have been queued to hardware but not > yet completed? Do we need a new Request status RequestProcessing(?) to > deal with such a corner case or maybe it's not a problem? > > I think Request::cancel() should be called when a request and its buffers are not in the hardware. -Hiro > > + > > + 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 > > -- > Regards, > Niklas Söderlund > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Hiro, On Tue, May 11, 2021 at 01:45:26PM +0900, Hirokazu Honda wrote: > Hi Jacopo, thank you for the patch. > > On Tue, May 11, 2021 at 5:25 AM Niklas Söderlund < > niklas.soderlund@ragnatech.se> wrote: > > > Hi Jacopo, > > > > Thanks for your work. > > > > On 2021-05-10 12:52:29 +0200, 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); > > > + > > > + 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); > > > > Shall we do (*buffer)->metadata().status = FrameMetadata::FrameCancelled? That's exactly what (*buffer)->cancel() does :) class FrameBuffer { .... void cancel() { metadata_.status = FrameMetadata::FrameCancelled; } } > > > > > + buffer = pending_.erase(buffer); > > > + } > > > > I wonder how this works if a buffer have been queued to hardware but not > > yet completed? Do we need a new Request status RequestProcessing(?) to > > deal with such a corner case or maybe it's not a problem? > > > > > I think Request::cancel() should be called when a request and its buffers > are not in the hardware. Ideally yes, but there's one issue. Once we add this method it becomes application visible, and application can call it, at any time, beside the fact that ph should be diligent and keep the state clean. Should then queueRequestDevice look like: ret = csi2.queueRawBuffer(raw) if (ret) return ret; ret = isp.queueViewfinderBuffer(buffer); if (ret) { dequeueRawBuffer() return ret; } is this even possible ? > > -Hiro > > > > > + > > > + 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 > > > > -- > > Regards, > > Niklas Söderlund > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > >
Hi Jacopo, On Tue, May 11, 2021 at 5:26 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > Hi Hiro, > > On Tue, May 11, 2021 at 01:45:26PM +0900, Hirokazu Honda wrote: > > Hi Jacopo, thank you for the patch. > > > > On Tue, May 11, 2021 at 5:25 AM Niklas Söderlund < > > niklas.soderlund@ragnatech.se> wrote: > > > > > Hi Jacopo, > > > > > > Thanks for your work. > > > > > > On 2021-05-10 12:52:29 +0200, 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); > > > > + > > > > + 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); > > > > > > > Shall we do (*buffer)->metadata().status = FrameMetadata::FrameCancelled? > > That's exactly what (*buffer)->cancel() does :) > > class FrameBuffer > { > .... > > void cancel() { metadata_.status = FrameMetadata::FrameCancelled; } > > } > > Ah, Ack. > > > > > > > > + buffer = pending_.erase(buffer); > > > > + } > > > > > > I wonder how this works if a buffer have been queued to hardware but > not > > > yet completed? Do we need a new Request status RequestProcessing(?) to > > > deal with such a corner case or maybe it's not a problem? > > > > > > > > I think Request::cancel() should be called when a request and its buffers > > are not in the hardware. > > Ideally yes, but there's one issue. Once we add this method it becomes > application visible, and application can call it, at any time, beside > the fact that ph should be diligent and keep the state clean. Should > then queueRequestDevice look like: > > ret = csi2.queueRawBuffer(raw) > if (ret) > return ret; > > ret = isp.queueViewfinderBuffer(buffer); > if (ret) { > dequeueRawBuffer() > return ret; > } > > is this even possible ? > > I think so. IPU3 is implemented so, isn't it? > > > > -Hiro > > > > > > > > + > > > > + 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 > > > > > > -- > > > Regards, > > > Niklas Söderlund > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > >
Hi Hiro, On Tue, May 11, 2021 at 06:20:30PM +0900, Hirokazu Honda wrote: > Hi Jacopo, > > On Tue, May 11, 2021 at 5:26 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > Hi Hiro, > > > > On Tue, May 11, 2021 at 01:45:26PM +0900, Hirokazu Honda wrote: > > > Hi Jacopo, thank you for the patch. > > > > > > On Tue, May 11, 2021 at 5:25 AM Niklas Söderlund < > > > niklas.soderlund@ragnatech.se> wrote: > > > > > > > Hi Jacopo, > > > > > > > > Thanks for your work. > > > > > > > > On 2021-05-10 12:52:29 +0200, 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); > > > > > + > > > > > + 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); > > > > > > > > > > Shall we do (*buffer)->metadata().status = FrameMetadata::FrameCancelled? > > > > That's exactly what (*buffer)->cancel() does :) > > > > class FrameBuffer > > { > > .... > > > > void cancel() { metadata_.status = FrameMetadata::FrameCancelled; } > > > > } > > > > > Ah, Ack. > > > > > > > > > > > > > + buffer = pending_.erase(buffer); > > > > > + } > > > > > > > > I wonder how this works if a buffer have been queued to hardware but > > not > > > > yet completed? Do we need a new Request status RequestProcessing(?) to > > > > deal with such a corner case or maybe it's not a problem? > > > > > > > > > > > I think Request::cancel() should be called when a request and its buffers > > > are not in the hardware. > > > > Ideally yes, but there's one issue. Once we add this method it becomes > > application visible, and application can call it, at any time, beside > > the fact that ph should be diligent and keep the state clean. Should > > then queueRequestDevice look like: > > > > ret = csi2.queueRawBuffer(raw) > > if (ret) > > return ret; > > > > ret = isp.queueViewfinderBuffer(buffer); > > if (ret) { > > dequeueRawBuffer() > > return ret; > > } > > > > is this even possible ? > > > > > I think so. IPU3 is implemented so, isn't it? IPU3 should be ok, as well as RPi as their state machine is IPA driven, so they queue the raw frame and wait for the IPA to compute parameters before queuing buffers to the ISP. Other pipelines might require special attention to exit from a failed queueRequestDevice() in a clean state. But that's very ph specific. Looking at how you're changing the ipu3 one, do you think using Request::cancel() could be useful ? > > > > > > > > -Hiro > > > > > > > > > > > + > > > > > + 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 > > > > > > > > -- > > > > Regards, > > > > Niklas Söderlund > > > > _______________________________________________ > > > > libcamera-devel mailing list > > > > libcamera-devel@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > >
Hi Jacopo, On Tue, May 11, 2021 at 7:44 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > Hi Hiro, > > On Tue, May 11, 2021 at 06:20:30PM +0900, Hirokazu Honda wrote: > > Hi Jacopo, > > > > On Tue, May 11, 2021 at 5:26 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > Hi Hiro, > > > > > > On Tue, May 11, 2021 at 01:45:26PM +0900, Hirokazu Honda wrote: > > > > Hi Jacopo, thank you for the patch. > > > > > > > > On Tue, May 11, 2021 at 5:25 AM Niklas Söderlund < > > > > niklas.soderlund@ragnatech.se> wrote: > > > > > > > > > Hi Jacopo, > > > > > > > > > > Thanks for your work. > > > > > > > > > > On 2021-05-10 12:52:29 +0200, 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); > > > > > > + > > > > > > + 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); > > > > > > > > > > > > > Shall we do (*buffer)->metadata().status = > FrameMetadata::FrameCancelled? > > > > > > That's exactly what (*buffer)->cancel() does :) > > > > > > class FrameBuffer > > > { > > > .... > > > > > > void cancel() { metadata_.status = > FrameMetadata::FrameCancelled; } > > > > > > } > > > > > > > > Ah, Ack. > > > > > > > > > > > > > > > > > > + buffer = pending_.erase(buffer); > > > > > > + } > > > > > > > > > > I wonder how this works if a buffer have been queued to hardware > but > > > not > > > > > yet completed? Do we need a new Request status > RequestProcessing(?) to > > > > > deal with such a corner case or maybe it's not a problem? > > > > > > > > > > > > > > I think Request::cancel() should be called when a request and its > buffers > > > > are not in the hardware. > > > > > > Ideally yes, but there's one issue. Once we add this method it becomes > > > application visible, and application can call it, at any time, beside > > > the fact that ph should be diligent and keep the state clean. Should > > > then queueRequestDevice look like: > > > > > > ret = csi2.queueRawBuffer(raw) > > > if (ret) > > > return ret; > > > > > > ret = isp.queueViewfinderBuffer(buffer); > > > if (ret) { > > > dequeueRawBuffer() > > > return ret; > > > } > > > > > > is this even possible ? > > > > > > > > I think so. IPU3 is implemented so, isn't it? > > IPU3 should be ok, as well as RPi as their state machine is IPA > driven, so they queue the raw frame and wait for the IPA to compute > parameters before queuing buffers to the ISP. > > Other pipelines might require special attention to exit from a failed > queueRequestDevice() in a clean state. But that's very ph specific. > Looking at how you're changing the ipu3 one, do you think using > Request::cancel() could be useful ? > > Yes, I did the same thing in the change. I will replace the code with Request::cancel(). > > > > > > > > > > > > -Hiro > > > > > > > > > > > > > > + > > > > > > + 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 > > > > > > > > > > -- > > > > > Regards, > > > > > Niklas Söderlund > > > > > _______________________________________________ > > > > > libcamera-devel mailing list > > > > > libcamera-devel@lists.libcamera.org > > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > > > > >
Hello, On Tue, May 11, 2021 at 10:26:55AM +0200, Jacopo Mondi wrote: > Hi Hiro, > > On Tue, May 11, 2021 at 01:45:26PM +0900, Hirokazu Honda wrote: > > Hi Jacopo, thank you for the patch. > > > > On Tue, May 11, 2021 at 5:25 AM Niklas Söderlund < > > niklas.soderlund@ragnatech.se> wrote: > > > > > Hi Jacopo, > > > > > > Thanks for your work. > > > > > > On 2021-05-10 12:52:29 +0200, 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); > > > > + > > > > + 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); > > > > > > > Shall we do (*buffer)->metadata().status = FrameMetadata::FrameCancelled? > > That's exactly what (*buffer)->cancel() does :) > > class FrameBuffer > { > .... > > void cancel() { metadata_.status = FrameMetadata::FrameCancelled; } > > } > > > > > > > > > + buffer = pending_.erase(buffer); > > > > + } > > > > > > I wonder how this works if a buffer have been queued to hardware but not > > > yet completed? Do we need a new Request status RequestProcessing(?) to > > > deal with such a corner case or maybe it's not a problem? > > > > > > > > I think Request::cancel() should be called when a request and its buffers > > are not in the hardware. > > Ideally yes, but there's one issue. Once we add this method it becomes > application visible, and application can call it, at any time, beside Just to add that I made the method private and only accessible to friend class for the moment for this specific reason.
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
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(+)