[PATCH v2 0/7] media: hantro: Add 10-bit support

[PATCH v2 0/7] media: hantro: Add 10-bit support

[Jernej Skrabec]

This series adds support for 10-bit format handling in Hantro driver.
Most patches adjust postproc behaviour to be more universal. There is
a lot of assumptions, which need to be replaced with more generalized.

Tested on Allwinner H6. Tested with vp92-2-20-10bit-yuv420.webm sample,
which produces correct checksum.

Please take a look.

Best regards,
Jernej

Changes from RFC:
- fixed typo in function name
- removed merged patch (P010 format)
- removed broken patch (sunxi frequency change)
- added new patch (media: hantro: postproc: Properly calculate chroma offset)
- added r-b from Ezequiel

Ezequiel Garcia (1):
  media: Add P010 tiled format

Jernej Skrabec (6):
  media: hantro: Support format filtering by depth
  media: hantro: postproc: Fix buffer size calculation
  media: hantro: postproc: Fix legacy regs configuration
  media: hantro: postproc: Properly calculate chroma offset
  media: hantro: Store VP9 bit depth in context
  media: hantro: sunxi: Enable 10-bit decoding

 drivers/media/v4l2-core/v4l2-common.c         |  1 +
 drivers/media/v4l2-core/v4l2-ioctl.c          |  1 +
 drivers/staging/media/hantro/hantro.h         |  4 ++
 drivers/staging/media/hantro/hantro_drv.c     | 23 +++++++++
 .../staging/media/hantro/hantro_g2_vp9_dec.c  |  8 ---
 .../staging/media/hantro/hantro_postproc.c    | 38 +++++++++++---
 drivers/staging/media/hantro/hantro_v4l2.c    | 50 +++++++++++++++++--
 drivers/staging/media/hantro/hantro_v4l2.h    |  3 ++
 drivers/staging/media/hantro/sunxi_vpu_hw.c   | 27 ++++++++++
 include/uapi/linux/videodev2.h                |  1 +
 10 files changed, 136 insertions(+), 20 deletions(-)

-- 
2.36.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/7] media: Add P010 tiled format

[Jernej Skrabec]

From: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Add P010 tiled format

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
[rebased and updated pixel format name]
Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/media/v4l2-core/v4l2-common.c | 1 +
 drivers/media/v4l2-core/v4l2-ioctl.c  | 1 +
 include/uapi/linux/videodev2.h        | 1 +
 3 files changed, 3 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 1e38ad8906a2..e0fbe6ba4b6c 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -278,6 +278,7 @@ const struct v4l2_format_info *v4l2_format_info(u32 format)
 
 		/* Tiled YUV formats */
 		{ .format = V4L2_PIX_FMT_NV12_4L4, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 2 },
+		{ .format = V4L2_PIX_FMT_P010_4L4, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 2, 4, 0, 0 }, .hdiv = 2, .vdiv = 2 },
 
 		/* YUV planar formats, non contiguous variant */
 		{ .format = V4L2_PIX_FMT_YUV420M, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 2 },
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index e2526701294e..e24d38c0a178 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1310,6 +1310,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
 	case V4L2_PIX_FMT_NV12_4L4:	descr = "Y/CbCr 4:2:0 (4x4 Linear)"; break;
 	case V4L2_PIX_FMT_NV12_16L16:	descr = "Y/CbCr 4:2:0 (16x16 Linear)"; break;
 	case V4L2_PIX_FMT_NV12_32L32:   descr = "Y/CbCr 4:2:0 (32x32 Linear)"; break;
+	case V4L2_PIX_FMT_P010_4L4:	descr = "P010 tiled"; 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 5311ac4fde35..32bedeb04152 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -630,6 +630,7 @@ struct v4l2_pix_format {
 #define V4L2_PIX_FMT_NV12_4L4 v4l2_fourcc('V', 'T', '1', '2')   /* 12  Y/CbCr 4:2:0  4x4 tiles */
 #define V4L2_PIX_FMT_NV12_16L16 v4l2_fourcc('H', 'M', '1', '2') /* 12  Y/CbCr 4:2:0 16x16 tiles */
 #define V4L2_PIX_FMT_NV12_32L32 v4l2_fourcc('S', 'T', '1', '2') /* 12  Y/CbCr 4:2:0 32x32 tiles */
+#define V4L2_PIX_FMT_P010_4L4 v4l2_fourcc('T', '0', '1', '0') /* 12  Y/CbCr 4:2:0 10-bit 4x4 macroblocks */
 
 /* Tiled YUV formats, non contiguous planes */
 #define V4L2_PIX_FMT_NV12MT  v4l2_fourcc('T', 'M', '1', '2') /* 12  Y/CbCr 4:2:0 64x32 tiles */
-- 
2.36.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/7] media: Add P010 tiled format

[Ezequiel Garcia]

Hi Jernej,

Thanks for the patch.

On Thu, Jun 16, 2022 at 10:25:07PM +0200, Jernej Skrabec wrote:
> From: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> 
> Add P010 tiled format
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> [rebased and updated pixel format name]
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  drivers/media/v4l2-core/v4l2-common.c | 1 +
>  drivers/media/v4l2-core/v4l2-ioctl.c  | 1 +
>  include/uapi/linux/videodev2.h        | 1 +

We also need to add the documentation in
Documentation/userspace-api/media/v4l/pixfmt-yuv-planar.rst.

See Benjamin's patch for reference:

commit 5374d8fb75f313294c7d97e85c22bead34d63f2b
Author: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Date:   Thu May 19 08:11:46 2022 +0100

    media: Add P010 video format

    P010 is a YUV format with 10-bits per component with interleaved UV.

>  3 files changed, 3 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index 1e38ad8906a2..e0fbe6ba4b6c 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -278,6 +278,7 @@ const struct v4l2_format_info *v4l2_format_info(u32 format)
>  
>  		/* Tiled YUV formats */
>  		{ .format = V4L2_PIX_FMT_NV12_4L4, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 1, 2, 0, 0 }, .hdiv = 2, .vdiv = 2 },
> +		{ .format = V4L2_PIX_FMT_P010_4L4, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 2, .bpp = { 2, 4, 0, 0 }, .hdiv = 2, .vdiv = 2 },
>  
>  		/* YUV planar formats, non contiguous variant */
>  		{ .format = V4L2_PIX_FMT_YUV420M, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 3, .comp_planes = 3, .bpp = { 1, 1, 1, 0 }, .hdiv = 2, .vdiv = 2 },
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index e2526701294e..e24d38c0a178 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1310,6 +1310,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
>  	case V4L2_PIX_FMT_NV12_4L4:	descr = "Y/CbCr 4:2:0 (4x4 Linear)"; break;
>  	case V4L2_PIX_FMT_NV12_16L16:	descr = "Y/CbCr 4:2:0 (16x16 Linear)"; break;
>  	case V4L2_PIX_FMT_NV12_32L32:   descr = "Y/CbCr 4:2:0 (32x32 Linear)"; break;
> +	case V4L2_PIX_FMT_P010_4L4:	descr = "P010 tiled"; break;

I would make this "10-bit Y/CbCr 4:2:0 (4x4 Linear)".

Thanks,
Ezequiel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/7] media: hantro: Support format filtering by depth

[Jernej Skrabec]

In preparation for supporting 10-bit formats, add mechanism which will
filter formats based on pixel depth.

Hantro G2 supports only one decoding format natively and that is based
on bit depth of current video frame. Additionally, it makes no sense to
upconvert bitness, so filter those out too.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/staging/media/hantro/hantro.h      |  4 ++
 drivers/staging/media/hantro/hantro_v4l2.c | 48 ++++++++++++++++++++--
 drivers/staging/media/hantro/hantro_v4l2.h |  1 +
 3 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h
