diff mbox series

media: v4l2-subdev: Document that routing support depends on streams

Message ID 20230818155518.440-1-laurent.pinchart@ideasonboard.com
State Accepted
Commit da7e34e558a522d4a71660a165815979b1397074
Headers show
Series media: v4l2-subdev: Document that routing support depends on streams | expand

Commit Message

Laurent Pinchart Aug. 18, 2023, 3:55 p.m. UTC
Routing support, through the subdev .set_routing() operation, requires
the subdev to support streams. This is however not clearly documented
anywhere. Fix it by expanding the operation's documentation to indicate
that subdevs must set the V4L2_SUBDEV_FL_STREAMS flag.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/media/v4l2-subdev.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Laurent Pinchart Aug. 20, 2023, 10:56 p.m. UTC | #1
Hi Sakari,

On Fri, Aug 18, 2023 at 04:18:41PM +0000, Sakari Ailus wrote:
> On Fri, Aug 18, 2023 at 06:55:18PM +0300, Laurent Pinchart wrote:
> > Routing support, through the subdev .set_routing() operation, requires
> > the subdev to support streams. This is however not clearly documented
> > anywhere. Fix it by expanding the operation's documentation to indicate
> > that subdevs must set the V4L2_SUBDEV_FL_STREAMS flag.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/media/v4l2-subdev.h | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index b325df0d54d6..0b04ed1994b6 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -822,8 +822,9 @@ struct v4l2_subdev_state {
> >   *		     operation shall fail if the pad index it has been called on
> >   *		     is not valid or in case of unrecoverable failures.
> >   *
> > - * @set_routing: enable or disable data connection routes described in the
> > - *		 subdevice routing table.
> > + * @set_routing: Enable or disable data connection routes described in the
> > + *		 subdevice routing table. Subdevs that implement this operation
> > + *		 must set the V4L2_SUBDEV_FL_STREAMS flag.
> 
> Could we set the flag in the core when this op exists for a sub-device?

That won't work in all cases, as a driver could expose immutable routes
by creating them in the .init_cfg() function, without implementing
.set_routing().

Another option would be to check if the drivers has created routes after
the .init_cfg() called (indirectly) from v4l2_subdev_init_finalize(). It
may be a bit fragile though.

> We could do similarly for events when the sub-device has a control handler.

A subdev could generate non-control events too. In most cases I suppose
it would still create a control handler, but I don't think we should
require that.

> The device node should probably exist in almost all cases, but I'm not sure
> right now whether there is a reasonable test for it.
Sakari Ailus Sept. 2, 2023, 12:10 p.m. UTC | #2
Hi Laurent,

On Mon, Aug 21, 2023 at 01:56:04AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Fri, Aug 18, 2023 at 04:18:41PM +0000, Sakari Ailus wrote:
> > On Fri, Aug 18, 2023 at 06:55:18PM +0300, Laurent Pinchart wrote:
> > > Routing support, through the subdev .set_routing() operation, requires
> > > the subdev to support streams. This is however not clearly documented
> > > anywhere. Fix it by expanding the operation's documentation to indicate
> > > that subdevs must set the V4L2_SUBDEV_FL_STREAMS flag.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  include/media/v4l2-subdev.h | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > index b325df0d54d6..0b04ed1994b6 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -822,8 +822,9 @@ struct v4l2_subdev_state {
> > >   *		     operation shall fail if the pad index it has been called on
> > >   *		     is not valid or in case of unrecoverable failures.
> > >   *
> > > - * @set_routing: enable or disable data connection routes described in the
> > > - *		 subdevice routing table.
> > > + * @set_routing: Enable or disable data connection routes described in the
> > > + *		 subdevice routing table. Subdevs that implement this operation
> > > + *		 must set the V4L2_SUBDEV_FL_STREAMS flag.
> > 
> > Could we set the flag in the core when this op exists for a sub-device?
> 
> That won't work in all cases, as a driver could expose immutable routes
> by creating them in the .init_cfg() function, without implementing
> .set_routing().
> 
> Another option would be to check if the drivers has created routes after
> the .init_cfg() called (indirectly) from v4l2_subdev_init_finalize(). It
> may be a bit fragile though.

If you have either, then the sub-device does support streams. As otherwise,
there are no streams exposed to the user space. Right?

> 
> > We could do similarly for events when the sub-device has a control handler.
> 
> A subdev could generate non-control events too. In most cases I suppose
> it would still create a control handler, but I don't think we should
> require that.

I suggested this mainly as there are tonnes of drivers that expose controls
(and thus control events) but do not have the events flag set. Doing this
automatically for almost all drivers would reduce bugs.

> 
> > The device node should probably exist in almost all cases, but I'm not sure
> > right now whether there is a reasonable test for it.
Tomi Valkeinen Sept. 4, 2023, 5:34 a.m. UTC | #3
On 18/08/2023 18:55, Laurent Pinchart wrote:
> Routing support, through the subdev .set_routing() operation, requires
> the subdev to support streams. This is however not clearly documented
> anywhere. Fix it by expanding the operation's documentation to indicate
> that subdevs must set the V4L2_SUBDEV_FL_STREAMS flag.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   include/media/v4l2-subdev.h | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index b325df0d54d6..0b04ed1994b6 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -822,8 +822,9 @@ struct v4l2_subdev_state {
>    *		     operation shall fail if the pad index it has been called on
>    *		     is not valid or in case of unrecoverable failures.
>    *
> - * @set_routing: enable or disable data connection routes described in the
> - *		 subdevice routing table.
> + * @set_routing: Enable or disable data connection routes described in the
> + *		 subdevice routing table. Subdevs that implement this operation
> + *		 must set the V4L2_SUBDEV_FL_STREAMS flag.
>    *
>    * @enable_streams: Enable the streams defined in streams_mask on the given
>    *	source pad. Subdevs that implement this operation must use the active

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi
Sakari Ailus Sept. 4, 2023, 6:35 a.m. UTC | #4
On Mon, Sep 04, 2023 at 08:33:37AM +0300, Tomi Valkeinen wrote:
> On 02/09/2023 15:10, Sakari Ailus wrote:
> > Hi Laurent,
> > 
> > On Mon, Aug 21, 2023 at 01:56:04AM +0300, Laurent Pinchart wrote:
> > > Hi Sakari,
> > > 
> > > On Fri, Aug 18, 2023 at 04:18:41PM +0000, Sakari Ailus wrote:
> > > > On Fri, Aug 18, 2023 at 06:55:18PM +0300, Laurent Pinchart wrote:
> > > > > Routing support, through the subdev .set_routing() operation, requires
> > > > > the subdev to support streams. This is however not clearly documented
> > > > > anywhere. Fix it by expanding the operation's documentation to indicate
> > > > > that subdevs must set the V4L2_SUBDEV_FL_STREAMS flag.
> > > > > 
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > ---
> > > > >   include/media/v4l2-subdev.h | 5 +++--
> > > > >   1 file changed, 3 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > > index b325df0d54d6..0b04ed1994b6 100644
> > > > > --- a/include/media/v4l2-subdev.h
> > > > > +++ b/include/media/v4l2-subdev.h
> > > > > @@ -822,8 +822,9 @@ struct v4l2_subdev_state {
> > > > >    *		     operation shall fail if the pad index it has been called on
> > > > >    *		     is not valid or in case of unrecoverable failures.
> > > > >    *
> > > > > - * @set_routing: enable or disable data connection routes described in the
> > > > > - *		 subdevice routing table.
> > > > > + * @set_routing: Enable or disable data connection routes described in the
> > > > > + *		 subdevice routing table. Subdevs that implement this operation
> > > > > + *		 must set the V4L2_SUBDEV_FL_STREAMS flag.
> > > > 
> > > > Could we set the flag in the core when this op exists for a sub-device?
> > > 
> > > That won't work in all cases, as a driver could expose immutable routes
> > > by creating them in the .init_cfg() function, without implementing
> > > .set_routing().
> > > 
> > > Another option would be to check if the drivers has created routes after
> > > the .init_cfg() called (indirectly) from v4l2_subdev_init_finalize(). It
> > > may be a bit fragile though.
> > 
> > If you have either, then the sub-device does support streams. As otherwise,
> > there are no streams exposed to the user space. Right?
> 
> We need to know the existence of V4L2_SUBDEV_FL_STREAMS flag before calling
> init_cfg, in __v4l2_subdev_state_alloc.

Good point. Still, it'd be great to get rid of such flags. They are
incorrectly set in so many places. :-(
diff mbox series

Patch

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index b325df0d54d6..0b04ed1994b6 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -822,8 +822,9 @@  struct v4l2_subdev_state {
  *		     operation shall fail if the pad index it has been called on
  *		     is not valid or in case of unrecoverable failures.
  *
- * @set_routing: enable or disable data connection routes described in the
- *		 subdevice routing table.
+ * @set_routing: Enable or disable data connection routes described in the
+ *		 subdevice routing table. Subdevs that implement this operation
+ *		 must set the V4L2_SUBDEV_FL_STREAMS flag.
  *
  * @enable_streams: Enable the streams defined in streams_mask on the given
  *	source pad. Subdevs that implement this operation must use the active