[libcamera-devel] [PATCH 03/13] pipeline: raspberrypi: Refactor and move pipeline handler code

[Laurent Pinchart]

Hi Naush,

Another comment.

On Thu, Apr 27, 2023 at 04:14:47PM +0300, Laurent Pinchart via libcamera-devel 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
> 
> > 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%)

[snip]

> > 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('.')

This isn't nice, especially given that the common directory will contain
a pipeline_base.h, which is a quite generic name. You can drop this and
use relative include paths in the source files.

[snip]

-- 
Regards,

Laurent Pinchart