* [RFCv4 PATCH 0/3] This RFC patch series implements properties for the media controller.
@ 2018-11-21 15:40 Hans Verkuil
2018-11-21 15:40 ` [RFCv4 PATCH 1/3] uapi/linux/media.h: add property support Hans Verkuil
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Hans Verkuil @ 2018-11-21 15:40 UTC (permalink / raw)
To: linux-media
The main changes since RFCv3 are:
- Add entity index to media_v2_pad
- Add source/sink pad index to media_v2_link
- Add owner_idx and owner type flags to media_v2_prop
An updated v4l2-ctl and v4l2-compliance that can report properties
is available here:
https://git.linuxtv.org/hverkuil/v4l-utils.git/log/?h=props
Currently I support u64, s64 and const char * property types. And also
a 'group' type that groups sub-properties. But it can be extended to any
type including binary data if needed. No array support (as we have for
controls), but there are enough reserved fields in media_v2_prop
to add this if needed.
I added properties for entities and pads to vimc, so I could test this.
Note that the changes to media_device_get_topology() are hard to read
from the patch. It is easier to just look at the source code:
https://git.linuxtv.org/hverkuil/media_tree.git/tree/drivers/media/media-device.c?h=mc-props
I have some ideas to improve this some more:
1) Add the properties directly to media_gobj. This would simplify some
of the code, but it would require a media_gobj_init function to
initialize the property list. In general I am a bit unhappy about
media_gobj_create: it doesn't really create the graph object, instead
it just adds it to the media_device. It's confusing and it is something
I would like to change.
2) The links between pads are stored in media_entity instead of in media_pad.
This is a bit unexpected and makes it harder to traverse the data
structures since to find the links for a pad you need to walk the entity
links and find the links for that pad. Putting all links in the entity
also mixes up pad and interface links, and it would be much cleaner if
those are separated.
3) I still think adding support for backlinks to G_TOPOLOGY is a good idea.
Since the current data structure represents a flattened tree that is easy
to navigate the only thing missing for userspace is backlink support.
This is still something that userspace needs to figure out when the kernel
has this readily available. I think that with this in place applications
can just do all the lookups directly on the topology data structure.
1+2 are internal cleanups that can be done later.
3 is a low-priority future enhancement. This might become easier to implement
once 1+2 are done.
This is pretty much the last RFC. If everyone agree with this approach, then
I can make a final patch series, adding documentation etc.
Regards,
Hans
Hans Verkuil (3):
uapi/linux/media.h: add property support
media controller: add properties support
vimc: add property test code
drivers/media/media-device.c | 335 +++++++++++++++++-----
drivers/media/media-entity.c | 107 ++++++-
drivers/media/platform/vimc/vimc-common.c | 50 ++++
include/media/media-device.h | 6 +
include/media/media-entity.h | 318 ++++++++++++++++++++
include/uapi/linux/media.h | 88 +++++-
6 files changed, 819 insertions(+), 85 deletions(-)
--
2.19.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFCv4 PATCH 1/3] uapi/linux/media.h: add property support
2018-11-21 15:40 [RFCv4 PATCH 0/3] This RFC patch series implements properties for the media controller Hans Verkuil
@ 2018-11-21 15:40 ` Hans Verkuil
2018-12-12 8:18 ` Mauro Carvalho Chehab
2018-11-21 15:40 ` [RFCv4 PATCH 2/3] media controller: add properties support Hans Verkuil
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Hans Verkuil @ 2018-11-21 15:40 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
Add a new topology struct that includes properties and adds
index fields to quickly find references from one object to
another in the topology arrays.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
include/uapi/linux/media.h | 88 ++++++++++++++++++++++++++++++++++++--
1 file changed, 84 insertions(+), 4 deletions(-)
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index e5d0c5c611b5..a81e9723204c 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -144,6 +144,8 @@ struct media_device_info {
/* Entity flags */
#define MEDIA_ENT_FL_DEFAULT (1 << 0)
#define MEDIA_ENT_FL_CONNECTOR (1 << 1)
+#define MEDIA_ENT_FL_PAD_IDX (1 << 2)
+#define MEDIA_ENT_FL_PROP_IDX (1 << 3)
/* OR with the entity id value to find the next entity */
#define MEDIA_ENT_ID_FLAG_NEXT (1 << 31)
@@ -210,6 +212,9 @@ struct media_entity_desc {
#define MEDIA_PAD_FL_SINK (1 << 0)
#define MEDIA_PAD_FL_SOURCE (1 << 1)
#define MEDIA_PAD_FL_MUST_CONNECT (1 << 2)
+#define MEDIA_PAD_FL_LINK_IDX (1 << 3)
+#define MEDIA_PAD_FL_PROP_IDX (1 << 4)
+#define MEDIA_PAD_FL_ENTITY_IDX (1 << 5)
struct media_pad_desc {
__u32 entity; /* entity ID */
@@ -221,6 +226,8 @@ struct media_pad_desc {
#define MEDIA_LNK_FL_ENABLED (1 << 0)
#define MEDIA_LNK_FL_IMMUTABLE (1 << 1)
#define MEDIA_LNK_FL_DYNAMIC (1 << 2)
+#define MEDIA_LNK_FL_SOURCE_IDX (1 << 3)
+#define MEDIA_LNK_FL_SINK_IDX (1 << 4)
#define MEDIA_LNK_FL_LINK_TYPE (0xf << 28)
# define MEDIA_LNK_FL_DATA_LINK (0 << 28)
@@ -296,7 +303,9 @@ struct media_v2_entity {
char name[64];
__u32 function; /* Main function of the entity */
__u32 flags;
- __u32 reserved[5];
+ __u16 pad_idx;
+ __u16 prop_idx;
+ __u32 reserved[4];
} __attribute__ ((packed));
/* Should match the specific fields at media_intf_devnode */
@@ -305,11 +314,14 @@ struct media_v2_intf_devnode {
__u32 minor;
} __attribute__ ((packed));
+#define MEDIA_INTF_FL_LINK_IDX (1 << 0)
+
struct media_v2_interface {
__u32 id;
__u32 intf_type;
__u32 flags;
- __u32 reserved[9];
+ __u16 link_idx;
+ __u16 reserved[17];
union {
struct media_v2_intf_devnode devnode;
@@ -331,7 +343,10 @@ struct media_v2_pad {
__u32 entity_id;
__u32 flags;
__u32 index;
- __u32 reserved[4];
+ __u16 link_idx;
+ __u16 prop_idx;
+ __u16 entity_idx;
+ __u16 reserved[5];
} __attribute__ ((packed));
struct media_v2_link {
@@ -339,9 +354,68 @@ struct media_v2_link {
__u32 source_id;
__u32 sink_id;
__u32 flags;
- __u32 reserved[6];
+ __u16 source_idx;
+ __u16 sink_idx;
+ __u32 reserved[5];
} __attribute__ ((packed));
+#define MEDIA_PROP_TYPE_GROUP 1
+#define MEDIA_PROP_TYPE_U64 2
+#define MEDIA_PROP_TYPE_S64 3
+#define MEDIA_PROP_TYPE_STRING 4
+
+#define MEDIA_PROP_FL_OWNER 0xf
+# define MEDIA_PROP_FL_ENTITY 0
+# define MEDIA_PROP_FL_PAD 1
+# define MEDIA_PROP_FL_LINK 2
+# define MEDIA_PROP_FL_INTF 3
+# define MEDIA_PROP_FL_PROP 4
+#define MEDIA_PROP_FL_PROP_IDX (1 << 4)
+
+/**
+ * struct media_v2_prop - A media property
+ *
+ * @id: The unique non-zero ID of this property
+ * @owner_id: The ID of the object this property belongs to
+ * @type: Property type
+ * @flags: Property flags
+ * @name: Property name
+ * @payload_size: Property payload size, 0 for U64/S64
+ * @payload_offset: Property payload starts at this offset from &prop.id.
+ * This is 0 for U64/S64.
+ * @prop_idx: Index to sub-properties, 0 means there are no sub-properties.
+ * @owner_idx: Index to entities/pads/properties, depending on the owner ID
+ * type.
+ * @reserved: Property reserved field, will be zeroed.
+ */
+struct media_v2_prop {
+ __u32 id;
+ __u32 owner_id;
+ __u32 type;
+ __u32 flags;
+ char name[32];
+ __u32 payload_size;
+ __u32 payload_offset;
+ __u16 prop_idx;
+ __u16 owner_idx;
+ __u32 reserved[17];
+} __attribute__ ((packed));
+
+static inline const char *media_prop2s(const struct media_v2_prop *prop)
+{
+ return (const char *)prop + prop->payload_offset;
+}
+
+static inline __u64 media_prop2u64(const struct media_v2_prop *prop)
+{
+ return *(const __u64 *)((const char *)prop + prop->payload_offset);
+}
+
+static inline __s64 media_prop2s64(const struct media_v2_prop *prop)
+{
+ return *(const __s64 *)((const char *)prop + prop->payload_offset);
+}
+
struct media_v2_topology {
__u64 topology_version;
@@ -360,6 +434,10 @@ struct media_v2_topology {
__u32 num_links;
__u32 reserved4;
__u64 ptr_links;
+
+ __u32 num_props;
+ __u32 props_payload_size;
+ __u64 ptr_props;
} __attribute__ ((packed));
/* ioctls */
@@ -368,6 +446,8 @@ struct media_v2_topology {
#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)
+/* Old MEDIA_IOC_G_TOPOLOGY ioctl without props support */
+#define MEDIA_IOC_G_TOPOLOGY_OLD 0xc0487c04
#define MEDIA_IOC_G_TOPOLOGY _IOWR('|', 0x04, struct media_v2_topology)
#define MEDIA_IOC_REQUEST_ALLOC _IOR ('|', 0x05, int)
--
2.19.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [RFCv4 PATCH 2/3] media controller: add properties support
2018-11-21 15:40 [RFCv4 PATCH 0/3] This RFC patch series implements properties for the media controller Hans Verkuil
2018-11-21 15:40 ` [RFCv4 PATCH 1/3] uapi/linux/media.h: add property support Hans Verkuil
@ 2018-11-21 15:40 ` Hans Verkuil
2018-12-12 9:02 ` Mauro Carvalho Chehab
2018-11-21 15:40 ` [RFCv4 PATCH 3/3] vimc: add property test code Hans Verkuil
2018-12-12 7:58 ` [RFCv4 PATCH 0/3] This RFC patch series implements properties for the media controller Mauro Carvalho Chehab
3 siblings, 1 reply; 13+ messages in thread
From: Hans Verkuil @ 2018-11-21 15:40 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
Add support for properties. In this initial implementation properties
can be added to entities and pads. In addition, properties can be
nested.
Since this patch adds the topology_idx to the graph objects it is now
easy to fill in the index fields in the topology to allow userspace
to quickly look up a reference from one object to another.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/media-device.c | 335 +++++++++++++++++++++++++++--------
drivers/media/media-entity.c | 107 ++++++++++-
include/media/media-device.h | 6 +
include/media/media-entity.h | 318 +++++++++++++++++++++++++++++++++
4 files changed, 685 insertions(+), 81 deletions(-)
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index bed24372e61f..099ab3532475 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -235,6 +235,21 @@ static long media_device_setup_link(struct media_device *mdev, void *arg)
return __media_entity_setup_link(link, linkd->flags);
}
+static void walk_props(struct list_head *head, u32 *prop_idx, u32 *payload_size)
+{
+ struct media_prop *prop;
+
+ if (list_empty(head))
+ return;
+
+ list_for_each_entry(prop, head, list) {
+ prop->graph_obj.topology_idx = (*prop_idx)++;
+ *payload_size += prop->payload_size;
+ }
+ list_for_each_entry(prop, head, list)
+ walk_props(&prop->props, prop_idx, payload_size);
+}
+
static long media_device_get_topology(struct media_device *mdev, void *arg)
{
struct media_v2_topology *topo = arg;
@@ -242,27 +257,96 @@ static long media_device_get_topology(struct media_device *mdev, void *arg)
struct media_interface *intf;
struct media_pad *pad;
struct media_link *link;
+ struct media_prop *prop, *subprop;
struct media_v2_entity kentity, __user *uentity;
struct media_v2_interface kintf, __user *uintf;
struct media_v2_pad kpad, __user *upad;
struct media_v2_link klink, __user *ulink;
+ struct media_v2_prop kprop, __user *uprop;
+ u32 payload_size = 0;
+ u32 payload_offset = 0;
+ u32 entity_idx = 0;
+ u32 interface_idx = 0;
+ u32 pad_idx = 0;
+ u32 link_idx = 0;
+ u32 prop_idx = 0;
unsigned int i;
int ret = 0;
topo->topology_version = mdev->topology_version;
+ /* Set entity/pad/link indices and number of entities */
+ media_device_for_each_entity(entity, mdev) {
+ entity->graph_obj.topology_idx = entity_idx++;
+ walk_props(&entity->props, &prop_idx, &payload_size);
+ for (i = 0; i < entity->num_pads; i++) {
+ pad = entity->pads + i;
+ pad->graph_obj.topology_idx = pad_idx++;
+ walk_props(&pad->props, &prop_idx, &payload_size);
+ }
+ /* Note: links are ordered by source pad index */
+ list_for_each_entry(link, &entity->links, list)
+ if (!link->is_backlink)
+ link->graph_obj.topology_idx = link_idx++;
+ }
+ if (topo->ptr_entities && entity_idx > topo->num_entities)
+ ret = -ENOSPC;
+ topo->num_entities = entity_idx;
+ topo->reserved1 = 0;
+
+ /* Set interface/link indices and number of interfaces */
+ media_device_for_each_intf(intf, mdev) {
+ intf->graph_obj.topology_idx = interface_idx++;
+ list_for_each_entry(link, &intf->links, list)
+ link->graph_obj.topology_idx = link_idx++;
+ }
+
+ if (topo->ptr_interfaces && interface_idx > topo->num_interfaces)
+ ret = -ENOSPC;
+ topo->num_interfaces = interface_idx;
+ topo->reserved2 = 0;
+
+ /* Set number of pads */
+ if (topo->ptr_pads && pad_idx > topo->num_pads)
+ ret = -ENOSPC;
+ topo->num_pads = pad_idx;
+ topo->reserved3 = 0;
+
+ /* Set number of links */
+ if (topo->ptr_links && link_idx > topo->num_links)
+ ret = -ENOSPC;
+ topo->num_links = link_idx;
+ topo->reserved4 = 0;
+
+ /* Set number of properties */
+ if (topo->ptr_props &&
+ (prop_idx > topo->num_props ||
+ payload_size > topo->props_payload_size))
+ ret = -ENOSPC;
+ topo->num_props = prop_idx;
+ topo->props_payload_size = payload_size;
+
+ if (ret)
+ return ret;
+
+ /*
+ * We use u16 for the graph object indices,
+ * so check that it will fit in 16 bits.
+ */
+ if (WARN_ON(entity_idx >= 0x10000 ||
+ interface_idx >= 0x10000 ||
+ pad_idx >= 0x10000 ||
+ link_idx >= 0x10000 ||
+ prop_idx >= 0x10000))
+ return -EINVAL;
+
/* Get entities and number of entities */
- i = 0;
uentity = media_get_uptr(topo->ptr_entities);
- media_device_for_each_entity(entity, mdev) {
- i++;
- if (ret || !uentity)
- continue;
+ if (!uentity)
+ goto skip_entities;
- if (i > topo->num_entities) {
- ret = -ENOSPC;
- continue;
- }
+ media_device_for_each_entity(entity, mdev) {
+ u16 idx = entity->graph_obj.topology_idx;
/* Copy fields to userspace struct if not error */
memset(&kentity, 0, sizeof(kentity));
@@ -270,27 +354,30 @@ static long media_device_get_topology(struct media_device *mdev, void *arg)
kentity.function = entity->function;
kentity.flags = entity->flags;
strscpy(kentity.name, entity->name,
- sizeof(kentity.name));
+ sizeof(kentity.name));
+ if (entity->num_pads) {
+ kentity.pad_idx = entity->pads[0].graph_obj.topology_idx;
+ kentity.flags |= MEDIA_ENT_FL_PAD_IDX;
+ }
+ if (!list_empty(&entity->props)) {
+ prop = list_first_entry(&entity->props,
+ struct media_prop, list);
+ kentity.prop_idx = prop->graph_obj.topology_idx;
+ kentity.flags |= MEDIA_ENT_FL_PROP_IDX;
+ }
- if (copy_to_user(uentity, &kentity, sizeof(kentity)))
- ret = -EFAULT;
- uentity++;
+ if (copy_to_user(uentity + idx, &kentity, sizeof(kentity)))
+ return -EFAULT;
}
- topo->num_entities = i;
- topo->reserved1 = 0;
+skip_entities:
/* Get interfaces and number of interfaces */
- i = 0;
uintf = media_get_uptr(topo->ptr_interfaces);
- media_device_for_each_intf(intf, mdev) {
- i++;
- if (ret || !uintf)
- continue;
+ if (!uintf)
+ goto skip_interfaces;
- if (i > topo->num_interfaces) {
- ret = -ENOSPC;
- continue;
- }
+ media_device_for_each_intf(intf, mdev) {
+ u16 idx = intf->graph_obj.topology_idx;
memset(&kintf, 0, sizeof(kintf));
@@ -298,6 +385,12 @@ static long media_device_get_topology(struct media_device *mdev, void *arg)
kintf.id = intf->graph_obj.id;
kintf.intf_type = intf->type;
kintf.flags = intf->flags;
+ if (!list_empty(&intf->links)) {
+ link = list_first_entry(&intf->links,
+ struct media_link, list);
+ kintf.link_idx = link->graph_obj.topology_idx;
+ kintf.flags |= MEDIA_INTF_FL_LINK_IDX;
+ }
if (media_type(&intf->graph_obj) == MEDIA_GRAPH_INTF_DEVNODE) {
struct media_intf_devnode *devnode;
@@ -308,74 +401,115 @@ static long media_device_get_topology(struct media_device *mdev, void *arg)
kintf.devnode.minor = devnode->minor;
}
- if (copy_to_user(uintf, &kintf, sizeof(kintf)))
- ret = -EFAULT;
- uintf++;
+ if (copy_to_user(uintf + idx, &kintf, sizeof(kintf)))
+ return -EFAULT;
}
- topo->num_interfaces = i;
- topo->reserved2 = 0;
+skip_interfaces:
/* Get pads and number of pads */
- i = 0;
upad = media_get_uptr(topo->ptr_pads);
- media_device_for_each_pad(pad, mdev) {
- i++;
- if (ret || !upad)
- continue;
+ if (!upad)
+ goto skip_pads;
- if (i > topo->num_pads) {
- ret = -ENOSPC;
- continue;
- }
+ media_device_for_each_pad(pad, mdev) {
+ u16 idx = pad->graph_obj.topology_idx;
memset(&kpad, 0, sizeof(kpad));
/* Copy pad fields to userspace struct */
kpad.id = pad->graph_obj.id;
kpad.entity_id = pad->entity->graph_obj.id;
- kpad.flags = pad->flags;
+ kpad.entity_idx = pad->entity->graph_obj.topology_idx;
+ kpad.flags = pad->flags | MEDIA_PAD_FL_ENTITY_IDX;
kpad.index = pad->index;
- if (copy_to_user(upad, &kpad, sizeof(kpad)))
- ret = -EFAULT;
- upad++;
+ entity = pad->entity;
+ list_for_each_entry(link, &entity->links, list) {
+ if (link->source == pad) {
+ kpad.link_idx = link->graph_obj.topology_idx;
+ kpad.flags |= MEDIA_PAD_FL_LINK_IDX;
+ break;
+ }
+ }
+ if (!list_empty(&pad->props)) {
+ prop = list_first_entry(&pad->props,
+ struct media_prop, list);
+ kpad.prop_idx = prop->graph_obj.topology_idx;
+ kpad.flags |= MEDIA_PAD_FL_PROP_IDX;
+ }
+
+ if (copy_to_user(upad + idx, &kpad, sizeof(kpad)))
+ return -EFAULT;
}
- topo->num_pads = i;
- topo->reserved3 = 0;
+skip_pads:
/* Get links and number of links */
- i = 0;
ulink = media_get_uptr(topo->ptr_links);
- media_device_for_each_link(link, mdev) {
- if (link->is_backlink)
- continue;
-
- i++;
+ if (!ulink)
+ goto skip_links;
- if (ret || !ulink)
- continue;
+ media_device_for_each_link(link, mdev) {
+ u16 idx;
- if (i > topo->num_links) {
- ret = -ENOSPC;
+ if (link->is_backlink)
continue;
- }
+ idx = link->graph_obj.topology_idx;
memset(&klink, 0, sizeof(klink));
/* Copy link fields to userspace struct */
klink.id = link->graph_obj.id;
klink.source_id = link->gobj0->id;
+ klink.source_idx = link->gobj0->topology_idx;
klink.sink_id = link->gobj1->id;
- klink.flags = link->flags;
+ klink.sink_idx = link->gobj1->topology_idx;
+ klink.flags = link->flags | MEDIA_LNK_FL_SOURCE_IDX |
+ MEDIA_LNK_FL_SINK_IDX;
- if (copy_to_user(ulink, &klink, sizeof(klink)))
- ret = -EFAULT;
- ulink++;
+ if (copy_to_user(ulink + idx, &klink, sizeof(klink)))
+ return -EFAULT;
}
- topo->num_links = i;
- topo->reserved4 = 0;
+skip_links:
+
+ /* Get properties and number of properties */
+ uprop = media_get_uptr(topo->ptr_props);
+ if (!uprop)
+ goto skip_props;
+
+ payload_offset = topo->num_props * sizeof(*uprop);
+ media_device_for_each_prop(prop, mdev) {
+ u16 idx = prop->graph_obj.topology_idx;
+
+ memset(&kprop, 0, sizeof(kprop));
+
+ /* Copy prop fields to userspace struct */
+ kprop.id = prop->graph_obj.id;
+ kprop.owner_id = prop->owner->id;
+ kprop.owner_idx = prop->owner->topology_idx;
+ kprop.type = prop->type;
+ kprop.payload_size = prop->payload_size;
+ if (prop->payload_size)
+ kprop.payload_offset =
+ payload_offset - idx * sizeof(*uprop);
+ memcpy(kprop.name, prop->name, sizeof(kprop.name));
+ kprop.flags = media_type(prop->owner);
+ if (!list_empty(&prop->props)) {
+ subprop = list_first_entry(&prop->props,
+ struct media_prop, list);
+ kprop.prop_idx = subprop->graph_obj.topology_idx;
+ kprop.flags |= MEDIA_PROP_FL_PROP_IDX;
+ }
- return ret;
+ if (copy_to_user(uprop + idx, &kprop, sizeof(kprop)))
+ return -EFAULT;
+ if (copy_to_user((u8 __user *)uprop + payload_offset,
+ prop->payload, prop->payload_size))
+ return -EFAULT;
+ payload_offset += prop->payload_size;
+ }
+skip_props:
+
+ return 0;
}
static long media_device_request_alloc(struct media_device *mdev,
@@ -408,9 +542,10 @@ static long copy_arg_to_user(void __user *uarg, void *karg, unsigned int cmd)
/* Do acquire the graph mutex */
#define MEDIA_IOC_FL_GRAPH_MUTEX BIT(0)
-#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user) \
+#define MEDIA_IOC_ARG(__cmd, alts, func, fl, from_user, to_user) \
[_IOC_NR(MEDIA_IOC_##__cmd)] = { \
.cmd = MEDIA_IOC_##__cmd, \
+ .alternatives = (alts), \
.fn = (long (*)(struct media_device *, void *))func, \
.flags = fl, \
.arg_from_user = from_user, \
@@ -418,23 +553,33 @@ static long copy_arg_to_user(void __user *uarg, void *karg, unsigned int cmd)
}
#define MEDIA_IOC(__cmd, func, fl) \
- MEDIA_IOC_ARG(__cmd, func, fl, copy_arg_from_user, copy_arg_to_user)
+ MEDIA_IOC_ARG(__cmd, NULL, func, fl, copy_arg_from_user, copy_arg_to_user)
+#define MEDIA_IOC_ALTS(__cmd, alts, func, fl) \
+ MEDIA_IOC_ARG(__cmd, alts, func, fl, copy_arg_from_user, copy_arg_to_user)
/* the table is indexed by _IOC_NR(cmd) */
struct media_ioctl_info {
unsigned int cmd;
+ const unsigned int *alternatives;
unsigned short flags;
long (*fn)(struct media_device *dev, void *arg);
long (*arg_from_user)(void *karg, void __user *uarg, unsigned int cmd);
long (*arg_to_user)(void __user *uarg, void *karg, unsigned int cmd);
};
+static const unsigned int topo_alts[] = {
+ /* Old MEDIA_IOC_G_TOPOLOGY without props support */
+ MEDIA_IOC_G_TOPOLOGY_OLD,
+ 0
+};
+
static const struct media_ioctl_info ioctl_info[] = {
MEDIA_IOC(DEVICE_INFO, media_device_get_info, MEDIA_IOC_FL_GRAPH_MUTEX),
MEDIA_IOC(ENUM_ENTITIES, media_device_enum_entities, MEDIA_IOC_FL_GRAPH_MUTEX),
MEDIA_IOC(ENUM_LINKS, media_device_enum_links, MEDIA_IOC_FL_GRAPH_MUTEX),
MEDIA_IOC(SETUP_LINK, media_device_setup_link, MEDIA_IOC_FL_GRAPH_MUTEX),
MEDIA_IOC(G_TOPOLOGY, media_device_get_topology, MEDIA_IOC_FL_GRAPH_MUTEX),
+ MEDIA_IOC_ALTS(G_TOPOLOGY, topo_alts, media_device_get_topology, MEDIA_IOC_FL_GRAPH_MUTEX),
MEDIA_IOC(REQUEST_ALLOC, media_device_request_alloc, 0),
};
@@ -448,17 +593,29 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
char __karg[256], *karg = __karg;
long ret;
- if (_IOC_NR(cmd) >= ARRAY_SIZE(ioctl_info)
- || ioctl_info[_IOC_NR(cmd)].cmd != cmd)
+ if (_IOC_NR(cmd) >= ARRAY_SIZE(ioctl_info))
return -ENOIOCTLCMD;
info = &ioctl_info[_IOC_NR(cmd)];
+ if (info->cmd != cmd) {
+ const unsigned int *p;
+
+ for (p = info->alternatives; p && *p; p++)
+ if (*p == cmd)
+ break;
+ if (!p || !*p)
+ return -ENOIOCTLCMD;
+ }
+
if (_IOC_SIZE(info->cmd) > sizeof(__karg)) {
karg = kmalloc(_IOC_SIZE(info->cmd), GFP_KERNEL);
if (!karg)
return -ENOMEM;
}
+ if (_IOC_SIZE(info->cmd) > _IOC_SIZE(cmd))
+ memset(karg + _IOC_SIZE(cmd), 0,
+ _IOC_SIZE(info->cmd) - _IOC_SIZE(cmd));
if (info->arg_from_user) {
ret = info->arg_from_user(karg, arg, cmd);
@@ -571,6 +728,16 @@ static void media_device_release(struct media_devnode *devnode)
dev_dbg(devnode->parent, "Media device released\n");
}
+static void init_prop_list(struct media_device *mdev, struct list_head *list)
+{
+ struct media_prop *prop;
+
+ list_for_each_entry(prop, list, list) {
+ media_gobj_create(mdev, MEDIA_GRAPH_PROP, &prop->graph_obj);
+ init_prop_list(mdev, &prop->props);
+ }
+}
+
/**
* media_device_register_entity - Register an entity with a media device
* @mdev: The media device
@@ -592,9 +759,8 @@ int __must_check media_device_register_entity(struct media_device *mdev,
/* Warn if we apparently re-register an entity */
WARN_ON(entity->graph_obj.mdev != NULL);
entity->graph_obj.mdev = mdev;
- INIT_LIST_HEAD(&entity->links);
- entity->num_links = 0;
- entity->num_backlinks = 0;
+ if (!entity->inited)
+ media_entity_init(entity);
ret = ida_alloc_min(&mdev->entity_internal_idx, 1, GFP_KERNEL);
if (ret < 0)
@@ -608,10 +774,17 @@ int __must_check media_device_register_entity(struct media_device *mdev,
/* Initialize media_gobj embedded at the entity */
media_gobj_create(mdev, MEDIA_GRAPH_ENTITY, &entity->graph_obj);
+ /* Initialize objects at the props */
+ init_prop_list(mdev, &entity->props);
+
/* Initialize objects at the pads */
- for (i = 0; i < entity->num_pads; i++)
+ for (i = 0; i < entity->num_pads; i++) {
media_gobj_create(mdev, MEDIA_GRAPH_PAD,
- &entity->pads[i].graph_obj);
+ &entity->pads[i].graph_obj);
+
+ /* Initialize objects at the pad props */
+ init_prop_list(mdev, &entity->pads[i].props);
+ }
/* invoke entity_notify callbacks */
list_for_each_entry_safe(notify, next, &mdev->entity_notify, list)
@@ -640,6 +813,18 @@ int __must_check media_device_register_entity(struct media_device *mdev,
}
EXPORT_SYMBOL_GPL(media_device_register_entity);
+static void media_device_free_props(struct list_head *list)
+{
+ while (!list_empty(list)) {
+ struct media_prop *prop;
+
+ prop = list_first_entry(list, struct media_prop, list);
+ list_del(&prop->list);
+ media_gobj_destroy(&prop->graph_obj);
+ kfree(prop);
+ }
+}
+
static void __media_device_unregister_entity(struct media_entity *entity)
{
struct media_device *mdev = entity->graph_obj.mdev;
@@ -661,8 +846,13 @@ static void __media_device_unregister_entity(struct media_entity *entity)
__media_entity_remove_links(entity);
/* Remove all pads that belong to this entity */
- for (i = 0; i < entity->num_pads; i++)
+ for (i = 0; i < entity->num_pads; i++) {
+ media_device_free_props(&entity->pads[i].props);
media_gobj_destroy(&entity->pads[i].graph_obj);
+ }
+
+ /* Remove all props that belong to this entity */
+ media_device_free_props(&entity->props);
/* Remove the entity */
media_gobj_destroy(&entity->graph_obj);
@@ -701,6 +891,7 @@ void media_device_init(struct media_device *mdev)
INIT_LIST_HEAD(&mdev->interfaces);
INIT_LIST_HEAD(&mdev->pads);
INIT_LIST_HEAD(&mdev->links);
+ INIT_LIST_HEAD(&mdev->props);
INIT_LIST_HEAD(&mdev->entity_notify);
mutex_init(&mdev->req_queue_mutex);
diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index 0b1cb3559140..dfd0073eae31 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -34,6 +34,8 @@ static inline const char *gobj_type(enum media_gobj_type type)
return "link";
case MEDIA_GRAPH_INTF_DEVNODE:
return "intf-devnode";
+ case MEDIA_GRAPH_PROP:
+ return "prop";
default:
return "unknown";
}
@@ -106,7 +108,7 @@ static void dev_dbg_obj(const char *event_name, struct media_gobj *gobj)
switch (media_type(gobj)) {
case MEDIA_GRAPH_ENTITY:
dev_dbg(gobj->mdev->dev,
- "%s id %u: entity '%s'\n",
+ "%s id 0x%08x: entity '%s'\n",
event_name, media_id(gobj),
gobj_to_entity(gobj)->name);
break;
@@ -115,7 +117,7 @@ static void dev_dbg_obj(const char *event_name, struct media_gobj *gobj)
struct media_link *link = gobj_to_link(gobj);
dev_dbg(gobj->mdev->dev,
- "%s id %u: %s link id %u ==> id %u\n",
+ "%s id 0x%08x: %s link id 0x%08x ==> id 0x%08x\n",
event_name, media_id(gobj),
media_type(link->gobj0) == MEDIA_GRAPH_PAD ?
"data" : "interface",
@@ -128,7 +130,7 @@ static void dev_dbg_obj(const char *event_name, struct media_gobj *gobj)
struct media_pad *pad = gobj_to_pad(gobj);
dev_dbg(gobj->mdev->dev,
- "%s id %u: %s%spad '%s':%d\n",
+ "%s id 0x%08x: %s%spad '%s':%d\n",
event_name, media_id(gobj),
pad->flags & MEDIA_PAD_FL_SINK ? "sink " : "",
pad->flags & MEDIA_PAD_FL_SOURCE ? "source " : "",
@@ -141,12 +143,22 @@ static void dev_dbg_obj(const char *event_name, struct media_gobj *gobj)
struct media_intf_devnode *devnode = intf_to_devnode(intf);
dev_dbg(gobj->mdev->dev,
- "%s id %u: intf_devnode %s - major: %d, minor: %d\n",
+ "%s id 0x%08x: intf_devnode %s - major: %d, minor: %d\n",
event_name, media_id(gobj),
intf_type(intf),
devnode->major, devnode->minor);
break;
}
+ case MEDIA_GRAPH_PROP:
+ {
+ struct media_prop *prop = gobj_to_prop(gobj);
+
+ dev_dbg(gobj->mdev->dev,
+ "%s id 0x%08x: prop '%s':0x%08x\n",
+ event_name, media_id(gobj),
+ prop->name, media_id(prop->owner));
+ break;
+ }
}
#endif
}
@@ -175,6 +187,9 @@ void media_gobj_create(struct media_device *mdev,
case MEDIA_GRAPH_INTF_DEVNODE:
list_add_tail(&gobj->list, &mdev->interfaces);
break;
+ case MEDIA_GRAPH_PROP:
+ list_add_tail(&gobj->list, &mdev->props);
+ break;
}
mdev->topology_version++;
@@ -212,6 +227,8 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
if (num_pads >= MEDIA_ENTITY_MAX_PADS)
return -E2BIG;
+ if (!entity->inited)
+ media_entity_init(entity);
entity->num_pads = num_pads;
entity->pads = pads;
@@ -221,6 +238,7 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
for (i = 0; i < num_pads; i++) {
pads[i].entity = entity;
pads[i].index = i;
+ INIT_LIST_HEAD(&pads[i].props);
if (mdev)
media_gobj_create(mdev, MEDIA_GRAPH_PAD,
&entity->pads[i].graph_obj);
@@ -233,6 +251,66 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
}
EXPORT_SYMBOL_GPL(media_entity_pads_init);
+static struct media_prop *media_create_prop(struct media_gobj *owner, u32 type,
+ const char *name, const void *ptr, u32 payload_size)
+{
+ struct media_prop *prop = kzalloc(sizeof(*prop) + payload_size,
+ GFP_KERNEL);
+
+ if (!prop)
+ return ERR_PTR(-ENOMEM);
+ prop->type = type;
+ strscpy(prop->name, name, sizeof(prop->name));
+ if (owner->mdev)
+ media_gobj_create(owner->mdev, MEDIA_GRAPH_PROP,
+ &prop->graph_obj);
+ prop->owner = owner;
+ prop->payload_size = payload_size;
+ if (payload_size && ptr)
+ memcpy(prop->payload, ptr, payload_size);
+ INIT_LIST_HEAD(&prop->props);
+ return prop;
+}
+
+struct media_prop *media_entity_add_prop(struct media_entity *ent, u32 type,
+ const char *name, const void *ptr, u32 payload_size)
+{
+ struct media_prop *prop;
+
+ if (!ent->inited)
+ media_entity_init(ent);
+ prop = media_create_prop(&ent->graph_obj, type,
+ name, ptr, payload_size);
+ if (!IS_ERR(prop))
+ list_add_tail(&prop->list, &ent->props);
+ return prop;
+}
+EXPORT_SYMBOL_GPL(media_entity_add_prop);
+
+struct media_prop *media_pad_add_prop(struct media_pad *pad, u32 type,
+ const char *name, const void *ptr, u32 payload_size)
+{
+ struct media_prop *prop = media_create_prop(&pad->graph_obj, type,
+ name, ptr, payload_size);
+
+ if (!IS_ERR(prop))
+ list_add_tail(&prop->list, &pad->props);
+ return prop;
+}
+EXPORT_SYMBOL_GPL(media_pad_add_prop);
+
+struct media_prop *media_prop_add_prop(struct media_prop *prop, u32 type,
+ const char *name, const void *ptr, u32 payload_size)
+{
+ struct media_prop *subprop = media_create_prop(&prop->graph_obj, type,
+ name, ptr, payload_size);
+
+ if (!IS_ERR(subprop))
+ list_add_tail(&subprop->list, &prop->props);
+ return subprop;
+}
+EXPORT_SYMBOL_GPL(media_prop_add_prop);
+
/* -----------------------------------------------------------------------------
* Graph traversal
*/
@@ -618,14 +696,25 @@ EXPORT_SYMBOL_GPL(media_entity_put);
* Links management
*/
-static struct media_link *media_add_link(struct list_head *head)
+static struct media_link *media_add_link(struct list_head *head,
+ u16 pad, bool add_tail)
{
- struct media_link *link;
+ struct media_link *link, *tmp;
link = kzalloc(sizeof(*link), GFP_KERNEL);
if (link == NULL)
return NULL;
+ if (add_tail) {
+ list_add_tail(&link->list, head);
+ return link;
+ }
+ list_for_each_entry(tmp, head, list) {
+ if (tmp->source->index == pad) {
+ list_add(&link->list, &tmp->list);
+ return link;
+ }
+ }
list_add_tail(&link->list, head);
return link;
@@ -699,7 +788,7 @@ media_create_pad_link(struct media_entity *source, u16 source_pad,
BUG_ON(source_pad >= source->num_pads);
BUG_ON(sink_pad >= sink->num_pads);
- link = media_add_link(&source->links);
+ link = media_add_link(&source->links, source_pad, false);
if (link == NULL)
return -ENOMEM;
@@ -714,7 +803,7 @@ media_create_pad_link(struct media_entity *source, u16 source_pad,
/* Create the backlink. Backlinks are used to help graph traversal and
* are not reported to userspace.
*/
- backlink = media_add_link(&sink->links);
+ backlink = media_add_link(&sink->links, 0, true);
if (backlink == NULL) {
__media_entity_remove_link(source, link);
return -ENOMEM;
@@ -998,7 +1087,7 @@ struct media_link *media_create_intf_link(struct media_entity *entity,
{
struct media_link *link;
- link = media_add_link(&intf->links);
+ link = media_add_link(&intf->links, 0, true);
if (link == NULL)
return NULL;
diff --git a/include/media/media-device.h b/include/media/media-device.h
index c8ddbfe8b74c..d422a1e1c367 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -101,6 +101,7 @@ struct media_device_ops {
* @interfaces: List of registered interfaces
* @pads: List of registered pads
* @links: List of registered links
+ * @props: List of registered properties
* @entity_notify: List of registered entity_notify callbacks
* @graph_mutex: Protects access to struct media_device data
* @pm_count_walk: Graph walk for power state walk. Access serialised using
@@ -170,6 +171,7 @@ struct media_device {
struct list_head interfaces;
struct list_head pads;
struct list_head links;
+ struct list_head props;
/* notify callback list invoked when a new entity is registered */
struct list_head entity_notify;
@@ -411,6 +413,10 @@ void media_device_unregister_entity_notify(struct media_device *mdev,
#define media_device_for_each_link(link, mdev) \
list_for_each_entry(link, &(mdev)->links, graph_obj.list)
+/* Iterate over all props. */
+#define media_device_for_each_prop(prop, mdev) \
+ list_for_each_entry(prop, &(mdev)->props, graph_obj.list)
+
/**
* media_device_pci_init() - create and initialize a
* struct &media_device from a PCI device.
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index e5f6960d92f6..2a8c2b22657b 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -21,6 +21,7 @@
#include <linux/bitmap.h>
#include <linux/bug.h>
+#include <linux/err.h>
#include <linux/fwnode.h>
#include <linux/kernel.h>
#include <linux/list.h>
@@ -36,12 +37,14 @@
* @MEDIA_GRAPH_LINK: Identify a media link
* @MEDIA_GRAPH_INTF_DEVNODE: Identify a media Kernel API interface via
* a device node
+ * @MEDIA_GRAPH_PROP: Identify a media property
*/
enum media_gobj_type {
MEDIA_GRAPH_ENTITY,
MEDIA_GRAPH_PAD,
MEDIA_GRAPH_LINK,
MEDIA_GRAPH_INTF_DEVNODE,
+ MEDIA_GRAPH_PROP,
};
#define MEDIA_BITS_PER_TYPE 8
@@ -65,6 +68,7 @@ enum media_gobj_type {
struct media_gobj {
struct media_device *mdev;
u32 id;
+ u16 topology_idx;
struct list_head list;
};
@@ -193,6 +197,7 @@ enum media_pad_signal_type {
* @flags: Pad flags, as defined in
* :ref:`include/uapi/linux/media.h <media_header>`
* (seek for ``MEDIA_PAD_FL_*``)
+ * @props: The list pad properties
*/
struct media_pad {
struct media_gobj graph_obj; /* must be first field in struct */
@@ -200,6 +205,33 @@ struct media_pad {
u16 index;
enum media_pad_signal_type sig_type;
unsigned long flags;
+ struct list_head props;
+};
+
+/**
+ * struct media_prop - A media property graph object.
+ *
+ * @graph_obj: Embedded structure containing the media object common data
+ * @list: Linked list associated with the object that owns the link.
+ * @owner: Graph object this property belongs to
+ * @index: Property index for the owner property array, numbered
+ * from 0 to n
+ * @type: Property type
+ * @payload_size: Property payload size (i.e. additional bytes beyond this
+ * struct)
+ * @props: The list of sub-properties
+ * @name: Property name
+ * @payload: Property payload starts here
+ */
+struct media_prop {
+ struct media_gobj graph_obj; /* must be first field in struct */
+ struct list_head list;
+ struct media_gobj *owner;
+ u32 type;
+ u32 payload_size;
+ struct list_head props;
+ char name[32];
+ u8 payload[];
};
/**
@@ -266,6 +298,7 @@ enum media_entity_type {
* @flags: Entity flags, as defined in
* :ref:`include/uapi/linux/media.h <media_header>`
* (seek for ``MEDIA_ENT_FL_*``)
+ * @inited: This struct was initialized.
* @num_pads: Number of sink and source pads.
* @num_links: Total number of links, forward and back, enabled and disabled.
* @num_backlinks: Number of backlinks
@@ -273,6 +306,7 @@ enum media_entity_type {
* re-used if entities are unregistered or registered again.
* @pads: Pads array with the size defined by @num_pads.
* @links: List of data links.
+ * @props: List of entity properties.
* @ops: Entity operations.
* @stream_count: Stream count for the entity.
* @use_count: Use count for the entity.
@@ -300,6 +334,7 @@ struct media_entity {
enum media_entity_type obj_type;
u32 function;
unsigned long flags;
+ bool inited;
u16 num_pads;
u16 num_links;
@@ -308,6 +343,7 @@ struct media_entity {
struct media_pad *pads;
struct list_head links;
+ struct list_head props;
const struct media_entity_operations *ops;
@@ -362,6 +398,20 @@ struct media_intf_devnode {
u32 minor;
};
+/**
+ * media_entity_init() - initialize the media entity struct
+ *
+ * @entity: pointer to &media_entity
+ */
+static inline void media_entity_init(struct media_entity *entity)
+{
+ INIT_LIST_HEAD(&entity->links);
+ INIT_LIST_HEAD(&entity->props);
+ entity->num_links = 0;
+ entity->num_backlinks = 0;
+ entity->inited = true;
+}
+
/**
* media_entity_id() - return the media entity graph object id
*
@@ -595,6 +645,15 @@ static inline bool media_entity_enum_intersects(
#define gobj_to_intf(gobj) \
container_of(gobj, struct media_interface, graph_obj)
+/**
+ * gobj_to_prop - returns the struct &media_prop pointer from the
+ * @gobj contained on it.
+ *
+ * @gobj: Pointer to the struct &media_gobj graph object
+ */
+#define gobj_to_prop(gobj) \
+ container_of(gobj, struct media_prop, graph_obj)
+
/**
* intf_to_devnode - returns the struct media_intf_devnode pointer from the
* @intf contained on it.
@@ -775,6 +834,265 @@ int media_create_pad_links(const struct media_device *mdev,
void __media_entity_remove_links(struct media_entity *entity);
+/**
+ * media_entity_add_prop() - Add property to entity
+ *
+ * @entity: entity where to add the property
+ * @type: property type
+ * @name: property name
+ * @ptr: property pointer to payload
+ * @payload_size: property payload size
+ *
+ * Returns the new property on success, or an error pointer on failure.
+ */
+struct media_prop *media_entity_add_prop(struct media_entity *ent, u32 type,
+ const char *name, const void *ptr, u32 payload_size);
+
+/**
+ * media_pad_add_prop() - Add property to pad
+ *
+ * @pad: pad where to add the property
+ * @type: property type
+ * @name: property name
+ * @ptr: property pointer to payload
+ * @payload_size: property payload size
+ *
+ * Returns the new property on success, or an error pointer on failure.
+ */
+struct media_prop *media_pad_add_prop(struct media_pad *pad, u32 type,
+ const char *name, const void *ptr, u32 payload_size);
+
+/**
+ * media_prop() - Add sub-property to property
+ *
+ * @prop: property where to add the sub-property
+ * @type: sub-property type
+ * @name: sub-property name
+ * @ptr: sub-property pointer to payload
+ * @payload_size: sub-property payload size
+ *
+ * Returns the new property on success, or an error pointer on failure.
+ */
+struct media_prop *media_prop_add_prop(struct media_prop *prop, u32 type,
+ const char *name, const void *ptr, u32 payload_size);
+
+/**
+ * media_entity_add_prop_group() - Add group property to entity
+ *
+ * @entity: entity where to add the property
+ * @name: property name
+ *
+ * Returns the new property on success, or an error pointer on failure.
+ */
+static inline struct media_prop *
+media_entity_add_prop_group(struct media_entity *entity, const char *name)
+{
+ return media_entity_add_prop(entity, MEDIA_PROP_TYPE_GROUP,
+ name, NULL, 0);
+}
+
+/**
+ * media_entity_add_prop_u64() - Add u64 property to entity
+ *
+ * @entity: entity where to add the property
+ * @name: property name
+ * @val: property value
+ *
+ * Returns 0 on success, or an error on failure.
+ */
+static inline int media_entity_add_prop_u64(struct media_entity *entity,
+ const char *name, u64 val)
+{
+ struct media_prop *prop =
+ media_entity_add_prop(entity, MEDIA_PROP_TYPE_U64,
+ name, &val, sizeof(val));
+
+ return PTR_ERR_OR_ZERO(prop);
+}
+
+/**
+ * media_entity_add_prop_s64() - Add s64 property to entity
+ *
+ * @entity: entity where to add the property
+ * @name: property name
+ * @val: property value
+ *
+ * Returns 0 on success, or an error on failure.
+ */
+static inline int media_entity_add_prop_s64(struct media_entity *entity,
+ const char *name, s64 val)
+{
+ struct media_prop *prop =
+ media_entity_add_prop(entity, MEDIA_PROP_TYPE_S64,
+ name, &val, sizeof(val));
+
+ return PTR_ERR_OR_ZERO(prop);
+}
+
+/**
+ * media_entity_add_prop_string() - Add string property to entity
+ *
+ * @entity: entity where to add the property
+ * @name: property name
+ * @string: property string value
+ *
+ * Returns 0 on success, or an error on failure.
+ */
+static inline int media_entity_add_prop_string(struct media_entity *entity,
+ const char *name, const char *string)
+{
+ struct media_prop *prop =
+ media_entity_add_prop(entity, MEDIA_PROP_TYPE_STRING,
+ name, string, strlen(string) + 1);
+
+ return PTR_ERR_OR_ZERO(prop);
+}
+
+/**
+ * media_pad_add_prop_group() - Add group property to pad
+ *
+ * @pad: pad where to add the property
+ * @name: property name
+ *
+ * Returns the new property on success, or an error pointer on failure.
+ */
+static inline struct media_prop *media_pad_add_prop_group(struct media_pad *pad,
+ const char *name)
+{
+ return media_pad_add_prop(pad, MEDIA_PROP_TYPE_GROUP,
+ name, NULL, 0);
+}
+
+/**
+ * media_pad_add_prop_u64() - Add u64 property to pad
+ *
+ * @pad: pad where to add the property
+ * @name: property name
+ * @val: property value
+ *
+ * Returns 0 on success, or an error on failure.
+ */
+static inline int media_pad_add_prop_u64(struct media_pad *pad,
+ const char *name, u64 val)
+{
+ struct media_prop *prop =
+ media_pad_add_prop(pad, MEDIA_PROP_TYPE_U64,
+ name, &val, sizeof(val));
+
+ return PTR_ERR_OR_ZERO(prop);
+}
+
+/**
+ * media_pad_add_prop_s64() - Add s64 property to pad
+ *
+ * @pad: pad where to add the property
+ * @name: property name
+ * @val: property value
+ *
+ * Returns 0 on success, or an error on failure.
+ */
+static inline int media_pad_add_prop_s64(struct media_pad *pad,
+ const char *name, s64 val)
+{
+ struct media_prop *prop =
+ media_pad_add_prop(pad, MEDIA_PROP_TYPE_S64,
+ name, &val, sizeof(val));
+
+ return PTR_ERR_OR_ZERO(prop);
+}
+
+/**
+ * media_pad_add_prop_string() - Add string property to pad
+ *
+ * @pad: pad where to add the property
+ * @name: property name
+ * @string: property string value
+ *
+ * Returns 0 on success, or an error on failure.
+ */
+static inline int media_pad_add_prop_string(struct media_pad *pad,
+ const char *name, const char *string)
+{
+ struct media_prop *prop =
+ media_pad_add_prop(pad, MEDIA_PROP_TYPE_STRING,
+ name, string, strlen(string) + 1);
+
+ return PTR_ERR_OR_ZERO(prop);
+}
+
+/**
+ * media_prop_add_prop_group() - Add group sub-property to property
+ *
+ * @prop: property where to add the sub-property
+ * @name: sub-property name
+ *
+ * Returns the new property on success, or an error pointer on failure.
+ */
+static inline struct media_prop *
+media_prop_add_prop_group(struct media_prop *prop, const char *name)
+{
+ return media_prop_add_prop(prop, MEDIA_PROP_TYPE_GROUP,
+ name, NULL, 0);
+}
+
+/**
+ * media_prop_add_prop_u64() - Add u64 property to property
+ *
+ * @prop: property where to add the sub-property
+ * @name: sub-property name
+ * @val: sub-property value
+ *
+ * Returns 0 on success, or an error on failure.
+ */
+static inline int media_prop_add_prop_u64(struct media_prop *prop,
+ const char *name, u64 val)
+{
+ struct media_prop *subprop =
+ media_prop_add_prop(prop, MEDIA_PROP_TYPE_U64,
+ name, &val, sizeof(val));
+
+ return PTR_ERR_OR_ZERO(subprop);
+}
+
+/**
+ * media_prop_add_prop_s64() - Add s64 property to property
+ *
+ * @prop: property where to add the sub-property
+ * @name: sub-property name
+ * @val: sub-property value
+ *
+ * Returns 0 on success, or an error on failure.
+ */
+static inline int media_prop_add_prop_s64(struct media_prop *prop,
+ const char *name, s64 val)
+{
+ struct media_prop *subprop =
+ media_prop_add_prop(prop, MEDIA_PROP_TYPE_S64,
+ name, &val, sizeof(val));
+
+ return PTR_ERR_OR_ZERO(subprop);
+}
+
+/**
+ * media_prop_add_prop_string() - Add string property to property
+ *
+ * @prop: property where to add the sub-property
+ * @name: sub-property name
+ * @string: sub-property string value
+ *
+ * Returns 0 on success, or an error on failure.
+ */
+static inline int media_prop_add_prop_string(struct media_prop *prop,
+ const char *name,
+ const char *string)
+{
+ struct media_prop *subprop =
+ media_prop_add_prop(prop, MEDIA_PROP_TYPE_STRING,
+ name, string, strlen(string) + 1);
+
+ return PTR_ERR_OR_ZERO(subprop);
+}
+
/**
* media_entity_remove_links() - remove all links associated with an entity
*
--
2.19.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [RFCv4 PATCH 3/3] vimc: add property test code
2018-11-21 15:40 [RFCv4 PATCH 0/3] This RFC patch series implements properties for the media controller Hans Verkuil
2018-11-21 15:40 ` [RFCv4 PATCH 1/3] uapi/linux/media.h: add property support Hans Verkuil
2018-11-21 15:40 ` [RFCv4 PATCH 2/3] media controller: add properties support Hans Verkuil
@ 2018-11-21 15:40 ` Hans Verkuil
2018-12-12 9:04 ` Mauro Carvalho Chehab
2018-12-12 7:58 ` [RFCv4 PATCH 0/3] This RFC patch series implements properties for the media controller Mauro Carvalho Chehab
3 siblings, 1 reply; 13+ messages in thread
From: Hans Verkuil @ 2018-11-21 15:40 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
Add properties to entities and pads to be able to test the
properties API.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
drivers/media/platform/vimc/vimc-common.c | 50 +++++++++++++++++++++++
1 file changed, 50 insertions(+)
diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
index dee1b9dfc4f6..2f70e4e64790 100644
--- a/drivers/media/platform/vimc/vimc-common.c
+++ b/drivers/media/platform/vimc/vimc-common.c
@@ -415,6 +415,7 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
const unsigned long *pads_flag,
const struct v4l2_subdev_ops *sd_ops)
{
+ struct media_prop *prop = NULL;
int ret;
/* Allocate the pads */
@@ -452,6 +453,55 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
goto err_clean_m_ent;
}
+ ret = media_entity_add_prop_u64(&sd->entity, "u64", ~1);
+ if (!ret)
+ ret = media_entity_add_prop_s64(&sd->entity, "s64", -5);
+ if (!ret)
+ ret = media_entity_add_prop_string(&sd->entity, "string",
+ sd->name);
+ if (!ret) {
+ prop = media_entity_add_prop_group(&sd->entity, "empty-group");
+ ret = PTR_ERR_OR_ZERO(prop);
+ }
+ if (!ret) {
+ prop = media_entity_add_prop_group(&sd->entity, "group");
+ ret = PTR_ERR_OR_ZERO(prop);
+ }
+ if (!ret)
+ ret = media_prop_add_prop_u64(prop, "u64", 42);
+ if (!ret)
+ ret = media_prop_add_prop_s64(prop, "s64", -42);
+ if (!ret)
+ ret = media_prop_add_prop_string(prop, "string", "42");
+ if (!ret)
+ ret = media_pad_add_prop_u64(&sd->entity.pads[num_pads - 1],
+ "u64", ~1);
+ if (!ret)
+ ret = media_pad_add_prop_s64(&sd->entity.pads[num_pads - 1],
+ "s64", -5);
+ if (!ret) {
+ prop = media_pad_add_prop_group(&sd->entity.pads[num_pads - 1],
+ "group");
+ ret = PTR_ERR_OR_ZERO(prop);
+ }
+ if (!ret)
+ ret = media_prop_add_prop_u64(prop, "u64", 24);
+ if (!ret)
+ ret = media_prop_add_prop_s64(prop, "s64", -24);
+ if (!ret)
+ ret = media_pad_add_prop_string(&sd->entity.pads[0],
+ "string", sd->name);
+ if (!ret)
+ ret = media_prop_add_prop_string(prop, "string", "24");
+ if (!ret) {
+ prop = media_prop_add_prop_group(prop, "subgroup");
+ ret = PTR_ERR_OR_ZERO(prop);
+ }
+ if (!ret)
+ ret = media_prop_add_prop_string(prop, "string", "substring");
+ if (ret)
+ goto err_clean_m_ent;
+
return 0;
err_clean_m_ent:
--
2.19.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [RFCv4 PATCH 0/3] This RFC patch series implements properties for the media controller.
2018-11-21 15:40 [RFCv4 PATCH 0/3] This RFC patch series implements properties for the media controller Hans Verkuil
` (2 preceding siblings ...)
2018-11-21 15:40 ` [RFCv4 PATCH 3/3] vimc: add property test code Hans Verkuil
@ 2018-12-12 7:58 ` Mauro Carvalho Chehab
2018-12-12 8:27 ` Hans Verkuil
3 siblings, 1 reply; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2018-12-12 7:58 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media
Hi Hans,
Em Wed, 21 Nov 2018 16:40:21 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> The main changes since RFCv3 are:
>
> - Add entity index to media_v2_pad
> - Add source/sink pad index to media_v2_link
> - Add owner_idx and owner type flags to media_v2_prop
Sorry, but I didn't get why this is needed for properties to work
(if the changes are not directly related to properties, please add
on separate patches, in order to make easier for review/understanding).
The lack of an uAPI documentation at the patchset makes it harder
to understand.
For the last one, you added a documentation at kAPI:
> + * @owner_idx: Index to entities/pads/properties, depending on the owner ID
> + * type.
But it doesn't really explain anything. Is it the new owner_id
field? Is it something else? Why do we need bot owner_id and
owner_idx?
>
> An updated v4l2-ctl and v4l2-compliance that can report properties
> is available here:
>
> https://git.linuxtv.org/hverkuil/v4l-utils.git/log/?h=props
>
> Currently I support u64, s64 and const char * property types. And also
> a 'group' type that groups sub-properties. But it can be extended to any
> type including binary data if needed. No array support (as we have for
> controls), but there are enough reserved fields in media_v2_prop
> to add this if needed.
>
> I added properties for entities and pads to vimc, so I could test this.
>
> Note that the changes to media_device_get_topology() are hard to read
> from the patch. It is easier to just look at the source code:
>
> https://git.linuxtv.org/hverkuil/media_tree.git/tree/drivers/media/media-device.c?h=mc-props
>
> I have some ideas to improve this some more:
>
> 1) Add the properties directly to media_gobj. This would simplify some
> of the code, but it would require a media_gobj_init function to
> initialize the property list. In general I am a bit unhappy about
> media_gobj_create: it doesn't really create the graph object, instead
> it just adds it to the media_device. It's confusing and it is something
> I would like to change.
>
> 2) The links between pads are stored in media_entity instead of in media_pad.
> This is a bit unexpected and makes it harder to traverse the data
> structures since to find the links for a pad you need to walk the entity
> links and find the links for that pad. Putting all links in the entity
> also mixes up pad and interface links, and it would be much cleaner if
> those are separated.
>
> 3) I still think adding support for backlinks to G_TOPOLOGY is a good idea.
> Since the current data structure represents a flattened tree that is easy
> to navigate the only thing missing for userspace is backlink support.
> This is still something that userspace needs to figure out when the kernel
> has this readily available. I think that with this in place applications
> can just do all the lookups directly on the topology data structure.
Apps don't need to follow the exact data struct model as the Kernel,
and can dynamically create any indexes they need in order to quickly
seek for a link (if search performance would be a problem).
I don't like the idea of reporting all links twice to userspace.
Specially after Spectre/Meltdown, context switches are expensive.
Duplicating data is a very bad idea, as it will enforce an specific
data model at the application and at userspace. We want to be able
to change the internals (on both sides) if needed for whatever
reason.
Also, what happens if the duplicated information is not really
the same (that could happen due to a bug somewhere)? Should
apps validate it? Worse than that, if we report the same link
twice (on both directions), userspace will send link changes
at the backlinks, making the Kernel code more complex (and
bound forever to an specific implementation) for no good reason.
>
> 1+2 are internal cleanups that can be done later.
>
> 3 is a low-priority future enhancement. This might become easier to implement
> once 1+2 are done.
>
> This is pretty much the last RFC. If everyone agree with this approach, then
> I can make a final patch series, adding documentation etc.
>
> Regards,
>
> Hans
>
>
> Hans Verkuil (3):
> uapi/linux/media.h: add property support
> media controller: add properties support
> vimc: add property test code
>
> drivers/media/media-device.c | 335 +++++++++++++++++-----
> drivers/media/media-entity.c | 107 ++++++-
> drivers/media/platform/vimc/vimc-common.c | 50 ++++
> include/media/media-device.h | 6 +
> include/media/media-entity.h | 318 ++++++++++++++++++++
> include/uapi/linux/media.h | 88 +++++-
> 6 files changed, 819 insertions(+), 85 deletions(-)
>
Thanks,
Mauro
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFCv4 PATCH 1/3] uapi/linux/media.h: add property support
2018-11-21 15:40 ` [RFCv4 PATCH 1/3] uapi/linux/media.h: add property support Hans Verkuil
@ 2018-12-12 8:18 ` Mauro Carvalho Chehab
2018-12-12 8:43 ` Hans Verkuil
0 siblings, 1 reply; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2018-12-12 8:18 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Hans Verkuil
Em Wed, 21 Nov 2018 16:40:22 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> Add a new topology struct that includes properties and adds
> index fields to quickly find references from one object to
> another in the topology arrays.
As mentioned on patch 0/3, hard to review it without documentation.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> include/uapi/linux/media.h | 88 ++++++++++++++++++++++++++++++++++++--
> 1 file changed, 84 insertions(+), 4 deletions(-)
>
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index e5d0c5c611b5..a81e9723204c 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -144,6 +144,8 @@ struct media_device_info {
> /* Entity flags */
> #define MEDIA_ENT_FL_DEFAULT (1 << 0)
> #define MEDIA_ENT_FL_CONNECTOR (1 << 1)
> +#define MEDIA_ENT_FL_PAD_IDX (1 << 2)
> +#define MEDIA_ENT_FL_PROP_IDX (1 << 3)
>
> /* OR with the entity id value to find the next entity */
> #define MEDIA_ENT_ID_FLAG_NEXT (1 << 31)
> @@ -210,6 +212,9 @@ struct media_entity_desc {
> #define MEDIA_PAD_FL_SINK (1 << 0)
> #define MEDIA_PAD_FL_SOURCE (1 << 1)
> #define MEDIA_PAD_FL_MUST_CONNECT (1 << 2)
> +#define MEDIA_PAD_FL_LINK_IDX (1 << 3)
> +#define MEDIA_PAD_FL_PROP_IDX (1 << 4)
> +#define MEDIA_PAD_FL_ENTITY_IDX (1 << 5)
>
> struct media_pad_desc {
> __u32 entity; /* entity ID */
> @@ -221,6 +226,8 @@ struct media_pad_desc {
> #define MEDIA_LNK_FL_ENABLED (1 << 0)
> #define MEDIA_LNK_FL_IMMUTABLE (1 << 1)
> #define MEDIA_LNK_FL_DYNAMIC (1 << 2)
> +#define MEDIA_LNK_FL_SOURCE_IDX (1 << 3)
> +#define MEDIA_LNK_FL_SINK_IDX (1 << 4)
Why do we need those flags?
>
> #define MEDIA_LNK_FL_LINK_TYPE (0xf << 28)
> # define MEDIA_LNK_FL_DATA_LINK (0 << 28)
> @@ -296,7 +303,9 @@ struct media_v2_entity {
> char name[64];
> __u32 function; /* Main function of the entity */
> __u32 flags;
> - __u32 reserved[5];
> + __u16 pad_idx;
> + __u16 prop_idx;
Hmm... pad_idx = 0 and prop_idx = 0 can't be used, in order to
avoid breaking backward compat. It should be documented somewhere.
I've no idea what you intend here... an entity has multiple pads.
linking it to a single one sounds a very bad idea. Also, this
information is already present at the pads.
> + __u32 reserved[4];
> } __attribute__ ((packed));
>
> /* Should match the specific fields at media_intf_devnode */
> @@ -305,11 +314,14 @@ struct media_v2_intf_devnode {
> __u32 minor;
> } __attribute__ ((packed));
>
> +#define MEDIA_INTF_FL_LINK_IDX (1 << 0)
> +
Why do we need it?
> struct media_v2_interface {
> __u32 id;
> __u32 intf_type;
> __u32 flags;
> - __u32 reserved[9];
> + __u16 link_idx;
> + __u16 reserved[17];
Why do we need a link_idx? The link itself already points to
the interface. Also, that would limit the API if we ever
need to have one interface with multiple links.
>
> union {
> struct media_v2_intf_devnode devnode;
> @@ -331,7 +343,10 @@ struct media_v2_pad {
> __u32 entity_id;
> __u32 flags;
> __u32 index;
> - __u32 reserved[4];
> + __u16 link_idx;
> + __u16 prop_idx;
> + __u16 entity_idx;
Same comments as above for link_idx and entity_idx.
> + __u16 reserved[5];
> } __attribute__ ((packed));
>
> struct media_v2_link {
> @@ -339,9 +354,68 @@ struct media_v2_link {
> __u32 source_id;
> __u32 sink_id;
> __u32 flags;
> - __u32 reserved[6];
> + __u16 source_idx;
> + __u16 sink_idx;
What do you mean by source_idx and sink_idx?
> + __u32 reserved[5];
> } __attribute__ ((packed));
>
> +#define MEDIA_PROP_TYPE_GROUP 1
> +#define MEDIA_PROP_TYPE_U64 2
> +#define MEDIA_PROP_TYPE_S64 3
> +#define MEDIA_PROP_TYPE_STRING 4
> +
> +#define MEDIA_PROP_FL_OWNER 0xf
OWNER_TYPE? Just 0-15? Perhaps we should reserve more
bits for it.
> +# define MEDIA_PROP_FL_ENTITY 0
> +# define MEDIA_PROP_FL_PAD 1
> +# define MEDIA_PROP_FL_LINK 2
> +# define MEDIA_PROP_FL_INTF 3
> +# define MEDIA_PROP_FL_PROP 4
> +#define MEDIA_PROP_FL_PROP_IDX (1 << 4)
Hmm... both OWNER and PROP_IDX are MEDIA_PROP_FL_*...
are all of them mapped as flags? If so, it doesn't seem a
good idea to me to map both as flags.
> +
> +/**
> + * struct media_v2_prop - A media property
> + *
> + * @id: The unique non-zero ID of this property
> + * @owner_id: The ID of the object this property belongs to
> + * @type: Property type
> + * @flags: Property flags
But here you added and explicit field for type and flags...
I'm confused.
> + * @name: Property name
> + * @payload_size: Property payload size, 0 for U64/S64
> + * @payload_offset: Property payload starts at this offset from &prop.id.
> + * This is 0 for U64/S64.
> + * @prop_idx: Index to sub-properties, 0 means there are no sub-properties.
> + * @owner_idx: Index to entities/pads/properties, depending on the owner ID
> + * type.
> + * @reserved: Property reserved field, will be zeroed.
> + */
> +struct media_v2_prop {
> + __u32 id;
> + __u32 owner_id;
> + __u32 type;
> + __u32 flags;
> + char name[32];
> + __u32 payload_size;
> + __u32 payload_offset;
> + __u16 prop_idx;
> + __u16 owner_idx;
> + __u32 reserved[17];
> +} __attribute__ ((packed));
> +
> +static inline const char *media_prop2s(const struct media_v2_prop *prop)
Please call it "media_prop2string". Just "s" here is confusing.
> +{
> + return (const char *)prop + prop->payload_offset;
> +}
> +
> +static inline __u64 media_prop2u64(const struct media_v2_prop *prop)
> +{
> + return *(const __u64 *)((const char *)prop + prop->payload_offset);
> +}
> +
> +static inline __s64 media_prop2s64(const struct media_v2_prop *prop)
> +{
> + return *(const __s64 *)((const char *)prop + prop->payload_offset);
> +}
> +
> struct media_v2_topology {
> __u64 topology_version;
>
> @@ -360,6 +434,10 @@ struct media_v2_topology {
> __u32 num_links;
> __u32 reserved4;
> __u64 ptr_links;
> +
> + __u32 num_props;
> + __u32 props_payload_size;
> + __u64 ptr_props;
> } __attribute__ ((packed));
>
> /* ioctls */
> @@ -368,6 +446,8 @@ struct media_v2_topology {
> #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)
> +/* Old MEDIA_IOC_G_TOPOLOGY ioctl without props support */
> +#define MEDIA_IOC_G_TOPOLOGY_OLD 0xc0487c04
I would avoid calling it "_OLD". we may have a V3, V4, V5, ... of this
ioctl.
I would, instead, define an _IOWR_COMPAT macro that would take an extra
parameter, in order to get the size of part of the struct, e. g. something
like:
#define MEDIA_IOC_G_TOPOLOGY_V1 _IOWR_COMPAT('|', 0x04, struct media_v2_topology, num_props)
Also, I don't see any good reason why to keep this at uAPI (except for a
mc-compliance tool that would test both versions - but this could be
defined directly there).
> #define MEDIA_IOC_G_TOPOLOGY _IOWR('|', 0x04, struct media_v2_topology)
> #define MEDIA_IOC_REQUEST_ALLOC _IOR ('|', 0x05, int)
>
Thanks,
Mauro
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFCv4 PATCH 0/3] This RFC patch series implements properties for the media controller.
2018-12-12 7:58 ` [RFCv4 PATCH 0/3] This RFC patch series implements properties for the media controller Mauro Carvalho Chehab
@ 2018-12-12 8:27 ` Hans Verkuil
2018-12-12 20:32 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 13+ messages in thread
From: Hans Verkuil @ 2018-12-12 8:27 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-media
On 12/12/18 8:58 AM, Mauro Carvalho Chehab wrote:
> Hi Hans,
>
> Em Wed, 21 Nov 2018 16:40:21 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>
>> The main changes since RFCv3 are:
>>
>> - Add entity index to media_v2_pad
>> - Add source/sink pad index to media_v2_link
>> - Add owner_idx and owner type flags to media_v2_prop
>
> Sorry, but I didn't get why this is needed for properties to work
> (if the changes are not directly related to properties, please add
> on separate patches, in order to make easier for review/understanding).
I can separate it.
> The lack of an uAPI documentation at the patchset makes it harder
> to understand.
>
> For the last one, you added a documentation at kAPI:
>
>> + * @owner_idx: Index to entities/pads/properties, depending on the owner ID
>> + * type.
>
> But it doesn't really explain anything. Is it the new owner_id
> field? Is it something else? Why do we need bot owner_id and
> owner_idx?
Currently when G_TOPOLOGY returns e.g. a link it has two IDs: one
for the sink object, one for the source object. The application now
has to traverse the entities array to find the entities referred to
by the link IDs.
That's painful, but by providing indices into the entities array as
well, userspace can just do a direct lookup entities[sink_idx] to
find the entity with ID sink_id.
I expect that this will will make it possible in many cases to avoid
userspace from having to create their own datastructure, but instead
they can use the returned information directly.
>
>>
>> An updated v4l2-ctl and v4l2-compliance that can report properties
>> is available here:
>>
>> https://git.linuxtv.org/hverkuil/v4l-utils.git/log/?h=props
>>
>> Currently I support u64, s64 and const char * property types. And also
>> a 'group' type that groups sub-properties. But it can be extended to any
>> type including binary data if needed. No array support (as we have for
>> controls), but there are enough reserved fields in media_v2_prop
>> to add this if needed.
>>
>> I added properties for entities and pads to vimc, so I could test this.
>>
>> Note that the changes to media_device_get_topology() are hard to read
>> from the patch. It is easier to just look at the source code:
>>
>> https://git.linuxtv.org/hverkuil/media_tree.git/tree/drivers/media/media-device.c?h=mc-props
>>
>> I have some ideas to improve this some more:
>>
>> 1) Add the properties directly to media_gobj. This would simplify some
>> of the code, but it would require a media_gobj_init function to
>> initialize the property list. In general I am a bit unhappy about
>> media_gobj_create: it doesn't really create the graph object, instead
>> it just adds it to the media_device. It's confusing and it is something
>> I would like to change.
>>
>> 2) The links between pads are stored in media_entity instead of in media_pad.
>> This is a bit unexpected and makes it harder to traverse the data
>> structures since to find the links for a pad you need to walk the entity
>> links and find the links for that pad. Putting all links in the entity
>> also mixes up pad and interface links, and it would be much cleaner if
>> those are separated.
>>
>> 3) I still think adding support for backlinks to G_TOPOLOGY is a good idea.
>> Since the current data structure represents a flattened tree that is easy
>> to navigate the only thing missing for userspace is backlink support.
>> This is still something that userspace needs to figure out when the kernel
>> has this readily available. I think that with this in place applications
>> can just do all the lookups directly on the topology data structure.
>
> Apps don't need to follow the exact data struct model as the Kernel,
> and can dynamically create any indexes they need in order to quickly
> seek for a link (if search performance would be a problem).
But if the kernel can directly without additional cost provide that
information to userspace, why on earth shouldn't we do that?
> I don't like the idea of reporting all links twice to userspace.
> Specially after Spectre/Meltdown, context switches are expensive.
You need to call G_TOPOLOGY in any case, so returning backlinks
in addition to all the other data should not add to the expense. Or
am I missing something?
>
> Duplicating data is a very bad idea, as it will enforce an specific
> data model at the application and at userspace. We want to be able
> to change the internals (on both sides) if needed for whatever
> reason.
Whenever you have a link, you also have a backlink. If userspace doesn't
need that information, they can just not query for it (i.e. set the
backlinks pointer to 0). But if they do need it, and the kernel has it
readily available, then why not export this information? Why force
applications to make their own data structures?
>
> Also, what happens if the duplicated information is not really
> the same (that could happen due to a bug somewhere)? Should
Well, that's called a bug and it should be fixed.
And if we have support for backlinks, then v4l2-compliance will most
definitely check for consistency.
> apps validate it? Worse than that, if we report the same link
> twice (on both directions), userspace will send link changes
> at the backlinks, making the Kernel code more complex (and
> bound forever to an specific implementation) for no good reason.
Huh? When introducing an S_TOPOLOGY I would expect that initially
only the flags in the links array can be changed. The backlinks
array would not be involved.
Now, having all said this, creating support for backlinks isn't
that easy without first making data structure changes in the kernel.
So I have no plans to work on backlink support any time soon (if at
all).
Regards,
Hans
>
>>
>> 1+2 are internal cleanups that can be done later.
>>
>> 3 is a low-priority future enhancement. This might become easier to implement
>> once 1+2 are done.
>>
>> This is pretty much the last RFC. If everyone agree with this approach, then
>> I can make a final patch series, adding documentation etc.
>>
>> Regards,
>>
>> Hans
>>
>>
>> Hans Verkuil (3):
>> uapi/linux/media.h: add property support
>> media controller: add properties support
>> vimc: add property test code
>>
>> drivers/media/media-device.c | 335 +++++++++++++++++-----
>> drivers/media/media-entity.c | 107 ++++++-
>> drivers/media/platform/vimc/vimc-common.c | 50 ++++
>> include/media/media-device.h | 6 +
>> include/media/media-entity.h | 318 ++++++++++++++++++++
>> include/uapi/linux/media.h | 88 +++++-
>> 6 files changed, 819 insertions(+), 85 deletions(-)
>>
>
>
>
> Thanks,
> Mauro
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFCv4 PATCH 1/3] uapi/linux/media.h: add property support
2018-12-12 8:18 ` Mauro Carvalho Chehab
@ 2018-12-12 8:43 ` Hans Verkuil
2018-12-13 12:51 ` Hans Verkuil
0 siblings, 1 reply; 13+ messages in thread
From: Hans Verkuil @ 2018-12-12 8:43 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-media, Hans Verkuil
On 12/12/18 9:18 AM, Mauro Carvalho Chehab wrote:
> Em Wed, 21 Nov 2018 16:40:22 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Add a new topology struct that includes properties and adds
>> index fields to quickly find references from one object to
>> another in the topology arrays.
>
> As mentioned on patch 0/3, hard to review it without documentation.
>
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>> include/uapi/linux/media.h | 88 ++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 84 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
>> index e5d0c5c611b5..a81e9723204c 100644
>> --- a/include/uapi/linux/media.h
>> +++ b/include/uapi/linux/media.h
>> @@ -144,6 +144,8 @@ struct media_device_info {
>> /* Entity flags */
>> #define MEDIA_ENT_FL_DEFAULT (1 << 0)
>> #define MEDIA_ENT_FL_CONNECTOR (1 << 1)
>> +#define MEDIA_ENT_FL_PAD_IDX (1 << 2)
>> +#define MEDIA_ENT_FL_PROP_IDX (1 << 3)
>>
>> /* OR with the entity id value to find the next entity */
>> #define MEDIA_ENT_ID_FLAG_NEXT (1 << 31)
>> @@ -210,6 +212,9 @@ struct media_entity_desc {
>> #define MEDIA_PAD_FL_SINK (1 << 0)
>> #define MEDIA_PAD_FL_SOURCE (1 << 1)
>> #define MEDIA_PAD_FL_MUST_CONNECT (1 << 2)
>> +#define MEDIA_PAD_FL_LINK_IDX (1 << 3)
>> +#define MEDIA_PAD_FL_PROP_IDX (1 << 4)
>> +#define MEDIA_PAD_FL_ENTITY_IDX (1 << 5)
>>
>> struct media_pad_desc {
>> __u32 entity; /* entity ID */
>> @@ -221,6 +226,8 @@ struct media_pad_desc {
>> #define MEDIA_LNK_FL_ENABLED (1 << 0)
>> #define MEDIA_LNK_FL_IMMUTABLE (1 << 1)
>> #define MEDIA_LNK_FL_DYNAMIC (1 << 2)
>
>> +#define MEDIA_LNK_FL_SOURCE_IDX (1 << 3)
>> +#define MEDIA_LNK_FL_SINK_IDX (1 << 4)
>
> Why do we need those flags?
>
>>
>> #define MEDIA_LNK_FL_LINK_TYPE (0xf << 28)
>> # define MEDIA_LNK_FL_DATA_LINK (0 << 28)
>> @@ -296,7 +303,9 @@ struct media_v2_entity {
>> char name[64];
>> __u32 function; /* Main function of the entity */
>> __u32 flags;
>> - __u32 reserved[5];
>> + __u16 pad_idx;
>> + __u16 prop_idx;
>
> Hmm... pad_idx = 0 and prop_idx = 0 can't be used, in order to
> avoid breaking backward compat. It should be documented somewhere.
And that's why we have the flags: to indicate these index fields have
valid indices. I thought about reversing index 0, preventing it from
being used, but that's just ugly.
>
> I've no idea what you intend here... an entity has multiple pads.
> linking it to a single one sounds a very bad idea. Also, this
> information is already present at the pads.
pad_idx is an index into the pads array where the pads for this entity
are stored. So all the pads for this entity are available starting at
index pad_idx and until the end of the pads array or the first pad with
a different entity ID. This way there is no need to create your own data
structures just to traverse the data structures.
Same for properties.
>
>
>> + __u32 reserved[4];
>> } __attribute__ ((packed));
>>
>> /* Should match the specific fields at media_intf_devnode */
>> @@ -305,11 +314,14 @@ struct media_v2_intf_devnode {
>> __u32 minor;
>> } __attribute__ ((packed));
>>
>> +#define MEDIA_INTF_FL_LINK_IDX (1 << 0)
>> +
>
> Why do we need it?
>
>> struct media_v2_interface {
>> __u32 id;
>> __u32 intf_type;
>> __u32 flags;
>> - __u32 reserved[9];
>> + __u16 link_idx;
>> + __u16 reserved[17];
>
> Why do we need a link_idx? The link itself already points to
> the interface. Also, that would limit the API if we ever
> need to have one interface with multiple links.
Same here: it is the index into the links array where the link for
this interface is stored.
>
>>
>> union {
>> struct media_v2_intf_devnode devnode;
>> @@ -331,7 +343,10 @@ struct media_v2_pad {
>> __u32 entity_id;
>> __u32 flags;
>> __u32 index;
>> - __u32 reserved[4];
>> + __u16 link_idx;
>> + __u16 prop_idx;
>> + __u16 entity_idx;
>
> Same comments as above for link_idx and entity_idx.
And same comments from me.
>
>> + __u16 reserved[5];
>> } __attribute__ ((packed));
>>
>> struct media_v2_link {
>> @@ -339,9 +354,68 @@ struct media_v2_link {
>> __u32 source_id;
>> __u32 sink_id;
>> __u32 flags;
>> - __u32 reserved[6];
>> + __u16 source_idx;
>> + __u16 sink_idx;
>
> What do you mean by source_idx and sink_idx?
Index of the source entity in the entities array. Ditto for the sink.
O(1) lookup for applications.
>
>> + __u32 reserved[5];
>> } __attribute__ ((packed));
>>
>> +#define MEDIA_PROP_TYPE_GROUP 1
>> +#define MEDIA_PROP_TYPE_U64 2
>> +#define MEDIA_PROP_TYPE_S64 3
>> +#define MEDIA_PROP_TYPE_STRING 4
>> +
>> +#define MEDIA_PROP_FL_OWNER 0xf
>
> OWNER_TYPE? Just 0-15? Perhaps we should reserve more
> bits for it.
You are right, this should be 0xff (equivalent to MEDIA_BITS_PER_TYPE
as defined in media/media-entity.h).
>
>> +# define MEDIA_PROP_FL_ENTITY 0
>> +# define MEDIA_PROP_FL_PAD 1
>> +# define MEDIA_PROP_FL_LINK 2
>> +# define MEDIA_PROP_FL_INTF 3
>> +# define MEDIA_PROP_FL_PROP 4
>> +#define MEDIA_PROP_FL_PROP_IDX (1 << 4)
>
> Hmm... both OWNER and PROP_IDX are MEDIA_PROP_FL_*...
> are all of them mapped as flags? If so, it doesn't seem a
> good idea to me to map both as flags.
I need to rename MEDIA_PROP_FL_OWNER to something like MEDIA_PROP_FL_OWNER_TYPE_MSK.
I.e. the first 8 (currently 4) bits of the flags field give the type of the object
that the property belongs to (entity, pad, link, interface, property).
The naming is confusing here.
>
>> +
>> +/**
>> + * struct media_v2_prop - A media property
>> + *
>> + * @id: The unique non-zero ID of this property
>> + * @owner_id: The ID of the object this property belongs to
>> + * @type: Property type
>> + * @flags: Property flags
>
> But here you added and explicit field for type and flags...
> I'm confused.
The type describes the property type: group, string, u64, s64.
Flags describe (among others) the type of the object that owns the property.
An alternative is to drop this and add a macro that extracts the object
type from an object ID since this is stored in the top 8 bits of the object
(graph) ID.
>
>> + * @name: Property name
>> + * @payload_size: Property payload size, 0 for U64/S64
>> + * @payload_offset: Property payload starts at this offset from &prop.id.
>> + * This is 0 for U64/S64.
>> + * @prop_idx: Index to sub-properties, 0 means there are no sub-properties.
>> + * @owner_idx: Index to entities/pads/properties, depending on the owner ID
>> + * type.
>> + * @reserved: Property reserved field, will be zeroed.
>> + */
>> +struct media_v2_prop {
>> + __u32 id;
>> + __u32 owner_id;
>> + __u32 type;
>> + __u32 flags;
>> + char name[32];
>> + __u32 payload_size;
>> + __u32 payload_offset;
>> + __u16 prop_idx;
>> + __u16 owner_idx;
>> + __u32 reserved[17];
>> +} __attribute__ ((packed));
>> +
>> +static inline const char *media_prop2s(const struct media_v2_prop *prop)
>
> Please call it "media_prop2string". Just "s" here is confusing.
What about media_prop2str()? Would that work for you?
>
>> +{
>> + return (const char *)prop + prop->payload_offset;
>> +}
>> +
>> +static inline __u64 media_prop2u64(const struct media_v2_prop *prop)
>> +{
>> + return *(const __u64 *)((const char *)prop + prop->payload_offset);
>> +}
>> +
>> +static inline __s64 media_prop2s64(const struct media_v2_prop *prop)
>> +{
>> + return *(const __s64 *)((const char *)prop + prop->payload_offset);
>> +}
>> +
>> struct media_v2_topology {
>> __u64 topology_version;
>>
>> @@ -360,6 +434,10 @@ struct media_v2_topology {
>> __u32 num_links;
>> __u32 reserved4;
>> __u64 ptr_links;
>> +
>> + __u32 num_props;
>> + __u32 props_payload_size;
>> + __u64 ptr_props;
>> } __attribute__ ((packed));
>>
>> /* ioctls */
>> @@ -368,6 +446,8 @@ struct media_v2_topology {
>> #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)
>> +/* Old MEDIA_IOC_G_TOPOLOGY ioctl without props support */
>> +#define MEDIA_IOC_G_TOPOLOGY_OLD 0xc0487c04
>
> I would avoid calling it "_OLD". we may have a V3, V4, V5, ... of this
> ioctl.
>
> I would, instead, define an _IOWR_COMPAT macro that would take an extra
> parameter, in order to get the size of part of the struct, e. g. something
> like:
>
> #define MEDIA_IOC_G_TOPOLOGY_V1 _IOWR_COMPAT('|', 0x04, struct media_v2_topology, num_props)
That's not a bad idea, actually.
>
> Also, I don't see any good reason why to keep this at uAPI (except for a
> mc-compliance tool that would test both versions - but this could be
> defined directly there).
Makes sense.
>
>> #define MEDIA_IOC_G_TOPOLOGY _IOWR('|', 0x04, struct media_v2_topology)
>> #define MEDIA_IOC_REQUEST_ALLOC _IOR ('|', 0x05, int)
>>
>
> Thanks,
> Mauro
>
Regards,
Hans
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFCv4 PATCH 2/3] media controller: add properties support
2018-11-21 15:40 ` [RFCv4 PATCH 2/3] media controller: add properties support Hans Verkuil
@ 2018-12-12 9:02 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2018-12-12 9:02 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Hans Verkuil
Em Wed, 21 Nov 2018 16:40:23 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> Add support for properties. In this initial implementation properties
> can be added to entities and pads. In addition, properties can be
> nested.
>
> Since this patch adds the topology_idx to the graph objects it is now
> easy to fill in the index fields in the topology to allow userspace
> to quickly look up a reference from one object to another.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/media/media-device.c | 335 +++++++++++++++++++++++++++--------
> drivers/media/media-entity.c | 107 ++++++++++-
> include/media/media-device.h | 6 +
> include/media/media-entity.h | 318 +++++++++++++++++++++++++++++++++
> 4 files changed, 685 insertions(+), 81 deletions(-)
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index bed24372e61f..099ab3532475 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -235,6 +235,21 @@ static long media_device_setup_link(struct media_device *mdev, void *arg)
> return __media_entity_setup_link(link, linkd->flags);
> }
>
> +static void walk_props(struct list_head *head, u32 *prop_idx, u32 *payload_size)
> +{
> + struct media_prop *prop;
> +
> + if (list_empty(head))
> + return;
> +
> + list_for_each_entry(prop, head, list) {
> + prop->graph_obj.topology_idx = (*prop_idx)++;
> + *payload_size += prop->payload_size;
> + }
> + list_for_each_entry(prop, head, list)
> + walk_props(&prop->props, prop_idx, payload_size);
> +}
> +
> static long media_device_get_topology(struct media_device *mdev, void *arg)
> {
> struct media_v2_topology *topo = arg;
> @@ -242,27 +257,96 @@ static long media_device_get_topology(struct media_device *mdev, void *arg)
> struct media_interface *intf;
> struct media_pad *pad;
> struct media_link *link;
> + struct media_prop *prop, *subprop;
> struct media_v2_entity kentity, __user *uentity;
> struct media_v2_interface kintf, __user *uintf;
> struct media_v2_pad kpad, __user *upad;
> struct media_v2_link klink, __user *ulink;
> + struct media_v2_prop kprop, __user *uprop;
> + u32 payload_size = 0;
> + u32 payload_offset = 0;
> + u32 entity_idx = 0;
> + u32 interface_idx = 0;
> + u32 pad_idx = 0;
> + u32 link_idx = 0;
> + u32 prop_idx = 0;
> unsigned int i;
> int ret = 0;
>
> topo->topology_version = mdev->topology_version;
>
> + /* Set entity/pad/link indices and number of entities */
> + media_device_for_each_entity(entity, mdev) {
> + entity->graph_obj.topology_idx = entity_idx++;
> + walk_props(&entity->props, &prop_idx, &payload_size);
> + for (i = 0; i < entity->num_pads; i++) {
> + pad = entity->pads + i;
> + pad->graph_obj.topology_idx = pad_idx++;
> + walk_props(&pad->props, &prop_idx, &payload_size);
> + }
> + /* Note: links are ordered by source pad index */
> + list_for_each_entry(link, &entity->links, list)
> + if (!link->is_backlink)
> + link->graph_obj.topology_idx = link_idx++;
> + }
> + if (topo->ptr_entities && entity_idx > topo->num_entities)
> + ret = -ENOSPC;
> + topo->num_entities = entity_idx;
> + topo->reserved1 = 0;
> +
> + /* Set interface/link indices and number of interfaces */
> + media_device_for_each_intf(intf, mdev) {
> + intf->graph_obj.topology_idx = interface_idx++;
> + list_for_each_entry(link, &intf->links, list)
> + link->graph_obj.topology_idx = link_idx++;
> + }
> +
> + if (topo->ptr_interfaces && interface_idx > topo->num_interfaces)
> + ret = -ENOSPC;
> + topo->num_interfaces = interface_idx;
> + topo->reserved2 = 0;
> +
> + /* Set number of pads */
> + if (topo->ptr_pads && pad_idx > topo->num_pads)
> + ret = -ENOSPC;
> + topo->num_pads = pad_idx;
> + topo->reserved3 = 0;
> +
> + /* Set number of links */
> + if (topo->ptr_links && link_idx > topo->num_links)
> + ret = -ENOSPC;
> + topo->num_links = link_idx;
> + topo->reserved4 = 0;
> +
> + /* Set number of properties */
> + if (topo->ptr_props &&
> + (prop_idx > topo->num_props ||
> + payload_size > topo->props_payload_size))
> + ret = -ENOSPC;
> + topo->num_props = prop_idx;
> + topo->props_payload_size = payload_size;
> +
> + if (ret)
> + return ret;
> +
> + /*
> + * We use u16 for the graph object indices,
> + * so check that it will fit in 16 bits.
> + */
> + if (WARN_ON(entity_idx >= 0x10000 ||
> + interface_idx >= 0x10000 ||
> + pad_idx >= 0x10000 ||
> + link_idx >= 0x10000 ||
> + prop_idx >= 0x10000))
> + return -EINVAL;
> +
This is really hacky!!! It shows that the proposed API is not OK.
I understand if we had some checks like the above internally at
the Kernel, avoiding it to have a very large number of objects,
but the API should be independent on whatever internal limit
our current implementation has.
> /* Get entities and number of entities */
> - i = 0;
> uentity = media_get_uptr(topo->ptr_entities);
> - media_device_for_each_entity(entity, mdev) {
> - i++;
> - if (ret || !uentity)
> - continue;
> + if (!uentity)
> + goto skip_entities;
>
> - if (i > topo->num_entities) {
> - ret = -ENOSPC;
> - continue;
> - }
> + media_device_for_each_entity(entity, mdev) {
> + u16 idx = entity->graph_obj.topology_idx;
>
> /* Copy fields to userspace struct if not error */
> memset(&kentity, 0, sizeof(kentity));
> @@ -270,27 +354,30 @@ static long media_device_get_topology(struct media_device *mdev, void *arg)
> kentity.function = entity->function;
> kentity.flags = entity->flags;
> strscpy(kentity.name, entity->name,
> - sizeof(kentity.name));
> + sizeof(kentity.name));
This hunk seems broken, as it is just mangling the indentation.
> + if (entity->num_pads) {
> + kentity.pad_idx = entity->pads[0].graph_obj.topology_idx;
> + kentity.flags |= MEDIA_ENT_FL_PAD_IDX;
> + }
Already mentioned before: why? (won't repeat myself below on similar
patterns)
Userspace can just do the same while parsing the topology, if it needs a
per-entity pad index.
> + if (!list_empty(&entity->props)) {
> + prop = list_first_entry(&entity->props,
> + struct media_prop, list);
> + kentity.prop_idx = prop->graph_obj.topology_idx;
> + kentity.flags |= MEDIA_ENT_FL_PROP_IDX;
> + }
After thinking a little bit more about that, I'm also in doubt why
a property index is needed.
It should trivial for the application to do the mapping, if each
property contains the object ID of the associated graph object.
The Kernel shouldn't even warrant that the properties would be
reported on a particular order.
By adding a prop_idx, you will need to ensure that all properties
for a certain type would be placed together.
If we remove all those *_idx, the code would just be (removing
sanity checks) something similar to:
media_device_for_each_props(props, mdev)
copy_to_user(uprops, &kprops, sizeof(kprops));
>
> - if (copy_to_user(uentity, &kentity, sizeof(kentity)))
> - ret = -EFAULT;
> - uentity++;
> + if (copy_to_user(uentity + idx, &kentity, sizeof(kentity)))
> + return -EFAULT;
> }
> - topo->num_entities = i;
> - topo->reserved1 = 0;
> +skip_entities:
>
> /* Get interfaces and number of interfaces */
> - i = 0;
> uintf = media_get_uptr(topo->ptr_interfaces);
> - media_device_for_each_intf(intf, mdev) {
> - i++;
> - if (ret || !uintf)
> - continue;
> + if (!uintf)
> + goto skip_interfaces;
>
> - if (i > topo->num_interfaces) {
> - ret = -ENOSPC;
> - continue;
> - }
> + media_device_for_each_intf(intf, mdev) {
> + u16 idx = intf->graph_obj.topology_idx;
>
> memset(&kintf, 0, sizeof(kintf));
>
> @@ -298,6 +385,12 @@ static long media_device_get_topology(struct media_device *mdev, void *arg)
> kintf.id = intf->graph_obj.id;
> kintf.intf_type = intf->type;
> kintf.flags = intf->flags;
> + if (!list_empty(&intf->links)) {
> + link = list_first_entry(&intf->links,
> + struct media_link, list);
> + kintf.link_idx = link->graph_obj.topology_idx;
> + kintf.flags |= MEDIA_INTF_FL_LINK_IDX;
> + }
>
> if (media_type(&intf->graph_obj) == MEDIA_GRAPH_INTF_DEVNODE) {
> struct media_intf_devnode *devnode;
> @@ -308,74 +401,115 @@ static long media_device_get_topology(struct media_device *mdev, void *arg)
> kintf.devnode.minor = devnode->minor;
> }
>
> - if (copy_to_user(uintf, &kintf, sizeof(kintf)))
> - ret = -EFAULT;
> - uintf++;
> + if (copy_to_user(uintf + idx, &kintf, sizeof(kintf)))
> + return -EFAULT;
> }
> - topo->num_interfaces = i;
> - topo->reserved2 = 0;
> +skip_interfaces:
>
> /* Get pads and number of pads */
> - i = 0;
> upad = media_get_uptr(topo->ptr_pads);
> - media_device_for_each_pad(pad, mdev) {
> - i++;
> - if (ret || !upad)
> - continue;
> + if (!upad)
> + goto skip_pads;
>
> - if (i > topo->num_pads) {
> - ret = -ENOSPC;
> - continue;
> - }
> + media_device_for_each_pad(pad, mdev) {
> + u16 idx = pad->graph_obj.topology_idx;
>
> memset(&kpad, 0, sizeof(kpad));
>
> /* Copy pad fields to userspace struct */
> kpad.id = pad->graph_obj.id;
> kpad.entity_id = pad->entity->graph_obj.id;
> - kpad.flags = pad->flags;
> + kpad.entity_idx = pad->entity->graph_obj.topology_idx;
> + kpad.flags = pad->flags | MEDIA_PAD_FL_ENTITY_IDX;
> kpad.index = pad->index;
>
> - if (copy_to_user(upad, &kpad, sizeof(kpad)))
> - ret = -EFAULT;
> - upad++;
> + entity = pad->entity;
> + list_for_each_entry(link, &entity->links, list) {
> + if (link->source == pad) {
> + kpad.link_idx = link->graph_obj.topology_idx;
> + kpad.flags |= MEDIA_PAD_FL_LINK_IDX;
> + break;
> + }
> + }
> + if (!list_empty(&pad->props)) {
> + prop = list_first_entry(&pad->props,
> + struct media_prop, list);
> + kpad.prop_idx = prop->graph_obj.topology_idx;
> + kpad.flags |= MEDIA_PAD_FL_PROP_IDX;
> + }
> +
> + if (copy_to_user(upad + idx, &kpad, sizeof(kpad)))
> + return -EFAULT;
> }
> - topo->num_pads = i;
> - topo->reserved3 = 0;
> +skip_pads:
>
> /* Get links and number of links */
> - i = 0;
> ulink = media_get_uptr(topo->ptr_links);
> - media_device_for_each_link(link, mdev) {
> - if (link->is_backlink)
> - continue;
> -
> - i++;
> + if (!ulink)
> + goto skip_links;
>
> - if (ret || !ulink)
> - continue;
> + media_device_for_each_link(link, mdev) {
> + u16 idx;
>
> - if (i > topo->num_links) {
> - ret = -ENOSPC;
> + if (link->is_backlink)
> continue;
> - }
> + idx = link->graph_obj.topology_idx;
>
> memset(&klink, 0, sizeof(klink));
>
> /* Copy link fields to userspace struct */
> klink.id = link->graph_obj.id;
> klink.source_id = link->gobj0->id;
> + klink.source_idx = link->gobj0->topology_idx;
> klink.sink_id = link->gobj1->id;
> - klink.flags = link->flags;
> + klink.sink_idx = link->gobj1->topology_idx;
> + klink.flags = link->flags | MEDIA_LNK_FL_SOURCE_IDX |
> + MEDIA_LNK_FL_SINK_IDX;
>
> - if (copy_to_user(ulink, &klink, sizeof(klink)))
> - ret = -EFAULT;
> - ulink++;
> + if (copy_to_user(ulink + idx, &klink, sizeof(klink)))
> + return -EFAULT;
> }
> - topo->num_links = i;
> - topo->reserved4 = 0;
> +skip_links:
> +
> + /* Get properties and number of properties */
> + uprop = media_get_uptr(topo->ptr_props);
> + if (!uprop)
> + goto skip_props;
> +
> + payload_offset = topo->num_props * sizeof(*uprop);
> + media_device_for_each_prop(prop, mdev) {
> + u16 idx = prop->graph_obj.topology_idx;
> +
> + memset(&kprop, 0, sizeof(kprop));
> +
> + /* Copy prop fields to userspace struct */
> + kprop.id = prop->graph_obj.id;
> + kprop.owner_id = prop->owner->id;
> + kprop.owner_idx = prop->owner->topology_idx;
> + kprop.type = prop->type;
> + kprop.payload_size = prop->payload_size;
> + if (prop->payload_size)
> + kprop.payload_offset =
> + payload_offset - idx * sizeof(*uprop);
> + memcpy(kprop.name, prop->name, sizeof(kprop.name));
> + kprop.flags = media_type(prop->owner);
> + if (!list_empty(&prop->props)) {
> + subprop = list_first_entry(&prop->props,
> + struct media_prop, list);
> + kprop.prop_idx = subprop->graph_obj.topology_idx;
> + kprop.flags |= MEDIA_PROP_FL_PROP_IDX;
> + }
>
> - return ret;
> + if (copy_to_user(uprop + idx, &kprop, sizeof(kprop)))
> + return -EFAULT;
> + if (copy_to_user((u8 __user *)uprop + payload_offset,
> + prop->payload, prop->payload_size))
> + return -EFAULT;
> + payload_offset += prop->payload_size;
> + }
> +skip_props:
> +
> + return 0;
> }
Didn't review the above code, as, IMHO, it is a way more complex than
it should be.
>
> static long media_device_request_alloc(struct media_device *mdev,
> @@ -408,9 +542,10 @@ static long copy_arg_to_user(void __user *uarg, void *karg, unsigned int cmd)
> /* Do acquire the graph mutex */
> #define MEDIA_IOC_FL_GRAPH_MUTEX BIT(0)
>
> -#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user) \
> +#define MEDIA_IOC_ARG(__cmd, alts, func, fl, from_user, to_user) \
> [_IOC_NR(MEDIA_IOC_##__cmd)] = { \
> .cmd = MEDIA_IOC_##__cmd, \
> + .alternatives = (alts), \
> .fn = (long (*)(struct media_device *, void *))func, \
> .flags = fl, \
> .arg_from_user = from_user, \
> @@ -418,23 +553,33 @@ static long copy_arg_to_user(void __user *uarg, void *karg, unsigned int cmd)
> }
>
> #define MEDIA_IOC(__cmd, func, fl) \
> - MEDIA_IOC_ARG(__cmd, func, fl, copy_arg_from_user, copy_arg_to_user)
> + MEDIA_IOC_ARG(__cmd, NULL, func, fl, copy_arg_from_user, copy_arg_to_user)
> +#define MEDIA_IOC_ALTS(__cmd, alts, func, fl) \
> + MEDIA_IOC_ARG(__cmd, alts, func, fl, copy_arg_from_user, copy_arg_to_user)
>
> /* the table is indexed by _IOC_NR(cmd) */
> struct media_ioctl_info {
> unsigned int cmd;
> + const unsigned int *alternatives;
> unsigned short flags;
> long (*fn)(struct media_device *dev, void *arg);
> long (*arg_from_user)(void *karg, void __user *uarg, unsigned int cmd);
> long (*arg_to_user)(void __user *uarg, void *karg, unsigned int cmd);
> };
>
> +static const unsigned int topo_alts[] = {
> + /* Old MEDIA_IOC_G_TOPOLOGY without props support */
> + MEDIA_IOC_G_TOPOLOGY_OLD,
> + 0
> +};
> +
> static const struct media_ioctl_info ioctl_info[] = {
> MEDIA_IOC(DEVICE_INFO, media_device_get_info, MEDIA_IOC_FL_GRAPH_MUTEX),
> MEDIA_IOC(ENUM_ENTITIES, media_device_enum_entities, MEDIA_IOC_FL_GRAPH_MUTEX),
> MEDIA_IOC(ENUM_LINKS, media_device_enum_links, MEDIA_IOC_FL_GRAPH_MUTEX),
> MEDIA_IOC(SETUP_LINK, media_device_setup_link, MEDIA_IOC_FL_GRAPH_MUTEX),
> MEDIA_IOC(G_TOPOLOGY, media_device_get_topology, MEDIA_IOC_FL_GRAPH_MUTEX),
> + MEDIA_IOC_ALTS(G_TOPOLOGY, topo_alts, media_device_get_topology, MEDIA_IOC_FL_GRAPH_MUTEX),
> MEDIA_IOC(REQUEST_ALLOC, media_device_request_alloc, 0),
> };
>
> @@ -448,17 +593,29 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
> char __karg[256], *karg = __karg;
> long ret;
>
> - if (_IOC_NR(cmd) >= ARRAY_SIZE(ioctl_info)
> - || ioctl_info[_IOC_NR(cmd)].cmd != cmd)
> + if (_IOC_NR(cmd) >= ARRAY_SIZE(ioctl_info))
> return -ENOIOCTLCMD;
>
> info = &ioctl_info[_IOC_NR(cmd)];
>
> + if (info->cmd != cmd) {
> + const unsigned int *p;
> +
> + for (p = info->alternatives; p && *p; p++)
> + if (*p == cmd)
> + break;
> + if (!p || !*p)
> + return -ENOIOCTLCMD;
> + }
> +
> if (_IOC_SIZE(info->cmd) > sizeof(__karg)) {
> karg = kmalloc(_IOC_SIZE(info->cmd), GFP_KERNEL);
> if (!karg)
> return -ENOMEM;
> }
> + if (_IOC_SIZE(info->cmd) > _IOC_SIZE(cmd))
> + memset(karg + _IOC_SIZE(cmd), 0,
> + _IOC_SIZE(info->cmd) - _IOC_SIZE(cmd));
>
> if (info->arg_from_user) {
> ret = info->arg_from_user(karg, arg, cmd);
> @@ -571,6 +728,16 @@ static void media_device_release(struct media_devnode *devnode)
> dev_dbg(devnode->parent, "Media device released\n");
> }
>
> +static void init_prop_list(struct media_device *mdev, struct list_head *list)
> +{
> + struct media_prop *prop;
> +
> + list_for_each_entry(prop, list, list) {
> + media_gobj_create(mdev, MEDIA_GRAPH_PROP, &prop->graph_obj);
> + init_prop_list(mdev, &prop->props);
> + }
> +}
> +
> /**
> * media_device_register_entity - Register an entity with a media device
> * @mdev: The media device
> @@ -592,9 +759,8 @@ int __must_check media_device_register_entity(struct media_device *mdev,
> /* Warn if we apparently re-register an entity */
> WARN_ON(entity->graph_obj.mdev != NULL);
Side note: it would probably make sense to change the above to:
if (WARN_ON(entity->graph_obj.mdev != NULL))
return -EINVAL;
(if the WARN_ON still applies - see below)
> entity->graph_obj.mdev = mdev;
> - INIT_LIST_HEAD(&entity->links);
> - entity->num_links = 0;
> - entity->num_backlinks = 0;
> + if (!entity->inited)
> + media_entity_init(entity);
Why to do such change?
If this can now be called twice, why are you keeping the WARN_ON()?
>
> ret = ida_alloc_min(&mdev->entity_internal_idx, 1, GFP_KERNEL);
> if (ret < 0)
> @@ -608,10 +774,17 @@ int __must_check media_device_register_entity(struct media_device *mdev,
> /* Initialize media_gobj embedded at the entity */
> media_gobj_create(mdev, MEDIA_GRAPH_ENTITY, &entity->graph_obj);
>
> + /* Initialize objects at the props */
> + init_prop_list(mdev, &entity->props);
> +
> /* Initialize objects at the pads */
> - for (i = 0; i < entity->num_pads; i++)
> + for (i = 0; i < entity->num_pads; i++) {
> media_gobj_create(mdev, MEDIA_GRAPH_PAD,
> - &entity->pads[i].graph_obj);
> + &entity->pads[i].graph_obj);
> +
> + /* Initialize objects at the pad props */
> + init_prop_list(mdev, &entity->pads[i].props);
> + }
>
> /* invoke entity_notify callbacks */
> list_for_each_entry_safe(notify, next, &mdev->entity_notify, list)
> @@ -640,6 +813,18 @@ int __must_check media_device_register_entity(struct media_device *mdev,
> }
> EXPORT_SYMBOL_GPL(media_device_register_entity);
>
> +static void media_device_free_props(struct list_head *list)
> +{
> + while (!list_empty(list)) {
> + struct media_prop *prop;
> +
> + prop = list_first_entry(list, struct media_prop, list);
> + list_del(&prop->list);
> + media_gobj_destroy(&prop->graph_obj);
> + kfree(prop);
> + }
> +}
> +
> static void __media_device_unregister_entity(struct media_entity *entity)
> {
> struct media_device *mdev = entity->graph_obj.mdev;
> @@ -661,8 +846,13 @@ static void __media_device_unregister_entity(struct media_entity *entity)
> __media_entity_remove_links(entity);
>
> /* Remove all pads that belong to this entity */
> - for (i = 0; i < entity->num_pads; i++)
> + for (i = 0; i < entity->num_pads; i++) {
> + media_device_free_props(&entity->pads[i].props);
> media_gobj_destroy(&entity->pads[i].graph_obj);
> + }
> +
> + /* Remove all props that belong to this entity */
> + media_device_free_props(&entity->props);
>
> /* Remove the entity */
> media_gobj_destroy(&entity->graph_obj);
> @@ -701,6 +891,7 @@ void media_device_init(struct media_device *mdev)
> INIT_LIST_HEAD(&mdev->interfaces);
> INIT_LIST_HEAD(&mdev->pads);
> INIT_LIST_HEAD(&mdev->links);
> + INIT_LIST_HEAD(&mdev->props);
> INIT_LIST_HEAD(&mdev->entity_notify);
>
> mutex_init(&mdev->req_queue_mutex);
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 0b1cb3559140..dfd0073eae31 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -34,6 +34,8 @@ static inline const char *gobj_type(enum media_gobj_type type)
> return "link";
> case MEDIA_GRAPH_INTF_DEVNODE:
> return "intf-devnode";
> + case MEDIA_GRAPH_PROP:
> + return "prop";
> default:
> return "unknown";
> }
> @@ -106,7 +108,7 @@ static void dev_dbg_obj(const char *event_name, struct media_gobj *gobj)
> switch (media_type(gobj)) {
> case MEDIA_GRAPH_ENTITY:
> dev_dbg(gobj->mdev->dev,
> - "%s id %u: entity '%s'\n",
> + "%s id 0x%08x: entity '%s'\n",
Makes sense, but should probably be on a separate patch
(same applies to other similar debug patches below - for the existing
stuff).
> event_name, media_id(gobj),
> gobj_to_entity(gobj)->name);
> break;
> @@ -115,7 +117,7 @@ static void dev_dbg_obj(const char *event_name, struct media_gobj *gobj)
> struct media_link *link = gobj_to_link(gobj);
>
> dev_dbg(gobj->mdev->dev,
> - "%s id %u: %s link id %u ==> id %u\n",
> + "%s id 0x%08x: %s link id 0x%08x ==> id 0x%08x\n",
> event_name, media_id(gobj),
> media_type(link->gobj0) == MEDIA_GRAPH_PAD ?
> "data" : "interface",
> @@ -128,7 +130,7 @@ static void dev_dbg_obj(const char *event_name, struct media_gobj *gobj)
> struct media_pad *pad = gobj_to_pad(gobj);
>
> dev_dbg(gobj->mdev->dev,
> - "%s id %u: %s%spad '%s':%d\n",
> + "%s id 0x%08x: %s%spad '%s':%d\n",
> event_name, media_id(gobj),
> pad->flags & MEDIA_PAD_FL_SINK ? "sink " : "",
> pad->flags & MEDIA_PAD_FL_SOURCE ? "source " : "",
> @@ -141,12 +143,22 @@ static void dev_dbg_obj(const char *event_name, struct media_gobj *gobj)
> struct media_intf_devnode *devnode = intf_to_devnode(intf);
>
> dev_dbg(gobj->mdev->dev,
> - "%s id %u: intf_devnode %s - major: %d, minor: %d\n",
> + "%s id 0x%08x: intf_devnode %s - major: %d, minor: %d\n",
> event_name, media_id(gobj),
> intf_type(intf),
> devnode->major, devnode->minor);
> break;
> }
> + case MEDIA_GRAPH_PROP:
> + {
> + struct media_prop *prop = gobj_to_prop(gobj);
> +
> + dev_dbg(gobj->mdev->dev,
> + "%s id 0x%08x: prop '%s':0x%08x\n",
> + event_name, media_id(gobj),
> + prop->name, media_id(prop->owner));
> + break;
> + }
> }
> #endif
> }
> @@ -175,6 +187,9 @@ void media_gobj_create(struct media_device *mdev,
> case MEDIA_GRAPH_INTF_DEVNODE:
> list_add_tail(&gobj->list, &mdev->interfaces);
> break;
> + case MEDIA_GRAPH_PROP:
> + list_add_tail(&gobj->list, &mdev->props);
> + break;
> }
>
> mdev->topology_version++;
> @@ -212,6 +227,8 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
> if (num_pads >= MEDIA_ENTITY_MAX_PADS)
> return -E2BIG;
>
> + if (!entity->inited)
> + media_entity_init(entity);
> entity->num_pads = num_pads;
> entity->pads = pads;
>
> @@ -221,6 +238,7 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
> for (i = 0; i < num_pads; i++) {
> pads[i].entity = entity;
> pads[i].index = i;
> + INIT_LIST_HEAD(&pads[i].props);
> if (mdev)
> media_gobj_create(mdev, MEDIA_GRAPH_PAD,
> &entity->pads[i].graph_obj);
> @@ -233,6 +251,66 @@ int media_entity_pads_init(struct media_entity *entity, u16 num_pads,
> }
> EXPORT_SYMBOL_GPL(media_entity_pads_init);
>
> +static struct media_prop *media_create_prop(struct media_gobj *owner, u32 type,
> + const char *name, const void *ptr, u32 payload_size)
> +{
> + struct media_prop *prop = kzalloc(sizeof(*prop) + payload_size,
> + GFP_KERNEL);
> +
> + if (!prop)
> + return ERR_PTR(-ENOMEM);
> + prop->type = type;
> + strscpy(prop->name, name, sizeof(prop->name));
> + if (owner->mdev)
> + media_gobj_create(owner->mdev, MEDIA_GRAPH_PROP,
> + &prop->graph_obj);
> + prop->owner = owner;
> + prop->payload_size = payload_size;
> + if (payload_size && ptr)
> + memcpy(prop->payload, ptr, payload_size);
> + INIT_LIST_HEAD(&prop->props);
> + return prop;
> +}
> +
> +struct media_prop *media_entity_add_prop(struct media_entity *ent, u32 type,
> + const char *name, const void *ptr, u32 payload_size)
> +{
> + struct media_prop *prop;
> +
> + if (!ent->inited)
> + media_entity_init(ent);
> + prop = media_create_prop(&ent->graph_obj, type,
> + name, ptr, payload_size);
> + if (!IS_ERR(prop))
> + list_add_tail(&prop->list, &ent->props);
> + return prop;
> +}
> +EXPORT_SYMBOL_GPL(media_entity_add_prop);
> +
> +struct media_prop *media_pad_add_prop(struct media_pad *pad, u32 type,
> + const char *name, const void *ptr, u32 payload_size)
> +{
> + struct media_prop *prop = media_create_prop(&pad->graph_obj, type,
> + name, ptr, payload_size);
> +
> + if (!IS_ERR(prop))
> + list_add_tail(&prop->list, &pad->props);
> + return prop;
> +}
> +EXPORT_SYMBOL_GPL(media_pad_add_prop);
> +
> +struct media_prop *media_prop_add_prop(struct media_prop *prop, u32 type,
> + const char *name, const void *ptr, u32 payload_size)
> +{
> + struct media_prop *subprop = media_create_prop(&prop->graph_obj, type,
> + name, ptr, payload_size);
> +
> + if (!IS_ERR(subprop))
> + list_add_tail(&subprop->list, &prop->props);
> + return subprop;
> +}
> +EXPORT_SYMBOL_GPL(media_prop_add_prop);
> +
> /* -----------------------------------------------------------------------------
> * Graph traversal
> */
My next comments apply to the code that starts here:
-----------------------------------
> @@ -618,14 +696,25 @@ EXPORT_SYMBOL_GPL(media_entity_put);
> * Links management
> */
>
> -static struct media_link *media_add_link(struct list_head *head)
> +static struct media_link *media_add_link(struct list_head *head,
> + u16 pad, bool add_tail)
> {
> - struct media_link *link;
> + struct media_link *link, *tmp;
>
> link = kzalloc(sizeof(*link), GFP_KERNEL);
> if (link == NULL)
> return NULL;
>
> + if (add_tail) {
> + list_add_tail(&link->list, head);
> + return link;
> + }
> + list_for_each_entry(tmp, head, list) {
> + if (tmp->source->index == pad) {
> + list_add(&link->list, &tmp->list);
> + return link;
> + }
> + }
> list_add_tail(&link->list, head);
>
> return link;
> @@ -699,7 +788,7 @@ media_create_pad_link(struct media_entity *source, u16 source_pad,
> BUG_ON(source_pad >= source->num_pads);
> BUG_ON(sink_pad >= sink->num_pads);
>
> - link = media_add_link(&source->links);
> + link = media_add_link(&source->links, source_pad, false);
> if (link == NULL)
> return -ENOMEM;
>
> @@ -714,7 +803,7 @@ media_create_pad_link(struct media_entity *source, u16 source_pad,
> /* Create the backlink. Backlinks are used to help graph traversal and
> * are not reported to userspace.
> */
> - backlink = media_add_link(&sink->links);
> + backlink = media_add_link(&sink->links, 0, true);
> if (backlink == NULL) {
> __media_entity_remove_link(source, link);
> return -ENOMEM;
> @@ -998,7 +1087,7 @@ struct media_link *media_create_intf_link(struct media_entity *entity,
> {
> struct media_link *link;
>
> - link = media_add_link(&intf->links);
> + link = media_add_link(&intf->links, 0, true);
> if (link == NULL)
> return NULL;
Those hunks seem confusing to me... it doesn't sound directly related
to adding properties. Is it fixing a bug? What bug?
Anyway, it sounds better to split on a separate patch that would
provide a better explanation about why this change is needed and
how it was implemented.
>
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index c8ddbfe8b74c..d422a1e1c367 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -101,6 +101,7 @@ struct media_device_ops {
> * @interfaces: List of registered interfaces
> * @pads: List of registered pads
> * @links: List of registered links
> + * @props: List of registered properties
> * @entity_notify: List of registered entity_notify callbacks
> * @graph_mutex: Protects access to struct media_device data
> * @pm_count_walk: Graph walk for power state walk. Access serialised using
> @@ -170,6 +171,7 @@ struct media_device {
> struct list_head interfaces;
> struct list_head pads;
> struct list_head links;
> + struct list_head props;
>
> /* notify callback list invoked when a new entity is registered */
> struct list_head entity_notify;
> @@ -411,6 +413,10 @@ void media_device_unregister_entity_notify(struct media_device *mdev,
> #define media_device_for_each_link(link, mdev) \
> list_for_each_entry(link, &(mdev)->links, graph_obj.list)
>
> +/* Iterate over all props. */
> +#define media_device_for_each_prop(prop, mdev) \
> + list_for_each_entry(prop, &(mdev)->props, graph_obj.list)
> +
> /**
> * media_device_pci_init() - create and initialize a
> * struct &media_device from a PCI device.
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index e5f6960d92f6..2a8c2b22657b 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -21,6 +21,7 @@
>
> #include <linux/bitmap.h>
> #include <linux/bug.h>
> +#include <linux/err.h>
> #include <linux/fwnode.h>
> #include <linux/kernel.h>
> #include <linux/list.h>
> @@ -36,12 +37,14 @@
> * @MEDIA_GRAPH_LINK: Identify a media link
> * @MEDIA_GRAPH_INTF_DEVNODE: Identify a media Kernel API interface via
> * a device node
> + * @MEDIA_GRAPH_PROP: Identify a media property
> */
> enum media_gobj_type {
> MEDIA_GRAPH_ENTITY,
> MEDIA_GRAPH_PAD,
> MEDIA_GRAPH_LINK,
> MEDIA_GRAPH_INTF_DEVNODE,
> + MEDIA_GRAPH_PROP,
> };
>
> #define MEDIA_BITS_PER_TYPE 8
> @@ -65,6 +68,7 @@ enum media_gobj_type {
> struct media_gobj {
> struct media_device *mdev;
> u32 id;
> + u16 topology_idx;
> struct list_head list;
> };
>
> @@ -193,6 +197,7 @@ enum media_pad_signal_type {
> * @flags: Pad flags, as defined in
> * :ref:`include/uapi/linux/media.h <media_header>`
> * (seek for ``MEDIA_PAD_FL_*``)
> + * @props: The list pad properties
> */
> struct media_pad {
> struct media_gobj graph_obj; /* must be first field in struct */
> @@ -200,6 +205,33 @@ struct media_pad {
> u16 index;
> enum media_pad_signal_type sig_type;
> unsigned long flags;
> + struct list_head props;
> +};
> +
> +/**
> + * struct media_prop - A media property graph object.
> + *
> + * @graph_obj: Embedded structure containing the media object common data
> + * @list: Linked list associated with the object that owns the link.
> + * @owner: Graph object this property belongs to
> + * @index: Property index for the owner property array, numbered
> + * from 0 to n
> + * @type: Property type
> + * @payload_size: Property payload size (i.e. additional bytes beyond this
> + * struct)
> + * @props: The list of sub-properties
> + * @name: Property name
> + * @payload: Property payload starts here
> + */
> +struct media_prop {
> + struct media_gobj graph_obj; /* must be first field in struct */
> + struct list_head list;
> + struct media_gobj *owner;
> + u32 type;
> + u32 payload_size;
> + struct list_head props;
> + char name[32];
> + u8 payload[];
> };
>
> /**
> @@ -266,6 +298,7 @@ enum media_entity_type {
> * @flags: Entity flags, as defined in
> * :ref:`include/uapi/linux/media.h <media_header>`
> * (seek for ``MEDIA_ENT_FL_*``)
> + * @inited: This struct was initialized.
> * @num_pads: Number of sink and source pads.
> * @num_links: Total number of links, forward and back, enabled and disabled.
> * @num_backlinks: Number of backlinks
> @@ -273,6 +306,7 @@ enum media_entity_type {
> * re-used if entities are unregistered or registered again.
> * @pads: Pads array with the size defined by @num_pads.
> * @links: List of data links.
> + * @props: List of entity properties.
> * @ops: Entity operations.
> * @stream_count: Stream count for the entity.
> * @use_count: Use count for the entity.
> @@ -300,6 +334,7 @@ struct media_entity {
> enum media_entity_type obj_type;
> u32 function;
> unsigned long flags;
> + bool inited;
>
> u16 num_pads;
> u16 num_links;
> @@ -308,6 +343,7 @@ struct media_entity {
>
> struct media_pad *pads;
> struct list_head links;
> + struct list_head props;
>
> const struct media_entity_operations *ops;
>
> @@ -362,6 +398,20 @@ struct media_intf_devnode {
> u32 minor;
> };
>
> +/**
> + * media_entity_init() - initialize the media entity struct
> + *
> + * @entity: pointer to &media_entity
> + */
> +static inline void media_entity_init(struct media_entity *entity)
> +{
> + INIT_LIST_HEAD(&entity->links);
> + INIT_LIST_HEAD(&entity->props);
> + entity->num_links = 0;
> + entity->num_backlinks = 0;
> + entity->inited = true;
> +}
> +
IMO, it would be a lot easier to understand the code if you
keep this at the C file.
> /**
> * media_entity_id() - return the media entity graph object id
> *
> @@ -595,6 +645,15 @@ static inline bool media_entity_enum_intersects(
> #define gobj_to_intf(gobj) \
> container_of(gobj, struct media_interface, graph_obj)
>
> +/**
> + * gobj_to_prop - returns the struct &media_prop pointer from the
> + * @gobj contained on it.
> + *
> + * @gobj: Pointer to the struct &media_gobj graph object
> + */
> +#define gobj_to_prop(gobj) \
> + container_of(gobj, struct media_prop, graph_obj)
> +
> /**
> * intf_to_devnode - returns the struct media_intf_devnode pointer from the
> * @intf contained on it.
> @@ -775,6 +834,265 @@ int media_create_pad_links(const struct media_device *mdev,
>
> void __media_entity_remove_links(struct media_entity *entity);
>
> +/**
> + * media_entity_add_prop() - Add property to entity
> + *
> + * @entity: entity where to add the property
> + * @type: property type
> + * @name: property name
> + * @ptr: property pointer to payload
> + * @payload_size: property payload size
> + *
> + * Returns the new property on success, or an error pointer on failure.
> + */
> +struct media_prop *media_entity_add_prop(struct media_entity *ent, u32 type,
> + const char *name, const void *ptr, u32 payload_size);
> +
> +/**
> + * media_pad_add_prop() - Add property to pad
> + *
> + * @pad: pad where to add the property
> + * @type: property type
> + * @name: property name
> + * @ptr: property pointer to payload
> + * @payload_size: property payload size
> + *
> + * Returns the new property on success, or an error pointer on failure.
> + */
> +struct media_prop *media_pad_add_prop(struct media_pad *pad, u32 type,
> + const char *name, const void *ptr, u32 payload_size);
> +
> +/**
> + * media_prop() - Add sub-property to property
> + *
> + * @prop: property where to add the sub-property
> + * @type: sub-property type
> + * @name: sub-property name
> + * @ptr: sub-property pointer to payload
> + * @payload_size: sub-property payload size
> + *
> + * Returns the new property on success, or an error pointer on failure.
> + */
> +struct media_prop *media_prop_add_prop(struct media_prop *prop, u32 type,
> + const char *name, const void *ptr, u32 payload_size);
> +
> +/**
> + * media_entity_add_prop_group() - Add group property to entity
> + *
> + * @entity: entity where to add the property
> + * @name: property name
> + *
> + * Returns the new property on success, or an error pointer on failure.
> + */
> +static inline struct media_prop *
> +media_entity_add_prop_group(struct media_entity *entity, const char *name)
> +{
> + return media_entity_add_prop(entity, MEDIA_PROP_TYPE_GROUP,
> + name, NULL, 0);
> +}
> +
> +/**
> + * media_entity_add_prop_u64() - Add u64 property to entity
> + *
> + * @entity: entity where to add the property
> + * @name: property name
> + * @val: property value
> + *
> + * Returns 0 on success, or an error on failure.
> + */
> +static inline int media_entity_add_prop_u64(struct media_entity *entity,
> + const char *name, u64 val)
> +{
> + struct media_prop *prop =
> + media_entity_add_prop(entity, MEDIA_PROP_TYPE_U64,
> + name, &val, sizeof(val));
> +
> + return PTR_ERR_OR_ZERO(prop);
> +}
> +
> +/**
> + * media_entity_add_prop_s64() - Add s64 property to entity
> + *
> + * @entity: entity where to add the property
> + * @name: property name
> + * @val: property value
> + *
> + * Returns 0 on success, or an error on failure.
> + */
> +static inline int media_entity_add_prop_s64(struct media_entity *entity,
> + const char *name, s64 val)
> +{
> + struct media_prop *prop =
> + media_entity_add_prop(entity, MEDIA_PROP_TYPE_S64,
> + name, &val, sizeof(val));
> +
> + return PTR_ERR_OR_ZERO(prop);
> +}
> +
> +/**
> + * media_entity_add_prop_string() - Add string property to entity
> + *
> + * @entity: entity where to add the property
> + * @name: property name
> + * @string: property string value
> + *
> + * Returns 0 on success, or an error on failure.
> + */
> +static inline int media_entity_add_prop_string(struct media_entity *entity,
> + const char *name, const char *string)
> +{
> + struct media_prop *prop =
> + media_entity_add_prop(entity, MEDIA_PROP_TYPE_STRING,
> + name, string, strlen(string) + 1);
> +
> + return PTR_ERR_OR_ZERO(prop);
> +}
> +
> +/**
> + * media_pad_add_prop_group() - Add group property to pad
> + *
> + * @pad: pad where to add the property
> + * @name: property name
> + *
> + * Returns the new property on success, or an error pointer on failure.
> + */
> +static inline struct media_prop *media_pad_add_prop_group(struct media_pad *pad,
> + const char *name)
> +{
> + return media_pad_add_prop(pad, MEDIA_PROP_TYPE_GROUP,
> + name, NULL, 0);
> +}
> +
> +/**
> + * media_pad_add_prop_u64() - Add u64 property to pad
> + *
> + * @pad: pad where to add the property
> + * @name: property name
> + * @val: property value
> + *
> + * Returns 0 on success, or an error on failure.
> + */
> +static inline int media_pad_add_prop_u64(struct media_pad *pad,
> + const char *name, u64 val)
> +{
> + struct media_prop *prop =
> + media_pad_add_prop(pad, MEDIA_PROP_TYPE_U64,
> + name, &val, sizeof(val));
> +
> + return PTR_ERR_OR_ZERO(prop);
> +}
> +
> +/**
> + * media_pad_add_prop_s64() - Add s64 property to pad
> + *
> + * @pad: pad where to add the property
> + * @name: property name
> + * @val: property value
> + *
> + * Returns 0 on success, or an error on failure.
> + */
> +static inline int media_pad_add_prop_s64(struct media_pad *pad,
> + const char *name, s64 val)
> +{
> + struct media_prop *prop =
> + media_pad_add_prop(pad, MEDIA_PROP_TYPE_S64,
> + name, &val, sizeof(val));
> +
> + return PTR_ERR_OR_ZERO(prop);
> +}
> +
> +/**
> + * media_pad_add_prop_string() - Add string property to pad
> + *
> + * @pad: pad where to add the property
> + * @name: property name
> + * @string: property string value
> + *
> + * Returns 0 on success, or an error on failure.
> + */
> +static inline int media_pad_add_prop_string(struct media_pad *pad,
> + const char *name, const char *string)
> +{
> + struct media_prop *prop =
> + media_pad_add_prop(pad, MEDIA_PROP_TYPE_STRING,
> + name, string, strlen(string) + 1);
> +
> + return PTR_ERR_OR_ZERO(prop);
> +}
> +
> +/**
> + * media_prop_add_prop_group() - Add group sub-property to property
> + *
> + * @prop: property where to add the sub-property
> + * @name: sub-property name
> + *
> + * Returns the new property on success, or an error pointer on failure.
> + */
> +static inline struct media_prop *
> +media_prop_add_prop_group(struct media_prop *prop, const char *name)
> +{
> + return media_prop_add_prop(prop, MEDIA_PROP_TYPE_GROUP,
> + name, NULL, 0);
> +}
> +
> +/**
> + * media_prop_add_prop_u64() - Add u64 property to property
> + *
> + * @prop: property where to add the sub-property
> + * @name: sub-property name
> + * @val: sub-property value
> + *
> + * Returns 0 on success, or an error on failure.
> + */
> +static inline int media_prop_add_prop_u64(struct media_prop *prop,
> + const char *name, u64 val)
> +{
> + struct media_prop *subprop =
> + media_prop_add_prop(prop, MEDIA_PROP_TYPE_U64,
> + name, &val, sizeof(val));
> +
> + return PTR_ERR_OR_ZERO(subprop);
> +}
> +
> +/**
> + * media_prop_add_prop_s64() - Add s64 property to property
> + *
> + * @prop: property where to add the sub-property
> + * @name: sub-property name
> + * @val: sub-property value
> + *
> + * Returns 0 on success, or an error on failure.
> + */
> +static inline int media_prop_add_prop_s64(struct media_prop *prop,
> + const char *name, s64 val)
> +{
> + struct media_prop *subprop =
> + media_prop_add_prop(prop, MEDIA_PROP_TYPE_S64,
> + name, &val, sizeof(val));
> +
> + return PTR_ERR_OR_ZERO(subprop);
> +}
> +
> +/**
> + * media_prop_add_prop_string() - Add string property to property
> + *
> + * @prop: property where to add the sub-property
> + * @name: sub-property name
> + * @string: sub-property string value
> + *
> + * Returns 0 on success, or an error on failure.
> + */
> +static inline int media_prop_add_prop_string(struct media_prop *prop,
> + const char *name,
> + const char *string)
> +{
> + struct media_prop *subprop =
> + media_prop_add_prop(prop, MEDIA_PROP_TYPE_STRING,
> + name, string, strlen(string) + 1);
> +
> + return PTR_ERR_OR_ZERO(subprop);
> +}
> +
> /**
> * media_entity_remove_links() - remove all links associated with an entity
> *
Thanks,
Mauro
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFCv4 PATCH 3/3] vimc: add property test code
2018-11-21 15:40 ` [RFCv4 PATCH 3/3] vimc: add property test code Hans Verkuil
@ 2018-12-12 9:04 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2018-12-12 9:04 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Hans Verkuil
Em Wed, 21 Nov 2018 16:40:24 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> Add properties to entities and pads to be able to test the
> properties API.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/media/platform/vimc/vimc-common.c | 50 +++++++++++++++++++++++
> 1 file changed, 50 insertions(+)
>
> diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
> index dee1b9dfc4f6..2f70e4e64790 100644
> --- a/drivers/media/platform/vimc/vimc-common.c
> +++ b/drivers/media/platform/vimc/vimc-common.c
> @@ -415,6 +415,7 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
> const unsigned long *pads_flag,
> const struct v4l2_subdev_ops *sd_ops)
> {
> + struct media_prop *prop = NULL;
> int ret;
>
> /* Allocate the pads */
> @@ -452,6 +453,55 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
> goto err_clean_m_ent;
> }
>
> + ret = media_entity_add_prop_u64(&sd->entity, "u64", ~1);
> + if (!ret)
> + ret = media_entity_add_prop_s64(&sd->entity, "s64", -5);
> + if (!ret)
> + ret = media_entity_add_prop_string(&sd->entity, "string",
> + sd->name);
> + if (!ret) {
> + prop = media_entity_add_prop_group(&sd->entity, "empty-group");
> + ret = PTR_ERR_OR_ZERO(prop);
> + }
> + if (!ret) {
> + prop = media_entity_add_prop_group(&sd->entity, "group");
> + ret = PTR_ERR_OR_ZERO(prop);
> + }
> + if (!ret)
> + ret = media_prop_add_prop_u64(prop, "u64", 42);
> + if (!ret)
> + ret = media_prop_add_prop_s64(prop, "s64", -42);
> + if (!ret)
> + ret = media_prop_add_prop_string(prop, "string", "42");
> + if (!ret)
> + ret = media_pad_add_prop_u64(&sd->entity.pads[num_pads - 1],
> + "u64", ~1);
> + if (!ret)
> + ret = media_pad_add_prop_s64(&sd->entity.pads[num_pads - 1],
> + "s64", -5);
> + if (!ret) {
> + prop = media_pad_add_prop_group(&sd->entity.pads[num_pads - 1],
> + "group");
> + ret = PTR_ERR_OR_ZERO(prop);
> + }
> + if (!ret)
> + ret = media_prop_add_prop_u64(prop, "u64", 24);
> + if (!ret)
> + ret = media_prop_add_prop_s64(prop, "s64", -24);
> + if (!ret)
> + ret = media_pad_add_prop_string(&sd->entity.pads[0],
> + "string", sd->name);
> + if (!ret)
> + ret = media_prop_add_prop_string(prop, "string", "24");
> + if (!ret) {
> + prop = media_prop_add_prop_group(prop, "subgroup");
> + ret = PTR_ERR_OR_ZERO(prop);
> + }
> + if (!ret)
> + ret = media_prop_add_prop_string(prop, "string", "substring");
> + if (ret)
> + goto err_clean_m_ent;
> +
> return 0;
>
> err_clean_m_ent:
Sounds OK to me.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFCv4 PATCH 0/3] This RFC patch series implements properties for the media controller.
2018-12-12 8:27 ` Hans Verkuil
@ 2018-12-12 20:32 ` Mauro Carvalho Chehab
2018-12-13 9:29 ` Hans Verkuil
0 siblings, 1 reply; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2018-12-12 20:32 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media
Em Wed, 12 Dec 2018 09:27:17 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> On 12/12/18 8:58 AM, Mauro Carvalho Chehab wrote:
> > Hi Hans,
> >
> > Em Wed, 21 Nov 2018 16:40:21 +0100
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >
> >> The main changes since RFCv3 are:
> >>
> >> - Add entity index to media_v2_pad
> >> - Add source/sink pad index to media_v2_link
> >> - Add owner_idx and owner type flags to media_v2_prop
> >
> > Sorry, but I didn't get why this is needed for properties to work
> > (if the changes are not directly related to properties, please add
> > on separate patches, in order to make easier for review/understanding).
>
> I can separate it.
>
> > The lack of an uAPI documentation at the patchset makes it harder
> > to understand.
> >
> > For the last one, you added a documentation at kAPI:
> >
> >> + * @owner_idx: Index to entities/pads/properties, depending on the owner ID
> >> + * type.
> >
> > But it doesn't really explain anything. Is it the new owner_id
> > field? Is it something else? Why do we need bot owner_id and
> > owner_idx?
>
> Currently when G_TOPOLOGY returns e.g. a link it has two IDs: one
> for the sink object, one for the source object. The application now
> has to traverse the entities array to find the entities referred to
> by the link IDs.
With is O(n).
>
> That's painful, but by providing indices into the entities array as
> well, userspace can just do a direct lookup entities[sink_idx] to
> find the entity with ID sink_id.
This only works when there is an 1:1 map (like the current case of
links). At the moment we have a 1:n association (like the pads that
belong to and entity), application will still need to do a O(n)
lookup (with can easily become O(n^2) if it needs to do for all
entities.
It should be very simple for all lookups to be O(1), Applications (or
the library) just need to parse the properties once, storing the
object ID on a hash table (unordered map, in c++). With that, all
lookups will be O(1), and a lookup for every single object would be
O(n).
Or, if O(log n) is good enough (probably it is), a binary tree
(or a sorted array) would equally work.
> I expect that this will will make it possible in many cases to avoid
> userspace from having to create their own datastructure, but instead
> they can use the returned information directly.
It will still need to traverse the objects, depending on what
userspace wants. Also, in order to optimize object search for the 1:n
case, you would need to return data on certain order, as, otherwise
userspace will still need to traverse the entire pads array.
Basically, you're creating a lot of complexity inside the
Kernel and adding hacks like checking if the index has just 16
bits inside the G_TOPOLOGY return code just to avoid userspace
to create their own index table.
The way I see is that we should have an userspace library that it
would do the index itself. For userspace apps, the data model used
to optimize lookups will be transparent.
One advantage is that, once better algorithms are developed, it
should be easier to switch the library to take advantage of them.
The userspace library may even decide on runtime if it would use
a binary tree or a hash, depending on the number of objects returned
by G_TOPOLOGY.
>
> >
> >>
> >> An updated v4l2-ctl and v4l2-compliance that can report properties
> >> is available here:
> >>
> >> https://git.linuxtv.org/hverkuil/v4l-utils.git/log/?h=props
> >>
> >> Currently I support u64, s64 and const char * property types. And also
> >> a 'group' type that groups sub-properties. But it can be extended to any
> >> type including binary data if needed. No array support (as we have for
> >> controls), but there are enough reserved fields in media_v2_prop
> >> to add this if needed.
> >>
> >> I added properties for entities and pads to vimc, so I could test this.
> >>
> >> Note that the changes to media_device_get_topology() are hard to read
> >> from the patch. It is easier to just look at the source code:
> >>
> >> https://git.linuxtv.org/hverkuil/media_tree.git/tree/drivers/media/media-device.c?h=mc-props
> >>
> >> I have some ideas to improve this some more:
> >>
> >> 1) Add the properties directly to media_gobj. This would simplify some
> >> of the code, but it would require a media_gobj_init function to
> >> initialize the property list. In general I am a bit unhappy about
> >> media_gobj_create: it doesn't really create the graph object, instead
> >> it just adds it to the media_device. It's confusing and it is something
> >> I would like to change.
> >>
> >> 2) The links between pads are stored in media_entity instead of in media_pad.
> >> This is a bit unexpected and makes it harder to traverse the data
> >> structures since to find the links for a pad you need to walk the entity
> >> links and find the links for that pad. Putting all links in the entity
> >> also mixes up pad and interface links, and it would be much cleaner if
> >> those are separated.
> >>
> >> 3) I still think adding support for backlinks to G_TOPOLOGY is a good idea.
> >> Since the current data structure represents a flattened tree that is easy
> >> to navigate the only thing missing for userspace is backlink support.
> >> This is still something that userspace needs to figure out when the kernel
> >> has this readily available. I think that with this in place applications
> >> can just do all the lookups directly on the topology data structure.
> >
> > Apps don't need to follow the exact data struct model as the Kernel,
> > and can dynamically create any indexes they need in order to quickly
> > seek for a link (if search performance would be a problem).
>
> But if the kernel can directly without additional cost provide that
> information to userspace, why on earth shouldn't we do that?
Based on what I saw on your patches, this is not transparent and
has a cost of adding ugly hacks to limit the number of objects.
> > I don't like the idea of reporting all links twice to userspace.
> > Specially after Spectre/Meltdown, context switches are expensive.
>
> You need to call G_TOPOLOGY in any case, so returning backlinks
> in addition to all the other data should not add to the expense. Or
> am I missing something?
You're doubling the size of the link table for no good reason.
If the number of links is high, you'll be doubling the number
of pages to be used between kernelspace-userspace.
>
> >
> > Duplicating data is a very bad idea, as it will enforce an specific
> > data model at the application and at userspace. We want to be able
> > to change the internals (on both sides) if needed for whatever
> > reason.
>
> Whenever you have a link, you also have a backlink. If userspace doesn't
> need that information, they can just not query for it (i.e. set the
> backlinks pointer to 0). But if they do need it, and the kernel has it
> readily available, then why not export this information? Why force
> applications to make their own data structures?
The point is that we're limiting the Kernel to always have backlinks
stored. The current data model we used internally has it, but IMHO
we should get rid of that. I remember I wrote something in the past
on that direction - not sure if I submitted or not. I ended by
giving up merging it because I got out of time to do such cleanup
(and, for the current stuff mapped via MC, this is not really an
issue).
Yet, I remember someone mentioning that the number of links on some
kinds of hardware (Industrial I/O?) is at the order of 10k. If we
ever need to support things like that, we should for sure redesign
the Kernel and get rid of doubling the storage for links.
> > Also, what happens if the duplicated information is not really
> > the same (that could happen due to a bug somewhere)? Should
>
> Well, that's called a bug and it should be fixed.
>
> And if we have support for backlinks, then v4l2-compliance will most
> definitely check for consistency.
>
> > apps validate it? Worse than that, if we report the same link
> > twice (on both directions), userspace will send link changes
> > at the backlinks, making the Kernel code more complex (and
> > bound forever to an specific implementation) for no good reason.
>
> Huh? When introducing an S_TOPOLOGY I would expect that initially
> only the flags in the links array can be changed. The backlinks
> array would not be involved.
>
> Now, having all said this, creating support for backlinks isn't
> that easy without first making data structure changes in the kernel.
>
> So I have no plans to work on backlink support any time soon (if at
> all).
Ok, so let's postpone any discussions with regards to it to the
future, if you decide to work on it.
>
> Regards,
>
> Hans
>
> >
> >>
> >> 1+2 are internal cleanups that can be done later.
> >>
> >> 3 is a low-priority future enhancement. This might become easier to implement
> >> once 1+2 are done.
> >>
> >> This is pretty much the last RFC. If everyone agree with this approach, then
> >> I can make a final patch series, adding documentation etc.
> >>
> >> Regards,
> >>
> >> Hans
> >>
> >>
> >> Hans Verkuil (3):
> >> uapi/linux/media.h: add property support
> >> media controller: add properties support
> >> vimc: add property test code
> >>
> >> drivers/media/media-device.c | 335 +++++++++++++++++-----
> >> drivers/media/media-entity.c | 107 ++++++-
> >> drivers/media/platform/vimc/vimc-common.c | 50 ++++
> >> include/media/media-device.h | 6 +
> >> include/media/media-entity.h | 318 ++++++++++++++++++++
> >> include/uapi/linux/media.h | 88 +++++-
> >> 6 files changed, 819 insertions(+), 85 deletions(-)
> >>
> >
> >
> >
> > Thanks,
> > Mauro
> >
>
Thanks,
Mauro
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFCv4 PATCH 0/3] This RFC patch series implements properties for the media controller.
2018-12-12 20:32 ` Mauro Carvalho Chehab
@ 2018-12-13 9:29 ` Hans Verkuil
0 siblings, 0 replies; 13+ messages in thread
From: Hans Verkuil @ 2018-12-13 9:29 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-media
On 12/12/18 9:32 PM, Mauro Carvalho Chehab wrote:
> Em Wed, 12 Dec 2018 09:27:17 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>
>> On 12/12/18 8:58 AM, Mauro Carvalho Chehab wrote:
>>> Hi Hans,
>>>
>>> Em Wed, 21 Nov 2018 16:40:21 +0100
>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>
>>>> The main changes since RFCv3 are:
>>>>
>>>> - Add entity index to media_v2_pad
>>>> - Add source/sink pad index to media_v2_link
>>>> - Add owner_idx and owner type flags to media_v2_prop
>>>
>>> Sorry, but I didn't get why this is needed for properties to work
>>> (if the changes are not directly related to properties, please add
>>> on separate patches, in order to make easier for review/understanding).
>>
>> I can separate it.
>>
>>> The lack of an uAPI documentation at the patchset makes it harder
>>> to understand.
>>>
>>> For the last one, you added a documentation at kAPI:
>>>
>>>> + * @owner_idx: Index to entities/pads/properties, depending on the owner ID
>>>> + * type.
>>>
>>> But it doesn't really explain anything. Is it the new owner_id
>>> field? Is it something else? Why do we need bot owner_id and
>>> owner_idx?
>>
>> Currently when G_TOPOLOGY returns e.g. a link it has two IDs: one
>> for the sink object, one for the source object. The application now
>> has to traverse the entities array to find the entities referred to
>> by the link IDs.
>
> With is O(n).
>
>>
>> That's painful, but by providing indices into the entities array as
>> well, userspace can just do a direct lookup entities[sink_idx] to
>> find the entity with ID sink_id.
>
> This only works when there is an 1:1 map (like the current case of
> links). At the moment we have a 1:n association (like the pads that
> belong to and entity), application will still need to do a O(n)
> lookup (with can easily become O(n^2) if it needs to do for all
> entities.
Not if you put all the pads belong to an entity together, then the
entity's pad index points to the first pad, and userspace can just
walk over the following pads until either the end of the array or
the pad's entity ID is different from the one you are looking at.
I.e., if you have two entities E1 and E2 and each has two pads,
E1P1, E1P2, E2P1, E2P2, then right now the pads can be in any order,
e.g.:
E1P2 E2P1 E1P1 E2P2
But if you keep pads belonging to one entity together:
E1P1 E1P2 E2P1 E2P2
then each entity can point to the first pad in the pads array and
just walk over the remaining pads.
A pad can also contain an index right back to the entity it belongs
to, so you can directly look up the entity.
The same can be done for links, properties, etc.
All this would make the resulting topology much easier to traverse
without having to build your own data structure. Why make it hard
for userspace when we can just order the data more intelligently?
It doesn't cost the kernel any additional time, it's just a matter
of being smarter.
The current approach is especially painful when you are using just plain
C.
Anyway, I'm working on splitting up the series, separating the property
code from the 'layout' code. That way properties can be added independently
from the layout changes.
Regards,
Hans
>
> It should be very simple for all lookups to be O(1), Applications (or
> the library) just need to parse the properties once, storing the
> object ID on a hash table (unordered map, in c++). With that, all
> lookups will be O(1), and a lookup for every single object would be
> O(n).
>
> Or, if O(log n) is good enough (probably it is), a binary tree
> (or a sorted array) would equally work.
>
>> I expect that this will will make it possible in many cases to avoid
>> userspace from having to create their own datastructure, but instead
>> they can use the returned information directly.
>
> It will still need to traverse the objects, depending on what
> userspace wants. Also, in order to optimize object search for the 1:n
> case, you would need to return data on certain order, as, otherwise
> userspace will still need to traverse the entire pads array.
>
> Basically, you're creating a lot of complexity inside the
> Kernel and adding hacks like checking if the index has just 16
> bits inside the G_TOPOLOGY return code just to avoid userspace
> to create their own index table.
>
> The way I see is that we should have an userspace library that it
> would do the index itself. For userspace apps, the data model used
> to optimize lookups will be transparent.
>
> One advantage is that, once better algorithms are developed, it
> should be easier to switch the library to take advantage of them.
> The userspace library may even decide on runtime if it would use
> a binary tree or a hash, depending on the number of objects returned
> by G_TOPOLOGY.
>
>>
>>>
>>>>
>>>> An updated v4l2-ctl and v4l2-compliance that can report properties
>>>> is available here:
>>>>
>>>> https://git.linuxtv.org/hverkuil/v4l-utils.git/log/?h=props
>>>>
>>>> Currently I support u64, s64 and const char * property types. And also
>>>> a 'group' type that groups sub-properties. But it can be extended to any
>>>> type including binary data if needed. No array support (as we have for
>>>> controls), but there are enough reserved fields in media_v2_prop
>>>> to add this if needed.
>>>>
>>>> I added properties for entities and pads to vimc, so I could test this.
>>>>
>>>> Note that the changes to media_device_get_topology() are hard to read
>>>> from the patch. It is easier to just look at the source code:
>>>>
>>>> https://git.linuxtv.org/hverkuil/media_tree.git/tree/drivers/media/media-device.c?h=mc-props
>>>>
>>>> I have some ideas to improve this some more:
>>>>
>>>> 1) Add the properties directly to media_gobj. This would simplify some
>>>> of the code, but it would require a media_gobj_init function to
>>>> initialize the property list. In general I am a bit unhappy about
>>>> media_gobj_create: it doesn't really create the graph object, instead
>>>> it just adds it to the media_device. It's confusing and it is something
>>>> I would like to change.
>>>>
>>>> 2) The links between pads are stored in media_entity instead of in media_pad.
>>>> This is a bit unexpected and makes it harder to traverse the data
>>>> structures since to find the links for a pad you need to walk the entity
>>>> links and find the links for that pad. Putting all links in the entity
>>>> also mixes up pad and interface links, and it would be much cleaner if
>>>> those are separated.
>>>>
>>>> 3) I still think adding support for backlinks to G_TOPOLOGY is a good idea.
>>>> Since the current data structure represents a flattened tree that is easy
>>>> to navigate the only thing missing for userspace is backlink support.
>>>> This is still something that userspace needs to figure out when the kernel
>>>> has this readily available. I think that with this in place applications
>>>> can just do all the lookups directly on the topology data structure.
>>>
>>> Apps don't need to follow the exact data struct model as the Kernel,
>>> and can dynamically create any indexes they need in order to quickly
>>> seek for a link (if search performance would be a problem).
>>
>> But if the kernel can directly without additional cost provide that
>> information to userspace, why on earth shouldn't we do that?
>
> Based on what I saw on your patches, this is not transparent and
> has a cost of adding ugly hacks to limit the number of objects.
>
>>> I don't like the idea of reporting all links twice to userspace.
>>> Specially after Spectre/Meltdown, context switches are expensive.
>>
>> You need to call G_TOPOLOGY in any case, so returning backlinks
>> in addition to all the other data should not add to the expense. Or
>> am I missing something?
>
> You're doubling the size of the link table for no good reason.
> If the number of links is high, you'll be doubling the number
> of pages to be used between kernelspace-userspace.
>
>>
>>>
>>> Duplicating data is a very bad idea, as it will enforce an specific
>>> data model at the application and at userspace. We want to be able
>>> to change the internals (on both sides) if needed for whatever
>>> reason.
>>
>> Whenever you have a link, you also have a backlink. If userspace doesn't
>> need that information, they can just not query for it (i.e. set the
>> backlinks pointer to 0). But if they do need it, and the kernel has it
>> readily available, then why not export this information? Why force
>> applications to make their own data structures?
>
> The point is that we're limiting the Kernel to always have backlinks
> stored. The current data model we used internally has it, but IMHO
> we should get rid of that. I remember I wrote something in the past
> on that direction - not sure if I submitted or not. I ended by
> giving up merging it because I got out of time to do such cleanup
> (and, for the current stuff mapped via MC, this is not really an
> issue).
>
> Yet, I remember someone mentioning that the number of links on some
> kinds of hardware (Industrial I/O?) is at the order of 10k. If we
> ever need to support things like that, we should for sure redesign
> the Kernel and get rid of doubling the storage for links.
>
>>> Also, what happens if the duplicated information is not really
>>> the same (that could happen due to a bug somewhere)? Should
>>
>> Well, that's called a bug and it should be fixed.
>>
>> And if we have support for backlinks, then v4l2-compliance will most
>> definitely check for consistency.
>>
>>> apps validate it? Worse than that, if we report the same link
>>> twice (on both directions), userspace will send link changes
>>> at the backlinks, making the Kernel code more complex (and
>>> bound forever to an specific implementation) for no good reason.
>>
>> Huh? When introducing an S_TOPOLOGY I would expect that initially
>> only the flags in the links array can be changed. The backlinks
>> array would not be involved.
>>
>> Now, having all said this, creating support for backlinks isn't
>> that easy without first making data structure changes in the kernel.
>>
>> So I have no plans to work on backlink support any time soon (if at
>> all).
>
> Ok, so let's postpone any discussions with regards to it to the
> future, if you decide to work on it.
>
>>
>> Regards,
>>
>> Hans
>>
>>>
>>>>
>>>> 1+2 are internal cleanups that can be done later.
>>>>
>>>> 3 is a low-priority future enhancement. This might become easier to implement
>>>> once 1+2 are done.
>>>>
>>>> This is pretty much the last RFC. If everyone agree with this approach, then
>>>> I can make a final patch series, adding documentation etc.
>>>>
>>>> Regards,
>>>>
>>>> Hans
>>>>
>>>>
>>>> Hans Verkuil (3):
>>>> uapi/linux/media.h: add property support
>>>> media controller: add properties support
>>>> vimc: add property test code
>>>>
>>>> drivers/media/media-device.c | 335 +++++++++++++++++-----
>>>> drivers/media/media-entity.c | 107 ++++++-
>>>> drivers/media/platform/vimc/vimc-common.c | 50 ++++
>>>> include/media/media-device.h | 6 +
>>>> include/media/media-entity.h | 318 ++++++++++++++++++++
>>>> include/uapi/linux/media.h | 88 +++++-
>>>> 6 files changed, 819 insertions(+), 85 deletions(-)
>>>>
>>>
>>>
>>>
>>> Thanks,
>>> Mauro
>>>
>>
>
>
>
> Thanks,
> Mauro
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFCv4 PATCH 1/3] uapi/linux/media.h: add property support
2018-12-12 8:43 ` Hans Verkuil
@ 2018-12-13 12:51 ` Hans Verkuil
0 siblings, 0 replies; 13+ messages in thread
From: Hans Verkuil @ 2018-12-13 12:51 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-media, Hans Verkuil
On 12/12/18 9:43 AM, Hans Verkuil wrote:
> On 12/12/18 9:18 AM, Mauro Carvalho Chehab wrote:
>>> /* ioctls */
>>> @@ -368,6 +446,8 @@ struct media_v2_topology {
>>> #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)
>>> +/* Old MEDIA_IOC_G_TOPOLOGY ioctl without props support */
>>> +#define MEDIA_IOC_G_TOPOLOGY_OLD 0xc0487c04
>>
>> I would avoid calling it "_OLD". we may have a V3, V4, V5, ... of this
>> ioctl.
>>
>> I would, instead, define an _IOWR_COMPAT macro that would take an extra
>> parameter, in order to get the size of part of the struct, e. g. something
>> like:
>>
>> #define MEDIA_IOC_G_TOPOLOGY_V1 _IOWR_COMPAT('|', 0x04, struct media_v2_topology, num_props)
>
> That's not a bad idea, actually.
>
>>
>> Also, I don't see any good reason why to keep this at uAPI (except for a
>> mc-compliance tool that would test both versions - but this could be
>> defined directly there).
>
> Makes sense.
Ah, this means that applications like gstreamer cannot fall back to the older
G_TOPOLOGY ioctl. They don't know which kernel they are running against, so
they will probably want to try the new one first before falling back to the
old version.
I wonder how drm does this. Does anyone know?
Regards,
Hans
>
>>
>>> #define MEDIA_IOC_G_TOPOLOGY _IOWR('|', 0x04, struct media_v2_topology)
>>> #define MEDIA_IOC_REQUEST_ALLOC _IOR ('|', 0x05, int)
>>>
>>
>> Thanks,
>> Mauro
>>
>
> Regards,
>
> Hans
>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-12-13 12:51 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-21 15:40 [RFCv4 PATCH 0/3] This RFC patch series implements properties for the media controller Hans Verkuil
2018-11-21 15:40 ` [RFCv4 PATCH 1/3] uapi/linux/media.h: add property support Hans Verkuil
2018-12-12 8:18 ` Mauro Carvalho Chehab
2018-12-12 8:43 ` Hans Verkuil
2018-12-13 12:51 ` Hans Verkuil
2018-11-21 15:40 ` [RFCv4 PATCH 2/3] media controller: add properties support Hans Verkuil
2018-12-12 9:02 ` Mauro Carvalho Chehab
2018-11-21 15:40 ` [RFCv4 PATCH 3/3] vimc: add property test code Hans Verkuil
2018-12-12 9:04 ` Mauro Carvalho Chehab
2018-12-12 7:58 ` [RFCv4 PATCH 0/3] This RFC patch series implements properties for the media controller Mauro Carvalho Chehab
2018-12-12 8:27 ` Hans Verkuil
2018-12-12 20:32 ` Mauro Carvalho Chehab
2018-12-13 9:29 ` Hans Verkuil
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).