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

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

[Jonas Karlman]

This is a revival of a 3 year old series [1] now that NV15/NV20/NV30 support
for display driver have landed in mainline tree.

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

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

Patch 3 change to use bytesperline and buffer height to configure strides.
Patch 4 change to use values from SPS/PPS control to configure the HW.
Patch 5 remove an unnecessary call to validate sps at streaming start.

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

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

Tested on a ROCK Pi 4 (RK3399) and Rock64 (RK3328):

  v4l2-compliance 1.24.1, 64 bits, 64-bit time_t
  ...
  Total for rkvdec device /dev/video1: 46, Succeeded: 46, Failed: 0, Warnings: 0

  Running test suite JVT-FR-EXT with decoder FFmpeg-H.264-V4L2-request
  ...
  Ran 65/69 tests successfully

  Running test suite JVT-AVC_V1 with decoder FFmpeg-H.264-V4L2-request
  ...
  Ran 127/135 tests successfully

Before this series:

  Running test suite JVT-FR-EXT with decoder FFmpeg-H.264-V4L2-request
  ...
  Ran 44/69 tests successfully

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

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

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

Following commits adds support for NV15/NV20/NV30 to VOP driver:
728c15b4b5f3 ("drm/fourcc: Add NV20 and NV30 YUV formats")
d4b384228562 ("drm/rockchip: vop: Add NV15, NV20 and NV30 support")

To fully runtime test this series you may need above drm commits and ffmpeg
patches from [2], this series and drm patches is also available at [3].

[1] https://lore.kernel.org/linux-media/20200706215430.22859-1-jonas@kwiboo.se/
[2] https://github.com/Kwiboo/FFmpeg/commits/v4l2-request-n6.1-dev/
[3] https://github.com/Kwiboo/linux-rockchip/commits/linuxtv-rkvdec-high-10-v4/
[4] https://gist.github.com/Kwiboo/f4ac15576b2c72887ae2bc5d58b5c865
[5] https://gist.github.com/Kwiboo/459a1c8f1dcb56e45dc7a7a29cc28adf

Regards,
Jonas

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

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

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

-- 
2.42.0

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

[Jonas Karlman]

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

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v4:
- No change

v3:
- Consider bpp_div in calculation

 drivers/media/v4l2-core/v4l2-common.c | 78 +++++++++++++--------------
 1 file changed, 39 insertions(+), 39 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 3a4b15a98e02..834b426da8b1 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -350,6 +350,34 @@ static inline unsigned int v4l2_format_block_height(const struct v4l2_format_inf
 	return info->block_h[plane];
 }
 
+static inline unsigned int v4l2_format_plane_width(const struct v4l2_format_info *info, int plane,
+						   unsigned int width)
+{
+	unsigned int hdiv = plane ? info->hdiv : 1;
+	unsigned int aligned_width =
+		ALIGN(width, v4l2_format_block_width(info, plane));
+
+	return DIV_ROUND_UP(aligned_width, hdiv) *
+	       info->bpp[plane] / info->bpp_div[plane];
+}
+
+static inline unsigned int v4l2_format_plane_height(const struct v4l2_format_info *info, int plane,
+						    unsigned int height)
+{
+	unsigned int vdiv = plane ? info->vdiv : 1;
+	unsigned int aligned_height =
+		ALIGN(height, v4l2_format_block_height(info, plane));
+
+	return DIV_ROUND_UP(aligned_height, vdiv);
+}
+
+static inline unsigned int v4l2_format_plane_size(const struct v4l2_format_info *info, int plane,
+						  unsigned int width, unsigned int height)
+{
+	return v4l2_format_plane_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)
 {
@@ -385,37 +413,19 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 
 	if (info->mem_planes == 1) {
 		plane = &pixfmt->plane_fmt[0];
-		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0] / info->bpp_div[0];
+		plane->bytesperline = v4l2_format_plane_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) / info->bpp_div[i];
-		}
+		for (i = 0; i < info->comp_planes; i++)
+			plane->sizeimage +=
+				v4l2_format_plane_size(info, i, width, height);
 	} else {
 		for (i = 0; i < info->comp_planes; i++) {
-			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
-			unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
-			unsigned int aligned_width;
-			unsigned int aligned_height;
-
-			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
-			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
-
 			plane = &pixfmt->plane_fmt[i];
 			plane->bytesperline =
-				info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv) / info->bpp_div[i];
-			plane->sizeimage =
-				plane->bytesperline * DIV_ROUND_UP(aligned_height, vdiv);
+				v4l2_format_plane_width(info, i, width);
+			plane->sizeimage = plane->bytesperline *
+				v4l2_format_plane_height(info, i, height);
 		}
 	}
 	return 0;
@@ -439,22 +449,12 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
 	pixfmt->width = width;
 	pixfmt->height = height;
 	pixfmt->pixelformat = pixelformat;
-	pixfmt->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0] / info->bpp_div[0];
+	pixfmt->bytesperline = v4l2_format_plane_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) / info->bpp_div[i];
-	}
+	for (i = 0; i < info->comp_planes; i++)
+		pixfmt->sizeimage +=
+			v4l2_format_plane_size(info, i, width, height);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
-- 
2.42.0

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

[Nicolas Dufresne]

Le dimanche 05 novembre 2023 à 16:55 +0000, Jonas Karlman a écrit :
> Add helper functions to calculate plane bytesperline and sizeimage,
> these new helpers consider bpp div, block width and height when
> calculating plane bytesperline and sizeimage.

Is this only refactoring to reduce duplicated code ? I haven't seen
what is new in there yet, maybe the commit message could clarify.

regards,
Nicolas

> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
> v4:
> - No change
> 
> v3:
> - Consider bpp_div in calculation
> 
>  drivers/media/v4l2-core/v4l2-common.c | 78 +++++++++++++--------------
>  1 file changed, 39 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index 3a4b15a98e02..834b426da8b1 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -350,6 +350,34 @@ static inline unsigned int v4l2_format_block_height(const struct v4l2_format_inf
>  	return info->block_h[plane];
>  }
>  
> +static inline unsigned int v4l2_format_plane_width(const struct v4l2_format_info *info, int plane,
> +						   unsigned int width)
> +{
> +	unsigned int hdiv = plane ? info->hdiv : 1;
> +	unsigned int aligned_width =
> +		ALIGN(width, v4l2_format_block_width(info, plane));
> +
> +	return DIV_ROUND_UP(aligned_width, hdiv) *
> +	       info->bpp[plane] / info->bpp_div[plane];
> +}
> +
> +static inline unsigned int v4l2_format_plane_height(const struct v4l2_format_info *info, int plane,
> +						    unsigned int height)
> +{
> +	unsigned int vdiv = plane ? info->vdiv : 1;
> +	unsigned int aligned_height =
> +		ALIGN(height, v4l2_format_block_height(info, plane));
> +
> +	return DIV_ROUND_UP(aligned_height, vdiv);
> +}
> +
> +static inline unsigned int v4l2_format_plane_size(const struct v4l2_format_info *info, int plane,
> +						  unsigned int width, unsigned int height)
> +{
> +	return v4l2_format_plane_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)
>  {
> @@ -385,37 +413,19 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
>  
>  	if (info->mem_planes == 1) {
>  		plane = &pixfmt->plane_fmt[0];
> -		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0] / info->bpp_div[0];
> +		plane->bytesperline = v4l2_format_plane_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) / info->bpp_div[i];
> -		}
> +		for (i = 0; i < info->comp_planes; i++)
> +			plane->sizeimage +=
> +				v4l2_format_plane_size(info, i, width, height);
>  	} else {
>  		for (i = 0; i < info->comp_planes; i++) {
> -			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
> -			unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
> -			unsigned int aligned_width;
> -			unsigned int aligned_height;
> -
> -			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
> -			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
> -
>  			plane = &pixfmt->plane_fmt[i];
>  			plane->bytesperline =
> -				info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv) / info->bpp_div[i];
> -			plane->sizeimage =
> -				plane->bytesperline * DIV_ROUND_UP(aligned_height, vdiv);
> +				v4l2_format_plane_width(info, i, width);
> +			plane->sizeimage = plane->bytesperline *
> +				v4l2_format_plane_height(info, i, height);
>  		}
>  	}
>  	return 0;
> @@ -439,22 +449,12 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
>  	pixfmt->width = width;
>  	pixfmt->height = height;
>  	pixfmt->pixelformat = pixelformat;
> -	pixfmt->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0] / info->bpp_div[0];
> +	pixfmt->bytesperline = v4l2_format_plane_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) / info->bpp_div[i];
> -	}
> +	for (i = 0; i < info->comp_planes; i++)
> +		pixfmt->sizeimage +=
> +			v4l2_format_plane_size(info, i, width, height);
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);

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

[Jonas Karlman]

On 2023-11-08 03:45, Nicolas Dufresne wrote:
> Le dimanche 05 novembre 2023 à 16:55 +0000, Jonas Karlman a écrit :
>> Add helper functions to calculate plane bytesperline and sizeimage,
>> these new helpers consider bpp div, block width and height when
>> calculating plane bytesperline and sizeimage.
> 
> Is this only refactoring to reduce duplicated code ? I haven't seen
> what is new in there yet, maybe the commit message could clarify.

Yes, this is just some refactoring and is not really required.

Cannot remember exactly why I though this was needed back in v1/v2. I
probably had an issue with wrong bytesperline/sizeimage due to the
fractal bytes per pixel nature of the NV15/NV20 pixel formats. Hacked
the calculation and forgot to update in one of the 3 places they get
calculated, then decided to refactor after that failed.

Will update commit message to clarify that this is only refactoring and
has no real impact for this series in v5.

Regards,
Jonas

> 
> regards,
> Nicolas
> 
>>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>> v4:
>> - No change
>>
>> v3:
>> - Consider bpp_div in calculation
>>
>>  drivers/media/v4l2-core/v4l2-common.c | 78 +++++++++++++--------------
>>  1 file changed, 39 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
>> index 3a4b15a98e02..834b426da8b1 100644
>> --- a/drivers/media/v4l2-core/v4l2-common.c
>> +++ b/drivers/media/v4l2-core/v4l2-common.c
>> @@ -350,6 +350,34 @@ static inline unsigned int v4l2_format_block_height(const struct v4l2_format_inf
>>  	return info->block_h[plane];
>>  }
>>  
>> +static inline unsigned int v4l2_format_plane_width(const struct v4l2_format_info *info, int plane,
>> +						   unsigned int width)
>> +{
>> +	unsigned int hdiv = plane ? info->hdiv : 1;
>> +	unsigned int aligned_width =
>> +		ALIGN(width, v4l2_format_block_width(info, plane));
>> +
>> +	return DIV_ROUND_UP(aligned_width, hdiv) *
>> +	       info->bpp[plane] / info->bpp_div[plane];
>> +}
>> +
>> +static inline unsigned int v4l2_format_plane_height(const struct v4l2_format_info *info, int plane,
>> +						    unsigned int height)
>> +{
>> +	unsigned int vdiv = plane ? info->vdiv : 1;
>> +	unsigned int aligned_height =
>> +		ALIGN(height, v4l2_format_block_height(info, plane));
>> +
>> +	return DIV_ROUND_UP(aligned_height, vdiv);
>> +}
>> +
>> +static inline unsigned int v4l2_format_plane_size(const struct v4l2_format_info *info, int plane,
>> +						  unsigned int width, unsigned int height)
>> +{
>> +	return v4l2_format_plane_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)
>>  {
>> @@ -385,37 +413,19 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
>>  
>>  	if (info->mem_planes == 1) {
>>  		plane = &pixfmt->plane_fmt[0];
>> -		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0] / info->bpp_div[0];
>> +		plane->bytesperline = v4l2_format_plane_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) / info->bpp_div[i];
>> -		}
>> +		for (i = 0; i < info->comp_planes; i++)
>> +			plane->sizeimage +=
>> +				v4l2_format_plane_size(info, i, width, height);
>>  	} else {
>>  		for (i = 0; i < info->comp_planes; i++) {
>> -			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
>> -			unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
>> -			unsigned int aligned_width;
>> -			unsigned int aligned_height;
>> -
>> -			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
>> -			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
>> -
>>  			plane = &pixfmt->plane_fmt[i];
>>  			plane->bytesperline =
>> -				info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv) / info->bpp_div[i];
>> -			plane->sizeimage =
>> -				plane->bytesperline * DIV_ROUND_UP(aligned_height, vdiv);
>> +				v4l2_format_plane_width(info, i, width);
>> +			plane->sizeimage = plane->bytesperline *
>> +				v4l2_format_plane_height(info, i, height);
>>  		}
>>  	}
>>  	return 0;
>> @@ -439,22 +449,12 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
>>  	pixfmt->width = width;
>>  	pixfmt->height = height;
>>  	pixfmt->pixelformat = pixelformat;
>> -	pixfmt->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0] / info->bpp_div[0];
>> +	pixfmt->bytesperline = v4l2_format_plane_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) / info->bpp_div[i];
>> -	}
>> +	for (i = 0; i < info->comp_planes; i++)
>> +		pixfmt->sizeimage +=
>> +			v4l2_format_plane_size(info, i, width, height);
>>  	return 0;
>>  }
>>  EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
> 

[PATCH v4 02/11] media: v4l2: Add NV15 and NV20 pixel formats

[Jonas Karlman]

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

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

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

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

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v4:
- No change

v3:
- Use bpp and bpp_div instead of the misuse of block_w/block_h
- Update documentation, expand to use full 4x4 sample image

 .../media/v4l/pixfmt-yuv-planar.rst           | 128 ++++++++++++++++++
 drivers/media/v4l2-core/v4l2-common.c         |   2 +
 drivers/media/v4l2-core/v4l2-ioctl.c          |   2 +
 include/uapi/linux/videodev2.h                |   2 +
 4 files changed, 134 insertions(+)

diff --git a/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst b/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
index 1840224faa41..4366cdcb970e 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'
+      - 10
+      - 4:2:0
+      - Cb, Cr
+      - Yes
+      - Linear
     * - V4L2_PIX_FMT_NV12M
       - 'NM12'
       - 8
@@ -158,6 +165,13 @@ All components are stored with the same number of bits per component.
       - Cr, Cb
       - Yes
       - Linear
+    * - V4L2_PIX_FMT_NV20
+      - 'NV20'
+      - 10
+      - 4:2:2
+      - Cb, Cr
+      - Yes
+      - Linear
     * - V4L2_PIX_FMT_NV16M
       - 'NM16'
       - 8
@@ -288,6 +302,57 @@ of the luma plane.
       - Cr\ :sub:`11`
 
 
+.. _V4L2-PIX-FMT-NV15:
+
+NV15
+----
+
+Semi-planar 10-bit YUV 4:2:0 format similar to NV12, using 10-bit components
+with no padding between each component. A group of 4 components are stored over
+5 bytes in little endian order.
+
+.. flat-table:: Sample 4x4 NV15 Image (1 byte per cell)
+    :header-rows:  0
+    :stub-columns: 0
+
+    * - start + 0:
+      - Y'\ :sub:`00[7:0]`
+      - Y'\ :sub:`01[5:0]`\ Y'\ :sub:`00[9:8]`
+      - Y'\ :sub:`02[3:0]`\ Y'\ :sub:`01[9:6]`
+      - Y'\ :sub:`03[1:0]`\ Y'\ :sub:`02[9:4]`
+      - Y'\ :sub:`03[9:2]`
+    * - start + 5:
+      - Y'\ :sub:`10[7:0]`
+      - Y'\ :sub:`11[5:0]`\ Y'\ :sub:`10[9:8]`
+      - Y'\ :sub:`12[3:0]`\ Y'\ :sub:`11[9:6]`
+      - Y'\ :sub:`13[1:0]`\ Y'\ :sub:`12[9:4]`
+      - Y'\ :sub:`13[9:2]`
+    * - start + 10:
+      - Y'\ :sub:`20[7:0]`
+      - Y'\ :sub:`21[5:0]`\ Y'\ :sub:`20[9:8]`
+      - Y'\ :sub:`22[3:0]`\ Y'\ :sub:`21[9:6]`
+      - Y'\ :sub:`23[1:0]`\ Y'\ :sub:`22[9:4]`
+      - Y'\ :sub:`23[9:2]`
+    * - start + 15:
+      - Y'\ :sub:`30[7:0]`
+      - Y'\ :sub:`31[5:0]`\ Y'\ :sub:`30[9:8]`
+      - Y'\ :sub:`32[3:0]`\ Y'\ :sub:`31[9:6]`
+      - Y'\ :sub:`33[1:0]`\ Y'\ :sub:`32[9:4]`
+      - Y'\ :sub:`33[9:2]`
+    * - start + 20:
+      - Cb\ :sub:`00[7:0]`
+      - Cr\ :sub:`00[5:0]`\ Cb\ :sub:`00[9:8]`
+      - Cb\ :sub:`01[3:0]`\ Cr\ :sub:`00[9:6]`
+      - Cr\ :sub:`01[1:0]`\ Cb\ :sub:`01[9:4]`
+      - Cr\ :sub:`01[9:2]`
+    * - start + 25:
+      - Cb\ :sub:`10[7:0]`
+      - Cr\ :sub:`10[5:0]`\ Cb\ :sub:`10[9:8]`
+      - Cb\ :sub:`11[3:0]`\ Cr\ :sub:`10[9:6]`
+      - Cr\ :sub:`11[1:0]`\ Cb\ :sub:`11[9:4]`
+      - Cr\ :sub:`11[9:2]`
+
+
 .. _V4L2-PIX-FMT-NV12MT:
 .. _V4L2-PIX-FMT-NV12MT-16X16:
 .. _V4L2-PIX-FMT-NV12-4L4:
