<div dir="ltr"><div>Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, May 6, 2021 at 9:27 AM Marco Felsch <<a href="mailto:m.felsch@pengutronix.de">m.felsch@pengutronix.de</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Laurent, Phi-Bang,<br>
<br>
On 21-05-06 00:36, Laurent Pinchart wrote:<br>
> Hi Phi-Bang,<br>
> <br>
> On Wed, May 05, 2021 at 06:20:29PM +0200, Phi-bang Nguyen wrote:<br>
> > On Wed, May 5, 2021 at 5:40 PM Marco Felsch wrote:<br>
> > > On 21-05-03 14:25, Phi-Bang Nguyen wrote:<br>
> > > > There is a usecase that the same driver may go with a converter<br>
> > > > on a platform and with another converter on another platform.<br>
> > ><br>
> > > What did you mean by "another converter on another platform"? What other<br>
> > > convert should the platform take? IMHO if there are more than one<br>
> > > convert than you should use a own pipeline handler.<br>
> ><br>
> > Thank you for your comment.<br>
> > <br>
> > The simple pipeline handler well fits our needs so we don't make a specific<br>
> > pipeline handler.<br>
> > Perhaps, it will be better if I describe with an example.<br>
> > <br>
> > We have a driver named "mtk-seninf" that uses the "mtk-mdp" converter on<br>
> > the MT8167 platform and the "mtk-mdp3" converter on the MT8183 platform.<br>
> > With the current code, if I add two lines to the supportedDevices list as<br>
> > below :<br>
> > <br>
> > { "mtk-seninf", "mtk-mdp", 3 },<br>
> > { "mtk-seninf", "mtk-mdp3", 3 },<br>
> > <br>
> > On the MT8183 platform, it will not work because libcamera will find and<br>
> > acquire the first entry : "mtk-seninf" together with "mtk-mdp" converter<br>
> > and skip the rest.<br>
> > So, that's the purpose of my patch.<br>
> <br>
> Makes sense to me.<br>
<br>
After that explanation to me as well, should we add this example to the<br>
commit message?<br>
<br>
Regards,<br>
Marco<br>
<br>
> <br>
> > > > Currently, the simple pipeline handler only takes the 1st driver<br>
> > > > it can acquire in the supported devices list and skip the rest.<br>
> > > > This makes it take the wrong converter for the above usecase.<br>
> > > ><br>
> > > > So, make the changes to support the above usecase.<br>
> > > ><br>
> > > > Signed-off-by: Phi-Bang Nguyen <<a href="mailto:pnguyen@baylibre.com" target="_blank">pnguyen@baylibre.com</a>><br>
> > > > ---<br>
> > > > src/libcamera/pipeline/simple/simple.cpp | 34 +++++++++++++++---------<br>
> > > > 1 file changed, 22 insertions(+), 12 deletions(-)<br>
> > > ><br>
> > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp<br>
> > > > index f6095d38..9a572694 100644<br>
> > > > --- a/src/libcamera/pipeline/simple/simple.cpp<br>
> > > > +++ b/src/libcamera/pipeline/simple/simple.cpp<br>
> > > > @@ -127,16 +127,19 @@ class SimplePipelineHandler;<br>
> > > ><br>
> > > > struct SimplePipelineInfo {<br>
> > > > const char *driver;<br>
> > > > - const char *converter;<br>
> > > > - unsigned int numStreams;<br>
> > > > + /*<br>
> > > > + * Each converter in the list contains the name<br>
> > > > + * and the number of streams it supports.<br>
> > > > + */<br>
> > > > + std::vector<std::pair<const char *, unsigned int>> converters;<br>
> > > > };<br>
> > > ><br>
> > > > namespace {<br>
> > > ><br>
> > > > static const SimplePipelineInfo supportedDevices[] = {<br>
> > > > - { "imx7-csi", "pxp", 1 },<br>
> > > > - { "qcom-camss", nullptr, 1 },<br>
> > > > - { "sun6i-csi", nullptr, 1 },<br>
> > > > + { "imx7-csi", { { "pxp", 1 } } },<br>
> > > > + { "qcom-camss", { { nullptr, 1 } } },<br>
> > > > + { "sun6i-csi", { { nullptr, 1 } } },<br>
> <br>
> There's no need to add an empty entry to the vector, you can write<br>
> <br>
> { "qcom-camss", { } },<br>
> { "sun6i-csi", { } },<br>
> <br>
> > > > };<br>
> > > ><br>
> > > > } /* namespace */<br>
> > > > @@ -145,7 +148,7 @@ class SimpleCameraData : public CameraData<br>
> > > > {<br>
> > > > public:<br>
> > > > SimpleCameraData(SimplePipelineHandler *pipe,<br>
> > > > - const SimplePipelineInfo *info,<br>
> > > > + unsigned int numStreams,<br>
> > > > MediaEntity *sensor);<br>
> > > ><br>
> > > > bool isValid() const { return sensor_ != nullptr; }<br>
> > > > @@ -266,9 +269,9 @@ private:<br>
> > > > */<br>
> > > ><br>
> > > > SimpleCameraData::SimpleCameraData(SimplePipelineHandler *pipe,<br>
> > > > - const SimplePipelineInfo *info,<br>
> > > > + unsigned int numStreams,<br>
> > > > MediaEntity *sensor)<br>
> > > > - : CameraData(pipe), streams_(info->numStreams)<br>
> > > > + : CameraData(pipe), streams_(numStreams)<br>
> > > > {<br>
> > > > int ret;<br>
> > > ><br>
> > > > @@ -934,6 +937,7 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)<br>
> > > > {<br>
> > > > const SimplePipelineInfo *info = nullptr;<br>
> > > > MediaDevice *converter = nullptr;<br>
> > > > + unsigned int numStreams = 1;<br>
> > > ><br>
> > > > for (const SimplePipelineInfo &inf : supportedDevices) {<br>
> > > > DeviceMatch dm(inf.driver);<br>
> > > > @@ -947,9 +951,15 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)<br>
> > > > if (!media_)<br>
> > > > return false;<br>
> > > ><br>
> > > > - if (info->converter) {<br>
> > > > - DeviceMatch converterMatch(info->converter);<br>
> > > > - converter = acquireMediaDevice(enumerator, converterMatch);<br>
> > > > + for (const auto &cvt : info->converters) {<br>
> <br>
> I'd write<br>
> <br>
> for (const auto &[name, streams] : info->converters) {<br>
> <br>
> The code will be more explicit as you can use name and streams instead<br>
> of cvt.first and cvt.second.<br>
> <br>
> > > > + if (cvt.first) {<br>
> <br>
> This condition can be dropped if the empty vector entries are removed.<br>
> <br>
> Other than that, the patch looks good to me. Could you test those<br>
> changes and send a v2 ?<br><br></blockquote><div>Thank you ! I made the suggested changes, tested it and pushed v2.</div><div>I also reworked the commit message.</div><div><br></div><div>Regards,</div><div>Phi-Bang</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> > > > + DeviceMatch converterMatch(cvt.first);<br>
> > > > + converter = acquireMediaDevice(enumerator, converterMatch);<br>
> > > > + if (converter) {<br>
> > > > + numStreams = cvt.second;<br>
> > > > + break;<br>
> > > > + }<br>
> > > > + }<br>
> > > > }<br>
> > > ><br>
> > > > /* Locate the sensors. */<br>
> > > > @@ -983,7 +993,7 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)<br>
> > > ><br>
> > > > for (MediaEntity *sensor : sensors) {<br>
> > > > std::unique_ptr<SimpleCameraData> data =<br>
> > > > - std::make_unique<SimpleCameraData>(this, info, sensor);<br>
> > > > + std::make_unique<SimpleCameraData>(this, numStreams, sensor);<br>
> > > > if (!data->isValid()) {<br>
> > > > LOG(SimplePipeline, Error)<br>
> > > > << "No valid pipeline for sensor '"<br>
> <br>
> -- <br>
> Regards,<br>
> <br>
> Laurent Pinchart<br>
> <br>
<br>
-- <br>
Pengutronix e.K. | |<br>
Steuerwalder Str. 21 | <a href="http://www.pengutronix.de/" rel="noreferrer" target="_blank">http://www.pengutronix.de/</a> |<br>
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |<br>
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |<br>
</blockquote></div></div>