[libcamera-devel] ipa: rkisp1: Move the IPA to the ipa::rkisp1 namespace
diff mbox series

Message ID 20210423063919.26273-1-jeanmichel.hautbois@ideasonboard.com
State Accepted
Commit 8a2fb73337ffe562a25b4dd9cbdf2e78046da9f3
Headers show
Series
  • [libcamera-devel] ipa: rkisp1: Move the IPA to the ipa::rkisp1 namespace
Related show

Commit Message

Jean-Michel Hautbois April 23, 2021, 6:39 a.m. UTC 
Simplify name-spacing of the RKISP1 components by placing it in the
ipa::rkisp1 namespace directly.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/ipa/rkisp1/rkisp1.cpp | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

Comments

Kieran Bingham April 23, 2021, 9:02 a.m. UTC | #1 
Hi JM,

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@ideasonboard.com>

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>



> ---
>  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();
>  }
>  }
>  
>
Jean-Michel Hautbois April 23, 2021, 9:07 a.m. UTC | #2 
Hi Kieran,

On 23/04/2021 11:02, Kieran Bingham wrote:
> Hi JM,
> 
> 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

Not that I copy/pasted your commit ^_^

>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> 
> 
>> ---
>>  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();
>>  }
>>  }
>>  
>>
>
Laurent Pinchart April 25, 2021, 10:03 p.m. UTC | #3 
On Fri, Apr 23, 2021 at 10:02:15AM +0100, Kieran Bingham wrote:
> Hi JM,
> 
> 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@ideasonboard.com>
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

We should consider renaming IPARkISP1Interface to Interface though, and
IPARkISP1 to... Implementation ?

> > ---
> >  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();
> >  }
> >  }
> >
Jean-Michel Hautbois April 26, 2021, 5:49 a.m. UTC | #4 
Hi Laurent,

On 26/04/2021 00:03, Laurent Pinchart wrote:
> On Fri, Apr 23, 2021 at 10:02:15AM +0100, Kieran Bingham wrote:
>> Hi JM,
>>
>> 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@ideasonboard.com>
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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 ?

JM
>>> ---
>>>  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();
>>>  }
>>>  }
>>>  
>
Laurent Pinchart April 26, 2021, 5:57 a.m. UTC | #5 
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@ideasonboard.com>
> >>
> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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();
> >>>  }
> >>>  }
> >>>
Jean-Michel Hautbois April 26, 2021, 6:01 a.m. UTC | #6 
On 26/04/2021 07:57, Laurent Pinchart wrote:
> 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@ideasonboard.com>
>>>>
>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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.
For reading ease couldn't we have:
class RkISP1Implementation: public RkISP1Interface

That would keep the naming without the IPA :-).
It is "just" a mojom patching, right :-p ?

>>>>> ---
>>>>>  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();
>>>>>  }
>>>>>  }
>>>>>  
>
Laurent Pinchart April 26, 2021, 6:42 a.m. UTC | #7 
Hi Jean-Michel,

On Mon, Apr 26, 2021 at 08:01:11AM +0200, Jean-Michel Hautbois wrote:
> On 26/04/2021 07:57, Laurent Pinchart wrote:
> > 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@ideasonboard.com>
> >>>>
> >>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >>>
> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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.
>
> For reading ease couldn't we have:
> class RkISP1Implementation: public RkISP1Interface
> 
> That would keep the naming without the IPA :-).
> It is "just" a mojom patching, right :-p ?

Or

class IPAImplementation : public IPAInterface

? :-)

> >>>>> ---
> >>>>>  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();
> >>>>>  }
> >>>>>  }
> >>>>>
Sebastian Fricke April 26, 2021, 7:08 a.m. UTC | #8 
Hey Laurent and Jean-Michel,

