* [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
* 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 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 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
* [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
* 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
* [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 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-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 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 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
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).