[v8,44/55,media] uapi/media.h: Add MEDIA_IOC_G_TOPOLOGY ioctl

Commit Message

Mauro Carvalho Chehab Sept. 6, 2015, 12:03 p.m. UTC

Add a new ioctl that will report the entire topology on
one go.

Signed-off-by: Mauro Carvalho Chehab <[e-mail]>

Comments

Hans Verkuil Sept. 11, 2015, 1:58 p.m. UTC | #1

On 09/06/2015 02:03 PM, Mauro Carvalho Chehab wrote:
> Add a new ioctl that will report the entire topology on
> one go.
> 
> Signed-off-by: Mauro Carvalho Chehab <[e-mail]>

Acked-by: Hans Verkuil <[e-mail]>

I do have one suggestion: media_v2_interface, media_v2_pad and media_v2_link all
have a 'flags' field, but it is not clear from the code in the header which flag
#defines are used for which struct. Perhaps a few comments would help with that.

> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index a1bd7afba110..b17f6763aff4 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h

<snip>

> +
> +struct media_v2_pad {
> +	__u32 id;
> +	__u32 entity_id;
> +	__u32 flags;
> +	__u16 reserved[9];

Strange odd number here for no clear reason. Perhaps use 8 or 10 instead?

Anyway, you have my Ack.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Laurent Pinchart Dec. 6, 2015, 12:47 a.m. UTC | #2

Hi Mauro,

Thank you for the patch.

On Sunday 06 September 2015 09:03:04 Mauro Carvalho Chehab wrote:
> Add a new ioctl that will report the entire topology on
> one go.
> 
> Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> 
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 7320cdc45833..2d5ad40254b7 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -181,6 +181,8 @@ struct media_interface {
>   */
>  struct media_intf_devnode {
>  	struct media_interface		intf;
> +
> +	/* Should match the fields at media_v2_intf_devnode */
>  	u32				major;
>  	u32				minor;
>  };
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index a1bd7afba110..b17f6763aff4 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -206,6 +206,10 @@ struct media_pad_desc {
>  #define MEDIA_LNK_FL_IMMUTABLE		(1 << 1)
>  #define MEDIA_LNK_FL_DYNAMIC		(1 << 2)
> 
> +#define MEDIA_LNK_FL_LINK_TYPE		(0xf << 28)
> +#  define MEDIA_LNK_FL_DATA_LINK	(0 << 28)
> +#  define MEDIA_LNK_FL_INTERFACE_LINK	(1 << 28)
> +
>  struct media_link_desc {
>  	struct media_pad_desc source;
>  	struct media_pad_desc sink;
> @@ -249,11 +253,93 @@ struct media_links_enum {
>  #define MEDIA_INTF_T_ALSA_RAWMIDI       (MEDIA_INTF_T_ALSA_BASE + 4)
>  #define MEDIA_INTF_T_ALSA_HWDEP         (MEDIA_INTF_T_ALSA_BASE + 5)
> 
> -/* TBD: declare the structs needed for the new G_TOPOLOGY ioctl */
> +/*
> + * MC next gen API definitions
> + *
> + * NOTE: The declarations below are close to the MC RFC for the Media
> + *	 Controller, the next generation. Yet, there are a few adjustments
> + *	 to do, as we want to be able to have a functional API before
> + *	 the MC properties change. Those will be properly marked below.
> + *	 Please also notice that I removed "num_pads", "num_links",
> + *	 from the proposal, as a proper userspace application will likely
> + *	 use lists for pads/links, just as we intend to do in Kernelspace.
> + *	 The API definition should be freed from fields that are bound to
> + *	 some specific data structure.
> + *
> + * FIXME: Currently, I opted to name the new types as "media_v2", as this
> + *	  won't cause any conflict with the Kernelspace namespace, nor with
> + *	  the previous kAPI media_*_desc namespace. This can be changed
> + *	  later, before the adding this API upstream.

Yes, that's a good idea. Or at least we need to remove this comment if we 
decide to keep the v2 names :-)

> + */
> +
> +
> +struct media_v2_entity {
> +	__u32 id;
> +	char name[64];		/* FIXME: move to a property? (RFC says so) */

I agree with Sakari that we can keep the name here even if we also expose it 
as a property. However, there's one issue we need to address : we need to 
clearly define what the name field should contain and how it should be 
constructed, otherwise we'll end up with the exact same mess as today, and I 
don't want that. We can discuss it in this mail thread or as replies to a 
future documentation patch.

> +	__u16 reserved[14];

Sakari and Hans have already commented on using __u32 instead of __u16 for 
reserved fields, as well as on the number of reserved fields. I agree with 
them but have nothing to add.

> +};
> +
> +/* Should match the specific fields at media_intf_devnode */
> +struct media_v2_intf_devnode {
> +	__u32 major;
> +	__u32 minor;
> +};
> +
> +struct media_v2_interface {
> +	__u32 id;
> +	__u32 intf_type;
> +	__u32 flags;
> +	__u32 reserved[9];
> +
> +	union {
> +		struct media_v2_intf_devnode devnode;
> +		__u32 raw[16];
> +	};
> +};
> +
> +struct media_v2_pad {
> +	__u32 id;
> +	__u32 entity_id;
> +	__u32 flags;
> +	__u16 reserved[9];
> +};
> +
> +struct media_v2_link {
> +    __u32 id;
> +    __u32 source_id;
> +    __u32 sink_id;
> +    __u32 flags;
> +    __u32 reserved[5];
> +};
> +
> +struct media_v2_topology {
> +	__u32 topology_version;
> +
> +	__u32 num_entities;
> +	struct media_v2_entity *entities;

The kernel seems to be moving to using __u64 instead of pointers in userspace-
facing structures to avoid compat32 code.

> +
> +	__u32 num_interfaces;
> +	struct media_v2_interface *interfaces;
> +
> +	__u32 num_pads;
> +	struct media_v2_pad *pads;
> +
> +	__u32 num_links;
> +	struct media_v2_link *links;
> +
> +	struct {
> +		__u32 reserved_num;
> +		void *reserved_ptr;
> +	} reserved_types[16];
> +	__u32 reserved[8];

I'd just create __u32 reserved fields without any reserved_types, we can 
always use the reserved fields to add new types later.

> +};
> +
> +/* ioctls */
> 
>  #define MEDIA_IOC_DEVICE_INFO		_IOWR('|', 0x00, struct 
media_device_info)
>  #define MEDIA_IOC_ENUM_ENTITIES		_IOWR('|', 0x01, struct 
media_entity_desc)
> #define MEDIA_IOC_ENUM_LINKS		_IOWR('|', 0x02, struct media_links_enum)
> #define MEDIA_IOC_SETUP_LINK		_IOWR('|', 0x03, struct media_link_desc)
> +#define MEDIA_IOC_G_TOPOLOGY		_IOWR('|', 0x04, struct media_v2_topology)
> 
>  #endif /* __LINUX_MEDIA_H */

Mauro Carvalho Chehab Dec. 8, 2015, 7:23 p.m. UTC | #3

Em Sun, 06 Dec 2015 02:47:39 +0200
Laurent Pinchart <[e-mail]> escreveu:

> Hi Mauro,
> 
> Thank you for the patch.
> 
> On Sunday 06 September 2015 09:03:04 Mauro Carvalho Chehab wrote:
> > Add a new ioctl that will report the entire topology on
> > one go.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> > 
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index 7320cdc45833..2d5ad40254b7 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -181,6 +181,8 @@ struct media_interface {
> >   */
> >  struct media_intf_devnode {
> >  	struct media_interface		intf;
> > +
> > +	/* Should match the fields at media_v2_intf_devnode */
> >  	u32				major;
> >  	u32				minor;
> >  };
> > diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> > index a1bd7afba110..b17f6763aff4 100644
> > --- a/include/uapi/linux/media.h
> > +++ b/include/uapi/linux/media.h
> > @@ -206,6 +206,10 @@ struct media_pad_desc {
> >  #define MEDIA_LNK_FL_IMMUTABLE		(1 << 1)
> >  #define MEDIA_LNK_FL_DYNAMIC		(1 << 2)
> > 
> > +#define MEDIA_LNK_FL_LINK_TYPE		(0xf << 28)
> > +#  define MEDIA_LNK_FL_DATA_LINK	(0 << 28)
> > +#  define MEDIA_LNK_FL_INTERFACE_LINK	(1 << 28)
> > +
> >  struct media_link_desc {
> >  	struct media_pad_desc source;
> >  	struct media_pad_desc sink;
> > @@ -249,11 +253,93 @@ struct media_links_enum {
> >  #define MEDIA_INTF_T_ALSA_RAWMIDI       (MEDIA_INTF_T_ALSA_BASE + 4)
> >  #define MEDIA_INTF_T_ALSA_HWDEP         (MEDIA_INTF_T_ALSA_BASE + 5)
> > 
> > -/* TBD: declare the structs needed for the new G_TOPOLOGY ioctl */
> > +/*
> > + * MC next gen API definitions
> > + *
> > + * NOTE: The declarations below are close to the MC RFC for the Media
> > + *	 Controller, the next generation. Yet, there are a few adjustments
> > + *	 to do, as we want to be able to have a functional API before
> > + *	 the MC properties change. Those will be properly marked below.
> > + *	 Please also notice that I removed "num_pads", "num_links",
> > + *	 from the proposal, as a proper userspace application will likely
> > + *	 use lists for pads/links, just as we intend to do in Kernelspace.
> > + *	 The API definition should be freed from fields that are bound to
> > + *	 some specific data structure.
> > + *
> > + * FIXME: Currently, I opted to name the new types as "media_v2", as this
> > + *	  won't cause any conflict with the Kernelspace namespace, nor with
> > + *	  the previous kAPI media_*_desc namespace. This can be changed
> > + *	  later, before the adding this API upstream.
> 
> Yes, that's a good idea. Or at least we need to remove this comment if we 
> decide to keep the v2 names :-)

True ;)

> 
> > + */
> > +
> > +
> > +struct media_v2_entity {
> > +	__u32 id;
> > +	char name[64];		/* FIXME: move to a property? (RFC says so) */
> 
> I agree with Sakari that we can keep the name here even if we also expose it 
> as a property. However, there's one issue we need to address : we need to 
> clearly define what the name field should contain and how it should be 
> constructed, otherwise we'll end up with the exact same mess as today, and I 
> don't want that. We can discuss it in this mail thread or as replies to a 
> future documentation patch.


Well, let's discuss it then. What's your proposal?

I guess there are actually some different goals that we want to archive
with "name":

1) an ideally short name used when displaying an entity on some graph. 
It would be good if such "short name" would be unique, but it won't
hurt much if such name is not unique;

2) an unique ID identifier that will likely take the position of an
entity inside the machine bus, like PCI ID, USB ID, I2C bus + I2C addr...

I guess such UID would be better addressed via properties, as it
would likely be constructed in userspace via a different set of
properties, depending on the device and/or bus type.

> 
> > +	__u16 reserved[14];
> 
> Sakari and Hans have already commented on using __u32 instead of __u16 for 
> reserved fields, as well as on the number of reserved fields. I agree with 
> them but have nothing to add.

Ok. I'll change this on a later patch.

> 
> > +};
> > +
> > +/* Should match the specific fields at media_intf_devnode */
> > +struct media_v2_intf_devnode {
> > +	__u32 major;
> > +	__u32 minor;
> > +};
> > +
> > +struct media_v2_interface {
> > +	__u32 id;
> > +	__u32 intf_type;
> > +	__u32 flags;
> > +	__u32 reserved[9];
> > +
> > +	union {
> > +		struct media_v2_intf_devnode devnode;
> > +		__u32 raw[16];
> > +	};
> > +};
> > +
> > +struct media_v2_pad {
> > +	__u32 id;
> > +	__u32 entity_id;
> > +	__u32 flags;
> > +	__u16 reserved[9];
> > +};
> > +
> > +struct media_v2_link {
> > +    __u32 id;
> > +    __u32 source_id;
> > +    __u32 sink_id;
> > +    __u32 flags;
> > +    __u32 reserved[5];
> > +};
> > +
> > +struct media_v2_topology {
> > +	__u32 topology_version;
> > +
> > +	__u32 num_entities;
> > +	struct media_v2_entity *entities;
> 
> The kernel seems to be moving to using __u64 instead of pointers in userspace-
> facing structures to avoid compat32 code.

We had such discussion at the MC summit. I don't object to change to
__u64, but we need to reach a consensus ;)

> 
> > +
> > +	__u32 num_interfaces;
> > +	struct media_v2_interface *interfaces;
> > +
> > +	__u32 num_pads;
> > +	struct media_v2_pad *pads;
> > +
> > +	__u32 num_links;
> > +	struct media_v2_link *links;
> > +
> > +	struct {
> > +		__u32 reserved_num;
> > +		void *reserved_ptr;
> > +	} reserved_types[16];
> > +	__u32 reserved[8];
> 
> I'd just create __u32 reserved fields without any reserved_types, we can 
> always use the reserved fields to add new types later.

It used to be __u32... Hans requested to change it to an struct.

Btw, I also prefer to use __u32 here ;)

> 
> > +};
> > +
> > +/* ioctls */
> > 
> >  #define MEDIA_IOC_DEVICE_INFO		_IOWR('|', 0x00, struct 
> media_device_info)
> >  #define MEDIA_IOC_ENUM_ENTITIES		_IOWR('|', 0x01, struct 
> media_entity_desc)
> > #define MEDIA_IOC_ENUM_LINKS		_IOWR('|', 0x02, struct media_links_enum)
> > #define MEDIA_IOC_SETUP_LINK		_IOWR('|', 0x03, struct media_link_desc)
> > +#define MEDIA_IOC_G_TOPOLOGY		_IOWR('|', 0x04, struct media_v2_topology)
> > 
> >  #endif /* __LINUX_MEDIA_H */
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Arnd Bergmann Dec. 8, 2015, 7:48 p.m. UTC | #4

On Tuesday 08 December 2015 17:23:40 Mauro Carvalho Chehab wrote:
> > > +
> > > +struct media_v2_topology {
> > > +   __u32 topology_version;
> > > +
> > > +   __u32 num_entities;
> > > +   struct media_v2_entity *entities;
> > 
> > The kernel seems to be moving to using __u64 instead of pointers in userspace-
> > facing structures to avoid compat32 code.
> 
> We had such discussion at the MC summit. I don't object to change to
> __u64, but we need to reach a consensus 
> 

Just saw the email fly by. Using a __u64 to pass a pointer is generally
the preferred method for most subsystems these days.

However, that means you probably want to extract the pointer from
that using something like

static inline void __user *get_upointer(u64 arg)
{
	return (void __user *)(uintptr_t)arg;
}

This is the only way that I know that works on both 32-bit and 64-bit
architectures as well as the oddball s390 compat mode (which you don't
care about because there are no media devices on s390).

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 7320cdc45833..2d5ad40254b7 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -181,6 +181,8 @@  struct media_interface {
  */
 struct media_intf_devnode {
 	struct media_interface		intf;
+
+	/* Should match the fields at media_v2_intf_devnode */
 	u32				major;
 	u32				minor;
 };
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index a1bd7afba110..b17f6763aff4 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -206,6 +206,10 @@  struct media_pad_desc {
 #define MEDIA_LNK_FL_IMMUTABLE		(1 << 1)
 #define MEDIA_LNK_FL_DYNAMIC		(1 << 2)
 
+#define MEDIA_LNK_FL_LINK_TYPE		(0xf << 28)
+#  define MEDIA_LNK_FL_DATA_LINK	(0 << 28)
+#  define MEDIA_LNK_FL_INTERFACE_LINK	(1 << 28)
+
 struct media_link_desc {
 	struct media_pad_desc source;
 	struct media_pad_desc sink;
@@ -249,11 +253,93 @@  struct media_links_enum {
 #define MEDIA_INTF_T_ALSA_RAWMIDI       (MEDIA_INTF_T_ALSA_BASE + 4)
 #define MEDIA_INTF_T_ALSA_HWDEP         (MEDIA_INTF_T_ALSA_BASE + 5)
 
-/* TBD: declare the structs needed for the new G_TOPOLOGY ioctl */
+/*
+ * MC next gen API definitions
+ *
+ * NOTE: The declarations below are close to the MC RFC for the Media
+ *	 Controller, the next generation. Yet, there are a few adjustments
+ *	 to do, as we want to be able to have a functional API before
+ *	 the MC properties change. Those will be properly marked below.
+ *	 Please also notice that I removed "num_pads", "num_links",
+ *	 from the proposal, as a proper userspace application will likely
+ *	 use lists for pads/links, just as we intend to do in Kernelspace.
+ *	 The API definition should be freed from fields that are bound to
+ *	 some specific data structure.
+ *
+ * FIXME: Currently, I opted to name the new types as "media_v2", as this
+ *	  won't cause any conflict with the Kernelspace namespace, nor with
+ *	  the previous kAPI media_*_desc namespace. This can be changed
+ *	  later, before the adding this API upstream.
+ */
+
+
+struct media_v2_entity {
+	__u32 id;
+	char name[64];		/* FIXME: move to a property? (RFC says so) */
+	__u16 reserved[14];
+};
+
+/* Should match the specific fields at media_intf_devnode */
+struct media_v2_intf_devnode {
+	__u32 major;
+	__u32 minor;
+};
+
+struct media_v2_interface {
+	__u32 id;
+	__u32 intf_type;
+	__u32 flags;
+	__u32 reserved[9];
+
+	union {
+		struct media_v2_intf_devnode devnode;
+		__u32 raw[16];
+	};
+};
+
+struct media_v2_pad {
+	__u32 id;
+	__u32 entity_id;
+	__u32 flags;
+	__u16 reserved[9];
+};
+
+struct media_v2_link {
+    __u32 id;
+    __u32 source_id;
+    __u32 sink_id;
+    __u32 flags;
+    __u32 reserved[5];
+};
+
+struct media_v2_topology {
+	__u32 topology_version;
+
+	__u32 num_entities;
+	struct media_v2_entity *entities;
+
+	__u32 num_interfaces;
+	struct media_v2_interface *interfaces;
+
+	__u32 num_pads;
+	struct media_v2_pad *pads;
+
+	__u32 num_links;
+	struct media_v2_link *links;
+
+	struct {
+		__u32 reserved_num;
+		void *reserved_ptr;
+	} reserved_types[16];
+	__u32 reserved[8];
+};
+
+/* ioctls */
 
 #define MEDIA_IOC_DEVICE_INFO		_IOWR('|', 0x00, struct media_device_info)
 #define MEDIA_IOC_ENUM_ENTITIES		_IOWR('|', 0x01, struct media_entity_desc)
 #define MEDIA_IOC_ENUM_LINKS		_IOWR('|', 0x02, struct media_links_enum)
 #define MEDIA_IOC_SETUP_LINK		_IOWR('|', 0x03, struct media_link_desc)
+#define MEDIA_IOC_G_TOPOLOGY		_IOWR('|', 0x04, struct media_v2_topology)
 
 #endif /* __LINUX_MEDIA_H */