[libcamera-devel] ipa: vimc: Add configure() function

13216 diff mbox series

As part of an effort to make the vimc IPA usable for testing, extend it
with a configure function. The configuration is currently ignored by the
IPA.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Umang, I've had this in my tree for some time, if it helps the work
you're doing on testing buffer mapping in vimc, please feel free to use
this patch (as-is, or squashed into anything else).
---
 include/libcamera/ipa/vimc.mojom     |  5 +++++
 src/ipa/vimc/vimc.cpp                | 11 +++++++++++
 src/libcamera/pipeline/vimc/vimc.cpp | 16 ++++++++++++++++
 3 files changed, 32 insertions(+)

Hi Laurent,

On Thu, Aug 05, 2021 at 08:01:47PM +0300, Laurent Pinchart wrote:
> As part of an effort to make the vimc IPA usable for testing, extend it
> with a configure function. The configuration is currently ignored by the
> IPA.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Umang, I've had this in my tree for some time, if it helps the work
> you're doing on testing buffer mapping in vimc, please feel free to use
> this patch (as-is, or squashed into anything else).
> ---
>  include/libcamera/ipa/vimc.mojom     |  5 +++++
>  src/ipa/vimc/vimc.cpp                | 11 +++++++++++
>  src/libcamera/pipeline/vimc/vimc.cpp | 16 ++++++++++++++++
>  3 files changed, 32 insertions(+)
> 
> diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom
> index 86bc318a878e..85d25e27ebcd 100644
> --- a/include/libcamera/ipa/vimc.mojom
> +++ b/include/libcamera/ipa/vimc.mojom
> @@ -19,6 +19,11 @@ enum IPAOperationCode {
>  
>  interface IPAVimcInterface {
>  	init(libcamera.IPASettings settings) => (int32 ret);
> +
> +	configure(libcamera.IPACameraSensorInfo sensorInfo,
> +		  map<uint32, libcamera.IPAStream> streamConfig,
> +		  map<uint32, libcamera.ControlInfoMap> entityControls) => ();

Since we're going to use it for testing, should we return int?

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> +
>  	start() => (int32 ret);
>  	stop();
>  };
> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> index 0c0ee006fdc7..5da5c6c09d68 100644
> --- a/src/ipa/vimc/vimc.cpp
> +++ b/src/ipa/vimc/vimc.cpp
> @@ -34,6 +34,10 @@ public:
>  	int start() override;
>  	void stop() override;
>  
> +	void configure(const IPACameraSensorInfo &sensorInfo,
> +		       const std::map<unsigned int, IPAStream> &streamConfig,
> +		       const std::map<unsigned int, ControlInfoMap> &entityControls) override;
> +
>  private:
>  	void initTrace();
>  	void trace(enum ipa::vimc::IPAOperationCode operation);
> @@ -86,6 +90,13 @@ void IPAVimc::stop()
>  	LOG(IPAVimc, Debug) << "stop vimc IPA!";
>  }
>  
> +void IPAVimc::configure([[maybe_unused]] const IPACameraSensorInfo &sensorInfo,
> +			[[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> +			[[maybe_unused]] const std::map<unsigned int, ControlInfoMap> &entityControls)
> +{
> +	LOG(IPAVimc, Debug) << "configure()";
> +}
> +
>  void IPAVimc::initTrace()
>  {
>  	struct stat fifoStat;
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index 12f7517fd0ae..b7dd6595d608 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -295,6 +295,22 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
>  
>  	cfg.setStream(&data->stream_);
>  
> +	if (data->ipa_) {
> +		/* Inform IPA of stream configuration and sensor controls. */
> +		std::map<unsigned int, IPAStream> streamConfig;
> +		streamConfig.emplace(std::piecewise_construct,
> +				     std::forward_as_tuple(0),
> +				     std::forward_as_tuple(cfg.pixelFormat, cfg.size));
> +
> +		std::map<unsigned int, ControlInfoMap> entityControls;
> +		entityControls.emplace(0, data->sensor_->controls());
> +
> +		IPACameraSensorInfo sensorInfo;
> +		data->sensor_->sensorInfo(&sensorInfo);
> +
> +		data->ipa_->configure(sensorInfo, streamConfig, entityControls);
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> Regards,
> 
> Laurent Pinchart
>

Hi Laurent,

thanks for the patch.

On 8/5/21 10:31 PM, Laurent Pinchart wrote:
> As part of an effort to make the vimc IPA usable for testing, extend it
> with a configure function. The configuration is currently ignored by the
> IPA.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Umang, I've had this in my tree for some time, if it helps the work
> you're doing on testing buffer mapping in vimc, please feel free to use
> this patch (as-is, or squashed into anything else).
Ok. :-)
> ---
>   include/libcamera/ipa/vimc.mojom     |  5 +++++
>   src/ipa/vimc/vimc.cpp                | 11 +++++++++++
>   src/libcamera/pipeline/vimc/vimc.cpp | 16 ++++++++++++++++
>   3 files changed, 32 insertions(+)
>
> diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom
> index 86bc318a878e..85d25e27ebcd 100644
> --- a/include/libcamera/ipa/vimc.mojom
> +++ b/include/libcamera/ipa/vimc.mojom
> @@ -19,6 +19,11 @@ enum IPAOperationCode {
>   
>   interface IPAVimcInterface {
>   	init(libcamera.IPASettings settings) => (int32 ret);
> +
> +	configure(libcamera.IPACameraSensorInfo sensorInfo,
> +		  map<uint32, libcamera.IPAStream> streamConfig,
> +		  map<uint32, libcamera.ControlInfoMap> entityControls) => ();
> +

We should return int here, as IPU3 does. I'll fixup this and carry it in 
my ongoing series.

We can merge this as part of that series.

Rest looks good:
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

>   	start() => (int32 ret);
>   	stop();
>   };
> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> index 0c0ee006fdc7..5da5c6c09d68 100644
> --- a/src/ipa/vimc/vimc.cpp
> +++ b/src/ipa/vimc/vimc.cpp
> @@ -34,6 +34,10 @@ public:
>   	int start() override;
>   	void stop() override;
>   
> +	void configure(const IPACameraSensorInfo &sensorInfo,
> +		       const std::map<unsigned int, IPAStream> &streamConfig,
> +		       const std::map<unsigned int, ControlInfoMap> &entityControls) override;
> +
>   private:
>   	void initTrace();
>   	void trace(enum ipa::vimc::IPAOperationCode operation);
> @@ -86,6 +90,13 @@ void IPAVimc::stop()
>   	LOG(IPAVimc, Debug) << "stop vimc IPA!";
>   }
>   
> +void IPAVimc::configure([[maybe_unused]] const IPACameraSensorInfo &sensorInfo,
> +			[[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> +			[[maybe_unused]] const std::map<unsigned int, ControlInfoMap> &entityControls)
> +{
> +	LOG(IPAVimc, Debug) << "configure()";
> +}
> +
>   void IPAVimc::initTrace()
>   {
>   	struct stat fifoStat;
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index 12f7517fd0ae..b7dd6595d608 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -295,6 +295,22 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
>   
>   	cfg.setStream(&data->stream_);
>   
> +	if (data->ipa_) {
> +		/* Inform IPA of stream configuration and sensor controls. */
> +		std::map<unsigned int, IPAStream> streamConfig;
> +		streamConfig.emplace(std::piecewise_construct,
> +				     std::forward_as_tuple(0),
> +				     std::forward_as_tuple(cfg.pixelFormat, cfg.size));
> +
> +		std::map<unsigned int, ControlInfoMap> entityControls;
> +		entityControls.emplace(0, data->sensor_->controls());
> +
> +		IPACameraSensorInfo sensorInfo;
> +		data->sensor_->sensorInfo(&sensorInfo);
> +
> +		data->ipa_->configure(sensorInfo, streamConfig, entityControls);
> +	}
> +
>   	return 0;
>   }
>