[libcamera-devel] [PATCH v2 10/10] libcamera: pipeline: vimc: add dummy IPA

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Mon, Jun 03, 2019 at 07:16:37PM -0400, Paul Elder wrote:
> Make the vimc pipeline handler get the dummy IPA, to show how an IPA can
> be acquired by a pipeline handler.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> Changes in v2:
> - save IPA to pipeline data
> - no IPA is fatal error
> 
>  src/libcamera/pipeline/vimc.cpp | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index 78feeb8..f000c21 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -10,10 +10,12 @@
>  
>  #include <libcamera/camera.h>
>  #include <libcamera/ipa/ipa_module_info.h>
> +#include <libcamera/ipa/ipa_interface.h>

sort() :-)

>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  
>  #include "device_enumerator.h"
> +#include "ipa_manager.h"
>  #include "log.h"
>  #include "media_device.h"
>  #include "pipeline_handler.h"
> @@ -252,6 +254,15 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
>  	if (!media)
>  		return false;
>  
> +	IPAManager *ipaManager = IPAManager::instance();
> +	ipa_ = ipaManager->createIPA(this);

You could also write this

	ipa_ = IPAManager::instance()->createIPA(this);

> +	if (ipa_ == nullptr) {
> +		LOG(VIMC, Error) << "no matching IPA found";
> +		return false;

As the IPA isn't required yet, should we make this a non-fatal error and
downgrade the message to a warning ? My review of v1 wasn't very clear,
I meant to ask if it should really be an error, or just a warning. What
do you think ?

> +	} else {
> +		ipa_->init();

I'm tempted to move the init() call to createIPA(), what do you think ?

> +	}
> +
>  	std::unique_ptr<VimcCameraData> data = utils::make_unique<VimcCameraData>(this);
>  
>  	/* Locate and open the capture video node. */

-- 
Regards,

Laurent Pinchart