[PATCH 0/9] media: rkvdec: Add H.264 High 10 and 4:2:2 profile support

[PATCH 0/9] media: rkvdec: Add H.264 High 10 and 4:2:2 profile support

[Jonas Karlman]

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

Patch 1 adds profile and level controls.

Patch 2 and 3 fixes two issues when decoding field encoded content. Patch 3
also prepare for changes to step_width in the final patch.

Patch 5 updates the v4l2_format helpers to consider block width and height
when calculating plane bytesperline and sizeimage.

Patch 6 adds two new pixelformats for 10-bit 4:2:0/4:2:2.

Patch 7 change to use bytesperline and buffer height to configure strides.

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

This series depend on the "handle unsupported H.264 bitstreams" series at [1].

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

[1] https://patchwork.linuxtv.org/cover/64977/
[2] https://patchwork.freedesktop.org/series/78099/
[3] https://github.com/Kwiboo/FFmpeg/commits/v4l2-request-hwaccel-4.3-rkvdec-high-10
[4] https://github.com/Kwiboo/linux-rockchip/commits/linuxtv-rkvdec-high-10

Regards,
Jonas

Jonas Karlman (9):
  media: rkvdec: h264: Support profile and level controls
  media: rkvdec: h264: Fix reference frame_num wrap for second field
  media: rkvdec: h264: Fix pic width and height in mbs
  media: rkvdec: h264: Fix bit depth wrap in pps packet
  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 to calculate
    stride
  media: rkvdec: Add validate_fmt ops for pixelformat validation
  media: rkvdec: h264: Support High 10 and 4:2:2 profiles

 .../userspace-api/media/v4l/pixfmt-nv15.rst   | 101 ++++++++++++++++
 .../userspace-api/media/v4l/pixfmt-nv20.rst   |  99 ++++++++++++++++
 .../userspace-api/media/v4l/yuv-formats.rst   |   2 +
 drivers/media/v4l2-core/v4l2-common.c         |  80 ++++++-------
 drivers/media/v4l2-core/v4l2-ioctl.c          |   2 +
 drivers/staging/media/rkvdec/rkvdec-h264.c    | 108 +++++++++++++++---
 drivers/staging/media/rkvdec/rkvdec.c         |  43 +++++--
 drivers/staging/media/rkvdec/rkvdec.h         |   1 +
 include/uapi/linux/videodev2.h                |   3 +
 9 files changed, 369 insertions(+), 70 deletions(-)
 create mode 100644 Documentation/userspace-api/media/v4l/pixfmt-nv15.rst
 create mode 100644 Documentation/userspace-api/media/v4l/pixfmt-nv20.rst

-- 
2.17.1

[PATCH 1/9] media: rkvdec: h264: Support profile and level controls

[Jonas Karlman]

The Rockchip Video Decoder used in RK3399 supports H.264 profiles from
Baseline to High 4:2:2 up to Level 5.1, except for the Extended profile.

Expose the V4L2_CID_MPEG_VIDEO_H264_PROFILE and the
V4L2_CID_MPEG_VIDEO_H264_LEVEL control, so that userspace can query the
driver for the list of supported profiles and level.

In current state only Baseline to High profile is supported by the driver.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 drivers/staging/media/rkvdec/rkvdec.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 0f81b47792f6..b1de55aa6535 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -94,6 +94,19 @@ static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
 		.cfg.def = V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B,
 		.cfg.max = V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B,
 	},
+	{
+		.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.menu_skip_mask =
+			BIT(V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED),
+		.cfg.def = V4L2_MPEG_VIDEO_H264_PROFILE_MAIN,
+	},
+	{
+		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_LEVEL,
+		.cfg.min = V4L2_MPEG_VIDEO_H264_LEVEL_1_0,
+		.cfg.max = V4L2_MPEG_VIDEO_H264_LEVEL_5_1,
+	},
 };
 
 static const struct rkvdec_ctrls rkvdec_h264_ctrls = {
-- 
2.17.1

Re: [PATCH 1/9] media: rkvdec: h264: Support profile and level controls

[Ezequiel Garcia]

On Wed, 2020-07-01 at 21:56 +0000, Jonas Karlman wrote:
> The Rockchip Video Decoder used in RK3399 supports H.264 profiles from
> Baseline to High 4:2:2 up to Level 5.1, except for the Extended profile.
> 
> Expose the V4L2_CID_MPEG_VIDEO_H264_PROFILE and the
> V4L2_CID_MPEG_VIDEO_H264_LEVEL control, so that userspace can query the
> driver for the list of supported profiles and level.
> 
> In current state only Baseline to High profile is supported by the driver.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

I think the patch is good so:

Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>

However, feel free to just drop this patch and support the profiles
and levels at the end of the patchset, once High 10 and High 422
support is there.

Thanks,
Ezequiel

> ---
>  drivers/staging/media/rkvdec/rkvdec.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> index 0f81b47792f6..b1de55aa6535 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.c
> +++ b/drivers/staging/media/rkvdec/rkvdec.c
> @@ -94,6 +94,19 @@ static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
>  		.cfg.def = V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B,
>  		.cfg.max = V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B,
>  	},
> +	{
> +		.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.menu_skip_mask =
> +			BIT(V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED),
> +		.cfg.def = V4L2_MPEG_VIDEO_H264_PROFILE_MAIN,
> +	},
> +	{
> +		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_LEVEL,
> +		.cfg.min = V4L2_MPEG_VIDEO_H264_LEVEL_1_0,
> +		.cfg.max = V4L2_MPEG_VIDEO_H264_LEVEL_5_1,
> +	},
>  };
>  
>  static const struct rkvdec_ctrls rkvdec_h264_ctrls = {

Re: [PATCH 1/9] media: rkvdec: h264: Support profile and level controls

[Jonas Karlman]

On 2020-07-03 04:54, Ezequiel Garcia wrote:
> On Wed, 2020-07-01 at 21:56 +0000, Jonas Karlman wrote:
>> The Rockchip Video Decoder used in RK3399 supports H.264 profiles from
>> Baseline to High 4:2:2 up to Level 5.1, except for the Extended profile.
>>
>> Expose the V4L2_CID_MPEG_VIDEO_H264_PROFILE and the
>> V4L2_CID_MPEG_VIDEO_H264_LEVEL control, so that userspace can query the
>> driver for the list of supported profiles and level.
>>
>> In current state only Baseline to High profile is supported by the driver.
>>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> 
> I think the patch is good so:
> 
> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
> 
> However, feel free to just drop this patch and support the profiles
> and levels at the end of the patchset, once High 10 and High 422
> support is there.

Sure, that makes more sense, will move to end in v2.

Regards,
Jonas

> 
> Thanks,
> Ezequiel
> 
>> ---
>>  drivers/staging/media/rkvdec/rkvdec.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
>> index 0f81b47792f6..b1de55aa6535 100644
>> --- a/drivers/staging/media/rkvdec/rkvdec.c
>> +++ b/drivers/staging/media/rkvdec/rkvdec.c
>> @@ -94,6 +94,19 @@ static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
>>  		.cfg.def = V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B,
>>  		.cfg.max = V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B,
>>  	},
>> +	{
>> +		.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.menu_skip_mask =
>> +			BIT(V4L2_MPEG_VIDEO_H264_PROFILE_EXTENDED),
>> +		.cfg.def = V4L2_MPEG_VIDEO_H264_PROFILE_MAIN,
>> +	},
>> +	{
>> +		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_LEVEL,
>> +		.cfg.min = V4L2_MPEG_VIDEO_H264_LEVEL_1_0,
>> +		.cfg.max = V4L2_MPEG_VIDEO_H264_LEVEL_5_1,
>> +	},
>>  };
>>  
>>  static const struct rkvdec_ctrls rkvdec_h264_ctrls = {
> 
> 

[PATCH 3/9] media: rkvdec: h264: Fix pic width and height in mbs

[Jonas Karlman]

The width and height in mbs is currently configured based on OUTPUT buffer
resolution, this works for frame pictures but can cause issues for field
pictures or when frmsize step_width is changed to support 10-bit decoding.

When frame_mbs_only_flag is 0 the height in mbs should be height of
the field instead of height of frame.

Validate pic_width_in_mbs_minus1 and pic_height_in_map_units_minus1
against CAPTURE buffer resolution and use these values to configure HW.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 44 +++++++++++++++++++---
 1 file changed, 39 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index f0cfed84d60d..c9aebeb8f9b3 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -672,8 +672,8 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
 		  LOG2_MAX_PIC_ORDER_CNT_LSB_MINUS4);
 	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_DELTA_PIC_ORDER_ALWAYS_ZERO),
 		  DELTA_PIC_ORDER_ALWAYS_ZERO_FLAG);
-	WRITE_PPS(DIV_ROUND_UP(ctx->coded_fmt.fmt.pix_mp.width, 16), PIC_WIDTH_IN_MBS);
-	WRITE_PPS(DIV_ROUND_UP(ctx->coded_fmt.fmt.pix_mp.height, 16), PIC_HEIGHT_IN_MBS);
+	WRITE_PPS(sps->pic_width_in_mbs_minus1 + 1, PIC_WIDTH_IN_MBS);
+	WRITE_PPS(sps->pic_height_in_map_units_minus1 + 1, PIC_HEIGHT_IN_MBS);
 	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY),
 		  FRAME_MBS_ONLY_FLAG);
 	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD),
@@ -1058,10 +1058,33 @@ static void rkvdec_h264_stop(struct rkvdec_ctx *ctx)
 	kfree(h264_ctx);
 }
 
