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

[Umang Jain]

Hi Laurent,

Thanks for review,

one comment below

On 8/14/21 6:05 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Fri, Aug 13, 2021 at 08:14:36PM +0530, Umang Jain wrote:
>> VIMC pipeline-handler have dmabuf-backed mock FrameBuffers which are
> s/pipeline-handler/pipeline handler/
>
>> specifically targetted mimicking IPA buffers(parameter and statistics).
> s/buffers/buffers /
>
>> Map these mock buffers to the VIMC IPA that would enable exercising IPA
>> IPC code paths. This will provide leverage to our test suite to test
>> IPA IPC code paths, which are common to various platforms.
>>
>> 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 | 16 ++++++++++++++++
>>   3 files changed, 45 insertions(+)
>>
>> diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom
>> index ee66353d..8cb240d3 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 0535c740..082b2db7 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/mapped_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::IPAOperationCode 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, MappedFrameBuffer::MapFlag::Read));
> The point of emplace compared to insert is to avoid a copy construction.
> This should be written as
>
> 		buffers_.emplace(std::piecewise_construct,
> 				 std::forward_as_tuple(buffer.id),
> 				 std::forward_as_tuple(&fb, MappedFrameBuffer::MapFlag::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 edf8c58e..c44a6242 100644
>> --- a/src/libcamera/pipeline/vimc/vimc.cpp
>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
>> @@ -63,6 +63,7 @@ public:
>>   
>>   	std::unique_ptr<ipa::vimc::IPAProxyVimc> ipa_;
>>   	std::vector<std::unique_ptr<FrameBuffer>> mockIPABufs_;
>> +	std::vector<IPABuffer> ipaBuffers_;
>>   };
>>   
>>   class VimcCameraConfiguration : public CameraConfiguration
>> @@ -334,6 +335,15 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlLis
>>   	if (ret < 0)
>>   		return ret;
>>   
>> +	/* Map the mock IPA buffers to VIMC IPA to exercise IPC code paths */
>> +	unsigned int ipaBufferId = 1;
>> +	for (unsigned int i = 0; i < data->mockIPABufs_.size(); i++) {
>> +		std::unique_ptr<FrameBuffer> &buffer = data->mockIPABufs_[i];
>> +		buffer->setCookie(ipaBufferId++);
> You can use utils::enumerate() here:
>
> 	for (auto [i, buffer] : utils::enumerate(data->mackIPABufs_)) {
> 		buffer->setCookie(i++);
>
>> +		data->ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());
>> +	}
>> +	data->ipa_->mapBuffers(data->ipaBuffers_);
> The return value should be checked.

I didn't find mapBuffers/unmapBuffers() to return a value, in any of the 
mojom interfaces we have today.

I think mapBuffers() should have return value indeed, can I skip it here 
for now, and create a patch on top which touches all the concerned IPAs 
as well?

>
>> +
>>   	ret = data->ipa_->start();
>>   	if (ret) {
>>   		data->video_->releaseBuffers();
>> @@ -354,7 +364,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);
> You could use here
>
> 	for (const std::unique_ptr<FrameBuffer> &buffer : data->mockIPABufs_)
> 		ids.push_back(buffer->cookie());
>
> and replace VimcCameraData::ipaBuffers_ with a local variable in
> PipelineHandlerVimc::start().
>
>> +	data->ipa_->unmapBuffers(ids);
>>   	data->ipa_->stop();
>> +
>>   	data->video_->releaseBuffers();
>>   }
>>