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

2015-12-08 Thread Mauro Carvalho Chehab 
Em Sun, 06 Dec 2015 02:47:39 +0200
Laurent Pinchart  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 
> > 
> > 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;
> > +

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

2015-12-08 Thread Arnd Bergmann 
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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-12-05 Thread Laurent Pinchart 
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 
> 
> 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

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

2015-09-11 Thread Hans Verkuil 
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 

Acked-by: Hans Verkuil 

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



> +
> +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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-09-06 Thread Mauro Carvalho Chehab 
Add a new ioctl that will report the entire topology on
one go.

Signed-off-by: Mauro Carvalho Chehab 

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 */
-- 
2.4.3


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

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

2015-08-31 Thread Hans Verkuil 
On 08/30/2015 05:06 AM, Mauro Carvalho Chehab wrote:
> Add a new ioctl that will report the entire topology on
> one go.
> 
> Signed-off-by: Mauro Carvalho Chehab 
> 
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 756e1960fd7f..358a0c6b1f86 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 4186891e5e81..fa0b68e670b0 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -251,11 +251,94 @@ 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 todo in Kernelspace.

s/todo/to do/

> + *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
> + * latter, before the adding this API upstream.

s/latter/later/ :-)

I think this comment belongs to the commit log and not in this header.

> + */
> +
> +
> +#define MEDIA_NEW_LNK_FL_ENABLED MEDIA_LNK_FL_ENABLED
> +#define MEDIA_NEW_LNK_FL_IMMUTABLE   MEDIA_LNK_FL_IMMUTABLE
> +#define MEDIA_NEW_LNK_FL_DYNAMIC MEDIA_NEW_FL_DYNAMIC
> +#define MEDIA_NEW_LNK_FL_INTERFACE_LINK  (1 << 3)

Shouldn't this be MEDIA_V2_ instead of MEDIA_NEW_?

Do we need the INTERFACE_LINK flag? You can deduce it by checking the
ID type.

I don't have a clear preference one way or another, just wondering about the
reason for adding it.

> +
> +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;

Like in media_link I would use a union here as well to be able to refer to
source/sink_id and entity/interface_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;
> +
> + __u32 reserved[64];

As mentioned before: use this instead to prevent horrible 32/64 bit arch
compat code:

struct {
__u32 reserved_num;
void *reserved_ptr;
} reserved_types[16];
__u32 reserved[8];

Sizes for these arrays are TBD.

> +};
> +
> +/* 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 */
> 

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at

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

2015-08-31 Thread Mauro Carvalho Chehab 
Em Mon, 31 Aug 2015 14:00:42 +0200
Hans Verkuil  escreveu:

> On 08/30/2015 05:06 AM, Mauro Carvalho Chehab wrote:
> > Add a new ioctl that will report the entire topology on
> > one go.
> > 
> > Signed-off-by: Mauro Carvalho Chehab 
> > 
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index 756e1960fd7f..358a0c6b1f86 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 4186891e5e81..fa0b68e670b0 100644
> > --- a/include/uapi/linux/media.h
> > +++ b/include/uapi/linux/media.h
> > @@ -251,11 +251,94 @@ 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 todo in Kernelspace.
> 
> s/todo/to do/
> 
> > + *  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
> > + *   latter, before the adding this API upstream.
> 
> s/latter/later/ :-)
> 
> I think this comment belongs to the commit log and not in this header.

True, but I opted to keep it here for now to produce some discussions ;)

I'm actually in doubt if we should rename the flags as proposed below,
and use the newer flags only at G_TOPOLOGY or if we should keep the same
namespace for them and accept newer flags with legacy ioctls.

> 
> > + */
> > +
> > +
> > +#define MEDIA_NEW_LNK_FL_ENABLED   MEDIA_LNK_FL_ENABLED
> > +#define MEDIA_NEW_LNK_FL_IMMUTABLE MEDIA_LNK_FL_IMMUTABLE
> > +#define MEDIA_NEW_LNK_FL_DYNAMIC   MEDIA_NEW_FL_DYNAMIC
> > +#define MEDIA_NEW_LNK_FL_INTERFACE_LINK(1 << 3)
> 
> Shouldn't this be MEDIA_V2_ instead of MEDIA_NEW_?
> 
> Do we need the INTERFACE_LINK flag? You can deduce it by checking the
> ID type.

Yes, this can be deduced from the type of the objects inside the link.

I guess I added it because of some comment from your media.h RFC
proposal.

Right now, I'm using it at the application to better represent the graph
elements:


http://git.linuxtv.org/cgit.cgi/mchehab/experimental-v4l-utils.git/tree/contrib/test/mc_nextgen_test.c?h=mc-next-gen=fdc16ece9732c94cfa76eee86978158c5976c00a#n438
 

http://git.linuxtv.org/cgit.cgi/mchehab/experimental-v4l-utils.git/tree/contrib/test/mc_nextgen_test.c?h=mc-next-gen=fdc16ece9732c94cfa76eee86978158c5976c00a#n444

But it could, instead, be doing something like:

if media_type(link->gobj1.id == MEDIA_GRAPH_PAD)
link_is_pad = true;
else
link_is_pad = false;


Btw, I'm using the same type for both data and interface links, as
I don't see any reason why to differentiate internally: they all share
the same linked list at mdev and the same object ID range.

> 
> I don't have a clear preference one way or another, just wondering about the
> reason for adding it.
> 
> > +
> > +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;
> 
> Like in media_link I would

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

2015-08-29 Thread Mauro Carvalho Chehab 
Add a new ioctl that will report the entire topology on
one go.

Signed-off-by: Mauro Carvalho Chehab mche...@osg.samsung.com

diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 756e1960fd7f..358a0c6b1f86 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 4186891e5e81..fa0b68e670b0 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -251,11 +251,94 @@ 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 todo 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
+ *   latter, before the adding this API upstream.
+ */
+
+
+#define MEDIA_NEW_LNK_FL_ENABLED   MEDIA_LNK_FL_ENABLED
+#define MEDIA_NEW_LNK_FL_IMMUTABLE MEDIA_LNK_FL_IMMUTABLE
+#define MEDIA_NEW_LNK_FL_DYNAMIC   MEDIA_NEW_FL_DYNAMIC
+#define MEDIA_NEW_LNK_FL_INTERFACE_LINK(1  3)
+
+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;
+
+   __u32 reserved[64];
+};
+
+/* 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 */
-- 
2.4.3

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html