On 26.04.2021 09:42, Laurent Pinchart wrote:
>Hi Jean-Michel,
>
>On Mon, Apr 26, 2021 at 08:01:11AM +0200, Jean-Michel Hautbois wrote:
>> On 26/04/2021 07:57, Laurent Pinchart wrote:
>> > 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@ideasonboard.com>
>> >>>>
>> >>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> >>>
>> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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.
>>
>> For reading ease couldn't we have:
>> class RkISP1Implementation: public RkISP1Interface
>>
>> That would keep the naming without the IPA :-).
>> It is "just" a mojom patching, right :-p ?
>
>Or
>
>class IPAImplementation : public IPAInterface


>
>? :-)

I like that the most as it still highlights that we talk about IPAs,
while being general enough to act as a namespace for different
implementations.

Greetings,
Sebastian

>
>> >>>>> ---
>> >>>>>  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
>_______________________________________________
>libcamera-devel mailing list
>libcamera-devel@lists.libcamera.org
>https://lists.libcamera.org/listinfo/libcamera-devel
Jean-Michel Hautbois April 26, 2021, 8:59 a.m. UTC | #9 
On 26/04/2021 09:08, Sebastian Fricke wrote:
> Hey Laurent and Jean-Michel,
> 
> On 26.04.2021 09:42, Laurent Pinchart wrote:
>> Hi Jean-Michel,
>>
>> On Mon, Apr 26, 2021 at 08:01:11AM +0200, Jean-Michel Hautbois wrote:
>>> On 26/04/2021 07:57, Laurent Pinchart wrote:
>>> > 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@ideasonboard.com>
>>> >>>>
>>> >>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>> >>>
>>> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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.
>>>
>>> For reading ease couldn't we have:
>>> class RkISP1Implementation: public RkISP1Interface
>>>
>>> That would keep the naming without the IPA :-).
>>> It is "just" a mojom patching, right :-p ?
>>
>> Or
>>
>> class IPAImplementation : public IPAInterface

But there is already a base class IPAInterface in
include/libcamera/ipa/ipa_interface.h !
According to me, RkISP1Interface inherits from the IPAInterface right now...

Or did I miss something here... (I probably have)
> 
>>
>> ? :-)
> 
> I like that the most as it still highlights that we talk about IPAs,
> while being general enough to act as a namespace for different
> implementations.
> 
> Greetings,
> Sebastian
> 
>>
>>> >>>>> ---
>>> >>>>>  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
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham April 26, 2021, 9:09 a.m. UTC | #10 
Hi JM,

On 26/04/2021 09:59, Jean-Michel Hautbois wrote:
> 
> 
> On 26/04/2021 09:08, Sebastian Fricke wrote:
>> Hey Laurent and Jean-Michel,
>>
>> On 26.04.2021 09:42, Laurent Pinchart wrote:
>>> Hi Jean-Michel,
>>>
>>> On Mon, Apr 26, 2021 at 08:01:11AM +0200, Jean-Michel Hautbois wrote:
>>>> On 26/04/2021 07:57, Laurent Pinchart wrote:
>>>>> 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@ideasonboard.com>
>>>>>>>>
>>>>>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>>>>>
>>>>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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.
>>>>
>>>> For reading ease couldn't we have:
>>>> class RkISP1Implementation: public RkISP1Interface
>>>>
>>>> That would keep the naming without the IPA :-).
>>>> It is "just" a mojom patching, right :-p ?
>>>
>>> Or
>>>
>>> class IPAImplementation : public IPAInterface
> 
> But there is already a base class IPAInterface in
> include/libcamera/ipa/ipa_interface.h !
> According to me, RkISP1Interface inherits from the IPAInterface right now...
> 
> Or did I miss something here... (I probably have)

Isn't it the namespacing?

I.e.

libcamera::ipa::IPAInterface

is a different class to

libcamera::ipa::rkisp1::IPAInterface


Thus the namespace specifies the ... specification of the interface ...

Regards
--
Kieran

Patch
diff mbox series 

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();
 }
 }