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

[Jean-Michel Hautbois]

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

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