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

[Laurent Pinchart]

Hi Niklas,

(CC'ing Dave)

On Mon, Jun 29, 2020 at 04:51:09PM +0200, Niklas Söderlund wrote:
> On 2020-06-29 02:19:31 +0300, Laurent Pinchart wrote:
> > The IPAInterface now accepts custom configuration data. Use it to pass
> > 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.
> > 
> > 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),
> > +};
> > +
> >  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) {
> > +		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_) };
> 
> Sending a pointer to the IPA caught my eye as potentially dangerous. If 
> I understand the situation correctly this is a temporary workaround 
> while vc_sm_cma is reworked to support dmabuf? Do we know the status of 
> that work?

Dave, do you have any update on this ?

> In the mean time should we add a warning/todo here so we know 
> whats going on if we ever troubleshoot an issue to this location?

A warning would be a bit too verbose I think. We can add a \todo
comment. Would you like to submit a patch ? You can use master as a
base, and I'll handle the rebase.

> This have however nothing to do with this patch,
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> 
> >  	}
> >  
> >  	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