| Message ID | 20210503122518.745088-1-pnguyen@baylibre.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi, 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. Regards, Marco > 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@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 } } }, > }; > > } /* 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) { > + if (cvt.first) { > + 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 '" > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Marco & Laurent, On Wed, May 5, 2021 at 5:40 PM Marco Felsch <m.felsch@pengutronix.de> wrote: > Hi, > > 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. Regards, > Marco > Best Regards, Phi-Bang > > 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@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 } } }, > > }; > > > > } /* 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) { > > + if (cvt.first) { > > + 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 '" > > -- > > 2.25.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | >
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@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 '"
Hi Laurent, Phi-Bang, On 21-05-06 00:36, Laurent Pinchart wrote: > 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. After that explanation to me as well, should we add this example to the commit message? Regards, Marco > > > > > 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@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 >
Hi Laurent, On Thu, May 6, 2021 at 9:27 AM Marco Felsch <m.felsch@pengutronix.de> wrote: > Hi Laurent, Phi-Bang, > > On 21-05-06 00:36, Laurent Pinchart wrote: > > 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. > > After that explanation to me as well, should we add this example to the > commit message? > > Regards, > Marco > > > > > > > > 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@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 ? > > Thank you ! I made the suggested changes, tested it and pushed v2. I also reworked the commit message. Regards, Phi-Bang > > > > + 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 > > > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | >
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 } } }, }; } /* 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) { + if (cvt.first) { + 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 '"
There is a usecase that the same driver may go with a converter on a platform and with another converter on another platform. 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@baylibre.com> --- src/libcamera/pipeline/simple/simple.cpp | 34 +++++++++++++++--------- 1 file changed, 22 insertions(+), 12 deletions(-)