Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-media@vger.kernel.org, sakari.ailus@linux.intel.com,
	Jacopo Mondi <jacopo+renesas@jmondi.org>,
	niklas.soderlund+renesas@ragnatech.se,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	Michal Simek <michal.simek@xilinx.com>
Subject: Re: [PATCH v5 18/24] v4l: subdev: Add [GS]_ROUTING subdev ioctls and operations
Date: Thu, 22 Apr 2021 19:58:51 +0300
Message-ID: <175fb74c-bc04-681d-bc96-507f2cfdac0d@ideasonboard.com> (raw)
In-Reply-To: <YIGiOZDR/PP0Rt+I@pendragon.ideasonboard.com>

On 22/04/2021 19:20, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Thu, Apr 22, 2021 at 02:16:10PM +0300, Tomi Valkeinen wrote:
>> On 18/04/2021 21:32, Laurent Pinchart wrote:
>>> On Thu, Apr 15, 2021 at 04:04:44PM +0300, 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.get_routing() and
>>>> v4l2_subdev_pad_ops.set_routing().
>>>>
>>>> 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>
>>>>
>>>> - 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
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>
>>> Seems few people haven't contributed to this patch one way or another
>>> :-)
>>
>> The more the merrier!
>>
>>>> ---
>>>>    drivers/media/v4l2-core/v4l2-ioctl.c  | 25 ++++++++++++++-
>>>>    drivers/media/v4l2-core/v4l2-subdev.c | 45 +++++++++++++++++++++++++++
>>>>    include/media/v4l2-subdev.h           | 24 ++++++++++++++
>>>>    include/uapi/linux/v4l2-subdev.h      | 44 ++++++++++++++++++++++++++
>>>>    4 files changed, 137 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>>>> index 6a5d1c6d11d6..f5732962753f 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>
>>>> @@ -3108,6 +3109,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 *route = parg;
>>>
>>> s/route/routing/ to match the code below.
>>>
>>>> +
>>>> +		if (route->num_routes > 256)
>>>> +			return -EINVAL;
>>>> +
>>>> +		*user_ptr = u64_to_user_ptr(route->routes);
>>>> +		*kernel_ptr = (void **)&route->routes;
>>>> +		*array_size = sizeof(struct v4l2_subdev_route)
>>>> +			    * route->num_routes;
>>>> +		ret = 1;
>>>> +		break;
>>>> +	}
>>>>    	}
>>>>    
>>>>    	return ret;
>>>> @@ -3369,8 +3385,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 956dafab43d4..95a4c3091fa6 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>>> @@ -681,6 +681,51 @@ 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 = {
>>>> +			.which = routing->which,
>>>> +			.num_routes = routing->num_routes,
>>>> +			.routes = (struct v4l2_subdev_route *)(uintptr_t)routing->routes,
>>>> +		};
>>>> +		int ret;
>>>> +
>>>> +		ret = v4l2_subdev_call(sd, pad, get_routing, &krouting);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +
>>>> +		routing->num_routes = krouting.num_routes;
>>>
>>> Shouldn't num_routes be set even in case of errors ?
>>
>> Yes, you're right.
>>
>>>> +
>>>> +		return 0;
>>>> +	}
>>>> +
>>>> +	case VIDIOC_SUBDEV_S_ROUTING: {
>>>> +		struct v4l2_subdev_routing *routing = arg;
>>>> +		struct v4l2_subdev_route *route = (struct v4l2_subdev_route *)(uintptr_t)
>>>> +						  routing->routes;
>>>
>>> I'd name the variable routes as it points to an array.
>>>
>>>> +		struct v4l2_subdev_krouting krouting = {};
>>>> +		unsigned int i;
>>>> +
>>>> +		if (routing->which != V4L2_SUBDEV_FORMAT_TRY && ro_subdev)
>>>> +			return -EPERM;
>>>> +
>>>> +		for (i = 0; i < routing->num_routes; ++i) {
>>>> +			if (route[i].sink_pad >= sd->entity.num_pads ||
>>>> +			    route[i].source_pad >= sd->entity.num_pads)
>>>> +				return -EINVAL;
>>>> +
>>>> +			if (!(sd->entity.pads[route[i].sink_pad].flags & MEDIA_PAD_FL_SINK) ||
>>>> +			    !(sd->entity.pads[route[i].source_pad].flags & MEDIA_PAD_FL_SOURCE))
>>>> +				return -EINVAL;
>>>> +		}
>>>> +
>>>> +		krouting.which = routing->which;
>>>> +		krouting.num_routes = routing->num_routes;
>>>> +		krouting.routes = route;
>>>> +
>>>> +		return v4l2_subdev_call(sd, pad, set_routing, &krouting);
>>>> +	}
>>>> +
>>>>    	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 436d0445aafd..3826ab918731 100644
>>>> --- a/include/media/v4l2-subdev.h
>>>> +++ b/include/media/v4l2-subdev.h
>>>> @@ -650,6 +650,22 @@ struct v4l2_subdev_pad_config {
>>>>    	struct v4l2_rect try_compose;
>>>>    };
>>>>    
>>>> +/**
>>>> + * struct v4l2_subdev_krouting - subdev routing table
>>>> + *
>>>> + * @which: format type (from enum v4l2_subdev_format_whence)
>>>> + * @routes: &struct v4l2_subdev_route
>>>> + * @num_routes: number of routes
>>>> + *
>>>> + * This structure is used to translate argument received from
>>>
>>> s/argument/the arguments/
>>>
>>>> + * VIDIOC_SUBDEV_G/S_ROUTING() ioctl to sudev device drivers operations.
>>>
>>> s/sudev/subdev/
>>>
>>>> + */
>>>> +struct v4l2_subdev_krouting {
>>>> +	u32 which;
>>>> +	struct v4l2_subdev_route *routes;
>>>> +	unsigned int num_routes;
>>>> +};
>>>> +
>>>>    /**
>>>>     * struct v4l2_subdev_pad_ops - v4l2-subdev pad level operations
>>>>     *
>>>> @@ -711,6 +727,10 @@ struct v4l2_subdev_pad_config {
>>>>     *		     applied to the hardware. The operation shall fail if the
>>>>     *		     pad index it has been called on is not valid or in case of
>>>>     *		     unrecoverable failures.
>>>> + *
>>>> + * @get_routing: get the subdevice routing table.
>>>> + * @set_routing: enable or disable data connection routes described in the
>>>> + *		 subdevice routing table.
>>>>     */
>>>>    struct v4l2_subdev_pad_ops {
>>>>    	int (*init_cfg)(struct v4l2_subdev *sd,
>>>> @@ -755,6 +775,10 @@ struct v4l2_subdev_pad_ops {
>>>>    			       struct v4l2_mbus_config *config);
>>>>    	int (*set_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
>>>>    			       struct v4l2_mbus_config *config);
>>>> +	int (*get_routing)(struct v4l2_subdev *sd,
>>>> +			   struct v4l2_subdev_krouting *route);
>>>> +	int (*set_routing)(struct v4l2_subdev *sd,
>>>> +			   struct v4l2_subdev_krouting *route);
>>>>    };
>>>>    
>>>>    /**
>>>> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
>>>> index 658106f5b5dc..f2a17cbd1e9a 100644
>>>> --- a/include/uapi/linux/v4l2-subdev.h
>>>> +++ b/include/uapi/linux/v4l2-subdev.h
>>>> @@ -188,6 +188,48 @@ struct v4l2_subdev_capability {
>>>>    /* The v4l2 sub-device video device node is registered in read-only mode. */
>>>>    #define V4L2_SUBDEV_CAP_RO_SUBDEV		0x00000001
>>>>    
>>>> +#define V4L2_SUBDEV_ROUTE_FL_ACTIVE		BIT(0)
>>>> +#define V4L2_SUBDEV_ROUTE_FL_IMMUTABLE		BIT(1)
>>>> +
>>>> +/**
>>>> + * struct v4l2_subdev_route - A route inside a subdev
>>>> + *
>>>> + * @sink_pad: the sink pad index
>>>> + * @sink_stream: the sink stream identifier
>>>> + * @source_pad: the source pad index
>>>> + * @source_stream: the source stream identifier
>>>> + * @flags: route flags:
>>>> + *
>>>> + *	V4L2_SUBDEV_ROUTE_FL_ACTIVE: Is the route active? An active
>>>> + *	route will start when streaming is enabled on a video node.
>>>> + *	Set by the user.
>>>> + *
>>>> + *	V4L2_SUBDEV_ROUTE_FL_IMMUTABLE: Is the route immutable, i.e.
>>>> + *	can it be activated and inactivated? Set by the driver.
>>>
>>> This should be moved to the macros.
>>>
>>>> + */
>>>> +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 - Subdev routing information
>>>> + *
>>>> + * @which: format type (from enum v4l2_subdev_format_whence)
>>>
>>> v4l2_subdev_format_whence shoudn't have had "format" in its name :-S We
>>> can't do much about that, but should "format type" be changed to "routes
>>> type", or maybe "configuration type" ?
>>>
>>>> + * @routes: pointer to the routes array
>>>> + * @num_routes: the total number of routes in the routes array
>>>
>>> Won't kerneldoc complain about the undocumented reserved field ?
>>>
>>>> + */
>>>> +struct v4l2_subdev_routing {
>>>> +	__u32 which;
>>>> +	__u64 routes;
>>>> +	__u32 num_routes;
>>>> +	__u32 reserved[5];
>>>> +};
>>>> +
>>>>    /* Backwards compatibility define --- to be removed */
>>>>    #define v4l2_subdev_edid v4l2_edid
>>>>    
>>>> @@ -215,5 +257,7 @@ struct v4l2_subdev_capability {
>>>>    #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)
>>>
>>> I would keep the ioctls sorted by number.
>>
>> The ioctls are not sorted (even if it may look like it above =). I'm not
>> sure if there's a better place for them than the bottom.
> 
> Isn't the last block of ioctls (the ones reusing the V4L2 video node
> numbers) sorted ?

Oh, right, true. But, actually, these new ioctls shouldn't be here, as 
they are not "identical to the ioctls in videodev2.h". They should be 
moved above that comment, and the ioctls above that comment are not sorted.

  Tomi

  reply index

Thread overview: 72+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-15 13:04 [PATCH v5 00/24] v4l: subdev internal routing Tomi Valkeinen
2021-04-15 13:04 ` [PATCH v5 01/24] media: entity: Use pad as a starting point for graph walk Tomi Valkeinen
2021-04-15 13:04 ` [PATCH v5 02/24] media: entity: Use pads instead of entities in the media graph walk stack Tomi Valkeinen
2021-04-15 13:04 ` [PATCH v5 03/24] media: entity: Walk the graph based on pads Tomi Valkeinen
2021-04-18 17:47   ` Laurent Pinchart
2021-04-20 11:30     ` Sakari Ailus
2021-04-15 13:04 ` [PATCH v5 04/24] v4l: mc: Start walk from a specific pad in use count calculation Tomi Valkeinen
2021-04-15 13:04 ` [PATCH v5 05/24] media: entity: Add iterator helper for entity pads Tomi Valkeinen
2021-04-18 17:52   ` Laurent Pinchart
2021-04-22 12:04     ` Tomi Valkeinen
2021-04-15 13:04 ` [PATCH v5 06/24] media: entity: Move the pipeline from entity to pads Tomi Valkeinen
2021-04-18 18:00   ` Laurent Pinchart
2021-04-20 11:38     ` Sakari Ailus
2021-04-15 13:04 ` [PATCH v5 07/24] media: entity: Use pad as the starting point for a pipeline Tomi Valkeinen
2021-04-15 13:04 ` [PATCH v5 08/24] media: entity: Add has_route entity operation Tomi Valkeinen
2021-04-15 13:04 ` [PATCH v5 09/24] media: entity: Add media_entity_has_route() function Tomi Valkeinen
2021-04-15 13:04 ` [PATCH v5 10/24] media: entity: Use routing information during graph traversal Tomi Valkeinen
2021-04-15 13:04 ` [PATCH v5 11/24] media: entity: Skip link validation for pads to which there is no route to Tomi Valkeinen
2021-04-18 18:06   ` Laurent Pinchart
2021-04-20 11:41     ` Sakari Ailus
2021-04-23 12:37       ` Tomi Valkeinen
2021-04-29 12:06         ` Sakari Ailus
2021-04-29 14:10           ` Laurent Pinchart
2021-04-15 13:04 ` [PATCH v5 12/24] media: entity: Add an iterator helper for connected pads Tomi Valkeinen
2021-04-18 18:20   ` Laurent Pinchart
2021-04-20 11:48     ` Sakari Ailus
2021-04-29  1:33       ` Laurent Pinchart
2021-04-29 11:56         ` Sakari Ailus
2021-04-29 12:04           ` Tomi Valkeinen
2021-04-29 12:07             ` Sakari Ailus
2021-04-29 12:14               ` Tomi Valkeinen
2021-04-15 13:04 ` [PATCH v5 13/24] media: entity: Add only connected pads to the pipeline Tomi Valkeinen
2021-04-15 13:04 ` [PATCH v5 14/24] media: entity: Add debug information in graph walk route check Tomi Valkeinen
2021-04-15 13:04 ` [PATCH v5 15/24] v4l: Add bus type to frame descriptors Tomi Valkeinen
2021-04-18 19:23   ` Laurent Pinchart
2021-04-20 11:50     ` Sakari Ailus
2021-04-22 12:30       ` Tomi Valkeinen
2021-04-29 11:58         ` Sakari Ailus
2021-04-29 14:09           ` Laurent Pinchart
2021-04-15 13:04 ` [PATCH v5 16/24] v4l: Add CSI-2 bus configuration " Tomi Valkeinen
2021-04-18 19:24   ` Laurent Pinchart
2021-04-20 16:32     ` Sakari Ailus
2021-04-15 13:04 ` [PATCH v5 17/24] v4l: Add stream to frame descriptor Tomi Valkeinen
2021-04-18 19:27   ` Laurent Pinchart
2021-04-22 12:47     ` Tomi Valkeinen
2021-04-22 16:18       ` Laurent Pinchart
2021-04-15 13:04 ` [PATCH v5 18/24] v4l: subdev: Add [GS]_ROUTING subdev ioctls and operations Tomi Valkeinen
2021-04-18 18:32   ` Laurent Pinchart
2021-04-22 11:16     ` Tomi Valkeinen
2021-04-22 16:20       ` Laurent Pinchart
2021-04-22 16:58         ` Tomi Valkeinen [this message]
2021-04-20 16:35   ` Sakari Ailus
2021-04-15 13:04 ` [PATCH v5 19/24] media: Documentation: Add GS_ROUTING documentation Tomi Valkeinen
2021-04-15 13:04 ` [PATCH v5 20/24] v4l: mc: Add an S_ROUTING helper function for power state changes Tomi Valkeinen
2021-04-18 18:55   ` Laurent Pinchart
2021-04-20 16:41     ` Sakari Ailus
2021-04-15 13:04 ` [PATCH v5 21/24] v4l: subdev: routing kernel helper functions Tomi Valkeinen
2021-04-18 19:18   ` Laurent Pinchart
2021-04-15 13:04 ` [PATCH v5 22/24] v4l: subdev: add v4l2_subdev_get_format_dir() Tomi Valkeinen
2021-04-18 19:04   ` Laurent Pinchart
2021-04-21 13:04     ` Tomi Valkeinen
2021-04-29  1:43       ` Laurent Pinchart
2021-05-04  6:49         ` Tomi Valkeinen
2021-04-15 13:04 ` [PATCH v5 23/24] v4l: subdev: Take routing information into account in link validation Tomi Valkeinen
2021-04-15 13:04 ` [PATCH v5 24/24] v4l: subdev: increase V4L2_FRAME_DESC_ENTRY_MAX to 8 Tomi Valkeinen
2021-04-18 19:06   ` Laurent Pinchart
2021-04-16  8:38 ` [PATCH v5 00/24] v4l: subdev internal routing Niklas Söderlund
2021-04-16  8:47   ` Tomi Valkeinen
2021-04-16  8:56     ` Niklas Söderlund
2021-04-18 17:32 ` Laurent Pinchart
2021-04-21 12:57   ` Tomi Valkeinen
2021-04-29  1:27     ` Laurent Pinchart

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=175fb74c-bc04-681d-bc96-507f2cfdac0d@ideasonboard.com \
    --to=tomi.valkeinen@ideasonboard.com \
    --cc=hverkuil-cisco@xs4all.nl \
    --cc=jacopo+renesas@jmondi.org \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@kernel.org \
    --cc=michal.simek@xilinx.com \
    --cc=niklas.soderlund+renesas@ragnatech.se \
    --cc=sakari.ailus@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git