Re: [PATCH v11 22/36] media: subdev: Add [GS]_ROUTING subdev ioctls and operations

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

Hi Tomasz,

On 07/07/2022 12:15, Tomasz Figa wrote:
> Hi Tomi, Laurent,
> 
> On Tue, Mar 01, 2022 at 06:11:42PM +0200, Tomi Valkeinen wrote:
>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>> Add support for subdev internal routing. A route is defined as a single
>> stream from a sink pad to a source pad.
>>
>> The userspace can configure the routing via two new ioctls,
>> VIDIOC_SUBDEV_G_ROUTING and VIDIOC_SUBDEV_S_ROUTING, and subdevs can
>> implement the functionality with v4l2_subdev_pad_ops.set_routing().
> 
> Thanks for the patch! Please check my comment inline.
> 
>>
>> 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>
>>
>> - Expand and refine documentation.
>> - Make the 'routes' pointer a __u64 __user pointer so that a compat32
>>    version of the ioctl is not required.
>> - Add struct v4l2_subdev_krouting to be used for subdevice operations.
>>
>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>>
>> - Fix typecasing warnings
>> - Check sink & source pad types
>> - Add 'which' field
>> - Add V4L2_SUBDEV_ROUTE_FL_SOURCE
>> - Routing to subdev state
>> - Dropped get_routing subdev op
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/media/v4l2-core/v4l2-ioctl.c  | 25 ++++++++-
>>   drivers/media/v4l2-core/v4l2-subdev.c | 73 +++++++++++++++++++++++++++
>>   include/media/v4l2-subdev.h           | 22 ++++++++
>>   include/uapi/linux/v4l2-subdev.h      | 57 +++++++++++++++++++++
>>   4 files changed, 176 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index 642cb90f457c..add3b28d446e 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -16,6 +16,7 @@
>>   #include <linux/kernel.h>
>>   #include <linux/version.h>
>>   
>> +#include <linux/v4l2-subdev.h>
>>   #include <linux/videodev2.h>
>>   
>>   #include <media/v4l2-common.h>
>> @@ -3093,6 +3094,21 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
>>   		ret = 1;
>>   		break;
>>   	}
>> +
>> +	case VIDIOC_SUBDEV_G_ROUTING:
>> +	case VIDIOC_SUBDEV_S_ROUTING: {
>> +		struct v4l2_subdev_routing *routing = parg;
>> +
>> +		if (routing->num_routes > 256)
> 
> Should we define and document this constant?

It's just an arbitrary high number as a sanity check. We don't have any 
specific reason to limit the number of routes. If we publicize it to the 
userspace, then the userspace might depend on it. On the other hand, 
failing mystically when num-routes > 256 is also not so nice (not that 
we have any drivers that go there, 8 has been the max so far).

I think we should just keep it here internally, and try to make sure to 
increase it if we ever get drivers that might support more routes.

>> +			return -EINVAL;
>> +
>> +		*user_ptr = u64_to_user_ptr(routing->routes);
>> +		*kernel_ptr = (void **)&routing->routes;
>> +		*array_size = sizeof(struct v4l2_subdev_route)
>> +			    * routing->num_routes;
>> +		ret = 1;
>> +		break;
>> +	}
>>   	}
>>   
>>   	return ret;
>> @@ -3356,8 +3372,15 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg,
>>   	/*
>>   	 * Some ioctls can return an error, but still have valid
>>   	 * results that must be returned.
>> +	 *
>> +	 * FIXME: subdev IOCTLS are partially handled here and partially in
>> +	 * v4l2-subdev.c and the 'always_copy' flag can only be set for IOCTLS
>> +	 * defined here as part of the 'v4l2_ioctls' array. As
>> +	 * VIDIOC_SUBDEV_G_ROUTING needs to return results to applications even
>> +	 * in case of failure, but it is not defined here as part of the
>> +	 * 'v4l2_ioctls' array, insert an ad-hoc check to address that.
>>   	 */
>> -	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 3ad24093abe9..89c97bde4575 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -377,6 +377,10 @@ subdev_ioctl_get_state(struct v4l2_subdev *sd, struct v4l2_subdev_fh *subdev_fh,
>>   	case VIDIOC_SUBDEV_S_SELECTION:
>>   		which = ((struct v4l2_subdev_selection *)arg)->which;
>>   		break;
>> +	case VIDIOC_SUBDEV_G_ROUTING:
>> +	case VIDIOC_SUBDEV_S_ROUTING:
>> +		which = ((struct v4l2_subdev_routing *)arg)->which;
>> +		break;
>>   	}
>>   
>>   	return which == V4L2_SUBDEV_FORMAT_TRY ?
>> @@ -692,6 +696,74 @@ 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: {
>> +		struct v4l2_subdev_routing *routing = arg;
>> +		struct v4l2_subdev_krouting *krouting;
>> +
>> +		if (!(sd->flags & V4L2_SUBDEV_FL_MULTIPLEXED))
>> +			return -ENOIOCTLCMD;
>> +
>> +		memset(routing->reserved, 0, sizeof(routing->reserved));
>> +
>> +		krouting = &state->routing;
>> +
>> +		if (routing->num_routes < krouting->num_routes) {
>> +			routing->num_routes = krouting->num_routes;
>> +			return -ENOSPC;
>> +		}
>> +
>> +		memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes,
>> +		       krouting->routes,
>> +		       krouting->num_routes * sizeof(*krouting->routes));
>> +		routing->num_routes = krouting->num_routes;
>> +
>> +		return 0;
>> +	}
>> +
>> +	case VIDIOC_SUBDEV_S_ROUTING: {
>> +		struct v4l2_subdev_routing *routing = arg;
>> +		struct v4l2_subdev_route *routes =
>> +			(struct v4l2_subdev_route *)(uintptr_t)routing->routes;
>> +		struct v4l2_subdev_krouting krouting = {};
>> +		unsigned int i;
>> +
>> +		if (!(sd->flags & V4L2_SUBDEV_FL_MULTIPLEXED))
>> +			return -ENOIOCTLCMD;
>> +
>> +		if (routing->which != V4L2_SUBDEV_FORMAT_TRY && ro_subdev)
>> +			return -EPERM;
>> +
>> +		memset(routing->reserved, 0, sizeof(routing->reserved));
>> +
>> +		for (i = 0; i < routing->num_routes; ++i) {
>> +			const struct v4l2_subdev_route *route = &routes[i];
>> +			const struct media_pad *pads = sd->entity.pads;
>> +
>> +			/* Do not check sink pad for source routes */
>> +			if (!(route->flags & V4L2_SUBDEV_ROUTE_FL_SOURCE)) {
>> +				if (route->sink_pad >= sd->entity.num_pads)
>> +					return -EINVAL;
>> +
>> +				if (!(pads[route->sink_pad].flags &
>> +				      MEDIA_PAD_FL_SINK))
>> +					return -EINVAL;
>> +			}
>> +
>> +			if (route->source_pad >= sd->entity.num_pads)
>> +				return -EINVAL;
>> +
>> +			if (!(pads[route->source_pad].flags &
>> +			      MEDIA_PAD_FL_SOURCE))
>> +				return -EINVAL;
>> +		}
>> +
>> +		krouting.num_routes = routing->num_routes;
>> +		krouting.routes = routes;
>> +
>> +		return v4l2_subdev_call(sd, pad, set_routing, state,
>> +					routing->which, &krouting);
>> +	}
>> +
>>   	default:
>>   		return v4l2_subdev_call(sd, core, ioctl, cmd, arg);
>>   	}
>> @@ -979,6 +1051,7 @@ void __v4l2_subdev_state_free(struct v4l2_subdev_state *state)
>>   
>>   	mutex_destroy(&state->_lock);
>>   
>> +	kfree(state->routing.routes);
> 
> Do we have any guarantee that this array was allocated with kmalloc()?
> Maybe kvfree() could be more appropriate here?

The array is allocated with kmemdup() in v4l2_subdev_set_routing().

  Tomi