[PATCH v7 00/12] media: rkvdec: Add H.264 High 10 and 4:2:2 profile support

[PATCH v7 00/12] media: rkvdec: Add H.264 High 10 and 4:2:2 profile support

[Sebastian Fricke]

This series add H.264 High 10 and 4:2:2 profile support to the Rockchip
Video Decoder driver.

Patch 1 add helpers for calculating plane bytesperline and sizeimage.
Patch 2 add two new pixelformats for semi-planer 10-bit 4:2:0/4:2:2 YUV.

Patch 3 change to use bytesperline and buffer height to configure strides.
Patch 4 change to use values from SPS/PPS control to configure the HW.

Patch 5-9 refactor code to support filtering of CAPUTRE formats based
on the image format returned from a get_image_fmt ops.

Patch 10 add final bits to support H.264 High 10 and 4:2:2 profiles.

Patch 11 Limit minimum profile to constrained baseline due to
unsupported features in the full baseline profile

Patch 12 add a fix for enumerated frame sizes returned to userspace.

Tested with Fluster on a ROCK Pi 4 (RK3399)

129/135 on JVT-AVC_V1
44/69 on JVT-FR-EXT

Changes in v7:
- Split out the change with the minimum profile
- s/v4l2_format_plane_width/v4l2_format_plane_stride/
- Move V4L2_PIX_FMT_NV15/V4L2_PIX_FMT_NV20 documentation as suggested
- Change return value from int to bool on rkvdec_image_fmt_match
- Add reviewed-by tags
Link to v6: https://lore.kernel.org/linux-media/20240909192522.1076704-1-jonas@kwiboo.se/

Changes in v6:
- Change to use fmt_idx instead of j++ tucked inside a condition (Dan)
- Add patch to fix enumerated frame sizes returned to userspace (Alex)
- Fluster test score is same as v4 and v5, see [4] and [5]
Link to v5: https://lore.kernel.org/linux-media/20240618194647.742037-1-jonas@kwiboo.se/

Changes in v5:
- Drop Remove SPS validation at streaming start patch
- Move buffer align from rkvdec_fill_decoded_pixfmt to min/step_width
- Use correct profiles for V4L2_CID_MPEG_VIDEO_H264_PROFILE
- Collect r-b and t-b tags
- Fluster test score is same as v4, see [4] and [5]
Link to v4: https://lore.kernel.org/linux-media/20231105165521.3592037-1-jonas@kwiboo.se/

Changes in v4:
- Fix failed v4l2-compliance tests related to CAPTURE queue
- Rework CAPTURE format filter anv validate to use an image format
- Run fluster test suite JVT-FR-EXT [4] and JVT-AVC_V1 [5]
Link to v3: https://lore.kernel.org/linux-media/20231029183427.1781554-1-jonas@kwiboo.se/

Changes in v3:
- Drop merged patches
- Use bpp and bpp_div instead of prior misuse of block_w/block_h
- New patch to use values from SPS/PPS control to configure the HW
- New patch to remove an unnecessary call to validate sps at streaming start
- Reworked pixel format validation
Link to v2: https://lore.kernel.org/linux-media/20200706215430.22859-1-jonas@kwiboo.se/

Changes in v2:
- Collect r-b tags
- SPS pic width and height in mbs validation moved to rkvdec_try_ctrl
- New patch to not override output buffer sizeimage
- Reworked pixel format validation
- Only align decoded buffer instead of changing frmsize step_width
Link to v1: https://lore.kernel.org/linux-media/20200701215616.30874-1-jonas@kwiboo.se/

To fully runtime test this series you may need FFmpeg patches from [1]
and fluster patches from [2], this series is also available at [3].

[1] https://github.com/Kwiboo/FFmpeg/commits/v4l2request-2024-v2-rkvdec/
[2] https://github.com/Kwiboo/fluster/commits/ffmpeg-v4l2request-rkvdec/
[3] https://github.com/Kwiboo/linux-rockchip/commits/linuxtv-rkvdec-high-10-v6/
[4] https://gist.github.com/Kwiboo/f4ac15576b2c72887ae2bc5d58b5c865
[5] https://gist.github.com/Kwiboo/459a1c8f1dcb56e45dc7a7a29cc28adf

Regards,
Jonas

Alex Bee (1):
  media: rkvdec: h264: Don't hardcode SPS/PPS parameters

Jonas Karlman (10):
  media: v4l2-common: Add helpers to calculate bytesperline and
    sizeimage
  media: v4l2: Add NV15 and NV20 pixel formats
  media: rkvdec: h264: Use bytesperline and buffer height as virstride
  media: rkvdec: Extract rkvdec_fill_decoded_pixfmt into helper
  media: rkvdec: Move rkvdec_reset_decoded_fmt helper
  media: rkvdec: Extract decoded format enumeration into helper
  media: rkvdec: Add image format concept
  media: rkvdec: Add get_image_fmt ops
  media: rkvdec: h264: Support High 10 and 4:2:2 profiles
  media: rkvdec: Fix enumerate frame sizes

 .../media/v4l/pixfmt-yuv-planar.rst           | 128 ++++++++++
 drivers/media/v4l2-core/v4l2-common.c         |  80 +++---
 drivers/media/v4l2-core/v4l2-ioctl.c          |   2 +
 drivers/staging/media/rkvdec/rkvdec-h264.c    |  64 +++--
 drivers/staging/media/rkvdec/rkvdec.c         | 239 +++++++++++++-----
 drivers/staging/media/rkvdec/rkvdec.h         |  18 +-
 include/uapi/linux/videodev2.h                |   2 +
 7 files changed, 410 insertions(+), 123 deletions(-)

--
2.46.0

---
Alex Bee (1):
      media: rkvdec: h264: Don't hardcode SPS/PPS parameters

Jonas Karlman (10):
      media: v4l2-common: Add helpers to calculate bytesperline and sizeimage
      media: v4l2: Add NV15 and NV20 pixel formats
      media: rkvdec: h264: Use bytesperline and buffer height as virstride
      media: rkvdec: Extract rkvdec_fill_decoded_pixfmt into helper
      media: rkvdec: Move rkvdec_reset_decoded_fmt helper
      media: rkvdec: Extract decoded format enumeration into helper
      media: rkvdec: Add image format concept
      media: rkvdec: Add get_image_fmt ops
      media: rkvdec: h264: Support High 10 and 4:2:2 profiles
      media: rkvdec: Fix frame size enumeration

Sebastian Fricke (1):
      media: rkvdec: h264: Limit minimum profile to constrained baseline

 .../userspace-api/media/v4l/pixfmt-yuv-planar.rst  | 128 +++++++++++
 drivers/media/v4l2-core/v4l2-common.c              |  80 +++----
 drivers/media/v4l2-core/v4l2-ioctl.c               |   2 +
 drivers/staging/media/rkvdec/rkvdec-h264.c         |  64 ++++--
 drivers/staging/media/rkvdec/rkvdec.c              | 239 +++++++++++++++------
 drivers/staging/media/rkvdec/rkvdec.h              |  18 +-
 include/uapi/linux/videodev2.h                     |   2 +
 7 files changed, 410 insertions(+), 123 deletions(-)
---
base-commit: c2b96a6818159fba8a3bcc38262da9e77f9b3ec7
change-id: 20250116-rkvdec_h264_high10_and_422_support-a8f2da8a09e6

Best regards,
-- 
Sebastian Fricke <sebastian.fricke@collabora.com>

[PATCH v7 01/12] media: v4l2-common: Add helpers to calculate bytesperline and sizeimage

[Sebastian Fricke]

From: Jonas Karlman <jonas@kwiboo.se>

Add helper functions to calculate plane bytesperline and sizeimage,
these new helpers consider bpp div, block width and height when
calculating plane bytesperline and sizeimage.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Tested-by: Christopher Obbard <chris.obbard@collabora.com>
---
 drivers/media/v4l2-core/v4l2-common.c | 78 +++++++++++++++++------------------
 1 file changed, 39 insertions(+), 39 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index e4b2de3833ee3d440493f94fca5ee2049b013ef0..fdab16b9d481300eecf2202b3930fdf0a97a3023 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -357,6 +357,34 @@ static inline unsigned int v4l2_format_block_height(const struct v4l2_format_inf
 	return info->block_h[plane];
 }
 
+static inline unsigned int v4l2_format_plane_stride(const struct v4l2_format_info *info, int plane,
+						   unsigned int width)
+{
+	unsigned int hdiv = plane ? info->hdiv : 1;
+	unsigned int aligned_width =
+		ALIGN(width, v4l2_format_block_width(info, plane));
+
+	return DIV_ROUND_UP(aligned_width, hdiv) *
+	       info->bpp[plane] / info->bpp_div[plane];
+}
+
+static inline unsigned int v4l2_format_plane_height(const struct v4l2_format_info *info, int plane,
+						    unsigned int height)
+{
+	unsigned int vdiv = plane ? info->vdiv : 1;
+	unsigned int aligned_height =
+		ALIGN(height, v4l2_format_block_height(info, plane));
+
+	return DIV_ROUND_UP(aligned_height, vdiv);
+}
+
+static inline unsigned int v4l2_format_plane_size(const struct v4l2_format_info *info, int plane,
+						  unsigned int width, unsigned int height)
+{
+	return v4l2_format_plane_stride(info, plane, width) *
+	       v4l2_format_plane_height(info, plane, height);
+}
+
 void v4l2_apply_frmsize_constraints(u32 *width, u32 *height,
 				    const struct v4l2_frmsize_stepwise *frmsize)
 {
@@ -392,37 +420,19 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 
 	if (info->mem_planes == 1) {
 		plane = &pixfmt->plane_fmt[0];
-		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0] / info->bpp_div[0];
+		plane->bytesperline = v4l2_format_plane_stride(info, 0, width);
 		plane->sizeimage = 0;
 
-		for (i = 0; i < info->comp_planes; i++) {
-			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
-			unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
-			unsigned int aligned_width;
-			unsigned int aligned_height;
-
-			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
-			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
-
-			plane->sizeimage += info->bpp[i] *
-				DIV_ROUND_UP(aligned_width, hdiv) *
-				DIV_ROUND_UP(aligned_height, vdiv) / info->bpp_div[i];
-		}
+		for (i = 0; i < info->comp_planes; i++)
+			plane->sizeimage +=
+				v4l2_format_plane_size(info, i, width, height);
 	} else {
 		for (i = 0; i < info->comp_planes; i++) {
-			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
-			unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
-			unsigned int aligned_width;
-			unsigned int aligned_height;
-
-			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
-			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
-
 			plane = &pixfmt->plane_fmt[i];
 			plane->bytesperline =
-				info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv) / info->bpp_div[i];
-			plane->sizeimage =
-				plane->bytesperline * DIV_ROUND_UP(aligned_height, vdiv);
+				v4l2_format_plane_stride(info, i, width);
+			plane->sizeimage = plane->bytesperline *
+				v4l2_format_plane_height(info, i, height);
 		}
 	}
 	return 0;
@@ -446,22 +456,12 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
 	pixfmt->width = width;
 	pixfmt->height = height;
 	pixfmt->pixelformat = pixelformat;
-	pixfmt->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0] / info->bpp_div[0];
+	pixfmt->bytesperline = v4l2_format_plane_stride(info, 0, width);
 	pixfmt->sizeimage = 0;
 
-	for (i = 0; i < info->comp_planes; i++) {
-		unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
-		unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
-		unsigned int aligned_width;
-		unsigned int aligned_height;
-
-		aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
-		aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
-
-		pixfmt->sizeimage += info->bpp[i] *
-			DIV_ROUND_UP(aligned_width, hdiv) *
-			DIV_ROUND_UP(aligned_height, vdiv) / info->bpp_div[i];
-	}
+	for (i = 0; i < info->comp_planes; i++)
+		pixfmt->sizeimage +=
+			v4l2_format_plane_size(info, i, width, height);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);

-- 
2.25.1

[PATCH v7 02/12] media: v4l2: Add NV15 and NV20 pixel formats

[Sebastian Fricke]

From: Jonas Karlman <jonas@kwiboo.se>

Add NV15 and NV20 pixel formats used by the Rockchip Video Decoder for
10-bit buffers.

NV15 and NV20 is 10-bit 4:2:0/4:2:2 semi-planar YUV formats similar to
NV12 and NV16, using 10-bit components with no padding between each
component. Instead, a group of 4 luminance/chrominance samples are
stored over 5 bytes in little endian order:

YYYY = UVUV = 4 * 10 bits = 40 bits = 5 bytes

The '15' and '20' suffix refers to the optimum effective bits per pixel
which is achieved when the total number of luminance samples is a
multiple of 8 for NV15 and 4 for NV20.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Tested-by: Christopher Obbard <chris.obbard@collabora.com>
---
 .../userspace-api/media/v4l/pixfmt-yuv-planar.rst  | 128 +++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-common.c              |   2 +
 drivers/media/v4l2-core/v4l2-ioctl.c               |   2 +
 include/uapi/linux/videodev2.h                     |   2 +
 4 files changed, 134 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst b/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
index b788f693385541e49216fb576bf10badcefc5e49..6e4f399f1f88106a2df34bc808a227fd8744fc5e 100644
--- a/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
+++ b/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
@@ -137,6 +137,13 @@ All components are stored with the same number of bits per component.
       - Cb, Cr
       - No
       - Linear
+    * - V4L2_PIX_FMT_NV15
+      - 'NV15'
+      - 10
+      - 4:2:0
+      - Cb, Cr
+      - Yes
+      - Linear
     * - V4L2_PIX_FMT_NV15_4L4
       - 'VT15'
       - 15
@@ -186,6 +193,13 @@ All components are stored with the same number of bits per component.
       - Cr, Cb
       - No
       - Linear
+    * - V4L2_PIX_FMT_NV20
+      - 'NV20'
+      - 10
+      - 4:2:2
+      - Cb, Cr
+      - Yes
+      - Linear
     * - V4L2_PIX_FMT_NV24
       - 'NV24'
       - 8
@@ -302,6 +316,57 @@ of the luma plane.
       - Cr\ :sub:`11`
 
 
+.. _V4L2-PIX-FMT-NV15:
+
+NV15
+----
+
+Semi-planar 10-bit YUV 4:2:0 format similar to NV12, using 10-bit components
+with no padding between each component. A group of 4 components are stored over
+5 bytes in little endian order.
+
+.. flat-table:: Sample 4x4 NV15 Image (1 byte per cell)
+    :header-rows:  0
+    :stub-columns: 0
+
+    * - start + 0:
+      - Y'\ :sub:`00[7:0]`
+      - Y'\ :sub:`01[5:0]`\ Y'\ :sub:`00[9:8]`
+      - Y'\ :sub:`02[3:0]`\ Y'\ :sub:`01[9:6]`
+      - Y'\ :sub:`03[1:0]`\ Y'\ :sub:`02[9:4]`
+      - Y'\ :sub:`03[9:2]`
+    * - start + 5:
+      - Y'\ :sub:`10[7:0]`
+      - Y'\ :sub:`11[5:0]`\ Y'\ :sub:`10[9:8]`
+      - Y'\ :sub:`12[3:0]`\ Y'\ :sub:`11[9:6]`
+      - Y'\ :sub:`13[1:0]`\ Y'\ :sub:`12[9:4]`
+      - Y'\ :sub:`13[9:2]`
+    * - start + 10:
+      - Y'\ :sub:`20[7:0]`
+      - Y'\ :sub:`21[5:0]`\ Y'\ :sub:`20[9:8]`
+      - Y'\ :sub:`22[3:0]`\ Y'\ :sub:`21[9:6]`
+      - Y'\ :sub:`23[1:0]`\ Y'\ :sub:`22[9:4]`
+      - Y'\ :sub:`23[9:2]`
+    * - start + 15:
+      - Y'\ :sub:`30[7:0]`
+      - Y'\ :sub:`31[5:0]`\ Y'\ :sub:`30[9:8]`
+      - Y'\ :sub:`32[3:0]`\ Y'\ :sub:`31[9:6]`
+      - Y'\ :sub:`33[1:0]`\ Y'\ :sub:`32[9:4]`
+      - Y'\ :sub:`33[9:2]`
+    * - start + 20:
+      - Cb\ :sub:`00[7:0]`
+      - Cr\ :sub:`00[5:0]`\ Cb\ :sub:`00[9:8]`
+      - Cb\ :sub:`01[3:0]`\ Cr\ :sub:`00[9:6]`
+      - Cr\ :sub:`01[1:0]`\ Cb\ :sub:`01[9:4]`
+      - Cr\ :sub:`01[9:2]`
+    * - start + 25:
+      - Cb\ :sub:`10[7:0]`
+      - Cr\ :sub:`10[5:0]`\ Cb\ :sub:`10[9:8]`
+      - Cb\ :sub:`11[3:0]`\ Cr\ :sub:`10[9:6]`
+      - Cr\ :sub:`11[1:0]`\ Cb\ :sub:`11[9:4]`
+      - Cr\ :sub:`11[9:2]`
+
+
 .. _V4L2-PIX-FMT-NV12MT:
 .. _V4L2-PIX-FMT-NV12MT-16X16:
 .. _V4L2-PIX-FMT-NV12-4L4:
@@ -631,6 +696,69 @@ number of lines as the luma plane.
       - Cr\ :sub:`32`
 
 
+.. _V4L2-PIX-FMT-NV20:
+
+NV20
+----
+
+Semi-planar 10-bit YUV 4:2:2 format similar to NV16, using 10-bit components
+with no padding between each component. A group of 4 components are stored over
+5 bytes in little endian order.
+
+.. flat-table:: Sample 4x4 NV20 Image (1 byte per cell)
+    :header-rows:  0
+    :stub-columns: 0
+
+    * - start + 0:
+      - Y'\ :sub:`00[7:0]`
+      - Y'\ :sub:`01[5:0]`\ Y'\ :sub:`00[9:8]`
+      - Y'\ :sub:`02[3:0]`\ Y'\ :sub:`01[9:6]`
+      - Y'\ :sub:`03[1:0]`\ Y'\ :sub:`02[9:4]`
+      - Y'\ :sub:`03[9:2]`
+    * - start + 5:
+      - Y'\ :sub:`10[7:0]`
+      - Y'\ :sub:`11[5:0]`\ Y'\ :sub:`10[9:8]`
+      - Y'\ :sub:`12[3:0]`\ Y'\ :sub:`11[9:6]`
+      - Y'\ :sub:`13[1:0]`\ Y'\ :sub:`12[9:4]`
+      - Y'\ :sub:`13[9:2]`
+    * - start + 10:
+      - Y'\ :sub:`20[7:0]`
+      - Y'\ :sub:`21[5:0]`\ Y'\ :sub:`20[9:8]`
+      - Y'\ :sub:`22[3:0]`\ Y'\ :sub:`21[9:6]`
+      - Y'\ :sub:`23[1:0]`\ Y'\ :sub:`22[9:4]`
+      - Y'\ :sub:`23[9:2]`
+    * - start + 15:
+      - Y'\ :sub:`30[7:0]`
+      - Y'\ :sub:`31[5:0]`\ Y'\ :sub:`30[9:8]`
+      - Y'\ :sub:`32[3:0]`\ Y'\ :sub:`31[9:6]`
+      - Y'\ :sub:`33[1:0]`\ Y'\ :sub:`32[9:4]`
+      - Y'\ :sub:`33[9:2]`
+    * - start + 20:
+      - Cb\ :sub:`00[7:0]`
+      - Cr\ :sub:`00[5:0]`\ Cb\ :sub:`00[9:8]`
+      - Cb\ :sub:`01[3:0]`\ Cr\ :sub:`00[9:6]`
+      - Cr\ :sub:`01[1:0]`\ Cb\ :sub:`01[9:4]`
+      - Cr\ :sub:`01[9:2]`
+    * - start + 25:
+      - Cb\ :sub:`10[7:0]`
+      - Cr\ :sub:`10[5:0]`\ Cb\ :sub:`10[9:8]`
+      - Cb\ :sub:`11[3:0]`\ Cr\ :sub:`10[9:6]`
+      - Cr\ :sub:`11[1:0]`\ Cb\ :sub:`11[9:4]`
+      - Cr\ :sub:`11[9:2]`
+    * - start + 30:
+      - Cb\ :sub:`20[7:0]`
+      - Cr\ :sub:`20[5:0]`\ Cb\ :sub:`20[9:8]`
+      - Cb\ :sub:`21[3:0]`\ Cr\ :sub:`20[9:6]`
+      - Cr\ :sub:`21[1:0]`\ Cb\ :sub:`21[9:4]`
+      - Cr\ :sub:`21[9:2]`
+    * - start + 35:
+      - Cb\ :sub:`30[7:0]`
+      - Cr\ :sub:`30[5:0]`\ Cb\ :sub:`30[9:8]`
+      - Cb\ :sub:`31[3:0]`\ Cr\ :sub:`30[9:6]`
+      - Cr\ :sub:`31[1:0]`\ Cb\ :sub:`31[9:4]`
+      - Cr\ :sub:`31[9:2]`
+
+
 .. _V4L2-PIX-FMT-NV24:
 .. _V4L2-PIX-FMT-NV42:
 
diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index fdab16b9d481300eecf2202b3930fdf0a97a3023..07a999f75755bf4d1aca6c3726242b6cb35b70e8 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -277,8 +277,10 @@ const struct v4l2_format_info *v4l2_format_info(u32 format)
 		/* YUV planar formats */
 		{ .format = V4L2_PIX_FMT_NV12,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 2, .vdiv = 2 },
 		{ .format = V4L2_PIX_FMT_NV21,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 2, .vdiv = 2 },
+		{ .format = V4L2_PIX_FMT_NV15,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 10, 0, 0 }, .bpp_div = { 4, 4, 1, 1 }, .hdiv = 2, .vdiv = 2 },
 		{ .format = V4L2_PIX_FMT_NV16,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 2, .vdiv = 1 },
 		{ .format = V4L2_PIX_FMT_NV61,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 2, .vdiv = 1 },
+		{ .format = V4L2_PIX_FMT_NV20,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 10, 0, 0 }, .bpp_div = { 4, 4, 1, 1 }, .hdiv = 2, .vdiv = 1 },
 		{ .format = V4L2_PIX_FMT_NV24,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 1, .vdiv = 1 },
 		{ .format = V4L2_PIX_FMT_NV42,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 1, .vdiv = 1 },
 		{ .format = V4L2_PIX_FMT_P010,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 2, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 2, .vdiv = 1 },
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 0304daa8471d1ee7808a418232384fb8d2264dea..864cd2939222e717e9a5d5d96cbf7402debadd9b 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1361,8 +1361,10 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
 	case V4L2_PIX_FMT_YUV48_12:	descr = "12-bit YUV 4:4:4 Packed"; break;
 	case V4L2_PIX_FMT_NV12:		descr = "Y/UV 4:2:0"; break;
 	case V4L2_PIX_FMT_NV21:		descr = "Y/VU 4:2:0"; break;
+	case V4L2_PIX_FMT_NV15:		descr = "10-bit Y/UV 4:2:0 (Packed)"; break;
 	case V4L2_PIX_FMT_NV16:		descr = "Y/UV 4:2:2"; break;
 	case V4L2_PIX_FMT_NV61:		descr = "Y/VU 4:2:2"; break;
