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

[Laurent Pinchart]

Hi Jacopo,

On Tue, Jun 30, 2020 at 12:45:08PM +0200, Jacopo Mondi wrote:
> 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 ?

Today the RPi IPA is started much earlier, to work around the lack of
API to pass custom configuration data. This change allows starting the
IPA at the right time.

> > 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.

They're different things though, so I think separate enums are best.
Ideally the IPAOperationData operation field should be an enum field of
the appropriate type. See the comment about IPC below.

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

See below :-)

> >  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() ?

See the rest of the series. This is used to pass multiple configuration
parameters, while processEvents() and queueFrameAction only handle a
single operation. I'd like to make the two more similar though, but I
believe it will involve a rework of how we handle IPA-specific
parameters, including for IPC serialization, and possibly involve some
type of IDL. I'd thus rather not invest too much time now in something
that will very likely change.

> > +		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