LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/3] media: cedrus: Add support for 4k videos
@ 2019-10-26  7:49 Jernej Skrabec
  2019-10-26  7:49 ` [PATCH 1/3] media: cedrus: Properly signal size in mode register Jernej Skrabec
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Jernej Skrabec @ 2019-10-26  7:49 UTC (permalink / raw)
  To: mripard, paul.kocialkowski
  Cc: mchehab, hverkuil-cisco, gregkh, wens, linux-media, devel,
	linux-arm-kernel, linux-kernel, linux-sunxi

While cedrus driver accepts videos up to 3840x2160, they are not decoded
correctly. Driver doesn't correctly set mode register for widths greater
than 2048 (patch 1). H264 engine also needs additional buffers which are
not provided currently (patch 2). Finally, there are several different
resolutions which can be considered 4k. Biggest is 4096x2304 which is
also supported by HW. Set that new maximum size (patch 3).

HEVC engine was not yet tested with 4k video, but as far as I know, it
doesn't need any special setting besides patch 1.

Following video was used for H264 video testing:
http://jernej.libreelec.tv/videos/h264/PUPPIES%20BATH%20IN%204K%20(ULTRA%20HD)(Original_H.264-AAC)%20(4ksamples.com).mp4

Note that at this point memory allocation is suboptimal and H264 engine
allocates far more memory that it is really needed. For above video to
work, I had to set CMA size to 512 MiB and add "vmalloc=512M" to kernel
arguments. Memory optimizations will be done later.

This series is based on top of https://patchwork.linuxtv.org/cover/59653/

Best regards,
Jernej

Jernej Skrabec (3):
  media: cedrus: Properly signal size in mode register
  media: cedrus: Fix H264 4k support
  media: cedrus: Increase maximum supported size

 drivers/staging/media/sunxi/cedrus/cedrus.h   |  7 ++
 .../staging/media/sunxi/cedrus/cedrus_h264.c  | 85 +++++++++++++++++--
 .../staging/media/sunxi/cedrus/cedrus_h265.c  |  2 +-
 .../staging/media/sunxi/cedrus/cedrus_hw.c    |  9 +-
 .../staging/media/sunxi/cedrus/cedrus_hw.h    |  2 +-
 .../staging/media/sunxi/cedrus/cedrus_mpeg2.c |  2 +-
 .../staging/media/sunxi/cedrus/cedrus_regs.h  | 13 +++
 .../staging/media/sunxi/cedrus/cedrus_video.c |  4 +-
 8 files changed, 108 insertions(+), 16 deletions(-)

-- 
2.23.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] media: cedrus: Properly signal size in mode register
  2019-10-26  7:49 [PATCH 0/3] media: cedrus: Add support for 4k videos Jernej Skrabec
@ 2019-10-26  7:49 ` Jernej Skrabec
  2019-11-04 10:02   ` Paul Kocialkowski
  2019-10-26  7:49 ` [PATCH 2/3] media: cedrus: Fix H264 4k support Jernej Skrabec
  2019-10-26  7:49 ` [PATCH 3/3] media: cedrus: Increase maximum supported size Jernej Skrabec
  2 siblings, 1 reply; 13+ messages in thread
From: Jernej Skrabec @ 2019-10-26  7:49 UTC (permalink / raw)
  To: mripard, paul.kocialkowski
  Cc: mchehab, hverkuil-cisco, gregkh, wens, linux-media, devel,
	linux-arm-kernel, linux-kernel, linux-sunxi

Mode register also holds information if video width is bigger than 2048
and if it is equal to 4096.

Rework cedrus_engine_enable() to properly signal this properties.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/staging/media/sunxi/cedrus/cedrus_h264.c  | 2 +-
 drivers/staging/media/sunxi/cedrus/cedrus_h265.c  | 2 +-
 drivers/staging/media/sunxi/cedrus/cedrus_hw.c    | 9 +++++++--
 drivers/staging/media/sunxi/cedrus/cedrus_hw.h    | 2 +-
 drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c | 2 +-
 drivers/staging/media/sunxi/cedrus/cedrus_regs.h  | 2 ++
 6 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
index 7487f6ab7576..d2c854ecdf15 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
@@ -485,7 +485,7 @@ static void cedrus_h264_setup(struct cedrus_ctx *ctx,
 {
 	struct cedrus_dev *dev = ctx->dev;
 
-	cedrus_engine_enable(dev, CEDRUS_CODEC_H264);
+	cedrus_engine_enable(ctx, CEDRUS_CODEC_H264);
 
 	cedrus_write(dev, VE_H264_SDROT_CTRL, 0);
 	cedrus_write(dev, VE_H264_EXTRA_BUFFER1,
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
index 9bc921866f70..6945dc74e1d7 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
@@ -276,7 +276,7 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
 	}
 
 	/* Activate H265 engine. */
-	cedrus_engine_enable(dev, CEDRUS_CODEC_H265);
+	cedrus_engine_enable(ctx, CEDRUS_CODEC_H265);
 
 	/* Source offset and length in bits. */
 
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
index 570a9165dd5d..3acfa21bc124 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
@@ -30,7 +30,7 @@
 #include "cedrus_hw.h"
 #include "cedrus_regs.h"
 
-int cedrus_engine_enable(struct cedrus_dev *dev, enum cedrus_codec codec)
+int cedrus_engine_enable(struct cedrus_ctx *ctx, enum cedrus_codec codec)
 {
 	u32 reg = 0;
 
@@ -58,7 +58,12 @@ int cedrus_engine_enable(struct cedrus_dev *dev, enum cedrus_codec codec)
 		return -EINVAL;
 	}
 
-	cedrus_write(dev, VE_MODE, reg);
+	if (ctx->src_fmt.width == 4096)
+		reg |= VE_MODE_PIC_WIDTH_IS_4096;
+	if (ctx->src_fmt.width > 2048)
+		reg |= VE_MODE_PIC_WIDTH_MORE_2048;
+
+	cedrus_write(ctx->dev, VE_MODE, reg);
 
 	return 0;
 }
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
index 27d0882397aa..604ff932fbf5 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
@@ -16,7 +16,7 @@
 #ifndef _CEDRUS_HW_H_
 #define _CEDRUS_HW_H_
 
-int cedrus_engine_enable(struct cedrus_dev *dev, enum cedrus_codec codec);
+int cedrus_engine_enable(struct cedrus_ctx *ctx, enum cedrus_codec codec);
 void cedrus_engine_disable(struct cedrus_dev *dev);
 
 void cedrus_dst_format_set(struct cedrus_dev *dev,
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
index 13c34927bad5..8bcd6b8f9e2d 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
@@ -96,7 +96,7 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
 	quantization = run->mpeg2.quantization;
 
 	/* Activate MPEG engine. */
-	cedrus_engine_enable(dev, CEDRUS_CODEC_MPEG2);
+	cedrus_engine_enable(ctx, CEDRUS_CODEC_MPEG2);
 
 	/* Set intra quantization matrix. */
 
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
index 4275a307d282..ace3d49fcd82 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
@@ -35,6 +35,8 @@
 
 #define VE_MODE					0x00
 
+#define VE_MODE_PIC_WIDTH_IS_4096		BIT(22)
+#define VE_MODE_PIC_WIDTH_MORE_2048		BIT(21)
 #define VE_MODE_REC_WR_MODE_2MB			(0x01 << 20)
 #define VE_MODE_REC_WR_MODE_1MB			(0x00 << 20)
 #define VE_MODE_DDR_MODE_BW_128			(0x03 << 16)
-- 
2.23.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 2/3] media: cedrus: Fix H264 4k support
  2019-10-26  7:49 [PATCH 0/3] media: cedrus: Add support for 4k videos Jernej Skrabec
  2019-10-26  7:49 ` [PATCH 1/3] media: cedrus: Properly signal size in mode register Jernej Skrabec