+	case V4L2_PIX_FMT_NV20:		descr = "10-bit Y/UV 4:2:2 (Packed)"; break;
 	case V4L2_PIX_FMT_NV24:		descr = "Y/UV 4:4:4"; break;
 	case V4L2_PIX_FMT_NV42:		descr = "Y/VU 4:4:4"; break;
 	case V4L2_PIX_FMT_P010:		descr = "10-bit Y/UV 4:2:0"; break;
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index e7c4dce390074b8b69593f1cd252284616f1b9db..700e7033b88e6c22116a3f7101f4a9f662e0fb99 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -643,8 +643,10 @@ struct v4l2_pix_format {
 /* two planes -- one Y, one Cr + Cb interleaved  */
 #define V4L2_PIX_FMT_NV12    v4l2_fourcc('N', 'V', '1', '2') /* 12  Y/CbCr 4:2:0  */
 #define V4L2_PIX_FMT_NV21    v4l2_fourcc('N', 'V', '2', '1') /* 12  Y/CrCb 4:2:0  */
+#define V4L2_PIX_FMT_NV15    v4l2_fourcc('N', 'V', '1', '5') /* 15  Y/CbCr 4:2:0 10-bit packed */
 #define V4L2_PIX_FMT_NV16    v4l2_fourcc('N', 'V', '1', '6') /* 16  Y/CbCr 4:2:2  */
 #define V4L2_PIX_FMT_NV61    v4l2_fourcc('N', 'V', '6', '1') /* 16  Y/CrCb 4:2:2  */
+#define V4L2_PIX_FMT_NV20    v4l2_fourcc('N', 'V', '2', '0') /* 20  Y/CbCr 4:2:2 10-bit packed */
 #define V4L2_PIX_FMT_NV24    v4l2_fourcc('N', 'V', '2', '4') /* 24  Y/CbCr 4:4:4  */
 #define V4L2_PIX_FMT_NV42    v4l2_fourcc('N', 'V', '4', '2') /* 24  Y/CrCb 4:4:4  */
 #define V4L2_PIX_FMT_P010    v4l2_fourcc('P', '0', '1', '0') /* 24  Y/CbCr 4:2:0 10-bit per component */

-- 
2.25.1

[PATCH v7 03/12] media: rkvdec: h264: Use bytesperline and buffer height as virstride

[Sebastian Fricke]

From: Jonas Karlman <jonas@kwiboo.se>

Use bytesperline and buffer height to calculate the strides configured.

This does not really change anything other than ensuring the
bytesperline that is signaled to userspace matches what is configured
in HW.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Tested-by: Christopher Obbard <chris.obbard@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 4fc167b42cf0c74fcc7adad5b867d9d7cd8af29b..7a1e76d423df55bd01c1ff26ff2c6d1b81ee7169 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -896,9 +896,9 @@ static void config_registers(struct rkvdec_ctx *ctx,
 	dma_addr_t rlc_addr;
 	dma_addr_t refer_addr;
 	u32 rlc_len;
-	u32 hor_virstride = 0;
-	u32 ver_virstride = 0;
-	u32 y_virstride = 0;
+	u32 hor_virstride;
+	u32 ver_virstride;
+	u32 y_virstride;
 	u32 yuv_virstride = 0;
 	u32 offset;
 	dma_addr_t dst_addr;
@@ -909,16 +909,16 @@ static void config_registers(struct rkvdec_ctx *ctx,
 
 	f = &ctx->decoded_fmt;
 	dst_fmt = &f->fmt.pix_mp;
-	hor_virstride = (sps->bit_depth_luma_minus8 + 8) * dst_fmt->width / 8;
-	ver_virstride = round_up(dst_fmt->height, 16);
+	hor_virstride = dst_fmt->plane_fmt[0].bytesperline;
+	ver_virstride = dst_fmt->height;
 	y_virstride = hor_virstride * ver_virstride;
 
 	if (sps->chroma_format_idc == 0)
 		yuv_virstride = y_virstride;
 	else if (sps->chroma_format_idc == 1)
-		yuv_virstride += y_virstride + y_virstride / 2;
+		yuv_virstride = y_virstride + y_virstride / 2;
 	else if (sps->chroma_format_idc == 2)
-		yuv_virstride += 2 * y_virstride;
+		yuv_virstride = 2 * y_virstride;
 
 	reg = RKVDEC_Y_HOR_VIRSTRIDE(hor_virstride / 16) |
 	      RKVDEC_UV_HOR_VIRSTRIDE(hor_virstride / 16) |

-- 
2.25.1

[PATCH v7 04/12] media: rkvdec: h264: Don't hardcode SPS/PPS parameters

[Sebastian Fricke]

From: Alex Bee <knaerzche@gmail.com>

Some SPS/PPS parameters are currently hardcoded in the driver even
though they exist in the stable uapi controls.

Use values from SPS/PPS controls instead of hardcoding them.

Signed-off-by: Alex Bee <knaerzche@gmail.com>
[jonas@kwiboo.se: constraint_set_flags condition, commit message]
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Tested-by: Christopher Obbard <chris.obbard@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 7a1e76d423df55bd01c1ff26ff2c6d1b81ee7169..8bce8902b8dda39bb2058c8504bd52ccae6b4204 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -655,13 +655,14 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
 
 #define WRITE_PPS(value, field) set_ps_field(hw_ps->info, field, value)
 	/* write sps */
-	WRITE_PPS(0xf, SEQ_PARAMETER_SET_ID);
-	WRITE_PPS(0xff, PROFILE_IDC);
-	WRITE_PPS(1, CONSTRAINT_SET3_FLAG);
+	WRITE_PPS(sps->seq_parameter_set_id, SEQ_PARAMETER_SET_ID);
+	WRITE_PPS(sps->profile_idc, PROFILE_IDC);
+	WRITE_PPS(!!(sps->constraint_set_flags & (1 << 3)), CONSTRAINT_SET3_FLAG);
 	WRITE_PPS(sps->chroma_format_idc, CHROMA_FORMAT_IDC);
 	WRITE_PPS(sps->bit_depth_luma_minus8, BIT_DEPTH_LUMA);
 	WRITE_PPS(sps->bit_depth_chroma_minus8, BIT_DEPTH_CHROMA);
-	WRITE_PPS(0, QPPRIME_Y_ZERO_TRANSFORM_BYPASS_FLAG);
+	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_QPPRIME_Y_ZERO_TRANSFORM_BYPASS),
+		  QPPRIME_Y_ZERO_TRANSFORM_BYPASS_FLAG);
 	WRITE_PPS(sps->log2_max_frame_num_minus4, LOG2_MAX_FRAME_NUM_MINUS4);
 	WRITE_PPS(sps->max_num_ref_frames, MAX_NUM_REF_FRAMES);
 	WRITE_PPS(sps->pic_order_cnt_type, PIC_ORDER_CNT_TYPE);
@@ -688,8 +689,8 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
 		  DIRECT_8X8_INFERENCE_FLAG);
 
 	/* write pps */
-	WRITE_PPS(0xff, PIC_PARAMETER_SET_ID);
-	WRITE_PPS(0x1f, PPS_SEQ_PARAMETER_SET_ID);
+	WRITE_PPS(pps->pic_parameter_set_id, PIC_PARAMETER_SET_ID);
+	WRITE_PPS(pps->seq_parameter_set_id, PPS_SEQ_PARAMETER_SET_ID);
 	WRITE_PPS(!!(pps->flags & V4L2_H264_PPS_FLAG_ENTROPY_CODING_MODE),
 		  ENTROPY_CODING_MODE_FLAG);
 	WRITE_PPS(!!(pps->flags & V4L2_H264_PPS_FLAG_BOTTOM_FIELD_PIC_ORDER_IN_FRAME_PRESENT),

-- 
2.25.1

[PATCH v7 05/12] media: rkvdec: Extract rkvdec_fill_decoded_pixfmt into helper

[Sebastian Fricke]

From: Jonas Karlman <jonas@kwiboo.se>

Extract call to v4l2_fill_pixfmt_mp() and ajusting of sizeimage into a
helper. Replace current code with a call to the new helper.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Tested-by: Christopher Obbard <chris.obbard@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index f9bef5173bf25c0cd6b09d2b253d36aa78d7b0c4..e354360f4acc1fe9b7c4b90745f0a818fa2b7cab 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -27,6 +27,16 @@
 #include "rkvdec.h"
 #include "rkvdec-regs.h"
 
+static void rkvdec_fill_decoded_pixfmt(struct rkvdec_ctx *ctx,
+				       struct v4l2_pix_format_mplane *pix_mp)
+{
+	v4l2_fill_pixfmt_mp(pix_mp, pix_mp->pixelformat,
+			    pix_mp->width, pix_mp->height);
+	pix_mp->plane_fmt[0].sizeimage += 128 *
+		DIV_ROUND_UP(pix_mp->width, 16) *
+		DIV_ROUND_UP(pix_mp->height, 16);
+}
+
 static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
@@ -192,13 +202,9 @@ static void rkvdec_reset_decoded_fmt(struct rkvdec_ctx *ctx)
 
 	rkvdec_reset_fmt(ctx, f, ctx->coded_fmt_desc->decoded_fmts[0]);
 	f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
-	v4l2_fill_pixfmt_mp(&f->fmt.pix_mp,
-			    ctx->coded_fmt_desc->decoded_fmts[0],
-			    ctx->coded_fmt.fmt.pix_mp.width,
-			    ctx->coded_fmt.fmt.pix_mp.height);
-	f->fmt.pix_mp.plane_fmt[0].sizeimage += 128 *
-		DIV_ROUND_UP(f->fmt.pix_mp.width, 16) *
-		DIV_ROUND_UP(f->fmt.pix_mp.height, 16);
+	f->fmt.pix_mp.width = ctx->coded_fmt.fmt.pix_mp.width;
+	f->fmt.pix_mp.height = ctx->coded_fmt.fmt.pix_mp.height;
+	rkvdec_fill_decoded_pixfmt(ctx, &f->fmt.pix_mp);
 }
 
 static int rkvdec_enum_framesizes(struct file *file, void *priv,
@@ -264,12 +270,7 @@ static int rkvdec_try_capture_fmt(struct file *file, void *priv,
 				       &pix_mp->height,
 				       &coded_desc->frmsize);
 
-	v4l2_fill_pixfmt_mp(pix_mp, pix_mp->pixelformat,
-			    pix_mp->width, pix_mp->height);
-	pix_mp->plane_fmt[0].sizeimage +=
-		128 *
-		DIV_ROUND_UP(pix_mp->width, 16) *
-		DIV_ROUND_UP(pix_mp->height, 16);
+	rkvdec_fill_decoded_pixfmt(ctx, pix_mp);
 	pix_mp->field = V4L2_FIELD_NONE;
 
 	return 0;

-- 
2.25.1

[PATCH v7 06/12] media: rkvdec: Move rkvdec_reset_decoded_fmt helper

[Sebastian Fricke]

From: Jonas Karlman <jonas@kwiboo.se>

Move rkvdec_reset_decoded_fmt() and the called rkvdec_reset_fmt() helper
functions in preparation for adding a new caller in an upcoming patch.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Tested-by: Christopher Obbard <chris.obbard@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec.c | 46 +++++++++++++++++------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index e354360f4acc1fe9b7c4b90745f0a818fa2b7cab..1f8f98cf91dc205f4a9da2712c81e90b726a6e57 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -37,6 +37,29 @@ static void rkvdec_fill_decoded_pixfmt(struct rkvdec_ctx *ctx,
 		DIV_ROUND_UP(pix_mp->height, 16);
 }
 
+static void rkvdec_reset_fmt(struct rkvdec_ctx *ctx, struct v4l2_format *f,
+			     u32 fourcc)
+{
+	memset(f, 0, sizeof(*f));
+	f->fmt.pix_mp.pixelformat = fourcc;
+	f->fmt.pix_mp.field = V4L2_FIELD_NONE;
+	f->fmt.pix_mp.colorspace = V4L2_COLORSPACE_REC709;
+	f->fmt.pix_mp.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
+	f->fmt.pix_mp.quantization = V4L2_QUANTIZATION_DEFAULT;
+	f->fmt.pix_mp.xfer_func = V4L2_XFER_FUNC_DEFAULT;
+}
+
+static void rkvdec_reset_decoded_fmt(struct rkvdec_ctx *ctx)
+{
+	struct v4l2_format *f = &ctx->decoded_fmt;
+
+	rkvdec_reset_fmt(ctx, f, ctx->coded_fmt_desc->decoded_fmts[0]);
+	f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+	f->fmt.pix_mp.width = ctx->coded_fmt.fmt.pix_mp.width;
+	f->fmt.pix_mp.height = ctx->coded_fmt.fmt.pix_mp.height;
+	rkvdec_fill_decoded_pixfmt(ctx, &f->fmt.pix_mp);
+}
+
 static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
@@ -169,18 +192,6 @@ rkvdec_find_coded_fmt_desc(u32 fourcc)
 	return NULL;
 }
 
-static void rkvdec_reset_fmt(struct rkvdec_ctx *ctx, struct v4l2_format *f,
-			     u32 fourcc)
-{
-	memset(f, 0, sizeof(*f));
-	f->fmt.pix_mp.pixelformat = fourcc;
-	f->fmt.pix_mp.field = V4L2_FIELD_NONE;
-	f->fmt.pix_mp.colorspace = V4L2_COLORSPACE_REC709;
-	f->fmt.pix_mp.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
-	f->fmt.pix_mp.quantization = V4L2_QUANTIZATION_DEFAULT;
-	f->fmt.pix_mp.xfer_func = V4L2_XFER_FUNC_DEFAULT;
-}
-
 static void rkvdec_reset_coded_fmt(struct rkvdec_ctx *ctx)
 {
 	struct v4l2_format *f = &ctx->coded_fmt;
@@ -196,17 +207,6 @@ static void rkvdec_reset_coded_fmt(struct rkvdec_ctx *ctx)
 		ctx->coded_fmt_desc->ops->adjust_fmt(ctx, f);
 }
 