-static void rkvdec_h264_run_preamble(struct rkvdec_ctx *ctx,
-				     struct rkvdec_h264_run *run)
+static int validate_sps(struct rkvdec_ctx *ctx,
+			const struct v4l2_ctrl_h264_sps *sps)
+{
+	unsigned int width, height;
+
+	if (WARN_ON(!sps))
+		return -EINVAL;
+
+	width = (sps->pic_width_in_mbs_minus1 + 1) * 16;
+	height = (sps->pic_height_in_map_units_minus1 + 1) * 16;
+
+	if (width > ctx->decoded_fmt.fmt.pix_mp.width ||
+	    height > ctx->decoded_fmt.fmt.pix_mp.height) {
+		dev_err(ctx->dev->dev,
+			"unexpected bitstream resolution %ux%u\n",
+			width, height);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int rkvdec_h264_run_preamble(struct rkvdec_ctx *ctx,
+				    struct rkvdec_h264_run *run)
 {
 	struct v4l2_ctrl *ctrl;
+	int ret;
 
 	ctrl = v4l2_ctrl_find(&ctx->ctrl_hdl,
 			      V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS);
@@ -1080,6 +1103,12 @@ static void rkvdec_h264_run_preamble(struct rkvdec_ctx *ctx,
 	run->scaling_matrix = ctrl ? ctrl->p_cur.p : NULL;
 
 	rkvdec_run_preamble(ctx, &run->base);
+
+	ret = validate_sps(ctx, run->sps);
+	if (ret)
+		return ret;
+
+	return 0;
 }
 
 static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
@@ -1088,8 +1117,13 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
 	struct rkvdec_dev *rkvdec = ctx->dev;
 	struct rkvdec_h264_ctx *h264_ctx = ctx->priv;
 	struct rkvdec_h264_run run;
+	int ret;
 
-	rkvdec_h264_run_preamble(ctx, &run);
+	ret = rkvdec_h264_run_preamble(ctx, &run);
+	if (ret) {
+		rkvdec_run_postamble(ctx, &run.base);
+		return ret;
+	}
 
 	/* Build the P/B{0,1} ref lists. */
 	v4l2_h264_init_reflist_builder(&reflist_builder, run.decode_params,
-- 
2.17.1

Re: [PATCH 3/9] media: rkvdec: h264: Fix pic width and height in mbs

[Ezequiel Garcia]

On Wed, 2020-07-01 at 21:56 +0000, Jonas Karlman wrote:
> The width and height in mbs is currently configured based on OUTPUT buffer
> resolution, this works for frame pictures but can cause issues for field
> pictures or when frmsize step_width is changed to support 10-bit decoding.
> 
> When frame_mbs_only_flag is 0 the height in mbs should be height of
> the field instead of height of frame.
> 
> Validate pic_width_in_mbs_minus1 and pic_height_in_map_units_minus1
> against CAPTURE buffer resolution and use these values to configure HW.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  drivers/staging/media/rkvdec/rkvdec-h264.c | 44 +++++++++++++++++++---
>  1 file changed, 39 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index f0cfed84d60d..c9aebeb8f9b3 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -672,8 +672,8 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
>  		  LOG2_MAX_PIC_ORDER_CNT_LSB_MINUS4);
>  	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_DELTA_PIC_ORDER_ALWAYS_ZERO),
>  		  DELTA_PIC_ORDER_ALWAYS_ZERO_FLAG);
> -	WRITE_PPS(DIV_ROUND_UP(ctx->coded_fmt.fmt.pix_mp.width, 16), PIC_WIDTH_IN_MBS);
> -	WRITE_PPS(DIV_ROUND_UP(ctx->coded_fmt.fmt.pix_mp.height, 16), PIC_HEIGHT_IN_MBS);
> +	WRITE_PPS(sps->pic_width_in_mbs_minus1 + 1, PIC_WIDTH_IN_MBS);
> +	WRITE_PPS(sps->pic_height_in_map_units_minus1 + 1, PIC_HEIGHT_IN_MBS);
>  	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY),
>  		  FRAME_MBS_ONLY_FLAG);
>  	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD),
> @@ -1058,10 +1058,33 @@ static void rkvdec_h264_stop(struct rkvdec_ctx *ctx)
>  	kfree(h264_ctx);
>  }
>  
> -static void rkvdec_h264_run_preamble(struct rkvdec_ctx *ctx,
> -				     struct rkvdec_h264_run *run)
> +static int validate_sps(struct rkvdec_ctx *ctx,
> +			const struct v4l2_ctrl_h264_sps *sps)
> +{
> +	unsigned int width, height;
> +
> +	if (WARN_ON(!sps))
> +		return -EINVAL;
> +
> +	width = (sps->pic_width_in_mbs_minus1 + 1) * 16;
> +	height = (sps->pic_height_in_map_units_minus1 + 1) * 16;
> +
> +	if (width > ctx->decoded_fmt.fmt.pix_mp.width ||
> +	    height > ctx->decoded_fmt.fmt.pix_mp.height) {

Why using decoded_fmt instead of coded_fmt?

Also, by the time the SPS control is passed, the OUTPUT
and CAPTURE formats should be already set, so it should be
possible to validate the SPS at TRY_EXT_CTRLS, using
v4l2_ctrl_ops.try_ctrl.

That would be much better, since once the application
calls STREAMON on both queues, I think things are
expected to be validated as much as possible.

Thanks,
Ezequiel

> +		dev_err(ctx->dev->dev,
> +			"unexpected bitstream resolution %ux%u\n",
> +			width, height);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rkvdec_h264_run_preamble(struct rkvdec_ctx *ctx,
> +				    struct rkvdec_h264_run *run)
>  {
>  	struct v4l2_ctrl *ctrl;
> +	int ret;
>  
>  	ctrl = v4l2_ctrl_find(&ctx->ctrl_hdl,
>  			      V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS);
> @@ -1080,6 +1103,12 @@ static void rkvdec_h264_run_preamble(struct rkvdec_ctx *ctx,
>  	run->scaling_matrix = ctrl ? ctrl->p_cur.p : NULL;
>  
>  	rkvdec_run_preamble(ctx, &run->base);
> +
> +	ret = validate_sps(ctx, run->sps);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
>  }
>  
>  static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
> @@ -1088,8 +1117,13 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
>  	struct rkvdec_dev *rkvdec = ctx->dev;
>  	struct rkvdec_h264_ctx *h264_ctx = ctx->priv;
>  	struct rkvdec_h264_run run;
> +	int ret;
>  
> -	rkvdec_h264_run_preamble(ctx, &run);
> +	ret = rkvdec_h264_run_preamble(ctx, &run);
> +	if (ret) {
> +		rkvdec_run_postamble(ctx, &run.base);
> +		return ret;
> +	}
>  
>  	/* Build the P/B{0,1} ref lists. */
>  	v4l2_h264_init_reflist_builder(&reflist_builder, run.decode_params,

Re: [PATCH 3/9] media: rkvdec: h264: Fix pic width and height in mbs

[Jonas Karlman]

On 2020-07-03 04:48, Ezequiel Garcia wrote:
> On Wed, 2020-07-01 at 21:56 +0000, Jonas Karlman wrote:
>> The width and height in mbs is currently configured based on OUTPUT buffer
>> resolution, this works for frame pictures but can cause issues for field
>> pictures or when frmsize step_width is changed to support 10-bit decoding.
>>
>> When frame_mbs_only_flag is 0 the height in mbs should be height of
>> the field instead of height of frame.
>>
>> Validate pic_width_in_mbs_minus1 and pic_height_in_map_units_minus1
>> against CAPTURE buffer resolution and use these values to configure HW.
>>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>>  drivers/staging/media/rkvdec/rkvdec-h264.c | 44 +++++++++++++++++++---
>>  1 file changed, 39 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
>> index f0cfed84d60d..c9aebeb8f9b3 100644
>> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
>> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
>> @@ -672,8 +672,8 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
>>  		  LOG2_MAX_PIC_ORDER_CNT_LSB_MINUS4);
>>  	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_DELTA_PIC_ORDER_ALWAYS_ZERO),
>>  		  DELTA_PIC_ORDER_ALWAYS_ZERO_FLAG);
>> -	WRITE_PPS(DIV_ROUND_UP(ctx->coded_fmt.fmt.pix_mp.width, 16), PIC_WIDTH_IN_MBS);
>> -	WRITE_PPS(DIV_ROUND_UP(ctx->coded_fmt.fmt.pix_mp.height, 16), PIC_HEIGHT_IN_MBS);
>> +	WRITE_PPS(sps->pic_width_in_mbs_minus1 + 1, PIC_WIDTH_IN_MBS);
>> +	WRITE_PPS(sps->pic_height_in_map_units_minus1 + 1, PIC_HEIGHT_IN_MBS);
>>  	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY),
>>  		  FRAME_MBS_ONLY_FLAG);
>>  	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD),
>> @@ -1058,10 +1058,33 @@ static void rkvdec_h264_stop(struct rkvdec_ctx *ctx)
>>  	kfree(h264_ctx);
>>  }
>>  
>> -static void rkvdec_h264_run_preamble(struct rkvdec_ctx *ctx,
>> -				     struct rkvdec_h264_run *run)
>> +static int validate_sps(struct rkvdec_ctx *ctx,
>> +			const struct v4l2_ctrl_h264_sps *sps)
>> +{
>> +	unsigned int width, height;
>> +
>> +	if (WARN_ON(!sps))
>> +		return -EINVAL;
>> +
>> +	width = (sps->pic_width_in_mbs_minus1 + 1) * 16;
>> +	height = (sps->pic_height_in_map_units_minus1 + 1) * 16;
>> +
>> +	if (width > ctx->decoded_fmt.fmt.pix_mp.width ||
>> +	    height > ctx->decoded_fmt.fmt.pix_mp.height) {
> 
> Why using decoded_fmt instead of coded_fmt?

I used decoded_fmt because that would be the outer limits of what can be
decoded into in the CAPTURE buffer. Not sure if or how coded_fmt is validated
that it does not exceed the decoded_fmt resolution.

> 
> Also, by the time the SPS control is passed, the OUTPUT
> and CAPTURE formats should be already set, so it should be
> possible to validate the SPS at TRY_EXT_CTRLS, using
> v4l2_ctrl_ops.try_ctrl.

I was not sure how to access the rkvdec_ctx from v4l2_ctrl_ops.try_ctrl
so I went with similar approach as was done in the VP9 series, looks like
we can use container_of and ctrl->handler to find rkvdec_ctx.

Will try to move the validation into rkvdec_try_ctrl for v2.

Regards,
Jonas

> 
> That would be much better, since once the application
> calls STREAMON on both queues, I think things are
> expected to be validated as much as possible.
> 
> Thanks,
> Ezequiel
> 
>> +		dev_err(ctx->dev->dev,
>> +			"unexpected bitstream resolution %ux%u\n",
>> +			width, height);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int rkvdec_h264_run_preamble(struct rkvdec_ctx *ctx,
>> +				    struct rkvdec_h264_run *run)
>>  {
>>  	struct v4l2_ctrl *ctrl;
>> +	int ret;
>>  
>>  	ctrl = v4l2_ctrl_find(&ctx->ctrl_hdl,
>>  			      V4L2_CID_MPEG_VIDEO_H264_DECODE_PARAMS);
>> @@ -1080,6 +1103,12 @@ static void rkvdec_h264_run_preamble(struct rkvdec_ctx *ctx,
>>  	run->scaling_matrix = ctrl ? ctrl->p_cur.p : NULL;
>>  
>>  	rkvdec_run_preamble(ctx, &run->base);
>> +
>> +	ret = validate_sps(ctx, run->sps);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>>  }
>>  
>>  static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
>> @@ -1088,8 +1117,13 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
>>  	struct rkvdec_dev *rkvdec = ctx->dev;
>>  	struct rkvdec_h264_ctx *h264_ctx = ctx->priv;
>>  	struct rkvdec_h264_run run;
>> +	int ret;
>>  
>> -	rkvdec_h264_run_preamble(ctx, &run);
>> +	ret = rkvdec_h264_run_preamble(ctx, &run);
>> +	if (ret) {
>> +		rkvdec_run_postamble(ctx, &run.base);
>> +		return ret;
>> +	}
>>  
>>  	/* Build the P/B{0,1} ref lists. */
>>  	v4l2_h264_init_reflist_builder(&reflist_builder, run.decode_params,
> 
> 

[PATCH 2/9] media: rkvdec: h264: Fix reference frame_num wrap for second field

[Jonas Karlman]

When decoding the second field in a complementary field pair the second
field is sharing the same frame_num with the first field.

Currently the frame_num for the first field is wrapped when it matches the
field being decoded, this cause issues to decode the second field in a
complementary field pair.

Fix this by using inclusive comparison, less than or equal.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 7b66e2743a4f..f0cfed84d60d 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -754,7 +754,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
 			continue;
 
 		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM ||
-		    dpb[i].frame_num < sl_params->frame_num) {
+		    dpb[i].frame_num <= sl_params->frame_num) {
 			p[i] = dpb[i].frame_num;
 			continue;
 		}
-- 
2.17.1

Re: [PATCH 2/9] media: rkvdec: h264: Fix reference frame_num wrap for second field

[Ezequiel Garcia]

On Wed, 2020-07-01 at 21:56 +0000, Jonas Karlman wrote:
> When decoding the second field in a complementary field pair the second
> field is sharing the same frame_num with the first field.
> 
> Currently the frame_num for the first field is wrapped when it matches the
> field being decoded, this cause issues to decode the second field in a
> complementary field pair.
> 
> Fix this by using inclusive comparison, less than or equal.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>

Thanks!
Ezequiel

> ---
>  drivers/staging/media/rkvdec/rkvdec-h264.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index 7b66e2743a4f..f0cfed84d60d 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -754,7 +754,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
>  			continue;
>  
>  		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM ||
> -		    dpb[i].frame_num < sl_params->frame_num) {
> +		    dpb[i].frame_num <= sl_params->frame_num) {
>  			p[i] = dpb[i].frame_num;
>  			continue;
>  		}

Re: [PATCH 2/9] media: rkvdec: h264: Fix reference frame_num wrap for second field

[Ezequiel Garcia]

+Boris

On Wed, 2020-07-01 at 21:56 +0000, Jonas Karlman wrote:
> When decoding the second field in a complementary field pair the second
> field is sharing the same frame_num with the first field.
> 
> Currently the frame_num for the first field is wrapped when it matches the
> field being decoded, this cause issues to decode the second field in a
> complementary field pair.
> 
> Fix this by using inclusive comparison, less than or equal.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  drivers/staging/media/rkvdec/rkvdec-h264.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index 7b66e2743a4f..f0cfed84d60d 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -754,7 +754,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
>  			continue;
>  

Taking a closer look here, I see this comment:

        /*
         * Assign an invalid pic_num if DPB entry at that position is inactive.
         * If we assign 0 in that position hardware will treat that as a real
         * reference picture with pic_num 0, triggering output picture
         * corruption.
         */

ChromeOS driver was setting 0xff on a non-active hw_rps table entry,
but we are not doing so.

Are we missing anything, or is this not really required
(and so the comment is outdated)?

Thanks,
Ezequiel

>  		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM ||
> -		    dpb[i].frame_num < sl_params->frame_num) {
> +		    dpb[i].frame_num <= sl_params->frame_num) {
>  			p[i] = dpb[i].frame_num;
>  			continue;
>  		}

[PATCH 5/9] 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 block width and height when calculating plane
bytesperline and sizeimage.

This prepare support for new pixel formats added in next patch that make
use of block width and height.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 drivers/media/v4l2-core/v4l2-common.c | 77 +++++++++++++--------------
 1 file changed, 38 insertions(+), 39 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 3dc17ebe14fa..4102c373b48a 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -333,6 +333,33 @@ static inline unsigned int v4l2_format_block_height(const struct v4l2_format_inf
 	return info->block_h[plane];
 }
 
+static inline unsigned int v4l2_format_plane_width(const struct v4l2_format_info *info, int plane,
+						   unsigned int width)
+{
+	unsigned int hdiv = plane ? info->hdiv : 1;
+	unsigned int bytes = DIV_ROUND_UP(width * info->bpp[plane],
+				v4l2_format_block_width(info, plane) *
+				v4l2_format_block_height(info, plane));
+
+	return DIV_ROUND_UP(bytes, hdiv);
+}
+
+static inline unsigned int v4l2_format_plane_height(const struct v4l2_format_info *info, int plane,
+						    unsigned int height)
+{
+	unsigned int vdiv = plane ? info->vdiv : 1;
+	unsigned int lines = ALIGN(height, v4l2_format_block_height(info, plane));
+
+	return DIV_ROUND_UP(lines, vdiv);
+}
+
+static inline unsigned int v4l2_format_plane_size(const struct v4l2_format_info *info, int plane,
+						  unsigned int width, unsigned int height)
+{
+	return v4l2_format_plane_width(info, plane, width) *
+	       v4l2_format_plane_height(info, plane, height);
+}
+
 void v4l2_apply_frmsize_constraints(u32 *width, u32 *height,
 				    const struct v4l2_frmsize_stepwise *frmsize)
 {
@@ -368,37 +395,19 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 
 	if (info->mem_planes == 1) {
 		plane = &pixfmt->plane_fmt[0];
-		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
+		plane->bytesperline = v4l2_format_plane_width(info, 0, width);
 		plane->sizeimage = 0;
 
-		for (i = 0; i < info->comp_planes; i++) {
-			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
-			unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
-			unsigned int aligned_width;
-			unsigned int aligned_height;
-
-			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
-			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
-
-			plane->sizeimage += info->bpp[i] *
-				DIV_ROUND_UP(aligned_width, hdiv) *
-				DIV_ROUND_UP(aligned_height, vdiv);
-		}
+		for (i = 0; i < info->comp_planes; i++)
+			plane->sizeimage +=
+				v4l2_format_plane_size(info, i, width, height);
 	} else {
 		for (i = 0; i < info->comp_planes; i++) {
-			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
-			unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
-			unsigned int aligned_width;
-			unsigned int aligned_height;
-
-			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
-			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
-
 			plane = &pixfmt->plane_fmt[i];
 			plane->bytesperline =
-				info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv);
-			plane->sizeimage =
-				plane->bytesperline * DIV_ROUND_UP(aligned_height, vdiv);
+				v4l2_format_plane_width(info, i, width);
+			plane->sizeimage = plane->bytesperline *
+				v4l2_format_plane_height(info, i, height);
 		}
 	}
 	return 0;
@@ -422,22 +431,12 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
 	pixfmt->width = width;
 	pixfmt->height = height;
 	pixfmt->pixelformat = pixelformat;
-	pixfmt->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
+	pixfmt->bytesperline = v4l2_format_plane_width(info, 0, width);
 	pixfmt->sizeimage = 0;
 
-	for (i = 0; i < info->comp_planes; i++) {
-		unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
-		unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
-		unsigned int aligned_width;
-		unsigned int aligned_height;
-
-		aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
-		aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
-
-		pixfmt->sizeimage += info->bpp[i] *
-			DIV_ROUND_UP(aligned_width, hdiv) *
-			DIV_ROUND_UP(aligned_height, vdiv);
-	}
+	for (i = 0; i < info->comp_planes; i++)
+		pixfmt->sizeimage +=
+			v4l2_format_plane_size(info, i, width, height);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
-- 
2.17.1

[PATCH 4/9] media: rkvdec: h264: Fix bit depth wrap in pps packet

[Jonas Karlman]

The luma and chroma bit depth fields in the pps packet is 3 bits wide.
8 is wrongly added to the bit depth value written to these 3-bit fields.
Because only the 3 LSB is written the hardware is configured correctly.

Correct this by not adding 8 to the luma and chroma bit depth value.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index c9aebeb8f9b3..9c8e49642cd9 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -662,8 +662,8 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
 	WRITE_PPS(0xff, PROFILE_IDC);
 	WRITE_PPS(1, CONSTRAINT_SET3_FLAG);
 	WRITE_PPS(sps->chroma_format_idc, CHROMA_FORMAT_IDC);
-	WRITE_PPS(sps->bit_depth_luma_minus8 + 8, BIT_DEPTH_LUMA);
-	WRITE_PPS(sps->bit_depth_chroma_minus8 + 8, BIT_DEPTH_CHROMA);
+	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->log2_max_frame_num_minus4, LOG2_MAX_FRAME_NUM_MINUS4);
 	WRITE_PPS(sps->max_num_ref_frames, MAX_NUM_REF_FRAMES);
-- 
2.17.1

Re: [PATCH 4/9] media: rkvdec: h264: Fix bit depth wrap in pps packet

[Ezequiel Garcia]

On Wed, 2020-07-01 at 21:56 +0000, Jonas Karlman wrote:
> The luma and chroma bit depth fields in the pps packet is 3 bits wide.
> 8 is wrongly added to the bit depth value written to these 3-bit fields.
> Because only the 3 LSB is written the hardware is configured correctly.
> 
> Correct this by not adding 8 to the luma and chroma bit depth value.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>

Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>

Thanks!
Ezequiel

> ---
>  drivers/staging/media/rkvdec/rkvdec-h264.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index c9aebeb8f9b3..9c8e49642cd9 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -662,8 +662,8 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
>  	WRITE_PPS(0xff, PROFILE_IDC);
>  	WRITE_PPS(1, CONSTRAINT_SET3_FLAG);
>  	WRITE_PPS(sps->chroma_format_idc, CHROMA_FORMAT_IDC);
> -	WRITE_PPS(sps->bit_depth_luma_minus8 + 8, BIT_DEPTH_LUMA);
> -	WRITE_PPS(sps->bit_depth_chroma_minus8 + 8, BIT_DEPTH_CHROMA);
> +	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->log2_max_frame_num_minus4, LOG2_MAX_FRAME_NUM_MINUS4);
>  	WRITE_PPS(sps->max_num_ref_frames, MAX_NUM_REF_FRAMES);

[PATCH 6/9] 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 a packed 10-bit 4:2:0/4:2:2 semi-planar Y/CbCr format
similar to P010 and P210 but has no padding between components. Instead,
luminance and chrominance samples are grouped into 4s so that each group is
packed into an integer number of bytes:

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

The '15' 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>
---
 .../userspace-api/media/v4l/pixfmt-nv15.rst   | 101 ++++++++++++++++++
 .../userspace-api/media/v4l/pixfmt-nv20.rst   |  99 +++++++++++++++++
 .../userspace-api/media/v4l/yuv-formats.rst   |   2 +
 drivers/media/v4l2-core/v4l2-common.c         |   3 +
 drivers/media/v4l2-core/v4l2-ioctl.c          |   2 +
 include/uapi/linux/videodev2.h                |   3 +
 6 files changed, 210 insertions(+)
 create mode 100644 Documentation/userspace-api/media/v4l/pixfmt-nv15.rst
 create mode 100644 Documentation/userspace-api/media/v4l/pixfmt-nv20.rst

diff --git a/Documentation/userspace-api/media/v4l/pixfmt-nv15.rst b/Documentation/userspace-api/media/v4l/pixfmt-nv15.rst
new file mode 100644
index 000000000000..d059db58c6e0
--- /dev/null
+++ b/Documentation/userspace-api/media/v4l/pixfmt-nv15.rst
@@ -0,0 +1,101 @@
+.. Permission is granted to copy, distribute and/or modify this
+.. document under the terms of the GNU Free Documentation License,
+.. Version 1.1 or any later version published by the Free Software
+.. Foundation, with no Invariant Sections, no Front-Cover Texts
+.. and no Back-Cover Texts. A copy of the license is included at
+.. Documentation/userspace-api/media/fdl-appendix.rst.
+..
+.. TODO: replace it to GFDL-1.1-or-later WITH no-invariant-sections
+
+.. _V4L2-PIX-FMT-NV15:
+
+**************************
+V4L2_PIX_FMT_NV15 ('NV15')
+**************************
+
+Format with ½ horizontal and vertical chroma resolution, also known as
+YUV 4:2:0. One luminance and one chrominance plane with alternating
+chroma samples similar to ``V4L2_PIX_FMT_NV12`` but with 10-bit samples
+that are grouped into four and packed into five bytes.
+
+The '15' suffix refers to the optimum effective bits per pixel which is
+achieved when the total number of luminance samples is a multiple of 8.
+
+
+Description
+===========
+
+This is a packed 10-bit two-plane version of the YUV 4:2:0 format. The
+three components are separated into two sub-images or planes. The Y plane
+is first. The Y plane has five bytes per each group of four pixels. A
+combined CbCr plane immediately follows the Y plane in memory. The CbCr
+plane is the same width, in bytes, as the Y plane (and of the image), but
+is half as tall in pixels. Each CbCr pair belongs to four pixels. For
+example, Cb\ :sub:`00`/Cr\ :sub:`00` belongs to Y'\ :sub:`00`,
+Y'\ :sub:`01`, Y'\ :sub:`10`, Y'\ :sub:`11`.
+
+If the Y plane has pad bytes after each row, then the CbCr plane has as
+many pad bytes after its rows.
+
+**Byte Order.**
+Little endian. Each cell is one byte. Pixels cross the byte boundary.
+
+
+.. flat-table::
+    :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:
+      - 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]`
+
+
+**Color Sample Location:**
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+
+    * -
+      - 0
+      -
+      - 1
+      - 2
+      -
+      - 3
+    * - 0
+      - Y
+      -
+      - Y
+      - Y
+      -
+      - Y
+    * -
+      -
+      - C
+      -
+      -
+      - C
+      -
+    * - 1
+      - Y
+      -
+      - Y
+      - Y
+      -
+      - Y
diff --git a/Documentation/userspace-api/media/v4l/pixfmt-nv20.rst b/Documentation/userspace-api/media/v4l/pixfmt-nv20.rst
new file mode 100644
index 000000000000..a8123be0baa3
--- /dev/null
+++ b/Documentation/userspace-api/media/v4l/pixfmt-nv20.rst
@@ -0,0 +1,99 @@
+.. Permission is granted to copy, distribute and/or modify this
+.. document under the terms of the GNU Free Documentation License,
+.. Version 1.1 or any later version published by the Free Software
+.. Foundation, with no Invariant Sections, no Front-Cover Texts
+.. and no Back-Cover Texts. A copy of the license is included at
+.. Documentation/userspace-api/media/fdl-appendix.rst.
+..
+.. TODO: replace it to GFDL-1.1-or-later WITH no-invariant-sections
+
+.. _V4L2-PIX-FMT-NV20:
+
+**************************
+V4L2_PIX_FMT_NV20 ('NV20')
+**************************
+
+Format with ½ horizontal chroma resolution, also known as YUV 4:2:2.
+One luminance and one chrominance plane with alternating chroma samples
+similar to ``V4L2_PIX_FMT_NV16`` but with 10-bit samples
+that are grouped into four and packed into five bytes.
+
+The '20' suffix refers to the optimum effective bits per pixel which is
+achieved when the total number of luminance samples is a multiple of 4.
+
+
+Description
+===========
+
+This is a packed 10-bit two-plane version of the YUV 4:2:2 format. The
+three components are separated into two sub-images or planes. The Y plane
+is first. The Y plane has five bytes per each group of four pixels. A
+combined CbCr plane immediately follows the Y plane in memory. The CbCr
+plane is the same width and height, in bytes, as the Y plane (and of the
+image). Each CbCr pair belongs to two pixels. For example,
+Cb\ :sub:`00`/Cr\ :sub:`00` belongs to Y'\ :sub:`00`, Y'\ :sub:`01`.
+
+If the Y plane has pad bytes after each row, then the CbCr plane has as
+many pad bytes after its rows.
+
+**Byte Order.**
+Little endian. Each cell is one byte. Pixels cross the byte boundary.
+
+
+.. flat-table::
+    :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:
+      - 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 + 15:
+      - 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]`
+
+
+**Color Sample Location:**
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+
+    * -
+      - 0
+      -
+      - 1
+      - 2
+      -
+      - 3
+    * - 0
+      - Y
+      - C
+      - Y
+      - Y
+      - C
+      - Y
+    * - 1
+      - Y
+      - C
+      - Y
+      - Y
+      - C
+      - Y
diff --git a/Documentation/userspace-api/media/v4l/yuv-formats.rst b/Documentation/userspace-api/media/v4l/yuv-formats.rst
index 8ee92d0cd769..7cca883f178a 100644
--- a/Documentation/userspace-api/media/v4l/yuv-formats.rst
+++ b/Documentation/userspace-api/media/v4l/yuv-formats.rst
@@ -61,4 +61,6 @@ to brightness information.
     pixfmt-nv16
     pixfmt-nv16m
     pixfmt-nv24
