Re: [PATCH v2 16/30] v4l: subdev: Add [GS]_ROUTING subdev ioctls and operations

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com