-static void rkvdec_reset_decoded_fmt(struct rkvdec_ctx *ctx)
-{
-	struct v4l2_format *f = &ctx->decoded_fmt;
-
-	rkvdec_reset_fmt(ctx, f, ctx->coded_fmt_desc->decoded_fmts[0]);
-	f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
-	f->fmt.pix_mp.width = ctx->coded_fmt.fmt.pix_mp.width;
-	f->fmt.pix_mp.height = ctx->coded_fmt.fmt.pix_mp.height;
-	rkvdec_fill_decoded_pixfmt(ctx, &f->fmt.pix_mp);
-}
-
 static int rkvdec_enum_framesizes(struct file *file, void *priv,
 				  struct v4l2_frmsizeenum *fsize)
 {

-- 
2.25.1

[PATCH v7 07/12] media: rkvdec: Extract decoded format enumeration into helper

[Sebastian Fricke]

From: Jonas Karlman <jonas@kwiboo.se>

Add a rkvdec_is_valid_fmt() helper that check if a fourcc is a supported
CAPTURE format, and a rkvdec_enum_decoded_fmt() helper that enumerates
valid formats.

This moves current code into helper functions in preparation for adding
CAPTURE format filtering and validation in next patch.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Tested-by: Christopher Obbard <chris.obbard@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec.c | 49 +++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 1f8f98cf91dc205f4a9da2712c81e90b726a6e57..52e64b399dcc4a0c028cac908c0b1ceb49956c5f 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -27,6 +27,32 @@
 #include "rkvdec.h"
 #include "rkvdec-regs.h"
 
+static u32 rkvdec_enum_decoded_fmt(struct rkvdec_ctx *ctx, int index)
+{
+	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
+
+	if (WARN_ON(!desc))
+		return 0;
+
+	if (index >= desc->num_decoded_fmts)
+		return 0;
+
+	return desc->decoded_fmts[index];
+}
+
+static bool rkvdec_is_valid_fmt(struct rkvdec_ctx *ctx, u32 fourcc)
+{
+	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
+	unsigned int i;
+
+	for (i = 0; i < desc->num_decoded_fmts; i++) {
+		if (desc->decoded_fmts[i] == fourcc)
+			return true;
+	}
+
+	return false;
+}
+
 static void rkvdec_fill_decoded_pixfmt(struct rkvdec_ctx *ctx,
 				       struct v4l2_pix_format_mplane *pix_mp)
 {
@@ -52,8 +78,10 @@ static void rkvdec_reset_fmt(struct rkvdec_ctx *ctx, struct v4l2_format *f,
 static void rkvdec_reset_decoded_fmt(struct rkvdec_ctx *ctx)
 {
 	struct v4l2_format *f = &ctx->decoded_fmt;
+	u32 fourcc;
 
-	rkvdec_reset_fmt(ctx, f, ctx->coded_fmt_desc->decoded_fmts[0]);
+	fourcc = rkvdec_enum_decoded_fmt(ctx, 0);
+	rkvdec_reset_fmt(ctx, f, fourcc);
 	f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
 	f->fmt.pix_mp.width = ctx->coded_fmt.fmt.pix_mp.width;
 	f->fmt.pix_mp.height = ctx->coded_fmt.fmt.pix_mp.height;
@@ -244,7 +272,6 @@ static int rkvdec_try_capture_fmt(struct file *file, void *priv,
 	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
 	struct rkvdec_ctx *ctx = fh_to_rkvdec_ctx(priv);
 	const struct rkvdec_coded_fmt_desc *coded_desc;
-	unsigned int i;
 
 	/*
 	 * The codec context should point to a coded format desc, if the format
@@ -255,13 +282,8 @@ static int rkvdec_try_capture_fmt(struct file *file, void *priv,
 	if (WARN_ON(!coded_desc))
 		return -EINVAL;
 
-	for (i = 0; i < coded_desc->num_decoded_fmts; i++) {
-		if (coded_desc->decoded_fmts[i] == pix_mp->pixelformat)
-			break;
-	}
-
-	if (i == coded_desc->num_decoded_fmts)
-		pix_mp->pixelformat = coded_desc->decoded_fmts[0];
+	if (!rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat))
+		pix_mp->pixelformat = rkvdec_enum_decoded_fmt(ctx, 0);
 
 	/* Always apply the frmsize constraint of the coded end. */
 	pix_mp->width = max(pix_mp->width, ctx->coded_fmt.fmt.pix_mp.width);
@@ -425,14 +447,13 @@ static int rkvdec_enum_capture_fmt(struct file *file, void *priv,
 				   struct v4l2_fmtdesc *f)
 {
 	struct rkvdec_ctx *ctx = fh_to_rkvdec_ctx(priv);
+	u32 fourcc;
 
-	if (WARN_ON(!ctx->coded_fmt_desc))
-		return -EINVAL;
-
-	if (f->index >= ctx->coded_fmt_desc->num_decoded_fmts)
+	fourcc = rkvdec_enum_decoded_fmt(ctx, f->index);
+	if (!fourcc)
 		return -EINVAL;
 
-	f->pixelformat = ctx->coded_fmt_desc->decoded_fmts[f->index];
+	f->pixelformat = fourcc;
 	return 0;
 }
 

-- 
2.25.1

[PATCH v7 08/12] media: rkvdec: Add image format concept

[Sebastian Fricke]

From: Jonas Karlman <jonas@kwiboo.se>

Add an enum rkvdec_image_fmt used to signal an image format, e.g.
4:2:0 8-bit, 4:2:0 10-bit or any.

Tag each supported CAPUTRE format with an image format and use this tag
to filter out unsupported CAPTURE formats.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Tested-by: Christopher Obbard <chris.obbard@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec.c | 48 ++++++++++++++++++++++++++---------
 drivers/staging/media/rkvdec/rkvdec.h | 13 +++++++++-
 2 files changed, 48 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 52e64b399dcc4a0c028cac908c0b1ceb49956c5f..70154948b4e32e2c439f259b0f1e1bbc8b52b063 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -27,26 +27,45 @@
 #include "rkvdec.h"
 #include "rkvdec-regs.h"
 
-static u32 rkvdec_enum_decoded_fmt(struct rkvdec_ctx *ctx, int index)
+static bool rkvdec_image_fmt_match(enum rkvdec_image_fmt fmt1,
+				   enum rkvdec_image_fmt fmt2)
+{
+	return fmt1 == fmt2 || fmt2 == RKVDEC_IMG_FMT_ANY ||
+	       fmt1 == RKVDEC_IMG_FMT_ANY;
+}
+
+static u32 rkvdec_enum_decoded_fmt(struct rkvdec_ctx *ctx, int index,
+				   enum rkvdec_image_fmt image_fmt)
 {
 	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
+	int fmt_idx = -1;
+	unsigned int i;
 
 	if (WARN_ON(!desc))
 		return 0;
 
-	if (index >= desc->num_decoded_fmts)
-		return 0;
+	for (i = 0; i < desc->num_decoded_fmts; i++) {
+		if (!rkvdec_image_fmt_match(desc->decoded_fmts[i].image_fmt,
+					    image_fmt))
+			continue;
+		fmt_idx++;
+		if (index == fmt_idx)
+			return desc->decoded_fmts[i].fourcc;
+	}
 
-	return desc->decoded_fmts[index];
+	return 0;
 }
 
-static bool rkvdec_is_valid_fmt(struct rkvdec_ctx *ctx, u32 fourcc)
+static bool rkvdec_is_valid_fmt(struct rkvdec_ctx *ctx, u32 fourcc,
+				enum rkvdec_image_fmt image_fmt)
 {
 	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
 	unsigned int i;
 
 	for (i = 0; i < desc->num_decoded_fmts; i++) {
-		if (desc->decoded_fmts[i] == fourcc)
+		if (rkvdec_image_fmt_match(desc->decoded_fmts[i].image_fmt,
+					   image_fmt) &&
+		    desc->decoded_fmts[i].fourcc == fourcc)
 			return true;
 	}
 
@@ -80,7 +99,7 @@ static void rkvdec_reset_decoded_fmt(struct rkvdec_ctx *ctx)
 	struct v4l2_format *f = &ctx->decoded_fmt;
 	u32 fourcc;
 
-	fourcc = rkvdec_enum_decoded_fmt(ctx, 0);
+	fourcc = rkvdec_enum_decoded_fmt(ctx, 0, ctx->image_fmt);
 	rkvdec_reset_fmt(ctx, f, fourcc);
 	f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
 	f->fmt.pix_mp.width = ctx->coded_fmt.fmt.pix_mp.width;
@@ -149,8 +168,11 @@ static const struct rkvdec_ctrls rkvdec_h264_ctrls = {
 	.num_ctrls = ARRAY_SIZE(rkvdec_h264_ctrl_descs),
 };
 
-static const u32 rkvdec_h264_vp9_decoded_fmts[] = {
-	V4L2_PIX_FMT_NV12,
+static const struct rkvdec_decoded_fmt_desc rkvdec_h264_vp9_decoded_fmts[] = {
+	{
+		.fourcc = V4L2_PIX_FMT_NV12,
+		.image_fmt = RKVDEC_IMG_FMT_420_8BIT,
+	},
 };
 
 static const struct rkvdec_ctrl_desc rkvdec_vp9_ctrl_descs[] = {
@@ -282,8 +304,9 @@ static int rkvdec_try_capture_fmt(struct file *file, void *priv,
 	if (WARN_ON(!coded_desc))
 		return -EINVAL;
 
-	if (!rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat))
-		pix_mp->pixelformat = rkvdec_enum_decoded_fmt(ctx, 0);
+	if (!rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, ctx->image_fmt))
+		pix_mp->pixelformat = rkvdec_enum_decoded_fmt(ctx, 0,
+							      ctx->image_fmt);
 
 	/* Always apply the frmsize constraint of the coded end. */
 	pix_mp->width = max(pix_mp->width, ctx->coded_fmt.fmt.pix_mp.width);
@@ -400,6 +423,7 @@ static int rkvdec_s_output_fmt(struct file *file, void *priv,
 	 *
 	 * Note that this will propagates any size changes to the decoded format.
 	 */
+	ctx->image_fmt = RKVDEC_IMG_FMT_ANY;
 	rkvdec_reset_decoded_fmt(ctx);
 
 	/* Propagate colorspace information to capture. */
@@ -449,7 +473,7 @@ static int rkvdec_enum_capture_fmt(struct file *file, void *priv,
 	struct rkvdec_ctx *ctx = fh_to_rkvdec_ctx(priv);
 	u32 fourcc;
 
-	fourcc = rkvdec_enum_decoded_fmt(ctx, f->index);
+	fourcc = rkvdec_enum_decoded_fmt(ctx, f->index, ctx->image_fmt);
 	if (!fourcc)
 		return -EINVAL;
 
diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
index 633335ebb9c498bb69ae4072b772f30858a89e48..6f8cf50c5d99aad2f52e321f54f3ca17166ddf98 100644
--- a/drivers/staging/media/rkvdec/rkvdec.h
+++ b/drivers/staging/media/rkvdec/rkvdec.h
@@ -75,13 +75,23 @@ struct rkvdec_coded_fmt_ops {
 	int (*try_ctrl)(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl);
 };
 
+enum rkvdec_image_fmt {
+	RKVDEC_IMG_FMT_ANY = 0,
+	RKVDEC_IMG_FMT_420_8BIT,
+};
+
+struct rkvdec_decoded_fmt_desc {
+	u32 fourcc;
+	enum rkvdec_image_fmt image_fmt;
+};
+
 struct rkvdec_coded_fmt_desc {
 	u32 fourcc;
 	struct v4l2_frmsize_stepwise frmsize;
 	const struct rkvdec_ctrls *ctrls;
 	const struct rkvdec_coded_fmt_ops *ops;
 	unsigned int num_decoded_fmts;
-	const u32 *decoded_fmts;
+	const struct rkvdec_decoded_fmt_desc *decoded_fmts;
 	u32 subsystem_flags;
 };
 
@@ -104,6 +114,7 @@ struct rkvdec_ctx {
 	const struct rkvdec_coded_fmt_desc *coded_fmt_desc;
 	struct v4l2_ctrl_handler ctrl_hdl;
 	struct rkvdec_dev *dev;
+	enum rkvdec_image_fmt image_fmt;
 	void *priv;
 };
 

-- 
2.25.1

[PATCH v7 09/12] media: rkvdec: Add get_image_fmt ops

[Sebastian Fricke]

From: Jonas Karlman <jonas@kwiboo.se>

Add support for a get_image_fmt() ops that returns the required image
format.

The CAPTURE format is reset when the required image format changes and
the buffer queue is not busy.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Tested-by: Christopher Obbard <chris.obbard@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec.c | 49 +++++++++++++++++++++++++++++++++--
 drivers/staging/media/rkvdec/rkvdec.h |  2 ++
 2 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 70154948b4e32e2c439f259b0f1e1bbc8b52b063..5394079509305c619f1d0c1f542bfc409317c3b7 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -111,15 +111,60 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
 	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
+	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
+	enum rkvdec_image_fmt image_fmt;
+	struct vb2_queue *vq;
+	int ret;
+
+	if (desc->ops->try_ctrl) {
+		ret = desc->ops->try_ctrl(ctx, ctrl);
+		if (ret)
+			return ret;
+	}
+
+	if (!desc->ops->get_image_fmt)
+		return 0;
 
-	if (desc->ops->try_ctrl)
-		return desc->ops->try_ctrl(ctx, ctrl);
+	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
+	if (ctx->image_fmt == image_fmt)
+		return 0;
+
+	if (rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, image_fmt))
+		return 0;
+
+	/* format change not allowed when queue is busy */
+	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
+			     V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
+	if (vb2_is_busy(vq))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int rkvdec_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
+	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
+	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
+	enum rkvdec_image_fmt image_fmt;
+
+	if (!desc->ops->get_image_fmt)
+		return 0;
+
+	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
+	if (ctx->image_fmt == image_fmt)
+		return 0;
+
+	ctx->image_fmt = image_fmt;
+	if (!rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, ctx->image_fmt))
+		rkvdec_reset_decoded_fmt(ctx);
 
 	return 0;
 }
 
 static const struct v4l2_ctrl_ops rkvdec_ctrl_ops = {
 	.try_ctrl = rkvdec_try_ctrl,
+	.s_ctrl = rkvdec_s_ctrl,
 };
 
 static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
index 6f8cf50c5d99aad2f52e321f54f3ca17166ddf98..e466a2753ccfc13738e0a672bc578e521af2c3f2 100644
--- a/drivers/staging/media/rkvdec/rkvdec.h
+++ b/drivers/staging/media/rkvdec/rkvdec.h
@@ -73,6 +73,8 @@ struct rkvdec_coded_fmt_ops {
 		     struct vb2_v4l2_buffer *dst_buf,
 		     enum vb2_buffer_state result);
 	int (*try_ctrl)(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl);
+	enum rkvdec_image_fmt (*get_image_fmt)(struct rkvdec_ctx *ctx,
+					       struct v4l2_ctrl *ctrl);
 };
 
 enum rkvdec_image_fmt {

-- 
2.25.1

[PATCH v7 10/12] media: rkvdec: h264: Support High 10 and 4:2:2 profiles

[Sebastian Fricke]

From: Jonas Karlman <jonas@kwiboo.se>

Add support and enable decoding of H264 High 10 and 4:2:2 profiles.

Decoded CAPTURE buffer width is aligned to 64 pixels to accommodate HW
requirement of 10-bit format buffers, fixes decoding of:

- Hi422FR13_SONY_A
- Hi422FR14_SONY_A
- Hi422FR15_SONY_A
- Hi422FR6_SONY_A
- Hi422FR7_SONY_A
- Hi422FR8_SONY_A
- Hi422FR9_SONY_A
- Hi422FREXT18_SONY_A

The get_image_fmt() ops is implemented to select an image format
required for the provided SPS control.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Tested-by: Christopher Obbard <chris.obbard@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 37 ++++++++++++++++++++++-------
 drivers/staging/media/rkvdec/rkvdec.c      | 38 +++++++++++++++++++++++-------
 drivers/staging/media/rkvdec/rkvdec.h      |  3 +++
 3 files changed, 60 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 8bce8902b8dda39bb2058c8504bd52ccae6b4204..d14b4d173448dbcce4ab978a83806064b100ca24 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -1027,24 +1027,42 @@ static int rkvdec_h264_adjust_fmt(struct rkvdec_ctx *ctx,
 	return 0;
 }
 
+static enum rkvdec_image_fmt rkvdec_h264_get_image_fmt(struct rkvdec_ctx *ctx,
+						       struct v4l2_ctrl *ctrl)
+{
+	const struct v4l2_ctrl_h264_sps *sps = ctrl->p_new.p_h264_sps;
+
+	if (ctrl->id != V4L2_CID_STATELESS_H264_SPS)
+		return RKVDEC_IMG_FMT_ANY;
+
+	if (sps->bit_depth_luma_minus8 == 0) {
+		if (sps->chroma_format_idc == 2)
+			return RKVDEC_IMG_FMT_422_8BIT;
+		else
+			return RKVDEC_IMG_FMT_420_8BIT;
+	} else if (sps->bit_depth_luma_minus8 == 2) {
+		if (sps->chroma_format_idc == 2)
+			return RKVDEC_IMG_FMT_422_10BIT;
+		else
+			return RKVDEC_IMG_FMT_420_10BIT;
+	}
+
+	return RKVDEC_IMG_FMT_ANY;
+}
+
 static int rkvdec_h264_validate_sps(struct rkvdec_ctx *ctx,
 				    const struct v4l2_ctrl_h264_sps *sps)
 {
 	unsigned int width, height;
 
-	/*
-	 * TODO: The hardware supports 10-bit and 4:2:2 profiles,
-	 * but it's currently broken in the driver.
-	 * Reject them for now, until it's fixed.
-	 */
-	if (sps->chroma_format_idc > 1)
-		/* Only 4:0:0 and 4:2:0 are supported */
+	if (sps->chroma_format_idc > 2)
+		/* Only 4:0:0, 4:2:0 and 4:2:2 are supported */
 		return -EINVAL;
 	if (sps->bit_depth_luma_minus8 != sps->bit_depth_chroma_minus8)
 		/* Luma and chroma bit depth mismatch */
 		return -EINVAL;
-	if (sps->bit_depth_luma_minus8 != 0)
-		/* Only 8-bit is supported */
+	if (sps->bit_depth_luma_minus8 != 0 && sps->bit_depth_luma_minus8 != 2)
+		/* Only 8-bit and 10-bit is supported */
 		return -EINVAL;
 
 	width = (sps->pic_width_in_mbs_minus1 + 1) * 16;
@@ -1190,4 +1208,5 @@ const struct rkvdec_coded_fmt_ops rkvdec_h264_fmt_ops = {
 	.stop = rkvdec_h264_stop,
 	.run = rkvdec_h264_run,
 	.try_ctrl = rkvdec_h264_try_ctrl,
+	.get_image_fmt = rkvdec_h264_get_image_fmt,
 };
diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 5394079509305c619f1d0c1f542bfc409317c3b7..351444d569316cf52850a1831710fb5c1aceaa70 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -196,9 +196,10 @@ static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
 	{
 		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_PROFILE,
 		.cfg.min = V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE,
-		.cfg.max = V4L2_MPEG_VIDEO_H264_PROFILE_HIGH,
+		.cfg.max = V4L2_MPEG_VIDEO_H264_PROFILE_HIGH_422_INTRA,
 		.cfg.menu_skip_mask =
-			BIT(V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED),
+			BIT(V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED) |
+			BIT(V4L2_MPEG_VIDEO_H264_PROFILE_HIGH_444_PREDICTIVE),
 		.cfg.def = V4L2_MPEG_VIDEO_H264_PROFILE_MAIN,
 	},
 	{
@@ -213,11 +214,23 @@ static const struct rkvdec_ctrls rkvdec_h264_ctrls = {
 	.num_ctrls = ARRAY_SIZE(rkvdec_h264_ctrl_descs),
 };
 
-static const struct rkvdec_decoded_fmt_desc rkvdec_h264_vp9_decoded_fmts[] = {
+static const struct rkvdec_decoded_fmt_desc rkvdec_h264_decoded_fmts[] = {
 	{
 		.fourcc = V4L2_PIX_FMT_NV12,
 		.image_fmt = RKVDEC_IMG_FMT_420_8BIT,
 	},
+	{
+		.fourcc = V4L2_PIX_FMT_NV15,
+		.image_fmt = RKVDEC_IMG_FMT_420_10BIT,
+	},
+	{
+		.fourcc = V4L2_PIX_FMT_NV16,
+		.image_fmt = RKVDEC_IMG_FMT_422_8BIT,
+	},
+	{
+		.fourcc = V4L2_PIX_FMT_NV20,
+		.image_fmt = RKVDEC_IMG_FMT_422_10BIT,
+	},
 };
 
 static const struct rkvdec_ctrl_desc rkvdec_vp9_ctrl_descs[] = {
@@ -240,21 +253,28 @@ static const struct rkvdec_ctrls rkvdec_vp9_ctrls = {
 	.num_ctrls = ARRAY_SIZE(rkvdec_vp9_ctrl_descs),
 };
 
+static const struct rkvdec_decoded_fmt_desc rkvdec_vp9_decoded_fmts[] = {
+	{
+		.fourcc = V4L2_PIX_FMT_NV12,
+		.image_fmt = RKVDEC_IMG_FMT_420_8BIT,
+	},
+};
+
 static const struct rkvdec_coded_fmt_desc rkvdec_coded_fmts[] = {
 	{
 		.fourcc = V4L2_PIX_FMT_H264_SLICE,
 		.frmsize = {
-			.min_width = 48,
+			.min_width = 64,
 			.max_width = 4096,
-			.step_width = 16,
+			.step_width = 64,
 			.min_height = 48,
 			.max_height = 2560,
 			.step_height = 16,
 		},
 		.ctrls = &rkvdec_h264_ctrls,
 		.ops = &rkvdec_h264_fmt_ops,
-		.num_decoded_fmts = ARRAY_SIZE(rkvdec_h264_vp9_decoded_fmts),
-		.decoded_fmts = rkvdec_h264_vp9_decoded_fmts,
+		.num_decoded_fmts = ARRAY_SIZE(rkvdec_h264_decoded_fmts),
+		.decoded_fmts = rkvdec_h264_decoded_fmts,
 		.subsystem_flags = VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF,
 	},
 	{
@@ -269,8 +289,8 @@ static const struct rkvdec_coded_fmt_desc rkvdec_coded_fmts[] = {
 		},
 		.ctrls = &rkvdec_vp9_ctrls,
 		.ops = &rkvdec_vp9_fmt_ops,
-		.num_decoded_fmts = ARRAY_SIZE(rkvdec_h264_vp9_decoded_fmts),
-		.decoded_fmts = rkvdec_h264_vp9_decoded_fmts,
+		.num_decoded_fmts = ARRAY_SIZE(rkvdec_vp9_decoded_fmts),
+		.decoded_fmts = rkvdec_vp9_decoded_fmts,
 	}
 };
 
diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
index e466a2753ccfc13738e0a672bc578e521af2c3f2..9a9f4fced7a184b952d341d75c7faedaa75163d6 100644
--- a/drivers/staging/media/rkvdec/rkvdec.h
+++ b/drivers/staging/media/rkvdec/rkvdec.h
@@ -80,6 +80,9 @@ struct rkvdec_coded_fmt_ops {
 enum rkvdec_image_fmt {
 	RKVDEC_IMG_FMT_ANY = 0,
 	RKVDEC_IMG_FMT_420_8BIT,
+	RKVDEC_IMG_FMT_420_10BIT,
+	RKVDEC_IMG_FMT_422_8BIT,
+	RKVDEC_IMG_FMT_422_10BIT,
 };
 
 struct rkvdec_decoded_fmt_desc {

-- 
2.25.1

[PATCH v7 11/12] media: rkvdec: h264: Limit minimum profile to constrained baseline

[Sebastian Fricke]

Neither the hardware nor the kernel API support FMO/ASO features
required by the full baseline profile. Therefore limit the minimum
profile to the constrained baseline profile explicitly.

Suggested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 351444d569316cf52850a1831710fb5c1aceaa70..2859041bcc932bd638b4288bb8eba6b1443a08e3 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -195,7 +195,7 @@ static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
 	},
 	{
 		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_PROFILE,
-		.cfg.min = V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE,
+		.cfg.min = V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE,
 		.cfg.max = V4L2_MPEG_VIDEO_H264_PROFILE_HIGH_422_INTRA,
 		.cfg.menu_skip_mask =
 			BIT(V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED) |

-- 
2.25.1

[PATCH v7 12/12] media: rkvdec: Fix frame size enumeration

[Sebastian Fricke]

From: Jonas Karlman <jonas@kwiboo.se>

The VIDIOC_ENUM_FRAMESIZES ioctl should return all frame sizes (i.e.
width and height in pixels) that the device supports for the given pixel
format.

It doesn't make a lot of sense to return the frame-sizes in a stepwise
manner, which is used to enforce hardware alignments requirements for
CAPTURE buffers, for coded formats.

Instead, applications should receive an indication, about the maximum
supported frame size for that hardware decoder, via a continuous
frame-size enumeration.

Fixes: cd33c830448b ("media: rkvdec: Add the rkvdec driver")
Suggested-by: Alex Bee <knaerzche@gmail.com>
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/staging/media/rkvdec/rkvdec.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 2859041bcc932bd638b4288bb8eba6b1443a08e3..619031b8cadc8d7185712ec6121a895e6ab89046 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -334,8 +334,14 @@ static int rkvdec_enum_framesizes(struct file *file, void *priv,
 	if (!fmt)
 		return -EINVAL;
 
-	fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
-	fsize->stepwise = fmt->frmsize;
+	fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS;
+	fsize->stepwise.min_width = 1;
+	fsize->stepwise.max_width = fmt->frmsize.max_width;
+	fsize->stepwise.step_width = 1;
+	fsize->stepwise.min_height = 1;
+	fsize->stepwise.max_height = fmt->frmsize.max_height;
+	fsize->stepwise.step_height = 1;
+
 	return 0;
 }
 

-- 
2.25.1

[PATCH] fixup! media: v4l2-common: Add helpers to calculate bytesperline and sizeimage

[Sebastian Fricke]

---
 drivers/media/v4l2-core/v4l2-common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 07a999f75755..aa86b8c6aa75 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -360,7 +360,7 @@ static inline unsigned int v4l2_format_block_height(const struct v4l2_format_inf
 }
 
 static inline unsigned int v4l2_format_plane_stride(const struct v4l2_format_info *info, int plane,
-						   unsigned int width)
+						    unsigned int width)
 {
 	unsigned int hdiv = plane ? info->hdiv : 1;
 	unsigned int aligned_width =
-- 
2.25.1

Re: [PATCH] fixup! media: v4l2-common: Add helpers to calculate bytesperline and sizeimage

[Sebastian Fricke]

Hey,

sorry about missing this in the patch series, if you don't like the fixup path, then I can send a new patch series as well.
I just thought the change was minor enough and addressed the final comments.

Regards,
Sebastian

 ---- On Tue, 25 Feb 2025 13:40:08 +0100  Sebastian Fricke <sebastian.fricke@collabora.com> wrote --- 
 > ---
 >  drivers/media/v4l2-core/v4l2-common.c | 2 +-
 >  1 file changed, 1 insertion(+), 1 deletion(-)
 > 
 > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
 > index 07a999f75755..aa86b8c6aa75 100644
 > --- a/drivers/media/v4l2-core/v4l2-common.c
 > +++ b/drivers/media/v4l2-core/v4l2-common.c
 > @@ -360,7 +360,7 @@ static inline unsigned int v4l2_format_block_height(const struct v4l2_format_inf
 >  }
 >  
 >  static inline unsigned int v4l2_format_plane_stride(const struct v4l2_format_info *info, int plane,
 > -                           unsigned int width)
 > +                            unsigned int width)
 >  {
 >      unsigned int hdiv = plane ? info->hdiv : 1;
 >      unsigned int aligned_width =
 > -- 
 > 2.25.1
 > 
 > 

Re: [PATCH] fixup! media: v4l2-common: Add helpers to calculate bytesperline and sizeimage

[Hans Verkuil]

On 2/25/25 13:46, Sebastian Fricke wrote:
> Hey,
> 
> sorry about missing this in the patch series, if you don't like the fixup path, then I can send a new patch series as well.
> I just thought the change was minor enough and addressed the final comments.

Ah, this relates to patch 01/12 of this patch series:

https://patchwork.linuxtv.org/project/linux-media/list/?series=14577

Next time it might be better to just reply to the offending patch with the fixup.

That way it is clear for the maintainer what the fixup is for.

Regards,

	Hans

> 
> Regards,
> Sebastian
> 
>  ---- On Tue, 25 Feb 2025 13:40:08 +0100  Sebastian Fricke <sebastian.fricke@collabora.com> wrote --- 
>  > ---
>  >  drivers/media/v4l2-core/v4l2-common.c | 2 +-
>  >  1 file changed, 1 insertion(+), 1 deletion(-)
>  > 
>  > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
>  > index 07a999f75755..aa86b8c6aa75 100644
>  > --- a/drivers/media/v4l2-core/v4l2-common.c
>  > +++ b/drivers/media/v4l2-core/v4l2-common.c
>  > @@ -360,7 +360,7 @@ static inline unsigned int v4l2_format_block_height(const struct v4l2_format_inf
>  >  }
>  >  
>  >  static inline unsigned int v4l2_format_plane_stride(const struct v4l2_format_info *info, int plane,
>  > -                           unsigned int width)
>  > +                            unsigned int width)
>  >  {
>  >      unsigned int hdiv = plane ? info->hdiv : 1;
>  >      unsigned int aligned_width =
>  > -- 
>  > 2.25.1
>  > 
>  > 
> 

Re: [PATCH v7 01/12] media: v4l2-common: Add helpers to calculate bytesperline and sizeimage

[Hans Verkuil]

On 2/25/25 10:40, Sebastian Fricke wrote:
> From: Jonas Karlman <jonas@kwiboo.se>
> 
> Add helper functions to calculate plane bytesperline and sizeimage,
> these new helpers consider bpp div, block width and height when
> calculating plane bytesperline and sizeimage.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Tested-by: Christopher Obbard <chris.obbard@collabora.com>
> ---
>  drivers/media/v4l2-core/v4l2-common.c | 78 +++++++++++++++++------------------
>  1 file changed, 39 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index e4b2de3833ee3d440493f94fca5ee2049b013ef0..fdab16b9d481300eecf2202b3930fdf0a97a3023 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -357,6 +357,34 @@ static inline unsigned int v4l2_format_block_height(const struct v4l2_format_inf
>  	return info->block_h[plane];
>  }
>  
> +static inline unsigned int v4l2_format_plane_stride(const struct v4l2_format_info *info, int plane,
> +						   unsigned int width)

Note that there was a small fixup for this function here:

https://patchwork.linuxtv.org/project/linux-media/patch/20250225124008.195405-1-sebastian.fricke@collabora.com/

Just so it isn't forgotten when this series is merged.

Regards,

	Hans

> +{
> +	unsigned int hdiv = plane ? info->hdiv : 1;
> +	unsigned int aligned_width =
> +		ALIGN(width, v4l2_format_block_width(info, plane));
> +
> +	return DIV_ROUND_UP(aligned_width, hdiv) *
> +	       info->bpp[plane] / info->bpp_div[plane];
> +}
> +
> +static inline unsigned int v4l2_format_plane_height(const struct v4l2_format_info *info, int plane,
> +						    unsigned int height)
> +{
> +	unsigned int vdiv = plane ? info->vdiv : 1;
> +	unsigned int aligned_height =
> +		ALIGN(height, v4l2_format_block_height(info, plane));
> +
> +	return DIV_ROUND_UP(aligned_height, vdiv);
> +}
> +
> +static inline unsigned int v4l2_format_plane_size(const struct v4l2_format_info *info, int plane,
> +						  unsigned int width, unsigned int height)
> +{
> +	return v4l2_format_plane_stride(info, plane, width) *
> +	       v4l2_format_plane_height(info, plane, height);
> +}
> +
>  void v4l2_apply_frmsize_constraints(u32 *width, u32 *height,
>  				    const struct v4l2_frmsize_stepwise *frmsize)
>  {
> @@ -392,37 +420,19 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
>  
>  	if (info->mem_planes == 1) {
>  		plane = &pixfmt->plane_fmt[0];
> -		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0] / info->bpp_div[0];
> +		plane->bytesperline = v4l2_format_plane_stride(info, 0, width);
>  		plane->sizeimage = 0;
>  
> -		for (i = 0; i < info->comp_planes; i++) {
> -			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
> -			unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
> -			unsigned int aligned_width;
> -			unsigned int aligned_height;
> -
> -			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
> -			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
> -
> -			plane->sizeimage += info->bpp[i] *
> -				DIV_ROUND_UP(aligned_width, hdiv) *
> -				DIV_ROUND_UP(aligned_height, vdiv) / info->bpp_div[i];
> -		}
> +		for (i = 0; i < info->comp_planes; i++)
> +			plane->sizeimage +=
> +				v4l2_format_plane_size(info, i, width, height);
>  	} else {
>  		for (i = 0; i < info->comp_planes; i++) {
> -			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
> -			unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
> -			unsigned int aligned_width;
> -			unsigned int aligned_height;
> -
> -			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
> -			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
> -
>  			plane = &pixfmt->plane_fmt[i];
>  			plane->bytesperline =
> -				info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv) / info->bpp_div[i];
> -			plane->sizeimage =
> -				plane->bytesperline * DIV_ROUND_UP(aligned_height, vdiv);
> +				v4l2_format_plane_stride(info, i, width);
> +			plane->sizeimage = plane->bytesperline *
> +				v4l2_format_plane_height(info, i, height);
>  		}
>  	}
>  	return 0;
> @@ -446,22 +456,12 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
>  	pixfmt->width = width;
>  	pixfmt->height = height;
>  	pixfmt->pixelformat = pixelformat;
> -	pixfmt->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0] / info->bpp_div[0];
> +	pixfmt->bytesperline = v4l2_format_plane_stride(info, 0, width);
>  	pixfmt->sizeimage = 0;
>  
> -	for (i = 0; i < info->comp_planes; i++) {
> -		unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
> -		unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
> -		unsigned int aligned_width;
> -		unsigned int aligned_height;
> -
> -		aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
> -		aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
> -
> -		pixfmt->sizeimage += info->bpp[i] *
> -			DIV_ROUND_UP(aligned_width, hdiv) *
> -			DIV_ROUND_UP(aligned_height, vdiv) / info->bpp_div[i];
> -	}
> +	for (i = 0; i < info->comp_planes; i++)
> +		pixfmt->sizeimage +=
> +			v4l2_format_plane_size(info, i, width, height);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
> 

Re: [PATCH] fixup! media: v4l2-common: Add helpers to calculate bytesperline and sizeimage

[Nicolas Dufresne]

Le mardi 25 février 2025 à 13:50 +0100, Hans Verkuil a écrit :
> On 2/25/25 13:46, Sebastian Fricke wrote:
> > Hey,
> > 
> > sorry about missing this in the patch series, if you don't like the
> > fixup path, then I can send a new patch series as well.
> > I just thought the change was minor enough and addressed the final
> > comments.
> 
> Ah, this relates to patch 01/12 of this patch series:
> 
> https://patchwork.linuxtv.org/project/linux-media/list/?series=14577
> 
> Next time it might be better to just reply to the offending patch
> with the fixup.
> 
> That way it is clear for the maintainer what the fixup is for.

I believe you could have used:

  git send-email -in-reply-to=9db356bd-4d71-4975-91c6-7435bee8aef3@xs4all.nl <somepatch>

To make that a reply.

cheers,
Nicolas
> 
> Regards,
> 
> 	Hans
> 
> > 
> > Regards,
> > Sebastian
> > 
> >  ---- On Tue, 25 Feb 2025 13:40:08 +0100  Sebastian Fricke
> > <sebastian.fricke@collabora.com> wrote --- 
> >  > ---
> >  >  drivers/media/v4l2-core/v4l2-common.c | 2 +-
> >  >  1 file changed, 1 insertion(+), 1 deletion(-)
> >  > 
> >  > diff --git a/drivers/media/v4l2-core/v4l2-common.c
> > b/drivers/media/v4l2-core/v4l2-common.c
> >  > index 07a999f75755..aa86b8c6aa75 100644
> >  > --- a/drivers/media/v4l2-core/v4l2-common.c
> >  > +++ b/drivers/media/v4l2-core/v4l2-common.c
> >  > @@ -360,7 +360,7 @@ static inline unsigned int
> > v4l2_format_block_height(const struct v4l2_format_inf
> >  >  }
> >  >  
> >  >  static inline unsigned int v4l2_format_plane_stride(const
> > struct v4l2_format_info *info, int plane,
> >  > -                           unsigned int width)
> >  > +                            unsigned int width)
> >  >  {
> >  >      unsigned int hdiv = plane ? info->hdiv : 1;
> >  >      unsigned int aligned_width =
> >  > -- 
> >  > 2.25.1
> >  > 
> >  > 
> > 
> 

-- 
Nicolas Dufresne
Principal Engineer at Collabora

Re: [PATCH v7 09/12] media: rkvdec: Add get_image_fmt ops

[Hans Verkuil]

On 25/02/2025 10:40, Sebastian Fricke wrote:
> From: Jonas Karlman <jonas@kwiboo.se>
> 
> Add support for a get_image_fmt() ops that returns the required image
> format.
> 
> The CAPTURE format is reset when the required image format changes and
> the buffer queue is not busy.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Tested-by: Christopher Obbard <chris.obbard@collabora.com>
> ---
>  drivers/staging/media/rkvdec/rkvdec.c | 49 +++++++++++++++++++++++++++++++++--
>  drivers/staging/media/rkvdec/rkvdec.h |  2 ++
>  2 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> index 70154948b4e32e2c439f259b0f1e1bbc8b52b063..5394079509305c619f1d0c1f542bfc409317c3b7 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.c
> +++ b/drivers/staging/media/rkvdec/rkvdec.c
> @@ -111,15 +111,60 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
>  	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
> +	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
> +	enum rkvdec_image_fmt image_fmt;
> +	struct vb2_queue *vq;
> +	int ret;
> +
> +	if (desc->ops->try_ctrl) {
> +		ret = desc->ops->try_ctrl(ctx, ctrl);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (!desc->ops->get_image_fmt)
> +		return 0;
>  
> -	if (desc->ops->try_ctrl)
> -		return desc->ops->try_ctrl(ctx, ctrl);
> +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
> +	if (ctx->image_fmt == image_fmt)
> +		return 0;
> +
> +	if (rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, image_fmt))
> +		return 0;
> +
> +	/* format change not allowed when queue is busy */
> +	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> +			     V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> +	if (vb2_is_busy(vq))
> +		return -EINVAL;

This makes no sense to me. This just tries a control, and that should just
work, regardless of vb2_is_busy(). It's a 'try', so you are not actually
changing anything.

> +
> +	return 0;
> +}
> +
> +static int rkvdec_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
> +	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
> +	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
> +	enum rkvdec_image_fmt image_fmt;
> +
> +	if (!desc->ops->get_image_fmt)
> +		return 0;
> +
> +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
> +	if (ctx->image_fmt == image_fmt)
> +		return 0;

If you really can't set a control when the queue is busy, then that should
be tested here, not in try_ctrl. And then you return -EBUSY.

Am I missing something here?

Regards,

	Hans

> +
> +	ctx->image_fmt = image_fmt;
> +	if (!rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, ctx->image_fmt))
> +		rkvdec_reset_decoded_fmt(ctx);
>  
>  	return 0;
>  }
>  
>  static const struct v4l2_ctrl_ops rkvdec_ctrl_ops = {
>  	.try_ctrl = rkvdec_try_ctrl,
> +	.s_ctrl = rkvdec_s_ctrl,
>  };
>  
>  static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
> index 6f8cf50c5d99aad2f52e321f54f3ca17166ddf98..e466a2753ccfc13738e0a672bc578e521af2c3f2 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.h
> +++ b/drivers/staging/media/rkvdec/rkvdec.h
> @@ -73,6 +73,8 @@ struct rkvdec_coded_fmt_ops {
>  		     struct vb2_v4l2_buffer *dst_buf,
>  		     enum vb2_buffer_state result);
>  	int (*try_ctrl)(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl);
> +	enum rkvdec_image_fmt (*get_image_fmt)(struct rkvdec_ctx *ctx,
> +					       struct v4l2_ctrl *ctrl);
>  };
>  
>  enum rkvdec_image_fmt {
> 

Re: [PATCH v7 09/12] media: rkvdec: Add get_image_fmt ops

[Nicolas Dufresne]

Le lundi 07 avril 2025 à 13:09 +0200, Hans Verkuil a écrit :
> On 25/02/2025 10:40, Sebastian Fricke wrote:
> > From: Jonas Karlman <jonas@kwiboo.se>
> > 
> > Add support for a get_image_fmt() ops that returns the required image
> > format.
> > 
> > The CAPTURE format is reset when the required image format changes and
> > the buffer queue is not busy.
> > 
> > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > Tested-by: Christopher Obbard <chris.obbard@collabora.com>
> > ---
> >  drivers/staging/media/rkvdec/rkvdec.c | 49 +++++++++++++++++++++++++++++++++--
> >  drivers/staging/media/rkvdec/rkvdec.h |  2 ++
> >  2 files changed, 49 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> > index 70154948b4e32e2c439f259b0f1e1bbc8b52b063..5394079509305c619f1d0c1f542bfc409317c3b7 100644
> > --- a/drivers/staging/media/rkvdec/rkvdec.c
> > +++ b/drivers/staging/media/rkvdec/rkvdec.c
> > @@ -111,15 +111,60 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
> >  {
> >  	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
> >  	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
> > +	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
> > +	enum rkvdec_image_fmt image_fmt;
> > +	struct vb2_queue *vq;
> > +	int ret;
> > +
> > +	if (desc->ops->try_ctrl) {
> > +		ret = desc->ops->try_ctrl(ctx, ctrl);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	if (!desc->ops->get_image_fmt)
> > +		return 0;
> >  
> > -	if (desc->ops->try_ctrl)
> > -		return desc->ops->try_ctrl(ctx, ctrl);
> > +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
> > +	if (ctx->image_fmt == image_fmt)
> > +		return 0;
> > +
> > +	if (rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, image_fmt))
> > +		return 0;
> > +
> > +	/* format change not allowed when queue is busy */
> > +	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> > +			     V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> > +	if (vb2_is_busy(vq))
> > +		return -EINVAL;
> 
> This makes no sense to me. This just tries a control, and that should just
> work, regardless of vb2_is_busy(). It's a 'try', so you are not actually
> changing anything.

See comment below, notice that this code is only reached if the control
introduce parameters that are not compatible with the current capture
queue fmt. The entire function uses "success" early exit, so the
further down you get in the function, the less likely your control is
valid.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int rkvdec_s_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
> > +	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
> > +	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
> > +	enum rkvdec_image_fmt image_fmt;
> > +
> > +	if (!desc->ops->get_image_fmt)
> > +		return 0;
> > +
> > +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
> > +	if (ctx->image_fmt == image_fmt)
> > +		return 0;
> 
> If you really can't set a control when the queue is busy, then that should
> be tested here, not in try_ctrl. And then you return -EBUSY.
> 
> Am I missing something here?

When I reviewed, I had imagine that s_ctrl on a request would just run
a try. Now that I read that more careful, I see that it does a true set
on separate copy. So yes, this can safely be moved here.

Since you seem wondering "If you really can't set a control", let me
explain what Jonas wants to protect against. RKVdec does not have any
color conversion code, the header compound control (which header
depends on the codec), contains details such as sub-sampling and color
depth. Without color conversion, when the image format is locked (the
busy queue), you can't request the HW to decode a frame witch does not
fit. This could otherwise lead to buffer overflow in the HW,
fortunately protected by the iommu, but you don't really want to depend
on the mmu.

I've never used try_ctrl in my decade of v4l2, so obviously, now that I
know that s_ctrl on request is not a try, I'm fine with rejecting this
PR, sending a new version and making a PR again. But if I was to use
this API in userspace, my intuitive expectation would be that this
should fail try(), even if its very rarely valid to check the queue
state in try control.

Nicolas

> 
> Regards,
> 
> 	Hans
> 
> > +
> > +	ctx->image_fmt = image_fmt;
> > +	if (!rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, ctx->image_fmt))
> > +		rkvdec_reset_decoded_fmt(ctx);
> >  
> >  	return 0;
> >  }
> >  
> >  static const struct v4l2_ctrl_ops rkvdec_ctrl_ops = {
> >  	.try_ctrl = rkvdec_try_ctrl,
> > +	.s_ctrl = rkvdec_s_ctrl,
> >  };
> >  
> >  static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
> > diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
> > index 6f8cf50c5d99aad2f52e321f54f3ca17166ddf98..e466a2753ccfc13738e0a672bc578e521af2c3f2 100644
> > --- a/drivers/staging/media/rkvdec/rkvdec.h
> > +++ b/drivers/staging/media/rkvdec/rkvdec.h
> > @@ -73,6 +73,8 @@ struct rkvdec_coded_fmt_ops {
> >  		     struct vb2_v4l2_buffer *dst_buf,
> >  		     enum vb2_buffer_state result);
> >  	int (*try_ctrl)(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl);
> > +	enum rkvdec_image_fmt (*get_image_fmt)(struct rkvdec_ctx *ctx,
> > +					       struct v4l2_ctrl *ctrl);
> >  };
> >  
> >  enum rkvdec_image_fmt {
> > 

-- 
Nicolas Dufresne
Principal Engineer at Collabora

Re: [PATCH v7 09/12] media: rkvdec: Add get_image_fmt ops

[Hans Verkuil]

On 07/04/2025 15:52, Nicolas Dufresne wrote:
> Le lundi 07 avril 2025 à 13:09 +0200, Hans Verkuil a écrit :
>> On 25/02/2025 10:40, Sebastian Fricke wrote:
>>> From: Jonas Karlman <jonas@kwiboo.se>
>>>
>>> Add support for a get_image_fmt() ops that returns the required image
>>> format.
>>>
>>> The CAPTURE format is reset when the required image format changes and
>>> the buffer queue is not busy.
>>>
>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>> Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>> Tested-by: Christopher Obbard <chris.obbard@collabora.com>
>>> ---
>>>  drivers/staging/media/rkvdec/rkvdec.c | 49 +++++++++++++++++++++++++++++++++--
>>>  drivers/staging/media/rkvdec/rkvdec.h |  2 ++
>>>  2 files changed, 49 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
>>> index 70154948b4e32e2c439f259b0f1e1bbc8b52b063..5394079509305c619f1d0c1f542bfc409317c3b7 100644
>>> --- a/drivers/staging/media/rkvdec/rkvdec.c
>>> +++ b/drivers/staging/media/rkvdec/rkvdec.c
>>> @@ -111,15 +111,60 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
>>>  {
>>>  	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
>>>  	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
>>> +	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
>>> +	enum rkvdec_image_fmt image_fmt;
>>> +	struct vb2_queue *vq;
>>> +	int ret;
>>> +
>>> +	if (desc->ops->try_ctrl) {
>>> +		ret = desc->ops->try_ctrl(ctx, ctrl);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	if (!desc->ops->get_image_fmt)
>>> +		return 0;
>>>  
>>> -	if (desc->ops->try_ctrl)
>>> -		return desc->ops->try_ctrl(ctx, ctrl);
>>> +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
>>> +	if (ctx->image_fmt == image_fmt)
>>> +		return 0;
>>> +
>>> +	if (rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, image_fmt))
>>> +		return 0;
>>> +
>>> +	/* format change not allowed when queue is busy */
>>> +	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
>>> +			     V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
>>> +	if (vb2_is_busy(vq))
>>> +		return -EINVAL;

Looking closer, this code is just wrong. It does these format change
tests for any control, so if more controls are added in the future, then
those will be checked the same way, which makes no sense.

These tests belong to the actual control that you 'try'. In this case
rkvdec_h264_validate_sps(). This function already checks the width and
height, but it should also check the image format. It is all in the
wrong place.

>>
>> This makes no sense to me. This just tries a control, and that should just
>> work, regardless of vb2_is_busy(). It's a 'try', so you are not actually
>> changing anything.
> 
> See comment below, notice that this code is only reached if the control
> introduce parameters that are not compatible with the current capture
> queue fmt. The entire function uses "success" early exit, so the
> further down you get in the function, the less likely your control is
> valid.
> 
>>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int rkvdec_s_ctrl(struct v4l2_ctrl *ctrl)
>>> +{

If there is a try_ctrl op specified, then the control framework
will call that first before calling s_ctrl. So any validation that
try_ctrl did does not need to be done again in s_ctrl.

The same comment with try_ctrl is valid here as well: if there are
image format checks that need to be done, then those need to be done
per control and not as a generic check. If new controls are added in
the future, then you don't want the same checks to apply to the new
controls as well.

Regards,

	Hans

>>> +	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
>>> +	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
>>> +	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
>>> +	enum rkvdec_image_fmt image_fmt;
>>> +
>>> +	if (!desc->ops->get_image_fmt)
>>> +		return 0;
>>> +
>>> +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
>>> +	if (ctx->image_fmt == image_fmt)
>>> +		return 0;
>>
>> If you really can't set a control when the queue is busy, then that should
>> be tested here, not in try_ctrl. And then you return -EBUSY.
>>
>> Am I missing something here?
> 
> When I reviewed, I had imagine that s_ctrl on a request would just run
> a try. Now that I read that more careful, I see that it does a true set
> on separate copy. So yes, this can safely be moved here.
> 
> Since you seem wondering "If you really can't set a control", let me
> explain what Jonas wants to protect against. RKVdec does not have any
> color conversion code, the header compound control (which header
> depends on the codec), contains details such as sub-sampling and color
> depth. Without color conversion, when the image format is locked (the
> busy queue), you can't request the HW to decode a frame witch does not
> fit. This could otherwise lead to buffer overflow in the HW,
> fortunately protected by the iommu, but you don't really want to depend
> on the mmu.
> 
> I've never used try_ctrl in my decade of v4l2, so obviously, now that I
> know that s_ctrl on request is not a try, I'm fine with rejecting this
> PR, sending a new version and making a PR again. But if I was to use
> this API in userspace, my intuitive expectation would be that this
> should fail try(), even if its very rarely valid to check the queue
> state in try control.
> 
> Nicolas
> 
>>
>> Regards,
>>
>> 	Hans
>>
>>> +
>>> +	ctx->image_fmt = image_fmt;
>>> +	if (!rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, ctx->image_fmt))
>>> +		rkvdec_reset_decoded_fmt(ctx);
>>>  
>>>  	return 0;
>>>  }
>>>  
>>>  static const struct v4l2_ctrl_ops rkvdec_ctrl_ops = {
>>>  	.try_ctrl = rkvdec_try_ctrl,
>>> +	.s_ctrl = rkvdec_s_ctrl,
>>>  };
>>>  
>>>  static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
>>> index 6f8cf50c5d99aad2f52e321f54f3ca17166ddf98..e466a2753ccfc13738e0a672bc578e521af2c3f2 100644
>>> --- a/drivers/staging/media/rkvdec/rkvdec.h
>>> +++ b/drivers/staging/media/rkvdec/rkvdec.h
>>> @@ -73,6 +73,8 @@ struct rkvdec_coded_fmt_ops {
>>>  		     struct vb2_v4l2_buffer *dst_buf,
>>>  		     enum vb2_buffer_state result);
>>>  	int (*try_ctrl)(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl);
>>> +	enum rkvdec_image_fmt (*get_image_fmt)(struct rkvdec_ctx *ctx,
>>> +					       struct v4l2_ctrl *ctrl);
>>>  };
>>>  
>>>  enum rkvdec_image_fmt {
>>>
> 

Re: [PATCH v7 09/12] media: rkvdec: Add get_image_fmt ops

[Nicolas Dufresne]

Le lundi 07 avril 2025 à 16:17 +0200, Hans Verkuil a écrit :
> On 07/04/2025 15:52, Nicolas Dufresne wrote:
> > Le lundi 07 avril 2025 à 13:09 +0200, Hans Verkuil a écrit :
> > > On 25/02/2025 10:40, Sebastian Fricke wrote:
> > > > From: Jonas Karlman <jonas@kwiboo.se>
> > > > 
> > > > Add support for a get_image_fmt() ops that returns the required image
> > > > format.
> > > > 
> > > > The CAPTURE format is reset when the required image format changes and
> > > > the buffer queue is not busy.
> > > > 
> > > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > > > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > > Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > > Tested-by: Christopher Obbard <chris.obbard@collabora.com>
> > > > ---
> > > >  drivers/staging/media/rkvdec/rkvdec.c | 49 +++++++++++++++++++++++++++++++++--
> > > >  drivers/staging/media/rkvdec/rkvdec.h |  2 ++
> > > >  2 files changed, 49 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> > > > index 70154948b4e32e2c439f259b0f1e1bbc8b52b063..5394079509305c619f1d0c1f542bfc409317c3b7 100644
> > > > --- a/drivers/staging/media/rkvdec/rkvdec.c
> > > > +++ b/drivers/staging/media/rkvdec/rkvdec.c
> > > > @@ -111,15 +111,60 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
> > > >  {
> > > >  	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
> > > >  	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
> > > > +	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
> > > > +	enum rkvdec_image_fmt image_fmt;
> > > > +	struct vb2_queue *vq;
> > > > +	int ret;
> > > > +
> > > > +	if (desc->ops->try_ctrl) {
> > > > +		ret = desc->ops->try_ctrl(ctx, ctrl);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > +
> > > > +	if (!desc->ops->get_image_fmt)
> > > > +		return 0;
> > > >  
> > > > -	if (desc->ops->try_ctrl)
> > > > -		return desc->ops->try_ctrl(ctx, ctrl);
> > > > +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
> > > > +	if (ctx->image_fmt == image_fmt)
> > > > +		return 0;
> > > > +
> > > > +	if (rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, image_fmt))
> > > > +		return 0;
> > > > +
> > > > +	/* format change not allowed when queue is busy */
> > > > +	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> > > > +			     V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> > > > +	if (vb2_is_busy(vq))
> > > > +		return -EINVAL;
> 
> Looking closer, this code is just wrong. It does these format change
> tests for any control, so if more controls are added in the future, then
> those will be checked the same way, which makes no sense.

"Just wrong" should be kept for code that is semantically incorrect,
just a suggestion for choice of wording.

> 
> These tests belong to the actual control that you 'try'. In this case
> rkvdec_h264_validate_sps(). This function already checks the width and
> height, but it should also check the image format. It is all in the
> wrong place.

We can do that too. Though, this was generalized since once you enable
the other codecs, you endup with code duplication. I know this series
is an extract from a larger one.

So let's suggest to make a helper that combines rkvdec_is_valid_fmt()
and the busy check. Though on that, please reply to my comment below
(which you skipped).

> 
> > > 
> > > This makes no sense to me. This just tries a control, and that should just
> > > work, regardless of vb2_is_busy(). It's a 'try', so you are not actually
> > > changing anything.
> > 
> > See comment below, notice that this code is only reached if the control
> > introduce parameters that are not compatible with the current capture
> > queue fmt. The entire function uses "success" early exit, so the
> > further down you get in the function, the less likely your control is
> > valid.
> > 
> > > 
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int rkvdec_s_ctrl(struct v4l2_ctrl *ctrl)
> > > > +{
> 
> If there is a try_ctrl op specified, then the control framework
> will call that first before calling s_ctrl. So any validation that
> try_ctrl did does not need to be done again in s_ctrl.
> 
> The same comment with try_ctrl is valid here as well: if there are
> image format checks that need to be done, then those need to be done
> per control and not as a generic check. If new controls are added in
> the future, then you don't want the same checks to apply to the new
> controls as well.

I don't think the behaviour of try_ctrl and that being embedded in set
calls was being questioned by anyone. Can you reply to the last
paragraph below ?

> 
> Regards,
> 
> 	Hans
> 
> > > > +	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
> > > > +	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
> > > > +	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
> > > > +	enum rkvdec_image_fmt image_fmt;
> > > > +
> > > > +	if (!desc->ops->get_image_fmt)
> > > > +		return 0;
> > > > +
> > > > +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
> > > > +	if (ctx->image_fmt == image_fmt)
> > > > +		return 0;
> > > 
> > > If you really can't set a control when the queue is busy, then that should
> > > be tested here, not in try_ctrl. And then you return -EBUSY.
> > > 
> > > Am I missing something here?
> > 
> > When I reviewed, I had imagine that s_ctrl on a request would just run
> > a try. Now that I read that more careful, I see that it does a true set
> > on separate copy. So yes, this can safely be moved here.
> > 
> > Since you seem wondering "If you really can't set a control", let me
> > explain what Jonas wants to protect against. RKVdec does not have any
> > color conversion code, the header compound control (which header
> > depends on the codec), contains details such as sub-sampling and color
> > depth. Without color conversion, when the image format is locked (the
> > busy queue), you can't request the HW to decode a frame witch does not
> > fit. This could otherwise lead to buffer overflow in the HW,
> > fortunately protected by the iommu, but you don't really want to depend
> > on the mmu.
> > 
> > I've never used try_ctrl in my decade of v4l2, so obviously, now that I
> > know that s_ctrl on request is not a try, I'm fine with rejecting this
> > PR, sending a new version and making a PR again. But if I was to use
> > this API in userspace, my intuitive expectation would be that this
> > should fail try(), even if its very rarely valid to check the queue
> > state in try control.

Here, since we seem to disagree on the behaviour try should have for
this specific validation. What you asked on first pass is to make it so
that TRY will succeed, and SET will fail. I don't really like that
suggestion.

Nicolas

> > 
> > Nicolas
> > 
> > > 
> > > Regards,
> > > 
> > > 	Hans
> > > 
> > > > +
> > > > +	ctx->image_fmt = image_fmt;
> > > > +	if (!rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, ctx->image_fmt))
> > > > +		rkvdec_reset_decoded_fmt(ctx);
> > > >  
> > > >  	return 0;
> > > >  }
> > > >  
> > > >  static const struct v4l2_ctrl_ops rkvdec_ctrl_ops = {
> > > >  	.try_ctrl = rkvdec_try_ctrl,
> > > > +	.s_ctrl = rkvdec_s_ctrl,
> > > >  };
> > > >  
> > > >  static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
> > > > diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
> > > > index 6f8cf50c5d99aad2f52e321f54f3ca17166ddf98..e466a2753ccfc13738e0a672bc578e521af2c3f2 100644
> > > > --- a/drivers/staging/media/rkvdec/rkvdec.h
> > > > +++ b/drivers/staging/media/rkvdec/rkvdec.h
> > > > @@ -73,6 +73,8 @@ struct rkvdec_coded_fmt_ops {
> > > >  		     struct vb2_v4l2_buffer *dst_buf,
> > > >  		     enum vb2_buffer_state result);
> > > >  	int (*try_ctrl)(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl);
> > > > +	enum rkvdec_image_fmt (*get_image_fmt)(struct rkvdec_ctx *ctx,
> > > > +					       struct v4l2_ctrl *ctrl);
> > > >  };
> > > >  
> > > >  enum rkvdec_image_fmt {
> > > > 
> > 

-- 
Nicolas Dufresne
Principal Engineer at Collabora

Re: [PATCH v7 09/12] media: rkvdec: Add get_image_fmt ops

[Jonas Karlman]

On 2025-04-07 16:59, Nicolas Dufresne wrote:
> Le lundi 07 avril 2025 à 16:17 +0200, Hans Verkuil a écrit :
>> On 07/04/2025 15:52, Nicolas Dufresne wrote:
>>> Le lundi 07 avril 2025 à 13:09 +0200, Hans Verkuil a écrit :
>>>> On 25/02/2025 10:40, Sebastian Fricke wrote:
>>>>> From: Jonas Karlman <jonas@kwiboo.se>
>>>>>
>>>>> Add support for a get_image_fmt() ops that returns the required image
>>>>> format.
>>>>>
>>>>> The CAPTURE format is reset when the required image format changes and
>>>>> the buffer queue is not busy.
>>>>>
>>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>>>> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>>>> Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>>>> Tested-by: Christopher Obbard <chris.obbard@collabora.com>
>>>>> ---
>>>>>  drivers/staging/media/rkvdec/rkvdec.c | 49 +++++++++++++++++++++++++++++++++--
>>>>>  drivers/staging/media/rkvdec/rkvdec.h |  2 ++
>>>>>  2 files changed, 49 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
>>>>> index 70154948b4e32e2c439f259b0f1e1bbc8b52b063..5394079509305c619f1d0c1f542bfc409317c3b7 100644
>>>>> --- a/drivers/staging/media/rkvdec/rkvdec.c
>>>>> +++ b/drivers/staging/media/rkvdec/rkvdec.c
>>>>> @@ -111,15 +111,60 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
>>>>>  {
>>>>>  	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
>>>>>  	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
>>>>> +	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
>>>>> +	enum rkvdec_image_fmt image_fmt;
>>>>> +	struct vb2_queue *vq;
>>>>> +	int ret;
>>>>> +
>>>>> +	if (desc->ops->try_ctrl) {
>>>>> +		ret = desc->ops->try_ctrl(ctx, ctrl);
>>>>> +		if (ret)
>>>>> +			return ret;
>>>>> +	}
>>>>> +
>>>>> +	if (!desc->ops->get_image_fmt)
>>>>> +		return 0;
>>>>>  
>>>>> -	if (desc->ops->try_ctrl)
>>>>> -		return desc->ops->try_ctrl(ctx, ctrl);
>>>>> +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
>>>>> +	if (ctx->image_fmt == image_fmt)
>>>>> +		return 0;
>>>>> +
>>>>> +	if (rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, image_fmt))
>>>>> +		return 0;
>>>>> +
>>>>> +	/* format change not allowed when queue is busy */
>>>>> +	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
>>>>> +			     V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
>>>>> +	if (vb2_is_busy(vq))
>>>>> +		return -EINVAL;
>>
>> Looking closer, this code is just wrong. It does these format change
>> tests for any control, so if more controls are added in the future, then
>> those will be checked the same way, which makes no sense.
> 
> "Just wrong" should be kept for code that is semantically incorrect,
> just a suggestion for choice of wording.
> 
>>
>> These tests belong to the actual control that you 'try'. In this case
>> rkvdec_h264_validate_sps(). This function already checks the width and
>> height, but it should also check the image format. It is all in the
>> wrong place.

Keep in mind that rkvdec_try_ctrl and rkvdec_s_ctrl are only used for
CID_STATELESS_H264_SPS (and in future also CID_STATELESS_HEVC_SPS) not
any other control, so this is already in the correct place?

Maybe the naming of the functions are too generic, they could be named
rkvdec_sps_try_ctrl and rkvdec_sps_s_ctrl or similar if that makes more
sense?

Regards,
Jonas

> 
> We can do that too. Though, this was generalized since once you enable
> the other codecs, you endup with code duplication. I know this series
> is an extract from a larger one.
> 
> So let's suggest to make a helper that combines rkvdec_is_valid_fmt()
> and the busy check. Though on that, please reply to my comment below
> (which you skipped).
> 
>>
>>>>
>>>> This makes no sense to me. This just tries a control, and that should just
>>>> work, regardless of vb2_is_busy(). It's a 'try', so you are not actually
>>>> changing anything.
>>>
>>> See comment below, notice that this code is only reached if the control
>>> introduce parameters that are not compatible with the current capture
>>> queue fmt. The entire function uses "success" early exit, so the
>>> further down you get in the function, the less likely your control is
>>> valid.
>>>
>>>>
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static int rkvdec_s_ctrl(struct v4l2_ctrl *ctrl)
>>>>> +{
>>
>> If there is a try_ctrl op specified, then the control framework
>> will call that first before calling s_ctrl. So any validation that
>> try_ctrl did does not need to be done again in s_ctrl.
>>
>> The same comment with try_ctrl is valid here as well: if there are
>> image format checks that need to be done, then those need to be done
>> per control and not as a generic check. If new controls are added in
>> the future, then you don't want the same checks to apply to the new
>> controls as well.
> 
> I don't think the behaviour of try_ctrl and that being embedded in set
> calls was being questioned by anyone. Can you reply to the last
> paragraph below ?
> 
>>
>> Regards,
>>
>> 	Hans
>>
>>>>> +	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
>>>>> +	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
>>>>> +	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
>>>>> +	enum rkvdec_image_fmt image_fmt;
>>>>> +
>>>>> +	if (!desc->ops->get_image_fmt)
>>>>> +		return 0;
>>>>> +
>>>>> +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
>>>>> +	if (ctx->image_fmt == image_fmt)
>>>>> +		return 0;
>>>>
>>>> If you really can't set a control when the queue is busy, then that should
>>>> be tested here, not in try_ctrl. And then you return -EBUSY.
>>>>
>>>> Am I missing something here?
>>>
>>> When I reviewed, I had imagine that s_ctrl on a request would just run
>>> a try. Now that I read that more careful, I see that it does a true set
>>> on separate copy. So yes, this can safely be moved here.
>>>
>>> Since you seem wondering "If you really can't set a control", let me
>>> explain what Jonas wants to protect against. RKVdec does not have any
>>> color conversion code, the header compound control (which header
>>> depends on the codec), contains details such as sub-sampling and color
>>> depth. Without color conversion, when the image format is locked (the
>>> busy queue), you can't request the HW to decode a frame witch does not
>>> fit. This could otherwise lead to buffer overflow in the HW,
>>> fortunately protected by the iommu, but you don't really want to depend
>>> on the mmu.
>>>
>>> I've never used try_ctrl in my decade of v4l2, so obviously, now that I
>>> know that s_ctrl on request is not a try, I'm fine with rejecting this
>>> PR, sending a new version and making a PR again. But if I was to use
>>> this API in userspace, my intuitive expectation would be that this
>>> should fail try(), even if its very rarely valid to check the queue
>>> state in try control.
> 
> Here, since we seem to disagree on the behaviour try should have for
> this specific validation. What you asked on first pass is to make it so
> that TRY will succeed, and SET will fail. I don't really like that
> suggestion.
> 
> Nicolas
> 
>>>
>>> Nicolas
>>>
>>>>
>>>> Regards,
>>>>
>>>> 	Hans
>>>>
>>>>> +
>>>>> +	ctx->image_fmt = image_fmt;
>>>>> +	if (!rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, ctx->image_fmt))
>>>>> +		rkvdec_reset_decoded_fmt(ctx);
>>>>>  
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>>  static const struct v4l2_ctrl_ops rkvdec_ctrl_ops = {
>>>>>  	.try_ctrl = rkvdec_try_ctrl,
>>>>> +	.s_ctrl = rkvdec_s_ctrl,
>>>>>  };
>>>>>  
>>>>>  static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
>>>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
>>>>> index 6f8cf50c5d99aad2f52e321f54f3ca17166ddf98..e466a2753ccfc13738e0a672bc578e521af2c3f2 100644
>>>>> --- a/drivers/staging/media/rkvdec/rkvdec.h
>>>>> +++ b/drivers/staging/media/rkvdec/rkvdec.h
>>>>> @@ -73,6 +73,8 @@ struct rkvdec_coded_fmt_ops {
>>>>>  		     struct vb2_v4l2_buffer *dst_buf,
>>>>>  		     enum vb2_buffer_state result);
>>>>>  	int (*try_ctrl)(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl);
>>>>> +	enum rkvdec_image_fmt (*get_image_fmt)(struct rkvdec_ctx *ctx,
>>>>> +					       struct v4l2_ctrl *ctrl);
>>>>>  };
>>>>>  
>>>>>  enum rkvdec_image_fmt {
>>>>>
>>>
> 

Re: [PATCH v7 09/12] media: rkvdec: Add get_image_fmt ops

[Jonas Karlman]

On 2025-04-07 17:07, Jonas Karlman wrote:
> On 2025-04-07 16:59, Nicolas Dufresne wrote:
>> Le lundi 07 avril 2025 à 16:17 +0200, Hans Verkuil a écrit :
>>> On 07/04/2025 15:52, Nicolas Dufresne wrote:
>>>> Le lundi 07 avril 2025 à 13:09 +0200, Hans Verkuil a écrit :
>>>>> On 25/02/2025 10:40, Sebastian Fricke wrote:
>>>>>> From: Jonas Karlman <jonas@kwiboo.se>
>>>>>>
>>>>>> Add support for a get_image_fmt() ops that returns the required image
>>>>>> format.
>>>>>>
>>>>>> The CAPTURE format is reset when the required image format changes and
>>>>>> the buffer queue is not busy.
>>>>>>
>>>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>>>>> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>>>>> Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>>>>> Tested-by: Christopher Obbard <chris.obbard@collabora.com>
>>>>>> ---
>>>>>>  drivers/staging/media/rkvdec/rkvdec.c | 49 +++++++++++++++++++++++++++++++++--
>>>>>>  drivers/staging/media/rkvdec/rkvdec.h |  2 ++
>>>>>>  2 files changed, 49 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
>>>>>> index 70154948b4e32e2c439f259b0f1e1bbc8b52b063..5394079509305c619f1d0c1f542bfc409317c3b7 100644
>>>>>> --- a/drivers/staging/media/rkvdec/rkvdec.c
>>>>>> +++ b/drivers/staging/media/rkvdec/rkvdec.c
>>>>>> @@ -111,15 +111,60 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
>>>>>>  {
>>>>>>  	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
>>>>>>  	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
>>>>>> +	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
>>>>>> +	enum rkvdec_image_fmt image_fmt;
>>>>>> +	struct vb2_queue *vq;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	if (desc->ops->try_ctrl) {
>>>>>> +		ret = desc->ops->try_ctrl(ctx, ctrl);
>>>>>> +		if (ret)
>>>>>> +			return ret;
>>>>>> +	}
>>>>>> +
>>>>>> +	if (!desc->ops->get_image_fmt)
>>>>>> +		return 0;
>>>>>>  
>>>>>> -	if (desc->ops->try_ctrl)
>>>>>> -		return desc->ops->try_ctrl(ctx, ctrl);
>>>>>> +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
>>>>>> +	if (ctx->image_fmt == image_fmt)
>>>>>> +		return 0;
>>>>>> +
>>>>>> +	if (rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, image_fmt))
>>>>>> +		return 0;
>>>>>> +
>>>>>> +	/* format change not allowed when queue is busy */
>>>>>> +	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
>>>>>> +			     V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
>>>>>> +	if (vb2_is_busy(vq))
>>>>>> +		return -EINVAL;
>>>
>>> Looking closer, this code is just wrong. It does these format change
>>> tests for any control, so if more controls are added in the future, then
>>> those will be checked the same way, which makes no sense.
>>
>> "Just wrong" should be kept for code that is semantically incorrect,
>> just a suggestion for choice of wording.
>>
>>>
>>> These tests belong to the actual control that you 'try'. In this case
>>> rkvdec_h264_validate_sps(). This function already checks the width and
>>> height, but it should also check the image format. It is all in the
>>> wrong place.
> 
> Keep in mind that rkvdec_try_ctrl and rkvdec_s_ctrl are only used for
> CID_STATELESS_H264_SPS (and in future also CID_STATELESS_HEVC_SPS) not
> any other control, so this is already in the correct place?
> 
> Maybe the naming of the functions are too generic, they could be named
> rkvdec_sps_try_ctrl and rkvdec_sps_s_ctrl or similar if that makes more
> sense?
> 
> Regards,
> Jonas
> 
>>
>> We can do that too. Though, this was generalized since once you enable
>> the other codecs, you endup with code duplication. I know this series
>> is an extract from a larger one.
>>
>> So let's suggest to make a helper that combines rkvdec_is_valid_fmt()
>> and the busy check. Though on that, please reply to my comment below
>> (which you skipped).
>>
>>>
>>>>>
>>>>> This makes no sense to me. This just tries a control, and that should just
>>>>> work, regardless of vb2_is_busy(). It's a 'try', so you are not actually
>>>>> changing anything.
>>>>
>>>> See comment below, notice that this code is only reached if the control
>>>> introduce parameters that are not compatible with the current capture
>>>> queue fmt. The entire function uses "success" early exit, so the
>>>> further down you get in the function, the less likely your control is
>>>> valid.
>>>>
>>>>>
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int rkvdec_s_ctrl(struct v4l2_ctrl *ctrl)
>>>>>> +{
>>>
>>> If there is a try_ctrl op specified, then the control framework
>>> will call that first before calling s_ctrl. So any validation that
>>> try_ctrl did does not need to be done again in s_ctrl.
>>>
>>> The same comment with try_ctrl is valid here as well: if there are
>>> image format checks that need to be done, then those need to be done
>>> per control and not as a generic check. If new controls are added in
>>> the future, then you don't want the same checks to apply to the new
>>> controls as well.
>>
>> I don't think the behaviour of try_ctrl and that being embedded in set
>> calls was being questioned by anyone. Can you reply to the last
>> paragraph below ?
>>
>>>
>>> Regards,
>>>
>>> 	Hans
>>>
>>>>>> +	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
>>>>>> +	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
>>>>>> +	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
>>>>>> +	enum rkvdec_image_fmt image_fmt;
>>>>>> +
>>>>>> +	if (!desc->ops->get_image_fmt)
>>>>>> +		return 0;
>>>>>> +
>>>>>> +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
>>>>>> +	if (ctx->image_fmt == image_fmt)
>>>>>> +		return 0;
>>>>>
>>>>> If you really can't set a control when the queue is busy, then that should
>>>>> be tested here, not in try_ctrl. And then you return -EBUSY.
>>>>>
>>>>> Am I missing something here?
>>>>
>>>> When I reviewed, I had imagine that s_ctrl on a request would just run
>>>> a try. Now that I read that more careful, I see that it does a true set
>>>> on separate copy. So yes, this can safely be moved here.
>>>>
>>>> Since you seem wondering "If you really can't set a control", let me
>>>> explain what Jonas wants to protect against. RKVdec does not have any
>>>> color conversion code, the header compound control (which header
>>>> depends on the codec), contains details such as sub-sampling and color
>>>> depth. Without color conversion, when the image format is locked (the
>>>> busy queue), you can't request the HW to decode a frame witch does not
>>>> fit. This could otherwise lead to buffer overflow in the HW,
>>>> fortunately protected by the iommu, but you don't really want to depend
>>>> on the mmu.
>>>>
>>>> I've never used try_ctrl in my decade of v4l2, so obviously, now that I
>>>> know that s_ctrl on request is not a try, I'm fine with rejecting this
>>>> PR, sending a new version and making a PR again. But if I was to use
>>>> this API in userspace, my intuitive expectation would be that this
>>>> should fail try(), even if its very rarely valid to check the queue
>>>> state in try control.
>>
>> Here, since we seem to disagree on the behaviour try should have for
>> this specific validation. What you asked on first pass is to make it so
>> that TRY will succeed, and SET will fail. I don't really like that
>> suggestion.

The s_ctrl here does not validate if the image_fmt is valid or not,
compared to try_ctrl, s_ctrl instead tries to do the opposite, if the
ctrl value cause image_fmt to change the decoded fmt is reset.

Something that is only valid when the queue is not busy, and because try
is called before set, I did not see any reason to do that check here as
set would never be called by core.

Please elaborate how this code is just wrong, it is only called for sps
ctrl, as intended, try will report an error as suggested in docs and set
will reset the decoded fmt to match the new sps ctrl value.

Regards,
Jonas

>>
>> Nicolas
>>
>>>>
>>>> Nicolas
>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>> 	Hans
>>>>>
>>>>>> +
>>>>>> +	ctx->image_fmt = image_fmt;
>>>>>> +	if (!rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, ctx->image_fmt))
>>>>>> +		rkvdec_reset_decoded_fmt(ctx);
>>>>>>  
>>>>>>  	return 0;
>>>>>>  }
>>>>>>  
>>>>>>  static const struct v4l2_ctrl_ops rkvdec_ctrl_ops = {
>>>>>>  	.try_ctrl = rkvdec_try_ctrl,
>>>>>> +	.s_ctrl = rkvdec_s_ctrl,
>>>>>>  };
>>>>>>  
>>>>>>  static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
>>>>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
>>>>>> index 6f8cf50c5d99aad2f52e321f54f3ca17166ddf98..e466a2753ccfc13738e0a672bc578e521af2c3f2 100644
>>>>>> --- a/drivers/staging/media/rkvdec/rkvdec.h
>>>>>> +++ b/drivers/staging/media/rkvdec/rkvdec.h
>>>>>> @@ -73,6 +73,8 @@ struct rkvdec_coded_fmt_ops {
>>>>>>  		     struct vb2_v4l2_buffer *dst_buf,
>>>>>>  		     enum vb2_buffer_state result);
>>>>>>  	int (*try_ctrl)(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl);
>>>>>> +	enum rkvdec_image_fmt (*get_image_fmt)(struct rkvdec_ctx *ctx,
>>>>>> +					       struct v4l2_ctrl *ctrl);
>>>>>>  };
>>>>>>  
>>>>>>  enum rkvdec_image_fmt {
>>>>>>
>>>>
>>
> 

Re: [PATCH v7 09/12] media: rkvdec: Add get_image_fmt ops

[Nicolas Dufresne]

Le lundi 07 avril 2025 à 17:07 +0200, Jonas Karlman a écrit :
> On 2025-04-07 16:59, Nicolas Dufresne wrote:
> > Le lundi 07 avril 2025 à 16:17 +0200, Hans Verkuil a écrit :
> > > On 07/04/2025 15:52, Nicolas Dufresne wrote:
> > > > Le lundi 07 avril 2025 à 13:09 +0200, Hans Verkuil a écrit :
> > > > > On 25/02/2025 10:40, Sebastian Fricke wrote:
> > > > > > From: Jonas Karlman <jonas@kwiboo.se>
> > > > > > 
> > > > > > Add support for a get_image_fmt() ops that returns the required image
> > > > > > format.
> > > > > > 
> > > > > > The CAPTURE format is reset when the required image format changes and
> > > > > > the buffer queue is not busy.
> > > > > > 
> > > > > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > > > > > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > > > > Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > > > > Tested-by: Christopher Obbard <chris.obbard@collabora.com>
> > > > > > ---
> > > > > >  drivers/staging/media/rkvdec/rkvdec.c | 49 +++++++++++++++++++++++++++++++++--
> > > > > >  drivers/staging/media/rkvdec/rkvdec.h |  2 ++
> > > > > >  2 files changed, 49 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> > > > > > index 70154948b4e32e2c439f259b0f1e1bbc8b52b063..5394079509305c619f1d0c1f542bfc409317c3b7 100644
> > > > > > --- a/drivers/staging/media/rkvdec/rkvdec.c
> > > > > > +++ b/drivers/staging/media/rkvdec/rkvdec.c
> > > > > > @@ -111,15 +111,60 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
> > > > > >  {
> > > > > >  	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
> > > > > >  	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
> > > > > > +	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
> > > > > > +	enum rkvdec_image_fmt image_fmt;
> > > > > > +	struct vb2_queue *vq;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	if (desc->ops->try_ctrl) {
> > > > > > +		ret = desc->ops->try_ctrl(ctx, ctrl);
> > > > > > +		if (ret)
> > > > > > +			return ret;
> > > > > > +	}
> > > > > > +
> > > > > > +	if (!desc->ops->get_image_fmt)
> > > > > > +		return 0;
> > > > > >  
> > > > > > -	if (desc->ops->try_ctrl)
> > > > > > -		return desc->ops->try_ctrl(ctx, ctrl);
> > > > > > +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
> > > > > > +	if (ctx->image_fmt == image_fmt)
> > > > > > +		return 0;
> > > > > > +
> > > > > > +	if (rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, image_fmt))
> > > > > > +		return 0;
> > > > > > +
> > > > > > +	/* format change not allowed when queue is busy */
> > > > > > +	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> > > > > > +			     V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> > > > > > +	if (vb2_is_busy(vq))
> > > > > > +		return -EINVAL;
> > > 
> > > Looking closer, this code is just wrong. It does these format change
> > > tests for any control, so if more controls are added in the future, then
> > > those will be checked the same way, which makes no sense.
> > 
> > "Just wrong" should be kept for code that is semantically incorrect,
> > just a suggestion for choice of wording.
> > 
> > > 
> > > These tests belong to the actual control that you 'try'. In this case
> > > rkvdec_h264_validate_sps(). This function already checks the width and
> > > height, but it should also check the image format. It is all in the
> > > wrong place.
> 
> Keep in mind that rkvdec_try_ctrl and rkvdec_s_ctrl are only used for
> CID_STATELESS_H264_SPS (and in future also CID_STATELESS_HEVC_SPS) not
> any other control, so this is already in the correct place?
> 
> Maybe the naming of the functions are too generic, they could be named
> rkvdec_sps_try_ctrl and rkvdec_sps_s_ctrl or similar if that makes more
> sense?

So we are missing that check for VP9? It will be needed for AV1 when
rkvdec2 support gets added.

Nicolas

> 
> Regards,
> Jonas
> 
> > 
> > We can do that too. Though, this was generalized since once you enable
> > the other codecs, you endup with code duplication. I know this series
> > is an extract from a larger one.
> > 
> > So let's suggest to make a helper that combines rkvdec_is_valid_fmt()
> > and the busy check. Though on that, please reply to my comment below
> > (which you skipped).
> > 
> > > 
> > > > > 
> > > > > This makes no sense to me. This just tries a control, and that should just
> > > > > work, regardless of vb2_is_busy(). It's a 'try', so you are not actually
> > > > > changing anything.
> > > > 
> > > > See comment below, notice that this code is only reached if the control
> > > > introduce parameters that are not compatible with the current capture
> > > > queue fmt. The entire function uses "success" early exit, so the
> > > > further down you get in the function, the less likely your control is
> > > > valid.
> > > > 
> > > > > 
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int rkvdec_s_ctrl(struct v4l2_ctrl *ctrl)
> > > > > > +{
> > > 
> > > If there is a try_ctrl op specified, then the control framework
> > > will call that first before calling s_ctrl. So any validation that
> > > try_ctrl did does not need to be done again in s_ctrl.
> > > 
> > > The same comment with try_ctrl is valid here as well: if there are
> > > image format checks that need to be done, then those need to be done
> > > per control and not as a generic check. If new controls are added in
> > > the future, then you don't want the same checks to apply to the new
> > > controls as well.
> > 
> > I don't think the behaviour of try_ctrl and that being embedded in set
> > calls was being questioned by anyone. Can you reply to the last
> > paragraph below ?
> > 
> > > 
> > > Regards,
> > > 
> > > 	Hans
> > > 
> > > > > > +	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
> > > > > > +	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
> > > > > > +	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
> > > > > > +	enum rkvdec_image_fmt image_fmt;
> > > > > > +
> > > > > > +	if (!desc->ops->get_image_fmt)
> > > > > > +		return 0;
> > > > > > +
> > > > > > +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
> > > > > > +	if (ctx->image_fmt == image_fmt)
> > > > > > +		return 0;
> > > > > 
> > > > > If you really can't set a control when the queue is busy, then that should
> > > > > be tested here, not in try_ctrl. And then you return -EBUSY.
> > > > > 
> > > > > Am I missing something here?
> > > > 
> > > > When I reviewed, I had imagine that s_ctrl on a request would just run
> > > > a try. Now that I read that more careful, I see that it does a true set
> > > > on separate copy. So yes, this can safely be moved here.
> > > > 
> > > > Since you seem wondering "If you really can't set a control", let me
> > > > explain what Jonas wants to protect against. RKVdec does not have any
> > > > color conversion code, the header compound control (which header
> > > > depends on the codec), contains details such as sub-sampling and color
> > > > depth. Without color conversion, when the image format is locked (the
> > > > busy queue), you can't request the HW to decode a frame witch does not
> > > > fit. This could otherwise lead to buffer overflow in the HW,
> > > > fortunately protected by the iommu, but you don't really want to depend
> > > > on the mmu.
> > > > 
> > > > I've never used try_ctrl in my decade of v4l2, so obviously, now that I
> > > > know that s_ctrl on request is not a try, I'm fine with rejecting this
> > > > PR, sending a new version and making a PR again. But if I was to use
> > > > this API in userspace, my intuitive expectation would be that this
> > > > should fail try(), even if its very rarely valid to check the queue
> > > > state in try control.
> > 
> > Here, since we seem to disagree on the behaviour try should have for
> > this specific validation. What you asked on first pass is to make it so
> > that TRY will succeed, and SET will fail. I don't really like that
> > suggestion.
> > 
> > Nicolas
> > 
> > > > 
> > > > Nicolas
> > > > 
> > > > > 
> > > > > Regards,
> > > > > 
> > > > > 	Hans
> > > > > 
> > > > > > +
> > > > > > +	ctx->image_fmt = image_fmt;
> > > > > > +	if (!rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, ctx->image_fmt))
> > > > > > +		rkvdec_reset_decoded_fmt(ctx);
> > > > > >  
> > > > > >  	return 0;
> > > > > >  }
> > > > > >  
> > > > > >  static const struct v4l2_ctrl_ops rkvdec_ctrl_ops = {
> > > > > >  	.try_ctrl = rkvdec_try_ctrl,
> > > > > > +	.s_ctrl = rkvdec_s_ctrl,
> > > > > >  };
> > > > > >  
> > > > > >  static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
> > > > > > diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
> > > > > > index 6f8cf50c5d99aad2f52e321f54f3ca17166ddf98..e466a2753ccfc13738e0a672bc578e521af2c3f2 100644
> > > > > > --- a/drivers/staging/media/rkvdec/rkvdec.h
> > > > > > +++ b/drivers/staging/media/rkvdec/rkvdec.h
> > > > > > @@ -73,6 +73,8 @@ struct rkvdec_coded_fmt_ops {
> > > > > >  		     struct vb2_v4l2_buffer *dst_buf,
> > > > > >  		     enum vb2_buffer_state result);
> > > > > >  	int (*try_ctrl)(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl);
> > > > > > +	enum rkvdec_image_fmt (*get_image_fmt)(struct rkvdec_ctx *ctx,
> > > > > > +					       struct v4l2_ctrl *ctrl);
> > > > > >  };
> > > > > >  
> > > > > >  enum rkvdec_image_fmt {
> > > > > > 
> > > > 
> > 

-- 
Nicolas Dufresne
Principal Engineer at Collabora

Re: [PATCH v7 09/12] media: rkvdec: Add get_image_fmt ops

[Jonas Karlman]

On 2025-04-07 17:35, Nicolas Dufresne wrote:
> Le lundi 07 avril 2025 à 17:07 +0200, Jonas Karlman a écrit :
>> On 2025-04-07 16:59, Nicolas Dufresne wrote:
>>> Le lundi 07 avril 2025 à 16:17 +0200, Hans Verkuil a écrit :
>>>> On 07/04/2025 15:52, Nicolas Dufresne wrote:
>>>>> Le lundi 07 avril 2025 à 13:09 +0200, Hans Verkuil a écrit :
>>>>>> On 25/02/2025 10:40, Sebastian Fricke wrote:
>>>>>>> From: Jonas Karlman <jonas@kwiboo.se>
>>>>>>>
>>>>>>> Add support for a get_image_fmt() ops that returns the required image
>>>>>>> format.
>>>>>>>
>>>>>>> The CAPTURE format is reset when the required image format changes and
>>>>>>> the buffer queue is not busy.
>>>>>>>
>>>>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>>>>>> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>>>>>> Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>>>>>> Tested-by: Christopher Obbard <chris.obbard@collabora.com>
>>>>>>> ---
>>>>>>>  drivers/staging/media/rkvdec/rkvdec.c | 49 +++++++++++++++++++++++++++++++++--
>>>>>>>  drivers/staging/media/rkvdec/rkvdec.h |  2 ++
>>>>>>>  2 files changed, 49 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
>>>>>>> index 70154948b4e32e2c439f259b0f1e1bbc8b52b063..5394079509305c619f1d0c1f542bfc409317c3b7 100644
>>>>>>> --- a/drivers/staging/media/rkvdec/rkvdec.c
>>>>>>> +++ b/drivers/staging/media/rkvdec/rkvdec.c
>>>>>>> @@ -111,15 +111,60 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
>>>>>>>  {
>>>>>>>  	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
>>>>>>>  	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
>>>>>>> +	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
>>>>>>> +	enum rkvdec_image_fmt image_fmt;
>>>>>>> +	struct vb2_queue *vq;
>>>>>>> +	int ret;
>>>>>>> +
>>>>>>> +	if (desc->ops->try_ctrl) {
>>>>>>> +		ret = desc->ops->try_ctrl(ctx, ctrl);
>>>>>>> +		if (ret)
>>>>>>> +			return ret;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	if (!desc->ops->get_image_fmt)
>>>>>>> +		return 0;
>>>>>>>  
>>>>>>> -	if (desc->ops->try_ctrl)
>>>>>>> -		return desc->ops->try_ctrl(ctx, ctrl);
>>>>>>> +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
>>>>>>> +	if (ctx->image_fmt == image_fmt)
>>>>>>> +		return 0;
>>>>>>> +
>>>>>>> +	if (rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, image_fmt))
>>>>>>> +		return 0;
>>>>>>> +
>>>>>>> +	/* format change not allowed when queue is busy */
>>>>>>> +	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
>>>>>>> +			     V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
>>>>>>> +	if (vb2_is_busy(vq))
>>>>>>> +		return -EINVAL;
>>>>
>>>> Looking closer, this code is just wrong. It does these format change
>>>> tests for any control, so if more controls are added in the future, then
>>>> those will be checked the same way, which makes no sense.
>>>
>>> "Just wrong" should be kept for code that is semantically incorrect,
>>> just a suggestion for choice of wording.
>>>
>>>>
>>>> These tests belong to the actual control that you 'try'. In this case
>>>> rkvdec_h264_validate_sps(). This function already checks the width and
>>>> height, but it should also check the image format. It is all in the
>>>> wrong place.
>>
>> Keep in mind that rkvdec_try_ctrl and rkvdec_s_ctrl are only used for
>> CID_STATELESS_H264_SPS (and in future also CID_STATELESS_HEVC_SPS) not
>> any other control, so this is already in the correct place?
>>
>> Maybe the naming of the functions are too generic, they could be named
>> rkvdec_sps_try_ctrl and rkvdec_sps_s_ctrl or similar if that makes more
>> sense?
> 
> So we are missing that check for VP9? It will be needed for AV1 when
> rkvdec2 support gets added.

Correct, it was not needed for VP9 on rkvdec1 as it only support a
single image fmt (NV12 / YUV420_8BIT).

In the code you only see rkvdec_ctrl_ops referenced once (or possible a
second time with HEVC work-in-progress patches).

	.cfg.id = V4L2_CID_STATELESS_H264_SPS,
	.cfg.ops = &rkvdec_ctrl_ops,

Regards,
Jonas

> 
> Nicolas
> 
>>
>> Regards,
>> Jonas
>>
>>>
>>> We can do that too. Though, this was generalized since once you enable
>>> the other codecs, you endup with code duplication. I know this series
>>> is an extract from a larger one.
>>>
>>> So let's suggest to make a helper that combines rkvdec_is_valid_fmt()
>>> and the busy check. Though on that, please reply to my comment below
>>> (which you skipped).
>>>
>>>>
>>>>>>
>>>>>> This makes no sense to me. This just tries a control, and that should just
>>>>>> work, regardless of vb2_is_busy(). It's a 'try', so you are not actually
>>>>>> changing anything.
>>>>>
>>>>> See comment below, notice that this code is only reached if the control
>>>>> introduce parameters that are not compatible with the current capture
>>>>> queue fmt. The entire function uses "success" early exit, so the
>>>>> further down you get in the function, the less likely your control is
>>>>> valid.
>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +	return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int rkvdec_s_ctrl(struct v4l2_ctrl *ctrl)
>>>>>>> +{
>>>>
>>>> If there is a try_ctrl op specified, then the control framework
>>>> will call that first before calling s_ctrl. So any validation that
>>>> try_ctrl did does not need to be done again in s_ctrl.
>>>>
>>>> The same comment with try_ctrl is valid here as well: if there are
>>>> image format checks that need to be done, then those need to be done
>>>> per control and not as a generic check. If new controls are added in
>>>> the future, then you don't want the same checks to apply to the new
>>>> controls as well.
>>>
>>> I don't think the behaviour of try_ctrl and that being embedded in set
>>> calls was being questioned by anyone. Can you reply to the last
>>> paragraph below ?
>>>
>>>>
>>>> Regards,
>>>>
>>>> 	Hans
>>>>
>>>>>>> +	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
>>>>>>> +	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
>>>>>>> +	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
>>>>>>> +	enum rkvdec_image_fmt image_fmt;
>>>>>>> +
>>>>>>> +	if (!desc->ops->get_image_fmt)
>>>>>>> +		return 0;
>>>>>>> +
>>>>>>> +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
>>>>>>> +	if (ctx->image_fmt == image_fmt)
>>>>>>> +		return 0;
>>>>>>
>>>>>> If you really can't set a control when the queue is busy, then that should
>>>>>> be tested here, not in try_ctrl. And then you return -EBUSY.
>>>>>>
>>>>>> Am I missing something here?
>>>>>
>>>>> When I reviewed, I had imagine that s_ctrl on a request would just run
>>>>> a try. Now that I read that more careful, I see that it does a true set
>>>>> on separate copy. So yes, this can safely be moved here.
>>>>>
>>>>> Since you seem wondering "If you really can't set a control", let me
>>>>> explain what Jonas wants to protect against. RKVdec does not have any
>>>>> color conversion code, the header compound control (which header
>>>>> depends on the codec), contains details such as sub-sampling and color
>>>>> depth. Without color conversion, when the image format is locked (the
>>>>> busy queue), you can't request the HW to decode a frame witch does not
>>>>> fit. This could otherwise lead to buffer overflow in the HW,
>>>>> fortunately protected by the iommu, but you don't really want to depend
>>>>> on the mmu.
>>>>>
>>>>> I've never used try_ctrl in my decade of v4l2, so obviously, now that I
>>>>> know that s_ctrl on request is not a try, I'm fine with rejecting this
>>>>> PR, sending a new version and making a PR again. But if I was to use
>>>>> this API in userspace, my intuitive expectation would be that this
>>>>> should fail try(), even if its very rarely valid to check the queue
>>>>> state in try control.
>>>
>>> Here, since we seem to disagree on the behaviour try should have for
>>> this specific validation. What you asked on first pass is to make it so
>>> that TRY will succeed, and SET will fail. I don't really like that
>>> suggestion.
>>>
>>> Nicolas
>>>
>>>>>
>>>>> Nicolas
>>>>>
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> 	Hans
>>>>>>
>>>>>>> +
>>>>>>> +	ctx->image_fmt = image_fmt;
>>>>>>> +	if (!rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, ctx->image_fmt))
>>>>>>> +		rkvdec_reset_decoded_fmt(ctx);
>>>>>>>  
>>>>>>>  	return 0;
>>>>>>>  }
>>>>>>>  
>>>>>>>  static const struct v4l2_ctrl_ops rkvdec_ctrl_ops = {
>>>>>>>  	.try_ctrl = rkvdec_try_ctrl,
>>>>>>> +	.s_ctrl = rkvdec_s_ctrl,
>>>>>>>  };
>>>>>>>  
>>>>>>>  static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
>>>>>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
>>>>>>> index 6f8cf50c5d99aad2f52e321f54f3ca17166ddf98..e466a2753ccfc13738e0a672bc578e521af2c3f2 100644
>>>>>>> --- a/drivers/staging/media/rkvdec/rkvdec.h
>>>>>>> +++ b/drivers/staging/media/rkvdec/rkvdec.h
>>>>>>> @@ -73,6 +73,8 @@ struct rkvdec_coded_fmt_ops {
>>>>>>>  		     struct vb2_v4l2_buffer *dst_buf,
>>>>>>>  		     enum vb2_buffer_state result);
>>>>>>>  	int (*try_ctrl)(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl);
>>>>>>> +	enum rkvdec_image_fmt (*get_image_fmt)(struct rkvdec_ctx *ctx,
>>>>>>> +					       struct v4l2_ctrl *ctrl);
>>>>>>>  };
>>>>>>>  
>>>>>>>  enum rkvdec_image_fmt {
>>>>>>>
>>>>>
>>>
> 

Re: [PATCH v7 09/12] media: rkvdec: Add get_image_fmt ops

[Hans Verkuil]

On 07/04/2025 16:59, Nicolas Dufresne wrote:
> Le lundi 07 avril 2025 à 16:17 +0200, Hans Verkuil a écrit :
>> On 07/04/2025 15:52, Nicolas Dufresne wrote:
>>> Le lundi 07 avril 2025 à 13:09 +0200, Hans Verkuil a écrit :
>>>> On 25/02/2025 10:40, Sebastian Fricke wrote:
>>>>> From: Jonas Karlman <jonas@kwiboo.se>
>>>>>
>>>>> Add support for a get_image_fmt() ops that returns the required image
>>>>> format.
>>>>>
>>>>> The CAPTURE format is reset when the required image format changes and
>>>>> the buffer queue is not busy.
>>>>>
>>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>>>> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>>>> Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>>>> Tested-by: Christopher Obbard <chris.obbard@collabora.com>
>>>>> ---
>>>>>  drivers/staging/media/rkvdec/rkvdec.c | 49 +++++++++++++++++++++++++++++++++--
>>>>>  drivers/staging/media/rkvdec/rkvdec.h |  2 ++
>>>>>  2 files changed, 49 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
>>>>> index 70154948b4e32e2c439f259b0f1e1bbc8b52b063..5394079509305c619f1d0c1f542bfc409317c3b7 100644
>>>>> --- a/drivers/staging/media/rkvdec/rkvdec.c
>>>>> +++ b/drivers/staging/media/rkvdec/rkvdec.c
>>>>> @@ -111,15 +111,60 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
>>>>>  {
>>>>>  	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
>>>>>  	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
>>>>> +	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
>>>>> +	enum rkvdec_image_fmt image_fmt;
>>>>> +	struct vb2_queue *vq;
>>>>> +	int ret;
>>>>> +
>>>>> +	if (desc->ops->try_ctrl) {
>>>>> +		ret = desc->ops->try_ctrl(ctx, ctrl);
>>>>> +		if (ret)
>>>>> +			return ret;
>>>>> +	}
>>>>> +
>>>>> +	if (!desc->ops->get_image_fmt)
>>>>> +		return 0;
>>>>>  
>>>>> -	if (desc->ops->try_ctrl)
>>>>> -		return desc->ops->try_ctrl(ctx, ctrl);
>>>>> +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
>>>>> +	if (ctx->image_fmt == image_fmt)
>>>>> +		return 0;
>>>>> +
>>>>> +	if (rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, image_fmt))
>>>>> +		return 0;
>>>>> +
>>>>> +	/* format change not allowed when queue is busy */
>>>>> +	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
>>>>> +			     V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
>>>>> +	if (vb2_is_busy(vq))
>>>>> +		return -EINVAL;
>>
>> Looking closer, this code is just wrong. It does these format change
>> tests for any control, so if more controls are added in the future, then
>> those will be checked the same way, which makes no sense.
> 
> "Just wrong" should be kept for code that is semantically incorrect,
> just a suggestion for choice of wording.

Having vb2_is_busy in a try function (whether trying a control or a format)
is simply wrong. Having these checks at a high level (i.e. being done for
any control) is asking for problems in the future. It only works right
now because there is just one control.

> 
>>
>> These tests belong to the actual control that you 'try'. In this case
>> rkvdec_h264_validate_sps(). This function already checks the width and
>> height, but it should also check the image format. It is all in the
>> wrong place.
> 
> We can do that too. Though, this was generalized since once you enable
> the other codecs, you endup with code duplication. I know this series
> is an extract from a larger one.
> 
> So let's suggest to make a helper that combines rkvdec_is_valid_fmt()
> and the busy check. Though on that, please reply to my comment below
> (which you skipped).

Absolutely, this needs a helper function.

> 
>>
>>>>
>>>> This makes no sense to me. This just tries a control, and that should just
>>>> work, regardless of vb2_is_busy(). It's a 'try', so you are not actually
>>>> changing anything.
>>>
>>> See comment below, notice that this code is only reached if the control
>>> introduce parameters that are not compatible with the current capture
>>> queue fmt. The entire function uses "success" early exit, so the
>>> further down you get in the function, the less likely your control is
>>> valid.
>>>
>>>>
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static int rkvdec_s_ctrl(struct v4l2_ctrl *ctrl)
>>>>> +{
>>
>> If there is a try_ctrl op specified, then the control framework
>> will call that first before calling s_ctrl. So any validation that
>> try_ctrl did does not need to be done again in s_ctrl.
>>
>> The same comment with try_ctrl is valid here as well: if there are
>> image format checks that need to be done, then those need to be done
>> per control and not as a generic check. If new controls are added in
>> the future, then you don't want the same checks to apply to the new
>> controls as well.
> 
> I don't think the behaviour of try_ctrl and that being embedded in set
> calls was being questioned by anyone. Can you reply to the last
> paragraph below ?
> 
>>
>> Regards,
>>
>> 	Hans
>>
>>>>> +	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
>>>>> +	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
>>>>> +	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
>>>>> +	enum rkvdec_image_fmt image_fmt;
>>>>> +
>>>>> +	if (!desc->ops->get_image_fmt)
>>>>> +		return 0;
>>>>> +
>>>>> +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
>>>>> +	if (ctx->image_fmt == image_fmt)
>>>>> +		return 0;
>>>>
>>>> If you really can't set a control when the queue is busy, then that should
>>>> be tested here, not in try_ctrl. And then you return -EBUSY.
>>>>
>>>> Am I missing something here?
>>>
>>> When I reviewed, I had imagine that s_ctrl on a request would just run
>>> a try. Now that I read that more careful, I see that it does a true set
>>> on separate copy. So yes, this can safely be moved here.
>>>
>>> Since you seem wondering "If you really can't set a control", let me
>>> explain what Jonas wants to protect against. RKVdec does not have any
>>> color conversion code, the header compound control (which header
>>> depends on the codec), contains details such as sub-sampling and color
>>> depth. Without color conversion, when the image format is locked (the
>>> busy queue), you can't request the HW to decode a frame witch does not
>>> fit. This could otherwise lead to buffer overflow in the HW,
>>> fortunately protected by the iommu, but you don't really want to depend
>>> on the mmu.
>>>
>>> I've never used try_ctrl in my decade of v4l2, so obviously, now that I
>>> know that s_ctrl on request is not a try, I'm fine with rejecting this
>>> PR, sending a new version and making a PR again. But if I was to use
>>> this API in userspace, my intuitive expectation would be that this
>>> should fail try(), even if its very rarely valid to check the queue
>>> state in try control.
> 
> Here, since we seem to disagree on the behaviour try should have for
> this specific validation. What you asked on first pass is to make it so
> that TRY will succeed, and SET will fail. I don't really like that
> suggestion.

Ah, no, that's not what I asked.

There are two independent issues:

1) The tests for a valid image format are done for all controls instead of
   just the control that really needs it. That's asking for problems, and
   that needs to be addressed by creating a helper function and using it
   in the relevant control code. Alternatively, just check against the
   control id in try_ctrl/s_ctrl explicitly. That's fine too, although I
   prefer a helper function.

2) vb2_is_busy() does not belong in try_ctrl. 'try' should never depend
   on whether buffers are allocated. You have two options here:

   a) try_ctrl checks if the image_fmt is valid for the current format,
      and it returns -EINVAL if it isn't. This requires that userspace
      then selects a different format first. No call to vb2_is_busy is
      needed.

   b) try_ctrl doesn't check image_fmt against the current format, it just
      accepts any value. Instead s_ctrl does the check: if it invalid, then
      it returns -EBUSY if vb2_is_busy() is true, or it updates the format.

I see that cedrus also has vb2_is_busy() in try_ctrl, and worse, it actually
updates the capture format in the try_ctrl, which is definitely a cedrus bug
(try should never have side-effects).

The core question is whether changing the V4L2_CID_STATELESS_H264_SPS should
make format changes. I can't off-hand think of any other control that does
that. It is certainly not documented.

The only control that comes close is V4L2_CID_ROTATE, and I think that control
was a huge mistake. It was also never properly documented how it should behave.

My preference is option a. Controls shouldn't change the format, it is really
confusing. If you do want option b, then all drivers that use this control
have to be checked first to ensure that they all behave the same, and the
control documentation must be updated.

Regards,

	Hans

> 
> Nicolas
> 
>>>
>>> Nicolas
>>>
>>>>
>>>> Regards,
>>>>
>>>> 	Hans
>>>>
>>>>> +
>>>>> +	ctx->image_fmt = image_fmt;
>>>>> +	if (!rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, ctx->image_fmt))
>>>>> +		rkvdec_reset_decoded_fmt(ctx);
>>>>>  
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>>  static const struct v4l2_ctrl_ops rkvdec_ctrl_ops = {
>>>>>  	.try_ctrl = rkvdec_try_ctrl,
>>>>> +	.s_ctrl = rkvdec_s_ctrl,
>>>>>  };
>>>>>  
>>>>>  static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
>>>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
>>>>> index 6f8cf50c5d99aad2f52e321f54f3ca17166ddf98..e466a2753ccfc13738e0a672bc578e521af2c3f2 100644
>>>>> --- a/drivers/staging/media/rkvdec/rkvdec.h
>>>>> +++ b/drivers/staging/media/rkvdec/rkvdec.h
>>>>> @@ -73,6 +73,8 @@ struct rkvdec_coded_fmt_ops {
>>>>>  		     struct vb2_v4l2_buffer *dst_buf,
>>>>>  		     enum vb2_buffer_state result);
>>>>>  	int (*try_ctrl)(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl);
>>>>> +	enum rkvdec_image_fmt (*get_image_fmt)(struct rkvdec_ctx *ctx,
>>>>> +					       struct v4l2_ctrl *ctrl);
>>>>>  };
>>>>>  
>>>>>  enum rkvdec_image_fmt {
>>>>>
>>>
> 

Re: [PATCH v7 09/12] media: rkvdec: Add get_image_fmt ops

[Nicolas Dufresne]

Hi Hans,

Le mardi 08 avril 2025 à 10:28 +0200, Hans Verkuil a écrit :
> On 07/04/2025 16:59, Nicolas Dufresne wrote:
> > Le lundi 07 avril 2025 à 16:17 +0200, Hans Verkuil a écrit :
> > > On 07/04/2025 15:52, Nicolas Dufresne wrote:
> > > > Le lundi 07 avril 2025 à 13:09 +0200, Hans Verkuil a écrit :
> > > > > On 25/02/2025 10:40, Sebastian Fricke wrote:
> > > > > > From: Jonas Karlman <jonas@kwiboo.se>
> > > > > > 
> > > > > > Add support for a get_image_fmt() ops that returns the required image
> > > > > > format.
> > > > > > 
> > > > > > The CAPTURE format is reset when the required image format changes and
> > > > > > the buffer queue is not busy.
> > > > > > 
> > > > > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > > > > > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > > > > Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > > > > Tested-by: Christopher Obbard <chris.obbard@collabora.com>
> > > > > > ---
> > > > > >  drivers/staging/media/rkvdec/rkvdec.c | 49 +++++++++++++++++++++++++++++++++--
> > > > > >  drivers/staging/media/rkvdec/rkvdec.h |  2 ++
> > > > > >  2 files changed, 49 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> > > > > > index 70154948b4e32e2c439f259b0f1e1bbc8b52b063..5394079509305c619f1d0c1f542bfc409317c3b7 100644
> > > > > > --- a/drivers/staging/media/rkvdec/rkvdec.c
> > > > > > +++ b/drivers/staging/media/rkvdec/rkvdec.c
> > > > > > @@ -111,15 +111,60 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
> > > > > >  {
> > > > > >  	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
> > > > > >  	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
> > > > > > +	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
> > > > > > +	enum rkvdec_image_fmt image_fmt;
> > > > > > +	struct vb2_queue *vq;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	if (desc->ops->try_ctrl) {
> > > > > > +		ret = desc->ops->try_ctrl(ctx, ctrl);
> > > > > > +		if (ret)
> > > > > > +			return ret;
> > > > > > +	}
> > > > > > +
> > > > > > +	if (!desc->ops->get_image_fmt)
> > > > > > +		return 0;
> > > > > >  
> > > > > > -	if (desc->ops->try_ctrl)
> > > > > > -		return desc->ops->try_ctrl(ctx, ctrl);
> > > > > > +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
> > > > > > +	if (ctx->image_fmt == image_fmt)
> > > > > > +		return 0;
> > > > > > +
> > > > > > +	if (rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, image_fmt))
> > > > > > +		return 0;
> > > > > > +
> > > > > > +	/* format change not allowed when queue is busy */
> > > > > > +	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
> > > > > > +			     V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> > > > > > +	if (vb2_is_busy(vq))
> > > > > > +		return -EINVAL;
> > > 
> > > Looking closer, this code is just wrong. It does these format change
> > > tests for any control, so if more controls are added in the future, then
> > > those will be checked the same way, which makes no sense.
> > 
> > "Just wrong" should be kept for code that is semantically incorrect,
> > just a suggestion for choice of wording.
> 
> Having vb2_is_busy in a try function (whether trying a control or a format)
> is simply wrong. Having these checks at a high level (i.e. being done for
> any control) is asking for problems in the future. It only works right
> now because there is just one control.

Your main rejection argument has been that this is done for any
control. Jonas invalidated your argument yesterday:

> Please elaborate how this code is just wrong, it is only called for sps
> ctrl, as intended, try will report an error as suggested in docs and set
> will reset the decoded fmt to match the new sps ctrl value.

> 
> > 
> > > 
> > > These tests belong to the actual control that you 'try'. In this case
> > > rkvdec_h264_validate_sps(). This function already checks the width and
> > > height, but it should also check the image format. It is all in the
> > > wrong place.
> > 
> > We can do that too. Though, this was generalized since once you enable
> > the other codecs, you endup with code duplication. I know this series
> > is an extract from a larger one.
> > 
> > So let's suggest to make a helper that combines rkvdec_is_valid_fmt()
> > and the busy check. Though on that, please reply to my comment below
> > (which you skipped).
> 
> Absolutely, this needs a helper function.

In the next version, We should ake sure this is renamed, so readers
understand its already a helper, and is only called for specific CID.
Jonas comment also invalid my wrong suggestion here.

> 
> > 
> > > 
> > > > > 
> > > > > This makes no sense to me. This just tries a control, and that should just
> > > > > work, regardless of vb2_is_busy(). It's a 'try', so you are not actually
> > > > > changing anything.
> > > > 
> > > > See comment below, notice that this code is only reached if the control
> > > > introduce parameters that are not compatible with the current capture
> > > > queue fmt. The entire function uses "success" early exit, so the
> > > > further down you get in the function, the less likely your control is
> > > > valid.
> > > > 
> > > > > 
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int rkvdec_s_ctrl(struct v4l2_ctrl *ctrl)
> > > > > > +{
> > > 
> > > If there is a try_ctrl op specified, then the control framework
> > > will call that first before calling s_ctrl. So any validation that
> > > try_ctrl did does not need to be done again in s_ctrl.
> > > 
> > > The same comment with try_ctrl is valid here as well: if there are
> > > image format checks that need to be done, then those need to be done
> > > per control and not as a generic check. If new controls are added in
> > > the future, then you don't want the same checks to apply to the new
> > > controls as well.
> > 
> > I don't think the behaviour of try_ctrl and that being embedded in set
> > calls was being questioned by anyone. Can you reply to the last
> > paragraph below ?
> > 
> > > 
> > > Regards,
> > > 
> > > 	Hans
> > > 
> > > > > > +	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
> > > > > > +	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
> > > > > > +	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
> > > > > > +	enum rkvdec_image_fmt image_fmt;
> > > > > > +
> > > > > > +	if (!desc->ops->get_image_fmt)
> > > > > > +		return 0;
> > > > > > +
> > > > > > +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
> > > > > > +	if (ctx->image_fmt == image_fmt)
> > > > > > +		return 0;
> > > > > 
> > > > > If you really can't set a control when the queue is busy, then that should
> > > > > be tested here, not in try_ctrl. And then you return -EBUSY.
> > > > > 
> > > > > Am I missing something here?
> > > > 
> > > > When I reviewed, I had imagine that s_ctrl on a request would just run
> > > > a try. Now that I read that more careful, I see that it does a true set
> > > > on separate copy. So yes, this can safely be moved here.
> > > > 
> > > > Since you seem wondering "If you really can't set a control", let me
> > > > explain what Jonas wants to protect against. RKVdec does not have any
> > > > color conversion code, the header compound control (which header
> > > > depends on the codec), contains details such as sub-sampling and color
> > > > depth. Without color conversion, when the image format is locked (the
> > > > busy queue), you can't request the HW to decode a frame witch does not
> > > > fit. This could otherwise lead to buffer overflow in the HW,
> > > > fortunately protected by the iommu, but you don't really want to depend
> > > > on the mmu.
> > > > 
> > > > I've never used try_ctrl in my decade of v4l2, so obviously, now that I
> > > > know that s_ctrl on request is not a try, I'm fine with rejecting this
> > > > PR, sending a new version and making a PR again. But if I was to use
> > > > this API in userspace, my intuitive expectation would be that this
> > > > should fail try(), even if its very rarely valid to check the queue
> > > > state in try control.
> > 
> > Here, since we seem to disagree on the behaviour try should have for
> > this specific validation. What you asked on first pass is to make it so
> > that TRY will succeed, and SET will fail. I don't really like that
> > suggestion.
> 
> Ah, no, that's not what I asked.
> 
> There are two independent issues:
> 
> 1) The tests for a valid image format are done for all controls instead of
>    just the control that really needs it. That's asking for problems, and
>    that needs to be addressed by creating a helper function and using it
>    in the relevant control code. Alternatively, just check against the
>    control id in try_ctrl/s_ctrl explicitly. That's fine too, although I
>    prefer a helper function.

This is false, this is done only the the relevant controls as explained
by Jonas.

> 
> 2) vb2_is_busy() does not belong in try_ctrl. 'try' should never depend
>    on whether buffers are allocated. You have two options here:

