Re: [RFCv2 PATCH 1/3] uapi/linux/media.h: add property support
On Tue, Sep 04, 2018 at 06:43:20PM +0300, Sakari Ailus wrote: > media_v2_prop, called e.g. payload_length. I also think we should have the > size (and length) of the property in a specific unit, such as bytes, so the > parser does not have to know a given property type to determine it > correctly. Ah, please ignore this. I see you ended up to the same conclusion yourself already. -- Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [RFCv2 PATCH 1/3] uapi/linux/media.h: add property support
Hi Hans,
On Tue, Sep 04, 2018 at 03:50:33PM +0200, Hans Verkuil wrote:
> On 09/04/18 15:01, Sakari Ailus wrote:
> > Hi Hans,
> >
> > Thanks for the set.
> >
> > On Tue, Aug 07, 2018 at 12:28:45PM +0200, Hans Verkuil wrote:
> >> From: Hans Verkuil
> >>
> >> Add a new topology struct that includes properties.
> >>
> >> Signed-off-by: Hans Verkuil
> >> ---
> >> include/uapi/linux/media.h | 40 ++
> >> 1 file changed, 40 insertions(+)
> >>
> >> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> >> index 36f76e777ef9..1910c091601e 100644
> >> --- a/include/uapi/linux/media.h
> >> +++ b/include/uapi/linux/media.h
> >> @@ -342,6 +342,40 @@ struct media_v2_link {
> >>__u32 reserved[6];
> >> } __attribute__ ((packed));
> >>
> >> +#define MEDIA_PROP_TYPE_U64 1
> >> +#define MEDIA_PROP_TYPE_S64 2
> >> +#define MEDIA_PROP_TYPE_STRING3
> >> +
> >> +/**
> >> + * struct media_v2_prop - A media property
> >> + *
> >> + * @id: The unique non-zero ID of this property
> >> + * @owner_id: The ID of the object this property belongs to
> >
> > This assumes everything has a graph object ID. Speaking of "everything",
> > one of the use cases for this could be telling the user whether the media
> > device registration is finished.
> >
> > One approach could be to create a special graph object with a constant ID
> > for that purpose. As the type is a part of the ID field, we could simply
> > create a new type of constant objects.
>
> Why would you use properties for that? Why not just allow polling for
> exceptions (i.e. new events) on the media device node, similar to events
> for V4L2? It's very easy to use with select() et al.
Yes, that's needed; there was actually a support for media events on an
older version of my take of the request API.
But events are not all that's needed: how does the user know registration
has finished if it has already happened when the user opens a device?
Event-wise, I think a "topology change" event is all that's needed.
>
> > On a sidenote, the proposed API effectively prohibits conveying structured
> > data. One use case for that would be to tell about camera modules. We don't
> > have that right now as part of the uAPI as there's nothing the kernel
> > currently knows about camera modules --- because there's nothing to control
> > there. The user space would still be interested in knowing quite a few
> > parameters of these devices which means the concept of the camera module
> > would be good to have in precisely this kind of an interface. What should
> > be there could in practice be what does the module contain (some of the
> > components are sub-devices whereas some would not be, such as an IR filter
> > or the lens) and their parameters (e.g. focal length).
>
> You can easily (but indeed not yet implemented in this initial version) create
> hierarchical properties by simply setting the owner_id to a parent property.
Good point. For arrays we could e.g. add a new field to struct
media_v2_prop, called e.g. payload_length. I also think we should have the
size (and length) of the property in a specific unit, such as bytes, so the
parser does not have to know a given property type to determine it
correctly.
>
> And you obviously need to carefully define and document how data is
> represented in properties.
>
> If people feel strongly enough about this, then I can implement it in the
> next RFC. But I want feedback on this RFC first.
Feel free to go ahead with that but I'm fine with the current scope as
well. Just remember it was one of the use cases. :-)
>
> > I'm not demanding this must be a part of the API in the beginning, however
> > this was one of the reasons why I originally proposed using JSON:
> >
> > https://www.spinics.net/lists/linux-media/msg90160.html>
> >
> > The downside with JSON would be that it does not fit very well with the
> > rest of the MC API.
> >
> >> + * @type: Property type
> >> + * @flags:Property flags
> >> + * @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.
> >> + * @name: Property name
> >> + * @uval: Property value (unsigned)
> >> + * @sval: Property value (signed)
> >> + */
> >> +struct media_v2_prop {
> >> + __u32 id;
> >> + __u32 owner_id;
> >> + __u32 type;
> >> + __u32 flags;
> >> + __u32 payload_size;
> >> + __u32 payload_offset;
> >> + __u32 reserved[18];
> >
> > That's plenty. What is the typical size of the properties array for a media
> > device with, say, a few hundred entities with some four pads each, linked
> > with a single link on average?
> >
> >> + char name[32];
> >
> > Is 32 enough? For controls and formats it's been sometimes a bit limiting.
> >
> > Another approach could be to make this variable size, but that would
Re: [RFCv2 PATCH 1/3] uapi/linux/media.h: add property support
On 09/04/18 15:01, Sakari Ailus wrote:
> Hi Hans,
>
> Thanks for the set.
>
> On Tue, Aug 07, 2018 at 12:28:45PM +0200, Hans Verkuil wrote:
>> From: Hans Verkuil
>>
>> Add a new topology struct that includes properties.
>>
>> Signed-off-by: Hans Verkuil
>> ---
>> include/uapi/linux/media.h | 40 ++
>> 1 file changed, 40 insertions(+)
>>
>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
>> index 36f76e777ef9..1910c091601e 100644
>> --- a/include/uapi/linux/media.h
>> +++ b/include/uapi/linux/media.h
>> @@ -342,6 +342,40 @@ struct media_v2_link {
>> __u32 reserved[6];
>> } __attribute__ ((packed));
>>
>> +#define MEDIA_PROP_TYPE_U64 1
>> +#define MEDIA_PROP_TYPE_S64 2
>> +#define MEDIA_PROP_TYPE_STRING 3
>> +
>> +/**
>> + * struct media_v2_prop - A media property
>> + *
>> + * @id: The unique non-zero ID of this property
>> + * @owner_id: The ID of the object this property belongs to
>
> This assumes everything has a graph object ID. Speaking of "everything",
> one of the use cases for this could be telling the user whether the media
> device registration is finished.
>
> One approach could be to create a special graph object with a constant ID
> for that purpose. As the type is a part of the ID field, we could simply
> create a new type of constant objects.
Why would you use properties for that? Why not just allow polling for
exceptions (i.e. new events) on the media device node, similar to events
for V4L2? It's very easy to use with select() et al.
> On a sidenote, the proposed API effectively prohibits conveying structured
> data. One use case for that would be to tell about camera modules. We don't
> have that right now as part of the uAPI as there's nothing the kernel
> currently knows about camera modules --- because there's nothing to control
> there. The user space would still be interested in knowing quite a few
> parameters of these devices which means the concept of the camera module
> would be good to have in precisely this kind of an interface. What should
> be there could in practice be what does the module contain (some of the
> components are sub-devices whereas some would not be, such as an IR filter
> or the lens) and their parameters (e.g. focal length).
You can easily (but indeed not yet implemented in this initial version) create
hierarchical properties by simply setting the owner_id to a parent property.
And you obviously need to carefully define and document how data is
represented in properties.
If people feel strongly enough about this, then I can implement it in the
next RFC. But I want feedback on this RFC first.
> I'm not demanding this must be a part of the API in the beginning, however
> this was one of the reasons why I originally proposed using JSON:
>
> https://www.spinics.net/lists/linux-media/msg90160.html>
>
> The downside with JSON would be that it does not fit very well with the
> rest of the MC API.
>
>> + * @type: Property type
>> + * @flags: Property flags
>> + * @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.
>> + * @name: Property name
>> + * @uval: Property value (unsigned)
>> + * @sval: Property value (signed)
>> + */
>> +struct media_v2_prop {
>> +__u32 id;
>> +__u32 owner_id;
>> +__u32 type;
>> +__u32 flags;
>> +__u32 payload_size;
>> +__u32 payload_offset;
>> +__u32 reserved[18];
>
> That's plenty. What is the typical size of the properties array for a media
> device with, say, a few hundred entities with some four pads each, linked
> with a single link on average?
>
>> +char name[32];
>
> Is 32 enough? For controls and formats it's been sometimes a bit limiting.
>
> Another approach could be to make this variable size, but that would make
> parsing a little bit more complicated. As you already have separated the
> property descriptor and the payload, that wouldn't be a major change
> anymore.
I'll have to experiment a bit with that.
Thanks for looking at this!
Regards,
Hans
>
>> +union {
>> +__u64 uval;
>> +__s64 sval;
>> +};
>> +} __attribute__ ((packed));
>> +
>> struct media_v2_topology {
>> __u64 topology_version;
>>
>> @@ -360,6 +394,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 */
>> @@ -368,6 +406,8 @@ struct media_v2_topology {
>> #define MEDIA_IOC_ENUM_ENTITIES _IOWR('|', 0x01, struct
>> media_entity_desc)
>> #define MEDIA_IOC_ENUM_LINKS_IOWR('|', 0x02, struct
>> media_links_enum)
>> #define MEDIA_IOC_SETUP_LINK_IOWR('|', 0x03, struct media_link_
Re: [RFCv2 PATCH 1/3] uapi/linux/media.h: add property support
Hi Hans,
Thanks for the set.
On Tue, Aug 07, 2018 at 12:28:45PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil
>
> Add a new topology struct that includes properties.
>
> Signed-off-by: Hans Verkuil
> ---
> include/uapi/linux/media.h | 40 ++
> 1 file changed, 40 insertions(+)
>
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index 36f76e777ef9..1910c091601e 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -342,6 +342,40 @@ struct media_v2_link {
> __u32 reserved[6];
> } __attribute__ ((packed));
>
> +#define MEDIA_PROP_TYPE_U64 1
> +#define MEDIA_PROP_TYPE_S64 2
> +#define MEDIA_PROP_TYPE_STRING 3
> +
> +/**
> + * struct media_v2_prop - A media property
> + *
> + * @id: The unique non-zero ID of this property
> + * @owner_id:The ID of the object this property belongs to
This assumes everything has a graph object ID. Speaking of "everything",
one of the use cases for this could be telling the user whether the media
device registration is finished.
One approach could be to create a special graph object with a constant ID
for that purpose. As the type is a part of the ID field, we could simply
create a new type of constant objects.
On a sidenote, the proposed API effectively prohibits conveying structured
data. One use case for that would be to tell about camera modules. We don't
have that right now as part of the uAPI as there's nothing the kernel
currently knows about camera modules --- because there's nothing to control
there. The user space would still be interested in knowing quite a few
parameters of these devices which means the concept of the camera module
would be good to have in precisely this kind of an interface. What should
be there could in practice be what does the module contain (some of the
components are sub-devices whereas some would not be, such as an IR filter
or the lens) and their parameters (e.g. focal length).
I'm not demanding this must be a part of the API in the beginning, however
this was one of the reasons why I originally proposed using JSON:
https://www.spinics.net/lists/linux-media/msg90160.html>
The downside with JSON would be that it does not fit very well with the
rest of the MC API.
> + * @type:Property type
> + * @flags: Property flags
> + * @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.
> + * @name:Property name
> + * @uval:Property value (unsigned)
> + * @sval:Property value (signed)
> + */
> +struct media_v2_prop {
> + __u32 id;
> + __u32 owner_id;
> + __u32 type;
> + __u32 flags;
> + __u32 payload_size;
> + __u32 payload_offset;
> + __u32 reserved[18];
That's plenty. What is the typical size of the properties array for a media
device with, say, a few hundred entities with some four pads each, linked
with a single link on average?
> + char name[32];
Is 32 enough? For controls and formats it's been sometimes a bit limiting.
Another approach could be to make this variable size, but that would make
parsing a little bit more complicated. As you already have separated the
property descriptor and the payload, that wouldn't be a major change
anymore.
> + union {
> + __u64 uval;
> + __s64 sval;
> + };
> +} __attribute__ ((packed));
> +
> struct media_v2_topology {
> __u64 topology_version;
>
> @@ -360,6 +394,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 */
> @@ -368,6 +406,8 @@ struct media_v2_topology {
> #define MEDIA_IOC_ENUM_ENTITIES _IOWR('|', 0x01, struct
> media_entity_desc)
> #define MEDIA_IOC_ENUM_LINKS _IOWR('|', 0x02, struct media_links_enum)
> #define MEDIA_IOC_SETUP_LINK _IOWR('|', 0x03, struct media_link_desc)
> +/* Old MEDIA_IOC_G_TOPOLOGY ioctl without props support */
> +#define MEDIA_IOC_G_TOPOLOGY_OLD 0xc0487c04
> #define MEDIA_IOC_G_TOPOLOGY _IOWR('|', 0x04, struct media_v2_topology)
>
> #ifndef __KERNEL__
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [RFCv2 PATCH 1/3] uapi/linux/media.h: add property support
On 08/07/2018 12:28 PM, Hans Verkuil wrote:
> From: Hans Verkuil
>
> Add a new topology struct that includes properties.
>
> Signed-off-by: Hans Verkuil
> ---
> include/uapi/linux/media.h | 40 ++
> 1 file changed, 40 insertions(+)
>
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index 36f76e777ef9..1910c091601e 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -342,6 +342,40 @@ struct media_v2_link {
> __u32 reserved[6];
> } __attribute__ ((packed));
>
> +#define MEDIA_PROP_TYPE_U64 1
> +#define MEDIA_PROP_TYPE_S64 2
> +#define MEDIA_PROP_TYPE_STRING 3
> +
> +/**
> + * struct media_v2_prop - A media property
> + *
> + * @id: The unique non-zero ID of this property
> + * @owner_id:The ID of the object this property belongs to
> + * @type:Property type
> + * @flags: Property flags
> + * @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.
> + * @name:Property name
> + * @uval:Property value (unsigned)
> + * @sval:Property value (signed)
> + */
> +struct media_v2_prop {
> + __u32 id;
> + __u32 owner_id;
> + __u32 type;
> + __u32 flags;
> + __u32 payload_size;
> + __u32 payload_offset;
> + __u32 reserved[18];
> + char name[32];
> + union {
> + __u64 uval;
> + __s64 sval;
> + };
I'm inclined to drop this union and always use payload_size/offset.
Just add simple access functions here to get to the payload:
static inline const char *media_prop2s(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);
}
Regards,
Hans
> +} __attribute__ ((packed));
> +
> struct media_v2_topology {
> __u64 topology_version;
>
> @@ -360,6 +394,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 */
> @@ -368,6 +406,8 @@ struct media_v2_topology {
> #define MEDIA_IOC_ENUM_ENTITIES _IOWR('|', 0x01, struct
> media_entity_desc)
> #define MEDIA_IOC_ENUM_LINKS _IOWR('|', 0x02, struct media_links_enum)
> #define MEDIA_IOC_SETUP_LINK _IOWR('|', 0x03, struct media_link_desc)
> +/* Old MEDIA_IOC_G_TOPOLOGY ioctl without props support */
> +#define MEDIA_IOC_G_TOPOLOGY_OLD 0xc0487c04
> #define MEDIA_IOC_G_TOPOLOGY _IOWR('|', 0x04, struct media_v2_topology)
>
> #ifndef __KERNEL__
>
