| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7a5167163f1df60899c9e2bfef1e42e0c67567f7.camel@ndufresne.ca>
Date: Thu, 18 Jul 2024 12:46:35 -0400
From: Nicolas Dufresne <nicolas@...fresne.ca>
To: Benjamin Gaignard <benjamin.gaignard@...labora.com>, 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 jeudi 18 juillet 2024 à 16:43 +0200, Benjamin Gaignard a écrit :
> 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.
Sorry, but I will re-iterate that I don't mean to add a uAPI but an **internal**
flag between driver and framework. This way the framework knows at least.
>
> 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