I read this statement as try_ctrl cannot fail when setting an SPS while
the queue is active. Since you don't have rationale for it, but really
want to see that, we will sacrifice the symmetry of TRY/SET in the next
version. TRY will pass, and SET will reset the capture format if the
queue is not busy, and return busy otherwise. Nobody ever wanted
try_ctrl for stateless decoders, its not even mention in the specific
documentation. This is effectively option b) below.

> 
>    a) try_ctrl checks if the image_fmt is valid for the current format,
>       and it returns -EINVAL if it isn't. This requires that userspace
>       then selects a different format first. No call to vb2_is_busy is
>       needed.

That shows you don't really know what this is about. Please read how
the initialization process works, up to point 2. A call to
S_FMT(CAPTURE) is optional. Its the driver that select the CAPTURE
format based on the bitstream parameters (and bitstream format /
CAPTURE). Everything is design with input and output in mind. The
application sets the input format and parameters, the driver choses the
output (CAPTURE queue) format. With the very strict rule that nothing
in the parameters that can be against the locked capture format can
ever be set.

https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/dev-stateless-decoder.html#initialization


> 
>    b) try_ctrl doesn't check image_fmt against the current format, it just
>       accepts any value. Instead s_ctrl does the check: if it invalid, then
>       it returns -EBUSY if vb2_is_busy() is true, or it updates the format.

