| Message ID | 20240917124345.16681-2-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] media: Documentation: Deprecate s_stream video op, update docs | expand |
On Tue, Sep 17, 2024 at 03:43:45PM +0300, Sakari Ailus wrote: > Document the expected {enable,disable}_streams callback behaviour for > drivers that are stream-unaware i.e. don't specify the > V4L2_SUBDEV_CAP_STREAMS sub-device capability flat. In this specific case, > the mask argument can be ignored. Wouldn't it be better to use BIT(0) in that case to simplifiy interoperability with stream-aware devices ? > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > include/media/v4l2-subdev.h | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index 3cc6b4a5935f..67a6e6ec58b8 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -834,11 +834,19 @@ struct v4l2_subdev_state { > * v4l2_subdev_init_finalize() at initialization time). Do not call > * directly, use v4l2_subdev_enable_streams() instead. > * > + * Drivers that support only a single stream without setting the > + * V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to > + * be concerned with the mask argument. > + * > * @disable_streams: Disable the streams defined in streams_mask on the given > * source pad. Subdevs that implement this operation must use the active > * state management provided by the subdev core (enabled through a call to > * v4l2_subdev_init_finalize() at initialization time). Do not call > * directly, use v4l2_subdev_disable_streams() instead. > + * > + * Drivers that support only a single stream without setting the > + * V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to > + * be concerned with the mask argument. > */ > struct v4l2_subdev_pad_ops { > int (*enum_mbus_code)(struct v4l2_subdev *sd,
Hi Laurent, On Tue, Sep 17, 2024 at 04:00:47PM +0300, Laurent Pinchart wrote: > On Tue, Sep 17, 2024 at 03:43:45PM +0300, Sakari Ailus wrote: > > Document the expected {enable,disable}_streams callback behaviour for > > drivers that are stream-unaware i.e. don't specify the > > V4L2_SUBDEV_CAP_STREAMS sub-device capability flat. In this specific case, > > the mask argument can be ignored. > > Wouldn't it be better to use BIT(0) in that case to simplifiy > interoperability with stream-aware devices ? That's indeed the current implementation.
On 17/09/2024 16:00, Laurent Pinchart wrote: > On Tue, Sep 17, 2024 at 03:43:45PM +0300, Sakari Ailus wrote: >> Document the expected {enable,disable}_streams callback behaviour for >> drivers that are stream-unaware i.e. don't specify the >> V4L2_SUBDEV_CAP_STREAMS sub-device capability flat. In this specific case, >> the mask argument can be ignored. > > Wouldn't it be better to use BIT(0) in that case to simplifiy > interoperability with stream-aware devices ? The caller has to set BIT(0), but I think here the documentation is about the callee. If the driver is not stream aware and implements the callbacks, it will get BIT(0) as the mask parameter (do we enforce this?), but as there's nothing it can do with the parameter it "does not need to be concerned with the mask argument". Tomi >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >> --- >> include/media/v4l2-subdev.h | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h >> index 3cc6b4a5935f..67a6e6ec58b8 100644 >> --- a/include/media/v4l2-subdev.h >> +++ b/include/media/v4l2-subdev.h >> @@ -834,11 +834,19 @@ struct v4l2_subdev_state { >> * v4l2_subdev_init_finalize() at initialization time). Do not call >> * directly, use v4l2_subdev_enable_streams() instead. >> * >> + * Drivers that support only a single stream without setting the >> + * V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to >> + * be concerned with the mask argument. >> + * >> * @disable_streams: Disable the streams defined in streams_mask on the given >> * source pad. Subdevs that implement this operation must use the active >> * state management provided by the subdev core (enabled through a call to >> * v4l2_subdev_init_finalize() at initialization time). Do not call >> * directly, use v4l2_subdev_disable_streams() instead. >> + * >> + * Drivers that support only a single stream without setting the >> + * V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to >> + * be concerned with the mask argument. >> */ >> struct v4l2_subdev_pad_ops { >> int (*enum_mbus_code)(struct v4l2_subdev *sd, >
On Tue, Sep 17, 2024 at 05:16:25PM +0300, Tomi Valkeinen wrote: > On 17/09/2024 16:00, Laurent Pinchart wrote: > > On Tue, Sep 17, 2024 at 03:43:45PM +0300, Sakari Ailus wrote: > >> Document the expected {enable,disable}_streams callback behaviour for > >> drivers that are stream-unaware i.e. don't specify the > >> V4L2_SUBDEV_CAP_STREAMS sub-device capability flat. In this specific case, > >> the mask argument can be ignored. > > > > Wouldn't it be better to use BIT(0) in that case to simplifiy > > interoperability with stream-aware devices ? > > The caller has to set BIT(0), but I think here the documentation is > about the callee. > > If the driver is not stream aware and implements the callbacks, it will > get BIT(0) as the mask parameter (do we enforce this?), but as there's > nothing it can do with the parameter it "does not need to be concerned > with the mask argument". Right. I had misunderstood the patch. > >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > >> --- > >> include/media/v4l2-subdev.h | 8 ++++++++ > >> 1 file changed, 8 insertions(+) > >> > >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > >> index 3cc6b4a5935f..67a6e6ec58b8 100644 > >> --- a/include/media/v4l2-subdev.h > >> +++ b/include/media/v4l2-subdev.h > >> @@ -834,11 +834,19 @@ struct v4l2_subdev_state { > >> * v4l2_subdev_init_finalize() at initialization time). Do not call > >> * directly, use v4l2_subdev_enable_streams() instead. > >> * > >> + * Drivers that support only a single stream without setting the > >> + * V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to s/capatility/capability/ Same below. > >> + * be concerned with the mask argument. How about "can ignore the mask argument" instead ? I interpreted as "not need to be concerned with" from the point of view of the caller. > >> + * > >> * @disable_streams: Disable the streams defined in streams_mask on the given > >> * source pad. Subdevs that implement this operation must use the active > >> * state management provided by the subdev core (enabled through a call to > >> * v4l2_subdev_init_finalize() at initialization time). Do not call > >> * directly, use v4l2_subdev_disable_streams() instead. > >> + * > >> + * Drivers that support only a single stream without setting the > >> + * V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to > >> + * be concerned with the mask argument. > >> */ > >> struct v4l2_subdev_pad_ops { > >> int (*enum_mbus_code)(struct v4l2_subdev *sd,
On 17/09/2024 17:57, Laurent Pinchart wrote: > On Tue, Sep 17, 2024 at 05:16:25PM +0300, Tomi Valkeinen wrote: >> On 17/09/2024 16:00, Laurent Pinchart wrote: >>> On Tue, Sep 17, 2024 at 03:43:45PM +0300, Sakari Ailus wrote: >>>> Document the expected {enable,disable}_streams callback behaviour for >>>> drivers that are stream-unaware i.e. don't specify the >>>> V4L2_SUBDEV_CAP_STREAMS sub-device capability flat. In this specific case, >>>> the mask argument can be ignored. >>> >>> Wouldn't it be better to use BIT(0) in that case to simplifiy >>> interoperability with stream-aware devices ? >> >> The caller has to set BIT(0), but I think here the documentation is >> about the callee. >> >> If the driver is not stream aware and implements the callbacks, it will >> get BIT(0) as the mask parameter (do we enforce this?), but as there's >> nothing it can do with the parameter it "does not need to be concerned >> with the mask argument". > > Right. I had misunderstood the patch. > >>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>>> --- >>>> include/media/v4l2-subdev.h | 8 ++++++++ >>>> 1 file changed, 8 insertions(+) >>>> >>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h >>>> index 3cc6b4a5935f..67a6e6ec58b8 100644 >>>> --- a/include/media/v4l2-subdev.h >>>> +++ b/include/media/v4l2-subdev.h >>>> @@ -834,11 +834,19 @@ struct v4l2_subdev_state { >>>> * v4l2_subdev_init_finalize() at initialization time). Do not call >>>> * directly, use v4l2_subdev_enable_streams() instead. >>>> * >>>> + * Drivers that support only a single stream without setting the >>>> + * V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to > > s/capatility/capability/ > > Same below. > >>>> + * be concerned with the mask argument. > > How about "can ignore the mask argument" instead ? I interpreted as "not > need to be concerned with" from the point of view of the caller. Or maybe, to be transparent, "can ignore the mask argument as it is always 1". Tomi
Hi Laurent, On Tue, Sep 17, 2024 at 05:57:35PM +0300, Laurent Pinchart wrote: > On Tue, Sep 17, 2024 at 05:16:25PM +0300, Tomi Valkeinen wrote: > > On 17/09/2024 16:00, Laurent Pinchart wrote: > > > On Tue, Sep 17, 2024 at 03:43:45PM +0300, Sakari Ailus wrote: > > >> Document the expected {enable,disable}_streams callback behaviour for > > >> drivers that are stream-unaware i.e. don't specify the > > >> V4L2_SUBDEV_CAP_STREAMS sub-device capability flat. In this specific case, > > >> the mask argument can be ignored. > > > > > > Wouldn't it be better to use BIT(0) in that case to simplifiy > > > interoperability with stream-aware devices ? > > > > The caller has to set BIT(0), but I think here the documentation is > > about the callee. > > > > If the driver is not stream aware and implements the callbacks, it will > > get BIT(0) as the mask parameter (do we enforce this?), but as there's > > nothing it can do with the parameter it "does not need to be concerned > > with the mask argument". > > Right. I had misunderstood the patch. > > > >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > >> --- > > >> include/media/v4l2-subdev.h | 8 ++++++++ > > >> 1 file changed, 8 insertions(+) > > >> > > >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > > >> index 3cc6b4a5935f..67a6e6ec58b8 100644 > > >> --- a/include/media/v4l2-subdev.h > > >> +++ b/include/media/v4l2-subdev.h > > >> @@ -834,11 +834,19 @@ struct v4l2_subdev_state { > > >> * v4l2_subdev_init_finalize() at initialization time). Do not call > > >> * directly, use v4l2_subdev_enable_streams() instead. > > >> * > > >> + * Drivers that support only a single stream without setting the > > >> + * V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to > > s/capatility/capability/ > > Same below. > > > >> + * be concerned with the mask argument. > > How about "can ignore the mask argument" instead ? I interpreted as "not > need to be concerned with" from the point of view of the caller. Sounds good. I'll address these in v3, after waiting a bit for further comments. > > > >> + * > > >> * @disable_streams: Disable the streams defined in streams_mask on the given > > >> * source pad. Subdevs that implement this operation must use the active > > >> * state management provided by the subdev core (enabled through a call to > > >> * v4l2_subdev_init_finalize() at initialization time). Do not call > > >> * directly, use v4l2_subdev_disable_streams() instead. > > >> + * > > >> + * Drivers that support only a single stream without setting the > > >> + * V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to > > >> + * be concerned with the mask argument. > > >> */ > > >> struct v4l2_subdev_pad_ops { > > >> int (*enum_mbus_code)(struct v4l2_subdev *sd, >
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index 3cc6b4a5935f..67a6e6ec58b8 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -834,11 +834,19 @@ struct v4l2_subdev_state { * v4l2_subdev_init_finalize() at initialization time). Do not call * directly, use v4l2_subdev_enable_streams() instead. * + * Drivers that support only a single stream without setting the + * V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to + * be concerned with the mask argument. + * * @disable_streams: Disable the streams defined in streams_mask on the given * source pad. Subdevs that implement this operation must use the active * state management provided by the subdev core (enabled through a call to * v4l2_subdev_init_finalize() at initialization time). Do not call * directly, use v4l2_subdev_disable_streams() instead. + * + * Drivers that support only a single stream without setting the + * V4L2_SUBDEV_CAP_STREAMS sub-device capatility flag do not need to + * be concerned with the mask argument. */ struct v4l2_subdev_pad_ops { int (*enum_mbus_code)(struct v4l2_subdev *sd,
Document the expected {enable,disable}_streams callback behaviour for drivers that are stream-unaware i.e. don't specify the V4L2_SUBDEV_CAP_STREAMS sub-device capability flat. In this specific case, the mask argument can be ignored. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- include/media/v4l2-subdev.h | 8 ++++++++ 1 file changed, 8 insertions(+)