| Message ID | 20220629103018.4025635-10-chenghaoyang@google.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hello, On 6/29/22 16:00, Harvey Yang via libcamera-devel wrote: > Like the shipped ipu3 HAL, handle the frame delay with one extra input, Can you point to the shipped ipu3 HAL frame delay handling code here for reference? > needed for the first frame and StillCapture's non-sequential frame > requests. > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 73 +++++++++++++++++++++++----- > 1 file changed, 62 insertions(+), 11 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index e26c2736..8689cf8b 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -90,6 +90,9 @@ public: > > ControlInfoMap ipaControls_; > > + bool firstRequest_ = true; > + int lastRequestSequence_ = -1; this is only used under hasCapture so maybe lastCaptureRequestSeq_ is more appropriate? > + > private: > void metadataReady(unsigned int id, const ControlList &metadata); > void paramsBufferReady(unsigned int id); > @@ -565,6 +568,9 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > CIO2Device *cio2 = &data->cio2_; > V4L2DeviceFormat outputFormat; > > + data->firstRequest_ = true; > + data->lastRequestSequence_ = -1; > + Does this needs to be reset on every start()/stop() cycles? > ImgUDevice::PipeConfig imguConfig1 = config->imguConfig1(); > useImgu1_ = !imguConfig1.isNull(); > > @@ -1427,6 +1433,11 @@ void IPU3CameraData::paramsBufferReady(unsigned int id) > if (!info) > return; > > + int yuvCount = firstRequest_ ? 2 : 1; > + int stillCount = firstRequest_ || (lastRequestSequence_ + 1) != static_cast<int>(info->request->sequence()) ? 2 : 1; > + This is getting a bit non-trivial to follow. Can you probably include a brief comment here to explain some rationale? As per my understanding, if the lastRequestSequence_ was a StillCapture, stillCount will be 1 for current request capture, and it will 2 for all other cases? We need stillCount to be 2 (usually) because .... ? > + firstRequest_ = false; > + > bool hasYuv = false; > bool hasCapture = false; > /* Queue all buffers from the request aimed for the ImgU. */ > @@ -1436,33 +1447,53 @@ void IPU3CameraData::paramsBufferReady(unsigned int id) > > if (stream == &outStream_) { > hasYuv = true; > - imgu0_->output_->queueBuffer(outbuffer); > + > + for (int i = 0; i < yuvCount; ++i) { > + bufferReturnCounters[outbuffer] += 1; > + imgu0_->output_->queueBuffer(outbuffer); Have you checked / discussed what happens if you re-queue the same buffer on videodevice twice (before even dequeuing it)? I am not sure, because the current documentation doesn't specify this case I think. I've asked around. > + } > } else if (stream == &vfStream_) { > hasYuv = true; > - imgu0_->viewfinder_->queueBuffer(outbuffer); > + > + for (int i = 0; i < yuvCount; ++i) { > + bufferReturnCounters[outbuffer] += 1; > + imgu0_->viewfinder_->queueBuffer(outbuffer); > + } > } else if (stream == &outCaptureStream_) { > hasCapture = true; > > - imgu1_->output_->queueBuffer(outbuffer); > + for (int i = 0; i < stillCount; ++i) { > + bufferReturnCounters[outbuffer] += 1; > + imgu1_->output_->queueBuffer(outbuffer); > + } > } > } > > if (hasYuv) { > - bufferReturnCounters[info->rawBuffer] += 1; > - > - imgu0_->param_->queueBuffer(info->paramBuffer); > - imgu0_->stat_->queueBuffer(info->statBuffer); > - imgu0_->input_->queueBuffer(info->rawBuffer); > + for (int i = 0; i < yuvCount; ++i) { > + bufferReturnCounters[info->paramBuffer] += 1; > + bufferReturnCounters[info->statBuffer] += 1; > + bufferReturnCounters[info->rawBuffer] += 1; > + > + imgu0_->param_->queueBuffer(info->paramBuffer); > + imgu0_->stat_->queueBuffer(info->statBuffer); > + imgu0_->input_->queueBuffer(info->rawBuffer); > + } > } else { > info->paramDequeued = true; > info->metadataProcessed = true; > } > > if (hasCapture) { > - bufferReturnCounters[info->rawBuffer] += 1; > + lastRequestSequence_ = info->request->sequence(); > > - imgu1_->param_->queueBuffer(info->captureParamBuffer); > - imgu1_->input_->queueBuffer(info->rawBuffer); > + for (int i = 0; i < stillCount; ++i) { > + bufferReturnCounters[info->captureParamBuffer] += 1; > + bufferReturnCounters[info->rawBuffer] += 1; > + > + imgu1_->param_->queueBuffer(info->captureParamBuffer); > + imgu1_->input_->queueBuffer(info->rawBuffer); > + } > } else { > info->captureParamDequeued = true; > } > @@ -1503,6 +1534,11 @@ void IPU3CameraData::tryReturnBuffer(FrameBuffer *buffer) > */ > void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) > { > + if (--bufferReturnCounters[buffer] > 0) > + return; > + > + bufferReturnCounters.erase(buffer); > + > IPU3Frames::Info *info = frameInfos_.find(buffer); > if (!info) > return; > @@ -1569,6 +1605,11 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) > > void IPU3CameraData::paramBufferReady(FrameBuffer *buffer) > { > + if (--bufferReturnCounters[buffer] > 0) > + return; > + > + bufferReturnCounters.erase(buffer); > + > IPU3Frames::Info *info = frameInfos_.find(buffer); > if (!info) > return; > @@ -1589,6 +1630,11 @@ void IPU3CameraData::paramBufferReady(FrameBuffer *buffer) > > void IPU3CameraData::captureParamBufferReady(FrameBuffer *buffer) > { > + if (--bufferReturnCounters[buffer] > 0) > + return; > + > + bufferReturnCounters.erase(buffer); > + > IPU3Frames::Info *info = frameInfos_.find(buffer); > if (!info) > return; > @@ -1609,6 +1655,11 @@ void IPU3CameraData::captureParamBufferReady(FrameBuffer *buffer) > > void IPU3CameraData::statBufferReady(FrameBuffer *buffer) > { > + if (--bufferReturnCounters[buffer] > 0) > + return; > + > + bufferReturnCounters.erase(buffer); > + > IPU3Frames::Info *info = frameInfos_.find(buffer); > if (!info) > return;
Hi Umang, On Wed, Jul 27, 2022 at 9:25 PM Umang Jain <umang.jain@ideasonboard.com> wrote: > Hello, > > On 6/29/22 16:00, Harvey Yang via libcamera-devel wrote: > > Like the shipped ipu3 HAL, handle the frame delay with one extra input, > > > Can you point to the shipped ipu3 HAL frame delay handling code here for > reference? > > Sure. Added. > > needed for the first frame and StillCapture's non-sequential frame > > requests. > > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 73 +++++++++++++++++++++++----- > > 1 file changed, 62 insertions(+), 11 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp > b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index e26c2736..8689cf8b 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -90,6 +90,9 @@ public: > > > > ControlInfoMap ipaControls_; > > > > + bool firstRequest_ = true; > > + int lastRequestSequence_ = -1; > > > this is only used under hasCapture so maybe lastCaptureRequestSeq_ is > more appropriate? > > Right, thanks! > > + > > private: > > void metadataReady(unsigned int id, const ControlList &metadata); > > void paramsBufferReady(unsigned int id); > > @@ -565,6 +568,9 @@ int PipelineHandlerIPU3::configure(Camera *camera, > CameraConfiguration *c) > > CIO2Device *cio2 = &data->cio2_; > > V4L2DeviceFormat outputFormat; > > > > + data->firstRequest_ = true; > > + data->lastRequestSequence_ = -1; > > + > > > Does this needs to be reset on every start()/stop() cycles? > > Ah, right. Resetting them when start() should be enough. > > ImgUDevice::PipeConfig imguConfig1 = config->imguConfig1(); > > useImgu1_ = !imguConfig1.isNull(); > > > > @@ -1427,6 +1433,11 @@ void IPU3CameraData::paramsBufferReady(unsigned > int id) > > if (!info) > > return; > > > > + int yuvCount = firstRequest_ ? 2 : 1; > > + int stillCount = firstRequest_ || (lastRequestSequence_ + 1) != > static_cast<int>(info->request->sequence()) ? 2 : 1; > > + > > > This is getting a bit non-trivial to follow. Can you probably include a > brief comment here to explain some rationale? > > As per my understanding, if the lastRequestSequence_ was a StillCapture, > stillCount will be 1 for current request capture, and it will 2 for all > other cases? > > We need stillCount to be 2 (usually) because .... ? > > If it's the first request, or the StillCapture request doesn't come sequentially, we'll need to queue the buffers twice (i.e. |stillCount| being 2) to mitigate the frame delay. Added a comment in the code. Please check :) > > + firstRequest_ = false; > > + > > bool hasYuv = false; > > bool hasCapture = false; > > /* Queue all buffers from the request aimed for the ImgU. */ > > @@ -1436,33 +1447,53 @@ void IPU3CameraData::paramsBufferReady(unsigned > int id) > > > > if (stream == &outStream_) { > > hasYuv = true; > > - imgu0_->output_->queueBuffer(outbuffer); > > + > > + for (int i = 0; i < yuvCount; ++i) { > > + bufferReturnCounters[outbuffer] += 1; > > + imgu0_->output_->queueBuffer(outbuffer); > > > Have you checked / discussed what happens if you re-queue the same > buffer on videodevice twice (before even dequeuing it)? I am not sure, > because the current documentation doesn't specify this case I think. > > I've asked around. > > Yeah, I've tried it on soraka and discussed it with Han-lin & Laurent & Tomasz. It's also the way that the shipped HAL is handling the frame delay. Please check :) > > + } > > } else if (stream == &vfStream_) { > > hasYuv = true; > > - imgu0_->viewfinder_->queueBuffer(outbuffer); > > + > > + for (int i = 0; i < yuvCount; ++i) { > > + bufferReturnCounters[outbuffer] += 1; > > + > imgu0_->viewfinder_->queueBuffer(outbuffer); > > + } > > } else if (stream == &outCaptureStream_) { > > hasCapture = true; > > > > - imgu1_->output_->queueBuffer(outbuffer); > > + for (int i = 0; i < stillCount; ++i) { > > + bufferReturnCounters[outbuffer] += 1; > > + imgu1_->output_->queueBuffer(outbuffer); > > + } > > } > > } > > > > if (hasYuv) { > > - bufferReturnCounters[info->rawBuffer] += 1; > > - > > - imgu0_->param_->queueBuffer(info->paramBuffer); > > - imgu0_->stat_->queueBuffer(info->statBuffer); > > - imgu0_->input_->queueBuffer(info->rawBuffer); > > + for (int i = 0; i < yuvCount; ++i) { > > + bufferReturnCounters[info->paramBuffer] += 1; > > + bufferReturnCounters[info->statBuffer] += 1; > > + bufferReturnCounters[info->rawBuffer] += 1; > > + > > + imgu0_->param_->queueBuffer(info->paramBuffer); > > + imgu0_->stat_->queueBuffer(info->statBuffer); > > + imgu0_->input_->queueBuffer(info->rawBuffer); > > + } > > } else { > > info->paramDequeued = true; > > info->metadataProcessed = true; > > } > > > > if (hasCapture) { > > - bufferReturnCounters[info->rawBuffer] += 1; > > + lastRequestSequence_ = info->request->sequence(); > > > > - imgu1_->param_->queueBuffer(info->captureParamBuffer); > > - imgu1_->input_->queueBuffer(info->rawBuffer); > > + for (int i = 0; i < stillCount; ++i) { > > + bufferReturnCounters[info->captureParamBuffer] += > 1; > > + bufferReturnCounters[info->rawBuffer] += 1; > > + > > + > imgu1_->param_->queueBuffer(info->captureParamBuffer); > > + imgu1_->input_->queueBuffer(info->rawBuffer); > > + } > > } else { > > info->captureParamDequeued = true; > > } > > @@ -1503,6 +1534,11 @@ void IPU3CameraData::tryReturnBuffer(FrameBuffer > *buffer) > > */ > > void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) > > { > > + if (--bufferReturnCounters[buffer] > 0) > > + return; > > + > > + bufferReturnCounters.erase(buffer); > > + > > IPU3Frames::Info *info = frameInfos_.find(buffer); > > if (!info) > > return; > > @@ -1569,6 +1605,11 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer > *buffer) > > > > void IPU3CameraData::paramBufferReady(FrameBuffer *buffer) > > { > > + if (--bufferReturnCounters[buffer] > 0) > > + return; > > + > > + bufferReturnCounters.erase(buffer); > > + > > IPU3Frames::Info *info = frameInfos_.find(buffer); > > if (!info) > > return; > > @@ -1589,6 +1630,11 @@ void IPU3CameraData::paramBufferReady(FrameBuffer > *buffer) > > > > void IPU3CameraData::captureParamBufferReady(FrameBuffer *buffer) > > { > > + if (--bufferReturnCounters[buffer] > 0) > > + return; > > + > > + bufferReturnCounters.erase(buffer); > > + > > IPU3Frames::Info *info = frameInfos_.find(buffer); > > if (!info) > > return; > > @@ -1609,6 +1655,11 @@ void > IPU3CameraData::captureParamBufferReady(FrameBuffer *buffer) > > > > void IPU3CameraData::statBufferReady(FrameBuffer *buffer) > > { > > + if (--bufferReturnCounters[buffer] > 0) > > + return; > > + > > + bufferReturnCounters.erase(buffer); > > + > > IPU3Frames::Info *info = frameInfos_.find(buffer); > > if (!info) > > return; >
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index e26c2736..8689cf8b 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -90,6 +90,9 @@ public: ControlInfoMap ipaControls_; + bool firstRequest_ = true; + int lastRequestSequence_ = -1; + private: void metadataReady(unsigned int id, const ControlList &metadata); void paramsBufferReady(unsigned int id); @@ -565,6 +568,9 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) CIO2Device *cio2 = &data->cio2_; V4L2DeviceFormat outputFormat; + data->firstRequest_ = true; + data->lastRequestSequence_ = -1; + ImgUDevice::PipeConfig imguConfig1 = config->imguConfig1(); useImgu1_ = !imguConfig1.isNull(); @@ -1427,6 +1433,11 @@ void IPU3CameraData::paramsBufferReady(unsigned int id) if (!info) return; + int yuvCount = firstRequest_ ? 2 : 1; + int stillCount = firstRequest_ || (lastRequestSequence_ + 1) != static_cast<int>(info->request->sequence()) ? 2 : 1; + + firstRequest_ = false; + bool hasYuv = false; bool hasCapture = false; /* Queue all buffers from the request aimed for the ImgU. */ @@ -1436,33 +1447,53 @@ void IPU3CameraData::paramsBufferReady(unsigned int id) if (stream == &outStream_) { hasYuv = true; - imgu0_->output_->queueBuffer(outbuffer); + + for (int i = 0; i < yuvCount; ++i) { + bufferReturnCounters[outbuffer] += 1; + imgu0_->output_->queueBuffer(outbuffer); + } } else if (stream == &vfStream_) { hasYuv = true; - imgu0_->viewfinder_->queueBuffer(outbuffer); + + for (int i = 0; i < yuvCount; ++i) { + bufferReturnCounters[outbuffer] += 1; + imgu0_->viewfinder_->queueBuffer(outbuffer); + } } else if (stream == &outCaptureStream_) { hasCapture = true; - imgu1_->output_->queueBuffer(outbuffer); + for (int i = 0; i < stillCount; ++i) { + bufferReturnCounters[outbuffer] += 1; + imgu1_->output_->queueBuffer(outbuffer); + } } } if (hasYuv) { - bufferReturnCounters[info->rawBuffer] += 1; - - imgu0_->param_->queueBuffer(info->paramBuffer); - imgu0_->stat_->queueBuffer(info->statBuffer); - imgu0_->input_->queueBuffer(info->rawBuffer); + for (int i = 0; i < yuvCount; ++i) { + bufferReturnCounters[info->paramBuffer] += 1; + bufferReturnCounters[info->statBuffer] += 1; + bufferReturnCounters[info->rawBuffer] += 1; + + imgu0_->param_->queueBuffer(info->paramBuffer); + imgu0_->stat_->queueBuffer(info->statBuffer); + imgu0_->input_->queueBuffer(info->rawBuffer); + } } else { info->paramDequeued = true; info->metadataProcessed = true; } if (hasCapture) { - bufferReturnCounters[info->rawBuffer] += 1; + lastRequestSequence_ = info->request->sequence(); - imgu1_->param_->queueBuffer(info->captureParamBuffer); - imgu1_->input_->queueBuffer(info->rawBuffer); + for (int i = 0; i < stillCount; ++i) { + bufferReturnCounters[info->captureParamBuffer] += 1; + bufferReturnCounters[info->rawBuffer] += 1; + + imgu1_->param_->queueBuffer(info->captureParamBuffer); + imgu1_->input_->queueBuffer(info->rawBuffer); + } } else { info->captureParamDequeued = true; } @@ -1503,6 +1534,11 @@ void IPU3CameraData::tryReturnBuffer(FrameBuffer *buffer) */ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) { + if (--bufferReturnCounters[buffer] > 0) + return; + + bufferReturnCounters.erase(buffer); + IPU3Frames::Info *info = frameInfos_.find(buffer); if (!info) return; @@ -1569,6 +1605,11 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) void IPU3CameraData::paramBufferReady(FrameBuffer *buffer) { + if (--bufferReturnCounters[buffer] > 0) + return; + + bufferReturnCounters.erase(buffer); + IPU3Frames::Info *info = frameInfos_.find(buffer); if (!info) return; @@ -1589,6 +1630,11 @@ void IPU3CameraData::paramBufferReady(FrameBuffer *buffer) void IPU3CameraData::captureParamBufferReady(FrameBuffer *buffer) { + if (--bufferReturnCounters[buffer] > 0) + return; + + bufferReturnCounters.erase(buffer); + IPU3Frames::Info *info = frameInfos_.find(buffer); if (!info) return; @@ -1609,6 +1655,11 @@ void IPU3CameraData::captureParamBufferReady(FrameBuffer *buffer) void IPU3CameraData::statBufferReady(FrameBuffer *buffer) { + if (--bufferReturnCounters[buffer] > 0) + return; + + bufferReturnCounters.erase(buffer); + IPU3Frames::Info *info = frameInfos_.find(buffer); if (!info) return;
Like the shipped ipu3 HAL, handle the frame delay with one extra input, needed for the first frame and StillCapture's non-sequential frame requests. Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 73 +++++++++++++++++++++++----- 1 file changed, 62 insertions(+), 11 deletions(-)