* [PATCH 1/3] media: subdev: Drop implicit zeroing of stream field
@ 2023-06-19 11:27 Tomi Valkeinen
2023-06-19 11:27 ` [PATCH 2/3] media: subdev: Constify v4l2_subdev_set_routing_with_fmt() param Tomi Valkeinen
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Tomi Valkeinen @ 2023-06-19 11:27 UTC (permalink / raw)
To: linux-media, linux-kernel, Mauro Carvalho Chehab, Sakari Ailus,
Laurent Pinchart, Jacopo Mondi
Cc: Tomi Valkeinen
Now that the kernel drivers have been fixed to initialize the stream
field, and we have the client capability which the userspace uses to say
it has initialized the stream field, we can drop the implicit zeroing of
the stream field in the various check functions.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
drivers/media/v4l2-core/v4l2-subdev.c | 15 ---------------
1 file changed, 15 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 2ec179cd1264..c1ac6d7a63d2 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -200,9 +200,6 @@ static inline int check_format(struct v4l2_subdev *sd,
if (!format)
return -EINVAL;
- if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS))
- format->stream = 0;
-
return check_which(format->which) ? : check_pad(sd, format->pad) ? :
check_state(sd, state, format->which, format->pad, format->stream);
}
@@ -230,9 +227,6 @@ static int call_enum_mbus_code(struct v4l2_subdev *sd,
if (!code)
return -EINVAL;
- if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS))
- code->stream = 0;
-
return check_which(code->which) ? : check_pad(sd, code->pad) ? :
check_state(sd, state, code->which, code->pad, code->stream) ? :
sd->ops->pad->enum_mbus_code(sd, state, code);
@@ -245,9 +239,6 @@ static int call_enum_frame_size(struct v4l2_subdev *sd,
if (!fse)
return -EINVAL;
- if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS))
- fse->stream = 0;
-
return check_which(fse->which) ? : check_pad(sd, fse->pad) ? :
check_state(sd, state, fse->which, fse->pad, fse->stream) ? :
sd->ops->pad->enum_frame_size(sd, state, fse);
@@ -283,9 +274,6 @@ static int call_enum_frame_interval(struct v4l2_subdev *sd,
if (!fie)
return -EINVAL;
- if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS))
- fie->stream = 0;
-
return check_which(fie->which) ? : check_pad(sd, fie->pad) ? :
check_state(sd, state, fie->which, fie->pad, fie->stream) ? :
sd->ops->pad->enum_frame_interval(sd, state, fie);
@@ -298,9 +286,6 @@ static inline int check_selection(struct v4l2_subdev *sd,
if (!sel)
return -EINVAL;
- if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS))
- sel->stream = 0;
-
return check_which(sel->which) ? : check_pad(sd, sel->pad) ? :
check_state(sd, state, sel->which, sel->pad, sel->stream);
}
--
2.34.1
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 2/3] media: subdev: Constify v4l2_subdev_set_routing_with_fmt() param 2023-06-19 11:27 [PATCH 1/3] media: subdev: Drop implicit zeroing of stream field Tomi Valkeinen @ 2023-06-19 11:27 ` Tomi Valkeinen 2023-07-17 14:12 ` Jacopo Mondi 2023-06-19 11:27 ` [PATCH 3/3] media: subdev: Add debug prints to enable/disable_streams Tomi Valkeinen 2023-07-17 14:11 ` [PATCH 1/3] media: subdev: Drop implicit zeroing of stream field Jacopo Mondi 2 siblings, 1 reply; 8+ messages in thread From: Tomi Valkeinen @ 2023-06-19 11:27 UTC (permalink / raw) To: linux-media, linux-kernel, Mauro Carvalho Chehab, Sakari Ailus, Laurent Pinchart, Jacopo Mondi Cc: Tomi Valkeinen The routing parameter of v4l2_subdev_set_routing_with_fmt() is missing 'const'. Add it. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/media/v4l2-core/v4l2-subdev.c | 2 +- include/media/v4l2-subdev.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index c1ac6d7a63d2..73f716a42569 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -1590,7 +1590,7 @@ EXPORT_SYMBOL_GPL(__v4l2_subdev_next_active_route); int v4l2_subdev_set_routing_with_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state, - struct v4l2_subdev_krouting *routing, + const struct v4l2_subdev_krouting *routing, const struct v4l2_mbus_framefmt *fmt) { struct v4l2_subdev_stream_configs *stream_configs; diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index b325df0d54d6..ca0bacb88537 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -1532,7 +1532,7 @@ __v4l2_subdev_next_active_route(const struct v4l2_subdev_krouting *routing, */ int v4l2_subdev_set_routing_with_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state, - struct v4l2_subdev_krouting *routing, + const struct v4l2_subdev_krouting *routing, const struct v4l2_mbus_framefmt *fmt); /** -- 2.34.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] media: subdev: Constify v4l2_subdev_set_routing_with_fmt() param 2023-06-19 11:27 ` [PATCH 2/3] media: subdev: Constify v4l2_subdev_set_routing_with_fmt() param Tomi Valkeinen @ 2023-07-17 14:12 ` Jacopo Mondi 0 siblings, 0 replies; 8+ messages in thread From: Jacopo Mondi @ 2023-07-17 14:12 UTC (permalink / raw) To: Tomi Valkeinen Cc: linux-media, linux-kernel, Mauro Carvalho Chehab, Sakari Ailus, Laurent Pinchart, Jacopo Mondi Hi Tomi On Mon, Jun 19, 2023 at 02:27:06PM +0300, Tomi Valkeinen wrote: > The routing parameter of v4l2_subdev_set_routing_with_fmt() is missing > 'const'. Add it. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 2 +- > include/media/v4l2-subdev.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index c1ac6d7a63d2..73f716a42569 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -1590,7 +1590,7 @@ EXPORT_SYMBOL_GPL(__v4l2_subdev_next_active_route); > > int v4l2_subdev_set_routing_with_fmt(struct v4l2_subdev *sd, > struct v4l2_subdev_state *state, > - struct v4l2_subdev_krouting *routing, > + const struct v4l2_subdev_krouting *routing, > const struct v4l2_mbus_framefmt *fmt) > { > struct v4l2_subdev_stream_configs *stream_configs; > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index b325df0d54d6..ca0bacb88537 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -1532,7 +1532,7 @@ __v4l2_subdev_next_active_route(const struct v4l2_subdev_krouting *routing, > */ > int v4l2_subdev_set_routing_with_fmt(struct v4l2_subdev *sd, > struct v4l2_subdev_state *state, > - struct v4l2_subdev_krouting *routing, > + const struct v4l2_subdev_krouting *routing, > const struct v4l2_mbus_framefmt *fmt); > > /** > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] media: subdev: Add debug prints to enable/disable_streams 2023-06-19 11:27 [PATCH 1/3] media: subdev: Drop implicit zeroing of stream field Tomi Valkeinen 2023-06-19 11:27 ` [PATCH 2/3] media: subdev: Constify v4l2_subdev_set_routing_with_fmt() param Tomi Valkeinen @ 2023-06-19 11:27 ` Tomi Valkeinen 2023-07-17 14:18 ` Jacopo Mondi 2023-07-17 14:11 ` [PATCH 1/3] media: subdev: Drop implicit zeroing of stream field Jacopo Mondi 2 siblings, 1 reply; 8+ messages in thread From: Tomi Valkeinen @ 2023-06-19 11:27 UTC (permalink / raw) To: linux-media, linux-kernel, Mauro Carvalho Chehab, Sakari Ailus, Laurent Pinchart, Jacopo Mondi Cc: Tomi Valkeinen It is often useful to see when streaming for a device is being enabled or disabled. Add debug prints for this to v4l2_subdev_enable_streams() and v4l2_subdev_disable_streams(). Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/media/v4l2-core/v4l2-subdev.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 73f716a42569..0f86a165b202 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -1977,11 +1977,16 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad, goto done; } + dev_dbg(dev, "enable streams %u:%#llx\n", pad, streams_mask); + /* Call the .enable_streams() operation. */ ret = v4l2_subdev_call(sd, pad, enable_streams, state, pad, streams_mask); - if (ret) + if (ret) { + dev_dbg(dev, "enable streams %u:%#llx failed: %d\n", pad, + streams_mask, ret); goto done; + } /* Mark the streams as enabled. */ for (i = 0; i < state->stream_configs.num_configs; ++i) { @@ -2089,11 +2094,16 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad, goto done; } + dev_dbg(dev, "disable streams %u:%#llx\n", pad, streams_mask); + /* Call the .disable_streams() operation. */ ret = v4l2_subdev_call(sd, pad, disable_streams, state, pad, streams_mask); - if (ret) + if (ret) { + dev_dbg(dev, "disable streams %u:%#llx failed: %d\n", pad, + streams_mask, ret); goto done; + } /* Mark the streams as disabled. */ for (i = 0; i < state->stream_configs.num_configs; ++i) { -- 2.34.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] media: subdev: Add debug prints to enable/disable_streams 2023-06-19 11:27 ` [PATCH 3/3] media: subdev: Add debug prints to enable/disable_streams Tomi Valkeinen @ 2023-07-17 14:18 ` Jacopo Mondi 0 siblings, 0 replies; 8+ messages in thread From: Jacopo Mondi @ 2023-07-17 14:18 UTC (permalink / raw) To: Tomi Valkeinen Cc: linux-media, linux-kernel, Mauro Carvalho Chehab, Sakari Ailus, Laurent Pinchart, Jacopo Mondi Hi Tomi On Mon, Jun 19, 2023 at 02:27:07PM +0300, Tomi Valkeinen wrote: > It is often useful to see when streaming for a device is being enabled > or disabled. Add debug prints for this to v4l2_subdev_enable_streams() > and v4l2_subdev_disable_streams(). > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 73f716a42569..0f86a165b202 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -1977,11 +1977,16 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad, > goto done; > } > > + dev_dbg(dev, "enable streams %u:%#llx\n", pad, streams_mask); With the use of the # incantation clarified offline: Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > + > /* Call the .enable_streams() operation. */ > ret = v4l2_subdev_call(sd, pad, enable_streams, state, pad, > streams_mask); > - if (ret) > + if (ret) { > + dev_dbg(dev, "enable streams %u:%#llx failed: %d\n", pad, > + streams_mask, ret); > goto done; > + } > > /* Mark the streams as enabled. */ > for (i = 0; i < state->stream_configs.num_configs; ++i) { > @@ -2089,11 +2094,16 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad, > goto done; > } > > + dev_dbg(dev, "disable streams %u:%#llx\n", pad, streams_mask); > + > /* Call the .disable_streams() operation. */ > ret = v4l2_subdev_call(sd, pad, disable_streams, state, pad, > streams_mask); > - if (ret) > + if (ret) { > + dev_dbg(dev, "disable streams %u:%#llx failed: %d\n", pad, > + streams_mask, ret); > goto done; > + } > > /* Mark the streams as disabled. */ > for (i = 0; i < state->stream_configs.num_configs; ++i) { > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] media: subdev: Drop implicit zeroing of stream field 2023-06-19 11:27 [PATCH 1/3] media: subdev: Drop implicit zeroing of stream field Tomi Valkeinen 2023-06-19 11:27 ` [PATCH 2/3] media: subdev: Constify v4l2_subdev_set_routing_with_fmt() param Tomi Valkeinen 2023-06-19 11:27 ` [PATCH 3/3] media: subdev: Add debug prints to enable/disable_streams Tomi Valkeinen @ 2023-07-17 14:11 ` Jacopo Mondi 2023-07-17 14:39 ` Tomi Valkeinen 2 siblings, 1 reply; 8+ messages in thread From: Jacopo Mondi @ 2023-07-17 14:11 UTC (permalink / raw) To: Tomi Valkeinen Cc: linux-media, linux-kernel, Mauro Carvalho Chehab, Sakari Ailus, Laurent Pinchart, Jacopo Mondi Hi Tomi On Mon, Jun 19, 2023 at 02:27:05PM +0300, Tomi Valkeinen wrote: > Now that the kernel drivers have been fixed to initialize the stream > field, and we have the client capability which the userspace uses to say Not sure I got this. Isn't the capabilities flag intended for drivers to tell userspace it support streams ? This seems to suggest it is userspace setting it ? > it has initialized the stream field, we can drop the implicit zeroing of > the stream field in the various check functions. > I guess this is safe, but I'm not sure why it wasn't before. If a driver doesn't support streams (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) then it should have ignored the 'stream' field even if it wasn't zeroed. So I suspect I am missing the reason for zeroing in first place... > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 15 --------------- > 1 file changed, 15 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 2ec179cd1264..c1ac6d7a63d2 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -200,9 +200,6 @@ static inline int check_format(struct v4l2_subdev *sd, > if (!format) > return -EINVAL; > > - if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) > - format->stream = 0; > - > return check_which(format->which) ? : check_pad(sd, format->pad) ? : > check_state(sd, state, format->which, format->pad, format->stream); > } > @@ -230,9 +227,6 @@ static int call_enum_mbus_code(struct v4l2_subdev *sd, > if (!code) > return -EINVAL; > > - if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) > - code->stream = 0; > - > return check_which(code->which) ? : check_pad(sd, code->pad) ? : > check_state(sd, state, code->which, code->pad, code->stream) ? : > sd->ops->pad->enum_mbus_code(sd, state, code); > @@ -245,9 +239,6 @@ static int call_enum_frame_size(struct v4l2_subdev *sd, > if (!fse) > return -EINVAL; > > - if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) > - fse->stream = 0; > - > return check_which(fse->which) ? : check_pad(sd, fse->pad) ? : > check_state(sd, state, fse->which, fse->pad, fse->stream) ? : > sd->ops->pad->enum_frame_size(sd, state, fse); > @@ -283,9 +274,6 @@ static int call_enum_frame_interval(struct v4l2_subdev *sd, > if (!fie) > return -EINVAL; > > - if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) > - fie->stream = 0; > - > return check_which(fie->which) ? : check_pad(sd, fie->pad) ? : > check_state(sd, state, fie->which, fie->pad, fie->stream) ? : > sd->ops->pad->enum_frame_interval(sd, state, fie); > @@ -298,9 +286,6 @@ static inline int check_selection(struct v4l2_subdev *sd, > if (!sel) > return -EINVAL; > > - if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) > - sel->stream = 0; > - > return check_which(sel->which) ? : check_pad(sd, sel->pad) ? : > check_state(sd, state, sel->which, sel->pad, sel->stream); > } > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] media: subdev: Drop implicit zeroing of stream field 2023-07-17 14:11 ` [PATCH 1/3] media: subdev: Drop implicit zeroing of stream field Jacopo Mondi @ 2023-07-17 14:39 ` Tomi Valkeinen 2023-07-17 14:51 ` Jacopo Mondi 0 siblings, 1 reply; 8+ messages in thread From: Tomi Valkeinen @ 2023-07-17 14:39 UTC (permalink / raw) To: Jacopo Mondi Cc: linux-media, linux-kernel, Mauro Carvalho Chehab, Sakari Ailus, Laurent Pinchart On 17/07/2023 17:11, Jacopo Mondi wrote: > Hi Tomi > > On Mon, Jun 19, 2023 at 02:27:05PM +0300, Tomi Valkeinen wrote: >> Now that the kernel drivers have been fixed to initialize the stream >> field, and we have the client capability which the userspace uses to say > > Not sure I got this. Isn't the capabilities flag intended for drivers > to tell userspace it support streams ? This seems to suggest it is > userspace setting it ? Client capabilities tell the capabilities of the client. It's the new VIDIOC_SUBDEV_S_CLIENT_CAP/VIDIOC_SUBDEV_G_CLIENT_CAP ioctl. >> it has initialized the stream field, we can drop the implicit zeroing of >> the stream field in the various check functions. >> > > I guess this is safe, but I'm not sure why it wasn't before. If a > driver doesn't support streams (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) > then it should have ignored the 'stream' field even if it wasn't > zeroed. So I suspect I am missing the reason for zeroing in first > place... The code being removed here was a quick fix. The issue was that before we had the client capability flag for "userspace supports streams", the 'stream' field could contain garbage. Also some kernel drivers were not properly initializing struct v4l2_subdev_format to zero, so again the 'stream' field could contain garbage. The code removed here made sure that if a non-streams-supporting device was used, the 'stream' field would be zero as expected, and the v4l2 framework would not get confused by seeing a non-zero stream. The non-streams-enabled drivers themselves would not use the field anyway, of course, but the framework has code that expects the 'stream' to be zero (e.g. check_state() checks that stream == 0 if the device hasn't set V4L2_SUBDEV_FL_STREAMS). Now the kernel drivers have been fixed to initialize the struct properly, and we have the VIDIOC_SUBDEV_S_CLIENT_CAP to handle the userspace part. Thus this code is no longer needed, and, I think, just might confused the reader. And, in fact, I think it might hide an error. If a subdev is used that does not support streams, but the userspace supports streams. If the userspace uses an ioctl with stream != 0 for that subdev, it's clearly an error. However, with the code removed here, the error would go unnoticed as the kernel clears the stream field. Tomi >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> drivers/media/v4l2-core/v4l2-subdev.c | 15 --------------- >> 1 file changed, 15 deletions(-) >> >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c >> index 2ec179cd1264..c1ac6d7a63d2 100644 >> --- a/drivers/media/v4l2-core/v4l2-subdev.c >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >> @@ -200,9 +200,6 @@ static inline int check_format(struct v4l2_subdev *sd, >> if (!format) >> return -EINVAL; >> >> - if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) >> - format->stream = 0; >> - >> return check_which(format->which) ? : check_pad(sd, format->pad) ? : >> check_state(sd, state, format->which, format->pad, format->stream); >> } >> @@ -230,9 +227,6 @@ static int call_enum_mbus_code(struct v4l2_subdev *sd, >> if (!code) >> return -EINVAL; >> >> - if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) >> - code->stream = 0; >> - >> return check_which(code->which) ? : check_pad(sd, code->pad) ? : >> check_state(sd, state, code->which, code->pad, code->stream) ? : >> sd->ops->pad->enum_mbus_code(sd, state, code); >> @@ -245,9 +239,6 @@ static int call_enum_frame_size(struct v4l2_subdev *sd, >> if (!fse) >> return -EINVAL; >> >> - if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) >> - fse->stream = 0; >> - >> return check_which(fse->which) ? : check_pad(sd, fse->pad) ? : >> check_state(sd, state, fse->which, fse->pad, fse->stream) ? : >> sd->ops->pad->enum_frame_size(sd, state, fse); >> @@ -283,9 +274,6 @@ static int call_enum_frame_interval(struct v4l2_subdev *sd, >> if (!fie) >> return -EINVAL; >> >> - if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) >> - fie->stream = 0; >> - >> return check_which(fie->which) ? : check_pad(sd, fie->pad) ? : >> check_state(sd, state, fie->which, fie->pad, fie->stream) ? : >> sd->ops->pad->enum_frame_interval(sd, state, fie); >> @@ -298,9 +286,6 @@ static inline int check_selection(struct v4l2_subdev *sd, >> if (!sel) >> return -EINVAL; >> >> - if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) >> - sel->stream = 0; >> - >> return check_which(sel->which) ? : check_pad(sd, sel->pad) ? : >> check_state(sd, state, sel->which, sel->pad, sel->stream); >> } >> -- >> 2.34.1 >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] media: subdev: Drop implicit zeroing of stream field 2023-07-17 14:39 ` Tomi Valkeinen @ 2023-07-17 14:51 ` Jacopo Mondi 0 siblings, 0 replies; 8+ messages in thread From: Jacopo Mondi @ 2023-07-17 14:51 UTC (permalink / raw) To: Tomi Valkeinen Cc: Jacopo Mondi, linux-media, linux-kernel, Mauro Carvalho Chehab, Sakari Ailus, Laurent Pinchart Hi Tomi On Mon, Jul 17, 2023 at 05:39:21PM +0300, Tomi Valkeinen wrote: > On 17/07/2023 17:11, Jacopo Mondi wrote: > > Hi Tomi > > > > On Mon, Jun 19, 2023 at 02:27:05PM +0300, Tomi Valkeinen wrote: > > > Now that the kernel drivers have been fixed to initialize the stream > > > field, and we have the client capability which the userspace uses to say > > > > Not sure I got this. Isn't the capabilities flag intended for drivers > > to tell userspace it support streams ? This seems to suggest it is > > userspace setting it ? > > Client capabilities tell the capabilities of the client. It's the new > VIDIOC_SUBDEV_S_CLIENT_CAP/VIDIOC_SUBDEV_G_CLIENT_CAP ioctl. > > > > it has initialized the stream field, we can drop the implicit zeroing of > > > the stream field in the various check functions. > > > > > > > I guess this is safe, but I'm not sure why it wasn't before. If a > > driver doesn't support streams (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) > > then it should have ignored the 'stream' field even if it wasn't > > zeroed. So I suspect I am missing the reason for zeroing in first > > place... > > The code being removed here was a quick fix. The issue was that before we > had the client capability flag for "userspace supports streams", the > 'stream' field could contain garbage. Also some kernel drivers were not > properly initializing struct v4l2_subdev_format to zero, so again the > 'stream' field could contain garbage. > > The code removed here made sure that if a non-streams-supporting device was > used, the 'stream' field would be zero as expected, and the v4l2 framework > would not get confused by seeing a non-zero stream. The non-streams-enabled > drivers themselves would not use the field anyway, of course, but the > framework has code that expects the 'stream' to be zero (e.g. check_state() > checks that stream == 0 if the device hasn't set V4L2_SUBDEV_FL_STREAMS). > > Now the kernel drivers have been fixed to initialize the struct properly, > and we have the VIDIOC_SUBDEV_S_CLIENT_CAP to handle the userspace part. > Thus this code is no longer needed, and, I think, just might confused the > reader. > > And, in fact, I think it might hide an error. If a subdev is used that does > not support streams, but the userspace supports streams. If the userspace > uses an ioctl with stream != 0 for that subdev, it's clearly an error. > However, with the code removed here, the error would go unnoticed as the > kernel clears the stream field. > Thanks for the clarifications! I admit I missed VIDIOC_SUBDEV_S_CLIENT_CAP! Now it makes sense Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > Tomi > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > > --- > > > drivers/media/v4l2-core/v4l2-subdev.c | 15 --------------- > > > 1 file changed, 15 deletions(-) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > > index 2ec179cd1264..c1ac6d7a63d2 100644 > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > > @@ -200,9 +200,6 @@ static inline int check_format(struct v4l2_subdev *sd, > > > if (!format) > > > return -EINVAL; > > > > > > - if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) > > > - format->stream = 0; > > > - > > > return check_which(format->which) ? : check_pad(sd, format->pad) ? : > > > check_state(sd, state, format->which, format->pad, format->stream); > > > } > > > @@ -230,9 +227,6 @@ static int call_enum_mbus_code(struct v4l2_subdev *sd, > > > if (!code) > > > return -EINVAL; > > > > > > - if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) > > > - code->stream = 0; > > > - > > > return check_which(code->which) ? : check_pad(sd, code->pad) ? : > > > check_state(sd, state, code->which, code->pad, code->stream) ? : > > > sd->ops->pad->enum_mbus_code(sd, state, code); > > > @@ -245,9 +239,6 @@ static int call_enum_frame_size(struct v4l2_subdev *sd, > > > if (!fse) > > > return -EINVAL; > > > > > > - if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) > > > - fse->stream = 0; > > > - > > > return check_which(fse->which) ? : check_pad(sd, fse->pad) ? : > > > check_state(sd, state, fse->which, fse->pad, fse->stream) ? : > > > sd->ops->pad->enum_frame_size(sd, state, fse); > > > @@ -283,9 +274,6 @@ static int call_enum_frame_interval(struct v4l2_subdev *sd, > > > if (!fie) > > > return -EINVAL; > > > > > > - if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) > > > - fie->stream = 0; > > > - > > > return check_which(fie->which) ? : check_pad(sd, fie->pad) ? : > > > check_state(sd, state, fie->which, fie->pad, fie->stream) ? : > > > sd->ops->pad->enum_frame_interval(sd, state, fie); > > > @@ -298,9 +286,6 @@ static inline int check_selection(struct v4l2_subdev *sd, > > > if (!sel) > > > return -EINVAL; > > > > > > - if (!(sd->flags & V4L2_SUBDEV_FL_STREAMS)) > > > - sel->stream = 0; > > > - > > > return check_which(sel->which) ? : check_pad(sd, sel->pad) ? : > > > check_state(sd, state, sel->which, sel->pad, sel->stream); > > > } > > > -- > > > 2.34.1 > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-07-17 14:51 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-06-19 11:27 [PATCH 1/3] media: subdev: Drop implicit zeroing of stream field Tomi Valkeinen 2023-06-19 11:27 ` [PATCH 2/3] media: subdev: Constify v4l2_subdev_set_routing_with_fmt() param Tomi Valkeinen 2023-07-17 14:12 ` Jacopo Mondi 2023-06-19 11:27 ` [PATCH 3/3] media: subdev: Add debug prints to enable/disable_streams Tomi Valkeinen 2023-07-17 14:18 ` Jacopo Mondi 2023-07-17 14:11 ` [PATCH 1/3] media: subdev: Drop implicit zeroing of stream field Jacopo Mondi 2023-07-17 14:39 ` Tomi Valkeinen 2023-07-17 14:51 ` Jacopo Mondi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®