[RFCv5 PATCH 0/4] Add properties support to the media controller

[RFCv5 PATCH 0/4] Add properties support to the media controller

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

The main changes since RFCv4 are:

- Dropped all the indexing code that I added to make it easier to
  traverse the topology. I still think that's a good idea, but that
  can be done in a future patch.
- Split the second patch into two: the first part adds the core support,
  the second adds the functions to let drivers add properties.

An updated v4l2-ctl and v4l2-compliance that can report properties
is available here:

https://git.linuxtv.org/hverkuil/v4l-utils.git/log/?h=props2

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.

Regards,

	Hans

Hans Verkuil (4):
  uapi/linux/media.h: add property support
  media controller: add properties support
  media: add functions to add properties to objects
  vimc: add property test code

 drivers/media/media-device.c              | 129 ++++++++-
 drivers/media/media-entity.c              |  90 +++++-
 drivers/media/platform/vimc/vimc-common.c |  50 ++++
 include/media/media-device.h              |   6 +
 include/media/media-entity.h              | 322 ++++++++++++++++++++++
 include/uapi/linux/media.h                |  56 ++++
 6 files changed, 639 insertions(+), 14 deletions(-)

-- 
2.19.2

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

[hverkuil-cisco]

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
+ * @owner_type:	The type of the object this property belongs to
+ * @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.
+ * @reserved:	Property reserved field, will be zeroed.
+ */
+struct media_v2_prop {
+	__u32 id;
+	__u32 type;
+	__u32 owner_id;
+	__u32 owner_type;
+	__u32 flags;
+	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);
+}
+
 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;
 } __attribute__ ((packed));
 
 /* ioctls */
-- 
2.19.2

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

[Mauro Carvalho Chehab]

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.

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

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

[Hans Verkuil]

On 12/13/18 5:41 PM, Mauro Carvalho Chehab wrote:
> 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.

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
> 

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

[Mauro Carvalho Chehab]

Em Thu, 13 Dec 2018 18:13:03 +0100
Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:

> On 12/13/18 5:41 PM, Mauro Carvalho Chehab wrote:
> > 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.  
> 
> 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

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

[Mauro Carvalho Chehab]

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

[RFCv5 PATCH 2/4] media controller: add properties support

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Add support for properties. In this initial implementation properties
can be added to entities and pads. In addition, properties can be
nested.

Most of the changes are straightforward, but I had to make some changes
to the way entities are initialized, since the struct has to be
initialized either when media_entity_pads_init() is called or when
media_device_register_entity() is called, whichever comes first.

The properties list in the entity has to be initialized in both cases,
otherwise you can't add properties to it.

The entity struct is now initialized in media_entity_init and called
from both functions if ent->inited is 0.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/media-device.c | 129 ++++++++++++++++++++++++++++++++---
 drivers/media/media-entity.c |  26 +++++--
 include/media/media-device.h |   6 ++
 include/media/media-entity.h |  58 ++++++++++++++++
 4 files changed, 205 insertions(+), 14 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index bed24372e61f..a932e6848d47 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -242,10 +242,14 @@ 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;
 	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;
+	unsigned int props_payload_size = 0;
+	unsigned int prop_payload_offset;
 	unsigned int i;
 	int ret = 0;
 
@@ -375,6 +379,48 @@ static long media_device_get_topology(struct media_device *mdev, void *arg)
 	topo->num_links = i;
 	topo->reserved4 = 0;
 
