[PATCH v2 2/2] libcamera: camera_manager: Add environment variable to order pipelines match
Julien Vuillaumier
julien.vuillaumier at nxp.com
Thu Mar 28 16:54:46 CET 2024
Hi Laurent,
On 27/03/2024 21:35, Laurent Pinchart wrote:
>
> 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.
Libcamera users are likely familiar with simple pipeline and probably
expect it to work on i.MX8/9 - as I understand it seems commonly used.
Also, there are features from simple pipeline not available in imx8-isi
pipeline, for instance support for more complex topologies - agreed,
imx8-isi could be extended to integrate that. Another example could be
Software ISP in simple pipeline - that may be valuable for i.MX
platforms having ISI device but no hardware ISP (i.MX93). On those
platforms I presume that Software ISP + IPA support using simple
pipeline could come almost for free - but not sure that extending
imx8-isi to add the same support would be worth the effort and the extra
maintenance cost.
Imx8-isi pipeline presumably brings value compared to simple pipeline on
systems using a smart sensor, when the use case requires duplicating
the stream with rescaling / CSC.
That said, the simple pipeline supports an optional scaler. So, an ISI
channel could possibly be setup in M2M mode to act as the scaler and
achieve a similar functionality (though less efficient with regard to
memory bandwidth usage).
Thus, I think we should keep support for mxc.isi device in the simple
pipeline as it is well-known by libcamera users, and also to benefit
from all its features.
>
>>>> 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.
On i.MX8MP indeed, depending on DT configuration, one can configure the
topology as:
CSI->ISP (no ISI): rkisp1 pipeline match
CSI->ISI (no ISP): imx8-isi or single pipeline match
Concurrency is only about imx8-isi vs single pipeline, for the ISI case.
>
> 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.
On i.MX8MP, even with dynamic routing in the kernel, I am not sure which
unique pipeline could handle the 2 uses cases :
ISP case: would presumably remain something for rkisp1 pipeline
ISI / no-ISP case: the 2 options of simple or imx8-isi pipelines would
still apply. Rkisp1 pipeline looks out of scope here.
> How does the situation differ on the i.MX9 ?
i.MX9 SoCs have an ISI device. Some of them also have a (new) ISP. That
ISP has a single hardware/context instance, and multi-camera use case is
operated via memory-to-memory operation. Camera raw frames are captured
from ISI channels video devices, and associated buffers passed to a
software instance of the ISP device. On the principle, the flow of
buffers handled by the pipeline is similar to the one done by IPU3
pipeline.
A dedicated pipeline is needed to support this ISP and its associated
IPA. ISI device usage is a common factor between that pipeline and
imx8-isi pipeline. However, usage and operation of the 2 pipelines being
very different, there is very limited implementation that can be shared
between them. Therefore, working assumption has been that evolution and
maintenance would be simpler by keeping the 2 separated.
It may be a bit premature to discuss the details of this ISP pipeline
right now: the ISP driver has not been posted to linux-media yet, which
is presumably a prerequisite before addressing the libcamera pipeline
part.
To come back to the original question, i.MX9 user may want to use simple
or imx8-isi pipeline with a smart camera, or the ISP pipeline with a raw
camera. But to pick the ISP pipeline, its match() procedure has to be
invoked first so that ISI devices are not acquired by simple or imx8-isi
pipelines.
>>>> 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
Thanks,
Julien
More information about the libcamera-devel
mailing list