[libcamera-devel] [PATCH] pipeline: simple: Rework the supportedDevices list
Phi-bang Nguyen
pnguyen at baylibre.com
Thu May 6 20:12:43 CEST 2021
Hi Laurent,
On Thu, May 6, 2021 at 9:27 AM Marco Felsch <m.felsch at 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 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 ?
>
> 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 |
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210506/e499bbc2/attachment.htm>
More information about the libcamera-devel
mailing list