[PATCH v2 2/2] libcamera: camera_manager: Add environment variable to order pipelines match
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Mar 27 21:35:50 CET 2024
Hi Julien,
On Fri, Mar 22, 2024 at 03:15:50PM +0100, Julien Vuillaumier wrote:
> On 22/03/2024 09:50, Jacopo Mondi wrote:
> > On Thu, Mar 21, 2024 at 08:26:06PM +0100, Julien Vuillaumier wrote:
> >> On 20/03/2024 13:53, Jacopo Mondi wrote:
> >>> On Fri, Mar 08, 2024 at 12:00:56PM +0100, Julien Vuillaumier wrote:
> >>>> To match the enumerated media devices, each pipeline handler registered
> >>>
> >>> s/pipeline handler registered/registered pipeline handler/
> >>>
> >>>> is used in no specific order. It is a limitation when several pipelines
> >>>> can match the devices, and user has to select a specific pipeline.
> >>>>
> >>>> For this purpose, environment variable LIBCAMERA_PIPELINES_MATCH_LIST is
> >>>> created that gives the option to define an ordered list of pipelines
> >>>
> >>> s/that gives/to give/
> >>>
> >>>> to invoke during the match process.
> >>>
> >>> Or just: "to match on."
> >>>
> >>>> LIBCAMERA_PIPELINES_MATCH_LIST="<name1>[,<name2>[,<name3>...]]]"
> >>>>
> >>>> Example:
> >>>> LIBCAMERA_PIPELINES_MATCH_LIST="PipelineHandlerRkISP1,SimplePipelineHandler"
> >>>>
> >>>> Signed-off-by: Julien Vuillaumier <julien.vuillaumier at nxp.com>
> >>>> ---
> >>>> Documentation/environment_variables.rst | 5 ++
> >>>> include/libcamera/internal/camera_manager.h | 1 +
> >>>> src/libcamera/camera_manager.cpp | 57 ++++++++++++++++-----
> >>>> 3 files changed, 51 insertions(+), 12 deletions(-)
> >>>>
> >>>> diff --git a/Documentation/environment_variables.rst b/Documentation/environment_variables.rst
> >>>> index a9b230bc..ea4da3c9 100644
> >>>> --- a/Documentation/environment_variables.rst
> >>>> +++ b/Documentation/environment_variables.rst
> >>>> @@ -37,6 +37,11 @@ LIBCAMERA_IPA_MODULE_PATH
> >>>>
> >>>> Example value: ``${HOME}/.libcamera/lib:/opt/libcamera/vendor/lib``
> >>>>
> >>>> +LIBCAMERA_PIPELINES_MATCH_LIST
> >>>> + Define an ordered list of pipeline names to be used to match the media devices in the system.
> >>>
> >>> long line over 80 cols which could eaily be broken in 2 lines
> >>>
> >>>> +
> >>>> + Example value: ``PipelineHandlerRkISP1,SimplePipelineHandler``
> >>>
> >>> These are the names of the PipelineHandlers classes, which is fine,
> >>> but maybe for users it is better to use something they can more easily
> >>> reference, like the meson option associated with the pipeline ?
> >>>
> >>> This will require a map though...
> >>
> >> As of today the pipeline name, defined as PipelineHandler class name, is
> >> presented to the user (log=DEBUG), during the match procedure:
> >> "Found registered pipeline handler <xyz>"
> >> "Pipeline handler <xyz> matched"
> >>
> >> Thus, reusing that same name to define the ordered list of pipelines for the
> >> match seemed fairly consistent.
> >
> > Yes, it's consistent indeed
> >
> >> But agreed, PipelineHandler names could be simpler. Currently, apart from
> >
> > I'm just a bit concerned about the fact users should either inspect
> > the code or debug logs, while using something like the meson option is
> > easier to access ?
> >
> >> those logs during match, pipeline name looks to be used only in an ipa
> >> interface test.
> >> If deemed useful, we could change the current names of each PipelineHandler
> >> to replace the class name, as you suggested, by the meson option name
> >> associated with that pipeline.
> >
> > No wait, I was suggesting using the meson option to populate the
> > LIBCAMERA_PIPELINES_MATCH_LIST env variable.
> >
> >> A simple way of doing that would be adding to the existing
> >> REGISTER_PIPELINE_HANDLER(handler) a parameter 'name' to be passed to
> >> PipelineHandlerFactory constructor, instead of the stringified class name.
> >> Registration of each existing pipelines would need to be updated
> >> accordingly.
> >>
> >> Do you recommend changing PipelineHandler name used in the context of that
> >> change?
> >> In that case, does the above proposal (add a short name to the register
> >> macro) makes sense?
> >
> > Maybe I'm just over-concerned. A way out could be to expand the env
> > variable description to clearly indicate what identifiers to use as
> > pipeline handler names. Something like:
> >
> > LIBCAMERA_PIPELINES_MATCH_LIST
> > Define an ordered list of pipeline handlers to be used to match the media
> > devices in the system. The pipeline handler names used to populate
> > the variable are the ones passed to the the
> > REGISTER_PIPELINE_HANDLER() macro in the source code.
> >
> > Example value: ``PipelineHandlerRkISP1,SimplePipelineHandler``
> >
> > Alternatively what I was proposing was to use the identifiers for
> > the 'pipelines' meson option
> >
> > option('pipelines',
> > type : 'array',
> > value : ['auto'],
> > choices : [
> > 'all',
> > 'auto',
> > 'imx8-isi',
> > 'ipu3',
> > 'rkisp1',
> > 'rpi/vc4',
> > 'simple',
> > 'uvcvideo',
> > 'vimc'
> > ],
> >
> >
> > LIBCAMERA_PIPELINES_MATCH_LIST=``rkisp1,simple,imx8-isi``
> >
> > and keep a map in CameraManager class, but this indeed requires more
> > maintainership.
>
> I share the concern that adding a map of nicknames in CameraManager
> class would add some extra maintenance.
> In that respect, the option of adding an explicit name parameter to
> REGISTER_PIPELINE_HANDLER() macro in order to assign to each pipeline
> its meson option name, was looking simpler to me.
If we want shorter names, that's the way to go indeed.
> Now, your proposal to expand the env variable description, documenting
> how the pipeline handler names are assigned with the register macro,
> looks perfectly fine to me.
> If that option makes it acceptable to keep the current pipeline handler
> names, and nobody objects against the principle, I propose to move
> forward with that for the v3.
>
> > Now that I wrote this, isn't it easier to just compile only the
> > pipeline handler you need ? I presume this doesn't apply to generic
> > distro though
>
> It does not apply to generic distro, indeed.
>
> But also when using a custom libcamera package, one may want to change
> pipeline handler at runtime because of different use case, camera
> (raw/smart), test purpose... Reconfiguring and rebuilding libcamera with
> the unique targeted pipeline is not a very practical option here.
I'm still struggling a bit to understand the use cases (besiding testing
during development). Please see below.
> >>>> +
> >>>> LIBCAMERA_RPI_CONFIG_FILE
> >>>> Define a custom configuration file to use in the Raspberry Pi pipeline handler.
> >>>>
> >>>> diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h
> >>>> index 33ebe069..c57e509a 100644
> >>>> --- a/include/libcamera/internal/camera_manager.h
> >>>> +++ b/include/libcamera/internal/camera_manager.h
> >>>> @@ -45,6 +45,7 @@ private:
> >>>> int init();
> >>>> void createPipelineHandlers();
> >>>> void cleanup() LIBCAMERA_TSA_EXCLUDES(mutex_);
> >>>> + void pipelineFactoryMatch(const PipelineHandlerFactoryBase *factory);
> >>>
> >>> Please move this one line up to match the function definition order in
> >>> the .cpp file
> >>>
> >>>>
> >>>> /*
> >>>> * This mutex protects
> >>>> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> >>>> index 355f3ada..620be1c8 100644
> >>>> --- a/src/libcamera/camera_manager.cpp
> >>>> +++ b/src/libcamera/camera_manager.cpp
> >>>> @@ -99,13 +99,36 @@ int CameraManager::Private::init()
> >>>>
> >>>> void CameraManager::Private::createPipelineHandlers()
> >>>> {
> >>>> - CameraManager *const o = LIBCAMERA_O_PTR();
> >>>> -
> >>>> /*
> >>>> * \todo Try to read handlers and order from configuration
> >>>> - * file and only fallback on all handlers if there is no
> >>>> - * configuration file.
> >>>> + * file and only fallback on environment variable or all handlers, if
> >>>> + * there is no configuration file.
> >>>> + */
> >>>> +
> >>>> + /*
> >>>> + * When a list of preferred pipelines is defined, iterate through the
> >>>> + * ordered list to match the devices enumerated.
> >>>
> >>> s/devices enumerated/enumerated devices/
> >>>
> >>>> + * Otherwise, device matching is done in no specific order with each
> >>>> + * registered pipeline handler.
> >>>> */
> >>>
> >>> What about moving this comment block in the below if (pipesList)
> >>> branch ?
> >>>
> >>>> + const char *pipesList =
> >>>> + utils::secure_getenv("LIBCAMERA_PIPELINES_MATCH_LIST");
> >>>> + if (pipesList) {
> >>>> + for (const auto &pipeName : utils::split(pipesList, ",")) {
> >>>> + const PipelineHandlerFactoryBase *factory;
> >>>> + factory = PipelineHandlerFactoryBase::getFactoryByName(pipeName);
> >>>> + if (!factory)
> >>>> + continue;
> >>>> +
> >>>> + LOG(Camera, Debug)
> >>>> + << "Found listed pipeline handler '"
> >>>> + << pipeName << "'";
> >>>> + pipelineFactoryMatch(factory);
> >>>> + }
> >>>> +
> >>>> + return;
> >>>> + }
> >>>> +
> >>>> const std::vector<PipelineHandlerFactoryBase *> &factories =
> >>>> PipelineHandlerFactoryBase::factories();
> >>>>
> >>>> @@ -117,15 +140,25 @@ void CameraManager::Private::createPipelineHandlers()
> >>>> * Try each pipeline handler until it exhaust
> >>>> * all pipelines it can provide.
> >>>> */
> >>>> - while (1) {
> >>>> - std::shared_ptr<PipelineHandler> pipe = factory->create(o);
> >>>> - if (!pipe->match(enumerator_.get()))
> >>>> - break;
> >>>> + pipelineFactoryMatch(factory);
> >>>> + }
> >>>> +}
> >>>>
> >>>> - LOG(Camera, Debug)
> >>>> - << "Pipeline handler \"" << factory->name()
> >>>> - << "\" matched";
> >>>> - }
> >>>> +void CameraManager::Private::pipelineFactoryMatch(const PipelineHandlerFactoryBase *factory)
> >>>> +{
> >>>> + CameraManager *const o = LIBCAMERA_O_PTR();
> >>>> +
> >>>> + /*
> >>>> + * Provide as many matching pipelines as possible
> >>>> + */
> >>>
> >>> Fits on a single line
> >>>
> >>> Also, I would
> >>> /* Match all the registed pipeline handlers. */
> >>>
> >>>> + while (1) {
> >>>> + std::shared_ptr<PipelineHandler> pipe = factory->create(o);
> >>>> + if (!pipe->match(enumerator_.get()))
> >>>> + break;
> >>>> +
> >>>> + LOG(Camera, Debug)
> >>>> + << "Pipeline handler \"" << factory->name()
> >>>> + << "\" matched";
> >>>> }
> >>>> }
> >>>
> >>> Overall, I concur this is an useful addition. I know there are ideas
> >>> about assigning a priority to pipeline handlers at creation time as
> >>> currently the only conflict is on the ISI which can be
> >>> matched both by the imx8-isi pipeline and the simple pipeline.
> >>
> >> As you mentioned, imx8-isi and simple pipelines are concurrent.
For this very specific problem, shouldn't we simply drop imx8-isi
support from the simple pipeline handler ? It was added there before we
had a dedicated pipeline handler for the imx8-isi. Now that we have one,
all the use cases that the simple pipeline handler could cover should be
handled by the imx8-isi pipeline handler. If some are missing, that's a
bug, and the imx8-isi pipeline handler should be extended.
> >> With imx9
> >> family, there is an additional cause of concurrency as platforms can/will
> >> use topologies with both ISI + ISP. And depending on the use case, user may
> >> want to use either simple, or imx8-isi or the isi+isp pipeline.
Note that the i.MX8MP also has an ISI and 2 ISP instances. Currently, we
need to select what to route the CSIS output to in DT, which is not a
good solution, but from a libcamera point of view, it means that only
one of the imx8-isi and rkisp1 pipeline handler can ever match at
runtime.
This should be fixed in the kernel, to enable dynamic routing
configuration from userspace. That's no simple work, and I'm not aware
of anyone actively working on it, but should this be completed at some
point, we would need to also improve libcamera to cover all the use
cases with a single pipeline handler. I don't really see why users would
want to select between multiple pipeline handlers.
How does the situation differ on the i.MX9 ?
> >> This change is a basic way to dynamically assign relative priorities to the
> >> pipelines. It was suggested in v1 review comments to also have a similar
> >> configuration possible from a global config file.
> >>
> >>> For the time being, with the above issues fixed
> >>> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list