+    pixfmt-nv15
+    pixfmt-nv20
     pixfmt-m420
diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 4102c373b48a..0caac755d303 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -267,6 +267,9 @@ const struct v4l2_format_info *v4l2_format_info(u32 format)
 		{ .format = V4L2_PIX_FMT_NV24,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 1, .vdiv = 1 },
 		{ .format = V4L2_PIX_FMT_NV42,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 1, .vdiv = 1 },
 
+		{ .format = V4L2_PIX_FMT_NV15,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 5, 0, 0 }, .hdiv = 2, .vdiv = 2, .block_w = { 4, 2, 0, 0 }, .block_h = { 1, 1, 0, 0 } },
+		{ .format = V4L2_PIX_FMT_NV20,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 5, 0, 0 }, .hdiv = 2, .vdiv = 1, .block_w = { 4, 2, 0, 0 }, .block_h = { 1, 1, 0, 0 } },
+
 		{ .format = V4L2_PIX_FMT_YUV410,  .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 4, .vdiv = 4 },
 		{ .format = V4L2_PIX_FMT_YVU410,  .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 4, .vdiv = 4 },
 		{ .format = V4L2_PIX_FMT_YUV411P, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 4, .vdiv = 1 },
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 02bfef0da76d..4657274bb37a 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1315,6 +1315,8 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
 	case V4L2_PIX_FMT_NV61:		descr = "Y/CrCb 4:2:2"; break;
 	case V4L2_PIX_FMT_NV24:		descr = "Y/CbCr 4:4:4"; break;
 	case V4L2_PIX_FMT_NV42:		descr = "Y/CrCb 4:4:4"; break;
