[v3,01/23] libcamera: rkisp1: Drop base IPA headers inclusion

20663 diff mbox series

From: Umang Jain <umang.jain@ideasonboard.com>

The IPA headers ipa_interface.h and core_ipa_interface.h are
included as part of the rkisp1_ipa_interface.h generated from
module_ipa_interface.h.tmpl. Drop them as deemed redundant.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 src/ipa/rkisp1/rkisp1.cpp                | 1 -
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 1 -
 2 files changed, 2 deletions(-)

On Wed, Jul 17, 2024 at 10:54:22AM +0200, Milan Zamazal wrote:
> From: Umang Jain <umang.jain@ideasonboard.com>
> 
> The IPA headers ipa_interface.h and core_ipa_interface.h are
> included as part of the rkisp1_ipa_interface.h generated from
> module_ipa_interface.h.tmpl. Drop them as deemed redundant.

libcamera/ipa/ipa_interface.h defines the interface exposed by the IPA
module binary. That's the top-level ipaCreate() function, and the base
IPAInterface class.

The file is included in rkisp1_ipa_interface.h for the definition of the
IPAInterface class, as IPARkISP1Interface derives from it. rkisp1.cpp
doesn't make use of the base IPAInterface class, so there's no need to
include ipa_interface.h for that.

The ipaCreate() function declaration, however, is needed by rkisp1.cpp.
As it's declared by ipa_interface.h, we include the header in the
top-level .cpp file of each IPA module, and I think it should stay
there, following the "include what you use" principle, to avoid
depending on indirect includes.

Can you drop the first hunk of this patch and update the commit message
?

> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  src/ipa/rkisp1/rkisp1.cpp                | 1 -
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 23e0826c..c5c85c8d 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -19,7 +19,6 @@
>  
>  #include <libcamera/control_ids.h>
>  #include <libcamera/framebuffer.h>
> -#include <libcamera/ipa/ipa_interface.h>
>  #include <libcamera/ipa/ipa_module_info.h>
>  #include <libcamera/ipa/rkisp1_ipa_interface.h>
>  #include <libcamera/request.h>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 4cbf105d..97cd78a7 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -27,7 +27,6 @@
>  #include <libcamera/stream.h>
>  #include <libcamera/transform.h>
>  
> -#include <libcamera/ipa/core_ipa_interface.h>
>  #include <libcamera/ipa/rkisp1_ipa_interface.h>
>  #include <libcamera/ipa/rkisp1_ipa_proxy.h>
>

Hi Laurent,

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> On Wed, Jul 17, 2024 at 10:54:22AM +0200, Milan Zamazal wrote:
>> From: Umang Jain <umang.jain@ideasonboard.com>
>> 
>
>> The IPA headers ipa_interface.h and core_ipa_interface.h are
>> included as part of the rkisp1_ipa_interface.h generated from
>> module_ipa_interface.h.tmpl. Drop them as deemed redundant.
>
> libcamera/ipa/ipa_interface.h defines the interface exposed by the IPA
> module binary. That's the top-level ipaCreate() function, and the base
> IPAInterface class.
>
> The file is included in rkisp1_ipa_interface.h for the definition of the
> IPAInterface class, as IPARkISP1Interface derives from it. rkisp1.cpp
> doesn't make use of the base IPAInterface class, so there's no need to
> include ipa_interface.h for that.
>
> The ipaCreate() function declaration, however, is needed by rkisp1.cpp.
> As it's declared by ipa_interface.h, we include the header in the
> top-level .cpp file of each IPA module, and I think it should stay
> there, following the "include what you use" principle, to avoid
> depending on indirect includes.
>
> Can you drop the first hunk of this patch and update the commit message
> ?

Oops, the first four patches by Umang were included by mistake, due to a
wrong rebase, sorry about it.

Anyway, maybe Umang would like to fix and resubmit this patch as
explained above?  (If not, I can handle it as a penance for my
mistake :-), separately from this series.)

