[libcamera-devel,09/11] libcamera: ipu3: Add helper for parameter and statistic buffers

10346 diff mbox series

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@ragnatech.se>
---
 src/libcamera/pipeline/ipu3/frames.cpp  | 164 ++++++++++++++++++++++++
 src/libcamera/pipeline/ipu3/frames.h    |  68 ++++++++++
 src/libcamera/pipeline/ipu3/meson.build |   1 +
 3 files changed, 233 insertions(+)
 create mode 100644 src/libcamera/pipeline/ipu3/frames.cpp
 create mode 100644 src/libcamera/pipeline/ipu3/frames.h

Hi Niklas,

On Thu, Nov 05, 2020 at 01:15:44AM +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@ragnatech.se>
> ---
>  src/libcamera/pipeline/ipu3/frames.cpp  | 164 ++++++++++++++++++++++++
>  src/libcamera/pipeline/ipu3/frames.h    |  68 ++++++++++
>  src/libcamera/pipeline/ipu3/meson.build |   1 +
>  3 files changed, 233 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..824844e345e13679
> --- /dev/null
> +++ b/src/libcamera/pipeline/ipu3/frames.cpp
> @@ -0,0 +1,164 @@
> +/* 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/ipa/ipa_interface.h>
> +
> +#include "libcamera/internal/pipeline_handler.h"
> +#include "libcamera/internal/v4l2_videodevice.h"
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(IPU3)
> +
> +IPU3Frames::IPU3Frames(PipelineHandler *pipe, IPAProxy *ipa)
> +	: pipe_(pipe), ipa_(ipa), nextId_(0)
> +{
> +}
> +
> +void IPU3Frames::mapBuffers(const std::vector<std::unique_ptr<FrameBuffer>> &paramBuffers,
> +			    const std::vector<std::unique_ptr<FrameBuffer>> &statBuffers)
> +{
> +	unsigned int ipaBufferId = 1;
> +
> +	for (const std::unique_ptr<FrameBuffer> &buffer : paramBuffers) {
> +		buffer->setCookie(ipaBufferId++);
> +		ipaBuffers_.push_back({ .id = buffer->cookie(),
> +					.planes = buffer->planes() });
> +		availableParamBuffers_.push(buffer.get());
> +	}
> +
> +	for (const std::unique_ptr<FrameBuffer> &buffer : statBuffers) {
> +		buffer->setCookie(ipaBufferId++);
> +		ipaBuffers_.push_back({ .id = buffer->cookie(),
> +					.planes = buffer->planes() });
> +		availableStatBuffers_.push(buffer.get());
> +	}
> +
> +	ipa_->mapBuffers(ipaBuffers_);
> +
> +	nextId_ = 0;
> +	frameInfo_.clear();
> +}
> +
> +void IPU3Frames::unmapBuffers()
> +{
> +	availableParamBuffers_ = {};
> +	availableStatBuffers_ = {};
> +
> +	std::vector<unsigned int> ids;
> +	for (IPABuffer &ipabuf : ipaBuffers_)
> +		ids.push_back(ipabuf.id);
> +
> +	ipa_->unmapBuffers(ids);
> +	ipaBuffers_.clear();
> +}

I would do these two operations in the pipeline. Having the helper
calling the IPA is not nice imho and this really does too many
things, set cookies in buffers, map buffers in the IPA, fill the
available buffers in the helper, reset the id counter...

I would make this an:
        IPU3Frames::initialize() or reset()

that initializes the available buffers and reset counters and leave
the IPA buffer mapping and cookies initialization to the pipeline handler.

> +
> +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();
> +
> +	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->paramFilled = false;
> +	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;

So, paramDequeued is set when rawOnly, and paramFilled when the IPA is
done, but never really checked for here. Shouldn't this be a single
flag maybe 'paramDone' set in case the ipa is done or the request
contains a raw frame only ?

> +
> +	/* Return params and stat buffer for reuse. */
> +	availableParamBuffers_.push(info->paramBuffer);
> +	availableStatBuffers_.push(info->statBuffer);
> +
> +	/* Delete the extended frame information. */
> +	frameInfo_.erase(info->id);
> +
> +	pipe_->completeRequest(request);