@ 2019-10-26  7:49 ` Jernej Skrabec
  2019-11-04 10:13   ` Paul Kocialkowski
  2019-10-26  7:49 ` [PATCH 3/3] media: cedrus: Increase maximum supported size Jernej Skrabec
  2 siblings, 1 reply; 13+ messages in thread
From: Jernej Skrabec @ 2019-10-26  7:49 UTC (permalink / raw)
  To: mripard, paul.kocialkowski
  Cc: mchehab, hverkuil-cisco, gregkh, wens, linux-media, devel,
	linux-arm-kernel, linux-kernel, linux-sunxi

H264 decoder needs additional or bigger buffers in order to decode 4k
videos.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/staging/media/sunxi/cedrus/cedrus.h   |  7 ++
 .../staging/media/sunxi/cedrus/cedrus_h264.c  | 83 +++++++++++++++++--
 .../staging/media/sunxi/cedrus/cedrus_regs.h  | 11 +++
 3 files changed, 93 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
index c45fb9a7ad07..96765555ab8a 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
@@ -116,8 +116,15 @@ struct cedrus_ctx {
 			ssize_t		mv_col_buf_size;
 			void		*pic_info_buf;
 			dma_addr_t	pic_info_buf_dma;
+			ssize_t		pic_info_buf_size;
 			void		*neighbor_info_buf;
 			dma_addr_t	neighbor_info_buf_dma;
+			void		*deblk_buf;
+			dma_addr_t	deblk_buf_dma;
+			ssize_t		deblk_buf_size;
+			void		*intra_pred_buf;
+			dma_addr_t	intra_pred_buf_dma;
+			ssize_t		intra_pred_buf_size;
 		} h264;
 		struct {
 			void		*mv_col_buf;
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
index d2c854ecdf15..19962f4213d4 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
@@ -39,7 +39,6 @@ struct cedrus_h264_sram_ref_pic {
 #define CEDRUS_H264_FRAME_NUM		18
 
 #define CEDRUS_NEIGHBOR_INFO_BUF_SIZE	(16 * SZ_1K)
-#define CEDRUS_PIC_INFO_BUF_SIZE	(128 * SZ_1K)
 
 static void cedrus_h264_write_sram(struct cedrus_dev *dev,
 				   enum cedrus_h264_sram_off off,
@@ -342,6 +341,20 @@ static void cedrus_set_params(struct cedrus_ctx *ctx,
 		     VE_H264_VLD_ADDR_FIRST | VE_H264_VLD_ADDR_VALID |
 		     VE_H264_VLD_ADDR_LAST);
 
+	if (ctx->src_fmt.width > 2048) {
+		cedrus_write(dev, VE_BUF_CTRL,
+			     VE_BUF_CTRL_INTRAPRED_MIXED_RAM |
+			     VE_BUF_CTRL_DBLK_MIXED_RAM);
+		cedrus_write(dev, VE_DBLK_DRAM_BUF_ADDR,
+			     ctx->codec.h264.deblk_buf_dma);
+		cedrus_write(dev, VE_INTRAPRED_DRAM_BUF_ADDR,
+			     ctx->codec.h264.intra_pred_buf_dma);
+	} else {
+		cedrus_write(dev, VE_BUF_CTRL,
+			     VE_BUF_CTRL_INTRAPRED_INT_SRAM |
+			     VE_BUF_CTRL_DBLK_INT_SRAM);
+	}
+
 	/*
 	 * FIXME: Since the bitstream parsing is done in software, and
 	 * in userspace, this shouldn't be needed anymore. But it
@@ -502,18 +515,28 @@ static void cedrus_h264_setup(struct cedrus_ctx *ctx,
 static int cedrus_h264_start(struct cedrus_ctx *ctx)
 {
 	struct cedrus_dev *dev = ctx->dev;
+	unsigned int pic_info_size;
 	unsigned int field_size;
 	unsigned int mv_col_size;
 	int ret;
 
+	if (ctx->src_fmt.width > 2048)
+		pic_info_size = CEDRUS_H264_FRAME_NUM * 0x4000;
+	else
+		pic_info_size = CEDRUS_H264_FRAME_NUM * 0x1000;
+
 	/*
-	 * FIXME: It seems that the H6 cedarX code is using a formula
-	 * here based on the size of the frame, while all the older
-	 * code is using a fixed size, so that might need to be
-	 * changed at some point.
+	 * FIXME: If V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY is set,
+	 * there is no need to multiply by 2.
 	 */
+	pic_info_size += ctx->src_fmt.height * 2 * 64;
+
+	if (pic_info_size < 130 * SZ_1K)
+		pic_info_size = 130 * SZ_1K;
+
+	ctx->codec.h264.pic_info_buf_size = pic_info_size;
 	ctx->codec.h264.pic_info_buf =
-		dma_alloc_coherent(dev->dev, CEDRUS_PIC_INFO_BUF_SIZE,
+		dma_alloc_coherent(dev->dev, ctx->codec.h264.pic_info_buf_size,
 				   &ctx->codec.h264.pic_info_buf_dma,
 				   GFP_KERNEL);
 	if (!ctx->codec.h264.pic_info_buf)
@@ -566,15 +589,51 @@ static int cedrus_h264_start(struct cedrus_ctx *ctx)
 		goto err_neighbor_buf;
 	}
 
+	if (ctx->src_fmt.width > 2048) {
+		ctx->codec.h264.deblk_buf_size =
+			ALIGN(ctx->src_fmt.width, 32) * 12;
+		ctx->codec.h264.deblk_buf =
+			dma_alloc_coherent(dev->dev,
+					   ctx->codec.h264.deblk_buf_size,
+					   &ctx->codec.h264.deblk_buf_dma,
+					   GFP_KERNEL);
+		if (!ctx->codec.h264.deblk_buf) {
+			ret = -ENOMEM;
+			goto err_mv_col_buf;
+		}
+
+		ctx->codec.h264.intra_pred_buf_size =
+			ALIGN(ctx->src_fmt.width, 64) * 5;
+		ctx->codec.h264.intra_pred_buf =
+			dma_alloc_coherent(dev->dev,
+					   ctx->codec.h264.intra_pred_buf_size,
+					   &ctx->codec.h264.intra_pred_buf_dma,
+					   GFP_KERNEL);
+		if (!ctx->codec.h264.intra_pred_buf) {
+			ret = -ENOMEM;
+			goto err_deblk_buf;
+		}
+	}
+
 	return 0;
 
+err_deblk_buf:
+	dma_free_coherent(dev->dev, ctx->codec.h264.deblk_buf_size,
+			  ctx->codec.h264.deblk_buf,
+			  ctx->codec.h264.deblk_buf_dma);
+
+err_mv_col_buf:
+	dma_free_coherent(dev->dev, ctx->codec.h264.mv_col_buf_size,
+			  ctx->codec.h264.mv_col_buf,
+			  ctx->codec.h264.mv_col_buf_dma);
+
 err_neighbor_buf:
 	dma_free_coherent(dev->dev, CEDRUS_NEIGHBOR_INFO_BUF_SIZE,
 			  ctx->codec.h264.neighbor_info_buf,
 			  ctx->codec.h264.neighbor_info_buf_dma);
 
 err_pic_buf:
-	dma_free_coherent(dev->dev, CEDRUS_PIC_INFO_BUF_SIZE,
+	dma_free_coherent(dev->dev, ctx->codec.h264.pic_info_buf_size,
 			  ctx->codec.h264.pic_info_buf,
 			  ctx->codec.h264.pic_info_buf_dma);
 	return ret;
@@ -590,9 +649,17 @@ static void cedrus_h264_stop(struct cedrus_ctx *ctx)
 	dma_free_coherent(dev->dev, CEDRUS_NEIGHBOR_INFO_BUF_SIZE,
 			  ctx->codec.h264.neighbor_info_buf,
 			  ctx->codec.h264.neighbor_info_buf_dma);
-	dma_free_coherent(dev->dev, CEDRUS_PIC_INFO_BUF_SIZE,
+	dma_free_coherent(dev->dev, ctx->codec.h264.pic_info_buf_size,
 			  ctx->codec.h264.pic_info_buf,
 			  ctx->codec.h264.pic_info_buf_dma);
+	if (ctx->codec.h264.deblk_buf_size)
+		dma_free_coherent(dev->dev, ctx->codec.h264.deblk_buf_size,
+				  ctx->codec.h264.deblk_buf,
+				  ctx->codec.h264.deblk_buf_dma);
+	if (ctx->codec.h264.intra_pred_buf_size)
+		dma_free_coherent(dev->dev, ctx->codec.h264.intra_pred_buf_size,
+				  ctx->codec.h264.intra_pred_buf,
+				  ctx->codec.h264.intra_pred_buf_dma);
 }
 
 static void cedrus_h264_trigger(struct cedrus_ctx *ctx)
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
index ace3d49fcd82..7beb03d3bb39 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
@@ -46,6 +46,17 @@
 #define VE_MODE_DEC_H264			(0x01 << 0)
 #define VE_MODE_DEC_MPEG			(0x00 << 0)
 
+#define VE_BUF_CTRL				0x50
+
+#define VE_BUF_CTRL_INTRAPRED_EXT_RAM		(0x02 << 2)
+#define VE_BUF_CTRL_INTRAPRED_MIXED_RAM		(0x01 << 2)
+#define VE_BUF_CTRL_INTRAPRED_INT_SRAM		(0x00 << 2)
+#define VE_BUF_CTRL_DBLK_EXT_RAM		(0x02 << 0)
+#define VE_BUF_CTRL_DBLK_MIXED_RAM		(0x01 << 0)
+#define VE_BUF_CTRL_DBLK_INT_SRAM		(0x00 << 0)
+
+#define VE_DBLK_DRAM_BUF_ADDR			0x54
+#define VE_INTRAPRED_DRAM_BUF_ADDR		0x58
 #define VE_PRIMARY_CHROMA_BUF_LEN		0xc4
 #define VE_PRIMARY_FB_LINE_STRIDE		0xc8
 
-- 
2.23.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 3/3] media: cedrus: Increase maximum supported size
  2019-10-26  7:49 [PATCH 0/3] media: cedrus: Add support for 4k videos Jernej Skrabec
  2019-10-26  7:49 ` [PATCH 1/3] media: cedrus: Properly signal size in mode register Jernej Skrabec
  2019-10-26  7:49 ` [PATCH 2/3] media: cedrus: Fix H264 4k support Jernej Skrabec
@ 2019-10-26  7:49 ` Jernej Skrabec
  2019-11-04 10:14   ` Paul Kocialkowski
  2 siblings, 1 reply; 13+ messages in thread
From: Jernej Skrabec @ 2019-10-26  7:49 UTC (permalink / raw)
  To: mripard, paul.kocialkowski
  Cc: mchehab, hverkuil-cisco, gregkh, wens, linux-media, devel,
	linux-arm-kernel, linux-kernel, linux-sunxi

There are few variations of 4k resolutions. The biggest one is
4096x2304 which is also supported by HW. It has also nice property that
both width and size are divisible by maximum HEVC CTB size, which is 64.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/staging/media/sunxi/cedrus/cedrus_video.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
index cc15a5cf107d..15cf1f10221b 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
@@ -29,8 +29,8 @@
 
 #define CEDRUS_MIN_WIDTH	16U
 #define CEDRUS_MIN_HEIGHT	16U
-#define CEDRUS_MAX_WIDTH	3840U
-#define CEDRUS_MAX_HEIGHT	2160U
+#define CEDRUS_MAX_WIDTH	4096U
+#define CEDRUS_MAX_HEIGHT	2304U
 
 static struct cedrus_format cedrus_formats[] = {
 	{
-- 
2.23.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] media: cedrus: Properly signal size in mode register
  2019-10-26  7:49 ` [PATCH 1/3] media: cedrus: Properly signal size in mode register Jernej Skrabec
@ 2019-11-04 10:02   ` Paul Kocialkowski
  2019-11-04 16:33     ` Jernej Škrabec
  0 siblings, 1 reply; 13+ messages in thread
From: Paul Kocialkowski @ 2019-11-04 10:02 UTC (permalink / raw)
  To: Jernej Skrabec
  Cc: mripard, mchehab, hverkuil-cisco, gregkh, wens, linux-media,
	devel, linux-arm-kernel, linux-kernel, linux-sunxi

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

Hi Jernej,

On Sat 26 Oct 19, 09:49, Jernej Skrabec wrote:
> Mode register also holds information if video width is bigger than 2048
> and if it is equal to 4096.
> 
> Rework cedrus_engine_enable() to properly signal this properties.

Thanks for the patch, looks good to me!

Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

One minor thing: maybe we should have a way to set the maximum dimensions
depending on the generation of the engine in use and the actual maximum
supported by the hardware.

Maybe either as dedicated new fields in struct cedrus_variant or as
capability flags.

Anyway that can be done later since we were already hardcoding this.

Cheers,

Paul

> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  drivers/staging/media/sunxi/cedrus/cedrus_h264.c  | 2 +-
>  drivers/staging/media/sunxi/cedrus/cedrus_h265.c  | 2 +-
>  drivers/staging/media/sunxi/cedrus/cedrus_hw.c    | 9 +++++++--
>  drivers/staging/media/sunxi/cedrus/cedrus_hw.h    | 2 +-
>  drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c | 2 +-
>  drivers/staging/media/sunxi/cedrus/cedrus_regs.h  | 2 ++
>  6 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> index 7487f6ab7576..d2c854ecdf15 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> @@ -485,7 +485,7 @@ static void cedrus_h264_setup(struct cedrus_ctx *ctx,
>  {
>  	struct cedrus_dev *dev = ctx->dev;
>  
> -	cedrus_engine_enable(dev, CEDRUS_CODEC_H264);
> +	cedrus_engine_enable(ctx, CEDRUS_CODEC_H264);
>  
>  	cedrus_write(dev, VE_H264_SDROT_CTRL, 0);
>  	cedrus_write(dev, VE_H264_EXTRA_BUFFER1,
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> index 9bc921866f70..6945dc74e1d7 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> @@ -276,7 +276,7 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
>  	}
>  
>  	/* Activate H265 engine. */
> -	cedrus_engine_enable(dev, CEDRUS_CODEC_H265);
> +	cedrus_engine_enable(ctx, CEDRUS_CODEC_H265);
>  
>  	/* Source offset and length in bits. */
>  
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> index 570a9165dd5d..3acfa21bc124 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> @@ -30,7 +30,7 @@
>  #include "cedrus_hw.h"
>  #include "cedrus_regs.h"
>  
> -int cedrus_engine_enable(struct cedrus_dev *dev, enum cedrus_codec codec)
> +int cedrus_engine_enable(struct cedrus_ctx *ctx, enum cedrus_codec codec)
>  {
>  	u32 reg = 0;
>  
> @@ -58,7 +58,12 @@ int cedrus_engine_enable(struct cedrus_dev *dev, enum cedrus_codec codec)
>  		return -EINVAL;
>  	}
>  
> -	cedrus_write(dev, VE_MODE, reg);
> +	if (ctx->src_fmt.width == 4096)
> +		reg |= VE_MODE_PIC_WIDTH_IS_4096;
> +	if (ctx->src_fmt.width > 2048)
> +		reg |= VE_MODE_PIC_WIDTH_MORE_2048;
> +
> +	cedrus_write(ctx->dev, VE_MODE, reg);
>  
>  	return 0;
>  }
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> index 27d0882397aa..604ff932fbf5 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> @@ -16,7 +16,7 @@
>  #ifndef _CEDRUS_HW_H_
>  #define _CEDRUS_HW_H_
>  
> -int cedrus_engine_enable(struct cedrus_dev *dev, enum cedrus_codec codec);
> +int cedrus_engine_enable(struct cedrus_ctx *ctx, enum cedrus_codec codec);
>  void cedrus_engine_disable(struct cedrus_dev *dev);
>  
>  void cedrus_dst_format_set(struct cedrus_dev *dev,
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> index 13c34927bad5..8bcd6b8f9e2d 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> @@ -96,7 +96,7 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
>  	quantization = run->mpeg2.quantization;
>  
>  	/* Activate MPEG engine. */
> -	cedrus_engine_enable(dev, CEDRUS_CODEC_MPEG2);
> +	cedrus_engine_enable(ctx, CEDRUS_CODEC_MPEG2);
>  
>  	/* Set intra quantization matrix. */
>  
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> index 4275a307d282..ace3d49fcd82 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> @@ -35,6 +35,8 @@
>  
>  #define VE_MODE					0x00
>  
> +#define VE_MODE_PIC_WIDTH_IS_4096		BIT(22)
> +#define VE_MODE_PIC_WIDTH_MORE_2048		BIT(21)
>  #define VE_MODE_REC_WR_MODE_2MB			(0x01 << 20)
>  #define VE_MODE_REC_WR_MODE_1MB			(0x00 << 20)
>  #define VE_MODE_DDR_MODE_BW_128			(0x03 << 16)
> -- 
> 2.23.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] media: cedrus: Fix H264 4k support
  2019-10-26  7:49 ` [PATCH 2/3] media: cedrus: Fix H264 4k support Jernej Skrabec
@ 2019-11-04 10:13   ` Paul Kocialkowski
  2019-11-04 16:53     ` Jernej Škrabec
  0 siblings, 1 reply; 13+ messages in thread
From: Paul Kocialkowski @ 2019-11-04 10:13 UTC (permalink / raw)
  To: Jernej Skrabec
  Cc: mripard, mchehab, hverkuil-cisco, gregkh, wens, linux-media,
	devel, linux-arm-kernel, linux-kernel, linux-sunxi

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

Hi,

On Sat 26 Oct 19, 09:49, Jernej Skrabec wrote:
> H264 decoder needs additional or bigger buffers in order to decode 4k
> videos.

Thanks for the fixup, we hadn't looked into those bits at all during initial
bringup of H.264!

See a few minor comments below.

> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  drivers/staging/media/sunxi/cedrus/cedrus.h   |  7 ++
>  .../staging/media/sunxi/cedrus/cedrus_h264.c  | 83 +++++++++++++++++--
>  .../staging/media/sunxi/cedrus/cedrus_regs.h  | 11 +++
>  3 files changed, 93 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
> index c45fb9a7ad07..96765555ab8a 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> @@ -116,8 +116,15 @@ struct cedrus_ctx {
>  			ssize_t		mv_col_buf_size;
>  			void		*pic_info_buf;
>  			dma_addr_t	pic_info_buf_dma;
> +			ssize_t		pic_info_buf_size;
>  			void		*neighbor_info_buf;
>  			dma_addr_t	neighbor_info_buf_dma;
> +			void		*deblk_buf;
> +			dma_addr_t	deblk_buf_dma;
> +			ssize_t		deblk_buf_size;
> +			void		*intra_pred_buf;
> +			dma_addr_t	intra_pred_buf_dma;
> +			ssize_t		intra_pred_buf_size;
>  		} h264;
>  		struct {
>  			void		*mv_col_buf;
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> index d2c854ecdf15..19962f4213d4 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> @@ -39,7 +39,6 @@ struct cedrus_h264_sram_ref_pic {
>  #define CEDRUS_H264_FRAME_NUM		18
>  
>  #define CEDRUS_NEIGHBOR_INFO_BUF_SIZE	(16 * SZ_1K)
> -#define CEDRUS_PIC_INFO_BUF_SIZE	(128 * SZ_1K)

Could we keep a define with the minimum size that you are using later
(increased to 130 * SZ_1K)?

>  static void cedrus_h264_write_sram(struct cedrus_dev *dev,
>  				   enum cedrus_h264_sram_off off,
> @@ -342,6 +341,20 @@ static void cedrus_set_params(struct cedrus_ctx *ctx,
>  		     VE_H264_VLD_ADDR_FIRST | VE_H264_VLD_ADDR_VALID |
>  		     VE_H264_VLD_ADDR_LAST);
>  
> +	if (ctx->src_fmt.width > 2048) {
> +		cedrus_write(dev, VE_BUF_CTRL,
> +			     VE_BUF_CTRL_INTRAPRED_MIXED_RAM |
> +			     VE_BUF_CTRL_DBLK_MIXED_RAM);
> +		cedrus_write(dev, VE_DBLK_DRAM_BUF_ADDR,
> +			     ctx->codec.h264.deblk_buf_dma);
> +		cedrus_write(dev, VE_INTRAPRED_DRAM_BUF_ADDR,
> +			     ctx->codec.h264.intra_pred_buf_dma);
> +	} else {
> +		cedrus_write(dev, VE_BUF_CTRL,
> +			     VE_BUF_CTRL_INTRAPRED_INT_SRAM |
> +			     VE_BUF_CTRL_DBLK_INT_SRAM);
> +	}
> +
>  	/*
>  	 * FIXME: Since the bitstream parsing is done in software, and
>  	 * in userspace, this shouldn't be needed anymore. But it
> @@ -502,18 +515,28 @@ static void cedrus_h264_setup(struct cedrus_ctx *ctx,
>  static int cedrus_h264_start(struct cedrus_ctx *ctx)
>  {
>  	struct cedrus_dev *dev = ctx->dev;
> +	unsigned int pic_info_size;
>  	unsigned int field_size;
>  	unsigned int mv_col_size;
>  	int ret;
>  

Maybe add a comment here this is a half-magic sub-optimal formula?

> +	if (ctx->src_fmt.width > 2048)
> +		pic_info_size = CEDRUS_H264_FRAME_NUM * 0x4000;
> +	else
> +		pic_info_size = CEDRUS_H264_FRAME_NUM * 0x1000;
> +
>  	/*
> -	 * FIXME: It seems that the H6 cedarX code is using a formula
> -	 * here based on the size of the frame, while all the older
> -	 * code is using a fixed size, so that might need to be
> -	 * changed at some point.
> +	 * FIXME: If V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY is set,
> +	 * there is no need to multiply by 2.
>  	 */
> +	pic_info_size += ctx->src_fmt.height * 2 * 64;
> +
> +	if (pic_info_size < 130 * SZ_1K)
> +		pic_info_size = 130 * SZ_1K;

This is where I think we could have a "minimum pic info size" define.

> +
> +	ctx->codec.h264.pic_info_buf_size = pic_info_size;
>  	ctx->codec.h264.pic_info_buf =
> -		dma_alloc_coherent(dev->dev, CEDRUS_PIC_INFO_BUF_SIZE,
> +		dma_alloc_coherent(dev->dev, ctx->codec.h264.pic_info_buf_size,
>  				   &ctx->codec.h264.pic_info_buf_dma,
>  				   GFP_KERNEL);
>  	if (!ctx->codec.h264.pic_info_buf)
> @@ -566,15 +589,51 @@ static int cedrus_h264_start(struct cedrus_ctx *ctx)
>  		goto err_neighbor_buf;
>  	}
>  
> +	if (ctx->src_fmt.width > 2048) {

Feel free to add a comment here to explain where the 12 below comes from if you
have some idea, or that it's a somewhat magical value that generally works.

> +		ctx->codec.h264.deblk_buf_size =
> +			ALIGN(ctx->src_fmt.width, 32) * 12;
> +		ctx->codec.h264.deblk_buf =
> +			dma_alloc_coherent(dev->dev,
> +					   ctx->codec.h264.deblk_buf_size,
> +					   &ctx->codec.h264.deblk_buf_dma,
> +					   GFP_KERNEL);
> +		if (!ctx->codec.h264.deblk_buf) {
> +			ret = -ENOMEM;
> +			goto err_mv_col_buf;
> +		}
> +

Same here, a comment would be welcome about the 5 value below.

Cheers,

Paul

> +		ctx->codec.h264.intra_pred_buf_size =
> +			ALIGN(ctx->src_fmt.width, 64) * 5;
> +		ctx->codec.h264.intra_pred_buf =
> +			dma_alloc_coherent(dev->dev,
> +					   ctx->codec.h264.intra_pred_buf_size,
> +					   &ctx->codec.h264.intra_pred_buf_dma,
> +					   GFP_KERNEL);
> +		if (!ctx->codec.h264.intra_pred_buf) {
> +			ret = -ENOMEM;
> +			goto err_deblk_buf;
> +		}
> +	}
> +
>  	return 0;
>  
> +err_deblk_buf:
> +	dma_free_coherent(dev->dev, ctx->codec.h264.deblk_buf_size,
> +			  ctx->codec.h264.deblk_buf,
> +			  ctx->codec.h264.deblk_buf_dma);
> +
> +err_mv_col_buf:
> +	dma_free_coherent(dev->dev, ctx->codec.h264.mv_col_buf_size,
> +			  ctx->codec.h264.mv_col_buf,
> +			  ctx->codec.h264.mv_col_buf_dma);
> +
>  err_neighbor_buf:
>  	dma_free_coherent(dev->dev, CEDRUS_NEIGHBOR_INFO_BUF_SIZE,
>  			  ctx->codec.h264.neighbor_info_buf,
>  			  ctx->codec.h264.neighbor_info_buf_dma);
>  
>  err_pic_buf:
> -	dma_free_coherent(dev->dev, CEDRUS_PIC_INFO_BUF_SIZE,
> +	dma_free_coherent(dev->dev, ctx->codec.h264.pic_info_buf_size,
>  			  ctx->codec.h264.pic_info_buf,
>  			  ctx->codec.h264.pic_info_buf_dma);
>  	return ret;
> @@ -590,9 +649,17 @@ static void cedrus_h264_stop(struct cedrus_ctx *ctx)
>  	dma_free_coherent(dev->dev, CEDRUS_NEIGHBOR_INFO_BUF_SIZE,
>  			  ctx->codec.h264.neighbor_info_buf,
>  			  ctx->codec.h264.neighbor_info_buf_dma);
> -	dma_free_coherent(dev->dev, CEDRUS_PIC_INFO_BUF_SIZE,
> +	dma_free_coherent(dev->dev, ctx->codec.h264.pic_info_buf_size,
>  			  ctx->codec.h264.pic_info_buf,
>  			  ctx->codec.h264.pic_info_buf_dma);
> +	if (ctx->codec.h264.deblk_buf_size)
> +		dma_free_coherent(dev->dev, ctx->codec.h264.deblk_buf_size,
> +				  ctx->codec.h264.deblk_buf,
> +				  ctx->codec.h264.deblk_buf_dma);
> +	if (ctx->codec.h264.intra_pred_buf_size)
> +		dma_free_coherent(dev->dev, ctx->codec.h264.intra_pred_buf_size,
> +				  ctx->codec.h264.intra_pred_buf,
> +				  ctx->codec.h264.intra_pred_buf_dma);
>  }
>  
>  static void cedrus_h264_trigger(struct cedrus_ctx *ctx)
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> index ace3d49fcd82..7beb03d3bb39 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> @@ -46,6 +46,17 @@
>  #define VE_MODE_DEC_H264			(0x01 << 0)
>  #define VE_MODE_DEC_MPEG			(0x00 << 0)
>  
> +#define VE_BUF_CTRL				0x50
> +
> +#define VE_BUF_CTRL_INTRAPRED_EXT_RAM		(0x02 << 2)
> +#define VE_BUF_CTRL_INTRAPRED_MIXED_RAM		(0x01 << 2)
> +#define VE_BUF_CTRL_INTRAPRED_INT_SRAM		(0x00 << 2)
> +#define VE_BUF_CTRL_DBLK_EXT_RAM		(0x02 << 0)
> +#define VE_BUF_CTRL_DBLK_MIXED_RAM		(0x01 << 0)
> +#define VE_BUF_CTRL_DBLK_INT_SRAM		(0x00 << 0)
> +
> +#define VE_DBLK_DRAM_BUF_ADDR			0x54
> +#define VE_INTRAPRED_DRAM_BUF_ADDR		0x58
>  #define VE_PRIMARY_CHROMA_BUF_LEN		0xc4
>  #define VE_PRIMARY_FB_LINE_STRIDE		0xc8
>  
> -- 
> 2.23.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] media: cedrus: Increase maximum supported size
  2019-10-26  7:49 ` [PATCH 3/3] media: cedrus: Increase maximum supported size Jernej Skrabec
@ 2019-11-04 10:14   ` Paul Kocialkowski
  0 siblings, 0 replies; 13+ messages in thread
From: Paul Kocialkowski @ 2019-11-04 10:14 UTC (permalink / raw)
  To: Jernej Skrabec
  Cc: mripard, mchehab, hverkuil-cisco, gregkh, wens, linux-media,
	devel, linux-arm-kernel, linux-kernel, linux-sunxi

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

Hi,

On Sat 26 Oct 19, 09:49, Jernej Skrabec wrote:
> There are few variations of 4k resolutions. The biggest one is
> 4096x2304 which is also supported by HW. It has also nice property that
> both width and size are divisible by maximum HEVC CTB size, which is 64.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

As I said on the other email, it would be nice to eventually reflect the actual
per-platform maximum dimensions instead of hardcoding the maximum for all
platforms confounded.

Cheers,

Paul

> ---
>  drivers/staging/media/sunxi/cedrus/cedrus_video.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> index cc15a5cf107d..15cf1f10221b 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
> @@ -29,8 +29,8 @@
>  
>  #define CEDRUS_MIN_WIDTH	16U
>  #define CEDRUS_MIN_HEIGHT	16U
> -#define CEDRUS_MAX_WIDTH	3840U
> -#define CEDRUS_MAX_HEIGHT	2160U
> +#define CEDRUS_MAX_WIDTH	4096U
> +#define CEDRUS_MAX_HEIGHT	2304U
>  
>  static struct cedrus_format cedrus_formats[] = {
>  	{
> -- 
> 2.23.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] media: cedrus: Properly signal size in mode register
  2019-11-04 10:02   ` Paul Kocialkowski
@ 2019-11-04 16:33     ` Jernej Škrabec
  2019-11-05  8:10       ` Paul Kocialkowski
  0 siblings, 1 reply; 13+ messages in thread
From: Jernej Škrabec @ 2019-11-04 16:33 UTC (permalink / raw)
  To: Paul Kocialkowski
  Cc: mripard, mchehab, hverkuil-cisco, gregkh, wens, linux-media,
	devel, linux-arm-kernel, linux-kernel, linux-sunxi

Dne ponedeljek, 04. november 2019 ob 11:02:28 CET je Paul Kocialkowski 
napisal(a):
> Hi Jernej,
> 
> On Sat 26 Oct 19, 09:49, Jernej Skrabec wrote:
> > Mode register also holds information if video width is bigger than 2048
> > and if it is equal to 4096.
> > 
> > Rework cedrus_engine_enable() to properly signal this properties.
> 
> Thanks for the patch, looks good to me!
> 
> Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> 
> One minor thing: maybe we should have a way to set the maximum dimensions
> depending on the generation of the engine in use and the actual maximum
> supported by the hardware.
> 
> Maybe either as dedicated new fields in struct cedrus_variant or as
> capability flags.

I was thinking about first solution, but after going trough manuals, it was 
unclear what are real limitations. For example, H3 manual states that it is 
capable of decoding H264 1080p@60Hz. However, I know for a fact that it is 
also capable of decoding 4k videos, but probably not at 60 Hz. I don't own 
anything older that A83T, so I don't know what are capabilities of those SoCs. 
Anyway, being slow is still ok for some tasks, like transcoding, so we can't 
limit decoding to 1080p just because it's slow. It is probably still faster 
than doing it in SW. Not to mention that it's still ok for some videos, a lot 
of them uses 24 fps.

Best regards,
Jernej

> 
> Anyway that can be done later since we were already hardcoding this.
> 
> Cheers,
> 
> Paul
> 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> > 
> >  drivers/staging/media/sunxi/cedrus/cedrus_h264.c  | 2 +-
> >  drivers/staging/media/sunxi/cedrus/cedrus_h265.c  | 2 +-
> >  drivers/staging/media/sunxi/cedrus/cedrus_hw.c    | 9 +++++++--
> >  drivers/staging/media/sunxi/cedrus/cedrus_hw.h    | 2 +-
> >  drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c | 2 +-
> >  drivers/staging/media/sunxi/cedrus/cedrus_regs.h  | 2 ++
> >  6 files changed, 13 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index
> > 7487f6ab7576..d2c854ecdf15 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > @@ -485,7 +485,7 @@ static void cedrus_h264_setup(struct cedrus_ctx *ctx,
> > 
> >  {
> >  
> >  	struct cedrus_dev *dev = ctx->dev;
> > 
> > -	cedrus_engine_enable(dev, CEDRUS_CODEC_H264);
> > +	cedrus_engine_enable(ctx, CEDRUS_CODEC_H264);
> > 
> >  	cedrus_write(dev, VE_H264_SDROT_CTRL, 0);
> >  	cedrus_write(dev, VE_H264_EXTRA_BUFFER1,
> > 
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index
> > 9bc921866f70..6945dc74e1d7 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > @@ -276,7 +276,7 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
> > 
> >  	}
> >  	
> >  	/* Activate H265 engine. */
> > 
> > -	cedrus_engine_enable(dev, CEDRUS_CODEC_H265);
> > +	cedrus_engine_enable(ctx, CEDRUS_CODEC_H265);
> > 
> >  	/* Source offset and length in bits. */
> > 
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> > b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c index
> > 570a9165dd5d..3acfa21bc124 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> > @@ -30,7 +30,7 @@
> > 
> >  #include "cedrus_hw.h"
> >  #include "cedrus_regs.h"
> > 
> > -int cedrus_engine_enable(struct cedrus_dev *dev, enum cedrus_codec codec)
> > +int cedrus_engine_enable(struct cedrus_ctx *ctx, enum cedrus_codec codec)
> > 
> >  {
> >  
> >  	u32 reg = 0;
> > 
> > @@ -58,7 +58,12 @@ int cedrus_engine_enable(struct cedrus_dev *dev, enum
> > cedrus_codec codec)> 
> >  		return -EINVAL;
> >  	
> >  	}
> > 
> > -	cedrus_write(dev, VE_MODE, reg);
> > +	if (ctx->src_fmt.width == 4096)
> > +		reg |= VE_MODE_PIC_WIDTH_IS_4096;
> > +	if (ctx->src_fmt.width > 2048)
> > +		reg |= VE_MODE_PIC_WIDTH_MORE_2048;
> > +
> > +	cedrus_write(ctx->dev, VE_MODE, reg);
> > 
> >  	return 0;
> >  
> >  }
> > 
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> > b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h index
> > 27d0882397aa..604ff932fbf5 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> > @@ -16,7 +16,7 @@
> > 
> >  #ifndef _CEDRUS_HW_H_
> >  #define _CEDRUS_HW_H_
> > 
> > -int cedrus_engine_enable(struct cedrus_dev *dev, enum cedrus_codec
> > codec);
> > +int cedrus_engine_enable(struct cedrus_ctx *ctx, enum cedrus_codec
> > codec);
> > 
> >  void cedrus_engine_disable(struct cedrus_dev *dev);
> >  
> >  void cedrus_dst_format_set(struct cedrus_dev *dev,
> > 
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c index
> > 13c34927bad5..8bcd6b8f9e2d 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > @@ -96,7 +96,7 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx,
> > struct cedrus_run *run)> 
> >  	quantization = run->mpeg2.quantization;
> >  	
> >  	/* Activate MPEG engine. */
> > 
> > -	cedrus_engine_enable(dev, CEDRUS_CODEC_MPEG2);
> > +	cedrus_engine_enable(ctx, CEDRUS_CODEC_MPEG2);
> > 
> >  	/* Set intra quantization matrix. */
> > 
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> > b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h index
> > 4275a307d282..ace3d49fcd82 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> > @@ -35,6 +35,8 @@
> > 
> >  #define VE_MODE					0x00
> > 
> > +#define VE_MODE_PIC_WIDTH_IS_4096		BIT(22)
> > +#define VE_MODE_PIC_WIDTH_MORE_2048		BIT(21)
> > 
> >  #define VE_MODE_REC_WR_MODE_2MB			(0x01 << 20)
> >  #define VE_MODE_REC_WR_MODE_1MB			(0x00 << 20)
> >  #define VE_MODE_DDR_MODE_BW_128			(0x03 << 16)





^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] media: cedrus: Fix H264 4k support
  2019-11-04 10:13   ` Paul Kocialkowski
@ 2019-11-04 16:53     ` Jernej Škrabec
  2019-11-05  8:07       ` Paul Kocialkowski
  0 siblings, 1 reply; 13+ messages in thread
From: Jernej Škrabec @ 2019-11-04 16:53 UTC (permalink / raw)
  To: Paul Kocialkowski
  Cc: mripard, mchehab, hverkuil-cisco, gregkh, wens, linux-media,
	devel, linux-arm-kernel, linux-kernel, linux-sunxi

Dne ponedeljek, 04. november 2019 ob 11:13:19 CET je Paul Kocialkowski 
napisal(a):
> Hi,
> 
> On Sat 26 Oct 19, 09:49, Jernej Skrabec wrote:
> > H264 decoder needs additional or bigger buffers in order to decode 4k
> > videos.
> 
> Thanks for the fixup, we hadn't looked into those bits at all during initial
> bringup of H.264!
> 
> See a few minor comments below.
> 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> > 
> >  drivers/staging/media/sunxi/cedrus/cedrus.h   |  7 ++
> >  .../staging/media/sunxi/cedrus/cedrus_h264.c  | 83 +++++++++++++++++--
> >  .../staging/media/sunxi/cedrus/cedrus_regs.h  | 11 +++
> >  3 files changed, 93 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > b/drivers/staging/media/sunxi/cedrus/cedrus.h index
> > c45fb9a7ad07..96765555ab8a 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> > @@ -116,8 +116,15 @@ struct cedrus_ctx {
> > 
> >  			ssize_t		mv_col_buf_size;
> >  			void		*pic_info_buf;
> >  			dma_addr_t	pic_info_buf_dma;
> > 
> > +			ssize_t		pic_info_buf_size;
> > 
> >  			void		*neighbor_info_buf;
> >  			dma_addr_t	neighbor_info_buf_dma;
> > 
> > +			void		*deblk_buf;
> > +			dma_addr_t	deblk_buf_dma;
> > +			ssize_t		deblk_buf_size;
> > +			void		*intra_pred_buf;
> > +			dma_addr_t	intra_pred_buf_dma;
> > +			ssize_t		intra_pred_buf_size;
> > 
> >  		} h264;
> >  		struct {
> >  		
> >  			void		*mv_col_buf;
> > 
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index
> > d2c854ecdf15..19962f4213d4 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > @@ -39,7 +39,6 @@ struct cedrus_h264_sram_ref_pic {
> > 
> >  #define CEDRUS_H264_FRAME_NUM		18
> >  
> >  #define CEDRUS_NEIGHBOR_INFO_BUF_SIZE	(16 * SZ_1K)
> > 
> > -#define CEDRUS_PIC_INFO_BUF_SIZE	(128 * SZ_1K)
> 
> Could we keep a define with the minimum size that you are using later
> (increased to 130 * SZ_1K)?

Sure.

> 
> >  static void cedrus_h264_write_sram(struct cedrus_dev *dev,
> >  
> >  				   enum cedrus_h264_sram_off off,
> > 
> > @@ -342,6 +341,20 @@ static void cedrus_set_params(struct cedrus_ctx *ctx,
> > 
> >  		     VE_H264_VLD_ADDR_FIRST | VE_H264_VLD_ADDR_VALID |
> >  		     VE_H264_VLD_ADDR_LAST);
> > 
> > +	if (ctx->src_fmt.width > 2048) {
> > +		cedrus_write(dev, VE_BUF_CTRL,
> > +			     VE_BUF_CTRL_INTRAPRED_MIXED_RAM |
> > +			     VE_BUF_CTRL_DBLK_MIXED_RAM);
> > +		cedrus_write(dev, VE_DBLK_DRAM_BUF_ADDR,
> > +			     ctx->codec.h264.deblk_buf_dma);
> > +		cedrus_write(dev, VE_INTRAPRED_DRAM_BUF_ADDR,
> > +			     ctx->codec.h264.intra_pred_buf_dma);
> > +	} else {
> > +		cedrus_write(dev, VE_BUF_CTRL,
> > +			     VE_BUF_CTRL_INTRAPRED_INT_SRAM |
> > +			     VE_BUF_CTRL_DBLK_INT_SRAM);
> > +	}
> > +
> > 
> >  	/*
> >  	
> >  	 * FIXME: Since the bitstream parsing is done in software, and
> >  	 * in userspace, this shouldn't be needed anymore. But it
> > 
> > @@ -502,18 +515,28 @@ static void cedrus_h264_setup(struct cedrus_ctx
> > *ctx,
> > 
> >  static int cedrus_h264_start(struct cedrus_ctx *ctx)
> >  {
> >  
> >  	struct cedrus_dev *dev = ctx->dev;
> > 
> > +	unsigned int pic_info_size;
> > 
> >  	unsigned int field_size;
> >  	unsigned int mv_col_size;
> >  	int ret;
> 
> Maybe add a comment here this is a half-magic sub-optimal formula?

Well, I'm not sure how much suboptimal formulas this and those below are. They 
are taken from CedarX source. I would imagine that they didn't waste too much 
memory. What kind of comment would be ok for you? "Formula taken from CedarX 
source"?

Best regards,
Jernej

> 
> > +	if (ctx->src_fmt.width > 2048)
> > +		pic_info_size = CEDRUS_H264_FRAME_NUM * 0x4000;
> > +	else
> > +		pic_info_size = CEDRUS_H264_FRAME_NUM * 0x1000;
> > +
> > 
> >  	/*
> > 
> > -	 * FIXME: It seems that the H6 cedarX code is using a formula
> > -	 * here based on the size of the frame, while all the older
> > -	 * code is using a fixed size, so that might need to be
> > -	 * changed at some point.
> > +	 * FIXME: If V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY is set,
> > +	 * there is no need to multiply by 2.
> > 
> >  	 */
> > 
> > +	pic_info_size += ctx->src_fmt.height * 2 * 64;
> > +
> > +	if (pic_info_size < 130 * SZ_1K)
> > +		pic_info_size = 130 * SZ_1K;
> 
> This is where I think we could have a "minimum pic info size" define.
> 
> > +
> > +	ctx->codec.h264.pic_info_buf_size = pic_info_size;
> > 
> >  	ctx->codec.h264.pic_info_buf =
> > 
> > -		dma_alloc_coherent(dev->dev, CEDRUS_PIC_INFO_BUF_SIZE,
> > +		dma_alloc_coherent(dev->dev, ctx-
>codec.h264.pic_info_buf_size,
> > 
> >  				   &ctx-
>codec.h264.pic_info_buf_dma,
> >  				   GFP_KERNEL);
> >  	
> >  	if (!ctx->codec.h264.pic_info_buf)
> > 
> > @@ -566,15 +589,51 @@ static int cedrus_h264_start(struct cedrus_ctx *ctx)
> > 
> >  		goto err_neighbor_buf;
> >  	
> >  	}
> > 
> > +	if (ctx->src_fmt.width > 2048) {
> 
> Feel free to add a comment here to explain where the 12 below comes from if
> you have some idea, or that it's a somewhat magical value that generally
> works.
> > +		ctx->codec.h264.deblk_buf_size =
> > +			ALIGN(ctx->src_fmt.width, 32) * 12;
> > +		ctx->codec.h264.deblk_buf =
> > +			dma_alloc_coherent(dev->dev,
> > +					   ctx-
>codec.h264.deblk_buf_size,
> > +					   &ctx-
>codec.h264.deblk_buf_dma,
> > +					   GFP_KERNEL);
> > +		if (!ctx->codec.h264.deblk_buf) {
> > +			ret = -ENOMEM;
> > +			goto err_mv_col_buf;
> > +		}
> > +
> 
> Same here, a comment would be welcome about the 5 value below.
> 
> Cheers,
> 
> Paul
> 
> > +		ctx->codec.h264.intra_pred_buf_size =
> > +			ALIGN(ctx->src_fmt.width, 64) * 5;
> > +		ctx->codec.h264.intra_pred_buf =
> > +			dma_alloc_coherent(dev->dev,
> > +					   ctx-
>codec.h264.intra_pred_buf_size,
> > +					   &ctx-
>codec.h264.intra_pred_buf_dma,
> > +					   GFP_KERNEL);
> > +		if (!ctx->codec.h264.intra_pred_buf) {
> > +			ret = -ENOMEM;
> > +			goto err_deblk_buf;
> > +		}
> > +	}
> > +
> > 
> >  	return 0;
> > 
> > +err_deblk_buf:
> > +	dma_free_coherent(dev->dev, ctx->codec.h264.deblk_buf_size,
> > +			  ctx->codec.h264.deblk_buf,
> > +			  ctx->codec.h264.deblk_buf_dma);
> > +
> > +err_mv_col_buf:
> > +	dma_free_coherent(dev->dev, ctx->codec.h264.mv_col_buf_size,
> > +			  ctx->codec.h264.mv_col_buf,
> > +			  ctx->codec.h264.mv_col_buf_dma);
> > +
> > 
> >  err_neighbor_buf:
> >  	dma_free_coherent(dev->dev, CEDRUS_NEIGHBOR_INFO_BUF_SIZE,
> >  	
> >  			  ctx->codec.h264.neighbor_info_buf,
> >  			  ctx->codec.h264.neighbor_info_buf_dma);
> >  
> >  err_pic_buf:
> > -	dma_free_coherent(dev->dev, CEDRUS_PIC_INFO_BUF_SIZE,
> > +	dma_free_coherent(dev->dev, ctx->codec.h264.pic_info_buf_size,
> > 
> >  			  ctx->codec.h264.pic_info_buf,
> >  			  ctx->codec.h264.pic_info_buf_dma);
> >  	
> >  	return ret;
> > 
> > @@ -590,9 +649,17 @@ static void cedrus_h264_stop(struct cedrus_ctx *ctx)
> > 
> >  	dma_free_coherent(dev->dev, CEDRUS_NEIGHBOR_INFO_BUF_SIZE,
> >  	
> >  			  ctx->codec.h264.neighbor_info_buf,
> >  			  ctx->codec.h264.neighbor_info_buf_dma);
> > 
> > -	dma_free_coherent(dev->dev, CEDRUS_PIC_INFO_BUF_SIZE,
> > +	dma_free_coherent(dev->dev, ctx->codec.h264.pic_info_buf_size,
> > 
> >  			  ctx->codec.h264.pic_info_buf,
> >  			  ctx->codec.h264.pic_info_buf_dma);
> > 
> > +	if (ctx->codec.h264.deblk_buf_size)
> > +		dma_free_coherent(dev->dev, ctx-
>codec.h264.deblk_buf_size,
> > +				  ctx->codec.h264.deblk_buf,
> > +				  ctx->codec.h264.deblk_buf_dma);
> > +	if (ctx->codec.h264.intra_pred_buf_size)
> > +		dma_free_coherent(dev->dev, ctx-
>codec.h264.intra_pred_buf_size,
> > +				  ctx->codec.h264.intra_pred_buf,
> > +				  ctx-
>codec.h264.intra_pred_buf_dma);
> > 
> >  }
> >  
> >  static void cedrus_h264_trigger(struct cedrus_ctx *ctx)
> > 
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> > b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h index
> > ace3d49fcd82..7beb03d3bb39 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> > @@ -46,6 +46,17 @@
> > 
> >  #define VE_MODE_DEC_H264			(0x01 << 0)
> >  #define VE_MODE_DEC_MPEG			(0x00 << 0)
> > 
> > +#define VE_BUF_CTRL				0x50
> > +
> > +#define VE_BUF_CTRL_INTRAPRED_EXT_RAM		(0x02 << 2)
> > +#define VE_BUF_CTRL_INTRAPRED_MIXED_RAM		(0x01 << 2)
> > +#define VE_BUF_CTRL_INTRAPRED_INT_SRAM		(0x00 << 2)
> > +#define VE_BUF_CTRL_DBLK_EXT_RAM		(0x02 << 0)
> > +#define VE_BUF_CTRL_DBLK_MIXED_RAM		(0x01 << 0)
> > +#define VE_BUF_CTRL_DBLK_INT_SRAM		(0x00 << 0)
> > +
> > +#define VE_DBLK_DRAM_BUF_ADDR			0x54
> > +#define VE_INTRAPRED_DRAM_BUF_ADDR		0x58
> > 
> >  #define VE_PRIMARY_CHROMA_BUF_LEN		0xc4
> >  #define VE_PRIMARY_FB_LINE_STRIDE		0xc8





^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] media: cedrus: Fix H264 4k support
  2019-11-04 16:53     ` Jernej Škrabec
@ 2019-11-05  8:07       ` Paul Kocialkowski
  0 siblings, 0 replies; 13+ messages in thread
From: Paul Kocialkowski @ 2019-11-05  8:07 UTC (permalink / raw)
  To: Jernej Škrabec
  Cc: mripard, mchehab, hverkuil-cisco, gregkh, wens, linux-media,
	devel, linux-arm-kernel, linux-kernel, linux-sunxi

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

Hi Jernej,

On Mon 04 Nov 19, 17:53, Jernej Škrabec wrote:
> Dne ponedeljek, 04. november 2019 ob 11:13:19 CET je Paul Kocialkowski 
> napisal(a):
> > Hi,
> > 
> > On Sat 26 Oct 19, 09:49, Jernej Skrabec wrote:
> > > H264 decoder needs additional or bigger buffers in order to decode 4k
> > > videos.
> > 
> > Thanks for the fixup, we hadn't looked into those bits at all during initial
> > bringup of H.264!
> > 
> > See a few minor comments below.
> > 
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > ---
> > > 
> > >  drivers/staging/media/sunxi/cedrus/cedrus.h   |  7 ++
> > >  .../staging/media/sunxi/cedrus/cedrus_h264.c  | 83 +++++++++++++++++--
> > >  .../staging/media/sunxi/cedrus/cedrus_regs.h  | 11 +++
> > >  3 files changed, 93 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > b/drivers/staging/media/sunxi/cedrus/cedrus.h index
> > > c45fb9a7ad07..96765555ab8a 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> > > @@ -116,8 +116,15 @@ struct cedrus_ctx {
> > > 
> > >  			ssize_t		mv_col_buf_size;
> > >  			void		*pic_info_buf;
> > >  			dma_addr_t	pic_info_buf_dma;
> > > 
> > > +			ssize_t		pic_info_buf_size;
> > > 
> > >  			void		*neighbor_info_buf;
> > >  			dma_addr_t	neighbor_info_buf_dma;
> > > 
> > > +			void		*deblk_buf;
> > > +			dma_addr_t	deblk_buf_dma;
> > > +			ssize_t		deblk_buf_size;
> > > +			void		*intra_pred_buf;
> > > +			dma_addr_t	intra_pred_buf_dma;
> > > +			ssize_t		intra_pred_buf_size;
> > > 
> > >  		} h264;
> > >  		struct {
> > >  		
> > >  			void		*mv_col_buf;
> > > 
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index
> > > d2c854ecdf15..19962f4213d4 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > @@ -39,7 +39,6 @@ struct cedrus_h264_sram_ref_pic {
> > > 
> > >  #define CEDRUS_H264_FRAME_NUM		18
> > >  
> > >  #define CEDRUS_NEIGHBOR_INFO_BUF_SIZE	(16 * SZ_1K)
> > > 
> > > -#define CEDRUS_PIC_INFO_BUF_SIZE	(128 * SZ_1K)
> > 
> > Could we keep a define with the minimum size that you are using later
> > (increased to 130 * SZ_1K)?
> 
> Sure.
> 
> > 
> > >  static void cedrus_h264_write_sram(struct cedrus_dev *dev,
> > >  
> > >  				   enum cedrus_h264_sram_off off,
> > > 
> > > @@ -342,6 +341,20 @@ static void cedrus_set_params(struct cedrus_ctx *ctx,
> > > 
> > >  		     VE_H264_VLD_ADDR_FIRST | VE_H264_VLD_ADDR_VALID |
> > >  		     VE_H264_VLD_ADDR_LAST);
> > > 
> > > +	if (ctx->src_fmt.width > 2048) {
> > > +		cedrus_write(dev, VE_BUF_CTRL,
> > > +			     VE_BUF_CTRL_INTRAPRED_MIXED_RAM |
> > > +			     VE_BUF_CTRL_DBLK_MIXED_RAM);
> > > +		cedrus_write(dev, VE_DBLK_DRAM_BUF_ADDR,
> > > +			     ctx->codec.h264.deblk_buf_dma);
> > > +		cedrus_write(dev, VE_INTRAPRED_DRAM_BUF_ADDR,
> > > +			     ctx->codec.h264.intra_pred_buf_dma);
> > > +	} else {
> > > +		cedrus_write(dev, VE_BUF_CTRL,
> > > +			     VE_BUF_CTRL_INTRAPRED_INT_SRAM |
> > > +			     VE_BUF_CTRL_DBLK_INT_SRAM);
> > > +	}
> > > +
> > > 
> > >  	/*
> > >  	
> > >  	 * FIXME: Since the bitstream parsing is done in software, and
> > >  	 * in userspace, this shouldn't be needed anymore. But it
> > > 
> > > @@ -502,18 +515,28 @@ static void cedrus_h264_setup(struct cedrus_ctx
> > > *ctx,
> > > 
> > >  static int cedrus_h264_start(struct cedrus_ctx *ctx)
> > >  {
> > >  
> > >  	struct cedrus_dev *dev = ctx->dev;
> > > 
> > > +	unsigned int pic_info_size;
> > > 
> > >  	unsigned int field_size;
> > >  	unsigned int mv_col_size;
> > >  	int ret;
> > 
> > Maybe add a comment here this is a half-magic sub-optimal formula?
> 
> Well, I'm not sure how much suboptimal formulas this and those below are. They 
> are taken from CedarX source. I would imagine that they didn't waste too much 
> memory. What kind of comment would be ok for you? "Formula taken from CedarX 
> source"?

Yes, something like that would work fine. The point is to make it clear that
it is not an obvious or direct calculation based on something from the spec.

Cheers,

Paul

> Best regards,
> Jernej
> 
> > 
> > > +	if (ctx->src_fmt.width > 2048)
> > > +		pic_info_size = CEDRUS_H264_FRAME_NUM * 0x4000;
> > > +	else
> > > +		pic_info_size = CEDRUS_H264_FRAME_NUM * 0x1000;
> > > +
> > > 
> > >  	/*
> > > 
> > > -	 * FIXME: It seems that the H6 cedarX code is using a formula
> > > -	 * here based on the size of the frame, while all the older
> > > -	 * code is using a fixed size, so that might need to be
> > > -	 * changed at some point.
> > > +	 * FIXME: If V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY is set,
> > > +	 * there is no need to multiply by 2.
> > > 
> > >  	 */
> > > 
> > > +	pic_info_size += ctx->src_fmt.height * 2 * 64;
> > > +
> > > +	if (pic_info_size < 130 * SZ_1K)
> > > +		pic_info_size = 130 * SZ_1K;
> > 
> > This is where I think we could have a "minimum pic info size" define.
> > 
> > > +
> > > +	ctx->codec.h264.pic_info_buf_size = pic_info_size;
> > > 
> > >  	ctx->codec.h264.pic_info_buf =
> > > 
> > > -		dma_alloc_coherent(dev->dev, CEDRUS_PIC_INFO_BUF_SIZE,
> > > +		dma_alloc_coherent(dev->dev, ctx-
> >codec.h264.pic_info_buf_size,
> > > 
> > >  				   &ctx-
> >codec.h264.pic_info_buf_dma,
> > >  				   GFP_KERNEL);
> > >  	
> > >  	if (!ctx->codec.h264.pic_info_buf)
> > > 
> > > @@ -566,15 +589,51 @@ static int cedrus_h264_start(struct cedrus_ctx *ctx)
> > > 
> > >  		goto err_neighbor_buf;
> > >  	
> > >  	}
> > > 
> > > +	if (ctx->src_fmt.width > 2048) {
> > 
> > Feel free to add a comment here to explain where the 12 below comes from if
> > you have some idea, or that it's a somewhat magical value that generally
> > works.
> > > +		ctx->codec.h264.deblk_buf_size =
> > > +			ALIGN(ctx->src_fmt.width, 32) * 12;
> > > +		ctx->codec.h264.deblk_buf =
> > > +			dma_alloc_coherent(dev->dev,
> > > +					   ctx-
> >codec.h264.deblk_buf_size,
> > > +					   &ctx-
> >codec.h264.deblk_buf_dma,
> > > +					   GFP_KERNEL);
> > > +		if (!ctx->codec.h264.deblk_buf) {
> > > +			ret = -ENOMEM;
> > > +			goto err_mv_col_buf;
> > > +		}
> > > +
> > 
> > Same here, a comment would be welcome about the 5 value below.
> > 
> > Cheers,
> > 
> > Paul
> > 
> > > +		ctx->codec.h264.intra_pred_buf_size =
> > > +			ALIGN(ctx->src_fmt.width, 64) * 5;
> > > +		ctx->codec.h264.intra_pred_buf =
> > > +			dma_alloc_coherent(dev->dev,
> > > +					   ctx-
> >codec.h264.intra_pred_buf_size,
> > > +					   &ctx-
> >codec.h264.intra_pred_buf_dma,
> > > +					   GFP_KERNEL);
> > > +		if (!ctx->codec.h264.intra_pred_buf) {
> > > +			ret = -ENOMEM;
> > > +			goto err_deblk_buf;
> > > +		}
> > > +	}
> > > +
> > > 
> > >  	return 0;
> > > 
> > > +err_deblk_buf:
> > > +	dma_free_coherent(dev->dev, ctx->codec.h264.deblk_buf_size,
> > > +			  ctx->codec.h264.deblk_buf,
> > > +			  ctx->codec.h264.deblk_buf_dma);
> > > +
> > > +err_mv_col_buf:
> > > +	dma_free_coherent(dev->dev, ctx->codec.h264.mv_col_buf_size,
> > > +			  ctx->codec.h264.mv_col_buf,
> > > +			  ctx->codec.h264.mv_col_buf_dma);
> > > +
> > > 
> > >  err_neighbor_buf:
> > >  	dma_free_coherent(dev->dev, CEDRUS_NEIGHBOR_INFO_BUF_SIZE,
> > >  	
> > >  			  ctx->codec.h264.neighbor_info_buf,
> > >  			  ctx->codec.h264.neighbor_info_buf_dma);
> > >  
> > >  err_pic_buf:
> > > -	dma_free_coherent(dev->dev, CEDRUS_PIC_INFO_BUF_SIZE,
> > > +	dma_free_coherent(dev->dev, ctx->codec.h264.pic_info_buf_size,
> > > 
> > >  			  ctx->codec.h264.pic_info_buf,
> > >  			  ctx->codec.h264.pic_info_buf_dma);
> > >  	
> > >  	return ret;
> > > 
> > > @@ -590,9 +649,17 @@ static void cedrus_h264_stop(struct cedrus_ctx *ctx)
> > > 
> > >  	dma_free_coherent(dev->dev, CEDRUS_NEIGHBOR_INFO_BUF_SIZE,
> > >  	
> > >  			  ctx->codec.h264.neighbor_info_buf,
> > >  			  ctx->codec.h264.neighbor_info_buf_dma);
> > > 
> > > -	dma_free_coherent(dev->dev, CEDRUS_PIC_INFO_BUF_SIZE,
> > > +	dma_free_coherent(dev->dev, ctx->codec.h264.pic_info_buf_size,
> > > 
> > >  			  ctx->codec.h264.pic_info_buf,
> > >  			  ctx->codec.h264.pic_info_buf_dma);
> > > 
> > > +	if (ctx->codec.h264.deblk_buf_size)
> > > +		dma_free_coherent(dev->dev, ctx-
> >codec.h264.deblk_buf_size,
> > > +				  ctx->codec.h264.deblk_buf,
> > > +				  ctx->codec.h264.deblk_buf_dma);
> > > +	if (ctx->codec.h264.intra_pred_buf_size)
> > > +		dma_free_coherent(dev->dev, ctx-
> >codec.h264.intra_pred_buf_size,
> > > +				  ctx->codec.h264.intra_pred_buf,
> > > +				  ctx-
> >codec.h264.intra_pred_buf_dma);
> > > 
> > >  }
> > >  
> > >  static void cedrus_h264_trigger(struct cedrus_ctx *ctx)
> > > 
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> > > b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h index
> > > ace3d49fcd82..7beb03d3bb39 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> > > @@ -46,6 +46,17 @@
> > > 
> > >  #define VE_MODE_DEC_H264			(0x01 << 0)
> > >  #define VE_MODE_DEC_MPEG			(0x00 << 0)
> > > 
> > > +#define VE_BUF_CTRL				0x50
> > > +
> > > +#define VE_BUF_CTRL_INTRAPRED_EXT_RAM		(0x02 << 2)
> > > +#define VE_BUF_CTRL_INTRAPRED_MIXED_RAM		(0x01 << 2)
> > > +#define VE_BUF_CTRL_INTRAPRED_INT_SRAM		(0x00 << 2)
> > > +#define VE_BUF_CTRL_DBLK_EXT_RAM		(0x02 << 0)
> > > +#define VE_BUF_CTRL_DBLK_MIXED_RAM		(0x01 << 0)
> > > +#define VE_BUF_CTRL_DBLK_INT_SRAM		(0x00 << 0)
> > > +
> > > +#define VE_DBLK_DRAM_BUF_ADDR			0x54
> > > +#define VE_INTRAPRED_DRAM_BUF_ADDR		0x58
> > > 
> > >  #define VE_PRIMARY_CHROMA_BUF_LEN		0xc4
> > >  #define VE_PRIMARY_FB_LINE_STRIDE		0xc8
> 
> 
> 
> 

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] media: cedrus: Properly signal size in mode register
  2019-11-04 16:33     ` Jernej Škrabec
@ 2019-11-05  8:10       ` Paul Kocialkowski
  2019-11-06 17:41         ` Jernej Škrabec
       [not found]         ` <jwv1ruj7on7.fsf-monnier+gmane.comp.hardware.netbook.arm.sunxi@gnu.org>
  0 siblings, 2 replies; 13+ messages in thread
From: Paul Kocialkowski @ 2019-11-05  8:10 UTC (permalink / raw)
  To: Jernej Škrabec
  Cc: mripard, mchehab, hverkuil-cisco, gregkh, wens, linux-media,
	devel, linux-arm-kernel, linux-kernel, linux-sunxi

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

Hi,

On Mon 04 Nov 19, 17:33, Jernej Škrabec wrote:
> Dne ponedeljek, 04. november 2019 ob 11:02:28 CET je Paul Kocialkowski 
> napisal(a):
> > Hi Jernej,
> > 
> > On Sat 26 Oct 19, 09:49, Jernej Skrabec wrote:
> > > Mode register also holds information if video width is bigger than 2048
> > > and if it is equal to 4096.
> > > 
> > > Rework cedrus_engine_enable() to properly signal this properties.
> > 
> > Thanks for the patch, looks good to me!
> > 
> > Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > 
> > One minor thing: maybe we should have a way to set the maximum dimensions
> > depending on the generation of the engine in use and the actual maximum
> > supported by the hardware.
> > 
> > Maybe either as dedicated new fields in struct cedrus_variant or as
> > capability flags.
> 
> I was thinking about first solution, but after going trough manuals, it was 
> unclear what are real limitations. For example, H3 manual states that it is 
> capable of decoding H264 1080p@60Hz. However, I know for a fact that it is 
> also capable of decoding 4k videos, but probably not at 60 Hz. I don't own 
> anything older that A83T, so I don't know what are capabilities of those SoCs. 

So I guess in this case we should try and see. I could try to look into it at
some point in the future too if you're not particulary interested.

> Anyway, being slow is still ok for some tasks, like transcoding, so we can't 
> limit decoding to 1080p just because it's slow. It is probably still faster 
> than doing it in SW. Not to mention that it's still ok for some videos, a lot 
> of them uses 24 fps.

I agree, it's best to expose the maximum supported resolution by the hardware,
even if it means running at a lower fps.

Do you know if we have a way to report some estimation of the maximum supported
fps to userspace? It would be useful to let userspace decide whether it's a
better fit than software decoding.

Cheers,

Paul

> Best regards,
> Jernej
> 
> > 
> > Anyway that can be done later since we were already hardcoding this.
> > 
> > Cheers,
> > 
> > Paul
> > 
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > ---
> > > 
> > >  drivers/staging/media/sunxi/cedrus/cedrus_h264.c  | 2 +-
> > >  drivers/staging/media/sunxi/cedrus/cedrus_h265.c  | 2 +-
> > >  drivers/staging/media/sunxi/cedrus/cedrus_hw.c    | 9 +++++++--
> > >  drivers/staging/media/sunxi/cedrus/cedrus_hw.h    | 2 +-
> > >  drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c | 2 +-
> > >  drivers/staging/media/sunxi/cedrus/cedrus_regs.h  | 2 ++
> > >  6 files changed, 13 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index
> > > 7487f6ab7576..d2c854ecdf15 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > @@ -485,7 +485,7 @@ static void cedrus_h264_setup(struct cedrus_ctx *ctx,
> > > 
> > >  {
> > >  
> > >  	struct cedrus_dev *dev = ctx->dev;
> > > 
> > > -	cedrus_engine_enable(dev, CEDRUS_CODEC_H264);
> > > +	cedrus_engine_enable(ctx, CEDRUS_CODEC_H264);
> > > 
> > >  	cedrus_write(dev, VE_H264_SDROT_CTRL, 0);
> > >  	cedrus_write(dev, VE_H264_EXTRA_BUFFER1,
> > > 
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > > b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index
> > > 9bc921866f70..6945dc74e1d7 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > > @@ -276,7 +276,7 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
> > > 
> > >  	}
> > >  	
> > >  	/* Activate H265 engine. */
> > > 
> > > -	cedrus_engine_enable(dev, CEDRUS_CODEC_H265);
> > > +	cedrus_engine_enable(ctx, CEDRUS_CODEC_H265);
> > > 
> > >  	/* Source offset and length in bits. */
> > > 
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> > > b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c index
> > > 570a9165dd5d..3acfa21bc124 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> > > @@ -30,7 +30,7 @@
> > > 
> > >  #include "cedrus_hw.h"
> > >  #include "cedrus_regs.h"
> > > 
> > > -int cedrus_engine_enable(struct cedrus_dev *dev, enum cedrus_codec codec)
> > > +int cedrus_engine_enable(struct cedrus_ctx *ctx, enum cedrus_codec codec)
> > > 
> > >  {
> > >  
> > >  	u32 reg = 0;
> > > 
> > > @@ -58,7 +58,12 @@ int cedrus_engine_enable(struct cedrus_dev *dev, enum
> > > cedrus_codec codec)> 
> > >  		return -EINVAL;
> > >  	
> > >  	}
> > > 
> > > -	cedrus_write(dev, VE_MODE, reg);
> > > +	if (ctx->src_fmt.width == 4096)
> > > +		reg |= VE_MODE_PIC_WIDTH_IS_4096;
> > > +	if (ctx->src_fmt.width > 2048)
> > > +		reg |= VE_MODE_PIC_WIDTH_MORE_2048;
> > > +
> > > +	cedrus_write(ctx->dev, VE_MODE, reg);
> > > 
> > >  	return 0;
> > >  
> > >  }
> > > 
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> > > b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h index
> > > 27d0882397aa..604ff932fbf5 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> > > @@ -16,7 +16,7 @@
> > > 
> > >  #ifndef _CEDRUS_HW_H_
> > >  #define _CEDRUS_HW_H_
> > > 
> > > -int cedrus_engine_enable(struct cedrus_dev *dev, enum cedrus_codec
> > > codec);
> > > +int cedrus_engine_enable(struct cedrus_ctx *ctx, enum cedrus_codec
> > > codec);
> > > 
> > >  void cedrus_engine_disable(struct cedrus_dev *dev);
> > >  
> > >  void cedrus_dst_format_set(struct cedrus_dev *dev,
> > > 
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c index
> > > 13c34927bad5..8bcd6b8f9e2d 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > @@ -96,7 +96,7 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx,
> > > struct cedrus_run *run)> 
> > >  	quantization = run->mpeg2.quantization;
> > >  	
> > >  	/* Activate MPEG engine. */
> > > 
> > > -	cedrus_engine_enable(dev, CEDRUS_CODEC_MPEG2);
> > > +	cedrus_engine_enable(ctx, CEDRUS_CODEC_MPEG2);
> > > 
> > >  	/* Set intra quantization matrix. */
> > > 
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> > > b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h index
> > > 4275a307d282..ace3d49fcd82 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> > > @@ -35,6 +35,8 @@
> > > 
> > >  #define VE_MODE					0x00
> > > 
> > > +#define VE_MODE_PIC_WIDTH_IS_4096		BIT(22)
> > > +#define VE_MODE_PIC_WIDTH_MORE_2048		BIT(21)
> > > 
> > >  #define VE_MODE_REC_WR_MODE_2MB			(0x01 << 20)
> > >  #define VE_MODE_REC_WR_MODE_1MB			(0x00 << 20)
> > >  #define VE_MODE_DDR_MODE_BW_128			(0x03 << 16)
> 
> 
> 
> 

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] media: cedrus: Properly signal size in mode register
  2019-11-05  8:10       ` Paul Kocialkowski
@ 2019-11-06 17:41         ` Jernej Škrabec
       [not found]         ` <jwv1ruj7on7.fsf-monnier+gmane.comp.hardware.netbook.arm.sunxi@gnu.org>
  1 sibling, 0 replies; 13+ messages in thread
From: Jernej Škrabec @ 2019-11-06 17:41 UTC (permalink / raw)
  To: Paul Kocialkowski
  Cc: mripard, mchehab, hverkuil-cisco, gregkh, wens, linux-media,
	devel, linux-arm-kernel, linux-kernel, linux-sunxi

Dne torek, 05. november 2019 ob 09:10:34 CET je Paul Kocialkowski napisal(a):
> Hi,
> 
> On Mon 04 Nov 19, 17:33, Jernej Škrabec wrote:
> > Dne ponedeljek, 04. november 2019 ob 11:02:28 CET je Paul Kocialkowski
> > 
> > napisal(a):
> > > Hi Jernej,
> > > 
> > > On Sat 26 Oct 19, 09:49, Jernej Skrabec wrote:
> > > > Mode register also holds information if video width is bigger than
> > > > 2048
> > > > and if it is equal to 4096.
> > > > 
> > > > Rework cedrus_engine_enable() to properly signal this properties.
> > > 
> > > Thanks for the patch, looks good to me!
> > > 
> > > Acked-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > 
> > > One minor thing: maybe we should have a way to set the maximum
> > > dimensions
> > > depending on the generation of the engine in use and the actual maximum
> > > supported by the hardware.
> > > 
> > > Maybe either as dedicated new fields in struct cedrus_variant or as
> > > capability flags.
> > 
> > I was thinking about first solution, but after going trough manuals, it
> > was
> > unclear what are real limitations. For example, H3 manual states that it
> > is
> > capable of decoding H264 1080p@60Hz. However, I know for a fact that it is
> > also capable of decoding 4k videos, but probably not at 60 Hz. I don't own
> > anything older that A83T, so I don't know what are capabilities of those
> > SoCs.
> So I guess in this case we should try and see. I could try to look into it
> at some point in the future too if you're not particulary interested.

Well, I can take a look at my HW, but I have only few SoCs with more or less 
same capability.

> > Anyway, being slow is still ok for some tasks, like transcoding, so we
> > can't limit decoding to 1080p just because it's slow. It is probably
> > still faster than doing it in SW. Not to mention that it's still ok for
> > some videos, a lot of them uses 24 fps.
> 
> I agree, it's best to expose the maximum supported resolution by the
> hardware, even if it means running at a lower fps.
> 
> Do you know if we have a way to report some estimation of the maximum
> supported fps to userspace? It would be useful to let userspace decide
> whether it's a better fit than software decoding.

I took a quick look at existing controls, but I don't see anything 
appropriate.

Best regards,
Jernej

> 
> Cheers,
> 
> Paul
> 
> > Best regards,
> > Jernej
> > 
> > > Anyway that can be done later since we were already hardcoding this.
> > > 
> > > Cheers,
> > > 
> > > Paul
> > > 
> > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > > ---
> > > > 
> > > >  drivers/staging/media/sunxi/cedrus/cedrus_h264.c  | 2 +-
> > > >  drivers/staging/media/sunxi/cedrus/cedrus_h265.c  | 2 +-
> > > >  drivers/staging/media/sunxi/cedrus/cedrus_hw.c    | 9 +++++++--
> > > >  drivers/staging/media/sunxi/cedrus/cedrus_hw.h    | 2 +-
> > > >  drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c | 2 +-
> > > >  drivers/staging/media/sunxi/cedrus/cedrus_regs.h  | 2 ++
> > > >  6 files changed, 13 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index
> > > > 7487f6ab7576..d2c854ecdf15 100644
> > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > > > @@ -485,7 +485,7 @@ static void cedrus_h264_setup(struct cedrus_ctx
> > > > *ctx,
> > > > 
> > > >  {
> > > >  
> > > >  	struct cedrus_dev *dev = ctx->dev;
> > > > 
> > > > -	cedrus_engine_enable(dev, CEDRUS_CODEC_H264);
> > > > +	cedrus_engine_enable(ctx, CEDRUS_CODEC_H264);
> > > > 
> > > >  	cedrus_write(dev, VE_H264_SDROT_CTRL, 0);
> > > >  	cedrus_write(dev, VE_H264_EXTRA_BUFFER1,
> > > > 
> > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > > > b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index
> > > > 9bc921866f70..6945dc74e1d7 100644
> > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > > > @@ -276,7 +276,7 @@ static void cedrus_h265_setup(struct cedrus_ctx
> > > > *ctx,
> > > > 
> > > >  	}
> > > >  	
> > > >  	/* Activate H265 engine. */
> > > > 
> > > > -	cedrus_engine_enable(dev, CEDRUS_CODEC_H265);
> > > > +	cedrus_engine_enable(ctx, CEDRUS_CODEC_H265);
> > > > 
> > > >  	/* Source offset and length in bits. */
> > > > 
> > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> > > > b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c index
> > > > 570a9165dd5d..3acfa21bc124 100644
> > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.c
> > > > @@ -30,7 +30,7 @@
> > > > 
> > > >  #include "cedrus_hw.h"
> > > >  #include "cedrus_regs.h"
> > > > 
> > > > -int cedrus_engine_enable(struct cedrus_dev *dev, enum cedrus_codec
> > > > codec)
> > > > +int cedrus_engine_enable(struct cedrus_ctx *ctx, enum cedrus_codec
> > > > codec)
> > > > 
> > > >  {
> > > >  
> > > >  	u32 reg = 0;
> > > > 
> > > > @@ -58,7 +58,12 @@ int cedrus_engine_enable(struct cedrus_dev *dev,
> > > > enum
> > > > cedrus_codec codec)>
> > > > 
> > > >  		return -EINVAL;
> > > >  	
> > > >  	}
> > > > 
> > > > -	cedrus_write(dev, VE_MODE, reg);
> > > > +	if (ctx->src_fmt.width == 4096)
> > > > +		reg |= VE_MODE_PIC_WIDTH_IS_4096;
> > > > +	if (ctx->src_fmt.width > 2048)
> > > > +		reg |= VE_MODE_PIC_WIDTH_MORE_2048;
> > > > +
> > > > +	cedrus_write(ctx->dev, VE_MODE, reg);
> > > > 
> > > >  	return 0;
> > > >  
> > > >  }
> > > > 
> > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> > > > b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h index
> > > > 27d0882397aa..604ff932fbf5 100644
> > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_hw.h
> > > > @@ -16,7 +16,7 @@
> > > > 
> > > >  #ifndef _CEDRUS_HW_H_
> > > >  #define _CEDRUS_HW_H_
> > > > 
> > > > -int cedrus_engine_enable(struct cedrus_dev *dev, enum cedrus_codec
> > > > codec);
> > > > +int cedrus_engine_enable(struct cedrus_ctx *ctx, enum cedrus_codec
> > > > codec);
> > > > 
> > > >  void cedrus_engine_disable(struct cedrus_dev *dev);
> > > >  
> > > >  void cedrus_dst_format_set(struct cedrus_dev *dev,
> > > > 
> > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > > b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c index
> > > > 13c34927bad5..8bcd6b8f9e2d 100644
> > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > > @@ -96,7 +96,7 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx
> > > > *ctx,
> > > > struct cedrus_run *run)>
> > > > 
> > > >  	quantization = run->mpeg2.quantization;
> > > >  	
> > > >  	/* Activate MPEG engine. */
> > > > 
> > > > -	cedrus_engine_enable(dev, CEDRUS_CODEC_MPEG2);
> > > > +	cedrus_engine_enable(ctx, CEDRUS_CODEC_MPEG2);
> > > > 
> > > >  	/* Set intra quantization matrix. */
> > > > 
> > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> > > > b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h index
> > > > 4275a307d282..ace3d49fcd82 100644
> > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> > > > @@ -35,6 +35,8 @@
> > > > 
> > > >  #define VE_MODE					0x00
> > > > 
> > > > +#define VE_MODE_PIC_WIDTH_IS_4096		BIT(22)
> > > > +#define VE_MODE_PIC_WIDTH_MORE_2048		BIT(21)
> > > > 
> > > >  #define VE_MODE_REC_WR_MODE_2MB			(0x01 << 20)
> > > >  #define VE_MODE_REC_WR_MODE_1MB			(0x00 << 20)
> > > >  #define VE_MODE_DDR_MODE_BW_128			(0x03 << 16)





^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [linux-sunxi] Re: [PATCH 1/3] media: cedrus: Properly signal size in mode register
       [not found]         ` <jwv1ruj7on7.fsf-monnier+gmane.comp.hardware.netbook.arm.sunxi@gnu.org>
@ 2019-11-09 12:56           ` " Paul Kocialkowski
  0 siblings, 0 replies; 13+ messages in thread
From: Paul Kocialkowski @ 2019-11-09 12:56 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: linux-sunxi, linux-media, linux-arm-kernel, linux-kernel

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

Hi Stefan,

On Thu 07 Nov 19, 09:24, Stefan Monnier wrote:
> > Do you know if we have a way to report some estimation of the maximum supported
> > fps to userspace? It would be useful to let userspace decide whether it's a
> > better fit than software decoding.
> 
> Even if the fps ends up too low for the player's taste, I can't imagine
> why software decoding would be preferable: it seems it could be only
> even (substantially) slower.  Or are there speed-up options in software
> decoding not available in hardware decoding (such as playing only every
> N'th frame, maybe?).

This may be true for the Allwinner case as we know it today but not true in
general. It could happen that the CPU is perfectly able to decode as fast as or
faster than the hardware implementation, but userspace would still try to use
hardware video decoding when it can provide good enough performance so that the
CPU can do other things in the meantime.

Having a good idea of the expected performance is important for userspace to
make this kind of policy decision.

This is kind of a common misconception that hardware offloading always implies
a performance improvment. In our cases where the CPU is a bottleneck, it is
more often true than not, but this is by far not true in general.

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, back to index

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-26  7:49 [PATCH 0/3] media: cedrus: Add support for 4k videos Jernej Skrabec
2019-10-26  7:49 ` [PATCH 1/3] media: cedrus: Properly signal size in mode register Jernej Skrabec
2019-11-04 10:02   ` Paul Kocialkowski
2019-11-04 16:33     ` Jernej Škrabec
2019-11-05  8:10       ` Paul Kocialkowski
2019-11-06 17:41         ` Jernej Škrabec
     [not found]         ` <jwv1ruj7on7.fsf-monnier+gmane.comp.hardware.netbook.arm.sunxi@gnu.org>
2019-11-09 12:56           ` [linux-sunxi] " Paul Kocialkowski
2019-10-26  7:49 ` [PATCH 2/3] media: cedrus: Fix H264 4k support Jernej Skrabec
2019-11-04 10:13   ` Paul Kocialkowski
2019-11-04 16:53     ` Jernej Škrabec
2019-11-05  8:07       ` Paul Kocialkowski
2019-10-26  7:49 ` [PATCH 3/3] media: cedrus: Increase maximum supported size Jernej Skrabec
2019-11-04 10:14   ` Paul Kocialkowski

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git