+	case V4L2_PIX_FMT_NV15:		descr = "10-bit Y/CbCr 4:2:0 (Packed)"; break;
+	case V4L2_PIX_FMT_NV20:		descr = "10-bit Y/CbCr 4:2:2 (Packed)"; break;
 	case V4L2_PIX_FMT_NV12M:	descr = "Y/CbCr 4:2:0 (N-C)"; break;
 	case V4L2_PIX_FMT_NV21M:	descr = "Y/CrCb 4:2:0 (N-C)"; break;
 	case V4L2_PIX_FMT_NV16M:	descr = "Y/CbCr 4:2:2 (N-C)"; break;
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 303805438814..bd23aeaf0706 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -610,6 +610,9 @@ struct v4l2_pix_format {
 #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_NV15    v4l2_fourcc('N', 'V', '1', '5') /* 15  Y/CbCr 4:2:0 10-bit packed */
+#define V4L2_PIX_FMT_NV20    v4l2_fourcc('N', 'V', '2', '0') /* 20  Y/CbCr 4:2:2 10-bit packed */
+
 /* two non contiguous planes - one Y, one Cr + Cb interleaved  */
 #define V4L2_PIX_FMT_NV12M   v4l2_fourcc('N', 'M', '1', '2') /* 12  Y/CbCr 4:2:0  */
 #define V4L2_PIX_FMT_NV21M   v4l2_fourcc('N', 'M', '2', '1') /* 21  Y/CrCb 4:2:0  */
-- 
2.17.1

[PATCH 7/9] media: rkvdec: h264: Use bytesperline and buffer height to calculate stride

[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 was is configured in HW.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 27 +++++++++++++---------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 9c8e49642cd9..1cb6af590138 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -891,10 +891,11 @@ 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 yuv_virstride = 0;
+	u32 hor_virstride;
+	u32 ver_virstride;
+	u32 y_virstride;
+	u32 uv_virstride;
+	u32 yuv_virstride;
 	u32 offset;
 	dma_addr_t dst_addr;
 	u32 reg, i;
@@ -904,16 +905,20 @@ 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;
+	if (sps->chroma_format_idc == 1)
+		uv_virstride = y_virstride / 2;
 	else if (sps->chroma_format_idc == 2)
-		yuv_virstride += 2 * y_virstride;
+		uv_virstride = y_virstride;
+	else if (sps->chroma_format_idc == 3)
+		uv_virstride = 2 * y_virstride;
+	else
+		uv_virstride = 0;
+
+	yuv_virstride = y_virstride + uv_virstride;
 
 	reg = RKVDEC_Y_HOR_VIRSTRIDE(hor_virstride / 16) |
 	      RKVDEC_UV_HOR_VIRSTRIDE(hor_virstride / 16) |
-- 
2.17.1

Re: [PATCH 7/9] media: rkvdec: h264: Use bytesperline and buffer height to calculate stride

[Ezequiel Garcia]

Hi Jonas,

On Wed, 2020-07-01 at 21:56 +0000, Jonas Karlman wrote:
> 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 was is configured in HW.
> 

Are you seeing any issue due to this?

> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  drivers/staging/media/rkvdec/rkvdec-h264.c | 27 +++++++++++++---------
>  1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
> index 9c8e49642cd9..1cb6af590138 100644
> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
> @@ -891,10 +891,11 @@ 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 yuv_virstride = 0;
> +	u32 hor_virstride;
> +	u32 ver_virstride;
> +	u32 y_virstride;
> +	u32 uv_virstride;
> +	u32 yuv_virstride;
>  	u32 offset;
>  	dma_addr_t dst_addr;
>  	u32 reg, i;
> @@ -904,16 +905,20 @@ 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;
>  

So far so good.

> -	if (sps->chroma_format_idc == 0)
> -		yuv_virstride = y_virstride;
> -	else if (sps->chroma_format_idc == 1)
> -		yuv_virstride += y_virstride + y_virstride / 2;
> +	if (sps->chroma_format_idc == 1)
> +		uv_virstride = y_virstride / 2;
>  	else if (sps->chroma_format_idc == 2)
> -		yuv_virstride += 2 * y_virstride;
> +		uv_virstride = y_virstride;
> +	else if (sps->chroma_format_idc == 3)
> +		uv_virstride = 2 * y_virstride;
> +	else
> +		uv_virstride = 0;
> +
> +	yuv_virstride = y_virstride + uv_virstride;
>  

Is the chunk above related to the patch, or mostly
cleaning/improving the code?

Thanks,
Ezequiel

>  	reg = RKVDEC_Y_HOR_VIRSTRIDE(hor_virstride / 16) |
>  	      RKVDEC_UV_HOR_VIRSTRIDE(hor_virstride / 16) |

Re: [PATCH 7/9] media: rkvdec: h264: Use bytesperline and buffer height to calculate stride

[Jonas Karlman]

On 2020-07-03 05:21, Ezequiel Garcia wrote:
> Hi Jonas,
> 
> On Wed, 2020-07-01 at 21:56 +0000, Jonas Karlman wrote:
>> 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 was is configured in HW.
>>
> 
> Are you seeing any issue due to this?

Not seeing any issue, I just feelt more confident when both the driver and
application use the same value to configure the stride and when used for
drm framebuffer pitch.

> 
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>>  drivers/staging/media/rkvdec/rkvdec-h264.c | 27 +++++++++++++---------
>>  1 file changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
>> index 9c8e49642cd9..1cb6af590138 100644
>> --- a/drivers/staging/media/rkvdec/rkvdec-h264.c
>> +++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
>> @@ -891,10 +891,11 @@ 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 yuv_virstride = 0;
>> +	u32 hor_virstride;
>> +	u32 ver_virstride;
>> +	u32 y_virstride;
>> +	u32 uv_virstride;
>> +	u32 yuv_virstride;
>>  	u32 offset;
>>  	dma_addr_t dst_addr;
>>  	u32 reg, i;
>> @@ -904,16 +905,20 @@ 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;
>>  
> 
> So far so good.
> 
>> -	if (sps->chroma_format_idc == 0)
>> -		yuv_virstride = y_virstride;
>> -	else if (sps->chroma_format_idc == 1)
>> -		yuv_virstride += y_virstride + y_virstride / 2;
>> +	if (sps->chroma_format_idc == 1)
>> +		uv_virstride = y_virstride / 2;
>>  	else if (sps->chroma_format_idc == 2)
>> -		yuv_virstride += 2 * y_virstride;
>> +		uv_virstride = y_virstride;
>> +	else if (sps->chroma_format_idc == 3)
>> +		uv_virstride = 2 * y_virstride;
>> +	else
>> +		uv_virstride = 0;
>> +
>> +	yuv_virstride = y_virstride + uv_virstride;
>>  
> 
> Is the chunk above related to the patch, or mostly
> cleaning/improving the code?

You are correct it is mostly cleaning/improving the code,
this should probably be moved to a separate patch or skipped altogether.

Initial 10-bit implementation was made for HEVC so I just backported the code
I ended up with for HEVC back to H.264. Will skip this cleaning/improving in
this series and possible include it as part of a future HEVC series.

Regards,
Jonas

> 
> Thanks,
> Ezequiel
> 
>>  	reg = RKVDEC_Y_HOR_VIRSTRIDE(hor_virstride / 16) |
>>  	      RKVDEC_UV_HOR_VIRSTRIDE(hor_virstride / 16) |
> 
> 

[PATCH 8/9] media: rkvdec: Add validate_fmt ops for pixelformat validation

[Jonas Karlman]

Add an optional validate_fmt operation that is used to validate the
pixelformat of CAPTURE buffers.

This is used in next patch to ensure correct pixelformat is used for 10-bit
and 4:2:2 content.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 drivers/staging/media/rkvdec/rkvdec.c | 8 ++++++++
 drivers/staging/media/rkvdec/rkvdec.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index b1de55aa6535..465444c58f13 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -239,6 +239,14 @@ static int rkvdec_try_capture_fmt(struct file *file, void *priv,
 	if (WARN_ON(!coded_desc))
 		return -EINVAL;
 
+	if (coded_desc->ops->validate_fmt) {
+		int ret;
+
+		ret = coded_desc->ops->validate_fmt(ctx, pix_mp->pixelformat);
+		if (ret)
+			return ret;
+	}
+
 	for (i = 0; i < coded_desc->num_decoded_fmts; i++) {
 		if (coded_desc->decoded_fmts[i] == pix_mp->pixelformat)
 			break;
diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
index 2fc9f46b6910..be4fc3645cde 100644
--- a/drivers/staging/media/rkvdec/rkvdec.h
+++ b/drivers/staging/media/rkvdec/rkvdec.h
@@ -64,6 +64,7 @@ vb2_to_rkvdec_decoded_buf(struct vb2_buffer *buf)
 struct rkvdec_coded_fmt_ops {
 	int (*adjust_fmt)(struct rkvdec_ctx *ctx,
 			  struct v4l2_format *f);
+	int (*validate_fmt)(struct rkvdec_ctx *ctx, u32 pixelformat);
 	int (*start)(struct rkvdec_ctx *ctx);
 	void (*stop)(struct rkvdec_ctx *ctx);
 	int (*run)(struct rkvdec_ctx *ctx);
-- 
2.17.1

Re: [PATCH 8/9] media: rkvdec: Add validate_fmt ops for pixelformat validation

[Ezequiel Garcia]

Hi Jonas,

Thanks for working on this.

On Wed, 2020-07-01 at 21:56 +0000, Jonas Karlman wrote:
> Add an optional validate_fmt operation that is used to validate the
> pixelformat of CAPTURE buffers.
> 
> This is used in next patch to ensure correct pixelformat is used for 10-bit
> and 4:2:2 content.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  drivers/staging/media/rkvdec/rkvdec.c | 8 ++++++++
>  drivers/staging/media/rkvdec/rkvdec.h | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> index b1de55aa6535..465444c58f13 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.c
> +++ b/drivers/staging/media/rkvdec/rkvdec.c
> @@ -239,6 +239,14 @@ static int rkvdec_try_capture_fmt(struct file *file, void *priv,
>  	if (WARN_ON(!coded_desc))
>  		return -EINVAL;
>  
> +	if (coded_desc->ops->validate_fmt) {
> +		int ret;
> +
> +		ret = coded_desc->ops->validate_fmt(ctx, pix_mp->pixelformat);
> +		if (ret)
> +			return ret;
> +	}
> + 

I don't think this approach will be enough. Unless I'm mistaken,
it's perfectly legal as per the stateless spec to first
call S_FMT on the OUTPUT queue (which is propagated to the CAPTURE side),
and then set the SPS and other controls.

The application is not required to do a TRY_FMT after S_EXT_CTRLS.

What I believe is needed is for the S_EXT_CTRLS to modify
and restrict the CAPTURE format accordingly, so the application
gets the correct format on G_FMT (and restrict future TRY_FMT).

Also, V4L2 spec asks drivers not to fail on S_FMT
format mismatch, but instead to adjust and return a legal format
back to the application [1].

Let me know what you think and thanks again.

Ezequiel

[1] Documentation/userspace-api/media/v4l/vidioc-g-fmt.rst

>  	for (i = 0; i < coded_desc->num_decoded_fmts; i++) {
>  		if (coded_desc->decoded_fmts[i] == pix_mp->pixelformat)
>  			break;
> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
> index 2fc9f46b6910..be4fc3645cde 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.h
> +++ b/drivers/staging/media/rkvdec/rkvdec.h
> @@ -64,6 +64,7 @@ vb2_to_rkvdec_decoded_buf(struct vb2_buffer *buf)
>  struct rkvdec_coded_fmt_ops {
>  	int (*adjust_fmt)(struct rkvdec_ctx *ctx,
>  			  struct v4l2_format *f);
> +	int (*validate_fmt)(struct rkvdec_ctx *ctx, u32 pixelformat);
>  	int (*start)(struct rkvdec_ctx *ctx);
>  	void (*stop)(struct rkvdec_ctx *ctx);
>  	int (*run)(struct rkvdec_ctx *ctx);

Re: [PATCH 8/9] media: rkvdec: Add validate_fmt ops for pixelformat validation

[Jonas Karlman]

On 2020-07-03 05:14, Ezequiel Garcia wrote:
> Hi Jonas,
> 
> Thanks for working on this.
> 
> On Wed, 2020-07-01 at 21:56 +0000, Jonas Karlman wrote:
>> Add an optional validate_fmt operation that is used to validate the
>> pixelformat of CAPTURE buffers.
>>
>> This is used in next patch to ensure correct pixelformat is used for 10-bit
>> and 4:2:2 content.
>>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>>  drivers/staging/media/rkvdec/rkvdec.c | 8 ++++++++
>>  drivers/staging/media/rkvdec/rkvdec.h | 1 +
>>  2 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
>> index b1de55aa6535..465444c58f13 100644
>> --- a/drivers/staging/media/rkvdec/rkvdec.c
>> +++ b/drivers/staging/media/rkvdec/rkvdec.c
>> @@ -239,6 +239,14 @@ static int rkvdec_try_capture_fmt(struct file *file, void *priv,
>>  	if (WARN_ON(!coded_desc))
>>  		return -EINVAL;
>>  
>> +	if (coded_desc->ops->validate_fmt) {
>> +		int ret;
>> +
>> +		ret = coded_desc->ops->validate_fmt(ctx, pix_mp->pixelformat);
>> +		if (ret)
>> +			return ret;
>> +	}
>> + 
> 
> I don't think this approach will be enough. Unless I'm mistaken,
> it's perfectly legal as per the stateless spec to first
> call S_FMT on the OUTPUT queue (which is propagated to the CAPTURE side),
> and then set the SPS and other controls.

I agree that this will not be enough to cover all use cases stated in the spec.

> 
> The application is not required to do a TRY_FMT after S_EXT_CTRLS.

If I remember correctly we were required to implement a TRY_FMT loop in
ffmpeg due to cedrus defaulting to SUNXI_TILED_NV12 instead of linear NV12
on platforms where display controller did not support the tiled modifier.

So having TRY_FMT as part of the init sequence has been my only test-case.

> 
> What I believe is needed is for the S_EXT_CTRLS to modify
> and restrict the CAPTURE format accordingly, so the application
> gets the correct format on G_FMT (and restrict future TRY_FMT).

This sounds like a proper solution, I do belive we may have a chicken or
the egg problem depending on if application call S_EXT_CTRLS or S_FMT first.

I guess we may need to lock down on a format at whatever comes first,
S_FMT on CAPTURE or S_EXT_CTRLS with SPS ctrl.

I have an idea on how this could be addressed, will explore and see
if I can come up with something new.

Regards,
Jonas

> 
> Also, V4L2 spec asks drivers not to fail on S_FMT
> format mismatch, but instead to adjust and return a legal format
> back to the application [1].
> 
> Let me know what you think and thanks again.
> 
> Ezequiel
> 
> [1] Documentation/userspace-api/media/v4l/vidioc-g-fmt.rst
> 
>>  	for (i = 0; i < coded_desc->num_decoded_fmts; i++) {
>>  		if (coded_desc->decoded_fmts[i] == pix_mp->pixelformat)
>>  			break;
>> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
>> index 2fc9f46b6910..be4fc3645cde 100644
>> --- a/drivers/staging/media/rkvdec/rkvdec.h
>> +++ b/drivers/staging/media/rkvdec/rkvdec.h
>> @@ -64,6 +64,7 @@ vb2_to_rkvdec_decoded_buf(struct vb2_buffer *buf)
>>  struct rkvdec_coded_fmt_ops {
>>  	int (*adjust_fmt)(struct rkvdec_ctx *ctx,
>>  			  struct v4l2_format *f);
>> +	int (*validate_fmt)(struct rkvdec_ctx *ctx, u32 pixelformat);
>>  	int (*start)(struct rkvdec_ctx *ctx);
>>  	void (*stop)(struct rkvdec_ctx *ctx);
>>  	int (*run)(struct rkvdec_ctx *ctx);
> 
> 

Re: [PATCH 8/9] media: rkvdec: Add validate_fmt ops for pixelformat validation

[Ezequiel Garcia]

On Fri, 2020-07-03 at 06:55 +0000, Jonas Karlman wrote:
> On 2020-07-03 05:14, Ezequiel Garcia wrote:
> > Hi Jonas,
> > 
> > Thanks for working on this.
> > 
> > On Wed, 2020-07-01 at 21:56 +0000, Jonas Karlman wrote:
> > > Add an optional validate_fmt operation that is used to validate the
> > > pixelformat of CAPTURE buffers.
> > > 
> > > This is used in next patch to ensure correct pixelformat is used for 10-bit
> > > and 4:2:2 content.
> > > 
> > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > > ---
> > >  drivers/staging/media/rkvdec/rkvdec.c | 8 ++++++++
> > >  drivers/staging/media/rkvdec/rkvdec.h | 1 +
> > >  2 files changed, 9 insertions(+)
> > > 
> > > diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> > > index b1de55aa6535..465444c58f13 100644
> > > --- a/drivers/staging/media/rkvdec/rkvdec.c
> > > +++ b/drivers/staging/media/rkvdec/rkvdec.c
> > > @@ -239,6 +239,14 @@ static int rkvdec_try_capture_fmt(struct file *file, void *priv,
> > >  	if (WARN_ON(!coded_desc))
> > >  		return -EINVAL;
> > >  
> > > +	if (coded_desc->ops->validate_fmt) {
> > > +		int ret;
> > > +
> > > +		ret = coded_desc->ops->validate_fmt(ctx, pix_mp->pixelformat);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > + 
> > 
> > I don't think this approach will be enough. Unless I'm mistaken,
> > it's perfectly legal as per the stateless spec to first
> > call S_FMT on the OUTPUT queue (which is propagated to the CAPTURE side),
> > and then set the SPS and other controls.
> 
> I agree that this will not be enough to cover all use cases stated in the spec.
> 
> > The application is not required to do a TRY_FMT after S_EXT_CTRLS.
> 
> If I remember correctly we were required to implement a TRY_FMT loop in
> ffmpeg due to cedrus defaulting to SUNXI_TILED_NV12 instead of linear NV12
> on platforms where display controller did not support the tiled modifier.
> 
> So having TRY_FMT as part of the init sequence has been my only test-case.
> 
> > What I believe is needed is for the S_EXT_CTRLS to modify
> > and restrict the CAPTURE format accordingly, so the application
> > gets the correct format on G_FMT (and restrict future TRY_FMT).
> 
> This sounds like a proper solution, I do belive we may have a chicken or
> the egg problem depending on if application call S_EXT_CTRLS or S_FMT first.
> 

IIUC, the order is specified in the stateless spec [1].

1) S_FMT on OUTPUT (to set the coded pixelformat). CAPTURE format
format is propagated here and a default format is set.

2) S_EXT_CTRLS, parameters are set. We don't do anything here,
but here we'd validate the SPS and restrict the CAPTURE pixelformat
(and perhaps reset the default CAPTURE pixelformat).

3) G_FMT on CAPTURE.

4) (optional) ENUM_FMT / S_FMT on CAPTURE, to negotiate
something different from default.

Regards,
Ezequiel 

[1] Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst

> I guess we may need to lock down on a format at whatever comes first,
> S_FMT on CAPTURE or S_EXT_CTRLS with SPS ctrl.
> 
> I have an idea on how this could be addressed, will explore and see
> if I can come up with something new.
> 
> Regards,
> Jonas
> 
> > Also, V4L2 spec asks drivers not to fail on S_FMT
> > format mismatch, but instead to adjust and return a legal format
> > back to the application [1].
> > 
> > Let me know what you think and thanks again.
> > 
> > Ezequiel
> > 
> > [1] Documentation/userspace-api/media/v4l/vidioc-g-fmt.rst
> > 
> > >  	for (i = 0; i < coded_desc->num_decoded_fmts; i++) {
> > >  		if (coded_desc->decoded_fmts[i] == pix_mp->pixelformat)
> > >  			break;
> > > diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
> > > index 2fc9f46b6910..be4fc3645cde 100644
> > > --- a/drivers/staging/media/rkvdec/rkvdec.h
> > > +++ b/drivers/staging/media/rkvdec/rkvdec.h
> > > @@ -64,6 +64,7 @@ vb2_to_rkvdec_decoded_buf(struct vb2_buffer *buf)
> > >  struct rkvdec_coded_fmt_ops {
> > >  	int (*adjust_fmt)(struct rkvdec_ctx *ctx,
> > >  			  struct v4l2_format *f);
> > > +	int (*validate_fmt)(struct rkvdec_ctx *ctx, u32 pixelformat);
> > >  	int (*start)(struct rkvdec_ctx *ctx);
> > >  	void (*stop)(struct rkvdec_ctx *ctx);
> > >  	int (*run)(struct rkvdec_ctx *ctx);

Re: [PATCH 8/9] media: rkvdec: Add validate_fmt ops for pixelformat validation

[Jonas Karlman]

On 2020-07-03 16:58, Ezequiel Garcia wrote:
> On Fri, 2020-07-03 at 06:55 +0000, Jonas Karlman wrote:
>> On 2020-07-03 05:14, Ezequiel Garcia wrote:
>>> Hi Jonas,
>>>
>>> Thanks for working on this.
>>>
>>> On Wed, 2020-07-01 at 21:56 +0000, Jonas Karlman wrote:
>>>> Add an optional validate_fmt operation that is used to validate the
>>>> pixelformat of CAPTURE buffers.
>>>>
>>>> This is used in next patch to ensure correct pixelformat is used for 10-bit
>>>> and 4:2:2 content.
>>>>
>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>>> ---
>>>>  drivers/staging/media/rkvdec/rkvdec.c | 8 ++++++++
>>>>  drivers/staging/media/rkvdec/rkvdec.h | 1 +
>>>>  2 files changed, 9 insertions(+)
>>>>
>>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
>>>> index b1de55aa6535..465444c58f13 100644
>>>> --- a/drivers/staging/media/rkvdec/rkvdec.c
>>>> +++ b/drivers/staging/media/rkvdec/rkvdec.c
>>>> @@ -239,6 +239,14 @@ static int rkvdec_try_capture_fmt(struct file *file, void *priv,
>>>>  	if (WARN_ON(!coded_desc))
>>>>  		return -EINVAL;
>>>>  
>>>> +	if (coded_desc->ops->validate_fmt) {
>>>> +		int ret;
>>>> +
>>>> +		ret = coded_desc->ops->validate_fmt(ctx, pix_mp->pixelformat);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +	}
>>>> + 
>>>
>>> I don't think this approach will be enough. Unless I'm mistaken,
>>> it's perfectly legal as per the stateless spec to first
>>> call S_FMT on the OUTPUT queue (which is propagated to the CAPTURE side),
>>> and then set the SPS and other controls.
>>
>> I agree that this will not be enough to cover all use cases stated in the spec.
>>
>>> The application is not required to do a TRY_FMT after S_EXT_CTRLS.
>>
>> If I remember correctly we were required to implement a TRY_FMT loop in
>> ffmpeg due to cedrus defaulting to SUNXI_TILED_NV12 instead of linear NV12
>> on platforms where display controller did not support the tiled modifier.
>>
>> So having TRY_FMT as part of the init sequence has been my only test-case.
>>
>>> What I believe is needed is for the S_EXT_CTRLS to modify
>>> and restrict the CAPTURE format accordingly, so the application
>>> gets the correct format on G_FMT (and restrict future TRY_FMT).
>>
>> This sounds like a proper solution, I do belive we may have a chicken or
>> the egg problem depending on if application call S_EXT_CTRLS or S_FMT first.
>>
> 
> IIUC, the order is specified in the stateless spec [1].
> 
> 1) S_FMT on OUTPUT (to set the coded pixelformat). CAPTURE format
> format is propagated here and a default format is set.
> 
> 2) S_EXT_CTRLS, parameters are set. We don't do anything here,
> but here we'd validate the SPS and restrict the CAPTURE pixelformat
> (and perhaps reset the default CAPTURE pixelformat).
> 
> 3) G_FMT on CAPTURE.
> 
> 4) (optional) ENUM_FMT / S_FMT on CAPTURE, to negotiate
> something different from default.

There is also the following scenario that we may need to support:

1) S_FMT on OUTPUT, default CAPTURE format is set.

2) skip S_EXT_CTRLS, mandatory controls is only validated in req_validate.

3) G_FMT on CAPTURE, returns default CAPTURE format.

4) S_FMT on CAPTURE, CAPTURE format is changed from default to selected format.

5) STREAMON

From this point on I would expect S_EXT_CTRLS with a V4L2_CTRL_WHICH_REQUEST_VAL
flag to reject any SPS not matching the selected CAPTURE format. Effectively
allowing S_FMT to lock down a format instead of an initial S_EXT_CTRLS during init.

This means that we have to both allow and reject a SPS depending on the state.

Regards,
Jonas

> 
> Regards,
> Ezequiel 
> 
> [1] Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
> 
>> I guess we may need to lock down on a format at whatever comes first,
>> S_FMT on CAPTURE or S_EXT_CTRLS with SPS ctrl.
>>
>> I have an idea on how this could be addressed, will explore and see
>> if I can come up with something new.
>>
>> Regards,
>> Jonas
>>
>>> Also, V4L2 spec asks drivers not to fail on S_FMT
>>> format mismatch, but instead to adjust and return a legal format
>>> back to the application [1].
>>>
>>> Let me know what you think and thanks again.
>>>
>>> Ezequiel
>>>
>>> [1] Documentation/userspace-api/media/v4l/vidioc-g-fmt.rst
>>>
>>>>  	for (i = 0; i < coded_desc->num_decoded_fmts; i++) {
>>>>  		if (coded_desc->decoded_fmts[i] == pix_mp->pixelformat)
>>>>  			break;
>>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
>>>> index 2fc9f46b6910..be4fc3645cde 100644
>>>> --- a/drivers/staging/media/rkvdec/rkvdec.h
>>>> +++ b/drivers/staging/media/rkvdec/rkvdec.h
>>>> @@ -64,6 +64,7 @@ vb2_to_rkvdec_decoded_buf(struct vb2_buffer *buf)
>>>>  struct rkvdec_coded_fmt_ops {
>>>>  	int (*adjust_fmt)(struct rkvdec_ctx *ctx,
>>>>  			  struct v4l2_format *f);
>>>> +	int (*validate_fmt)(struct rkvdec_ctx *ctx, u32 pixelformat);
>>>>  	int (*start)(struct rkvdec_ctx *ctx);
>>>>  	void (*stop)(struct rkvdec_ctx *ctx);
>>>>  	int (*run)(struct rkvdec_ctx *ctx);
> 
> 

Re: [PATCH 8/9] media: rkvdec: Add validate_fmt ops for pixelformat validation

[Ezequiel Garcia]

On Fri, 2020-07-03 at 19:17 +0000, Jonas Karlman wrote:
> On 2020-07-03 16:58, Ezequiel Garcia wrote:
> > On Fri, 2020-07-03 at 06:55 +0000, Jonas Karlman wrote:
> > > On 2020-07-03 05:14, Ezequiel Garcia wrote:
> > > > Hi Jonas,
> > > > 
> > > > Thanks for working on this.
> > > > 
> > > > On Wed, 2020-07-01 at 21:56 +0000, Jonas Karlman wrote:
> > > > > Add an optional validate_fmt operation that is used to validate the
> > > > > pixelformat of CAPTURE buffers.
> > > > > 
> > > > > This is used in next patch to ensure correct pixelformat is used for 10-bit
> > > > > and 4:2:2 content.
> > > > > 
> > > > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > > > > ---
> > > > >  drivers/staging/media/rkvdec/rkvdec.c | 8 ++++++++
> > > > >  drivers/staging/media/rkvdec/rkvdec.h | 1 +
> > > > >  2 files changed, 9 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> > > > > index b1de55aa6535..465444c58f13 100644
> > > > > --- a/drivers/staging/media/rkvdec/rkvdec.c
> > > > > +++ b/drivers/staging/media/rkvdec/rkvdec.c
> > > > > @@ -239,6 +239,14 @@ static int rkvdec_try_capture_fmt(struct file *file, void *priv,
> > > > >  	if (WARN_ON(!coded_desc))
> > > > >  		return -EINVAL;
> > > > >  
> > > > > +	if (coded_desc->ops->validate_fmt) {
> > > > > +		int ret;
> > > > > +
> > > > > +		ret = coded_desc->ops->validate_fmt(ctx, pix_mp->pixelformat);
> > > > > +		if (ret)
> > > > > +			return ret;
> > > > > +	}
> > > > > + 
> > > > 
> > > > I don't think this approach will be enough. Unless I'm mistaken,
> > > > it's perfectly legal as per the stateless spec to first
> > > > call S_FMT on the OUTPUT queue (which is propagated to the CAPTURE side),
> > > > and then set the SPS and other controls.
> > > 
> > > I agree that this will not be enough to cover all use cases stated in the spec.
> > > 
> > > > The application is not required to do a TRY_FMT after S_EXT_CTRLS.
> > > 
> > > If I remember correctly we were required to implement a TRY_FMT loop in
> > > ffmpeg due to cedrus defaulting to SUNXI_TILED_NV12 instead of linear NV12
> > > on platforms where display controller did not support the tiled modifier.
> > > 
> > > So having TRY_FMT as part of the init sequence has been my only test-case.
> > > 
> > > > What I believe is needed is for the S_EXT_CTRLS to modify
> > > > and restrict the CAPTURE format accordingly, so the application
> > > > gets the correct format on G_FMT (and restrict future TRY_FMT).
> > > 
> > > This sounds like a proper solution, I do belive we may have a chicken or
> > > the egg problem depending on if application call S_EXT_CTRLS or S_FMT first.
> > > 
> > 
> > IIUC, the order is specified in the stateless spec [1].
> > 
> > 1) S_FMT on OUTPUT (to set the coded pixelformat). CAPTURE format
> > format is propagated here and a default format is set.
> > 
> > 2) S_EXT_CTRLS, parameters are set. We don't do anything here,
> > but here we'd validate the SPS and restrict the CAPTURE pixelformat
> > (and perhaps reset the default CAPTURE pixelformat).
> > 
> > 3) G_FMT on CAPTURE.
> > 
> > 4) (optional) ENUM_FMT / S_FMT on CAPTURE, to negotiate
> > something different from default.
> 
> There is also the following scenario that we may need to support:
> 
> 1) S_FMT on OUTPUT, default CAPTURE format is set.
> 
> 2) skip S_EXT_CTRLS, mandatory controls is only validated in req_validate.
> 
> 3) G_FMT on CAPTURE, returns default CAPTURE format.
> 
> 4) S_FMT on CAPTURE, CAPTURE format is changed from default to selected format.
> 
> 5) STREAMON
> 
> From this point on I would expect S_EXT_CTRLS with a V4L2_CTRL_WHICH_REQUEST_VAL
> flag to reject any SPS not matching the selected CAPTURE format. Effectively
> allowing S_FMT to lock down a format instead of an initial S_EXT_CTRLS during init.
> 
> This means that we have to both allow and reject a SPS depending on the state.
> 

Isn't it cleaner from an API to require the SPS at (2), right before
G_FMT on CAPTURE?

Unless you think it has clear advantages to provide more flexibility
to the user, i.e. it will allow to support specific use-cases,
then I would avoid making this flexible.

The spec currently doesn mention step 2 as being optional, although
there is a note mentioning controls can be overwritten.

Thanks,
Ezequiel