That contradicts slightly your answer "Ah, no, that's not what I
asked.". But can be done without any spec violation like option a)
includes.

> 
> I see that cedrus also has vb2_is_busy() in try_ctrl, and worse, it actually
> updates the capture format in the try_ctrl, which is definitely a cedrus bug
> (try should never have side-effects).

I think looking at another work-in-progress driver is distraction. We
all know that try should not change the driver state (regardless the
type of try). If you are correct, then it should be fixed there too,
you should inform Jernej and Paul.

> 
> The core question is whether changing the V4L2_CID_STATELESS_H264_SPS should
> make format changes. I can't off-hand think of any other control that does
> that. It is certainly not documented.

That is also wrong, it is well documented. Its not because you don't
understand a problem that its by definition wrong.

> 
> The only control that comes close is V4L2_CID_ROTATE, and I think that control
> was a huge mistake. It was also never properly documented how it should behave.

Documentation says:
> Rotating the image to 90 and 270 will reverse the height and
> width of the display window.

> It is necessary to set the new height
> and width of the picture using the
> :ref:`VIDIOC_S_FMT <VIDIOC_G_FMT>` ioctl according to the
>  rotation angle selected.

The link is confusing, but S_ and G_ share the same link. So its well
documented that the users must call S_FMT and manually flip the width
and height.

