Linux-Renesas-SoC Archive on lore.kernel.org
 help / color / Atom feed
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: "Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>,
	Benoit Parrot <bparrot@ti.com>,
	linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
	Michal Simek <michal.simek@xilinx.com>
Subject: Re: [PATCH v2 16/30] v4l: subdev: Add [GS]_ROUTING subdev ioctls and operations
Date: Wed, 16 Jan 2019 01:51:45 +0200
Message-ID: <20190115235145.GF31088@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20181101233144.31507-17-niklas.soderlund+renesas@ragnatech.se>

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
> 

-- 
Regards,

Laurent Pinchart

  reply index

Thread overview: 90+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-01 23:31 [PATCH v2 00/30] v4l: add support for multiplexed streams Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 01/30] media: entity: Use pad as a starting point for graph walk Niklas Söderlund
2019-01-15 21:43   ` Laurent Pinchart
2018-11-01 23:31 ` [PATCH v2 02/30] media: entity: Use pads instead of entities in the media graph walk stack Niklas Söderlund
2019-01-15 22:03   ` Laurent Pinchart
2019-01-15 22:13     ` Sakari Ailus
2019-01-15 22:07   ` Laurent Pinchart
2018-11-01 23:31 ` [PATCH v2 03/30] media: entity: Walk the graph based on pads Niklas Söderlund
2019-01-15 22:21   ` Laurent Pinchart
     [not found]     ` <20190115223406.mxgzl36cp54gb7nv@kekkonen.localdomain>
