[v1,3/3] pipeline: rpi: Set a default route for entities where applicable
diff mbox series

Message ID 20250625084212.858487-4-naush@raspberrypi.com
State New
Headers show
Series
  • Raspberry Pi: Media graph enumeration updates
Related show

Commit Message

Naushir Patuck June 25, 2025, 8:41 a.m. UTC 
If an entity advertises streams support, and no routes are active, set
a default active route for Stream 0:[pad 0 -> pad 1]. This allows
serialiser/deserialiser devices to run without user setup in the default
case.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 .../pipeline/rpi/common/pipeline_base.cpp     | 33 ++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart June 26, 2025, 12:44 p.m. UTC | #1 
Hi Naush,

Thank you for the patch.

On Wed, Jun 25, 2025 at 09:41:18AM +0100, Naushir Patuck wrote:
> If an entity advertises streams support, and no routes are active, set
> a default active route for Stream 0:[pad 0 -> pad 1]. This allows
> serialiser/deserialiser devices to run without user setup in the default
> case.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  .../pipeline/rpi/common/pipeline_base.cpp     | 33 ++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index 219bd81cc63a..eafbf702ed0b 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -107,7 +107,38 @@ int configureMediaDevices(std::vector<std::pair<std::unique_ptr<V4L2Subdevice>,
>  		}
>  
>  		/*
> -		 * Next, enable the entity -> entity links, and setup the pad format.
> +		 * Next, setup the stream routing if needed. By default this
> +		 * sets up Stream 0:[pad 0 -> pad 1] route. Anything more
> +		 * complicated must currently be setup and activated externally.
> +		 *
> +		 * If we find any active routes, we don't change anything.
> +		 */
> +		if (device->caps().hasStreams()) {
> +			V4L2Subdevice::Routing routing;
> +
> +			ret = device->getRouting(&routing);
> +			if (ret)
> +				return ret;
> +
> +			/* If we find an active route, don't do anything more. */
> +			for (auto const &r : routing) {
> +				if (r.flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE)
> +					return 0;
> +			}
> +
> +			/*
> +			 * Set up a default Stream 0:[pad 0 -> pad 1] route if nothing
> +			 * has already been set.
> +			 */
> +			routing = { { V4L2Subdevice::Stream{ 0, 0 },
> +				      V4L2Subdevice::Stream{ 1, 0 },
> +				      V4L2_SUBDEV_ROUTE_FL_ACTIVE } };

That's really shooting in the dark :-(

We should model serializers and deserializers with dedicated objects,
like we do with the CameraSensor class.

> +
> +			ret = device->setRouting(&routing);
> +		}
> +
> +		/*
> +		 * Finally, enable the entity -> entity links, and setup the pad format.
>  		 *
>  		 * \todo Some bridge devices may chainge the media bus code, so we
>  		 * ought to read the source pad format and propagate it to the sink pad.
Naushir Patuck June 26, 2025, 12:52 p.m. UTC | #2 
On Thu, 26 Jun 2025 at 13:45, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Naush,
>
> Thank you for the patch.
>
> On Wed, Jun 25, 2025 at 09:41:18AM +0100, Naushir Patuck wrote:
> > If an entity advertises streams support, and no routes are active, set
> > a default active route for Stream 0:[pad 0 -> pad 1]. This allows
> > serialiser/deserialiser devices to run without user setup in the default
> > case.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  .../pipeline/rpi/common/pipeline_base.cpp     | 33 ++++++++++++++++++-
> >  1 file changed, 32 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > index 219bd81cc63a..eafbf702ed0b 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > @@ -107,7 +107,38 @@ int configureMediaDevices(std::vector<std::pair<std::unique_ptr<V4L2Subdevice>,
> >               }
> >
> >               /*
> > -              * Next, enable the entity -> entity links, and setup the pad format.
> > +              * Next, setup the stream routing if needed. By default this
> > +              * sets up Stream 0:[pad 0 -> pad 1] route. Anything more
> > +              * complicated must currently be setup and activated externally.
> > +              *
> > +              * If we find any active routes, we don't change anything.
> > +              */
> > +             if (device->caps().hasStreams()) {
> > +                     V4L2Subdevice::Routing routing;
> > +
> > +                     ret = device->getRouting(&routing);
> > +                     if (ret)
> > +                             return ret;
> > +
> > +                     /* If we find an active route, don't do anything more. */
> > +                     for (auto const &r : routing) {
> > +                             if (r.flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE)
> > +                                     return 0;
> > +                     }
> > +
> > +                     /*
> > +                      * Set up a default Stream 0:[pad 0 -> pad 1] route if nothing
> > +                      * has already been set.
> > +                      */
> > +                     routing = { { V4L2Subdevice::Stream{ 0, 0 },
> > +                                   V4L2Subdevice::Stream{ 1, 0 },
> > +                                   V4L2_SUBDEV_ROUTE_FL_ACTIVE } };
>
> That's really shooting in the dark :-(
>
> We should model serializers and deserializers with dedicated objects,
> like we do with the CameraSensor class.

Sadly I agree, but how can we expose this to the user/application to
configure for their needs?

Naush

>
> > +
> > +                     ret = device->setRouting(&routing);
> > +             }
> > +
> > +             /*
> > +              * Finally, enable the entity -> entity links, and setup the pad format.
> >                *
> >                * \todo Some bridge devices may chainge the media bus code, so we
> >                * ought to read the source pad format and propagate it to the sink pad.
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart June 26, 2025, 1:02 p.m. UTC | #3 
On Thu, Jun 26, 2025 at 01:52:51PM +0100, Naushir Patuck wrote:
> On Thu, 26 Jun 2025 at 13:45, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Naush,
> >
> > Thank you for the patch.
> >
> > On Wed, Jun 25, 2025 at 09:41:18AM +0100, Naushir Patuck wrote:
> > > If an entity advertises streams support, and no routes are active, set
> > > a default active route for Stream 0:[pad 0 -> pad 1]. This allows
> > > serialiser/deserialiser devices to run without user setup in the default
> > > case.
> > >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > ---
> > >  .../pipeline/rpi/common/pipeline_base.cpp     | 33 ++++++++++++++++++-
> > >  1 file changed, 32 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > index 219bd81cc63a..eafbf702ed0b 100644
> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > @@ -107,7 +107,38 @@ int configureMediaDevices(std::vector<std::pair<std::unique_ptr<V4L2Subdevice>,
> > >               }
> > >
> > >               /*
> > > -              * Next, enable the entity -> entity links, and setup the pad format.
> > > +              * Next, setup the stream routing if needed. By default this
> > > +              * sets up Stream 0:[pad 0 -> pad 1] route. Anything more
> > > +              * complicated must currently be setup and activated externally.
> > > +              *
> > > +              * If we find any active routes, we don't change anything.
> > > +              */
> > > +             if (device->caps().hasStreams()) {
> > > +                     V4L2Subdevice::Routing routing;
> > > +
> > > +                     ret = device->getRouting(&routing);
> > > +                     if (ret)
> > > +                             return ret;
> > > +
> > > +                     /* If we find an active route, don't do anything more. */
> > > +                     for (auto const &r : routing) {
> > > +                             if (r.flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE)
> > > +                                     return 0;
> > > +                     }
> > > +
> > > +                     /*
> > > +                      * Set up a default Stream 0:[pad 0 -> pad 1] route if nothing
> > > +                      * has already been set.
> > > +                      */
> > > +                     routing = { { V4L2Subdevice::Stream{ 0, 0 },
> > > +                                   V4L2Subdevice::Stream{ 1, 0 },
> > > +                                   V4L2_SUBDEV_ROUTE_FL_ACTIVE } };
> >
> > That's really shooting in the dark :-(
> >
> > We should model serializers and deserializers with dedicated objects,
> > like we do with the CameraSensor class.
> 
> Sadly I agree, but how can we expose this to the user/application to
> configure for their needs?

I don't think we need to, at least not yet (*). The main issue to solve
in my opinion is that individual pipeline handlers should not have to
manually handle serializers, deserializers and muxes. The part of the
pipeline in front of the CSI-2 receiver should be handled by helpers
classes shared by pipeline handlers. That will give us the ability to
implement specialized classes for specific components, and applying a
default configuration matching their needs. After that, we can work on
exposing some control over that pipeline to applications.

* I was going to write that I am for once limiting the amount of
requested yak shaving, but looking at what folows the first sentence,
I'm not so sure :-)

> > > +
> > > +                     ret = device->setRouting(&routing);
> > > +             }
> > > +
> > > +             /*
> > > +              * Finally, enable the entity -> entity links, and setup the pad format.
> > >                *
> > >                * \todo Some bridge devices may chainge the media bus code, so we
> > >                * ought to read the source pad format and propagate it to the sink pad.

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
index 219bd81cc63a..eafbf702ed0b 100644
--- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
+++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
@@ -107,7 +107,38 @@  int configureMediaDevices(std::vector<std::pair<std::unique_ptr<V4L2Subdevice>,
 		}
 
 		/*
-		 * Next, enable the entity -> entity links, and setup the pad format.
+		 * Next, setup the stream routing if needed. By default this
+		 * sets up Stream 0:[pad 0 -> pad 1] route. Anything more
+		 * complicated must currently be setup and activated externally.
+		 *
+		 * If we find any active routes, we don't change anything.
+		 */
+		if (device->caps().hasStreams()) {
+			V4L2Subdevice::Routing routing;
+
+			ret = device->getRouting(&routing);
+			if (ret)
+				return ret;
+
+			/* If we find an active route, don't do anything more. */
+			for (auto const &r : routing) {
+				if (r.flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE)
+					return 0;
+			}
+
+			/*
+			 * Set up a default Stream 0:[pad 0 -> pad 1] route if nothing
+			 * has already been set.
+			 */
+			routing = { { V4L2Subdevice::Stream{ 0, 0 },
+				      V4L2Subdevice::Stream{ 1, 0 },
+				      V4L2_SUBDEV_ROUTE_FL_ACTIVE } };
+
+			ret = device->setRouting(&routing);
+		}
+
+		/*
+		 * Finally, enable the entity -> entity links, and setup the pad format.
 		 *
 		 * \todo Some bridge devices may chainge the media bus code, so we
 		 * ought to read the source pad format and propagate it to the sink pad.