Re: [PATCH v4 1/2] media: videodev2: Add flags to unconditionnaly enumerate pixels formats

[Benjamin Gaignard <benjamin.gaignard@collabora.com>]

Le 18/07/2024 à 16:02, Nicolas Dufresne a écrit :
> Le jeudi 18 juillet 2024 à 09:56 -0400, Nicolas Dufresne a écrit :
>> Hi,
>>
>> Le jeudi 18 juillet 2024 à 09:04 +0200, Benjamin Gaignard a écrit :
>>> Le 17/07/2024 à 19:50, Nicolas Dufresne a écrit :
>> [...]
>>
>>>>>>>> @@ -1569,6 +1569,7 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
>>>>>>>>     	int ret = check_fmt(file, p->type);
>>>>>>>>     	u32 mbus_code;
>>>>>>>>     	u32 cap_mask;
>>>>>>>> +	u32 flags;
>>>>>>>>
>>>>>>>>     	if (ret)
>>>>>>>>     		return ret;
>>>>>>>> @@ -1578,8 +1579,10 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
>>>>>>>>     		p->mbus_code = 0;
>>>>>>>>
>>>>>>>>     	mbus_code = p->mbus_code;
>>>>>>>> +	flags = p->flags & V4L2_FMT_FLAG_ENUM_ALL_FORMATS;
>>>>>>>>     	memset_after(p, 0, type);
>>>>>>>>     	p->mbus_code = mbus_code;
>>>>>>>> +	p->flags = flags;
>>>>>>> Won't this set V4L2_FMT_FLAG_ENUM_ALL_FORMATS (if present) in the
>>>>>>> flags returned to userspace ? Shouldn't be drivers to set
>>>>>>> V4L2_FMT_FLAG_ALL_FORMATS instead ?
>>>>>> memset_after zeroed flags field so we need this to send V4L2_FMT_FLAG_ENUM_ALL_FORMATS
>>>>>> flag to drivers. Return it to userspace is a side effect but I don't that is problem
>>>>>> since it set it anyway.
>>>>>>
>>>>> Ok, if the expectation is that the flag is preserved through the ioctl
>>>>> call, this is fine with me
>>>> I might be missing something other similar features are explicitly advertised by
>>>> drivers. This way, the generic layer can keep or clear the flag based of if its
>>>> supported. The fact the flag persist the ioctl() or not endup having a useful
>>>> semantic.
>>>>
>>>> Could we do the same?
>>> It is the only flag set by userspace when calling the ioctl(), all others
>>> are set by the drivers.
>>> I can clean it from the ioctl() structure after driver call but that won't change anything.
>> This does not answer my question. In other similar feature, we have an
>> **internal** flag set by drivers to tell the framework that such feature is
>> abled. Using that, the framework can keep or remove that flag based on if its
>> supported or not. This way, userspace have a clue if the driver do have this
>> support or if the returned result (in that case) is just a subset matching the
>> configuration. We don't seem to have done the same level of effort here.
> For the reference, you actually use that semantic in GStreamer implementation,
> but the kernel code seems broken.
>
> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/7078/diffs#eb90d5495df2f085f163996014c748a36f143f76_516_527

device_caps u32 field is already almost fully used, only one 1 bit is free.
I could use it but, for me, the capability to enumerate all the formats
doesn't fit well in the existing list.

Benjamin

>
>> Nicolas
>>
>>>>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>>>>
>>>>>>>>     	switch (p->type) {
>>>>>>>>     	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>>>>>>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>>>>>>> index fe6b67e83751..b6a5da79ba21 100644
>>>>>>>> --- a/include/uapi/linux/videodev2.h
>>>>>>>> +++ b/include/uapi/linux/videodev2.h
>>>>>>>> @@ -886,6 +886,8 @@ struct v4l2_fmtdesc {
>>>>>>>>     #define V4L2_FMT_FLAG_CSC_HSV_ENC		V4L2_FMT_FLAG_CSC_YCBCR_ENC
>>>>>>>>     #define V4L2_FMT_FLAG_CSC_QUANTIZATION		0x0100
>>>>>>>>     #define V4L2_FMT_FLAG_META_LINE_BASED		0x0200
>>>>>>>> +#define V4L2_FMT_FLAG_ENUM_ALL_FORMATS		0x0400
>>>>>>>> +#define V4L2_FMT_FLAG_ALL_FORMATS		0x0800
>>>>>>>>
>>>>>>>>     	/* Frame Size and frame rate enumeration */
>>>>>>>>     /*
>>>>>>>> --
>>>>>>>> 2.43.0
>>>>>>>>
>>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> Kernel mailing list -- kernel@mailman.collabora.com
>>>>>>> To unsubscribe send an email to kernel-leave@mailman.collabora.com
>>>>>>> This list is managed by https://mailman.collabora.com
>>> _______________________________________________
>>> Kernel mailing list -- kernel@mailman.collabora.com
>>> To unsubscribe send an email to kernel-leave@mailman.collabora.com
>>> This list is managed by https://mailman.collabora.com
>> _______________________________________________
>> Kernel mailing list -- kernel@mailman.collabora.com
>> To unsubscribe send an email to kernel-leave@mailman.collabora.com
>> This list is managed by https://mailman.collabora.com