[libcamera-devel] [PATCH] pipeline: simple: Rework the supportedDevices list

[Laurent Pinchart]

Hi Phi-Bang,

On Wed, May 05, 2021 at 06:20:29PM +0200, Phi-bang Nguyen wrote:
> On Wed, May 5, 2021 at 5:40 PM Marco Felsch wrote:
> > On 21-05-03 14:25, Phi-Bang Nguyen wrote:
> > > There is a usecase that the same driver may go with a converter
> > > on a platform and with another converter on another platform.
> >
> > What did you mean by "another converter on another platform"? What other
> > convert should the platform take? IMHO if there are more than one
> > convert than you should use a own pipeline handler.
>
> Thank you for your comment.
> 
> The simple pipeline handler well fits our needs so we don't make a specific
> pipeline handler.
> Perhaps, it will be better if I describe with an example.
> 
> We have a driver named "mtk-seninf" that uses the "mtk-mdp" converter on
> the MT8167 platform and the "mtk-mdp3" converter on the MT8183 platform.
> With the current code, if I add two lines to the supportedDevices list as
> below :
> 
> { "mtk-seninf", "mtk-mdp", 3 },
>  { "mtk-seninf", "mtk-mdp3", 3 },
> 
> On the MT8183 platform, it will not work because libcamera will find and
> acquire the first entry : "mtk-seninf" together with "mtk-mdp" converter
> and skip the rest.
> So, that's the purpose of my patch.

Makes sense to me.

> > > Currently, the simple pipeline handler only takes the 1st driver
> > > it can acquire in the supported devices list and skip the rest.
> > > This makes it take the wrong converter for the above usecase.
> > >
> > > So, make the changes to support the above usecase.
> > >
> > > Signed-off-by: Phi-Bang Nguyen <pnguyen at baylibre.com>
> > > ---
> > >  src/libcamera/pipeline/simple/simple.cpp | 34 +++++++++++++++---------
> > >  1 file changed, 22 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > > index f6095d38..9a572694 100644
> > > --- a/src/libcamera/pipeline/simple/simple.cpp
> > > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > > @@ -127,16 +127,19 @@ class SimplePipelineHandler;
> > >
> > >  struct SimplePipelineInfo {
> > >       const char *driver;
> > > -     const char *converter;
> > > -     unsigned int numStreams;
> > > +     /*
> > > +      * Each converter in the list contains the name
> > > +      * and the number of streams it supports.
> > > +      */
> > > +     std::vector<std::pair<const char *, unsigned int>> converters;
> > >  };
> > >
> > >  namespace {
> > >
> > >  static const SimplePipelineInfo supportedDevices[] = {
> > > -     { "imx7-csi", "pxp", 1 },
> > > -     { "qcom-camss", nullptr, 1 },
> > > -     { "sun6i-csi", nullptr, 1 },
> > > +     { "imx7-csi", { { "pxp", 1 } } },
> > > +     { "qcom-camss", { { nullptr, 1 } } },
> > > +     { "sun6i-csi", { { nullptr, 1 } } },

There's no need to add an empty entry to the vector, you can write

     { "qcom-camss", { } },
     { "sun6i-csi", { } },

> > >  };
> > >
> > >  } /* namespace */
> > > @@ -145,7 +148,7 @@ class SimpleCameraData : public CameraData
> > >  {
> > >  public:
> > >       SimpleCameraData(SimplePipelineHandler *pipe,
> > > -                      const SimplePipelineInfo *info,
> > > +                      unsigned int numStreams,
> > >                        MediaEntity *sensor);
> > >
> > >       bool isValid() const { return sensor_ != nullptr; }
> > > @@ -266,9 +269,9 @@ private:
> > >   */
> > >
> > >  SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,
> > > -                                const SimplePipelineInfo *info,
> > > +                                unsigned int numStreams,
> > >                                  MediaEntity *sensor)
> > > -     : CameraData(pipe), streams_(info->numStreams)
> > > +     : CameraData(pipe), streams_(numStreams)
> > >  {
> > >       int ret;
> > >
> > > @@ -934,6 +937,7 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> > >  {
> > >       const SimplePipelineInfo *info = nullptr;
> > >       MediaDevice *converter = nullptr;
> > > +     unsigned int numStreams = 1;
> > >
> > >       for (const SimplePipelineInfo &inf : supportedDevices) {
> > >               DeviceMatch dm(inf.driver);
> > > @@ -947,9 +951,15 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> > >       if (!media_)
> > >               return false;
> > >
> > > -     if (info->converter) {
> > > -             DeviceMatch converterMatch(info->converter);
> > > -             converter = acquireMediaDevice(enumerator, converterMatch);
> > > +     for (const auto &cvt : info->converters) {

I'd write

	for (const auto &[name, streams] : info->converters) {

The code will be more explicit as you can use name and streams instead
of cvt.first and cvt.second.

> > > +             if (cvt.first) {

This condition can be dropped if the empty vector entries are removed.

Other than that, the patch looks good to me. Could you test those
changes and send a v2 ?

> > > +                     DeviceMatch converterMatch(cvt.first);
> > > +                     converter = acquireMediaDevice(enumerator, converterMatch);
> > > +                     if (converter) {
> > > +                             numStreams = cvt.second;
> > > +                             break;
> > > +                     }
> > > +             }
> > >       }
> > >
> > >       /* Locate the sensors. */
> > > @@ -983,7 +993,7 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> > >
> > >       for (MediaEntity *sensor : sensors) {
> > >               std::unique_ptr<SimpleCameraData> data =
> > > -                     std::make_unique<SimpleCameraData>(this, info, sensor);
> > > +                     std::make_unique<SimpleCameraData>(this, numStreams, sensor);
> > >               if (!data->isValid()) {
> > >                       LOG(SimplePipeline, Error)
> > >                               << "No valid pipeline for sensor '"

-- 
Regards,

Laurent Pinchart