I was never involved with that one, but its a very different approach.
I think its written with single queue in mind (not M2M). It means you
can have pending control state. This for stateless CODEC would be so
complex to handle. For request based driver, we should probably never
allow that kind of API. If you need to set a control in the future, use
a request. This, when that control should be applied becomes very
explicit and can be synchronized across multiple queues.

> 
> My preference is option a. Controls shouldn't change the format, it is really
> confusing. If you do want option b, then all drivers that use this control
> have to be checked first to ensure that they all behave the same, and the
> control documentation must be updated.

Option b) it is, since there is no option a).

Nicolas

> 
> Regards,
> 
> 	Hans
> 
> > 
> > Nicolas
> > 
> > > > 
> > > > Nicolas
> > > > 
> > > > > 
> > > > > Regards,
> > > > > 
> > > > > 	Hans
> > > > > 
> > > > > > +
> > > > > > +	ctx->image_fmt = image_fmt;
> > > > > > +	if (!rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, ctx->image_fmt))
> > > > > > +		rkvdec_reset_decoded_fmt(ctx);
> > > > > >  
> > > > > >  	return 0;
> > > > > >  }
> > > > > >  
> > > > > >  static const struct v4l2_ctrl_ops rkvdec_ctrl_ops = {
> > > > > >  	.try_ctrl = rkvdec_try_ctrl,
> > > > > > +	.s_ctrl = rkvdec_s_ctrl,
> > > > > >  };
> > > > > >  
> > > > > >  static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
> > > > > > diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
> > > > > > index 6f8cf50c5d99aad2f52e321f54f3ca17166ddf98..e466a2753ccfc13738e0a672bc578e521af2c3f2 100644
> > > > > > --- a/drivers/staging/media/rkvdec/rkvdec.h
> > > > > > +++ b/drivers/staging/media/rkvdec/rkvdec.h
> > > > > > @@ -73,6 +73,8 @@ struct rkvdec_coded_fmt_ops {
> > > > > >  		     struct vb2_v4l2_buffer *dst_buf,
> > > > > >  		     enum vb2_buffer_state result);
> > > > > >  	int (*try_ctrl)(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl);
> > > > > > +	enum rkvdec_image_fmt (*get_image_fmt)(struct rkvdec_ctx *ctx,
> > > > > > +					       struct v4l2_ctrl *ctrl);
> > > > > >  };
> > > > > >  
> > > > > >  enum rkvdec_image_fmt {
> > > > > > 
> > > > 
> > 

Re: [PATCH v7 09/12] media: rkvdec: Add get_image_fmt ops

[Hans Verkuil]

On 4/8/25 15:36, Nicolas Dufresne wrote:
> Hi Hans,
> 
> Le mardi 08 avril 2025 à 10:28 +0200, Hans Verkuil a écrit :
>> On 07/04/2025 16:59, Nicolas Dufresne wrote:
>>> Le lundi 07 avril 2025 à 16:17 +0200, Hans Verkuil a écrit :
>>>> On 07/04/2025 15:52, Nicolas Dufresne wrote:
>>>>> Le lundi 07 avril 2025 à 13:09 +0200, Hans Verkuil a écrit :
>>>>>> On 25/02/2025 10:40, Sebastian Fricke wrote:
>>>>>>> From: Jonas Karlman <jonas@kwiboo.se>
>>>>>>>
>>>>>>> Add support for a get_image_fmt() ops that returns the required image
>>>>>>> format.
>>>>>>>
>>>>>>> The CAPTURE format is reset when the required image format changes and
>>>>>>> the buffer queue is not busy.
>>>>>>>
>>>>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>>>>>> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>>>>>> Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>>>>>> Tested-by: Christopher Obbard <chris.obbard@collabora.com>
>>>>>>> ---
>>>>>>>  drivers/staging/media/rkvdec/rkvdec.c | 49 +++++++++++++++++++++++++++++++++--
>>>>>>>  drivers/staging/media/rkvdec/rkvdec.h |  2 ++
>>>>>>>  2 files changed, 49 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
>>>>>>> index 70154948b4e32e2c439f259b0f1e1bbc8b52b063..5394079509305c619f1d0c1f542bfc409317c3b7 100644
>>>>>>> --- a/drivers/staging/media/rkvdec/rkvdec.c
>>>>>>> +++ b/drivers/staging/media/rkvdec/rkvdec.c
>>>>>>> @@ -111,15 +111,60 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
>>>>>>>  {
>>>>>>>  	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
>>>>>>>  	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
>>>>>>> +	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
>>>>>>> +	enum rkvdec_image_fmt image_fmt;
>>>>>>> +	struct vb2_queue *vq;
>>>>>>> +	int ret;
>>>>>>> +
>>>>>>> +	if (desc->ops->try_ctrl) {
>>>>>>> +		ret = desc->ops->try_ctrl(ctx, ctrl);
>>>>>>> +		if (ret)
>>>>>>> +			return ret;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	if (!desc->ops->get_image_fmt)
>>>>>>> +		return 0;
>>>>>>>  
>>>>>>> -	if (desc->ops->try_ctrl)
>>>>>>> -		return desc->ops->try_ctrl(ctx, ctrl);
>>>>>>> +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
>>>>>>> +	if (ctx->image_fmt == image_fmt)
>>>>>>> +		return 0;
>>>>>>> +
>>>>>>> +	if (rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, image_fmt))
>>>>>>> +		return 0;
>>>>>>> +
>>>>>>> +	/* format change not allowed when queue is busy */
>>>>>>> +	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
>>>>>>> +			     V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
>>>>>>> +	if (vb2_is_busy(vq))
>>>>>>> +		return -EINVAL;
>>>>
>>>> Looking closer, this code is just wrong. It does these format change
>>>> tests for any control, so if more controls are added in the future, then
>>>> those will be checked the same way, which makes no sense.
>>>
>>> "Just wrong" should be kept for code that is semantically incorrect,
>>> just a suggestion for choice of wording.
>>
>> Having vb2_is_busy in a try function (whether trying a control or a format)
>> is simply wrong. Having these checks at a high level (i.e. being done for
>> any control) is asking for problems in the future. It only works right
>> now because there is just one control.
> 
> Your main rejection argument has been that this is done for any
> control. Jonas invalidated your argument yesterday:
> 
>> Please elaborate how this code is just wrong, it is only called for sps
>> ctrl, as intended, try will report an error as suggested in docs and set
>> will reset the decoded fmt to match the new sps ctrl value.

1) it is confusing since in this function there is no indication that there
   is just one control.
2) it is not future proof in case more controls are added.

