| Message ID | 20240717131430.159727-2-benjamin.gaignard@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enumerate all pixels formats | expand |
Hi Benjamin On Wed, Jul 17, 2024 at 03:14:29PM GMT, Benjamin Gaignard wrote: > Add new flags to enumerate all pixels formats when calling VIDIOC_ENUM_FMT ioctl. > When this V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag is set drivers must > ignore the configuration and return the hardware supported pixel > formats for the specified queue. > To distinguish this particular enumeration case V4L2_FMT_FLAG_ALL_FORMATS > flag must be set by the drivers to highlight support of this feature > to user space applications. > This will permit to discover which pixel formats are supported > without setting codec-specific information so userland can more easily > know if the driver suits its needs well. > The main target are stateless decoders so update the documentation > about how to use this flag. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > changes in version 4: > - Explicitly document that the new flags are targeting mem2mem devices. > > .../userspace-api/media/v4l/dev-stateless-decoder.rst | 6 ++++++ > .../userspace-api/media/v4l/vidioc-enum-fmt.rst | 11 +++++++++++ > .../userspace-api/media/videodev2.h.rst.exceptions | 2 ++ > drivers/media/v4l2-core/v4l2-ioctl.c | 3 +++ > include/uapi/linux/videodev2.h | 2 ++ > 5 files changed, 24 insertions(+) > > diff --git a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst > index 35ed05f2695e..b0b657de910d 100644 > --- a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst > +++ b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst > @@ -58,6 +58,12 @@ Querying capabilities > default values for these controls being used, and a returned set of formats > that may not be usable for the media the client is trying to decode. > > + * If the ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` flag is set the driver must enumerate > + all the supported formats without taking care of codec-dependent controls > + set on the ``OUTPUT`` queue. To indicate that the driver has take care of this > + flag it must set ``V4L2_FMT_FLAG_ALL_FORMATS`` flag for each format while > + enumerating. > + > 3. The client may use :c:func:`VIDIOC_ENUM_FRAMESIZES` to detect supported > resolutions for a given format, passing desired pixel format in > :c:type:`v4l2_frmsizeenum`'s ``pixel_format``. > diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst > index 3adb3d205531..15bc2f59c05a 100644 > --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst > +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst > @@ -234,6 +234,17 @@ the ``mbus_code`` field is handled differently: > valid. The buffer consists of ``height`` lines, each having ``width`` > Data Units of data and the offset (in bytes) between the beginning of > each two consecutive lines is ``bytesperline``. > + * - ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` > + - 0x0400 > + - Set by userland applications to enumerate all possible pixel formats > + without taking care of any OUTPUT or CAPTURE queue configuration. > + This flag is relevant only for mem2mem devices. > + * - ``V4L2_FMT_FLAG_ALL_FORMATS`` > + - 0x0800 > + - Set by the driver to indicated that format have been enumerated because > + :ref:`V4L2_FMT_FLAG_ENUM_ALL_FORMATS <v4l2-pix-fmt-flag-set-csc>` has > + been set by the userland application. > + This flag is relevant only for mem2mem devices. Thanks, however I think this can be wrapper on the previous line > > Return Value > ============ > diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > index bdc628e8c1d6..7a3a1e9dc055 100644 > --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions > +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > @@ -216,6 +216,8 @@ replace define V4L2_FMT_FLAG_CSC_YCBCR_ENC fmtdesc-flags > replace define V4L2_FMT_FLAG_CSC_HSV_ENC fmtdesc-flags > replace define V4L2_FMT_FLAG_CSC_QUANTIZATION fmtdesc-flags > replace define V4L2_FMT_FLAG_META_LINE_BASED fmtdesc-flags > +replace define V4L2_FMT_FLAG_ENUM_ALL_FORMATS fmtdesc-flags > +replace define V4L2_FMT_FLAG_ALL_FORMATS fmtdesc-flags > > # V4L2 timecode types > replace define V4L2_TC_TYPE_24FPS timecode-type > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > index 4c76d17b4629..5785a98b6ba2 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -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 ? > > 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 > >
Le 17/07/2024 à 15:20, Jacopo Mondi a écrit : > Hi Benjamin > > On Wed, Jul 17, 2024 at 03:14:29PM GMT, Benjamin Gaignard wrote: >> Add new flags to enumerate all pixels formats when calling VIDIOC_ENUM_FMT ioctl. >> When this V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag is set drivers must >> ignore the configuration and return the hardware supported pixel >> formats for the specified queue. >> To distinguish this particular enumeration case V4L2_FMT_FLAG_ALL_FORMATS >> flag must be set by the drivers to highlight support of this feature >> to user space applications. >> This will permit to discover which pixel formats are supported >> without setting codec-specific information so userland can more easily >> know if the driver suits its needs well. >> The main target are stateless decoders so update the documentation >> about how to use this flag. >> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> --- >> changes in version 4: >> - Explicitly document that the new flags are targeting mem2mem devices. >> >> .../userspace-api/media/v4l/dev-stateless-decoder.rst | 6 ++++++ >> .../userspace-api/media/v4l/vidioc-enum-fmt.rst | 11 +++++++++++ >> .../userspace-api/media/videodev2.h.rst.exceptions | 2 ++ >> drivers/media/v4l2-core/v4l2-ioctl.c | 3 +++ >> include/uapi/linux/videodev2.h | 2 ++ >> 5 files changed, 24 insertions(+) >> >> diff --git a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst >> index 35ed05f2695e..b0b657de910d 100644 >> --- a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst >> +++ b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst >> @@ -58,6 +58,12 @@ Querying capabilities >> default values for these controls being used, and a returned set of formats >> that may not be usable for the media the client is trying to decode. >> >> + * If the ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` flag is set the driver must enumerate >> + all the supported formats without taking care of codec-dependent controls >> + set on the ``OUTPUT`` queue. To indicate that the driver has take care of this >> + flag it must set ``V4L2_FMT_FLAG_ALL_FORMATS`` flag for each format while >> + enumerating. >> + >> 3. The client may use :c:func:`VIDIOC_ENUM_FRAMESIZES` to detect supported >> resolutions for a given format, passing desired pixel format in >> :c:type:`v4l2_frmsizeenum`'s ``pixel_format``. >> diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst >> index 3adb3d205531..15bc2f59c05a 100644 >> --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst >> +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst >> @@ -234,6 +234,17 @@ the ``mbus_code`` field is handled differently: >> valid. The buffer consists of ``height`` lines, each having ``width`` >> Data Units of data and the offset (in bytes) between the beginning of >> each two consecutive lines is ``bytesperline``. >> + * - ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` >> + - 0x0400 >> + - Set by userland applications to enumerate all possible pixel formats >> + without taking care of any OUTPUT or CAPTURE queue configuration. >> + This flag is relevant only for mem2mem devices. >> + * - ``V4L2_FMT_FLAG_ALL_FORMATS`` >> + - 0x0800 >> + - Set by the driver to indicated that format have been enumerated because >> + :ref:`V4L2_FMT_FLAG_ENUM_ALL_FORMATS <v4l2-pix-fmt-flag-set-csc>` has >> + been set by the userland application. >> + This flag is relevant only for mem2mem devices. > Thanks, however I think this can be wrapper on the previous line ok > >> Return Value >> ============ >> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions >> index bdc628e8c1d6..7a3a1e9dc055 100644 >> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions >> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions >> @@ -216,6 +216,8 @@ replace define V4L2_FMT_FLAG_CSC_YCBCR_ENC fmtdesc-flags >> replace define V4L2_FMT_FLAG_CSC_HSV_ENC fmtdesc-flags >> replace define V4L2_FMT_FLAG_CSC_QUANTIZATION fmtdesc-flags >> replace define V4L2_FMT_FLAG_META_LINE_BASED fmtdesc-flags >> +replace define V4L2_FMT_FLAG_ENUM_ALL_FORMATS fmtdesc-flags >> +replace define V4L2_FMT_FLAG_ALL_FORMATS fmtdesc-flags >> >> # V4L2 timecode types >> replace define V4L2_TC_TYPE_24FPS timecode-type >> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c >> index 4c76d17b4629..5785a98b6ba2 100644 >> --- a/drivers/media/v4l2-core/v4l2-ioctl.c >> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c >> @@ -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. > >> 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
Hi Benjamin On Wed, Jul 17, 2024 at 03:44:24PM GMT, Benjamin Gaignard wrote: > > Le 17/07/2024 à 15:20, Jacopo Mondi a écrit : > > Hi Benjamin > > > > On Wed, Jul 17, 2024 at 03:14:29PM GMT, Benjamin Gaignard wrote: > > > Add new flags to enumerate all pixels formats when calling VIDIOC_ENUM_FMT ioctl. > > > When this V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag is set drivers must > > > ignore the configuration and return the hardware supported pixel > > > formats for the specified queue. > > > To distinguish this particular enumeration case V4L2_FMT_FLAG_ALL_FORMATS > > > flag must be set by the drivers to highlight support of this feature > > > to user space applications. > > > This will permit to discover which pixel formats are supported > > > without setting codec-specific information so userland can more easily > > > know if the driver suits its needs well. > > > The main target are stateless decoders so update the documentation > > > about how to use this flag. > > > > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > > > --- > > > changes in version 4: > > > - Explicitly document that the new flags are targeting mem2mem devices. > > > > > > .../userspace-api/media/v4l/dev-stateless-decoder.rst | 6 ++++++ > > > .../userspace-api/media/v4l/vidioc-enum-fmt.rst | 11 +++++++++++ > > > .../userspace-api/media/videodev2.h.rst.exceptions | 2 ++ > > > drivers/media/v4l2-core/v4l2-ioctl.c | 3 +++ > > > include/uapi/linux/videodev2.h | 2 ++ > > > 5 files changed, 24 insertions(+) > > > > > > diff --git a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst > > > index 35ed05f2695e..b0b657de910d 100644 > > > --- a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst > > > +++ b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst > > > @@ -58,6 +58,12 @@ Querying capabilities > > > default values for these controls being used, and a returned set of formats > > > that may not be usable for the media the client is trying to decode. > > > > > > + * If the ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` flag is set the driver must enumerate > > > + all the supported formats without taking care of codec-dependent controls > > > + set on the ``OUTPUT`` queue. To indicate that the driver has take care of this > > > + flag it must set ``V4L2_FMT_FLAG_ALL_FORMATS`` flag for each format while > > > + enumerating. > > > + > > > 3. The client may use :c:func:`VIDIOC_ENUM_FRAMESIZES` to detect supported > > > resolutions for a given format, passing desired pixel format in > > > :c:type:`v4l2_frmsizeenum`'s ``pixel_format``. > > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst > > > index 3adb3d205531..15bc2f59c05a 100644 > > > --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst > > > +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst > > > @@ -234,6 +234,17 @@ the ``mbus_code`` field is handled differently: > > > valid. The buffer consists of ``height`` lines, each having ``width`` > > > Data Units of data and the offset (in bytes) between the beginning of > > > each two consecutive lines is ``bytesperline``. > > > + * - ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` > > > + - 0x0400 > > > + - Set by userland applications to enumerate all possible pixel formats > > > + without taking care of any OUTPUT or CAPTURE queue configuration. > > > + This flag is relevant only for mem2mem devices. > > > + * - ``V4L2_FMT_FLAG_ALL_FORMATS`` > > > + - 0x0800 > > > + - Set by the driver to indicated that format have been enumerated because > > > + :ref:`V4L2_FMT_FLAG_ENUM_ALL_FORMATS <v4l2-pix-fmt-flag-set-csc>` has > > > + been set by the userland application. > > > + This flag is relevant only for mem2mem devices. > > Thanks, however I think this can be wrapper on the previous line > > ok > > > > > > Return Value > > > ============ > > > diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > > > index bdc628e8c1d6..7a3a1e9dc055 100644 > > > --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions > > > +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > > > @@ -216,6 +216,8 @@ replace define V4L2_FMT_FLAG_CSC_YCBCR_ENC fmtdesc-flags > > > replace define V4L2_FMT_FLAG_CSC_HSV_ENC fmtdesc-flags > > > replace define V4L2_FMT_FLAG_CSC_QUANTIZATION fmtdesc-flags > > > replace define V4L2_FMT_FLAG_META_LINE_BASED fmtdesc-flags > > > +replace define V4L2_FMT_FLAG_ENUM_ALL_FORMATS fmtdesc-flags > > > +replace define V4L2_FMT_FLAG_ALL_FORMATS fmtdesc-flags > > > > > > # V4L2 timecode types > > > replace define V4L2_TC_TYPE_24FPS timecode-type > > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > > > index 4c76d17b4629..5785a98b6ba2 100644 > > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > > > @@ -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 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
Hey Benjamin, typos in the subject line: s/unconditionnaly/unconditionally/ s/pixels/pixel/ On 17.07.2024 15:14, Benjamin Gaignard wrote: >Add new flags to enumerate all pixels formats when calling VIDIOC_ENUM_FMT ioctl. s/pixels/pixel/ >When this V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag is set drivers must >ignore the configuration and return the hardware supported pixel >formats for the specified queue. >To distinguish this particular enumeration case V4L2_FMT_FLAG_ALL_FORMATS >flag must be set by the drivers to highlight support of this feature >to user space applications. >This will permit to discover which pixel formats are supported >without setting codec-specific information so userland can more easily >know if the driver suits its needs well. >The main target are stateless decoders so update the documentation >about how to use this flag. > >Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >--- >changes in version 4: >- Explicitly document that the new flags are targeting mem2mem devices. > > .../userspace-api/media/v4l/dev-stateless-decoder.rst | 6 ++++++ > .../userspace-api/media/v4l/vidioc-enum-fmt.rst | 11 +++++++++++ > .../userspace-api/media/videodev2.h.rst.exceptions | 2 ++ > drivers/media/v4l2-core/v4l2-ioctl.c | 3 +++ > include/uapi/linux/videodev2.h | 2 ++ > 5 files changed, 24 insertions(+) > >diff --git a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst >index 35ed05f2695e..b0b657de910d 100644 >--- a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst >+++ b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst >@@ -58,6 +58,12 @@ Querying capabilities > default values for these controls being used, and a returned set of formats > that may not be usable for the media the client is trying to decode. > >+ * If the ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` flag is set the driver must enumerate >+ all the supported formats without taking care of codec-dependent controls >+ set on the ``OUTPUT`` queue. To indicate that the driver has take care of this >+ flag it must set ``V4L2_FMT_FLAG_ALL_FORMATS`` flag for each format while >+ enumerating. >+ > 3. The client may use :c:func:`VIDIOC_ENUM_FRAMESIZES` to detect supported > resolutions for a given format, passing desired pixel format in > :c:type:`v4l2_frmsizeenum`'s ``pixel_format``. >diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst >index 3adb3d205531..15bc2f59c05a 100644 >--- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst >+++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst >@@ -234,6 +234,17 @@ the ``mbus_code`` field is handled differently: > valid. The buffer consists of ``height`` lines, each having ``width`` > Data Units of data and the offset (in bytes) between the beginning of > each two consecutive lines is ``bytesperline``. >+ * - ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` >+ - 0x0400 >+ - Set by userland applications to enumerate all possible pixel formats >+ without taking care of any OUTPUT or CAPTURE queue configuration. >+ This flag is relevant only for mem2mem devices. >+ * - ``V4L2_FMT_FLAG_ALL_FORMATS`` >+ - 0x0800 >+ - Set by the driver to indicated that format have been enumerated because s/indicated/indicate/ Also, either: "..that a format has been.." or "..that formats have been.." but not "..that format have been.." Regards, Sebastian >+ :ref:`V4L2_FMT_FLAG_ENUM_ALL_FORMATS <v4l2-pix-fmt-flag-set-csc>` has >+ been set by the userland application. >+ This flag is relevant only for mem2mem devices. > > Return Value > ============ >diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions >index bdc628e8c1d6..7a3a1e9dc055 100644 >--- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions >+++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions >@@ -216,6 +216,8 @@ replace define V4L2_FMT_FLAG_CSC_YCBCR_ENC fmtdesc-flags > replace define V4L2_FMT_FLAG_CSC_HSV_ENC fmtdesc-flags > replace define V4L2_FMT_FLAG_CSC_QUANTIZATION fmtdesc-flags > replace define V4L2_FMT_FLAG_META_LINE_BASED fmtdesc-flags >+replace define V4L2_FMT_FLAG_ENUM_ALL_FORMATS fmtdesc-flags >+replace define V4L2_FMT_FLAG_ALL_FORMATS fmtdesc-flags > > # V4L2 timecode types > replace define V4L2_TC_TYPE_24FPS timecode-type >diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c >index 4c76d17b4629..5785a98b6ba2 100644 >--- a/drivers/media/v4l2-core/v4l2-ioctl.c >+++ b/drivers/media/v4l2-core/v4l2-ioctl.c >@@ -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; > > 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
Hi Le mercredi 17 juillet 2024 à 16:04 +0200, Jacopo Mondi a écrit : > Hi Benjamin > > On Wed, Jul 17, 2024 at 03:44:24PM GMT, Benjamin Gaignard wrote: > > > > Le 17/07/2024 à 15:20, Jacopo Mondi a écrit : > > > Hi Benjamin > > > > > > On Wed, Jul 17, 2024 at 03:14:29PM GMT, Benjamin Gaignard wrote: > > > > Add new flags to enumerate all pixels formats when calling VIDIOC_ENUM_FMT ioctl. > > > > When this V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag is set drivers must > > > > ignore the configuration and return the hardware supported pixel > > > > formats for the specified queue. > > > > To distinguish this particular enumeration case V4L2_FMT_FLAG_ALL_FORMATS > > > > flag must be set by the drivers to highlight support of this feature > > > > to user space applications. > > > > This will permit to discover which pixel formats are supported > > > > without setting codec-specific information so userland can more easily > > > > know if the driver suits its needs well. > > > > The main target are stateless decoders so update the documentation > > > > about how to use this flag. > > > > > > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > > > > --- > > > > changes in version 4: > > > > - Explicitly document that the new flags are targeting mem2mem devices. > > > > > > > > .../userspace-api/media/v4l/dev-stateless-decoder.rst | 6 ++++++ > > > > .../userspace-api/media/v4l/vidioc-enum-fmt.rst | 11 +++++++++++ > > > > .../userspace-api/media/videodev2.h.rst.exceptions | 2 ++ > > > > drivers/media/v4l2-core/v4l2-ioctl.c | 3 +++ > > > > include/uapi/linux/videodev2.h | 2 ++ > > > > 5 files changed, 24 insertions(+) > > > > > > > > diff --git a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst > > > > index 35ed05f2695e..b0b657de910d 100644 > > > > --- a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst > > > > +++ b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst > > > > @@ -58,6 +58,12 @@ Querying capabilities > > > > default values for these controls being used, and a returned set of formats > > > > that may not be usable for the media the client is trying to decode. > > > > > > > > + * If the ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` flag is set the driver must enumerate > > > > + all the supported formats without taking care of codec-dependent controls > > > > + set on the ``OUTPUT`` queue. To indicate that the driver has take care of this > > > > + flag it must set ``V4L2_FMT_FLAG_ALL_FORMATS`` flag for each format while > > > > + enumerating. > > > > + > > > > 3. The client may use :c:func:`VIDIOC_ENUM_FRAMESIZES` to detect supported > > > > resolutions for a given format, passing desired pixel format in > > > > :c:type:`v4l2_frmsizeenum`'s ``pixel_format``. > > > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst > > > > index 3adb3d205531..15bc2f59c05a 100644 > > > > --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst > > > > +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst > > > > @@ -234,6 +234,17 @@ the ``mbus_code`` field is handled differently: > > > > valid. The buffer consists of ``height`` lines, each having ``width`` > > > > Data Units of data and the offset (in bytes) between the beginning of > > > > each two consecutive lines is ``bytesperline``. > > > > + * - ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` > > > > + - 0x0400 > > > > + - Set by userland applications to enumerate all possible pixel formats > > > > + without taking care of any OUTPUT or CAPTURE queue configuration. > > > > + This flag is relevant only for mem2mem devices. > > > > + * - ``V4L2_FMT_FLAG_ALL_FORMATS`` > > > > + - 0x0800 > > > > + - Set by the driver to indicated that format have been enumerated because > > > > + :ref:`V4L2_FMT_FLAG_ENUM_ALL_FORMATS <v4l2-pix-fmt-flag-set-csc>` has > > > > + been set by the userland application. > > > > + This flag is relevant only for mem2mem devices. > > > Thanks, however I think this can be wrapper on the previous line > > > > ok > > > > > > > > > Return Value > > > > ============ > > > > diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > > > > index bdc628e8c1d6..7a3a1e9dc055 100644 > > > > --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions > > > > +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > > > > @@ -216,6 +216,8 @@ replace define V4L2_FMT_FLAG_CSC_YCBCR_ENC fmtdesc-flags > > > > replace define V4L2_FMT_FLAG_CSC_HSV_ENC fmtdesc-flags > > > > replace define V4L2_FMT_FLAG_CSC_QUANTIZATION fmtdesc-flags > > > > replace define V4L2_FMT_FLAG_META_LINE_BASED fmtdesc-flags > > > > +replace define V4L2_FMT_FLAG_ENUM_ALL_FORMATS fmtdesc-flags > > > > +replace define V4L2_FMT_FLAG_ALL_FORMATS fmtdesc-flags > > > > > > > > # V4L2 timecode types > > > > replace define V4L2_TC_TYPE_24FPS timecode-type > > > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > > > > index 4c76d17b4629..5785a98b6ba2 100644 > > > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > > > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > > > > @@ -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? > > 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 >
Le 17/07/2024 à 19:50, Nicolas Dufresne a écrit : > Hi > > Le mercredi 17 juillet 2024 à 16:04 +0200, Jacopo Mondi a écrit : >> Hi Benjamin >> >> On Wed, Jul 17, 2024 at 03:44:24PM GMT, Benjamin Gaignard wrote: >>> Le 17/07/2024 à 15:20, Jacopo Mondi a écrit : >>>> Hi Benjamin >>>> >>>> On Wed, Jul 17, 2024 at 03:14:29PM GMT, Benjamin Gaignard wrote: >>>>> Add new flags to enumerate all pixels formats when calling VIDIOC_ENUM_FMT ioctl. >>>>> When this V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag is set drivers must >>>>> ignore the configuration and return the hardware supported pixel >>>>> formats for the specified queue. >>>>> To distinguish this particular enumeration case V4L2_FMT_FLAG_ALL_FORMATS >>>>> flag must be set by the drivers to highlight support of this feature >>>>> to user space applications. >>>>> This will permit to discover which pixel formats are supported >>>>> without setting codec-specific information so userland can more easily >>>>> know if the driver suits its needs well. >>>>> The main target are stateless decoders so update the documentation >>>>> about how to use this flag. >>>>> >>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >>>>> --- >>>>> changes in version 4: >>>>> - Explicitly document that the new flags are targeting mem2mem devices. >>>>> >>>>> .../userspace-api/media/v4l/dev-stateless-decoder.rst | 6 ++++++ >>>>> .../userspace-api/media/v4l/vidioc-enum-fmt.rst | 11 +++++++++++ >>>>> .../userspace-api/media/videodev2.h.rst.exceptions | 2 ++ >>>>> drivers/media/v4l2-core/v4l2-ioctl.c | 3 +++ >>>>> include/uapi/linux/videodev2.h | 2 ++ >>>>> 5 files changed, 24 insertions(+) >>>>> >>>>> diff --git a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst >>>>> index 35ed05f2695e..b0b657de910d 100644 >>>>> --- a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst >>>>> +++ b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst >>>>> @@ -58,6 +58,12 @@ Querying capabilities >>>>> default values for these controls being used, and a returned set of formats >>>>> that may not be usable for the media the client is trying to decode. >>>>> >>>>> + * If the ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` flag is set the driver must enumerate >>>>> + all the supported formats without taking care of codec-dependent controls >>>>> + set on the ``OUTPUT`` queue. To indicate that the driver has take care of this >>>>> + flag it must set ``V4L2_FMT_FLAG_ALL_FORMATS`` flag for each format while >>>>> + enumerating. >>>>> + >>>>> 3. The client may use :c:func:`VIDIOC_ENUM_FRAMESIZES` to detect supported >>>>> resolutions for a given format, passing desired pixel format in >>>>> :c:type:`v4l2_frmsizeenum`'s ``pixel_format``. >>>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst >>>>> index 3adb3d205531..15bc2f59c05a 100644 >>>>> --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst >>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst >>>>> @@ -234,6 +234,17 @@ the ``mbus_code`` field is handled differently: >>>>> valid. The buffer consists of ``height`` lines, each having ``width`` >>>>> Data Units of data and the offset (in bytes) between the beginning of >>>>> each two consecutive lines is ``bytesperline``. >>>>> + * - ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` >>>>> + - 0x0400 >>>>> + - Set by userland applications to enumerate all possible pixel formats >>>>> + without taking care of any OUTPUT or CAPTURE queue configuration. >>>>> + This flag is relevant only for mem2mem devices. >>>>> + * - ``V4L2_FMT_FLAG_ALL_FORMATS`` >>>>> + - 0x0800 >>>>> + - Set by the driver to indicated that format have been enumerated because >>>>> + :ref:`V4L2_FMT_FLAG_ENUM_ALL_FORMATS <v4l2-pix-fmt-flag-set-csc>` has >>>>> + been set by the userland application. >>>>> + This flag is relevant only for mem2mem devices. >>>> Thanks, however I think this can be wrapper on the previous line >>> ok >>> >>>>> Return Value >>>>> ============ >>>>> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions >>>>> index bdc628e8c1d6..7a3a1e9dc055 100644 >>>>> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions >>>>> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions >>>>> @@ -216,6 +216,8 @@ replace define V4L2_FMT_FLAG_CSC_YCBCR_ENC fmtdesc-flags >>>>> replace define V4L2_FMT_FLAG_CSC_HSV_ENC fmtdesc-flags >>>>> replace define V4L2_FMT_FLAG_CSC_QUANTIZATION fmtdesc-flags >>>>> replace define V4L2_FMT_FLAG_META_LINE_BASED fmtdesc-flags >>>>> +replace define V4L2_FMT_FLAG_ENUM_ALL_FORMATS fmtdesc-flags >>>>> +replace define V4L2_FMT_FLAG_ALL_FORMATS fmtdesc-flags >>>>> >>>>> # V4L2 timecode types >>>>> replace define V4L2_TC_TYPE_24FPS timecode-type >>>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c >>>>> index 4c76d17b4629..5785a98b6ba2 100644 >>>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c >>>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c >>>>> @@ -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. > >> 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 >
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@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
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@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
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@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
On 7/17/24 15:14, Benjamin Gaignard wrote: > Add new flags to enumerate all pixels formats when calling VIDIOC_ENUM_FMT ioctl. > When this V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag is set drivers must > ignore the configuration and return the hardware supported pixel > formats for the specified queue. > To distinguish this particular enumeration case V4L2_FMT_FLAG_ALL_FORMATS > flag must be set by the drivers to highlight support of this feature > to user space applications. > This will permit to discover which pixel formats are supported > without setting codec-specific information so userland can more easily > know if the driver suits its needs well. > The main target are stateless decoders so update the documentation > about how to use this flag. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > changes in version 4: > - Explicitly document that the new flags are targeting mem2mem devices. > > .../userspace-api/media/v4l/dev-stateless-decoder.rst | 6 ++++++ > .../userspace-api/media/v4l/vidioc-enum-fmt.rst | 11 +++++++++++ > .../userspace-api/media/videodev2.h.rst.exceptions | 2 ++ > drivers/media/v4l2-core/v4l2-ioctl.c | 3 +++ > include/uapi/linux/videodev2.h | 2 ++ > 5 files changed, 24 insertions(+) > > diff --git a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst > index 35ed05f2695e..b0b657de910d 100644 > --- a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst > +++ b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst > @@ -58,6 +58,12 @@ Querying capabilities > default values for these controls being used, and a returned set of formats > that may not be usable for the media the client is trying to decode. > > + * If the ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` flag is set the driver must enumerate > + all the supported formats without taking care of codec-dependent controls > + set on the ``OUTPUT`` queue. To indicate that the driver has take care of this > + flag it must set ``V4L2_FMT_FLAG_ALL_FORMATS`` flag for each format while > + enumerating. > + > 3. The client may use :c:func:`VIDIOC_ENUM_FRAMESIZES` to detect supported > resolutions for a given format, passing desired pixel format in > :c:type:`v4l2_frmsizeenum`'s ``pixel_format``. > diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst > index 3adb3d205531..15bc2f59c05a 100644 > --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst > +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst > @@ -234,6 +234,17 @@ the ``mbus_code`` field is handled differently: > valid. The buffer consists of ``height`` lines, each having ``width`` > Data Units of data and the offset (in bytes) between the beginning of > each two consecutive lines is ``bytesperline``. > + * - ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` > + - 0x0400 > + - Set by userland applications to enumerate all possible pixel formats > + without taking care of any OUTPUT or CAPTURE queue configuration. > + This flag is relevant only for mem2mem devices. > + * - ``V4L2_FMT_FLAG_ALL_FORMATS`` > + - 0x0800 > + - Set by the driver to indicated that format have been enumerated because > + :ref:`V4L2_FMT_FLAG_ENUM_ALL_FORMATS <v4l2-pix-fmt-flag-set-csc>` has > + been set by the userland application. > + This flag is relevant only for mem2mem devices. > > Return Value > ============ > diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > index bdc628e8c1d6..7a3a1e9dc055 100644 > --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions > +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > @@ -216,6 +216,8 @@ replace define V4L2_FMT_FLAG_CSC_YCBCR_ENC fmtdesc-flags > replace define V4L2_FMT_FLAG_CSC_HSV_ENC fmtdesc-flags > replace define V4L2_FMT_FLAG_CSC_QUANTIZATION fmtdesc-flags > replace define V4L2_FMT_FLAG_META_LINE_BASED fmtdesc-flags > +replace define V4L2_FMT_FLAG_ENUM_ALL_FORMATS fmtdesc-flags > +replace define V4L2_FMT_FLAG_ALL_FORMATS fmtdesc-flags > > # V4L2 timecode types > replace define V4L2_TC_TYPE_24FPS timecode-type > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > index 4c76d17b4629..5785a98b6ba2 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -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; Nacked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Sorry, you can't just change a read-only field (i.e. set by the driver) to something that can be set by the application as well. That can easily cause problems: e.g. applications that leave the flags field uninitialized. I will try to look at alternatives tomorrow, but this approach is definitely a no-go. Regards, Hans > memset_after(p, 0, type); > p->mbus_code = mbus_code; > + p->flags = flags; > > 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 */ > /*
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
On 17/07/2024 15:14, Benjamin Gaignard wrote: > Add new flags to enumerate all pixels formats when calling VIDIOC_ENUM_FMT ioctl. > When this V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag is set drivers must > ignore the configuration and return the hardware supported pixel > formats for the specified queue. > To distinguish this particular enumeration case V4L2_FMT_FLAG_ALL_FORMATS > flag must be set by the drivers to highlight support of this feature > to user space applications. > This will permit to discover which pixel formats are supported > without setting codec-specific information so userland can more easily > know if the driver suits its needs well. > The main target are stateless decoders so update the documentation > about how to use this flag. > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> > --- > changes in version 4: > - Explicitly document that the new flags are targeting mem2mem devices. > > .../userspace-api/media/v4l/dev-stateless-decoder.rst | 6 ++++++ > .../userspace-api/media/v4l/vidioc-enum-fmt.rst | 11 +++++++++++ > .../userspace-api/media/videodev2.h.rst.exceptions | 2 ++ > drivers/media/v4l2-core/v4l2-ioctl.c | 3 +++ > include/uapi/linux/videodev2.h | 2 ++ > 5 files changed, 24 insertions(+) > > diff --git a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst > index 35ed05f2695e..b0b657de910d 100644 > --- a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst > +++ b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst > @@ -58,6 +58,12 @@ Querying capabilities > default values for these controls being used, and a returned set of formats > that may not be usable for the media the client is trying to decode. > > + * If the ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` flag is set the driver must enumerate > + all the supported formats without taking care of codec-dependent controls > + set on the ``OUTPUT`` queue. To indicate that the driver has take care of this > + flag it must set ``V4L2_FMT_FLAG_ALL_FORMATS`` flag for each format while > + enumerating. > + > 3. The client may use :c:func:`VIDIOC_ENUM_FRAMESIZES` to detect supported > resolutions for a given format, passing desired pixel format in > :c:type:`v4l2_frmsizeenum`'s ``pixel_format``. > diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst > index 3adb3d205531..15bc2f59c05a 100644 > --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst > +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst > @@ -234,6 +234,17 @@ the ``mbus_code`` field is handled differently: > valid. The buffer consists of ``height`` lines, each having ``width`` > Data Units of data and the offset (in bytes) between the beginning of > each two consecutive lines is ``bytesperline``. > + * - ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` > + - 0x0400 > + - Set by userland applications to enumerate all possible pixel formats > + without taking care of any OUTPUT or CAPTURE queue configuration. > + This flag is relevant only for mem2mem devices. > + * - ``V4L2_FMT_FLAG_ALL_FORMATS`` > + - 0x0800 > + - Set by the driver to indicated that format have been enumerated because > + :ref:`V4L2_FMT_FLAG_ENUM_ALL_FORMATS <v4l2-pix-fmt-flag-set-csc>` has > + been set by the userland application. > + This flag is relevant only for mem2mem devices. > > Return Value > ============ > diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > index bdc628e8c1d6..7a3a1e9dc055 100644 > --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions > +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > @@ -216,6 +216,8 @@ replace define V4L2_FMT_FLAG_CSC_YCBCR_ENC fmtdesc-flags > replace define V4L2_FMT_FLAG_CSC_HSV_ENC fmtdesc-flags > replace define V4L2_FMT_FLAG_CSC_QUANTIZATION fmtdesc-flags > replace define V4L2_FMT_FLAG_META_LINE_BASED fmtdesc-flags > +replace define V4L2_FMT_FLAG_ENUM_ALL_FORMATS fmtdesc-flags > +replace define V4L2_FMT_FLAG_ALL_FORMATS fmtdesc-flags > > # V4L2 timecode types > replace define V4L2_TC_TYPE_24FPS timecode-type > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > index 4c76d17b4629..5785a98b6ba2 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -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; > > 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 For reasons mentioned earlier, you cannot make the flags field an input to the driver, that will almost certainly break some applications that do not zero the flags field today, since they expect it to be set by the driver. What probably will work is to add a flag to the index field: this would be similar to what I do in VIDIOC_QUERYCTRL where you can OR the id field with V4L2_CTRL_FLAG_NEXT_CTRL to modify the behavior. It is perfectly fine to use the top bits of the index for this. Drivers that do not support this will just fail with EINVAL, and drivers that do support this will return a proper format. Applications can easily test support for this by just calling ENUM_FMT with 0 ORed with the new flag: if it is supported, then it will return 0, otherwise -EINVAL. With this scheme I think you can also drop the proposed V4L2_FMT_FLAG_ALL_FORMATS flag. But as I mentioned in my reply to the cover letter of this series, I am not convinced we really need this, and if we do, then I am not convinced about the name of the flag. And I would also like to know if such a feature can be used by m2m drivers that are not codecs (e.g. scalers, csc converters), and if that would impact the name/description of the flag. It is currently very specific for decoders, but I prefer to see a more general solution, not just for one specific corner case. Regards, Hans > > /* Frame Size and frame rate enumeration */ > /*
On 19/07/2024 15:15, Hans Verkuil wrote: > On 17/07/2024 15:14, Benjamin Gaignard wrote: >> Add new flags to enumerate all pixels formats when calling VIDIOC_ENUM_FMT ioctl. >> When this V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag is set drivers must >> ignore the configuration and return the hardware supported pixel >> formats for the specified queue. >> To distinguish this particular enumeration case V4L2_FMT_FLAG_ALL_FORMATS >> flag must be set by the drivers to highlight support of this feature >> to user space applications. >> This will permit to discover which pixel formats are supported >> without setting codec-specific information so userland can more easily >> know if the driver suits its needs well. >> The main target are stateless decoders so update the documentation >> about how to use this flag. >> >> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> >> --- >> changes in version 4: >> - Explicitly document that the new flags are targeting mem2mem devices. >> >> .../userspace-api/media/v4l/dev-stateless-decoder.rst | 6 ++++++ >> .../userspace-api/media/v4l/vidioc-enum-fmt.rst | 11 +++++++++++ >> .../userspace-api/media/videodev2.h.rst.exceptions | 2 ++ >> drivers/media/v4l2-core/v4l2-ioctl.c | 3 +++ >> include/uapi/linux/videodev2.h | 2 ++ >> 5 files changed, 24 insertions(+) >> >> diff --git a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst >> index 35ed05f2695e..b0b657de910d 100644 >> --- a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst >> +++ b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst >> @@ -58,6 +58,12 @@ Querying capabilities >> default values for these controls being used, and a returned set of formats >> that may not be usable for the media the client is trying to decode. >> >> + * If the ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` flag is set the driver must enumerate >> + all the supported formats without taking care of codec-dependent controls >> + set on the ``OUTPUT`` queue. To indicate that the driver has take care of this >> + flag it must set ``V4L2_FMT_FLAG_ALL_FORMATS`` flag for each format while >> + enumerating. >> + >> 3. The client may use :c:func:`VIDIOC_ENUM_FRAMESIZES` to detect supported >> resolutions for a given format, passing desired pixel format in >> :c:type:`v4l2_frmsizeenum`'s ``pixel_format``. >> diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst >> index 3adb3d205531..15bc2f59c05a 100644 >> --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst >> +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst >> @@ -234,6 +234,17 @@ the ``mbus_code`` field is handled differently: >> valid. The buffer consists of ``height`` lines, each having ``width`` >> Data Units of data and the offset (in bytes) between the beginning of >> each two consecutive lines is ``bytesperline``. >> + * - ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` >> + - 0x0400 >> + - Set by userland applications to enumerate all possible pixel formats >> + without taking care of any OUTPUT or CAPTURE queue configuration. >> + This flag is relevant only for mem2mem devices. >> + * - ``V4L2_FMT_FLAG_ALL_FORMATS`` >> + - 0x0800 >> + - Set by the driver to indicated that format have been enumerated because >> + :ref:`V4L2_FMT_FLAG_ENUM_ALL_FORMATS <v4l2-pix-fmt-flag-set-csc>` has >> + been set by the userland application. >> + This flag is relevant only for mem2mem devices. >> >> Return Value >> ============ >> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions >> index bdc628e8c1d6..7a3a1e9dc055 100644 >> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions >> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions >> @@ -216,6 +216,8 @@ replace define V4L2_FMT_FLAG_CSC_YCBCR_ENC fmtdesc-flags >> replace define V4L2_FMT_FLAG_CSC_HSV_ENC fmtdesc-flags >> replace define V4L2_FMT_FLAG_CSC_QUANTIZATION fmtdesc-flags >> replace define V4L2_FMT_FLAG_META_LINE_BASED fmtdesc-flags >> +replace define V4L2_FMT_FLAG_ENUM_ALL_FORMATS fmtdesc-flags >> +replace define V4L2_FMT_FLAG_ALL_FORMATS fmtdesc-flags >> >> # V4L2 timecode types >> replace define V4L2_TC_TYPE_24FPS timecode-type >> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c >> index 4c76d17b4629..5785a98b6ba2 100644 >> --- a/drivers/media/v4l2-core/v4l2-ioctl.c >> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c >> @@ -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; >> >> 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 > > For reasons mentioned earlier, you cannot make the flags field an input to the > driver, that will almost certainly break some applications that do not zero > the flags field today, since they expect it to be set by the driver. > > What probably will work is to add a flag to the index field: this would be > similar to what I do in VIDIOC_QUERYCTRL where you can OR the id field with > V4L2_CTRL_FLAG_NEXT_CTRL to modify the behavior. > > It is perfectly fine to use the top bits of the index for this. > > Drivers that do not support this will just fail with EINVAL, and drivers that > do support this will return a proper format. > > Applications can easily test support for this by just calling ENUM_FMT with 0 > ORed with the new flag: if it is supported, then it will return 0, otherwise > -EINVAL. > > With this scheme I think you can also drop the proposed V4L2_FMT_FLAG_ALL_FORMATS > flag. Please note that this is an addition to the uAPI, so this also implies that there should be patches for v4l-utils (v4l2-ctl and v4l2-compliance) and ideally patches for one or more of the media test-drivers to implement this flag. Together with v4l2-compliance that will ensure that the new flag is actually tested by the regression tests. Regards, Hans > > But as I mentioned in my reply to the cover letter of this series, I am not > convinced we really need this, and if we do, then I am not convinced about > the name of the flag. > > And I would also like to know if such a feature can be used by m2m drivers > that are not codecs (e.g. scalers, csc converters), and if that would impact > the name/description of the flag. > > It is currently very specific for decoders, but I prefer to see a more general > solution, not just for one specific corner case. > > Regards, > > Hans > >> >> /* Frame Size and frame rate enumeration */ >> /* > >
diff --git a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst index 35ed05f2695e..b0b657de910d 100644 --- a/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst +++ b/Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst @@ -58,6 +58,12 @@ Querying capabilities default values for these controls being used, and a returned set of formats that may not be usable for the media the client is trying to decode. + * If the ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` flag is set the driver must enumerate + all the supported formats without taking care of codec-dependent controls + set on the ``OUTPUT`` queue. To indicate that the driver has take care of this + flag it must set ``V4L2_FMT_FLAG_ALL_FORMATS`` flag for each format while + enumerating. + 3. The client may use :c:func:`VIDIOC_ENUM_FRAMESIZES` to detect supported resolutions for a given format, passing desired pixel format in :c:type:`v4l2_frmsizeenum`'s ``pixel_format``. diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst index 3adb3d205531..15bc2f59c05a 100644 --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst @@ -234,6 +234,17 @@ the ``mbus_code`` field is handled differently: valid. The buffer consists of ``height`` lines, each having ``width`` Data Units of data and the offset (in bytes) between the beginning of each two consecutive lines is ``bytesperline``. + * - ``V4L2_FMT_FLAG_ENUM_ALL_FORMATS`` + - 0x0400 + - Set by userland applications to enumerate all possible pixel formats + without taking care of any OUTPUT or CAPTURE queue configuration. + This flag is relevant only for mem2mem devices. + * - ``V4L2_FMT_FLAG_ALL_FORMATS`` + - 0x0800 + - Set by the driver to indicated that format have been enumerated because + :ref:`V4L2_FMT_FLAG_ENUM_ALL_FORMATS <v4l2-pix-fmt-flag-set-csc>` has + been set by the userland application. + This flag is relevant only for mem2mem devices. Return Value ============ diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions index bdc628e8c1d6..7a3a1e9dc055 100644 --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions @@ -216,6 +216,8 @@ replace define V4L2_FMT_FLAG_CSC_YCBCR_ENC fmtdesc-flags replace define V4L2_FMT_FLAG_CSC_HSV_ENC fmtdesc-flags replace define V4L2_FMT_FLAG_CSC_QUANTIZATION fmtdesc-flags replace define V4L2_FMT_FLAG_META_LINE_BASED fmtdesc-flags +replace define V4L2_FMT_FLAG_ENUM_ALL_FORMATS fmtdesc-flags +replace define V4L2_FMT_FLAG_ALL_FORMATS fmtdesc-flags # V4L2 timecode types replace define V4L2_TC_TYPE_24FPS timecode-type diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index 4c76d17b4629..5785a98b6ba2 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -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; 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 */ /*
Add new flags to enumerate all pixels formats when calling VIDIOC_ENUM_FMT ioctl. When this V4L2_FMT_FLAG_ENUM_ALL_FORMATS flag is set drivers must ignore the configuration and return the hardware supported pixel formats for the specified queue. To distinguish this particular enumeration case V4L2_FMT_FLAG_ALL_FORMATS flag must be set by the drivers to highlight support of this feature to user space applications. This will permit to discover which pixel formats are supported without setting codec-specific information so userland can more easily know if the driver suits its needs well. The main target are stateless decoders so update the documentation about how to use this flag. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com> --- changes in version 4: - Explicitly document that the new flags are targeting mem2mem devices. .../userspace-api/media/v4l/dev-stateless-decoder.rst | 6 ++++++ .../userspace-api/media/v4l/vidioc-enum-fmt.rst | 11 +++++++++++ .../userspace-api/media/videodev2.h.rst.exceptions | 2 ++ drivers/media/v4l2-core/v4l2-ioctl.c | 3 +++ include/uapi/linux/videodev2.h | 2 ++ 5 files changed, 24 insertions(+)