[libcamera-devel] [PATCH v2 4/4] ipa: vimc: Send and retrieve FrameBuffers from IPA

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Fri, Aug 13, 2021 at 08:14:37PM +0530, Umang Jain wrote:
> Plumb through VIMC mojo interface to enable buffers passing.
> Since VIMC is a virtual test driver and does not have actual
> IPA buffers (parameters and statistics), we can only pass them
> as is, to and from the IPA. The buffers used here are mock
> FrameBuffers which are dmabuf backed(in other words, mmap()able

s/backed/backed /

> through MappedFramebuffer inside the IPA).
> 
> This commits shows:
> - Passing and filling the parameter buffer from the pipeline handler to
>   the IPA.
> - Passing request controls ControlList to the IPA.
> 
> Any tests using VIMC will now loop in the IPA paths. Any tests running
> in isolated mode will help us to test IPA IPC code paths especially
> around (de)serialization of data passing from pipeline handlers to the
> IPA. Future IPA interface tests can simply extend the VIMC mojom
> interface to achieve/test a specific use case as required.

The code here looks fine overall, but of course it doesn't do much,
because there's not much to do. I think that the main reason for this
patch series is to test buffer sharing with IPA and fd passing over IPC.
>From that point of view, passing buffer ids as integers at runtime
doesn't really improve test coverage. On the other hand, I think it can
be useful to exercise IPA communication at runtime, for instance to
catch performance issues. I'm not entirely sure, however, that we would
be able to catch such performance issues with vimc, but we may be able
to catch bad bugs still.

> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  include/libcamera/ipa/vimc.mojom     | 14 +++++++++++++-
>  src/ipa/vimc/vimc.cpp                | 19 +++++++++++++++++++
>  src/libcamera/pipeline/vimc/vimc.cpp | 11 +++++++++++
>  3 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom
> index 8cb240d3..85a2d8e4 100644
> --- a/include/libcamera/ipa/vimc.mojom
> +++ b/include/libcamera/ipa/vimc.mojom
> @@ -29,8 +29,20 @@ interface IPAVimcInterface {
>  
>  	mapBuffers(array<libcamera.IPABuffer> buffers);
>  	unmapBuffers(array<uint32> ids);
> +
> +	/*
> +	 * VIMC being a virtual test driver does not have actual parameters
> +	 * and statistics buffers. However, VIMC pipeline creates mock IPA
> +	 * buffers and map it to IPA.

Technically speaking, it's not because vimc is a driver for a virtual
device, it's because it doesn't implement that feature. I would just
write "VIMC does not have ...". Same comment for other patches in the
series, including commit message.

> +	 *
> +	 * Since, we are not working with actual IPA buffers here,

The buffers are real, and they're passed to the IPA, so they're IPA
buffers. The point of this series is to implement operations that are
not required by the driver but that mimick how other pipeline handler
operate, for testing purpose. How about this ?

	/*
	 * The vimc driver doesn't use parameters buffers. To maximize coverage
	 * of unit tests that rely on the VIMC pipeline handler, we still define
	 * interface functions that mimick how other pipeline handlers typically
	 * handle parameters at runtime.
	 */

Maybe commit messages could be update accordingly.

> +	 * following are mostly stub functions, introduced to leverage IPA
> +	 * IPC tests.
> +	 */
> +	[async] fillParams(uint32 frame, uint32 bufferId);
> +	[async] processControls(uint32 frame, libcamera.ControlList controls);
>  };
>  
>  interface IPAVimcEventInterface {
> -	dummyEvent(uint32 val);
> +	paramsFilled(uint32 bufferId);
>  };
> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> index 082b2db7..be189181 100644
> --- a/src/ipa/vimc/vimc.cpp
> +++ b/src/ipa/vimc/vimc.cpp
> @@ -43,6 +43,9 @@ public:
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
>  
> +	void fillParams(uint32_t frame, uint32_t bufferId) override;
> +	void processControls(uint32_t frame, const ControlList &controls) override;
> +
>  private:
>  	void initTrace();
>  	void trace(enum ipa::vimc::IPAOperationCode operation);
> @@ -125,6 +128,22 @@ void IPAVimc::unmapBuffers(const std::vector<unsigned int> &ids)
>  	}
>  }
>  
> +void IPAVimc::fillParams([[maybe_unused]] uint32_t frame, uint32_t bufferId)
> +{
> +	auto it = buffers_.find(bufferId);
> +	if (it == buffers_.end()) {
> +		LOG(IPAVimc, Error) << "Could not find parameter buffer";
> +		return;
> +	}
> +
> +	paramsFilled.emit(bufferId);
> +}
> +
> +void IPAVimc::processControls([[maybe_unused]] uint32_t frame,
> +			      [[maybe_unused]] const ControlList &controls)
> +{
> +}
> +
>  void IPAVimc::initTrace()
>  {
>  	struct stat fifoStat;
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index c44a6242..bfd545c8 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -52,6 +52,7 @@ public:
>  	int init();
>  	int allocateMockIPABuffers();
>  	void bufferReady(FrameBuffer *buffer);
> +	void paramsFilled(unsigned int id);
>  
>  	MediaDevice *media_;
>  	std::unique_ptr<CameraSensor> sensor_;
> @@ -433,6 +434,8 @@ int PipelineHandlerVimc::queueRequestDevice(Camera *camera, Request *request)
>  	if (ret < 0)
>  		return ret;
>  
> +	data->ipa_->processControls(request->sequence(), request->controls());
> +
>  	return 0;
>  }
>  
> @@ -466,6 +469,8 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
>  		return false;
>  	}
>  
> +	data->ipa_->paramsFilled.connect(data.get(), &VimcCameraData::paramsFilled);
> +
>  	std::string conf = data->ipa_->configurationFile("vimc.conf");
>  	data->ipa_->init(IPASettings{ conf, data->sensor_->model() });
>  
> @@ -570,6 +575,8 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)
>  
>  	pipe_->completeBuffer(request, buffer);
>  	pipe_->completeRequest(request);
> +
> +	ipa_->fillParams(request->sequence(), mockIPABufs_[0]->cookie());

We're always using the same buffer, but that's not an issue for now as
we don't actually use the buffer anywhere. Down the line, I could
imagine storing control values in the buffer, and applying those
controls in the pipeline handler, but that's for latter.

>  }
>  
>  int VimcCameraData::allocateMockIPABuffers()
> @@ -591,6 +598,10 @@ int VimcCameraData::allocateMockIPABuffers()
>  	return video_->exportBuffers(bufCount, &mockIPABufs_);
>  }
>  
> +void VimcCameraData::paramsFilled([[maybe_unused]] unsigned int id)
> +{
> +}
> +
>  REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc)
>  
>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart