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

12216 diff mbox series

The supportedDevices may contain entries which have the same driver
but different converters. For example, if we add these two entries:

{ "mtk-seninf", "mtk-mdp", 3 },
{ "mtk-seninf", "mtk-mdp3", 3 },

the simple pipeline handler will always take the first one where it
can acquire the driver and skip the rest.

So, make the changes to support this usecase.

Signed-off-by: Phi-Bang Nguyen <pnguyen@baylibre.com>
---
 src/libcamera/pipeline/simple/simple.cpp | 30 +++++++++++++++---------
 1 file changed, 19 insertions(+), 11 deletions(-)

Hi Laurent,

Just a friendly reminder of this patch :-)

Thanks !

Phi-Bang

On Thu, May 6, 2021 at 8:06 PM Phi-Bang Nguyen <pnguyen@baylibre.com> wrote:

> The supportedDevices may contain entries which have the same driver
> but different converters. For example, if we add these two entries:
>
> { "mtk-seninf", "mtk-mdp", 3 },
> { "mtk-seninf", "mtk-mdp3", 3 },
>
> the simple pipeline handler will always take the first one where it
> can acquire the driver and skip the rest.
>
> So, make the changes to support this usecase.
>
> Signed-off-by: Phi-Bang Nguyen <pnguyen@baylibre.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 30 +++++++++++++++---------
>  1 file changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/src/libcamera/pipeline/simple/simple.cpp
> b/src/libcamera/pipeline/simple/simple.cpp
> index f6095d38..4c87ec4c 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", {} },
> +       { "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,13 @@ bool SimplePipelineHandler::match(DeviceEnumerator
> *enumerator)
>         if (!media_)
>                 return false;
>
> -       if (info->converter) {
> -               DeviceMatch converterMatch(info->converter);
> +       for (const auto &[name, streams] : info->converters) {
> +               DeviceMatch converterMatch(name);
>                 converter = acquireMediaDevice(enumerator, converterMatch);
> +               if (converter) {
> +                       numStreams = streams;
> +                       break;
> +               }
>         }
>
>         /* Locate the sensors. */
> @@ -983,7 +991,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
>
>

Hi Phi-Bang,

Thank you for the patch, and sorry for the delay.

On Thu, May 06, 2021 at 08:06:06PM +0200, Phi-Bang Nguyen wrote:
> The supportedDevices may contain entries which have the same driver
> but different converters. For example, if we add these two entries:
> 
> { "mtk-seninf", "mtk-mdp", 3 },
> { "mtk-seninf", "mtk-mdp3", 3 },
> 
> the simple pipeline handler will always take the first one where it
> can acquire the driver and skip the rest.
> 
> So, make the changes to support this usecase.
> 
> Signed-off-by: Phi-Bang Nguyen <pnguyen@baylibre.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

and pushed.

> ---
>  src/libcamera/pipeline/simple/simple.cpp | 30 +++++++++++++++---------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index f6095d38..4c87ec4c 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", {} },
> +	{ "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,13 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
>  	if (!media_)
>  		return false;
>  
> -	if (info->converter) {
> -		DeviceMatch converterMatch(info->converter);
> +	for (const auto &[name, streams] : info->converters) {
> +		DeviceMatch converterMatch(name);
>  		converter = acquireMediaDevice(enumerator, converterMatch);
> +		if (converter) {
> +			numStreams = streams;
> +			break;
> +		}
>  	}
>  
>  	/* Locate the sensors. */
> @@ -983,7 +991,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 '"