index 26308bb29adc..2989ebc631cc 100644
--- a/drivers/staging/media/hantro/hantro.h
+++ b/drivers/staging/media/hantro/hantro.h
@@ -227,6 +227,7 @@ struct hantro_dev {
  *
  * @ctrl_handler:	Control handler used to register controls.
  * @jpeg_quality:	User-specified JPEG compression quality.
+ * @bit_depth:		Bit depth of current frame
  *
  * @codec_ops:		Set of operations related to codec mode.
  * @postproc:		Post-processing context.
@@ -252,6 +253,7 @@ struct hantro_ctx {
 
 	struct v4l2_ctrl_handler ctrl_handler;
 	int jpeg_quality;
+	int bit_depth;
 
 	const struct hantro_codec_ops *codec_ops;
 	struct hantro_postproc_ctx postproc;
@@ -277,6 +279,7 @@ struct hantro_ctx {
  * @enc_fmt:	Format identifier for encoder registers.
  * @frmsize:	Supported range of frame sizes (only for bitstream formats).
  * @postprocessed: Indicates if this format needs the post-processor.
+ * @match_depth: Indicates if format bit depth must match video bit depth
  */
 struct hantro_fmt {
 	char *name;
@@ -287,6 +290,7 @@ struct hantro_fmt {
 	enum hantro_enc_fmt enc_fmt;
 	struct v4l2_frmsize_stepwise frmsize;
 	bool postprocessed;
+	bool match_depth;
 };
 
 struct hantro_reg {
diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
index 29cc61d53b71..334f18a4120d 100644
--- a/drivers/staging/media/hantro/hantro_v4l2.c
+++ b/drivers/staging/media/hantro/hantro_v4l2.c
@@ -64,6 +64,42 @@ hantro_get_postproc_formats(const struct hantro_ctx *ctx,
 	return ctx->dev->variant->postproc_fmts;
 }
 
+int hantro_get_format_depth(u32 fourcc)
+{
+	switch (fourcc) {
+	case V4L2_PIX_FMT_P010:
+	case V4L2_PIX_FMT_P010_4L4:
+		return 10;
+	default:
+		return 8;
+	}
+}
+
+static bool
+hantro_check_depth_match(const struct hantro_ctx *ctx,
+			 const struct hantro_fmt *fmt)
+{
+	int fmt_depth, ctx_depth = 8;
+
+	if (!fmt->match_depth && !fmt->postprocessed)
+		return true;
+
+	/* 0 means default depth, which is 8 */
+	if (ctx->bit_depth)
+		ctx_depth = ctx->bit_depth;
+
+	fmt_depth = hantro_get_format_depth(fmt->fourcc);
+
+	/*
+	 * Allow only downconversion for postproc formats for now.
+	 * It may be possible to relax that on some HW.
+	 */
+	if (!fmt->match_depth)
+		return fmt_depth <= ctx_depth;
+
+	return fmt_depth == ctx_depth;
+}
+
 static const struct hantro_fmt *
 hantro_find_format(const struct hantro_ctx *ctx, u32 fourcc)
 {
@@ -91,7 +127,8 @@ hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream)
 	formats = hantro_get_formats(ctx, &num_fmts);
 	for (i = 0; i < num_fmts; i++) {
 		if (bitstream == (formats[i].codec_mode !=
-				  HANTRO_MODE_NONE))
+				  HANTRO_MODE_NONE) &&
+		    hantro_check_depth_match(ctx, &formats[i]))
 			return &formats[i];
 	}
 	return NULL;
@@ -162,11 +199,13 @@ static int vidioc_enum_fmt(struct file *file, void *priv,
 	formats = hantro_get_formats(ctx, &num_fmts);
 	for (i = 0; i < num_fmts; i++) {
 		bool mode_none = formats[i].codec_mode == HANTRO_MODE_NONE;
+		fmt = &formats[i];
 
 		if (skip_mode_none == mode_none)
 			continue;
+		if (!hantro_check_depth_match(ctx, fmt))
+			continue;
 		if (j == f->index) {
-			fmt = &formats[i];
 			f->pixelformat = fmt->fourcc;
 			return 0;
 		}
@@ -182,8 +221,11 @@ static int vidioc_enum_fmt(struct file *file, void *priv,
 		return -EINVAL;
 	formats = hantro_get_postproc_formats(ctx, &num_fmts);
 	for (i = 0; i < num_fmts; i++) {
+		fmt = &formats[i];
+
+		if (!hantro_check_depth_match(ctx, fmt))
+			continue;
 		if (j == f->index) {
-			fmt = &formats[i];
 			f->pixelformat = fmt->fourcc;
 			return 0;
 		}
diff --git a/drivers/staging/media/hantro/hantro_v4l2.h b/drivers/staging/media/hantro/hantro_v4l2.h
index 18bc682c8556..b17e84c82582 100644
--- a/drivers/staging/media/hantro/hantro_v4l2.h
+++ b/drivers/staging/media/hantro/hantro_v4l2.h
@@ -22,5 +22,6 @@ extern const struct v4l2_ioctl_ops hantro_ioctl_ops;
 extern const struct vb2_ops hantro_queue_ops;
 
 void hantro_reset_fmts(struct hantro_ctx *ctx);
+int hantro_get_format_depth(u32 fourcc);
 
 #endif /* HANTRO_V4L2_H_ */
-- 
2.36.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/7] media: hantro: Support format filtering by depth

[Ezequiel Garcia]

Hi Jernej,

On Thu, Jun 16, 2022 at 10:25:08PM +0200, Jernej Skrabec wrote:
> In preparation for supporting 10-bit formats, add mechanism which will
> filter formats based on pixel depth.
> 
> Hantro G2 supports only one decoding format natively and that is based
> on bit depth of current video frame. Additionally, it makes no sense to
> upconvert bitness, so filter those out too.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>

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

Thanks,
Ezequiel

> ---
>  drivers/staging/media/hantro/hantro.h      |  4 ++
>  drivers/staging/media/hantro/hantro_v4l2.c | 48 ++++++++++++++++++++--
>  drivers/staging/media/hantro/hantro_v4l2.h |  1 +
>  3 files changed, 50 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h
> index 26308bb29adc..2989ebc631cc 100644
> --- a/drivers/staging/media/hantro/hantro.h
> +++ b/drivers/staging/media/hantro/hantro.h
> @@ -227,6 +227,7 @@ struct hantro_dev {
>   *
>   * @ctrl_handler:	Control handler used to register controls.
>   * @jpeg_quality:	User-specified JPEG compression quality.
> + * @bit_depth:		Bit depth of current frame
>   *
>   * @codec_ops:		Set of operations related to codec mode.
>   * @postproc:		Post-processing context.
> @@ -252,6 +253,7 @@ struct hantro_ctx {
>  
>  	struct v4l2_ctrl_handler ctrl_handler;
>  	int jpeg_quality;
> +	int bit_depth;
>  
>  	const struct hantro_codec_ops *codec_ops;
>  	struct hantro_postproc_ctx postproc;
> @@ -277,6 +279,7 @@ struct hantro_ctx {
>   * @enc_fmt:	Format identifier for encoder registers.
>   * @frmsize:	Supported range of frame sizes (only for bitstream formats).
>   * @postprocessed: Indicates if this format needs the post-processor.
> + * @match_depth: Indicates if format bit depth must match video bit depth
>   */
>  struct hantro_fmt {
>  	char *name;
> @@ -287,6 +290,7 @@ struct hantro_fmt {
>  	enum hantro_enc_fmt enc_fmt;
>  	struct v4l2_frmsize_stepwise frmsize;
>  	bool postprocessed;
> +	bool match_depth;
>  };
>  
>  struct hantro_reg {
> diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
> index 29cc61d53b71..334f18a4120d 100644
> --- a/drivers/staging/media/hantro/hantro_v4l2.c
> +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> @@ -64,6 +64,42 @@ hantro_get_postproc_formats(const struct hantro_ctx *ctx,
>  	return ctx->dev->variant->postproc_fmts;
>  }
>  
> +int hantro_get_format_depth(u32 fourcc)
> +{
> +	switch (fourcc) {
> +	case V4L2_PIX_FMT_P010:
> +	case V4L2_PIX_FMT_P010_4L4:
> +		return 10;
> +	default:
> +		return 8;
> +	}
> +}
> +
> +static bool
> +hantro_check_depth_match(const struct hantro_ctx *ctx,
> +			 const struct hantro_fmt *fmt)
> +{
> +	int fmt_depth, ctx_depth = 8;
> +
> +	if (!fmt->match_depth && !fmt->postprocessed)
> +		return true;
> +
> +	/* 0 means default depth, which is 8 */
> +	if (ctx->bit_depth)
> +		ctx_depth = ctx->bit_depth;
> +
> +	fmt_depth = hantro_get_format_depth(fmt->fourcc);
> +
> +	/*
> +	 * Allow only downconversion for postproc formats for now.
> +	 * It may be possible to relax that on some HW.
> +	 */
> +	if (!fmt->match_depth)
> +		return fmt_depth <= ctx_depth;
> +
> +	return fmt_depth == ctx_depth;
> +}
> +
>  static const struct hantro_fmt *
>  hantro_find_format(const struct hantro_ctx *ctx, u32 fourcc)
>  {
> @@ -91,7 +127,8 @@ hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream)
>  	formats = hantro_get_formats(ctx, &num_fmts);
>  	for (i = 0; i < num_fmts; i++) {
>  		if (bitstream == (formats[i].codec_mode !=
> -				  HANTRO_MODE_NONE))
> +				  HANTRO_MODE_NONE) &&
> +		    hantro_check_depth_match(ctx, &formats[i]))
>  			return &formats[i];
>  	}
>  	return NULL;
> @@ -162,11 +199,13 @@ static int vidioc_enum_fmt(struct file *file, void *priv,
>  	formats = hantro_get_formats(ctx, &num_fmts);
>  	for (i = 0; i < num_fmts; i++) {
>  		bool mode_none = formats[i].codec_mode == HANTRO_MODE_NONE;
> +		fmt = &formats[i];
>  
>  		if (skip_mode_none == mode_none)
>  			continue;
> +		if (!hantro_check_depth_match(ctx, fmt))
> +			continue;
>  		if (j == f->index) {
> -			fmt = &formats[i];
>  			f->pixelformat = fmt->fourcc;
>  			return 0;
>  		}
> @@ -182,8 +221,11 @@ static int vidioc_enum_fmt(struct file *file, void *priv,
>  		return -EINVAL;
>  	formats = hantro_get_postproc_formats(ctx, &num_fmts);
>  	for (i = 0; i < num_fmts; i++) {
> +		fmt = &formats[i];
> +
> +		if (!hantro_check_depth_match(ctx, fmt))
> +			continue;
>  		if (j == f->index) {
> -			fmt = &formats[i];
>  			f->pixelformat = fmt->fourcc;
>  			return 0;
>  		}
> diff --git a/drivers/staging/media/hantro/hantro_v4l2.h b/drivers/staging/media/hantro/hantro_v4l2.h
> index 18bc682c8556..b17e84c82582 100644
> --- a/drivers/staging/media/hantro/hantro_v4l2.h
> +++ b/drivers/staging/media/hantro/hantro_v4l2.h
> @@ -22,5 +22,6 @@ extern const struct v4l2_ioctl_ops hantro_ioctl_ops;
>  extern const struct vb2_ops hantro_queue_ops;
>  
>  void hantro_reset_fmts(struct hantro_ctx *ctx);
> +int hantro_get_format_depth(u32 fourcc);
>  
>  #endif /* HANTRO_V4L2_H_ */
> -- 
> 2.36.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/7] media: hantro: postproc: Fix buffer size calculation

[Jernej Skrabec]

When allocating aux buffers for postprocessing, it's assumed that base
buffer size is the same as that of output. Coincidentally, that's true
most of the time, but not always. 10-bit source also needs aux buffer
size which is appropriate for 10-bit native format, even if the output
format is 8-bit. Similarly, mv sizes and other extra buffer size also
depends on source width/height, not destination.

Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 .../staging/media/hantro/hantro_postproc.c    | 24 +++++++++++++------
 drivers/staging/media/hantro/hantro_v4l2.c    |  2 +-
 drivers/staging/media/hantro/hantro_v4l2.h    |  2 ++
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_postproc.c b/drivers/staging/media/hantro/hantro_postproc.c
index ab168c1c0d28..b77cc55e43ea 100644
--- a/drivers/staging/media/hantro/hantro_postproc.c
+++ b/drivers/staging/media/hantro/hantro_postproc.c
@@ -12,6 +12,7 @@
 #include "hantro_hw.h"
 #include "hantro_g1_regs.h"
 #include "hantro_g2_regs.h"
+#include "hantro_v4l2.h"
 
 #define HANTRO_PP_REG_WRITE(vpu, reg_name, val) \
 { \
@@ -174,18 +175,27 @@ int hantro_postproc_alloc(struct hantro_ctx *ctx)
 	struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
 	struct vb2_queue *cap_queue = &m2m_ctx->cap_q_ctx.q;
 	unsigned int num_buffers = cap_queue->num_buffers;
+	struct v4l2_pix_format_mplane pix_mp;
+	const struct hantro_fmt *fmt;
 	unsigned int i, buf_size;
 
-	buf_size = ctx->dst_fmt.plane_fmt[0].sizeimage;
+	/* this should always pick native format */
+	fmt = hantro_get_default_fmt(ctx, false);
+	if (!fmt)
+		return -EINVAL;
+	v4l2_fill_pixfmt_mp(&pix_mp, fmt->fourcc, ctx->src_fmt.width,
+			    ctx->src_fmt.height);
+
+	buf_size = pix_mp.plane_fmt[0].sizeimage;
 	if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE)
-		buf_size += hantro_h264_mv_size(ctx->dst_fmt.width,
-						ctx->dst_fmt.height);
+		buf_size += hantro_h264_mv_size(pix_mp.width,
+						pix_mp.height);
 	else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_VP9_FRAME)
-		buf_size += hantro_vp9_mv_size(ctx->dst_fmt.width,
-					       ctx->dst_fmt.height);
+		buf_size += hantro_vp9_mv_size(pix_mp.width,
+					       pix_mp.height);
 	else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_HEVC_SLICE)
-		buf_size += hantro_hevc_mv_size(ctx->dst_fmt.width,
-						ctx->dst_fmt.height);
+		buf_size += hantro_hevc_mv_size(pix_mp.width,
+						pix_mp.height);
 
 	for (i = 0; i < num_buffers; ++i) {
 		struct hantro_aux_buf *priv = &ctx->postproc.dec_q[i];
diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
index 334f18a4120d..2c7a805289e7 100644
--- a/drivers/staging/media/hantro/hantro_v4l2.c
+++ b/drivers/staging/media/hantro/hantro_v4l2.c
@@ -118,7 +118,7 @@ hantro_find_format(const struct hantro_ctx *ctx, u32 fourcc)
 	return NULL;
 }
 
-static const struct hantro_fmt *
+const struct hantro_fmt *
 hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream)
 {
 	const struct hantro_fmt *formats;
diff --git a/drivers/staging/media/hantro/hantro_v4l2.h b/drivers/staging/media/hantro/hantro_v4l2.h
index b17e84c82582..64f6f57e9d7a 100644
--- a/drivers/staging/media/hantro/hantro_v4l2.h
+++ b/drivers/staging/media/hantro/hantro_v4l2.h
@@ -23,5 +23,7 @@ extern const struct vb2_ops hantro_queue_ops;
 
 void hantro_reset_fmts(struct hantro_ctx *ctx);
 int hantro_get_format_depth(u32 fourcc);
+const struct hantro_fmt *
+hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream);
 
 #endif /* HANTRO_V4L2_H_ */
-- 
2.36.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/7] media: hantro: postproc: Fix buffer size calculation

[Ezequiel Garcia]

Hi Jernej,

On Thu, Jun 16, 2022 at 10:25:09PM +0200, Jernej Skrabec wrote:
> When allocating aux buffers for postprocessing, it's assumed that base
> buffer size is the same as that of output. Coincidentally, that's true
> most of the time, but not always. 10-bit source also needs aux buffer
> size which is appropriate for 10-bit native format, even if the output
> format is 8-bit. Similarly, mv sizes and other extra buffer size also
> depends on source width/height, not destination.
> 
> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

I took a new look at this patch.

> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  .../staging/media/hantro/hantro_postproc.c    | 24 +++++++++++++------
>  drivers/staging/media/hantro/hantro_v4l2.c    |  2 +-
>  drivers/staging/media/hantro/hantro_v4l2.h    |  2 ++
>  3 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_postproc.c b/drivers/staging/media/hantro/hantro_postproc.c
> index ab168c1c0d28..b77cc55e43ea 100644
> --- a/drivers/staging/media/hantro/hantro_postproc.c
> +++ b/drivers/staging/media/hantro/hantro_postproc.c
> @@ -12,6 +12,7 @@
>  #include "hantro_hw.h"
>  #include "hantro_g1_regs.h"
>  #include "hantro_g2_regs.h"
> +#include "hantro_v4l2.h"
>  
>  #define HANTRO_PP_REG_WRITE(vpu, reg_name, val) \
>  { \
> @@ -174,18 +175,27 @@ int hantro_postproc_alloc(struct hantro_ctx *ctx)
>  	struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
>  	struct vb2_queue *cap_queue = &m2m_ctx->cap_q_ctx.q;
>  	unsigned int num_buffers = cap_queue->num_buffers;
> +	struct v4l2_pix_format_mplane pix_mp;
> +	const struct hantro_fmt *fmt;
>  	unsigned int i, buf_size;
>  
> -	buf_size = ctx->dst_fmt.plane_fmt[0].sizeimage;
> +	/* this should always pick native format */
> +	fmt = hantro_get_default_fmt(ctx, false);

Clearly this is correct.

When the driver enables the post-processor it decodes a coded format (H264, etc.)
to a native format (NV12_4L4 or P010_4L4) and feeds this into the postprocessor
engine to produce some other format (YUYV, NV12, etc.).

The buffers allocated here should be taken from the native format,
so it's correct to use hantro_get_default_fmt().

> +	if (!fmt)
> +		return -EINVAL;
> +	v4l2_fill_pixfmt_mp(&pix_mp, fmt->fourcc, ctx->src_fmt.width,
> +			    ctx->src_fmt.height);

The issue comes at this point, where we negotiate the buffer size based on the
source size (OUTPUT queue size), instead of negotiating based
on the Native size.

  Coded -> [ Decoder ] -> Native -> [ Post-processor ] -> Decoded

So, while the patch is surely improving things, I wonder if it won't
cause other issues.

This reminds me we are still lacking a more complete test-suite for this
driver, so that we can validate changes and ensure there are no
regressions.

Perhaps we could hack Fluster to not only test the conformance,
but also test the post-processor?

Thanks,
Ezequiel


> +
> +	buf_size = pix_mp.plane_fmt[0].sizeimage;
>  	if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE)
> -		buf_size += hantro_h264_mv_size(ctx->dst_fmt.width,
> -						ctx->dst_fmt.height);
> +		buf_size += hantro_h264_mv_size(pix_mp.width,
> +						pix_mp.height);
>  	else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_VP9_FRAME)
> -		buf_size += hantro_vp9_mv_size(ctx->dst_fmt.width,
> -					       ctx->dst_fmt.height);
> +		buf_size += hantro_vp9_mv_size(pix_mp.width,
> +					       pix_mp.height);
>  	else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_HEVC_SLICE)
> -		buf_size += hantro_hevc_mv_size(ctx->dst_fmt.width,
> -						ctx->dst_fmt.height);
> +		buf_size += hantro_hevc_mv_size(pix_mp.width,
> +						pix_mp.height);
>  
>  	for (i = 0; i < num_buffers; ++i) {
>  		struct hantro_aux_buf *priv = &ctx->postproc.dec_q[i];
> diff --git a/drivers/staging/media/hantro/hantro_v4l2.c b/drivers/staging/media/hantro/hantro_v4l2.c
> index 334f18a4120d..2c7a805289e7 100644
> --- a/drivers/staging/media/hantro/hantro_v4l2.c
> +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> @@ -118,7 +118,7 @@ hantro_find_format(const struct hantro_ctx *ctx, u32 fourcc)
>  	return NULL;
>  }
>  
> -static const struct hantro_fmt *
> +const struct hantro_fmt *
>  hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream)
>  {
>  	const struct hantro_fmt *formats;
> diff --git a/drivers/staging/media/hantro/hantro_v4l2.h b/drivers/staging/media/hantro/hantro_v4l2.h
> index b17e84c82582..64f6f57e9d7a 100644
> --- a/drivers/staging/media/hantro/hantro_v4l2.h
> +++ b/drivers/staging/media/hantro/hantro_v4l2.h
> @@ -23,5 +23,7 @@ extern const struct vb2_ops hantro_queue_ops;
>  
>  void hantro_reset_fmts(struct hantro_ctx *ctx);
>  int hantro_get_format_depth(u32 fourcc);
> +const struct hantro_fmt *
> +hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream);
>  
>  #endif /* HANTRO_V4L2_H_ */
> -- 
> 2.36.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/7] media: hantro: postproc: Fix buffer size calculation

[Jernej Škrabec]

Dne torek, 28. junij 2022 ob 17:54:19 CEST je Ezequiel Garcia napisal(a):
> Hi Jernej,
> 
> On Thu, Jun 16, 2022 at 10:25:09PM +0200, Jernej Skrabec wrote:
> > When allocating aux buffers for postprocessing, it's assumed that base
> > buffer size is the same as that of output. Coincidentally, that's true
> > most of the time, but not always. 10-bit source also needs aux buffer
> > size which is appropriate for 10-bit native format, even if the output
> > format is 8-bit. Similarly, mv sizes and other extra buffer size also
> > depends on source width/height, not destination.
> > 
> > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> 
> I took a new look at this patch.
> 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > ---
> > 
> >  .../staging/media/hantro/hantro_postproc.c    | 24 +++++++++++++------
> >  drivers/staging/media/hantro/hantro_v4l2.c    |  2 +-
> >  drivers/staging/media/hantro/hantro_v4l2.h    |  2 ++
> >  3 files changed, 20 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_postproc.c
> > b/drivers/staging/media/hantro/hantro_postproc.c index
> > ab168c1c0d28..b77cc55e43ea 100644
> > --- a/drivers/staging/media/hantro/hantro_postproc.c
> > +++ b/drivers/staging/media/hantro/hantro_postproc.c
> > @@ -12,6 +12,7 @@
> > 
> >  #include "hantro_hw.h"
> >  #include "hantro_g1_regs.h"
> >  #include "hantro_g2_regs.h"
> > 
> > +#include "hantro_v4l2.h"
> > 
> >  #define HANTRO_PP_REG_WRITE(vpu, reg_name, val) \
> >  { \
> > 
> > @@ -174,18 +175,27 @@ int hantro_postproc_alloc(struct hantro_ctx *ctx)
> > 
> >  	struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
> >  	struct vb2_queue *cap_queue = &m2m_ctx->cap_q_ctx.q;
> >  	unsigned int num_buffers = cap_queue->num_buffers;
> > 
> > +	struct v4l2_pix_format_mplane pix_mp;
> > +	const struct hantro_fmt *fmt;
> > 
> >  	unsigned int i, buf_size;
> > 
> > -	buf_size = ctx->dst_fmt.plane_fmt[0].sizeimage;
> > +	/* this should always pick native format */
> > +	fmt = hantro_get_default_fmt(ctx, false);
> 
> Clearly this is correct.
> 
> When the driver enables the post-processor it decodes a coded format (H264,
> etc.) to a native format (NV12_4L4 or P010_4L4) and feeds this into the
> postprocessor engine to produce some other format (YUYV, NV12, etc.).
> 
> The buffers allocated here should be taken from the native format,
> so it's correct to use hantro_get_default_fmt().
> 
> > +	if (!fmt)
> > +		return -EINVAL;
> > +	v4l2_fill_pixfmt_mp(&pix_mp, fmt->fourcc, ctx->src_fmt.width,
> > +			    ctx->src_fmt.height);
> 
> The issue comes at this point, where we negotiate the buffer size based on
> the source size (OUTPUT queue size), instead of negotiating based
> on the Native size.
> 
>   Coded -> [ Decoder ] -> Native -> [ Post-processor ] -> Decoded

I'm not sure what is the difference between source and native size? You mean 
one coded in controls and one set via output format? IMO they should always be 
the same, otherwise it can be considered a bug in userspace application.

Best regards,
Jernej

> 
> So, while the patch is surely improving things, I wonder if it won't
> cause other issues.
> 
> This reminds me we are still lacking a more complete test-suite for this
> driver, so that we can validate changes and ensure there are no
> regressions.
> 
> Perhaps we could hack Fluster to not only test the conformance,
> but also test the post-processor?
> 
> Thanks,
> Ezequiel
> 
> > +
> > +	buf_size = pix_mp.plane_fmt[0].sizeimage;
> > 
> >  	if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE)
> > 
> > -		buf_size += hantro_h264_mv_size(ctx->dst_fmt.width,
> > -						ctx-
>dst_fmt.height);
> > +		buf_size += hantro_h264_mv_size(pix_mp.width,
> > +						
pix_mp.height);
> > 
> >  	else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_VP9_FRAME)
> > 
> > -		buf_size += hantro_vp9_mv_size(ctx->dst_fmt.width,
> > -					       ctx-
>dst_fmt.height);
> > +		buf_size += hantro_vp9_mv_size(pix_mp.width,
> > +					       pix_mp.height);
> > 
> >  	else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_HEVC_SLICE)
> > 
> > -		buf_size += hantro_hevc_mv_size(ctx->dst_fmt.width,
> > -						ctx-
>dst_fmt.height);
> > +		buf_size += hantro_hevc_mv_size(pix_mp.width,
> > +						
pix_mp.height);
> > 
> >  	for (i = 0; i < num_buffers; ++i) {
> >  	
> >  		struct hantro_aux_buf *priv = &ctx->postproc.dec_q[i];
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_v4l2.c
> > b/drivers/staging/media/hantro/hantro_v4l2.c index
> > 334f18a4120d..2c7a805289e7 100644
> > --- a/drivers/staging/media/hantro/hantro_v4l2.c
> > +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> > @@ -118,7 +118,7 @@ hantro_find_format(const struct hantro_ctx *ctx, u32
> > fourcc)> 
> >  	return NULL;
> >  
> >  }
> > 
> > -static const struct hantro_fmt *
> > +const struct hantro_fmt *
> > 
> >  hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream)
> >  {
> >  
> >  	const struct hantro_fmt *formats;
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_v4l2.h
> > b/drivers/staging/media/hantro/hantro_v4l2.h index
> > b17e84c82582..64f6f57e9d7a 100644
> > --- a/drivers/staging/media/hantro/hantro_v4l2.h
> > +++ b/drivers/staging/media/hantro/hantro_v4l2.h
> > @@ -23,5 +23,7 @@ extern const struct vb2_ops hantro_queue_ops;
> > 
> >  void hantro_reset_fmts(struct hantro_ctx *ctx);
> >  int hantro_get_format_depth(u32 fourcc);
> > 
> > +const struct hantro_fmt *
> > +hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream);
> > 
> >  #endif /* HANTRO_V4L2_H_ */





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/7] media: hantro: postproc: Fix buffer size calculation