When I was reviewing this I had to dig into the code before I realized that
there really was just one control.

So while this code is 'correct' in that it won't break, it is really hard
to understand.

> 
>>
>>>
>>>>
>>>> These tests belong to the actual control that you 'try'. In this case
>>>> rkvdec_h264_validate_sps(). This function already checks the width and
>>>> height, but it should also check the image format. It is all in the
>>>> wrong place.
>>>
>>> We can do that too. Though, this was generalized since once you enable
>>> the other codecs, you endup with code duplication. I know this series
>>> is an extract from a larger one.
>>>
>>> So let's suggest to make a helper that combines rkvdec_is_valid_fmt()
>>> and the busy check. Though on that, please reply to my comment below
>>> (which you skipped).
>>
>> Absolutely, this needs a helper function.
> 
> In the next version, We should ake sure this is renamed, so readers
> understand its already a helper, and is only called for specific CID.
> Jonas comment also invalid my wrong suggestion here.
> 
>>
>>>
>>>>
>>>>>>
>>>>>> This makes no sense to me. This just tries a control, and that should just
>>>>>> work, regardless of vb2_is_busy(). It's a 'try', so you are not actually
>>>>>> changing anything.
>>>>>
>>>>> See comment below, notice that this code is only reached if the control
>>>>> introduce parameters that are not compatible with the current capture
>>>>> queue fmt. The entire function uses "success" early exit, so the
>>>>> further down you get in the function, the less likely your control is
>>>>> valid.
>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +	return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int rkvdec_s_ctrl(struct v4l2_ctrl *ctrl)
>>>>>>> +{
>>>>
>>>> If there is a try_ctrl op specified, then the control framework
>>>> will call that first before calling s_ctrl. So any validation that
>>>> try_ctrl did does not need to be done again in s_ctrl.
>>>>
>>>> The same comment with try_ctrl is valid here as well: if there are
>>>> image format checks that need to be done, then those need to be done
>>>> per control and not as a generic check. If new controls are added in
>>>> the future, then you don't want the same checks to apply to the new
>>>> controls as well.
>>>
>>> I don't think the behaviour of try_ctrl and that being embedded in set
>>> calls was being questioned by anyone. Can you reply to the last
>>> paragraph below ?
>>>
>>>>
>>>> Regards,
>>>>
>>>> 	Hans
>>>>
>>>>>>> +	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
>>>>>>> +	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
>>>>>>> +	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
>>>>>>> +	enum rkvdec_image_fmt image_fmt;
>>>>>>> +
>>>>>>> +	if (!desc->ops->get_image_fmt)
>>>>>>> +		return 0;
>>>>>>> +
>>>>>>> +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
>>>>>>> +	if (ctx->image_fmt == image_fmt)
>>>>>>> +		return 0;
>>>>>>
>>>>>> If you really can't set a control when the queue is busy, then that should
>>>>>> be tested here, not in try_ctrl. And then you return -EBUSY.
>>>>>>
>>>>>> Am I missing something here?
>>>>>
>>>>> When I reviewed, I had imagine that s_ctrl on a request would just run
>>>>> a try. Now that I read that more careful, I see that it does a true set
>>>>> on separate copy. So yes, this can safely be moved here.
>>>>>
>>>>> Since you seem wondering "If you really can't set a control", let me
>>>>> explain what Jonas wants to protect against. RKVdec does not have any
>>>>> color conversion code, the header compound control (which header
>>>>> depends on the codec), contains details such as sub-sampling and color
>>>>> depth. Without color conversion, when the image format is locked (the
>>>>> busy queue), you can't request the HW to decode a frame witch does not
>>>>> fit. This could otherwise lead to buffer overflow in the HW,
>>>>> fortunately protected by the iommu, but you don't really want to depend
>>>>> on the mmu.
>>>>>
>>>>> I've never used try_ctrl in my decade of v4l2, so obviously, now that I
>>>>> know that s_ctrl on request is not a try, I'm fine with rejecting this
>>>>> PR, sending a new version and making a PR again. But if I was to use
>>>>> this API in userspace, my intuitive expectation would be that this
>>>>> should fail try(), even if its very rarely valid to check the queue
>>>>> state in try control.
>>>
>>> Here, since we seem to disagree on the behaviour try should have for
>>> this specific validation. What you asked on first pass is to make it so
>>> that TRY will succeed, and SET will fail. I don't really like that
>>> suggestion.
>>
>> Ah, no, that's not what I asked.
>>
>> There are two independent issues:
>>
>> 1) The tests for a valid image format are done for all controls instead of
>>    just the control that really needs it. That's asking for problems, and
>>    that needs to be addressed by creating a helper function and using it
>>    in the relevant control code. Alternatively, just check against the
>>    control id in try_ctrl/s_ctrl explicitly. That's fine too, although I
>>    prefer a helper function.
> 
> This is false, this is done only the the relevant controls as explained
> by Jonas.

See my comment above. It's not at all obvious that there is just one control,
it is just bad coding practice. All I ask is that it is made explicit in the
code that it is just for one control.

> 
>>
>> 2) vb2_is_busy() does not belong in try_ctrl. 'try' should never depend
>>    on whether buffers are allocated. You have two options here:
> 
> I read this statement as try_ctrl cannot fail when setting an SPS while
> the queue is active. Since you don't have rationale for it, but really
> want to see that, we will sacrifice the symmetry of TRY/SET in the next
> version. TRY will pass, and SET will reset the capture format if the
> queue is not busy, and return busy otherwise. Nobody ever wanted
> try_ctrl for stateless decoders, its not even mention in the specific
> documentation. This is effectively option b) below.

VIDIOC_TRY_EXT_CTRLS is always available. Typically it just validates controls
(i.e. make sure the values are in range, etc). It is really rare that drivers
need to implement try_ctrl.

It is also called by VIDIOC_S_EXT_CTRLS: this avoids that the same validation code is
implemented in both the try_ctrl and s_ctrl callbacks.

Unless otherwise stated, controls are independent of whether you have buffers
allocate or are streaming, you can get/set them any time.

> 
>>
>>    a) try_ctrl checks if the image_fmt is valid for the current format,
>>       and it returns -EINVAL if it isn't. This requires that userspace
>>       then selects a different format first. No call to vb2_is_busy is
>>       needed.
> 
> That shows you don't really know what this is about. Please read how
> the initialization process works, up to point 2. A call to

You are absolutely right. I should know how it works, but because I rarely
use it, I forget the details.

> S_FMT(CAPTURE) is optional. Its the driver that select the CAPTURE
> format based on the bitstream parameters (and bitstream format /
> CAPTURE). Everything is design with input and output in mind. The
> application sets the input format and parameters, the driver choses the
> output (CAPTURE queue) format. With the very strict rule that nothing
> in the parameters that can be against the locked capture format can
> ever be set.
> 
> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/dev-stateless-decoder.html#initialization
> 
> 
>>
>>    b) try_ctrl doesn't check image_fmt against the current format, it just
>>       accepts any value. Instead s_ctrl does the check: if it invalid, then
>>       it returns -EBUSY if vb2_is_busy() is true, or it updates the format.
> 
> That contradicts slightly your answer "Ah, no, that's not what I
> asked.". But can be done without any spec violation like option a)
> includes.
> 
>>
>> I see that cedrus also has vb2_is_busy() in try_ctrl, and worse, it actually
>> updates the capture format in the try_ctrl, which is definitely a cedrus bug
>> (try should never have side-effects).
> 
> I think looking at another work-in-progress driver is distraction. We
> all know that try should not change the driver state (regardless the
> type of try). If you are correct, then it should be fixed there too,
> you should inform Jernej and Paul.

And I will. Cedrus is not really a work-in-progress driver, I think it should
be moved out of staging. See the topic for the media summit.

In any case, if we can agree on the right approach for rkvdec, then I can make
a patch for cedrus.

> 
>>
>> The core question is whether changing the V4L2_CID_STATELESS_H264_SPS should
>> make format changes. I can't off-hand think of any other control that does
>> that. It is certainly not documented.
> 
> That is also wrong, it is well documented. Its not because you don't
> understand a problem that its by definition wrong.