@@ -500,6 +565,69 @@ number of lines as the luma plane.
       - Cr\ :sub:`32`
 
 
+.. _V4L2-PIX-FMT-NV20:
+
+NV20
+----
+
+Semi-planar 10-bit YUV 4:2:2 format similar to NV16, using 10-bit components
+with no padding between each component. A group of 4 components are stored over
+5 bytes in little endian order.
+
+.. flat-table:: Sample 4x4 NV20 Image (1 byte per cell)
+    :header-rows:  0
+    :stub-columns: 0
+
+    * - start + 0:
+      - Y'\ :sub:`00[7:0]`
+      - Y'\ :sub:`01[5:0]`\ Y'\ :sub:`00[9:8]`
+      - Y'\ :sub:`02[3:0]`\ Y'\ :sub:`01[9:6]`
+      - Y'\ :sub:`03[1:0]`\ Y'\ :sub:`02[9:4]`
+      - Y'\ :sub:`03[9:2]`
+    * - start + 5:
+      - Y'\ :sub:`10[7:0]`
+      - Y'\ :sub:`11[5:0]`\ Y'\ :sub:`10[9:8]`
+      - Y'\ :sub:`12[3:0]`\ Y'\ :sub:`11[9:6]`
+      - Y'\ :sub:`13[1:0]`\ Y'\ :sub:`12[9:4]`
+      - Y'\ :sub:`13[9:2]`
+    * - start + 10:
+      - Y'\ :sub:`20[7:0]`
+      - Y'\ :sub:`21[5:0]`\ Y'\ :sub:`20[9:8]`
+      - Y'\ :sub:`22[3:0]`\ Y'\ :sub:`21[9:6]`
+      - Y'\ :sub:`23[1:0]`\ Y'\ :sub:`22[9:4]`
+      - Y'\ :sub:`23[9:2]`
+    * - start + 15:
+      - Y'\ :sub:`30[7:0]`
+      - Y'\ :sub:`31[5:0]`\ Y'\ :sub:`30[9:8]`
+      - Y'\ :sub:`32[3:0]`\ Y'\ :sub:`31[9:6]`
+      - Y'\ :sub:`33[1:0]`\ Y'\ :sub:`32[9:4]`
+      - Y'\ :sub:`33[9:2]`
+    * - start + 20:
+      - Cb\ :sub:`00[7:0]`
+      - Cr\ :sub:`00[5:0]`\ Cb\ :sub:`00[9:8]`
+      - Cb\ :sub:`01[3:0]`\ Cr\ :sub:`00[9:6]`
+      - Cr\ :sub:`01[1:0]`\ Cb\ :sub:`01[9:4]`
+      - Cr\ :sub:`01[9:2]`
+    * - start + 25:
+      - Cb\ :sub:`10[7:0]`
+      - Cr\ :sub:`10[5:0]`\ Cb\ :sub:`10[9:8]`
+      - Cb\ :sub:`11[3:0]`\ Cr\ :sub:`10[9:6]`
+      - Cr\ :sub:`11[1:0]`\ Cb\ :sub:`11[9:4]`
+      - Cr\ :sub:`11[9:2]`
+    * - start + 30:
+      - Cb\ :sub:`20[7:0]`
+      - Cr\ :sub:`20[5:0]`\ Cb\ :sub:`20[9:8]`
+      - Cb\ :sub:`21[3:0]`\ Cr\ :sub:`20[9:6]`
+      - Cr\ :sub:`21[1:0]`\ Cb\ :sub:`21[9:4]`
+      - Cr\ :sub:`21[9:2]`
+    * - start + 35:
+      - Cb\ :sub:`30[7:0]`
+      - Cr\ :sub:`30[5:0]`\ Cb\ :sub:`30[9:8]`
+      - Cb\ :sub:`31[3:0]`\ Cr\ :sub:`30[9:6]`
+      - Cr\ :sub:`31[1:0]`\ Cb\ :sub:`31[9:4]`
+      - Cr\ :sub:`31[9:2]`
+
+
 .. _V4L2-PIX-FMT-NV24:
 .. _V4L2-PIX-FMT-NV42:
 
diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 834b426da8b1..c65ffab5800a 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -270,8 +270,10 @@ const struct v4l2_format_info *v4l2_format_info(u32 format)
 		/* YUV planar formats */
 		{ .format = V4L2_PIX_FMT_NV12,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 2, .vdiv = 2 },
 		{ .format = V4L2_PIX_FMT_NV21,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 2, .vdiv = 2 },
+		{ .format = V4L2_PIX_FMT_NV15,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 10, 0, 0 }, .bpp_div = { 4, 4, 1, 1 }, .hdiv = 2, .vdiv = 2 },
 		{ .format = V4L2_PIX_FMT_NV16,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 2, .vdiv = 1 },
 		{ .format = V4L2_PIX_FMT_NV61,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 2, .vdiv = 1 },
+		{ .format = V4L2_PIX_FMT_NV20,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 10, 0, 0 }, .bpp_div = { 4, 4, 1, 1 }, .hdiv = 2, .vdiv = 1 },
 		{ .format = V4L2_PIX_FMT_NV24,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 1, .vdiv = 1 },
 		{ .format = V4L2_PIX_FMT_NV42,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 1, .vdiv = 1 },
 		{ .format = V4L2_PIX_FMT_P010,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 2, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 2, .vdiv = 1 },
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 9b1de54ce379..937434e5f2c1 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1347,8 +1347,10 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
 	case V4L2_PIX_FMT_YUV48_12:	descr = "12-bit YUV 4:4:4 Packed"; break;
 	case V4L2_PIX_FMT_NV12:		descr = "Y/UV 4:2:0"; break;
 	case V4L2_PIX_FMT_NV21:		descr = "Y/VU 4:2:0"; break;
+	case V4L2_PIX_FMT_NV15:		descr = "10-bit Y/UV 4:2:0 (Packed)"; break;
 	case V4L2_PIX_FMT_NV16:		descr = "Y/UV 4:2:2"; break;
 	case V4L2_PIX_FMT_NV61:		descr = "Y/VU 4:2:2"; break;