+	/* Get properties and number of properties */
+	i = 0;
+	uprop = media_get_uptr(topo->ptr_props);
+	media_device_for_each_prop(prop, mdev) {
+		i++;
+		props_payload_size += ALIGN(prop->payload_size, sizeof(u64));
+	}
+	if (i > topo->num_props ||
+	    props_payload_size > topo->props_payload_size)
+		ret = ret ? : -ENOSPC;
+	topo->props_payload_size = props_payload_size;
+	topo->num_props = i;
+	topo->reserved4 = 0;
+
+	if (ret || !uprop)
+		return ret;
+
+	prop_payload_offset = topo->num_props * sizeof(*uprop);
+	media_device_for_each_prop(prop, mdev) {
+		memset(&kprop, 0, sizeof(kprop));
+
+		/* Copy prop fields to userspace struct */
+		kprop.id = prop->graph_obj.id;
+		kprop.type = prop->type;
+		kprop.owner_id = prop->owner->id;
+		kprop.owner_type = media_type(prop->owner);
+		kprop.payload_size = prop->payload_size;
+		if (prop->payload_size) {
+			kprop.payload_offset = prop_payload_offset;
+			if (copy_to_user((u8 __user *)uprop + prop_payload_offset,
+					 prop->payload, prop->payload_size))
+				return -EFAULT;
+			prop_payload_offset += ALIGN(prop->payload_size, sizeof(u64));
+		}
+		memcpy(kprop.name, prop->name, sizeof(kprop.name));
+
+		if (copy_to_user(uprop, &kprop, sizeof(kprop)))
+			return -EFAULT;
+		uprop++;
+		prop_payload_offset -= sizeof(*uprop);
+	}
+
 	return ret;
 }
 
@@ -408,9 +454,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 +465,39 @@ 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);
 };
 
+#define _IOWR_COMPAT(type, nr, size) \
+	_IOC(_IOC_READ | _IOC_WRITE, (type), (nr), (size))
+
+/* Old MEDIA_IOC_G_TOPOLOGY ioctl without props support */
+#define MEDIA_IOC_G_TOPOLOGY_V1 _IOWR_COMPAT('|', 0x04, offsetof(struct media_v2_topology, num_props))
+
+static const unsigned int topo_alts[] = {
+	/* Old MEDIA_IOC_G_TOPOLOGY without props support */
+	MEDIA_IOC_G_TOPOLOGY_V1,
+	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 +511,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 +646,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 +677,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 +692,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 +731,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 +764,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 +809,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..62c4d5b4d33f 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);
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..5d05ebf712d0 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
@@ -193,6 +196,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 +204,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 +297,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:	Non-zero if 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 +305,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 +333,7 @@ struct media_entity {
 	enum media_entity_type obj_type;
 	u32 function;
 	unsigned long flags;
+	unsigned int inited;
 
 	u16 num_pads;
 	u16 num_links;
@@ -308,6 +342,7 @@ struct media_entity {
 
 	struct media_pad *pads;
 	struct list_head links;
+	struct list_head props;
 
 	const struct media_entity_operations *ops;
 
@@ -362,6 +397,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 +644,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.
-- 
2.19.2

Re: [RFCv5 PATCH 2/4] media controller: add properties support

[Mauro Carvalho Chehab]

Em Thu, 13 Dec 2018 14:41:11 +0100
hverkuil-cisco@xs4all.nl escreveu:

> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> Add support for properties. In this initial implementation properties
> can be added to entities and pads. In addition, properties can be
> nested.
> 
> Most of the changes are straightforward, but I had to make some changes
> to the way entities are initialized, since the struct has to be
> initialized either when media_entity_pads_init() is called or when
> media_device_register_entity() is called, whichever comes first.
> 
> The properties list in the entity has to be initialized in both cases,
> otherwise you can't add properties to it.
> 
> The entity struct is now initialized in media_entity_init and called
> from both functions if ent->inited is 0.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/media-device.c | 129 ++++++++++++++++++++++++++++++++---
>  drivers/media/media-entity.c |  26 +++++--
>  include/media/media-device.h |   6 ++
>  include/media/media-entity.h |  58 ++++++++++++++++
>  4 files changed, 205 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index bed24372e61f..a932e6848d47 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -242,10 +242,14 @@ 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;
>  	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;
> +	unsigned int props_payload_size = 0;
> +	unsigned int prop_payload_offset;
>  	unsigned int i;
>  	int ret = 0;
>  
> @@ -375,6 +379,48 @@ static long media_device_get_topology(struct media_device *mdev, void *arg)
>  	topo->num_links = i;
>  	topo->reserved4 = 0;
>  
> +	/* Get properties and number of properties */
> +	i = 0;
> +	uprop = media_get_uptr(topo->ptr_props);
> +	media_device_for_each_prop(prop, mdev) {
> +		i++;
> +		props_payload_size += ALIGN(prop->payload_size, sizeof(u64));

I wouldn't be aligning it to u64. Instead, I would use something like:

	ALIGN(prop->payload_size, sizeof(void *))

This way, it would waste less space on 32-bits CPU.

> +	}
> +	if (i > topo->num_props ||
> +	    props_payload_size > topo->props_payload_size)
> +		ret = ret ? : -ENOSPC;
> +	topo->props_payload_size = props_payload_size;
> +	topo->num_props = i;
> +	topo->reserved4 = 0;
> +
> +	if (ret || !uprop)
> +		return ret;
> +
> +	prop_payload_offset = topo->num_props * sizeof(*uprop);
> +	media_device_for_each_prop(prop, mdev) {
> +		memset(&kprop, 0, sizeof(kprop));
> +
> +		/* Copy prop fields to userspace struct */
> +		kprop.id = prop->graph_obj.id;
> +		kprop.type = prop->type;
> +		kprop.owner_id = prop->owner->id;
> +		kprop.owner_type = media_type(prop->owner);
> +		kprop.payload_size = prop->payload_size;
> +		if (prop->payload_size) {
> +			kprop.payload_offset = prop_payload_offset;
> +			if (copy_to_user((u8 __user *)uprop + prop_payload_offset,
> +					 prop->payload, prop->payload_size))
> +				return -EFAULT;
> +			prop_payload_offset += ALIGN(prop->payload_size, sizeof(u64));
> +		}
> +		memcpy(kprop.name, prop->name, sizeof(kprop.name));
> +
> +		if (copy_to_user(uprop, &kprop, sizeof(kprop)))
> +			return -EFAULT;
> +		uprop++;
> +		prop_payload_offset -= sizeof(*uprop);
> +	}
> +
>  	return ret;
>  }
>  
> @@ -408,9 +454,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 +465,39 @@ 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);
>  };
>  
> +#define _IOWR_COMPAT(type, nr, size) \
> +	_IOC(_IOC_READ | _IOC_WRITE, (type), (nr), (size))
> +
> +/* Old MEDIA_IOC_G_TOPOLOGY ioctl without props support */
> +#define MEDIA_IOC_G_TOPOLOGY_V1 _IOWR_COMPAT('|', 0x04, offsetof(struct media_v2_topology, num_props))
> +
> +static const unsigned int topo_alts[] = {
> +	/* Old MEDIA_IOC_G_TOPOLOGY without props support */
> +	MEDIA_IOC_G_TOPOLOGY_V1,
> +	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 +511,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 +646,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 +677,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);

