| Message ID | 20231003120813.77726-9-sakari.ailus@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Generic line based metadata support, internal pads | expand |
On 03/10/2023 14:08, Sakari Ailus wrote: > On VIDIOC_SUBDEV_[GS]_ROUTING, only return as many routes back to the user > as there's room. Do not consider it an error if more routes existed. > Simply inform the user there are more routes. I'm not convinced by this change. Can userspace do anything useful with only a partially filled routes array? Just silently allowing bad data is asking for problems, IMHO. Regards, Hans > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > .../userspace-api/media/v4l/vidioc-subdev-g-routing.rst | 4 ---- > drivers/media/v4l2-core/v4l2-subdev.c | 8 ++------ > 2 files changed, 2 insertions(+), 10 deletions(-) > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst > index ced53ea5f23c..99d3c15fd759 100644 > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst > @@ -145,10 +145,6 @@ On success 0 is returned, on error -1 and the ``errno`` variable is set > appropriately. The generic error codes are described at the > :ref:`Generic Error Codes <gen-errors>` chapter. > > -ENOSPC > - The application provided ``num_routes`` is not big enough to contain > - all the available routes the subdevice exposes. > - > EINVAL > The sink or source pad identifiers reference a non-existing pad, or reference > pads of different types (ie. the sink_pad identifiers refers to a source pad). > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 9a34e13dfd96..dd48e7e549fb 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -956,14 +956,10 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, > > krouting = &state->routing; > > - if (routing->len_routes < krouting->num_routes) { > - routing->num_routes = krouting->num_routes; > - return -ENOSPC; > - } > - > memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes, > krouting->routes, > - krouting->num_routes * sizeof(*krouting->routes)); > + min(krouting->num_routes, routing->len_routes) * > + sizeof(*krouting->routes)); > routing->num_routes = krouting->num_routes; > > return 0;
On 03/10/2023 15:08, Sakari Ailus wrote: > On VIDIOC_SUBDEV_[GS]_ROUTING, only return as many routes back to the user > as there's room. Do not consider it an error if more routes existed. > Simply inform the user there are more routes. Inform how? And I agree with Hans here. How about return ENOSPC, but the kernel fills in num_routes to tell the userspace how many there actually are? Tomi > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > .../userspace-api/media/v4l/vidioc-subdev-g-routing.rst | 4 ---- > drivers/media/v4l2-core/v4l2-subdev.c | 8 ++------ > 2 files changed, 2 insertions(+), 10 deletions(-) > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst > index ced53ea5f23c..99d3c15fd759 100644 > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst > @@ -145,10 +145,6 @@ On success 0 is returned, on error -1 and the ``errno`` variable is set > appropriately. The generic error codes are described at the > :ref:`Generic Error Codes <gen-errors>` chapter. > > -ENOSPC > - The application provided ``num_routes`` is not big enough to contain > - all the available routes the subdevice exposes. > - > EINVAL > The sink or source pad identifiers reference a non-existing pad, or reference > pads of different types (ie. the sink_pad identifiers refers to a source pad). > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 9a34e13dfd96..dd48e7e549fb 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -956,14 +956,10 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, > > krouting = &state->routing; > > - if (routing->len_routes < krouting->num_routes) { > - routing->num_routes = krouting->num_routes; > - return -ENOSPC; > - } > - > memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes, > krouting->routes, > - krouting->num_routes * sizeof(*krouting->routes)); > + min(krouting->num_routes, routing->len_routes) * > + sizeof(*krouting->routes)); > routing->num_routes = krouting->num_routes; > > return 0;
Hi Hans, On Thu, Oct 05, 2023 at 01:08:25PM +0200, Hans Verkuil wrote: > On 03/10/2023 14:08, Sakari Ailus wrote: > > On VIDIOC_SUBDEV_[GS]_ROUTING, only return as many routes back to the user > > as there's room. Do not consider it an error if more routes existed. > > Simply inform the user there are more routes. > > I'm not convinced by this change. Can userspace do anything useful with only > a partially filled routes array? > > Just silently allowing bad data is asking for problems, IMHO. The idea is that the driver may return more routes than the user sets but the user may be interested in just one route, the one it enabled. Should the user space be required to obtain streams even if they are not of interest?
On Mon, Oct 09, 2023 at 01:56:21PM +0300, Tomi Valkeinen wrote: > On 03/10/2023 15:08, Sakari Ailus wrote: > > On VIDIOC_SUBDEV_[GS]_ROUTING, only return as many routes back to the user > > as there's room. Do not consider it an error if more routes existed. > > Simply inform the user there are more routes. > > Inform how? And I agree with Hans here. How about return ENOSPC, but the > kernel fills in num_routes to tell the userspace how many there actually > are? For VIDIOC_SUBDEV_G_ROUTING I have no objection. For VIDIOC_SUBDEV_S_ROUTING, however, I would prefer the ioctl to succeed if the routes can be applied but there's not enough space to return them all to the application. The application may not have an interest in getting the applied routes back. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > .../userspace-api/media/v4l/vidioc-subdev-g-routing.rst | 4 ---- > > drivers/media/v4l2-core/v4l2-subdev.c | 8 ++------ > > 2 files changed, 2 insertions(+), 10 deletions(-) > > > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst > > index ced53ea5f23c..99d3c15fd759 100644 > > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst > > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst > > @@ -145,10 +145,6 @@ On success 0 is returned, on error -1 and the ``errno`` variable is set > > appropriately. The generic error codes are described at the > > :ref:`Generic Error Codes <gen-errors>` chapter. > > > > -ENOSPC > > - The application provided ``num_routes`` is not big enough to contain > > - all the available routes the subdevice exposes. > > - > > EINVAL > > The sink or source pad identifiers reference a non-existing pad, or reference > > pads of different types (ie. the sink_pad identifiers refers to a source pad). > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > index 9a34e13dfd96..dd48e7e549fb 100644 > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > @@ -956,14 +956,10 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, > > > > krouting = &state->routing; > > > > - if (routing->len_routes < krouting->num_routes) { > > - routing->num_routes = krouting->num_routes; > > - return -ENOSPC; > > - } > > - > > memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes, > > krouting->routes, > > - krouting->num_routes * sizeof(*krouting->routes)); > > + min(krouting->num_routes, routing->len_routes) * > > + sizeof(*krouting->routes)); > > routing->num_routes = krouting->num_routes; > > > > return 0;
Hi Hans, On Fri, Oct 27, 2023 at 03:13:55PM +0200, Hans Verkuil wrote: > On 25/10/2023 23:07, Laurent Pinchart wrote: > > On Mon, Oct 09, 2023 at 01:56:21PM +0300, Tomi Valkeinen wrote: > >> On 03/10/2023 15:08, Sakari Ailus wrote: > >>> On VIDIOC_SUBDEV_[GS]_ROUTING, only return as many routes back to the user > >>> as there's room. Do not consider it an error if more routes existed. > >>> Simply inform the user there are more routes. > >> > >> Inform how? And I agree with Hans here. How about return ENOSPC, but the > >> kernel fills in num_routes to tell the userspace how many there actually > >> are? > > > > For VIDIOC_SUBDEV_G_ROUTING I have no objection. For > > VIDIOC_SUBDEV_S_ROUTING, however, I would prefer the ioctl to succeed if > > the routes can be applied but there's not enough space to return them > > all to the application. The application may not have an interest in > > getting the applied routes back. > > For S_ROUTING, do we still want to update num_routes in that case? Even > though we return 0 since we could actually set the routes. num_routes should be updated in this case, otherwise the caller won't know there are more routes. The caller can then decide what to do about it, to get the routes using G_ROUTING for instance: the length of the routing table is now known. > > I think it depends a bit on the naming of these fields in v7. I recall there was a comment on the naming now, possibly from you, but I can't find the comments at the moment. :-I I'm open to renaming them but I don't think the current naming is bad either. > > How likely is it that an application would run into this anyway? I suspect a > typical app will get the routes first, then modify it. Laurent can probably comment on this from libcamera perspective, but I presume that most users of routes are somehow specific to the device. I would expect source sub-devices that produce the streams are typical places where you may have dependencies between streams, but they do exist elsewhere (but there are very few cases). > > It's worth giving this a try, but it depends a bit on how easy it is to > document it. If you need to jump through hoops to try to explain it to an > end user, then perhaps this is overly complicated. The documentation is changed in patch "media: v4l: subdev: Add len_routes field to struct v4l2_subdev_routing", two patches prior to this. The changes are split across several patches in order to avoid fewer difficult to review patches. I'll change the prefix of this patch to "v4l: subdev:" as well.
diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst index ced53ea5f23c..99d3c15fd759 100644 --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst @@ -145,10 +145,6 @@ On success 0 is returned, on error -1 and the ``errno`` variable is set appropriately. The generic error codes are described at the :ref:`Generic Error Codes <gen-errors>` chapter. -ENOSPC - The application provided ``num_routes`` is not big enough to contain - all the available routes the subdevice exposes. - EINVAL The sink or source pad identifiers reference a non-existing pad, or reference pads of different types (ie. the sink_pad identifiers refers to a source pad). diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 9a34e13dfd96..dd48e7e549fb 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -956,14 +956,10 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, krouting = &state->routing; - if (routing->len_routes < krouting->num_routes) { - routing->num_routes = krouting->num_routes; - return -ENOSPC; - } - memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes, krouting->routes, - krouting->num_routes * sizeof(*krouting->routes)); + min(krouting->num_routes, routing->len_routes) * + sizeof(*krouting->routes)); routing->num_routes = krouting->num_routes; return 0;
On VIDIOC_SUBDEV_[GS]_ROUTING, only return as many routes back to the user as there's room. Do not consider it an error if more routes existed. Simply inform the user there are more routes. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- .../userspace-api/media/v4l/vidioc-subdev-g-routing.rst | 4 ---- drivers/media/v4l2-core/v4l2-subdev.c | 8 ++------ 2 files changed, 2 insertions(+), 10 deletions(-)