+	case V4L2_PIX_FMT_NV20:		descr = "10-bit Y/UV 4:2:2 (Packed)"; break;
 	case V4L2_PIX_FMT_NV24:		descr = "Y/UV 4:4:4"; break;
 	case V4L2_PIX_FMT_NV42:		descr = "Y/VU 4:4:4"; break;
 	case V4L2_PIX_FMT_P010:		descr = "10-bit Y/UV 4:2:0"; break;
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index c3d4e490ce7c..617340c43e40 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -638,8 +638,10 @@ struct v4l2_pix_format {
 /* two planes -- one Y, one Cr + Cb interleaved  */
 #define V4L2_PIX_FMT_NV12    v4l2_fourcc('N', 'V', '1', '2') /* 12  Y/CbCr 4:2:0  */
 #define V4L2_PIX_FMT_NV21    v4l2_fourcc('N', 'V', '2', '1') /* 12  Y/CrCb 4:2:0  */
+#define V4L2_PIX_FMT_NV15    v4l2_fourcc('N', 'V', '1', '5') /* 15  Y/CbCr 4:2:0 10-bit packed */
 #define V4L2_PIX_FMT_NV16    v4l2_fourcc('N', 'V', '1', '6') /* 16  Y/CbCr 4:2:2  */
 #define V4L2_PIX_FMT_NV61    v4l2_fourcc('N', 'V', '6', '1') /* 16  Y/CrCb 4:2:2  */
+#define V4L2_PIX_FMT_NV20    v4l2_fourcc('N', 'V', '2', '0') /* 20  Y/CbCr 4:2:2 10-bit packed */
 #define V4L2_PIX_FMT_NV24    v4l2_fourcc('N', 'V', '2', '4') /* 24  Y/CbCr 4:4:4  */
 #define V4L2_PIX_FMT_NV42    v4l2_fourcc('N', 'V', '4', '2') /* 24  Y/CrCb 4:4:4  */
 #define V4L2_PIX_FMT_P010    v4l2_fourcc('P', '0', '1', '0') /* 24  Y/CbCr 4:2:0 10-bit per component */
-- 
2.42.0

Re: [PATCH v4 02/11] media: v4l2: Add NV15 and NV20 pixel formats

[Nicolas Dufresne]

Hi,

Le dimanche 05 novembre 2023 à 16:55 +0000, Jonas Karlman a écrit :
> Add NV15 and NV20 pixel formats used by the Rockchip Video Decoder for
> 10-bit buffers.
> 
> NV15 and NV20 is 10-bit 4:2:0/4:2:2 semi-planar YUV formats similar to
> NV12 and NV16, using 10-bit components with no padding between each
> component. Instead, a group of 4 luminance/chrominance samples are
> stored over 5 bytes in little endian order:
> 
> YYYY = UVUV = 4 * 10 bits = 40 bits = 5 bytes
> 
> The '15' and '20' suffix refers to the optimum effective bits per pixel
> which is achieved when the total number of luminance samples is a
> multiple of 8 for NV15 and 4 for NV20.

I had this mostly documented already in September:

https://patchwork.linuxtv.org/project/linux-media/patch/20230914150651.3114134-3-nicolas.dufresne@collabora.com/
https://patchwork.linuxtv.org/project/linux-media/patch/20230914150651.3114134-4-nicolas.dufresne@collabora.com/

I think I was improving a bit the documentation avoiding to repeat what
NV15 (little and big endian) is over and over. I've had no feedback on
V2, so assumed it going to be picked, but then it will certainly
conflict with this change.

> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
> v4:
> - No change
> 
> v3:
> - Use bpp and bpp_div instead of the misuse of block_w/block_h
> - Update documentation, expand to use full 4x4 sample image
> 
>  .../media/v4l/pixfmt-yuv-planar.rst           | 128 ++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-common.c         |   2 +
>  drivers/media/v4l2-core/v4l2-ioctl.c          |   2 +
>  include/uapi/linux/videodev2.h                |   2 +
>  4 files changed, 134 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst b/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
> index 1840224faa41..4366cdcb970e 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'
> +      - 10
> +      - 4:2:0
> +      - Cb, Cr
> +      - Yes
> +      - Linear

I'd like to see 8 and 10bit formats grouped and not mixed.

>      * - V4L2_PIX_FMT_NV12M
>        - 'NM12'
>        - 8
> @@ -158,6 +165,13 @@ All components are stored with the same number of bits per component.
>        - Cr, Cb
>        - Yes
>        - Linear
> +    * - V4L2_PIX_FMT_NV20
> +      - 'NV20'
> +      - 10
> +      - 4:2:2
> +      - Cb, Cr
> +      - Yes
> +      - Linear
>      * - V4L2_PIX_FMT_NV16M
>        - 'NM16'
>        - 8
> @@ -288,6 +302,57 @@ of the luma plane.
>        - Cr\ :sub:`11`
>  
>  
> +.. _V4L2-PIX-FMT-NV15:
> +
> +NV15
> +----
> +
> +Semi-planar 10-bit YUV 4:2:0 format similar to NV12, using 10-bit components
> +with no padding between each component. A group of 4 components are stored over
> +5 bytes in little endian order.
> +
> +.. flat-table:: Sample 4x4 NV15 Image (1 byte per cell)
> +    :header-rows:  0
> +    :stub-columns: 0
> +
> +    * - start + 0:
> +      - Y'\ :sub:`00[7:0]`
> +      - Y'\ :sub:`01[5:0]`\ Y'\ :sub:`00[9:8]`
> +      - Y'\ :sub:`02[3:0]`\ Y'\ :sub:`01[9:6]`
> +      - Y'\ :sub:`03[1:0]`\ Y'\ :sub:`02[9:4]`
> +      - Y'\ :sub:`03[9:2]`
> +    * - start + 5:
> +      - Y'\ :sub:`10[7:0]`
> +      - Y'\ :sub:`11[5:0]`\ Y'\ :sub:`10[9:8]`
> +      - Y'\ :sub:`12[3:0]`\ Y'\ :sub:`11[9:6]`
> +      - Y'\ :sub:`13[1:0]`\ Y'\ :sub:`12[9:4]`
> +      - Y'\ :sub:`13[9:2]`
> +    * - start + 10:
> +      - Y'\ :sub:`20[7:0]`
> +      - Y'\ :sub:`21[5:0]`\ Y'\ :sub:`20[9:8]`
> +      - Y'\ :sub:`22[3:0]`\ Y'\ :sub:`21[9:6]`
> +      - Y'\ :sub:`23[1:0]`\ Y'\ :sub:`22[9:4]`
> +      - Y'\ :sub:`23[9:2]`
> +    * - start + 15:
> +      - Y'\ :sub:`30[7:0]`
> +      - Y'\ :sub:`31[5:0]`\ Y'\ :sub:`30[9:8]`
> +      - Y'\ :sub:`32[3:0]`\ Y'\ :sub:`31[9:6]`
> +      - Y'\ :sub:`33[1:0]`\ Y'\ :sub:`32[9:4]`
> +      - Y'\ :sub:`33[9:2]`
> +    * - start + 20:
> +      - Cb\ :sub:`00[7:0]`
> +      - Cr\ :sub:`00[5:0]`\ Cb\ :sub:`00[9:8]`
> +      - Cb\ :sub:`01[3:0]`\ Cr\ :sub:`00[9:6]`
> +      - Cr\ :sub:`01[1:0]`\ Cb\ :sub:`01[9:4]`
> +      - Cr\ :sub:`01[9:2]`
> +    * - start + 25:
> +      - Cb\ :sub:`10[7:0]`
> +      - Cr\ :sub:`10[5:0]`\ Cb\ :sub:`10[9:8]`
> +      - Cb\ :sub:`11[3:0]`\ Cr\ :sub:`10[9:6]`
> +      - Cr\ :sub:`11[1:0]`\ Cb\ :sub:`11[9:4]`
> +      - Cr\ :sub:`11[9:2]`
> +
> +
>  .. _V4L2-PIX-FMT-NV12MT:
>  .. _V4L2-PIX-FMT-NV12MT-16X16:
>  .. _V4L2-PIX-FMT-NV12-4L4:
> @@ -500,6 +565,69 @@ number of lines as the luma plane.
>        - Cr\ :sub:`32`
>  
>  
> +.. _V4L2-PIX-FMT-NV20:
> +
> +NV20
> +----
> +
> +Semi-planar 10-bit YUV 4:2:2 format similar to NV16, using 10-bit components
> +with no padding between each component. A group of 4 components are stored over
> +5 bytes in little endian order.
> +
> +.. flat-table:: Sample 4x4 NV20 Image (1 byte per cell)
> +    :header-rows:  0
> +    :stub-columns: 0
> +
> +    * - start + 0:
> +      - Y'\ :sub:`00[7:0]`
> +      - Y'\ :sub:`01[5:0]`\ Y'\ :sub:`00[9:8]`
> +      - Y'\ :sub:`02[3:0]`\ Y'\ :sub:`01[9:6]`
> +      - Y'\ :sub:`03[1:0]`\ Y'\ :sub:`02[9:4]`
> +      - Y'\ :sub:`03[9:2]`
> +    * - start + 5:
> +      - Y'\ :sub:`10[7:0]`
> +      - Y'\ :sub:`11[5:0]`\ Y'\ :sub:`10[9:8]`
> +      - Y'\ :sub:`12[3:0]`\ Y'\ :sub:`11[9:6]`
> +      - Y'\ :sub:`13[1:0]`\ Y'\ :sub:`12[9:4]`
> +      - Y'\ :sub:`13[9:2]`
> +    * - start + 10:
> +      - Y'\ :sub:`20[7:0]`
> +      - Y'\ :sub:`21[5:0]`\ Y'\ :sub:`20[9:8]`
> +      - Y'\ :sub:`22[3:0]`\ Y'\ :sub:`21[9:6]`
> +      - Y'\ :sub:`23[1:0]`\ Y'\ :sub:`22[9:4]`
> +      - Y'\ :sub:`23[9:2]`
> +    * - start + 15:
> +      - Y'\ :sub:`30[7:0]`
> +      - Y'\ :sub:`31[5:0]`\ Y'\ :sub:`30[9:8]`
> +      - Y'\ :sub:`32[3:0]`\ Y'\ :sub:`31[9:6]`
> +      - Y'\ :sub:`33[1:0]`\ Y'\ :sub:`32[9:4]`
> +      - Y'\ :sub:`33[9:2]`
> +    * - start + 20:
> +      - Cb\ :sub:`00[7:0]`
> +      - Cr\ :sub:`00[5:0]`\ Cb\ :sub:`00[9:8]`
> +      - Cb\ :sub:`01[3:0]`\ Cr\ :sub:`00[9:6]`
> +      - Cr\ :sub:`01[1:0]`\ Cb\ :sub:`01[9:4]`
> +      - Cr\ :sub:`01[9:2]`
> +    * - start + 25:
> +      - Cb\ :sub:`10[7:0]`
> +      - Cr\ :sub:`10[5:0]`\ Cb\ :sub:`10[9:8]`
> +      - Cb\ :sub:`11[3:0]`\ Cr\ :sub:`10[9:6]`
> +      - Cr\ :sub:`11[1:0]`\ Cb\ :sub:`11[9:4]`
> +      - Cr\ :sub:`11[9:2]`
> +    * - start + 30:
> +      - Cb\ :sub:`20[7:0]`
> +      - Cr\ :sub:`20[5:0]`\ Cb\ :sub:`20[9:8]`
> +      - Cb\ :sub:`21[3:0]`\ Cr\ :sub:`20[9:6]`
> +      - Cr\ :sub:`21[1:0]`\ Cb\ :sub:`21[9:4]`
> +      - Cr\ :sub:`21[9:2]`
> +    * - start + 35:
> +      - Cb\ :sub:`30[7:0]`
> +      - Cr\ :sub:`30[5:0]`\ Cb\ :sub:`30[9:8]`
> +      - Cb\ :sub:`31[3:0]`\ Cr\ :sub:`30[9:6]`
> +      - Cr\ :sub:`31[1:0]`\ Cb\ :sub:`31[9:4]`
> +      - Cr\ :sub:`31[9:2]`
> +
> +
>  .. _V4L2-PIX-FMT-NV24:
>  .. _V4L2-PIX-FMT-NV42:
>  
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index 834b426da8b1..c65ffab5800a 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -270,8 +270,10 @@ const struct v4l2_format_info *v4l2_format_info(u32 format)
>  		/* YUV planar formats */
>  		{ .format = V4L2_PIX_FMT_NV12,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 2, .vdiv = 2 },
>  		{ .format = V4L2_PIX_FMT_NV21,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 2, .vdiv = 2 },
> +		{ .format = V4L2_PIX_FMT_NV15,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 10, 0, 0 }, .bpp_div = { 4, 4, 1, 1 }, .hdiv = 2, .vdiv = 2 },
>  		{ .format = V4L2_PIX_FMT_NV16,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 2, .vdiv = 1 },
>  		{ .format = V4L2_PIX_FMT_NV61,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 2, .vdiv = 1 },
> +		{ .format = V4L2_PIX_FMT_NV20,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 10, 0, 0 }, .bpp_div = { 4, 4, 1, 1 }, .hdiv = 2, .vdiv = 1 },
>  		{ .format = V4L2_PIX_FMT_NV24,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 1, .vdiv = 1 },
>  		{ .format = V4L2_PIX_FMT_NV42,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 1, .vdiv = 1 },
>  		{ .format = V4L2_PIX_FMT_P010,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 2, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 2, .vdiv = 1 },
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 9b1de54ce379..937434e5f2c1 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1347,8 +1347,10 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
>  	case V4L2_PIX_FMT_YUV48_12:	descr = "12-bit YUV 4:4:4 Packed"; break;
>  	case V4L2_PIX_FMT_NV12:		descr = "Y/UV 4:2:0"; break;
>  	case V4L2_PIX_FMT_NV21:		descr = "Y/VU 4:2:0"; break;
> +	case V4L2_PIX_FMT_NV15:		descr = "10-bit Y/UV 4:2:0 (Packed)"; break;
>  	case V4L2_PIX_FMT_NV16:		descr = "Y/UV 4:2:2"; break;
>  	case V4L2_PIX_FMT_NV61:		descr = "Y/VU 4:2:2"; break;
> +	case V4L2_PIX_FMT_NV20:		descr = "10-bit Y/UV 4:2:2 (Packed)"; break;
>  	case V4L2_PIX_FMT_NV24:		descr = "Y/UV 4:4:4"; break;
>  	case V4L2_PIX_FMT_NV42:		descr = "Y/VU 4:4:4"; break;
>  	case V4L2_PIX_FMT_P010:		descr = "10-bit Y/UV 4:2:0"; break;
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index c3d4e490ce7c..617340c43e40 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -638,8 +638,10 @@ struct v4l2_pix_format {
>  /* two planes -- one Y, one Cr + Cb interleaved  */
>  #define V4L2_PIX_FMT_NV12    v4l2_fourcc('N', 'V', '1', '2') /* 12  Y/CbCr 4:2:0  */
>  #define V4L2_PIX_FMT_NV21    v4l2_fourcc('N', 'V', '2', '1') /* 12  Y/CrCb 4:2:0  */
> +#define V4L2_PIX_FMT_NV15    v4l2_fourcc('N', 'V', '1', '5') /* 15  Y/CbCr 4:2:0 10-bit packed */
>  #define V4L2_PIX_FMT_NV16    v4l2_fourcc('N', 'V', '1', '6') /* 16  Y/CbCr 4:2:2  */
>  #define V4L2_PIX_FMT_NV61    v4l2_fourcc('N', 'V', '6', '1') /* 16  Y/CrCb 4:2:2  */
> +#define V4L2_PIX_FMT_NV20    v4l2_fourcc('N', 'V', '2', '0') /* 20  Y/CbCr 4:2:2 10-bit packed */

Would make sense to group all semi planar 10bit together.

>  #define V4L2_PIX_FMT_NV24    v4l2_fourcc('N', 'V', '2', '4') /* 24  Y/CbCr 4:4:4  */
>  #define V4L2_PIX_FMT_NV42    v4l2_fourcc('N', 'V', '4', '2') /* 24  Y/CrCb 4:4:4  */
>  #define V4L2_PIX_FMT_P010    v4l2_fourcc('P', '0', '1', '0') /* 24  Y/CbCr 4:2:0 10-bit per component */

regards,
Nicolas

Re: [PATCH v4 02/11] media: v4l2: Add NV15 and NV20 pixel formats

[Jonas Karlman]

On 2023-11-08 03:52, Nicolas Dufresne wrote:
> Hi,
> 
> Le dimanche 05 novembre 2023 à 16:55 +0000, Jonas Karlman a écrit :
>> Add NV15 and NV20 pixel formats used by the Rockchip Video Decoder for
>> 10-bit buffers.
>>
>> NV15 and NV20 is 10-bit 4:2:0/4:2:2 semi-planar YUV formats similar to
>> NV12 and NV16, using 10-bit components with no padding between each
>> component. Instead, a group of 4 luminance/chrominance samples are
>> stored over 5 bytes in little endian order:
>>
>> YYYY = UVUV = 4 * 10 bits = 40 bits = 5 bytes
>>
>> The '15' and '20' suffix refers to the optimum effective bits per pixel
>> which is achieved when the total number of luminance samples is a
>> multiple of 8 for NV15 and 4 for NV20.
> 
> I had this mostly documented already in September:
> 
> https://patchwork.linuxtv.org/project/linux-media/patch/20230914150651.3114134-3-nicolas.dufresne@collabora.com/
> https://patchwork.linuxtv.org/project/linux-media/patch/20230914150651.3114134-4-nicolas.dufresne@collabora.com/
> 
> I think I was improving a bit the documentation avoiding to repeat what
> NV15 (little and big endian) is over and over. I've had no feedback on
> V2, so assumed it going to be picked, but then it will certainly
> conflict with this change.

Sorry, I had completely missed this pending series, sent out a short
comment earlier and will rebase this on top of your series for v5.

> 
>>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>> v4:
>> - No change
>>
>> v3:
>> - Use bpp and bpp_div instead of the misuse of block_w/block_h
>> - Update documentation, expand to use full 4x4 sample image
>>
>>  .../media/v4l/pixfmt-yuv-planar.rst           | 128 ++++++++++++++++++
>>  drivers/media/v4l2-core/v4l2-common.c         |   2 +
>>  drivers/media/v4l2-core/v4l2-ioctl.c          |   2 +
>>  include/uapi/linux/videodev2.h                |   2 +
>>  4 files changed, 134 insertions(+)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst b/Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst
>> index 1840224faa41..4366cdcb970e 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'
>> +      - 10
>> +      - 4:2:0
>> +      - Cb, Cr
>> +      - Yes
>> +      - Linear
> 
> I'd like to see 8 and 10bit formats grouped and not mixed.

Sure, will fix in v5.

> 
>>      * - V4L2_PIX_FMT_NV12M
>>        - 'NM12'
>>        - 8
>> @@ -158,6 +165,13 @@ All components are stored with the same number of bits per component.
>>        - Cr, Cb
>>        - Yes
>>        - Linear
>> +    * - V4L2_PIX_FMT_NV20
>> +      - 'NV20'
>> +      - 10
>> +      - 4:2:2
>> +      - Cb, Cr
>> +      - Yes
>> +      - Linear
>>      * - V4L2_PIX_FMT_NV16M
>>        - 'NM16'
>>        - 8
>> @@ -288,6 +302,57 @@ of the luma plane.
>>        - Cr\ :sub:`11`
>>  
>>  
>> +.. _V4L2-PIX-FMT-NV15:
>> +
>> +NV15
>> +----
>> +
>> +Semi-planar 10-bit YUV 4:2:0 format similar to NV12, using 10-bit components
>> +with no padding between each component. A group of 4 components are stored over
>> +5 bytes in little endian order.
>> +
>> +.. flat-table:: Sample 4x4 NV15 Image (1 byte per cell)
>> +    :header-rows:  0
>> +    :stub-columns: 0
>> +
>> +    * - start + 0:
>> +      - Y'\ :sub:`00[7:0]`
>> +      - Y'\ :sub:`01[5:0]`\ Y'\ :sub:`00[9:8]`
>> +      - Y'\ :sub:`02[3:0]`\ Y'\ :sub:`01[9:6]`
>> +      - Y'\ :sub:`03[1:0]`\ Y'\ :sub:`02[9:4]`
>> +      - Y'\ :sub:`03[9:2]`
>> +    * - start + 5:
>> +      - Y'\ :sub:`10[7:0]`
>> +      - Y'\ :sub:`11[5:0]`\ Y'\ :sub:`10[9:8]`
>> +      - Y'\ :sub:`12[3:0]`\ Y'\ :sub:`11[9:6]`
>> +      - Y'\ :sub:`13[1:0]`\ Y'\ :sub:`12[9:4]`
>> +      - Y'\ :sub:`13[9:2]`
>> +    * - start + 10:
>> +      - Y'\ :sub:`20[7:0]`
>> +      - Y'\ :sub:`21[5:0]`\ Y'\ :sub:`20[9:8]`
>> +      - Y'\ :sub:`22[3:0]`\ Y'\ :sub:`21[9:6]`
>> +      - Y'\ :sub:`23[1:0]`\ Y'\ :sub:`22[9:4]`
>> +      - Y'\ :sub:`23[9:2]`
>> +    * - start + 15:
>> +      - Y'\ :sub:`30[7:0]`
>> +      - Y'\ :sub:`31[5:0]`\ Y'\ :sub:`30[9:8]`
>> +      - Y'\ :sub:`32[3:0]`\ Y'\ :sub:`31[9:6]`
>> +      - Y'\ :sub:`33[1:0]`\ Y'\ :sub:`32[9:4]`
>> +      - Y'\ :sub:`33[9:2]`
>> +    * - start + 20:
>> +      - Cb\ :sub:`00[7:0]`
>> +      - Cr\ :sub:`00[5:0]`\ Cb\ :sub:`00[9:8]`
>> +      - Cb\ :sub:`01[3:0]`\ Cr\ :sub:`00[9:6]`
>> +      - Cr\ :sub:`01[1:0]`\ Cb\ :sub:`01[9:4]`
>> +      - Cr\ :sub:`01[9:2]`
>> +    * - start + 25:
>> +      - Cb\ :sub:`10[7:0]`
>> +      - Cr\ :sub:`10[5:0]`\ Cb\ :sub:`10[9:8]`
>> +      - Cb\ :sub:`11[3:0]`\ Cr\ :sub:`10[9:6]`
>> +      - Cr\ :sub:`11[1:0]`\ Cb\ :sub:`11[9:4]`
>> +      - Cr\ :sub:`11[9:2]`
>> +
>> +
>>  .. _V4L2-PIX-FMT-NV12MT:
>>  .. _V4L2-PIX-FMT-NV12MT-16X16:
>>  .. _V4L2-PIX-FMT-NV12-4L4:
>> @@ -500,6 +565,69 @@ number of lines as the luma plane.
>>        - Cr\ :sub:`32`
>>  
>>  
>> +.. _V4L2-PIX-FMT-NV20:
>> +
>> +NV20
>> +----
>> +
>> +Semi-planar 10-bit YUV 4:2:2 format similar to NV16, using 10-bit components
>> +with no padding between each component. A group of 4 components are stored over
>> +5 bytes in little endian order.
>> +
>> +.. flat-table:: Sample 4x4 NV20 Image (1 byte per cell)
>> +    :header-rows:  0
>> +    :stub-columns: 0
>> +
>> +    * - start + 0:
>> +      - Y'\ :sub:`00[7:0]`
>> +      - Y'\ :sub:`01[5:0]`\ Y'\ :sub:`00[9:8]`
>> +      - Y'\ :sub:`02[3:0]`\ Y'\ :sub:`01[9:6]`
>> +      - Y'\ :sub:`03[1:0]`\ Y'\ :sub:`02[9:4]`
>> +      - Y'\ :sub:`03[9:2]`
>> +    * - start + 5:
>> +      - Y'\ :sub:`10[7:0]`
>> +      - Y'\ :sub:`11[5:0]`\ Y'\ :sub:`10[9:8]`
>> +      - Y'\ :sub:`12[3:0]`\ Y'\ :sub:`11[9:6]`
>> +      - Y'\ :sub:`13[1:0]`\ Y'\ :sub:`12[9:4]`
>> +      - Y'\ :sub:`13[9:2]`
>> +    * - start + 10:
>> +      - Y'\ :sub:`20[7:0]`
>> +      - Y'\ :sub:`21[5:0]`\ Y'\ :sub:`20[9:8]`
>> +      - Y'\ :sub:`22[3:0]`\ Y'\ :sub:`21[9:6]`
>> +      - Y'\ :sub:`23[1:0]`\ Y'\ :sub:`22[9:4]`
>> +      - Y'\ :sub:`23[9:2]`
>> +    * - start + 15:
>> +      - Y'\ :sub:`30[7:0]`
>> +      - Y'\ :sub:`31[5:0]`\ Y'\ :sub:`30[9:8]`
>> +      - Y'\ :sub:`32[3:0]`\ Y'\ :sub:`31[9:6]`
>> +      - Y'\ :sub:`33[1:0]`\ Y'\ :sub:`32[9:4]`
>> +      - Y'\ :sub:`33[9:2]`
>> +    * - start + 20:
>> +      - Cb\ :sub:`00[7:0]`
>> +      - Cr\ :sub:`00[5:0]`\ Cb\ :sub:`00[9:8]`
>> +      - Cb\ :sub:`01[3:0]`\ Cr\ :sub:`00[9:6]`
>> +      - Cr\ :sub:`01[1:0]`\ Cb\ :sub:`01[9:4]`
>> +      - Cr\ :sub:`01[9:2]`
>> +    * - start + 25:
>> +      - Cb\ :sub:`10[7:0]`
>> +      - Cr\ :sub:`10[5:0]`\ Cb\ :sub:`10[9:8]`
>> +      - Cb\ :sub:`11[3:0]`\ Cr\ :sub:`10[9:6]`
>> +      - Cr\ :sub:`11[1:0]`\ Cb\ :sub:`11[9:4]`
>> +      - Cr\ :sub:`11[9:2]`
>> +    * - start + 30:
>> +      - Cb\ :sub:`20[7:0]`
>> +      - Cr\ :sub:`20[5:0]`\ Cb\ :sub:`20[9:8]`
>> +      - Cb\ :sub:`21[3:0]`\ Cr\ :sub:`20[9:6]`
>> +      - Cr\ :sub:`21[1:0]`\ Cb\ :sub:`21[9:4]`
>> +      - Cr\ :sub:`21[9:2]`
>> +    * - start + 35:
>> +      - Cb\ :sub:`30[7:0]`
>> +      - Cr\ :sub:`30[5:0]`\ Cb\ :sub:`30[9:8]`
>> +      - Cb\ :sub:`31[3:0]`\ Cr\ :sub:`30[9:6]`
>> +      - Cr\ :sub:`31[1:0]`\ Cb\ :sub:`31[9:4]`
>> +      - Cr\ :sub:`31[9:2]`
>> +
>> +
>>  .. _V4L2-PIX-FMT-NV24:
>>  .. _V4L2-PIX-FMT-NV42:
>>  
>> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
>> index 834b426da8b1..c65ffab5800a 100644
>> --- a/drivers/media/v4l2-core/v4l2-common.c
>> +++ b/drivers/media/v4l2-core/v4l2-common.c
>> @@ -270,8 +270,10 @@ const struct v4l2_format_info *v4l2_format_info(u32 format)
>>  		/* YUV planar formats */
>>  		{ .format = V4L2_PIX_FMT_NV12,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 2, .vdiv = 2 },
>>  		{ .format = V4L2_PIX_FMT_NV21,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 2, .vdiv = 2 },
>> +		{ .format = V4L2_PIX_FMT_NV15,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 10, 0, 0 }, .bpp_div = { 4, 4, 1, 1 }, .hdiv = 2, .vdiv = 2 },
>>  		{ .format = V4L2_PIX_FMT_NV16,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 2, .vdiv = 1 },
>>  		{ .format = V4L2_PIX_FMT_NV61,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 2, .vdiv = 1 },
>> +		{ .format = V4L2_PIX_FMT_NV20,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 10, 0, 0 }, .bpp_div = { 4, 4, 1, 1 }, .hdiv = 2, .vdiv = 1 },
>>  		{ .format = V4L2_PIX_FMT_NV24,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 1, .vdiv = 1 },
>>  		{ .format = V4L2_PIX_FMT_NV42,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 1, .vdiv = 1 },
>>  		{ .format = V4L2_PIX_FMT_P010,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 2, 2, 0, 0 }, .bpp_div = { 1, 1, 1, 1 }, .hdiv = 2, .vdiv = 1 },
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index 9b1de54ce379..937434e5f2c1 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -1347,8 +1347,10 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
>>  	case V4L2_PIX_FMT_YUV48_12:	descr = "12-bit YUV 4:4:4 Packed"; break;
>>  	case V4L2_PIX_FMT_NV12:		descr = "Y/UV 4:2:0"; break;
>>  	case V4L2_PIX_FMT_NV21:		descr = "Y/VU 4:2:0"; break;
>> +	case V4L2_PIX_FMT_NV15:		descr = "10-bit Y/UV 4:2:0 (Packed)"; break;
>>  	case V4L2_PIX_FMT_NV16:		descr = "Y/UV 4:2:2"; break;
>>  	case V4L2_PIX_FMT_NV61:		descr = "Y/VU 4:2:2"; break;
>> +	case V4L2_PIX_FMT_NV20:		descr = "10-bit Y/UV 4:2:2 (Packed)"; break;
>>  	case V4L2_PIX_FMT_NV24:		descr = "Y/UV 4:4:4"; break;
>>  	case V4L2_PIX_FMT_NV42:		descr = "Y/VU 4:4:4"; break;
>>  	case V4L2_PIX_FMT_P010:		descr = "10-bit Y/UV 4:2:0"; break;
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index c3d4e490ce7c..617340c43e40 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -638,8 +638,10 @@ struct v4l2_pix_format {
>>  /* two planes -- one Y, one Cr + Cb interleaved  */
>>  #define V4L2_PIX_FMT_NV12    v4l2_fourcc('N', 'V', '1', '2') /* 12  Y/CbCr 4:2:0  */
>>  #define V4L2_PIX_FMT_NV21    v4l2_fourcc('N', 'V', '2', '1') /* 12  Y/CrCb 4:2:0  */
>> +#define V4L2_PIX_FMT_NV15    v4l2_fourcc('N', 'V', '1', '5') /* 15  Y/CbCr 4:2:0 10-bit packed */
>>  #define V4L2_PIX_FMT_NV16    v4l2_fourcc('N', 'V', '1', '6') /* 16  Y/CbCr 4:2:2  */
>>  #define V4L2_PIX_FMT_NV61    v4l2_fourcc('N', 'V', '6', '1') /* 16  Y/CrCb 4:2:2  */
>> +#define V4L2_PIX_FMT_NV20    v4l2_fourcc('N', 'V', '2', '0') /* 20  Y/CbCr 4:2:2 10-bit packed */
> 
> Would make sense to group all semi planar 10bit together.

Sure, will fix in v5.

Regards,
Jonas

> 
>>  #define V4L2_PIX_FMT_NV24    v4l2_fourcc('N', 'V', '2', '4') /* 24  Y/CbCr 4:4:4  */
>>  #define V4L2_PIX_FMT_NV42    v4l2_fourcc('N', 'V', '4', '2') /* 24  Y/CrCb 4:4:4  */
>>  #define V4L2_PIX_FMT_P010    v4l2_fourcc('P', '0', '1', '0') /* 24  Y/CbCr 4:2:0 10-bit per component */
> 
> regards,
> Nicolas

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

[Jonas Karlman]

Use bytesperline and buffer height to calculate the strides configured.

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

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v4:
- No change

v3:
- Remove unnecessary yuv_virstride +=

 drivers/staging/media/rkvdec/rkvdec-h264.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

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

Re: [PATCH v4 03/11] media: rkvdec: h264: Use bytesperline and buffer height as virstride

[Nicolas Dufresne]

Le dimanche 05 novembre 2023 à 16:55 +0000, Jonas Karlman a écrit :
> Use bytesperline and buffer height to calculate the strides configured.
> 
> This does not really change anything other than ensuring the
> bytesperline that is signaled to userspace matches what is configured
> in HW.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

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

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

[Jonas Karlman]

From: Alex Bee <knaerzche@gmail.com>

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

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

Signed-off-by: Alex Bee <knaerzche@gmail.com>
[jonas@kwiboo.se: constraint_set_flags condition, commit message]
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v4:
- No change

v3:
- New patch

 drivers/staging/media/rkvdec/rkvdec-h264.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

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

Re: [PATCH v4 04/11] media: rkvdec: h264: Don't hardcode SPS/PPS parameters

[Nicolas Dufresne]

Le dimanche 05 novembre 2023 à 16:55 +0000, Jonas Karlman a écrit :
> From: Alex Bee <knaerzche@gmail.com>
> 
> Some SPS/PPS parameters are currently hardcoded in the driver even
> though they exist in the stable uapi controls.
> 
> Use values from SPS/PPS controls instead of hardcoding them.
> 
> Signed-off-by: Alex Bee <knaerzche@gmail.com>
> [jonas@kwiboo.se: constraint_set_flags condition, commit message]
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

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

[PATCH v4 05/11] media: rkvdec: h264: Remove SPS validation at streaming start

[Jonas Karlman]

SPS parameters is validated in try_ctrl() ops so there is no need to
re-validate when streaming starts.

Remove the unnecessary call to validate sps at streaming start.

Suggested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v4:
- No change

v3:
- New patch

 drivers/staging/media/rkvdec/rkvdec-h264.c | 19 ++-----------------
 1 file changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 8bce8902b8dd..815d5359ddd5 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -1070,17 +1070,6 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
 	struct rkvdec_dev *rkvdec = ctx->dev;
 	struct rkvdec_h264_priv_tbl *priv_tbl;
 	struct rkvdec_h264_ctx *h264_ctx;
-	struct v4l2_ctrl *ctrl;
-	int ret;
-
-	ctrl = v4l2_ctrl_find(&ctx->ctrl_hdl,
-			      V4L2_CID_STATELESS_H264_SPS);
-	if (!ctrl)
-		return -EINVAL;
-
-	ret = rkvdec_h264_validate_sps(ctx, ctrl->p_new.p_h264_sps);
-	if (ret)
-		return ret;
 
 	h264_ctx = kzalloc(sizeof(*h264_ctx), GFP_KERNEL);
 	if (!h264_ctx)
@@ -1089,8 +1078,8 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
 	priv_tbl = dma_alloc_coherent(rkvdec->dev, sizeof(*priv_tbl),
 				      &h264_ctx->priv_tbl.dma, GFP_KERNEL);
 	if (!priv_tbl) {
-		ret = -ENOMEM;
-		goto err_free_ctx;
+		kfree(h264_ctx);
+		return -ENOMEM;
 	}
 
 	h264_ctx->priv_tbl.size = sizeof(*priv_tbl);
@@ -1100,10 +1089,6 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
 
 	ctx->priv = h264_ctx;
 	return 0;
-
-err_free_ctx:
-	kfree(h264_ctx);
-	return ret;
 }
 
 static void rkvdec_h264_stop(struct rkvdec_ctx *ctx)