> Regards,
> Jonas
> 
> > Regards,
> > Ezequiel 
> > 
> > [1] Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
> > 
> > > I guess we may need to lock down on a format at whatever comes first,
> > > S_FMT on CAPTURE or S_EXT_CTRLS with SPS ctrl.
> > > 
> > > I have an idea on how this could be addressed, will explore and see
> > > if I can come up with something new.
> > > 
> > > Regards,
> > > Jonas
> > > 
> > > > Also, V4L2 spec asks drivers not to fail on S_FMT
> > > > format mismatch, but instead to adjust and return a legal format
> > > > back to the application [1].
> > > > 
> > > > Let me know what you think and thanks again.
> > > > 
> > > > Ezequiel
> > > > 
> > > > [1] Documentation/userspace-api/media/v4l/vidioc-g-fmt.rst
> > > > 
> > > > >  	for (i = 0; i < coded_desc->num_decoded_fmts; i++) {
> > > > >  		if (coded_desc->decoded_fmts[i] == pix_mp->pixelformat)
> > > > >  			break;
> > > > > diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
> > > > > index 2fc9f46b6910..be4fc3645cde 100644
> > > > > --- a/drivers/staging/media/rkvdec/rkvdec.h
> > > > > +++ b/drivers/staging/media/rkvdec/rkvdec.h
> > > > > @@ -64,6 +64,7 @@ vb2_to_rkvdec_decoded_buf(struct vb2_buffer *buf)
> > > > >  struct rkvdec_coded_fmt_ops {
> > > > >  	int (*adjust_fmt)(struct rkvdec_ctx *ctx,
> > > > >  			  struct v4l2_format *f);
> > > > > +	int (*validate_fmt)(struct rkvdec_ctx *ctx, u32 pixelformat);
> > > > >  	int (*start)(struct rkvdec_ctx *ctx);
> > > > >  	void (*stop)(struct rkvdec_ctx *ctx);
> > > > >  	int (*run)(struct rkvdec_ctx *ctx);

Re: [PATCH 8/9] media: rkvdec: Add validate_fmt ops for pixelformat validation

[Tomasz Figa]

On Fri, Jul 3, 2020 at 9:18 PM Jonas Karlman <jonas@kwiboo.se> wrote:
>
> On 2020-07-03 16:58, Ezequiel Garcia wrote:
> > On Fri, 2020-07-03 at 06:55 +0000, Jonas Karlman wrote:
> >> On 2020-07-03 05:14, Ezequiel Garcia wrote:
> >>> Hi Jonas,
> >>>
> >>> Thanks for working on this.
> >>>
> >>> On Wed, 2020-07-01 at 21:56 +0000, Jonas Karlman wrote:
> >>>> Add an optional validate_fmt operation that is used to validate the
> >>>> pixelformat of CAPTURE buffers.
> >>>>
> >>>> This is used in next patch to ensure correct pixelformat is used for 10-bit
> >>>> and 4:2:2 content.
> >>>>
> >>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> >>>> ---
> >>>>  drivers/staging/media/rkvdec/rkvdec.c | 8 ++++++++
> >>>>  drivers/staging/media/rkvdec/rkvdec.h | 1 +
> >>>>  2 files changed, 9 insertions(+)
> >>>>
> >>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> >>>> index b1de55aa6535..465444c58f13 100644
> >>>> --- a/drivers/staging/media/rkvdec/rkvdec.c
> >>>> +++ b/drivers/staging/media/rkvdec/rkvdec.c
> >>>> @@ -239,6 +239,14 @@ static int rkvdec_try_capture_fmt(struct file *file, void *priv,
> >>>>    if (WARN_ON(!coded_desc))
> >>>>            return -EINVAL;
> >>>>
> >>>> +  if (coded_desc->ops->validate_fmt) {
> >>>> +          int ret;
> >>>> +
> >>>> +          ret = coded_desc->ops->validate_fmt(ctx, pix_mp->pixelformat);
> >>>> +          if (ret)
> >>>> +                  return ret;
> >>>> +  }
> >>>> +
> >>>
> >>> I don't think this approach will be enough. Unless I'm mistaken,
> >>> it's perfectly legal as per the stateless spec to first
> >>> call S_FMT on the OUTPUT queue (which is propagated to the CAPTURE side),
> >>> and then set the SPS and other controls.
> >>
> >> I agree that this will not be enough to cover all use cases stated in the spec.
> >>
> >>> The application is not required to do a TRY_FMT after S_EXT_CTRLS.
> >>
> >> If I remember correctly we were required to implement a TRY_FMT loop in
> >> ffmpeg due to cedrus defaulting to SUNXI_TILED_NV12 instead of linear NV12
> >> on platforms where display controller did not support the tiled modifier.
> >>
> >> So having TRY_FMT as part of the init sequence has been my only test-case.
> >>
> >>> What I believe is needed is for the S_EXT_CTRLS to modify
> >>> and restrict the CAPTURE format accordingly, so the application
> >>> gets the correct format on G_FMT (and restrict future TRY_FMT).
> >>
> >> This sounds like a proper solution, I do belive we may have a chicken or
> >> the egg problem depending on if application call S_EXT_CTRLS or S_FMT first.
> >>
> >
> > IIUC, the order is specified in the stateless spec [1].
> >
> > 1) S_FMT on OUTPUT (to set the coded pixelformat). CAPTURE format
> > format is propagated here and a default format is set.
> >
> > 2) S_EXT_CTRLS, parameters are set. We don't do anything here,
> > but here we'd validate the SPS and restrict the CAPTURE pixelformat
> > (and perhaps reset the default CAPTURE pixelformat).
> >
> > 3) G_FMT on CAPTURE.
> >
> > 4) (optional) ENUM_FMT / S_FMT on CAPTURE, to negotiate
> > something different from default.
>
> There is also the following scenario that we may need to support:
>
> 1) S_FMT on OUTPUT, default CAPTURE format is set.
>
> 2) skip S_EXT_CTRLS, mandatory controls is only validated in req_validate.
>
> 3) G_FMT on CAPTURE, returns default CAPTURE format.
>
> 4) S_FMT on CAPTURE, CAPTURE format is changed from default to selected format.
>
> 5) STREAMON
>
> From this point on I would expect S_EXT_CTRLS with a V4L2_CTRL_WHICH_REQUEST_VAL
> flag to reject any SPS not matching the selected CAPTURE format. Effectively
> allowing S_FMT to lock down a format instead of an initial S_EXT_CTRLS during init.
>
> This means that we have to both allow and reject a SPS depending on the state.
>

That would end up with a really bad API behavior inconsistency. Rather
than that, we have to define what is the authoritative source of the
width and height and I believe that for streams that have this kind of
information in the metadata (e.g. header controls), that should be the
metadata.

So, for example, for H.264, the driver would have to always keep the
width and height of the OUTPUT format fixed to whatever the SPS
control specifies, regardless of what one attempts to set via S_FMT.
If an SPS is set that has different width and height values, the
OUTPUT format should be updated by the driver, in turn the CAPTURE
format should be reset as well and then a SOURCE_CHANGE event should
be signaled, like with the stateful decoders.

Best regards,
Tomasz