[Nicolas Dufresne]

Le mardi 28 juin 2022 à 18:13 +0200, Jernej Škrabec a écrit :
> Dne torek, 28. junij 2022 ob 17:54:19 CEST je Ezequiel Garcia napisal(a):
> > Hi Jernej,
> > 
> > On Thu, Jun 16, 2022 at 10:25:09PM +0200, Jernej Skrabec wrote:
> > > When allocating aux buffers for postprocessing, it's assumed that base
> > > buffer size is the same as that of output. Coincidentally, that's true
> > > most of the time, but not always. 10-bit source also needs aux buffer
> > > size which is appropriate for 10-bit native format, even if the output
> > > format is 8-bit. Similarly, mv sizes and other extra buffer size also
> > > depends on source width/height, not destination.
> > > 
> > > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> > 
> > I took a new look at this patch.
> > 
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > > ---
> > > 
> > >  .../staging/media/hantro/hantro_postproc.c    | 24 +++++++++++++------
> > >  drivers/staging/media/hantro/hantro_v4l2.c    |  2 +-
> > >  drivers/staging/media/hantro/hantro_v4l2.h    |  2 ++
> > >  3 files changed, 20 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/hantro/hantro_postproc.c
> > > b/drivers/staging/media/hantro/hantro_postproc.c index
> > > ab168c1c0d28..b77cc55e43ea 100644
> > > --- a/drivers/staging/media/hantro/hantro_postproc.c
> > > +++ b/drivers/staging/media/hantro/hantro_postproc.c
> > > @@ -12,6 +12,7 @@
> > > 
> > >  #include "hantro_hw.h"
> > >  #include "hantro_g1_regs.h"
> > >  #include "hantro_g2_regs.h"
> > > 
> > > +#include "hantro_v4l2.h"
> > > 
> > >  #define HANTRO_PP_REG_WRITE(vpu, reg_name, val) \
> > >  { \
> > > 
> > > @@ -174,18 +175,27 @@ int hantro_postproc_alloc(struct hantro_ctx *ctx)
> > > 
> > >  	struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
> > >  	struct vb2_queue *cap_queue = &m2m_ctx->cap_q_ctx.q;
> > >  	unsigned int num_buffers = cap_queue->num_buffers;
> > > 
> > > +	struct v4l2_pix_format_mplane pix_mp;
> > > +	const struct hantro_fmt *fmt;
> > > 
> > >  	unsigned int i, buf_size;
> > > 
> > > -	buf_size = ctx->dst_fmt.plane_fmt[0].sizeimage;
> > > +	/* this should always pick native format */
> > > +	fmt = hantro_get_default_fmt(ctx, false);
> > 
> > Clearly this is correct.
> > 
> > When the driver enables the post-processor it decodes a coded format (H264,
> > etc.) to a native format (NV12_4L4 or P010_4L4) and feeds this into the
> > postprocessor engine to produce some other format (YUYV, NV12, etc.).
> > 
> > The buffers allocated here should be taken from the native format,
> > so it's correct to use hantro_get_default_fmt().
> > 
> > > +	if (!fmt)
> > > +		return -EINVAL;
> > > +	v4l2_fill_pixfmt_mp(&pix_mp, fmt->fourcc, ctx->src_fmt.width,
> > > +			    ctx->src_fmt.height);
> > 
> > The issue comes at this point, where we negotiate the buffer size based on
> > the source size (OUTPUT queue size), instead of negotiating based
> > on the Native size.
> > 
> >   Coded -> [ Decoder ] -> Native -> [ Post-processor ] -> Decoded
> 
> I'm not sure what is the difference between source and native size? You mean 
> one coded in controls and one set via output format? IMO they should always be 
> the same, otherwise it can be considered a bug in userspace application.

Indeed the src_fmt should use coded width/height (as per spec). The driver will
then adapt to its own requirement resulting into the "native" width height being
returned. Notice that s_ctrl() should fail in case of miss-match (this is CODEC
specific), or streamon() should fail if the codec specific control have never
been set (as we always initialise this, it will fail due to default being an
invalid value anyway).

As a side effect, when userland read the default format (G_FMT(CAPTURE), the
width/height should match the src_dst for this driver. This is the native size.
The optional path that this driver enables is enumeration of CAPTURE format and
frame sizes, combined with to select from these. The driver will create a
secondary set of buffers in the case.

Nicolas

> 
> Best regards,
> Jernej
> 
> > 
> > So, while the patch is surely improving things, I wonder if it won't
> > cause other issues.
> > 
> > This reminds me we are still lacking a more complete test-suite for this
> > driver, so that we can validate changes and ensure there are no
> > regressions.
> > 
> > Perhaps we could hack Fluster to not only test the conformance,
> > but also test the post-processor?
> > 
> > Thanks,
> > Ezequiel
> > 
> > > +
> > > +	buf_size = pix_mp.plane_fmt[0].sizeimage;
> > > 
> > >  	if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE)
> > > 
> > > -		buf_size += hantro_h264_mv_size(ctx->dst_fmt.width,
> > > -						ctx-
> > dst_fmt.height);
> > > +		buf_size += hantro_h264_mv_size(pix_mp.width,
> > > +						
> pix_mp.height);
> > > 
> > >  	else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_VP9_FRAME)
> > > 
> > > -		buf_size += hantro_vp9_mv_size(ctx->dst_fmt.width,
> > > -					       ctx-
> > dst_fmt.height);
> > > +		buf_size += hantro_vp9_mv_size(pix_mp.width,
> > > +					       pix_mp.height);
> > > 
> > >  	else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_HEVC_SLICE)
> > > 
> > > -		buf_size += hantro_hevc_mv_size(ctx->dst_fmt.width,
> > > -						ctx-
> > dst_fmt.height);
> > > +		buf_size += hantro_hevc_mv_size(pix_mp.width,
> > > +						
> pix_mp.height);
> > > 
> > >  	for (i = 0; i < num_buffers; ++i) {
> > >  	
> > >  		struct hantro_aux_buf *priv = &ctx->postproc.dec_q[i];
> > > 
> > > diff --git a/drivers/staging/media/hantro/hantro_v4l2.c
> > > b/drivers/staging/media/hantro/hantro_v4l2.c index
> > > 334f18a4120d..2c7a805289e7 100644
> > > --- a/drivers/staging/media/hantro/hantro_v4l2.c
> > > +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> > > @@ -118,7 +118,7 @@ hantro_find_format(const struct hantro_ctx *ctx, u32
> > > fourcc)> 
> > >  	return NULL;
> > >  
> > >  }
> > > 
> > > -static const struct hantro_fmt *
> > > +const struct hantro_fmt *
> > > 
> > >  hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream)
> > >  {
> > >  
> > >  	const struct hantro_fmt *formats;
> > > 
> > > diff --git a/drivers/staging/media/hantro/hantro_v4l2.h
> > > b/drivers/staging/media/hantro/hantro_v4l2.h index
> > > b17e84c82582..64f6f57e9d7a 100644
> > > --- a/drivers/staging/media/hantro/hantro_v4l2.h
> > > +++ b/drivers/staging/media/hantro/hantro_v4l2.h
> > > @@ -23,5 +23,7 @@ extern const struct vb2_ops hantro_queue_ops;
> > > 
> > >  void hantro_reset_fmts(struct hantro_ctx *ctx);
> > >  int hantro_get_format_depth(u32 fourcc);
> > > 
> > > +const struct hantro_fmt *
> > > +hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream);
> > > 
> > >  #endif /* HANTRO_V4L2_H_ */
> 
> 
> 
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/7] media: hantro: postproc: Fix buffer size calculation

