| Message ID | 20240701075720.46076-2-umang.jain@ideasonboard.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series |
|
| Related | show |
Umang Jain <umang.jain@ideasonboard.com> writes: > 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> Reviewed-by: Milan Zamazal <mzamazal@redhat.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 d31cdbab..a43116d7 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>
Quoting Umang Jain (2024-07-01 08:57:17) > 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. I wonder why these were put in in the first place. Are there any artificats that would mean we should include them becuase of 'include what you use' ? But that said - I really can't see a reason to inlcude the base interface when we include a targetted/specific interface.. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > 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 d31cdbab..a43116d7 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> > > -- > 2.44.0 >
On Mon, Jul 01, 2024 at 10:46:45AM +0100, Kieran Bingham wrote: > Quoting Umang Jain (2024-07-01 08:57:17) > > 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. > > I wonder why these were put in in the first place. Are there any > artificats that would mean we should include them becuase of 'include > what you use' ? But that said - I really can't see a reason to inlcude > the base interface when we include a targetted/specific interface.. 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. > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > 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 d31cdbab..a43116d7 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, On 01/07/24 7:08 pm, Laurent Pinchart wrote: > On Mon, Jul 01, 2024 at 10:46:45AM +0100, Kieran Bingham wrote: >> Quoting Umang Jain (2024-07-01 08:57:17) >>> 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. >> I wonder why these were put in in the first place. Are there any >> artificats that would mean we should include them becuase of 'include >> what you use' ? But that said - I really can't see a reason to inlcude >> the base interface when we include a targetted/specific interface.. > 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. Ah, I should have spotted that. Completely makes sense. Hence, only the hunk of pipeline/rkisp1/rkisp1.cpp makes sense for the series. Rest is not applicable. > >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>> 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 d31cdbab..a43116d7 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> >>>
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index d31cdbab..a43116d7 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>
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(-)