[libcamera-devel] [PATCH] ipa: rkisp1: Move the IPA to the ipa::rkisp1 namespace

[Laurent Pinchart]

Hi Jean-Michel,

On Mon, Apr 26, 2021 at 07:49:11AM +0200, Jean-Michel Hautbois wrote:
> On 26/04/2021 00:03, Laurent Pinchart wrote:
> > On Fri, Apr 23, 2021 at 10:02:15AM +0100, Kieran Bingham wrote:
> >> On 23/04/2021 07:39, Jean-Michel Hautbois wrote:
> >>> Simplify name-spacing of the RKISP1 components by placing it in the
> >>> ipa::rkisp1 namespace directly.
> >>
> >> Given that I did the same for the IPU3 - Perhaps I'm biased, but I think
> >> this is better ;-D
> >>
> >>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> >>
> >> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > 
> > We should consider renaming IPARkISP1Interface to Interface though, and
> > IPARkISP1 to... Implementation ?
> 
> Thanks.
> Do you mean having the following ?
> 
> -class IPARkISP1 : public IPARkISP1Interface
> +class Implementation : public Interface
> 
> It would be the same for all IPAs then ?

That's the idea, yes. It's just an idea though :-) It looks a bit...
bare maybe ? But if we keep IPA and RkISP1 in the name of the classes,
it defeats the purpose of namespaces a little bit.

> >>> ---
> >>>  src/ipa/rkisp1/rkisp1.cpp | 28 ++++++++++++++++------------
> >>>  1 file changed, 16 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> >>> index 8a57b080..6d45673c 100644
> >>> --- a/src/ipa/rkisp1/rkisp1.cpp
> >>> +++ b/src/ipa/rkisp1/rkisp1.cpp
> >>> @@ -28,7 +28,9 @@ namespace libcamera {
> >>>  
> >>>  LOG_DEFINE_CATEGORY(IPARkISP1)
> >>>  
> >>> -class IPARkISP1 : public ipa::rkisp1::IPARkISP1Interface
> >>> +namespace ipa::rkisp1 {
> >>> +
> >>> +class IPARkISP1 : public IPARkISP1Interface
> >>>  {
> >>>  public:
> >>>  	int init(unsigned int hwRevision) override;
> >>> @@ -40,7 +42,7 @@ public:
> >>>  		      const std::map<uint32_t, ControlInfoMap> &entityControls) override;
> >>>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> >>>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
> >>> -	void processEvent(const ipa::rkisp1::RkISP1Event &event) override;
> >>> +	void processEvent(const RkISP1Event &event) override;
> >>>  
> >>>  private:
> >>>  	void queueRequest(unsigned int frame, rkisp1_params_cfg *params,
> >>> @@ -171,10 +173,10 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids)
> >>>  	}
> >>>  }
> >>>  
> >>> -void IPARkISP1::processEvent(const ipa::rkisp1::RkISP1Event &event)
> >>> +void IPARkISP1::processEvent(const RkISP1Event &event)
> >>>  {
> >>>  	switch (event.op) {
> >>> -	case ipa::rkisp1::EventSignalStatBuffer: {
> >>> +	case EventSignalStatBuffer: {
> >>>  		unsigned int frame = event.frame;
> >>>  		unsigned int bufferId = event.bufferId;
> >>>  
> >>> @@ -184,7 +186,7 @@ void IPARkISP1::processEvent(const ipa::rkisp1::RkISP1Event &event)
> >>>  		updateStatistics(frame, stats);
> >>>  		break;
> >>>  	}
> >>> -	case ipa::rkisp1::EventQueueRequest: {
> >>> +	case EventQueueRequest: {
> >>>  		unsigned int frame = event.frame;
> >>>  		unsigned int bufferId = event.bufferId;
> >>>  
> >>> @@ -215,8 +217,8 @@ void IPARkISP1::queueRequest(unsigned int frame, rkisp1_params_cfg *params,
> >>>  		params->module_en_update = RKISP1_CIF_ISP_MODULE_AEC;
> >>>  	}
> >>>  
> >>> -	ipa::rkisp1::RkISP1Action op;
> >>> -	op.op = ipa::rkisp1::ActionParamFilled;
> >>> +	RkISP1Action op;
> >>> +	op.op = ActionParamFilled;
> >>>  
> >>>  	queueFrameAction.emit(frame, op);
> >>>  }
> >>> @@ -268,8 +270,8 @@ void IPARkISP1::updateStatistics(unsigned int frame,
> >>>  
> >>>  void IPARkISP1::setControls(unsigned int frame)
> >>>  {
> >>> -	ipa::rkisp1::RkISP1Action op;
> >>> -	op.op = ipa::rkisp1::ActionV4L2Set;
> >>> +	RkISP1Action op;
> >>> +	op.op = ActionV4L2Set;
> >>>  
> >>>  	ControlList ctrls(ctrls_);
> >>>  	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
> >>> @@ -286,13 +288,15 @@ void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)
> >>>  	if (aeState)
> >>>  		ctrls.set(controls::AeLocked, aeState == 2);
> >>>  
> >>> -	ipa::rkisp1::RkISP1Action op;
> >>> -	op.op = ipa::rkisp1::ActionMetadata;
> >>> +	RkISP1Action op;
> >>> +	op.op = ActionMetadata;
> >>>  	op.controls = ctrls;
> >>>  
> >>>  	queueFrameAction.emit(frame, op);
> >>>  }
> >>>  
> >>> +} /* namespace ipa::rkisp1 */
> >>> +
> >>>  /*
> >>>   * External IPA module interface
> >>>   */
> >>> @@ -307,7 +311,7 @@ const struct IPAModuleInfo ipaModuleInfo = {
> >>>  
> >>>  IPAInterface *ipaCreate()
> >>>  {
> >>> -	return new IPARkISP1();
> >>> +	return new ipa::rkisp1::IPARkISP1();
> >>>  }
> >>>  }
> >>>  

-- 
Regards,

Laurent Pinchart