[Ezequiel Garcia]

Hi Nicolas, Jernej,

On Tue, Jun 28, 2022 at 04:06:13PM -0400, Nicolas Dufresne wrote:
> Le mardi 28 juin 2022 à 18:13 +0200, Jernej Škrabec a écrit :
> > Dne torek, 28. junij 2022 ob 17:54:19 CEST je Ezequiel Garcia napisal(a):
> > > Hi Jernej,
> > > 
> > > On Thu, Jun 16, 2022 at 10:25:09PM +0200, Jernej Skrabec wrote:
> > > > When allocating aux buffers for postprocessing, it's assumed that base
> > > > buffer size is the same as that of output. Coincidentally, that's true
> > > > most of the time, but not always. 10-bit source also needs aux buffer
> > > > size which is appropriate for 10-bit native format, even if the output
> > > > format is 8-bit. Similarly, mv sizes and other extra buffer size also
> > > > depends on source width/height, not destination.
> > > > 
> > > > Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> > > 
> > > I took a new look at this patch.
> > > 
> > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > > > ---
> > > > 
> > > >  .../staging/media/hantro/hantro_postproc.c    | 24 +++++++++++++------
> > > >  drivers/staging/media/hantro/hantro_v4l2.c    |  2 +-
> > > >  drivers/staging/media/hantro/hantro_v4l2.h    |  2 ++
> > > >  3 files changed, 20 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/media/hantro/hantro_postproc.c
> > > > b/drivers/staging/media/hantro/hantro_postproc.c index
> > > > ab168c1c0d28..b77cc55e43ea 100644
> > > > --- a/drivers/staging/media/hantro/hantro_postproc.c
> > > > +++ b/drivers/staging/media/hantro/hantro_postproc.c
> > > > @@ -12,6 +12,7 @@
> > > > 
> > > >  #include "hantro_hw.h"
> > > >  #include "hantro_g1_regs.h"
> > > >  #include "hantro_g2_regs.h"
> > > > 
> > > > +#include "hantro_v4l2.h"
> > > > 
> > > >  #define HANTRO_PP_REG_WRITE(vpu, reg_name, val) \
> > > >  { \
> > > > 
> > > > @@ -174,18 +175,27 @@ int hantro_postproc_alloc(struct hantro_ctx *ctx)
> > > > 
> > > >  	struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
> > > >  	struct vb2_queue *cap_queue = &m2m_ctx->cap_q_ctx.q;
> > > >  	unsigned int num_buffers = cap_queue->num_buffers;
> > > > 
> > > > +	struct v4l2_pix_format_mplane pix_mp;
> > > > +	const struct hantro_fmt *fmt;
> > > > 
> > > >  	unsigned int i, buf_size;
> > > > 
> > > > -	buf_size = ctx->dst_fmt.plane_fmt[0].sizeimage;
> > > > +	/* this should always pick native format */
> > > > +	fmt = hantro_get_default_fmt(ctx, false);
> > > 
> > > Clearly this is correct.
> > > 
> > > When the driver enables the post-processor it decodes a coded format (H264,
> > > etc.) to a native format (NV12_4L4 or P010_4L4) and feeds this into the
> > > postprocessor engine to produce some other format (YUYV, NV12, etc.).
> > > 
> > > The buffers allocated here should be taken from the native format,
> > > so it's correct to use hantro_get_default_fmt().
> > > 
> > > > +	if (!fmt)
> > > > +		return -EINVAL;
> > > > +	v4l2_fill_pixfmt_mp(&pix_mp, fmt->fourcc, ctx->src_fmt.width,
> > > > +			    ctx->src_fmt.height);
> > > 
> > > The issue comes at this point, where we negotiate the buffer size based on
> > > the source size (OUTPUT queue size), instead of negotiating based
> > > on the Native size.
> > > 
> > >   Coded -> [ Decoder ] -> Native -> [ Post-processor ] -> Decoded
> > 
> > I'm not sure what is the difference between source and native size? You mean 
> > one coded in controls and one set via output format? IMO they should always be 
> > the same, otherwise it can be considered a bug in userspace application.
> 
> Indeed the src_fmt should use coded width/height (as per spec). The driver will
> then adapt to its own requirement resulting into the "native" width height being
> returned. Notice that s_ctrl() should fail in case of miss-match (this is CODEC
> specific), or streamon() should fail if the codec specific control have never
> been set (as we always initialise this, it will fail due to default being an
> invalid value anyway).
> 
> As a side effect, when userland read the default format (G_FMT(CAPTURE), the
> width/height should match the src_dst for this driver. This is the native size.
> The optional path that this driver enables is enumeration of CAPTURE format and
> frame sizes, combined with to select from these. The driver will create a
> secondary set of buffers in the case.
> 

OK, the patch looks good then.

Thanks,
Ezequiel

> Nicolas
> 
> > 
> > Best regards,
> > Jernej
> > 
> > > 
> > > So, while the patch is surely improving things, I wonder if it won't
> > > cause other issues.
> > > 
> > > This reminds me we are still lacking a more complete test-suite for this
> > > driver, so that we can validate changes and ensure there are no
> > > regressions.
> > > 
> > > Perhaps we could hack Fluster to not only test the conformance,
> > > but also test the post-processor?
> > > 
> > > Thanks,
> > > Ezequiel
> > > 
> > > > +
> > > > +	buf_size = pix_mp.plane_fmt[0].sizeimage;
> > > > 
> > > >  	if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_H264_SLICE)
> > > > 
> > > > -		buf_size += hantro_h264_mv_size(ctx->dst_fmt.width,
> > > > -						ctx-
> > > dst_fmt.height);
> > > > +		buf_size += hantro_h264_mv_size(pix_mp.width,
> > > > +						
> > pix_mp.height);
> > > > 
> > > >  	else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_VP9_FRAME)
> > > > 
> > > > -		buf_size += hantro_vp9_mv_size(ctx->dst_fmt.width,
> > > > -					       ctx-
> > > dst_fmt.height);
> > > > +		buf_size += hantro_vp9_mv_size(pix_mp.width,
> > > > +					       pix_mp.height);
> > > > 
> > > >  	else if (ctx->vpu_src_fmt->fourcc == V4L2_PIX_FMT_HEVC_SLICE)
> > > > 
> > > > -		buf_size += hantro_hevc_mv_size(ctx->dst_fmt.width,
> > > > -						ctx-
> > > dst_fmt.height);
> > > > +		buf_size += hantro_hevc_mv_size(pix_mp.width,
> > > > +						
> > pix_mp.height);
> > > > 
> > > >  	for (i = 0; i < num_buffers; ++i) {
> > > >  	
> > > >  		struct hantro_aux_buf *priv = &ctx->postproc.dec_q[i];
> > > > 
> > > > diff --git a/drivers/staging/media/hantro/hantro_v4l2.c
> > > > b/drivers/staging/media/hantro/hantro_v4l2.c index
> > > > 334f18a4120d..2c7a805289e7 100644
> > > > --- a/drivers/staging/media/hantro/hantro_v4l2.c
> > > > +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> > > > @@ -118,7 +118,7 @@ hantro_find_format(const struct hantro_ctx *ctx, u32
> > > > fourcc)> 
> > > >  	return NULL;
> > > >  
> > > >  }
> > > > 
> > > > -static const struct hantro_fmt *
> > > > +const struct hantro_fmt *
> > > > 
> > > >  hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream)
> > > >  {
> > > >  
> > > >  	const struct hantro_fmt *formats;
> > > > 
> > > > diff --git a/drivers/staging/media/hantro/hantro_v4l2.h
> > > > b/drivers/staging/media/hantro/hantro_v4l2.h index
> > > > b17e84c82582..64f6f57e9d7a 100644
> > > > --- a/drivers/staging/media/hantro/hantro_v4l2.h
> > > > +++ b/drivers/staging/media/hantro/hantro_v4l2.h
> > > > @@ -23,5 +23,7 @@ extern const struct vb2_ops hantro_queue_ops;
> > > > 
> > > >  void hantro_reset_fmts(struct hantro_ctx *ctx);
> > > >  int hantro_get_format_depth(u32 fourcc);
> > > > 
> > > > +const struct hantro_fmt *
> > > > +hantro_get_default_fmt(const struct hantro_ctx *ctx, bool bitstream);
> > > > 
> > > >  #endif /* HANTRO_V4L2_H_ */
> > 
> > 
> > 
> > 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/7] media: hantro: postproc: Fix legacy regs configuration

