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

[Nicolas Dufresne <nicolas@ndufresne.ca>]

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@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