On 12/13/18 5:41 PM, Mauro Carvalho Chehab wrote:
> Em Thu, 13 Dec 2018 14:41:10 +0100
> hverkuil-cisco@xxxxxxxxx escreveu:
>
>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>
>> 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.verkuil@xxxxxxxxx>
>> ---
>> 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.
But then every object would somehow have to list all the properties
that belong to it. That doesn't easily fit in e.g. the entities array
that's returned by G_TOPOLOGY.
>
>> + * @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;
I'm fine with that as well, but you expose how the ID is constructed as part of
the uAPI. And you can't later change that.
If nobody has a problem with that, then I can switch to this.
>
>> + * @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?).
Ah, sorry, groups were added later and the comments above were not updated.
A group property has no payload, so these payload fields are 0. A group really
just has a name and an ID, and that ID is referred to as the owner_id by
subproperties.
So you can have an entity with a 'sensor' group property, and that can have
a sub-property called 'orientation'.
These properties will be part of the uAPI, so they will have to be defined
and documented. So in this example you'd have to document the sensor.orientation
property.
>
>> + * @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).
Why should name be 64 bit aligned? Not that I mind moving 'flags' after
'name'.
>
>> + char name[32];
>> + __u32 payload_size;
>> + __u32 payload_offset;
>> + __u32 reserved[18];
If we keep owner_type, then 18 should be changed to 17. I forgot that.
>> +} __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()?
No, groups have no payload.
>
>> 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.
This struct doesn't have any docbook documentation. I can add that once everyone agrees
with this API.
Regards,
Hans
>
>> } __attribute__ ((packed));
>>
>> /* ioctls */
>
>
>
> Thanks,
> Mauro
>