>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>  src/ipa/rkisp1/rkisp1.cpp                | 1 -
>>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 1 -
>>  2 files changed, 2 deletions(-)
>> 
>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>> index 23e0826c..c5c85c8d 100644
>> --- a/src/ipa/rkisp1/rkisp1.cpp
>> +++ b/src/ipa/rkisp1/rkisp1.cpp
>> @@ -19,7 +19,6 @@
>>  
>>  #include <libcamera/control_ids.h>
>>  #include <libcamera/framebuffer.h>
>> -#include <libcamera/ipa/ipa_interface.h>
>>  #include <libcamera/ipa/ipa_module_info.h>
>>  #include <libcamera/ipa/rkisp1_ipa_interface.h>
>>  #include <libcamera/request.h>
>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> index 4cbf105d..97cd78a7 100644
>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> @@ -27,7 +27,6 @@
>>  #include <libcamera/stream.h>
>>  #include <libcamera/transform.h>
>>  
>> -#include <libcamera/ipa/core_ipa_interface.h>
>>  #include <libcamera/ipa/rkisp1_ipa_interface.h>
>>  #include <libcamera/ipa/rkisp1_ipa_proxy.h>
>>

Milan Zamazal <mzamazal@redhat.com> writes:

> Hi Laurent,
>
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
>
>> On Wed, Jul 17, 2024 at 10:54:22AM +0200, Milan Zamazal wrote:
>>> From: Umang Jain <umang.jain@ideasonboard.com>
>>> 
>>
>>> The IPA headers ipa_interface.h and core_ipa_interface.h are
>>> included as part of the rkisp1_ipa_interface.h generated from
>>> module_ipa_interface.h.tmpl. Drop them as deemed redundant.
>>
>> libcamera/ipa/ipa_interface.h defines the interface exposed by the IPA
>> module binary. That's the top-level ipaCreate() function, and the base
>> IPAInterface class.
>>
>> The file is included in rkisp1_ipa_interface.h for the definition of the
>> IPAInterface class, as IPARkISP1Interface derives from it. rkisp1.cpp
>> doesn't make use of the base IPAInterface class, so there's no need to
>> include ipa_interface.h for that.
>>
>> The ipaCreate() function declaration, however, is needed by rkisp1.cpp.
>> As it's declared by ipa_interface.h, we include the header in the
>> top-level .cpp file of each IPA module, and I think it should stay
>> there, following the "include what you use" principle, to avoid
>> depending on indirect includes.
>>
>> Can you drop the first hunk of this patch and update the commit message
>> ?
>
> Oops, the first four patches by Umang were included by mistake, due to a
> wrong rebase, sorry about it.
>
> Anyway, maybe Umang would like to fix and resubmit this patch as
> explained above?  (If not, I can handle it as a penance for my
> mistake :-), separately from this series.)

After closer look, I can see the other include is also needed, so the
patch can be dropped completely.

FWIW LSP does a good job to indicate really unused headers.  I'm trying
to clean up the includes based on that.  There may be some pitfalls but
I suppose it's worth to attempt.

>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>>> ---
>>>  src/ipa/rkisp1/rkisp1.cpp                | 1 -
>>>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 1 -
>>>  2 files changed, 2 deletions(-)
>>> 
>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>>> index 23e0826c..c5c85c8d 100644
>>> --- a/src/ipa/rkisp1/rkisp1.cpp
>>> +++ b/src/ipa/rkisp1/rkisp1.cpp
>>> @@ -19,7 +19,6 @@
>>>  
>>>  #include <libcamera/control_ids.h>
>>>  #include <libcamera/framebuffer.h>
>>> -#include <libcamera/ipa/ipa_interface.h>
>>>  #include <libcamera/ipa/ipa_module_info.h>
>>>  #include <libcamera/ipa/rkisp1_ipa_interface.h>
>>>  #include <libcamera/request.h>
>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>> index 4cbf105d..97cd78a7 100644
>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>> @@ -27,7 +27,6 @@
>>>  #include <libcamera/stream.h>
>>>  #include <libcamera/transform.h>
>>>  
>>> -#include <libcamera/ipa/core_ipa_interface.h>
>>>  #include <libcamera/ipa/rkisp1_ipa_interface.h>
>>>  #include <libcamera/ipa/rkisp1_ipa_proxy.h>
>>>