On 04/16/2018 08:01 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 16 Apr 2018 15:21:16 +0200
> Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
>
>> From: Hans Verkuil <hansverk@xxxxxxxxx>
>>
>> This adds support for 'proper' functions to the existing API.
>> This information was before only available through the new v2
>> API, with this change it's available to both.
>>
>> Yes, the plan is to allow entities to expose multiple functions for
>> multi-function devices, but we do not support it anywhere so this
>> is still vaporware.
>
> I'm not convinced about that. I would, instead, just keep it as-is
> and be sure that applications stop use the legacy calls.
You can't. First of all, since the new API does not provide the pad index
(fixed in patch 6/9) it is impossible to use the new API with any driver
that supports SETUP_LINK. So any such driver that uses any of the newer
subdevs with a function that is mapped to MEDIA_ENT_T_DEVNODE_UNKNOWN
is currently not reporting that correctly. A good example is the
imx driver. But also others if they are combined with such newer subdevs.
There is nothing wrong with the old API, except for not reporting the
proper function value in field 'type' due to historical concerns.
There is NO WAY we can suddenly prohibit applications from using the old
API since the new API was never usable. And besides that, we have no method
of knowing who uses the old API since such applications are likely custom
for specific hardware.
All that is really missing in the 'old' API (I hate the terms 'old' and
'new', they are misleading) is a proper 'function' field. Let's just add it
and make it consistent with the documentation about entity functions.
>
>>
>> Signed-off-by: Hans Verkuil <hansverk@xxxxxxxxx>
>> ---
>> drivers/media/media-device.c | 1 +
>> include/uapi/linux/media.h | 7 ++++++-
>> 2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>> index 7c3ab37c258a..dca1e5a3e0f9 100644
>> --- a/drivers/media/media-device.c
>> +++ b/drivers/media/media-device.c
>> @@ -115,6 +115,7 @@ static long media_device_enum_entities(struct media_device *mdev,
>> if (ent->name)
>> strlcpy(entd->name, ent->name, sizeof(entd->name));
>> entd->type = ent->function;
>> + entd->function = ent->function;
>> entd->revision = 0; /* Unused */
>
> I got confused here, until I went to the code and noticed that
> entd->type is actually touched after this.
>
> If we're willing to do that, you should add a comment there explaining
> why we need to pass both type and function to userspace.
True.
Regards,
Hans
>
>> entd->flags = ent->flags;
>> entd->group_id = 0; /* Unused */
>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
>> index 86c7dcc9cba3..ac08acffdb65 100644
>> --- a/include/uapi/linux/media.h
>> +++ b/include/uapi/linux/media.h
>> @@ -146,6 +146,10 @@ struct media_device_info {
>> /* OR with the entity id value to find the next entity */
>> #define MEDIA_ENT_ID_FLAG_NEXT (1 << 31)
>>
>> +/* Appeared in 4.18.0 */
>> +#define MEDIA_ENTITY_DESC_HAS_FUNCTION(media_version) \
>> + ((media_version) >= 0x00041200)
>> +
>> struct media_entity_desc {
>> __u32 id;
>> char name[32];
>> @@ -155,8 +159,9 @@ struct media_entity_desc {
>> __u32 group_id;
>> __u16 pads;
>> __u16 links;
>> + __u32 function;
>>
>> - __u32 reserved[4];
>> + __u32 reserved[3];
>>
>> union {
>> /* Node specifications */
>
>
>
> Thanks,
> Mauro
>
