| Message ID | 20230818155518.440-1-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: v4l2-subdev: Document that routing support depends on streams | expand |
Hi Laurent, 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? We could do similarly for events when the sub-device has a control handler. 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.
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.
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.
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. Tomi
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
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. :-(
On 04/09/2023 09:35, Sakari Ailus wrote: > 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. :-( Hmm, well, if we want, we could return an error at the end of __v4l2_subdev_init_finalize, if V4L2_SUBDEV_FL_STREAMS is set, but there is no .set_routing and .init_cfg did not set up routing, OR if V4L2_SUBDEV_FL_STREAMS is not set, but .set_routing exists. We could also check for V4L2_SUBDEV_FL_STREAMS in v4l2_subdev_set_routing, and return an error if it's not set. I think if a subdev erroneously does not set V4L2_SUBDEV_FL_STREAMS, the driver won't get very far. So it's not an issue that can go hidden for very long. Tomi
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
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(-)