[libcamera-devel] [PATCH v3 11/11] libcamera: ipu3: Share parameter and statistic buffers with IPA
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Feb 5 11:34:17 CET 2021
Hello Kieran
On 05/02/2021 10:31, Kieran Bingham wrote:
> Hi Laurent/Niklas,
>
> On 04/02/2021 22:13, Niklas Söderlund wrote:
>> Hi Laurent,
>>
>> Thanks for your feedback.
>>
>> On 2021-02-04 20:54:41 +0200, Laurent Pinchart wrote:
>>> Hi Niklas,
>>>
>>> Thank you for the patch.
>>>
>>> On Thu, Feb 04, 2021 at 05:29:43PM +0100, Niklas Söderlund wrote:
>>>> Use the IPU3Frames helper to share parameters and statistics buffers
>>>> with the IPA. Track which parameter and statistic buffer is used for
>>>> which request and make sure the parameter buffers is filled in by the
>>>> IPA before it's needed and that the statistic buffer is consumed and
>>>> meta data generated before completing the request.
>>>>
>>>> With this change the IPU3 pipeline is prepared to fully operate with an
>>>> IPA component.
>>>>
>>>> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>>>> ---
>>>> * Changes since v1
>>>> - Update commit message.
>>>> - s/frameInfo_/frameInfos_/
>>>> - Refactor away isComplete variable.
>>>>
>>>> * Changes since v2
>>>> - Emedd the IPU3Frames instance instead of allocating it.
>>>> - Use -ENOBUFS instead of _ENOENT in queueRequestDevice().
>>>> - Keep isComplete in cio2BufferReady()
>>>> - Rework the queue logic.
>>>> ---
>>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 149 ++++++++++++++++++++-------
>>>> 1 file changed, 112 insertions(+), 37 deletions(-)
>>>>
>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>> index 092db6389833a481..b79db25050d2c1d6 100644
>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>> @@ -29,6 +29,7 @@
>>>> #include "libcamera/internal/v4l2_controls.h"
>>>>
>>>> #include "cio2.h"
>>>> +#include "frames.h"
>>>> #include "imgu.h"
>>>>
>>>> namespace libcamera {
>>>> @@ -61,6 +62,8 @@ public:
>>>>
>>>> void imguOutputBufferReady(FrameBuffer *buffer);
>>>> void cio2BufferReady(FrameBuffer *buffer);
>>>> + void paramBufferReady(FrameBuffer *buffer);
>>>> + void statBufferReady(FrameBuffer *buffer);
>>>>
>>>> CIO2Device cio2_;
>>>> ImgUDevice *imgu_;
>>>> @@ -71,6 +74,7 @@ public:
>>>>
>>>> uint32_t exposureTime_;
>>>> std::unique_ptr<DelayedControls> delayedCtrls_;
>>>> + IPU3Frames frameInfos_;
>>>>
>>>> private:
>>>> void queueFrameAction(unsigned int id, const IPAOperationData &op);
>>>> @@ -609,6 +613,8 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
>>>>
>>>> data->ipa_->mapBuffers(ipaBuffers_);
>>>>
>>>> + data->frameInfos_.init(imgu->paramBuffers_, imgu->statBuffers_);
>>>> +
>>>> return 0;
>>>> }
>>>>
>>>> @@ -616,6 +622,8 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera)
>>>> {
>>>> IPU3CameraData *data = cameraData(camera);
>>>>
>>>> + data->frameInfos_.clear();
>>>> +
>>>> std::vector<unsigned int> ids;
>>>> for (IPABuffer &ipabuf : ipaBuffers_)
>>>> ids.push_back(ipabuf.id);
>>>> @@ -713,7 +721,10 @@ void PipelineHandlerIPU3::stop(Camera *camera)
>>>> int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
>>>> {
>>>> IPU3CameraData *data = cameraData(camera);
>>>> - int error = 0;
>>>> +
>>>> + IPU3Frames::Info *info = data->frameInfos_.create(request);
>>>> + if (!info)
>>>> + return -ENOBUFS;
>>>>
>>>> /*
>>>> * Queue a buffer on the CIO2, using the raw stream buffer provided in
>>>> @@ -724,24 +735,9 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
>>>> if (!rawBuffer)
>>>> return -ENOMEM;
>>>
>>> What will happen to info in this case ? As there will be no raw buffer
>>> associated with it, won't it linger forever in a list ?
>>
>> Good point. I will add a IPU3Frames::remove(Info *info); interface which
>> may be used to remove tracking of a request prematurely. It also makes
>> the case where the CIO2 buffer is canceled cleaner.
>>
>>>
>>>>
>>>> - /* Queue all buffers from the request aimed for the ImgU. */
>>>> - for (auto it : request->buffers()) {
>>>> - const Stream *stream = it.first;
>>>> - FrameBuffer *buffer = it.second;
>>>> - int ret;
>>>> + info->rawBuffer = rawBuffer;
>>>>
>>>> - if (stream == &data->outStream_)
>>>> - ret = data->imgu_->output_->queueBuffer(buffer);
>>>> - else if (stream == &data->vfStream_)
>>>> - ret = data->imgu_->viewfinder_->queueBuffer(buffer);
>>>> - else
>>>> - continue;
>>>> -
>>>> - if (ret < 0)
>>>> - error = ret;
>>>> - }
>>>> -
>>>> - return error;
>>>> + return 0;
>>>> }
>>>>
>>>> bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
>>>> @@ -1007,6 +1003,10 @@ int PipelineHandlerIPU3::registerCameras()
>>>> &IPU3CameraData::imguOutputBufferReady);
>>>> data->imgu_->viewfinder_->bufferReady.connect(data.get(),
>>>> &IPU3CameraData::imguOutputBufferReady);
>>>> + data->imgu_->param_->bufferReady.connect(data.get(),
>>>> + &IPU3CameraData::paramBufferReady);
>>>> + data->imgu_->stat_->bufferReady.connect(data.get(),
>>>> + &IPU3CameraData::statBufferReady);
>>>>
>>>> /* Create and register the Camera instance. */
>>>> std::string cameraId = cio2->sensor()->id();
>>>> @@ -1039,7 +1039,7 @@ int IPU3CameraData::loadIPA()
>>>> return 0;
>>>> }
>>>>
>>>> -void IPU3CameraData::queueFrameAction([[maybe_unused]] unsigned int id,
>>>> +void IPU3CameraData::queueFrameAction(unsigned int id,
>>>> const IPAOperationData &action)
>>>> {
>>>> switch (action.operation) {
>>>> @@ -1048,6 +1048,41 @@ void IPU3CameraData::queueFrameAction([[maybe_unused]] unsigned int id,
>>>> delayedCtrls_->push(controls);
>>>> break;
>>>> }
>>>> + case IPU3_IPA_ACTION_PARAM_FILLED: {
>>>> + IPU3Frames::Info *info = frameInfos_.find(id);
>>>> + if (!info)
>>>> + break;
>>>
>>> Should we warn in this case ? Maybe in IPU3Frames::info() itself to
>>> avoid duplicating log calls ? I'm tempted to even assert.
>>
>> I will add LOG() to IPU3Frames::find() at the Error level. We can then
>> easily turn them into Fatal if we think it better in the future.
>>
>>>
>>>> +
>>>> + /* Queue all buffers from the request aimed for the ImgU. */
>>>> + for (auto it : info->request->buffers()) {
>>>> + const Stream *stream = it.first;
>>>> + FrameBuffer *outbuffer = it.second;
>>>> +
>>>> + if (stream == &outStream_)
>>>> + imgu_->output_->queueBuffer(outbuffer);
>>>> + else if (stream == &vfStream_)
>>>> + imgu_->viewfinder_->queueBuffer(outbuffer);
>>>> + }
>>>> +
>>>> + imgu_->param_->queueBuffer(info->paramBuffer);
>>>> + imgu_->stat_->queueBuffer(info->statBuffer);
>>>> + imgu_->input_->queueBuffer(info->rawBuffer);
>>>> +
>>>> + break;
>>>> + }
>>>> + case IPU3_IPA_ACTION_METADATA_READY: {
>>>> + IPU3Frames::Info *info = frameInfos_.find(id);
>>>> + if (!info)
>>>> + break;
>>>> +
>>>> + Request *request = info->request;
>>>> + request->metadata() = action.controls[0];
>>>> + info->metadataProcessed = true;
>>>> + if (frameInfos_.tryComplete(info))
>>>> + pipe_->completeRequest(request);
>>>
>>> Maybe we could pass the request to IPU3Frames, and handle completion
>>> automatically internally, but that's for later.
>>
>> We did that in earlier versions but it was disliked as a reference to
>> the pipeline handler would then need to be stored inside the IPU3Frames.
>> I still think it's nicer then the code duplication you point out here.
>> But will keep it as is for now.
>
>
> Hrm ... I think I've just realised the implications of this here.
>
> Do IPA's really have the power to 'complete requests' ? Shouldn't that
> be a capability/responsibility restricted to the pipeline handlers?
>
> I can see that restricting would mean there would be some notification
> required back to the pipeline handler so that the IPA would be able to
> say "I'm finished with this object", but I sort of expected that to be
> required anyway. Perhaps directly trying to complete the request is just
> a shortcut on that then.
Please consider the function that this code is in, and try to determine
if this is in the PipelineHandler or the IPA - and you might find that
your entire concern is completely invalid ;-)
<Sorry, I thought the IPU3Frames was an IPA concept at first>
--
Kieran
>
>
> --
> Kieran
>
>>>
>>>> +
>>>> + break;
>>>> + }
>>>> default:
>>>> LOG(IPU3, Error) << "Unknown action " << action.operation;
>>>> break;
>>>> @@ -1068,11 +1103,12 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
>>>> {
>>>> Request *request = buffer->request();
>>>>
>>>> - if (!pipe_->completeBuffer(request, buffer))
>>>> - /* Request not completed yet, return here. */
>>>> + pipe_->completeBuffer(request, buffer);
>>>> +
>>>> + IPU3Frames::Info *info = frameInfos_.find(buffer);
>>>> + if (!info)
>>>> return;
>>>>
>>>> - /* Mark the request as complete. */
>>>> request->metadata().set(controls::draft::PipelineDepth, 3);
>>>> /* \todo Move the ExposureTime control to the IPA. */
>>>> request->metadata().set(controls::ExposureTime, exposureTime_);
>>>> @@ -1081,7 +1117,12 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
>>>> Rectangle cropRegion = request->controls().get(controls::ScalerCrop);
>>>> request->metadata().set(controls::ScalerCrop, cropRegion);
>>>> }
>>>> - pipe_->completeRequest(request);
>>>> +
>>>> + if (buffer->metadata().status == FrameMetadata::FrameCancelled)
>>>> + info->metadataProcessed = true;
>>>> +
>>>> + if (frameInfos_.tryComplete(info))
>>>> + pipe_->completeRequest(request);
>>>> }
>>>>
>>>> /**
>>>> @@ -1093,26 +1134,60 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer)
>>>> */
>>>> void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)
>>>> {
>>>> - /* \todo Handle buffer failures when state is set to BufferError. */
>>>> - if (buffer->metadata().status == FrameMetadata::FrameCancelled)
>>>> + IPU3Frames::Info *info = frameInfos_.find(buffer);
>>>> + if (!info)
>>>> return;
>>>>
>>>> Request *request = buffer->request();
>>>>
>>>> - /*
>>>> - * If the request contains a buffer for the RAW stream only, complete it
>>>> - * now as there's no need for ImgU processing.
>>>> - */
>>>> - if (request->findBuffer(&rawStream_)) {
>>>> - bool isComplete = pipe_->completeBuffer(request, buffer);
>>>> - if (isComplete) {
>>>> - request->metadata().set(controls::draft::PipelineDepth, 2);
>>>> - pipe_->completeRequest(request);
>>>> - return;
>>>> - }
>>>> + if (request->findBuffer(&rawStream_))
>>>> + pipe_->completeBuffer(request, buffer);
>>>
>>> If you moved this after the next if (...) { } block, you could...
>>>
>>>> +
>>>> + /* If the buffer is cancelled force a complete of the whole request. */
>>>> + if (buffer->metadata().status == FrameMetadata::FrameCancelled) {
>>>> + for (auto it : request->buffers())
>>>> + if (it.second != buffer)
>>>
>>> ... drop this check.
>>
>> Good point.
>>
>>>
>>>> + pipe_->completeBuffer(request, it.second);
>>>> +
>>>> + info->paramDequeued = true;
>>>> + info->metadataProcessed = true;
>>>> + ASSERT(frameInfos_.tryComplete(info));
>>>> + pipe_->completeRequest(request);
>>>> +
>>>> + return;
>>>> }
>>>>
>>>> - imgu_->input_->queueBuffer(buffer);
>>>> + IPAOperationData op;
>>>> + op.operation = IPU3_IPA_EVENT_FILL_PARAMS;
>>>> + op.data = { info->id, info->paramBuffer->cookie() };
>>>> + op.controls = { request->controls() };
>>>
>>> This is the right time to call IPU3_IPA_EVENT_FILL_PARAMS, but the
>>> request controls need to be passed to the IPA earlier, right when the
>>> request is queued, as the IPA needs to look ahead in the request queue
>>> to compute parameters.
>>>
>>> This is the main issue in this patch, the rest is fairly minor.
>>
>> I see your point I will fix this for v4.
>>
>>>
>>>> + ipa_->processEvent(op);
>>>> +}
>>>> +
>>>> +void IPU3CameraData::paramBufferReady(FrameBuffer *buffer)
>>>> +{
>>>> + IPU3Frames::Info *info = frameInfos_.find(buffer);
>>>> + if (!info)
>>>> + return;
>>>> +
>>>> + info->paramDequeued = true;
>>>> + if (frameInfos_.tryComplete(info))
>>>> + pipe_->completeRequest(buffer->request());
>>>> +}
>>>> +
>>>> +void IPU3CameraData::statBufferReady(FrameBuffer *buffer)
>>>> +{
>>>> + if (buffer->metadata().status == FrameMetadata::FrameCancelled)
>>>> + return;
>>>
>>> Why do we need to check for the status for stats buffers but not for
>>> params buffers ?
>>
>> Because there is no point in giving a cancelled statistics buffer to the
>> IPA. While for the param all we really care about is marking the buffer
>> free and moving on. But this error path can't have been tested it should
>> set info->metadataProcessed = true and try to complete the request. Will
>> fix for v4.
>>
>>>
>>>> +
>>>> + IPU3Frames::Info *info = frameInfos_.find(buffer);
>>>> + if (!info)
>>>> + return;
>>>> +
>>>> + IPAOperationData op;
>>>> + op.operation = IPU3_IPA_EVENT_STAT_READY;
>>>> + op.data = { info->id, info->statBuffer->cookie() };
>>>> + ipa_->processEvent(op);
>>>> }
>>>>
>>>> REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3)
>>>
>>> --
>>> Regards,
>>>
>>> Laurent Pinchart
>>
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list