2019-01-15 23:28       ` Laurent Pinchart
2019-01-22 14:50         ` Sakari Ailus
2019-02-14 15:15   ` Jacopo Mondi
2018-11-01 23:31 ` [PATCH v2 04/30] v4l: mc: Start walk from a specific pad in use count calculation Niklas Söderlund
2019-01-15 22:24   ` Laurent Pinchart
2019-01-15 22:36     ` Sakari Ailus
2018-11-01 23:31 ` [PATCH v2 05/30] media: entity: Move the pipeline from entity to pads Niklas Söderlund
2019-01-15 22:38   ` Laurent Pinchart
2019-01-15 22:48     ` Sakari Ailus
2019-02-14 15:53   ` Jacopo Mondi
2018-11-01 23:31 ` [PATCH v2 06/30] media: entity: Use pad as the starting point for a pipeline Niklas Söderlund
2019-01-15 22:54   ` Laurent Pinchart
2019-01-22 15:31     ` Sakari Ailus
2019-01-22 15:37       ` Laurent Pinchart
2019-01-22 16:16         ` Sakari Ailus
2018-11-01 23:31 ` [PATCH v2 07/30] media: entity: Add has_route entity operation Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 08/30] media: entity: Add media_has_route() function Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 09/30] media: entity: Swap pads if route is checked from source to sink Niklas Söderlund
2019-01-15 22:57   ` Laurent Pinchart
2019-01-22 15:15     ` Sakari Ailus
2019-01-22 15:20       ` Laurent Pinchart
2019-02-18  9:21         ` Jacopo Mondi
2019-02-22 12:18           ` Laurent Pinchart
2019-03-04 12:35             ` Jacopo Mondi
2019-03-05 20:04               ` Laurent Pinchart
2019-03-06  8:29                 ` Jacopo Mondi
2018-11-01 23:31 ` [PATCH v2 10/30] media: entity: Use routing information during graph traversal Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 11/30] media: entity: Skip link validation for pads to which there is no route to Niklas Söderlund
2019-01-15 23:13   ` Laurent Pinchart
2018-11-01 23:31 ` [PATCH v2 12/30] media: entity: Add an iterator helper for connected pads Niklas Söderlund
2019-01-15 23:24   ` Laurent Pinchart
2019-01-22 15:36     ` Sakari Ailus
2019-01-22 15:38       ` Laurent Pinchart
2019-01-22 16:21         ` Sakari Ailus
2018-11-01 23:31 ` [PATCH v2 13/30] media: entity: Add only connected pads to the pipeline Niklas Söderlund
2019-01-15 23:33   ` Laurent Pinchart
2018-11-01 23:31 ` [PATCH v2 14/30] media: entity: Add debug information in graph walk route check Niklas Söderlund
2019-01-15 23:35   ` Laurent Pinchart
2019-01-22 15:38     ` Sakari Ailus
2018-11-01 23:31 ` [PATCH v2 15/30] media: entity: Look for indirect routes Niklas Söderlund
2019-01-15 23:41   ` Laurent Pinchart
2019-01-22 15:56     ` Sakari Ailus
2018-11-01 23:31 ` [PATCH v2 16/30] v4l: subdev: Add [GS]_ROUTING subdev ioctls and operations Niklas Söderlund
2019-01-15 23:51   ` Laurent Pinchart [this message]
2019-01-22 16:14     ` Sakari Ailus
2019-01-22 17:00       ` Laurent Pinchart
2019-02-21 14:59     ` Jacopo Mondi
2019-02-21 23:49       ` Sakari Ailus
2019-02-22  8:46         ` Jacopo Mondi
2019-02-21 14:39   ` Jacopo Mondi
2019-02-21 22:31     ` Sakari Ailus
2019-02-22  8:40       ` Jacopo Mondi
2019-02-22 11:04         ` Sakari Ailus
2019-02-22 11:17           ` Jacopo Mondi
2019-02-22 11:29             ` Sakari Ailus
2019-02-22 13:37               ` Ian Arkver
2019-02-22 13:50               ` Geert Uytterhoeven
2018-11-01 23:31 ` [PATCH v2 17/30] v4l: subdev: compat: Implement handling for VIDIOC_SUBDEV_[GS]_ROUTING Niklas Söderlund
2019-01-08 10:04   ` Geert Uytterhoeven
2019-01-15 23:53   ` Laurent Pinchart
2019-01-22 15:57     ` Sakari Ailus
2019-02-18 11:21     ` Jacopo Mondi
2019-02-21 23:50       ` Sakari Ailus
2018-11-01 23:31 ` [PATCH v2 18/30] v4l: subdev: Take routing information into account in link validation Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 19/30] v4l: subdev: Improve link format validation debug messages Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 20/30] v4l: mc: Add an S_ROUTING helper function for power state changes Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 21/30] v4l: Add bus type to frame descriptors Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 22/30] v4l: Add CSI-2 bus configuration " Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 23/30] v4l: Add stream to frame descriptor Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 24/30] adv748x: csi2: add translation from pixelcode to CSI-2 datatype Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 25/30] adv748x: csi2: only allow formats on sink pads Niklas Söderlund
2019-02-21 14:18   ` Jacopo Mondi
2018-11-01 23:31 ` [PATCH v2 26/30] adv748x: csi2: describe the multiplexed stream Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 27/30] adv748x: csi2: add internal routing configuration Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 28/30] adv748x: afe: add routing support Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 29/30] rcar-csi2: use frame description information to configure CSI-2 bus Niklas Söderlund
2018-11-01 23:31 ` [PATCH v2 30/30] rcar-csi2: expose the subdevice internal routing Niklas Söderlund
2018-11-14 13:10   ` Nikita Yushchenko
2018-11-14 19:45     ` Niklas Söderlund
2018-12-03 22:16 ` [PATCH v2 00/30] v4l: add support for multiplexed streams Sakari Ailus
2018-12-05 22:09   ` Niklas Söderlund

Reply instructions:

You may reply publically 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=20190115235145.GF31088@pendragon.ideasonboard.com \
    --to=laurent.pinchart@ideasonboard.com \
    --cc=bparrot@ti.com \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-renesas-soc@vger.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-Renesas-SoC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-renesas-soc/0 linux-renesas-soc/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-renesas-soc linux-renesas-soc/ https://lore.kernel.org/linux-renesas-soc \
		linux-renesas-soc@vger.kernel.org linux-renesas-soc@archiver.kernel.org
	public-inbox-index linux-renesas-soc


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


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