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@xxxxxxxxxxxxxxxx> Thanks j > Tomi > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx> > > > --- > > > 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 > > > >
