Re: [RFCv5 PATCH 1/4] uapi/linux/media.h: add property support

[Mauro Carvalho Chehab <mchehab+samsung@kernel.org>]

Em Thu, 13 Dec 2018 14:41:13 -0200
Mauro Carvalho Chehab <mchehab+samsung@kernel.org> escreveu:

> Em Thu, 13 Dec 2018 14:41:10 +0100
> hverkuil-cisco@xs4all.nl escreveu:
> 
> > From: Hans Verkuil <hans.verkuil@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.verkuil@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.

Just to be clear: if we don't add it here, we would need to add a
properties ID for every object type (zero meaning that no properties
were associated to it).

> 
> > + * @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



Thanks,
Mauro