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

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2021-02-04 19:22:43 +0200, Laurent Pinchart wrote:
> 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>

Thanks.

> 
> 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.

Yes, that is a good idea.

> 
> > +
> > +	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

-- 
Regards,
Niklas Söderlund