On 04/16/18 21:41, Hans Verkuil wrote:
> On 04/16/2018 08:09 PM, Mauro Carvalho Chehab wrote:
>> Em Mon, 16 Apr 2018 15:03:35 -0300
>> Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxxx> escreveu:
>>
>>> Em Mon, 16 Apr 2018 15:21:18 +0200
>>> Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
>>>
>>>> From: Hans Verkuil <hansverk@xxxxxxxxx>
>>>>
>>>> The v2 pad structure never exposed the pad index, which made it impossible
>>>> to call the MEDIA_IOC_SETUP_LINK ioctl, which needs that information.
>>>>
>>>> It is really trivial to just expose this information, so implement this.
>>>
>>> Acked-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxxx>
>>
>> Err... I looked on it too fast... See my comments below.
>>
>> The same applies to patch 8/9.
>>
>>>>
>>>> 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 dca1e5a3e0f9..73ffea3e81c9 100644
>>>> --- a/drivers/media/media-device.c
>>>> +++ b/drivers/media/media-device.c
>>>> @@ -331,6 +331,7 @@ static long media_device_get_topology(struct media_device *mdev,
>>>> kpad.id = pad->graph_obj.id;
>>>> kpad.entity_id = pad->entity->graph_obj.id;
>>>> kpad.flags = pad->flags;
>>>> + kpad.index = pad->index;
>>>>
>>>> if (copy_to_user(upad, &kpad, sizeof(kpad)))
>>>> ret = -EFAULT;
>>>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
>>>> index ac08acffdb65..15f7f432f808 100644
>>>> --- a/include/uapi/linux/media.h
>>>> +++ b/include/uapi/linux/media.h
>>>> @@ -310,11 +310,16 @@ struct media_v2_interface {
>>>> };
>>>> } __attribute__ ((packed));
>>>>
>>>> +/* Appeared in 4.18.0 */
>>>> +#define MEDIA_V2_PAD_HAS_INDEX(media_version) \
>>>> + ((media_version) >= 0x00041200)
>>>> +
>>
>> I don't like this, for a couple of reasons:
>>
>> 1) it has a magic number on it, with is actually a parsed
>> version of LINUX_VERSION() macro;
>
> I can/should change that to KERNEL_VERSION().
>
>>
>> 2) it sounds really weird to ship a header file with a new
>> kernel version meant to provide backward compatibility with
>> older versions;
>>
>> 3) this isn't any different than:
>>
>> #define MEDIA_V2_PAD_HAS_INDEX -1
>>
>> I think we need to think a little bit more about that.
>
> What typically happens is that applications (like those in v4l-utils
> for example) copy the headers locally. So they are compiled with the headers
> of a specific kernel version, but they can run with very different kernels.
>
> This is normal for distros where you can install different kernel versions
> without needing to modify applications.
>
> In fact, we (Cisco) use the latest v4l-utils code on kernels ranging between
> 2.6.39 to 4.10 (I think that's the latest one in use).
>
> The media version tells you whether or not the kernel supports this feature.
> I don't see another way of doing this.
>
> In most other cases we can just say that if the field value is 0, then it
> should not be used. Unfortunately, 0 is a valid value for the pad index, for
> the entity flags and for the entity function (some drivers set it to
> MEDIA_ENT_F_UNKNOWN, which has value 0). This last one is most unfortunate,
> since this should never have happened and would have been detected if we had
> proper compliance tools.
Actually, I think that if I first ensure that all drivers correctly set function
to a non-zero value, then there is no need for a test macro and I can just say
that if it is 0, then fall back to 'type'.
It requires some analysis, but it's doable.
For the index and flags field there is no alternative that I can think of, though.
Regards,
Hans
>
> Regards,
>
> Hans
>
>>
>>
>>>> struct media_v2_pad {
>>>> __u32 id;
>>>> __u32 entity_id;
>>>> __u32 flags;
>>>> - __u32 reserved[5];
>>>> + __u32 index;
>>>> + __u32 reserved[4];
>>>> } __attribute__ ((packed));
>>>>
>>>> struct media_v2_link {
>>>
>>>
>>>
>>> Thanks,
>>> Mauro
>>
>>
>>
>> Thanks,
>> Mauro
>>
>
