| Message ID | 20231006132000.23504-14-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush On Fri, Oct 06, 2023 at 02:19:53PM +0100, Naushir Patuck via libcamera-devel wrote: > In the existing meson scripts, an IPA mojom interface file may not be > built if: > > - There are duplicate entries for the mojom file shared by different > pipeline handlers in pipeline_ipa_mojom_mapping, and > - The IPA is not listed first in pipeline_ipa_mojom_mapping, and > - The first listed IPA for the given mojom file is not selected in the > build. So this basically means that building with -Dpipelines=rpi/vc4 -Dipas=rpi/vc4 breaks ../src/libcamera/pipeline/rpi/vc4/../common/pipeline_base.h:30:10: fatal error: libcamera/ipa/raspberrypi_ipa_interface.h: No such file or directory 30 | #include <libcamera/ipa/raspberrypi_ipa_interface.h> | ^~~~~~~~ > > Fix this by using a separate list of already built mojom files > (mojoms_built) instead of overloading use of the existing > ipa_mojom_files list. Now, ipa_mojom_files gets filled in outside of > the IPA list enumeration loop. > > Fixes: 312e9910ba2e ("meson: ipa: Add mapping for pipeline handler to mojom interface file") > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> I recall we have discussed in the past why --- a/include/libcamera/ipa/meson.build +++ b/include/libcamera/ipa/meson.build @@ -86,12 +86,12 @@ foreach pipeline, file : pipeline_ipa_mojom_mapping continue endif - ipa_mojom_files += file - if pipeline not in pipelines continue endif + ipa_mojom_files += file + # {interface}.mojom-module mojom = custom_target(name + '_mojom_module', input : file, was not enough, but I can't find traces of that conversation anymore. With the above snippet applied only I've been able to build with -Dpipelines=rpi/pips, -Dipas=rpi/pisp -Dpipelines=rpi/vc4, -Dipas=rpi/vc4 -Dpipelines=rpi/pips,rpi/vc4, -Dipas=rpi/pisp,rpi/vc4 What am I missing ? > --- > include/libcamera/ipa/meson.build | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build > index e72803b4e243..f3b4881c9c91 100644 > --- a/include/libcamera/ipa/meson.build > +++ b/include/libcamera/ipa/meson.build > @@ -68,29 +68,28 @@ pipeline_ipa_mojom_mapping = { > 'vimc': 'vimc.mojom', > } > > -ipa_mojom_files = [] > -ipa_mojoms = [] > - > # > # Generate headers from templates. > # > > # TODO Define per-pipeline ControlInfoMap with yaml? > > +ipa_mojoms = [] > +mojoms_built = [] > foreach pipeline, file : pipeline_ipa_mojom_mapping > name = file.split('.')[0] > > - # Ensure we do not build duplicate mojom modules > - if file in ipa_mojom_files > + # Avoid building duplicate mojom interfaces with the same interface file > + if name in mojoms_built > continue > endif > > - ipa_mojom_files += file > - > if pipeline not in pipelines > continue > endif > > + mojoms_built += name > + > # {interface}.mojom-module > mojom = custom_target(name + '_mojom_module', > input : file, > @@ -155,6 +154,12 @@ foreach pipeline, file : pipeline_ipa_mojom_mapping > libcamera_generated_ipa_headers += [header, serializer, proxy_header] > endforeach > > +ipa_mojom_files = [] > +foreach pipeline, file : pipeline_ipa_mojom_mapping > + if file not in ipa_mojom_files > + ipa_mojom_files += file > + endif > +endforeach > ipa_mojom_files = files(ipa_mojom_files) > > # Pass this to the documentation generator in src/libcamera/ipa > -- > 2.34.1 >
Hi Jacopo, On Thu, 12 Oct 2023 at 11:03, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Naush > > On Fri, Oct 06, 2023 at 02:19:53PM +0100, Naushir Patuck via libcamera-devel wrote: > > In the existing meson scripts, an IPA mojom interface file may not be > > built if: > > > > - There are duplicate entries for the mojom file shared by different > > pipeline handlers in pipeline_ipa_mojom_mapping, and > > - The IPA is not listed first in pipeline_ipa_mojom_mapping, and > > - The first listed IPA for the given mojom file is not selected in the > > build. > > So this basically means that building with > -Dpipelines=rpi/vc4 -Dipas=rpi/vc4 > breaks > > ../src/libcamera/pipeline/rpi/vc4/../common/pipeline_base.h:30:10: fatal error: libcamera/ipa/raspberrypi_ipa_interface.h: No such file or directory > 30 | #include <libcamera/ipa/raspberrypi_ipa_interface.h> > | ^~~~~~~~ > > > > Fix this by using a separate list of already built mojom files > > (mojoms_built) instead of overloading use of the existing > > ipa_mojom_files list. Now, ipa_mojom_files gets filled in outside of > > the IPA list enumeration loop. > > > > Fixes: 312e9910ba2e ("meson: ipa: Add mapping for pipeline handler to mojom interface file") > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > I recall we have discussed in the past why > > --- a/include/libcamera/ipa/meson.build > +++ b/include/libcamera/ipa/meson.build > @@ -86,12 +86,12 @@ foreach pipeline, file : pipeline_ipa_mojom_mapping > continue > endif > > - ipa_mojom_files += file > - > if pipeline not in pipelines > continue > endif > > + ipa_mojom_files += file > + > # {interface}.mojom-module > mojom = custom_target(name + '_mojom_module', > input : file, > > was not enough, but I can't find traces of that conversation anymore. > > With the above snippet applied only I've been able to build with > > -Dpipelines=rpi/pips, -Dipas=rpi/pisp > -Dpipelines=rpi/vc4, -Dipas=rpi/vc4 > -Dpipelines=rpi/pips,rpi/vc4, -Dipas=rpi/pisp,rpi/vc4 > > What am I missing ? The above snippet will build the files correctly, but crucially it will fail to build the interface documentation files for ipas that are not enabled. I think the following files must unconditionally be generated for documentation purposes: build/src/libcamera/ipa/ipu3_ipa_interface.cpp build/src/libcamera/ipa/raspberrypi_ipa_interface.cpp build/src/libcamera/ipa/rkisp1_ipa_interface.cpp build/src/libcamera/ipa/core_ipa_interface.cpp build/src/libcamera/ipa/vimc_ipa_interface.cpp Regards, Naush > > > --- > > include/libcamera/ipa/meson.build | 19 ++++++++++++------- > > 1 file changed, 12 insertions(+), 7 deletions(-) > > > > diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build > > index e72803b4e243..f3b4881c9c91 100644 > > --- a/include/libcamera/ipa/meson.build > > +++ b/include/libcamera/ipa/meson.build > > @@ -68,29 +68,28 @@ pipeline_ipa_mojom_mapping = { > > 'vimc': 'vimc.mojom', > > } > > > > -ipa_mojom_files = [] > > -ipa_mojoms = [] > > - > > # > > # Generate headers from templates. > > # > > > > # TODO Define per-pipeline ControlInfoMap with yaml? > > > > +ipa_mojoms = [] > > +mojoms_built = [] > > foreach pipeline, file : pipeline_ipa_mojom_mapping > > name = file.split('.')[0] > > > > - # Ensure we do not build duplicate mojom modules > > - if file in ipa_mojom_files > > + # Avoid building duplicate mojom interfaces with the same interface file > > + if name in mojoms_built > > continue > > endif > > > > - ipa_mojom_files += file > > - > > if pipeline not in pipelines > > continue > > endif > > > > + mojoms_built += name > > + > > # {interface}.mojom-module > > mojom = custom_target(name + '_mojom_module', > > input : file, > > @@ -155,6 +154,12 @@ foreach pipeline, file : pipeline_ipa_mojom_mapping > > libcamera_generated_ipa_headers += [header, serializer, proxy_header] > > endforeach > > > > +ipa_mojom_files = [] > > +foreach pipeline, file : pipeline_ipa_mojom_mapping > > + if file not in ipa_mojom_files > > + ipa_mojom_files += file > > + endif > > +endforeach > > ipa_mojom_files = files(ipa_mojom_files) > > > > # Pass this to the documentation generator in src/libcamera/ipa > > -- > > 2.34.1 > >
Hi Naush On Thu, Oct 12, 2023 at 11:46:04AM +0100, Naushir Patuck wrote: > Hi Jacopo, > > On Thu, 12 Oct 2023 at 11:03, Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi Naush > > > > On Fri, Oct 06, 2023 at 02:19:53PM +0100, Naushir Patuck via libcamera-devel wrote: > > > In the existing meson scripts, an IPA mojom interface file may not be > > > built if: > > > > > > - There are duplicate entries for the mojom file shared by different > > > pipeline handlers in pipeline_ipa_mojom_mapping, and > > > - The IPA is not listed first in pipeline_ipa_mojom_mapping, and > > > - The first listed IPA for the given mojom file is not selected in the > > > build. > > > > So this basically means that building with > > -Dpipelines=rpi/vc4 -Dipas=rpi/vc4 > > breaks > > > > ../src/libcamera/pipeline/rpi/vc4/../common/pipeline_base.h:30:10: fatal error: libcamera/ipa/raspberrypi_ipa_interface.h: No such file or directory > > 30 | #include <libcamera/ipa/raspberrypi_ipa_interface.h> > > | ^~~~~~~~ > > > > > > Fix this by using a separate list of already built mojom files > > > (mojoms_built) instead of overloading use of the existing > > > ipa_mojom_files list. Now, ipa_mojom_files gets filled in outside of > > > the IPA list enumeration loop. > > > > > > Fixes: 312e9910ba2e ("meson: ipa: Add mapping for pipeline handler to mojom interface file") > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > I recall we have discussed in the past why > > > > --- a/include/libcamera/ipa/meson.build > > +++ b/include/libcamera/ipa/meson.build > > @@ -86,12 +86,12 @@ foreach pipeline, file : pipeline_ipa_mojom_mapping > > continue > > endif > > > > - ipa_mojom_files += file > > - > > if pipeline not in pipelines > > continue > > endif > > > > + ipa_mojom_files += file > > + > > # {interface}.mojom-module > > mojom = custom_target(name + '_mojom_module', > > input : file, > > > > was not enough, but I can't find traces of that conversation anymore. > > > > With the above snippet applied only I've been able to build with > > > > -Dpipelines=rpi/pips, -Dipas=rpi/pisp > > -Dpipelines=rpi/vc4, -Dipas=rpi/vc4 > > -Dpipelines=rpi/pips,rpi/vc4, -Dipas=rpi/pisp,rpi/vc4 > > > > What am I missing ? > > The above snippet will build the files correctly, but crucially it > will fail to build the interface documentation files for ipas that are > not enabled. I think the following files must unconditionally be > generated for documentation purposes: > > build/src/libcamera/ipa/ipu3_ipa_interface.cpp > build/src/libcamera/ipa/raspberrypi_ipa_interface.cpp > build/src/libcamera/ipa/rkisp1_ipa_interface.cpp > build/src/libcamera/ipa/core_ipa_interface.cpp > build/src/libcamera/ipa/vimc_ipa_interface.cpp > Oh! right! Can I add a line to the commit message to record that ? Fix this by using a separate list of already built mojom files (mojoms_built) instead of overloading use of the existing ipa_mojom_files list. Now, ipa_mojom_files gets filled in outside of the IPA list enumeration loop, this also guarantees the IPA documentation gets built even if the pipeline is not selected. > Regards, > Naush > > > > > > --- > > > include/libcamera/ipa/meson.build | 19 ++++++++++++------- > > > 1 file changed, 12 insertions(+), 7 deletions(-) > > > > > > diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build > > > index e72803b4e243..f3b4881c9c91 100644 > > > --- a/include/libcamera/ipa/meson.build > > > +++ b/include/libcamera/ipa/meson.build > > > @@ -68,29 +68,28 @@ pipeline_ipa_mojom_mapping = { > > > 'vimc': 'vimc.mojom', > > > } > > > > > > -ipa_mojom_files = [] > > > -ipa_mojoms = [] > > > - > > > # > > > # Generate headers from templates. > > > # > > > > > > # TODO Define per-pipeline ControlInfoMap with yaml? > > > > > > +ipa_mojoms = [] > > > +mojoms_built = [] > > > foreach pipeline, file : pipeline_ipa_mojom_mapping > > > name = file.split('.')[0] > > > > > > - # Ensure we do not build duplicate mojom modules > > > - if file in ipa_mojom_files > > > + # Avoid building duplicate mojom interfaces with the same interface file > > > + if name in mojoms_built > > > continue > > > endif > > > > > > - ipa_mojom_files += file > > > - > > > if pipeline not in pipelines > > > continue > > > endif > > > > > > + mojoms_built += name > > > + > > > # {interface}.mojom-module > > > mojom = custom_target(name + '_mojom_module', > > > input : file, > > > @@ -155,6 +154,12 @@ foreach pipeline, file : pipeline_ipa_mojom_mapping > > > libcamera_generated_ipa_headers += [header, serializer, proxy_header] > > > endforeach > > > > > > +ipa_mojom_files = [] > > > +foreach pipeline, file : pipeline_ipa_mojom_mapping > > > + if file not in ipa_mojom_files > > > + ipa_mojom_files += file > > > + endif > > > +endforeach > > > ipa_mojom_files = files(ipa_mojom_files) > > > > > > # Pass this to the documentation generator in src/libcamera/ipa > > > -- > > > 2.34.1 > > >
On Thu, 12 Oct 2023 at 12:31, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Naush > > On Thu, Oct 12, 2023 at 11:46:04AM +0100, Naushir Patuck wrote: > > Hi Jacopo, > > > > On Thu, 12 Oct 2023 at 11:03, Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Hi Naush > > > > > > On Fri, Oct 06, 2023 at 02:19:53PM +0100, Naushir Patuck via libcamera-devel wrote: > > > > In the existing meson scripts, an IPA mojom interface file may not be > > > > built if: > > > > > > > > - There are duplicate entries for the mojom file shared by different > > > > pipeline handlers in pipeline_ipa_mojom_mapping, and > > > > - The IPA is not listed first in pipeline_ipa_mojom_mapping, and > > > > - The first listed IPA for the given mojom file is not selected in the > > > > build. > > > > > > So this basically means that building with > > > -Dpipelines=rpi/vc4 -Dipas=rpi/vc4 > > > breaks > > > > > > ../src/libcamera/pipeline/rpi/vc4/../common/pipeline_base.h:30:10: fatal error: libcamera/ipa/raspberrypi_ipa_interface.h: No such file or directory > > > 30 | #include <libcamera/ipa/raspberrypi_ipa_interface.h> > > > | ^~~~~~~~ > > > > > > > > Fix this by using a separate list of already built mojom files > > > > (mojoms_built) instead of overloading use of the existing > > > > ipa_mojom_files list. Now, ipa_mojom_files gets filled in outside of > > > > the IPA list enumeration loop. > > > > > > > > Fixes: 312e9910ba2e ("meson: ipa: Add mapping for pipeline handler to mojom interface file") > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > > > I recall we have discussed in the past why > > > > > > --- a/include/libcamera/ipa/meson.build > > > +++ b/include/libcamera/ipa/meson.build > > > @@ -86,12 +86,12 @@ foreach pipeline, file : pipeline_ipa_mojom_mapping > > > continue > > > endif > > > > > > - ipa_mojom_files += file > > > - > > > if pipeline not in pipelines > > > continue > > > endif > > > > > > + ipa_mojom_files += file > > > + > > > # {interface}.mojom-module > > > mojom = custom_target(name + '_mojom_module', > > > input : file, > > > > > > was not enough, but I can't find traces of that conversation anymore. > > > > > > With the above snippet applied only I've been able to build with > > > > > > -Dpipelines=rpi/pips, -Dipas=rpi/pisp > > > -Dpipelines=rpi/vc4, -Dipas=rpi/vc4 > > > -Dpipelines=rpi/pips,rpi/vc4, -Dipas=rpi/pisp,rpi/vc4 > > > > > > What am I missing ? > > > > The above snippet will build the files correctly, but crucially it > > will fail to build the interface documentation files for ipas that are > > not enabled. I think the following files must unconditionally be > > generated for documentation purposes: > > > > build/src/libcamera/ipa/ipu3_ipa_interface.cpp > > build/src/libcamera/ipa/raspberrypi_ipa_interface.cpp > > build/src/libcamera/ipa/rkisp1_ipa_interface.cpp > > build/src/libcamera/ipa/core_ipa_interface.cpp > > build/src/libcamera/ipa/vimc_ipa_interface.cpp > > > > Oh! right! > > Can I add a line to the commit message to record that ? > > Fix this by using a separate list of already built mojom files > (mojoms_built) instead of overloading use of the existing > ipa_mojom_files list. Now, ipa_mojom_files gets filled in outside of > the IPA list enumeration loop, this also guarantees the IPA > documentation gets built even if the pipeline is not selected. Sounds good to me! > > > > > Regards, > > Naush > > > > > > > > > --- > > > > include/libcamera/ipa/meson.build | 19 ++++++++++++------- > > > > 1 file changed, 12 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build > > > > index e72803b4e243..f3b4881c9c91 100644 > > > > --- a/include/libcamera/ipa/meson.build > > > > +++ b/include/libcamera/ipa/meson.build > > > > @@ -68,29 +68,28 @@ pipeline_ipa_mojom_mapping = { > > > > 'vimc': 'vimc.mojom', > > > > } > > > > > > > > -ipa_mojom_files = [] > > > > -ipa_mojoms = [] > > > > - > > > > # > > > > # Generate headers from templates. > > > > # > > > > > > > > # TODO Define per-pipeline ControlInfoMap with yaml? > > > > > > > > +ipa_mojoms = [] > > > > +mojoms_built = [] > > > > foreach pipeline, file : pipeline_ipa_mojom_mapping > > > > name = file.split('.')[0] > > > > > > > > - # Ensure we do not build duplicate mojom modules > > > > - if file in ipa_mojom_files > > > > + # Avoid building duplicate mojom interfaces with the same interface file > > > > + if name in mojoms_built > > > > continue > > > > endif > > > > > > > > - ipa_mojom_files += file > > > > - > > > > if pipeline not in pipelines > > > > continue > > > > endif > > > > > > > > + mojoms_built += name > > > > + > > > > # {interface}.mojom-module > > > > mojom = custom_target(name + '_mojom_module', > > > > input : file, > > > > @@ -155,6 +154,12 @@ foreach pipeline, file : pipeline_ipa_mojom_mapping > > > > libcamera_generated_ipa_headers += [header, serializer, proxy_header] > > > > endforeach > > > > > > > > +ipa_mojom_files = [] > > > > +foreach pipeline, file : pipeline_ipa_mojom_mapping > > > > + if file not in ipa_mojom_files > > > > + ipa_mojom_files += file > > > > + endif > > > > +endforeach > > > > ipa_mojom_files = files(ipa_mojom_files) > > > > > > > > # Pass this to the documentation generator in src/libcamera/ipa > > > > -- > > > > 2.34.1 > > > >
diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build index e72803b4e243..f3b4881c9c91 100644 --- a/include/libcamera/ipa/meson.build +++ b/include/libcamera/ipa/meson.build @@ -68,29 +68,28 @@ pipeline_ipa_mojom_mapping = { 'vimc': 'vimc.mojom', } -ipa_mojom_files = [] -ipa_mojoms = [] - # # Generate headers from templates. # # TODO Define per-pipeline ControlInfoMap with yaml? +ipa_mojoms = [] +mojoms_built = [] foreach pipeline, file : pipeline_ipa_mojom_mapping name = file.split('.')[0] - # Ensure we do not build duplicate mojom modules - if file in ipa_mojom_files + # Avoid building duplicate mojom interfaces with the same interface file + if name in mojoms_built continue endif - ipa_mojom_files += file - if pipeline not in pipelines continue endif + mojoms_built += name + # {interface}.mojom-module mojom = custom_target(name + '_mojom_module', input : file, @@ -155,6 +154,12 @@ foreach pipeline, file : pipeline_ipa_mojom_mapping libcamera_generated_ipa_headers += [header, serializer, proxy_header] endforeach +ipa_mojom_files = [] +foreach pipeline, file : pipeline_ipa_mojom_mapping + if file not in ipa_mojom_files + ipa_mojom_files += file + endif +endforeach ipa_mojom_files = files(ipa_mojom_files) # Pass this to the documentation generator in src/libcamera/ipa
In the existing meson scripts, an IPA mojom interface file may not be built if: - There are duplicate entries for the mojom file shared by different pipeline handlers in pipeline_ipa_mojom_mapping, and - The IPA is not listed first in pipeline_ipa_mojom_mapping, and - The first listed IPA for the given mojom file is not selected in the build. Fix this by using a separate list of already built mojom files (mojoms_built) instead of overloading use of the existing ipa_mojom_files list. Now, ipa_mojom_files gets filled in outside of the IPA list enumeration loop. Fixes: 312e9910ba2e ("meson: ipa: Add mapping for pipeline handler to mojom interface file") Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Reviewed-by: David Plowman <david.plowman@raspberrypi.com> --- include/libcamera/ipa/meson.build | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)