* [PATCH 0/6] RkVDEC HEVC driver
@ 2022-07-13 16:24 Sebastian Fricke
2022-07-13 16:24 ` [PATCH 1/6] media: v4l2: Add NV15 pixel format Sebastian Fricke
` (4 more replies)
0 siblings, 5 replies; 17+ messages in thread
From: Sebastian Fricke @ 2022-07-13 16:24 UTC (permalink / raw)
To: linux-media
Cc: jernej.skrabec, knaerzche, kernel, bob.beckett, ezequiel,
mchehab, gregkh, linux-kernel, linux-rockchip, linux-staging,
nicolas.dufresne, Sebastian Fricke, Yury Norov, Andy Shevchenko,
Rasmus Villemoes
Implement the HEVC codec variation for the RkVDEC driver. Currently only
the RK3399 is supported, but it is possible to enable the RK3288 as it
also supports this codec.
Based on top of the media tree @ef7fcbbb9eabbe86d2287484bf366dd1821cc6b8
and the HEVC uABI MR by Benjamin Gaignard.
(https://patchwork.linuxtv.org/project/linux-media/list/?series=8360)
Tested with the GStreamer V4L2 HEVC plugin:
(https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1079)
Current Fluster score:
`Ran 131/147 tests successfully in 278.568 secs`
with
`python3 fluster.py run -d GStreamer-H.265-V4L2SL-Gst1.0 -ts JCT-VC-HEVC_V1 -j1`
failed conformance tests:
- DBLK_D_VIXS_2 (Success on Hantro G2)
- DSLICE_A_HHI_5 (Success on Hantro G2)
- EXT_A_ericsson_4 (Success on Hantro G2)
- PICSIZE_A_Bossen_1 (Hardware limitation)
- PICSIZE_B_Bossen_1 (Hardware limitation)
- PICSIZE_C_Bossen_1 (Hardware limitation)
- PICSIZE_D_Bossen_1 (Hardware limitation)
- PPS_A_qualcomm_7 (Success on Hantro G2)
- SAODBLK_A_MainConcept_4 (Success on Hantro G2)
- SAODBLK_B_MainConcept_4 (Success on Hantro G2)
- SLIST_B_Sony_9 (Success on Hantro G2)
- SLIST_D_Sony_9 (Success on Hantro G2)
- TSUNEQBD_A_MAIN10_Technicolor_2 (Success on Hantro G2)
- VPSSPSPPS_A_MainConcept_1 (Success on Hantro G2)
- WPP_D_ericsson_MAIN10_2 (Fail on Hantro G2)
- WPP_D_ericsson_MAIN_2 (Fail on Hantro G2)
Not tested with FFMpeg so far.
Known issues:
- Unable to reliably decode multiple videos concurrently
- The SAODBLK_* tests timeout if the timeout time in fluster is lower than 120
- Currently the uv_virstride is calculated in a manner that is hardcoded
for the two available formats NV12 and NV15. (@config_registers)
Notable design decisions:
- I opted for a bitfield to represent the PPS memory blob as it is the
perfect tool for that job. It describes the memory layout with any
additional required documentation, is easy to read and a native language
tool for that job
- The RPS memory blob is created using a bitmap implementation, which
uses a common Kernel API to avoid reinventing the wheel and to keep the
code clean.
- I deliberatly opted against the macro solution used in H264, which
declares Macros in mid function and declares the fields of the memory
blob as macros as well. And I would be glad to refactor the H264 code if
desired by the maintainer to use common Kernel APIs and native language
elements.
- The giant static array of cabac values is moved to a separate c file,
I did so because a separate .h file would be incorrect as it doesn't
expose anything of any value for any other file than the rkvdec-hevc.c
file. Other options were:
- Calculating the values instead of storing the results (doesn't seem
to be worth it)
- Supply them via firmware (Adding firmware makes the whole software
way more complicated and the usage of the driver less obvious)
Ignored Checkpatch warnings (as it fits to the current style of the file):
```
WARNING: line length of 162 exceeds 100 columns
#115: FILE: drivers/media/v4l2-core/v4l2-common.c:265:
+ { .format = V4L2_PIX_FMT_NV15, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 5, 0, 0 }, .hdiv = 2, .vdiv = 2,
ERROR: trailing statements should be on next line
#128: FILE: drivers/media/v4l2-core/v4l2-ioctl.c:1305:
+ case V4L2_PIX_FMT_NV15: descr = "10-bit Y/CbCr 4:2:0 (Packed)"; break;
```
v4l2-compliance test:
```
Total for rkvdec device /dev/video3: 46, Succeeded: 46, Failed: 0, Warnings: 0
```
kselftest module run for the bitmap changes:
```
$ sudo insmod /usr/lib/modules/5.19.0-rc3-finalseries/kernel/lib/test_bitmap.ko
[ 71.751716] test_bitmap: parselist: 14: input is '0-2047:128/256' OK, Time: 1750
[ 71.751787] test_bitmap: bitmap_print_to_pagebuf: input is '0-32767
[ 71.751787] ', Time: 6708
[ 71.760373] test_bitmap: set_value: 6/6 tests correct
```
Jonas Karlman (2):
media: v4l2: Add NV15 pixel format
media: v4l2-common: Add helpers to calculate bytesperline and
sizeimage
Sebastian Fricke (4):
bitops: bitmap helper to set variable length values
staging: media: rkvdec: Add valid pixel format check
staging: media: rkvdec: Enable S_CTRL IOCTL
staging: media: rkvdec: Add HEVC backend
.../media/v4l/pixfmt-yuv-planar.rst | 53 +
drivers/media/v4l2-core/v4l2-common.c | 79 +-
drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
drivers/staging/media/rkvdec/Makefile | 2 +-
drivers/staging/media/rkvdec/TODO | 22 +-
.../staging/media/rkvdec/rkvdec-hevc-data.c | 1844 +++++++++++++++++
drivers/staging/media/rkvdec/rkvdec-hevc.c | 859 ++++++++
drivers/staging/media/rkvdec/rkvdec-regs.h | 1 +
drivers/staging/media/rkvdec/rkvdec.c | 182 +-
drivers/staging/media/rkvdec/rkvdec.h | 3 +
include/linux/bitmap.h | 39 +
include/uapi/linux/videodev2.h | 1 +
lib/test_bitmap.c | 47 +
13 files changed, 3066 insertions(+), 67 deletions(-)
create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc-data.c
create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc.c
--
2.25.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/6] media: v4l2: Add NV15 pixel format
2022-07-13 16:24 [PATCH 0/6] RkVDEC HEVC driver Sebastian Fricke
@ 2022-07-13 16:24 ` Sebastian Fricke
2022-07-13 18:28 ` Laurent Pinchart
2022-07-13 16:24 ` [PATCH 2/6] media: v4l2-common: Add helpers to calculate bytesperline and sizeimage Sebastian Fricke
` (3 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Sebastian Fricke @ 2022-07-13 16:24 UTC (permalink / raw)
To: linux-media
Cc: jernej.skrabec, knaerzche, kernel, bob.beckett, ezequiel,
mchehab, gregkh, linux-kernel, linux-rockchip, linux-staging,
nicolas.dufresne, Jonas Karlman, Sebastian Fricke, Yury Norov,
Andy Shevchenko, Rasmus Villemoes, Hans Verkuil,
Benjamin Gaignard, Ming Qian, Sakari Ailus, Laurent Pinchart,
Ricardo Ribalda, Andrzej Pietrasiewicz, Sergey Senozhatsky
From: Jonas Karlman <jonas@kwiboo.se>
Add the NV15 pixel format used by the Rockchip Video Decoder for 10-bit
buffers.
NV15 is a packed 10-bit 4:2:0 Y/CbCr format similar to P010 and P210 but
has no padding between components. Instead, luminance and chrominance
samples are grouped into 4s so that each group is packed into an integer
number of bytes:
YYYY = UVUV = 4 * 10 bits = 40 bits = 5 bytes
The '15' 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.
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
---
.../media/v4l/pixfmt-yuv-planar.rst | 53 +++++++++++++++++++
drivers/media/v4l2-core/v4l2-common.c | 2 +
drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
include/uapi/linux/videodev2.h | 1 +
4 files changed, 57 insertions(+)
diff --git a/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst b/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
index a900ff66911a..42ab3fe4667f 100644
--- a/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
+++ b/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
@@ -79,6 +79,13 @@ All components are stored with the same number of bits per component.
- Cr, Cb
- Yes
- Linear
+ * - V4L2_PIX_FMT_NV15
+ - 'NV15'
+ - 15
+ - 4:2:0
+ - Cb, Cr
+ - Yes
+ - Linear
* - V4L2_PIX_FMT_NV12M
- 'NM12'
- 8
@@ -176,6 +183,7 @@ horizontally.
.. _V4L2-PIX-FMT-NV12:
.. _V4L2-PIX-FMT-NV21:
+.. _V4L2-PIX-FMT-NV15:
.. _V4L2-PIX-FMT-NV12M:
.. _V4L2-PIX-FMT-NV21M:
.. _V4L2-PIX-FMT-P010:
@@ -570,6 +578,51 @@ Data in the 10 high bits, zeros in the 6 low bits, arranged in little endian ord
- Cb\ :sub:`11`
- Cr\ :sub:`11`
+.. _V4L2_PIX_FMT_NV15:
+
+NV15
+----
+
+Like P010 but a packed 10-bit 4:2:0 semi-planar Y/CbCr format without padding between components.
+Instead, luminance and chrominance samples are grouped into 4s so that each group is packed into an integer
+number of bytes:
+YYYY = UVUV = 4 * 10 bits = 40 bits = 5 bytes
+
+.. flat-table:: Sample 4x4 NV15 Image
+ :header-rows: 0
+ :stub-columns: 0
+
+ * - start + 0:
+ - Y'\ :sub:`00`
+ - Y'\ :sub:`01`
+ - Y'\ :sub:`02`
+ - Y'\ :sub:`03`
+ * - start + 8:
+ - Y'\ :sub:`10`
+ - Y'\ :sub:`11`
+ - Y'\ :sub:`12`
+ - Y'\ :sub:`13`
+ * - start + 16:
+ - Y'\ :sub:`20`
+ - Y'\ :sub:`21`
+ - Y'\ :sub:`22`
+ - Y'\ :sub:`23`
+ * - start + 24:
+ - Y'\ :sub:`30`
+ - Y'\ :sub:`31`
+ - Y'\ :sub:`32`
+ - Y'\ :sub:`33`
+ * - start + 32:
+ - Cb\ :sub:`00`
+ - Cr\ :sub:`00`
+ - Cb\ :sub:`01`
+ - Cr\ :sub:`01`
+ * - start + 40:
+ - Cb\ :sub:`10`
+ - Cr\ :sub:`10`
+ - Cb\ :sub:`11`
+ - Cr\ :sub:`11`
+
.. raw:: latex
\endgroup
diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 1e38ad8906a2..23a0cb02ea3a 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -262,6 +262,8 @@ 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 }, .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 }, .hdiv = 2, .vdiv = 2 },
+ { .format = V4L2_PIX_FMT_NV15, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 5, 0, 0 }, .hdiv = 2, .vdiv = 2,
+ .block_w = { 4, 2, 0, 0 }, .block_h = { 1, 1, 0, 0 } },
{ .format = V4L2_PIX_FMT_NV16, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .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 }, .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 }, .hdiv = 1, .vdiv = 1 },
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index e2526701294e..9e5510cb255e 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1302,6 +1302,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
case V4L2_PIX_FMT_M420: descr = "YUV 4:2:0 (M420)"; break;
case V4L2_PIX_FMT_NV12: descr = "Y/CbCr 4:2:0"; break;
case V4L2_PIX_FMT_NV21: descr = "Y/CrCb 4:2:0"; break;
+ case V4L2_PIX_FMT_NV15: descr = "10-bit Y/CbCr 4:2:0 (Packed)"; break;
case V4L2_PIX_FMT_NV16: descr = "Y/CbCr 4:2:2"; break;
case V4L2_PIX_FMT_NV61: descr = "Y/CrCb 4:2:2"; break;
case V4L2_PIX_FMT_NV24: descr = "Y/CbCr 4:4:4"; break;
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 5a73b92ffe4d..47ff34d6b79f 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -598,6 +598,7 @@ 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_NV24 v4l2_fourcc('N', 'V', '2', '4') /* 24 Y/CbCr 4:4:4 */
--
2.25.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/6] media: v4l2-common: Add helpers to calculate bytesperline and sizeimage
2022-07-13 16:24 [PATCH 0/6] RkVDEC HEVC driver Sebastian Fricke
2022-07-13 16:24 ` [PATCH 1/6] media: v4l2: Add NV15 pixel format Sebastian Fricke
@ 2022-07-13 16:24 ` Sebastian Fricke
2022-07-13 16:47 ` [PATCH 0/6] RkVDEC HEVC driver Sebastian Fricke
` (2 subsequent siblings)
4 siblings, 0 replies; 17+ messages in thread
From: Sebastian Fricke @ 2022-07-13 16:24 UTC (permalink / raw)
To: linux-media
Cc: jernej.skrabec, knaerzche, kernel, bob.beckett, ezequiel,
mchehab, gregkh, linux-kernel, linux-rockchip, linux-staging,
nicolas.dufresne, Jonas Karlman, Yury Norov, Andy Shevchenko,
Rasmus Villemoes, Hans Verkuil, Sebastian Fricke,
Benjamin Gaignard
From: Jonas Karlman <jonas@kwiboo.se>
Add helper functions to calculate plane bytesperline and sizeimage, these
new helpers consider block width and height when calculating plane
bytesperline and sizeimage.
This prepare support for new pixel formats added in next patch that make
use of block width and height.
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
drivers/media/v4l2-core/v4l2-common.c | 77 +++++++++++++--------------
1 file changed, 38 insertions(+), 39 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 23a0cb02ea3a..2ed78f0af508 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -339,6 +339,33 @@ 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_width(const struct v4l2_format_info *info, int plane,
+ unsigned int width)
+{
+ unsigned int hdiv = plane ? info->hdiv : 1;
+ unsigned int bytes = DIV_ROUND_UP(width * info->bpp[plane],
+ v4l2_format_block_width(info, plane) *
+ v4l2_format_block_height(info, plane));
+
+ return DIV_ROUND_UP(bytes, hdiv);
+}
+
+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 lines = ALIGN(height, v4l2_format_block_height(info, plane));
+
+ return DIV_ROUND_UP(lines, 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_width(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)
{
@@ -374,37 +401,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];
+ plane->bytesperline = v4l2_format_plane_width(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);
- }
+ 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);
- plane->sizeimage =
- plane->bytesperline * DIV_ROUND_UP(aligned_height, vdiv);
+ v4l2_format_plane_width(info, i, width);
+ plane->sizeimage = plane->bytesperline *
+ v4l2_format_plane_height(info, i, height);
}
}
return 0;
@@ -428,22 +437,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];
+ pixfmt->bytesperline = v4l2_format_plane_width(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);
- }
+ 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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 0/6] RkVDEC HEVC driver
2022-07-13 16:24 [PATCH 0/6] RkVDEC HEVC driver Sebastian Fricke
2022-07-13 16:24 ` [PATCH 1/6] media: v4l2: Add NV15 pixel format Sebastian Fricke
2022-07-13 16:24 ` [PATCH 2/6] media: v4l2-common: Add helpers to calculate bytesperline and sizeimage Sebastian Fricke
@ 2022-07-13 16:47 ` Sebastian Fricke
2022-07-15 11:04 ` Robin Murphy
2022-07-16 16:27 ` Alex Bee
4 siblings, 0 replies; 17+ messages in thread
From: Sebastian Fricke @ 2022-07-13 16:47 UTC (permalink / raw)
To: linux-media
Cc: jernej.skrabec, knaerzche, kernel, bob.beckett, ezequiel,
mchehab, gregkh, linux-kernel, linux-rockchip, linux-staging,
nicolas.dufresne, Yury Norov, Andy Shevchenko, Rasmus Villemoes
Hello everyone,
sorry for this mess...
I have created two mail clusters by accident because I spotted during my
review of the patches while sending them that I forgot to recreate PATCH
3 after fixing a small issue. I then restarted git send-email with
patches 3 - 6, which didn't attach them to the previous mails :/.
Sorry again, I can send a proper version 2 once we have the first review
coming in.
Greetings,
Sebastian
On 13.07.2022 18:24, Sebastian Fricke wrote:
>Implement the HEVC codec variation for the RkVDEC driver. Currently only
>the RK3399 is supported, but it is possible to enable the RK3288 as it
>also supports this codec.
>
>Based on top of the media tree @ef7fcbbb9eabbe86d2287484bf366dd1821cc6b8
>and the HEVC uABI MR by Benjamin Gaignard.
>(https://patchwork.linuxtv.org/project/linux-media/list/?series=8360)
>
>Tested with the GStreamer V4L2 HEVC plugin:
>(https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1079)
>
>Current Fluster score:
>`Ran 131/147 tests successfully in 278.568 secs`
>with
>`python3 fluster.py run -d GStreamer-H.265-V4L2SL-Gst1.0 -ts JCT-VC-HEVC_V1 -j1`
>
>failed conformance tests:
>- DBLK_D_VIXS_2 (Success on Hantro G2)
>- DSLICE_A_HHI_5 (Success on Hantro G2)
>- EXT_A_ericsson_4 (Success on Hantro G2)
>- PICSIZE_A_Bossen_1 (Hardware limitation)
>- PICSIZE_B_Bossen_1 (Hardware limitation)
>- PICSIZE_C_Bossen_1 (Hardware limitation)
>- PICSIZE_D_Bossen_1 (Hardware limitation)
>- PPS_A_qualcomm_7 (Success on Hantro G2)
>- SAODBLK_A_MainConcept_4 (Success on Hantro G2)
>- SAODBLK_B_MainConcept_4 (Success on Hantro G2)
>- SLIST_B_Sony_9 (Success on Hantro G2)
>- SLIST_D_Sony_9 (Success on Hantro G2)
>- TSUNEQBD_A_MAIN10_Technicolor_2 (Success on Hantro G2)
>- VPSSPSPPS_A_MainConcept_1 (Success on Hantro G2)
>- WPP_D_ericsson_MAIN10_2 (Fail on Hantro G2)
>- WPP_D_ericsson_MAIN_2 (Fail on Hantro G2)
>
>Not tested with FFMpeg so far.
>
>Known issues:
>- Unable to reliably decode multiple videos concurrently
>- The SAODBLK_* tests timeout if the timeout time in fluster is lower than 120
>- Currently the uv_virstride is calculated in a manner that is hardcoded
>for the two available formats NV12 and NV15. (@config_registers)
>
>Notable design decisions:
>- I opted for a bitfield to represent the PPS memory blob as it is the
>perfect tool for that job. It describes the memory layout with any
>additional required documentation, is easy to read and a native language
>tool for that job
>- The RPS memory blob is created using a bitmap implementation, which
>uses a common Kernel API to avoid reinventing the wheel and to keep the
>code clean.
>- I deliberatly opted against the macro solution used in H264, which
>declares Macros in mid function and declares the fields of the memory
>blob as macros as well. And I would be glad to refactor the H264 code if
>desired by the maintainer to use common Kernel APIs and native language
>elements.
>- The giant static array of cabac values is moved to a separate c file,
>I did so because a separate .h file would be incorrect as it doesn't
>expose anything of any value for any other file than the rkvdec-hevc.c
>file. Other options were:
> - Calculating the values instead of storing the results (doesn't seem
> to be worth it)
> - Supply them via firmware (Adding firmware makes the whole software
> way more complicated and the usage of the driver less obvious)
>
>Ignored Checkpatch warnings (as it fits to the current style of the file):
>```
>WARNING: line length of 162 exceeds 100 columns
>#115: FILE: drivers/media/v4l2-core/v4l2-common.c:265:
>+ { .format = V4L2_PIX_FMT_NV15, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 5, 0, 0 }, .hdiv = 2, .vdiv = 2,
>
>ERROR: trailing statements should be on next line
>#128: FILE: drivers/media/v4l2-core/v4l2-ioctl.c:1305:
>+ case V4L2_PIX_FMT_NV15: descr = "10-bit Y/CbCr 4:2:0 (Packed)"; break;
>```
>
>v4l2-compliance test:
>```
>Total for rkvdec device /dev/video3: 46, Succeeded: 46, Failed: 0, Warnings: 0
>```
>
>kselftest module run for the bitmap changes:
>```
>$ sudo insmod /usr/lib/modules/5.19.0-rc3-finalseries/kernel/lib/test_bitmap.ko
>[ 71.751716] test_bitmap: parselist: 14: input is '0-2047:128/256' OK, Time: 1750
>[ 71.751787] test_bitmap: bitmap_print_to_pagebuf: input is '0-32767
>[ 71.751787] ', Time: 6708
>[ 71.760373] test_bitmap: set_value: 6/6 tests correct
>```
>
>Jonas Karlman (2):
> media: v4l2: Add NV15 pixel format
> media: v4l2-common: Add helpers to calculate bytesperline and
> sizeimage
>
>Sebastian Fricke (4):
> bitops: bitmap helper to set variable length values
> staging: media: rkvdec: Add valid pixel format check
> staging: media: rkvdec: Enable S_CTRL IOCTL
> staging: media: rkvdec: Add HEVC backend
>
> .../media/v4l/pixfmt-yuv-planar.rst | 53 +
> drivers/media/v4l2-core/v4l2-common.c | 79 +-
> drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
> drivers/staging/media/rkvdec/Makefile | 2 +-
> drivers/staging/media/rkvdec/TODO | 22 +-
> .../staging/media/rkvdec/rkvdec-hevc-data.c | 1844 +++++++++++++++++
> drivers/staging/media/rkvdec/rkvdec-hevc.c | 859 ++++++++
> drivers/staging/media/rkvdec/rkvdec-regs.h | 1 +
> drivers/staging/media/rkvdec/rkvdec.c | 182 +-
> drivers/staging/media/rkvdec/rkvdec.h | 3 +
> include/linux/bitmap.h | 39 +
> include/uapi/linux/videodev2.h | 1 +
> lib/test_bitmap.c | 47 +
> 13 files changed, 3066 insertions(+), 67 deletions(-)
> create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc-data.c
> create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc.c
>
>--
>2.25.1
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/6] media: v4l2: Add NV15 pixel format
2022-07-13 16:24 ` [PATCH 1/6] media: v4l2: Add NV15 pixel format Sebastian Fricke
@ 2022-07-13 18:28 ` Laurent Pinchart
2022-07-14 13:02 ` Nicolas Dufresne
0 siblings, 1 reply; 17+ messages in thread
From: Laurent Pinchart @ 2022-07-13 18:28 UTC (permalink / raw)
To: Sebastian Fricke
Cc: linux-media, jernej.skrabec, knaerzche, kernel, bob.beckett,
ezequiel, mchehab, gregkh, linux-kernel, linux-rockchip,
linux-staging, nicolas.dufresne, Jonas Karlman, Yury Norov,
Andy Shevchenko, Rasmus Villemoes, Hans Verkuil,
Benjamin Gaignard, Ming Qian, Sakari Ailus, Ricardo Ribalda,
Andrzej Pietrasiewicz, Sergey Senozhatsky
Hi Sebastian and Jonas,
Thank you for the patch.
On Wed, Jul 13, 2022 at 06:24:46PM +0200, Sebastian Fricke wrote:
> From: Jonas Karlman <jonas@kwiboo.se>
>
> Add the NV15 pixel format used by the Rockchip Video Decoder for 10-bit
> buffers.
>
> NV15 is a packed 10-bit 4:2:0 Y/CbCr format similar to P010 and P210 but
> has no padding between components. Instead, luminance and chrominance
> samples are grouped into 4s so that each group is packed into an integer
> number of bytes:
>
> YYYY = UVUV = 4 * 10 bits = 40 bits = 5 bytes
>
> The '15' 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.
>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> ---
> .../media/v4l/pixfmt-yuv-planar.rst | 53 +++++++++++++++++++
> drivers/media/v4l2-core/v4l2-common.c | 2 +
> drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
> include/uapi/linux/videodev2.h | 1 +
> 4 files changed, 57 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst b/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
> index a900ff66911a..42ab3fe4667f 100644
> --- a/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
> +++ b/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
> @@ -79,6 +79,13 @@ All components are stored with the same number of bits per component.
> - Cr, Cb
> - Yes
> - Linear
> + * - V4L2_PIX_FMT_NV15
> + - 'NV15'
> + - 15
> + - 4:2:0
> + - Cb, Cr
> + - Yes
> + - Linear
> * - V4L2_PIX_FMT_NV12M
> - 'NM12'
> - 8
> @@ -176,6 +183,7 @@ horizontally.
>
> .. _V4L2-PIX-FMT-NV12:
> .. _V4L2-PIX-FMT-NV21:
> +.. _V4L2-PIX-FMT-NV15:
> .. _V4L2-PIX-FMT-NV12M:
> .. _V4L2-PIX-FMT-NV21M:
> .. _V4L2-PIX-FMT-P010:
> @@ -570,6 +578,51 @@ Data in the 10 high bits, zeros in the 6 low bits, arranged in little endian ord
> - Cb\ :sub:`11`
> - Cr\ :sub:`11`
>
> +.. _V4L2_PIX_FMT_NV15:
> +
> +NV15
> +----
> +
> +Like P010 but a packed 10-bit 4:2:0 semi-planar Y/CbCr format without padding between components.
"packed 10-bit semi-planar" sounds confusing, as "packed YUV" usually
refers to YUYV-style formats, but I'm not sure how to express that
better.
> +Instead, luminance and chrominance samples are grouped into 4s so that each group is packed into an integer
> +number of bytes:
Could you please wrap the description at 80 columns ?
> +YYYY = UVUV = 4 * 10 bits = 40 bits = 5 bytes
> +
> +.. flat-table:: Sample 4x4 NV15 Image
> + :header-rows: 0
> + :stub-columns: 0
> +
> + * - start + 0:
> + - Y'\ :sub:`00`
> + - Y'\ :sub:`01`
> + - Y'\ :sub:`02`
> + - Y'\ :sub:`03`
> + * - start + 8:
> + - Y'\ :sub:`10`
> + - Y'\ :sub:`11`
> + - Y'\ :sub:`12`
> + - Y'\ :sub:`13`
> + * - start + 16:
> + - Y'\ :sub:`20`
> + - Y'\ :sub:`21`
> + - Y'\ :sub:`22`
> + - Y'\ :sub:`23`
> + * - start + 24:
> + - Y'\ :sub:`30`
> + - Y'\ :sub:`31`
> + - Y'\ :sub:`32`
> + - Y'\ :sub:`33`
> + * - start + 32:
> + - Cb\ :sub:`00`
> + - Cr\ :sub:`00`
> + - Cb\ :sub:`01`
> + - Cr\ :sub:`01`
> + * - start + 40:
> + - Cb\ :sub:`10`
> + - Cr\ :sub:`10`
> + - Cb\ :sub:`11`
> + - Cr\ :sub:`11`
This doesn't look right. You need to describe the data at the bit level,
so show how the 10-bit samples are packed into bytes.
> +
> .. raw:: latex
>
> \endgroup
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index 1e38ad8906a2..23a0cb02ea3a 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -262,6 +262,8 @@ 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 }, .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 }, .hdiv = 2, .vdiv = 2 },
> + { .format = V4L2_PIX_FMT_NV15, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 5, 0, 0 }, .hdiv = 2, .vdiv = 2,
> + .block_w = { 4, 2, 0, 0 }, .block_h = { 1, 1, 0, 0 } },
> { .format = V4L2_PIX_FMT_NV16, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .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 }, .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 }, .hdiv = 1, .vdiv = 1 },
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index e2526701294e..9e5510cb255e 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1302,6 +1302,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
> case V4L2_PIX_FMT_M420: descr = "YUV 4:2:0 (M420)"; break;
> case V4L2_PIX_FMT_NV12: descr = "Y/CbCr 4:2:0"; break;
> case V4L2_PIX_FMT_NV21: descr = "Y/CrCb 4:2:0"; break;
> + case V4L2_PIX_FMT_NV15: descr = "10-bit Y/CbCr 4:2:0 (Packed)"; break;
> case V4L2_PIX_FMT_NV16: descr = "Y/CbCr 4:2:2"; break;
> case V4L2_PIX_FMT_NV61: descr = "Y/CrCb 4:2:2"; break;
> case V4L2_PIX_FMT_NV24: descr = "Y/CbCr 4:4:4"; break;
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 5a73b92ffe4d..47ff34d6b79f 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -598,6 +598,7 @@ 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_NV24 v4l2_fourcc('N', 'V', '2', '4') /* 24 Y/CbCr 4:4:4 */
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/6] media: v4l2: Add NV15 pixel format
2022-07-13 18:28 ` Laurent Pinchart
@ 2022-07-14 13:02 ` Nicolas Dufresne
2022-07-15 0:04 ` Laurent Pinchart
0 siblings, 1 reply; 17+ messages in thread
From: Nicolas Dufresne @ 2022-07-14 13:02 UTC (permalink / raw)
To: Laurent Pinchart, Sebastian Fricke
Cc: linux-media, jernej.skrabec, knaerzche, kernel, bob.beckett,
ezequiel, mchehab, gregkh, linux-kernel, linux-rockchip,
linux-staging, Jonas Karlman, Yury Norov, Andy Shevchenko,
Rasmus Villemoes, Hans Verkuil, Benjamin Gaignard, Ming Qian,
Sakari Ailus, Ricardo Ribalda, Andrzej Pietrasiewicz,
Sergey Senozhatsky
Le mercredi 13 juillet 2022 à 21:28 +0300, Laurent Pinchart a écrit :
> Hi Sebastian and Jonas,
>
> Thank you for the patch.
>
> On Wed, Jul 13, 2022 at 06:24:46PM +0200, Sebastian Fricke wrote:
> > From: Jonas Karlman <jonas@kwiboo.se>
> >
> > Add the NV15 pixel format used by the Rockchip Video Decoder for 10-bit
> > buffers.
> >
> > NV15 is a packed 10-bit 4:2:0 Y/CbCr format similar to P010 and P210 but
> > has no padding between components. Instead, luminance and chrominance
> > samples are grouped into 4s so that each group is packed into an integer
> > number of bytes:
> >
> > YYYY = UVUV = 4 * 10 bits = 40 bits = 5 bytes
> >
> > The '15' 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.
> >
> > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> > ---
> > .../media/v4l/pixfmt-yuv-planar.rst | 53 +++++++++++++++++++
> > drivers/media/v4l2-core/v4l2-common.c | 2 +
> > drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
> > include/uapi/linux/videodev2.h | 1 +
> > 4 files changed, 57 insertions(+)
> >
> > diff --git a/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst b/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
> > index a900ff66911a..42ab3fe4667f 100644
> > --- a/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
> > +++ b/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
> > @@ -79,6 +79,13 @@ All components are stored with the same number of bits per component.
> > - Cr, Cb
> > - Yes
> > - Linear
> > + * - V4L2_PIX_FMT_NV15
> > + - 'NV15'
> > + - 15
> > + - 4:2:0
> > + - Cb, Cr
> > + - Yes
> > + - Linear
> > * - V4L2_PIX_FMT_NV12M
> > - 'NM12'
> > - 8
> > @@ -176,6 +183,7 @@ horizontally.
> >
> > .. _V4L2-PIX-FMT-NV12:
> > .. _V4L2-PIX-FMT-NV21:
> > +.. _V4L2-PIX-FMT-NV15:
> > .. _V4L2-PIX-FMT-NV12M:
> > .. _V4L2-PIX-FMT-NV21M:
> > .. _V4L2-PIX-FMT-P010:
> > @@ -570,6 +578,51 @@ Data in the 10 high bits, zeros in the 6 low bits, arranged in little endian ord
> > - Cb\ :sub:`11`
> > - Cr\ :sub:`11`
> >
> > +.. _V4L2_PIX_FMT_NV15:
> > +
> > +NV15
> > +----
> > +
> > +Like P010 but a packed 10-bit 4:2:0 semi-planar Y/CbCr format without padding between components.
>
> "packed 10-bit semi-planar" sounds confusing, as "packed YUV" usually
> refers to YUYV-style formats, but I'm not sure how to express that
> better.
Perhaps:
"Similar to P010 (10-bit 4:":0 semi-planer Y/CbCr), though unlike P010, the
there is not padding between components."
>
> > +Instead, luminance and chrominance samples are grouped into 4s so that each group is packed into an integer
> > +number of bytes:
>
> Could you please wrap the description at 80 columns ?
>
> > +YYYY = UVUV = 4 * 10 bits = 40 bits = 5 bytes
> > +
> > +.. flat-table:: Sample 4x4 NV15 Image
> > + :header-rows: 0
> > + :stub-columns: 0
> > +
> > + * - start + 0:
> > + - Y'\ :sub:`00`
> > + - Y'\ :sub:`01`
> > + - Y'\ :sub:`02`
> > + - Y'\ :sub:`03`
> > + * - start + 8:
> > + - Y'\ :sub:`10`
> > + - Y'\ :sub:`11`
> > + - Y'\ :sub:`12`
> > + - Y'\ :sub:`13`
> > + * - start + 16:
> > + - Y'\ :sub:`20`
> > + - Y'\ :sub:`21`
> > + - Y'\ :sub:`22`
> > + - Y'\ :sub:`23`
> > + * - start + 24:
> > + - Y'\ :sub:`30`
> > + - Y'\ :sub:`31`
> > + - Y'\ :sub:`32`
> > + - Y'\ :sub:`33`
> > + * - start + 32:
> > + - Cb\ :sub:`00`
> > + - Cr\ :sub:`00`
> > + - Cb\ :sub:`01`
> > + - Cr\ :sub:`01`
> > + * - start + 40:
> > + - Cb\ :sub:`10`
> > + - Cr\ :sub:`10`
> > + - Cb\ :sub:`11`
> > + - Cr\ :sub:`11`
>
> This doesn't look right. You need to describe the data at the bit level,
> so show how the 10-bit samples are packed into bytes.
A word of NV15 is 40 bits, so 1 word of NV12 is 5 bytes, 4 pixels. I believe
there is no choice here but to describe 4 pixels for Y plane, and 4 pixels for
CbCr plane. This might be a bit big though.
>
> > +
> > .. raw:: latex
> >
> > \endgroup
> > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> > index 1e38ad8906a2..23a0cb02ea3a 100644
> > --- a/drivers/media/v4l2-core/v4l2-common.c
> > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > @@ -262,6 +262,8 @@ 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 }, .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 }, .hdiv = 2, .vdiv = 2 },
> > + { .format = V4L2_PIX_FMT_NV15, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 5, 0, 0 }, .hdiv = 2, .vdiv = 2,
> > + .block_w = { 4, 2, 0, 0 }, .block_h = { 1, 1, 0, 0 } },
> > { .format = V4L2_PIX_FMT_NV16, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .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 }, .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 }, .hdiv = 1, .vdiv = 1 },
> > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> > index e2526701294e..9e5510cb255e 100644
> > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > @@ -1302,6 +1302,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
> > case V4L2_PIX_FMT_M420: descr = "YUV 4:2:0 (M420)"; break;
> > case V4L2_PIX_FMT_NV12: descr = "Y/CbCr 4:2:0"; break;
> > case V4L2_PIX_FMT_NV21: descr = "Y/CrCb 4:2:0"; break;
> > + case V4L2_PIX_FMT_NV15: descr = "10-bit Y/CbCr 4:2:0 (Packed)"; break;
> > case V4L2_PIX_FMT_NV16: descr = "Y/CbCr 4:2:2"; break;
> > case V4L2_PIX_FMT_NV61: descr = "Y/CrCb 4:2:2"; break;
> > case V4L2_PIX_FMT_NV24: descr = "Y/CbCr 4:4:4"; break;
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 5a73b92ffe4d..47ff34d6b79f 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -598,6 +598,7 @@ 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_NV24 v4l2_fourcc('N', 'V', '2', '4') /* 24 Y/CbCr 4:4:4 */
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/6] media: v4l2: Add NV15 pixel format
2022-07-14 13:02 ` Nicolas Dufresne
@ 2022-07-15 0:04 ` Laurent Pinchart
2022-07-21 16:58 ` Sebastian Fricke
0 siblings, 1 reply; 17+ messages in thread
From: Laurent Pinchart @ 2022-07-15 0:04 UTC (permalink / raw)
To: Nicolas Dufresne
Cc: Sebastian Fricke, linux-media, jernej.skrabec, knaerzche, kernel,
bob.beckett, ezequiel, mchehab, gregkh, linux-kernel,
linux-rockchip, linux-staging, Jonas Karlman, Yury Norov,
Andy Shevchenko, Rasmus Villemoes, Hans Verkuil,
Benjamin Gaignard, Ming Qian, Sakari Ailus, Ricardo Ribalda,
Andrzej Pietrasiewicz, Sergey Senozhatsky
On Thu, Jul 14, 2022 at 09:02:38AM -0400, Nicolas Dufresne wrote:
> Le mercredi 13 juillet 2022 à 21:28 +0300, Laurent Pinchart a écrit :
> > Hi Sebastian and Jonas,
> >
> > Thank you for the patch.
> >
> > On Wed, Jul 13, 2022 at 06:24:46PM +0200, Sebastian Fricke wrote:
> > > From: Jonas Karlman <jonas@kwiboo.se>
> > >
> > > Add the NV15 pixel format used by the Rockchip Video Decoder for 10-bit
> > > buffers.
> > >
> > > NV15 is a packed 10-bit 4:2:0 Y/CbCr format similar to P010 and P210 but
> > > has no padding between components. Instead, luminance and chrominance
> > > samples are grouped into 4s so that each group is packed into an integer
> > > number of bytes:
> > >
> > > YYYY = UVUV = 4 * 10 bits = 40 bits = 5 bytes
> > >
> > > The '15' 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.
> > >
> > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > > Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> > > ---
> > > .../media/v4l/pixfmt-yuv-planar.rst | 53 +++++++++++++++++++
> > > drivers/media/v4l2-core/v4l2-common.c | 2 +
> > > drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
> > > include/uapi/linux/videodev2.h | 1 +
> > > 4 files changed, 57 insertions(+)
> > >
> > > diff --git a/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst b/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
> > > index a900ff66911a..42ab3fe4667f 100644
> > > --- a/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
> > > +++ b/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
> > > @@ -79,6 +79,13 @@ All components are stored with the same number of bits per component.
> > > - Cr, Cb
> > > - Yes
> > > - Linear
> > > + * - V4L2_PIX_FMT_NV15
> > > + - 'NV15'
> > > + - 15
> > > + - 4:2:0
> > > + - Cb, Cr
> > > + - Yes
> > > + - Linear
> > > * - V4L2_PIX_FMT_NV12M
> > > - 'NM12'
> > > - 8
> > > @@ -176,6 +183,7 @@ horizontally.
> > >
> > > .. _V4L2-PIX-FMT-NV12:
> > > .. _V4L2-PIX-FMT-NV21:
> > > +.. _V4L2-PIX-FMT-NV15:
> > > .. _V4L2-PIX-FMT-NV12M:
> > > .. _V4L2-PIX-FMT-NV21M:
> > > .. _V4L2-PIX-FMT-P010:
> > > @@ -570,6 +578,51 @@ Data in the 10 high bits, zeros in the 6 low bits, arranged in little endian ord
> > > - Cb\ :sub:`11`
> > > - Cr\ :sub:`11`
> > >
> > > +.. _V4L2_PIX_FMT_NV15:
> > > +
> > > +NV15
> > > +----
> > > +
> > > +Like P010 but a packed 10-bit 4:2:0 semi-planar Y/CbCr format without padding between components.
> >
> > "packed 10-bit semi-planar" sounds confusing, as "packed YUV" usually
> > refers to YUYV-style formats, but I'm not sure how to express that
> > better.
>
> Perhaps:
>
> "Similar to P010 (10-bit 4:":0 semi-planer Y/CbCr), though unlike P010, the
> there is not padding between components."
>
> > > +Instead, luminance and chrominance samples are grouped into 4s so that each group is packed into an integer
> > > +number of bytes:
> >
> > Could you please wrap the description at 80 columns ?
> >
> > > +YYYY = UVUV = 4 * 10 bits = 40 bits = 5 bytes
> > > +
> > > +.. flat-table:: Sample 4x4 NV15 Image
> > > + :header-rows: 0
> > > + :stub-columns: 0
> > > +
> > > + * - start + 0:
> > > + - Y'\ :sub:`00`
> > > + - Y'\ :sub:`01`
> > > + - Y'\ :sub:`02`
> > > + - Y'\ :sub:`03`
> > > + * - start + 8:
> > > + - Y'\ :sub:`10`
> > > + - Y'\ :sub:`11`
> > > + - Y'\ :sub:`12`
> > > + - Y'\ :sub:`13`
> > > + * - start + 16:
> > > + - Y'\ :sub:`20`
> > > + - Y'\ :sub:`21`
> > > + - Y'\ :sub:`22`
> > > + - Y'\ :sub:`23`
> > > + * - start + 24:
> > > + - Y'\ :sub:`30`
> > > + - Y'\ :sub:`31`
> > > + - Y'\ :sub:`32`
> > > + - Y'\ :sub:`33`
> > > + * - start + 32:
> > > + - Cb\ :sub:`00`
> > > + - Cr\ :sub:`00`
> > > + - Cb\ :sub:`01`
> > > + - Cr\ :sub:`01`
> > > + * - start + 40:
> > > + - Cb\ :sub:`10`
> > > + - Cr\ :sub:`10`
> > > + - Cb\ :sub:`11`
> > > + - Cr\ :sub:`11`
> >
> > This doesn't look right. You need to describe the data at the bit level,
> > so show how the 10-bit samples are packed into bytes.
>
> A word of NV15 is 40 bits, so 1 word of NV12 is 5 bytes, 4 pixels. I believe
> there is no choice here but to describe 4 pixels for Y plane, and 4 pixels for
> CbCr plane. This might be a bit big though.
I agree, it may not be the prettiest, but I'd rather have a larger table
than a format that is understood differently by different drivers. Been
there, done that, not again :-)
> > > +
> > > .. raw:: latex
> > >
> > > \endgroup
> > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> > > index 1e38ad8906a2..23a0cb02ea3a 100644
> > > --- a/drivers/media/v4l2-core/v4l2-common.c
> > > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > > @@ -262,6 +262,8 @@ 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 }, .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 }, .hdiv = 2, .vdiv = 2 },
> > > + { .format = V4L2_PIX_FMT_NV15, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 5, 0, 0 }, .hdiv = 2, .vdiv = 2,
> > > + .block_w = { 4, 2, 0, 0 }, .block_h = { 1, 1, 0, 0 } },
> > > { .format = V4L2_PIX_FMT_NV16, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .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 }, .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 }, .hdiv = 1, .vdiv = 1 },
> > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> > > index e2526701294e..9e5510cb255e 100644
> > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > > @@ -1302,6 +1302,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
> > > case V4L2_PIX_FMT_M420: descr = "YUV 4:2:0 (M420)"; break;
> > > case V4L2_PIX_FMT_NV12: descr = "Y/CbCr 4:2:0"; break;
> > > case V4L2_PIX_FMT_NV21: descr = "Y/CrCb 4:2:0"; break;
> > > + case V4L2_PIX_FMT_NV15: descr = "10-bit Y/CbCr 4:2:0 (Packed)"; break;
> > > case V4L2_PIX_FMT_NV16: descr = "Y/CbCr 4:2:2"; break;
> > > case V4L2_PIX_FMT_NV61: descr = "Y/CrCb 4:2:2"; break;
> > > case V4L2_PIX_FMT_NV24: descr = "Y/CbCr 4:4:4"; break;
> > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > > index 5a73b92ffe4d..47ff34d6b79f 100644
> > > --- a/include/uapi/linux/videodev2.h
> > > +++ b/include/uapi/linux/videodev2.h
> > > @@ -598,6 +598,7 @@ 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_NV24 v4l2_fourcc('N', 'V', '2', '4') /* 24 Y/CbCr 4:4:4 */
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/6] RkVDEC HEVC driver
2022-07-13 16:24 [PATCH 0/6] RkVDEC HEVC driver Sebastian Fricke
` (2 preceding siblings ...)
2022-07-13 16:47 ` [PATCH 0/6] RkVDEC HEVC driver Sebastian Fricke
@ 2022-07-15 11:04 ` Robin Murphy
2022-07-15 15:36 ` Nicolas Dufresne
2022-07-16 16:27 ` Alex Bee
4 siblings, 1 reply; 17+ messages in thread
From: Robin Murphy @ 2022-07-15 11:04 UTC (permalink / raw)
To: Sebastian Fricke, linux-media
Cc: jernej.skrabec, knaerzche, kernel, bob.beckett, ezequiel,
mchehab, gregkh, linux-kernel, linux-rockchip, linux-staging,
nicolas.dufresne, Yury Norov, Andy Shevchenko, Rasmus Villemoes
On 2022-07-13 17:24, Sebastian Fricke wrote:
> Implement the HEVC codec variation for the RkVDEC driver. Currently only
> the RK3399 is supported, but it is possible to enable the RK3288 as it
> also supports this codec.
>
> Based on top of the media tree @ef7fcbbb9eabbe86d2287484bf366dd1821cc6b8
> and the HEVC uABI MR by Benjamin Gaignard.
> (https://patchwork.linuxtv.org/project/linux-media/list/?series=8360)
>
> Tested with the GStreamer V4L2 HEVC plugin:
> (https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1079)
>
> Current Fluster score:
> `Ran 131/147 tests successfully in 278.568 secs`
> with
> `python3 fluster.py run -d GStreamer-H.265-V4L2SL-Gst1.0 -ts JCT-VC-HEVC_V1 -j1`
>
> failed conformance tests:
> - DBLK_D_VIXS_2 (Success on Hantro G2)
> - DSLICE_A_HHI_5 (Success on Hantro G2)
> - EXT_A_ericsson_4 (Success on Hantro G2)
> - PICSIZE_A_Bossen_1 (Hardware limitation)
> - PICSIZE_B_Bossen_1 (Hardware limitation)
> - PICSIZE_C_Bossen_1 (Hardware limitation)
> - PICSIZE_D_Bossen_1 (Hardware limitation)
> - PPS_A_qualcomm_7 (Success on Hantro G2)
> - SAODBLK_A_MainConcept_4 (Success on Hantro G2)
> - SAODBLK_B_MainConcept_4 (Success on Hantro G2)
> - SLIST_B_Sony_9 (Success on Hantro G2)
> - SLIST_D_Sony_9 (Success on Hantro G2)
> - TSUNEQBD_A_MAIN10_Technicolor_2 (Success on Hantro G2)
> - VPSSPSPPS_A_MainConcept_1 (Success on Hantro G2)
> - WPP_D_ericsson_MAIN10_2 (Fail on Hantro G2)
> - WPP_D_ericsson_MAIN_2 (Fail on Hantro G2)
>
> Not tested with FFMpeg so far.
>
> Known issues:
> - Unable to reliably decode multiple videos concurrently
> - The SAODBLK_* tests timeout if the timeout time in fluster is lower than 120
> - Currently the uv_virstride is calculated in a manner that is hardcoded
> for the two available formats NV12 and NV15. (@config_registers)
>
> Notable design decisions:
> - I opted for a bitfield to represent the PPS memory blob as it is the
> perfect tool for that job. It describes the memory layout with any
> additional required documentation, is easy to read and a native language
> tool for that job
Can I point out how terrible an idea this is? The C language gives
virtually zero guarantee about how bitfields are actually represented in
memory. Platform ABIs (e.g. [1]) might nail things down a bit more, but
different platforms are free to make completely different choices so
portability still goes out the window. Even for a single platform,
different compilers (or at worst even different version of one compiler)
can still make incompatible choices e.g. WRT alignment of packed
members. Even if you narrow the scope as far as a specific version of
AArch64 GCC, I think this is still totally broken for big-endian.
The fact that you've had to use nonsensical types to trick a compiler
into meeting your expectations should already be a clue to how fragile
this is in general.
> - The RPS memory blob is created using a bitmap implementation, which
> uses a common Kernel API to avoid reinventing the wheel and to keep the
> code clean.
Similarly, Linux bitmaps are designed for use as, well, bitmaps. Abusing
them as a data interchange format for bit-aligned numerical values is
far from "clean" semantically. And I'm pretty sure it's also broken for
big-endian.
This kind of stuff may be standard practice in embedded development
where you're targeting a specific MCU with a specific toolchain, but I
don't believe it's suitable for upstream Linux. It would take pretty
much the same number of lines to use GENMASK definitions and bitfield.h
helpers to pack values into words which can then be written to memory in
a guaranteed format and endianness (certainly for the PPS; for the RPS
it may well end up a bit longer, but would be self-documenting and
certainly more readable than those loops). It mostly just means that for
any field which crosses a word boundary you'll end up with 2 definitions
and 2 assignments, which is hardly a problem (and in some ways more
honest about what's actually going on).
Thanks,
Robin.
[1]
https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#bit-fields
> - I deliberatly opted against the macro solution used in H264, which
> declares Macros in mid function and declares the fields of the memory
> blob as macros as well. And I would be glad to refactor the H264 code if
> desired by the maintainer to use common Kernel APIs and native language
> elements.
> - The giant static array of cabac values is moved to a separate c file,
> I did so because a separate .h file would be incorrect as it doesn't
> expose anything of any value for any other file than the rkvdec-hevc.c
> file. Other options were:
> - Calculating the values instead of storing the results (doesn't seem
> to be worth it)
> - Supply them via firmware (Adding firmware makes the whole software
> way more complicated and the usage of the driver less obvious)
>
> Ignored Checkpatch warnings (as it fits to the current style of the file):
> ```
> WARNING: line length of 162 exceeds 100 columns
> #115: FILE: drivers/media/v4l2-core/v4l2-common.c:265:
> + { .format = V4L2_PIX_FMT_NV15, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 5, 0, 0 }, .hdiv = 2, .vdiv = 2,
>
> ERROR: trailing statements should be on next line
> #128: FILE: drivers/media/v4l2-core/v4l2-ioctl.c:1305:
> + case V4L2_PIX_FMT_NV15: descr = "10-bit Y/CbCr 4:2:0 (Packed)"; break;
> ```
>
> v4l2-compliance test:
> ```
> Total for rkvdec device /dev/video3: 46, Succeeded: 46, Failed: 0, Warnings: 0
> ```
>
> kselftest module run for the bitmap changes:
> ```
> $ sudo insmod /usr/lib/modules/5.19.0-rc3-finalseries/kernel/lib/test_bitmap.ko
> [ 71.751716] test_bitmap: parselist: 14: input is '0-2047:128/256' OK, Time: 1750
> [ 71.751787] test_bitmap: bitmap_print_to_pagebuf: input is '0-32767
> [ 71.751787] ', Time: 6708
> [ 71.760373] test_bitmap: set_value: 6/6 tests correct
> ```
>
> Jonas Karlman (2):
> media: v4l2: Add NV15 pixel format
> media: v4l2-common: Add helpers to calculate bytesperline and
> sizeimage
>
> Sebastian Fricke (4):
> bitops: bitmap helper to set variable length values
> staging: media: rkvdec: Add valid pixel format check
> staging: media: rkvdec: Enable S_CTRL IOCTL
> staging: media: rkvdec: Add HEVC backend
>
> .../media/v4l/pixfmt-yuv-planar.rst | 53 +
> drivers/media/v4l2-core/v4l2-common.c | 79 +-
> drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
> drivers/staging/media/rkvdec/Makefile | 2 +-
> drivers/staging/media/rkvdec/TODO | 22 +-
> .../staging/media/rkvdec/rkvdec-hevc-data.c | 1844 +++++++++++++++++
> drivers/staging/media/rkvdec/rkvdec-hevc.c | 859 ++++++++
> drivers/staging/media/rkvdec/rkvdec-regs.h | 1 +
> drivers/staging/media/rkvdec/rkvdec.c | 182 +-
> drivers/staging/media/rkvdec/rkvdec.h | 3 +
> include/linux/bitmap.h | 39 +
> include/uapi/linux/videodev2.h | 1 +
> lib/test_bitmap.c | 47 +
> 13 files changed, 3066 insertions(+), 67 deletions(-)
> create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc-data.c
> create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc.c
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/6] RkVDEC HEVC driver
2022-07-15 11:04 ` Robin Murphy
@ 2022-07-15 15:36 ` Nicolas Dufresne
2022-07-16 6:45 ` Jernej Škrabec
2022-07-21 16:16 ` Sebastian Fricke
0 siblings, 2 replies; 17+ messages in thread
From: Nicolas Dufresne @ 2022-07-15 15:36 UTC (permalink / raw)
To: Robin Murphy, Sebastian Fricke, linux-media
Cc: jernej.skrabec, knaerzche, kernel, bob.beckett, ezequiel,
mchehab, gregkh, linux-kernel, linux-rockchip, linux-staging,
Yury Norov, Andy Shevchenko, Rasmus Villemoes
Le vendredi 15 juillet 2022 à 12:04 +0100, Robin Murphy a écrit :
> On 2022-07-13 17:24, Sebastian Fricke wrote:
> > Implement the HEVC codec variation for the RkVDEC driver. Currently only
> > the RK3399 is supported, but it is possible to enable the RK3288 as it
> > also supports this codec.
> >
> > Based on top of the media tree @ef7fcbbb9eabbe86d2287484bf366dd1821cc6b8
> > and the HEVC uABI MR by Benjamin Gaignard.
> > (https://patchwork.linuxtv.org/project/linux-media/list/?series=8360)
> >
> > Tested with the GStreamer V4L2 HEVC plugin:
> > (https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1079)
> >
> > Current Fluster score:
> > `Ran 131/147 tests successfully in 278.568 secs`
> > with
> > `python3 fluster.py run -d GStreamer-H.265-V4L2SL-Gst1.0 -ts JCT-VC-HEVC_V1 -j1`
> >
> > failed conformance tests:
> > - DBLK_D_VIXS_2 (Success on Hantro G2)
> > - DSLICE_A_HHI_5 (Success on Hantro G2)
> > - EXT_A_ericsson_4 (Success on Hantro G2)
> > - PICSIZE_A_Bossen_1 (Hardware limitation)
> > - PICSIZE_B_Bossen_1 (Hardware limitation)
> > - PICSIZE_C_Bossen_1 (Hardware limitation)
> > - PICSIZE_D_Bossen_1 (Hardware limitation)
> > - PPS_A_qualcomm_7 (Success on Hantro G2)
> > - SAODBLK_A_MainConcept_4 (Success on Hantro G2)
> > - SAODBLK_B_MainConcept_4 (Success on Hantro G2)
> > - SLIST_B_Sony_9 (Success on Hantro G2)
> > - SLIST_D_Sony_9 (Success on Hantro G2)
> > - TSUNEQBD_A_MAIN10_Technicolor_2 (Success on Hantro G2)
> > - VPSSPSPPS_A_MainConcept_1 (Success on Hantro G2)
> > - WPP_D_ericsson_MAIN10_2 (Fail on Hantro G2)
> > - WPP_D_ericsson_MAIN_2 (Fail on Hantro G2)
> >
> > Not tested with FFMpeg so far.
> >
> > Known issues:
> > - Unable to reliably decode multiple videos concurrently
> > - The SAODBLK_* tests timeout if the timeout time in fluster is lower than 120
> > - Currently the uv_virstride is calculated in a manner that is hardcoded
> > for the two available formats NV12 and NV15. (@config_registers)
> >
> > Notable design decisions:
> > - I opted for a bitfield to represent the PPS memory blob as it is the
> > perfect tool for that job. It describes the memory layout with any
> > additional required documentation, is easy to read and a native language
> > tool for that job
>
> Can I point out how terrible an idea this is? The C language gives
> virtually zero guarantee about how bitfields are actually represented in
> memory. Platform ABIs (e.g. [1]) might nail things down a bit more, but
> different platforms are free to make completely different choices so
> portability still goes out the window. Even for a single platform,
> different compilers (or at worst even different version of one compiler)
> can still make incompatible choices e.g. WRT alignment of packed
> members. Even if you narrow the scope as far as a specific version of
> AArch64 GCC, I think this is still totally broken for big-endian.
>
> The fact that you've had to use nonsensical types to trick a compiler
> into meeting your expectations should already be a clue to how fragile
> this is in general.
>
> > - The RPS memory blob is created using a bitmap implementation, which
> > uses a common Kernel API to avoid reinventing the wheel and to keep the
> > code clean.
>
> Similarly, Linux bitmaps are designed for use as, well, bitmaps. Abusing
> them as a data interchange format for bit-aligned numerical values is
> far from "clean" semantically. And I'm pretty sure it's also broken for
> big-endian.
>
> This kind of stuff may be standard practice in embedded development
> where you're targeting a specific MCU with a specific toolchain, but I
> don't believe it's suitable for upstream Linux. It would take pretty
> much the same number of lines to use GENMASK definitions and bitfield.h
> helpers to pack values into words which can then be written to memory in
> a guaranteed format and endianness (certainly for the PPS; for the RPS
> it may well end up a bit longer, but would be self-documenting and
> certainly more readable than those loops). It mostly just means that for
> any field which crosses a word boundary you'll end up with 2 definitions
> and 2 assignments, which is hardly a problem (and in some ways more
> honest about what's actually going on).
Thanks for the feedback, in multimedia (unlike register programming), we don't
really consider bitstreams as bitmap or bitfield. What we do really expect is to
use bit writer helpers (and sometimes a bit reader though we try and avoid the
second one in the kernel). Its more of less a cursor (a bit position) into a
memory that advance while writing. A bit writer should help protect against
overflow too.
When writing lets say a chain of 8 bits from a char, a proper helper is expected
to be very explicit on the ordering (write_u8_le/be or something better worded).
I would rather like to see all these blobs written this way personally then
having a cleared buffer and writing using bit offsets.
Perhaps I may suggest to start with implementing just that inside this driver?
It isn't very hard, and then the implementation can be reduced later and shared
later, with whatever exists without deviating from the intent of the existing
API ? I do believe that having this in linux-media can be useful in the future.
We will notably need to extend such a helper with multimedia specific coding
technique (golomb, boolean coding, etc.) for use in stateless encoder drivers.
Nicolas
>
> Thanks,
> Robin.
>
> [1]
> https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#bit-fields
>
> > - I deliberatly opted against the macro solution used in H264, which
> > declares Macros in mid function and declares the fields of the memory
> > blob as macros as well. And I would be glad to refactor the H264 code if
> > desired by the maintainer to use common Kernel APIs and native language
> > elements.
> > - The giant static array of cabac values is moved to a separate c file,
> > I did so because a separate .h file would be incorrect as it doesn't
> > expose anything of any value for any other file than the rkvdec-hevc.c
> > file. Other options were:
> > - Calculating the values instead of storing the results (doesn't seem
> > to be worth it)
> > - Supply them via firmware (Adding firmware makes the whole software
> > way more complicated and the usage of the driver less obvious)
> >
> > Ignored Checkpatch warnings (as it fits to the current style of the file):
> > ```
> > WARNING: line length of 162 exceeds 100 columns
> > #115: FILE: drivers/media/v4l2-core/v4l2-common.c:265:
> > + { .format = V4L2_PIX_FMT_NV15, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 5, 0, 0 }, .hdiv = 2, .vdiv = 2,
> >
> > ERROR: trailing statements should be on next line
> > #128: FILE: drivers/media/v4l2-core/v4l2-ioctl.c:1305:
> > + case V4L2_PIX_FMT_NV15: descr = "10-bit Y/CbCr 4:2:0 (Packed)"; break;
> > ```
> >
> > v4l2-compliance test:
> > ```
> > Total for rkvdec device /dev/video3: 46, Succeeded: 46, Failed: 0, Warnings: 0
> > ```
> >
> > kselftest module run for the bitmap changes:
> > ```
> > $ sudo insmod /usr/lib/modules/5.19.0-rc3-finalseries/kernel/lib/test_bitmap.ko
> > [ 71.751716] test_bitmap: parselist: 14: input is '0-2047:128/256' OK, Time: 1750
> > [ 71.751787] test_bitmap: bitmap_print_to_pagebuf: input is '0-32767
> > [ 71.751787] ', Time: 6708
> > [ 71.760373] test_bitmap: set_value: 6/6 tests correct
> > ```
> >
> > Jonas Karlman (2):
> > media: v4l2: Add NV15 pixel format
> > media: v4l2-common: Add helpers to calculate bytesperline and
> > sizeimage
> >
> > Sebastian Fricke (4):
> > bitops: bitmap helper to set variable length values
> > staging: media: rkvdec: Add valid pixel format check
> > staging: media: rkvdec: Enable S_CTRL IOCTL
> > staging: media: rkvdec: Add HEVC backend
> >
> > .../media/v4l/pixfmt-yuv-planar.rst | 53 +
> > drivers/media/v4l2-core/v4l2-common.c | 79 +-
> > drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
> > drivers/staging/media/rkvdec/Makefile | 2 +-
> > drivers/staging/media/rkvdec/TODO | 22 +-
> > .../staging/media/rkvdec/rkvdec-hevc-data.c | 1844 +++++++++++++++++
> > drivers/staging/media/rkvdec/rkvdec-hevc.c | 859 ++++++++
> > drivers/staging/media/rkvdec/rkvdec-regs.h | 1 +
> > drivers/staging/media/rkvdec/rkvdec.c | 182 +-
> > drivers/staging/media/rkvdec/rkvdec.h | 3 +
> > include/linux/bitmap.h | 39 +
> > include/uapi/linux/videodev2.h | 1 +
> > lib/test_bitmap.c | 47 +
> > 13 files changed, 3066 insertions(+), 67 deletions(-)
> > create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc-data.c
> > create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc.c
> >
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/6] RkVDEC HEVC driver
2022-07-15 15:36 ` Nicolas Dufresne
@ 2022-07-16 6:45 ` Jernej Škrabec
2022-07-18 14:54 ` Nicolas Dufresne
2022-07-21 16:16 ` Sebastian Fricke
1 sibling, 1 reply; 17+ messages in thread
From: Jernej Škrabec @ 2022-07-16 6:45 UTC (permalink / raw)
To: Robin Murphy, Sebastian Fricke, linux-media, Nicolas Dufresne
Cc: knaerzche, kernel, bob.beckett, ezequiel, mchehab, gregkh,
linux-kernel, linux-rockchip, linux-staging, Yury Norov,
Andy Shevchenko, Rasmus Villemoes
Dne petek, 15. julij 2022 ob 17:36:01 CEST je Nicolas Dufresne napisal(a):
> Le vendredi 15 juillet 2022 à 12:04 +0100, Robin Murphy a écrit :
> > On 2022-07-13 17:24, Sebastian Fricke wrote:
> > > Implement the HEVC codec variation for the RkVDEC driver. Currently only
> > > the RK3399 is supported, but it is possible to enable the RK3288 as it
> > > also supports this codec.
> > >
> > > Based on top of the media tree @ef7fcbbb9eabbe86d2287484bf366dd1821cc6b8
> > > and the HEVC uABI MR by Benjamin Gaignard.
> > > (https://patchwork.linuxtv.org/project/linux-media/list/?series=8360)
> > >
> > > Tested with the GStreamer V4L2 HEVC plugin:
> > > (https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/107
> > > 9)
> > >
> > > Current Fluster score:
> > > `Ran 131/147 tests successfully in 278.568 secs`
> > > with
> > > `python3 fluster.py run -d GStreamer-H.265-V4L2SL-Gst1.0 -ts
> > > JCT-VC-HEVC_V1 -j1`
> > >
> > > failed conformance tests:
> > > - DBLK_D_VIXS_2 (Success on Hantro G2)
> > > - DSLICE_A_HHI_5 (Success on Hantro G2)
> > > - EXT_A_ericsson_4 (Success on Hantro G2)
> > > - PICSIZE_A_Bossen_1 (Hardware limitation)
> > > - PICSIZE_B_Bossen_1 (Hardware limitation)
> > > - PICSIZE_C_Bossen_1 (Hardware limitation)
> > > - PICSIZE_D_Bossen_1 (Hardware limitation)
> > > - PPS_A_qualcomm_7 (Success on Hantro G2)
> > > - SAODBLK_A_MainConcept_4 (Success on Hantro G2)
> > > - SAODBLK_B_MainConcept_4 (Success on Hantro G2)
> > > - SLIST_B_Sony_9 (Success on Hantro G2)
> > > - SLIST_D_Sony_9 (Success on Hantro G2)
> > > - TSUNEQBD_A_MAIN10_Technicolor_2 (Success on Hantro G2)
> > > - VPSSPSPPS_A_MainConcept_1 (Success on Hantro G2)
> > > - WPP_D_ericsson_MAIN10_2 (Fail on Hantro G2)
> > > - WPP_D_ericsson_MAIN_2 (Fail on Hantro G2)
> > >
> > > Not tested with FFMpeg so far.
> > >
> > > Known issues:
> > > - Unable to reliably decode multiple videos concurrently
> > > - The SAODBLK_* tests timeout if the timeout time in fluster is lower
> > > than 120 - Currently the uv_virstride is calculated in a manner that is
> > > hardcoded for the two available formats NV12 and NV15.
> > > (@config_registers)
> > >
> > > Notable design decisions:
> > > - I opted for a bitfield to represent the PPS memory blob as it is the
> > > perfect tool for that job. It describes the memory layout with any
> > > additional required documentation, is easy to read and a native language
> > > tool for that job
> >
> > Can I point out how terrible an idea this is? The C language gives
> > virtually zero guarantee about how bitfields are actually represented in
> > memory. Platform ABIs (e.g. [1]) might nail things down a bit more, but
> > different platforms are free to make completely different choices so
> > portability still goes out the window. Even for a single platform,
> > different compilers (or at worst even different version of one compiler)
> > can still make incompatible choices e.g. WRT alignment of packed
> > members. Even if you narrow the scope as far as a specific version of
> > AArch64 GCC, I think this is still totally broken for big-endian.
> >
> > The fact that you've had to use nonsensical types to trick a compiler
> > into meeting your expectations should already be a clue to how fragile
> > this is in general.
> >
> > > - The RPS memory blob is created using a bitmap implementation, which
> > > uses a common Kernel API to avoid reinventing the wheel and to keep the
> > > code clean.
> >
> > Similarly, Linux bitmaps are designed for use as, well, bitmaps. Abusing
> > them as a data interchange format for bit-aligned numerical values is
> > far from "clean" semantically. And I'm pretty sure it's also broken for
> > big-endian.
> >
> > This kind of stuff may be standard practice in embedded development
> > where you're targeting a specific MCU with a specific toolchain, but I
> > don't believe it's suitable for upstream Linux. It would take pretty
> > much the same number of lines to use GENMASK definitions and bitfield.h
> > helpers to pack values into words which can then be written to memory in
> > a guaranteed format and endianness (certainly for the PPS; for the RPS
> > it may well end up a bit longer, but would be self-documenting and
> > certainly more readable than those loops). It mostly just means that for
> > any field which crosses a word boundary you'll end up with 2 definitions
> > and 2 assignments, which is hardly a problem (and in some ways more
> > honest about what's actually going on).
>
> Thanks for the feedback, in multimedia (unlike register programming), we
> don't really consider bitstreams as bitmap or bitfield. What we do really
> expect is to use bit writer helpers (and sometimes a bit reader though we
> try and avoid the second one in the kernel). Its more of less a cursor (a
> bit position) into a memory that advance while writing. A bit writer should
> help protect against overflow too.
>
> When writing lets say a chain of 8 bits from a char, a proper helper is
> expected to be very explicit on the ordering (write_u8_le/be or something
> better worded). I would rather like to see all these blobs written this way
> personally then having a cleared buffer and writing using bit offsets.
>
> Perhaps I may suggest to start with implementing just that inside this
> driver? It isn't very hard, and then the implementation can be reduced
> later and shared later, with whatever exists without deviating from the
> intent of the existing API ? I do believe that having this in linux-media
> can be useful in the future. We will notably need to extend such a helper
> with multimedia specific coding technique (golomb, boolean coding, etc.)
> for use in stateless encoder drivers.
I don't know RKVDEC, but at least Cedar has integrated bitstream parsing
engine. Is there something similar in RKVDEC? That way HW could be used
instead of SW implementation.
Best regards,
Jernej
>
> Nicolas
>
> > Thanks,
> > Robin.
> >
> > [1]
> > https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#bit-f
> > ields>
> > > - I deliberatly opted against the macro solution used in H264, which
> > > declares Macros in mid function and declares the fields of the memory
> > > blob as macros as well. And I would be glad to refactor the H264 code if
> > > desired by the maintainer to use common Kernel APIs and native language
> > > elements.
> > > - The giant static array of cabac values is moved to a separate c file,
> > > I did so because a separate .h file would be incorrect as it doesn't
> > > expose anything of any value for any other file than the rkvdec-hevc.c
> > >
> > > file. Other options were:
> > > - Calculating the values instead of storing the results (doesn't seem
> > > to be worth it)
> > > - Supply them via firmware (Adding firmware makes the whole software
> > > way more complicated and the usage of the driver less obvious)
> > >
> > > Ignored Checkpatch warnings (as it fits to the current style of the
> > > file):
> > > ```
> > > WARNING: line length of 162 exceeds 100 columns
> > > #115: FILE: drivers/media/v4l2-core/v4l2-common.c:265:
> > > + { .format = V4L2_PIX_FMT_NV15, .pixel_enc =
> > > V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 5,
> > > 0, 0 }, .hdiv = 2, .vdiv = 2,
> > >
> > > ERROR: trailing statements should be on next line
> > > #128: FILE: drivers/media/v4l2-core/v4l2-ioctl.c:1305:
> > > + case V4L2_PIX_FMT_NV15: descr = "10-bit Y/CbCr 4:2:0
> > > (Packed)"; break; ```
> > >
> > > v4l2-compliance test:
> > > ```
> > > Total for rkvdec device /dev/video3: 46, Succeeded: 46, Failed: 0,
> > > Warnings: 0 ```
> > >
> > > kselftest module run for the bitmap changes:
> > > ```
> > > $ sudo insmod
> > > /usr/lib/modules/5.19.0-rc3-finalseries/kernel/lib/test_bitmap.ko [
> > > 71.751716] test_bitmap: parselist: 14: input is '0-2047:128/256' OK,
> > > Time: 1750 [ 71.751787] test_bitmap: bitmap_print_to_pagebuf: input
> > > is '0-32767 [ 71.751787] ', Time: 6708
> > > [ 71.760373] test_bitmap: set_value: 6/6 tests correct
> > > ```
> > >
> > > Jonas Karlman (2):
> > > media: v4l2: Add NV15 pixel format
> > > media: v4l2-common: Add helpers to calculate bytesperline and
> > >
> > > sizeimage
> > >
> > > Sebastian Fricke (4):
> > > bitops: bitmap helper to set variable length values
> > > staging: media: rkvdec: Add valid pixel format check
> > > staging: media: rkvdec: Enable S_CTRL IOCTL
> > > staging: media: rkvdec: Add HEVC backend
> > >
> > > .../media/v4l/pixfmt-yuv-planar.rst | 53 +
> > > drivers/media/v4l2-core/v4l2-common.c | 79 +-
> > > drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
> > > drivers/staging/media/rkvdec/Makefile | 2 +-
> > > drivers/staging/media/rkvdec/TODO | 22 +-
> > > .../staging/media/rkvdec/rkvdec-hevc-data.c | 1844 +++++++++++++++++
> > > drivers/staging/media/rkvdec/rkvdec-hevc.c | 859 ++++++++
> > > drivers/staging/media/rkvdec/rkvdec-regs.h | 1 +
> > > drivers/staging/media/rkvdec/rkvdec.c | 182 +-
> > > drivers/staging/media/rkvdec/rkvdec.h | 3 +
> > > include/linux/bitmap.h | 39 +
> > > include/uapi/linux/videodev2.h | 1 +
> > > lib/test_bitmap.c | 47 +
> > > 13 files changed, 3066 insertions(+), 67 deletions(-)
> > > create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc-data.c
> > > create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc.c
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/6] RkVDEC HEVC driver
2022-07-13 16:24 [PATCH 0/6] RkVDEC HEVC driver Sebastian Fricke
` (3 preceding siblings ...)
2022-07-15 11:04 ` Robin Murphy
@ 2022-07-16 16:27 ` Alex Bee
2022-07-21 16:52 ` Sebastian Fricke
4 siblings, 1 reply; 17+ messages in thread
From: Alex Bee @ 2022-07-16 16:27 UTC (permalink / raw)
To: Sebastian Fricke, linux-media
Cc: jernej.skrabec, kernel, bob.beckett, ezequiel, mchehab, gregkh,
linux-kernel, linux-rockchip, linux-staging, nicolas.dufresne,
Yury Norov, Andy Shevchenko, Rasmus Villemoes
Hi Sebastian,
thanks a lot for your work on upstreaming this driver.
See some general comments below.
> Implement the HEVC codec variation for the RkVDEC driver. Currently only
> the RK3399 is supported, but it is possible to enable the RK3288 as it
> also supports this codec.
>
> Based on top of the media tree @ef7fcbbb9eabbe86d2287484bf366dd1821cc6b8
> and the HEVC uABI MR by Benjamin Gaignard.
> (https://patchwork.linuxtv.org/project/linux-media/list/?series=8360)
>
> Tested with the GStreamer V4L2 HEVC plugin:
> (https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1079)
>
> Current Fluster score:
> `Ran 131/147 tests successfully in 278.568 secs`
> with
> `python3 fluster.py run -d GStreamer-H.265-V4L2SL-Gst1.0 -ts JCT-VC-HEVC_V1 -j1`
>
> failed conformance tests:
> - DBLK_D_VIXS_2 (Success on Hantro G2)
> - DSLICE_A_HHI_5 (Success on Hantro G2)
> - EXT_A_ericsson_4 (Success on Hantro G2)
> - PICSIZE_A_Bossen_1 (Hardware limitation)
> - PICSIZE_B_Bossen_1 (Hardware limitation)
> - PICSIZE_C_Bossen_1 (Hardware limitation)
> - PICSIZE_D_Bossen_1 (Hardware limitation)
> - PPS_A_qualcomm_7 (Success on Hantro G2)
> - SAODBLK_A_MainConcept_4 (Success on Hantro G2)
> - SAODBLK_B_MainConcept_4 (Success on Hantro G2)
> - SLIST_B_Sony_9 (Success on Hantro G2)
> - SLIST_D_Sony_9 (Success on Hantro G2)
> - TSUNEQBD_A_MAIN10_Technicolor_2 (Success on Hantro G2)
> - VPSSPSPPS_A_MainConcept_1 (Success on Hantro G2)
> - WPP_D_ericsson_MAIN10_2 (Fail on Hantro G2)
> - WPP_D_ericsson_MAIN_2 (Fail on Hantro G2)
>
> Not tested with FFMpeg so far.
>
> Known issues:
> - Unable to reliably decode multiple videos concurrently
> - The SAODBLK_* tests timeout if the timeout time in fluster is lower than 120
> - Currently the uv_virstride is calculated in a manner that is hardcoded
> for the two available formats NV12 and NV15. (@config_registers)
>
> Notable design decisions:
> - I opted for a bitfield to represent the PPS memory blob as it is the
> perfect tool for that job. It describes the memory layout with any
> additional required documentation, is easy to read and a native language
> tool for that job
> - The RPS memory blob is created using a bitmap implementation, which
> uses a common Kernel API to avoid reinventing the wheel and to keep the
> code clean.
> - I deliberatly opted against the macro solution used in H264, which
> declares Macros in mid function and declares the fields of the memory
> blob as macros as well. And I would be glad to refactor the H264 code if
> desired by the maintainer to use common Kernel APIs and native language
> elements.
I fully disagree here: That way the code is much less
read-/understandable - your are putting bits at some random hardcoded
positions with not relation to the codec/hardware and expect everyone to
read and understand that huge docblock - the code should be more
self-explaining and we should at least try to get rid of those hardcoded
positions which, btw, will differ for newer versions of that hardware block.
I'm also not sure what makes you call that a "blob": It's configuration
of the hardware which in that case isn't put in registers, but in memory.
> - The giant static array of cabac values is moved to a separate c file,
> I did so because a separate .h file would be incorrect as it doesn't
> expose anything of any value for any other file than the rkvdec-hevc.c
> file. Other options were:
> - Calculating the values instead of storing the results (doesn't seem
> to be worth it)
I'm not sure "not worth it" should be argument for not doing doing
anything in general; especially not if it can explain the relation
between the standard and this driver.
Looking at tables of ITU-T Rec. H.265 "9.3.2.2 Initialization process
for context variables" and comparing to the first elements of that huge
array: It should be doable.
> - Supply them via firmware (Adding firmware makes the whole software
> way more complicated and the usage of the driver less obvious)
>
> Ignored Checkpatch warnings (as it fits to the current style of the file):
> ```
> WARNING: line length of 162 exceeds 100 columns
> #115: FILE: drivers/media/v4l2-core/v4l2-common.c:265:
> + { .format = V4L2_PIX_FMT_NV15, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 5, 0, 0 }, .hdiv = 2, .vdiv = 2,
>
> ERROR: trailing statements should be on next line
> #128: FILE: drivers/media/v4l2-core/v4l2-ioctl.c:1305:
> + case V4L2_PIX_FMT_NV15: descr = "10-bit Y/CbCr 4:2:0 (Packed)"; break;
> ```
>
> v4l2-compliance test:
> ```
> Total for rkvdec device /dev/video3: 46, Succeeded: 46, Failed: 0, Warnings: 0
> ```
>
> kselftest module run for the bitmap changes:
> ```
> $ sudo insmod /usr/lib/modules/5.19.0-rc3-finalseries/kernel/lib/test_bitmap.ko
> [ 71.751716] test_bitmap: parselist: 14: input is '0-2047:128/256' OK, Time: 1750
> [ 71.751787] test_bitmap: bitmap_print_to_pagebuf: input is '0-32767
> [ 71.751787] ', Time: 6708
> [ 71.760373] test_bitmap: set_value: 6/6 tests correct
> ```
>
> Jonas Karlman (2):
> media: v4l2: Add NV15 pixel format
> media: v4l2-common: Add helpers to calculate bytesperline and
> sizeimage
>
> Sebastian Fricke (4):
> bitops: bitmap helper to set variable length values
> staging: media: rkvdec: Add valid pixel format check
> staging: media: rkvdec: Enable S_CTRL IOCTL
> staging: media: rkvdec: Add HEVC backend
>
> .../media/v4l/pixfmt-yuv-planar.rst | 53 +
> drivers/media/v4l2-core/v4l2-common.c | 79 +-
> drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
> drivers/staging/media/rkvdec/Makefile | 2 +-
> drivers/staging/media/rkvdec/TODO | 22 +-
> .../staging/media/rkvdec/rkvdec-hevc-data.c | 1844 +++++++++++++++++
> drivers/staging/media/rkvdec/rkvdec-hevc.c | 859 ++++++++
> drivers/staging/media/rkvdec/rkvdec-regs.h | 1 +
> drivers/staging/media/rkvdec/rkvdec.c | 182 +-
> drivers/staging/media/rkvdec/rkvdec.h | 3 +
> include/linux/bitmap.h | 39 +
> include/uapi/linux/videodev2.h | 1 +
> lib/test_bitmap.c | 47 +
> 13 files changed, 3066 insertions(+), 67 deletions(-)
> create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc-data.c
> create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc.c
>
Regards,
Alex
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/6] RkVDEC HEVC driver
2022-07-16 6:45 ` Jernej Škrabec
@ 2022-07-18 14:54 ` Nicolas Dufresne
0 siblings, 0 replies; 17+ messages in thread
From: Nicolas Dufresne @ 2022-07-18 14:54 UTC (permalink / raw)
To: Jernej Škrabec, Robin Murphy, Sebastian Fricke, linux-media
Cc: knaerzche, kernel, bob.beckett, ezequiel, mchehab, gregkh,
linux-kernel, linux-rockchip, linux-staging, Yury Norov,
Andy Shevchenko, Rasmus Villemoes
Le samedi 16 juillet 2022 à 08:45 +0200, Jernej Škrabec a écrit :
> Dne petek, 15. julij 2022 ob 17:36:01 CEST je Nicolas Dufresne napisal(a):
> > Le vendredi 15 juillet 2022 à 12:04 +0100, Robin Murphy a écrit :
> > > On 2022-07-13 17:24, Sebastian Fricke wrote:
> > > > Implement the HEVC codec variation for the RkVDEC driver. Currently only
> > > > the RK3399 is supported, but it is possible to enable the RK3288 as it
> > > > also supports this codec.
> > > >
> > > > Based on top of the media tree @ef7fcbbb9eabbe86d2287484bf366dd1821cc6b8
> > > > and the HEVC uABI MR by Benjamin Gaignard.
> > > > (https://patchwork.linuxtv.org/project/linux-media/list/?series=8360)
> > > >
> > > > Tested with the GStreamer V4L2 HEVC plugin:
> > > > (https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/107
> > > > 9)
> > > >
> > > > Current Fluster score:
> > > > `Ran 131/147 tests successfully in 278.568 secs`
> > > > with
> > > > `python3 fluster.py run -d GStreamer-H.265-V4L2SL-Gst1.0 -ts
> > > > JCT-VC-HEVC_V1 -j1`
> > > >
> > > > failed conformance tests:
> > > > - DBLK_D_VIXS_2 (Success on Hantro G2)
> > > > - DSLICE_A_HHI_5 (Success on Hantro G2)
> > > > - EXT_A_ericsson_4 (Success on Hantro G2)
> > > > - PICSIZE_A_Bossen_1 (Hardware limitation)
> > > > - PICSIZE_B_Bossen_1 (Hardware limitation)
> > > > - PICSIZE_C_Bossen_1 (Hardware limitation)
> > > > - PICSIZE_D_Bossen_1 (Hardware limitation)
> > > > - PPS_A_qualcomm_7 (Success on Hantro G2)
> > > > - SAODBLK_A_MainConcept_4 (Success on Hantro G2)
> > > > - SAODBLK_B_MainConcept_4 (Success on Hantro G2)
> > > > - SLIST_B_Sony_9 (Success on Hantro G2)
> > > > - SLIST_D_Sony_9 (Success on Hantro G2)
> > > > - TSUNEQBD_A_MAIN10_Technicolor_2 (Success on Hantro G2)
> > > > - VPSSPSPPS_A_MainConcept_1 (Success on Hantro G2)
> > > > - WPP_D_ericsson_MAIN10_2 (Fail on Hantro G2)
> > > > - WPP_D_ericsson_MAIN_2 (Fail on Hantro G2)
> > > >
> > > > Not tested with FFMpeg so far.
> > > >
> > > > Known issues:
> > > > - Unable to reliably decode multiple videos concurrently
> > > > - The SAODBLK_* tests timeout if the timeout time in fluster is lower
> > > > than 120 - Currently the uv_virstride is calculated in a manner that is
> > > > hardcoded for the two available formats NV12 and NV15.
> > > > (@config_registers)
> > > >
> > > > Notable design decisions:
> > > > - I opted for a bitfield to represent the PPS memory blob as it is the
> > > > perfect tool for that job. It describes the memory layout with any
> > > > additional required documentation, is easy to read and a native language
> > > > tool for that job
> > >
> > > Can I point out how terrible an idea this is? The C language gives
> > > virtually zero guarantee about how bitfields are actually represented in
> > > memory. Platform ABIs (e.g. [1]) might nail things down a bit more, but
> > > different platforms are free to make completely different choices so
> > > portability still goes out the window. Even for a single platform,
> > > different compilers (or at worst even different version of one compiler)
> > > can still make incompatible choices e.g. WRT alignment of packed
> > > members. Even if you narrow the scope as far as a specific version of
> > > AArch64 GCC, I think this is still totally broken for big-endian.
> > >
> > > The fact that you've had to use nonsensical types to trick a compiler
> > > into meeting your expectations should already be a clue to how fragile
> > > this is in general.
> > >
> > > > - The RPS memory blob is created using a bitmap implementation, which
> > > > uses a common Kernel API to avoid reinventing the wheel and to keep the
> > > > code clean.
> > >
> > > Similarly, Linux bitmaps are designed for use as, well, bitmaps. Abusing
> > > them as a data interchange format for bit-aligned numerical values is
> > > far from "clean" semantically. And I'm pretty sure it's also broken for
> > > big-endian.
> > >
> > > This kind of stuff may be standard practice in embedded development
> > > where you're targeting a specific MCU with a specific toolchain, but I
> > > don't believe it's suitable for upstream Linux. It would take pretty
> > > much the same number of lines to use GENMASK definitions and bitfield.h
> > > helpers to pack values into words which can then be written to memory in
> > > a guaranteed format and endianness (certainly for the PPS; for the RPS
> > > it may well end up a bit longer, but would be self-documenting and
> > > certainly more readable than those loops). It mostly just means that for
> > > any field which crosses a word boundary you'll end up with 2 definitions
> > > and 2 assignments, which is hardly a problem (and in some ways more
> > > honest about what's actually going on).
> >
> > Thanks for the feedback, in multimedia (unlike register programming), we
> > don't really consider bitstreams as bitmap or bitfield. What we do really
> > expect is to use bit writer helpers (and sometimes a bit reader though we
> > try and avoid the second one in the kernel). Its more of less a cursor (a
> > bit position) into a memory that advance while writing. A bit writer should
> > help protect against overflow too.
> >
> > When writing lets say a chain of 8 bits from a char, a proper helper is
> > expected to be very explicit on the ordering (write_u8_le/be or something
> > better worded). I would rather like to see all these blobs written this way
> > personally then having a cleared buffer and writing using bit offsets.
> >
> > Perhaps I may suggest to start with implementing just that inside this
> > driver? It isn't very hard, and then the implementation can be reduced
> > later and shared later, with whatever exists without deviating from the
> > intent of the existing API ? I do believe that having this in linux-media
> > can be useful in the future. We will notably need to extend such a helper
> > with multimedia specific coding technique (golomb, boolean coding, etc.)
> > for use in stateless encoder drivers.
>
> I don't know RKVDEC, but at least Cedar has integrated bitstream parsing
> engine. Is there something similar in RKVDEC? That way HW could be used
> instead of SW implementation.
This is unrelated, since the code here generates a bitstream. Some of the
parameters you'd pass with registers with other drivers, are passed with memory
chunk in rkvdec. Not all these blob have a byte aligned memory layout, they are
instead bitstream without any consideration for byte alignment. So we need a
tool to create such a bitstream. Similar tool will be needed for adapting
encoders.
>
> Best regards,
> Jernej
>
> >
> > Nicolas
> >
> > > Thanks,
> > > Robin.
> > >
> > > [1]
> > > https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#bit-f
> > > ields>
> > > > - I deliberatly opted against the macro solution used in H264, which
> > > > declares Macros in mid function and declares the fields of the memory
> > > > blob as macros as well. And I would be glad to refactor the H264 code if
> > > > desired by the maintainer to use common Kernel APIs and native language
> > > > elements.
> > > > - The giant static array of cabac values is moved to a separate c file,
> > > > I did so because a separate .h file would be incorrect as it doesn't
> > > > expose anything of any value for any other file than the rkvdec-hevc.c
> > > >
> > > > file. Other options were:
> > > > - Calculating the values instead of storing the results (doesn't seem
> > > > to be worth it)
> > > > - Supply them via firmware (Adding firmware makes the whole software
> > > > way more complicated and the usage of the driver less obvious)
> > > >
> > > > Ignored Checkpatch warnings (as it fits to the current style of the
> > > > file):
> > > > ```
> > > > WARNING: line length of 162 exceeds 100 columns
> > > > #115: FILE: drivers/media/v4l2-core/v4l2-common.c:265:
> > > > + { .format = V4L2_PIX_FMT_NV15, .pixel_enc =
> > > > V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 5,
> > > > 0, 0 }, .hdiv = 2, .vdiv = 2,
> > > >
> > > > ERROR: trailing statements should be on next line
> > > > #128: FILE: drivers/media/v4l2-core/v4l2-ioctl.c:1305:
> > > > + case V4L2_PIX_FMT_NV15: descr = "10-bit Y/CbCr 4:2:0
> > > > (Packed)"; break; ```
> > > >
> > > > v4l2-compliance test:
> > > > ```
> > > > Total for rkvdec device /dev/video3: 46, Succeeded: 46, Failed: 0,
> > > > Warnings: 0 ```
> > > >
> > > > kselftest module run for the bitmap changes:
> > > > ```
> > > > $ sudo insmod
> > > > /usr/lib/modules/5.19.0-rc3-finalseries/kernel/lib/test_bitmap.ko [
> > > > 71.751716] test_bitmap: parselist: 14: input is '0-2047:128/256' OK,
> > > > Time: 1750 [ 71.751787] test_bitmap: bitmap_print_to_pagebuf: input
> > > > is '0-32767 [ 71.751787] ', Time: 6708
> > > > [ 71.760373] test_bitmap: set_value: 6/6 tests correct
> > > > ```
> > > >
> > > > Jonas Karlman (2):
> > > > media: v4l2: Add NV15 pixel format
> > > > media: v4l2-common: Add helpers to calculate bytesperline and
> > > >
> > > > sizeimage
> > > >
> > > > Sebastian Fricke (4):
> > > > bitops: bitmap helper to set variable length values
> > > > staging: media: rkvdec: Add valid pixel format check
> > > > staging: media: rkvdec: Enable S_CTRL IOCTL
> > > > staging: media: rkvdec: Add HEVC backend
> > > >
> > > > .../media/v4l/pixfmt-yuv-planar.rst | 53 +
> > > > drivers/media/v4l2-core/v4l2-common.c | 79 +-
> > > > drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
> > > > drivers/staging/media/rkvdec/Makefile | 2 +-
> > > > drivers/staging/media/rkvdec/TODO | 22 +-
> > > > .../staging/media/rkvdec/rkvdec-hevc-data.c | 1844 +++++++++++++++++
> > > > drivers/staging/media/rkvdec/rkvdec-hevc.c | 859 ++++++++
> > > > drivers/staging/media/rkvdec/rkvdec-regs.h | 1 +
> > > > drivers/staging/media/rkvdec/rkvdec.c | 182 +-
> > > > drivers/staging/media/rkvdec/rkvdec.h | 3 +
> > > > include/linux/bitmap.h | 39 +
> > > > include/uapi/linux/videodev2.h | 1 +
> > > > lib/test_bitmap.c | 47 +
> > > > 13 files changed, 3066 insertions(+), 67 deletions(-)
> > > > create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc-data.c
> > > > create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc.c
>
>
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/6] RkVDEC HEVC driver
2022-07-15 15:36 ` Nicolas Dufresne
2022-07-16 6:45 ` Jernej Škrabec
@ 2022-07-21 16:16 ` Sebastian Fricke
2022-07-21 16:18 ` Ezequiel Garcia
1 sibling, 1 reply; 17+ messages in thread
From: Sebastian Fricke @ 2022-07-21 16:16 UTC (permalink / raw)
To: Nicolas Dufresne
Cc: Robin Murphy, linux-media, jernej.skrabec, knaerzche, kernel,
bob.beckett, ezequiel, mchehab, gregkh, linux-kernel,
linux-rockchip, linux-staging, Yury Norov, Andy Shevchenko,
Rasmus Villemoes
Hey Nicolas & Robin,
Thanks for the feedback.
On 15.07.2022 11:36, Nicolas Dufresne wrote:
>Le vendredi 15 juillet 2022 à 12:04 +0100, Robin Murphy a écrit :
>> On 2022-07-13 17:24, Sebastian Fricke wrote:
>> > Implement the HEVC codec variation for the RkVDEC driver. Currently only
>> > the RK3399 is supported, but it is possible to enable the RK3288 as it
>> > also supports this codec.
>> >
>> > Based on top of the media tree @ef7fcbbb9eabbe86d2287484bf366dd1821cc6b8
>> > and the HEVC uABI MR by Benjamin Gaignard.
>> > (https://patchwork.linuxtv.org/project/linux-media/list/?series=8360)
>> >
>> > Tested with the GStreamer V4L2 HEVC plugin:
>> > (https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1079)
>> >
>> > Current Fluster score:
>> > `Ran 131/147 tests successfully in 278.568 secs`
>> > with
>> > `python3 fluster.py run -d GStreamer-H.265-V4L2SL-Gst1.0 -ts JCT-VC-HEVC_V1 -j1`
>> >
>> > failed conformance tests:
>> > - DBLK_D_VIXS_2 (Success on Hantro G2)
>> > - DSLICE_A_HHI_5 (Success on Hantro G2)
>> > - EXT_A_ericsson_4 (Success on Hantro G2)
>> > - PICSIZE_A_Bossen_1 (Hardware limitation)
>> > - PICSIZE_B_Bossen_1 (Hardware limitation)
>> > - PICSIZE_C_Bossen_1 (Hardware limitation)
>> > - PICSIZE_D_Bossen_1 (Hardware limitation)
>> > - PPS_A_qualcomm_7 (Success on Hantro G2)
>> > - SAODBLK_A_MainConcept_4 (Success on Hantro G2)
>> > - SAODBLK_B_MainConcept_4 (Success on Hantro G2)
>> > - SLIST_B_Sony_9 (Success on Hantro G2)
>> > - SLIST_D_Sony_9 (Success on Hantro G2)
>> > - TSUNEQBD_A_MAIN10_Technicolor_2 (Success on Hantro G2)
>> > - VPSSPSPPS_A_MainConcept_1 (Success on Hantro G2)
>> > - WPP_D_ericsson_MAIN10_2 (Fail on Hantro G2)
>> > - WPP_D_ericsson_MAIN_2 (Fail on Hantro G2)
>> >
>> > Not tested with FFMpeg so far.
>> >
>> > Known issues:
>> > - Unable to reliably decode multiple videos concurrently
>> > - The SAODBLK_* tests timeout if the timeout time in fluster is lower than 120
>> > - Currently the uv_virstride is calculated in a manner that is hardcoded
>> > for the two available formats NV12 and NV15. (@config_registers)
>> >
>> > Notable design decisions:
>> > - I opted for a bitfield to represent the PPS memory blob as it is the
>> > perfect tool for that job. It describes the memory layout with any
>> > additional required documentation, is easy to read and a native language
>> > tool for that job
>>
>> Can I point out how terrible an idea this is? The C language gives
>> virtually zero guarantee about how bitfields are actually represented in
>> memory. Platform ABIs (e.g. [1]) might nail things down a bit more, but
>> different platforms are free to make completely different choices so
>> portability still goes out the window. Even for a single platform,
>> different compilers (or at worst even different version of one compiler)
>> can still make incompatible choices e.g. WRT alignment of packed
>> members. Even if you narrow the scope as far as a specific version of
>> AArch64 GCC, I think this is still totally broken for big-endian.
>>
>> The fact that you've had to use nonsensical types to trick a compiler
>> into meeting your expectations should already be a clue to how fragile
>> this is in general.
>>
>> > - The RPS memory blob is created using a bitmap implementation, which
>> > uses a common Kernel API to avoid reinventing the wheel and to keep the
>> > code clean.
>>
>> Similarly, Linux bitmaps are designed for use as, well, bitmaps. Abusing
>> them as a data interchange format for bit-aligned numerical values is
>> far from "clean" semantically. And I'm pretty sure it's also broken for
>> big-endian.
>>
>> This kind of stuff may be standard practice in embedded development
>> where you're targeting a specific MCU with a specific toolchain, but I
>> don't believe it's suitable for upstream Linux. It would take pretty
>> much the same number of lines to use GENMASK definitions and bitfield.h
>> helpers to pack values into words which can then be written to memory in
>> a guaranteed format and endianness (certainly for the PPS; for the RPS
>> it may well end up a bit longer, but would be self-documenting and
>> certainly more readable than those loops). It mostly just means that for
>> any field which crosses a word boundary you'll end up with 2 definitions
>> and 2 assignments, which is hardly a problem (and in some ways more
>> honest about what's actually going on).
>
>Thanks for the feedback, in multimedia (unlike register programming), we don't
>really consider bitstreams as bitmap or bitfield. What we do really expect is to
>use bit writer helpers (and sometimes a bit reader though we try and avoid the
>second one in the kernel). Its more of less a cursor (a bit position) into a
>memory that advance while writing. A bit writer should help protect against
>overflow too.
>
>When writing lets say a chain of 8 bits from a char, a proper helper is expected
>to be very explicit on the ordering (write_u8_le/be or something better worded).
>I would rather like to see all these blobs written this way personally then
>having a cleared buffer and writing using bit offsets.
>
>Perhaps I may suggest to start with implementing just that inside this driver?
>It isn't very hard, and then the implementation can be reduced later and shared
>later, with whatever exists without deviating from the intent of the existing
>API ? I do believe that having this in linux-media can be useful in the future.
>We will notably need to extend such a helper with multimedia specific coding
>technique (golomb, boolean coding, etc.) for use in stateless encoder drivers.
I currently design a general bit-writer API to handle the mentioned
issues correctly. I'll post it as part of V2, due to my current workload
this will happen in 3 weeks at the earliest.
>
>Nicolas
>
>>
>> Thanks,
>> Robin.
Greetings,
Sebastian
>>
>> [1]
>> https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#bit-fields
>>
>> > - I deliberatly opted against the macro solution used in H264, which
>> > declares Macros in mid function and declares the fields of the memory
>> > blob as macros as well. And I would be glad to refactor the H264 code if
>> > desired by the maintainer to use common Kernel APIs and native language
>> > elements.
>> > - The giant static array of cabac values is moved to a separate c file,
>> > I did so because a separate .h file would be incorrect as it doesn't
>> > expose anything of any value for any other file than the rkvdec-hevc.c
>> > file. Other options were:
>> > - Calculating the values instead of storing the results (doesn't seem
>> > to be worth it)
>> > - Supply them via firmware (Adding firmware makes the whole software
>> > way more complicated and the usage of the driver less obvious)
>> >
>> > Ignored Checkpatch warnings (as it fits to the current style of the file):
>> > ```
>> > WARNING: line length of 162 exceeds 100 columns
>> > #115: FILE: drivers/media/v4l2-core/v4l2-common.c:265:
>> > + { .format = V4L2_PIX_FMT_NV15, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 5, 0, 0 }, .hdiv = 2, .vdiv = 2,
>> >
>> > ERROR: trailing statements should be on next line
>> > #128: FILE: drivers/media/v4l2-core/v4l2-ioctl.c:1305:
>> > + case V4L2_PIX_FMT_NV15: descr = "10-bit Y/CbCr 4:2:0 (Packed)"; break;
>> > ```
>> >
>> > v4l2-compliance test:
>> > ```
>> > Total for rkvdec device /dev/video3: 46, Succeeded: 46, Failed: 0, Warnings: 0
>> > ```
>> >
>> > kselftest module run for the bitmap changes:
>> > ```
>> > $ sudo insmod /usr/lib/modules/5.19.0-rc3-finalseries/kernel/lib/test_bitmap.ko
>> > [ 71.751716] test_bitmap: parselist: 14: input is '0-2047:128/256' OK, Time: 1750
>> > [ 71.751787] test_bitmap: bitmap_print_to_pagebuf: input is '0-32767
>> > [ 71.751787] ', Time: 6708
>> > [ 71.760373] test_bitmap: set_value: 6/6 tests correct
>> > ```
>> >
>> > Jonas Karlman (2):
>> > media: v4l2: Add NV15 pixel format
>> > media: v4l2-common: Add helpers to calculate bytesperline and
>> > sizeimage
>> >
>> > Sebastian Fricke (4):
>> > bitops: bitmap helper to set variable length values
>> > staging: media: rkvdec: Add valid pixel format check
>> > staging: media: rkvdec: Enable S_CTRL IOCTL
>> > staging: media: rkvdec: Add HEVC backend
>> >
>> > .../media/v4l/pixfmt-yuv-planar.rst | 53 +
>> > drivers/media/v4l2-core/v4l2-common.c | 79 +-
>> > drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
>> > drivers/staging/media/rkvdec/Makefile | 2 +-
>> > drivers/staging/media/rkvdec/TODO | 22 +-
>> > .../staging/media/rkvdec/rkvdec-hevc-data.c | 1844 +++++++++++++++++
>> > drivers/staging/media/rkvdec/rkvdec-hevc.c | 859 ++++++++
>> > drivers/staging/media/rkvdec/rkvdec-regs.h | 1 +
>> > drivers/staging/media/rkvdec/rkvdec.c | 182 +-
>> > drivers/staging/media/rkvdec/rkvdec.h | 3 +
>> > include/linux/bitmap.h | 39 +
>> > include/uapi/linux/videodev2.h | 1 +
>> > lib/test_bitmap.c | 47 +
>> > 13 files changed, 3066 insertions(+), 67 deletions(-)
>> > create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc-data.c
>> > create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc.c
>> >
>>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/6] RkVDEC HEVC driver
2022-07-21 16:16 ` Sebastian Fricke
@ 2022-07-21 16:18 ` Ezequiel Garcia
2022-07-21 20:11 ` Sebastian Fricke
0 siblings, 1 reply; 17+ messages in thread
From: Ezequiel Garcia @ 2022-07-21 16:18 UTC (permalink / raw)
To: Sebastian Fricke
Cc: Nicolas Dufresne, Robin Murphy, linux-media, Jernej Škrabec,
Alex Bee, Collabora Kernel ML, Robert Beckett,
Mauro Carvalho Chehab, Greg KH, Linux Kernel Mailing List,
open list:ARM/Rockchip SoC...,
open list:STAGING SUBSYSTEM, Yury Norov, Andy Shevchenko,
Rasmus Villemoes
On Thu, Jul 21, 2022 at 1:16 PM Sebastian Fricke
<sebastian.fricke@collabora.com> wrote:
>
> Hey Nicolas & Robin,
>
> Thanks for the feedback.
>
> On 15.07.2022 11:36, Nicolas Dufresne wrote:
> >Le vendredi 15 juillet 2022 à 12:04 +0100, Robin Murphy a écrit :
> >> On 2022-07-13 17:24, Sebastian Fricke wrote:
> >> > Implement the HEVC codec variation for the RkVDEC driver. Currently only
> >> > the RK3399 is supported, but it is possible to enable the RK3288 as it
> >> > also supports this codec.
> >> >
> >> > Based on top of the media tree @ef7fcbbb9eabbe86d2287484bf366dd1821cc6b8
> >> > and the HEVC uABI MR by Benjamin Gaignard.
> >> > (https://patchwork.linuxtv.org/project/linux-media/list/?series=8360)
> >> >
> >> > Tested with the GStreamer V4L2 HEVC plugin:
> >> > (https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1079)
> >> >
> >> > Current Fluster score:
> >> > `Ran 131/147 tests successfully in 278.568 secs`
> >> > with
> >> > `python3 fluster.py run -d GStreamer-H.265-V4L2SL-Gst1.0 -ts JCT-VC-HEVC_V1 -j1`
> >> >
> >> > failed conformance tests:
> >> > - DBLK_D_VIXS_2 (Success on Hantro G2)
> >> > - DSLICE_A_HHI_5 (Success on Hantro G2)
> >> > - EXT_A_ericsson_4 (Success on Hantro G2)
> >> > - PICSIZE_A_Bossen_1 (Hardware limitation)
> >> > - PICSIZE_B_Bossen_1 (Hardware limitation)
> >> > - PICSIZE_C_Bossen_1 (Hardware limitation)
> >> > - PICSIZE_D_Bossen_1 (Hardware limitation)
> >> > - PPS_A_qualcomm_7 (Success on Hantro G2)
> >> > - SAODBLK_A_MainConcept_4 (Success on Hantro G2)
> >> > - SAODBLK_B_MainConcept_4 (Success on Hantro G2)
> >> > - SLIST_B_Sony_9 (Success on Hantro G2)
> >> > - SLIST_D_Sony_9 (Success on Hantro G2)
> >> > - TSUNEQBD_A_MAIN10_Technicolor_2 (Success on Hantro G2)
> >> > - VPSSPSPPS_A_MainConcept_1 (Success on Hantro G2)
> >> > - WPP_D_ericsson_MAIN10_2 (Fail on Hantro G2)
> >> > - WPP_D_ericsson_MAIN_2 (Fail on Hantro G2)
> >> >
> >> > Not tested with FFMpeg so far.
> >> >
> >> > Known issues:
> >> > - Unable to reliably decode multiple videos concurrently
> >> > - The SAODBLK_* tests timeout if the timeout time in fluster is lower than 120
> >> > - Currently the uv_virstride is calculated in a manner that is hardcoded
> >> > for the two available formats NV12 and NV15. (@config_registers)
> >> >
> >> > Notable design decisions:
> >> > - I opted for a bitfield to represent the PPS memory blob as it is the
> >> > perfect tool for that job. It describes the memory layout with any
> >> > additional required documentation, is easy to read and a native language
> >> > tool for that job
> >>
> >> Can I point out how terrible an idea this is? The C language gives
> >> virtually zero guarantee about how bitfields are actually represented in
> >> memory. Platform ABIs (e.g. [1]) might nail things down a bit more, but
> >> different platforms are free to make completely different choices so
> >> portability still goes out the window. Even for a single platform,
> >> different compilers (or at worst even different version of one compiler)
> >> can still make incompatible choices e.g. WRT alignment of packed
> >> members. Even if you narrow the scope as far as a specific version of
> >> AArch64 GCC, I think this is still totally broken for big-endian.
> >>
> >> The fact that you've had to use nonsensical types to trick a compiler
> >> into meeting your expectations should already be a clue to how fragile
> >> this is in general.
> >>
> >> > - The RPS memory blob is created using a bitmap implementation, which
> >> > uses a common Kernel API to avoid reinventing the wheel and to keep the
> >> > code clean.
> >>
> >> Similarly, Linux bitmaps are designed for use as, well, bitmaps. Abusing
> >> them as a data interchange format for bit-aligned numerical values is
> >> far from "clean" semantically. And I'm pretty sure it's also broken for
> >> big-endian.
> >>
> >> This kind of stuff may be standard practice in embedded development
> >> where you're targeting a specific MCU with a specific toolchain, but I
> >> don't believe it's suitable for upstream Linux. It would take pretty
> >> much the same number of lines to use GENMASK definitions and bitfield.h
> >> helpers to pack values into words which can then be written to memory in
> >> a guaranteed format and endianness (certainly for the PPS; for the RPS
> >> it may well end up a bit longer, but would be self-documenting and
> >> certainly more readable than those loops). It mostly just means that for
> >> any field which crosses a word boundary you'll end up with 2 definitions
> >> and 2 assignments, which is hardly a problem (and in some ways more
> >> honest about what's actually going on).
> >
> >Thanks for the feedback, in multimedia (unlike register programming), we don't
> >really consider bitstreams as bitmap or bitfield. What we do really expect is to
> >use bit writer helpers (and sometimes a bit reader though we try and avoid the
> >second one in the kernel). Its more of less a cursor (a bit position) into a
> >memory that advance while writing. A bit writer should help protect against
> >overflow too.
> >
> >When writing lets say a chain of 8 bits from a char, a proper helper is expected
> >to be very explicit on the ordering (write_u8_le/be or something better worded).
> >I would rather like to see all these blobs written this way personally then
> >having a cleared buffer and writing using bit offsets.
> >
> >Perhaps I may suggest to start with implementing just that inside this driver?
> >It isn't very hard, and then the implementation can be reduced later and shared
> >later, with whatever exists without deviating from the intent of the existing
> >API ? I do believe that having this in linux-media can be useful in the future.
> >We will notably need to extend such a helper with multimedia specific coding
> >technique (golomb, boolean coding, etc.) for use in stateless encoder drivers.
>
> I currently design a general bit-writer API to handle the mentioned
> issues correctly. I'll post it as part of V2, due to my current workload
> this will happen in 3 weeks at the earliest.
>
I wonder if this is really the correct approach.
Introducing a new API and adding HEVC support at the same time,
sounds like scope creep to me.
How about you first introduce HEVC and then we move to the new API?
A generic bit-writer API might really take a long time to get mainlined.
Thanks!
Ezequiel
> >
> >Nicolas
> >
> >>
> >> Thanks,
> >> Robin.
>
> Greetings,
> Sebastian
>
> >>
> >> [1]
> >> https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#bit-fields
> >>
> >> > - I deliberatly opted against the macro solution used in H264, which
> >> > declares Macros in mid function and declares the fields of the memory
> >> > blob as macros as well. And I would be glad to refactor the H264 code if
> >> > desired by the maintainer to use common Kernel APIs and native language
> >> > elements.
> >> > - The giant static array of cabac values is moved to a separate c file,
> >> > I did so because a separate .h file would be incorrect as it doesn't
> >> > expose anything of any value for any other file than the rkvdec-hevc.c
> >> > file. Other options were:
> >> > - Calculating the values instead of storing the results (doesn't seem
> >> > to be worth it)
> >> > - Supply them via firmware (Adding firmware makes the whole software
> >> > way more complicated and the usage of the driver less obvious)
> >> >
> >> > Ignored Checkpatch warnings (as it fits to the current style of the file):
> >> > ```
> >> > WARNING: line length of 162 exceeds 100 columns
> >> > #115: FILE: drivers/media/v4l2-core/v4l2-common.c:265:
> >> > + { .format = V4L2_PIX_FMT_NV15, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 5, 0, 0 }, .hdiv = 2, .vdiv = 2,
> >> >
> >> > ERROR: trailing statements should be on next line
> >> > #128: FILE: drivers/media/v4l2-core/v4l2-ioctl.c:1305:
> >> > + case V4L2_PIX_FMT_NV15: descr = "10-bit Y/CbCr 4:2:0 (Packed)"; break;
> >> > ```
> >> >
> >> > v4l2-compliance test:
> >> > ```
> >> > Total for rkvdec device /dev/video3: 46, Succeeded: 46, Failed: 0, Warnings: 0
> >> > ```
> >> >
> >> > kselftest module run for the bitmap changes:
> >> > ```
> >> > $ sudo insmod /usr/lib/modules/5.19.0-rc3-finalseries/kernel/lib/test_bitmap.ko
> >> > [ 71.751716] test_bitmap: parselist: 14: input is '0-2047:128/256' OK, Time: 1750
> >> > [ 71.751787] test_bitmap: bitmap_print_to_pagebuf: input is '0-32767
> >> > [ 71.751787] ', Time: 6708
> >> > [ 71.760373] test_bitmap: set_value: 6/6 tests correct
> >> > ```
> >> >
> >> > Jonas Karlman (2):
> >> > media: v4l2: Add NV15 pixel format
> >> > media: v4l2-common: Add helpers to calculate bytesperline and
> >> > sizeimage
> >> >
> >> > Sebastian Fricke (4):
> >> > bitops: bitmap helper to set variable length values
> >> > staging: media: rkvdec: Add valid pixel format check
> >> > staging: media: rkvdec: Enable S_CTRL IOCTL
> >> > staging: media: rkvdec: Add HEVC backend
> >> >
> >> > .../media/v4l/pixfmt-yuv-planar.rst | 53 +
> >> > drivers/media/v4l2-core/v4l2-common.c | 79 +-
> >> > drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
> >> > drivers/staging/media/rkvdec/Makefile | 2 +-
> >> > drivers/staging/media/rkvdec/TODO | 22 +-
> >> > .../staging/media/rkvdec/rkvdec-hevc-data.c | 1844 +++++++++++++++++
> >> > drivers/staging/media/rkvdec/rkvdec-hevc.c | 859 ++++++++
> >> > drivers/staging/media/rkvdec/rkvdec-regs.h | 1 +
> >> > drivers/staging/media/rkvdec/rkvdec.c | 182 +-
> >> > drivers/staging/media/rkvdec/rkvdec.h | 3 +
> >> > include/linux/bitmap.h | 39 +
> >> > include/uapi/linux/videodev2.h | 1 +
> >> > lib/test_bitmap.c | 47 +
> >> > 13 files changed, 3066 insertions(+), 67 deletions(-)
> >> > create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc-data.c
> >> > create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc.c
> >> >
> >>
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/6] RkVDEC HEVC driver
2022-07-16 16:27 ` Alex Bee
@ 2022-07-21 16:52 ` Sebastian Fricke
0 siblings, 0 replies; 17+ messages in thread
From: Sebastian Fricke @ 2022-07-21 16:52 UTC (permalink / raw)
To: Alex Bee
Cc: linux-media, jernej.skrabec, kernel, bob.beckett, ezequiel,
mchehab, gregkh, linux-kernel, linux-rockchip, linux-staging,
nicolas.dufresne, Yury Norov, Andy Shevchenko, Rasmus Villemoes
Hey Alex,
Thanks for taking a look!
On 16.07.2022 18:27, Alex Bee wrote:
>Hi Sebastian,
>
>thanks a lot for your work on upstreaming this driver.
>
>See some general comments below.
>
>>Implement the HEVC codec variation for the RkVDEC driver. Currently only
>>the RK3399 is supported, but it is possible to enable the RK3288 as it
>>also supports this codec.
>>
>>Based on top of the media tree @ef7fcbbb9eabbe86d2287484bf366dd1821cc6b8
>>and the HEVC uABI MR by Benjamin Gaignard.
>>(https://patchwork.linuxtv.org/project/linux-media/list/?series=8360)
>>
>>Tested with the GStreamer V4L2 HEVC plugin:
>>(https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1079)
>>
>>Current Fluster score:
>>`Ran 131/147 tests successfully in 278.568 secs`
>>with
>>`python3 fluster.py run -d GStreamer-H.265-V4L2SL-Gst1.0 -ts JCT-VC-HEVC_V1 -j1`
>>
>>failed conformance tests:
>>- DBLK_D_VIXS_2 (Success on Hantro G2)
>>- DSLICE_A_HHI_5 (Success on Hantro G2)
>>- EXT_A_ericsson_4 (Success on Hantro G2)
>>- PICSIZE_A_Bossen_1 (Hardware limitation)
>>- PICSIZE_B_Bossen_1 (Hardware limitation)
>>- PICSIZE_C_Bossen_1 (Hardware limitation)
>>- PICSIZE_D_Bossen_1 (Hardware limitation)
>>- PPS_A_qualcomm_7 (Success on Hantro G2)
>>- SAODBLK_A_MainConcept_4 (Success on Hantro G2)
>>- SAODBLK_B_MainConcept_4 (Success on Hantro G2)
>>- SLIST_B_Sony_9 (Success on Hantro G2)
>>- SLIST_D_Sony_9 (Success on Hantro G2)
>>- TSUNEQBD_A_MAIN10_Technicolor_2 (Success on Hantro G2)
>>- VPSSPSPPS_A_MainConcept_1 (Success on Hantro G2)
>>- WPP_D_ericsson_MAIN10_2 (Fail on Hantro G2)
>>- WPP_D_ericsson_MAIN_2 (Fail on Hantro G2)
>>
>>Not tested with FFMpeg so far.
>>
>>Known issues:
>>- Unable to reliably decode multiple videos concurrently
>>- The SAODBLK_* tests timeout if the timeout time in fluster is lower than 120
>>- Currently the uv_virstride is calculated in a manner that is hardcoded
>>for the two available formats NV12 and NV15. (@config_registers)
>>
>>Notable design decisions:
>>- I opted for a bitfield to represent the PPS memory blob as it is the
>>perfect tool for that job. It describes the memory layout with any
>>additional required documentation, is easy to read and a native language
>>tool for that job
>>- The RPS memory blob is created using a bitmap implementation, which
>>uses a common Kernel API to avoid reinventing the wheel and to keep the
>>code clean.
>>- I deliberatly opted against the macro solution used in H264, which
>>declares Macros in mid function and declares the fields of the memory
>>blob as macros as well. And I would be glad to refactor the H264 code if
>>desired by the maintainer to use common Kernel APIs and native language
>>elements.
>
>I fully disagree here: That way the code is much less
>read-/understandable - your are putting bits at some random hardcoded
>positions with not relation to the codec/hardware and expect everyone
>to read and understand that huge docblock - the code should be more
>self-explaining and we should at least try to get rid of those
>hardcoded positions which, btw, will differ for newer versions of that
>hardware block.
So, I thought about this a bit:
My thoughts were going generally in two directions:
1. I create a general struct for an RPS layout and let the
different hardware blocks fill that struct accordingly during
initialization, this would enable to get rid of those hard coded
positions but would make the code a bit more complicated and it doesn't
seem worth it until we actually have a case where it is different (I
haven't test on RK3288 so far)
2. Implement a function for each hardware block and decide upon hardware
detection which function to use, this fits more to general kernel coding
style and to the manner rkvdec is coded. But this won't get rid of the
hardcoded positions and tbh the code before did have hard coded
positions as well and you needed to understand this code block as well:
```
#define REF_PIC_LONG_TERM_L0(i) PS_FIELD(i * 5, 1)
#define REF_PIC_IDX_L0(i) PS_FIELD(1 + (i * 5), 4)
#define REF_PIC_LONG_TERM_L1(i) PS_FIELD((i < 5 ? 75 : 132) + (i * 5), 1)
#define REF_PIC_IDX_L1(i) PS_FIELD((i < 4 ? 76 : 128) + (i * 5), 4)
```
And it least from my perspective this wasn't clean code either as it is
not obvious without understanding this bit in detail, how the RPS
structure looks like.
I'll try to make the code more self explaining for V2. (will need a bit
preparation for that as I have to rewrite my bit writer implementation
as it was requested to be usable for other purposes as well)
>
>I'm also not sure what makes you call that a "blob": It's
>configuration of the hardware which in that case isn't put in
>registers, but in memory.
That is just the term that I heard the most, so I adopted it, I can use
hardware configuration as well, but the important part for me is that
people understand me.
>
>>- The giant static array of cabac values is moved to a separate c file,
>>I did so because a separate .h file would be incorrect as it doesn't
>>expose anything of any value for any other file than the rkvdec-hevc.c
>>file. Other options were:
>> - Calculating the values instead of storing the results (doesn't seem
>> to be worth it)
>
>I'm not sure "not worth it" should be argument for not doing doing
>anything in general; especially not if it can explain the relation
>between the standard and this driver.
Thanks for the feedback, you are correct my explanation is a bit lazy
and I will look into calculating the values as an option more seriously.
>
>Looking at tables of ITU-T Rec. H.265 "9.3.2.2 Initialization process
>for context variables" and comparing to the first elements of that
>huge array: It should be doable.
I'll try it out and maybe already in include it in V2.
Thanks again for your feedback.
Greetings,
Sebastian
>
>> - Supply them via firmware (Adding firmware makes the whole software
>> way more complicated and the usage of the driver less obvious)
>>
>>Ignored Checkpatch warnings (as it fits to the current style of the file):
>>```
>>WARNING: line length of 162 exceeds 100 columns
>>#115: FILE: drivers/media/v4l2-core/v4l2-common.c:265:
>>+ { .format = V4L2_PIX_FMT_NV15, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 5, 0, 0 }, .hdiv = 2, .vdiv = 2,
>>
>>ERROR: trailing statements should be on next line
>>#128: FILE: drivers/media/v4l2-core/v4l2-ioctl.c:1305:
>>+ case V4L2_PIX_FMT_NV15: descr = "10-bit Y/CbCr 4:2:0 (Packed)"; break;
>>```
>>
>>v4l2-compliance test:
>>```
>>Total for rkvdec device /dev/video3: 46, Succeeded: 46, Failed: 0, Warnings: 0
>>```
>>
>>kselftest module run for the bitmap changes:
>>```
>>$ sudo insmod /usr/lib/modules/5.19.0-rc3-finalseries/kernel/lib/test_bitmap.ko
>>[ 71.751716] test_bitmap: parselist: 14: input is '0-2047:128/256' OK, Time: 1750
>>[ 71.751787] test_bitmap: bitmap_print_to_pagebuf: input is '0-32767
>>[ 71.751787] ', Time: 6708
>>[ 71.760373] test_bitmap: set_value: 6/6 tests correct
>>```
>>
>>Jonas Karlman (2):
>> media: v4l2: Add NV15 pixel format
>> media: v4l2-common: Add helpers to calculate bytesperline and
>> sizeimage
>>
>>Sebastian Fricke (4):
>> bitops: bitmap helper to set variable length values
>> staging: media: rkvdec: Add valid pixel format check
>> staging: media: rkvdec: Enable S_CTRL IOCTL
>> staging: media: rkvdec: Add HEVC backend
>>
>> .../media/v4l/pixfmt-yuv-planar.rst | 53 +
>> drivers/media/v4l2-core/v4l2-common.c | 79 +-
>> drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
>> drivers/staging/media/rkvdec/Makefile | 2 +-
>> drivers/staging/media/rkvdec/TODO | 22 +-
>> .../staging/media/rkvdec/rkvdec-hevc-data.c | 1844 +++++++++++++++++
>> drivers/staging/media/rkvdec/rkvdec-hevc.c | 859 ++++++++
>> drivers/staging/media/rkvdec/rkvdec-regs.h | 1 +
>> drivers/staging/media/rkvdec/rkvdec.c | 182 +-
>> drivers/staging/media/rkvdec/rkvdec.h | 3 +
>> include/linux/bitmap.h | 39 +
>> include/uapi/linux/videodev2.h | 1 +
>> lib/test_bitmap.c | 47 +
>> 13 files changed, 3066 insertions(+), 67 deletions(-)
>> create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc-data.c
>> create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc.c
>>
>Regards,
>
>Alex
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/6] media: v4l2: Add NV15 pixel format
2022-07-15 0:04 ` Laurent Pinchart
@ 2022-07-21 16:58 ` Sebastian Fricke
0 siblings, 0 replies; 17+ messages in thread
From: Sebastian Fricke @ 2022-07-21 16:58 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Nicolas Dufresne, linux-media, jernej.skrabec, knaerzche, kernel,
bob.beckett, ezequiel, mchehab, gregkh, linux-kernel,
linux-rockchip, linux-staging, Jonas Karlman, Yury Norov,
Andy Shevchenko, Rasmus Villemoes, Hans Verkuil,
Benjamin Gaignard, Ming Qian, Sakari Ailus, Ricardo Ribalda,
Andrzej Pietrasiewicz, Sergey Senozhatsky
Hey Laurent and Nicolas,
On 15.07.2022 03:04, Laurent Pinchart wrote:
>On Thu, Jul 14, 2022 at 09:02:38AM -0400, Nicolas Dufresne wrote:
>> Le mercredi 13 juillet 2022 à 21:28 +0300, Laurent Pinchart a écrit :
>> > Hi Sebastian and Jonas,
>> >
>> > Thank you for the patch.
Thank you for your review :).
>> >
>> > On Wed, Jul 13, 2022 at 06:24:46PM +0200, Sebastian Fricke wrote:
>> > > From: Jonas Karlman <jonas@kwiboo.se>
>> > >
>> > > Add the NV15 pixel format used by the Rockchip Video Decoder for 10-bit
>> > > buffers.
>> > >
>> > > NV15 is a packed 10-bit 4:2:0 Y/CbCr format similar to P010 and P210 but
>> > > has no padding between components. Instead, luminance and chrominance
>> > > samples are grouped into 4s so that each group is packed into an integer
>> > > number of bytes:
>> > >
>> > > YYYY = UVUV = 4 * 10 bits = 40 bits = 5 bytes
>> > >
>> > > The '15' 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.
>> > >
>> > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> > > Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
>> > > ---
>> > > .../media/v4l/pixfmt-yuv-planar.rst | 53 +++++++++++++++++++
>> > > drivers/media/v4l2-core/v4l2-common.c | 2 +
>> > > drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
>> > > include/uapi/linux/videodev2.h | 1 +
>> > > 4 files changed, 57 insertions(+)
>> > >
>> > > diff --git a/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst b/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
>> > > index a900ff66911a..42ab3fe4667f 100644
>> > > --- a/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
>> > > +++ b/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
>> > > @@ -79,6 +79,13 @@ All components are stored with the same number of bits per component.
>> > > - Cr, Cb
>> > > - Yes
>> > > - Linear
>> > > + * - V4L2_PIX_FMT_NV15
>> > > + - 'NV15'
>> > > + - 15
>> > > + - 4:2:0
>> > > + - Cb, Cr
>> > > + - Yes
>> > > + - Linear
>> > > * - V4L2_PIX_FMT_NV12M
>> > > - 'NM12'
>> > > - 8
>> > > @@ -176,6 +183,7 @@ horizontally.
>> > >
>> > > .. _V4L2-PIX-FMT-NV12:
>> > > .. _V4L2-PIX-FMT-NV21:
>> > > +.. _V4L2-PIX-FMT-NV15:
>> > > .. _V4L2-PIX-FMT-NV12M:
>> > > .. _V4L2-PIX-FMT-NV21M:
>> > > .. _V4L2-PIX-FMT-P010:
>> > > @@ -570,6 +578,51 @@ Data in the 10 high bits, zeros in the 6 low bits, arranged in little endian ord
>> > > - Cb\ :sub:`11`
>> > > - Cr\ :sub:`11`
>> > >
>> > > +.. _V4L2_PIX_FMT_NV15:
>> > > +
>> > > +NV15
>> > > +----
>> > > +
>> > > +Like P010 but a packed 10-bit 4:2:0 semi-planar Y/CbCr format without padding between components.
>> >
>> > "packed 10-bit semi-planar" sounds confusing, as "packed YUV" usually
>> > refers to YUYV-style formats, but I'm not sure how to express that
>> > better.
>>
>> Perhaps:
>>
>> "Similar to P010 (10-bit 4:":0 semi-planer Y/CbCr), though unlike P010, the
>> there is not padding between components."
I'll take that for V2.
>>
>> > > +Instead, luminance and chrominance samples are grouped into 4s so that each group is packed into an integer
>> > > +number of bytes:
>> >
>> > Could you please wrap the description at 80 columns ?
Will do.
>> >
>> > > +YYYY = UVUV = 4 * 10 bits = 40 bits = 5 bytes
>> > > +
>> > > +.. flat-table:: Sample 4x4 NV15 Image
>> > > + :header-rows: 0
>> > > + :stub-columns: 0
>> > > +
>> > > + * - start + 0:
>> > > + - Y'\ :sub:`00`
>> > > + - Y'\ :sub:`01`
>> > > + - Y'\ :sub:`02`
>> > > + - Y'\ :sub:`03`
>> > > + * - start + 8:
>> > > + - Y'\ :sub:`10`
>> > > + - Y'\ :sub:`11`
>> > > + - Y'\ :sub:`12`
>> > > + - Y'\ :sub:`13`
>> > > + * - start + 16:
>> > > + - Y'\ :sub:`20`
>> > > + - Y'\ :sub:`21`
>> > > + - Y'\ :sub:`22`
>> > > + - Y'\ :sub:`23`
>> > > + * - start + 24:
>> > > + - Y'\ :sub:`30`
>> > > + - Y'\ :sub:`31`
>> > > + - Y'\ :sub:`32`
>> > > + - Y'\ :sub:`33`
>> > > + * - start + 32:
>> > > + - Cb\ :sub:`00`
>> > > + - Cr\ :sub:`00`
>> > > + - Cb\ :sub:`01`
>> > > + - Cr\ :sub:`01`
>> > > + * - start + 40:
>> > > + - Cb\ :sub:`10`
>> > > + - Cr\ :sub:`10`
>> > > + - Cb\ :sub:`11`
>> > > + - Cr\ :sub:`11`
>> >
>> > This doesn't look right. You need to describe the data at the bit level,
>> > so show how the 10-bit samples are packed into bytes.
>>
>> A word of NV15 is 40 bits, so 1 word of NV12 is 5 bytes, 4 pixels. I believe
>> there is no choice here but to describe 4 pixels for Y plane, and 4 pixels for
>> CbCr plane. This might be a bit big though.
>
>I agree, it may not be the prettiest, but I'd rather have a larger table
>than a format that is understood differently by different drivers. Been
>there, done that, not again :-)
I'll prepare something for V2 (at my current workload in about 3 weeks
at the earliest).
Greetings,
Sebastian
>
>> > > +
>> > > .. raw:: latex
>> > >
>> > > \endgroup
>> > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
>> > > index 1e38ad8906a2..23a0cb02ea3a 100644
>> > > --- a/drivers/media/v4l2-core/v4l2-common.c
>> > > +++ b/drivers/media/v4l2-core/v4l2-common.c
>> > > @@ -262,6 +262,8 @@ 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 }, .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 }, .hdiv = 2, .vdiv = 2 },
>> > > + { .format = V4L2_PIX_FMT_NV15, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 5, 0, 0 }, .hdiv = 2, .vdiv = 2,
>> > > + .block_w = { 4, 2, 0, 0 }, .block_h = { 1, 1, 0, 0 } },
>> > > { .format = V4L2_PIX_FMT_NV16, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .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 }, .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 }, .hdiv = 1, .vdiv = 1 },
>> > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>> > > index e2526701294e..9e5510cb255e 100644
>> > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> > > @@ -1302,6 +1302,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
>> > > case V4L2_PIX_FMT_M420: descr = "YUV 4:2:0 (M420)"; break;
>> > > case V4L2_PIX_FMT_NV12: descr = "Y/CbCr 4:2:0"; break;
>> > > case V4L2_PIX_FMT_NV21: descr = "Y/CrCb 4:2:0"; break;
>> > > + case V4L2_PIX_FMT_NV15: descr = "10-bit Y/CbCr 4:2:0 (Packed)"; break;
>> > > case V4L2_PIX_FMT_NV16: descr = "Y/CbCr 4:2:2"; break;
>> > > case V4L2_PIX_FMT_NV61: descr = "Y/CrCb 4:2:2"; break;
>> > > case V4L2_PIX_FMT_NV24: descr = "Y/CbCr 4:4:4"; break;
>> > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> > > index 5a73b92ffe4d..47ff34d6b79f 100644
>> > > --- a/include/uapi/linux/videodev2.h
>> > > +++ b/include/uapi/linux/videodev2.h
>> > > @@ -598,6 +598,7 @@ 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_NV24 v4l2_fourcc('N', 'V', '2', '4') /* 24 Y/CbCr 4:4:4 */
>
>--
>Regards,
>
>Laurent Pinchart
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/6] RkVDEC HEVC driver
2022-07-21 16:18 ` Ezequiel Garcia
@ 2022-07-21 20:11 ` Sebastian Fricke
0 siblings, 0 replies; 17+ messages in thread
From: Sebastian Fricke @ 2022-07-21 20:11 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: Nicolas Dufresne, Robin Murphy, linux-media, Jernej Škrabec,
Alex Bee, Collabora Kernel ML, Robert Beckett,
Mauro Carvalho Chehab, Greg KH, Linux Kernel Mailing List,
open list:ARM/Rockchip SoC...,
open list:STAGING SUBSYSTEM, Yury Norov, Andy Shevchenko,
Rasmus Villemoes
Hey Ezequiel,
On 21.07.2022 13:18, Ezequiel Garcia wrote:
>On Thu, Jul 21, 2022 at 1:16 PM Sebastian Fricke
><sebastian.fricke@collabora.com> wrote:
>>
>> Hey Nicolas & Robin,
>>
>> Thanks for the feedback.
>>
>> On 15.07.2022 11:36, Nicolas Dufresne wrote:
>> >Le vendredi 15 juillet 2022 à 12:04 +0100, Robin Murphy a écrit :
>> >> On 2022-07-13 17:24, Sebastian Fricke wrote:
>> >> > Implement the HEVC codec variation for the RkVDEC driver. Currently only
>> >> > the RK3399 is supported, but it is possible to enable the RK3288 as it
>> >> > also supports this codec.
>> >> >
>> >> > Based on top of the media tree @ef7fcbbb9eabbe86d2287484bf366dd1821cc6b8
>> >> > and the HEVC uABI MR by Benjamin Gaignard.
>> >> > (https://patchwork.linuxtv.org/project/linux-media/list/?series=8360)
>> >> >
>> >> > Tested with the GStreamer V4L2 HEVC plugin:
>> >> > (https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1079)
>> >> >
>> >> > Current Fluster score:
>> >> > `Ran 131/147 tests successfully in 278.568 secs`
>> >> > with
>> >> > `python3 fluster.py run -d GStreamer-H.265-V4L2SL-Gst1.0 -ts JCT-VC-HEVC_V1 -j1`
>> >> >
>> >> > failed conformance tests:
>> >> > - DBLK_D_VIXS_2 (Success on Hantro G2)
>> >> > - DSLICE_A_HHI_5 (Success on Hantro G2)
>> >> > - EXT_A_ericsson_4 (Success on Hantro G2)
>> >> > - PICSIZE_A_Bossen_1 (Hardware limitation)
>> >> > - PICSIZE_B_Bossen_1 (Hardware limitation)
>> >> > - PICSIZE_C_Bossen_1 (Hardware limitation)
>> >> > - PICSIZE_D_Bossen_1 (Hardware limitation)
>> >> > - PPS_A_qualcomm_7 (Success on Hantro G2)
>> >> > - SAODBLK_A_MainConcept_4 (Success on Hantro G2)
>> >> > - SAODBLK_B_MainConcept_4 (Success on Hantro G2)
>> >> > - SLIST_B_Sony_9 (Success on Hantro G2)
>> >> > - SLIST_D_Sony_9 (Success on Hantro G2)
>> >> > - TSUNEQBD_A_MAIN10_Technicolor_2 (Success on Hantro G2)
>> >> > - VPSSPSPPS_A_MainConcept_1 (Success on Hantro G2)
>> >> > - WPP_D_ericsson_MAIN10_2 (Fail on Hantro G2)
>> >> > - WPP_D_ericsson_MAIN_2 (Fail on Hantro G2)
>> >> >
>> >> > Not tested with FFMpeg so far.
>> >> >
>> >> > Known issues:
>> >> > - Unable to reliably decode multiple videos concurrently
>> >> > - The SAODBLK_* tests timeout if the timeout time in fluster is lower than 120
>> >> > - Currently the uv_virstride is calculated in a manner that is hardcoded
>> >> > for the two available formats NV12 and NV15. (@config_registers)
>> >> >
>> >> > Notable design decisions:
>> >> > - I opted for a bitfield to represent the PPS memory blob as it is the
>> >> > perfect tool for that job. It describes the memory layout with any
>> >> > additional required documentation, is easy to read and a native language
>> >> > tool for that job
>> >>
>> >> Can I point out how terrible an idea this is? The C language gives
>> >> virtually zero guarantee about how bitfields are actually represented in
>> >> memory. Platform ABIs (e.g. [1]) might nail things down a bit more, but
>> >> different platforms are free to make completely different choices so
>> >> portability still goes out the window. Even for a single platform,
>> >> different compilers (or at worst even different version of one compiler)
>> >> can still make incompatible choices e.g. WRT alignment of packed
>> >> members. Even if you narrow the scope as far as a specific version of
>> >> AArch64 GCC, I think this is still totally broken for big-endian.
>> >>
>> >> The fact that you've had to use nonsensical types to trick a compiler
>> >> into meeting your expectations should already be a clue to how fragile
>> >> this is in general.
>> >>
>> >> > - The RPS memory blob is created using a bitmap implementation, which
>> >> > uses a common Kernel API to avoid reinventing the wheel and to keep the
>> >> > code clean.
>> >>
>> >> Similarly, Linux bitmaps are designed for use as, well, bitmaps. Abusing
>> >> them as a data interchange format for bit-aligned numerical values is
>> >> far from "clean" semantically. And I'm pretty sure it's also broken for
>> >> big-endian.
>> >>
>> >> This kind of stuff may be standard practice in embedded development
>> >> where you're targeting a specific MCU with a specific toolchain, but I
>> >> don't believe it's suitable for upstream Linux. It would take pretty
>> >> much the same number of lines to use GENMASK definitions and bitfield.h
>> >> helpers to pack values into words which can then be written to memory in
>> >> a guaranteed format and endianness (certainly for the PPS; for the RPS
>> >> it may well end up a bit longer, but would be self-documenting and
>> >> certainly more readable than those loops). It mostly just means that for
>> >> any field which crosses a word boundary you'll end up with 2 definitions
>> >> and 2 assignments, which is hardly a problem (and in some ways more
>> >> honest about what's actually going on).
>> >
>> >Thanks for the feedback, in multimedia (unlike register programming), we don't
>> >really consider bitstreams as bitmap or bitfield. What we do really expect is to
>> >use bit writer helpers (and sometimes a bit reader though we try and avoid the
>> >second one in the kernel). Its more of less a cursor (a bit position) into a
>> >memory that advance while writing. A bit writer should help protect against
>> >overflow too.
>> >
>> >When writing lets say a chain of 8 bits from a char, a proper helper is expected
>> >to be very explicit on the ordering (write_u8_le/be or something better worded).
>> >I would rather like to see all these blobs written this way personally then
>> >having a cleared buffer and writing using bit offsets.
>> >
>> >Perhaps I may suggest to start with implementing just that inside this driver?
>> >It isn't very hard, and then the implementation can be reduced later and shared
>> >later, with whatever exists without deviating from the intent of the existing
>> >API ? I do believe that having this in linux-media can be useful in the future.
>> >We will notably need to extend such a helper with multimedia specific coding
>> >technique (golomb, boolean coding, etc.) for use in stateless encoder drivers.
>>
>> I currently design a general bit-writer API to handle the mentioned
>> issues correctly. I'll post it as part of V2, due to my current workload
>> this will happen in 3 weeks at the earliest.
>>
>
>I wonder if this is really the correct approach.
>
>Introducing a new API and adding HEVC support at the same time,
>sounds like scope creep to me.
>
>How about you first introduce HEVC and then we move to the new API?
>A generic bit-writer API might really take a long time to get mainlined.
I'll do it that route then, I'll revert to the RPS & PPS handling as
found in the rkvdec-h264 codec variant and will post a 2nd series to
introduce a new generic bit-writer API and change the bit writing in all
applicable codec drivers.
>
>Thanks!
>Ezequiel
Greetings,
Sebastian
>
>> >
>> >Nicolas
>> >
>> >>
>> >> Thanks,
>> >> Robin.
>>
>> Greetings,
>> Sebastian
>>
>> >>
>> >> [1]
>> >> https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#bit-fields
>> >>
>> >> > - I deliberatly opted against the macro solution used in H264, which
>> >> > declares Macros in mid function and declares the fields of the memory
>> >> > blob as macros as well. And I would be glad to refactor the H264 code if
>> >> > desired by the maintainer to use common Kernel APIs and native language
>> >> > elements.
>> >> > - The giant static array of cabac values is moved to a separate c file,
>> >> > I did so because a separate .h file would be incorrect as it doesn't
>> >> > expose anything of any value for any other file than the rkvdec-hevc.c
>> >> > file. Other options were:
>> >> > - Calculating the values instead of storing the results (doesn't seem
>> >> > to be worth it)
>> >> > - Supply them via firmware (Adding firmware makes the whole software
>> >> > way more complicated and the usage of the driver less obvious)
>> >> >
>> >> > Ignored Checkpatch warnings (as it fits to the current style of the file):
>> >> > ```
>> >> > WARNING: line length of 162 exceeds 100 columns
>> >> > #115: FILE: drivers/media/v4l2-core/v4l2-common.c:265:
>> >> > + { .format = V4L2_PIX_FMT_NV15, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 5, 0, 0 }, .hdiv = 2, .vdiv = 2,
>> >> >
>> >> > ERROR: trailing statements should be on next line
>> >> > #128: FILE: drivers/media/v4l2-core/v4l2-ioctl.c:1305:
>> >> > + case V4L2_PIX_FMT_NV15: descr = "10-bit Y/CbCr 4:2:0 (Packed)"; break;
>> >> > ```
>> >> >
>> >> > v4l2-compliance test:
>> >> > ```
>> >> > Total for rkvdec device /dev/video3: 46, Succeeded: 46, Failed: 0, Warnings: 0
>> >> > ```
>> >> >
>> >> > kselftest module run for the bitmap changes:
>> >> > ```
>> >> > $ sudo insmod /usr/lib/modules/5.19.0-rc3-finalseries/kernel/lib/test_bitmap.ko
>> >> > [ 71.751716] test_bitmap: parselist: 14: input is '0-2047:128/256' OK, Time: 1750
>> >> > [ 71.751787] test_bitmap: bitmap_print_to_pagebuf: input is '0-32767
>> >> > [ 71.751787] ', Time: 6708
>> >> > [ 71.760373] test_bitmap: set_value: 6/6 tests correct
>> >> > ```
>> >> >
>> >> > Jonas Karlman (2):
>> >> > media: v4l2: Add NV15 pixel format
>> >> > media: v4l2-common: Add helpers to calculate bytesperline and
>> >> > sizeimage
>> >> >
>> >> > Sebastian Fricke (4):
>> >> > bitops: bitmap helper to set variable length values
>> >> > staging: media: rkvdec: Add valid pixel format check
>> >> > staging: media: rkvdec: Enable S_CTRL IOCTL
>> >> > staging: media: rkvdec: Add HEVC backend
>> >> >
>> >> > .../media/v4l/pixfmt-yuv-planar.rst | 53 +
>> >> > drivers/media/v4l2-core/v4l2-common.c | 79 +-
>> >> > drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
>> >> > drivers/staging/media/rkvdec/Makefile | 2 +-
>> >> > drivers/staging/media/rkvdec/TODO | 22 +-
>> >> > .../staging/media/rkvdec/rkvdec-hevc-data.c | 1844 +++++++++++++++++
>> >> > drivers/staging/media/rkvdec/rkvdec-hevc.c | 859 ++++++++
>> >> > drivers/staging/media/rkvdec/rkvdec-regs.h | 1 +
>> >> > drivers/staging/media/rkvdec/rkvdec.c | 182 +-
>> >> > drivers/staging/media/rkvdec/rkvdec.h | 3 +
>> >> > include/linux/bitmap.h | 39 +
>> >> > include/uapi/linux/videodev2.h | 1 +
>> >> > lib/test_bitmap.c | 47 +
>> >> > 13 files changed, 3066 insertions(+), 67 deletions(-)
>> >> > create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc-data.c
>> >> > create mode 100644 drivers/staging/media/rkvdec/rkvdec-hevc.c
>> >> >
>> >>
>> >
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-07-21 20:11 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-13 16:24 [PATCH 0/6] RkVDEC HEVC driver Sebastian Fricke
2022-07-13 16:24 ` [PATCH 1/6] media: v4l2: Add NV15 pixel format Sebastian Fricke
2022-07-13 18:28 ` Laurent Pinchart
2022-07-14 13:02 ` Nicolas Dufresne
2022-07-15 0:04 ` Laurent Pinchart
2022-07-21 16:58 ` Sebastian Fricke
2022-07-13 16:24 ` [PATCH 2/6] media: v4l2-common: Add helpers to calculate bytesperline and sizeimage Sebastian Fricke
2022-07-13 16:47 ` [PATCH 0/6] RkVDEC HEVC driver Sebastian Fricke
2022-07-15 11:04 ` Robin Murphy
2022-07-15 15:36 ` Nicolas Dufresne
2022-07-16 6:45 ` Jernej Škrabec
2022-07-18 14:54 ` Nicolas Dufresne
2022-07-21 16:16 ` Sebastian Fricke
2022-07-21 16:18 ` Ezequiel Garcia
2022-07-21 20:11 ` Sebastian Fricke
2022-07-16 16:27 ` Alex Bee
2022-07-21 16:52 ` Sebastian Fricke
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).