[Jernej Skrabec]

Some postproc legacy registers were set in VP9 code. Move them to
postproc and fix their value.

Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/staging/media/hantro/hantro_g2_vp9_dec.c |  8 --------
 drivers/staging/media/hantro/hantro_postproc.c   | 10 ++++++++++
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_g2_vp9_dec.c b/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
index 91c21b634fab..c9cb11fd95af 100644
--- a/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
+++ b/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
@@ -515,16 +515,8 @@ static void
 config_bit_depth(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_frame *dec_params)
 {
 	if (ctx->dev->variant->legacy_regs) {
-		u8 pp_shift = 0;
-
 		hantro_reg_write(ctx->dev, &g2_bit_depth_y, dec_params->bit_depth);
 		hantro_reg_write(ctx->dev, &g2_bit_depth_c, dec_params->bit_depth);
-		hantro_reg_write(ctx->dev, &g2_rs_out_bit_depth, dec_params->bit_depth);
-
-		if (dec_params->bit_depth > 8)
-			pp_shift = 16 - dec_params->bit_depth;
-
-		hantro_reg_write(ctx->dev, &g2_pp_pix_shift, pp_shift);
 		hantro_reg_write(ctx->dev, &g2_pix_shift, 0);
 	} else {
 		hantro_reg_write(ctx->dev, &g2_bit_depth_y_minus8, dec_params->bit_depth - 8);
diff --git a/drivers/staging/media/hantro/hantro_postproc.c b/drivers/staging/media/hantro/hantro_postproc.c
index b77cc55e43ea..8933b4af73ed 100644
--- a/drivers/staging/media/hantro/hantro_postproc.c
+++ b/drivers/staging/media/hantro/hantro_postproc.c
@@ -130,6 +130,16 @@ static void hantro_postproc_g2_enable(struct hantro_ctx *ctx)
 		hantro_write_addr(vpu, G2_RS_OUT_LUMA_ADDR, dst_dma);
 		hantro_write_addr(vpu, G2_RS_OUT_CHROMA_ADDR, dst_dma + chroma_offset);
 	}
+	if (ctx->dev->variant->legacy_regs) {
+		int out_depth = hantro_get_format_depth(ctx->dst_fmt.pixelformat);
+		u8 pp_shift = 0;
+
+		if (out_depth > 8)
+			pp_shift = 16 - out_depth;
+
+		hantro_reg_write(ctx->dev, &g2_rs_out_bit_depth, out_depth);
+		hantro_reg_write(ctx->dev, &g2_pp_pix_shift, pp_shift);
+	}
 	hantro_reg_write(vpu, &g2_out_rs_e, 1);
 }
 
