| Message ID | 20210408085101.1691729-4-hiroh@chromium.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thank you for the patch. 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@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. 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()) {
Hi Laurent, Hiro, On 20/04/2021 03:51, Laurent Pinchart wrote: > Hi Hiro, > > Thank you for the patch. > > 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@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 ... > 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()) { >
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@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. > > 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()) {
Hi Laurent and Kieran, On Wed, Apr 21, 2021 at 5:44 AM Laurent Pinchart <laurent.pinchart@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@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
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_. */ + 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()) {
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@chromium.org> --- include/libcamera/buffer.h | 3 +++ src/libcamera/pipeline/ipu3/ipu3.cpp | 19 ++++++++++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-)