[libcamera-devel] [PATCH v3 10/11] libcamera: ipu3: Add helper for parameter and statistic buffers

[Laurent Pinchart]

On Thu, Feb 04, 2021 at 05:29:42PM +0100, Niklas Söderlund wrote:
> Add a helper class to aid in associating a parameter and statistic
> buffer with each request queued to the pipeline. The helper helps with
> tracking the state of the extra buffers and in completing the request
> once all extra processing is done.
> 
> This change only adds the helper more work is needed to integrate it
> with the pipeline and an IPA.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> * Changes since v1
> - Move mapping of buffers to IPA to the pipeline handler and a separate
>   patch.
> - Move all pipeline interactions out of helper.
> 
> * Changes since v2
> - Add todo to remove dynamic memory allocation.
> - Remove unused find(Request *request).
> ---
>  src/libcamera/pipeline/ipu3/frames.cpp  | 129 ++++++++++++++++++++++++
>  src/libcamera/pipeline/ipu3/frames.h    |  61 +++++++++++
>  src/libcamera/pipeline/ipu3/meson.build |   1 +
>  3 files changed, 191 insertions(+)
>  create mode 100644 src/libcamera/pipeline/ipu3/frames.cpp
>  create mode 100644 src/libcamera/pipeline/ipu3/frames.h
> 
> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp
> new file mode 100644
> index 0000000000000000..b7f7f0a6575714d9
> --- /dev/null
> +++ b/src/libcamera/pipeline/ipu3/frames.cpp
> @@ -0,0 +1,129 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * frames.cpp - Intel IPU3 Frames helper
> + */
> +
> +#include "frames.h"
> +
> +#include <libcamera/buffer.h>
> +#include <libcamera/request.h>
> +
> +#include "libcamera/internal/pipeline_handler.h"
> +#include "libcamera/internal/v4l2_videodevice.h"
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(IPU3)
> +
> +IPU3Frames::IPU3Frames()
> +	: nextId_(0)
> +{
> +}
> +
> +void IPU3Frames::init(const std::vector<std::unique_ptr<FrameBuffer>> &paramBuffers,
> +		      const std::vector<std::unique_ptr<FrameBuffer>> &statBuffers)
> +{
> +	for (const std::unique_ptr<FrameBuffer> &buffer : paramBuffers)
> +		availableParamBuffers_.push(buffer.get());
> +
> +	for (const std::unique_ptr<FrameBuffer> &buffer : statBuffers)
> +		availableStatBuffers_.push(buffer.get());
> +
> +	nextId_ = 0;
> +	frameInfo_.clear();
> +}
> +
> +void IPU3Frames::clear()
> +{
> +	availableParamBuffers_ = {};
> +	availableStatBuffers_ = {};
> +}
> +
> +IPU3Frames::Info *IPU3Frames::create(Request *request)
> +{
> +	unsigned int id = nextId_++;
> +
> +	if (availableParamBuffers_.empty()) {
> +		LOG(IPU3, Error) << "Parameters buffer underrun";
> +		return nullptr;
> +	}
> +	FrameBuffer *paramBuffer = availableParamBuffers_.front();
> +
> +	if (availableStatBuffers_.empty()) {
> +		LOG(IPU3, Error) << "Statisitc buffer underrun";
> +		return nullptr;
> +	}
> +	FrameBuffer *statBuffer = availableStatBuffers_.front();
> +
> +	availableParamBuffers_.pop();
> +	availableStatBuffers_.pop();
> +
> +	/* \todo Remove the dynamic allocation of Info */
> +	std::unique_ptr<Info> info = std::make_unique<Info>();
> +
> +	info->id = id;
> +	info->request = request;
> +	info->rawBuffer = nullptr;
> +	info->paramBuffer = paramBuffer;
> +	info->statBuffer = statBuffer;
> +	info->paramDequeued = false;
> +	info->metadataProcessed = false;
> +
> +	frameInfo_[id] = std::move(info);
> +
> +	return frameInfo_[id].get();
> +}
> +
> +bool IPU3Frames::tryComplete(IPU3Frames::Info *info)
> +{
> +	Request *request = info->request;
> +
> +	if (request->hasPendingBuffers())
> +		return false;
> +
> +	if (!info->metadataProcessed)
> +		return false;
> +
> +	if (!info->paramDequeued)
> +		return false;
> +
> +	/* Return params and stat buffer for reuse. */
> +	availableParamBuffers_.push(info->paramBuffer);
> +	availableStatBuffers_.push(info->statBuffer);
> +
> +	/* Delete the extended frame information. */
> +	frameInfo_.erase(info->id);
> +
> +	return true;
> +}
> +
> +IPU3Frames::Info *IPU3Frames::find(unsigned int id)
> +{
> +	const auto &itInfo = frameInfo_.find(id);
> +
> +	if (itInfo != frameInfo_.end())
> +		return itInfo->second.get();
> +
> +	return nullptr;
> +}
> +
> +IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer)
> +{
> +	for (auto const &itInfo : frameInfo_) {
> +		Info *info = itInfo.second.get();
> +
> +		for (auto const itBuffers : info->request->buffers())
> +			if (itBuffers.second == buffer)
> +				return info;
> +
> +		if (info->rawBuffer == buffer || info->paramBuffer == buffer ||
> +		    info->statBuffer == buffer)
> +			return info;
> +	}
> +
> +	return nullptr;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h
> new file mode 100644
> index 0000000000000000..93379d20722d65fb
> --- /dev/null
> +++ b/src/libcamera/pipeline/ipu3/frames.h
> @@ -0,0 +1,61 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * frames.h - Intel IPU3 Frames helper
> + */
> +#ifndef __LIBCAMERA_PIPELINE_IPU3_FRAMES_H__
> +#define __LIBCAMERA_PIPELINE_IPU3_FRAMES_H__
> +
> +#include <map>
> +#include <memory>
> +#include <queue>
> +#include <vector>
> +
> +namespace libcamera {
> +
> +class FrameBuffer;
> +class IPAProxy;
> +class PipelineHandler;
> +class Request;
> +class V4L2VideoDevice;
> +struct IPABuffer;
> +
> +class IPU3Frames
> +{
> +public:
> +	struct Info {
> +		unsigned int id;
> +		Request *request;
> +
> +		FrameBuffer *rawBuffer;
> +		FrameBuffer *paramBuffer;
> +		FrameBuffer *statBuffer;
> +
> +		bool paramDequeued;
> +		bool metadataProcessed;
> +	};
> +
> +	IPU3Frames();
> +
> +	void init(const std::vector<std::unique_ptr<FrameBuffer>> &paramBuffers,
> +		  const std::vector<std::unique_ptr<FrameBuffer>> &statBuffers);
> +	void clear();
> +
> +	Info *create(Request *request);
> +	bool tryComplete(IPU3Frames::Info *info);

You can write this

	bool tryComplete(Info *info);

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

One idea for future improvements, especially if we want to share this
helper between pipeline handlers, would be to rework the API in a way
that would avoid the possibility of use-after-free if the pipeline
handler tries to access the info pointer after a call to tryComplete()
that returns true.

> +
> +	Info *find(unsigned int id);
> +	Info *find(FrameBuffer *buffer);
> +
> +private:
> +	std::queue<FrameBuffer *> availableParamBuffers_;
> +	std::queue<FrameBuffer *> availableStatBuffers_;
> +
> +	unsigned int nextId_;
> +	std::map<unsigned int, std::unique_ptr<Info>> frameInfo_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_PIPELINE_IPU3_FRAMES_H__ */
> diff --git a/src/libcamera/pipeline/ipu3/meson.build b/src/libcamera/pipeline/ipu3/meson.build
> index d60e07ae6ccac2bc..a1b0b31ac5bcf864 100644
> --- a/src/libcamera/pipeline/ipu3/meson.build
> +++ b/src/libcamera/pipeline/ipu3/meson.build
> @@ -2,6 +2,7 @@
>  
>  libcamera_sources += files([
>      'cio2.cpp',
> +    'frames.cpp',
>      'imgu.cpp',
>      'ipu3.cpp',
>  ])

-- 
Regards,

Laurent Pinchart