[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <069a9e2b-f0bc-46a7-aaec-f30157e9be2d@collabora.com>
Date: Thu, 18 Jul 2024 16:43:28 +0200
From: Benjamin Gaignard <benjamin.gaignard@...labora.com>
To: Nicolas Dufresne <nicolas@...fresne.ca>,
Jacopo Mondi <jacopo.mondi@...asonboard.com>
Cc: mchehab@...nel.org, ezequiel@...guardiasur.com.ar,
hverkuil-cisco@...all.nl, linux-media@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-rockchip@...ts.infradead.org,
kernel@...labora.com
Subject: Re: [PATCH v4 1/2] media: videodev2: Add flags to unconditionnaly
enumerate pixels formats
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@...asonboard.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@...lman.collabora.com
>>>>>>> To unsubscribe send an email to kernel-leave@...lman.collabora.com
>>>>>>> This list is managed by https://mailman.collabora.com
>>> _______________________________________________
>>> Kernel mailing list -- kernel@...lman.collabora.com
>>> To unsubscribe send an email to kernel-leave@...lman.collabora.com
>>> This list is managed by https://mailman.collabora.com
>> _______________________________________________
>> Kernel mailing list -- kernel@...lman.collabora.com
>> To unsubscribe send an email to kernel-leave@...lman.collabora.com
>> This list is managed by https://mailman.collabora.com
Powered by blists - more mailing lists