[libcamera-devel] [PATCH 13/20] build: ipa: Fix bug in building multiple IPA interfaces with the same mojom file

[Naushir Patuck]

On Thu, 12 Oct 2023 at 12:31, Jacopo Mondi
<jacopo.mondi at 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 at 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 at raspberrypi.com>
> > > > Reviewed-by: David Plowman <david.plowman at 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
> > > >