It's documented in the stateless decoder doc:

https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/dev-stateless-decoder.html

But not with the control documentation itself:

https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/ext-ctrls-codec-stateless.html

I was looking at that, and doesn't mention it. I'll see if I can make a patch for that.

> 
>>
>> The only control that comes close is V4L2_CID_ROTATE, and I think that control
>> was a huge mistake. It was also never properly documented how it should behave.
> 
> Documentation says:
>> Rotating the image to 90 and 270 will reverse the height and
>> width of the display window.
> 
>> It is necessary to set the new height
>> and width of the picture using the
>> :ref:`VIDIOC_S_FMT <VIDIOC_G_FMT>` ioctl according to the
>>  rotation angle selected.
> 
> The link is confusing, but S_ and G_ share the same link. So its well
> documented that the users must call S_FMT and manually flip the width
> and height.

Not really, if you look at how it is implemented in various drivers, then
they don't match this doc in at least several cases. In any case, just forget
about this control. It's all water under the bridge.

> 
> I was never involved with that one, but its a very different approach.
> I think its written with single queue in mind (not M2M). It means you
> can have pending control state. This for stateless CODEC would be so
> complex to handle. For request based driver, we should probably never
> allow that kind of API. If you need to set a control in the future, use
> a request. This, when that control should be applied becomes very
> explicit and can be synchronized across multiple queues.
> 
>>
>> My preference is option a. Controls shouldn't change the format, it is really
>> confusing. If you do want option b, then all drivers that use this control
>> have to be checked first to ensure that they all behave the same, and the
>> control documentation must be updated.
> 
> Option b) it is, since there is no option a).

So can we agree on the following (I think):

1) rkvdec_try_ctrl no longer checks the image_fmt. Effectively this means that there
   is no longer any need to change rkvdec_try_ctrl in this patch.

2) in rkvdec_s_ctrl we do the image_fmt check: if it changes, but vb2_is_busy is true,
   then return -EBUSY, otherwise call rkvdec_reset_decoded_fmt(). This code is specific
   for V4L2_CID_STATELESS_H264_SPS, so just make sure it is under an if/switch for that
   control ID.

3) I'll see if I can make a patch to clarify in the control documentation that setting
   it can change the format.

4) I'll make a patch for the cedrus driver as well to align with the approach in rkvdec.

Regards,

	Hans

> 
> Nicolas
> 
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>> Nicolas
>>>
>>>>>
>>>>> Nicolas
>>>>>
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> 	Hans
>>>>>>
>>>>>>> +
>>>>>>> +	ctx->image_fmt = image_fmt;
>>>>>>> +	if (!rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, ctx->image_fmt))
>>>>>>> +		rkvdec_reset_decoded_fmt(ctx);
>>>>>>>  
>>>>>>>  	return 0;
>>>>>>>  }
>>>>>>>  
>>>>>>>  static const struct v4l2_ctrl_ops rkvdec_ctrl_ops = {
>>>>>>>  	.try_ctrl = rkvdec_try_ctrl,
>>>>>>> +	.s_ctrl = rkvdec_s_ctrl,
>>>>>>>  };
>>>>>>>  
>>>>>>>  static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
>>>>>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
>>>>>>> index 6f8cf50c5d99aad2f52e321f54f3ca17166ddf98..e466a2753ccfc13738e0a672bc578e521af2c3f2 100644
>>>>>>> --- a/drivers/staging/media/rkvdec/rkvdec.h
>>>>>>> +++ b/drivers/staging/media/rkvdec/rkvdec.h
>>>>>>> @@ -73,6 +73,8 @@ struct rkvdec_coded_fmt_ops {
>>>>>>>  		     struct vb2_v4l2_buffer *dst_buf,
>>>>>>>  		     enum vb2_buffer_state result);
>>>>>>>  	int (*try_ctrl)(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl);
>>>>>>> +	enum rkvdec_image_fmt (*get_image_fmt)(struct rkvdec_ctx *ctx,
>>>>>>> +					       struct v4l2_ctrl *ctrl);
>>>>>>>  };
>>>>>>>  
>>>>>>>  enum rkvdec_image_fmt {
>>>>>>>
>>>>>
>>>

Re: [PATCH v7 09/12] media: rkvdec: Add get_image_fmt ops

[Jonas Karlman]

On 2025-04-08 16:32, Hans Verkuil wrote:
> On 4/8/25 15:36, Nicolas Dufresne wrote:
>> Hi Hans,
>>
>> Le mardi 08 avril 2025 à 10:28 +0200, Hans Verkuil a écrit :
>>> On 07/04/2025 16:59, Nicolas Dufresne wrote:
>>>> Le lundi 07 avril 2025 à 16:17 +0200, Hans Verkuil a écrit :
>>>>> On 07/04/2025 15:52, Nicolas Dufresne wrote:
>>>>>> Le lundi 07 avril 2025 à 13:09 +0200, Hans Verkuil a écrit :
>>>>>>> On 25/02/2025 10:40, Sebastian Fricke wrote:
>>>>>>>> From: Jonas Karlman <jonas@kwiboo.se>
>>>>>>>>
>>>>>>>> Add support for a get_image_fmt() ops that returns the required image
>>>>>>>> format.
>>>>>>>>
>>>>>>>> The CAPTURE format is reset when the required image format changes and
>>>>>>>> the buffer queue is not busy.
>>>>>>>>
>>>>>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>>>>>>> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>>>>>>> Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
>>>>>>>> Tested-by: Christopher Obbard <chris.obbard@collabora.com>
>>>>>>>> ---
>>>>>>>>  drivers/staging/media/rkvdec/rkvdec.c | 49 +++++++++++++++++++++++++++++++++--
>>>>>>>>  drivers/staging/media/rkvdec/rkvdec.h |  2 ++
>>>>>>>>  2 files changed, 49 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
>>>>>>>> index 70154948b4e32e2c439f259b0f1e1bbc8b52b063..5394079509305c619f1d0c1f542bfc409317c3b7 100644
>>>>>>>> --- a/drivers/staging/media/rkvdec/rkvdec.c
>>>>>>>> +++ b/drivers/staging/media/rkvdec/rkvdec.c
>>>>>>>> @@ -111,15 +111,60 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
>>>>>>>>  {
>>>>>>>>  	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
>>>>>>>>  	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
>>>>>>>> +	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
>>>>>>>> +	enum rkvdec_image_fmt image_fmt;
>>>>>>>> +	struct vb2_queue *vq;
>>>>>>>> +	int ret;
>>>>>>>> +
>>>>>>>> +	if (desc->ops->try_ctrl) {
>>>>>>>> +		ret = desc->ops->try_ctrl(ctx, ctrl);
>>>>>>>> +		if (ret)
>>>>>>>> +			return ret;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	if (!desc->ops->get_image_fmt)
>>>>>>>> +		return 0;
>>>>>>>>  
>>>>>>>> -	if (desc->ops->try_ctrl)
>>>>>>>> -		return desc->ops->try_ctrl(ctx, ctrl);
>>>>>>>> +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
>>>>>>>> +	if (ctx->image_fmt == image_fmt)
>>>>>>>> +		return 0;
>>>>>>>> +
>>>>>>>> +	if (rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, image_fmt))
>>>>>>>> +		return 0;
>>>>>>>> +
>>>>>>>> +	/* format change not allowed when queue is busy */
>>>>>>>> +	vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx,
>>>>>>>> +			     V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
>>>>>>>> +	if (vb2_is_busy(vq))
>>>>>>>> +		return -EINVAL;
>>>>>
>>>>> Looking closer, this code is just wrong. It does these format change
>>>>> tests for any control, so if more controls are added in the future, then
>>>>> those will be checked the same way, which makes no sense.
>>>>
>>>> "Just wrong" should be kept for code that is semantically incorrect,
>>>> just a suggestion for choice of wording.
>>>
>>> Having vb2_is_busy in a try function (whether trying a control or a format)
>>> is simply wrong. Having these checks at a high level (i.e. being done for
>>> any control) is asking for problems in the future. It only works right
>>> now because there is just one control.
>>
>> Your main rejection argument has been that this is done for any
>> control. Jonas invalidated your argument yesterday:
>>
>>> Please elaborate how this code is just wrong, it is only called for sps
>>> ctrl, as intended, try will report an error as suggested in docs and set
>>> will reset the decoded fmt to match the new sps ctrl value.
> 
> 1) it is confusing since in this function there is no indication that there
>    is just one control.
> 2) it is not future proof in case more controls are added.
> 
> When I was reviewing this I had to dig into the code before I realized that
> there really was just one control.
> 
> So while this code is 'correct' in that it won't break, it is really hard
> to understand.

Technically this code currently support format validation for any ctrl
that passes this function, the implemented get_image_fmt() ops will
return ANY for a ctrl that does not affect the image fmt. And the
rkvdec_is_valid_fmt() will return that all image fmt is valid for ANY.

> 
>>
>>>
>>>>
>>>>>
>>>>> These tests belong to the actual control that you 'try'. In this case
>>>>> rkvdec_h264_validate_sps(). This function already checks the width and
>>>>> height, but it should also check the image format. It is all in the
>>>>> wrong place.
>>>>
>>>> We can do that too. Though, this was generalized since once you enable
>>>> the other codecs, you endup with code duplication. I know this series
>>>> is an extract from a larger one.
>>>>
>>>> So let's suggest to make a helper that combines rkvdec_is_valid_fmt()
>>>> and the busy check. Though on that, please reply to my comment below
>>>> (which you skipped).
>>>
>>> Absolutely, this needs a helper function.
>>
>> In the next version, We should ake sure this is renamed, so readers
>> understand its already a helper, and is only called for specific CID.
>> Jonas comment also invalid my wrong suggestion here.
>>
>>>
>>>>
>>>>>
>>>>>>>
>>>>>>> This makes no sense to me. This just tries a control, and that should just
>>>>>>> work, regardless of vb2_is_busy(). It's a 'try', so you are not actually
>>>>>>> changing anything.
>>>>>>
>>>>>> See comment below, notice that this code is only reached if the control
>>>>>> introduce parameters that are not compatible with the current capture
>>>>>> queue fmt. The entire function uses "success" early exit, so the
>>>>>> further down you get in the function, the less likely your control is
>>>>>> valid.
>>>>>>
>>>>>>>
>>>>>>>> +
>>>>>>>> +	return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int rkvdec_s_ctrl(struct v4l2_ctrl *ctrl)
>>>>>>>> +{
>>>>>
>>>>> If there is a try_ctrl op specified, then the control framework
>>>>> will call that first before calling s_ctrl. So any validation that
>>>>> try_ctrl did does not need to be done again in s_ctrl.
>>>>>
>>>>> The same comment with try_ctrl is valid here as well: if there are
>>>>> image format checks that need to be done, then those need to be done
>>>>> per control and not as a generic check. If new controls are added in
>>>>> the future, then you don't want the same checks to apply to the new
>>>>> controls as well.
>>>>
>>>> I don't think the behaviour of try_ctrl and that being embedded in set
>>>> calls was being questioned by anyone. Can you reply to the last
>>>> paragraph below ?
>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>> 	Hans
>>>>>
>>>>>>>> +	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
>>>>>>>> +	const struct rkvdec_coded_fmt_desc *desc = ctx->coded_fmt_desc;
>>>>>>>> +	struct v4l2_pix_format_mplane *pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
>>>>>>>> +	enum rkvdec_image_fmt image_fmt;
>>>>>>>> +
>>>>>>>> +	if (!desc->ops->get_image_fmt)
>>>>>>>> +		return 0;
>>>>>>>> +
>>>>>>>> +	image_fmt = desc->ops->get_image_fmt(ctx, ctrl);
>>>>>>>> +	if (ctx->image_fmt == image_fmt)
>>>>>>>> +		return 0;
>>>>>>>
>>>>>>> If you really can't set a control when the queue is busy, then that should
>>>>>>> be tested here, not in try_ctrl. And then you return -EBUSY.
>>>>>>>
>>>>>>> Am I missing something here?
>>>>>>
>>>>>> When I reviewed, I had imagine that s_ctrl on a request would just run
>>>>>> a try. Now that I read that more careful, I see that it does a true set
>>>>>> on separate copy. So yes, this can safely be moved here.
>>>>>>
>>>>>> Since you seem wondering "If you really can't set a control", let me
>>>>>> explain what Jonas wants to protect against. RKVdec does not have any
>>>>>> color conversion code, the header compound control (which header
>>>>>> depends on the codec), contains details such as sub-sampling and color
>>>>>> depth. Without color conversion, when the image format is locked (the
>>>>>> busy queue), you can't request the HW to decode a frame witch does not
>>>>>> fit. This could otherwise lead to buffer overflow in the HW,
>>>>>> fortunately protected by the iommu, but you don't really want to depend
>>>>>> on the mmu.
>>>>>>
>>>>>> I've never used try_ctrl in my decade of v4l2, so obviously, now that I
>>>>>> know that s_ctrl on request is not a try, I'm fine with rejecting this
>>>>>> PR, sending a new version and making a PR again. But if I was to use
>>>>>> this API in userspace, my intuitive expectation would be that this
>>>>>> should fail try(), even if its very rarely valid to check the queue
>>>>>> state in try control.
>>>>
>>>> Here, since we seem to disagree on the behaviour try should have for
>>>> this specific validation. What you asked on first pass is to make it so
>>>> that TRY will succeed, and SET will fail. I don't really like that
>>>> suggestion.
>>>
>>> Ah, no, that's not what I asked.
>>>
>>> There are two independent issues:
>>>
>>> 1) The tests for a valid image format are done for all controls instead of
>>>    just the control that really needs it. That's asking for problems, and
>>>    that needs to be addressed by creating a helper function and using it
>>>    in the relevant control code. Alternatively, just check against the
>>>    control id in try_ctrl/s_ctrl explicitly. That's fine too, although I
>>>    prefer a helper function.
>>
>> This is false, this is done only the the relevant controls as explained
>> by Jonas.
> 
> See my comment above. It's not at all obvious that there is just one control,
> it is just bad coding practice. All I ask is that it is made explicit in the
> code that it is just for one control.
> 
>>
>>>
>>> 2) vb2_is_busy() does not belong in try_ctrl. 'try' should never depend
>>>    on whether buffers are allocated. You have two options here:
>>
>> I read this statement as try_ctrl cannot fail when setting an SPS while
>> the queue is active. Since you don't have rationale for it, but really
>> want to see that, we will sacrifice the symmetry of TRY/SET in the next
>> version. TRY will pass, and SET will reset the capture format if the
>> queue is not busy, and return busy otherwise. Nobody ever wanted
>> try_ctrl for stateless decoders, its not even mention in the specific
>> documentation. This is effectively option b) below.
> 
> VIDIOC_TRY_EXT_CTRLS is always available. Typically it just validates controls
> (i.e. make sure the values are in range, etc). It is really rare that drivers
> need to implement try_ctrl.
> 
> It is also called by VIDIOC_S_EXT_CTRLS: this avoids that the same validation code is
> implemented in both the try_ctrl and s_ctrl callbacks.
> 
> Unless otherwise stated, controls are independent of whether you have buffers
> allocate or are streaming, you can get/set them any time.
> 
>>
>>>
>>>    a) try_ctrl checks if the image_fmt is valid for the current format,
>>>       and it returns -EINVAL if it isn't. This requires that userspace
>>>       then selects a different format first. No call to vb2_is_busy is
>>>       needed.
>>
>> That shows you don't really know what this is about. Please read how
>> the initialization process works, up to point 2. A call to
> 
> You are absolutely right. I should know how it works, but because I rarely
> use it, I forget the details.
> 
>> S_FMT(CAPTURE) is optional. Its the driver that select the CAPTURE
>> format based on the bitstream parameters (and bitstream format /
>> CAPTURE). Everything is design with input and output in mind. The
>> application sets the input format and parameters, the driver choses the
>> output (CAPTURE queue) format. With the very strict rule that nothing
>> in the parameters that can be against the locked capture format can
>> ever be set.
>>
>> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/dev-stateless-decoder.html#initialization
>>
>>
>>>
>>>    b) try_ctrl doesn't check image_fmt against the current format, it just
>>>       accepts any value. Instead s_ctrl does the check: if it invalid, then
>>>       it returns -EBUSY if vb2_is_busy() is true, or it updates the format.
>>
>> That contradicts slightly your answer "Ah, no, that's not what I
>> asked.". But can be done without any spec violation like option a)
>> includes.
>>
>>>
>>> I see that cedrus also has vb2_is_busy() in try_ctrl, and worse, it actually
>>> updates the capture format in the try_ctrl, which is definitely a cedrus bug
>>> (try should never have side-effects).
>>
>> I think looking at another work-in-progress driver is distraction. We
>> all know that try should not change the driver state (regardless the
>> type of try). If you are correct, then it should be fixed there too,
>> you should inform Jernej and Paul.
> 
> And I will. Cedrus is not really a work-in-progress driver, I think it should
> be moved out of staging. See the topic for the media summit.
> 
> In any case, if we can agree on the right approach for rkvdec, then I can make
> a patch for cedrus.
> 
>>
>>>
>>> The core question is whether changing the V4L2_CID_STATELESS_H264_SPS should
>>> make format changes. I can't off-hand think of any other control that does
>>> that. It is certainly not documented.
>>
>> That is also wrong, it is well documented. Its not because you don't
>> understand a problem that its by definition wrong.
> 
> It's documented in the stateless decoder doc:
> 
> https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/dev-stateless-decoder.html
> 
> But not with the control documentation itself:
> 
> https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/ext-ctrls-codec-stateless.html
> 
> I was looking at that, and doesn't mention it. I'll see if I can make a patch for that.
> 
>>
>>>
>>> The only control that comes close is V4L2_CID_ROTATE, and I think that control
>>> was a huge mistake. It was also never properly documented how it should behave.
>>
>> Documentation says:
>>> Rotating the image to 90 and 270 will reverse the height and
>>> width of the display window.
>>
>>> It is necessary to set the new height
>>> and width of the picture using the
>>> :ref:`VIDIOC_S_FMT <VIDIOC_G_FMT>` ioctl according to the
>>>  rotation angle selected.
>>
>> The link is confusing, but S_ and G_ share the same link. So its well
>> documented that the users must call S_FMT and manually flip the width
>> and height.
> 
> Not really, if you look at how it is implemented in various drivers, then
> they don't match this doc in at least several cases. In any case, just forget
> about this control. It's all water under the bridge.
> 
>>
>> I was never involved with that one, but its a very different approach.
>> I think its written with single queue in mind (not M2M). It means you
>> can have pending control state. This for stateless CODEC would be so
>> complex to handle. For request based driver, we should probably never
>> allow that kind of API. If you need to set a control in the future, use
>> a request. This, when that control should be applied becomes very
>> explicit and can be synchronized across multiple queues.
>>
>>>
>>> My preference is option a. Controls shouldn't change the format, it is really
>>> confusing. If you do want option b, then all drivers that use this control
>>> have to be checked first to ensure that they all behave the same, and the
>>> control documentation must be updated.
>>
>> Option b) it is, since there is no option a).
> 
> So can we agree on the following (I think):
> 
> 1) rkvdec_try_ctrl no longer checks the image_fmt. Effectively this means that there
>    is no longer any need to change rkvdec_try_ctrl in this patch.
> 
> 2) in rkvdec_s_ctrl we do the image_fmt check: if it changes, but vb2_is_busy is true,
>    then return -EBUSY, otherwise call rkvdec_reset_decoded_fmt(). This code is specific
>    for V4L2_CID_STATELESS_H264_SPS, so just make sure it is under an if/switch for that
>    control ID.

As mentioned above, the get_image_fmt() ops currently can decide itself
if the ctrl being passed affect the image fmt or just return ANY.
Current ops return ANY for all ctrl beside STATELESS_H264_SPS.

The rational behind this separation was that the decision on image fmt
could solely be handled in the codec specific code file, as a way to
reduce any possible future issue when adding a new codec or a ctrl that
put limits on the buffer or image fmt.

Having a check and only call this for STATELESS_H264_SPS ctrl may
instead complicate things going forward, as both the common and codec
specific code will need to be changed when implementing next codec.

This also matches the way rkvdec try_ctrl() ops is currently implemented,
the decision on what ctrl it checks is decided inside the codec specific
implementation. And for me, consistency is very important ;-)

Regards,
Jonas

> 
> 3) I'll see if I can make a patch to clarify in the control documentation that setting
>    it can change the format.
> 
> 4) I'll make a patch for the cedrus driver as well to align with the approach in rkvdec.
> 
> Regards,
> 
> 	Hans
> 
>>
>> Nicolas
>>
>>>
>>> Regards,
>>>
>>> 	Hans
>>>
>>>>
>>>> Nicolas
>>>>
>>>>>>
>>>>>> Nicolas
>>>>>>
>>>>>>>
>>>>>>> Regards,
>>>>>>>
>>>>>>> 	Hans
>>>>>>>
>>>>>>>> +
>>>>>>>> +	ctx->image_fmt = image_fmt;
>>>>>>>> +	if (!rkvdec_is_valid_fmt(ctx, pix_mp->pixelformat, ctx->image_fmt))
>>>>>>>> +		rkvdec_reset_decoded_fmt(ctx);
>>>>>>>>  
>>>>>>>>  	return 0;
>>>>>>>>  }
>>>>>>>>  
>>>>>>>>  static const struct v4l2_ctrl_ops rkvdec_ctrl_ops = {
>>>>>>>>  	.try_ctrl = rkvdec_try_ctrl,
>>>>>>>> +	.s_ctrl = rkvdec_s_ctrl,
>>>>>>>>  };
>>>>>>>>  
>>>>>>>>  static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
>>>>>>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
>>>>>>>> index 6f8cf50c5d99aad2f52e321f54f3ca17166ddf98..e466a2753ccfc13738e0a672bc578e521af2c3f2 100644
>>>>>>>> --- a/drivers/staging/media/rkvdec/rkvdec.h
>>>>>>>> +++ b/drivers/staging/media/rkvdec/rkvdec.h
>>>>>>>> @@ -73,6 +73,8 @@ struct rkvdec_coded_fmt_ops {
>>>>>>>>  		     struct vb2_v4l2_buffer *dst_buf,
>>>>>>>>  		     enum vb2_buffer_state result);
>>>>>>>>  	int (*try_ctrl)(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl);
>>>>>>>> +	enum rkvdec_image_fmt (*get_image_fmt)(struct rkvdec_ctx *ctx,
>>>>>>>> +					       struct v4l2_ctrl *ctrl);
>>>>>>>>  };
>>>>>>>>  
>>>>>>>>  enum rkvdec_image_fmt {
>>>>>>>>
>>>>>>
>>>>

Re: [PATCH v7 09/12] media: rkvdec: Add get_image_fmt ops

[Nicolas Dufresne]

(just trimmed a bit)

Le mardi 08 avril 2025 à 16:32 +0200, Hans Verkuil a écrit :
> So can we agree on the following (I think):
> 
> 1) rkvdec_try_ctrl no longer checks the image_fmt. Effectively this means that there
>    is no longer any need to change rkvdec_try_ctrl in this patch.

Correct.

> 
> 2) in rkvdec_s_ctrl we do the image_fmt check: if it changes, but vb2_is_busy is true,
>    then return -EBUSY, otherwise call rkvdec_reset_decoded_fmt(). This code is specific
>    for V4L2_CID_STATELESS_H264_SPS, so just make sure it is under an if/switch for that
>    control ID.

Correct. In practice, the IOCTL implementation function will be renamed
to what Jonas proposed (translated to s_ctrl instead of try), with some
name like s_sps_ctrl() (can't remember exactly). This is because
upcoming HEVC will endup sharing this code. Its going to be 1 control
per codec that supports 10bit.

> 
> 3) I'll see if I can make a patch to clarify in the control documentation that setting
>    it can change the format.

Thanks.

> 
> 4) I'll make a patch for the cedrus driver as well to align with the approach in rkvdec.

Thank you ! Glad we got to the bottom of it. As for rkvdec TODO, I will
prepare an upstaging patch series, and fix anything needed.

Nicolas