-- 
2.42.0

Re: [PATCH v4 05/11] media: rkvdec: h264: Remove SPS validation at streaming start

[Nicolas Dufresne]

Le dimanche 05 novembre 2023 à 16:55 +0000, Jonas Karlman a écrit :
> SPS parameters is validated in try_ctrl() ops so there is no need to

                 are

> re-validate when streaming starts.
> 
> Remove the unnecessary call to validate sps at streaming start.

This patch is not working since user may change the format after the
control have been set. The proper fix should actually reset the SPS
(well all header controls) to match the the newly set format. Then this
validation won't be needed anymore.

The sequence that is problematic after this patch is:

S_FMT (OUTPUT, width, height);
S_CTRL (SPS) // valid
S_FMT(OUTPUT, width', height'); // SPS is no longer valid

One suggestion I may make is to add a ops so that each codec
implementation can reset their header controls to make it valid against
the new resolution. With that in place you'll be able drop the check.

Nicolas

p.s. you can also just drop this patch from the series and revisit it
later, though I think its worth fixing.

> 
> Suggested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
> v4:
> - No change
> 
> v3:
> - New patch
> 
>  drivers/staging/media/rkvdec/rkvdec-h264.c | 19 ++-----------------
>  1 file changed, 2 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index 8bce8902b8dd..815d5359ddd5 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -1070,17 +1070,6 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
>  	struct rkvdec_dev *rkvdec = ctx->dev;
>  	struct rkvdec_h264_priv_tbl *priv_tbl;
>  	struct rkvdec_h264_ctx *h264_ctx;
> -	struct v4l2_ctrl *ctrl;
> -	int ret;
> -
> -	ctrl = v4l2_ctrl_find(&ctx->ctrl_hdl,
> -			      V4L2_CID_STATELESS_H264_SPS);
> -	if (!ctrl)
> -		return -EINVAL;
> -
> -	ret = rkvdec_h264_validate_sps(ctx, ctrl->p_new.p_h264_sps);
> -	if (ret)
> -		return ret;
>  
>  	h264_ctx = kzalloc(sizeof(*h264_ctx), GFP_KERNEL);
>  	if (!h264_ctx)
> @@ -1089,8 +1078,8 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
>  	priv_tbl = dma_alloc_coherent(rkvdec->dev, sizeof(*priv_tbl),
>  				      &h264_ctx->priv_tbl.dma, GFP_KERNEL);
>  	if (!priv_tbl) {
> -		ret = -ENOMEM;
> -		goto err_free_ctx;
> +		kfree(h264_ctx);
> +		return -ENOMEM;
>  	}
>  
>  	h264_ctx->priv_tbl.size = sizeof(*priv_tbl);
> @@ -1100,10 +1089,6 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
>  
>  	ctx->priv = h264_ctx;
>  	return 0;
> -
> -err_free_ctx:
> -	kfree(h264_ctx);
> -	return ret;
>  }
>  
>  static void rkvdec_h264_stop(struct rkvdec_ctx *ctx)

Re: [PATCH v4 05/11] media: rkvdec: h264: Remove SPS validation at streaming start

[Jonas Karlman]

On 2023-11-07 23:01, Nicolas Dufresne wrote:
> Le dimanche 05 novembre 2023 à 16:55 +0000, Jonas Karlman a écrit :
>> SPS parameters is validated in try_ctrl() ops so there is no need to
> 
>                  are
> 
>> re-validate when streaming starts.
>>
>> Remove the unnecessary call to validate sps at streaming start.
> 
> This patch is not working since user may change the format after the
> control have been set. The proper fix should actually reset the SPS
> (well all header controls) to match the the newly set format. Then this
> validation won't be needed anymore.
> 
> The sequence that is problematic after this patch is:
> 
> S_FMT (OUTPUT, width, height);
> S_CTRL (SPS) // valid
> S_FMT(OUTPUT, width', height'); // SPS is no longer valid
> 
> One suggestion I may make is to add a ops so that each codec
> implementation can reset their header controls to make it valid against
> the new resolution. With that in place you'll be able drop the check.

According to the Initialization section of the V4L2 stateless
documentation a client is supposed to S_CTRL(SPS) after S_FMT(OUTPUT).

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

I guess that all stateless decoders probably should reset all ctrls to
default value on S_FMT(OUTPUT) or decoders may end up in an unexpected
state?

Is resetting a ctrl value back to default something that is supported by
v4l2 ctrl core? Did not find any obvious way to reset a ctrl value.

Will probably just drop this patch in v5.

Regards,
Jonas

> 
> Nicolas
> 
> p.s. you can also just drop this patch from the series and revisit it
> later, though I think its worth fixing.
> 
>>
>> Suggested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>> v4:
>> - No change
>>
>> v3:
>> - New patch
>>
>>  drivers/staging/media/rkvdec/rkvdec-h264.c | 19 ++-----------------
>>  1 file changed, 2 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
>> index 8bce8902b8dd..815d5359ddd5 100644
>> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
>> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
>> @@ -1070,17 +1070,6 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
>>  	struct rkvdec_dev *rkvdec = ctx->dev;
>>  	struct rkvdec_h264_priv_tbl *priv_tbl;
>>  	struct rkvdec_h264_ctx *h264_ctx;
>> -	struct v4l2_ctrl *ctrl;
>> -	int ret;
>> -
>> -	ctrl = v4l2_ctrl_find(&ctx->ctrl_hdl,
>> -			      V4L2_CID_STATELESS_H264_SPS);
>> -	if (!ctrl)
>> -		return -EINVAL;
>> -
>> -	ret = rkvdec_h264_validate_sps(ctx, ctrl->p_new.p_h264_sps);
>> -	if (ret)
>> -		return ret;
>>  
>>  	h264_ctx = kzalloc(sizeof(*h264_ctx), GFP_KERNEL);
>>  	if (!h264_ctx)
>> @@ -1089,8 +1078,8 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
>>  	priv_tbl = dma_alloc_coherent(rkvdec->dev, sizeof(*priv_tbl),
>>  				      &h264_ctx->priv_tbl.dma, GFP_KERNEL);
>>  	if (!priv_tbl) {
>> -		ret = -ENOMEM;
>> -		goto err_free_ctx;
>> +		kfree(h264_ctx);
>> +		return -ENOMEM;
>>  	}
>>  
>>  	h264_ctx->priv_tbl.size = sizeof(*priv_tbl);
>> @@ -1100,10 +1089,6 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
>>  
>>  	ctx->priv = h264_ctx;
>>  	return 0;
>> -
>> -err_free_ctx:
>> -	kfree(h264_ctx);
>> -	return ret;
>>  }
>>  
>>  static void rkvdec_h264_stop(struct rkvdec_ctx *ctx)
> 

Re: [PATCH v4 05/11] media: rkvdec: h264: Remove SPS validation at streaming start

[Nicolas Dufresne]

Le mardi 07 novembre 2023 à 23:56 +0100, Jonas Karlman a écrit :
> On 2023-11-07 23:01, Nicolas Dufresne wrote:
> > Le dimanche 05 novembre 2023 à 16:55 +0000, Jonas Karlman a écrit :
> > > SPS parameters is validated in try_ctrl() ops so there is no need to
> > 
> >                  are
> > 
> > > re-validate when streaming starts.
> > > 
> > > Remove the unnecessary call to validate sps at streaming start.
> > 
> > This patch is not working since user may change the format after the
> > control have been set. The proper fix should actually reset the SPS
> > (well all header controls) to match the the newly set format. Then this
> > validation won't be needed anymore.
> > 
> > The sequence that is problematic after this patch is:
> > 
> > S_FMT (OUTPUT, width, height);
> > S_CTRL (SPS) // valid
> > S_FMT(OUTPUT, width', height'); // SPS is no longer valid
> > 
> > One suggestion I may make is to add a ops so that each codec
> > implementation can reset their header controls to make it valid against
> > the new resolution. With that in place you'll be able drop the check.
> 
> According to the Initialization section of the V4L2 stateless
> documentation a client is supposed to S_CTRL(SPS) after S_FMT(OUTPUT).

Yes, but other part of the spec prevents us from failing if the
userspace restart in the middle of the process.

> 
> https://docs.kernel.org/userspace-api/media/v4l/dev-stateless-decoder.html#initialization
> 
> I guess that all stateless decoders probably should reset all ctrls to
> default value on S_FMT(OUTPUT) or decoders may end up in an unexpected
> state?
> 
> Is resetting a ctrl value back to default something that is supported by
> v4l2 ctrl core? Did not find any obvious way to reset a ctrl value.

In order to avoid having to do this, Hantro driver just ignores these
values from SPS control and do the following:

	reg = G1_REG_DEC_CTRL1_PIC_MB_WIDTH(MB_WIDTH(ctx->src_fmt.width)) |
	      G1_REG_DEC_CTRL1_PIC_MB_HEIGHT_P(MB_HEIGHT(ctx->src_fmt.height)) |
	      G1_REG_DEC_CTRL1_REF_FRAMES(sps->max_num_ref_frames);

Then all they do is reset the CAPTURE format whenever needed, matching
the bit depth that was previously set. The SPS is unfortunatly not
guarantied to be valid, but at first sight its safe in regard to
picture dimensions.

> 
> Will probably just drop this patch in v5.

That or do like in Hantro driver. What is scary though is that some of
the feature enabled by SPS may requires an auxiliary chunk of memory to
be allocated, and then this method falls appart. I think it would be
nice to fix that properly in all drivers in a future patchset.

> 
> Regards,
> Jonas
> 
> > 
> > Nicolas
> > 
> > p.s. you can also just drop this patch from the series and revisit it
> > later, though I think its worth fixing.
> > 
> > > 
> > > Suggested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > > ---
> > > v4:
> > > - No change
> > > 
> > > v3:
> > > - New patch
> > > 
> > >  drivers/staging/media/rkvdec/rkvdec-h264.c | 19 ++-----------------
> > >  1 file changed, 2 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > > index 8bce8902b8dd..815d5359ddd5 100644
> > > --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> > > +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> > > @@ -1070,17 +1070,6 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
> > >  	struct rkvdec_dev *rkvdec = ctx->dev;
> > >  	struct rkvdec_h264_priv_tbl *priv_tbl;
> > >  	struct rkvdec_h264_ctx *h264_ctx;
> > > -	struct v4l2_ctrl *ctrl;
> > > -	int ret;
> > > -
> > > -	ctrl = v4l2_ctrl_find(&ctx->ctrl_hdl,
> > > -			      V4L2_CID_STATELESS_H264_SPS);
> > > -	if (!ctrl)
> > > -		return -EINVAL;
> > > -
> > > -	ret = rkvdec_h264_validate_sps(ctx, ctrl->p_new.p_h264_sps);
> > > -	if (ret)
> > > -		return ret;
> > >  
> > >  	h264_ctx = kzalloc(sizeof(*h264_ctx), GFP_KERNEL);
> > >  	if (!h264_ctx)
> > > @@ -1089,8 +1078,8 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
> > >  	priv_tbl = dma_alloc_coherent(rkvdec->dev, sizeof(*priv_tbl),
> > >  				      &h264_ctx->priv_tbl.dma, GFP_KERNEL);
> > >  	if (!priv_tbl) {
> > > -		ret = -ENOMEM;
> > > -		goto err_free_ctx;
> > > +		kfree(h264_ctx);
> > > +		return -ENOMEM;
> > >  	}
> > >  
> > >  	h264_ctx->priv_tbl.size = sizeof(*priv_tbl);
> > > @@ -1100,10 +1089,6 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
> > >  
> > >  	ctx->priv = h264_ctx;
> > >  	return 0;
> > > -
> > > -err_free_ctx:
> > > -	kfree(h264_ctx);
> > > -	return ret;
> > >  }
> > >  
> > >  static void rkvdec_h264_stop(struct rkvdec_ctx *ctx)
> > 
> 