-- 
2.36.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 5/7] media: hantro: postproc: Properly calculate chroma offset

[Jernej Skrabec]

Currently chroma offset calculation assumes only 1 byte per luma, with
no consideration for stride.

Take necessary information from destination pixel format which makes
calculation completely universal.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/staging/media/hantro/hantro_postproc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/hantro/hantro_postproc.c b/drivers/staging/media/hantro/hantro_postproc.c
index 8933b4af73ed..a0928c508434 100644
--- a/drivers/staging/media/hantro/hantro_postproc.c
+++ b/drivers/staging/media/hantro/hantro_postproc.c
@@ -113,12 +113,14 @@ static void hantro_postproc_g2_enable(struct hantro_ctx *ctx)
 {
 	struct hantro_dev *vpu = ctx->dev;
 	struct vb2_v4l2_buffer *dst_buf;
-	size_t chroma_offset = ctx->dst_fmt.width * ctx->dst_fmt.height;
 	int down_scale = down_scale_factor(ctx);
+	size_t chroma_offset;
 	dma_addr_t dst_dma;
 
 	dst_buf = hantro_get_dst_buf(ctx);
 	dst_dma = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
+	chroma_offset = ctx->dst_fmt.plane_fmt[0].bytesperline *
+			ctx->dst_fmt.height;
 
 	if (down_scale) {
 		hantro_reg_write(vpu, &g2_down_scale_e, 1);
-- 
2.36.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/7] media: hantro: postproc: Properly calculate chroma offset

[Ezequiel Garcia]

Hi Jernej,

On Thu, Jun 16, 2022 at 10:25:11PM +0200, Jernej Skrabec wrote:
> Currently chroma offset calculation assumes only 1 byte per luma, with
> no consideration for stride.
> 
> Take necessary information from destination pixel format which makes
> calculation completely universal.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>

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

Thanks,
Ezequiel

> ---
>  drivers/staging/media/hantro/hantro_postproc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_postproc.c b/drivers/staging/media/hantro/hantro_postproc.c
> index 8933b4af73ed..a0928c508434 100644
> --- a/drivers/staging/media/hantro/hantro_postproc.c
> +++ b/drivers/staging/media/hantro/hantro_postproc.c
> @@ -113,12 +113,14 @@ static void hantro_postproc_g2_enable(struct hantro_ctx *ctx)
>  {
>  	struct hantro_dev *vpu = ctx->dev;
>  	struct vb2_v4l2_buffer *dst_buf;
> -	size_t chroma_offset = ctx->dst_fmt.width * ctx->dst_fmt.height;
>  	int down_scale = down_scale_factor(ctx);
> +	size_t chroma_offset;
>  	dma_addr_t dst_dma;
>  
>  	dst_buf = hantro_get_dst_buf(ctx);
>  	dst_dma = vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
> +	chroma_offset = ctx->dst_fmt.plane_fmt[0].bytesperline *
> +			ctx->dst_fmt.height;
>  
>  	if (down_scale) {
>  		hantro_reg_write(vpu, &g2_down_scale_e, 1);
> -- 
> 2.36.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 6/7] media: hantro: Store VP9 bit depth in context

[Jernej Skrabec]

Now that we have proper infrastructure for postprocessing 10-bit
formats, store VP9 bit depth in context.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/staging/media/hantro/hantro_drv.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index 01d33dcb0467..afddf7ac0731 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -324,6 +324,24 @@ static int hantro_hevc_s_ctrl(struct v4l2_ctrl *ctrl)
 	return 0;
 }
 
+static int hantro_vp9_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct hantro_ctx *ctx;
+
+	ctx = container_of(ctrl->handler,
+			   struct hantro_ctx, ctrl_handler);
+
+	switch (ctrl->id) {
+	case V4L2_CID_STATELESS_VP9_FRAME:
+		ctx->bit_depth = ctrl->p_new.p_vp9_frame->bit_depth;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static const struct v4l2_ctrl_ops hantro_ctrl_ops = {
 	.try_ctrl = hantro_try_ctrl,
 };
@@ -336,6 +354,10 @@ static const struct v4l2_ctrl_ops hantro_hevc_ctrl_ops = {
 	.s_ctrl = hantro_hevc_s_ctrl,
 };
 
+static const struct v4l2_ctrl_ops hantro_vp9_ctrl_ops = {
+	.s_ctrl = hantro_vp9_s_ctrl,
+};
+
 #define HANTRO_JPEG_ACTIVE_MARKERS	(V4L2_JPEG_ACTIVE_MARKER_APP0 | \
 					 V4L2_JPEG_ACTIVE_MARKER_COM | \
 					 V4L2_JPEG_ACTIVE_MARKER_DQT | \
@@ -503,6 +525,7 @@ static const struct hantro_ctrl controls[] = {
 		.codec = HANTRO_VP9_DECODER,
 		.cfg = {
 			.id = V4L2_CID_STATELESS_VP9_FRAME,
+			.ops = &hantro_vp9_ctrl_ops,
 		},
 	}, {
 		.codec = HANTRO_VP9_DECODER,
-- 
2.36.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 6/7] media: hantro: Store VP9 bit depth in context

[Ezequiel Garcia]

Jernej,

On Thu, Jun 16, 2022 at 10:25:12PM +0200, Jernej Skrabec wrote:
> Now that we have proper infrastructure for postprocessing 10-bit
> formats, store VP9 bit depth in context.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  drivers/staging/media/hantro/hantro_drv.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> index 01d33dcb0467..afddf7ac0731 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -324,6 +324,24 @@ static int hantro_hevc_s_ctrl(struct v4l2_ctrl *ctrl)
>  	return 0;
>  }
>  
> +static int hantro_vp9_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct hantro_ctx *ctx;
> +
> +	ctx = container_of(ctrl->handler,
> +			   struct hantro_ctx, ctrl_handler);
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_STATELESS_VP9_FRAME:
> +		ctx->bit_depth = ctrl->p_new.p_vp9_frame->bit_depth;

Since this affects the possible formats, shouldn't it reset
the decoded format?

In other words, it would mean calling hantro_reset_raw_fmt().

Hopefully, that would be OK and not cause any weird issues?

Nicolas, any feedback?

Thanks,
Ezequiel

> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct v4l2_ctrl_ops hantro_ctrl_ops = {
>  	.try_ctrl = hantro_try_ctrl,
>  };
> @@ -336,6 +354,10 @@ static const struct v4l2_ctrl_ops hantro_hevc_ctrl_ops = {
>  	.s_ctrl = hantro_hevc_s_ctrl,
>  };
>  
> +static const struct v4l2_ctrl_ops hantro_vp9_ctrl_ops = {
> +	.s_ctrl = hantro_vp9_s_ctrl,
> +};
> +
>  #define HANTRO_JPEG_ACTIVE_MARKERS	(V4L2_JPEG_ACTIVE_MARKER_APP0 | \
>  					 V4L2_JPEG_ACTIVE_MARKER_COM | \
>  					 V4L2_JPEG_ACTIVE_MARKER_DQT | \
> @@ -503,6 +525,7 @@ static const struct hantro_ctrl controls[] = {
>  		.codec = HANTRO_VP9_DECODER,
>  		.cfg = {
>  			.id = V4L2_CID_STATELESS_VP9_FRAME,
> +			.ops = &hantro_vp9_ctrl_ops,
>  		},
>  	}, {
>  		.codec = HANTRO_VP9_DECODER,
> -- 
> 2.36.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 6/7] media: hantro: Store VP9 bit depth in context

[Ezequiel Garcia]

Hi Jernej,

On Thu, Jun 16, 2022 at 10:25:12PM +0200, Jernej Skrabec wrote:
> Now that we have proper infrastructure for postprocessing 10-bit
> formats, store VP9 bit depth in context.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>

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

Thanks,
Ezequiel

> ---
>  drivers/staging/media/hantro/hantro_drv.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> index 01d33dcb0467..afddf7ac0731 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -324,6 +324,24 @@ static int hantro_hevc_s_ctrl(struct v4l2_ctrl *ctrl)
>  	return 0;
>  }
>  
> +static int hantro_vp9_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct hantro_ctx *ctx;
> +
> +	ctx = container_of(ctrl->handler,
> +			   struct hantro_ctx, ctrl_handler);
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_STATELESS_VP9_FRAME:
> +		ctx->bit_depth = ctrl->p_new.p_vp9_frame->bit_depth;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct v4l2_ctrl_ops hantro_ctrl_ops = {
>  	.try_ctrl = hantro_try_ctrl,
>  };
> @@ -336,6 +354,10 @@ static const struct v4l2_ctrl_ops hantro_hevc_ctrl_ops = {
>  	.s_ctrl = hantro_hevc_s_ctrl,
>  };
>  
> +static const struct v4l2_ctrl_ops hantro_vp9_ctrl_ops = {
> +	.s_ctrl = hantro_vp9_s_ctrl,
> +};
> +
>  #define HANTRO_JPEG_ACTIVE_MARKERS	(V4L2_JPEG_ACTIVE_MARKER_APP0 | \
>  					 V4L2_JPEG_ACTIVE_MARKER_COM | \
>  					 V4L2_JPEG_ACTIVE_MARKER_DQT | \
> @@ -503,6 +525,7 @@ static const struct hantro_ctrl controls[] = {
>  		.codec = HANTRO_VP9_DECODER,
>  		.cfg = {
>  			.id = V4L2_CID_STATELESS_VP9_FRAME,
> +			.ops = &hantro_vp9_ctrl_ops,
>  		},
>  	}, {
>  		.codec = HANTRO_VP9_DECODER,
> -- 
> 2.36.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 7/7] media: hantro: sunxi: Enable 10-bit decoding

