[libcamera-devel,v4,7/7] libcamera: ipa: Add support for CameraSensorInfo
diff mbox series

Message ID 20200427213236.333777-8-jacopo@jmondi.org
State Superseded
Headers show
Series
  • libcamera: Add CameraSensorInfo
Related show

Commit Message

Jacopo Mondi April 27, 2020, 9:32 p.m. UTC 
Add support for camera sensor information in the libcamera IPA protocol.

Define a new 'struct ipa_sensor_info' structure in the IPA context and
use it to perform translation between the C and the C++ API.

Update the IPAInterface::configure() operation to accept a new
CameraSensorInfo parameter and port all users of that function to
the new interface.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/ipa/ipa_interface.h                 | 26 +++++++-
 src/ipa/libipa/ipa_interface_wrapper.cpp    | 19 +++++-
 src/ipa/libipa/ipa_interface_wrapper.h      |  1 +
 src/ipa/rkisp1/rkisp1.cpp                   | 13 +++-
 src/ipa/vimc/vimc.cpp                       |  4 +-
 src/libcamera/include/ipa_context_wrapper.h |  4 +-
 src/libcamera/ipa_context_wrapper.cpp       | 23 ++++++-
 src/libcamera/ipa_interface.cpp             | 73 +++++++++++++++++++++
 src/libcamera/pipeline/rkisp1/rkisp1.cpp    | 14 +++-
 src/libcamera/proxy/ipa_proxy_linux.cpp     |  4 +-
 src/libcamera/proxy/ipa_proxy_thread.cpp    |  9 ++-
 test/ipa/ipa_wrappers_test.cpp              | 22 ++++++-
 12 files changed, 195 insertions(+), 17 deletions(-)

Comments