> Regards,
> Jonas
>
> >
> > Regards,
> > Ezequiel
> >
> > [1] Documentation/userspace-api/media/v4l/dev-stateless-decoder.rst
> >
> >> I guess we may need to lock down on a format at whatever comes first,
> >> S_FMT on CAPTURE or S_EXT_CTRLS with SPS ctrl.
> >>
> >> I have an idea on how this could be addressed, will explore and see
> >> if I can come up with something new.
> >>
> >> Regards,
> >> Jonas
> >>
> >>> Also, V4L2 spec asks drivers not to fail on S_FMT
> >>> format mismatch, but instead to adjust and return a legal format
> >>> back to the application [1].
> >>>
> >>> Let me know what you think and thanks again.
> >>>
> >>> Ezequiel
> >>>
> >>> [1] Documentation/userspace-api/media/v4l/vidioc-g-fmt.rst
> >>>
> >>>>    for (i = 0; i < coded_desc->num_decoded_fmts; i++) {
> >>>>            if (coded_desc->decoded_fmts[i] == pix_mp->pixelformat)
> >>>>                    break;
> >>>> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
> >>>> index 2fc9f46b6910..be4fc3645cde 100644
> >>>> --- a/drivers/staging/media/rkvdec/rkvdec.h
> >>>> +++ b/drivers/staging/media/rkvdec/rkvdec.h
> >>>> @@ -64,6 +64,7 @@ vb2_to_rkvdec_decoded_buf(struct vb2_buffer *buf)
> >>>>  struct rkvdec_coded_fmt_ops {
> >>>>    int (*adjust_fmt)(struct rkvdec_ctx *ctx,
> >>>>                      struct v4l2_format *f);
> >>>> +  int (*validate_fmt)(struct rkvdec_ctx *ctx, u32 pixelformat);
> >>>>    int (*start)(struct rkvdec_ctx *ctx);
> >>>>    void (*stop)(struct rkvdec_ctx *ctx);
> >>>>    int (*run)(struct rkvdec_ctx *ctx);
> >
> >

[PATCH 9/9] 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.

H264 frmsize step_width/height is changed to 64 pixels in order to ensure
proper align for 10-bit formats. Incompatible pixelformats gets rejected in
s_fmt/try_fmt ioctl call.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 31 ++++++++++++++++++++++
 drivers/staging/media/rkvdec/rkvdec.c      | 24 ++++++++---------
 2 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 1cb6af590138..73767e5c752f 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -1022,6 +1022,36 @@ static int rkvdec_h264_adjust_fmt(struct rkvdec_ctx *ctx,
 	return 0;
 }
 
+static int rkvdec_h264_validate_fmt(struct rkvdec_ctx *ctx, u32 pixelformat)
+{
+	struct v4l2_ctrl *ctrl;
+	const struct v4l2_ctrl_h264_sps *sps;
+	u32 valid_format = 0;
+
+	ctrl = v4l2_ctrl_find(&ctx->ctrl_hdl,
+			      V4L2_CID_MPEG_VIDEO_H264_SPS);
+	sps = ctrl ? ctrl->p_cur.p : NULL;
+	if (!sps)
+		return 0;
+
+	if (sps->bit_depth_luma_minus8 == 0) {
+		if (sps->chroma_format_idc == 2)
+			valid_format = V4L2_PIX_FMT_NV16;
+		else
+			valid_format = V4L2_PIX_FMT_NV12;
+	} else if (sps->bit_depth_luma_minus8 == 2) {
+		if (sps->chroma_format_idc == 2)
+			valid_format = V4L2_PIX_FMT_NV20;
+		else
+			valid_format = V4L2_PIX_FMT_NV15;
+	}
+
+	if (valid_format == pixelformat)
+		return 0;
+
+	return -EINVAL;
+}
+
 static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
 {
 	struct rkvdec_dev *rkvdec = ctx->dev;
@@ -1163,6 +1193,7 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
 
 const struct rkvdec_coded_fmt_ops rkvdec_h264_fmt_ops = {
 	.adjust_fmt = rkvdec_h264_adjust_fmt,
+	.validate_fmt = rkvdec_h264_validate_fmt,
 	.start = rkvdec_h264_start,
 	.stop = rkvdec_h264_stop,
 	.run = rkvdec_h264_run,
diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 465444c58f13..8d88fa8c4d4e 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -31,19 +31,14 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
 {
 	if (ctrl->id == V4L2_CID_MPEG_VIDEO_H264_SPS) {
 		const struct v4l2_ctrl_h264_sps *sps = ctrl->p_cur.p;
-		/*
-		 * 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;
 	}
 	return 0;
@@ -97,7 +92,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,
@@ -116,16 +111,19 @@ static const struct rkvdec_ctrls rkvdec_h264_ctrls = {
 
 static const u32 rkvdec_h264_decoded_fmts[] = {
 	V4L2_PIX_FMT_NV12,
+	V4L2_PIX_FMT_NV15,
+	V4L2_PIX_FMT_NV16,
+	V4L2_PIX_FMT_NV20,
 };
 
 static const struct rkvdec_coded_fmt_desc rkvdec_coded_fmts[] = {
 	{
 		.fourcc = V4L2_PIX_FMT_H264_SLICE,
 		.frmsize = {
-			.min_width = 48,
+			.min_width = 64,
 			.max_width = 4096,
-			.step_width = 16,
-			.min_height = 48,
+			.step_width = 64,
+			.min_height = 64,
 			.max_height = 2304,
 			.step_height = 16,
 		},
-- 
2.17.1

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

[Jonas Karlman]

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

Patch 1 and 3 fixes two issues when decoding field encoded content. Patch 2
ensures CAPTURE buffer is same resolution or larger than OUTPUT buffer.

Patch 5 allows userspace to set OUTPUT sizeimage.

Patch 6 updates the v4l2_format helpers to consider block width and height
when calculating plane bytesperline and sizeimage.

Patch 7 adds two new pixelformats for 10-bit 4:2:0/4:2:2.

Patch 8 change to use bytesperline and buffer height to configure strides.

Patch 9 and 10 add support for limit/lock down a pixelformat based on SPS.

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

Patch 12 adds profile and level controls.

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
See indivitual patch for changes.

This series depend on the "handle unsupported H.264 bitstreams" series at [1]
with a small fixup, s/p_cur/p_new/.

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

[1] https://patchwork.linuxtv.org/cover/64977/
[2] https://patchwork.freedesktop.org/series/78099/
[3] https://github.com/Kwiboo/FFmpeg/commits/v4l2-request-hwaccel-4.3-rkvdec-high-10
[4] https://github.com/Kwiboo/linux-rockchip/commits/linuxtv-rkvdec-high-10-v2

Regards,
Jonas

Jonas Karlman (12):
  media: rkvdec: h264: Fix reference frame_num wrap for second field
  media: rkvdec: Ensure decoded resolution fit coded resolution
  media: rkvdec: h264: Validate and use pic width and height in mbs
  media: rkvdec: h264: Fix bit depth wrap in pps packet
  media: rkvdec: h264: Do not override output buffer sizeimage
  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 to calculate
    stride
  media: rkvdec: Extract rkvdec_fill_decoded_pixfmt helper method
  media: rkvdec: Lock capture pixel format in s_ctrl and s_fmt
  media: rkvdec: h264: Support High 10 and 4:2:2 profiles
  media: rkvdec: h264: Support profile and level controls

 .../userspace-api/media/v4l/pixfmt-nv15.rst   | 101 ++++++++++++++
 .../userspace-api/media/v4l/pixfmt-nv20.rst   |  99 +++++++++++++
 .../userspace-api/media/v4l/yuv-formats.rst   |   2 +
 drivers/media/v4l2-core/v4l2-common.c         |  80 +++++------
 drivers/media/v4l2-core/v4l2-ioctl.c          |   2 +
 drivers/staging/media/rkvdec/rkvdec-h264.c    |  45 ++++--
 drivers/staging/media/rkvdec/rkvdec.c         | 130 ++++++++++++++----
 drivers/staging/media/rkvdec/rkvdec.h         |   2 +
 include/uapi/linux/videodev2.h                |   3 +
 9 files changed, 384 insertions(+), 80 deletions(-)
 create mode 100644 Documentation/userspace-api/media/v4l/pixfmt-nv15.rst
 create mode 100644 Documentation/userspace-api/media/v4l/pixfmt-nv20.rst

-- 
2.17.1

[PATCH v2 01/12] media: rkvdec: h264: Fix reference frame_num wrap for second field

[Jonas Karlman]

When decoding the second field in a complementary field pair the second
field is sharing the same frame_num with the first field.

Currently the frame_num for the first field is wrapped when it matches the
field being decoded, this cause issues to decode the second field in a
complementary field pair.

Fix this by using inclusive comparison, less than or equal.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
---
Changes in v2:
- Collect r-b tag
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 7b66e2743a4f..f0cfed84d60d 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -754,7 +754,7 @@ static void assemble_hw_rps(struct rkvdec_ctx *ctx,
 			continue;
 
 		if (dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_LONG_TERM ||
-		    dpb[i].frame_num < sl_params->frame_num) {
+		    dpb[i].frame_num <= sl_params->frame_num) {
 			p[i] = dpb[i].frame_num;
 			continue;
 		}
-- 
2.17.1

[PATCH v2 03/12] media: rkvdec: h264: Validate and use pic width and height in mbs

[Jonas Karlman]

The width and height in mbs is currently configured based on OUTPUT buffer
resolution, this works for frame pictures but can cause issues for field
pictures.

When frame_mbs_only_flag is 0 the height in mbs should be height of
the field instead of height of frame.

Validate pic_width_in_mbs_minus1 and pic_height_in_map_units_minus1
against OUTPUT buffer resolution and use these values to configure HW.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
Changes in v2:
- Validate against coded_fmt instead of decoded_fmt
- Validation moved to rkvdec_try_ctrl
---
 drivers/staging/media/rkvdec/rkvdec-h264.c |  4 ++--
 drivers/staging/media/rkvdec/rkvdec.c      | 10 ++++++++++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index f0cfed84d60d..3498e9eec3d8 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -672,8 +672,8 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
 		  LOG2_MAX_PIC_ORDER_CNT_LSB_MINUS4);
 	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_DELTA_PIC_ORDER_ALWAYS_ZERO),
 		  DELTA_PIC_ORDER_ALWAYS_ZERO_FLAG);
-	WRITE_PPS(DIV_ROUND_UP(ctx->coded_fmt.fmt.pix_mp.width, 16), PIC_WIDTH_IN_MBS);
-	WRITE_PPS(DIV_ROUND_UP(ctx->coded_fmt.fmt.pix_mp.height, 16), PIC_HEIGHT_IN_MBS);
+	WRITE_PPS(sps->pic_width_in_mbs_minus1 + 1, PIC_WIDTH_IN_MBS);
+	WRITE_PPS(sps->pic_height_in_map_units_minus1 + 1, PIC_HEIGHT_IN_MBS);
 	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY),
 		  FRAME_MBS_ONLY_FLAG);
 	WRITE_PPS(!!(sps->flags & V4L2_H264_SPS_FLAG_MB_ADAPTIVE_FRAME_FIELD),
diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 4ab8f7e0566b..7a9f78bc0a55 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -29,8 +29,11 @@
 
 static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
 {
+	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
+
 	if (ctrl->id == V4L2_CID_MPEG_VIDEO_H264_SPS) {
 		const struct v4l2_ctrl_h264_sps *sps = ctrl->p_new.p;
+		unsigned int width, height;
 		/*
 		 * TODO: The hardware supports 10-bit and 4:2:2 profiles,
 		 * but it's currently broken in the driver.
@@ -45,6 +48,13 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
 		if (sps->bit_depth_luma_minus8 != 0)
 			/* Only 8-bit is supported */
 			return -EINVAL;
+
+		width = (sps->pic_width_in_mbs_minus1 + 1) * 16;
+		height = (sps->pic_height_in_map_units_minus1 + 1) * 16;
+
+		if (width > ctx->coded_fmt.fmt.pix_mp.width ||
+		    height > ctx->coded_fmt.fmt.pix_mp.height)
+			return -EINVAL;
 	}
 	return 0;
 }
-- 
2.17.1

[PATCH v2 02/12] media: rkvdec: Ensure decoded resolution fit coded resolution

[Jonas Karlman]

Ensure decoded CAPTURE buffer resolution is larger or equal to the coded
OPTUPT buffer resolution.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
Changes in v2:
- New patch
---
 drivers/staging/media/rkvdec/rkvdec.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 55dc27171ce4..4ab8f7e0566b 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -235,6 +235,8 @@ static int rkvdec_try_capture_fmt(struct file *file, void *priv,
 		pix_mp->pixelformat = coded_desc->decoded_fmts[0];
 
 	/* Always apply the frmsize constraint of the coded end. */
+	pix_mp->width = max(pix_mp->width, ctx->coded_fmt.fmt.pix_mp.width);
+	pix_mp->height = max(pix_mp->height, ctx->coded_fmt.fmt.pix_mp.height);
 	v4l2_apply_frmsize_constraints(&pix_mp->width,
 				       &pix_mp->height,
 				       &coded_desc->frmsize);
-- 
2.17.1

[PATCH v2 06/12] 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 block width and height when calculating plane
bytesperline and sizeimage.

This prepare support for new pixel formats added in next patch that make
use of block width and height.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 drivers/media/v4l2-core/v4l2-common.c | 77 +++++++++++++--------------
 1 file changed, 38 insertions(+), 39 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 3dc17ebe14fa..4102c373b48a 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -333,6 +333,33 @@ static inline unsigned int v4l2_format_block_height(const struct v4l2_format_inf
 	return info->block_h[plane];
 }
 
+static inline unsigned int v4l2_format_plane_width(const struct v4l2_format_info *info, int plane,
+						   unsigned int width)
+{
+	unsigned int hdiv = plane ? info->hdiv : 1;
+	unsigned int bytes = DIV_ROUND_UP(width * info->bpp[plane],
+				v4l2_format_block_width(info, plane) *
+				v4l2_format_block_height(info, plane));
+
+	return DIV_ROUND_UP(bytes, hdiv);
+}
+
+static inline unsigned int v4l2_format_plane_height(const struct v4l2_format_info *info, int plane,
+						    unsigned int height)
+{
+	unsigned int vdiv = plane ? info->vdiv : 1;
+	unsigned int lines = ALIGN(height, v4l2_format_block_height(info, plane));
+
+	return DIV_ROUND_UP(lines, vdiv);
+}
+
+static inline unsigned int v4l2_format_plane_size(const struct v4l2_format_info *info, int plane,
+						  unsigned int width, unsigned int height)
+{
+	return v4l2_format_plane_width(info, plane, width) *
+	       v4l2_format_plane_height(info, plane, height);
+}
+
 void v4l2_apply_frmsize_constraints(u32 *width, u32 *height,
 				    const struct v4l2_frmsize_stepwise *frmsize)
 {
@@ -368,37 +395,19 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
 
 	if (info->mem_planes == 1) {
 		plane = &pixfmt->plane_fmt[0];
-		plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
+		plane->bytesperline = v4l2_format_plane_width(info, 0, width);
 		plane->sizeimage = 0;
 
-		for (i = 0; i < info->comp_planes; i++) {
-			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
-			unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
-			unsigned int aligned_width;
-			unsigned int aligned_height;
-
-			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
-			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
-
-			plane->sizeimage += info->bpp[i] *
-				DIV_ROUND_UP(aligned_width, hdiv) *
-				DIV_ROUND_UP(aligned_height, vdiv);
-		}
+		for (i = 0; i < info->comp_planes; i++)
+			plane->sizeimage +=
+				v4l2_format_plane_size(info, i, width, height);
 	} else {
 		for (i = 0; i < info->comp_planes; i++) {
-			unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
-			unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
-			unsigned int aligned_width;
-			unsigned int aligned_height;
-
-			aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
-			aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
-
 			plane = &pixfmt->plane_fmt[i];
 			plane->bytesperline =
-				info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv);
-			plane->sizeimage =
-				plane->bytesperline * DIV_ROUND_UP(aligned_height, vdiv);
+				v4l2_format_plane_width(info, i, width);
+			plane->sizeimage = plane->bytesperline *
+				v4l2_format_plane_height(info, i, height);
 		}
 	}
 	return 0;
@@ -422,22 +431,12 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
 	pixfmt->width = width;
 	pixfmt->height = height;
 	pixfmt->pixelformat = pixelformat;
-	pixfmt->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
+	pixfmt->bytesperline = v4l2_format_plane_width(info, 0, width);
 	pixfmt->sizeimage = 0;
 
-	for (i = 0; i < info->comp_planes; i++) {
-		unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
-		unsigned int vdiv = (i == 0) ? 1 : info->vdiv;
-		unsigned int aligned_width;
-		unsigned int aligned_height;
-
-		aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
-		aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
-
-		pixfmt->sizeimage += info->bpp[i] *
-			DIV_ROUND_UP(aligned_width, hdiv) *
-			DIV_ROUND_UP(aligned_height, vdiv);
-	}
+	for (i = 0; i < info->comp_planes; i++)
+		pixfmt->sizeimage +=
+			v4l2_format_plane_size(info, i, width, height);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
-- 
2.17.1

[PATCH v2 05/12] media: rkvdec: h264: Do not override output buffer sizeimage

[Jonas Karlman]

The output buffer sizeimage is currently forced to 2 bytes per pixel, this
can lead to high memory usage for 4K content when multiple output buffers
is created by userspace.

Do not override output buffer sizeimage and let userspace have control of
output buffer sizeimage by only setting sizeimage if none is provided.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
Changes in v2:
- New patch
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 6576b4a101ae..3a85545bcb38 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -1012,8 +1012,9 @@ static int rkvdec_h264_adjust_fmt(struct rkvdec_ctx *ctx,
 	struct v4l2_pix_format_mplane *fmt = &f->fmt.pix_mp;
 
 	fmt->num_planes = 1;
-	fmt->plane_fmt[0].sizeimage = fmt->width * fmt->height *
-				      RKVDEC_H264_MAX_DEPTH_IN_BYTES;
+	if (!fmt->plane_fmt[0].sizeimage)
+		fmt->plane_fmt[0].sizeimage = fmt->width * fmt->height *
+					      RKVDEC_H264_MAX_DEPTH_IN_BYTES;
 	return 0;
 }
 
-- 
2.17.1

[PATCH v2 04/12] media: rkvdec: h264: Fix bit depth wrap in pps packet

[Jonas Karlman]

The luma and chroma bit depth fields in the pps packet is 3 bits wide.
8 is wrongly added to the bit depth value written to these 3-bit fields.
Because only the 3 LSB is written the hardware is configured correctly.

Correct this by not adding 8 to the luma and chroma bit depth value.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
---
Changes in v2:
- Collect r-b tag
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 3498e9eec3d8..6576b4a101ae 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -662,8 +662,8 @@ static void assemble_hw_pps(struct rkvdec_ctx *ctx,
 	WRITE_PPS(0xff, PROFILE_IDC);
 	WRITE_PPS(1, CONSTRAINT_SET3_FLAG);
 	WRITE_PPS(sps->chroma_format_idc, CHROMA_FORMAT_IDC);
-	WRITE_PPS(sps->bit_depth_luma_minus8 + 8, BIT_DEPTH_LUMA);
-	WRITE_PPS(sps->bit_depth_chroma_minus8 + 8, BIT_DEPTH_CHROMA);
+	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->log2_max_frame_num_minus4, LOG2_MAX_FRAME_NUM_MINUS4);
 	WRITE_PPS(sps->max_num_ref_frames, MAX_NUM_REF_FRAMES);
-- 
2.17.1

[PATCH v2 08/12] media: rkvdec: h264: Use bytesperline and buffer height to calculate stride

[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>
---
Changes in v2:
- Drop code refactoring
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 3a85545bcb38..10756b9d6118 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -891,9 +891,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;
@@ -904,8 +904,8 @@ 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)
-- 
2.17.1

[PATCH v2 07/12] 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 a packed 10-bit 4:2:0/4:2:2 semi-planar Y/CbCr format
similar to P010 and P210 but has no padding between components. Instead,
luminance and chrominance samples are grouped into 4s so that each group is
packed into an integer number of bytes:

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

The '15' 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>
---
 .../userspace-api/media/v4l/pixfmt-nv15.rst   | 101 ++++++++++++++++++
 .../userspace-api/media/v4l/pixfmt-nv20.rst   |  99 +++++++++++++++++
 .../userspace-api/media/v4l/yuv-formats.rst   |   2 +
 drivers/media/v4l2-core/v4l2-common.c         |   3 +
 drivers/media/v4l2-core/v4l2-ioctl.c          |   2 +
 include/uapi/linux/videodev2.h                |   3 +
 6 files changed, 210 insertions(+)
 create mode 100644 Documentation/userspace-api/media/v4l/pixfmt-nv15.rst
 create mode 100644 Documentation/userspace-api/media/v4l/pixfmt-nv20.rst

diff --git a/Documentation/userspace-api/media/v4l/pixfmt-nv15.rst b/Documentation/userspace-api/media/v4l/pixfmt-nv15.rst
new file mode 100644
index 000000000000..d059db58c6e0
--- /dev/null
+++ b/Documentation/userspace-api/media/v4l/pixfmt-nv15.rst
@@ -0,0 +1,101 @@
+.. Permission is granted to copy, distribute and/or modify this
+.. document under the terms of the GNU Free Documentation License,
+.. Version 1.1 or any later version published by the Free Software
+.. Foundation, with no Invariant Sections, no Front-Cover Texts
+.. and no Back-Cover Texts. A copy of the license is included at
+.. Documentation/userspace-api/media/fdl-appendix.rst.
+..
+.. TODO: replace it to GFDL-1.1-or-later WITH no-invariant-sections
+
+.. _V4L2-PIX-FMT-NV15:
+
+**************************
+V4L2_PIX_FMT_NV15 ('NV15')
+**************************
+
+Format with ½ horizontal and vertical chroma resolution, also known as
+YUV 4:2:0. One luminance and one chrominance plane with alternating
+chroma samples similar to ``V4L2_PIX_FMT_NV12`` but with 10-bit samples
+that are grouped into four and packed into five bytes.
+
+The '15' suffix refers to the optimum effective bits per pixel which is
+achieved when the total number of luminance samples is a multiple of 8.
+
+
+Description
+===========
+
+This is a packed 10-bit two-plane version of the YUV 4:2:0 format. The
+three components are separated into two sub-images or planes. The Y plane
+is first. The Y plane has five bytes per each group of four pixels. A
+combined CbCr plane immediately follows the Y plane in memory. The CbCr
+plane is the same width, in bytes, as the Y plane (and of the image), but
+is half as tall in pixels. Each CbCr pair belongs to four pixels. For
+example, Cb\ :sub:`00`/Cr\ :sub:`00` belongs to Y'\ :sub:`00`,
+Y'\ :sub:`01`, Y'\ :sub:`10`, Y'\ :sub:`11`.
+
+If the Y plane has pad bytes after each row, then the CbCr plane has as
+many pad bytes after its rows.
+
+**Byte Order.**
+Little endian. Each cell is one byte. Pixels cross the byte boundary.
+
+
+.. flat-table::
+    :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:
+      - 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]`
+
+
+**Color Sample Location:**
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+
+    * -
+      - 0
+      -
+      - 1
+      - 2
+      -
+      - 3
+    * - 0
+      - Y
+      -
+      - Y
+      - Y
+      -
+      - Y
+    * -
+      -
+      - C
+      -
+      -
+      - C
+      -
+    * - 1
+      - Y
+      -
+      - Y
+      - Y
+      -
+      - Y
diff --git a/Documentation/userspace-api/media/v4l/pixfmt-nv20.rst b/Documentation/userspace-api/media/v4l/pixfmt-nv20.rst
new file mode 100644
index 000000000000..a8123be0baa3
--- /dev/null
+++ b/Documentation/userspace-api/media/v4l/pixfmt-nv20.rst
@@ -0,0 +1,99 @@
+.. Permission is granted to copy, distribute and/or modify this
+.. document under the terms of the GNU Free Documentation License,
+.. Version 1.1 or any later version published by the Free Software
+.. Foundation, with no Invariant Sections, no Front-Cover Texts
+.. and no Back-Cover Texts. A copy of the license is included at
+.. Documentation/userspace-api/media/fdl-appendix.rst.
+..
+.. TODO: replace it to GFDL-1.1-or-later WITH no-invariant-sections
+
+.. _V4L2-PIX-FMT-NV20:
+
+**************************
+V4L2_PIX_FMT_NV20 ('NV20')
+**************************
+
+Format with ½ horizontal chroma resolution, also known as YUV 4:2:2.
+One luminance and one chrominance plane with alternating chroma samples
+similar to ``V4L2_PIX_FMT_NV16`` but with 10-bit samples
+that are grouped into four and packed into five bytes.
+
+The '20' suffix refers to the optimum effective bits per pixel which is
+achieved when the total number of luminance samples is a multiple of 4.
+
+
+Description
+===========
+
+This is a packed 10-bit two-plane version of the YUV 4:2:2 format. The
+three components are separated into two sub-images or planes. The Y plane
+is first. The Y plane has five bytes per each group of four pixels. A
+combined CbCr plane immediately follows the Y plane in memory. The CbCr
+plane is the same width and height, in bytes, as the Y plane (and of the
+image). Each CbCr pair belongs to two pixels. For example,
+Cb\ :sub:`00`/Cr\ :sub:`00` belongs to Y'\ :sub:`00`, Y'\ :sub:`01`.
+
+If the Y plane has pad bytes after each row, then the CbCr plane has as
+many pad bytes after its rows.
+
+**Byte Order.**
+Little endian. Each cell is one byte. Pixels cross the byte boundary.
+
+
+.. flat-table::
+    :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:
+      - 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 + 15:
+      - 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]`
+
+
+**Color Sample Location:**
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+
+    * -
+      - 0
+      -
+      - 1
+      - 2
+      -
+      - 3
+    * - 0
+      - Y
+      - C
+      - Y
+      - Y
+      - C
+      - Y
+    * - 1
+      - Y
+      - C
+      - Y
+      - Y
+      - C
+      - Y
diff --git a/Documentation/userspace-api/media/v4l/yuv-formats.rst b/Documentation/userspace-api/media/v4l/yuv-formats.rst
index 8ee92d0cd769..7cca883f178a 100644
--- a/Documentation/userspace-api/media/v4l/yuv-formats.rst
+++ b/Documentation/userspace-api/media/v4l/yuv-formats.rst
@@ -61,4 +61,6 @@ to brightness information.
     pixfmt-nv16
     pixfmt-nv16m
     pixfmt-nv24
+    pixfmt-nv15
+    pixfmt-nv20
     pixfmt-m420
diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 4102c373b48a..0caac755d303 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -267,6 +267,9 @@ const struct v4l2_format_info *v4l2_format_info(u32 format)
 		{ .format = V4L2_PIX_FMT_NV24,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 1, .vdiv = 1 },
 		{ .format = V4L2_PIX_FMT_NV42,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 1, .vdiv = 1 },
 
+		{ .format = V4L2_PIX_FMT_NV15,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 5, 0, 0 }, .hdiv = 2, .vdiv = 2, .block_w = { 4, 2, 0, 0 }, .block_h = { 1, 1, 0, 0 } },
+		{ .format = V4L2_PIX_FMT_NV20,    .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 5, 5, 0, 0 }, .hdiv = 2, .vdiv = 1, .block_w = { 4, 2, 0, 0 }, .block_h = { 1, 1, 0, 0 } },
+
 		{ .format = V4L2_PIX_FMT_YUV410,  .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 4, .vdiv = 4 },
 		{ .format = V4L2_PIX_FMT_YVU410,  .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 4, .vdiv = 4 },
 		{ .format = V4L2_PIX_FMT_YUV411P, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 4, .vdiv = 1 },
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 02bfef0da76d..4657274bb37a 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1315,6 +1315,8 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
 	case V4L2_PIX_FMT_NV61:		descr = "Y/CrCb 4:2:2"; break;
 	case V4L2_PIX_FMT_NV24:		descr = "Y/CbCr 4:4:4"; break;
 	case V4L2_PIX_FMT_NV42:		descr = "Y/CrCb 4:4:4"; break;
+	case V4L2_PIX_FMT_NV15:		descr = "10-bit Y/CbCr 4:2:0 (Packed)"; break;
+	case V4L2_PIX_FMT_NV20:		descr = "10-bit Y/CbCr 4:2:2 (Packed)"; break;
 	case V4L2_PIX_FMT_NV12M:	descr = "Y/CbCr 4:2:0 (N-C)"; break;
 	case V4L2_PIX_FMT_NV21M:	descr = "Y/CrCb 4:2:0 (N-C)"; break;
 	case V4L2_PIX_FMT_NV16M:	descr = "Y/CbCr 4:2:2 (N-C)"; break;
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 303805438814..bd23aeaf0706 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -610,6 +610,9 @@ struct v4l2_pix_format {
 #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_NV15    v4l2_fourcc('N', 'V', '1', '5') /* 15  Y/CbCr 4:2:0 10-bit packed */
+#define V4L2_PIX_FMT_NV20    v4l2_fourcc('N', 'V', '2', '0') /* 20  Y/CbCr 4:2:2 10-bit packed */
+
 /* two non contiguous planes - one Y, one Cr + Cb interleaved  */
 #define V4L2_PIX_FMT_NV12M   v4l2_fourcc('N', 'M', '1', '2') /* 12  Y/CbCr 4:2:0  */
 #define V4L2_PIX_FMT_NV21M   v4l2_fourcc('N', 'M', '2', '1') /* 21  Y/CrCb 4:2:0  */
-- 
2.17.1

[PATCH v2 10/12] media: rkvdec: Lock capture pixel format in s_ctrl and s_fmt

[Jonas Karlman]

Add an optional valid_fmt operation that should return the valid
pixelformat of CAPTURE buffers.

This is used in next patch to ensure correct pixelformat is used for 10-bit
and 4:2:2 content.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
Changes in v2:
- Reworked validation of pixelformat

Limitations:
- Only a single valid format is supported, technically both CbCr and CrCb
  format variants could be supported by HW.

Example call order using ffmpeg for a 10-bit H.264 608x480 video:

0) Open /dev/video node
rkvdec_reset_decoded_fmt: current valid_fmt= new valid_fmt=
rkvdec_fill_decoded_pixfmt: pixelformat=NV12 width=64 height=48
rkvdec_s_ctrl: current valid_fmt= new valid_fmt=NV12
rkvdec_fill_decoded_pixfmt: pixelformat=NV12 width=64 height=48

1) Set the coded format on the OUTPUT queue via VIDIOC_S_FMT()
rkvdec_enum_output_fmt: index=0 pixelformat=S264
rkvdec_s_output_fmt: pixelformat=S264 width=608 height=480 valid_fmt=NV12
rkvdec_reset_decoded_fmt: current valid_fmt=NV12 new valid_fmt=
rkvdec_fill_decoded_pixfmt: pixelformat=NV12 width=640 height=480

2) Call VIDIOC_S_EXT_CTRLS() to set all the controls
rkvdec_try_ctrl: current valid_fmt=
rkvdec_s_ctrl: current valid_fmt= new valid_fmt=NV15
rkvdec_fill_decoded_pixfmt: pixelformat=NV15 width=640 height=480