It seems that you missed my comments on this hunk (or maybe I missed your
answer to my review of patch 2/3 of RFCv4[1]).

[1] https://lore.kernel.org/linux-media/20181212070225.2863a72e@coco.lan/

I'll repeat it here:

For this:

	>  	/* 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;

That should be on a separate patch, as it is unrelated to the
properties API addition.

>  	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);

Nitpick: I would add a comment for this explaining why the check is
required here.

>  
>  	ret = ida_alloc_min(&mdev->entity_internal_idx, 1, GFP_KERNEL);
>  	if (ret < 0)
> @@ -608,10 +692,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 +731,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 +764,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 +809,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..62c4d5b4d33f 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;
>  	}

I also suggested to move the above hunks to a separate patch, as this change
is not really related to the addition of the properties.

> +	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);
> 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..5d05ebf712d0 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
> @@ -193,6 +196,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 +204,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;

OK.

Despite my comment at uAPI about the N:1 case of removing the owner
there, here, I would keep it.

For the first version of properties API, we should stick with 1:1 map.

We can later expand for N:1 if needed - and if we don't export owner_id
at uAPI inside the properties structure.

> +	u32 type;
> +	u32 payload_size;
> +	struct list_head props;
> +	char name[32];
> +	u8 payload[];
>  };
>  
>  /**
> @@ -266,6 +297,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:	Non-zero if 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 +305,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 +333,7 @@ struct media_entity {
>  	enum media_entity_type obj_type;
>  	u32 function;
>  	unsigned long flags;
> +	unsigned int inited;
>  
>  	u16 num_pads;
>  	u16 num_links;
> @@ -308,6 +342,7 @@ struct media_entity {
>  
>  	struct media_pad *pads;
>  	struct list_head links;
> +	struct list_head props;
>  
>  	const struct media_entity_operations *ops;
>  
> @@ -362,6 +397,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;
> +}
> +

As I said before, I would keep it together with the C file, as it makes
easier to read (except, of course, if are there any reason why you
would need to call it on a different C file).

>  /**
>   * media_entity_id() - return the media entity graph object id
>   *
> @@ -595,6 +644,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.



Thanks,
Mauro

Re: [RFCv5 PATCH 2/4] media controller: add properties support

[Hans Verkuil]

On 12/13/18 6:14 PM, Mauro Carvalho Chehab wrote:
> Em Thu, 13 Dec 2018 14:41:11 +0100
> hverkuil-cisco@xs4all.nl escreveu:
> 
>> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>
>> Add support for properties. In this initial implementation properties
>> can be added to entities and pads. In addition, properties can be
>> nested.
>>
>> Most of the changes are straightforward, but I had to make some changes
>> to the way entities are initialized, since the struct has to be
>> initialized either when media_entity_pads_init() is called or when
>> media_device_register_entity() is called, whichever comes first.
>>
>> The properties list in the entity has to be initialized in both cases,
>> otherwise you can't add properties to it.
>>
>> The entity struct is now initialized in media_entity_init and called
>> from both functions if ent->inited is 0.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  drivers/media/media-device.c | 129 ++++++++++++++++++++++++++++++++---
>>  drivers/media/media-entity.c |  26 +++++--
>>  include/media/media-device.h |   6 ++
>>  include/media/media-entity.h |  58 ++++++++++++++++
>>  4 files changed, 205 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>> index bed24372e61f..a932e6848d47 100644
>> --- a/drivers/media/media-device.c
>> +++ b/drivers/media/media-device.c
>> @@ -242,10 +242,14 @@ 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;
>>  	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;
>> +	unsigned int props_payload_size = 0;
>> +	unsigned int prop_payload_offset;
>>  	unsigned int i;
>>  	int ret = 0;
>>  
>> @@ -375,6 +379,48 @@ static long media_device_get_topology(struct media_device *mdev, void *arg)
>>  	topo->num_links = i;
>>  	topo->reserved4 = 0;
>>  
>> +	/* Get properties and number of properties */
>> +	i = 0;
>> +	uprop = media_get_uptr(topo->ptr_props);
>> +	media_device_for_each_prop(prop, mdev) {
>> +		i++;
>> +		props_payload_size += ALIGN(prop->payload_size, sizeof(u64));
> 
> I wouldn't be aligning it to u64. Instead, I would use something like:
> 
> 	ALIGN(prop->payload_size, sizeof(void *))
> 
> This way, it would waste less space on 32-bits CPU.

OK.

> 
>> +	}
>> +	if (i > topo->num_props ||
>> +	    props_payload_size > topo->props_payload_size)
>> +		ret = ret ? : -ENOSPC;
>> +	topo->props_payload_size = props_payload_size;
>> +	topo->num_props = i;
>> +	topo->reserved4 = 0;
>> +
>> +	if (ret || !uprop)
>> +		return ret;
>> +
>> +	prop_payload_offset = topo->num_props * sizeof(*uprop);
>> +	media_device_for_each_prop(prop, mdev) {
>> +		memset(&kprop, 0, sizeof(kprop));
>> +
>> +		/* Copy prop fields to userspace struct */
>> +		kprop.id = prop->graph_obj.id;
>> +		kprop.type = prop->type;
>> +		kprop.owner_id = prop->owner->id;
>> +		kprop.owner_type = media_type(prop->owner);
>> +		kprop.payload_size = prop->payload_size;
>> +		if (prop->payload_size) {
>> +			kprop.payload_offset = prop_payload_offset;
>> +			if (copy_to_user((u8 __user *)uprop + prop_payload_offset,
>> +					 prop->payload, prop->payload_size))
>> +				return -EFAULT;
>> +			prop_payload_offset += ALIGN(prop->payload_size, sizeof(u64));
>> +		}
>> +		memcpy(kprop.name, prop->name, sizeof(kprop.name));
>> +
>> +		if (copy_to_user(uprop, &kprop, sizeof(kprop)))
>> +			return -EFAULT;
>> +		uprop++;
>> +		prop_payload_offset -= sizeof(*uprop);
>> +	}
>> +
>>  	return ret;
>>  }
>>  
>> @@ -408,9 +454,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 +465,39 @@ 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);
>>  };
>>  
>> +#define _IOWR_COMPAT(type, nr, size) \
>> +	_IOC(_IOC_READ | _IOC_WRITE, (type), (nr), (size))
>> +
>> +/* Old MEDIA_IOC_G_TOPOLOGY ioctl without props support */
>> +#define MEDIA_IOC_G_TOPOLOGY_V1 _IOWR_COMPAT('|', 0x04, offsetof(struct media_v2_topology, num_props))
>> +
>> +static const unsigned int topo_alts[] = {
>> +	/* Old MEDIA_IOC_G_TOPOLOGY without props support */
>> +	MEDIA_IOC_G_TOPOLOGY_V1,
>> +	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 +511,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 +646,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 +677,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);
> 
> It seems that you missed my comments on this hunk (or maybe I missed your
> answer to my review of patch 2/3 of RFCv4[1]).