pipe_ is only used here, and calling back into the pipeline handler is
an implicit behaviour I would avoid.

What about the pipeline handler doing:
        if (frameInfo_->tryComplete(id))
                completeRequest(frameInfo_->find(id)->request)
?

> +
> +	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;
> +}
> +
> +IPU3Frames::Info *IPU3Frames::find(Request *request)
> +{
> +	for (auto const &itInfo : frameInfo_) {
> +		Info *info = itInfo.second.get();
> +
> +		if (info->request == request)
> +			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..5c072f8ddbc9660f
> --- /dev/null
> +++ b/src/libcamera/pipeline/ipu3/frames.h
> @@ -0,0 +1,68 @@
> +/* 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 paramFilled;
> +		bool paramDequeued;
> +		bool metadataProcessed;
> +	};
> +
> +	IPU3Frames(PipelineHandler *pipe, IPAProxy *ipa);
> +
> +	void mapBuffers(const std::vector<std::unique_ptr<FrameBuffer>> &paramBuffers,
> +			const std::vector<std::unique_ptr<FrameBuffer>> &statBuffers);
> +	void unmapBuffers();
> +
> +	Info *create(Request *request);
> +	bool tryComplete(IPU3Frames::Info *info);
> +
> +	Info *find(unsigned int id);
> +	Info *find(FrameBuffer *buffer);
> +	Info *find(Request *request);
> +
> +private:
> +	PipelineHandler *pipe_;
> +	IPAProxy *ipa_;
> +
> +	std::queue<FrameBuffer *> availableParamBuffers_;
> +	std::queue<FrameBuffer *> availableStatBuffers_;
> +
> +	std::vector<IPABuffer> ipaBuffers_;
> +
> +	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',
>  ])
> --
> 2.29.2
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Niklas,

Thank you for the patch.

On Wed, Nov 18, 2020 at 05:52:24PM +0100, Jacopo Mondi wrote:
> On Thu, Nov 05, 2020 at 01:15:44AM +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@ragnatech.se>
> > ---
> >  src/libcamera/pipeline/ipu3/frames.cpp  | 164 ++++++++++++++++++++++++
> >  src/libcamera/pipeline/ipu3/frames.h    |  68 ++++++++++
> >  src/libcamera/pipeline/ipu3/meson.build |   1 +
> >  3 files changed, 233 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..824844e345e13679
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/ipu3/frames.cpp
> > @@ -0,0 +1,164 @@
> > +/* 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/ipa/ipa_interface.h>
> > +
> > +#include "libcamera/internal/pipeline_handler.h"
> > +#include "libcamera/internal/v4l2_videodevice.h"
> > +
> > +namespace libcamera {
> > +
> > +LOG_DECLARE_CATEGORY(IPU3)
> > +
> > +IPU3Frames::IPU3Frames(PipelineHandler *pipe, IPAProxy *ipa)
> > +	: pipe_(pipe), ipa_(ipa), nextId_(0)
> > +{
> > +}
> > +
> > +void IPU3Frames::mapBuffers(const std::vector<std::unique_ptr<FrameBuffer>> &paramBuffers,
> > +			    const std::vector<std::unique_ptr<FrameBuffer>> &statBuffers)
> > +{
> > +	unsigned int ipaBufferId = 1;
> > +
> > +	for (const std::unique_ptr<FrameBuffer> &buffer : paramBuffers) {
> > +		buffer->setCookie(ipaBufferId++);
> > +		ipaBuffers_.push_back({ .id = buffer->cookie(),
> > +					.planes = buffer->planes() });
> > +		availableParamBuffers_.push(buffer.get());
> > +	}
> > +
> > +	for (const std::unique_ptr<FrameBuffer> &buffer : statBuffers) {
> > +		buffer->setCookie(ipaBufferId++);
> > +		ipaBuffers_.push_back({ .id = buffer->cookie(),
> > +					.planes = buffer->planes() });
> > +		availableStatBuffers_.push(buffer.get());
> > +	}
> > +
> > +	ipa_->mapBuffers(ipaBuffers_);
> > +
> > +	nextId_ = 0;
> > +	frameInfo_.clear();
> > +}
> > +
> > +void IPU3Frames::unmapBuffers()
> > +{
> > +	availableParamBuffers_ = {};
> > +	availableStatBuffers_ = {};
> > +
> > +	std::vector<unsigned int> ids;
> > +	for (IPABuffer &ipabuf : ipaBuffers_)
> > +		ids.push_back(ipabuf.id);
> > +
> > +	ipa_->unmapBuffers(ids);
> > +	ipaBuffers_.clear();
> > +}
> 
> I would do these two operations in the pipeline. Having the helper
> calling the IPA is not nice imho and this really does too many
> things, set cookies in buffers, map buffers in the IPA, fill the
> available buffers in the helper, reset the id counter...
> 
> I would make this an:
>         IPU3Frames::initialize() or reset()
> 
> that initializes the available buffers and reset counters and leave
> the IPA buffer mapping and cookies initialization to the pipeline handler.

I like moving the IPA handling to the pipeline handler. I think helpers
to make buffer mapping more transparent would be nice though, but I
wouldn't handle it in this class.

> > +
> > +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();
> > +
> > +	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->paramFilled = false;
> > +	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;
> 
> So, paramDequeued is set when rawOnly, and paramFilled when the IPA is
> done, but never really checked for here. Shouldn't this be a single
> flag maybe 'paramDone' set in case the ipa is done or the request
> contains a raw frame only ?
> 
> > +
> > +	/* Return params and stat buffer for reuse. */
> > +	availableParamBuffers_.push(info->paramBuffer);
> > +	availableStatBuffers_.push(info->statBuffer);
> > +
> > +	/* Delete the extended frame information. */
> > +	frameInfo_.erase(info->id);
> > +
> > +	pipe_->completeRequest(request);
> 
> pipe_ is only used here, and calling back into the pipeline handler is
> an implicit behaviour I would avoid.
> 
> What about the pipeline handler doing:
>         if (frameInfo_->tryComplete(id))
>                 completeRequest(frameInfo_->find(id)->request)
> ?

That's a nice suggestion :-)

> > +
> > +	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;
> > +}
> > +
> > +IPU3Frames::Info *IPU3Frames::find(Request *request)
> > +{
> > +	for (auto const &itInfo : frameInfo_) {
> > +		Info *info = itInfo.second.get();
> > +
> > +		if (info->request == request)
> > +			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..5c072f8ddbc9660f
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/ipu3/frames.h
> > @@ -0,0 +1,68 @@
> > +/* 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 paramFilled;
> > +		bool paramDequeued;
> > +		bool metadataProcessed;
> > +	};
> > +
> > +	IPU3Frames(PipelineHandler *pipe, IPAProxy *ipa);
> > +
> > +	void mapBuffers(const std::vector<std::unique_ptr<FrameBuffer>> &paramBuffers,
> > +			const std::vector<std::unique_ptr<FrameBuffer>> &statBuffers);
> > +	void unmapBuffers();
> > +
> > +	Info *create(Request *request);
> > +	bool tryComplete(IPU3Frames::Info *info);
> > +
> > +	Info *find(unsigned int id);
> > +	Info *find(FrameBuffer *buffer);
> > +	Info *find(Request *request);

This looks to me like a workaround for the lack of pipeline handler
private data associated with requests, am I wrong ? We don't need to
solve the problem in the Request class as a dependency for this series,
but should it be eventually handled there ?

I share Jacopo's feeling that this class looks a bit like a big carpet
under which all the dust is swept. There's nothing wrong with that as
such, as developing helpers usually takes the form of trying different
implementations and seeing what makes sense. Already addressing the
issues mentioned above would however be nice.

> > +
> > +private:
> > +	PipelineHandler *pipe_;
> > +	IPAProxy *ipa_;
> > +
> > +	std::queue<FrameBuffer *> availableParamBuffers_;
> > +	std::queue<FrameBuffer *> availableStatBuffers_;
> > +
> > +	std::vector<IPABuffer> ipaBuffers_;
> > +
> > +	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',
> >  ])