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

[Nicolas Dufresne]

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

> 
> Nicolas
> 
> > 
> > > 
> > > > Reviewed-by: Jacopo Mondi <jacopo.mondi at 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 at mailman.collabora.com
> > > > > > To unsubscribe send an email to kernel-leave at mailman.collabora.com
> > > > > > This list is managed by https://mailman.collabora.com
> > > 
> > _______________________________________________
> > Kernel mailing list -- kernel at mailman.collabora.com
> > To unsubscribe send an email to kernel-leave at mailman.collabora.com
> > This list is managed by https://mailman.collabora.com
> 
> _______________________________________________
> Kernel mailing list -- kernel at mailman.collabora.com
> To unsubscribe send an email to kernel-leave at mailman.collabora.com
> This list is managed by https://mailman.collabora.com