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

[Laurent Pinchart]

Hi Umang,

On Sat, Aug 14, 2021 at 10:26:45AM +0530, Umang Jain wrote:
> On 8/14/21 6:05 AM, Laurent Pinchart wrote:
> > 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?

Sure.

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

-- 
Regards,

Laurent Pinchart