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

[Umang Jain]

Hi Laurent,

On 8/10/21 7:35 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> 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?

> 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();
>>   }
>>