[libcamera-devel] [PATCH v1 6/9] ipa: raspberrypi: Pass lens shading table through configure() function

[Jacopo Mondi]

Hi Laurent,

On Mon, Jun 29, 2020 at 02:19:31AM +0300, Laurent Pinchart wrote:
> The IPAInterface now accepts custom configuration data. Use it to pass

The IPAInterface::configure()

> the lens shading table instead of using a custom IPA event. This will
> allow starting the IPA when starting the camera, instead of pre-starting
> it early in order to process the lens shading table allocation event.

If the IPA is meant to be started at Camera::start() time, and we pass
the lens shading table at ipa_->configure() time which happens before,
what is different ?

>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  include/libcamera/ipa/raspberrypi.h                |  5 ++++-
>  src/ipa/raspberrypi/raspberrypi.cpp                | 12 ++++++------
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++------
>  3 files changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index a18ce9a884b6..c335d0259549 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -10,6 +10,10 @@
>  #include <libcamera/control_ids.h>
>  #include <libcamera/controls.h>
>
> +enum RPiConfigParameters {
> +	RPI_IPA_CONFIG_LSTABLE = (1 << 0),
> +};
> +

Is there any value in keeping this enum separated if not creating a
distinction between operations run through processEvent() and
operations run at configure() time ? Can't the list of operation be
unique (which would also avoid calshing, if the same operations has
to be handled both at processEvent() and configure() time.

Also, why (1 << 0) in an enum ? Can't we start from 0 (or 1 as
RPiOperations does) and increase ?

>  enum RPiOperations {
>  	RPI_IPA_ACTION_V4L2_SET_STAGGERED = 1,
>  	RPI_IPA_ACTION_V4L2_SET_ISP,
> @@ -21,7 +25,6 @@ enum RPiOperations {
>  	RPI_IPA_EVENT_SIGNAL_STAT_READY,
>  	RPI_IPA_EVENT_SIGNAL_ISP_PREPARE,
>  	RPI_IPA_EVENT_QUEUE_REQUEST,
> -	RPI_IPA_EVENT_LS_TABLE_ALLOCATION,
>  };
>
>  enum RPiIpaMask {
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 860be22ddb5d..c9f7dea375a5 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -277,6 +277,12 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		applyAGC(&agcStatus);
>
>  	lastMode_ = mode_;
> +
> +	/* Store the lens shading table pointer and handle if available. */
> +	if (ipaConfig.operation & RPI_IPA_CONFIG_LSTABLE) {

That's why (1 << 0). What is the advantage of using flags, instead of
switching on the operation identifiers as done below in
processEvent() ?

> +		lsTable_ = reinterpret_cast<void *>(ipaConfig.data[0]);
> +		lsTableHandle_ = ipaConfig.data[1];
> +	}
>  }
>
>  void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
> @@ -358,12 +364,6 @@ void IPARPi::processEvent(const IPAOperationData &event)
>  		break;
>  	}
>
> -	case RPI_IPA_EVENT_LS_TABLE_ALLOCATION: {
> -		lsTable_ = reinterpret_cast<void *>(event.data[0]);
> -		lsTableHandle_ = event.data[1];
> -		break;
> -	}
> -
>  	default:
>  		LOG(IPARPI, Error) << "Unknown event " << event.operation;
>  		break;
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 0f9237a7f346..903796790f44 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1109,6 +1109,7 @@ int RPiCameraData::configureIPA()
>  {
>  	std::map<unsigned int, IPAStream> streamConfig;
>  	std::map<unsigned int, const ControlInfoMap &> entityControls;
> +	IPAOperationData ipaConfig = {};
>
>  	/* Get the device format to pass to the IPA. */
>  	V4L2DeviceFormat sensorFormat;
> @@ -1138,11 +1139,9 @@ int RPiCameraData::configureIPA()
>  		 * The vcsm allocation will always be in the memory region
>  		 * < 32-bits to allow Videocore to access the memory.
>  		 */
> -		IPAOperationData op;
> -		op.operation = RPI_IPA_EVENT_LS_TABLE_ALLOCATION;
> -		op.data = { static_cast<uint32_t>(ptr & 0xffffffff),
> -			    vcsm_.getVCHandle(lsTable_) };
> -		ipa_->processEvent(op);
> +		ipaConfig.operation = RPI_IPA_CONFIG_LSTABLE;
> +		ipaConfig.data = { static_cast<uint32_t>(ptr & 0xffffffff),
> +				   vcsm_.getVCHandle(lsTable_) };
>  	}
>
>  	CameraSensorInfo sensorInfo = {};
> @@ -1153,7 +1152,6 @@ int RPiCameraData::configureIPA()
>  	}
>
>  	/* Ready the IPA - it must know about the sensor resolution. */
> -	IPAOperationData ipaConfig;
>  	ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
>  			nullptr);
>
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel