Message ID | 20240717085444.289997-2-mzamazal@redhat.com |
---|---|
State | New |
Headers | show |
Series |
|
Related | show |
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> >>>
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>