Laurent Pinchart April 28, 2020, 2:21 a.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Mon, Apr 27, 2020 at 11:32:36PM +0200, Jacopo Mondi wrote:
> Add support for camera sensor information in the libcamera IPA protocol.
> 
> Define a new 'struct ipa_sensor_info' structure in the IPA context and
> use it to perform translation between the C and the C++ API.
> 
> Update the IPAInterface::configure() operation to accept a new
> CameraSensorInfo parameter and port all users of that function to
> the new interface.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  include/ipa/ipa_interface.h                 | 26 +++++++-
>  src/ipa/libipa/ipa_interface_wrapper.cpp    | 19 +++++-
>  src/ipa/libipa/ipa_interface_wrapper.h      |  1 +
>  src/ipa/rkisp1/rkisp1.cpp                   | 13 +++-
>  src/ipa/vimc/vimc.cpp                       |  4 +-
>  src/libcamera/include/ipa_context_wrapper.h |  4 +-
>  src/libcamera/ipa_context_wrapper.cpp       | 23 ++++++-
>  src/libcamera/ipa_interface.cpp             | 73 +++++++++++++++++++++
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp    | 14 +++-
>  src/libcamera/proxy/ipa_proxy_linux.cpp     |  4 +-
>  src/libcamera/proxy/ipa_proxy_thread.cpp    |  9 ++-
>  test/ipa/ipa_wrappers_test.cpp              | 22 ++++++-
>  12 files changed, 195 insertions(+), 17 deletions(-)
> 
> diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
> index e65844bc7b34..0a6d849d8f89 100644
> --- a/include/ipa/ipa_interface.h
> +++ b/include/ipa/ipa_interface.h
> @@ -18,6 +18,27 @@ struct ipa_context {
>  	const struct ipa_context_ops *ops;
>  };
>  
> +struct ipa_sensor_info {
> +	const char *model;
> +	uint8_t bits_per_pixel;
> +	struct {
> +		uint32_t width;
> +		uint32_t height;
> +	} active_area;
> +	struct {
> +		int32_t left;
> +		int32_t top;
> +		uint32_t width;
> +		uint32_t height;
> +	} analog_crop;
> +	struct {
> +		uint32_t width;
> +		uint32_t height;
> +	} output_size;
> +	uint32_t pixel_rate;

Is 32 bits enough ? (Same question for CameraSensorInfo)

> +	uint32_t line_length;
> +};
> +
>  struct ipa_stream {
>  	unsigned int id;
>  	unsigned int pixel_format;
> @@ -70,6 +91,7 @@ struct ipa_context_ops {
>  				   const struct ipa_callback_ops *callbacks,
>  				   void *cb_ctx);
>  	void (*configure)(struct ipa_context *ctx,
> +			  const struct ipa_sensor_info *sensor_info,
>  			  const struct ipa_stream *streams,
>  			  unsigned int num_streams,
>  			  const struct ipa_control_info_map *maps,
> @@ -116,6 +138,7 @@ struct IPAOperationData {
>  	std::vector<ControlList> controls;
>  };
>  
> +struct CameraSensorInfo;

Could you add a blank line ?

I've just realized that we'll need to move CameraSensorInfo out of
camera_sensor.h if we want to make it accessible to IPAs without
accessing internal APIs :-( That can be fixed on top, but could you add
a \todo somewhere ?

>  class IPAInterface
>  {
>  public:
> @@ -125,7 +148,8 @@ public:
>  	virtual int start() = 0;
>  	virtual void stop() = 0;
>  
> -	virtual void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +	virtual void configure(const CameraSensorInfo &sensorInfo,
> +			       const std::map<unsigned int, IPAStream> &streamConfig,
>  			       const std::map<unsigned int, const ControlInfoMap &> &entityControls) = 0;
>  
>  	virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> index f50f93a0185a..e65822c62315 100644
> --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> @@ -15,6 +15,7 @@
>  #include <ipa/ipa_interface.h>
>  
>  #include "byte_stream_buffer.h"
> +#include "camera_sensor.h"
>  
>  /**
>   * \file ipa_interface_wrapper.h
> @@ -111,6 +112,7 @@ void IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx,
>  }
>  
>  void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> +				    const struct ipa_sensor_info *sensor_info,
>  				    const struct ipa_stream *streams,
>  				    unsigned int num_streams,
>  				    const struct ipa_control_info_map *maps,
> @@ -120,6 +122,21 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
>  
>  	ctx->serializer_.reset();
>  
> +	/* Translate the IPA sensor info. */
> +	CameraSensorInfo sensorInfo{};
> +	sensorInfo.model = sensor_info->model;
> +	sensorInfo.bitsPerPixel = sensor_info->bits_per_pixel;
> +	sensorInfo.activeAreaSize = { sensor_info->active_area.width,
> +				      sensor_info->active_area.height };
> +	sensorInfo.analogCrop = { sensor_info->analog_crop.left,
> +				  sensor_info->analog_crop.top,
> +				  sensor_info->analog_crop.width,
> +				  sensor_info->analog_crop.height };
> +	sensorInfo.outputSize = { sensor_info->output_size.width,
> +				  sensor_info->output_size.height };
> +	sensorInfo.pixelRate = sensor_info->pixel_rate;
> +	sensorInfo.lineLength = sensor_info->line_length;
> +
>  	/* Translate the IPA stream configurations map. */
>  	std::map<unsigned int, IPAStream> ipaStreams;
>  
> @@ -145,7 +162,7 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
>  		entityControls.emplace(id, infoMaps[id]);
>  	}
>  
> -	ctx->ipa_->configure(ipaStreams, entityControls);
> +	ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls);
>  }
>  
>  void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
> diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h
> index e4bc6dd4535d..febd6e66d0c4 100644
> --- a/src/ipa/libipa/ipa_interface_wrapper.h
> +++ b/src/ipa/libipa/ipa_interface_wrapper.h
> @@ -30,6 +30,7 @@ private:
>  				       const struct ipa_callback_ops *callbacks,
>  				       void *cb_ctx);
>  	static void configure(struct ipa_context *ctx,
> +			      const struct ipa_sensor_info *sensor_info,
>  			      const struct ipa_stream *streams,
>  			      unsigned int num_streams,
>  			      const struct ipa_control_info_map *maps,
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index acbbe58c7a2e..b6da672c1384 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -22,6 +22,7 @@
>  #include <libcamera/request.h>
>  #include <libipa/ipa_interface_wrapper.h>
>  
> +#include "camera_sensor.h"

No need to include this here, there's already at least a forward
declaration available as the base class uses CameraSensorInfo in its
API.

>  #include "ipa_module.h"
>  #include "log.h"
>  #include "utils.h"
>  
> @@ -36,7 +37,8 @@ public:
>  	int start() override { return 0; }
>  	void stop() override {}
>  
> -	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +	void configure(const CameraSensorInfo &info,
> +		       const std::map<unsigned int, IPAStream> &streamConfig,
>  		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> @@ -66,7 +68,14 @@ private:
>  	uint32_t maxGain_;
>  };
>  
> -void IPARkISP1::configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +/**
> + * \todo The RkISP1 pipeline currently provides an empty CameraSensorInfo
> + * if the connected sensor does not provide enough information to properly
> + * assemble one. Make sure the reported sensor information are relevant
> + * before accessing them.
> + */
> +void IPARkISP1::configure(const CameraSensorInfo &info,
> +			  const std::map<unsigned int, IPAStream> &streamConfig,
>  			  const std::map<unsigned int, const ControlInfoMap &> &entityControls)
>  {
>  	if (entityControls.empty())
> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> index d2267e11737f..b8aadd8588cb 100644
> --- a/src/ipa/vimc/vimc.cpp
> +++ b/src/ipa/vimc/vimc.cpp
> @@ -19,6 +19,7 @@
>  
>  #include <libipa/ipa_interface_wrapper.h>
>  
> +#include "camera_sensor.h"

Same here.

>  #include "log.h"
>  
>  namespace libcamera {
> @@ -36,7 +37,8 @@ public:
>  	int start() override;
>  	void stop() override;
>  
> -	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +	void configure(const CameraSensorInfo &sensorInfo,
> +		       const std::map<unsigned int, IPAStream> &streamConfig,
>  		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override {}
> diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h
> index 0a48bfe732c8..cfb435ee1b91 100644
> --- a/src/libcamera/include/ipa_context_wrapper.h
> +++ b/src/libcamera/include/ipa_context_wrapper.h
> @@ -13,6 +13,7 @@
>  
>  namespace libcamera {
>  
> +struct CameraSensorInfo;

For the same reason the forward declaration isn't needed here.

>  class IPAContextWrapper final : public IPAInterface
>  {
>  public:
> @@ -22,7 +23,8 @@ public:
>  	int init() override;
>  	int start() override;
>  	void stop() override;
> -	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +	void configure(const CameraSensorInfo &sensorInfo,
> +		       const std::map<unsigned int, IPAStream> &streamConfig,
>  		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
>  
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
> index ab6ce396b88a..d591c67f8791 100644
> --- a/src/libcamera/ipa_context_wrapper.cpp
> +++ b/src/libcamera/ipa_context_wrapper.cpp
> @@ -12,6 +12,7 @@
>  #include <libcamera/controls.h>
>  
>  #include "byte_stream_buffer.h"
> +#include "camera_sensor.h"
>  #include "utils.h"
>  
>  /**
> @@ -104,17 +105,33 @@ void IPAContextWrapper::stop()
>  	ctx_->ops->stop(ctx_);
>  }
>  
> -void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
> +				  const std::map<unsigned int, IPAStream> &streamConfig,
>  				  const std::map<unsigned int, const ControlInfoMap &> &entityControls)
>  {
>  	if (intf_)
> -		return intf_->configure(streamConfig, entityControls);
> +		return intf_->configure(sensorInfo, streamConfig, entityControls);
>  
>  	if (!ctx_)
>  		return;
>  
>  	serializer_.reset();
>  
> +	/* Translate the camera sensor info. */
> +	struct ipa_sensor_info sensor_info = {};
> +	sensor_info.model = sensorInfo.model.c_str();
> +	sensor_info.bits_per_pixel = sensorInfo.bitsPerPixel;
> +	sensor_info.active_area.width = sensorInfo.activeAreaSize.width;
> +	sensor_info.active_area.height = sensorInfo.activeAreaSize.height;
> +	sensor_info.analog_crop.left = sensorInfo.analogCrop.x;
> +	sensor_info.analog_crop.top = sensorInfo.analogCrop.y;
> +	sensor_info.analog_crop.width = sensorInfo.analogCrop.width;
> +	sensor_info.analog_crop.height = sensorInfo.analogCrop.height;
> +	sensor_info.output_size.width = sensorInfo.outputSize.width;
> +	sensor_info.output_size.height = sensorInfo.outputSize.height;
> +	sensor_info.pixel_rate = sensorInfo.pixelRate;
> +	sensor_info.line_length = sensorInfo.lineLength;
> +
>  	/* Translate the IPA stream configurations map. */
>  	struct ipa_stream c_streams[streamConfig.size()];
>  
> @@ -154,7 +171,7 @@ void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &strea
>  		++i;
>  	}
>  
> -	ctx_->ops->configure(ctx_, c_streams, streamConfig.size(),
> +	ctx_->ops->configure(ctx_, &sensor_info, c_streams, streamConfig.size(),
>  			     c_info_maps, entityControls.size());
>  }
>  
> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> index 890d4340e4f3..13df090f3374 100644
> --- a/src/libcamera/ipa_interface.cpp
> +++ b/src/libcamera/ipa_interface.cpp
> @@ -92,6 +92,74 @@
>   * \brief The IPA context operations
>   */
>  
> +/**
> + * \struct ipa_sensor_info
> + * \brief Camera sensor information for the IPA context operations
> + * \sa libcamera::CameraSensorInfo
> + *
> + * \var ipa_sensor_info::model
> + * \brief The camera sensor model name
> + * \todo Remove this field as soon as no IPA depends on it anymore
> + *
> + * \var ipa_sensor_info::bits_per_pixel
> + * \brief The camera sensor image format bit depth
> + * \sa libcamera::CameraSensorInfo::bitsPerPixel
> + *
> + * \var ipa_sensor_info::active_area.width
> + * \brief The camera sensor pixel array active area width
> + * \sa libcamera::CameraSensorInfo::activeAreaSize
> + *
> + * \var ipa_sensor_info::active_area.height
> + * \brief The camera sensor pixel array active area height
> + * \sa libcamera::CameraSensorInfo::activeAreaSize
> + *
> + * \var ipa_sensor_info::active_area
> + * \brief The camera sensor pixel array active size
> + * \sa libcamera::CameraSensorInfo::activeAreaSize
> + *
> + * \var ipa_sensor_info::analog_crop.left
> + * \brief The left coordinate of the analog crop rectangle, relative to the
> + * pixel array active area
> + * \sa libcamera::CameraSensorInfo::analogCrop
> + *
> + * \var ipa_sensor_info::analog_crop.top
> + * \brief The top coordinate of the analog crop rectangle, relative to the pixel
> + * array active area
> + * \sa libcamera::CameraSensorInfo::analogCrop
> + *
> + * \var ipa_sensor_info::analog_crop.width
> + * \brief The horizontal size of the analog crop rectangle
> + * \sa libcamera::CameraSensorInfo::analogCrop
> + *
> + * \var ipa_sensor_info::analog_crop.height
> + * \brief The vertical size of the analog crop rectangle
> + * \sa libcamera::CameraSensorInfo::analogCrop
> + *
> + * \var ipa_sensor_info::analog_crop
> + * \brief The analog crop rectangle
> + * \sa libcamera::CameraSensorInfo::analogCrop
> + *
> + * \var ipa_sensor_info::output_size.width
> + * \brief The horizontal size of the output image
> + * \sa libcamera::CameraSensorInfo::outputSize
> + *
> + * \var ipa_sensor_info::output_size.height
> + * \brief The vertical size of the output image
> + * \sa libcamera::CameraSensorInfo::outputSize
> + *
> + * \var ipa_sensor_info::output_size
> + * \brief The size of the output image
> + * \sa libcamera::CameraSensorInfo::outputSize
> + *
> + * \var ipa_sensor_info::pixel_rate
> + * \brief The number of pixel produced in a second
> + * \sa libcamera::CameraSensorInfo::pixelClock

s/pixelClock/pixelRate/

No warning from Doxygen ? Could you check the compiled doc to see if
everything is fine ?

> + *
> + * \var ipa_sensor_info::line_length
> + * \brief The full line length, including blanking, in pixel units
> + * \sa libcamera::CameraSensorInfo::lineLength
> + */
> +
>  /**
>   * \struct ipa_stream
>   * \brief Stream information for the IPA context operations
> @@ -447,6 +515,7 @@ namespace libcamera {
>  /**
>   * \fn IPAInterface::configure()
>   * \brief Configure the IPA stream and sensor settings
> + * \param[in] sensorInfo Camera sensor information
>   * \param[in] streamConfig Configuration of all active streams
>   * \param[in] entityControls Controls provided by the pipeline entities
>   *
> @@ -454,6 +523,10 @@ namespace libcamera {
>   * the camera's streams and the sensor settings. The meaning of the numerical
>   * keys in the \a streamConfig and \a entityControls maps is defined by the IPA
>   * protocol.
> + *
> + * The \a sensorInfo conveys information about the camera sensor settings that
> + * the pipeline handler has selected for the configuration. The IPA may use
> + * that information to tune its algorithms.
>   */
>  
>  /**
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index f42264361785..4058b9014fb9 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -821,7 +821,17 @@ int PipelineHandlerRkISP1::start(Camera *camera)
>  
>  	activeCamera_ = camera;
>  
> -	/* Inform IPA of stream configuration and sensor controls. */
> +	/*
> +	 * Inform IPA of stream configuration and sensor controls.
> +	 */

Any reason to switch to a multiline comment ? I don't mind much, just
curious.

> +	CameraSensorInfo sensorInfo = {};
> +	ret = data->sensor_->sensorInfo(&sensorInfo);
> +	if (ret) {
> +		/* \todo Turn this in an hard failure. */
> +		LOG(RkISP1, Info) << "Camera sensor information not available";

Let's make it a Warning already then.

> +		sensorInfo = {};
> +	}
> +
>  	std::map<unsigned int, IPAStream> streamConfig;
>  	streamConfig[0] = {
>  		.pixelFormat = data->stream_.configuration().pixelFormat,
> @@ -831,7 +841,7 @@ int PipelineHandlerRkISP1::start(Camera *camera)
>  	std::map<unsigned int, const ControlInfoMap &> entityControls;
>  	entityControls.emplace(0, data->sensor_->controls());
>  
> -	data->ipa_->configure(streamConfig, entityControls);
> +	data->ipa_->configure(sensorInfo, streamConfig, entityControls);
>  
>  	return ret;
>  }
> diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> index 2aa80b946704..54b1abc4727b 100644
> --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> @@ -10,6 +10,7 @@
>  #include <ipa/ipa_interface.h>
>  #include <ipa/ipa_module_info.h>
>  
> +#include "camera_sensor.h"

No need for this either.

>  #include "ipa_module.h"
>  #include "ipa_proxy.h"
>  #include "ipc_unixsocket.h"
> @@ -29,7 +30,8 @@ public:
>  	int init() override { return 0; }
>  	int start() override { return 0; }
>  	void stop() override {}
> -	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +	void configure(const CameraSensorInfo &sensorInfo,
> +		       const std::map<unsigned int, IPAStream> &streamConfig,
>  		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override {}
> diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
> index 368ccca1cf60..b06734371b39 100644
> --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> @@ -10,6 +10,7 @@
>  #include <ipa/ipa_interface.h>
>  #include <ipa/ipa_module_info.h>
>  
> +#include "camera_sensor.h"

Same here, this file doesn't access the contents of CameraSensorInfo,
you don't need to include the header file.

>  #include "ipa_context_wrapper.h"
>  #include "ipa_module.h"
>  #include "ipa_proxy.h"
> @@ -29,7 +30,8 @@ public:
>  	int start() override;
>  	void stop() override;
>  
> -	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +	void configure(const CameraSensorInfo &sensorInfo,
> +		       const std::map<unsigned int, IPAStream> &streamConfig,
>  		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> @@ -126,10 +128,11 @@ void IPAProxyThread::stop()
>  	thread_.wait();
>  }
>  
> -void IPAProxyThread::configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
> +			       const std::map<unsigned int, IPAStream> &streamConfig,
>  			       const std::map<unsigned int, const ControlInfoMap &> &entityControls)
>  {
> -	ipa_->configure(streamConfig, entityControls);
> +	ipa_->configure(sensorInfo, streamConfig, entityControls);
>  }
>  
>  void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers)
> diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
> index fae1d56b67c7..a6bf0836ac4e 100644
> --- a/test/ipa/ipa_wrappers_test.cpp
> +++ b/test/ipa/ipa_wrappers_test.cpp
> @@ -15,6 +15,7 @@
>  #include <libcamera/controls.h>
>  #include <libipa/ipa_interface_wrapper.h>
>  
> +#include "camera_sensor.h"
>  #include "device_enumerator.h"
>  #include "ipa_context_wrapper.h"
>  #include "media_device.h"
> @@ -60,9 +61,17 @@ public:
>  		report(Op_stop, TestPass);
>  	}
>  
> -	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> +	void configure(const CameraSensorInfo &sensorInfo,
> +		       const std::map<unsigned int, IPAStream> &streamConfig,
>  		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override
>  	{
> +		/* Verify sensorInfo. */
> +		if (sensorInfo.outputSize.width != 2560 ||
> +		    sensorInfo.outputSize.height != 1940) {
> +			cerr << "configure(): Invalid sensor info sizes: ("

Leftover '('. I would remove the ':' too. And s/sizes/size/

> +			     << sensorInfo.outputSize.toString();
> +		}
> +
>  		/* Verify streamConfig. */
>  		if (streamConfig.size() != 2) {
>  			cerr << "configure(): Invalid number of streams "
> @@ -287,13 +296,22 @@ protected:
>  		int ret;
>  
>  		/* Test configure(). */
> +		CameraSensorInfo sensorInfo{
> +			.model = "sensor",
> +			.bitsPerPixel = 8,
> +			.activeAreaSize = { 2576, 1956 },
> +			.analogCrop = { 8, 8, 2560, 1940 },
> +			.outputSize = { 2560, 1940 },
> +			.pixelRate = 96000000,
> +			.lineLength = 2918,
> +		};
>  		std::map<unsigned int, IPAStream> config{
>  			{ 1, { V4L2_PIX_FMT_YUYV, { 1024, 768 } } },
>  			{ 2, { V4L2_PIX_FMT_NV12, { 800, 600 } } },
>  		};
>  		std::map<unsigned int, const ControlInfoMap &> controlInfo;
>  		controlInfo.emplace(42, subdev_->controls());
> -		ret = INVOKE(configure, config, controlInfo);
> +		ret = INVOKE(configure, sensorInfo, config, controlInfo);
>  		if (ret == TestFail)
>  			return TestFail;
>
Jacopo Mondi April 28, 2020, 7:31 a.m. UTC | #2 
Hi Laurent,

On Tue, Apr 28, 2020 at 05:21:31AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Apr 27, 2020 at 11:32:36PM +0200, Jacopo Mondi wrote:
> > Add support for camera sensor information in the libcamera IPA protocol.
> >
> > Define a new 'struct ipa_sensor_info' structure in the IPA context and
> > use it to perform translation between the C and the C++ API.
> >
> > Update the IPAInterface::configure() operation to accept a new
> > CameraSensorInfo parameter and port all users of that function to
> > the new interface.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  include/ipa/ipa_interface.h                 | 26 +++++++-
> >  src/ipa/libipa/ipa_interface_wrapper.cpp    | 19 +++++-
> >  src/ipa/libipa/ipa_interface_wrapper.h      |  1 +
> >  src/ipa/rkisp1/rkisp1.cpp                   | 13 +++-
> >  src/ipa/vimc/vimc.cpp                       |  4 +-
> >  src/libcamera/include/ipa_context_wrapper.h |  4 +-
> >  src/libcamera/ipa_context_wrapper.cpp       | 23 ++++++-
> >  src/libcamera/ipa_interface.cpp             | 73 +++++++++++++++++++++
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp    | 14 +++-
> >  src/libcamera/proxy/ipa_proxy_linux.cpp     |  4 +-
> >  src/libcamera/proxy/ipa_proxy_thread.cpp    |  9 ++-
> >  test/ipa/ipa_wrappers_test.cpp              | 22 ++++++-
> >  12 files changed, 195 insertions(+), 17 deletions(-)
> >
> > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
> > index e65844bc7b34..0a6d849d8f89 100644
> > --- a/include/ipa/ipa_interface.h
> > +++ b/include/ipa/ipa_interface.h
> > @@ -18,6 +18,27 @@ struct ipa_context {
> >  	const struct ipa_context_ops *ops;
> >  };
> >
> > +struct ipa_sensor_info {
> > +	const char *model;
> > +	uint8_t bits_per_pixel;
> > +	struct {
> > +		uint32_t width;
> > +		uint32_t height;
> > +	} active_area;
> > +	struct {
> > +		int32_t left;
> > +		int32_t top;
> > +		uint32_t width;
> > +		uint32_t height;
> > +	} analog_crop;
> > +	struct {
> > +		uint32_t width;
> > +		uint32_t height;
> > +	} output_size;
> > +	uint32_t pixel_rate;
>
> Is 32 bits enough ? (Same question for CameraSensorInfo)
>

The V4L2 control is 64 bits, let's make this 64 too

> > +	uint32_t line_length;
> > +};
> > +
> >  struct ipa_stream {
> >  	unsigned int id;
> >  	unsigned int pixel_format;
> > @@ -70,6 +91,7 @@ struct ipa_context_ops {
> >  				   const struct ipa_callback_ops *callbacks,
> >  				   void *cb_ctx);
> >  	void (*configure)(struct ipa_context *ctx,
> > +			  const struct ipa_sensor_info *sensor_info,
> >  			  const struct ipa_stream *streams,
> >  			  unsigned int num_streams,
> >  			  const struct ipa_control_info_map *maps,
> > @@ -116,6 +138,7 @@ struct IPAOperationData {
> >  	std::vector<ControlList> controls;
> >  };
> >
> > +struct CameraSensorInfo;
>
> Could you add a blank line ?
>
> I've just realized that we'll need to move CameraSensorInfo out of
> camera_sensor.h if we want to make it accessible to IPAs without
> accessing internal APIs :-( That can be fixed on top, but could you add
> a \todo somewhere ?
>

I had the same thought, but I then realized we already include
v4l2_controls, and I considerd this to be fine.

We might want to remove both includes.

> >  class IPAInterface
> >  {
> >  public:
> > @@ -125,7 +148,8 @@ public:
> >  	virtual int start() = 0;
> >  	virtual void stop() = 0;
> >
> > -	virtual void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +	virtual void configure(const CameraSensorInfo &sensorInfo,
> > +			       const std::map<unsigned int, IPAStream> &streamConfig,
> >  			       const std::map<unsigned int, const ControlInfoMap &> &entityControls) = 0;
> >
> >  	virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > index f50f93a0185a..e65822c62315 100644
> > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > @@ -15,6 +15,7 @@
> >  #include <ipa/ipa_interface.h>
> >
> >  #include "byte_stream_buffer.h"
> > +#include "camera_sensor.h"
> >
> >  /**
> >   * \file ipa_interface_wrapper.h
> > @@ -111,6 +112,7 @@ void IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx,
> >  }
> >
> >  void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> > +				    const struct ipa_sensor_info *sensor_info,
> >  				    const struct ipa_stream *streams,
> >  				    unsigned int num_streams,
> >  				    const struct ipa_control_info_map *maps,
> > @@ -120,6 +122,21 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> >
> >  	ctx->serializer_.reset();
> >
> > +	/* Translate the IPA sensor info. */
> > +	CameraSensorInfo sensorInfo{};
> > +	sensorInfo.model = sensor_info->model;
> > +	sensorInfo.bitsPerPixel = sensor_info->bits_per_pixel;
> > +	sensorInfo.activeAreaSize = { sensor_info->active_area.width,
> > +				      sensor_info->active_area.height };
> > +	sensorInfo.analogCrop = { sensor_info->analog_crop.left,
> > +				  sensor_info->analog_crop.top,
> > +				  sensor_info->analog_crop.width,
> > +				  sensor_info->analog_crop.height };
> > +	sensorInfo.outputSize = { sensor_info->output_size.width,
> > +				  sensor_info->output_size.height };
> > +	sensorInfo.pixelRate = sensor_info->pixel_rate;
> > +	sensorInfo.lineLength = sensor_info->line_length;
> > +
> >  	/* Translate the IPA stream configurations map. */
> >  	std::map<unsigned int, IPAStream> ipaStreams;
> >
> > @@ -145,7 +162,7 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> >  		entityControls.emplace(id, infoMaps[id]);
> >  	}
> >
> > -	ctx->ipa_->configure(ipaStreams, entityControls);
> > +	ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls);
> >  }
> >
> >  void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
> > diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h
> > index e4bc6dd4535d..febd6e66d0c4 100644
> > --- a/src/ipa/libipa/ipa_interface_wrapper.h
> > +++ b/src/ipa/libipa/ipa_interface_wrapper.h
> > @@ -30,6 +30,7 @@ private:
> >  				       const struct ipa_callback_ops *callbacks,
> >  				       void *cb_ctx);
> >  	static void configure(struct ipa_context *ctx,
> > +			      const struct ipa_sensor_info *sensor_info,
> >  			      const struct ipa_stream *streams,
> >  			      unsigned int num_streams,
> >  			      const struct ipa_control_info_map *maps,
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index acbbe58c7a2e..b6da672c1384 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -22,6 +22,7 @@
> >  #include <libcamera/request.h>
> >  #include <libipa/ipa_interface_wrapper.h>
> >
> > +#include "camera_sensor.h"
>
> No need to include this here, there's already at least a forward
> declaration available as the base class uses CameraSensorInfo in its
> API.
>

That's a bit fragile, as we rely on the forward declaration of
ipa_interface.h

Also, I tend to go for #include in cpp files as, compared to headers,
we probably are going to actually use the type, and this seems to be
easier to maintain, for a very little price.

> >  #include "ipa_module.h"
> >  #include "log.h"
> >  #include "utils.h"
> >
> > @@ -36,7 +37,8 @@ public:
> >  	int start() override { return 0; }
> >  	void stop() override {}
> >
> > -	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +	void configure(const CameraSensorInfo &info,
> > +		       const std::map<unsigned int, IPAStream> &streamConfig,
> >  		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> >  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > @@ -66,7 +68,14 @@ private:
> >  	uint32_t maxGain_;
> >  };
> >
> > -void IPARkISP1::configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +/**
> > + * \todo The RkISP1 pipeline currently provides an empty CameraSensorInfo
> > + * if the connected sensor does not provide enough information to properly
> > + * assemble one. Make sure the reported sensor information are relevant
> > + * before accessing them.
> > + */
> > +void IPARkISP1::configure(const CameraSensorInfo &info,
> > +			  const std::map<unsigned int, IPAStream> &streamConfig,
> >  			  const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> >  {
> >  	if (entityControls.empty())
> > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> > index d2267e11737f..b8aadd8588cb 100644
> > --- a/src/ipa/vimc/vimc.cpp
> > +++ b/src/ipa/vimc/vimc.cpp
> > @@ -19,6 +19,7 @@
> >
> >  #include <libipa/ipa_interface_wrapper.h>
> >
> > +#include "camera_sensor.h"
>
> Same here.
>

Same commment, but for now, I'll drop.

> >  #include "log.h"
> >
> >  namespace libcamera {
> > @@ -36,7 +37,8 @@ public:
> >  	int start() override;
> >  	void stop() override;
> >
> > -	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +	void configure(const CameraSensorInfo &sensorInfo,
> > +		       const std::map<unsigned int, IPAStream> &streamConfig,
> >  		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
> >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> >  	void unmapBuffers(const std::vector<unsigned int> &ids) override {}
> > diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h
> > index 0a48bfe732c8..cfb435ee1b91 100644
> > --- a/src/libcamera/include/ipa_context_wrapper.h
> > +++ b/src/libcamera/include/ipa_context_wrapper.h
> > @@ -13,6 +13,7 @@
> >
> >  namespace libcamera {
> >
> > +struct CameraSensorInfo;
>
> For the same reason the forward declaration isn't needed here.
>

This seems very fragile and also a bit obscure. To me it's like not
including an header as it is already included in an header we include.

> >  class IPAContextWrapper final : public IPAInterface
> >  {
> >  public:
> > @@ -22,7 +23,8 @@ public:
> >  	int init() override;
> >  	int start() override;
> >  	void stop() override;
> > -	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +	void configure(const CameraSensorInfo &sensorInfo,
> > +		       const std::map<unsigned int, IPAStream> &streamConfig,
> >  		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> >
> >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
> > index ab6ce396b88a..d591c67f8791 100644
> > --- a/src/libcamera/ipa_context_wrapper.cpp
> > +++ b/src/libcamera/ipa_context_wrapper.cpp
> > @@ -12,6 +12,7 @@
> >  #include <libcamera/controls.h>
> >
> >  #include "byte_stream_buffer.h"
> > +#include "camera_sensor.h"
> >  #include "utils.h"
> >
> >  /**
> > @@ -104,17 +105,33 @@ void IPAContextWrapper::stop()
> >  	ctx_->ops->stop(ctx_);
> >  }
> >
> > -void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
> > +				  const std::map<unsigned int, IPAStream> &streamConfig,
> >  				  const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> >  {
> >  	if (intf_)
> > -		return intf_->configure(streamConfig, entityControls);
> > +		return intf_->configure(sensorInfo, streamConfig, entityControls);
> >
> >  	if (!ctx_)
> >  		return;
> >
> >  	serializer_.reset();
> >
> > +	/* Translate the camera sensor info. */
> > +	struct ipa_sensor_info sensor_info = {};
> > +	sensor_info.model = sensorInfo.model.c_str();
> > +	sensor_info.bits_per_pixel = sensorInfo.bitsPerPixel;
> > +	sensor_info.active_area.width = sensorInfo.activeAreaSize.width;
> > +	sensor_info.active_area.height = sensorInfo.activeAreaSize.height;
> > +	sensor_info.analog_crop.left = sensorInfo.analogCrop.x;
> > +	sensor_info.analog_crop.top = sensorInfo.analogCrop.y;
> > +	sensor_info.analog_crop.width = sensorInfo.analogCrop.width;
> > +	sensor_info.analog_crop.height = sensorInfo.analogCrop.height;
> > +	sensor_info.output_size.width = sensorInfo.outputSize.width;
> > +	sensor_info.output_size.height = sensorInfo.outputSize.height;
> > +	sensor_info.pixel_rate = sensorInfo.pixelRate;
> > +	sensor_info.line_length = sensorInfo.lineLength;
> > +
> >  	/* Translate the IPA stream configurations map. */
> >  	struct ipa_stream c_streams[streamConfig.size()];
> >
> > @@ -154,7 +171,7 @@ void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &strea
> >  		++i;
> >  	}
> >
> > -	ctx_->ops->configure(ctx_, c_streams, streamConfig.size(),
> > +	ctx_->ops->configure(ctx_, &sensor_info, c_streams, streamConfig.size(),
> >  			     c_info_maps, entityControls.size());
> >  }
> >
> > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> > index 890d4340e4f3..13df090f3374 100644
> > --- a/src/libcamera/ipa_interface.cpp
> > +++ b/src/libcamera/ipa_interface.cpp
> > @@ -92,6 +92,74 @@
> >   * \brief The IPA context operations
> >   */
> >
> > +/**
> > + * \struct ipa_sensor_info
> > + * \brief Camera sensor information for the IPA context operations
> > + * \sa libcamera::CameraSensorInfo
> > + *
> > + * \var ipa_sensor_info::model
> > + * \brief The camera sensor model name
> > + * \todo Remove this field as soon as no IPA depends on it anymore
> > + *
> > + * \var ipa_sensor_info::bits_per_pixel
> > + * \brief The camera sensor image format bit depth
> > + * \sa libcamera::CameraSensorInfo::bitsPerPixel
> > + *
> > + * \var ipa_sensor_info::active_area.width
> > + * \brief The camera sensor pixel array active area width
> > + * \sa libcamera::CameraSensorInfo::activeAreaSize
> > + *
> > + * \var ipa_sensor_info::active_area.height
> > + * \brief The camera sensor pixel array active area height
> > + * \sa libcamera::CameraSensorInfo::activeAreaSize
> > + *
> > + * \var ipa_sensor_info::active_area
> > + * \brief The camera sensor pixel array active size
> > + * \sa libcamera::CameraSensorInfo::activeAreaSize
> > + *
> > + * \var ipa_sensor_info::analog_crop.left
> > + * \brief The left coordinate of the analog crop rectangle, relative to the
> > + * pixel array active area
> > + * \sa libcamera::CameraSensorInfo::analogCrop
> > + *
> > + * \var ipa_sensor_info::analog_crop.top
> > + * \brief The top coordinate of the analog crop rectangle, relative to the pixel
> > + * array active area
> > + * \sa libcamera::CameraSensorInfo::analogCrop
> > + *
> > + * \var ipa_sensor_info::analog_crop.width
> > + * \brief The horizontal size of the analog crop rectangle
> > + * \sa libcamera::CameraSensorInfo::analogCrop
> > + *
> > + * \var ipa_sensor_info::analog_crop.height
> > + * \brief The vertical size of the analog crop rectangle
> > + * \sa libcamera::CameraSensorInfo::analogCrop
> > + *
> > + * \var ipa_sensor_info::analog_crop
> > + * \brief The analog crop rectangle
> > + * \sa libcamera::CameraSensorInfo::analogCrop
> > + *
> > + * \var ipa_sensor_info::output_size.width
> > + * \brief The horizontal size of the output image
> > + * \sa libcamera::CameraSensorInfo::outputSize
> > + *
> > + * \var ipa_sensor_info::output_size.height
> > + * \brief The vertical size of the output image
> > + * \sa libcamera::CameraSensorInfo::outputSize
> > + *
> > + * \var ipa_sensor_info::output_size
> > + * \brief The size of the output image
> > + * \sa libcamera::CameraSensorInfo::outputSize
> > + *
> > + * \var ipa_sensor_info::pixel_rate
> > + * \brief The number of pixel produced in a second
> > + * \sa libcamera::CameraSensorInfo::pixelClock
>
> s/pixelClock/pixelRate/
>
> No warning from Doxygen ? Could you check the compiled doc to see if
> everything is fine ?

No warning here, weird. I'll fix this.

>
> > + *
> > + * \var ipa_sensor_info::line_length
> > + * \brief The full line length, including blanking, in pixel units
> > + * \sa libcamera::CameraSensorInfo::lineLength
> > + */
> > +
> >  /**
> >   * \struct ipa_stream
> >   * \brief Stream information for the IPA context operations
> > @@ -447,6 +515,7 @@ namespace libcamera {
> >  /**
> >   * \fn IPAInterface::configure()
> >   * \brief Configure the IPA stream and sensor settings
> > + * \param[in] sensorInfo Camera sensor information
> >   * \param[in] streamConfig Configuration of all active streams
> >   * \param[in] entityControls Controls provided by the pipeline entities
> >   *
> > @@ -454,6 +523,10 @@ namespace libcamera {
> >   * the camera's streams and the sensor settings. The meaning of the numerical
> >   * keys in the \a streamConfig and \a entityControls maps is defined by the IPA
> >   * protocol.
> > + *
> > + * The \a sensorInfo conveys information about the camera sensor settings that
> > + * the pipeline handler has selected for the configuration. The IPA may use
> > + * that information to tune its algorithms.
> >   */
> >
> >  /**
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index f42264361785..4058b9014fb9 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -821,7 +821,17 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> >
> >  	activeCamera_ = camera;
> >
> > -	/* Inform IPA of stream configuration and sensor controls. */
> > +	/*
> > +	 * Inform IPA of stream configuration and sensor controls.
> > +	 */
>
> Any reason to switch to a multiline comment ? I don't mind much, just
> curious.
>

I probably added a line of comment and then removed, then missed the
change during the rebase. I'll restore it.

> > +	CameraSensorInfo sensorInfo = {};
> > +	ret = data->sensor_->sensorInfo(&sensorInfo);
> > +	if (ret) {
> > +		/* \todo Turn this in an hard failure. */

Ah, that's the comment :)

> > +		LOG(RkISP1, Info) << "Camera sensor information not available";
>
> Let's make it a Warning already then.
>

Ack

> > +		sensorInfo = {};
> > +	}
> > +
> >  	std::map<unsigned int, IPAStream> streamConfig;
> >  	streamConfig[0] = {
> >  		.pixelFormat = data->stream_.configuration().pixelFormat,
> > @@ -831,7 +841,7 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> >  	std::map<unsigned int, const ControlInfoMap &> entityControls;
> >  	entityControls.emplace(0, data->sensor_->controls());
> >
> > -	data->ipa_->configure(streamConfig, entityControls);
> > +	data->ipa_->configure(sensorInfo, streamConfig, entityControls);
> >
> >  	return ret;
> >  }
> > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > index 2aa80b946704..54b1abc4727b 100644
> > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > @@ -10,6 +10,7 @@
> >  #include <ipa/ipa_interface.h>
> >  #include <ipa/ipa_module_info.h>
> >
> > +#include "camera_sensor.h"
>
> No need for this either.
>
> >  #include "ipa_module.h"
> >  #include "ipa_proxy.h"
> >  #include "ipc_unixsocket.h"
> > @@ -29,7 +30,8 @@ public:
> >  	int init() override { return 0; }
> >  	int start() override { return 0; }
> >  	void stop() override {}
> > -	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +	void configure(const CameraSensorInfo &sensorInfo,
> > +		       const std::map<unsigned int, IPAStream> &streamConfig,
> >  		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
> >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> >  	void unmapBuffers(const std::vector<unsigned int> &ids) override {}
> > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > index 368ccca1cf60..b06734371b39 100644
> > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > @@ -10,6 +10,7 @@
> >  #include <ipa/ipa_interface.h>
> >  #include <ipa/ipa_module_info.h>
> >
> > +#include "camera_sensor.h"
>
> Same here, this file doesn't access the contents of CameraSensorInfo,
> you don't need to include the header file.
>
> >  #include "ipa_context_wrapper.h"
> >  #include "ipa_module.h"
> >  #include "ipa_proxy.h"
> > @@ -29,7 +30,8 @@ public:
> >  	int start() override;
> >  	void stop() override;
> >
> > -	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +	void configure(const CameraSensorInfo &sensorInfo,
> > +		       const std::map<unsigned int, IPAStream> &streamConfig,
> >  		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> >  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > @@ -126,10 +128,11 @@ void IPAProxyThread::stop()
> >  	thread_.wait();
> >  }
> >
> > -void IPAProxyThread::configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
> > +			       const std::map<unsigned int, IPAStream> &streamConfig,
> >  			       const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> >  {
> > -	ipa_->configure(streamConfig, entityControls);
> > +	ipa_->configure(sensorInfo, streamConfig, entityControls);
> >  }
> >
> >  void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers)
> > diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
> > index fae1d56b67c7..a6bf0836ac4e 100644
> > --- a/test/ipa/ipa_wrappers_test.cpp
> > +++ b/test/ipa/ipa_wrappers_test.cpp
> > @@ -15,6 +15,7 @@
> >  #include <libcamera/controls.h>
> >  #include <libipa/ipa_interface_wrapper.h>
> >
> > +#include "camera_sensor.h"
> >  #include "device_enumerator.h"
> >  #include "ipa_context_wrapper.h"
> >  #include "media_device.h"
> > @@ -60,9 +61,17 @@ public:
> >  		report(Op_stop, TestPass);
> >  	}
> >
> > -	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > +	void configure(const CameraSensorInfo &sensorInfo,
> > +		       const std::map<unsigned int, IPAStream> &streamConfig,
> >  		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override
> >  	{
> > +		/* Verify sensorInfo. */
> > +		if (sensorInfo.outputSize.width != 2560 ||
> > +		    sensorInfo.outputSize.height != 1940) {
> > +			cerr << "configure(): Invalid sensor info sizes: ("
>
> Leftover '('. I would remove the ':' too. And s/sizes/size/
>

ah ups

> > +			     << sensorInfo.outputSize.toString();
> > +		}
> > +
> >  		/* Verify streamConfig. */
> >  		if (streamConfig.size() != 2) {
> >  			cerr << "configure(): Invalid number of streams "
> > @@ -287,13 +296,22 @@ protected:
> >  		int ret;
> >
> >  		/* Test configure(). */
> > +		CameraSensorInfo sensorInfo{
> > +			.model = "sensor",
> > +			.bitsPerPixel = 8,
> > +			.activeAreaSize = { 2576, 1956 },
> > +			.analogCrop = { 8, 8, 2560, 1940 },
> > +			.outputSize = { 2560, 1940 },
> > +			.pixelRate = 96000000,
> > +			.lineLength = 2918,
> > +		};
> >  		std::map<unsigned int, IPAStream> config{
> >  			{ 1, { V4L2_PIX_FMT_YUYV, { 1024, 768 } } },
> >  			{ 2, { V4L2_PIX_FMT_NV12, { 800, 600 } } },
> >  		};
> >  		std::map<unsigned int, const ControlInfoMap &> controlInfo;
> >  		controlInfo.emplace(42, subdev_->controls());
> > -		ret = INVOKE(configure, config, controlInfo);
> > +		ret = INVOKE(configure, sensorInfo, config, controlInfo);
> >  		if (ret == TestFail)
> >  			return TestFail;
> >
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart April 28, 2020, 12:15 p.m. UTC | #3 
Hi Jacopo,

On Tue, Apr 28, 2020 at 09:31:00AM +0200, Jacopo Mondi wrote:
> On Tue, Apr 28, 2020 at 05:21:31AM +0300, Laurent Pinchart wrote:
> > On Mon, Apr 27, 2020 at 11:32:36PM +0200, Jacopo Mondi wrote:
> > > Add support for camera sensor information in the libcamera IPA protocol.
> > >
> > > Define a new 'struct ipa_sensor_info' structure in the IPA context and
> > > use it to perform translation between the C and the C++ API.
> > >
> > > Update the IPAInterface::configure() operation to accept a new
> > > CameraSensorInfo parameter and port all users of that function to
> > > the new interface.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  include/ipa/ipa_interface.h                 | 26 +++++++-
> > >  src/ipa/libipa/ipa_interface_wrapper.cpp    | 19 +++++-
> > >  src/ipa/libipa/ipa_interface_wrapper.h      |  1 +
> > >  src/ipa/rkisp1/rkisp1.cpp                   | 13 +++-
> > >  src/ipa/vimc/vimc.cpp                       |  4 +-
> > >  src/libcamera/include/ipa_context_wrapper.h |  4 +-
> > >  src/libcamera/ipa_context_wrapper.cpp       | 23 ++++++-
> > >  src/libcamera/ipa_interface.cpp             | 73 +++++++++++++++++++++
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp    | 14 +++-
> > >  src/libcamera/proxy/ipa_proxy_linux.cpp     |  4 +-
> > >  src/libcamera/proxy/ipa_proxy_thread.cpp    |  9 ++-
> > >  test/ipa/ipa_wrappers_test.cpp              | 22 ++++++-
> > >  12 files changed, 195 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
> > > index e65844bc7b34..0a6d849d8f89 100644
> > > --- a/include/ipa/ipa_interface.h
> > > +++ b/include/ipa/ipa_interface.h
> > > @@ -18,6 +18,27 @@ struct ipa_context {
> > >  	const struct ipa_context_ops *ops;
> > >  };
> > >
> > > +struct ipa_sensor_info {
> > > +	const char *model;
> > > +	uint8_t bits_per_pixel;
> > > +	struct {
> > > +		uint32_t width;
> > > +		uint32_t height;
> > > +	} active_area;
> > > +	struct {
> > > +		int32_t left;
> > > +		int32_t top;
> > > +		uint32_t width;
> > > +		uint32_t height;
> > > +	} analog_crop;
> > > +	struct {
> > > +		uint32_t width;
> > > +		uint32_t height;
> > > +	} output_size;
> > > +	uint32_t pixel_rate;
> >
> > Is 32 bits enough ? (Same question for CameraSensorInfo)
> 
> The V4L2 control is 64 bits, let's make this 64 too
> 
> > > +	uint32_t line_length;
> > > +};
> > > +
> > >  struct ipa_stream {
> > >  	unsigned int id;
> > >  	unsigned int pixel_format;
> > > @@ -70,6 +91,7 @@ struct ipa_context_ops {
> > >  				   const struct ipa_callback_ops *callbacks,
> > >  				   void *cb_ctx);
> > >  	void (*configure)(struct ipa_context *ctx,
> > > +			  const struct ipa_sensor_info *sensor_info,
> > >  			  const struct ipa_stream *streams,
> > >  			  unsigned int num_streams,
> > >  			  const struct ipa_control_info_map *maps,
> > > @@ -116,6 +138,7 @@ struct IPAOperationData {
> > >  	std::vector<ControlList> controls;
> > >  };
> > >
> > > +struct CameraSensorInfo;
> >
> > Could you add a blank line ?
> >
> > I've just realized that we'll need to move CameraSensorInfo out of
> > camera_sensor.h if we want to make it accessible to IPAs without
> > accessing internal APIs :-( That can be fixed on top, but could you add
> > a \todo somewhere ?
> 
> I had the same thought, but I then realized we already include
> v4l2_controls, and I considerd this to be fine.
> 
> We might want to remove both includes.

Ouch :-S Indeed. Let's do so on top.

> > >  class IPAInterface
> > >  {
> > >  public:
> > > @@ -125,7 +148,8 @@ public:
> > >  	virtual int start() = 0;
> > >  	virtual void stop() = 0;
> > >
> > > -	virtual void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > > +	virtual void configure(const CameraSensorInfo &sensorInfo,
> > > +			       const std::map<unsigned int, IPAStream> &streamConfig,
> > >  			       const std::map<unsigned int, const ControlInfoMap &> &entityControls) = 0;
> > >
> > >  	virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
> > > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > index f50f93a0185a..e65822c62315 100644
> > > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
> > > @@ -15,6 +15,7 @@
> > >  #include <ipa/ipa_interface.h>
> > >
> > >  #include "byte_stream_buffer.h"
> > > +#include "camera_sensor.h"
> > >
> > >  /**
> > >   * \file ipa_interface_wrapper.h
> > > @@ -111,6 +112,7 @@ void IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx,
> > >  }
> > >
> > >  void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> > > +				    const struct ipa_sensor_info *sensor_info,
> > >  				    const struct ipa_stream *streams,
> > >  				    unsigned int num_streams,
> > >  				    const struct ipa_control_info_map *maps,
> > > @@ -120,6 +122,21 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> > >
> > >  	ctx->serializer_.reset();
> > >
> > > +	/* Translate the IPA sensor info. */
> > > +	CameraSensorInfo sensorInfo{};
> > > +	sensorInfo.model = sensor_info->model;
> > > +	sensorInfo.bitsPerPixel = sensor_info->bits_per_pixel;
> > > +	sensorInfo.activeAreaSize = { sensor_info->active_area.width,
> > > +				      sensor_info->active_area.height };
> > > +	sensorInfo.analogCrop = { sensor_info->analog_crop.left,
> > > +				  sensor_info->analog_crop.top,
> > > +				  sensor_info->analog_crop.width,
> > > +				  sensor_info->analog_crop.height };
> > > +	sensorInfo.outputSize = { sensor_info->output_size.width,
> > > +				  sensor_info->output_size.height };
> > > +	sensorInfo.pixelRate = sensor_info->pixel_rate;
> > > +	sensorInfo.lineLength = sensor_info->line_length;
> > > +
> > >  	/* Translate the IPA stream configurations map. */
> > >  	std::map<unsigned int, IPAStream> ipaStreams;
> > >
> > > @@ -145,7 +162,7 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
> > >  		entityControls.emplace(id, infoMaps[id]);
> > >  	}
> > >
> > > -	ctx->ipa_->configure(ipaStreams, entityControls);
> > > +	ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls);
> > >  }
> > >
> > >  void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
> > > diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h
> > > index e4bc6dd4535d..febd6e66d0c4 100644
> > > --- a/src/ipa/libipa/ipa_interface_wrapper.h
> > > +++ b/src/ipa/libipa/ipa_interface_wrapper.h
> > > @@ -30,6 +30,7 @@ private:
> > >  				       const struct ipa_callback_ops *callbacks,
> > >  				       void *cb_ctx);
> > >  	static void configure(struct ipa_context *ctx,
> > > +			      const struct ipa_sensor_info *sensor_info,
> > >  			      const struct ipa_stream *streams,
> > >  			      unsigned int num_streams,
> > >  			      const struct ipa_control_info_map *maps,
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index acbbe58c7a2e..b6da672c1384 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -22,6 +22,7 @@
> > >  #include <libcamera/request.h>
> > >  #include <libipa/ipa_interface_wrapper.h>
> > >
> > > +#include "camera_sensor.h"
> >
> > No need to include this here, there's already at least a forward
> > declaration available as the base class uses CameraSensorInfo in its
> > API.
> 
> That's a bit fragile, as we rely on the forward declaration of
> ipa_interface.h

There's a guaranteed forward declaration in ipa_interface.h. I tend to
rely on that when I don't need the full definition, as in here. Up to
you.

> Also, I tend to go for #include in cpp files as, compared to headers,
> we probably are going to actually use the type, and this seems to be
> easier to maintain, for a very little price.
> 
> > >  #include "ipa_module.h"
> > >  #include "log.h"
> > >  #include "utils.h"
> > >
> > > @@ -36,7 +37,8 @@ public:
> > >  	int start() override { return 0; }
> > >  	void stop() override {}
> > >
> > > -	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > > +	void configure(const CameraSensorInfo &info,
> > > +		       const std::map<unsigned int, IPAStream> &streamConfig,
> > >  		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > >  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > > @@ -66,7 +68,14 @@ private:
> > >  	uint32_t maxGain_;
> > >  };
> > >
> > > -void IPARkISP1::configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > > +/**
> > > + * \todo The RkISP1 pipeline currently provides an empty CameraSensorInfo
> > > + * if the connected sensor does not provide enough information to properly
> > > + * assemble one. Make sure the reported sensor information are relevant
> > > + * before accessing them.
> > > + */
> > > +void IPARkISP1::configure(const CameraSensorInfo &info,
> > > +			  const std::map<unsigned int, IPAStream> &streamConfig,
> > >  			  const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > >  {
> > >  	if (entityControls.empty())
> > > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> > > index d2267e11737f..b8aadd8588cb 100644
> > > --- a/src/ipa/vimc/vimc.cpp
> > > +++ b/src/ipa/vimc/vimc.cpp
> > > @@ -19,6 +19,7 @@
> > >
> > >  #include <libipa/ipa_interface_wrapper.h>
> > >
> > > +#include "camera_sensor.h"
> >
> > Same here.
> >
> 
> Same commment, but for now, I'll drop.
> 
> > >  #include "log.h"
> > >
> > >  namespace libcamera {
> > > @@ -36,7 +37,8 @@ public:
> > >  	int start() override;
> > >  	void stop() override;
> > >
> > > -	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > > +	void configure(const CameraSensorInfo &sensorInfo,
> > > +		       const std::map<unsigned int, IPAStream> &streamConfig,
> > >  		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
> > >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > >  	void unmapBuffers(const std::vector<unsigned int> &ids) override {}
> > > diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h
> > > index 0a48bfe732c8..cfb435ee1b91 100644
> > > --- a/src/libcamera/include/ipa_context_wrapper.h
> > > +++ b/src/libcamera/include/ipa_context_wrapper.h
> > > @@ -13,6 +13,7 @@
> > >
> > >  namespace libcamera {
> > >
> > > +struct CameraSensorInfo;
> >
> > For the same reason the forward declaration isn't needed here.
> 
> This seems very fragile and also a bit obscure. To me it's like not
> including an header as it is already included in an header we include.

The usage of CameraSensorInfo here is to implement IPAInterface. We
include ipa_interface.h to do so. IPAInterface uses CameraSensorInfo, so
ipa_interface.h is guaranteed to have at least a forward declaration. We
don't rely on an internal detail of ipa_interface.h, but on something it
guarantees.

> > >  class IPAContextWrapper final : public IPAInterface
> > >  {
> > >  public:
> > > @@ -22,7 +23,8 @@ public:
> > >  	int init() override;
> > >  	int start() override;
> > >  	void stop() override;
> > > -	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > > +	void configure(const CameraSensorInfo &sensorInfo,
> > > +		       const std::map<unsigned int, IPAStream> &streamConfig,
> > >  		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > >
> > >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > > diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
> > > index ab6ce396b88a..d591c67f8791 100644
> > > --- a/src/libcamera/ipa_context_wrapper.cpp
> > > +++ b/src/libcamera/ipa_context_wrapper.cpp
> > > @@ -12,6 +12,7 @@
> > >  #include <libcamera/controls.h>
> > >
> > >  #include "byte_stream_buffer.h"
> > > +#include "camera_sensor.h"
> > >  #include "utils.h"
> > >
> > >  /**
> > > @@ -104,17 +105,33 @@ void IPAContextWrapper::stop()
> > >  	ctx_->ops->stop(ctx_);
> > >  }
> > >
> > > -void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > > +void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
> > > +				  const std::map<unsigned int, IPAStream> &streamConfig,
> > >  				  const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > >  {
> > >  	if (intf_)
> > > -		return intf_->configure(streamConfig, entityControls);
> > > +		return intf_->configure(sensorInfo, streamConfig, entityControls);
> > >
> > >  	if (!ctx_)
> > >  		return;
> > >
> > >  	serializer_.reset();
> > >
> > > +	/* Translate the camera sensor info. */
> > > +	struct ipa_sensor_info sensor_info = {};
> > > +	sensor_info.model = sensorInfo.model.c_str();
> > > +	sensor_info.bits_per_pixel = sensorInfo.bitsPerPixel;
> > > +	sensor_info.active_area.width = sensorInfo.activeAreaSize.width;
> > > +	sensor_info.active_area.height = sensorInfo.activeAreaSize.height;
> > > +	sensor_info.analog_crop.left = sensorInfo.analogCrop.x;
> > > +	sensor_info.analog_crop.top = sensorInfo.analogCrop.y;
> > > +	sensor_info.analog_crop.width = sensorInfo.analogCrop.width;
> > > +	sensor_info.analog_crop.height = sensorInfo.analogCrop.height;
> > > +	sensor_info.output_size.width = sensorInfo.outputSize.width;
> > > +	sensor_info.output_size.height = sensorInfo.outputSize.height;
> > > +	sensor_info.pixel_rate = sensorInfo.pixelRate;
> > > +	sensor_info.line_length = sensorInfo.lineLength;
> > > +
> > >  	/* Translate the IPA stream configurations map. */
> > >  	struct ipa_stream c_streams[streamConfig.size()];
> > >
> > > @@ -154,7 +171,7 @@ void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &strea
> > >  		++i;
> > >  	}
> > >
> > > -	ctx_->ops->configure(ctx_, c_streams, streamConfig.size(),
> > > +	ctx_->ops->configure(ctx_, &sensor_info, c_streams, streamConfig.size(),
> > >  			     c_info_maps, entityControls.size());
> > >  }
> > >
> > > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> > > index 890d4340e4f3..13df090f3374 100644
> > > --- a/src/libcamera/ipa_interface.cpp
> > > +++ b/src/libcamera/ipa_interface.cpp
> > > @@ -92,6 +92,74 @@
> > >   * \brief The IPA context operations
> > >   */
> > >
> > > +/**
> > > + * \struct ipa_sensor_info
> > > + * \brief Camera sensor information for the IPA context operations
> > > + * \sa libcamera::CameraSensorInfo
> > > + *
> > > + * \var ipa_sensor_info::model
> > > + * \brief The camera sensor model name
> > > + * \todo Remove this field as soon as no IPA depends on it anymore
> > > + *
> > > + * \var ipa_sensor_info::bits_per_pixel
> > > + * \brief The camera sensor image format bit depth
> > > + * \sa libcamera::CameraSensorInfo::bitsPerPixel
> > > + *
> > > + * \var ipa_sensor_info::active_area.width
> > > + * \brief The camera sensor pixel array active area width
> > > + * \sa libcamera::CameraSensorInfo::activeAreaSize
> > > + *
> > > + * \var ipa_sensor_info::active_area.height
> > > + * \brief The camera sensor pixel array active area height
> > > + * \sa libcamera::CameraSensorInfo::activeAreaSize
> > > + *
> > > + * \var ipa_sensor_info::active_area
> > > + * \brief The camera sensor pixel array active size
> > > + * \sa libcamera::CameraSensorInfo::activeAreaSize
> > > + *
> > > + * \var ipa_sensor_info::analog_crop.left
> > > + * \brief The left coordinate of the analog crop rectangle, relative to the
> > > + * pixel array active area
> > > + * \sa libcamera::CameraSensorInfo::analogCrop
> > > + *
> > > + * \var ipa_sensor_info::analog_crop.top
> > > + * \brief The top coordinate of the analog crop rectangle, relative to the pixel
> > > + * array active area
> > > + * \sa libcamera::CameraSensorInfo::analogCrop
> > > + *
> > > + * \var ipa_sensor_info::analog_crop.width
> > > + * \brief The horizontal size of the analog crop rectangle
> > > + * \sa libcamera::CameraSensorInfo::analogCrop
> > > + *
> > > + * \var ipa_sensor_info::analog_crop.height
> > > + * \brief The vertical size of the analog crop rectangle
> > > + * \sa libcamera::CameraSensorInfo::analogCrop
> > > + *
> > > + * \var ipa_sensor_info::analog_crop
> > > + * \brief The analog crop rectangle
> > > + * \sa libcamera::CameraSensorInfo::analogCrop
> > > + *
> > > + * \var ipa_sensor_info::output_size.width
> > > + * \brief The horizontal size of the output image
> > > + * \sa libcamera::CameraSensorInfo::outputSize
> > > + *
> > > + * \var ipa_sensor_info::output_size.height
> > > + * \brief The vertical size of the output image
> > > + * \sa libcamera::CameraSensorInfo::outputSize
> > > + *
> > > + * \var ipa_sensor_info::output_size
> > > + * \brief The size of the output image
> > > + * \sa libcamera::CameraSensorInfo::outputSize
> > > + *
> > > + * \var ipa_sensor_info::pixel_rate
> > > + * \brief The number of pixel produced in a second
> > > + * \sa libcamera::CameraSensorInfo::pixelClock
> >
> > s/pixelClock/pixelRate/
> >
> > No warning from Doxygen ? Could you check the compiled doc to see if
> > everything is fine ?
> 
> No warning here, weird. I'll fix this.
> 
> > > + *
> > > + * \var ipa_sensor_info::line_length
> > > + * \brief The full line length, including blanking, in pixel units
> > > + * \sa libcamera::CameraSensorInfo::lineLength
> > > + */
> > > +
> > >  /**
> > >   * \struct ipa_stream
> > >   * \brief Stream information for the IPA context operations
> > > @@ -447,6 +515,7 @@ namespace libcamera {
> > >  /**
> > >   * \fn IPAInterface::configure()
> > >   * \brief Configure the IPA stream and sensor settings
> > > + * \param[in] sensorInfo Camera sensor information
> > >   * \param[in] streamConfig Configuration of all active streams
> > >   * \param[in] entityControls Controls provided by the pipeline entities
> > >   *
> > > @@ -454,6 +523,10 @@ namespace libcamera {
> > >   * the camera's streams and the sensor settings. The meaning of the numerical
> > >   * keys in the \a streamConfig and \a entityControls maps is defined by the IPA
> > >   * protocol.
> > > + *
> > > + * The \a sensorInfo conveys information about the camera sensor settings that
> > > + * the pipeline handler has selected for the configuration. The IPA may use
> > > + * that information to tune its algorithms.
> > >   */
> > >
> > >  /**
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index f42264361785..4058b9014fb9 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -821,7 +821,17 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> > >
> > >  	activeCamera_ = camera;
> > >
> > > -	/* Inform IPA of stream configuration and sensor controls. */
> > > +	/*
> > > +	 * Inform IPA of stream configuration and sensor controls.
> > > +	 */
> >
> > Any reason to switch to a multiline comment ? I don't mind much, just
> > curious.
> 
> I probably added a line of comment and then removed, then missed the
> change during the rebase. I'll restore it.
> 
> > > +	CameraSensorInfo sensorInfo = {};
> > > +	ret = data->sensor_->sensorInfo(&sensorInfo);
> > > +	if (ret) {
> > > +		/* \todo Turn this in an hard failure. */
> 
> Ah, that's the comment :)
> 
> > > +		LOG(RkISP1, Info) << "Camera sensor information not available";
> >
> > Let's make it a Warning already then.
> >
> 
> Ack
> 
> > > +		sensorInfo = {};
> > > +	}
> > > +
> > >  	std::map<unsigned int, IPAStream> streamConfig;
> > >  	streamConfig[0] = {
> > >  		.pixelFormat = data->stream_.configuration().pixelFormat,
> > > @@ -831,7 +841,7 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> > >  	std::map<unsigned int, const ControlInfoMap &> entityControls;
> > >  	entityControls.emplace(0, data->sensor_->controls());
> > >
> > > -	data->ipa_->configure(streamConfig, entityControls);
> > > +	data->ipa_->configure(sensorInfo, streamConfig, entityControls);
> > >
> > >  	return ret;
> > >  }
> > > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > index 2aa80b946704..54b1abc4727b 100644
> > > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
> > > @@ -10,6 +10,7 @@
> > >  #include <ipa/ipa_interface.h>
> > >  #include <ipa/ipa_module_info.h>
> > >
> > > +#include "camera_sensor.h"
> >
> > No need for this either.
> >
> > >  #include "ipa_module.h"
> > >  #include "ipa_proxy.h"
> > >  #include "ipc_unixsocket.h"
> > > @@ -29,7 +30,8 @@ public:
> > >  	int init() override { return 0; }
> > >  	int start() override { return 0; }
> > >  	void stop() override {}
> > > -	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > > +	void configure(const CameraSensorInfo &sensorInfo,
> > > +		       const std::map<unsigned int, IPAStream> &streamConfig,
> > >  		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
> > >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
> > >  	void unmapBuffers(const std::vector<unsigned int> &ids) override {}
> > > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > index 368ccca1cf60..b06734371b39 100644
> > > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
> > > @@ -10,6 +10,7 @@
> > >  #include <ipa/ipa_interface.h>
> > >  #include <ipa/ipa_module_info.h>
> > >
> > > +#include "camera_sensor.h"
> >
> > Same here, this file doesn't access the contents of CameraSensorInfo,
> > you don't need to include the header file.
> >
> > >  #include "ipa_context_wrapper.h"
> > >  #include "ipa_module.h"
> > >  #include "ipa_proxy.h"
> > > @@ -29,7 +30,8 @@ public:
> > >  	int start() override;
> > >  	void stop() override;
> > >
> > > -	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > > +	void configure(const CameraSensorInfo &sensorInfo,
> > > +		       const std::map<unsigned int, IPAStream> &streamConfig,
> > >  		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
> > >  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > >  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > > @@ -126,10 +128,11 @@ void IPAProxyThread::stop()
> > >  	thread_.wait();
> > >  }
> > >
> > > -void IPAProxyThread::configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > > +void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
> > > +			       const std::map<unsigned int, IPAStream> &streamConfig,
> > >  			       const std::map<unsigned int, const ControlInfoMap &> &entityControls)
> > >  {
> > > -	ipa_->configure(streamConfig, entityControls);
> > > +	ipa_->configure(sensorInfo, streamConfig, entityControls);
> > >  }
> > >
> > >  void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers)
> > > diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
> > > index fae1d56b67c7..a6bf0836ac4e 100644
> > > --- a/test/ipa/ipa_wrappers_test.cpp
> > > +++ b/test/ipa/ipa_wrappers_test.cpp
> > > @@ -15,6 +15,7 @@
> > >  #include <libcamera/controls.h>
> > >  #include <libipa/ipa_interface_wrapper.h>
> > >
> > > +#include "camera_sensor.h"
> > >  #include "device_enumerator.h"
> > >  #include "ipa_context_wrapper.h"
> > >  #include "media_device.h"
> > > @@ -60,9 +61,17 @@ public:
> > >  		report(Op_stop, TestPass);
> > >  	}
> > >
> > > -	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
> > > +	void configure(const CameraSensorInfo &sensorInfo,
> > > +		       const std::map<unsigned int, IPAStream> &streamConfig,
> > >  		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override
> > >  	{
> > > +		/* Verify sensorInfo. */
> > > +		if (sensorInfo.outputSize.width != 2560 ||
> > > +		    sensorInfo.outputSize.height != 1940) {
> > > +			cerr << "configure(): Invalid sensor info sizes: ("
> >
> > Leftover '('. I would remove the ':' too. And s/sizes/size/
> >
> 
> ah ups
> 
> > > +			     << sensorInfo.outputSize.toString();
> > > +		}
> > > +
> > >  		/* Verify streamConfig. */
> > >  		if (streamConfig.size() != 2) {
> > >  			cerr << "configure(): Invalid number of streams "
> > > @@ -287,13 +296,22 @@ protected:
> > >  		int ret;
> > >
> > >  		/* Test configure(). */
> > > +		CameraSensorInfo sensorInfo{
> > > +			.model = "sensor",
> > > +			.bitsPerPixel = 8,
> > > +			.activeAreaSize = { 2576, 1956 },
> > > +			.analogCrop = { 8, 8, 2560, 1940 },
> > > +			.outputSize = { 2560, 1940 },
> > > +			.pixelRate = 96000000,
> > > +			.lineLength = 2918,
> > > +		};
> > >  		std::map<unsigned int, IPAStream> config{
> > >  			{ 1, { V4L2_PIX_FMT_YUYV, { 1024, 768 } } },
> > >  			{ 2, { V4L2_PIX_FMT_NV12, { 800, 600 } } },
> > >  		};
> > >  		std::map<unsigned int, const ControlInfoMap &> controlInfo;
> > >  		controlInfo.emplace(42, subdev_->controls());
> > > -		ret = INVOKE(configure, config, controlInfo);
> > > +		ret = INVOKE(configure, sensorInfo, config, controlInfo);
> > >  		if (ret == TestFail)
> > >  			return TestFail;
> > >

Patch
diff mbox series 

diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
index e65844bc7b34..0a6d849d8f89 100644
--- a/include/ipa/ipa_interface.h
+++ b/include/ipa/ipa_interface.h
@@ -18,6 +18,27 @@  struct ipa_context {
 	const struct ipa_context_ops *ops;
 };
 
+struct ipa_sensor_info {
+	const char *model;
+	uint8_t bits_per_pixel;
+	struct {
+		uint32_t width;
+		uint32_t height;
+	} active_area;
+	struct {
+		int32_t left;
+		int32_t top;
+		uint32_t width;
+		uint32_t height;
+	} analog_crop;
+	struct {
+		uint32_t width;
+		uint32_t height;
+	} output_size;
+	uint32_t pixel_rate;
+	uint32_t line_length;
+};
+
 struct ipa_stream {
 	unsigned int id;
 	unsigned int pixel_format;
@@ -70,6 +91,7 @@  struct ipa_context_ops {
 				   const struct ipa_callback_ops *callbacks,
 				   void *cb_ctx);
 	void (*configure)(struct ipa_context *ctx,
+			  const struct ipa_sensor_info *sensor_info,
 			  const struct ipa_stream *streams,
 			  unsigned int num_streams,
 			  const struct ipa_control_info_map *maps,
@@ -116,6 +138,7 @@  struct IPAOperationData {
 	std::vector<ControlList> controls;
 };
 
+struct CameraSensorInfo;
 class IPAInterface
 {
 public:
@@ -125,7 +148,8 @@  public:
 	virtual int start() = 0;
 	virtual void stop() = 0;
 
-	virtual void configure(const std::map<unsigned int, IPAStream> &streamConfig,
+	virtual void configure(const CameraSensorInfo &sensorInfo,
+			       const std::map<unsigned int, IPAStream> &streamConfig,
 			       const std::map<unsigned int, const ControlInfoMap &> &entityControls) = 0;
 
 	virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0;
diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp
index f50f93a0185a..e65822c62315 100644
--- a/src/ipa/libipa/ipa_interface_wrapper.cpp
+++ b/src/ipa/libipa/ipa_interface_wrapper.cpp
@@ -15,6 +15,7 @@ 
 #include <ipa/ipa_interface.h>
 
 #include "byte_stream_buffer.h"
+#include "camera_sensor.h"
 
 /**
  * \file ipa_interface_wrapper.h
@@ -111,6 +112,7 @@  void IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx,
 }
 
 void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
+				    const struct ipa_sensor_info *sensor_info,
 				    const struct ipa_stream *streams,
 				    unsigned int num_streams,
 				    const struct ipa_control_info_map *maps,
@@ -120,6 +122,21 @@  void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
 
 	ctx->serializer_.reset();
 
+	/* Translate the IPA sensor info. */
+	CameraSensorInfo sensorInfo{};
+	sensorInfo.model = sensor_info->model;
+	sensorInfo.bitsPerPixel = sensor_info->bits_per_pixel;
+	sensorInfo.activeAreaSize = { sensor_info->active_area.width,
+				      sensor_info->active_area.height };
+	sensorInfo.analogCrop = { sensor_info->analog_crop.left,
+				  sensor_info->analog_crop.top,
+				  sensor_info->analog_crop.width,
+				  sensor_info->analog_crop.height };
+	sensorInfo.outputSize = { sensor_info->output_size.width,
+				  sensor_info->output_size.height };
+	sensorInfo.pixelRate = sensor_info->pixel_rate;
+	sensorInfo.lineLength = sensor_info->line_length;
+
 	/* Translate the IPA stream configurations map. */
 	std::map<unsigned int, IPAStream> ipaStreams;
 
@@ -145,7 +162,7 @@  void IPAInterfaceWrapper::configure(struct ipa_context *_ctx,
 		entityControls.emplace(id, infoMaps[id]);
 	}
 
-	ctx->ipa_->configure(ipaStreams, entityControls);
+	ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls);
 }
 
 void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx,
diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h
index e4bc6dd4535d..febd6e66d0c4 100644
--- a/src/ipa/libipa/ipa_interface_wrapper.h
+++ b/src/ipa/libipa/ipa_interface_wrapper.h
@@ -30,6 +30,7 @@  private:
 				       const struct ipa_callback_ops *callbacks,
 				       void *cb_ctx);
 	static void configure(struct ipa_context *ctx,
+			      const struct ipa_sensor_info *sensor_info,
 			      const struct ipa_stream *streams,
 			      unsigned int num_streams,
 			      const struct ipa_control_info_map *maps,
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index acbbe58c7a2e..b6da672c1384 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -22,6 +22,7 @@ 
 #include <libcamera/request.h>
 #include <libipa/ipa_interface_wrapper.h>
 
+#include "camera_sensor.h"
 #include "log.h"
 #include "utils.h"
 
@@ -36,7 +37,8 @@  public:
 	int start() override { return 0; }
 	void stop() override {}
 
-	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
+	void configure(const CameraSensorInfo &info,
+		       const std::map<unsigned int, IPAStream> &streamConfig,
 		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
 	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
 	void unmapBuffers(const std::vector<unsigned int> &ids) override;
@@ -66,7 +68,14 @@  private:
 	uint32_t maxGain_;
 };
 
-void IPARkISP1::configure(const std::map<unsigned int, IPAStream> &streamConfig,
+/**
+ * \todo The RkISP1 pipeline currently provides an empty CameraSensorInfo
+ * if the connected sensor does not provide enough information to properly
+ * assemble one. Make sure the reported sensor information are relevant
+ * before accessing them.
+ */
+void IPARkISP1::configure(const CameraSensorInfo &info,
+			  const std::map<unsigned int, IPAStream> &streamConfig,
 			  const std::map<unsigned int, const ControlInfoMap &> &entityControls)
 {
 	if (entityControls.empty())
diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
index d2267e11737f..b8aadd8588cb 100644
--- a/src/ipa/vimc/vimc.cpp
+++ b/src/ipa/vimc/vimc.cpp
@@ -19,6 +19,7 @@ 
 
 #include <libipa/ipa_interface_wrapper.h>
 
+#include "camera_sensor.h"
 #include "log.h"
 
 namespace libcamera {
@@ -36,7 +37,8 @@  public:
 	int start() override;
 	void stop() override;
 
-	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
+	void configure(const CameraSensorInfo &sensorInfo,
+		       const std::map<unsigned int, IPAStream> &streamConfig,
 		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
 	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
 	void unmapBuffers(const std::vector<unsigned int> &ids) override {}
diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h
index 0a48bfe732c8..cfb435ee1b91 100644
--- a/src/libcamera/include/ipa_context_wrapper.h
+++ b/src/libcamera/include/ipa_context_wrapper.h
@@ -13,6 +13,7 @@ 
 
 namespace libcamera {
 
+struct CameraSensorInfo;
 class IPAContextWrapper final : public IPAInterface
 {
 public:
@@ -22,7 +23,8 @@  public:
 	int init() override;
 	int start() override;
 	void stop() override;
-	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
+	void configure(const CameraSensorInfo &sensorInfo,
+		       const std::map<unsigned int, IPAStream> &streamConfig,
 		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
 
 	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp
index ab6ce396b88a..d591c67f8791 100644
--- a/src/libcamera/ipa_context_wrapper.cpp
+++ b/src/libcamera/ipa_context_wrapper.cpp
@@ -12,6 +12,7 @@ 
 #include <libcamera/controls.h>
 
 #include "byte_stream_buffer.h"
+#include "camera_sensor.h"
 #include "utils.h"
 
 /**
@@ -104,17 +105,33 @@  void IPAContextWrapper::stop()
 	ctx_->ops->stop(ctx_);
 }
 
-void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &streamConfig,
+void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo,
+				  const std::map<unsigned int, IPAStream> &streamConfig,
 				  const std::map<unsigned int, const ControlInfoMap &> &entityControls)
 {
 	if (intf_)
-		return intf_->configure(streamConfig, entityControls);
+		return intf_->configure(sensorInfo, streamConfig, entityControls);
 
 	if (!ctx_)
 		return;
 
 	serializer_.reset();
 
+	/* Translate the camera sensor info. */
+	struct ipa_sensor_info sensor_info = {};
+	sensor_info.model = sensorInfo.model.c_str();
+	sensor_info.bits_per_pixel = sensorInfo.bitsPerPixel;
+	sensor_info.active_area.width = sensorInfo.activeAreaSize.width;
+	sensor_info.active_area.height = sensorInfo.activeAreaSize.height;
+	sensor_info.analog_crop.left = sensorInfo.analogCrop.x;
+	sensor_info.analog_crop.top = sensorInfo.analogCrop.y;
+	sensor_info.analog_crop.width = sensorInfo.analogCrop.width;
+	sensor_info.analog_crop.height = sensorInfo.analogCrop.height;
+	sensor_info.output_size.width = sensorInfo.outputSize.width;
+	sensor_info.output_size.height = sensorInfo.outputSize.height;
+	sensor_info.pixel_rate = sensorInfo.pixelRate;
+	sensor_info.line_length = sensorInfo.lineLength;
+
 	/* Translate the IPA stream configurations map. */
 	struct ipa_stream c_streams[streamConfig.size()];
 
@@ -154,7 +171,7 @@  void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &strea
 		++i;
 	}
 
-	ctx_->ops->configure(ctx_, c_streams, streamConfig.size(),
+	ctx_->ops->configure(ctx_, &sensor_info, c_streams, streamConfig.size(),
 			     c_info_maps, entityControls.size());
 }
 
diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
index 890d4340e4f3..13df090f3374 100644
--- a/src/libcamera/ipa_interface.cpp
+++ b/src/libcamera/ipa_interface.cpp
@@ -92,6 +92,74 @@ 
  * \brief The IPA context operations
  */
 
+/**
+ * \struct ipa_sensor_info
+ * \brief Camera sensor information for the IPA context operations
+ * \sa libcamera::CameraSensorInfo
+ *
+ * \var ipa_sensor_info::model
+ * \brief The camera sensor model name
+ * \todo Remove this field as soon as no IPA depends on it anymore
+ *
+ * \var ipa_sensor_info::bits_per_pixel
+ * \brief The camera sensor image format bit depth
+ * \sa libcamera::CameraSensorInfo::bitsPerPixel
+ *
+ * \var ipa_sensor_info::active_area.width
+ * \brief The camera sensor pixel array active area width
+ * \sa libcamera::CameraSensorInfo::activeAreaSize
+ *
+ * \var ipa_sensor_info::active_area.height
+ * \brief The camera sensor pixel array active area height
+ * \sa libcamera::CameraSensorInfo::activeAreaSize
+ *
+ * \var ipa_sensor_info::active_area
+ * \brief The camera sensor pixel array active size
+ * \sa libcamera::CameraSensorInfo::activeAreaSize
+ *
+ * \var ipa_sensor_info::analog_crop.left
+ * \brief The left coordinate of the analog crop rectangle, relative to the
+ * pixel array active area
+ * \sa libcamera::CameraSensorInfo::analogCrop
+ *
+ * \var ipa_sensor_info::analog_crop.top
+ * \brief The top coordinate of the analog crop rectangle, relative to the pixel
+ * array active area
+ * \sa libcamera::CameraSensorInfo::analogCrop
+ *
+ * \var ipa_sensor_info::analog_crop.width
+ * \brief The horizontal size of the analog crop rectangle
+ * \sa libcamera::CameraSensorInfo::analogCrop
+ *
+ * \var ipa_sensor_info::analog_crop.height
+ * \brief The vertical size of the analog crop rectangle
+ * \sa libcamera::CameraSensorInfo::analogCrop
+ *
+ * \var ipa_sensor_info::analog_crop
+ * \brief The analog crop rectangle
+ * \sa libcamera::CameraSensorInfo::analogCrop
+ *
+ * \var ipa_sensor_info::output_size.width
+ * \brief The horizontal size of the output image
+ * \sa libcamera::CameraSensorInfo::outputSize
+ *
+ * \var ipa_sensor_info::output_size.height
+ * \brief The vertical size of the output image
+ * \sa libcamera::CameraSensorInfo::outputSize
+ *
+ * \var ipa_sensor_info::output_size
+ * \brief The size of the output image
+ * \sa libcamera::CameraSensorInfo::outputSize
+ *
+ * \var ipa_sensor_info::pixel_rate
+ * \brief The number of pixel produced in a second
+ * \sa libcamera::CameraSensorInfo::pixelClock
+ *
+ * \var ipa_sensor_info::line_length
+ * \brief The full line length, including blanking, in pixel units
+ * \sa libcamera::CameraSensorInfo::lineLength
+ */
+
 /**
  * \struct ipa_stream
  * \brief Stream information for the IPA context operations
@@ -447,6 +515,7 @@  namespace libcamera {
 /**
  * \fn IPAInterface::configure()
  * \brief Configure the IPA stream and sensor settings
+ * \param[in] sensorInfo Camera sensor information
  * \param[in] streamConfig Configuration of all active streams
  * \param[in] entityControls Controls provided by the pipeline entities
  *
@@ -454,6 +523,10 @@  namespace libcamera {
  * the camera's streams and the sensor settings. The meaning of the numerical
  * keys in the \a streamConfig and \a entityControls maps is defined by the IPA
  * protocol.
+ *
+ * The \a sensorInfo conveys information about the camera sensor settings that
+ * the pipeline handler has selected for the configuration. The IPA may use
+ * that information to tune its algorithms.
  */
 
 /**
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index f42264361785..4058b9014fb9 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -821,7 +821,17 @@  int PipelineHandlerRkISP1::start(Camera *camera)
 
 	activeCamera_ = camera;
 
-	/* Inform IPA of stream configuration and sensor controls. */
+	/*
+	 * Inform IPA of stream configuration and sensor controls.
+	 */
+	CameraSensorInfo sensorInfo = {};
+	ret = data->sensor_->sensorInfo(&sensorInfo);
+	if (ret) {
+		/* \todo Turn this in an hard failure. */
+		LOG(RkISP1, Info) << "Camera sensor information not available";
+		sensorInfo = {};
+	}
+
 	std::map<unsigned int, IPAStream> streamConfig;
 	streamConfig[0] = {
 		.pixelFormat = data->stream_.configuration().pixelFormat,
@@ -831,7 +841,7 @@  int PipelineHandlerRkISP1::start(Camera *camera)
 	std::map<unsigned int, const ControlInfoMap &> entityControls;
 	entityControls.emplace(0, data->sensor_->controls());
 
-	data->ipa_->configure(streamConfig, entityControls);
+	data->ipa_->configure(sensorInfo, streamConfig, entityControls);
 
 	return ret;
 }
diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp
index 2aa80b946704..54b1abc4727b 100644
--- a/src/libcamera/proxy/ipa_proxy_linux.cpp
+++ b/src/libcamera/proxy/ipa_proxy_linux.cpp
@@ -10,6 +10,7 @@ 
 #include <ipa/ipa_interface.h>
 #include <ipa/ipa_module_info.h>
 
+#include "camera_sensor.h"
 #include "ipa_module.h"
 #include "ipa_proxy.h"
 #include "ipc_unixsocket.h"
@@ -29,7 +30,8 @@  public:
 	int init() override { return 0; }
 	int start() override { return 0; }
 	void stop() override {}
-	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
+	void configure(const CameraSensorInfo &sensorInfo,
+		       const std::map<unsigned int, IPAStream> &streamConfig,
 		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {}
 	void mapBuffers(const std::vector<IPABuffer> &buffers) override {}
 	void unmapBuffers(const std::vector<unsigned int> &ids) override {}
diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp
index 368ccca1cf60..b06734371b39 100644
--- a/src/libcamera/proxy/ipa_proxy_thread.cpp
+++ b/src/libcamera/proxy/ipa_proxy_thread.cpp
@@ -10,6 +10,7 @@ 
 #include <ipa/ipa_interface.h>
 #include <ipa/ipa_module_info.h>
 
+#include "camera_sensor.h"
 #include "ipa_context_wrapper.h"
 #include "ipa_module.h"
 #include "ipa_proxy.h"
@@ -29,7 +30,8 @@  public:
 	int start() override;
 	void stop() override;
 
-	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
+	void configure(const CameraSensorInfo &sensorInfo,
+		       const std::map<unsigned int, IPAStream> &streamConfig,
 		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override;
 	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
 	void unmapBuffers(const std::vector<unsigned int> &ids) override;
@@ -126,10 +128,11 @@  void IPAProxyThread::stop()
 	thread_.wait();
 }
 
-void IPAProxyThread::configure(const std::map<unsigned int, IPAStream> &streamConfig,
+void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo,
+			       const std::map<unsigned int, IPAStream> &streamConfig,
 			       const std::map<unsigned int, const ControlInfoMap &> &entityControls)
 {
-	ipa_->configure(streamConfig, entityControls);
+	ipa_->configure(sensorInfo, streamConfig, entityControls);
 }
 
 void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers)
diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp
index fae1d56b67c7..a6bf0836ac4e 100644
--- a/test/ipa/ipa_wrappers_test.cpp
+++ b/test/ipa/ipa_wrappers_test.cpp
@@ -15,6 +15,7 @@ 
 #include <libcamera/controls.h>
 #include <libipa/ipa_interface_wrapper.h>
 
+#include "camera_sensor.h"
 #include "device_enumerator.h"
 #include "ipa_context_wrapper.h"
 #include "media_device.h"
@@ -60,9 +61,17 @@  public:
 		report(Op_stop, TestPass);
 	}
 
-	void configure(const std::map<unsigned int, IPAStream> &streamConfig,
+	void configure(const CameraSensorInfo &sensorInfo,
+		       const std::map<unsigned int, IPAStream> &streamConfig,
 		       const std::map<unsigned int, const ControlInfoMap &> &entityControls) override
 	{
+		/* Verify sensorInfo. */
+		if (sensorInfo.outputSize.width != 2560 ||
+		    sensorInfo.outputSize.height != 1940) {
+			cerr << "configure(): Invalid sensor info sizes: ("
+			     << sensorInfo.outputSize.toString();
+		}
+
 		/* Verify streamConfig. */
 		if (streamConfig.size() != 2) {
 			cerr << "configure(): Invalid number of streams "
@@ -287,13 +296,22 @@  protected:
 		int ret;
 
 		/* Test configure(). */
+		CameraSensorInfo sensorInfo{
+			.model = "sensor",
+			.bitsPerPixel = 8,
+			.activeAreaSize = { 2576, 1956 },
+			.analogCrop = { 8, 8, 2560, 1940 },
+			.outputSize = { 2560, 1940 },
+			.pixelRate = 96000000,
+			.lineLength = 2918,
+		};
 		std::map<unsigned int, IPAStream> config{
 			{ 1, { V4L2_PIX_FMT_YUYV, { 1024, 768 } } },
 			{ 2, { V4L2_PIX_FMT_NV12, { 800, 600 } } },
 		};
 		std::map<unsigned int, const ControlInfoMap &> controlInfo;
 		controlInfo.emplace(42, subdev_->controls());
-		ret = INVOKE(configure, config, controlInfo);
+		ret = INVOKE(configure, sensorInfo, config, controlInfo);
 		if (ret == TestFail)
 			return TestFail;