On 17/04/18 14:19, Hans Verkuil wrote:
> 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.
How about this:
/* Appeared in 4.18.0 */
#define MEDIA_V2_PAD_HAS_INDEX(media_version) \
((media_version) >= ((4 << 16) | (18 << 8) | 0))
It's better than 0x00041200, and it works in the absence of KERNEL_VERSION.
Regards,
Hans
