[libcamera-devel] [PATCH 3/4] ipa: vimc: Map and unmap buffers

[Laurent Pinchart]

Hi Umang,

On Tue, Aug 10, 2021 at 08:52:59AM +0530, Umang Jain wrote:
> On 8/10/21 7:35 AM, Laurent Pinchart wrote:
> > On Fri, Aug 06, 2021 at 03:44:08PM +0530, Umang Jain wrote:
> >> Since VIMC is a virtual test driver, the support for IPA
> >> buffers(parameter and statistics) is missing. We could create
> > s/buffers/buffers /
> >
> >> our own and pass it around, but created buffers needs to be
> >> backed by dmabuf file-descriptors ideally.
> >>
> >> This might prove cubersome given that we use vimc for test purposes.
> >> Hence, pass actual frame data buffers into the IPA instead, which
> >> would result in exercising these code paths for now. Introduce plumbing
> >> support for mapping and un-mapping of these buffers into the IPA.
> >
> > I know what this is about, but if I didn't I'd have trouble following
> > the rationale. Here's a possible improvement.
> >
> > VIMC is a virtual test driver that doesn't generate statistics or
> > consume parameters buffers. Its pipeline handler thus doesn't map any
> > buffer to the IPA. This limits the ability to test corresponding code
> > paths in unit tests, as they use VIMC extensively.
> >
> > Creating fake statistics and parameters buffers would be cumbersome. Map
> > the frame buffers to the IPA instead, to extend test coverage.
> >
> >> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> >> ---
> >>   include/libcamera/ipa/vimc.mojom     |  3 +++
> >>   src/ipa/vimc/vimc.cpp                | 26 ++++++++++++++++++++++++++
> >>   src/libcamera/pipeline/vimc/vimc.cpp | 25 ++++++++++++++++++++++++-
> >>   3 files changed, 53 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom
> >> index 99b6412b..8452461d 100644
> >> --- a/include/libcamera/ipa/vimc.mojom
> >> +++ b/include/libcamera/ipa/vimc.mojom
> >> @@ -26,6 +26,9 @@ interface IPAVimcInterface {
> >>   
> >>   	start() => (int32 ret);
> >>   	stop();
> >> +
> >> +	mapBuffers(array<libcamera.IPABuffer> buffers);
> >> +	unmapBuffers(array<uint32> ids);
> >>   };
> >>   
> >>   interface IPAVimcEventInterface {
> >> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> >> index 54d9086a..2b00687f 100644
> >> --- a/src/ipa/vimc/vimc.cpp
> >> +++ b/src/ipa/vimc/vimc.cpp
> >> @@ -19,6 +19,8 @@
> >>   #include <libcamera/ipa/ipa_interface.h>
> >>   #include <libcamera/ipa/ipa_module_info.h>
> >>   
> >> +#include "libcamera/internal/framebuffer.h"
> >> +
> >>   namespace libcamera {
> >>   
> >>   LOG_DEFINE_CATEGORY(IPAVimc)
> >> @@ -38,11 +40,15 @@ public:
> >>   		      const std::map<unsigned int, IPAStream> &streamConfig,
> >>   		      const std::map<unsigned int, ControlInfoMap> &entityControls) override;
> >>   
> >> +	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> >> +	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> >> +
> >>   private:
> >>   	void initTrace();
> >>   	void trace(enum ipa::vimc::IPATraceCode operation);
> >>   
> >>   	int fd_;
> >> +	std::map<unsigned int, MappedFrameBuffer> buffers_;
> >>   };
> >>   
> >>   IPAVimc::IPAVimc()
> >> @@ -99,6 +105,26 @@ int IPAVimc::configure([[maybe_unused]] const IPACameraSensorInfo &sensorInfo,
> >>   	return 0;
> >>   }
> >>   
> >> +void IPAVimc::mapBuffers(const std::vector<IPABuffer> &buffers)
> >> +{
> >> +	for (const IPABuffer &buffer : buffers) {
> >> +		const FrameBuffer fb(buffer.planes);
> >> +		buffers_.emplace(buffer.id,
> >> +				 MappedFrameBuffer(&fb, PROT_READ));
> >> +	}
> >> +}
> >> +
> >> +void IPAVimc::unmapBuffers(const std::vector<unsigned int> &ids)
> >> +{
> >> +	for (unsigned int id : ids) {
> >> +		auto it = buffers_.find(id);
> >> +		if (it == buffers_.end())
> >> +			continue;
> >> +
> >> +		buffers_.erase(it);
> >> +	}
> >> +}
> >> +
> >>   void IPAVimc::initTrace()
> >>   {
> >>   	struct stat fifoStat;
> >> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> >> index b7dd6595..fa21128d 100644
> >> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> >> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> >> @@ -45,7 +45,7 @@ class VimcCameraData : public CameraData
> >>   {
> >>   public:
> >>   	VimcCameraData(PipelineHandler *pipe, MediaDevice *media)
> >> -		: CameraData(pipe), media_(media)
> >> +		: CameraData(pipe), media_(media), buffers_(nullptr)
> >>   	{
> >>   	}
> >>   
> >> @@ -61,6 +61,9 @@ public:
> >>   	Stream stream_;
> >>   
> >>   	std::unique_ptr<ipa::vimc::IPAProxyVimc> ipa_;
> >> +
> >> +	std::vector<std::unique_ptr<FrameBuffer>> *buffers_;
> >> +	std::vector<IPABuffer> ipaBuffers_;
> >>   };
> >>   
> >>   class VimcCameraConfiguration : public CameraConfiguration
> >> @@ -319,6 +322,7 @@ int PipelineHandlerVimc::exportFrameBuffers(Camera *camera, Stream *stream,
> >>   {
> >>   	VimcCameraData *data = cameraData(camera);
> >>   	unsigned int count = stream->configuration().bufferCount;
> >> +	data->buffers_ = buffers;
> >>   
> >>   	return data->video_->exportBuffers(count, buffers);
> >>   }
> >> @@ -332,6 +336,19 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlLis
> >>   	if (ret < 0)
> >>   		return ret;
> >>   
> >> +	/*
> >> +	 * We don't have actual parameters/statistics buffers in VIMC.
> >> +	 * Hence, map frame buffers to exercise IPA IPC code paths for now.
> >> +	 */
> >> +	ASSERT(data->buffers_ != nullptr);
> >
> > We have a problem here. There's nothing that guarantees that the frame
> > buffers that have been queued have been exported by the pipeline
> > handler. I'm afraid we won't be able to use frame buffers for this :-(
> 
> I don't understand. We are using buffers which are exported (see the 
> above hunk of exportFormatBuffers())
> 
> The guarantees you have mentioned, Is it about checking
> data->video_->exportBuffers(count, buffers)
> 
> has been successful, before using the buffers for IPA paths?

All our unit tests and test applications use the FrameBufferAllocator,
but that's not a requirement. An application may construct FrameBuffer
instances itself, using dmabufs obtained elsewhere (exported by a
display driver for instance).

> > It looks like we'll have to instead allocate fake stats buffers, even if
> > it's cumbersome. There are at least two options to create dmabufs from
> > userspace, dmabuf heaps or udmabuf. I'm not sure which one is best, but
> > let's take into account the default permissions that distributions set
> > on the related devices, to minimize issues.
> >
> >> +	unsigned int ipaBufferId = 1;
> >> +	for (unsigned int i = 0; i< count; i++) {
> >> +		std::unique_ptr<FrameBuffer> &buffer = data->buffers_->at(i);
> >> +		buffer->setCookie(ipaBufferId++);
> >> +                data->ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());
> >> +	}
> >> +	data->ipa_->mapBuffers(data->ipaBuffers_);
> >> +
> >>   	ret = data->ipa_->start();
> >>   	if (ret) {
> >>   		data->video_->releaseBuffers();
> >> @@ -352,7 +369,13 @@ void PipelineHandlerVimc::stop(Camera *camera)
> >>   {
> >>   	VimcCameraData *data = cameraData(camera);
> >>   	data->video_->streamOff();
> >> +
> >> +	std::vector<unsigned int> ids;
> >> +	for (IPABuffer &ipabuf : data->ipaBuffers_)
> >> +		ids.push_back(ipabuf.id);
> >> +	data->ipa_->unmapBuffers(ids);
> >>   	data->ipa_->stop();
> >> +
> >>   	data->video_->releaseBuffers();
> >>   }
> >>   

-- 
Regards,

Laurent Pinchart