[libcamera-devel] [PATCH] pipeline: simple: Rework the supportedDevices list
Marco Felsch
m.felsch at pengutronix.de
Thu May 6 09:27:06 CEST 2021
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 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
>
--
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 |
More information about the libcamera-devel
mailing list