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

[Sebastian Fricke]

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 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.
>>
>> 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 at lists.libcamera.org
>https://lists.libcamera.org/listinfo/libcamera-devel