Re: [PATCH v4 05/11] media: rkvdec: h264: Remove SPS validation at streaming start

[Jonas Karlman]

On 2023-11-08 03:39, Nicolas Dufresne wrote:
> Le mardi 07 novembre 2023 à 23:56 +0100, Jonas Karlman a écrit :
>> On 2023-11-07 23:01, Nicolas Dufresne wrote:
>>> Le dimanche 05 novembre 2023 à 16:55 +0000, Jonas Karlman a écrit :
>>>> SPS parameters is validated in try_ctrl() ops so there is no need to
>>>
>>>                  are
>>>
>>>> re-validate when streaming starts.
>>>>
>>>> Remove the unnecessary call to validate sps at streaming start.
>>>
>>> This patch is not working since user may change the format after the
>>> control have been set. The proper fix should actually reset the SPS
>>> (well all header controls) to match the the newly set format. Then this
>>> validation won't be needed anymore.
>>>
>>> The sequence that is problematic after this patch is:
>>>
>>> S_FMT (OUTPUT, width, height);
>>> S_CTRL (SPS) // valid
>>> S_FMT(OUTPUT, width', height'); // SPS is no longer valid
>>>
>>> One suggestion I may make is to add a ops so that each codec
>>> implementation can reset their header controls to make it valid against
>>> the new resolution. With that in place you'll be able drop the check.
>>
>> According to the Initialization section of the V4L2 stateless
>> documentation a client is supposed to S_CTRL(SPS) after S_FMT(OUTPUT).
> 
> Yes, but other part of the spec prevents us from failing if the
> userspace restart in the middle of the process.
> 
>>
>> https://docs.kernel.org/userspace-api/media/v4l/dev-stateless-decoder.html#initialization
>>
>> I guess that all stateless decoders probably should reset all ctrls to
>> default value on S_FMT(OUTPUT) or decoders may end up in an unexpected
>> state?
>>
>> Is resetting a ctrl value back to default something that is supported by
>> v4l2 ctrl core? Did not find any obvious way to reset a ctrl value.
> 
> In order to avoid having to do this, Hantro driver just ignores these
> values from SPS control and do the following:
> 
> 	reg = G1_REG_DEC_CTRL1_PIC_MB_WIDTH(MB_WIDTH(ctx->src_fmt.width)) |
> 	      G1_REG_DEC_CTRL1_PIC_MB_HEIGHT_P(MB_HEIGHT(ctx->src_fmt.height)) |
> 	      G1_REG_DEC_CTRL1_REF_FRAMES(sps->max_num_ref_frames);
> 
> Then all they do is reset the CAPTURE format whenever needed, matching
> the bit depth that was previously set. The SPS is unfortunatly not
> guarantied to be valid, but at first sight its safe in regard to
> picture dimensions.

The commit 77e74be83083 ("media: rkvdec: h264: Validate and use pic
width and height in mbs") changed to use the SPS values to help fix
decoding of field encoded content, it also added this check.

Will drop this patch in v5, and should also re-add similar validation in
the HEVC series.

Regards,
Jonas

> 
>>
>> Will probably just drop this patch in v5.
> 
> That or do like in Hantro driver. What is scary though is that some of
> the feature enabled by SPS may requires an auxiliary chunk of memory to
> be allocated, and then this method falls appart. I think it would be
> nice to fix that properly in all drivers in a future patchset.
> 
>>
>> Regards,
>> Jonas
>>
>>>
>>> Nicolas
>>>
>>> p.s. you can also just drop this patch from the series and revisit it
>>> later, though I think its worth fixing.
>>>
>>>>
>>>> Suggested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>>> ---
>>>> v4:
>>>> - No change
>>>>
>>>> v3:
>>>> - New patch
>>>>
>>>>  drivers/staging/media/rkvdec/rkvdec-h264.c | 19 ++-----------------
>>>>  1 file changed, 2 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
>>>> index 8bce8902b8dd..815d5359ddd5 100644
>>>> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
>>>> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
>>>> @@ -1070,17 +1070,6 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
>>>>  	struct rkvdec_dev *rkvdec = ctx->dev;
>>>>  	struct rkvdec_h264_priv_tbl *priv_tbl;
>>>>  	struct rkvdec_h264_ctx *h264_ctx;
>>>> -	struct v4l2_ctrl *ctrl;
>>>> -	int ret;
>>>> -
>>>> -	ctrl = v4l2_ctrl_find(&ctx->ctrl_hdl,
>>>> -			      V4L2_CID_STATELESS_H264_SPS);
>>>> -	if (!ctrl)
>>>> -		return -EINVAL;
>>>> -
>>>> -	ret = rkvdec_h264_validate_sps(ctx, ctrl->p_new.p_h264_sps);
>>>> -	if (ret)
>>>> -		return ret;
>>>>  
>>>>  	h264_ctx = kzalloc(sizeof(*h264_ctx), GFP_KERNEL);
>>>>  	if (!h264_ctx)
>>>> @@ -1089,8 +1078,8 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
>>>>  	priv_tbl = dma_alloc_coherent(rkvdec->dev, sizeof(*priv_tbl),
>>>>  				      &h264_ctx->priv_tbl.dma, GFP_KERNEL);
>>>>  	if (!priv_tbl) {
>>>> -		ret = -ENOMEM;
>>>> -		goto err_free_ctx;
>>>> +		kfree(h264_ctx);
>>>> +		return -ENOMEM;
>>>>  	}
>>>>  
>>>>  	h264_ctx->priv_tbl.size = sizeof(*priv_tbl);
>>>> @@ -1100,10 +1089,6 @@ static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
>>>>  
>>>>  	ctx->priv = h264_ctx;
>>>>  	return 0;
>>>> -
>>>> -err_free_ctx:
>>>> -	kfree(h264_ctx);
>>>> -	return ret;
>>>>  }
>>>>  
>>>>  static void rkvdec_h264_stop(struct rkvdec_ctx *ctx)
>>>
>>
> 

[PATCH v4 06/11] media: rkvdec: Extract rkvdec_fill_decoded_pixfmt into helper

[Jonas Karlman]

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

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v4:
- Do not reset pix_mp->field in rkvdec_fill_decoded_pixfmt()

v3:
- No changes

 drivers/staging/media/rkvdec/rkvdec.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

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

Re: [PATCH v4 06/11] media: rkvdec: Extract rkvdec_fill_decoded_pixfmt into helper

[Nicolas Dufresne]

Le dimanche 05 novembre 2023 à 16:55 +0000, Jonas Karlman a écrit :
> Extract call to v4l2_fill_pixfmt_mp() and ajusting of sizeimage into a
> helper. Replace current code with a call to the new helper.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

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

[PATCH v4 07/11] media: rkvdec: Move rkvdec_reset_decoded_fmt helper

[Jonas Karlman]

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

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v4:
- No change

v3:
- New patch

 drivers/staging/media/rkvdec/rkvdec.c | 46 +++++++++++++--------------
 1 file changed, 23 insertions(+), 23 deletions(-)

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

Re: [PATCH v4 07/11] media: rkvdec: Move rkvdec_reset_decoded_fmt helper

[Nicolas Dufresne]

Le dimanche 05 novembre 2023 à 16:55 +0000, Jonas Karlman a écrit :
> Move rkvdec_reset_decoded_fmt() and the called rkvdec_reset_fmt() helper
> functions in preparation for adding a new caller in an upcoming patch.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

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

[PATCH v4 08/11] media: rkvdec: Extract decoded format enumeration into helper

[Jonas Karlman]

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

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

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v4:
- Rename rkvdec_decoded_fmts() to rkvdec_enum_decoded_fmt()
- Rename rkvdec_valid_fmt() to rkvdec_is_valid_fmt()

v3:
- New patch

 drivers/staging/media/rkvdec/rkvdec.c | 49 +++++++++++++++++++--------
 1 file changed, 35 insertions(+), 14 deletions(-)

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

Re: [PATCH v4 08/11] media: rkvdec: Extract decoded format enumeration into helper

[Nicolas Dufresne]

Le dimanche 05 novembre 2023 à 16:55 +0000, Jonas Karlman a écrit :
> Add a rkvdec_is_valid_fmt() helper that check if a fourcc is a supported
> CAPTURE format, and a rkvdec_enum_decoded_fmt() helper that enumerate
                                                              enumerates
> valid formats.
> 
> This move current code into helper functions in preparation for adding
       moves
> CAPTURE format filtering and validation in next patch.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

With the fixed,
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

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

[PATCH v4 09/11] media: rkvdec: Add image format concept

[Jonas Karlman]

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

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

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v4:
- Change fmt_opaque into an image format
- Split patch into two

v3:
- New patch

 drivers/staging/media/rkvdec/rkvdec.c | 45 ++++++++++++++++++++-------
 drivers/staging/media/rkvdec/rkvdec.h | 13 +++++++-
 2 files changed, 45 insertions(+), 13 deletions(-)

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

[PATCH v4 10/11] media: rkvdec: Add get_image_fmt ops

[Jonas Karlman]

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

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

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v4:
- Change fmt_opaque into an image format
- Split patch into two

v3:
- New patch

 drivers/staging/media/rkvdec/rkvdec.c | 49 +++++++++++++++++++++++++--
 drivers/staging/media/rkvdec/rkvdec.h |  2 ++
 2 files changed, 49 insertions(+), 2 deletions(-)

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

[PATCH v4 11/11] media: rkvdec: h264: Support High 10 and 4:2:2 profiles

[Jonas Karlman]

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

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

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

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

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
v4:
- Change to use get_image_fmt() ops

v3:
- Add get_fmt_opaque ops, the expected pixelformat is used as opaque
- Add new valid_fmt ops that validate pixelformat matches opaque
- Update H264_PROFILE control max value

 drivers/staging/media/rkvdec/rkvdec-h264.c | 37 ++++++++++++++++------
 drivers/staging/media/rkvdec/rkvdec.c      | 33 +++++++++++++++----
 drivers/staging/media/rkvdec/rkvdec.h      |  3 ++
 3 files changed, 57 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 815d5359ddd5..baac6d012ddd 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -1027,24 +1027,42 @@ static int rkvdec_h264_adjust_fmt(struct rkvdec_ctx *ctx,
 	return 0;
 }
 
