[libcamera-devel] [PATCH 03/13] pipeline: raspberrypi: Refactor and move pipeline handler code
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Apr 27 16:24:38 CEST 2023
Hi Naush,
On Thu, Apr 27, 2023 at 03:02:03PM +0100, Naushir Patuck wrote:
> On Thu, 27 Apr 2023 at 14:14, Laurent Pinchart wrote:
> > On Wed, Apr 26, 2023 at 02:10:47PM +0100, Naushir Patuck via libcamera-devel wrote:
> > > Split the Raspberry Pi pipeline handler code into common and VC4/BCM2835
> > > specific file structures.
> > >
> > > The common code files now live in src/libcamera/pipeline/rpi/common/
> > > and the vc4 specific files in src/libcamera/pipeline/rpi/vc4/.
> > >
> > > To build the pipeline handler, the meson configuration option to select
> > > the Raspberry Pi pipeline handler has now changed from "raspberrypi" to
> > > "rpi/vc4":
> > >
> > > meson setup build -Dpipelines=rpi/vc4
> >
> > This breaks the selection of IPA modules in src/ipa/meson.build:
> >
> > foreach pipeline : pipelines
> > if ipa_modules.contains(pipeline)
> > subdir(pipeline)
> > ipa_names += ipa_install_dir / ipa_name + '.so'
> > enabled_ipa_modules += pipeline
> > endif
> > endforeach
>
> Sorry, I forgot to note in my cover letter that patches 2 and 3 want to be
> squashed before merging for this very reason. I've keep them separate for
> easier review.
No worries. I appreciate the patches being separate for review :-)
> > > There are no functional changes in the pipeline handler code itself.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > ---
> > > Documentation/environment_variables.rst | 2 +-
> > > Documentation/guides/introduction.rst | 2 +-
> > > Documentation/guides/ipa.rst | 2 +-
> > > Documentation/guides/pipeline-handler.rst | 2 +-
> > > include/libcamera/ipa/meson.build | 2 +-
> > > meson.build | 2 +-
> > > meson_options.txt | 2 +-
> > > src/libcamera/pipeline/meson.build | 9 +++++++++
> > > .../{raspberrypi => rpi/common}/delayed_controls.cpp | 0
> > > .../{raspberrypi => rpi/common}/delayed_controls.h | 0
> > > src/libcamera/pipeline/rpi/common/meson.build | 8 ++++++++
> > > .../pipeline/{raspberrypi => rpi/common}/rpi_stream.cpp | 0
> > > .../pipeline/{raspberrypi => rpi/common}/rpi_stream.h | 0
> > > .../pipeline/{raspberrypi => rpi/vc4}/data/example.yaml | 0
> > > .../pipeline/{raspberrypi => rpi/vc4}/data/meson.build | 2 +-
> > > .../pipeline/{raspberrypi => rpi/vc4}/dma_heaps.cpp | 0
> > > .../pipeline/{raspberrypi => rpi/vc4}/dma_heaps.h | 0
> > > .../pipeline/{raspberrypi => rpi/vc4}/meson.build | 2 --
> > > .../pipeline/{raspberrypi => rpi/vc4}/raspberrypi.cpp | 2 +-
> > > 19 files changed, 26 insertions(+), 11 deletions(-)
> > > rename src/libcamera/pipeline/{raspberrypi => rpi/common}/delayed_controls.cpp (100%)
> > > rename src/libcamera/pipeline/{raspberrypi => rpi/common}/delayed_controls.h (100%)
> > > create mode 100644 src/libcamera/pipeline/rpi/common/meson.build
> > > rename src/libcamera/pipeline/{raspberrypi => rpi/common}/rpi_stream.cpp (100%)
> > > rename src/libcamera/pipeline/{raspberrypi => rpi/common}/rpi_stream.h (100%)
> > > rename src/libcamera/pipeline/{raspberrypi => rpi/vc4}/data/example.yaml (100%)
> > > rename src/libcamera/pipeline/{raspberrypi => rpi/vc4}/data/meson.build (63%)
> > > rename src/libcamera/pipeline/{raspberrypi => rpi/vc4}/dma_heaps.cpp (100%)
> > > rename src/libcamera/pipeline/{raspberrypi => rpi/vc4}/dma_heaps.h (100%)
> > > rename src/libcamera/pipeline/{raspberrypi => rpi/vc4}/meson.build (71%)
> > > rename src/libcamera/pipeline/{raspberrypi => rpi/vc4}/raspberrypi.cpp (99%)
> > >
> > > diff --git a/Documentation/environment_variables.rst b/Documentation/environment_variables.rst
> > > index ceeb251a2ac0..4bf38b877897 100644
> > > --- a/Documentation/environment_variables.rst
> > > +++ b/Documentation/environment_variables.rst
> > > @@ -40,7 +40,7 @@ LIBCAMERA_IPA_MODULE_PATH
> > > LIBCAMERA_RPI_CONFIG_FILE
> > > Define a custom configuration file to use in the Raspberry Pi pipeline handler.
> > >
> > > - Example value: ``/usr/local/share/libcamera/pipeline/raspberrypi/minimal_mem.yaml``
> > > + Example value: ``/usr/local/share/libcamera/pipeline/rpi/vc4/minimal_mem.yaml``
> > >
> > > Further details
> > > ---------------
> > > diff --git a/Documentation/guides/introduction.rst b/Documentation/guides/introduction.rst
> > > index 2d1760c1866b..700ec2d33c30 100644
> > > --- a/Documentation/guides/introduction.rst
> > > +++ b/Documentation/guides/introduction.rst
> > > @@ -288,7 +288,7 @@ with dedicated pipeline handlers:
> > >
> > > - Intel IPU3 (ipu3)
> > > - Rockchip RK3399 (rkisp1)
> > > - - RaspberryPi 3 and 4 (raspberrypi)
> > > + - RaspberryPi 3 and 4 (rpi/vc4)
> > >
> > > Furthermore, generic platform support is provided for the following:
> > >
> > > diff --git a/Documentation/guides/ipa.rst b/Documentation/guides/ipa.rst
> > > index 89839408672a..10301d89fc80 100644
> > > --- a/Documentation/guides/ipa.rst
> > > +++ b/Documentation/guides/ipa.rst
> > > @@ -279,7 +279,7 @@ For example, adding the raspberrypi.mojom file to meson:
> > > .. code-block:: none
> > >
> > > pipeline_ipa_mojom_mapping = [
> > > - 'raspberrypi': 'raspberrypi.mojom',
> > > + 'rpi/vc4': 'raspberrypi.mojom',
> > > ]
> > >
> > > This will cause the mojo data definition file to be compiled. Specifically, it
> > > diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst
> > > index 4d38fa23fbcd..7d143b0eaecb 100644
> > > --- a/Documentation/guides/pipeline-handler.rst
> > > +++ b/Documentation/guides/pipeline-handler.rst
> > > @@ -183,7 +183,7 @@ to the libcamera build options in the top level ``meson_options.txt``.
> > >
> > > option('pipelines',
> > > type : 'array',
> > > - choices : ['ipu3', 'raspberrypi', 'rkisp1', 'simple', 'uvcvideo', 'vimc', 'vivid'],
> > > + choices : ['ipu3', 'rpi/vc4', 'rkisp1', 'simple', 'uvcvideo', 'vimc', 'vivid'],
> >
> > 'rpi/vc4' should now go after 'rkisp1' in alphabetical order.
> >
> > > description : 'Select which pipeline handlers to include')
> > >
> > >
> > > diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
> > > index 67c31cb04ccf..bae15d64d7d8 100644
> > > --- a/include/libcamera/ipa/meson.build
> > > +++ b/include/libcamera/ipa/meson.build
> > > @@ -64,7 +64,7 @@ libcamera_generated_ipa_headers += custom_target('core_ipa_serializer_h',
> > > pipeline_ipa_mojom_mapping = {
> > > 'ipu3': 'ipu3.mojom',
> > > 'rkisp1': 'rkisp1.mojom',
> > > - 'raspberrypi': 'raspberrypi.mojom',
> > > + 'rpi/vc4': 'raspberrypi.mojom',
> > > 'vimc': 'vimc.mojom',
> > > }
> > >
> > > diff --git a/meson.build b/meson.build
> > > index 8628e6acebee..f60da3e17719 100644
> > > --- a/meson.build
> > > +++ b/meson.build
> > > @@ -179,7 +179,7 @@ arch_x86 = ['x86', 'x86_64']
> > > pipelines_support = {
> > > 'imx8-isi': arch_arm,
> > > 'ipu3': arch_x86,
> > > - 'raspberrypi': arch_arm,
> > > + 'rpi/vc4': arch_arm,
> >
> > This should also go after 'rkisp1'. Same below, and possibly in other
> > patches in this series.
> >
> > > 'rkisp1': arch_arm,
> > > 'simple': arch_arm,
> > > 'uvcvideo': ['any'],
> > > diff --git a/meson_options.txt b/meson_options.txt
> > > index 78a78b7214d5..e1f4c205aa94 100644
> > > --- a/meson_options.txt
> > > +++ b/meson_options.txt
> > > @@ -43,7 +43,7 @@ option('pipelines',
> > > 'auto',
> > > 'imx8-isi',
> > > 'ipu3',
> > > - 'raspberrypi',
> > > + 'rpi/vc4',
> > > 'rkisp1',
> > > 'simple',
> > > 'uvcvideo',
> > > diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build
> > > index f14869f3a3c0..4f55611013db 100644
> > > --- a/src/libcamera/pipeline/meson.build
> > > +++ b/src/libcamera/pipeline/meson.build
> > > @@ -3,6 +3,15 @@
> > > # Location of pipeline specific configuration files
> > > pipeline_data_dir = libcamera_datadir / 'pipeline'
> > >
> > > +# If the Raspberry Pi VC4 pipeline handler is enabled, ensure we include the
> > > +# rpi/common subdirectory in the build.
> > > +#
> > > +# This is done here and not within rpi/vc4/meson.build as meson does not
> > > +# allow the subdir command to traverse up the directory tree.
> > > +if pipelines.contains('rpi/vc4')
> > > + subdir('rpi/common')
> > > +endif
> >
> > I don't like how this becomes a special case :-S If we want to support
> > multi-level directory trees instead of a flat hierarchy, this should be
> > automatic.
> >
> > We could do something like this:
> >
> > ----
> > subdirs = []
> >
> > foreach pipeline : pipelines
> > pipeline = pipeline.split('/')[0]
> > if pipeline in subdirs
> > continue
> > endif
> >
> > subdir(pipeline)
> > subdirs += [pipeline]
> > endforeach
> > ----
> >
> > The logic in src/libcamera/pipeline/rpi/meson.build would be similar,
> > iterating over all pipelines and selecting the ones that start with
> > 'rpi' only. Something like the following should hopefully work:
> >
> > ----
> > # SPDX-License-Identifier: CC0-1.0
> >
> > subdir('common')
> >
> > foreach pipeline : pipelines
> > pipeline = pipeline.split('/')
> > if pipeline.length() < 2 or pipeline[0] != 'rpi'
> > continue
> > endif
> >
> > subdir(pipeline[1])
> > endforeach
> > ----
>
> This should be fine for the pipeline handler case, but the IPA case is a bit
> more awkward to do in the IPA case as it appends ipa_name and enabled_ipa_modules
> in the outer loop. Can you suggest a neat way to fix that?
I'll have a look once I finish reviewing the series. It may need to wait
until Monday I'm afraid.
> > > +
> > > foreach pipeline : pipelines
> > > subdir(pipeline)
> > > endforeach
> > > diff --git a/src/libcamera/pipeline/raspberrypi/delayed_controls.cpp b/src/libcamera/pipeline/rpi/common/delayed_controls.cpp
> > > similarity index 100%
> > > rename from src/libcamera/pipeline/raspberrypi/delayed_controls.cpp
> > > rename to src/libcamera/pipeline/rpi/common/delayed_controls.cpp
> > > diff --git a/src/libcamera/pipeline/raspberrypi/delayed_controls.h b/src/libcamera/pipeline/rpi/common/delayed_controls.h
> > > similarity index 100%
> > > rename from src/libcamera/pipeline/raspberrypi/delayed_controls.h
> > > rename to src/libcamera/pipeline/rpi/common/delayed_controls.h
> > > diff --git a/src/libcamera/pipeline/rpi/common/meson.build b/src/libcamera/pipeline/rpi/common/meson.build
> > > new file mode 100644
> > > index 000000000000..1dec6d3d028b
> > > --- /dev/null
> > > +++ b/src/libcamera/pipeline/rpi/common/meson.build
> > > @@ -0,0 +1,8 @@
> > > +# SPDX-License-Identifier: CC0-1.0
> > > +
> > > +libcamera_sources += files([
> > > + 'delayed_controls.cpp',
> > > + 'rpi_stream.cpp',
> > > +])
> > > +
> > > +includes += include_directories('.')
> > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp
> > > similarity index 100%
> > > rename from src/libcamera/pipeline/raspberrypi/rpi_stream.cpp
> > > rename to src/libcamera/pipeline/rpi/common/rpi_stream.cpp
> > > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h
> > > similarity index 100%
> > > rename from src/libcamera/pipeline/raspberrypi/rpi_stream.h
> > > rename to src/libcamera/pipeline/rpi/common/rpi_stream.h
> > > diff --git a/src/libcamera/pipeline/raspberrypi/data/example.yaml b/src/libcamera/pipeline/rpi/vc4/data/example.yaml
> > > similarity index 100%
> > > rename from src/libcamera/pipeline/raspberrypi/data/example.yaml
> > > rename to src/libcamera/pipeline/rpi/vc4/data/example.yaml
> > > diff --git a/src/libcamera/pipeline/raspberrypi/data/meson.build b/src/libcamera/pipeline/rpi/vc4/data/meson.build
> > > similarity index 63%
> > > rename from src/libcamera/pipeline/raspberrypi/data/meson.build
> > > rename to src/libcamera/pipeline/rpi/vc4/data/meson.build
> > > index 1c70433bbcbc..a7dfa02320e5 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/data/meson.build
> > > +++ b/src/libcamera/pipeline/rpi/vc4/data/meson.build
> > > @@ -5,4 +5,4 @@ conf_files = files([
> > > ])
> > >
> > > install_data(conf_files,
> > > - install_dir : pipeline_data_dir / 'raspberrypi')
> > > + install_dir : pipeline_data_dir / 'rpi/vc4')
> >
> > install_dir : pipeline_data_dir / 'rpi' / 'vc4')
> >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.cpp b/src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp
> > > similarity index 100%
> > > rename from src/libcamera/pipeline/raspberrypi/dma_heaps.cpp
> > > rename to src/libcamera/pipeline/rpi/vc4/dma_heaps.cpp
> > > diff --git a/src/libcamera/pipeline/raspberrypi/dma_heaps.h b/src/libcamera/pipeline/rpi/vc4/dma_heaps.h
> > > similarity index 100%
> > > rename from src/libcamera/pipeline/raspberrypi/dma_heaps.h
> > > rename to src/libcamera/pipeline/rpi/vc4/dma_heaps.h
> > > diff --git a/src/libcamera/pipeline/raspberrypi/meson.build b/src/libcamera/pipeline/rpi/vc4/meson.build
> > > similarity index 71%
> > > rename from src/libcamera/pipeline/raspberrypi/meson.build
> > > rename to src/libcamera/pipeline/rpi/vc4/meson.build
> > > index 59e8fb18c555..228823f30922 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/meson.build
> > > +++ b/src/libcamera/pipeline/rpi/vc4/meson.build
> > > @@ -1,10 +1,8 @@
> > > # SPDX-License-Identifier: CC0-1.0
> > >
> > > libcamera_sources += files([
> > > - 'delayed_controls.cpp',
> > > 'dma_heaps.cpp',
> >
> > I wondered if this could be a good candidate for the common directory,
> > but I think we should turn it into a core libcamera helper if it gets
> > shared by multiple pipeline handlers, so I'm happy to leave it here.
> >
> > > 'raspberrypi.cpp',
> > > - 'rpi_stream.cpp',
> > > ])
> > >
> > > subdir('data')
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp
> > > similarity index 99%
> > > rename from src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > rename to src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp
> > > index a4fff28bf198..4595773d2517 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/rpi/vc4/raspberrypi.cpp
> > > @@ -2,7 +2,7 @@
> > > /*
> > > * Copyright (C) 2019-2021, Raspberry Pi Ltd
> > > *
> > > - * raspberrypi.cpp - Pipeline handler for Raspberry Pi devices
> > > + * raspberrypi.cpp - Pipeline handler for VC4 based Raspberry Pi devices
> >
> > s/VC4 based/VC4-based/
> >
> > > */
> > > #include <algorithm>
> > > #include <assert.h>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list