Ah yes, I forgot that. I was a bit in a hurry to prepare a v5 since this is the last one
before my vacation.

> 
> [1] https://lore.kernel.org/linux-media/20181212070225.2863a72e@coco.lan/
> 
> I'll repeat it here:
> 
> For this:
> 
> 	>  	/* 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;
> 
> That should be on a separate patch, as it is unrelated to the
> properties API addition.
> 
>>  	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);
> 
> Nitpick: I would add a comment for this explaining why the check is
> required here.

Good point.

> 
>>  
>>  	ret = ida_alloc_min(&mdev->entity_internal_idx, 1, GFP_KERNEL);
>>  	if (ret < 0)
>> @@ -608,10 +692,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 +731,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 +764,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 +809,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..62c4d5b4d33f 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;
>>  	}
> 
> I also suggested to move the above hunks to a separate patch, as this change
> is not really related to the addition of the properties.

Missed that too, sorry.

> 
>> +	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);
>> 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..5d05ebf712d0 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
>> @@ -193,6 +196,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 +204,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;
> 
> OK.
> 
> Despite my comment at uAPI about the N:1 case of removing the owner
> there, here, I would keep it.
> 
> For the first version of properties API, we should stick with 1:1 map.
> 
> We can later expand for N:1 if needed - and if we don't export owner_id
> at uAPI inside the properties structure.

I don't understand this remark. The owner_id of a property needs to be exported,
how else would the application know to which object the property belongs?

> 
>> +	u32 type;
>> +	u32 payload_size;
>> +	struct list_head props;
>> +	char name[32];
>> +	u8 payload[];
>>  };
>>  
>>  /**
>> @@ -266,6 +297,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:	Non-zero if 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 +305,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 +333,7 @@ struct media_entity {
>>  	enum media_entity_type obj_type;
>>  	u32 function;
>>  	unsigned long flags;
>> +	unsigned int inited;
>>  
>>  	u16 num_pads;
>>  	u16 num_links;
>> @@ -308,6 +342,7 @@ struct media_entity {
>>  
>>  	struct media_pad *pads;
>>  	struct list_head links;
>> +	struct list_head props;
>>  
>>  	const struct media_entity_operations *ops;
>>  
>> @@ -362,6 +397,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;
>> +}
>> +
> 
> As I said before, I would keep it together with the C file, as it makes
> easier to read (except, of course, if are there any reason why you
> would need to call it on a different C file).

It needs to be called from both media-entity.c and media-device.c.

But it might be better to just put it in media-entity.c as a regular function.

> 
>>  /**
>>   * media_entity_id() - return the media entity graph object id
>>   *
>> @@ -595,6 +644,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.
> 
> 
> 
> Thanks,
> Mauro
> 

Regards,

	Hans

Re: [RFCv5 PATCH 2/4] media controller: add properties support

[Mauro Carvalho Chehab]

Em Thu, 13 Dec 2018 18:35:15 +0100
Hans Verkuil <hverkuil-cisco@xs4all.nl> escreveu:

> On 12/13/18 6:14 PM, Mauro Carvalho Chehab wrote:
> > Em Thu, 13 Dec 2018 14:41:11 +0100
> > hverkuil-cisco@xs4all.nl escreveu:
> >   
> >> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >>
> >> Add support for properties. In this initial implementation properties
> >> can be added to entities and pads. In addition, properties can be
> >> nested.
> >>
> >> Most of the changes are straightforward, but I had to make some changes
> >> to the way entities are initialized, since the struct has to be
> >> initialized either when media_entity_pads_init() is called or when
> >> media_device_register_entity() is called, whichever comes first.
> >>
> >> The properties list in the entity has to be initialized in both cases,
> >> otherwise you can't add properties to it.
> >>
> >> The entity struct is now initialized in media_entity_init and called
> >> from both functions if ent->inited is 0.
> >>
> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >> ---
> >>  drivers/media/media-device.c | 129 ++++++++++++++++++++++++++++++++---
> >>  drivers/media/media-entity.c |  26 +++++--
> >>  include/media/media-device.h |   6 ++
> >>  include/media/media-entity.h |  58 ++++++++++++++++
> >>  4 files changed, 205 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >> index bed24372e61f..a932e6848d47 100644
> >> --- a/drivers/media/media-device.c
> >> +++ b/drivers/media/media-device.c
> >> @@ -242,10 +242,14 @@ 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;
> >>  	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;
> >> +	unsigned int props_payload_size = 0;
> >> +	unsigned int prop_payload_offset;
> >>  	unsigned int i;
> >>  	int ret = 0;
> >>  
> >> @@ -375,6 +379,48 @@ static long media_device_get_topology(struct media_device *mdev, void *arg)
> >>  	topo->num_links = i;
> >>  	topo->reserved4 = 0;
> >>  
> >> +	/* Get properties and number of properties */
> >> +	i = 0;
> >> +	uprop = media_get_uptr(topo->ptr_props);
> >> +	media_device_for_each_prop(prop, mdev) {
> >> +		i++;
> >> +		props_payload_size += ALIGN(prop->payload_size, sizeof(u64));  
> > 
> > I wouldn't be aligning it to u64. Instead, I would use something like:
> > 
> > 	ALIGN(prop->payload_size, sizeof(void *))
> > 
> > This way, it would waste less space on 32-bits CPU.  
> 
> OK.
> 
> >   
> >> +	}
> >> +	if (i > topo->num_props ||
> >> +	    props_payload_size > topo->props_payload_size)
> >> +		ret = ret ? : -ENOSPC;
> >> +	topo->props_payload_size = props_payload_size;
> >> +	topo->num_props = i;
> >> +	topo->reserved4 = 0;
> >> +
> >> +	if (ret || !uprop)
> >> +		return ret;
> >> +
> >> +	prop_payload_offset = topo->num_props * sizeof(*uprop);
> >> +	media_device_for_each_prop(prop, mdev) {
> >> +		memset(&kprop, 0, sizeof(kprop));
> >> +
> >> +		/* Copy prop fields to userspace struct */
> >> +		kprop.id = prop->graph_obj.id;
> >> +		kprop.type = prop->type;
> >> +		kprop.owner_id = prop->owner->id;
> >> +		kprop.owner_type = media_type(prop->owner);
> >> +		kprop.payload_size = prop->payload_size;
> >> +		if (prop->payload_size) {
> >> +			kprop.payload_offset = prop_payload_offset;
> >> +			if (copy_to_user((u8 __user *)uprop + prop_payload_offset,
> >> +					 prop->payload, prop->payload_size))
> >> +				return -EFAULT;
> >> +			prop_payload_offset += ALIGN(prop->payload_size, sizeof(u64));
> >> +		}
> >> +		memcpy(kprop.name, prop->name, sizeof(kprop.name));
> >> +
> >> +		if (copy_to_user(uprop, &kprop, sizeof(kprop)))
> >> +			return -EFAULT;
> >> +		uprop++;
> >> +		prop_payload_offset -= sizeof(*uprop);
> >> +	}
> >> +
> >>  	return ret;
> >>  }
> >>  
> >> @@ -408,9 +454,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 +465,39 @@ 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);
> >>  };
> >>  
> >> +#define _IOWR_COMPAT(type, nr, size) \
> >> +	_IOC(_IOC_READ | _IOC_WRITE, (type), (nr), (size))
> >> +
> >> +/* Old MEDIA_IOC_G_TOPOLOGY ioctl without props support */
> >> +#define MEDIA_IOC_G_TOPOLOGY_V1 _IOWR_COMPAT('|', 0x04, offsetof(struct media_v2_topology, num_props))
> >> +
> >> +static const unsigned int topo_alts[] = {
> >> +	/* Old MEDIA_IOC_G_TOPOLOGY without props support */
> >> +	MEDIA_IOC_G_TOPOLOGY_V1,
> >> +	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 +511,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 +646,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 +677,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);  
> > 
> > It seems that you missed my comments on this hunk (or maybe I missed your
> > answer to my review of patch 2/3 of RFCv4[1]).  
> 
> Ah yes, I forgot that. I was a bit in a hurry to prepare a v5 since this is the last one
> before my vacation.
> 
> > 
> > [1] https://lore.kernel.org/linux-media/20181212070225.2863a72e@coco.lan/
> > 
> > I'll repeat it here:
> > 
> > For this:
> >   
> > 	>  	/* 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;
> > 
> > That should be on a separate patch, as it is unrelated to the
> > properties API addition.
> >   
> >>  	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);  
> > 
> > Nitpick: I would add a comment for this explaining why the check is
> > required here.  
> 
> Good point.
> 
> >   
> >>  
> >>  	ret = ida_alloc_min(&mdev->entity_internal_idx, 1, GFP_KERNEL);
> >>  	if (ret < 0)
> >> @@ -608,10 +692,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 +731,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 +764,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 +809,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..62c4d5b4d33f 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;
> >>  	}  
> > 
> > I also suggested to move the above hunks to a separate patch, as this change
> > is not really related to the addition of the properties.  
> 
> Missed that too, sorry.
> 
> >   
> >> +	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);
> >> 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..5d05ebf712d0 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
> >> @@ -193,6 +196,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 +204,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;  
> > 
> > OK.
> > 
> > Despite my comment at uAPI about the N:1 case of removing the owner
> > there, here, I would keep it.
> > 
> > For the first version of properties API, we should stick with 1:1 map.
> > 
> > We can later expand for N:1 if needed - and if we don't export owner_id
> > at uAPI inside the properties structure.  
> 
> I don't understand this remark. The owner_id of a property needs to be exported,
> how else would the application know to which object the property belongs?

I mean: there are two ways of exporting it:

1) entities, pads, links, interfaces, properties, ... would have a
properties_id field (where 0 would mean no properties associated).

That would allow a single property to be associated to multiple
elements (N:1). That would work really fine for "group" properties.

2) properties will have a owner_id. Mapping will be 1:1.
 
> >> +	u32 type;
> >> +	u32 payload_size;
> >> +	struct list_head props;
> >> +	char name[32];
> >> +	u8 payload[];
> >>  };
> >>  
> >>  /**
> >> @@ -266,6 +297,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:	Non-zero if 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 +305,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 +333,7 @@ struct media_entity {
> >>  	enum media_entity_type obj_type;
> >>  	u32 function;
> >>  	unsigned long flags;
> >> +	unsigned int inited;
> >>  
> >>  	u16 num_pads;
> >>  	u16 num_links;
> >> @@ -308,6 +342,7 @@ struct media_entity {
> >>  
> >>  	struct media_pad *pads;
> >>  	struct list_head links;
> >> +	struct list_head props;
> >>  
> >>  	const struct media_entity_operations *ops;
> >>  
> >> @@ -362,6 +397,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;
> >> +}
> >> +  
> > 
> > As I said before, I would keep it together with the C file, as it makes
> > easier to read (except, of course, if are there any reason why you
> > would need to call it on a different C file).  
> 
> It needs to be called from both media-entity.c and media-device.c.

OK. 

> But it might be better to just put it in media-entity.c as a regular function.

My personal preference would be to declare it as a regular function, 
instead of inlining it every time it is needed (but I can live with
the way it is right now).

> 
> >   
> >>  /**
> >>   * media_entity_id() - return the media entity graph object id
> >>   *
> >> @@ -595,6 +644,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.  
> > 
> > 
> > 
> > Thanks,
> > Mauro
> >   
> 
> Regards,
> 
> 	Hans



Thanks,
Mauro

[RFCv5 PATCH 3/4] media: add functions to add properties to objects

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Add functions to add properties to entities, pads and other
properties. This can be extended to include interfaces and links
in the future when needed.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/media-entity.c |  64 +++++++++
 include/media/media-entity.h | 264 +++++++++++++++++++++++++++++++++++
 2 files changed, 328 insertions(+)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index 62c4d5b4d33f..cdb35bc8e9a0 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -251,6 +251,70 @@ 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
  */
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 5d05ebf712d0..695acfd3fe9c 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -833,6 +833,270 @@ 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.2