+static enum rkvdec_image_fmt rkvdec_h264_get_image_fmt(struct rkvdec_ctx *ctx,
+						       struct v4l2_ctrl *ctrl)
+{
+	const struct v4l2_ctrl_h264_sps *sps = ctrl->p_new.p_h264_sps;
+
+	if (ctrl->id != V4L2_CID_STATELESS_H264_SPS)
+		return RKVDEC_IMG_FMT_ANY;
+
+	if (sps->bit_depth_luma_minus8 == 0) {
+		if (sps->chroma_format_idc == 2)
+			return RKVDEC_IMG_FMT_422_8BIT;
+		else
+			return RKVDEC_IMG_FMT_420_8BIT;
+	} else if (sps->bit_depth_luma_minus8 == 2) {
+		if (sps->chroma_format_idc == 2)
+			return RKVDEC_IMG_FMT_422_10BIT;
+		else
+			return RKVDEC_IMG_FMT_420_10BIT;
+	}
+
+	return RKVDEC_IMG_FMT_ANY;
+}
+
 static int rkvdec_h264_validate_sps(struct rkvdec_ctx *ctx,
 				    const struct v4l2_ctrl_h264_sps *sps)
 {
 	unsigned int width, height;
 
-	/*
-	 * TODO: The hardware supports 10-bit and 4:2:2 profiles,
-	 * but it's currently broken in the driver.
-	 * Reject them for now, until it's fixed.
-	 */
-	if (sps->chroma_format_idc > 1)
-		/* Only 4:0:0 and 4:2:0 are supported */
+	if (sps->chroma_format_idc > 2)
+		/* Only 4:0:0, 4:2:0 and 4:2:2 are supported */
 		return -EINVAL;
 	if (sps->bit_depth_luma_minus8 != sps->bit_depth_chroma_minus8)
 		/* Luma and chroma bit depth mismatch */
 		return -EINVAL;
-	if (sps->bit_depth_luma_minus8 != 0)
-		/* Only 8-bit is supported */
+	if (sps->bit_depth_luma_minus8 != 0 && sps->bit_depth_luma_minus8 != 2)
+		/* Only 8-bit and 10-bit is supported */
 		return -EINVAL;
 
 	width = (sps->pic_width_in_mbs_minus1 + 1) * 16;
@@ -1175,4 +1193,5 @@ const struct rkvdec_coded_fmt_ops rkvdec_h264_fmt_ops = {
 	.stop = rkvdec_h264_stop,
 	.run = rkvdec_h264_run,
 	.try_ctrl = rkvdec_h264_try_ctrl,
+	.get_image_fmt = rkvdec_h264_get_image_fmt,
 };
diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 225aa1f0ac48..eb59605ccf28 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -73,7 +73,7 @@ static void rkvdec_fill_decoded_pixfmt(struct rkvdec_ctx *ctx,
 				       struct v4l2_pix_format_mplane *pix_mp)
 {
 	v4l2_fill_pixfmt_mp(pix_mp, pix_mp->pixelformat,
-			    pix_mp->width, pix_mp->height);
+			    ALIGN(pix_mp->width, 64), pix_mp->height);
 	pix_mp->plane_fmt[0].sizeimage += 128 *
 		DIV_ROUND_UP(pix_mp->width, 16) *
 		DIV_ROUND_UP(pix_mp->height, 16);
@@ -193,7 +193,7 @@ static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
 	{
 		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_PROFILE,
 		.cfg.min = V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE,
-		.cfg.max = V4L2_MPEG_VIDEO_H264_PROFILE_HIGH,
+		.cfg.max = V4L2_MPEG_VIDEO_H264_PROFILE_HIGH_422,
 		.cfg.menu_skip_mask =
 			BIT(V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED),
 		.cfg.def = V4L2_MPEG_VIDEO_H264_PROFILE_MAIN,
@@ -210,11 +210,23 @@ static const struct rkvdec_ctrls rkvdec_h264_ctrls = {
 	.num_ctrls = ARRAY_SIZE(rkvdec_h264_ctrl_descs),
 };
 
-static const struct rkvdec_decoded_fmt_desc rkvdec_h264_vp9_decoded_fmts[] = {
+static const struct rkvdec_decoded_fmt_desc rkvdec_h264_decoded_fmts[] = {
 	{
 		.fourcc = V4L2_PIX_FMT_NV12,
 		.image_fmt = RKVDEC_IMG_FMT_420_8BIT,
 	},
+	{
+		.fourcc = V4L2_PIX_FMT_NV15,
+		.image_fmt = RKVDEC_IMG_FMT_420_10BIT,
+	},
+	{
+		.fourcc = V4L2_PIX_FMT_NV16,
+		.image_fmt = RKVDEC_IMG_FMT_422_8BIT,
+	},
+	{
+		.fourcc = V4L2_PIX_FMT_NV20,
+		.image_fmt = RKVDEC_IMG_FMT_422_10BIT,
+	},
 };
 
 static const struct rkvdec_ctrl_desc rkvdec_vp9_ctrl_descs[] = {
@@ -237,6 +249,13 @@ static const struct rkvdec_ctrls rkvdec_vp9_ctrls = {
 	.num_ctrls = ARRAY_SIZE(rkvdec_vp9_ctrl_descs),
 };
 
+static const struct rkvdec_decoded_fmt_desc rkvdec_vp9_decoded_fmts[] = {
+	{
+		.fourcc = V4L2_PIX_FMT_NV12,
+		.image_fmt = RKVDEC_IMG_FMT_420_8BIT,
+	},
+};
+
 static const struct rkvdec_coded_fmt_desc rkvdec_coded_fmts[] = {
 	{
 		.fourcc = V4L2_PIX_FMT_H264_SLICE,
@@ -250,8 +269,8 @@ static const struct rkvdec_coded_fmt_desc rkvdec_coded_fmts[] = {
 		},
 		.ctrls = &rkvdec_h264_ctrls,
 		.ops = &rkvdec_h264_fmt_ops,
-		.num_decoded_fmts = ARRAY_SIZE(rkvdec_h264_vp9_decoded_fmts),
-		.decoded_fmts = rkvdec_h264_vp9_decoded_fmts,
+		.num_decoded_fmts = ARRAY_SIZE(rkvdec_h264_decoded_fmts),
+		.decoded_fmts = rkvdec_h264_decoded_fmts,
 		.subsystem_flags = VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF,
 	},
 	{
@@ -266,8 +285,8 @@ static const struct rkvdec_coded_fmt_desc rkvdec_coded_fmts[] = {
 		},
 		.ctrls = &rkvdec_vp9_ctrls,
 		.ops = &rkvdec_vp9_fmt_ops,
-		.num_decoded_fmts = ARRAY_SIZE(rkvdec_h264_vp9_decoded_fmts),
-		.decoded_fmts = rkvdec_h264_vp9_decoded_fmts,
+		.num_decoded_fmts = ARRAY_SIZE(rkvdec_vp9_decoded_fmts),
+		.decoded_fmts = rkvdec_vp9_decoded_fmts,
 	}
 };
 
diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
index e466a2753ccf..9a9f4fced7a1 100644
--- a/drivers/staging/media/rkvdec/rkvdec.h
+++ b/drivers/staging/media/rkvdec/rkvdec.h
@@ -80,6 +80,9 @@ struct rkvdec_coded_fmt_ops {
 enum rkvdec_image_fmt {
 	RKVDEC_IMG_FMT_ANY = 0,
 	RKVDEC_IMG_FMT_420_8BIT,
+	RKVDEC_IMG_FMT_420_10BIT,
+	RKVDEC_IMG_FMT_422_8BIT,
+	RKVDEC_IMG_FMT_422_10BIT,
 };
 
 struct rkvdec_decoded_fmt_desc {
-- 
2.42.0

Re: [PATCH v4 11/11] media: rkvdec: h264: Support High 10 and 4:2:2 profiles

[Nicolas Dufresne]

Le dimanche 05 novembre 2023 à 16:55 +0000, Jonas Karlman a écrit :
> Add support and enable decoding of H264 High 10 and 4:2:2 profiles.
> 
> Decoded CAPTURE buffer width is aligned to 64 pixels to accommodate HW
> requirement of 10-bit format buffers, fixes decoding of:
> 
> - Hi422FR13_SONY_A
> - Hi422FR14_SONY_A
> - Hi422FR15_SONY_A
> - Hi422FR6_SONY_A
> - Hi422FR7_SONY_A
> - Hi422FR8_SONY_A
> - Hi422FR9_SONY_A
> - Hi422FREXT18_SONY_A
> 
> The get_image_fmt() ops is implemented to select an image format
> required for the provided SPS control.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
> v4:
> - Change to use get_image_fmt() ops
> 
> v3:
> - Add get_fmt_opaque ops, the expected pixelformat is used as opaque
> - Add new valid_fmt ops that validate pixelformat matches opaque
> - Update H264_PROFILE control max value
> 
>  drivers/staging/media/rkvdec/rkvdec-h264.c | 37 ++++++++++++++++------
>  drivers/staging/media/rkvdec/rkvdec.c      | 33 +++++++++++++++----
>  drivers/staging/media/rkvdec/rkvdec.h      |  3 ++
>  3 files changed, 57 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index 815d5359ddd5..baac6d012ddd 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -1027,24 +1027,42 @@ static int rkvdec_h264_adjust_fmt(struct rkvdec_ctx *ctx,
>  	return 0;
>  }
>  
> +static enum rkvdec_image_fmt rkvdec_h264_get_image_fmt(struct rkvdec_ctx *ctx,
> +						       struct v4l2_ctrl *ctrl)
> +{
> +	const struct v4l2_ctrl_h264_sps *sps = ctrl->p_new.p_h264_sps;
> +
> +	if (ctrl->id != V4L2_CID_STATELESS_H264_SPS)
> +		return RKVDEC_IMG_FMT_ANY;
> +
> +	if (sps->bit_depth_luma_minus8 == 0) {
> +		if (sps->chroma_format_idc == 2)
> +			return RKVDEC_IMG_FMT_422_8BIT;
> +		else
> +			return RKVDEC_IMG_FMT_420_8BIT;
> +	} else if (sps->bit_depth_luma_minus8 == 2) {
> +		if (sps->chroma_format_idc == 2)
> +			return RKVDEC_IMG_FMT_422_10BIT;
> +		else
> +			return RKVDEC_IMG_FMT_420_10BIT;
> +	}
> +
> +	return RKVDEC_IMG_FMT_ANY;
> +}
> +
>  static int rkvdec_h264_validate_sps(struct rkvdec_ctx *ctx,
>  				    const struct v4l2_ctrl_h264_sps *sps)
>  {
>  	unsigned int width, height;
>  
> -	/*
> -	 * TODO: The hardware supports 10-bit and 4:2:2 profiles,
> -	 * but it's currently broken in the driver.
> -	 * Reject them for now, until it's fixed.
> -	 */
> -	if (sps->chroma_format_idc > 1)
> -		/* Only 4:0:0 and 4:2:0 are supported */
> +	if (sps->chroma_format_idc > 2)
> +		/* Only 4:0:0, 4:2:0 and 4:2:2 are supported */
>  		return -EINVAL;
>  	if (sps->bit_depth_luma_minus8 != sps->bit_depth_chroma_minus8)
>  		/* Luma and chroma bit depth mismatch */
>  		return -EINVAL;
> -	if (sps->bit_depth_luma_minus8 != 0)
> -		/* Only 8-bit is supported */
> +	if (sps->bit_depth_luma_minus8 != 0 && sps->bit_depth_luma_minus8 != 2)
> +		/* Only 8-bit and 10-bit is supported */
>  		return -EINVAL;
>  
>  	width = (sps->pic_width_in_mbs_minus1 + 1) * 16;
> @@ -1175,4 +1193,5 @@ const struct rkvdec_coded_fmt_ops rkvdec_h264_fmt_ops = {
>  	.stop = rkvdec_h264_stop,
>  	.run = rkvdec_h264_run,
>  	.try_ctrl = rkvdec_h264_try_ctrl,
> +	.get_image_fmt = rkvdec_h264_get_image_fmt,
>  };
> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> index 225aa1f0ac48..eb59605ccf28 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.c
> +++ b/drivers/staging/media/rkvdec/rkvdec.c
> @@ -73,7 +73,7 @@ static void rkvdec_fill_decoded_pixfmt(struct rkvdec_ctx *ctx,
>  				       struct v4l2_pix_format_mplane *pix_mp)
>  {
>  	v4l2_fill_pixfmt_mp(pix_mp, pix_mp->pixelformat,
> -			    pix_mp->width, pix_mp->height);
> +			    ALIGN(pix_mp->width, 64), pix_mp->height);

If you align regardless if its 8/10bit (which I don't really mind, it
does not cost much and helps if you have a Mali GPU), please do in 
rkvdec_coded_fmts[].frmsize.step_width.

Otherwise you'll endup creating a config per bit depth, and probably
won't need any of the rk format stuff, since you could just update the
config, and enumerate from there. I don't mind your method though, but
lets not hardcode alignment where it shouldn't be.

>  	pix_mp->plane_fmt[0].sizeimage += 128 *
>  		DIV_ROUND_UP(pix_mp->width, 16) *
>  		DIV_ROUND_UP(pix_mp->height, 16);
> @@ -193,7 +193,7 @@ static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
>  	{
>  		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_PROFILE,
>  		.cfg.min = V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE,

Do we want to keep this small lie ? Baseline is not supported as we
don't support FMO and ASO. That being said, in framework like
GStreamer, we try to decode anyway, cause we know we don't have a
software fallback anyway.

> -		.cfg.max = V4L2_MPEG_VIDEO_H264_PROFILE_HIGH,
> +		.cfg.max = V4L2_MPEG_VIDEO_H264_PROFILE_HIGH_422,

Should include V4L2_MPEG_VIDEO_H264_PROFILE_HIGH_10_INTRA and
V4L2_MPEG_VIDEO_H264_PROFILE_HIGH_422_INTRA too ....

>  		.cfg.menu_skip_mask =
>  			BIT(V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED),

Which requires substracting
V4L2_MPEG_VIDEO_H264_PROFILE_HIGH_444_PREDICTIVE too.

>  		.cfg.def = V4L2_MPEG_VIDEO_H264_PROFILE_MAIN,
> @@ -210,11 +210,23 @@ static const struct rkvdec_ctrls rkvdec_h264_ctrls = {
>  	.num_ctrls = ARRAY_SIZE(rkvdec_h264_ctrl_descs),
>  };
>  
> -static const struct rkvdec_decoded_fmt_desc rkvdec_h264_vp9_decoded_fmts[] = {
> +static const struct rkvdec_decoded_fmt_desc rkvdec_h264_decoded_fmts[] = {
>  	{
>  		.fourcc = V4L2_PIX_FMT_NV12,
>  		.image_fmt = RKVDEC_IMG_FMT_420_8BIT,
>  	},
> +	{
> +		.fourcc = V4L2_PIX_FMT_NV15,
> +		.image_fmt = RKVDEC_IMG_FMT_420_10BIT,
> +	},
> +	{
> +		.fourcc = V4L2_PIX_FMT_NV16,
> +		.image_fmt = RKVDEC_IMG_FMT_422_8BIT,
> +	},
> +	{
> +		.fourcc = V4L2_PIX_FMT_NV20,
> +		.image_fmt = RKVDEC_IMG_FMT_422_10BIT,
> +	},
>  };
>  
>  static const struct rkvdec_ctrl_desc rkvdec_vp9_ctrl_descs[] = {
> @@ -237,6 +249,13 @@ static const struct rkvdec_ctrls rkvdec_vp9_ctrls = {
>  	.num_ctrls = ARRAY_SIZE(rkvdec_vp9_ctrl_descs),
>  };
>  
> +static const struct rkvdec_decoded_fmt_desc rkvdec_vp9_decoded_fmts[] = {
> +	{
> +		.fourcc = V4L2_PIX_FMT_NV12,
> +		.image_fmt = RKVDEC_IMG_FMT_420_8BIT,
> +	},
> +};
> +
>  static const struct rkvdec_coded_fmt_desc rkvdec_coded_fmts[] = {
>  	{
>  		.fourcc = V4L2_PIX_FMT_H264_SLICE,
> @@ -250,8 +269,8 @@ static const struct rkvdec_coded_fmt_desc rkvdec_coded_fmts[] = {
>  		},
>  		.ctrls = &rkvdec_h264_ctrls,
>  		.ops = &rkvdec_h264_fmt_ops,
> -		.num_decoded_fmts = ARRAY_SIZE(rkvdec_h264_vp9_decoded_fmts),
> -		.decoded_fmts = rkvdec_h264_vp9_decoded_fmts,
> +		.num_decoded_fmts = ARRAY_SIZE(rkvdec_h264_decoded_fmts),
> +		.decoded_fmts = rkvdec_h264_decoded_fmts,
>  		.subsystem_flags = VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF,
>  	},
>  	{
> @@ -266,8 +285,8 @@ static const struct rkvdec_coded_fmt_desc rkvdec_coded_fmts[] = {
>  		},
>  		.ctrls = &rkvdec_vp9_ctrls,
>  		.ops = &rkvdec_vp9_fmt_ops,
> -		.num_decoded_fmts = ARRAY_SIZE(rkvdec_h264_vp9_decoded_fmts),
> -		.decoded_fmts = rkvdec_h264_vp9_decoded_fmts,
> +		.num_decoded_fmts = ARRAY_SIZE(rkvdec_vp9_decoded_fmts),
> +		.decoded_fmts = rkvdec_vp9_decoded_fmts,
>  	}
>  };
>  
> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
> index e466a2753ccf..9a9f4fced7a1 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.h
> +++ b/drivers/staging/media/rkvdec/rkvdec.h
> @@ -80,6 +80,9 @@ struct rkvdec_coded_fmt_ops {
>  enum rkvdec_image_fmt {
>  	RKVDEC_IMG_FMT_ANY = 0,
>  	RKVDEC_IMG_FMT_420_8BIT,
> +	RKVDEC_IMG_FMT_420_10BIT,
> +	RKVDEC_IMG_FMT_422_8BIT,
> +	RKVDEC_IMG_FMT_422_10BIT,
>  };
>  
>  struct rkvdec_decoded_fmt_desc {

Re: [PATCH v4 11/11] media: rkvdec: h264: Support High 10 and 4:2:2 profiles

[Jonas Karlman]

On 2023-11-08 03:20, Nicolas Dufresne wrote:
> Le dimanche 05 novembre 2023 à 16:55 +0000, Jonas Karlman a écrit :
>> Add support and enable decoding of H264 High 10 and 4:2:2 profiles.
>>
>> Decoded CAPTURE buffer width is aligned to 64 pixels to accommodate HW
>> requirement of 10-bit format buffers, fixes decoding of:
>>
>> - Hi422FR13_SONY_A
>> - Hi422FR14_SONY_A
>> - Hi422FR15_SONY_A
>> - Hi422FR6_SONY_A
>> - Hi422FR7_SONY_A
>> - Hi422FR8_SONY_A
>> - Hi422FR9_SONY_A
>> - Hi422FREXT18_SONY_A
>>
>> The get_image_fmt() ops is implemented to select an image format
>> required for the provided SPS control.
>>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>> v4:
>> - Change to use get_image_fmt() ops
>>
>> v3:
>> - Add get_fmt_opaque ops, the expected pixelformat is used as opaque
>> - Add new valid_fmt ops that validate pixelformat matches opaque
>> - Update H264_PROFILE control max value
>>
>>  drivers/staging/media/rkvdec/rkvdec-h264.c | 37 ++++++++++++++++------
>>  drivers/staging/media/rkvdec/rkvdec.c      | 33 +++++++++++++++----
>>  drivers/staging/media/rkvdec/rkvdec.h      |  3 ++
>>  3 files changed, 57 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
>> index 815d5359ddd5..baac6d012ddd 100644
>> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
>> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
>> @@ -1027,24 +1027,42 @@ static int rkvdec_h264_adjust_fmt(struct rkvdec_ctx *ctx,
>>  	return 0;
>>  }
>>  
>> +static enum rkvdec_image_fmt rkvdec_h264_get_image_fmt(struct rkvdec_ctx *ctx,
>> +						       struct v4l2_ctrl *ctrl)
>> +{
>> +	const struct v4l2_ctrl_h264_sps *sps = ctrl->p_new.p_h264_sps;
>> +
>> +	if (ctrl->id != V4L2_CID_STATELESS_H264_SPS)
>> +		return RKVDEC_IMG_FMT_ANY;
>> +
>> +	if (sps->bit_depth_luma_minus8 == 0) {
>> +		if (sps->chroma_format_idc == 2)
>> +			return RKVDEC_IMG_FMT_422_8BIT;
>> +		else
>> +			return RKVDEC_IMG_FMT_420_8BIT;
>> +	} else if (sps->bit_depth_luma_minus8 == 2) {
>> +		if (sps->chroma_format_idc == 2)
>> +			return RKVDEC_IMG_FMT_422_10BIT;
>> +		else
>> +			return RKVDEC_IMG_FMT_420_10BIT;
>> +	}
>> +
>> +	return RKVDEC_IMG_FMT_ANY;
>> +}
>> +
>>  static int rkvdec_h264_validate_sps(struct rkvdec_ctx *ctx,
>>  				    const struct v4l2_ctrl_h264_sps *sps)
>>  {
>>  	unsigned int width, height;
>>  
>> -	/*
>> -	 * TODO: The hardware supports 10-bit and 4:2:2 profiles,
>> -	 * but it's currently broken in the driver.
>> -	 * Reject them for now, until it's fixed.
>> -	 */
>> -	if (sps->chroma_format_idc > 1)
>> -		/* Only 4:0:0 and 4:2:0 are supported */
>> +	if (sps->chroma_format_idc > 2)
>> +		/* Only 4:0:0, 4:2:0 and 4:2:2 are supported */
>>  		return -EINVAL;
>>  	if (sps->bit_depth_luma_minus8 != sps->bit_depth_chroma_minus8)
>>  		/* Luma and chroma bit depth mismatch */
>>  		return -EINVAL;
>> -	if (sps->bit_depth_luma_minus8 != 0)
>> -		/* Only 8-bit is supported */
>> +	if (sps->bit_depth_luma_minus8 != 0 && sps->bit_depth_luma_minus8 != 2)
>> +		/* Only 8-bit and 10-bit is supported */
>>  		return -EINVAL;
>>  
>>  	width = (sps->pic_width_in_mbs_minus1 + 1) * 16;
>> @@ -1175,4 +1193,5 @@ const struct rkvdec_coded_fmt_ops rkvdec_h264_fmt_ops = {
>>  	.stop = rkvdec_h264_stop,
>>  	.run = rkvdec_h264_run,
>>  	.try_ctrl = rkvdec_h264_try_ctrl,
>> +	.get_image_fmt = rkvdec_h264_get_image_fmt,
>>  };
>> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
>> index 225aa1f0ac48..eb59605ccf28 100644
>> --- a/drivers/staging/media/rkvdec/rkvdec.c
>> +++ b/drivers/staging/media/rkvdec/rkvdec.c
>> @@ -73,7 +73,7 @@ static void rkvdec_fill_decoded_pixfmt(struct rkvdec_ctx *ctx,
>>  				       struct v4l2_pix_format_mplane *pix_mp)
>>  {
>>  	v4l2_fill_pixfmt_mp(pix_mp, pix_mp->pixelformat,
>> -			    pix_mp->width, pix_mp->height);
>> +			    ALIGN(pix_mp->width, 64), pix_mp->height);
> 
> If you align regardless if its 8/10bit (which I don't really mind, it
> does not cost much and helps if you have a Mali GPU), please do in 
> rkvdec_coded_fmts[].frmsize.step_width.
> 
> Otherwise you'll endup creating a config per bit depth, and probably
> won't need any of the rk format stuff, since you could just update the
> config, and enumerate from there. I don't mind your method though, but
> lets not hardcode alignment where it shouldn't be.

Agree, will change to use step_width/min_width = 64 in v5, not sure why
I changed from using step_width in v2 to begin with:

"
Changes in v2:
- Only align decoded buffer instead of using frmsize step_width
"

https://lore.kernel.org/linux-media/20200706215430.22859-12-jonas@kwiboo.se/

> 
>>  	pix_mp->plane_fmt[0].sizeimage += 128 *
>>  		DIV_ROUND_UP(pix_mp->width, 16) *
>>  		DIV_ROUND_UP(pix_mp->height, 16);
>> @@ -193,7 +193,7 @@ static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
>>  	{
>>  		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_PROFILE,
>>  		.cfg.min = V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE,
> 
> Do we want to keep this small lie ? Baseline is not supported as we
> don't support FMO and ASO. That being said, in framework like
> GStreamer, we try to decode anyway, cause we know we don't have a
> software fallback anyway.

Found a note to change this to H264_PROFILE_CONSTRAINED_BASELINE from
the old v2 series. Will include such change in v5.

https://patchwork.linuxtv.org/project/linux-media/patch/20200706215430.22859-13-jonas@kwiboo.se/#119406

> 
>> -		.cfg.max = V4L2_MPEG_VIDEO_H264_PROFILE_HIGH,
>> +		.cfg.max = V4L2_MPEG_VIDEO_H264_PROFILE_HIGH_422,
> 
> Should include V4L2_MPEG_VIDEO_H264_PROFILE_HIGH_10_INTRA and
> V4L2_MPEG_VIDEO_H264_PROFILE_HIGH_422_INTRA too ....
> 
>>  		.cfg.menu_skip_mask =
>>  			BIT(V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED),
> 
> Which requires substracting
> V4L2_MPEG_VIDEO_H264_PROFILE_HIGH_444_PREDICTIVE too.

Will update in v5, thanks.

Regards,
Jonas

> 
>>  		.cfg.def = V4L2_MPEG_VIDEO_H264_PROFILE_MAIN,
>> @@ -210,11 +210,23 @@ static const struct rkvdec_ctrls rkvdec_h264_ctrls = {
>>  	.num_ctrls = ARRAY_SIZE(rkvdec_h264_ctrl_descs),
>>  };
>>  
>> -static const struct rkvdec_decoded_fmt_desc rkvdec_h264_vp9_decoded_fmts[] = {
>> +static const struct rkvdec_decoded_fmt_desc rkvdec_h264_decoded_fmts[] = {
>>  	{
>>  		.fourcc = V4L2_PIX_FMT_NV12,
>>  		.image_fmt = RKVDEC_IMG_FMT_420_8BIT,
>>  	},
>> +	{
>> +		.fourcc = V4L2_PIX_FMT_NV15,
>> +		.image_fmt = RKVDEC_IMG_FMT_420_10BIT,
>> +	},
>> +	{
>> +		.fourcc = V4L2_PIX_FMT_NV16,
>> +		.image_fmt = RKVDEC_IMG_FMT_422_8BIT,
>> +	},
>> +	{
>> +		.fourcc = V4L2_PIX_FMT_NV20,
>> +		.image_fmt = RKVDEC_IMG_FMT_422_10BIT,
>> +	},
>>  };
>>  
>>  static const struct rkvdec_ctrl_desc rkvdec_vp9_ctrl_descs[] = {
>> @@ -237,6 +249,13 @@ static const struct rkvdec_ctrls rkvdec_vp9_ctrls = {
>>  	.num_ctrls = ARRAY_SIZE(rkvdec_vp9_ctrl_descs),
>>  };
>>  
>> +static const struct rkvdec_decoded_fmt_desc rkvdec_vp9_decoded_fmts[] = {
>> +	{
>> +		.fourcc = V4L2_PIX_FMT_NV12,
>> +		.image_fmt = RKVDEC_IMG_FMT_420_8BIT,
>> +	},
>> +};
>> +
>>  static const struct rkvdec_coded_fmt_desc rkvdec_coded_fmts[] = {
>>  	{
>>  		.fourcc = V4L2_PIX_FMT_H264_SLICE,
>> @@ -250,8 +269,8 @@ static const struct rkvdec_coded_fmt_desc rkvdec_coded_fmts[] = {
>>  		},
>>  		.ctrls = &rkvdec_h264_ctrls,
>>  		.ops = &rkvdec_h264_fmt_ops,
>> -		.num_decoded_fmts = ARRAY_SIZE(rkvdec_h264_vp9_decoded_fmts),
>> -		.decoded_fmts = rkvdec_h264_vp9_decoded_fmts,
>> +		.num_decoded_fmts = ARRAY_SIZE(rkvdec_h264_decoded_fmts),
>> +		.decoded_fmts = rkvdec_h264_decoded_fmts,
>>  		.subsystem_flags = VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF,
>>  	},
>>  	{
>> @@ -266,8 +285,8 @@ static const struct rkvdec_coded_fmt_desc rkvdec_coded_fmts[] = {
>>  		},
>>  		.ctrls = &rkvdec_vp9_ctrls,
>>  		.ops = &rkvdec_vp9_fmt_ops,
>> -		.num_decoded_fmts = ARRAY_SIZE(rkvdec_h264_vp9_decoded_fmts),
>> -		.decoded_fmts = rkvdec_h264_vp9_decoded_fmts,
>> +		.num_decoded_fmts = ARRAY_SIZE(rkvdec_vp9_decoded_fmts),
>> +		.decoded_fmts = rkvdec_vp9_decoded_fmts,
>>  	}
>>  };
>>  
>> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
>> index e466a2753ccf..9a9f4fced7a1 100644
>> --- a/drivers/staging/media/rkvdec/rkvdec.h
>> +++ b/drivers/staging/media/rkvdec/rkvdec.h
>> @@ -80,6 +80,9 @@ struct rkvdec_coded_fmt_ops {
>>  enum rkvdec_image_fmt {
>>  	RKVDEC_IMG_FMT_ANY = 0,
>>  	RKVDEC_IMG_FMT_420_8BIT,
>> +	RKVDEC_IMG_FMT_420_10BIT,
>> +	RKVDEC_IMG_FMT_422_8BIT,
>> +	RKVDEC_IMG_FMT_422_10BIT,
>>  };
>>  
>>  struct rkvdec_decoded_fmt_desc {
> 

Re: [PATCH v4 00/11] media: rkvdec: Add H.264 High 10 and 4:2:2 profile support

[Nicolas Dufresne]

Hi Jonas,

thanks for this work.

Le dimanche 05 novembre 2023 à 16:54 +0000, Jonas Karlman a écrit :
> This is a revival of a 3 year old series [1] now that NV15/NV20/NV30 support
> for display driver have landed in mainline tree.
> 
> This series adds H.264 High 10 and 4:2:2 profile support to the Rockchip
> Video Decoder driver.
> 
> Patch 1 adds helpers for calculating plane bytesperline and sizeimage.
> Patch 2 adds two new pixelformats for semi-planer 10-bit 4:2:0/4:2:2 YUV.
> 
> Patch 3 change to use bytesperline and buffer height to configure strides.
> Patch 4 change to use values from SPS/PPS control to configure the HW.
> Patch 5 remove an unnecessary call to validate sps at streaming start.
> 
> Patch 6-10 refactor code to support filtering of CAPUTRE formats based
> on the image format returned from a get_image_fmt ops.
> 
> Patch 11 adds final bits to support H.264 High 10 and 4:2:2 profiles.
> 
> Tested on a ROCK Pi 4 (RK3399) and Rock64 (RK3328):
> 
>   v4l2-compliance 1.24.1, 64 bits, 64-bit time_t
>   ...
>   Total for rkvdec device /dev/video1: 46, Succeeded: 46, Failed: 0, Warnings: 0
> 
>   Running test suite JVT-FR-EXT with decoder FFmpeg-H.264-V4L2-request
>   ...
>   Ran 65/69 tests successfully

I added GStreamer support for these formats and could confirm this
results with GStreamer too.

https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/5612


For the set:
Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> 
>   Running test suite JVT-AVC_V1 with decoder FFmpeg-H.264-V4L2-request
>   ...
>   Ran 127/135 tests successfully

I'm getting 129 with GStreamer, which matches also with FFMpeg software
decoder.

> 
> Before this series:
> 
>   Running test suite JVT-FR-EXT with decoder FFmpeg-H.264-V4L2-request
>   ...
>   Ran 44/69 tests successfully

I had 43 with GStreamer, but then I notice you fixed something in
fluster. The good news this is now upstream, thanks for spotting.

https://github.com/fluendo/fluster/pull/148

> 
> Changes in v4:
> - Fix failed v4l2-compliance tests related to CAPTURE queue
> - Rework CAPTURE format filter anv validate to use an image format
> - Run fluster test suite JVT-FR-EXT [4] and JVT-AVC_V1 [5]
> Link to v3: https://lore.kernel.org/linux-media/20231029183427.1781554-1-jonas@kwiboo.se/
> 
> Changes in v3:
> - Drop merged patches
> - Use bpp and bpp_div instead of prior misuse of block_w/block_h
> - New patch to use values from SPS/PPS control to configure the HW
> - New patch to remove an unnecessary call to validate sps at streaming start
> - Reworked pixel format validation
> Link to v2: https://lore.kernel.org/linux-media/20200706215430.22859-1-jonas@kwiboo.se/
> 
> Changes in v2:
> - Collect r-b tags
> - SPS pic width and height in mbs validation moved to rkvdec_try_ctrl
> - New patch to not override output buffer sizeimage
> - Reworked pixel format validation
> - Only align decoded buffer instead of changing frmsize step_width
> Link to v1: https://lore.kernel.org/linux-media/20200701215616.30874-1-jonas@kwiboo.se/
> 
> Following commits adds support for NV15/NV20/NV30 to VOP driver:
> 728c15b4b5f3 ("drm/fourcc: Add NV20 and NV30 YUV formats")
> d4b384228562 ("drm/rockchip: vop: Add NV15, NV20 and NV30 support")
> 
> To fully runtime test this series you may need above drm commits and ffmpeg
> patches from [2], this series and drm patches is also available at [3].
> 
> [1] https://lore.kernel.org/linux-media/20200706215430.22859-1-jonas@kwiboo.se/
> [2] https://github.com/Kwiboo/FFmpeg/commits/v4l2-request-n6.1-dev/
> [3] https://github.com/Kwiboo/linux-rockchip/commits/linuxtv-rkvdec-high-10-v4/
> [4] https://gist.github.com/Kwiboo/f4ac15576b2c72887ae2bc5d58b5c865
> [5] https://gist.github.com/Kwiboo/459a1c8f1dcb56e45dc7a7a29cc28adf
> 
> Regards,
> Jonas
> 
> Alex Bee (1):
>   media: rkvdec: h264: Don't hardcode SPS/PPS parameters
> 
> Jonas Karlman (10):
>   media: v4l2-common: Add helpers to calculate bytesperline and
>     sizeimage
>   media: v4l2: Add NV15 and NV20 pixel formats
>   media: rkvdec: h264: Use bytesperline and buffer height as virstride
>   media: rkvdec: h264: Remove SPS validation at streaming start
>   media: rkvdec: Extract rkvdec_fill_decoded_pixfmt into helper
>   media: rkvdec: Move rkvdec_reset_decoded_fmt helper
>   media: rkvdec: Extract decoded format enumeration into helper
>   media: rkvdec: Add image format concept
>   media: rkvdec: Add get_image_fmt ops
>   media: rkvdec: h264: Support High 10 and 4:2:2 profiles
> 
>  .../media/v4l/pixfmt-yuv-planar.rst           | 128 +++++++++++
>  drivers/media/v4l2-core/v4l2-common.c         |  80 +++----
>  drivers/media/v4l2-core/v4l2-ioctl.c          |   2 +
>  drivers/staging/media/rkvdec/rkvdec-h264.c    |  83 +++----
>  drivers/staging/media/rkvdec/rkvdec.c         | 217 +++++++++++++-----
>  drivers/staging/media/rkvdec/rkvdec.h         |  18 +-
>  include/uapi/linux/videodev2.h                |   2 +
>  7 files changed, 396 insertions(+), 134 deletions(-)
> 

Re: [PATCH v4 00/11] media: rkvdec: Add H.264 High 10 and 4:2:2 profile support

[Jonas Karlman]

Hi Nicolas,

On 2023-11-07 22:43, Nicolas Dufresne wrote:
> Hi Jonas,
> 
> thanks for this work.
> 
> Le dimanche 05 novembre 2023 à 16:54 +0000, Jonas Karlman a écrit :
>> This is a revival of a 3 year old series [1] now that NV15/NV20/NV30 support
>> for display driver have landed in mainline tree.
>>
>> This series adds H.264 High 10 and 4:2:2 profile support to the Rockchip
>> Video Decoder driver.
>>
>> Patch 1 adds helpers for calculating plane bytesperline and sizeimage.
>> Patch 2 adds two new pixelformats for semi-planer 10-bit 4:2:0/4:2:2 YUV.
>>
>> Patch 3 change to use bytesperline and buffer height to configure strides.
>> Patch 4 change to use values from SPS/PPS control to configure the HW.
>> Patch 5 remove an unnecessary call to validate sps at streaming start.
>>
>> Patch 6-10 refactor code to support filtering of CAPUTRE formats based
>> on the image format returned from a get_image_fmt ops.
>>
>> Patch 11 adds final bits to support H.264 High 10 and 4:2:2 profiles.
>>
>> Tested on a ROCK Pi 4 (RK3399) and Rock64 (RK3328):
>>
>>   v4l2-compliance 1.24.1, 64 bits, 64-bit time_t
>>   ...
>>   Total for rkvdec device /dev/video1: 46, Succeeded: 46, Failed: 0, Warnings: 0
>>
>>   Running test suite JVT-FR-EXT with decoder FFmpeg-H.264-V4L2-request
>>   ...
>>   Ran 65/69 tests successfully
> 
> I added GStreamer support for these formats and could confirm this
> results with GStreamer too.
> 
> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/5612

Nice!

> 
> 
> For the set:
> Tested-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
>>
>>   Running test suite JVT-AVC_V1 with decoder FFmpeg-H.264-V4L2-request
>>   ...
>>   Ran 127/135 tests successfully
> 
> I'm getting 129 with GStreamer, which matches also with FFMpeg software
> decoder.

Great, I was able to track down a long term ref idx issue in the ffmpeg
v4l2-request implementation and can now also get to 129 :-)

> 
>>
>> Before this series:
>>
>>   Running test suite JVT-FR-EXT with decoder FFmpeg-H.264-V4L2-request
>>   ...
>>   Ran 44/69 tests successfully
> 
> I had 43 with GStreamer, but then I notice you fixed something in
> fluster. The good news this is now upstream, thanks for spotting.
> 
> https://github.com/fluendo/fluster/pull/148

Thanks for sending the PR!

Regards,
Jonas

> 
>>
>> Changes in v4:
>> - Fix failed v4l2-compliance tests related to CAPTURE queue
>> - Rework CAPTURE format filter anv validate to use an image format
>> - Run fluster test suite JVT-FR-EXT [4] and JVT-AVC_V1 [5]
>> Link to v3: https://lore.kernel.org/linux-media/20231029183427.1781554-1-jonas@kwiboo.se/
>>
>> Changes in v3:
>> - Drop merged patches
>> - Use bpp and bpp_div instead of prior misuse of block_w/block_h
>> - New patch to use values from SPS/PPS control to configure the HW
>> - New patch to remove an unnecessary call to validate sps at streaming start
>> - Reworked pixel format validation
>> Link to v2: https://lore.kernel.org/linux-media/20200706215430.22859-1-jonas@kwiboo.se/
>>
>> Changes in v2:
>> - Collect r-b tags
>> - SPS pic width and height in mbs validation moved to rkvdec_try_ctrl
>> - New patch to not override output buffer sizeimage
>> - Reworked pixel format validation
>> - Only align decoded buffer instead of changing frmsize step_width
>> Link to v1: https://lore.kernel.org/linux-media/20200701215616.30874-1-jonas@kwiboo.se/
>>
>> Following commits adds support for NV15/NV20/NV30 to VOP driver:
>> 728c15b4b5f3 ("drm/fourcc: Add NV20 and NV30 YUV formats")
>> d4b384228562 ("drm/rockchip: vop: Add NV15, NV20 and NV30 support")
>>
>> To fully runtime test this series you may need above drm commits and ffmpeg
>> patches from [2], this series and drm patches is also available at [3].
>>
>> [1] https://lore.kernel.org/linux-media/20200706215430.22859-1-jonas@kwiboo.se/
>> [2] https://github.com/Kwiboo/FFmpeg/commits/v4l2-request-n6.1-dev/
>> [3] https://github.com/Kwiboo/linux-rockchip/commits/linuxtv-rkvdec-high-10-v4/
>> [4] https://gist.github.com/Kwiboo/f4ac15576b2c72887ae2bc5d58b5c865
>> [5] https://gist.github.com/Kwiboo/459a1c8f1dcb56e45dc7a7a29cc28adf
>>
>> Regards,
>> Jonas
>>
>> Alex Bee (1):
>>   media: rkvdec: h264: Don't hardcode SPS/PPS parameters
>>
>> Jonas Karlman (10):
>>   media: v4l2-common: Add helpers to calculate bytesperline and
>>     sizeimage
>>   media: v4l2: Add NV15 and NV20 pixel formats
>>   media: rkvdec: h264: Use bytesperline and buffer height as virstride
>>   media: rkvdec: h264: Remove SPS validation at streaming start
>>   media: rkvdec: Extract rkvdec_fill_decoded_pixfmt into helper
>>   media: rkvdec: Move rkvdec_reset_decoded_fmt helper
>>   media: rkvdec: Extract decoded format enumeration into helper
>>   media: rkvdec: Add image format concept
>>   media: rkvdec: Add get_image_fmt ops
>>   media: rkvdec: h264: Support High 10 and 4:2:2 profiles
>>
>>  .../media/v4l/pixfmt-yuv-planar.rst           | 128 +++++++++++
>>  drivers/media/v4l2-core/v4l2-common.c         |  80 +++----
>>  drivers/media/v4l2-core/v4l2-ioctl.c          |   2 +
>>  drivers/staging/media/rkvdec/rkvdec-h264.c    |  83 +++----
>>  drivers/staging/media/rkvdec/rkvdec.c         | 217 +++++++++++++-----
>>  drivers/staging/media/rkvdec/rkvdec.h         |  18 +-
>>  include/uapi/linux/videodev2.h                |   2 +
>>  7 files changed, 396 insertions(+), 134 deletions(-)
>>
> 

Re: [PATCH v4 00/11] media: rkvdec: Add H.264 High 10 and 4:2:2 profile support

[Christopher Obbard]

Hi Jonas,

Happy new year!

On Sun, 2023-11-05 at 16:54 +0000, Jonas Karlman wrote:
> This is a revival of a 3 year old series [1] now that NV15/NV20/NV30 support
> for display driver have landed in mainline tree.
> 
> This series adds H.264 High 10 and 4:2:2 profile support to the Rockchip
> Video Decoder driver.
> 
> Patch 1 adds helpers for calculating plane bytesperline and sizeimage.
> Patch 2 adds two new pixelformats for semi-planer 10-bit 4:2:0/4:2:2 YUV.
> 
> Patch 3 change to use bytesperline and buffer height to configure strides.
> Patch 4 change to use values from SPS/PPS control to configure the HW.
> Patch 5 remove an unnecessary call to validate sps at streaming start.
> 
> Patch 6-10 refactor code to support filtering of CAPUTRE formats based
> on the image format returned from a get_image_fmt ops.
> 
> Patch 11 adds final bits to support H.264 High 10 and 4:2:2 profiles.

I send my Tested-by to v3 of this series, I also tested with v4 so for future
series please add:

Tested-by: Christopher Obbard <chris.obbard@collabora.com>

> 
> Tested on a ROCK Pi 4 (RK3399) and Rock64 (RK3328):
> 
>   v4l2-compliance 1.24.1, 64 bits, 64-bit time_t
>   ...
>   Total for rkvdec device /dev/video1: 46, Succeeded: 46, Failed: 0,
> Warnings: 0
> 
>   Running test suite JVT-FR-EXT with decoder FFmpeg-H.264-V4L2-request
>   ...
>   Ran 65/69 tests successfully
> 
>   Running test suite JVT-AVC_V1 with decoder FFmpeg-H.264-V4L2-request
>   ...
>   Ran 127/135 tests successfully
> 
> Before this series:
> 
>   Running test suite JVT-FR-EXT with decoder FFmpeg-H.264-V4L2-request
>   ...
>   Ran 44/69 tests successfully
> 
> Changes in v4:
> - Fix failed v4l2-compliance tests related to CAPTURE queue
> - Rework CAPTURE format filter anv validate to use an image format
> - Run fluster test suite JVT-FR-EXT [4] and JVT-AVC_V1 [5]
> Link to v3:
> https://lore.kernel.org/linux-media/20231029183427.1781554-1-jonas@kwiboo.se/
> 
> Changes in v3:
> - Drop merged patches
> - Use bpp and bpp_div instead of prior misuse of block_w/block_h
> - New patch to use values from SPS/PPS control to configure the HW
> - New patch to remove an unnecessary call to validate sps at streaming start
> - Reworked pixel format validation
> Link to v2:
> https://lore.kernel.org/linux-media/20200706215430.22859-1-jonas@kwiboo.se/
> 
> Changes in v2:
> - Collect r-b tags
> - SPS pic width and height in mbs validation moved to rkvdec_try_ctrl
> - New patch to not override output buffer sizeimage
> - Reworked pixel format validation
> - Only align decoded buffer instead of changing frmsize step_width
> Link to v1:
> https://lore.kernel.org/linux-media/20200701215616.30874-1-jonas@kwiboo.se/
> 
> Following commits adds support for NV15/NV20/NV30 to VOP driver:
> 728c15b4b5f3 ("drm/fourcc: Add NV20 and NV30 YUV formats")
> d4b384228562 ("drm/rockchip: vop: Add NV15, NV20 and NV30 support")
> 
> To fully runtime test this series you may need above drm commits and ffmpeg
> patches from [2], this series and drm patches is also available at [3].
> 
> [1]
> https://lore.kernel.org/linux-media/20200706215430.22859-1-jonas@kwiboo.se/
> [2] https://github.com/Kwiboo/FFmpeg/commits/v4l2-request-n6.1-dev/
> [3]
> https://github.com/Kwiboo/linux-rockchip/commits/linuxtv-rkvdec-high-10-v4/
> [4] https://gist.github.com/Kwiboo/f4ac15576b2c72887ae2bc5d58b5c865
> [5] https://gist.github.com/Kwiboo/459a1c8f1dcb56e45dc7a7a29cc28adf
> 
> Regards,
> Jonas
> 
> Alex Bee (1):
>   media: rkvdec: h264: Don't hardcode SPS/PPS parameters
> 
> Jonas Karlman (10):
>   media: v4l2-common: Add helpers to calculate bytesperline and
>     sizeimage
>   media: v4l2: Add NV15 and NV20 pixel formats
>   media: rkvdec: h264: Use bytesperline and buffer height as virstride
>   media: rkvdec: h264: Remove SPS validation at streaming start
>   media: rkvdec: Extract rkvdec_fill_decoded_pixfmt into helper
>   media: rkvdec: Move rkvdec_reset_decoded_fmt helper
>   media: rkvdec: Extract decoded format enumeration into helper
>   media: rkvdec: Add image format concept
>   media: rkvdec: Add get_image_fmt ops
>   media: rkvdec: h264: Support High 10 and 4:2:2 profiles
> 
>  .../media/v4l/pixfmt-yuv-planar.rst           | 128 +++++++++++
>  drivers/media/v4l2-core/v4l2-common.c         |  80 +++----
>  drivers/media/v4l2-core/v4l2-ioctl.c          |   2 +
>  drivers/staging/media/rkvdec/rkvdec-h264.c    |  83 +++----
>  drivers/staging/media/rkvdec/rkvdec.c         | 217 +++++++++++++-----
>  drivers/staging/media/rkvdec/rkvdec.h         |  18 +-
>  include/uapi/linux/videodev2.h                |   2 +
>  7 files changed, 396 insertions(+), 134 deletions(-)
> 

Re: [PATCH v4 00/11] media: rkvdec: Add H.264 High 10 and 4:2:2 profile support

[Diederik de Haas]

[-- Attachment #1: Type: text/plain, Size: 3893 bytes --]

On Sunday, 5 November 2023 17:54:59 CEST Jonas Karlman wrote:
> This is a revival of a 3 year old series [1] now that NV15/NV20/NV30 support
> for display driver have landed in mainline tree.
> 
> This series adds H.264 High 10 and 4:2:2 profile support to the Rockchip
> Video Decoder driver.
> 
> Patch 1 adds helpers for calculating plane bytesperline and sizeimage.
> Patch 2 adds two new pixelformats for semi-planer 10-bit 4:2:0/4:2:2 YUV.
> 
> Patch 3 change to use bytesperline and buffer height to configure strides.
> Patch 4 change to use values from SPS/PPS control to configure the HW.
> Patch 5 remove an unnecessary call to validate sps at streaming start.
> 
> Patch 6-10 refactor code to support filtering of CAPUTRE formats based
> on the image format returned from a get_image_fmt ops.
> 
> Patch 11 adds final bits to support H.264 High 10 and 4:2:2 profiles.
> 
> Tested on a ROCK Pi 4 (RK3399) and Rock64 (RK3328):
> 
>   v4l2-compliance 1.24.1, 64 bits, 64-bit time_t
>   ...
>   Total for rkvdec device /dev/video1: 46, Succeeded: 46, Failed: 0,
> Warnings: 0
> 
>   Running test suite JVT-FR-EXT with decoder FFmpeg-H.264-V4L2-request
>   ...
>   Ran 65/69 tests successfully
> 
>   Running test suite JVT-AVC_V1 with decoder FFmpeg-H.264-V4L2-request
>   ...
>   Ran 127/135 tests successfully
> 
> Before this series:
> 
>   Running test suite JVT-FR-EXT with decoder FFmpeg-H.264-V4L2-request
>   ...
>   Ran 44/69 tests successfully
> 
> ...
> 
> Following commits adds support for NV15/NV20/NV30 to VOP driver:
> 728c15b4b5f3 ("drm/fourcc: Add NV20 and NV30 YUV formats")
> d4b384228562 ("drm/rockchip: vop: Add NV15, NV20 and NV30 support")
> 
> To fully runtime test this series you may need above drm commits and ffmpeg
> patches from [2], this series and drm patches is also available at [3].
> 
> [1]
> https://lore.kernel.org/linux-media/20200706215430.22859-1-jonas@kwiboo.se/
> [2] https://github.com/Kwiboo/FFmpeg/commits/v4l2-request-n6.1-dev/ [3]
> https://github.com/Kwiboo/linux-rockchip/commits/linuxtv-rkvdec-high-10-v4/
> [4] https://gist.github.com/Kwiboo/f4ac15576b2c72887ae2bc5d58b5c865 [5]
> https://gist.github.com/Kwiboo/459a1c8f1dcb56e45dc7a7a29cc28adf

Reviving this old thread now that rkvdec2 'stuff' emerged.
I have (actually) done quite some tests with this (and "media: rkvdec: Add 
HEVC backend" patch set) and they have been part of my kernel builds ever 
since.
I _think_, but don't know, that is relevant for Andy's question:

On zondag 16 juni 2024 08:58:20 CEST Andy Yan <andyshrk@163.com> wrote:
> How can I test these patches? Do they require any additional userspace
> patches?

I have the same question and I think you'd need this and the HEVC patch set 
and then also patch FFmpeg and then it should enable HW acceleration.
So my question boils down to: with the rkvdec2 patch set, should V4L2-requests 
now also work with rkvdec, so not just Hantro anymore?

BTW: the libdrm commits have been merged upstream quite some time ago, so if 
you have a recent version of that, you don't need to patch that.
If you use FFmpeg 7.0, then Jonas has a branch for that too (haven't tried it 
yet though).

FWIW: my test results were a bit mixed. I didn't post them before as I don't 
fully/really understand this 'video stuff', and I didn't want you all to suffer 
from what was likely a PEBKAC issue.

On my PineTab2 (rk3566) I had some h.264 videos HW accelerated, but not all. 
My guess is that it's related to the resolution. 1920x1080 worked, while it 
didn't work with a 1280x640 video. The video still played, just not HW 
accelerated. IOW: improvements in some and otherwise it was just rendered by 
the CPU (I think), just like before.

On my Rock64 I got a pink tint with all videos, like described here:
https://github.com/mpv-player/mpv/issues/12968
IIUC, that's actually a problem in the lima driver?

Cheers,
  Diederik

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 00/11] media: rkvdec: Add H.264 High 10 and 4:2:2 profile support

[Nicolas Dufresne]

Hi,

Le dimanche 16 juin 2024 à 11:47 +0200, Diederik de Haas a écrit :
> On Sunday, 5 November 2023 17:54:59 CEST Jonas Karlman wrote:
> > This is a revival of a 3 year old series [1] now that NV15/NV20/NV30 support
> > for display driver have landed in mainline tree.
> > 
> > This series adds H.264 High 10 and 4:2:2 profile support to the Rockchip
> > Video Decoder driver.
> > 
> > Patch 1 adds helpers for calculating plane bytesperline and sizeimage.
> > Patch 2 adds two new pixelformats for semi-planer 10-bit 4:2:0/4:2:2 YUV.
> > 
> > Patch 3 change to use bytesperline and buffer height to configure strides.
> > Patch 4 change to use values from SPS/PPS control to configure the HW.
> > Patch 5 remove an unnecessary call to validate sps at streaming start.
> > 
> > Patch 6-10 refactor code to support filtering of CAPUTRE formats based
> > on the image format returned from a get_image_fmt ops.
> > 
> > Patch 11 adds final bits to support H.264 High 10 and 4:2:2 profiles.
> > 
> > Tested on a ROCK Pi 4 (RK3399) and Rock64 (RK3328):
> > 
> >   v4l2-compliance 1.24.1, 64 bits, 64-bit time_t
> >   ...
> >   Total for rkvdec device /dev/video1: 46, Succeeded: 46, Failed: 0,
> > Warnings: 0
> > 
> >   Running test suite JVT-FR-EXT with decoder FFmpeg-H.264-V4L2-request
> >   ...
> >   Ran 65/69 tests successfully
> > 
> >   Running test suite JVT-AVC_V1 with decoder FFmpeg-H.264-V4L2-request
> >   ...
> >   Ran 127/135 tests successfully
> > 
> > Before this series:
> > 
> >   Running test suite JVT-FR-EXT with decoder FFmpeg-H.264-V4L2-request
> >   ...
> >   Ran 44/69 tests successfully
> > 
> > ...
> > 
> > Following commits adds support for NV15/NV20/NV30 to VOP driver:
> > 728c15b4b5f3 ("drm/fourcc: Add NV20 and NV30 YUV formats")
> > d4b384228562 ("drm/rockchip: vop: Add NV15, NV20 and NV30 support")
> > 
> > To fully runtime test this series you may need above drm commits and ffmpeg
> > patches from [2], this series and drm patches is also available at [3].
> > 
> > [1]
> > https://lore.kernel.org/linux-media/20200706215430.22859-1-jonas@kwiboo.se/
> > [2] https://github.com/Kwiboo/FFmpeg/commits/v4l2-request-n6.1-dev/ [3]
> > https://github.com/Kwiboo/linux-rockchip/commits/linuxtv-rkvdec-high-10-v4/
> > [4] https://gist.github.com/Kwiboo/f4ac15576b2c72887ae2bc5d58b5c865 [5]
> > https://gist.github.com/Kwiboo/459a1c8f1dcb56e45dc7a7a29cc28adf
> 
> Reviving this old thread now that rkvdec2 'stuff' emerged.
> I have (actually) done quite some tests with this (and "media: rkvdec: Add 
> HEVC backend" patch set) and they have been part of my kernel builds ever 
> since.
> I _think_, but don't know, that is relevant for Andy's question:
> 
> On zondag 16 juni 2024 08:58:20 CEST Andy Yan <andyshrk@163.com> wrote:
> > How can I test these patches? Do they require any additional userspace
> > patches?
> 
> I have the same question and I think you'd need this and the HEVC patch set 
> and then also patch FFmpeg and then it should enable HW acceleration.
> So my question boils down to: with the rkvdec2 patch set, should V4L2-requests 
> now also work with rkvdec, so not just Hantro anymore?

FFmpeg changes are still downstream, and different people (even within
LibreELEC) seems to have slightly different version or alteration. It would be
really nice if this work could move upstream FFMpeg so that we can be more sure
what what "working with FFmpeg v4l2-requests" means.

Meanwhile, support in upstream GStreamer is stable on Hantro G2 and Mediatek
VCODEC. In theory, it works fine with RKVDEC, and it will certainly work with
RKVDEC2 when we get to write the HEVC support.

> 
> BTW: the libdrm commits have been merged upstream quite some time ago, so if 
> you have a recent version of that, you don't need to patch that.
> If you use FFmpeg 7.0, then Jonas has a branch for that too (haven't tried it 
> yet though).
> 
> FWIW: my test results were a bit mixed. I didn't post them before as I don't 
> fully/really understand this 'video stuff', and I didn't want you all to suffer 
> from what was likely a PEBKAC issue.
> 
> On my PineTab2 (rk3566) I had some h.264 videos HW accelerated, but not all. 
> My guess is that it's related to the resolution. 1920x1080 worked, while it 
> didn't work with a 1280x640 video. The video still played, just not HW 
> accelerated. IOW: improvements in some and otherwise it was just rendered by 
> the CPU (I think), just like before.

This is because all rk35XX have two hardware video decoders for H.264. This is
not to be be confused with rkvdec which is gone. It has a modified Hantro G1
core (limited to 1080p60) and rkvdec2 core (driver in progress). I don't think
Rockchip really expected the first one to be ever used, but upstream has been
pushy and its now enabled upstream. That has a side effect, which is that
userspace will have to work harder on these platform to pick the right HW for
the task.

> 
> On my Rock64 I got a pink tint with all videos, like described here:
> https://github.com/mpv-player/mpv/issues/12968
> IIUC, that's actually a problem in the lima driver?

Its not clear from the bug report. This visual artefact has been seen with
wayland compositors lately (notably weston). Notably, this can happen if you try
and import NV12 with mesa (panfrost and lima included) but forcing a TEXTURE_2D
target instead of external target. Normally this should be rejected by mesa, but
is accidentally not, and cause miss-render.

Nicolas

> 
> Cheers,
>   Diederik