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

[Nicolas Dufresne]

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.

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