3) Call VIDIOC_G_FMT() for CAPTURE queue
rkvdec_g_capture_fmt: pixelformat=NV15 width=640 height=480

4) Enumerate CAPTURE formats via VIDIOC_ENUM_FMT() on the CAPTURE queue
rkvdec_enum_capture_fmt: index=0 pixelformat=NV15 valid_fmt=NV15

5) Choose a different CAPTURE format than suggested via VIDIOC_G_FMT()
rkvdec_s_capture_fmt: pixelformat=NV15 width=608 height=480 valid_fmt=NV15
rkvdec_fill_decoded_pixfmt: pixelformat=NV15 width=640 height=480
rkvdec_g_capture_fmt: pixelformat=NV15 width=640 height=480
---
 drivers/staging/media/rkvdec/rkvdec.c | 59 ++++++++++++++++++++++++---
 drivers/staging/media/rkvdec/rkvdec.h |  2 +
 2 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 911132be6d50..11a88cb6407d 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -38,6 +38,16 @@ static void rkvdec_fill_decoded_pixfmt(struct rkvdec_ctx *ctx,
 	pix_mp->field = V4L2_FIELD_NONE;
 }
 
+static u32 rkvdec_valid_fmt(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl)
+{
+	const struct rkvdec_coded_fmt_desc *coded_desc = ctx->coded_fmt_desc;
+
+	if (coded_desc->ops->valid_fmt)
+		return coded_desc->ops->valid_fmt(ctx, ctrl);
+
+	return ctx->valid_fmt;
+}
+
 static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
@@ -60,6 +70,10 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
 			/* Only 8-bit is supported */
 			return -EINVAL;
 
+		if (ctx->valid_fmt && ctx->valid_fmt != rkvdec_valid_fmt(ctx, ctrl))
+			/* Only current valid format */
+			return -EINVAL;
+
 		width = (sps->pic_width_in_mbs_minus1 + 1) * 16;
 		height = (sps->pic_height_in_map_units_minus1 + 1) * 16;
 
@@ -70,8 +84,27 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
 	return 0;
 }
 
+static int rkvdec_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
+
+	if (ctrl->id == V4L2_CID_MPEG_VIDEO_H264_SPS && !ctx->valid_fmt) {
+		ctx->valid_fmt = rkvdec_valid_fmt(ctx, ctrl);
+		if (ctx->valid_fmt) {
+			struct v4l2_pix_format_mplane *pix_mp;
+
+			pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
+			pix_mp->pixelformat = ctx->valid_fmt;
+			rkvdec_fill_decoded_pixfmt(ctx, pix_mp);
+		}
+	}
+
+	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[] = {
@@ -188,6 +221,7 @@ static void rkvdec_reset_decoded_fmt(struct rkvdec_ctx *ctx)
 {
 	struct v4l2_format *f = &ctx->decoded_fmt;
 
+	ctx->valid_fmt = 0;
 	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;
@@ -243,13 +277,17 @@ 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 (ctx->valid_fmt) {
+		pix_mp->pixelformat = ctx->valid_fmt;
+	} else {
+		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 (i == coded_desc->num_decoded_fmts)
+			pix_mp->pixelformat = coded_desc->decoded_fmts[0];
+	}
 
 	/* Always apply the frmsize constraint of the coded end. */
 	pix_mp->width = max(pix_mp->width, ctx->coded_fmt.fmt.pix_mp.width);
@@ -324,6 +362,7 @@ static int rkvdec_s_capture_fmt(struct file *file, void *priv,
 		return ret;
 
 	ctx->decoded_fmt = *f;
+	ctx->valid_fmt = f->fmt.pix_mp.pixelformat;
 	return 0;
 }
 
@@ -413,6 +452,14 @@ static int rkvdec_enum_capture_fmt(struct file *file, void *priv,
 	if (WARN_ON(!ctx->coded_fmt_desc))
 		return -EINVAL;
 
+	if (ctx->valid_fmt) {
+		if (f->index)
+			return -EINVAL;
+
+		f->pixelformat = ctx->valid_fmt;
+		return 0;
+	}
+
 	if (f->index >= ctx->coded_fmt_desc->num_decoded_fmts)
 		return -EINVAL;
 
diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
index 2fc9f46b6910..50e67401fdbe 100644
--- a/drivers/staging/media/rkvdec/rkvdec.h
+++ b/drivers/staging/media/rkvdec/rkvdec.h
@@ -64,6 +64,7 @@ vb2_to_rkvdec_decoded_buf(struct vb2_buffer *buf)
 struct rkvdec_coded_fmt_ops {
 	int (*adjust_fmt)(struct rkvdec_ctx *ctx,
 			  struct v4l2_format *f);
+	u32 (*valid_fmt)(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl);
 	int (*start)(struct rkvdec_ctx *ctx);
 	void (*stop)(struct rkvdec_ctx *ctx);
 	int (*run)(struct rkvdec_ctx *ctx);
@@ -97,6 +98,7 @@ struct rkvdec_ctx {
 	struct v4l2_fh fh;
 	struct v4l2_format coded_fmt;
 	struct v4l2_format decoded_fmt;
+	u32 valid_fmt;
 	const struct rkvdec_coded_fmt_desc *coded_fmt_desc;
 	struct v4l2_ctrl_handler ctrl_hdl;
 	struct rkvdec_dev *dev;
-- 
2.17.1

Re: [PATCH v2 10/12] media: rkvdec: Lock capture pixel format in s_ctrl and s_fmt

[Ezequiel Garcia]

Hi Jonas,

Nice work!

On Mon, 2020-07-06 at 21:54 +0000, Jonas Karlman wrote:
> Add an optional valid_fmt operation that should return the valid
> pixelformat of CAPTURE buffers.
> 
> This is used in next patch to ensure correct pixelformat is used for 10-bit
> and 4:2:2 content.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
> Changes in v2:
> - Reworked validation of pixelformat
> 
> Limitations:
> - Only a single valid format is supported, technically both CbCr and CrCb
>   format variants could be supported by HW.
> 
> Example call order using ffmpeg for a 10-bit H.264 608x480 video:
> 
> 0) Open /dev/video node
> rkvdec_reset_decoded_fmt: current valid_fmt= new valid_fmt=

Controls and formats always have some defined value.
So the concept of "valid format" is always what's
currently valid, and is what TRY_FMT accepts.

The applications get the currently set format with G_FMT.
As per the specification, there is always a format
set, which is why have to do all that annoying format
reset dance.

For instance, if the initial v4l2_ctrl_h264_sps.bit_depth_luma_minus8
is 0x0 and .chroma_format_idc is 0x1 [1] as well then our initial
valid format would be NV12.

See below.

[1] v4l2_ctrl_h264_sps is all 0x0 by default, but
that's something to address on std_init_compound.
 
> rkvdec_fill_decoded_pixfmt: pixelformat=NV12 width=64 height=48
> rkvdec_s_ctrl: current valid_fmt= new valid_fmt=NV12
> rkvdec_fill_decoded_pixfmt: pixelformat=NV12 width=64 height=48
> 
> 1) Set the coded format on the OUTPUT queue via VIDIOC_S_FMT()
> rkvdec_enum_output_fmt: index=0 pixelformat=S264
> rkvdec_s_output_fmt: pixelformat=S264 width=608 height=480 valid_fmt=NV12
> rkvdec_reset_decoded_fmt: current valid_fmt=NV12 new valid_fmt=
> rkvdec_fill_decoded_pixfmt: pixelformat=NV12 width=640 height=480
> 
> 2) Call VIDIOC_S_EXT_CTRLS() to set all the controls
> rkvdec_try_ctrl: current valid_fmt=
> rkvdec_s_ctrl: current valid_fmt= new valid_fmt=NV15
> rkvdec_fill_decoded_pixfmt: pixelformat=NV15 width=640 height=480
> 
> 3) Call VIDIOC_G_FMT() for CAPTURE queue
> rkvdec_g_capture_fmt: pixelformat=NV15 width=640 height=480
> 
> 4) Enumerate CAPTURE formats via VIDIOC_ENUM_FMT() on the CAPTURE queue
> rkvdec_enum_capture_fmt: index=0 pixelformat=NV15 valid_fmt=NV15
> 
> 5) Choose a different CAPTURE format than suggested via VIDIOC_G_FMT()
> rkvdec_s_capture_fmt: pixelformat=NV15 width=608 height=480 valid_fmt=NV15
> rkvdec_fill_decoded_pixfmt: pixelformat=NV15 width=640 height=480
> rkvdec_g_capture_fmt: pixelformat=NV15 width=640 height=480
> ---
>  drivers/staging/media/rkvdec/rkvdec.c | 59 ++++++++++++++++++++++++---
>  drivers/staging/media/rkvdec/rkvdec.h |  2 +
>  2 files changed, 55 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> index 911132be6d50..11a88cb6407d 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.c
> +++ b/drivers/staging/media/rkvdec/rkvdec.c
> @@ -38,6 +38,16 @@ static void rkvdec_fill_decoded_pixfmt(struct rkvdec_ctx *ctx,
>  	pix_mp->field = V4L2_FIELD_NONE;
>  }
>  
> +static u32 rkvdec_valid_fmt(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl)
> +{
> +	const struct rkvdec_coded_fmt_desc *coded_desc = ctx->coded_fmt_desc;
> +
> +	if (coded_desc->ops->valid_fmt)
> +		return coded_desc->ops->valid_fmt(ctx, ctrl);
> +
> +	return ctx->valid_fmt;
> +}
> +
>  static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
> @@ -60,6 +70,10 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
>  			/* Only 8-bit is supported */
>  			return -EINVAL;
>  
> +		if (ctx->valid_fmt && ctx->valid_fmt != rkvdec_valid_fmt(ctx, ctrl))
> +			/* Only current valid format */
> +			return -EINVAL;
> +

I think the user should be able to set the SPS to anything supported
and syntax-legal, and that would effectively reset the valid format.

Maybe it also means these drivers can't accept a new SPS if there
are buffers allocated (vb2_is_busy) ?

>  		width = (sps->pic_width_in_mbs_minus1 + 1) * 16;
>  		height = (sps->pic_height_in_map_units_minus1 + 1) * 16;
>  
> @@ -70,8 +84,27 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
>  	return 0;
>  }
>  
> +static int rkvdec_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
> +
> +	if (ctrl->id == V4L2_CID_MPEG_VIDEO_H264_SPS && !ctx->valid_fmt) {

Here's a non-tested idea.

If the SPS is supported [2], then we just set it and store it
(store the entire SPS) and negotiate a new format.

The application can the use TRY/S_FMT to negotiate
a format, and so SPS can be used to validate that.

This way, you don't need to store valid_fmt
and you remove the single format limitation.

Does that make sense?

Thanks,
Ezequiel

[2] and possibly provided
!vb2_is_busy(v4l2_m2m_get_vq(ctx->fh.m2m_ctx, CAPTURE))

> +		ctx->valid_fmt = rkvdec_valid_fmt(ctx, ctrl);
> +		if (ctx->valid_fmt) {
> +			struct v4l2_pix_format_mplane *pix_mp;
> +
> +			pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
> +			pix_mp->pixelformat = ctx->valid_fmt;
> +			rkvdec_fill_decoded_pixfmt(ctx, pix_mp);
> +		}
> +	}
> +
> +	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[] = {
> @@ -188,6 +221,7 @@ static void rkvdec_reset_decoded_fmt(struct rkvdec_ctx *ctx)
>  {
>  	struct v4l2_format *f = &ctx->decoded_fmt;
>  
> +	ctx->valid_fmt = 0;
>  	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;
> @@ -243,13 +277,17 @@ 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 (ctx->valid_fmt) {
> +		pix_mp->pixelformat = ctx->valid_fmt;
> +	} else {
> +		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 (i == coded_desc->num_decoded_fmts)
> +			pix_mp->pixelformat = coded_desc->decoded_fmts[0];
> +	}
>  
>  	/* Always apply the frmsize constraint of the coded end. */
>  	pix_mp->width = max(pix_mp->width, ctx->coded_fmt.fmt.pix_mp.width);
> @@ -324,6 +362,7 @@ static int rkvdec_s_capture_fmt(struct file *file, void *priv,
>  		return ret;
>  
>  	ctx->decoded_fmt = *f;
> +	ctx->valid_fmt = f->fmt.pix_mp.pixelformat;
>  	return 0;
>  }
>  
> @@ -413,6 +452,14 @@ static int rkvdec_enum_capture_fmt(struct file *file, void *priv,
>  	if (WARN_ON(!ctx->coded_fmt_desc))
>  		return -EINVAL;
>  
> +	if (ctx->valid_fmt) {
> +		if (f->index)
> +			return -EINVAL;
> +
> +		f->pixelformat = ctx->valid_fmt;
> +		return 0;
> +	}
> +
>  	if (f->index >= ctx->coded_fmt_desc->num_decoded_fmts)
>  		return -EINVAL;
>  
> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
> index 2fc9f46b6910..50e67401fdbe 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.h
> +++ b/drivers/staging/media/rkvdec/rkvdec.h
> @@ -64,6 +64,7 @@ vb2_to_rkvdec_decoded_buf(struct vb2_buffer *buf)
>  struct rkvdec_coded_fmt_ops {
>  	int (*adjust_fmt)(struct rkvdec_ctx *ctx,
>  			  struct v4l2_format *f);
> +	u32 (*valid_fmt)(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl);
>  	int (*start)(struct rkvdec_ctx *ctx);
>  	void (*stop)(struct rkvdec_ctx *ctx);
>  	int (*run)(struct rkvdec_ctx *ctx);
> @@ -97,6 +98,7 @@ struct rkvdec_ctx {
>  	struct v4l2_fh fh;
>  	struct v4l2_format coded_fmt;
>  	struct v4l2_format decoded_fmt;
> +	u32 valid_fmt;
>  	const struct rkvdec_coded_fmt_desc *coded_fmt_desc;
>  	struct v4l2_ctrl_handler ctrl_hdl;
>  	struct rkvdec_dev *dev;

Re: [PATCH v2 10/12] media: rkvdec: Lock capture pixel format in s_ctrl and s_fmt

[Jonas Karlman]

On 2020-07-08 05:16, Ezequiel Garcia wrote:
> Hi Jonas,
> 
> Nice work!
> 
> On Mon, 2020-07-06 at 21:54 +0000, Jonas Karlman wrote:
>> Add an optional valid_fmt operation that should return the valid
>> pixelformat of CAPTURE buffers.
>>
>> This is used in next patch to ensure correct pixelformat is used for 10-bit
>> and 4:2:2 content.
>>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> ---
>> Changes in v2:
>> - Reworked validation of pixelformat
>>
>> Limitations:
>> - Only a single valid format is supported, technically both CbCr and CrCb
>>   format variants could be supported by HW.
>>
>> Example call order using ffmpeg for a 10-bit H.264 608x480 video:
>>
>> 0) Open /dev/video node
>> rkvdec_reset_decoded_fmt: current valid_fmt= new valid_fmt=
> 
> Controls and formats always have some defined value.
> So the concept of "valid format" is always what's
> currently valid, and is what TRY_FMT accepts.
> 
> The applications get the currently set format with G_FMT.
> As per the specification, there is always a format
> set, which is why have to do all that annoying format
> reset dance.
> 
> For instance, if the initial v4l2_ctrl_h264_sps.bit_depth_luma_minus8
> is 0x0 and .chroma_format_idc is 0x1 [1] as well then our initial
> valid format would be NV12.
> 
> See below.
> 
> [1] v4l2_ctrl_h264_sps is all 0x0 by default, but
> that's something to address on std_init_compound.
>  
>> rkvdec_fill_decoded_pixfmt: pixelformat=NV12 width=64 height=48
>> rkvdec_s_ctrl: current valid_fmt= new valid_fmt=NV12
>> rkvdec_fill_decoded_pixfmt: pixelformat=NV12 width=64 height=48
>>
>> 1) Set the coded format on the OUTPUT queue via VIDIOC_S_FMT()
>> rkvdec_enum_output_fmt: index=0 pixelformat=S264
>> rkvdec_s_output_fmt: pixelformat=S264 width=608 height=480 valid_fmt=NV12
>> rkvdec_reset_decoded_fmt: current valid_fmt=NV12 new valid_fmt=
>> rkvdec_fill_decoded_pixfmt: pixelformat=NV12 width=640 height=480
>>
>> 2) Call VIDIOC_S_EXT_CTRLS() to set all the controls
>> rkvdec_try_ctrl: current valid_fmt=
>> rkvdec_s_ctrl: current valid_fmt= new valid_fmt=NV15
>> rkvdec_fill_decoded_pixfmt: pixelformat=NV15 width=640 height=480
>>
>> 3) Call VIDIOC_G_FMT() for CAPTURE queue
>> rkvdec_g_capture_fmt: pixelformat=NV15 width=640 height=480
>>
>> 4) Enumerate CAPTURE formats via VIDIOC_ENUM_FMT() on the CAPTURE queue
>> rkvdec_enum_capture_fmt: index=0 pixelformat=NV15 valid_fmt=NV15
>>
>> 5) Choose a different CAPTURE format than suggested via VIDIOC_G_FMT()
>> rkvdec_s_capture_fmt: pixelformat=NV15 width=608 height=480 valid_fmt=NV15
>> rkvdec_fill_decoded_pixfmt: pixelformat=NV15 width=640 height=480
>> rkvdec_g_capture_fmt: pixelformat=NV15 width=640 height=480
>> ---
>>  drivers/staging/media/rkvdec/rkvdec.c | 59 ++++++++++++++++++++++++---
>>  drivers/staging/media/rkvdec/rkvdec.h |  2 +
>>  2 files changed, 55 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
>> index 911132be6d50..11a88cb6407d 100644
>> --- a/drivers/staging/media/rkvdec/rkvdec.c
>> +++ b/drivers/staging/media/rkvdec/rkvdec.c
>> @@ -38,6 +38,16 @@ static void rkvdec_fill_decoded_pixfmt(struct rkvdec_ctx *ctx,
>>  	pix_mp->field = V4L2_FIELD_NONE;
>>  }
>>  
>> +static u32 rkvdec_valid_fmt(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl)
>> +{
>> +	const struct rkvdec_coded_fmt_desc *coded_desc = ctx->coded_fmt_desc;
>> +
>> +	if (coded_desc->ops->valid_fmt)
>> +		return coded_desc->ops->valid_fmt(ctx, ctrl);
>> +
>> +	return ctx->valid_fmt;
>> +}
>> +
>>  static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
>>  {
>>  	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
>> @@ -60,6 +70,10 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
>>  			/* Only 8-bit is supported */
>>  			return -EINVAL;
>>  
>> +		if (ctx->valid_fmt && ctx->valid_fmt != rkvdec_valid_fmt(ctx, ctrl))
>> +			/* Only current valid format */
>> +			return -EINVAL;
>> +
> 
> I think the user should be able to set the SPS to anything supported
> and syntax-legal, and that would effectively reset the valid format.

I agree.

> 
> Maybe it also means these drivers can't accept a new SPS if there
> are buffers allocated (vb2_is_busy) ?

I think we need to be able to change SPS as long as the SPS change don't
require changing CAPTURE pixel format. Checking vb2_is_busy for when to
reject pixel format change sounds reasonable, else we probably would have
to trigger a V4L2_EVENT_SOURCE_CHANGE event or similar that a pixel format
change/negotiation is needed, however that feels out of scope for this series.

> 
>>  		width = (sps->pic_width_in_mbs_minus1 + 1) * 16;
>>  		height = (sps->pic_height_in_map_units_minus1 + 1) * 16;
>>  
>> @@ -70,8 +84,27 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
>>  	return 0;
>>  }
>>  
>> +static int rkvdec_s_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> +	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
>> +
>> +	if (ctrl->id == V4L2_CID_MPEG_VIDEO_H264_SPS && !ctx->valid_fmt) {
> 
> Here's a non-tested idea.
> 
> If the SPS is supported [2], then we just set it and store it
> (store the entire SPS) and negotiate a new format.
> 
> The application can the use TRY/S_FMT to negotiate
> a format, and so SPS can be used to validate that.
> 
> This way, you don't need to store valid_fmt
> and you remove the single format limitation.
> 
> Does that make sense?

I think it does :-)