Re: [RFCv5 PATCH 3/4] media: add functions to add properties to objects

[Mauro Carvalho Chehab]

Em Thu, 13 Dec 2018 14:41:12 +0100
hverkuil-cisco@xs4all.nl escreveu:

> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> Add functions to add properties to entities, pads and other
> properties. This can be extended to include interfaces and links
> in the future when needed.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/media-entity.c |  64 +++++++++
>  include/media/media-entity.h | 264 +++++++++++++++++++++++++++++++++++
>  2 files changed, 328 insertions(+)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 62c4d5b4d33f..cdb35bc8e9a0 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -251,6 +251,70 @@ 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
>   */
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 5d05ebf712d0..695acfd3fe9c 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -833,6 +833,270 @@ 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);
> +}

Hmm... group is just a string? On my past comments for patches 1/4 and 2/4
I assumed it would be an u32 array :-)

That's btw why I asked you to add some documentation. That would avoid
reviewers of guessing some things ;-)

Why are you using zero for the payload? At patch 1/4, you said that
only u64 and s64 would have payload_size == 0:

	+ * @payload_size: Property payload size, 0 for U64/S64

One would infer that, for all other types, payload_size would be
a non-zero value.

> +
> +/**
> + * 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);

Ok, so, you're assuming that, for strings, payload_type will include
the ending '\0' character, and that strings are NUL-terminated.

Please document it, as we usually have NUL-terminated strings OR
LEN + string. Having both is non-trivial.

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

[RFCv5 PATCH 4/4] vimc: add property test code

[hverkuil-cisco]

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 867e24dbd6b5..e3d5d4b3b44d 100644
--- a/drivers/media/platform/vimc/vimc-common.c
+++ b/drivers/media/platform/vimc/vimc-common.c
@@ -417,6 +417,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 */
@@ -454,6 +455,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.2

Re: [RFCv5 PATCH 4/4] vimc: add property test code

[Mauro Carvalho Chehab]

Em Thu, 13 Dec 2018 14:41:13 +0100
hverkuil-cisco@xs4all.nl escreveu:

> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Add properties to entities and pads to be able to test the
> properties API.

Looks OK to me.

> 
> 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 867e24dbd6b5..e3d5d4b3b44d 100644
> --- a/drivers/media/platform/vimc/vimc-common.c
> +++ b/drivers/media/platform/vimc/vimc-common.c
> @@ -417,6 +417,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 */
> @@ -454,6 +455,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:



Thanks,
Mauro