On 04/17/18 14:16, Mauro Carvalho Chehab wrote:
> Em Tue, 17 Apr 2018 14:01:06 +0200
> Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
>
>> On 04/17/18 13:55, Mauro Carvalho Chehab wrote:
>>> Em Tue, 17 Apr 2018 11:59:40 +0200
>>> Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
>>>
>>>> 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().
>>>
>>> I don't think so. The macro is not there at include/uapi.
>>>
>>>>>
>>>>>>
>>>>>> 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).
>>>
>>> Well, if you use a macro, the "compat" code at v4l-utils (or whatever other
>>> app you use) will be assuming the specific Kernel version you used when you
>>> built it, with is probably not what you want.
>>>
>>> The way of checking if a feature is there or not is, instead, to ask for
>>> the media version via MEDIA_IOC_DEVICE_INFO. It should provide the
>>> media API version.
>>>
>>> This is already filled with:
>>> info->media_version = LINUX_VERSION_CODE;
>>>
>>> So, all we need to do is to document that the new fields are available only
>>> for such version or above and add such check at v4l-utils.
>>
>> Yes, and that's what you stick in the macro argument:
>>
>> ioctl(fd, MEDIA_IOC_DEVICE_INFO, &info);
>> if (MEDIA_V2_PAD_HAS_INDEX(info.media_version)) {
>> // I can use the index field
>> }
>>
>> I think I did not document this clearly.
>
> Ok, makes sense. It should be better documented, IMO.
>
> Still have an issue with KERNEL_VERSION: this macro doesn't exist
> anymore on any Kernel header files. It is produced dynamically
> at /Makefile:
>
> define filechk_version.h
> (echo \#define LINUX_VERSION_CODE $(shell \
> expr $(VERSION) \* 65536 + 0$(PATCHLEVEL) \* 256 + 0$(SUBLEVEL)); \
> echo '#define KERNEL_VERSION(a,b,c) (((a) << 16) + ((b) << 8) + (c))';)
> endef
>
> Btw, this likely means that this is already broken:
>
> include/uapi/linux/media.h:#define MEDIA_API_VERSION KERNEL_VERSION(0, 1, 0)
>
> as userspace won't be able to evaluate it.
>
> We could hardcode its value, as you proposed, but, IMHO, that sucks.
This might be why I hardcoded it :-)
I personally don't have a problem with hardcoding it. In the end, it's just a number.
Regards,
Hans
