[PATCH] libcamera: pipeline: Add Mali-C55 ISP pipeline

[Barnabás Pőcze]

Hi


2024. március 19., kedd 15:43 keltezéssel, Jacopo Mondi <jacopo.mondi at ideasonboard.com> írta:

> Hi Barnabás
> 
> On Fri, Mar 15, 2024 at 02:28:26AM +0000, Barnabás Pőcze wrote:
> > Hi
> >
> >
> > 2024. március 14., csütörtök 15:07 keltezéssel, Jacopo Mondi <jacopo.mondi at ideasonboard.com> írta:
> >
> > > [...]
> > > diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > > new file mode 100644
> > > index 000000000000..4508214b864d
> > > --- /dev/null
> > > +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > > @@ -0,0 +1,1081 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2024, Ideas on Board Oy
> > > + *
> > > + * mali-c55.cpp - Pipeline Handler for ARM's Mali-C55 ISP
> > > + */
> > > +
> > > +#include <algorithm>
> > > +#include <array>
> > > +#include <map>
> > > +#include <memory>
> > > +#include <set>
> > > +#include <string>
> > > +
> > > +#include <linux/media-bus-format.h>
> > > +#include <linux/media.h>
> > > +
> > > +#include <libcamera/base/log.h>
> > > +
> > > +#include <libcamera/camera.h>
> > > +#include <libcamera/formats.h>
> > > +#include <libcamera/geometry.h>
> > > +#include <libcamera/stream.h>
> > > +
> > > +#include "libcamera/internal/bayer_format.h"
> > > +#include "libcamera/internal/camera.h"
> > > +#include "libcamera/internal/camera_sensor.h"
> > > +#include "libcamera/internal/device_enumerator.h"
> > > +#include "libcamera/internal/media_device.h"
> > > +#include "libcamera/internal/pipeline_handler.h"
> > > +#include "libcamera/internal/v4l2_subdevice.h"
> > > +#include "libcamera/internal/v4l2_videodevice.h"
> > > +
> > > +namespace {
> > > +
> > > +bool isFormatRaw(const libcamera::PixelFormat &pixFmt)
> > > +{
> > > +	return libcamera::PixelFormatInfo::info(pixFmt).colourEncoding ==
> > > +	       libcamera::PixelFormatInfo::ColourEncodingRAW;
> > > +}
> > > +
> > > +}; /* namespace */
> > > +
> > > +namespace libcamera {
> >
> > Does this need to be in the libcamera namespace? This will export a lot of
> 
> Do you mean the pipeline handler and the associated classes ? Maybe we
> should partition these even further in a per-pipeline namespaces, yes
> 
> > symbols that don't really need to be exported as far as I can see.
> 
> What are the implications in the generated .so file of exposing these
> symbols in the libcamera namespace ? I don't think further namespace
> nesting has any implication on the symbols' external visibility, but
> it's only meant to restrict access to symbols within the code

I was actually concerned about the external visibility; you're right that changing
which named namespace the symbol is in does not change its visibility.

What I had in mind is just using an anonymous namespace instead of "libcamera"
in this file, because as far as I can tell, these symbols don't need to be exported.
An anonymous namespace would have multiple advantages:

 * avoids name conflicts (unlikely but still),
 * fewer public symbols usually coincide with less work for the dynamic loader,
 * enables certain optimizations.

In any case, this is not critical at all. I only remarked on it because
it is such a low-hanging fruit in this case.


> 
> 
> > (Maybe in the future?) (Although to be fair, this affects most [all?] pipeline handlers.)
> 
> yeah, all pipelines (but rpi which has nested namespaces) are like
> this if I'm not mistaken..
> [...]


Regards,
Barnabás Pőcze