I will update the try_ctrl/s_ctrl code to make use of vb2_is_busy instead of
valid_fmt and change the valid_fmt operation to report if a format is supported
or not. Hopefully that should remove the single format limitation.

Regards,
Jonas

> 
> Thanks,
> Ezequiel
> 
> [2] and possibly provided
> !vb2_is_busy(v4l2_m2m_get_vq(ctx->fh.m2m_ctx, CAPTURE))
> 
>> +		ctx->valid_fmt = rkvdec_valid_fmt(ctx, ctrl);
>> +		if (ctx->valid_fmt) {
>> +			struct v4l2_pix_format_mplane *pix_mp;
>> +
>> +			pix_mp = &ctx->decoded_fmt.fmt.pix_mp;
>> +			pix_mp->pixelformat = ctx->valid_fmt;
>> +			rkvdec_fill_decoded_pixfmt(ctx, pix_mp);
>> +		}
>> +	}
>> +
>> +	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[] = {
>> @@ -188,6 +221,7 @@ static void rkvdec_reset_decoded_fmt(struct rkvdec_ctx *ctx)
>>  {
>>  	struct v4l2_format *f = &ctx->decoded_fmt;
>>  
>> +	ctx->valid_fmt = 0;
>>  	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;
>> @@ -243,13 +277,17 @@ 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 (ctx->valid_fmt) {
>> +		pix_mp->pixelformat = ctx->valid_fmt;
>> +	} else {
>> +		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 (i == coded_desc->num_decoded_fmts)
>> +			pix_mp->pixelformat = coded_desc->decoded_fmts[0];
>> +	}
>>  
>>  	/* Always apply the frmsize constraint of the coded end. */
>>  	pix_mp->width = max(pix_mp->width, ctx->coded_fmt.fmt.pix_mp.width);
>> @@ -324,6 +362,7 @@ static int rkvdec_s_capture_fmt(struct file *file, void *priv,
>>  		return ret;
>>  
>>  	ctx->decoded_fmt = *f;
>> +	ctx->valid_fmt = f->fmt.pix_mp.pixelformat;
>>  	return 0;
>>  }
>>  
>> @@ -413,6 +452,14 @@ static int rkvdec_enum_capture_fmt(struct file *file, void *priv,
>>  	if (WARN_ON(!ctx->coded_fmt_desc))
>>  		return -EINVAL;
>>  
>> +	if (ctx->valid_fmt) {
>> +		if (f->index)
>> +			return -EINVAL;
>> +
>> +		f->pixelformat = ctx->valid_fmt;
>> +		return 0;
>> +	}
>> +
>>  	if (f->index >= ctx->coded_fmt_desc->num_decoded_fmts)
>>  		return -EINVAL;
>>  
>> diff --git a/drivers/staging/media/rkvdec/rkvdec.h b/drivers/staging/media/rkvdec/rkvdec.h
>> index 2fc9f46b6910..50e67401fdbe 100644
>> --- a/drivers/staging/media/rkvdec/rkvdec.h
>> +++ b/drivers/staging/media/rkvdec/rkvdec.h
>> @@ -64,6 +64,7 @@ vb2_to_rkvdec_decoded_buf(struct vb2_buffer *buf)
>>  struct rkvdec_coded_fmt_ops {
>>  	int (*adjust_fmt)(struct rkvdec_ctx *ctx,
>>  			  struct v4l2_format *f);
>> +	u32 (*valid_fmt)(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl);
>>  	int (*start)(struct rkvdec_ctx *ctx);
>>  	void (*stop)(struct rkvdec_ctx *ctx);
>>  	int (*run)(struct rkvdec_ctx *ctx);
>> @@ -97,6 +98,7 @@ struct rkvdec_ctx {
>>  	struct v4l2_fh fh;
>>  	struct v4l2_format coded_fmt;
>>  	struct v4l2_format decoded_fmt;
>> +	u32 valid_fmt;
>>  	const struct rkvdec_coded_fmt_desc *coded_fmt_desc;
>>  	struct v4l2_ctrl_handler ctrl_hdl;
>>  	struct rkvdec_dev *dev;
> 
> 

[PATCH v2 11/12] 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 on 10-bit format buffers.

The new valid_fmt operation is implemented and return a valid pixelformat
for the provided SPS control.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
Changes in v2:
- Only align decoded buffer instead of using frmsize step_width
---
 drivers/staging/media/rkvdec/rkvdec-h264.c | 20 ++++++++++++++++++++
 drivers/staging/media/rkvdec/rkvdec.c      | 19 +++++++++----------
 2 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec-h264.c b/drivers/staging/media/rkvdec/rkvdec-h264.c
index 10756b9d6118..0757fc97d1ff 100644
--- a/drivers/staging/media/rkvdec/rkvdec-h264.c
+++ b/drivers/staging/media/rkvdec/rkvdec-h264.c
@@ -1018,6 +1018,25 @@ static int rkvdec_h264_adjust_fmt(struct rkvdec_ctx *ctx,
 	return 0;
 }
 
+static u32 rkvdec_h264_valid_fmt(struct rkvdec_ctx *ctx, struct v4l2_ctrl *ctrl)
+{
+	const struct v4l2_ctrl_h264_sps *sps = ctrl->p_new.p;
+
+	if (sps->bit_depth_luma_minus8 == 0) {
+		if (sps->chroma_format_idc == 2)
+			return V4L2_PIX_FMT_NV16;
+		else
+			return V4L2_PIX_FMT_NV12;
+	} else if (sps->bit_depth_luma_minus8 == 2) {
+		if (sps->chroma_format_idc == 2)
+			return V4L2_PIX_FMT_NV20;
+		else
+			return V4L2_PIX_FMT_NV15;
+	}
+
+	return 0;
+}
+
 static int rkvdec_h264_start(struct rkvdec_ctx *ctx)
 {
 	struct rkvdec_dev *rkvdec = ctx->dev;
@@ -1125,6 +1144,7 @@ static int rkvdec_h264_run(struct rkvdec_ctx *ctx)
 
 const struct rkvdec_coded_fmt_ops rkvdec_h264_fmt_ops = {
 	.adjust_fmt = rkvdec_h264_adjust_fmt,
+	.valid_fmt = rkvdec_h264_valid_fmt,
 	.start = rkvdec_h264_start,
 	.stop = rkvdec_h264_stop,
 	.run = rkvdec_h264_run,
diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 11a88cb6407d..4faee9262392 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -31,7 +31,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);
@@ -55,19 +55,15 @@ static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
 	if (ctrl->id == V4L2_CID_MPEG_VIDEO_H264_SPS) {
 		const struct v4l2_ctrl_h264_sps *sps = ctrl->p_new.p;
 		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;
 
 		if (ctx->valid_fmt && ctx->valid_fmt != rkvdec_valid_fmt(ctx, ctrl))
@@ -157,6 +153,9 @@ static const struct rkvdec_ctrls rkvdec_h264_ctrls = {
 
 static const u32 rkvdec_h264_decoded_fmts[] = {
 	V4L2_PIX_FMT_NV12,
+	V4L2_PIX_FMT_NV15,
+	V4L2_PIX_FMT_NV16,
+	V4L2_PIX_FMT_NV20,
 };
 
 static const struct rkvdec_coded_fmt_desc rkvdec_coded_fmts[] = {
-- 
2.17.1

[PATCH v2 09/12] media: rkvdec: Extract rkvdec_fill_decoded_pixfmt helper method

[Jonas Karlman]

This extract setting decoded pixfmt into a helper method, current code is
replaced with a call to the new helper method.

The helper method is also called from a new function in next patch.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
Changes in v2:
- New patch
---
 drivers/staging/media/rkvdec/rkvdec.c | 29 ++++++++++++++-------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 7a9f78bc0a55..911132be6d50 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -27,6 +27,17 @@
 #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);
+	pix_mp->field = V4L2_FIELD_NONE;
+}
+
 static int rkvdec_try_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct rkvdec_ctx *ctx = container_of(ctrl->handler, struct rkvdec_ctx, ctrl_hdl);
@@ -179,13 +190,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,
@@ -251,13 +258,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);
-	pix_mp->field = V4L2_FIELD_NONE;
+	rkvdec_fill_decoded_pixfmt(ctx, pix_mp);
 
 	return 0;
 }
-- 
2.17.1

[PATCH v2 12/12] media: rkvdec: h264: Support profile and level controls

[Jonas Karlman]

The Rockchip Video Decoder used in RK3399 supports H.264 profiles from
Baseline to High 4:2:2 up to Level 5.1, except for the Extended profile.

Expose the V4L2_CID_MPEG_VIDEO_H264_PROFILE and the
V4L2_CID_MPEG_VIDEO_H264_LEVEL control, so that userspace can query the
driver for the list of supported profiles and level.

Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
---
Changes in v2:
- Moved to end
- Collect r-b tag
---
 drivers/staging/media/rkvdec/rkvdec.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index 4faee9262392..b21031535330 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -144,6 +144,19 @@ static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
 		.cfg.def = V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B,
 		.cfg.max = V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B,
 	},
+	{
+		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_PROFILE,
+		.cfg.min = V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE,
+		.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,
+	},
+	{
+		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_LEVEL,
+		.cfg.min = V4L2_MPEG_VIDEO_H264_LEVEL_1_0,
+		.cfg.max = V4L2_MPEG_VIDEO_H264_LEVEL_5_1,
+	},
 };
 
 static const struct rkvdec_ctrls rkvdec_h264_ctrls = {
-- 
2.17.1

Re: [PATCH v2 12/12] media: rkvdec: h264: Support profile and level controls

[Ezequiel Garcia]

On Mon, 2020-07-06 at 21:54 +0000, Jonas Karlman wrote:
> The Rockchip Video Decoder used in RK3399 supports H.264 profiles from
> Baseline to High 4:2:2 up to Level 5.1, except for the Extended profile.
> 
> Expose the V4L2_CID_MPEG_VIDEO_H264_PROFILE and the
> V4L2_CID_MPEG_VIDEO_H264_LEVEL control, so that userspace can query the
> driver for the list of supported profiles and level.
> 
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
> Changes in v2:
> - Moved to end
> - Collect r-b tag
> ---
>  drivers/staging/media/rkvdec/rkvdec.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
> index 4faee9262392..b21031535330 100644
> --- a/drivers/staging/media/rkvdec/rkvdec.c
> +++ b/drivers/staging/media/rkvdec/rkvdec.c
> @@ -144,6 +144,19 @@ static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
>  		.cfg.def = V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B,
>  		.cfg.max = V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B,
>  	},
> +	{
> +		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_PROFILE,
> +		.cfg.min = V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE,

Nicolas recently pointed out to me that our drivers
can't support ASO/FMO baseline features, and so
seems we need to leave baseline out.

(Applies to Hantro as well).

Thanks,
Ezequiel

> +		.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,
> +	},
> +	{
> +		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_LEVEL,
> +		.cfg.min = V4L2_MPEG_VIDEO_H264_LEVEL_1_0,
> +		.cfg.max = V4L2_MPEG_VIDEO_H264_LEVEL_5_1,
> +	},
>  };
>  
>  static const struct rkvdec_ctrls rkvdec_h264_ctrls = {

Re: [PATCH v2 12/12] media: rkvdec: h264: Support profile and level controls

[Jonas Karlman]

On 2020-07-08 05:19, Ezequiel Garcia wrote:
> On Mon, 2020-07-06 at 21:54 +0000, Jonas Karlman wrote:
>> The Rockchip Video Decoder used in RK3399 supports H.264 profiles from
>> Baseline to High 4:2:2 up to Level 5.1, except for the Extended profile.
>>
>> Expose the V4L2_CID_MPEG_VIDEO_H264_PROFILE and the
>> V4L2_CID_MPEG_VIDEO_H264_LEVEL control, so that userspace can query the
>> driver for the list of supported profiles and level.
>>
>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
>> ---
>> Changes in v2:
>> - Moved to end
>> - Collect r-b tag
>> ---
>>  drivers/staging/media/rkvdec/rkvdec.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
>> index 4faee9262392..b21031535330 100644
>> --- a/drivers/staging/media/rkvdec/rkvdec.c
>> +++ b/drivers/staging/media/rkvdec/rkvdec.c
>> @@ -144,6 +144,19 @@ static const struct rkvdec_ctrl_desc rkvdec_h264_ctrl_descs[] = {
>>  		.cfg.def = V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B,
>>  		.cfg.max = V4L2_MPEG_VIDEO_H264_START_CODE_ANNEX_B,
>>  	},
>> +	{
>> +		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_PROFILE,
>> +		.cfg.min = V4L2_MPEG_VIDEO_H264_PROFILE_BASELINE,
> 
> Nicolas recently pointed out to me that our drivers
> can't support ASO/FMO baseline features, and so
> seems we need to leave baseline out.

I will change min to V4L2_MPEG_VIDEO_H264_PROFILE_CONSTRAINED_BASELINE in v3.

Regards,
Jonas

> 
> (Applies to Hantro as well).
> 
> Thanks,
> Ezequiel
> 
>> +		.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,
>> +	},
>> +	{
>> +		.cfg.id = V4L2_CID_MPEG_VIDEO_H264_LEVEL,
>> +		.cfg.min = V4L2_MPEG_VIDEO_H264_LEVEL_1_0,
>> +		.cfg.max = V4L2_MPEG_VIDEO_H264_LEVEL_5_1,
>> +	},
>>  };
>>  
>>  static const struct rkvdec_ctrls rkvdec_h264_ctrls = {
> 
>