[Jernej Skrabec]

Now that infrastructure for 10-bit decoding exists, enable it for
Allwinner H6.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/staging/media/hantro/sunxi_vpu_hw.c | 27 +++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/staging/media/hantro/sunxi_vpu_hw.c b/drivers/staging/media/hantro/sunxi_vpu_hw.c
index fbeac81e59e1..02ce8b064a8f 100644
--- a/drivers/staging/media/hantro/sunxi_vpu_hw.c
+++ b/drivers/staging/media/hantro/sunxi_vpu_hw.c
@@ -23,12 +23,39 @@ static const struct hantro_fmt sunxi_vpu_postproc_fmts[] = {
 			.step_height = 32,
 		},
 	},
+	{
+		.fourcc = V4L2_PIX_FMT_P010,
+		.codec_mode = HANTRO_MODE_NONE,
+		.postprocessed = true,
+		.frmsize = {
+			.min_width = FMT_MIN_WIDTH,
+			.max_width = FMT_UHD_WIDTH,
+			.step_width = 32,
+			.min_height = FMT_MIN_HEIGHT,
+			.max_height = FMT_UHD_HEIGHT,
+			.step_height = 32,
+		},
+	},
 };
 
 static const struct hantro_fmt sunxi_vpu_dec_fmts[] = {
 	{
 		.fourcc = V4L2_PIX_FMT_NV12_4L4,
 		.codec_mode = HANTRO_MODE_NONE,
+		.match_depth = true,
+		.frmsize = {
+			.min_width = FMT_MIN_WIDTH,
+			.max_width = FMT_UHD_WIDTH,
+			.step_width = 32,
+			.min_height = FMT_MIN_HEIGHT,
+			.max_height = FMT_UHD_HEIGHT,
+			.step_height = 32,
+		},
+	},
+	{
+		.fourcc = V4L2_PIX_FMT_P010_4L4,
+		.codec_mode = HANTRO_MODE_NONE,
+		.match_depth = true,
 		.frmsize = {
 			.min_width = FMT_MIN_WIDTH,
 			.max_width = FMT_UHD_WIDTH,
-- 
2.36.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 7/7] media: hantro: sunxi: Enable 10-bit decoding

[Ezequiel Garcia]

Hi Jernej,

On Thu, Jun 16, 2022 at 10:25:13PM +0200, Jernej Skrabec wrote:
> Now that infrastructure for 10-bit decoding exists, enable it for
> Allwinner H6.
> 

I don't have this hardware, but the patch seems OK.

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

> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  drivers/staging/media/hantro/sunxi_vpu_hw.c | 27 +++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/staging/media/hantro/sunxi_vpu_hw.c b/drivers/staging/media/hantro/sunxi_vpu_hw.c
> index fbeac81e59e1..02ce8b064a8f 100644
> --- a/drivers/staging/media/hantro/sunxi_vpu_hw.c
> +++ b/drivers/staging/media/hantro/sunxi_vpu_hw.c
> @@ -23,12 +23,39 @@ static const struct hantro_fmt sunxi_vpu_postproc_fmts[] = {
>  			.step_height = 32,
>  		},
>  	},
> +	{
> +		.fourcc = V4L2_PIX_FMT_P010,
> +		.codec_mode = HANTRO_MODE_NONE,
> +		.postprocessed = true,
> +		.frmsize = {
> +			.min_width = FMT_MIN_WIDTH,
> +			.max_width = FMT_UHD_WIDTH,
> +			.step_width = 32,
> +			.min_height = FMT_MIN_HEIGHT,
> +			.max_height = FMT_UHD_HEIGHT,
> +			.step_height = 32,
> +		},
> +	},
>  };
>  
>  static const struct hantro_fmt sunxi_vpu_dec_fmts[] = {
>  	{
>  		.fourcc = V4L2_PIX_FMT_NV12_4L4,
>  		.codec_mode = HANTRO_MODE_NONE,
> +		.match_depth = true,
> +		.frmsize = {
> +			.min_width = FMT_MIN_WIDTH,
> +			.max_width = FMT_UHD_WIDTH,
> +			.step_width = 32,
> +			.min_height = FMT_MIN_HEIGHT,
> +			.max_height = FMT_UHD_HEIGHT,
> +			.step_height = 32,
> +		},
> +	},
> +	{
> +		.fourcc = V4L2_PIX_FMT_P010_4L4,
> +		.codec_mode = HANTRO_MODE_NONE,
> +		.match_depth = true,
>  		.frmsize = {
>  			.min_width = FMT_MIN_WIDTH,
>  			.max_width = FMT_UHD_WIDTH,
> -- 
> 2.36.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/7] media: hantro: Add 10-bit support

[Benjamin Gaignard]

Le 16/06/2022 à 22:25, Jernej Skrabec a écrit :
> This series adds support for 10-bit format handling in Hantro driver.
> Most patches adjust postproc behaviour to be more universal. There is
> a lot of assumptions, which need to be replaced with more generalized.
>
> Tested on Allwinner H6. Tested with vp92-2-20-10bit-yuv420.webm sample,
> which produces correct checksum.
>
> Please take a look.

I have send a complementary series to this one to enable 10-bit
for HEVC on Hantro/G2 on IMX8MQ.
With these two patchset I'm able to decode all the 10-bit conformance
test files.

For this series:
Tested-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>

>
> Best regards,
> Jernej
>
> Changes from RFC:
> - fixed typo in function name
> - removed merged patch (P010 format)
> - removed broken patch (sunxi frequency change)
> - added new patch (media: hantro: postproc: Properly calculate chroma offset)
> - added r-b from Ezequiel
>
> Ezequiel Garcia (1):
>    media: Add P010 tiled format
>
> Jernej Skrabec (6):
>    media: hantro: Support format filtering by depth
>    media: hantro: postproc: Fix buffer size calculation
>    media: hantro: postproc: Fix legacy regs configuration
>    media: hantro: postproc: Properly calculate chroma offset
>    media: hantro: Store VP9 bit depth in context
>    media: hantro: sunxi: Enable 10-bit decoding
>
>   drivers/media/v4l2-core/v4l2-common.c         |  1 +
>   drivers/media/v4l2-core/v4l2-ioctl.c          |  1 +
>   drivers/staging/media/hantro/hantro.h         |  4 ++
>   drivers/staging/media/hantro/hantro_drv.c     | 23 +++++++++
>   .../staging/media/hantro/hantro_g2_vp9_dec.c  |  8 ---
>   .../staging/media/hantro/hantro_postproc.c    | 38 +++++++++++---
>   drivers/staging/media/hantro/hantro_v4l2.c    | 50 +++++++++++++++++--
>   drivers/staging/media/hantro/hantro_v4l2.h    |  3 ++
>   drivers/staging/media/hantro/sunxi_vpu_hw.c   | 27 ++++++++++
>   include/uapi/linux/videodev2.h                |  1 +
>   10 files changed, 136 insertions(+), 20 deletions(-)
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel