Em Thu, 13 Dec 2018 18:13:03 +0100
Hans Verkuil <hverkuil-cisco@xxxxxxxxx> escreveu:
> 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.
Already answered on a followup email.
If we remove it from here, we would need to add a property_id to every
object type (including to properties). IMHO, it could be a more future
proof approach.
Yet, as I said, I'm not sure about what would be the best approach.
The thing is: if we add it here, we'll be stick forever to 1:1.
However, I can't think right now on a good use case for N:1 (but
see below: proprieties like "group" are better represented as N:1).
>
> >
> >> + * @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;
Hmm... this is actually wrong. This is for the legacy API.
Yeah, right now, we're not exposing how object_id is built.
>
> 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.
It should be noticed that my mc_nextgen_test.c uses this knowledge:
static uint32_t media_type(uint32_t id)
{
return id >> 24;
}
static inline uint32_t media_localid(uint32_t id)
{
return id & 0xffffff;
}
I suspect that it is sane (and a good idea) to have macros like the
above at the uAPI header, as it makes life simpler for userspace
apps. The only drawback would be if we end by needing to redefine
it for whatever reason.
> 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.
Ok. Inferred that after reviewing patch 3/4.
> 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.
Ok, makes sense.
In this specific case, a N:1 approach for properties make a lot of
sense. I mean, on a device like a car system where a driver can have
a large number of sensors, it would make sense to have all sensors
pointing to a single "sensor" group properties ID.
> >> + * @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'.
It improves reading on some machines. If I'm not mistaken, on archs
like ARM and RISC, the cost of reading an integer is different if
the element is aligned or not. reading an aligned value can be done
with a single instruction. Reading unaligned data would require
reading two values and do some bit shifting logic.
I remember we had this care when applying the final version of the
media API. I would prefer to keep things
Why not?
> >> + 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.
Yep.
>
> >> +} __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.
Ok.
>
> >
> >> 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.
It makes sense to add one :-)
>
> Regards,
>
> Hans
>
> >
> >> } __attribute__ ((packed));
> >>
> >> /* ioctls */
> >
> >
> >
> > Thanks,
> > Mauro
> >
>
Thanks,
Mauro
