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