Em Thu, 13 Dec 2018 14:41:10 +0100
hverkuil-ci...@xs4all.nl escreveu:
> From: Hans Verkuil <hans.verk...@cisco.com>
>
> Extend the topology struct with a properties array.
>
> Add a new media_v2_prop structure to store property information.
>
> Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>
> ---
> include/uapi/linux/media.h | 56 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 56 insertions(+)
>
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index e5d0c5c611b5..12982327381e 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -342,6 +342,58 @@ struct media_v2_link {
> __u32 reserved[6];
> } __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_OWNER_TYPE_ENTITY 0
> +#define MEDIA_OWNER_TYPE_PAD 1
> +#define MEDIA_OWNER_TYPE_LINK 2
> +#define MEDIA_OWNER_TYPE_INTF 3
> +#define MEDIA_OWNER_TYPE_PROP 4
> +
> +/**
> + * struct media_v2_prop - A media property
> + *
> + * @id: The unique non-zero ID of this property
> + * @type: Property type
> + * @owner_id: The ID of the object this property belongs to
I'm in doubt about this. With this field, properties and objects
will have a 1:1 mapping. If this is removed, it would be possible
to map 'n' objects to a single property (N:1 mapping), with could
be interesting.
> + * @owner_type: The type of the object this property belongs to
I would remove this (and the corresponding defines). The type
can easily be identified from the owner_id - as it already contains
the object type embedded at the ID.
In other words, it is:
owner_type = (owner_id & MEDIA_ENT_TYPE_MASK) >> MEDIA_ENT_TYPE_SHIFT;
> + * @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.
Please specify how this will be used for the group type, with is not
obvious. I *suspect* that, on a group, you'll be adding a vector of
u32 (cpu endian) and payload_size is the number of elements at the
vector (or the vector size?).
> + * @reserved: Property reserved field, will be zeroed.
> + */
> +struct media_v2_prop {
> + __u32 id;
> + __u32 type;
> + __u32 owner_id;
> + __u32 owner_type;
> + __u32 flags;
The way it is defined, name won't be 64-bits aligned (well, it will, if
you remove the owner_type).
> + char name[32];
> + __u32 payload_size;
> + __u32 payload_offset;
> + __u32 reserved[18];
> +} __attribute__ ((packed));
> +
> +static inline const char *media_prop2string(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);
> +}
> +
Shouldn't you define also a media_prop2group()?
> struct media_v2_topology {
> __u64 topology_version;
>
> @@ -360,6 +412,10 @@ struct media_v2_topology {
> __u32 num_links;
> __u32 reserved4;
> __u64 ptr_links;
> +
> + __u32 num_props;
> + __u32 props_payload_size;
> + __u64 ptr_props;
Please document those new fields.
> } __attribute__ ((packed));
>
> /* ioctls */
Thanks,
Mauro
