| Message ID | 20181101233144.31507-17-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | v4l: add support for multiplexed streams | expand |
Hi Niklas, Thank you for the patch. On Fri, Nov 02, 2018 at 12:31:30AM +0100, Niklas Söderlund wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > > - Add sink and source streams for multiplexed links > - Copy the argument back in case of an error. This is needed to let the > caller know the number of routes. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/media/v4l2-core/v4l2-ioctl.c | 20 +++++++++++++- > drivers/media/v4l2-core/v4l2-subdev.c | 28 +++++++++++++++++++ > include/media/v4l2-subdev.h | 7 +++++ > include/uapi/linux/v4l2-subdev.h | 40 +++++++++++++++++++++++++++ Missing documentation :-( > 4 files changed, 94 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > index 7de041bae84fb2f2..40406acb51ec0906 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -19,6 +19,7 @@ > #include <linux/kernel.h> > #include <linux/version.h> > > +#include <linux/v4l2-subdev.h> > #include <linux/videodev2.h> > > #include <media/v4l2-common.h> > @@ -2924,6 +2925,23 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size, > } > break; > } > + > + case VIDIOC_SUBDEV_G_ROUTING: > + case VIDIOC_SUBDEV_S_ROUTING: { > + struct v4l2_subdev_routing *route = parg; > + > + if (route->num_routes > 0) { > + if (route->num_routes > 256) > + return -EINVAL; > + > + *user_ptr = (void __user *)route->routes; > + *kernel_ptr = (void *)&route->routes; > + *array_size = sizeof(struct v4l2_subdev_route) > + * route->num_routes; > + ret = 1; > + } > + break; > + } > } > > return ret; > @@ -3033,7 +3051,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg, > * Some ioctls can return an error, but still have valid > * results that must be returned. > */ > - if (err < 0 && !always_copy) > + if (err < 0 && !always_copy && cmd != VIDIOC_SUBDEV_G_ROUTING) This seems like a hack. Shouldn't VIDIOC_SUBDEV_G_ROUTING set always_copy instead ? > goto out; > > out_array_args: > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 792f41dffe2329b9..1d3b37cf548fa533 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -516,7 +516,35 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > case VIDIOC_SUBDEV_QUERYSTD: > return v4l2_subdev_call(sd, video, querystd, arg); > + > + case VIDIOC_SUBDEV_G_ROUTING: > + return v4l2_subdev_call(sd, pad, get_routing, arg); > + > + case VIDIOC_SUBDEV_S_ROUTING: { > + struct v4l2_subdev_routing *route = arg; > + unsigned int i; > + > + if (route->num_routes > sd->entity.num_pads) > + return -EINVAL; > + > + for (i = 0; i < route->num_routes; ++i) { > + unsigned int sink = route->routes[i].sink_pad; > + unsigned int source = route->routes[i].source_pad; > + struct media_pad *pads = sd->entity.pads; > + > + if (sink >= sd->entity.num_pads || > + source >= sd->entity.num_pads) > + return -EINVAL; > + > + if (!(pads[sink].flags & MEDIA_PAD_FL_SINK) || > + !(pads[source].flags & MEDIA_PAD_FL_SOURCE)) > + return -EINVAL; > + } > + > + return v4l2_subdev_call(sd, pad, set_routing, route); > + } > #endif > + > default: > return v4l2_subdev_call(sd, core, ioctl, cmd, arg); > } > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index 9102d6ca566e01f2..5acaeeb9b3cacefa 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -679,6 +679,9 @@ struct v4l2_subdev_pad_config { > * > * @set_frame_desc: set the low level media bus frame parameters, @fd array > * may be adjusted by the subdev driver to device capabilities. > + * > + * @get_routing: callback for VIDIOC_SUBDEV_G_ROUTING IOCTL handler. > + * @set_routing: callback for VIDIOC_SUBDEV_S_ROUTING IOCTL handler. Please define the purpose of those operations instead of just pointing to the userspace API. > */ > struct v4l2_subdev_pad_ops { > int (*init_cfg)(struct v4l2_subdev *sd, > @@ -719,6 +722,10 @@ struct v4l2_subdev_pad_ops { > struct v4l2_mbus_frame_desc *fd); > int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad, > struct v4l2_mbus_frame_desc *fd); > + int (*get_routing)(struct v4l2_subdev *sd, > + struct v4l2_subdev_routing *route); > + int (*set_routing)(struct v4l2_subdev *sd, > + struct v4l2_subdev_routing *route); > }; > > /** > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > index 03970ce3074193e6..af069bfb10ca23a5 100644 > --- a/include/uapi/linux/v4l2-subdev.h > +++ b/include/uapi/linux/v4l2-subdev.h > @@ -155,6 +155,44 @@ struct v4l2_subdev_selection { > __u32 reserved[8]; > }; > > +#define V4L2_SUBDEV_ROUTE_FL_ACTIVE (1 << 0) > +#define V4L2_SUBDEV_ROUTE_FL_IMMUTABLE (1 << 1) > + > +/** > + * struct v4l2_subdev_route - A signal route inside a subdev > + * @sink_pad: the sink pad > + * @sink_stream: the sink stream > + * @source_pad: the source pad > + * @source_stream: the source stream At this point in the series there's no concept of multiplexed streams, so the two fields don't make sense. You may want to reorder patches, or split this in two. > + * @flags: route flags: > + * > + * V4L2_SUBDEV_ROUTE_FL_ACTIVE: Is the stream in use or not? An > + * active stream will start when streaming is enabled on a video > + * node. Set by the user. This is very confusing as "stream" isn't defined. The documentation needs a rewrite with more details. > + * > + * V4L2_SUBDEV_ROUTE_FL_IMMUTABLE: Is the stream immutable, i.e. > + * can it be activated and inactivated? Set by the driver. > + */ > +struct v4l2_subdev_route { > + __u32 sink_pad; > + __u32 sink_stream; > + __u32 source_pad; > + __u32 source_stream; > + __u32 flags; > + __u32 reserved[5]; > +}; > + > +/** > + * struct v4l2_subdev_routing - Routing information > + * @routes: the routes array > + * @num_routes: the total number of routes in the routes array > + */ > +struct v4l2_subdev_routing { > + struct v4l2_subdev_route *routes; Missing __user ? > + __u32 num_routes; > + __u32 reserved[5]; > +}; > + > /* Backwards compatibility define --- to be removed */ > #define v4l2_subdev_edid v4l2_edid > > @@ -181,5 +219,7 @@ struct v4l2_subdev_selection { > #define VIDIOC_SUBDEV_ENUM_DV_TIMINGS _IOWR('V', 98, struct v4l2_enum_dv_timings) > #define VIDIOC_SUBDEV_QUERY_DV_TIMINGS _IOR('V', 99, struct v4l2_dv_timings) > #define VIDIOC_SUBDEV_DV_TIMINGS_CAP _IOWR('V', 100, struct v4l2_dv_timings_cap) > +#define VIDIOC_SUBDEV_G_ROUTING _IOWR('V', 38, struct v4l2_subdev_routing) > +#define VIDIOC_SUBDEV_S_ROUTING _IOWR('V', 39, struct v4l2_subdev_routing) > > #endif > -- > 2.19.1 >
On Wed, Jan 16, 2019 at 01:51:45AM +0200, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Fri, Nov 02, 2018 at 12:31:30AM +0100, Niklas Söderlund wrote: > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > > > > - Add sink and source streams for multiplexed links > > - Copy the argument back in case of an error. This is needed to let the > > caller know the number of routes. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > drivers/media/v4l2-core/v4l2-ioctl.c | 20 +++++++++++++- > > drivers/media/v4l2-core/v4l2-subdev.c | 28 +++++++++++++++++++ > > include/media/v4l2-subdev.h | 7 +++++ > > include/uapi/linux/v4l2-subdev.h | 40 +++++++++++++++++++++++++++ > > Missing documentation :-( > > > 4 files changed, 94 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > > index 7de041bae84fb2f2..40406acb51ec0906 100644 > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > > @@ -19,6 +19,7 @@ > > #include <linux/kernel.h> > > #include <linux/version.h> > > > > +#include <linux/v4l2-subdev.h> > > #include <linux/videodev2.h> > > > > #include <media/v4l2-common.h> > > @@ -2924,6 +2925,23 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size, > > } > > break; > > } > > + > > + case VIDIOC_SUBDEV_G_ROUTING: > > + case VIDIOC_SUBDEV_S_ROUTING: { > > + struct v4l2_subdev_routing *route = parg; > > + > > + if (route->num_routes > 0) { > > + if (route->num_routes > 256) > > + return -EINVAL; > > + > > + *user_ptr = (void __user *)route->routes; > > + *kernel_ptr = (void *)&route->routes; > > + *array_size = sizeof(struct v4l2_subdev_route) > > + * route->num_routes; > > + ret = 1; > > + } > > + break; > > + } > > } > > > > return ret; > > @@ -3033,7 +3051,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg, > > * Some ioctls can return an error, but still have valid > > * results that must be returned. > > */ > > - if (err < 0 && !always_copy) > > + if (err < 0 && !always_copy && cmd != VIDIOC_SUBDEV_G_ROUTING) > > This seems like a hack. Shouldn't VIDIOC_SUBDEV_G_ROUTING set > always_copy instead ? Sub-device IOCTLs are partially handled in v4l2-subdev.c, not here. In particular, __video_do_ioctl() that digs that information from v4l2_ioctls array is not in the call path for sub-device IOCTLs. So, it'd take a refactoring of IOCTL handling to change this, which I think should be a different patchset --- we're already at 30 patches here. > > > goto out; > > > > out_array_args: > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > index 792f41dffe2329b9..1d3b37cf548fa533 100644 > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > @@ -516,7 +516,35 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > > > case VIDIOC_SUBDEV_QUERYSTD: > > return v4l2_subdev_call(sd, video, querystd, arg); > > + > > + case VIDIOC_SUBDEV_G_ROUTING: > > + return v4l2_subdev_call(sd, pad, get_routing, arg); > > + > > + case VIDIOC_SUBDEV_S_ROUTING: { > > + struct v4l2_subdev_routing *route = arg; > > + unsigned int i; > > + > > + if (route->num_routes > sd->entity.num_pads) > > + return -EINVAL; > > + > > + for (i = 0; i < route->num_routes; ++i) { > > + unsigned int sink = route->routes[i].sink_pad; > > + unsigned int source = route->routes[i].source_pad; > > + struct media_pad *pads = sd->entity.pads; > > + > > + if (sink >= sd->entity.num_pads || > > + source >= sd->entity.num_pads) > > + return -EINVAL; > > + > > + if (!(pads[sink].flags & MEDIA_PAD_FL_SINK) || > > + !(pads[source].flags & MEDIA_PAD_FL_SOURCE)) > > + return -EINVAL; > > + } > > + > > + return v4l2_subdev_call(sd, pad, set_routing, route); > > + } > > #endif > > + > > default: > > return v4l2_subdev_call(sd, core, ioctl, cmd, arg); > > } > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > > index 9102d6ca566e01f2..5acaeeb9b3cacefa 100644 > > --- a/include/media/v4l2-subdev.h > > +++ b/include/media/v4l2-subdev.h > > @@ -679,6 +679,9 @@ struct v4l2_subdev_pad_config { > > * > > * @set_frame_desc: set the low level media bus frame parameters, @fd array > > * may be adjusted by the subdev driver to device capabilities. > > + * > > + * @get_routing: callback for VIDIOC_SUBDEV_G_ROUTING IOCTL handler. > > + * @set_routing: callback for VIDIOC_SUBDEV_S_ROUTING IOCTL handler. > > Please define the purpose of those operations instead of just pointing > to the userspace API. > > > */ > > struct v4l2_subdev_pad_ops { > > int (*init_cfg)(struct v4l2_subdev *sd, > > @@ -719,6 +722,10 @@ struct v4l2_subdev_pad_ops { > > struct v4l2_mbus_frame_desc *fd); > > int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad, > > struct v4l2_mbus_frame_desc *fd); > > + int (*get_routing)(struct v4l2_subdev *sd, > > + struct v4l2_subdev_routing *route); > > + int (*set_routing)(struct v4l2_subdev *sd, > > + struct v4l2_subdev_routing *route); > > }; > > > > /** > > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > > index 03970ce3074193e6..af069bfb10ca23a5 100644 > > --- a/include/uapi/linux/v4l2-subdev.h > > +++ b/include/uapi/linux/v4l2-subdev.h > > @@ -155,6 +155,44 @@ struct v4l2_subdev_selection { > > __u32 reserved[8]; > > }; > > > > +#define V4L2_SUBDEV_ROUTE_FL_ACTIVE (1 << 0) > > +#define V4L2_SUBDEV_ROUTE_FL_IMMUTABLE (1 << 1) > > + > > +/** > > + * struct v4l2_subdev_route - A signal route inside a subdev > > + * @sink_pad: the sink pad > > + * @sink_stream: the sink stream > > + * @source_pad: the source pad > > + * @source_stream: the source stream > > At this point in the series there's no concept of multiplexed streams, > so the two fields don't make sense. You may want to reorder patches, or > split this in two. I think it would make sense to reorder, as adding an IOCTL and then changing the argument struct would be something to avoid if possible. > > > + * @flags: route flags: > > + * > > + * V4L2_SUBDEV_ROUTE_FL_ACTIVE: Is the stream in use or not? An > > + * active stream will start when streaming is enabled on a video > > + * node. Set by the user. > > This is very confusing as "stream" isn't defined. The documentation > needs a rewrite with more details. Yes, we need a little more documentation on this. > > > + * > > + * V4L2_SUBDEV_ROUTE_FL_IMMUTABLE: Is the stream immutable, i.e. > > + * can it be activated and inactivated? Set by the driver. > > + */ > > +struct v4l2_subdev_route { > > + __u32 sink_pad; > > + __u32 sink_stream; > > + __u32 source_pad; > > + __u32 source_stream; > > + __u32 flags; > > + __u32 reserved[5]; > > +}; > > + > > +/** > > + * struct v4l2_subdev_routing - Routing information > > + * @routes: the routes array > > + * @num_routes: the total number of routes in the routes array > > + */ > > +struct v4l2_subdev_routing { > > + struct v4l2_subdev_route *routes; > > Missing __user ? We actually don't have any IOCTLs using __u64 pointer values in V4L2. I wonder what Hans thinks, too. I guess it's the way to go. Compat code is so awful. :-I > > > + __u32 num_routes; > > + __u32 reserved[5]; > > +}; > > + > > /* Backwards compatibility define --- to be removed */ > > #define v4l2_subdev_edid v4l2_edid > > > > @@ -181,5 +219,7 @@ struct v4l2_subdev_selection { > > #define VIDIOC_SUBDEV_ENUM_DV_TIMINGS _IOWR('V', 98, struct v4l2_enum_dv_timings) > > #define VIDIOC_SUBDEV_QUERY_DV_TIMINGS _IOR('V', 99, struct v4l2_dv_timings) > > #define VIDIOC_SUBDEV_DV_TIMINGS_CAP _IOWR('V', 100, struct v4l2_dv_timings_cap) > > +#define VIDIOC_SUBDEV_G_ROUTING _IOWR('V', 38, struct v4l2_subdev_routing) > > +#define VIDIOC_SUBDEV_S_ROUTING _IOWR('V', 39, struct v4l2_subdev_routing) > > > > #endif
Hi Sakari, On Tue, Jan 22, 2019 at 06:14:57PM +0200, Sakari Ailus wrote: > On Wed, Jan 16, 2019 at 01:51:45AM +0200, Laurent Pinchart wrote: > > On Fri, Nov 02, 2018 at 12:31:30AM +0100, Niklas Söderlund wrote: > >> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> > >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> > >> > >> - Add sink and source streams for multiplexed links > >> - Copy the argument back in case of an error. This is needed to let the > >> caller know the number of routes. > >> > >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > >> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > >> --- > >> drivers/media/v4l2-core/v4l2-ioctl.c | 20 +++++++++++++- > >> drivers/media/v4l2-core/v4l2-subdev.c | 28 +++++++++++++++++++ > >> include/media/v4l2-subdev.h | 7 +++++ > >> include/uapi/linux/v4l2-subdev.h | 40 +++++++++++++++++++++++++++ > > > > Missing documentation :-( > > > >> 4 files changed, 94 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > >> index 7de041bae84fb2f2..40406acb51ec0906 100644 > >> --- a/drivers/media/v4l2-core/v4l2-ioctl.c > >> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > >> @@ -19,6 +19,7 @@ > >> #include <linux/kernel.h> > >> #include <linux/version.h> > >> > >> +#include <linux/v4l2-subdev.h> > >> #include <linux/videodev2.h> > >> > >> #include <media/v4l2-common.h> > >> @@ -2924,6 +2925,23 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size, > >> } > >> break; > >> } > >> + > >> + case VIDIOC_SUBDEV_G_ROUTING: > >> + case VIDIOC_SUBDEV_S_ROUTING: { > >> + struct v4l2_subdev_routing *route = parg; > >> + > >> + if (route->num_routes > 0) { > >> + if (route->num_routes > 256) > >> + return -EINVAL; > >> + > >> + *user_ptr = (void __user *)route->routes; > >> + *kernel_ptr = (void *)&route->routes; > >> + *array_size = sizeof(struct v4l2_subdev_route) > >> + * route->num_routes; > >> + ret = 1; > >> + } > >> + break; > >> + } > >> } > >> > >> return ret; > >> @@ -3033,7 +3051,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg, > >> * Some ioctls can return an error, but still have valid > >> * results that must be returned. > >> */ > >> - if (err < 0 && !always_copy) > >> + if (err < 0 && !always_copy && cmd != VIDIOC_SUBDEV_G_ROUTING) > > > > This seems like a hack. Shouldn't VIDIOC_SUBDEV_G_ROUTING set > > always_copy instead ? > > Sub-device IOCTLs are partially handled in v4l2-subdev.c, not here. In > particular, __video_do_ioctl() that digs that information from v4l2_ioctls > array is not in the call path for sub-device IOCTLs. So, it'd take a > refactoring of IOCTL handling to change this, which I think should be a > different patchset --- we're already at 30 patches here. I'm OK with that. Could we add a FIXME comment ? > >> goto out; > >> > >> out_array_args: > >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > >> index 792f41dffe2329b9..1d3b37cf548fa533 100644 > >> --- a/drivers/media/v4l2-core/v4l2-subdev.c > >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c > >> @@ -516,7 +516,35 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > >> > >> case VIDIOC_SUBDEV_QUERYSTD: > >> return v4l2_subdev_call(sd, video, querystd, arg); > >> + > >> + case VIDIOC_SUBDEV_G_ROUTING: > >> + return v4l2_subdev_call(sd, pad, get_routing, arg); > >> + > >> + case VIDIOC_SUBDEV_S_ROUTING: { > >> + struct v4l2_subdev_routing *route = arg; > >> + unsigned int i; > >> + > >> + if (route->num_routes > sd->entity.num_pads) > >> + return -EINVAL; > >> + > >> + for (i = 0; i < route->num_routes; ++i) { > >> + unsigned int sink = route->routes[i].sink_pad; > >> + unsigned int source = route->routes[i].source_pad; > >> + struct media_pad *pads = sd->entity.pads; > >> + > >> + if (sink >= sd->entity.num_pads || > >> + source >= sd->entity.num_pads) > >> + return -EINVAL; > >> + > >> + if (!(pads[sink].flags & MEDIA_PAD_FL_SINK) || > >> + !(pads[source].flags & MEDIA_PAD_FL_SOURCE)) > >> + return -EINVAL; > >> + } > >> + > >> + return v4l2_subdev_call(sd, pad, set_routing, route); > >> + } > >> #endif > >> + > >> default: > >> return v4l2_subdev_call(sd, core, ioctl, cmd, arg); > >> } > >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > >> index 9102d6ca566e01f2..5acaeeb9b3cacefa 100644 > >> --- a/include/media/v4l2-subdev.h > >> +++ b/include/media/v4l2-subdev.h > >> @@ -679,6 +679,9 @@ struct v4l2_subdev_pad_config { > >> * > >> * @set_frame_desc: set the low level media bus frame parameters, @fd array > >> * may be adjusted by the subdev driver to device capabilities. > >> + * > >> + * @get_routing: callback for VIDIOC_SUBDEV_G_ROUTING IOCTL handler. > >> + * @set_routing: callback for VIDIOC_SUBDEV_S_ROUTING IOCTL handler. > > > > Please define the purpose of those operations instead of just pointing > > to the userspace API. > > > >> */ > >> struct v4l2_subdev_pad_ops { > >> int (*init_cfg)(struct v4l2_subdev *sd, > >> @@ -719,6 +722,10 @@ struct v4l2_subdev_pad_ops { > >> struct v4l2_mbus_frame_desc *fd); > >> int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad, > >> struct v4l2_mbus_frame_desc *fd); > >> + int (*get_routing)(struct v4l2_subdev *sd, > >> + struct v4l2_subdev_routing *route); > >> + int (*set_routing)(struct v4l2_subdev *sd, > >> + struct v4l2_subdev_routing *route); > >> }; > >> > >> /** > >> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > >> index 03970ce3074193e6..af069bfb10ca23a5 100644 > >> --- a/include/uapi/linux/v4l2-subdev.h > >> +++ b/include/uapi/linux/v4l2-subdev.h > >> @@ -155,6 +155,44 @@ struct v4l2_subdev_selection { > >> __u32 reserved[8]; > >> }; > >> > >> +#define V4L2_SUBDEV_ROUTE_FL_ACTIVE (1 << 0) > >> +#define V4L2_SUBDEV_ROUTE_FL_IMMUTABLE (1 << 1) > >> + > >> +/** > >> + * struct v4l2_subdev_route - A signal route inside a subdev > >> + * @sink_pad: the sink pad > >> + * @sink_stream: the sink stream > >> + * @source_pad: the source pad > >> + * @source_stream: the source stream > > > > At this point in the series there's no concept of multiplexed streams, > > so the two fields don't make sense. You may want to reorder patches, or > > split this in two. > > I think it would make sense to reorder, as adding an IOCTL and then > changing the argument struct would be something to avoid if possible. > > >> + * @flags: route flags: > >> + * > >> + * V4L2_SUBDEV_ROUTE_FL_ACTIVE: Is the stream in use or not? An > >> + * active stream will start when streaming is enabled on a video > >> + * node. Set by the user. > > > > This is very confusing as "stream" isn't defined. The documentation > > needs a rewrite with more details. > > Yes, we need a little more documentation on this. > > >> + * > >> + * V4L2_SUBDEV_ROUTE_FL_IMMUTABLE: Is the stream immutable, i.e. > >> + * can it be activated and inactivated? Set by the driver. > >> + */ > >> +struct v4l2_subdev_route { > >> + __u32 sink_pad; > >> + __u32 sink_stream; > >> + __u32 source_pad; > >> + __u32 source_stream; > >> + __u32 flags; > >> + __u32 reserved[5]; > >> +}; > >> + > >> +/** > >> + * struct v4l2_subdev_routing - Routing information > >> + * @routes: the routes array > >> + * @num_routes: the total number of routes in the routes array > >> + */ > >> +struct v4l2_subdev_routing { > >> + struct v4l2_subdev_route *routes; > > > > Missing __user ? > > We actually don't have any IOCTLs using __u64 pointer values in V4L2. I > wonder what Hans thinks, too. I guess it's the way to go. Compat code is so > awful. :-I > > > > >> + __u32 num_routes; > >> + __u32 reserved[5]; > >> +}; > >> + > >> /* Backwards compatibility define --- to be removed */ > >> #define v4l2_subdev_edid v4l2_edid > >> > >> @@ -181,5 +219,7 @@ struct v4l2_subdev_selection { > >> #define VIDIOC_SUBDEV_ENUM_DV_TIMINGS _IOWR('V', 98, struct v4l2_enum_dv_timings) > >> #define VIDIOC_SUBDEV_QUERY_DV_TIMINGS _IOR('V', 99, struct v4l2_dv_timings) > >> #define VIDIOC_SUBDEV_DV_TIMINGS_CAP _IOWR('V', 100, struct v4l2_dv_timings_cap) > >> +#define VIDIOC_SUBDEV_G_ROUTING _IOWR('V', 38, struct v4l2_subdev_routing) > >> +#define VIDIOC_SUBDEV_S_ROUTING _IOWR('V', 39, struct v4l2_subdev_routing) > >> > >> #endif
Hi Sakari, one quick question On Fri, Nov 02, 2018 at 12:31:30AM +0100, Niklas Söderlund wrote: > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > > - Add sink and source streams for multiplexed links > - Copy the argument back in case of an error. This is needed to let the > caller know the number of routes. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/media/v4l2-core/v4l2-ioctl.c | 20 +++++++++++++- > drivers/media/v4l2-core/v4l2-subdev.c | 28 +++++++++++++++++++ > include/media/v4l2-subdev.h | 7 +++++ > include/uapi/linux/v4l2-subdev.h | 40 +++++++++++++++++++++++++++ > 4 files changed, 94 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > index 7de041bae84fb2f2..40406acb51ec0906 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -19,6 +19,7 @@ > #include <linux/kernel.h> > #include <linux/version.h> > > +#include <linux/v4l2-subdev.h> > #include <linux/videodev2.h> > > #include <media/v4l2-common.h> > @@ -2924,6 +2925,23 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size, > } > break; > } > + > + case VIDIOC_SUBDEV_G_ROUTING: > + case VIDIOC_SUBDEV_S_ROUTING: { > + struct v4l2_subdev_routing *route = parg; > + > + if (route->num_routes > 0) { > + if (route->num_routes > 256) > + return -EINVAL; > + > + *user_ptr = (void __user *)route->routes; > + *kernel_ptr = (void *)&route->routes; > + *array_size = sizeof(struct v4l2_subdev_route) > + * route->num_routes; > + ret = 1; > + } > + break; > + } > } > > return ret; > @@ -3033,7 +3051,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg, > * Some ioctls can return an error, but still have valid > * results that must be returned. > */ > - if (err < 0 && !always_copy) > + if (err < 0 && !always_copy && cmd != VIDIOC_SUBDEV_G_ROUTING) > goto out; > > out_array_args: > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 792f41dffe2329b9..1d3b37cf548fa533 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -516,7 +516,35 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > case VIDIOC_SUBDEV_QUERYSTD: > return v4l2_subdev_call(sd, video, querystd, arg); > + > + case VIDIOC_SUBDEV_G_ROUTING: > + return v4l2_subdev_call(sd, pad, get_routing, arg); > + > + case VIDIOC_SUBDEV_S_ROUTING: { > + struct v4l2_subdev_routing *route = arg; > + unsigned int i; > + > + if (route->num_routes > sd->entity.num_pads) > + return -EINVAL; Can't the number of routes exceeds the total number of pad? To make an example, a subdevice with 2 sink pads, and 1 multiplexed source pad, with 2 streams would expose the following routing table, right? pad #0 = sink, 1 stream pad #1 = sink, 1 stream pad #2 = source, 2 streams Routing table: 0/0 -> 2/0 0/0 -> 2/1 1/0 -> 2/0 1/0 -> 2/1 In general, the number of accepted routes should depend on the number of streams, not pads, and that's better handled by drivers, am I wrong? Thanks j > + > + for (i = 0; i < route->num_routes; ++i) { > + unsigned int sink = route->routes[i].sink_pad; > + unsigned int source = route->routes[i].source_pad; > + struct media_pad *pads = sd->entity.pads; > + > + if (sink >= sd->entity.num_pads || > + source >= sd->entity.num_pads) > + return -EINVAL; > + > + if (!(pads[sink].flags & MEDIA_PAD_FL_SINK) || > + !(pads[source].flags & MEDIA_PAD_FL_SOURCE)) > + return -EINVAL; > + } > + > + return v4l2_subdev_call(sd, pad, set_routing, route); > + } > #endif > + > default: > return v4l2_subdev_call(sd, core, ioctl, cmd, arg); > } > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index 9102d6ca566e01f2..5acaeeb9b3cacefa 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -679,6 +679,9 @@ struct v4l2_subdev_pad_config { > * > * @set_frame_desc: set the low level media bus frame parameters, @fd array > * may be adjusted by the subdev driver to device capabilities. > + * > + * @get_routing: callback for VIDIOC_SUBDEV_G_ROUTING IOCTL handler. > + * @set_routing: callback for VIDIOC_SUBDEV_S_ROUTING IOCTL handler. > */ > struct v4l2_subdev_pad_ops { > int (*init_cfg)(struct v4l2_subdev *sd, > @@ -719,6 +722,10 @@ struct v4l2_subdev_pad_ops { > struct v4l2_mbus_frame_desc *fd); > int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad, > struct v4l2_mbus_frame_desc *fd); > + int (*get_routing)(struct v4l2_subdev *sd, > + struct v4l2_subdev_routing *route); > + int (*set_routing)(struct v4l2_subdev *sd, > + struct v4l2_subdev_routing *route); > }; > > /** > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > index 03970ce3074193e6..af069bfb10ca23a5 100644 > --- a/include/uapi/linux/v4l2-subdev.h > +++ b/include/uapi/linux/v4l2-subdev.h > @@ -155,6 +155,44 @@ struct v4l2_subdev_selection { > __u32 reserved[8]; > }; > > +#define V4L2_SUBDEV_ROUTE_FL_ACTIVE (1 << 0) > +#define V4L2_SUBDEV_ROUTE_FL_IMMUTABLE (1 << 1) > + > +/** > + * struct v4l2_subdev_route - A signal route inside a subdev > + * @sink_pad: the sink pad > + * @sink_stream: the sink stream > + * @source_pad: the source pad > + * @source_stream: the source stream > + * @flags: route flags: > + * > + * V4L2_SUBDEV_ROUTE_FL_ACTIVE: Is the stream in use or not? An > + * active stream will start when streaming is enabled on a video > + * node. Set by the user. > + * > + * V4L2_SUBDEV_ROUTE_FL_IMMUTABLE: Is the stream immutable, i.e. > + * can it be activated and inactivated? Set by the driver. > + */ > +struct v4l2_subdev_route { > + __u32 sink_pad; > + __u32 sink_stream; > + __u32 source_pad; > + __u32 source_stream; > + __u32 flags; > + __u32 reserved[5]; > +}; > + > +/** > + * struct v4l2_subdev_routing - Routing information > + * @routes: the routes array > + * @num_routes: the total number of routes in the routes array > + */ > +struct v4l2_subdev_routing { > + struct v4l2_subdev_route *routes; > + __u32 num_routes; > + __u32 reserved[5]; > +}; > + > /* Backwards compatibility define --- to be removed */ > #define v4l2_subdev_edid v4l2_edid > > @@ -181,5 +219,7 @@ struct v4l2_subdev_selection { > #define VIDIOC_SUBDEV_ENUM_DV_TIMINGS _IOWR('V', 98, struct v4l2_enum_dv_timings) > #define VIDIOC_SUBDEV_QUERY_DV_TIMINGS _IOR('V', 99, struct v4l2_dv_timings) > #define VIDIOC_SUBDEV_DV_TIMINGS_CAP _IOWR('V', 100, struct v4l2_dv_timings_cap) > +#define VIDIOC_SUBDEV_G_ROUTING _IOWR('V', 38, struct v4l2_subdev_routing) > +#define VIDIOC_SUBDEV_S_ROUTING _IOWR('V', 39, struct v4l2_subdev_routing) > > #endif > -- > 2.19.1 >
Hi Sakari, Laurent, Niklas, (another) quick question, but a different one :) On Wed, Jan 16, 2019 at 01:51:45AM +0200, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Fri, Nov 02, 2018 at 12:31:30AM +0100, Niklas Söderlund wrote: > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > > > > - Add sink and source streams for multiplexed links > > - Copy the argument back in case of an error. This is needed to let the > > caller know the number of routes. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > drivers/media/v4l2-core/v4l2-ioctl.c | 20 +++++++++++++- > > drivers/media/v4l2-core/v4l2-subdev.c | 28 +++++++++++++++++++ > > include/media/v4l2-subdev.h | 7 +++++ > > include/uapi/linux/v4l2-subdev.h | 40 +++++++++++++++++++++++++++ > > Missing documentation :-( > > > 4 files changed, 94 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > > index 7de041bae84fb2f2..40406acb51ec0906 100644 > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > > @@ -19,6 +19,7 @@ > > #include <linux/kernel.h> > > #include <linux/version.h> > > > > +#include <linux/v4l2-subdev.h> > > #include <linux/videodev2.h> > > > > #include <media/v4l2-common.h> > > @@ -2924,6 +2925,23 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size, > > } > > break; > > } > > + > > + case VIDIOC_SUBDEV_G_ROUTING: > > + case VIDIOC_SUBDEV_S_ROUTING: { > > + struct v4l2_subdev_routing *route = parg; > > + > > + if (route->num_routes > 0) { > > + if (route->num_routes > 256) > > + return -EINVAL; > > + > > + *user_ptr = (void __user *)route->routes; > > + *kernel_ptr = (void *)&route->routes; > > + *array_size = sizeof(struct v4l2_subdev_route) > > + * route->num_routes; > > + ret = 1; > > + } > > + break; > > + } > > } > > > > return ret; > > @@ -3033,7 +3051,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg, > > * Some ioctls can return an error, but still have valid > > * results that must be returned. > > */ > > - if (err < 0 && !always_copy) > > + if (err < 0 && !always_copy && cmd != VIDIOC_SUBDEV_G_ROUTING) > > This seems like a hack. Shouldn't VIDIOC_SUBDEV_G_ROUTING set > always_copy instead ? > > > goto out; > > > > out_array_args: > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > index 792f41dffe2329b9..1d3b37cf548fa533 100644 > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > @@ -516,7 +516,35 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > > > case VIDIOC_SUBDEV_QUERYSTD: > > return v4l2_subdev_call(sd, video, querystd, arg); > > + > > + case VIDIOC_SUBDEV_G_ROUTING: > > + return v4l2_subdev_call(sd, pad, get_routing, arg); > > + > > + case VIDIOC_SUBDEV_S_ROUTING: { > > + struct v4l2_subdev_routing *route = arg; > > + unsigned int i; > > + > > + if (route->num_routes > sd->entity.num_pads) > > + return -EINVAL; > > + > > + for (i = 0; i < route->num_routes; ++i) { How have you envisioned the number of routes to be negotiated with applications? I'm writing the documentation for this ioctl, and I would like to insert this part as well. Would a model like the one implemented in G_TOPOLOGY work in your opinion? In my understanding, at the moment applications do not have a way to reserve a known number of routes entries, but would likely reserve 'enough(tm)' (ie 256) and pass them to the G_ROUTING ioctl that the first time will likely adjust the number of num_routes and return -ENOSPC. Wouldn't it work to make the IOCTL behave in a way that it expects the first call to be performed with (num_routes == 0) and no routes entries reserved, and just adjust 'num_routes' in that case? So that applications should call G_ROUTING a first time with num_routes = 0, get back the number of routes entries, reserve memory for them, and then call G_ROUTING again to have the entries populated by the driver. Do you have different ideas or was this the intended behavior already? Thanks j > > + unsigned int sink = route->routes[i].sink_pad; > > + unsigned int source = route->routes[i].source_pad; > > + struct media_pad *pads = sd->entity.pads; > > + > > + if (sink >= sd->entity.num_pads || > > + source >= sd->entity.num_pads) > > + return -EINVAL; > > + > > + if (!(pads[sink].flags & MEDIA_PAD_FL_SINK) || > > + !(pads[source].flags & MEDIA_PAD_FL_SOURCE)) > > + return -EINVAL; > > + } > > + > > + return v4l2_subdev_call(sd, pad, set_routing, route); > > + } > > #endif > > + > > default: > > return v4l2_subdev_call(sd, core, ioctl, cmd, arg); > > } > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > > index 9102d6ca566e01f2..5acaeeb9b3cacefa 100644 > > --- a/include/media/v4l2-subdev.h > > +++ b/include/media/v4l2-subdev.h > > @@ -679,6 +679,9 @@ struct v4l2_subdev_pad_config { > > * > > * @set_frame_desc: set the low level media bus frame parameters, @fd array > > * may be adjusted by the subdev driver to device capabilities. > > + * > > + * @get_routing: callback for VIDIOC_SUBDEV_G_ROUTING IOCTL handler. > > + * @set_routing: callback for VIDIOC_SUBDEV_S_ROUTING IOCTL handler. > > Please define the purpose of those operations instead of just pointing > to the userspace API. > > > */ > > struct v4l2_subdev_pad_ops { > > int (*init_cfg)(struct v4l2_subdev *sd, > > @@ -719,6 +722,10 @@ struct v4l2_subdev_pad_ops { > > struct v4l2_mbus_frame_desc *fd); > > int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad, > > struct v4l2_mbus_frame_desc *fd); > > + int (*get_routing)(struct v4l2_subdev *sd, > > + struct v4l2_subdev_routing *route); > > + int (*set_routing)(struct v4l2_subdev *sd, > > + struct v4l2_subdev_routing *route); > > }; > > > > /** > > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > > index 03970ce3074193e6..af069bfb10ca23a5 100644 > > --- a/include/uapi/linux/v4l2-subdev.h > > +++ b/include/uapi/linux/v4l2-subdev.h > > @@ -155,6 +155,44 @@ struct v4l2_subdev_selection { > > __u32 reserved[8]; > > }; > > > > +#define V4L2_SUBDEV_ROUTE_FL_ACTIVE (1 << 0) > > +#define V4L2_SUBDEV_ROUTE_FL_IMMUTABLE (1 << 1) > > + > > +/** > > + * struct v4l2_subdev_route - A signal route inside a subdev > > + * @sink_pad: the sink pad > > + * @sink_stream: the sink stream > > + * @source_pad: the source pad > > + * @source_stream: the source stream > > At this point in the series there's no concept of multiplexed streams, > so the two fields don't make sense. You may want to reorder patches, or > split this in two. > > > + * @flags: route flags: > > + * > > + * V4L2_SUBDEV_ROUTE_FL_ACTIVE: Is the stream in use or not? An > > + * active stream will start when streaming is enabled on a video > > + * node. Set by the user. > > This is very confusing as "stream" isn't defined. The documentation > needs a rewrite with more details. > > > + * > > + * V4L2_SUBDEV_ROUTE_FL_IMMUTABLE: Is the stream immutable, i.e. > > + * can it be activated and inactivated? Set by the driver. > > + */ > > +struct v4l2_subdev_route { > > + __u32 sink_pad; > > + __u32 sink_stream; > > + __u32 source_pad; > > + __u32 source_stream; > > + __u32 flags; > > + __u32 reserved[5]; > > +}; > > + > > +/** > > + * struct v4l2_subdev_routing - Routing information > > + * @routes: the routes array > > + * @num_routes: the total number of routes in the routes array > > + */ > > +struct v4l2_subdev_routing { > > + struct v4l2_subdev_route *routes; > > Missing __user ? > > > + __u32 num_routes; > > + __u32 reserved[5]; > > +}; > > + > > /* Backwards compatibility define --- to be removed */ > > #define v4l2_subdev_edid v4l2_edid > > > > @@ -181,5 +219,7 @@ struct v4l2_subdev_selection { > > #define VIDIOC_SUBDEV_ENUM_DV_TIMINGS _IOWR('V', 98, struct v4l2_enum_dv_timings) > > #define VIDIOC_SUBDEV_QUERY_DV_TIMINGS _IOR('V', 99, struct v4l2_dv_timings) > > #define VIDIOC_SUBDEV_DV_TIMINGS_CAP _IOWR('V', 100, struct v4l2_dv_timings_cap) > > +#define VIDIOC_SUBDEV_G_ROUTING _IOWR('V', 38, struct v4l2_subdev_routing) > > +#define VIDIOC_SUBDEV_S_ROUTING _IOWR('V', 39, struct v4l2_subdev_routing) > > > > #endif > > -- > > 2.19.1 > > > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Thu, Feb 21, 2019 at 03:39:40PM +0100, Jacopo Mondi wrote: > Hi Sakari, > one quick question > > On Fri, Nov 02, 2018 at 12:31:30AM +0100, Niklas Söderlund wrote: > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > > > > - Add sink and source streams for multiplexed links > > - Copy the argument back in case of an error. This is needed to let the > > caller know the number of routes. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > drivers/media/v4l2-core/v4l2-ioctl.c | 20 +++++++++++++- > > drivers/media/v4l2-core/v4l2-subdev.c | 28 +++++++++++++++++++ > > include/media/v4l2-subdev.h | 7 +++++ > > include/uapi/linux/v4l2-subdev.h | 40 +++++++++++++++++++++++++++ > > 4 files changed, 94 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > > index 7de041bae84fb2f2..40406acb51ec0906 100644 > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > > @@ -19,6 +19,7 @@ > > #include <linux/kernel.h> > > #include <linux/version.h> > > > > +#include <linux/v4l2-subdev.h> > > #include <linux/videodev2.h> > > > > #include <media/v4l2-common.h> > > @@ -2924,6 +2925,23 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size, > > } > > break; > > } > > + > > + case VIDIOC_SUBDEV_G_ROUTING: > > + case VIDIOC_SUBDEV_S_ROUTING: { > > + struct v4l2_subdev_routing *route = parg; > > + > > + if (route->num_routes > 0) { > > + if (route->num_routes > 256) > > + return -EINVAL; > > + > > + *user_ptr = (void __user *)route->routes; > > + *kernel_ptr = (void *)&route->routes; > > + *array_size = sizeof(struct v4l2_subdev_route) > > + * route->num_routes; > > + ret = 1; > > + } > > + break; > > + } > > } > > > > return ret; > > @@ -3033,7 +3051,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg, > > * Some ioctls can return an error, but still have valid > > * results that must be returned. > > */ > > - if (err < 0 && !always_copy) > > + if (err < 0 && !always_copy && cmd != VIDIOC_SUBDEV_G_ROUTING) > > goto out; > > > > out_array_args: > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > index 792f41dffe2329b9..1d3b37cf548fa533 100644 > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > @@ -516,7 +516,35 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > > > case VIDIOC_SUBDEV_QUERYSTD: > > return v4l2_subdev_call(sd, video, querystd, arg); > > + > > + case VIDIOC_SUBDEV_G_ROUTING: > > + return v4l2_subdev_call(sd, pad, get_routing, arg); > > + > > + case VIDIOC_SUBDEV_S_ROUTING: { > > + struct v4l2_subdev_routing *route = arg; > > + unsigned int i; > > + > > + if (route->num_routes > sd->entity.num_pads) > > + return -EINVAL; > > Can't the number of routes exceeds the total number of pad? > > To make an example, a subdevice with 2 sink pads, and 1 multiplexed > source pad, with 2 streams would expose the following routing table, > right? > > pad #0 = sink, 1 stream > pad #1 = sink, 1 stream > pad #2 = source, 2 streams > > Routing table: > 0/0 -> 2/0 > 0/0 -> 2/1 > 1/0 -> 2/0 > 1/0 -> 2/1 > > In general, the number of accepted routes should depend on the number > of streams, not pads, and that's better handled by drivers, am I > wrong? Good point. Is the above configuration meaningful? I.e. you have a mux device that does some processing as well by combining the streams? It's far-fetched but at the moment the API does not really bend for that. It still might in the future. I guess we could just remove the check, and let drivers handle it. > > Thanks > j > > > + > > + for (i = 0; i < route->num_routes; ++i) { > > + unsigned int sink = route->routes[i].sink_pad; > > + unsigned int source = route->routes[i].source_pad; > > + struct media_pad *pads = sd->entity.pads; > > + > > + if (sink >= sd->entity.num_pads || > > + source >= sd->entity.num_pads) > > + return -EINVAL; > > + > > + if (!(pads[sink].flags & MEDIA_PAD_FL_SINK) || > > + !(pads[source].flags & MEDIA_PAD_FL_SOURCE)) > > + return -EINVAL; > > + } > > + > > + return v4l2_subdev_call(sd, pad, set_routing, route); > > + } > > #endif > > + > > default: > > return v4l2_subdev_call(sd, core, ioctl, cmd, arg); > > } > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > > index 9102d6ca566e01f2..5acaeeb9b3cacefa 100644 > > --- a/include/media/v4l2-subdev.h > > +++ b/include/media/v4l2-subdev.h > > @@ -679,6 +679,9 @@ struct v4l2_subdev_pad_config { > > * > > * @set_frame_desc: set the low level media bus frame parameters, @fd array > > * may be adjusted by the subdev driver to device capabilities. > > + * > > + * @get_routing: callback for VIDIOC_SUBDEV_G_ROUTING IOCTL handler. > > + * @set_routing: callback for VIDIOC_SUBDEV_S_ROUTING IOCTL handler. > > */ > > struct v4l2_subdev_pad_ops { > > int (*init_cfg)(struct v4l2_subdev *sd, > > @@ -719,6 +722,10 @@ struct v4l2_subdev_pad_ops { > > struct v4l2_mbus_frame_desc *fd); > > int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad, > > struct v4l2_mbus_frame_desc *fd); > > + int (*get_routing)(struct v4l2_subdev *sd, > > + struct v4l2_subdev_routing *route); > > + int (*set_routing)(struct v4l2_subdev *sd, > > + struct v4l2_subdev_routing *route); > > }; > > > > /** > > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > > index 03970ce3074193e6..af069bfb10ca23a5 100644 > > --- a/include/uapi/linux/v4l2-subdev.h > > +++ b/include/uapi/linux/v4l2-subdev.h > > @@ -155,6 +155,44 @@ struct v4l2_subdev_selection { > > __u32 reserved[8]; > > }; > > > > +#define V4L2_SUBDEV_ROUTE_FL_ACTIVE (1 << 0) > > +#define V4L2_SUBDEV_ROUTE_FL_IMMUTABLE (1 << 1) > > + > > +/** > > + * struct v4l2_subdev_route - A signal route inside a subdev > > + * @sink_pad: the sink pad > > + * @sink_stream: the sink stream > > + * @source_pad: the source pad > > + * @source_stream: the source stream > > + * @flags: route flags: > > + * > > + * V4L2_SUBDEV_ROUTE_FL_ACTIVE: Is the stream in use or not? An > > + * active stream will start when streaming is enabled on a video > > + * node. Set by the user. > > + * > > + * V4L2_SUBDEV_ROUTE_FL_IMMUTABLE: Is the stream immutable, i.e. > > + * can it be activated and inactivated? Set by the driver. > > + */ > > +struct v4l2_subdev_route { > > + __u32 sink_pad; > > + __u32 sink_stream; > > + __u32 source_pad; > > + __u32 source_stream; > > + __u32 flags; > > + __u32 reserved[5]; > > +}; > > + > > +/** > > + * struct v4l2_subdev_routing - Routing information > > + * @routes: the routes array > > + * @num_routes: the total number of routes in the routes array > > + */ > > +struct v4l2_subdev_routing { > > + struct v4l2_subdev_route *routes; This should be just __u64, to avoid writing compat code. The patch was written before we started doing that. :-) Please see e.g. the media device topology IOCTL. > > + __u32 num_routes; > > + __u32 reserved[5]; > > +}; > > + > > /* Backwards compatibility define --- to be removed */ > > #define v4l2_subdev_edid v4l2_edid > > > > @@ -181,5 +219,7 @@ struct v4l2_subdev_selection { > > #define VIDIOC_SUBDEV_ENUM_DV_TIMINGS _IOWR('V', 98, struct v4l2_enum_dv_timings) > > #define VIDIOC_SUBDEV_QUERY_DV_TIMINGS _IOR('V', 99, struct v4l2_dv_timings) > > #define VIDIOC_SUBDEV_DV_TIMINGS_CAP _IOWR('V', 100, struct v4l2_dv_timings_cap) > > +#define VIDIOC_SUBDEV_G_ROUTING _IOWR('V', 38, struct v4l2_subdev_routing) > > +#define VIDIOC_SUBDEV_S_ROUTING _IOWR('V', 39, struct v4l2_subdev_routing) > > > > #endif
Hi Jacopo, On Thu, Feb 21, 2019 at 03:59:20PM +0100, Jacopo Mondi wrote: > Hi Sakari, Laurent, Niklas, > (another) quick question, but a different one :) > > On Wed, Jan 16, 2019 at 01:51:45AM +0200, Laurent Pinchart wrote: > > Hi Niklas, > > > > Thank you for the patch. > > > > On Fri, Nov 02, 2018 at 12:31:30AM +0100, Niklas Söderlund wrote: > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > > > > > > - Add sink and source streams for multiplexed links > > > - Copy the argument back in case of an error. This is needed to let the > > > caller know the number of routes. > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > --- > > > drivers/media/v4l2-core/v4l2-ioctl.c | 20 +++++++++++++- > > > drivers/media/v4l2-core/v4l2-subdev.c | 28 +++++++++++++++++++ > > > include/media/v4l2-subdev.h | 7 +++++ > > > include/uapi/linux/v4l2-subdev.h | 40 +++++++++++++++++++++++++++ > > > > Missing documentation :-( > > > > > 4 files changed, 94 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > > > index 7de041bae84fb2f2..40406acb51ec0906 100644 > > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > > > @@ -19,6 +19,7 @@ > > > #include <linux/kernel.h> > > > #include <linux/version.h> > > > > > > +#include <linux/v4l2-subdev.h> > > > #include <linux/videodev2.h> > > > > > > #include <media/v4l2-common.h> > > > @@ -2924,6 +2925,23 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size, > > > } > > > break; > > > } > > > + > > > + case VIDIOC_SUBDEV_G_ROUTING: > > > + case VIDIOC_SUBDEV_S_ROUTING: { > > > + struct v4l2_subdev_routing *route = parg; > > > + > > > + if (route->num_routes > 0) { > > > + if (route->num_routes > 256) > > > + return -EINVAL; > > > + > > > + *user_ptr = (void __user *)route->routes; > > > + *kernel_ptr = (void *)&route->routes; > > > + *array_size = sizeof(struct v4l2_subdev_route) > > > + * route->num_routes; > > > + ret = 1; > > > + } > > > + break; > > > + } > > > } > > > > > > return ret; > > > @@ -3033,7 +3051,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg, > > > * Some ioctls can return an error, but still have valid > > > * results that must be returned. > > > */ > > > - if (err < 0 && !always_copy) > > > + if (err < 0 && !always_copy && cmd != VIDIOC_SUBDEV_G_ROUTING) > > > > This seems like a hack. Shouldn't VIDIOC_SUBDEV_G_ROUTING set > > always_copy instead ? > > > > > goto out; > > > > > > out_array_args: > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > > index 792f41dffe2329b9..1d3b37cf548fa533 100644 > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > > @@ -516,7 +516,35 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > > > > > case VIDIOC_SUBDEV_QUERYSTD: > > > return v4l2_subdev_call(sd, video, querystd, arg); > > > + > > > + case VIDIOC_SUBDEV_G_ROUTING: > > > + return v4l2_subdev_call(sd, pad, get_routing, arg); > > > + > > > + case VIDIOC_SUBDEV_S_ROUTING: { > > > + struct v4l2_subdev_routing *route = arg; > > > + unsigned int i; > > > + > > > + if (route->num_routes > sd->entity.num_pads) > > > + return -EINVAL; > > > + > > > + for (i = 0; i < route->num_routes; ++i) { > > How have you envisioned the number of routes to be negotiated with > applications? I'm writing the documentation for this ioctl, and I > would like to insert this part as well. > > Would a model like the one implemented in G_TOPOLOGY work in your > opinion? In my understanding, at the moment applications do not have a > way to reserve a known number of routes entries, but would likely > reserve 'enough(tm)' (ie 256) and pass them to the G_ROUTING ioctl that the > first time will likely adjust the number of num_routes and return -ENOSPC. > > Wouldn't it work to make the IOCTL behave in a way that it > expects the first call to be performed with (num_routes == 0) and no routes > entries reserved, and just adjust 'num_routes' in that case? > So that applications should call G_ROUTING a first time with > num_routes = 0, get back the number of routes entries, reserve memory > for them, and then call G_ROUTING again to have the entries populated > by the driver. Do you have different ideas or was this the intended > behavior already? I think whenever the number of routes isn't enough to return them all, the IOCTL should return -ENOSPC, and set the actual number of routes there. Not just zero. The user could e.g. try with a static allocation of some, and allocate memory if the static allocation turns not to be enough. Btw. the idea behind S_ROUTING behaviour was to allow multiple users to work on a single sub-device without having to know about each other. That's why there are flags to tell which routes are enabled and which are not. I'll be better available tomorrow, let's discuss e.g. on #v4l then.
Hi Sakari, On Fri, Feb 22, 2019 at 12:31:32AM +0200, Sakari Ailus wrote: > Hi Jacopo, > > On Thu, Feb 21, 2019 at 03:39:40PM +0100, Jacopo Mondi wrote: > > Hi Sakari, > > one quick question > > > > On Fri, Nov 02, 2018 at 12:31:30AM +0100, Niklas Söderlund wrote: > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > > > > > > - Add sink and source streams for multiplexed links > > > - Copy the argument back in case of an error. This is needed to let the > > > caller know the number of routes. > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > --- > > > drivers/media/v4l2-core/v4l2-ioctl.c | 20 +++++++++++++- > > > drivers/media/v4l2-core/v4l2-subdev.c | 28 +++++++++++++++++++ > > > include/media/v4l2-subdev.h | 7 +++++ > > > include/uapi/linux/v4l2-subdev.h | 40 +++++++++++++++++++++++++++ > > > 4 files changed, 94 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > > > index 7de041bae84fb2f2..40406acb51ec0906 100644 > > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > > > @@ -19,6 +19,7 @@ > > > #include <linux/kernel.h> > > > #include <linux/version.h> > > > > > > +#include <linux/v4l2-subdev.h> > > > #include <linux/videodev2.h> > > > > > > #include <media/v4l2-common.h> > > > @@ -2924,6 +2925,23 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size, > > > } > > > break; > > > } > > > + > > > + case VIDIOC_SUBDEV_G_ROUTING: > > > + case VIDIOC_SUBDEV_S_ROUTING: { > > > + struct v4l2_subdev_routing *route = parg; > > > + > > > + if (route->num_routes > 0) { > > > + if (route->num_routes > 256) > > > + return -EINVAL; > > > + > > > + *user_ptr = (void __user *)route->routes; > > > + *kernel_ptr = (void *)&route->routes; > > > + *array_size = sizeof(struct v4l2_subdev_route) > > > + * route->num_routes; > > > + ret = 1; > > > + } > > > + break; > > > + } > > > } > > > > > > return ret; > > > @@ -3033,7 +3051,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg, > > > * Some ioctls can return an error, but still have valid > > > * results that must be returned. > > > */ > > > - if (err < 0 && !always_copy) > > > + if (err < 0 && !always_copy && cmd != VIDIOC_SUBDEV_G_ROUTING) > > > goto out; > > > > > > out_array_args: > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > > index 792f41dffe2329b9..1d3b37cf548fa533 100644 > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > > @@ -516,7 +516,35 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > > > > > case VIDIOC_SUBDEV_QUERYSTD: > > > return v4l2_subdev_call(sd, video, querystd, arg); > > > + > > > + case VIDIOC_SUBDEV_G_ROUTING: > > > + return v4l2_subdev_call(sd, pad, get_routing, arg); > > > + > > > + case VIDIOC_SUBDEV_S_ROUTING: { > > > + struct v4l2_subdev_routing *route = arg; > > > + unsigned int i; > > > + > > > + if (route->num_routes > sd->entity.num_pads) > > > + return -EINVAL; > > > > Can't the number of routes exceeds the total number of pad? > > > > To make an example, a subdevice with 2 sink pads, and 1 multiplexed > > source pad, with 2 streams would expose the following routing table, > > right? > > > > pad #0 = sink, 1 stream > > pad #1 = sink, 1 stream > > pad #2 = source, 2 streams > > > > Routing table: > > 0/0 -> 2/0 > > 0/0 -> 2/1 > > 1/0 -> 2/0 > > 1/0 -> 2/1 > > > > In general, the number of accepted routes should depend on the number > > of streams, not pads, and that's better handled by drivers, am I > > wrong? > > Good point. Is the above configuration meaningful? I.e. you have a mux > device that does some processing as well by combining the streams? > > It's far-fetched but at the moment the API does not really bend for that. > It still might in the future. Well, how would you expect a routing table for a device that supports multiplexing using Virtual Channels to look like? As far as I got it, even with a single sink and a single source, it would be: [SINK/SINK_STREAM -> SOURCE/SOURCE_STREAM] 0/0 -> 1/0 0/0 -> 1/1 0/0 -> 1/2 0/0 -> 1/3 On the previous example, I thought about GMSL-like devices, that can output the video streams received from different remotes in a separate virtual channel, at the same time. A possible routing table in that case would be like: Pads 0, 1, 2, 3 = SINKS Pad 4 = SOURCE with 4 streams (1 for each VC) 0/0 -> 4/0 0/0 -> 4/1 0/0 -> 4/2 0/0 -> 4/3 1/0 -> 4/0 1/0 -> 4/1 1/0 -> 4/2 1/0 -> 4/3 2/0 -> 4/0 2/0 -> 4/1 2/0 -> 4/2 2/0 -> 4/3 3/0 -> 4/0 3/0 -> 4/1 3/0 -> 4/2 3/0 -> 4/3 With only one route per virtual channel active at a time. Does this match your idea? > > I guess we could just remove the check, and let drivers handle it. > Yup, I agree! > > > > Thanks > > j > > > > > + > > > + for (i = 0; i < route->num_routes; ++i) { > > > + unsigned int sink = route->routes[i].sink_pad; > > > + unsigned int source = route->routes[i].source_pad; > > > + struct media_pad *pads = sd->entity.pads; > > > + > > > + if (sink >= sd->entity.num_pads || > > > + source >= sd->entity.num_pads) > > > + return -EINVAL; > > > + > > > + if (!(pads[sink].flags & MEDIA_PAD_FL_SINK) || > > > + !(pads[source].flags & MEDIA_PAD_FL_SOURCE)) > > > + return -EINVAL; > > > + } > > > + > > > + return v4l2_subdev_call(sd, pad, set_routing, route); > > > + } > > > #endif > > > + > > > default: > > > return v4l2_subdev_call(sd, core, ioctl, cmd, arg); > > > } > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > > > index 9102d6ca566e01f2..5acaeeb9b3cacefa 100644 > > > --- a/include/media/v4l2-subdev.h > > > +++ b/include/media/v4l2-subdev.h > > > @@ -679,6 +679,9 @@ struct v4l2_subdev_pad_config { > > > * > > > * @set_frame_desc: set the low level media bus frame parameters, @fd array > > > * may be adjusted by the subdev driver to device capabilities. > > > + * > > > + * @get_routing: callback for VIDIOC_SUBDEV_G_ROUTING IOCTL handler. > > > + * @set_routing: callback for VIDIOC_SUBDEV_S_ROUTING IOCTL handler. > > > */ > > > struct v4l2_subdev_pad_ops { > > > int (*init_cfg)(struct v4l2_subdev *sd, > > > @@ -719,6 +722,10 @@ struct v4l2_subdev_pad_ops { > > > struct v4l2_mbus_frame_desc *fd); > > > int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad, > > > struct v4l2_mbus_frame_desc *fd); > > > + int (*get_routing)(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_routing *route); > > > + int (*set_routing)(struct v4l2_subdev *sd, > > > + struct v4l2_subdev_routing *route); > > > }; > > > > > > /** > > > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > > > index 03970ce3074193e6..af069bfb10ca23a5 100644 > > > --- a/include/uapi/linux/v4l2-subdev.h > > > +++ b/include/uapi/linux/v4l2-subdev.h > > > @@ -155,6 +155,44 @@ struct v4l2_subdev_selection { > > > __u32 reserved[8]; > > > }; > > > > > > +#define V4L2_SUBDEV_ROUTE_FL_ACTIVE (1 << 0) > > > +#define V4L2_SUBDEV_ROUTE_FL_IMMUTABLE (1 << 1) > > > + > > > +/** > > > + * struct v4l2_subdev_route - A signal route inside a subdev > > > + * @sink_pad: the sink pad > > > + * @sink_stream: the sink stream > > > + * @source_pad: the source pad > > > + * @source_stream: the source stream > > > + * @flags: route flags: > > > + * > > > + * V4L2_SUBDEV_ROUTE_FL_ACTIVE: Is the stream in use or not? An > > > + * active stream will start when streaming is enabled on a video > > > + * node. Set by the user. > > > + * > > > + * V4L2_SUBDEV_ROUTE_FL_IMMUTABLE: Is the stream immutable, i.e. > > > + * can it be activated and inactivated? Set by the driver. > > > + */ > > > +struct v4l2_subdev_route { > > > + __u32 sink_pad; > > > + __u32 sink_stream; > > > + __u32 source_pad; > > > + __u32 source_stream; > > > + __u32 flags; > > > + __u32 reserved[5]; > > > +}; > > > + > > > +/** > > > + * struct v4l2_subdev_routing - Routing information > > > + * @routes: the routes array > > > + * @num_routes: the total number of routes in the routes array > > > + */ > > > +struct v4l2_subdev_routing { > > > + struct v4l2_subdev_route *routes; > > This should be just __u64, to avoid writing compat code. The patch was > written before we started doing that. :-) Please see e.g. the media device > topology IOCTL. > Thanks, I had a look at the MEDIA_ ioctls yesterday, G_TOPOLOGY in particular, which uses several pointers to arrays. Unfortunately, I didn't come up with anything better than using a translation structure, from the IOCTL layer to the subdevice operations layer: https://paste.debian.net/hidden/b192969d/ (sharing a link for early comments, I can send v3 and you can comment there directly if you prefer to :) Thanks j > > > + __u32 num_routes; > > > + __u32 reserved[5]; > > > +}; > > > + > > > /* Backwards compatibility define --- to be removed */ > > > #define v4l2_subdev_edid v4l2_edid > > > > > > @@ -181,5 +219,7 @@ struct v4l2_subdev_selection { > > > #define VIDIOC_SUBDEV_ENUM_DV_TIMINGS _IOWR('V', 98, struct v4l2_enum_dv_timings) > > > #define VIDIOC_SUBDEV_QUERY_DV_TIMINGS _IOR('V', 99, struct v4l2_dv_timings) > > > #define VIDIOC_SUBDEV_DV_TIMINGS_CAP _IOWR('V', 100, struct v4l2_dv_timings_cap) > > > +#define VIDIOC_SUBDEV_G_ROUTING _IOWR('V', 38, struct v4l2_subdev_routing) > > > +#define VIDIOC_SUBDEV_S_ROUTING _IOWR('V', 39, struct v4l2_subdev_routing) > > > > > > #endif > > -- > Regards, > > Sakari Ailus > sakari.ailus@linux.intel.com
Hi Sakari, On Fri, Feb 22, 2019 at 01:49:36AM +0200, Sakari Ailus wrote: > Hi Jacopo, > > On Thu, Feb 21, 2019 at 03:59:20PM +0100, Jacopo Mondi wrote: > > Hi Sakari, Laurent, Niklas, > > (another) quick question, but a different one :) > > > > On Wed, Jan 16, 2019 at 01:51:45AM +0200, Laurent Pinchart wrote: > > > Hi Niklas, > > > > > > Thank you for the patch. > > > > > > On Fri, Nov 02, 2018 at 12:31:30AM +0100, Niklas Söderlund wrote: > > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > > > > > > > > - Add sink and source streams for multiplexed links > > > > - Copy the argument back in case of an error. This is needed to let the > > > > caller know the number of routes. > > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > > --- > > > > drivers/media/v4l2-core/v4l2-ioctl.c | 20 +++++++++++++- > > > > drivers/media/v4l2-core/v4l2-subdev.c | 28 +++++++++++++++++++ > > > > include/media/v4l2-subdev.h | 7 +++++ > > > > include/uapi/linux/v4l2-subdev.h | 40 +++++++++++++++++++++++++++ > > > > > > Missing documentation :-( > > > > > > > 4 files changed, 94 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > > > > index 7de041bae84fb2f2..40406acb51ec0906 100644 > > > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > > > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > > > > @@ -19,6 +19,7 @@ > > > > #include <linux/kernel.h> > > > > #include <linux/version.h> > > > > > > > > +#include <linux/v4l2-subdev.h> > > > > #include <linux/videodev2.h> > > > > > > > > #include <media/v4l2-common.h> > > > > @@ -2924,6 +2925,23 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size, > > > > } > > > > break; > > > > } > > > > + > > > > + case VIDIOC_SUBDEV_G_ROUTING: > > > > + case VIDIOC_SUBDEV_S_ROUTING: { > > > > + struct v4l2_subdev_routing *route = parg; > > > > + > > > > + if (route->num_routes > 0) { > > > > + if (route->num_routes > 256) > > > > + return -EINVAL; > > > > + > > > > + *user_ptr = (void __user *)route->routes; > > > > + *kernel_ptr = (void *)&route->routes; > > > > + *array_size = sizeof(struct v4l2_subdev_route) > > > > + * route->num_routes; > > > > + ret = 1; > > > > + } > > > > + break; > > > > + } > > > > } > > > > > > > > return ret; > > > > @@ -3033,7 +3051,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg, > > > > * Some ioctls can return an error, but still have valid > > > > * results that must be returned. > > > > */ > > > > - if (err < 0 && !always_copy) > > > > + if (err < 0 && !always_copy && cmd != VIDIOC_SUBDEV_G_ROUTING) > > > > > > This seems like a hack. Shouldn't VIDIOC_SUBDEV_G_ROUTING set > > > always_copy instead ? > > > > > > > goto out; > > > > > > > > out_array_args: > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > > > index 792f41dffe2329b9..1d3b37cf548fa533 100644 > > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > > > @@ -516,7 +516,35 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > > > > > > > case VIDIOC_SUBDEV_QUERYSTD: > > > > return v4l2_subdev_call(sd, video, querystd, arg); > > > > + > > > > + case VIDIOC_SUBDEV_G_ROUTING: > > > > + return v4l2_subdev_call(sd, pad, get_routing, arg); > > > > + > > > > + case VIDIOC_SUBDEV_S_ROUTING: { > > > > + struct v4l2_subdev_routing *route = arg; > > > > + unsigned int i; > > > > + > > > > + if (route->num_routes > sd->entity.num_pads) > > > > + return -EINVAL; > > > > + > > > > + for (i = 0; i < route->num_routes; ++i) { > > > > How have you envisioned the number of routes to be negotiated with > > applications? I'm writing the documentation for this ioctl, and I > > would like to insert this part as well. > > > > Would a model like the one implemented in G_TOPOLOGY work in your > > opinion? In my understanding, at the moment applications do not have a > > way to reserve a known number of routes entries, but would likely > > reserve 'enough(tm)' (ie 256) and pass them to the G_ROUTING ioctl that the > > first time will likely adjust the number of num_routes and return -ENOSPC. > > > > Wouldn't it work to make the IOCTL behave in a way that it > > expects the first call to be performed with (num_routes == 0) and no routes > > entries reserved, and just adjust 'num_routes' in that case? > > So that applications should call G_ROUTING a first time with > > num_routes = 0, get back the number of routes entries, reserve memory > > for them, and then call G_ROUTING again to have the entries populated > > by the driver. Do you have different ideas or was this the intended > > behavior already? > > I think whenever the number of routes isn't enough to return them all, the > IOCTL should return -ENOSPC, and set the actual number of routes there. Not > just zero. The user could e.g. try with a static allocation of some, and > allocate memory if the static allocation turns not to be enough. I see. That's fine, I just wanted to make sure I was not missing something... Fine with me if num_routes is not adequate, it should be corrected by the driver and -ENOSPC returned. How applications deal with allocation is up to them (even if documentation might suggest to perform and "call with 0, allocate, call again" sequence as a way to ease this) > > Btw. the idea behind S_ROUTING behaviour was to allow multiple users to > work on a single sub-device without having to know about each other. That's > why there are flags to tell which routes are enabled and which are not. > > I'll be better available tomorrow, let's discuss e.g. on #v4l then. > Unfortunately I won't too much today :) I'll keep pinging in the next days then, sorry about this :p Thanks j > -- > Sakari Ailus > sakari.ailus@linux.intel.com
Hi Jacopo, On Fri, Feb 22, 2019 at 09:40:19AM +0100, Jacopo Mondi wrote: > Hi Sakari, > > On Fri, Feb 22, 2019 at 12:31:32AM +0200, Sakari Ailus wrote: > > Hi Jacopo, > > > > On Thu, Feb 21, 2019 at 03:39:40PM +0100, Jacopo Mondi wrote: > > > Hi Sakari, > > > one quick question > > > > > > On Fri, Nov 02, 2018 at 12:31:30AM +0100, Niklas Söderlund wrote: > > > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > > > > > > > > - Add sink and source streams for multiplexed links > > > > - Copy the argument back in case of an error. This is needed to let the > > > > caller know the number of routes. > > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > > --- > > > > drivers/media/v4l2-core/v4l2-ioctl.c | 20 +++++++++++++- > > > > drivers/media/v4l2-core/v4l2-subdev.c | 28 +++++++++++++++++++ > > > > include/media/v4l2-subdev.h | 7 +++++ > > > > include/uapi/linux/v4l2-subdev.h | 40 +++++++++++++++++++++++++++ > > > > 4 files changed, 94 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > > > > index 7de041bae84fb2f2..40406acb51ec0906 100644 > > > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > > > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > > > > @@ -19,6 +19,7 @@ > > > > #include <linux/kernel.h> > > > > #include <linux/version.h> > > > > > > > > +#include <linux/v4l2-subdev.h> > > > > #include <linux/videodev2.h> > > > > > > > > #include <media/v4l2-common.h> > > > > @@ -2924,6 +2925,23 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size, > > > > } > > > > break; > > > > } > > > > + > > > > + case VIDIOC_SUBDEV_G_ROUTING: > > > > + case VIDIOC_SUBDEV_S_ROUTING: { > > > > + struct v4l2_subdev_routing *route = parg; > > > > + > > > > + if (route->num_routes > 0) { > > > > + if (route->num_routes > 256) > > > > + return -EINVAL; > > > > + > > > > + *user_ptr = (void __user *)route->routes; > > > > + *kernel_ptr = (void *)&route->routes; > > > > + *array_size = sizeof(struct v4l2_subdev_route) > > > > + * route->num_routes; > > > > + ret = 1; > > > > + } > > > > + break; > > > > + } > > > > } > > > > > > > > return ret; > > > > @@ -3033,7 +3051,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg, > > > > * Some ioctls can return an error, but still have valid > > > > * results that must be returned. > > > > */ > > > > - if (err < 0 && !always_copy) > > > > + if (err < 0 && !always_copy && cmd != VIDIOC_SUBDEV_G_ROUTING) > > > > goto out; > > > > > > > > out_array_args: > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > > > index 792f41dffe2329b9..1d3b37cf548fa533 100644 > > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > > > @@ -516,7 +516,35 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > > > > > > > case VIDIOC_SUBDEV_QUERYSTD: > > > > return v4l2_subdev_call(sd, video, querystd, arg); > > > > + > > > > + case VIDIOC_SUBDEV_G_ROUTING: > > > > + return v4l2_subdev_call(sd, pad, get_routing, arg); > > > > + > > > > + case VIDIOC_SUBDEV_S_ROUTING: { > > > > + struct v4l2_subdev_routing *route = arg; > > > > + unsigned int i; > > > > + > > > > + if (route->num_routes > sd->entity.num_pads) > > > > + return -EINVAL; > > > > > > Can't the number of routes exceeds the total number of pad? > > > > > > To make an example, a subdevice with 2 sink pads, and 1 multiplexed > > > source pad, with 2 streams would expose the following routing table, > > > right? > > > > > > pad #0 = sink, 1 stream > > > pad #1 = sink, 1 stream > > > pad #2 = source, 2 streams > > > > > > Routing table: > > > 0/0 -> 2/0 > > > 0/0 -> 2/1 > > > 1/0 -> 2/0 > > > 1/0 -> 2/1 > > > > > > In general, the number of accepted routes should depend on the number > > > of streams, not pads, and that's better handled by drivers, am I > > > wrong? > > > > Good point. Is the above configuration meaningful? I.e. you have a mux > > device that does some processing as well by combining the streams? > > > > It's far-fetched but at the moment the API does not really bend for that. > > It still might in the future. > > Well, how would you expect a routing table for a device that > supports multiplexing using Virtual Channels to look like? > > As far as I got it, even with a single sink and a single source, it > would be: > > [SINK/SINK_STREAM -> SOURCE/SOURCE_STREAM] > 0/0 -> 1/0 > 0/0 -> 1/1 > 0/0 -> 1/2 > 0/0 -> 1/3 > > On the previous example, I thought about GMSL-like devices, that can > output the video streams received from different remotes in a > separate virtual channel, at the same time. > > A possible routing table in that case would be like: > > Pads 0, 1, 2, 3 = SINKS > Pad 4 = SOURCE with 4 streams (1 for each VC) > > 0/0 -> 4/0 > 0/0 -> 4/1 > 0/0 -> 4/2 > 0/0 -> 4/3 > 1/0 -> 4/0 > 1/0 -> 4/1 > 1/0 -> 4/2 > 1/0 -> 4/3 > 2/0 -> 4/0 > 2/0 -> 4/1 > 2/0 -> 4/2 > 2/0 -> 4/3 > 3/0 -> 4/0 > 3/0 -> 4/1 > 3/0 -> 4/2 > 3/0 -> 4/3 If more than one pad can handle multiplexed streams, then you may end up in a situation like that. Indeed. > > With only one route per virtual channel active at a time. > > Does this match your idea? > > > > > I guess we could just remove the check, and let drivers handle it. > > > > Yup, I agree! > > > > > > > Thanks > > > j > > > > > > > + > > > > + for (i = 0; i < route->num_routes; ++i) { > > > > + unsigned int sink = route->routes[i].sink_pad; > > > > + unsigned int source = route->routes[i].source_pad; > > > > + struct media_pad *pads = sd->entity.pads; > > > > + > > > > + if (sink >= sd->entity.num_pads || > > > > + source >= sd->entity.num_pads) > > > > + return -EINVAL; > > > > + > > > > + if (!(pads[sink].flags & MEDIA_PAD_FL_SINK) || > > > > + !(pads[source].flags & MEDIA_PAD_FL_SOURCE)) > > > > + return -EINVAL; > > > > + } > > > > + > > > > + return v4l2_subdev_call(sd, pad, set_routing, route); > > > > + } > > > > #endif > > > > + > > > > default: > > > > return v4l2_subdev_call(sd, core, ioctl, cmd, arg); > > > > } > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > > > > index 9102d6ca566e01f2..5acaeeb9b3cacefa 100644 > > > > --- a/include/media/v4l2-subdev.h > > > > +++ b/include/media/v4l2-subdev.h > > > > @@ -679,6 +679,9 @@ struct v4l2_subdev_pad_config { > > > > * > > > > * @set_frame_desc: set the low level media bus frame parameters, @fd array > > > > * may be adjusted by the subdev driver to device capabilities. > > > > + * > > > > + * @get_routing: callback for VIDIOC_SUBDEV_G_ROUTING IOCTL handler. > > > > + * @set_routing: callback for VIDIOC_SUBDEV_S_ROUTING IOCTL handler. > > > > */ > > > > struct v4l2_subdev_pad_ops { > > > > int (*init_cfg)(struct v4l2_subdev *sd, > > > > @@ -719,6 +722,10 @@ struct v4l2_subdev_pad_ops { > > > > struct v4l2_mbus_frame_desc *fd); > > > > int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad, > > > > struct v4l2_mbus_frame_desc *fd); > > > > + int (*get_routing)(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_routing *route); > > > > + int (*set_routing)(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_routing *route); > > > > }; > > > > > > > > /** > > > > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > > > > index 03970ce3074193e6..af069bfb10ca23a5 100644 > > > > --- a/include/uapi/linux/v4l2-subdev.h > > > > +++ b/include/uapi/linux/v4l2-subdev.h > > > > @@ -155,6 +155,44 @@ struct v4l2_subdev_selection { > > > > __u32 reserved[8]; > > > > }; > > > > > > > > +#define V4L2_SUBDEV_ROUTE_FL_ACTIVE (1 << 0) > > > > +#define V4L2_SUBDEV_ROUTE_FL_IMMUTABLE (1 << 1) > > > > + > > > > +/** > > > > + * struct v4l2_subdev_route - A signal route inside a subdev > > > > + * @sink_pad: the sink pad > > > > + * @sink_stream: the sink stream > > > > + * @source_pad: the source pad > > > > + * @source_stream: the source stream > > > > + * @flags: route flags: > > > > + * > > > > + * V4L2_SUBDEV_ROUTE_FL_ACTIVE: Is the stream in use or not? An > > > > + * active stream will start when streaming is enabled on a video > > > > + * node. Set by the user. > > > > + * > > > > + * V4L2_SUBDEV_ROUTE_FL_IMMUTABLE: Is the stream immutable, i.e. > > > > + * can it be activated and inactivated? Set by the driver. > > > > + */ > > > > +struct v4l2_subdev_route { > > > > + __u32 sink_pad; > > > > + __u32 sink_stream; > > > > + __u32 source_pad; > > > > + __u32 source_stream; > > > > + __u32 flags; > > > > + __u32 reserved[5]; > > > > +}; > > > > + > > > > +/** > > > > + * struct v4l2_subdev_routing - Routing information > > > > + * @routes: the routes array > > > > + * @num_routes: the total number of routes in the routes array > > > > + */ > > > > +struct v4l2_subdev_routing { > > > > + struct v4l2_subdev_route *routes; > > > > This should be just __u64, to avoid writing compat code. The patch was > > written before we started doing that. :-) Please see e.g. the media device > > topology IOCTL. > > > > Thanks, I had a look at the MEDIA_ ioctls yesterday, G_TOPOLOGY in > particular, which uses several pointers to arrays. > > Unfortunately, I didn't come up with anything better than using a > translation structure, from the IOCTL layer to the subdevice > operations layer: > https://paste.debian.net/hidden/b192969d/ > (sharing a link for early comments, I can send v3 and you can comment > there directly if you prefer to :) Hmm. That is a downside indeed. It's still a lesser problem than the compat code in general --- which has been a source for bugs as well as nasty security problems over time. I think we need a naming scheme for such structs. How about just calling that struct e.g. v4l2_subdev_krouting instead? It's simple, easy to understand and it includes a suggestion which one is the kernel-only variant. You can btw. zero the struct memory by assigning { 0 } to it in declaration. memset() in general is much more trouble. In this case you could even do the assignments in delaration as well. There is a check that requires that one pad is a source and another is a sink; I think we could remove that check as well.
Hi Sakari,
thanks for your suggestions.
On Fri, Feb 22, 2019 at 01:04:29PM +0200, Sakari Ailus wrote:
> Hi Jacopo,
[snip]
> > On the previous example, I thought about GMSL-like devices, that can
> > output the video streams received from different remotes in a
> > separate virtual channel, at the same time.
> >
> > A possible routing table in that case would be like:
> >
> > Pads 0, 1, 2, 3 = SINKS
> > Pad 4 = SOURCE with 4 streams (1 for each VC)
> >
> > 0/0 -> 4/0
> > 0/0 -> 4/1
> > 0/0 -> 4/2
> > 0/0 -> 4/3
> > 1/0 -> 4/0
> > 1/0 -> 4/1
> > 1/0 -> 4/2
> > 1/0 -> 4/3
> > 2/0 -> 4/0
> > 2/0 -> 4/1
> > 2/0 -> 4/2
> > 2/0 -> 4/3
> > 3/0 -> 4/0
> > 3/0 -> 4/1
> > 3/0 -> 4/2
> > 3/0 -> 4/3
>
> If more than one pad can handle multiplexed streams, then you may end up in
> a situation like that. Indeed.
>
Please note that in this case there is only one pad that can handle
multiplexed stream. The size of the routing table is the
multiplication of the total number of pads by the product of all
streams per pad. In this case (4 * (1 * 1 * 1 * 4))
> >
> > With only one route per virtual channel active at a time.
[snip]
> >
> > Thanks, I had a look at the MEDIA_ ioctls yesterday, G_TOPOLOGY in
> > particular, which uses several pointers to arrays.
> >
> > Unfortunately, I didn't come up with anything better than using a
> > translation structure, from the IOCTL layer to the subdevice
> > operations layer:
> > https://paste.debian.net/hidden/b192969d/
> > (sharing a link for early comments, I can send v3 and you can comment
> > there directly if you prefer to :)
>
> Hmm. That is a downside indeed. It's still a lesser problem than the compat
> code in general --- which has been a source for bugs as well as nasty
> security problems over time.
>
Good!
> I think we need a naming scheme for such structs. How about just
> calling that struct e.g. v4l2_subdev_krouting instead? It's simple, easy to
> understand and it includes a suggestion which one is the kernel-only
> variant.
>
I kind of like that! thanks!
> You can btw. zero the struct memory by assigning { 0 } to it in
> declaration. memset() in general is much more trouble. In this case you
> could even do the assignments in delaration as well.
>
Thanks, noted. I have been lazy and copied memset from other places in
the ioctl handling code. I should check on your suggestions because I
remember one of the many 0-initialization statement was a GCC specific one,
don't remember which...
Initializing in-place would solve both issues :)
> There is a check that requires that one pad is a source and another is a
> sink; I think we could remove that check as well.
Correct, I'll handle that!
Thanks again
j
>
> --
> Regards,
>
> Sakari Ailus
> sakari.ailus@linux.intel.com
Hi Jacopo, On Fri, Feb 22, 2019 at 12:17:47PM +0100, Jacopo Mondi wrote: > Hi Sakari, > thanks for your suggestions. > > On Fri, Feb 22, 2019 at 01:04:29PM +0200, Sakari Ailus wrote: > > Hi Jacopo, > > [snip] > > > > On the previous example, I thought about GMSL-like devices, that can > > > output the video streams received from different remotes in a > > > separate virtual channel, at the same time. > > > > > > A possible routing table in that case would be like: > > > > > > Pads 0, 1, 2, 3 = SINKS > > > Pad 4 = SOURCE with 4 streams (1 for each VC) > > > > > > 0/0 -> 4/0 > > > 0/0 -> 4/1 > > > 0/0 -> 4/2 > > > 0/0 -> 4/3 > > > 1/0 -> 4/0 > > > 1/0 -> 4/1 > > > 1/0 -> 4/2 > > > 1/0 -> 4/3 > > > 2/0 -> 4/0 > > > 2/0 -> 4/1 > > > 2/0 -> 4/2 > > > 2/0 -> 4/3 > > > 3/0 -> 4/0 > > > 3/0 -> 4/1 > > > 3/0 -> 4/2 > > > 3/0 -> 4/3 > > > > If more than one pad can handle multiplexed streams, then you may end up in > > a situation like that. Indeed. > > > > Please note that in this case there is only one pad that can handle > multiplexed stream. The size of the routing table is the > multiplication of the total number of pads by the product of all > streams per pad. In this case (4 * (1 * 1 * 1 * 4)) Oh, good point, that's the case for G_ROUTING. I thought of S_ROUTING only. :-) > > > > > > > With only one route per virtual channel active at a time. > > [snip] > > > > > > > Thanks, I had a look at the MEDIA_ ioctls yesterday, G_TOPOLOGY in > > > particular, which uses several pointers to arrays. > > > > > > Unfortunately, I didn't come up with anything better than using a > > > translation structure, from the IOCTL layer to the subdevice > > > operations layer: > > > https://paste.debian.net/hidden/b192969d/ > > > (sharing a link for early comments, I can send v3 and you can comment > > > there directly if you prefer to :) > > > > Hmm. That is a downside indeed. It's still a lesser problem than the compat > > code in general --- which has been a source for bugs as well as nasty > > security problems over time. > > > > Good! > > > I think we need a naming scheme for such structs. How about just > > calling that struct e.g. v4l2_subdev_krouting instead? It's simple, easy to > > understand and it includes a suggestion which one is the kernel-only > > variant. > > > > I kind of like that! thanks! > > > You can btw. zero the struct memory by assigning { 0 } to it in > > declaration. memset() in general is much more trouble. In this case you > > could even do the assignments in delaration as well. > > > > Thanks, noted. I have been lazy and copied memset from other places in > the ioctl handling code. I should check on your suggestions because I > remember one of the many 0-initialization statement was a GCC specific one, > don't remember which... {} is GCC specific whereas { 0 } is not. But there have been long-standing GCC bugs related to the use of { 0 } which is quite unfortunate --- they've produced warnings or errors from code that is valid C...
Hi, On 22/02/2019 11:29, Sakari Ailus wrote: > Hi Jacopo, > > On Fri, Feb 22, 2019 at 12:17:47PM +0100, Jacopo Mondi wrote: >> Hi Sakari, >> thanks for your suggestions. >> >> On Fri, Feb 22, 2019 at 01:04:29PM +0200, Sakari Ailus wrote: >>> Hi Jacopo, >> >> [snip] >> >>>> On the previous example, I thought about GMSL-like devices, that can >>>> output the video streams received from different remotes in a >>>> separate virtual channel, at the same time. >>>> >>>> A possible routing table in that case would be like: >>>> >>>> Pads 0, 1, 2, 3 = SINKS >>>> Pad 4 = SOURCE with 4 streams (1 for each VC) >>>> >>>> 0/0 -> 4/0 >>>> 0/0 -> 4/1 >>>> 0/0 -> 4/2 >>>> 0/0 -> 4/3 >>>> 1/0 -> 4/0 >>>> 1/0 -> 4/1 >>>> 1/0 -> 4/2 >>>> 1/0 -> 4/3 >>>> 2/0 -> 4/0 >>>> 2/0 -> 4/1 >>>> 2/0 -> 4/2 >>>> 2/0 -> 4/3 >>>> 3/0 -> 4/0 >>>> 3/0 -> 4/1 >>>> 3/0 -> 4/2 >>>> 3/0 -> 4/3 >>> >>> If more than one pad can handle multiplexed streams, then you may end up in >>> a situation like that. Indeed. >>> >> >> Please note that in this case there is only one pad that can handle >> multiplexed stream. The size of the routing table is the >> multiplication of the total number of pads by the product of all >> streams per pad. In this case (4 * (1 * 1 * 1 * 4)) > > Oh, good point, that's the case for G_ROUTING. I thought of S_ROUTING only. > :-) > >> >>>> >>>> With only one route per virtual channel active at a time. >> >> [snip] >> >>>> >>>> Thanks, I had a look at the MEDIA_ ioctls yesterday, G_TOPOLOGY in >>>> particular, which uses several pointers to arrays. >>>> >>>> Unfortunately, I didn't come up with anything better than using a >>>> translation structure, from the IOCTL layer to the subdevice >>>> operations layer: >>>> https://paste.debian.net/hidden/b192969d/ >>>> (sharing a link for early comments, I can send v3 and you can comment >>>> there directly if you prefer to :) >>> >>> Hmm. That is a downside indeed. It's still a lesser problem than the compat >>> code in general --- which has been a source for bugs as well as nasty >>> security problems over time. >>> >> >> Good! >> >>> I think we need a naming scheme for such structs. How about just >>> calling that struct e.g. v4l2_subdev_krouting instead? It's simple, easy to >>> understand and it includes a suggestion which one is the kernel-only >>> variant. >>> >> >> I kind of like that! thanks! >> >>> You can btw. zero the struct memory by assigning { 0 } to it in >>> declaration. memset() in general is much more trouble. In this case you >>> could even do the assignments in delaration as well. >>> >> >> Thanks, noted. I have been lazy and copied memset from other places in >> the ioctl handling code. I should check on your suggestions because I >> remember one of the many 0-initialization statement was a GCC specific one, >> don't remember which... > > {} is GCC specific whereas { 0 } is not. But there have been long-standing > GCC bugs related to the use of { 0 } which is quite unfortunate --- they've > produced warnings or errors from code that is valid C... > Using = {} to intialise structs is all over the kernel and quite accepted. Eg. recent discussion with Mauro at [1]. [1] https://lore.kernel.org/linux-media/20181207105816.4c53aeaa@coco.lan/ Regards, Ian
Hi Sakari, On Fri, Feb 22, 2019 at 12:30 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > On Fri, Feb 22, 2019 at 12:17:47PM +0100, Jacopo Mondi wrote: > > On Fri, Feb 22, 2019 at 01:04:29PM +0200, Sakari Ailus wrote: > > > You can btw. zero the struct memory by assigning { 0 } to it in > > > declaration. memset() in general is much more trouble. In this case you > > > could even do the assignments in delaration as well. > > > > > > > Thanks, noted. I have been lazy and copied memset from other places in > > the ioctl handling code. I should check on your suggestions because I > > remember one of the many 0-initialization statement was a GCC specific one, > > don't remember which... > > {} is GCC specific whereas { 0 } is not. But there have been long-standing > GCC bugs related to the use of { 0 } which is quite unfortunate --- they've > produced warnings or errors from code that is valid C... Most of these are gone since commit cafa0010cd51fb71 ("Raise the minimum required gcc version to 4.6"). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index 7de041bae84fb2f2..40406acb51ec0906 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -19,6 +19,7 @@ #include <linux/kernel.h> #include <linux/version.h> +#include <linux/v4l2-subdev.h> #include <linux/videodev2.h> #include <media/v4l2-common.h> @@ -2924,6 +2925,23 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size, } break; } + + case VIDIOC_SUBDEV_G_ROUTING: + case VIDIOC_SUBDEV_S_ROUTING: { + struct v4l2_subdev_routing *route = parg; + + if (route->num_routes > 0) { + if (route->num_routes > 256) + return -EINVAL; + + *user_ptr = (void __user *)route->routes; + *kernel_ptr = (void *)&route->routes; + *array_size = sizeof(struct v4l2_subdev_route) + * route->num_routes; + ret = 1; + } + break; + } } return ret; @@ -3033,7 +3051,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg, * Some ioctls can return an error, but still have valid * results that must be returned. */ - if (err < 0 && !always_copy) + if (err < 0 && !always_copy && cmd != VIDIOC_SUBDEV_G_ROUTING) goto out; out_array_args: diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 792f41dffe2329b9..1d3b37cf548fa533 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -516,7 +516,35 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) case VIDIOC_SUBDEV_QUERYSTD: return v4l2_subdev_call(sd, video, querystd, arg); + + case VIDIOC_SUBDEV_G_ROUTING: + return v4l2_subdev_call(sd, pad, get_routing, arg); + + case VIDIOC_SUBDEV_S_ROUTING: { + struct v4l2_subdev_routing *route = arg; + unsigned int i; + + if (route->num_routes > sd->entity.num_pads) + return -EINVAL; + + for (i = 0; i < route->num_routes; ++i) { + unsigned int sink = route->routes[i].sink_pad; + unsigned int source = route->routes[i].source_pad; + struct media_pad *pads = sd->entity.pads; + + if (sink >= sd->entity.num_pads || + source >= sd->entity.num_pads) + return -EINVAL; + + if (!(pads[sink].flags & MEDIA_PAD_FL_SINK) || + !(pads[source].flags & MEDIA_PAD_FL_SOURCE)) + return -EINVAL; + } + + return v4l2_subdev_call(sd, pad, set_routing, route); + } #endif + default: return v4l2_subdev_call(sd, core, ioctl, cmd, arg); } diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index 9102d6ca566e01f2..5acaeeb9b3cacefa 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -679,6 +679,9 @@ struct v4l2_subdev_pad_config { * * @set_frame_desc: set the low level media bus frame parameters, @fd array * may be adjusted by the subdev driver to device capabilities. + * + * @get_routing: callback for VIDIOC_SUBDEV_G_ROUTING IOCTL handler. + * @set_routing: callback for VIDIOC_SUBDEV_S_ROUTING IOCTL handler. */ struct v4l2_subdev_pad_ops { int (*init_cfg)(struct v4l2_subdev *sd, @@ -719,6 +722,10 @@ struct v4l2_subdev_pad_ops { struct v4l2_mbus_frame_desc *fd); int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad, struct v4l2_mbus_frame_desc *fd); + int (*get_routing)(struct v4l2_subdev *sd, + struct v4l2_subdev_routing *route); + int (*set_routing)(struct v4l2_subdev *sd, + struct v4l2_subdev_routing *route); }; /** diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h index 03970ce3074193e6..af069bfb10ca23a5 100644 --- a/include/uapi/linux/v4l2-subdev.h +++ b/include/uapi/linux/v4l2-subdev.h @@ -155,6 +155,44 @@ struct v4l2_subdev_selection { __u32 reserved[8]; }; +#define V4L2_SUBDEV_ROUTE_FL_ACTIVE (1 << 0) +#define V4L2_SUBDEV_ROUTE_FL_IMMUTABLE (1 << 1) + +/** + * struct v4l2_subdev_route - A signal route inside a subdev + * @sink_pad: the sink pad + * @sink_stream: the sink stream + * @source_pad: the source pad + * @source_stream: the source stream + * @flags: route flags: + * + * V4L2_SUBDEV_ROUTE_FL_ACTIVE: Is the stream in use or not? An + * active stream will start when streaming is enabled on a video + * node. Set by the user. + * + * V4L2_SUBDEV_ROUTE_FL_IMMUTABLE: Is the stream immutable, i.e. + * can it be activated and inactivated? Set by the driver. + */ +struct v4l2_subdev_route { + __u32 sink_pad; + __u32 sink_stream; + __u32 source_pad; + __u32 source_stream; + __u32 flags; + __u32 reserved[5]; +}; + +/** + * struct v4l2_subdev_routing - Routing information + * @routes: the routes array + * @num_routes: the total number of routes in the routes array + */ +struct v4l2_subdev_routing { + struct v4l2_subdev_route *routes; + __u32 num_routes; + __u32 reserved[5]; +}; + /* Backwards compatibility define --- to be removed */ #define v4l2_subdev_edid v4l2_edid @@ -181,5 +219,7 @@ struct v4l2_subdev_selection { #define VIDIOC_SUBDEV_ENUM_DV_TIMINGS _IOWR('V', 98, struct v4l2_enum_dv_timings) #define VIDIOC_SUBDEV_QUERY_DV_TIMINGS _IOR('V', 99, struct v4l2_dv_timings) #define VIDIOC_SUBDEV_DV_TIMINGS_CAP _IOWR('V', 100, struct v4l2_dv_timings_cap) +#define VIDIOC_SUBDEV_G_ROUTING _IOWR('V', 38, struct v4l2_subdev_routing) +#define VIDIOC_SUBDEV_S_ROUTING _IOWR('V', 39, struct v4l2_subdev_routing) #endif