[PATCH v2 0/4] media: HEVC: RPS clean up

[PATCH v2 0/4] media: HEVC: RPS clean up

[Benjamin Gaignard]

This series aims to clean up Reference Picture Set usage and flags.

Long term flag was named with RPS prefix while it is not used for RPS
but for mark long term references in DBP. Remane it and remove the two
other useless RPS flags.

Clarify documentation about RPS lists content and make sure that Hantro
driver use them correctly (i.e without look up in DBP).

These patches are the last in my backlog impacting HEVC uAPI.
From my point of view, once they get merged, you could start talking
about how move HEVC uAPI to stable.

version 2:
- change DPB field name from rps to flags

Please note that the only purpose of commits 3 and 4 is to allow to test
G2 hardware block for IMX8MQ until a proper solution isuing power domain
can be found. Do not merge them.

GStreamer HEVC plugin merge request can be found here:
https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1079

With those piece of code fluster score is 77/147.

Benjamin

Benjamin Gaignard (4):
  media: hevc: Remove RPS named flags
  media: hevc: Embedded indexes in RPS
  media: hantro: Use syscon instead of 'ctrl' register
  arm64: dts: imx8mq: Add node to G2 hardware

 .../media/v4l/ext-ctrls-codec.rst             | 14 +++---
 arch/arm64/boot/dts/freescale/imx8mq.dtsi     | 43 +++++++++++++----
 drivers/staging/media/hantro/hantro.h         |  5 +-
 .../staging/media/hantro/hantro_g2_hevc_dec.c | 27 +++--------
 drivers/staging/media/hantro/imx8m_vpu_hw.c   | 48 ++++++++++++-------
 .../staging/media/sunxi/cedrus/cedrus_h265.c  |  2 +-
 include/media/hevc-ctrls.h                    |  6 +--
 7 files changed, 84 insertions(+), 61 deletions(-)

-- 
2.30.2

[PATCH v2 1/4] media: hevc: Remove RPS named flags

[Benjamin Gaignard]

Marking a picture as long-term reference is valid for DPB but not for RPS.
Change flag name to match with it description in HEVC spec chapiter
"8.3.2 Decoding process for reference picture set".
Remove the other unused RPS flags.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
version 2:
- change DPB field name from rps to flags

 Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 8 +++-----
 drivers/staging/media/hantro/hantro_g2_hevc_dec.c         | 2 +-
 drivers/staging/media/sunxi/cedrus/cedrus_h265.c          | 2 +-
 include/media/hevc-ctrls.h                                | 6 ++----
 4 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index e141f0e4eec9..38da33e61c3d 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -3166,11 +3166,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
 	:c:func:`v4l2_timeval_to_ns()` function to convert the struct
 	:c:type:`timeval` in struct :c:type:`v4l2_buffer` to a __u64.
     * - __u8
-      - ``rps``
-      - The reference set for the reference frame
-        (V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_BEFORE,
-        V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_AFTER or
-        V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR)
+      - ``flags``
+      - Long term flag for the reference frame
+        (V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE)
     * - __u8
       - ``field_pic``
       - Whether the reference is a field picture or a frame.
diff --git a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
index 76a921163b9a..bbf71dcea099 100644
--- a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
+++ b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
@@ -445,7 +445,7 @@ static int set_ref(struct hantro_ctx *ctx)
 		chroma_addr = luma_addr + cr_offset;
 		mv_addr = luma_addr + mv_offset;
 
-		if (dpb[i].rps == V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR)
+		if (dpb[i].flags == V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE)
 			dpb_longterm_e |= BIT(V4L2_HEVC_DPB_ENTRIES_NUM_MAX - 1 - i);
 
 		hantro_write_addr(vpu, G2_REG_ADDR_REF(i), luma_addr);
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
index 3d9561d4aadb..1edfc3823a57 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
@@ -169,7 +169,7 @@ static void cedrus_h265_ref_pic_list_write(struct cedrus_dev *dev,
 		unsigned int index = list[i];
 		u8 value = list[i];
 
-		if (dpb[index].rps == V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR)
+		if (dpb[index].flags == V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE)
 			value |= VE_DEC_H265_SRAM_REF_PIC_LIST_LT_REF;
 
 		/* Each SRAM word gathers up to 4 references. */
diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h
index ef63bc205756..01ccda48d8c5 100644
--- a/include/media/hevc-ctrls.h
+++ b/include/media/hevc-ctrls.h
@@ -127,15 +127,13 @@ struct v4l2_ctrl_hevc_pps {
 	__u64	flags;
 };
 
-#define V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_BEFORE	0x01
-#define V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_AFTER	0x02
-#define V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR		0x03
+#define V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE	0x01
 
 #define V4L2_HEVC_DPB_ENTRIES_NUM_MAX		16
 
 struct v4l2_hevc_dpb_entry {
 	__u64	timestamp;
-	__u8	rps;
+	__u8	flags;
 	__u8	field_pic;
 	__u16	pic_order_cnt[2];
 	__u8	padding[2];
-- 
2.30.2

[PATCH v2 2/4] media: hevc: Embedded indexes in RPS

[Benjamin Gaignard]

Reference Picture Set lists provide indexes of short and long term
reference in DBP array.
Fix Hantro to not do a look up in DBP entries.
Make documentation more clear about it.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
 .../media/v4l/ext-ctrls-codec.rst             |  6 ++---
 .../staging/media/hantro/hantro_g2_hevc_dec.c | 25 +++++--------------
 2 files changed, 9 insertions(+), 22 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
index 38da33e61c3d..b12ad5b3eaba 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
@@ -3381,15 +3381,15 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
     * - __u8
       - ``poc_st_curr_before[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
       - PocStCurrBefore as described in section 8.3.2 "Decoding process for reference
-        picture set.
+        picture set": provides the index of the short term before references in DPB array.
     * - __u8
       - ``poc_st_curr_after[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
       - PocStCurrAfter as described in section 8.3.2 "Decoding process for reference
-        picture set.
+        picture set": provides the index of the short term after references in DPB array.
     * - __u8
       - ``poc_lt_curr[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
       - PocLtCurr as described in section 8.3.2 "Decoding process for reference
-        picture set.
+        picture set": provides the index of the long term references in DPB array.
     * - __u64
       - ``flags``
       - See :ref:`Decode Parameters Flags <hevc_decode_params_flags>`
diff --git a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
index bbf71dcea099..33fe502de447 100644
--- a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
+++ b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
@@ -269,24 +269,11 @@ static void set_params(struct hantro_ctx *ctx)
 	hantro_reg_write(vpu, &g2_apf_threshold, 8);
 }
 
-static int find_ref_pic_index(const struct v4l2_hevc_dpb_entry *dpb, int pic_order_cnt)
-{
-	int i;
-
-	for (i = 0; i < V4L2_HEVC_DPB_ENTRIES_NUM_MAX; i++) {
-		if (dpb[i].pic_order_cnt[0] == pic_order_cnt)
-			return i;
-	}
-
-	return 0x0;
-}
-
 static void set_ref_pic_list(struct hantro_ctx *ctx)
 {
 	const struct hantro_hevc_dec_ctrls *ctrls = &ctx->hevc_dec.ctrls;
 	struct hantro_dev *vpu = ctx->dev;
 	const struct v4l2_ctrl_hevc_decode_params *decode_params = ctrls->decode_params;
-	const struct v4l2_hevc_dpb_entry *dpb = decode_params->dpb;
 	u32 list0[V4L2_HEVC_DPB_ENTRIES_NUM_MAX] = {};
 	u32 list1[V4L2_HEVC_DPB_ENTRIES_NUM_MAX] = {};
 	static const struct hantro_reg ref_pic_regs0[] = {
@@ -330,11 +317,11 @@ static void set_ref_pic_list(struct hantro_ctx *ctx)
 	/* List 0 contains: short term before, short term after and long term */
 	j = 0;
 	for (i = 0; i < decode_params->num_poc_st_curr_before && j < ARRAY_SIZE(list0); i++)
-		list0[j++] = find_ref_pic_index(dpb, decode_params->poc_st_curr_before[i]);
+		list0[j++] = decode_params->poc_st_curr_before[i];
 	for (i = 0; i < decode_params->num_poc_st_curr_after && j < ARRAY_SIZE(list0); i++)
-		list0[j++] = find_ref_pic_index(dpb, decode_params->poc_st_curr_after[i]);
+		list0[j++] = decode_params->poc_st_curr_after[i];
 	for (i = 0; i < decode_params->num_poc_lt_curr && j < ARRAY_SIZE(list0); i++)
-		list0[j++] = find_ref_pic_index(dpb, decode_params->poc_lt_curr[i]);
+		list0[j++] = decode_params->poc_lt_curr[i];
 
 	/* Fill the list, copying over and over */
 	i = 0;
@@ -343,11 +330,11 @@ static void set_ref_pic_list(struct hantro_ctx *ctx)
 
 	j = 0;
 	for (i = 0; i < decode_params->num_poc_st_curr_after && j < ARRAY_SIZE(list1); i++)
-		list1[j++] = find_ref_pic_index(dpb, decode_params->poc_st_curr_after[i]);
+		list1[j++] = decode_params->poc_st_curr_after[i];
 	for (i = 0; i < decode_params->num_poc_st_curr_before && j < ARRAY_SIZE(list1); i++)
-		list1[j++] = find_ref_pic_index(dpb, decode_params->poc_st_curr_before[i]);
+		list1[j++] = decode_params->poc_st_curr_before[i];
 	for (i = 0; i < decode_params->num_poc_lt_curr && j < ARRAY_SIZE(list1); i++)
-		list1[j++] = find_ref_pic_index(dpb, decode_params->poc_lt_curr[i]);
+		list1[j++] = decode_params->poc_lt_curr[i];
 
 	i = 0;
 	while (j < ARRAY_SIZE(list1))
-- 
2.30.2

[PATCH v2 3/4] media: hantro: Use syscon instead of 'ctrl' register

[Benjamin Gaignard]

In order to be able to share the control hardware block between
VPUs use a syscon instead a ioremap it in the driver.
To keep the compatibility with older DT if 'nxp,imx8mq-vpu-ctrl'
phandle is not found look at 'ctrl' reg-name.
With the method it becomes useless to provide a list of register
names so remove it.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

Please note that the only purpose of this commit is to allow to test
G2 hardware block for IMX8MQ until a proper solution isuing power domain
can be found. Do not merge it.

---
 drivers/staging/media/hantro/hantro.h       |  5 ++-
 drivers/staging/media/hantro/imx8m_vpu_hw.c | 48 +++++++++++++--------
 2 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h
index c2e2dca38628..20478988a55e 100644
--- a/drivers/staging/media/hantro/hantro.h
+++ b/drivers/staging/media/hantro/hantro.h
@@ -13,6 +13,7 @@
 #define HANTRO_H_
 
 #include <linux/platform_device.h>
+#include <linux/regmap.h>
 #include <linux/videodev2.h>
 #include <linux/wait.h>
 #include <linux/clk.h>
@@ -170,7 +171,7 @@ hantro_vdev_to_func(struct video_device *vdev)
  * @reg_bases:		Mapped addresses of VPU registers.
  * @enc_base:		Mapped address of VPU encoder register for convenience.
  * @dec_base:		Mapped address of VPU decoder register for convenience.
- * @ctrl_base:		Mapped address of VPU control block.
+ * @ctrl_base:		Regmap of VPU control block.
  * @vpu_mutex:		Mutex to synchronize V4L2 calls.
  * @irqlock:		Spinlock to synchronize access to data structures
  *			shared with interrupt handlers.
@@ -189,7 +190,7 @@ struct hantro_dev {
 	void __iomem **reg_bases;
 	void __iomem *enc_base;
 	void __iomem *dec_base;
-	void __iomem *ctrl_base;
+	struct regmap *ctrl_base;
 
 	struct mutex vpu_mutex;	/* video_device lock */
 	spinlock_t irqlock;
diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c b/drivers/staging/media/hantro/imx8m_vpu_hw.c
index ea919bfb9891..ec0b16d12b54 100644
--- a/drivers/staging/media/hantro/imx8m_vpu_hw.c
+++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c
@@ -7,6 +7,7 @@
 
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/mfd/syscon.h>
 
 #include "hantro.h"
 #include "hantro_jpeg.h"
@@ -25,30 +26,28 @@
 #define CTRL_G1_PP_FUSE		0x0c
 #define CTRL_G2_DEC_FUSE	0x10
 
+static const struct regmap_config ctrl_regmap_ctrl = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 0x14,
+};
+
 static void imx8m_soft_reset(struct hantro_dev *vpu, u32 reset_bits)
 {
-	u32 val;
-
 	/* Assert */
-	val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
-	val &= ~reset_bits;
-	writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
+	regmap_update_bits(vpu->ctrl_base, CTRL_SOFT_RESET, reset_bits, 0);
 
 	udelay(2);
 
 	/* Release */
-	val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
-	val |= reset_bits;
-	writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
+	regmap_update_bits(vpu->ctrl_base, CTRL_SOFT_RESET,
+			   reset_bits, reset_bits);
 }
 
 static void imx8m_clk_enable(struct hantro_dev *vpu, u32 clock_bits)
 {
-	u32 val;
-
-	val = readl(vpu->ctrl_base + CTRL_CLOCK_ENABLE);
-	val |= clock_bits;
-	writel(val, vpu->ctrl_base + CTRL_CLOCK_ENABLE);
+	regmap_update_bits(vpu->ctrl_base, CTRL_CLOCK_ENABLE,
+			   clock_bits, clock_bits);
 }
 
 static int imx8mq_runtime_resume(struct hantro_dev *vpu)
@@ -65,9 +64,9 @@ static int imx8mq_runtime_resume(struct hantro_dev *vpu)
 	imx8m_clk_enable(vpu, CLOCK_G1 | CLOCK_G2);
 
 	/* Set values of the fuse registers */
-	writel(0xffffffff, vpu->ctrl_base + CTRL_G1_DEC_FUSE);
-	writel(0xffffffff, vpu->ctrl_base + CTRL_G1_PP_FUSE);
-	writel(0xffffffff, vpu->ctrl_base + CTRL_G2_DEC_FUSE);
+	regmap_write(vpu->ctrl_base, CTRL_G1_DEC_FUSE, 0xffffffff);
+	regmap_write(vpu->ctrl_base, CTRL_G1_PP_FUSE, 0xffffffff);
+	regmap_write(vpu->ctrl_base, CTRL_G2_DEC_FUSE, 0xffffffff);
 
 	clk_bulk_disable_unprepare(vpu->variant->num_clocks, vpu->clocks);
 
@@ -189,7 +188,22 @@ static irqreturn_t imx8m_vpu_g2_irq(int irq, void *dev_id)
 
 static int imx8mq_vpu_hw_init(struct hantro_dev *vpu)
 {
-	vpu->ctrl_base = vpu->reg_bases[vpu->variant->num_regs - 1];
+	struct device_node *np = vpu->dev->of_node;
+
+	vpu->ctrl_base = syscon_regmap_lookup_by_phandle(np, "nxp,imx8m-vpu-ctrl");
+	if (IS_ERR(vpu->ctrl_base)) {
+		struct resource *res;
+		void __iomem *ctrl;
+
+		res = platform_get_resource_byname(vpu->pdev, IORESOURCE_MEM, "ctrl");
+		ctrl = devm_ioremap_resource(vpu->dev, res);
+		if (IS_ERR(ctrl))
+			return PTR_ERR(ctrl);
+
+		vpu->ctrl_base = devm_regmap_init_mmio(vpu->dev, ctrl, &ctrl_regmap_ctrl);
+		if (IS_ERR(vpu->ctrl_base))
+			return PTR_ERR(vpu->ctrl_base);
+	}
 
 	return 0;
 }
-- 
2.30.2

[PATCH v2 4/4] arm64: dts: imx8mq: Add node to G2 hardware

[Benjamin Gaignard]

Split the VPU node in two: one for G1 and one for G2 since they are
different hardware blocks.
Add syscon for the hardware control block.
Remove the reg-names property that is useless.
Each VPU node only needs one interrupt.
Change G2 assigned clock to match the specification.
In both nodes all the clocks need to be assigned to make
sure that the control block will be correctly clocked even if
only one device node is enabled.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>

Please note that the only purpose of this commit is to allow to test
G2 hardware block for IMX8MQ until a proper solution isuing power domain
can be found. Do not merge it.

---
 arch/arm64/boot/dts/freescale/imx8mq.dtsi | 43 ++++++++++++++++++-----
 1 file changed, 34 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
index 4066b1612655..9125e52147df 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
@@ -1432,15 +1432,16 @@ usb3_phy1: usb-phy@382f0040 {
 			status = "disabled";
 		};
 
-		vpu: video-codec@38300000 {
+		vpu_ctrl: syscon@38320000 {
+			compatible = "nxp,imx8mq-vpu-ctrl", "syscon";
+			reg = <0x38320000 0x10000>;
+		};
+
+		vpu_g1: video-codec@38300000 {
 			compatible = "nxp,imx8mq-vpu";
-			reg = <0x38300000 0x10000>,
-			      <0x38310000 0x10000>,
-			      <0x38320000 0x10000>;
-			reg-names = "g1", "g2", "ctrl";
-			interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
-				     <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
-			interrupt-names = "g1", "g2";
+			reg = <0x38300000 0x10000>;
+			interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "g1";
 			clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>,
 				 <&clk IMX8MQ_CLK_VPU_G2_ROOT>,
 				 <&clk IMX8MQ_CLK_VPU_DEC_ROOT>;
@@ -1453,9 +1454,33 @@ vpu: video-codec@38300000 {
 						 <&clk IMX8MQ_VPU_PLL_OUT>,
 						 <&clk IMX8MQ_SYS1_PLL_800M>,
 						 <&clk IMX8MQ_VPU_PLL>;
-			assigned-clock-rates = <600000000>, <600000000>,
+			assigned-clock-rates = <600000000>, <300000000>,
+					       <800000000>, <0>;
+			power-domains = <&pgc_vpu>;
+			nxp,imx8m-vpu-ctrl = <&vpu_ctrl>;
+		};
+
+		vpu_g2: video-codec@38310000 {
+			compatible = "nxp,imx8mq-vpu-g2";
+			reg = <0x38310000 0x10000>;
+			interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "g2";
+			clocks = <&clk IMX8MQ_CLK_VPU_G1_ROOT>,
+				 <&clk IMX8MQ_CLK_VPU_G2_ROOT>,
+				 <&clk IMX8MQ_CLK_VPU_DEC_ROOT>;
+			clock-names = "g1", "g2",  "bus";
+			assigned-clocks = <&clk IMX8MQ_CLK_VPU_G1>,
+					  <&clk IMX8MQ_CLK_VPU_G2>,
+					  <&clk IMX8MQ_CLK_VPU_BUS>,
+					  <&clk IMX8MQ_VPU_PLL_BYPASS>;
+			assigned-clock-parents = <&clk IMX8MQ_VPU_PLL_OUT>,
+						 <&clk IMX8MQ_VPU_PLL_OUT>,
+						 <&clk IMX8MQ_SYS1_PLL_800M>,
+						 <&clk IMX8MQ_VPU_PLL>;
+			assigned-clock-rates = <600000000>, <300000000>,
 					       <800000000>, <0>;
 			power-domains = <&pgc_vpu>;
+			nxp,imx8m-vpu-ctrl = <&vpu_ctrl>;
 		};
 
 		pcie0: pcie@33800000 {
-- 
2.30.2

Re: [PATCH v2 0/4] media: HEVC: RPS clean up

[Jernej Škrabec]

Hi Benjamin!

Dne torek, 12. oktober 2021 ob 16:35:48 CEST je Benjamin Gaignard napisal(a):
> This series aims to clean up Reference Picture Set usage and flags.
> 
> Long term flag was named with RPS prefix while it is not used for RPS
> but for mark long term references in DBP. Remane it and remove the two
> other useless RPS flags.
> 
> Clarify documentation about RPS lists content and make sure that Hantro
> driver use them correctly (i.e without look up in DBP).
> 
> These patches are the last in my backlog impacting HEVC uAPI.
> From my point of view, once they get merged, you could start talking
> about how move HEVC uAPI to stable.

With your changes, HEVC uAPI controls still won't be complete. Cedrus needs 
entry point control, which in turn needs dynamic array support. I'm a bit lazy 
implementing that control, but I guess I can take a look in a month or so. 
rkvdec also needs more fields for HEVC. With patches collected here:
https://github.com/LibreELEC/LibreELEC.tv/blob/master/projects/Rockchip/
patches/linux/default/linux-2001-v4l-wip-rkvdec-hevc.patch
fluster HEVC test score is reportedly 121/135 (8-bit tests only).

I would certainly wait with moving HEVC uAPI to stable.

Best regards,
Jernej

> 
> version 2:
> - change DPB field name from rps to flags
> 
> Please note that the only purpose of commits 3 and 4 is to allow to test
> G2 hardware block for IMX8MQ until a proper solution isuing power domain
> can be found. Do not merge them.
> 
> GStreamer HEVC plugin merge request can be found here:
> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1079
> 
> With those piece of code fluster score is 77/147.
> 
> Benjamin
> 
> Benjamin Gaignard (4):
>   media: hevc: Remove RPS named flags
>   media: hevc: Embedded indexes in RPS
>   media: hantro: Use syscon instead of 'ctrl' register
>   arm64: dts: imx8mq: Add node to G2 hardware
> 
>  .../media/v4l/ext-ctrls-codec.rst             | 14 +++---
>  arch/arm64/boot/dts/freescale/imx8mq.dtsi     | 43 +++++++++++++----
>  drivers/staging/media/hantro/hantro.h         |  5 +-
>  .../staging/media/hantro/hantro_g2_hevc_dec.c | 27 +++--------
>  drivers/staging/media/hantro/imx8m_vpu_hw.c   | 48 ++++++++++++-------
>  .../staging/media/sunxi/cedrus/cedrus_h265.c  |  2 +-
>  include/media/hevc-ctrls.h                    |  6 +--
>  7 files changed, 84 insertions(+), 61 deletions(-)
> 
> -- 
> 2.30.2
> 
> 

Re: [PATCH v2 0/4] media: HEVC: RPS clean up

[Benjamin Gaignard]

Le 12/10/2021 à 17:34, Jernej Škrabec a écrit :
> Hi Benjamin!
>
> Dne torek, 12. oktober 2021 ob 16:35:48 CEST je Benjamin Gaignard napisal(a):
>> This series aims to clean up Reference Picture Set usage and flags.
>>
>> Long term flag was named with RPS prefix while it is not used for RPS
>> but for mark long term references in DBP. Remane it and remove the two
>> other useless RPS flags.
>>
>> Clarify documentation about RPS lists content and make sure that Hantro
>> driver use them correctly (i.e without look up in DBP).
>>
>> These patches are the last in my backlog impacting HEVC uAPI.
>>  From my point of view, once they get merged, you could start talking
>> about how move HEVC uAPI to stable.
> With your changes, HEVC uAPI controls still won't be complete. Cedrus needs
> entry point control, which in turn needs dynamic array support. I'm a bit lazy
> implementing that control, but I guess I can take a look in a month or so.
> rkvdec also needs more fields for HEVC. With patches collected here:
> https://github.com/LibreELEC/LibreELEC.tv/blob/master/projects/Rockchip/
> patches/linux/default/linux-2001-v4l-wip-rkvdec-hevc.patch
> fluster HEVC test score is reportedly 121/135 (8-bit tests only).

Hi Jernej,

Thanks for your feedback, getting a list of missing items in HEVC uAPI
will definitively help to fill the hope.
The patch you mention for rkvdec are already merged in mainline kernel (at
least for uAPI part).
Cedrus needs are about num_entry_point_offsets, offset_len_minus1 and entry_point_offset_minus1[ i ]
in HEVC specifications ?

Regards,
Benjamin

>
> I would certainly wait with moving HEVC uAPI to stable.
>
> Best regards,
> Jernej
>
>> version 2:
>> - change DPB field name from rps to flags
>>
>> Please note that the only purpose of commits 3 and 4 is to allow to test
>> G2 hardware block for IMX8MQ until a proper solution isuing power domain
>> can be found. Do not merge them.
>>
>> GStreamer HEVC plugin merge request can be found here:
>> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1079
>>
>> With those piece of code fluster score is 77/147.
>>
>> Benjamin
>>
>> Benjamin Gaignard (4):
>>    media: hevc: Remove RPS named flags
>>    media: hevc: Embedded indexes in RPS
>>    media: hantro: Use syscon instead of 'ctrl' register
>>    arm64: dts: imx8mq: Add node to G2 hardware
>>
>>   .../media/v4l/ext-ctrls-codec.rst             | 14 +++---
>>   arch/arm64/boot/dts/freescale/imx8mq.dtsi     | 43 +++++++++++++----
>>   drivers/staging/media/hantro/hantro.h         |  5 +-
>>   .../staging/media/hantro/hantro_g2_hevc_dec.c | 27 +++--------
>>   drivers/staging/media/hantro/imx8m_vpu_hw.c   | 48 ++++++++++++-------
>>   .../staging/media/sunxi/cedrus/cedrus_h265.c  |  2 +-
>>   include/media/hevc-ctrls.h                    |  6 +--
>>   7 files changed, 84 insertions(+), 61 deletions(-)
>>
>> -- 
>> 2.30.2
>>
>>
>

Re: Re: [PATCH v2 0/4] media: HEVC: RPS clean up

[Jernej Škrabec]

CC: Alex Bee

Alex, please take a look to these patches too.

Dne torek, 12. oktober 2021 ob 17:57:50 CEST je Benjamin Gaignard napisal(a):
> 
> Le 12/10/2021 à 17:34, Jernej Škrabec a écrit :
> > Hi Benjamin!
> >
> > Dne torek, 12. oktober 2021 ob 16:35:48 CEST je Benjamin Gaignard 
napisal(a):
> >> This series aims to clean up Reference Picture Set usage and flags.
> >>
> >> Long term flag was named with RPS prefix while it is not used for RPS
> >> but for mark long term references in DBP. Remane it and remove the two
> >> other useless RPS flags.
> >>
> >> Clarify documentation about RPS lists content and make sure that Hantro
> >> driver use them correctly (i.e without look up in DBP).
> >>
> >> These patches are the last in my backlog impacting HEVC uAPI.
> >>  From my point of view, once they get merged, you could start talking
> >> about how move HEVC uAPI to stable.
> > With your changes, HEVC uAPI controls still won't be complete. Cedrus 
needs
> > entry point control, which in turn needs dynamic array support. I'm a bit 
lazy
> > implementing that control, but I guess I can take a look in a month or so.
> > rkvdec also needs more fields for HEVC. With patches collected here:
> > https://github.com/LibreELEC/LibreELEC.tv/blob/master/projects/Rockchip/
> > patches/linux/default/linux-2001-v4l-wip-rkvdec-hevc.patch
> > fluster HEVC test score is reportedly 121/135 (8-bit tests only).
> 
> Hi Jernej,
> 
> Thanks for your feedback, getting a list of missing items in HEVC uAPI
> will definitively help to fill the hope.
> The patch you mention for rkvdec are already merged in mainline kernel (at
> least for uAPI part).

Are they? What about:
video_parameter_set_id
seq_parameter_set_id
pic_parameter_set_id
short_term_ref_pic_set_size
long_term_ref_pic_set_size

At least I don't see them in linux-next. Maybe that information can be 
obtained in some other way?

> Cedrus needs are about num_entry_point_offsets, offset_len_minus1 and 
entry_point_offset_minus1[ i ]
> in HEVC specifications ?

Yes, Cedrus needs to know whole list of entry points. I don't think we need to 
worry about offset_len_minus1, list could be pre-processed - just number of 
entry points and their values.

Best regards,
Jernej

> 
> Regards,
> Benjamin
> 
> >
> > I would certainly wait with moving HEVC uAPI to stable.
> >
> > Best regards,
> > Jernej
> >
> >> version 2:
> >> - change DPB field name from rps to flags
> >>
> >> Please note that the only purpose of commits 3 and 4 is to allow to test
> >> G2 hardware block for IMX8MQ until a proper solution isuing power domain
> >> can be found. Do not merge them.
> >>
> >> GStreamer HEVC plugin merge request can be found here:
> >> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1079
> >>
> >> With those piece of code fluster score is 77/147.
> >>
> >> Benjamin
> >>
> >> Benjamin Gaignard (4):
> >>    media: hevc: Remove RPS named flags
> >>    media: hevc: Embedded indexes in RPS
> >>    media: hantro: Use syscon instead of 'ctrl' register
> >>    arm64: dts: imx8mq: Add node to G2 hardware
> >>
> >>   .../media/v4l/ext-ctrls-codec.rst             | 14 +++---
> >>   arch/arm64/boot/dts/freescale/imx8mq.dtsi     | 43 +++++++++++++----
> >>   drivers/staging/media/hantro/hantro.h         |  5 +-
> >>   .../staging/media/hantro/hantro_g2_hevc_dec.c | 27 +++--------
> >>   drivers/staging/media/hantro/imx8m_vpu_hw.c   | 48 ++++++++++++-------
> >>   .../staging/media/sunxi/cedrus/cedrus_h265.c  |  2 +-
> >>   include/media/hevc-ctrls.h                    |  6 +--
> >>   7 files changed, 84 insertions(+), 61 deletions(-)
> >>
> >> -- 
> >> 2.30.2
> >>
> >>
> >
> 

Re: [PATCH v2 0/4] media: HEVC: RPS clean up

[Alex Bee]

Hi Benjamin, Jernej
Am 12.10.21 um 18:08 schrieb Jernej Škrabec:
> CC: Alex Bee
> 
> Alex, please take a look to these patches too.
These patches don't remove anything that would be need for rkvdec hevc - 
but indeed - we need some more:
https://github.com/LibreELEC/LibreELEC.tv/blob/master/projects/Rockchip/patches/linux/default/linux-2001-v4l-wip-rkvdec-hevc.patch#L242-L305

v4l2_ctrl_hevc_sps:
__u8	video_parameter_set_id
__u8	seq_parameter_set_id

v4l2_ctrl_hevc_pps:
__u8	pic_parameter_set_id
__u16	short_term_ref_pic_set_size
__u16	long_term_ref_pic_set_size

As far as I can see, they are all part of the spec and should be 
therefore good to go in the uapi.

As you might now, even rkvdec is a frame-based decoder, it doesn't fully 
parse slice headers in HW for HEVC and we need to set references in SW 
which requires looping over the slices. Downstream we have a hack to 
give num_slices in v4l2_ctrl_hevc_sps for doing that.
That could fully go away, if V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS could 
get dynamic array control support and would make upstreaming this a lot 
easier - as far as I'm concered this would be required for RPi HEVC 
decoder as well.
As a last resort we could also implement a HW specifc control à la
V4L2_CID_HANTRO_HEVC_SLICE_HEADER_SKIP - but I'd like to avoid that, 
knowing it would certainly be better from performance pov.

Alex.
> 
> Dne torek, 12. oktober 2021 ob 17:57:50 CEST je Benjamin Gaignard napisal(a):
>>
>> Le 12/10/2021 à 17:34, Jernej Škrabec a écrit :
>>> Hi Benjamin!
>>>
>>> Dne torek, 12. oktober 2021 ob 16:35:48 CEST je Benjamin Gaignard
> napisal(a):
>>>> This series aims to clean up Reference Picture Set usage and flags.
>>>>
>>>> Long term flag was named with RPS prefix while it is not used for RPS
>>>> but for mark long term references in DBP. Remane it and remove the two
>>>> other useless RPS flags.
>>>>
>>>> Clarify documentation about RPS lists content and make sure that Hantro
>>>> driver use them correctly (i.e without look up in DBP).
>>>>
>>>> These patches are the last in my backlog impacting HEVC uAPI.
>>>>   From my point of view, once they get merged, you could start talking
>>>> about how move HEVC uAPI to stable.
>>> With your changes, HEVC uAPI controls still won't be complete. Cedrus
> needs
>>> entry point control, which in turn needs dynamic array support. I'm a bit
> lazy
>>> implementing that control, but I guess I can take a look in a month or so.
>>> rkvdec also needs more fields for HEVC. With patches collected here:
>>> https://github.com/LibreELEC/LibreELEC.tv/blob/master/projects/Rockchip/
>>> patches/linux/default/linux-2001-v4l-wip-rkvdec-hevc.patch
>>> fluster HEVC test score is reportedly 121/135 (8-bit tests only).
>>
>> Hi Jernej,
>>
>> Thanks for your feedback, getting a list of missing items in HEVC uAPI
>> will definitively help to fill the hope.
>> The patch you mention for rkvdec are already merged in mainline kernel (at
>> least for uAPI part).
> 
> Are they? What about:
> video_parameter_set_id
> seq_parameter_set_id
> pic_parameter_set_id
> short_term_ref_pic_set_size
> long_term_ref_pic_set_size
> 
> At least I don't see them in linux-next. Maybe that information can be
> obtained in some other way?
> 
>> Cedrus needs are about num_entry_point_offsets, offset_len_minus1 and
> entry_point_offset_minus1[ i ]
>> in HEVC specifications ?
> 
> Yes, Cedrus needs to know whole list of entry points. I don't think we need to
> worry about offset_len_minus1, list could be pre-processed - just number of
> entry points and their values.
> 
> Best regards,
> Jernej
> 
>>
>> Regards,
>> Benjamin
>>
>>>
>>> I would certainly wait with moving HEVC uAPI to stable.
>>>
>>> Best regards,
>>> Jernej
>>>
>>>> version 2:
>>>> - change DPB field name from rps to flags
>>>>
>>>> Please note that the only purpose of commits 3 and 4 is to allow to test
>>>> G2 hardware block for IMX8MQ until a proper solution isuing power domain
>>>> can be found. Do not merge them.
>>>>
>>>> GStreamer HEVC plugin merge request can be found here:
>>>> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1079
>>>>
>>>> With those piece of code fluster score is 77/147.
>>>>
>>>> Benjamin
>>>>
>>>> Benjamin Gaignard (4):
>>>>     media: hevc: Remove RPS named flags
>>>>     media: hevc: Embedded indexes in RPS
>>>>     media: hantro: Use syscon instead of 'ctrl' register
>>>>     arm64: dts: imx8mq: Add node to G2 hardware
>>>>
>>>>    .../media/v4l/ext-ctrls-codec.rst             | 14 +++---
>>>>    arch/arm64/boot/dts/freescale/imx8mq.dtsi     | 43 +++++++++++++----
>>>>    drivers/staging/media/hantro/hantro.h         |  5 +-
>>>>    .../staging/media/hantro/hantro_g2_hevc_dec.c | 27 +++--------
>>>>    drivers/staging/media/hantro/imx8m_vpu_hw.c   | 48 ++++++++++++-------
>>>>    .../staging/media/sunxi/cedrus/cedrus_h265.c  |  2 +-
>>>>    include/media/hevc-ctrls.h                    |  6 +--
>>>>    7 files changed, 84 insertions(+), 61 deletions(-)
>>>>
>>>> -- 
>>>> 2.30.2
>>>>
>>>>
>>>
>>
> 
> 

Re: [PATCH v2 0/4] media: HEVC: RPS clean up

[Benjamin Gaignard]

Le 15/10/2021 à 12:33, Alex Bee a écrit :
> Hi Benjamin, Jernej
> Am 12.10.21 um 18:08 schrieb Jernej Škrabec:
>> CC: Alex Bee
>>
>> Alex, please take a look to these patches too.
> These patches don't remove anything that would be need for rkvdec hevc 
> - but indeed - we need some more:
> https://github.com/LibreELEC/LibreELEC.tv/blob/master/projects/Rockchip/patches/linux/default/linux-2001-v4l-wip-rkvdec-hevc.patch#L242-L305 
>
>
> v4l2_ctrl_hevc_sps:
> __u8    video_parameter_set_id
> __u8    seq_parameter_set_id
>
> v4l2_ctrl_hevc_pps:
> __u8    pic_parameter_set_id
> __u16    short_term_ref_pic_set_size
> __u16    long_term_ref_pic_set_size
>
> As far as I can see, they are all part of the spec and should be 
> therefore good to go in the uapi.

Do you have any plan to upstream these fields in HEVC uAPI ?

Regards,
Benjamin

>
> As you might now, even rkvdec is a frame-based decoder, it doesn't 
> fully parse slice headers in HW for HEVC and we need to set references 
> in SW which requires looping over the slices. Downstream we have a 
> hack to give num_slices in v4l2_ctrl_hevc_sps for doing that.
> That could fully go away, if V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS 
> could get dynamic array control support and would make upstreaming 
> this a lot easier - as far as I'm concered this would be required for 
> RPi HEVC decoder as well.
> As a last resort we could also implement a HW specifc control à la
> V4L2_CID_HANTRO_HEVC_SLICE_HEADER_SKIP - but I'd like to avoid that, 
> knowing it would certainly be better from performance pov.
>
> Alex.
>>
>> Dne torek, 12. oktober 2021 ob 17:57:50 CEST je Benjamin Gaignard 
>> napisal(a):
>>>
>>> Le 12/10/2021 à 17:34, Jernej Škrabec a écrit :
>>>> Hi Benjamin!
>>>>
>>>> Dne torek, 12. oktober 2021 ob 16:35:48 CEST je Benjamin Gaignard
>> napisal(a):
>>>>> This series aims to clean up Reference Picture Set usage and flags.
>>>>>
>>>>> Long term flag was named with RPS prefix while it is not used for RPS
>>>>> but for mark long term references in DBP. Remane it and remove the 
>>>>> two
>>>>> other useless RPS flags.
>>>>>
>>>>> Clarify documentation about RPS lists content and make sure that 
>>>>> Hantro
>>>>> driver use them correctly (i.e without look up in DBP).
>>>>>
>>>>> These patches are the last in my backlog impacting HEVC uAPI.
>>>>>   From my point of view, once they get merged, you could start 
>>>>> talking
>>>>> about how move HEVC uAPI to stable.
>>>> With your changes, HEVC uAPI controls still won't be complete. Cedrus
>> needs
>>>> entry point control, which in turn needs dynamic array support. I'm 
>>>> a bit
>> lazy
>>>> implementing that control, but I guess I can take a look in a month 
>>>> or so.
>>>> rkvdec also needs more fields for HEVC. With patches collected here:
>>>> https://github.com/LibreELEC/LibreELEC.tv/blob/master/projects/Rockchip/ 
>>>>
>>>> patches/linux/default/linux-2001-v4l-wip-rkvdec-hevc.patch
>>>> fluster HEVC test score is reportedly 121/135 (8-bit tests only).
>>>
>>> Hi Jernej,
>>>
>>> Thanks for your feedback, getting a list of missing items in HEVC uAPI
>>> will definitively help to fill the hope.
>>> The patch you mention for rkvdec are already merged in mainline 
>>> kernel (at
>>> least for uAPI part).
>>
>> Are they? What about:
>> video_parameter_set_id
>> seq_parameter_set_id
>> pic_parameter_set_id
>> short_term_ref_pic_set_size
>> long_term_ref_pic_set_size
>>
>> At least I don't see them in linux-next. Maybe that information can be
>> obtained in some other way?
>>
>>> Cedrus needs are about num_entry_point_offsets, offset_len_minus1 and
>> entry_point_offset_minus1[ i ]
>>> in HEVC specifications ?
>>
>> Yes, Cedrus needs to know whole list of entry points. I don't think 
>> we need to
>> worry about offset_len_minus1, list could be pre-processed - just 
>> number of
>> entry points and their values.
>>
>> Best regards,
>> Jernej
>>
>>>
>>> Regards,
>>> Benjamin
>>>
>>>>
>>>> I would certainly wait with moving HEVC uAPI to stable.
>>>>
>>>> Best regards,
>>>> Jernej
>>>>
>>>>> version 2:
>>>>> - change DPB field name from rps to flags
>>>>>
>>>>> Please note that the only purpose of commits 3 and 4 is to allow 
>>>>> to test
>>>>> G2 hardware block for IMX8MQ until a proper solution isuing power 
>>>>> domain
>>>>> can be found. Do not merge them.
>>>>>
>>>>> GStreamer HEVC plugin merge request can be found here:
>>>>> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1079 
>>>>>
>>>>>
>>>>> With those piece of code fluster score is 77/147.
>>>>>
>>>>> Benjamin
>>>>>
>>>>> Benjamin Gaignard (4):
>>>>>     media: hevc: Remove RPS named flags
>>>>>     media: hevc: Embedded indexes in RPS
>>>>>     media: hantro: Use syscon instead of 'ctrl' register
>>>>>     arm64: dts: imx8mq: Add node to G2 hardware
>>>>>
>>>>>    .../media/v4l/ext-ctrls-codec.rst             | 14 +++---
>>>>>    arch/arm64/boot/dts/freescale/imx8mq.dtsi     | 43 
>>>>> +++++++++++++----
>>>>>    drivers/staging/media/hantro/hantro.h         |  5 +-
>>>>>    .../staging/media/hantro/hantro_g2_hevc_dec.c | 27 +++--------
>>>>>    drivers/staging/media/hantro/imx8m_vpu_hw.c   | 48 
>>>>> ++++++++++++-------
>>>>>    .../staging/media/sunxi/cedrus/cedrus_h265.c  |  2 +-
>>>>>    include/media/hevc-ctrls.h                    |  6 +--
>>>>>    7 files changed, 84 insertions(+), 61 deletions(-)
>>>>>
>>>>> -- 
>>>>> 2.30.2
>>>>>
>>>>>
>>>>
>>>
>>
>>
>

Re: [PATCH v2 0/4] media: HEVC: RPS clean up

[Alex Bee]

Am 15.10.21 um 16:06 schrieb Benjamin Gaignard:
>
> Le 15/10/2021 à 12:33, Alex Bee a écrit :
>> Hi Benjamin, Jernej
>> Am 12.10.21 um 18:08 schrieb Jernej Škrabec:
>>> CC: Alex Bee
>>>
>>> Alex, please take a look to these patches too.
>> These patches don't remove anything that would be need for rkvdec 
>> hevc - but indeed - we need some more:
>> https://github.com/LibreELEC/LibreELEC.tv/blob/master/projects/Rockchip/patches/linux/default/linux-2001-v4l-wip-rkvdec-hevc.patch#L242-L305 
>>
>>
>> v4l2_ctrl_hevc_sps:
>> __u8    video_parameter_set_id
>> __u8    seq_parameter_set_id
>>
>> v4l2_ctrl_hevc_pps:
>> __u8    pic_parameter_set_id
>> __u16    short_term_ref_pic_set_size
>> __u16    long_term_ref_pic_set_size
>>
>> As far as I can see, they are all part of the spec and should be 
>> therefore good to go in the uapi.
>
> Do you have any plan to upstream these fields in HEVC uAPI ?

I might be upstreaming them at some point, yes.

With this I just wanted to underline Jernej said: HEVC uapi is NOT ready 
to get unstaged yet.

>
> Regards,
> Benjamin
>
>>
>> As you might now, even rkvdec is a frame-based decoder, it doesn't 
>> fully parse slice headers in HW for HEVC and we need to set 
>> references in SW which requires looping over the slices. Downstream 
>> we have a hack to give num_slices in v4l2_ctrl_hevc_sps for doing that.
>> That could fully go away, if V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS 
>> could get dynamic array control support and would make upstreaming 
>> this a lot easier - as far as I'm concered this would be required for 
>> RPi HEVC decoder as well.
>> As a last resort we could also implement a HW specifc control à la
>> V4L2_CID_HANTRO_HEVC_SLICE_HEADER_SKIP - but I'd like to avoid that, 
>> knowing it would certainly be better from performance pov.
>>
>> Alex.
>>>
>>> Dne torek, 12. oktober 2021 ob 17:57:50 CEST je Benjamin Gaignard 
>>> napisal(a):
>>>>
>>>> Le 12/10/2021 à 17:34, Jernej Škrabec a écrit :
>>>>> Hi Benjamin!
>>>>>
>>>>> Dne torek, 12. oktober 2021 ob 16:35:48 CEST je Benjamin Gaignard
>>> napisal(a):
>>>>>> This series aims to clean up Reference Picture Set usage and flags.
>>>>>>
>>>>>> Long term flag was named with RPS prefix while it is not used for 
>>>>>> RPS
>>>>>> but for mark long term references in DBP. Remane it and remove 
>>>>>> the two
>>>>>> other useless RPS flags.
>>>>>>
>>>>>> Clarify documentation about RPS lists content and make sure that 
>>>>>> Hantro
>>>>>> driver use them correctly (i.e without look up in DBP).
>>>>>>
>>>>>> These patches are the last in my backlog impacting HEVC uAPI.
>>>>>>   From my point of view, once they get merged, you could start 
>>>>>> talking
>>>>>> about how move HEVC uAPI to stable.
>>>>> With your changes, HEVC uAPI controls still won't be complete. Cedrus
>>> needs
>>>>> entry point control, which in turn needs dynamic array support. 
>>>>> I'm a bit
>>> lazy
>>>>> implementing that control, but I guess I can take a look in a 
>>>>> month or so.
>>>>> rkvdec also needs more fields for HEVC. With patches collected here:
>>>>> https://github.com/LibreELEC/LibreELEC.tv/blob/master/projects/Rockchip/ 
>>>>>
>>>>> patches/linux/default/linux-2001-v4l-wip-rkvdec-hevc.patch
>>>>> fluster HEVC test score is reportedly 121/135 (8-bit tests only).
>>>>
>>>> Hi Jernej,
>>>>
>>>> Thanks for your feedback, getting a list of missing items in HEVC uAPI
>>>> will definitively help to fill the hope.
>>>> The patch you mention for rkvdec are already merged in mainline 
>>>> kernel (at
>>>> least for uAPI part).
>>>
>>> Are they? What about:
>>> video_parameter_set_id
>>> seq_parameter_set_id
>>> pic_parameter_set_id
>>> short_term_ref_pic_set_size
>>> long_term_ref_pic_set_size
>>>
>>> At least I don't see them in linux-next. Maybe that information can be
>>> obtained in some other way?
>>>
>>>> Cedrus needs are about num_entry_point_offsets, offset_len_minus1 and
>>> entry_point_offset_minus1[ i ]
>>>> in HEVC specifications ?
>>>
>>> Yes, Cedrus needs to know whole list of entry points. I don't think 
>>> we need to
>>> worry about offset_len_minus1, list could be pre-processed - just 
>>> number of
>>> entry points and their values.
>>>
>>> Best regards,
>>> Jernej
>>>
>>>>
>>>> Regards,
>>>> Benjamin
>>>>
>>>>>
>>>>> I would certainly wait with moving HEVC uAPI to stable.
>>>>>
>>>>> Best regards,
>>>>> Jernej
>>>>>
>>>>>> version 2:
>>>>>> - change DPB field name from rps to flags
>>>>>>
>>>>>> Please note that the only purpose of commits 3 and 4 is to allow 
>>>>>> to test
>>>>>> G2 hardware block for IMX8MQ until a proper solution isuing power 
>>>>>> domain
>>>>>> can be found. Do not merge them.
>>>>>>
>>>>>> GStreamer HEVC plugin merge request can be found here:
>>>>>> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1079 
>>>>>>
>>>>>>
>>>>>> With those piece of code fluster score is 77/147.
>>>>>>
>>>>>> Benjamin
>>>>>>
>>>>>> Benjamin Gaignard (4):
>>>>>>     media: hevc: Remove RPS named flags
>>>>>>     media: hevc: Embedded indexes in RPS
>>>>>>     media: hantro: Use syscon instead of 'ctrl' register
>>>>>>     arm64: dts: imx8mq: Add node to G2 hardware
>>>>>>
>>>>>>    .../media/v4l/ext-ctrls-codec.rst             | 14 +++---
>>>>>>    arch/arm64/boot/dts/freescale/imx8mq.dtsi     | 43 
>>>>>> +++++++++++++----
>>>>>>    drivers/staging/media/hantro/hantro.h         |  5 +-
>>>>>>    .../staging/media/hantro/hantro_g2_hevc_dec.c | 27 +++--------
>>>>>>    drivers/staging/media/hantro/imx8m_vpu_hw.c   | 48 
>>>>>> ++++++++++++-------
>>>>>>    .../staging/media/sunxi/cedrus/cedrus_h265.c  |  2 +-
>>>>>>    include/media/hevc-ctrls.h                    |  6 +--
>>>>>>    7 files changed, 84 insertions(+), 61 deletions(-)
>>>>>>
>>>>>> -- 
>>>>>> 2.30.2
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>>
>>

Re: [PATCH v2 0/4] media: HEVC: RPS clean up

[Benjamin Gaignard]

Le 15/10/2021 à 16:14, Alex Bee a écrit :
>
> Am 15.10.21 um 16:06 schrieb Benjamin Gaignard:
>>
>> Le 15/10/2021 à 12:33, Alex Bee a écrit :
>>> Hi Benjamin, Jernej
>>> Am 12.10.21 um 18:08 schrieb Jernej Škrabec:
>>>> CC: Alex Bee
>>>>
>>>> Alex, please take a look to these patches too.
>>> These patches don't remove anything that would be need for rkvdec 
>>> hevc - but indeed - we need some more:
>>> https://github.com/LibreELEC/LibreELEC.tv/blob/master/projects/Rockchip/patches/linux/default/linux-2001-v4l-wip-rkvdec-hevc.patch#L242-L305 
>>>
>>>
>>> v4l2_ctrl_hevc_sps:
>>> __u8    video_parameter_set_id
>>> __u8    seq_parameter_set_id
>>>
>>> v4l2_ctrl_hevc_pps:
>>> __u8    pic_parameter_set_id
>>> __u16    short_term_ref_pic_set_size
>>> __u16    long_term_ref_pic_set_size
>>>
>>> As far as I can see, they are all part of the spec and should be 
>>> therefore good to go in the uapi.
>>
>> Do you have any plan to upstream these fields in HEVC uAPI ?
>
> I might be upstreaming them at some point, yes.
>
> With this I just wanted to underline Jernej said: HEVC uapi is NOT ready
> to get unstaged yet.

Ok but the question is how to get it unstaged ?
Should we continue to do changes in staged uAPI ?
or send a RFC to move it to stable and review all the missing parts at that time ?

Regards,
Benjamin

>
>>
>> Regards,
>> Benjamin
>>
>>>
>>> As you might now, even rkvdec is a frame-based decoder, it doesn't 
>>> fully parse slice headers in HW for HEVC and we need to set 
>>> references in SW which requires looping over the slices. Downstream 
>>> we have a hack to give num_slices in v4l2_ctrl_hevc_sps for doing that.
>>> That could fully go away, if V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS 
>>> could get dynamic array control support and would make upstreaming 
>>> this a lot easier - as far as I'm concered this would be required 
>>> for RPi HEVC decoder as well.
>>> As a last resort we could also implement a HW specifc control à la
>>> V4L2_CID_HANTRO_HEVC_SLICE_HEADER_SKIP - but I'd like to avoid that, 
>>> knowing it would certainly be better from performance pov.
>>>
>>> Alex.
>>>>
>>>> Dne torek, 12. oktober 2021 ob 17:57:50 CEST je Benjamin Gaignard 
>>>> napisal(a):
>>>>>
>>>>> Le 12/10/2021 à 17:34, Jernej Škrabec a écrit :
>>>>>> Hi Benjamin!
>>>>>>
>>>>>> Dne torek, 12. oktober 2021 ob 16:35:48 CEST je Benjamin Gaignard
>>>> napisal(a):
>>>>>>> This series aims to clean up Reference Picture Set usage and flags.
>>>>>>>
>>>>>>> Long term flag was named with RPS prefix while it is not used 
>>>>>>> for RPS
>>>>>>> but for mark long term references in DBP. Remane it and remove 
>>>>>>> the two
>>>>>>> other useless RPS flags.
>>>>>>>
>>>>>>> Clarify documentation about RPS lists content and make sure that 
>>>>>>> Hantro
>>>>>>> driver use them correctly (i.e without look up in DBP).
>>>>>>>
>>>>>>> These patches are the last in my backlog impacting HEVC uAPI.
>>>>>>>   From my point of view, once they get merged, you could start 
>>>>>>> talking
>>>>>>> about how move HEVC uAPI to stable.
>>>>>> With your changes, HEVC uAPI controls still won't be complete. 
>>>>>> Cedrus
>>>> needs
>>>>>> entry point control, which in turn needs dynamic array support. 
>>>>>> I'm a bit
>>>> lazy
>>>>>> implementing that control, but I guess I can take a look in a 
>>>>>> month or so.
>>>>>> rkvdec also needs more fields for HEVC. With patches collected here:
>>>>>> https://github.com/LibreELEC/LibreELEC.tv/blob/master/projects/Rockchip/ 
>>>>>>
>>>>>> patches/linux/default/linux-2001-v4l-wip-rkvdec-hevc.patch
>>>>>> fluster HEVC test score is reportedly 121/135 (8-bit tests only).
>>>>>
>>>>> Hi Jernej,
>>>>>
>>>>> Thanks for your feedback, getting a list of missing items in HEVC 
>>>>> uAPI
>>>>> will definitively help to fill the hope.
>>>>> The patch you mention for rkvdec are already merged in mainline 
>>>>> kernel (at
>>>>> least for uAPI part).
>>>>
>>>> Are they? What about:
>>>> video_parameter_set_id
>>>> seq_parameter_set_id
>>>> pic_parameter_set_id
>>>> short_term_ref_pic_set_size
>>>> long_term_ref_pic_set_size
>>>>
>>>> At least I don't see them in linux-next. Maybe that information can be
>>>> obtained in some other way?
>>>>
>>>>> Cedrus needs are about num_entry_point_offsets, offset_len_minus1 and
>>>> entry_point_offset_minus1[ i ]
>>>>> in HEVC specifications ?
>>>>
>>>> Yes, Cedrus needs to know whole list of entry points. I don't think 
>>>> we need to
>>>> worry about offset_len_minus1, list could be pre-processed - just 
>>>> number of
>>>> entry points and their values.
>>>>
>>>> Best regards,
>>>> Jernej
>>>>
>>>>>
>>>>> Regards,
>>>>> Benjamin
>>>>>
>>>>>>
>>>>>> I would certainly wait with moving HEVC uAPI to stable.
>>>>>>
>>>>>> Best regards,
>>>>>> Jernej
>>>>>>
>>>>>>> version 2:
>>>>>>> - change DPB field name from rps to flags
>>>>>>>
>>>>>>> Please note that the only purpose of commits 3 and 4 is to allow 
>>>>>>> to test
>>>>>>> G2 hardware block for IMX8MQ until a proper solution isuing 
>>>>>>> power domain
>>>>>>> can be found. Do not merge them.
>>>>>>>
>>>>>>> GStreamer HEVC plugin merge request can be found here:
>>>>>>> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1079 
>>>>>>>
>>>>>>>
>>>>>>> With those piece of code fluster score is 77/147.
>>>>>>>
>>>>>>> Benjamin
>>>>>>>
>>>>>>> Benjamin Gaignard (4):
>>>>>>>     media: hevc: Remove RPS named flags
>>>>>>>     media: hevc: Embedded indexes in RPS
>>>>>>>     media: hantro: Use syscon instead of 'ctrl' register
>>>>>>>     arm64: dts: imx8mq: Add node to G2 hardware
>>>>>>>
>>>>>>>    .../media/v4l/ext-ctrls-codec.rst             | 14 +++---
>>>>>>>    arch/arm64/boot/dts/freescale/imx8mq.dtsi     | 43 
>>>>>>> +++++++++++++----
>>>>>>>    drivers/staging/media/hantro/hantro.h         |  5 +-
>>>>>>>    .../staging/media/hantro/hantro_g2_hevc_dec.c | 27 +++--------
>>>>>>>    drivers/staging/media/hantro/imx8m_vpu_hw.c   | 48 
>>>>>>> ++++++++++++-------
>>>>>>>    .../staging/media/sunxi/cedrus/cedrus_h265.c  |  2 +-
>>>>>>>    include/media/hevc-ctrls.h                    |  6 +--
>>>>>>>    7 files changed, 84 insertions(+), 61 deletions(-)
>>>>>>>
>>>>>>> -- 
>>>>>>> 2.30.2
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>

Re: [PATCH v2 0/4] media: HEVC: RPS clean up

[Benjamin Gaignard]

Le 12/10/2021 à 16:35, Benjamin Gaignard a écrit :
> This series aims to clean up Reference Picture Set usage and flags.
>
> Long term flag was named with RPS prefix while it is not used for RPS
> but for mark long term references in DBP. Remane it and remove the two
> other useless RPS flags.
>
> Clarify documentation about RPS lists content and make sure that Hantro
> driver use them correctly (i.e without look up in DBP).

This series generates feedback about what is missing to move HEVC uAPI
out of staging but lack of remarks about the proposed changes themselves.
Does someone has an opinion about this RPS flag clean up ?

Regards,
Benjamin

>
> These patches are the last in my backlog impacting HEVC uAPI.
>  From my point of view, once they get merged, you could start talking
> about how move HEVC uAPI to stable.
>
> version 2:
> - change DPB field name from rps to flags
>
> Please note that the only purpose of commits 3 and 4 is to allow to test
> G2 hardware block for IMX8MQ until a proper solution isuing power domain
> can be found. Do not merge them.
>
> GStreamer HEVC plugin merge request can be found here:
> https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/1079
>
> With those piece of code fluster score is 77/147.
>
> Benjamin
>
> Benjamin Gaignard (4):
>    media: hevc: Remove RPS named flags
>    media: hevc: Embedded indexes in RPS
>    media: hantro: Use syscon instead of 'ctrl' register
>    arm64: dts: imx8mq: Add node to G2 hardware
>
>   .../media/v4l/ext-ctrls-codec.rst             | 14 +++---
>   arch/arm64/boot/dts/freescale/imx8mq.dtsi     | 43 +++++++++++++----
>   drivers/staging/media/hantro/hantro.h         |  5 +-
>   .../staging/media/hantro/hantro_g2_hevc_dec.c | 27 +++--------
>   drivers/staging/media/hantro/imx8m_vpu_hw.c   | 48 ++++++++++++-------
>   .../staging/media/sunxi/cedrus/cedrus_h265.c  |  2 +-
>   include/media/hevc-ctrls.h                    |  6 +--
>   7 files changed, 84 insertions(+), 61 deletions(-)
>

Re: [PATCH v2 1/4] media: hevc: Remove RPS named flags

[Jernej Škrabec]

Hi Benjamin,

Dne torek, 12. oktober 2021 ob 16:35:49 CET je Benjamin Gaignard napisal(a):
> Marking a picture as long-term reference is valid for DPB but not for RPS.
> Change flag name to match with it description in HEVC spec chapiter

chapiter -> chapter

> "8.3.2 Decoding process for reference picture set".
> Remove the other unused RPS flags.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
> version 2:
> - change DPB field name from rps to flags
> 
>  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 8 +++-----
>  drivers/staging/media/hantro/hantro_g2_hevc_dec.c         | 2 +-
>  drivers/staging/media/sunxi/cedrus/cedrus_h265.c          | 2 +-
>  include/media/hevc-ctrls.h                                | 6 ++----
>  4 files changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/
Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> index e141f0e4eec9..38da33e61c3d 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
> @@ -3166,11 +3166,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>  	:c:func:`v4l2_timeval_to_ns()` function to convert the struct
>  	:c:type:`timeval` in struct :c:type:`v4l2_buffer` to a __u64.
>      * - __u8
> -      - ``rps``
> -      - The reference set for the reference frame
> -        (V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_BEFORE,
> -        V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_AFTER or
> -        V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR)
> +      - ``flags``
> +      - Long term flag for the reference frame
> +        (V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE)
>      * - __u8
>        - ``field_pic``
>        - Whether the reference is a field picture or a frame.
> diff --git a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c b/drivers/
staging/media/hantro/hantro_g2_hevc_dec.c
> index 76a921163b9a..bbf71dcea099 100644
> --- a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
> @@ -445,7 +445,7 @@ static int set_ref(struct hantro_ctx *ctx)
>  		chroma_addr = luma_addr + cr_offset;
>  		mv_addr = luma_addr + mv_offset;
>  
> -		if (dpb[i].rps == V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR)
> +		if (dpb[i].flags == 
V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE)
>  			dpb_longterm_e |= 
BIT(V4L2_HEVC_DPB_ENTRIES_NUM_MAX - 1 - i);
>  
>  		hantro_write_addr(vpu, G2_REG_ADDR_REF(i), luma_addr);
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/
staging/media/sunxi/cedrus/cedrus_h265.c
> index 3d9561d4aadb..1edfc3823a57 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> @@ -169,7 +169,7 @@ static void cedrus_h265_ref_pic_list_write(struct 
cedrus_dev *dev,
>  		unsigned int index = list[i];
>  		u8 value = list[i];
>  
> -		if (dpb[index].rps == V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR)
> +		if (dpb[index].flags == 
V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE)

Flags are usually checked with "&", same goes in Hantro case.

Once all fixed, this patch is:
Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

>  			value |= 
VE_DEC_H265_SRAM_REF_PIC_LIST_LT_REF;
>  
>  		/* Each SRAM word gathers up to 4 references. */
> diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h
> index ef63bc205756..01ccda48d8c5 100644
> --- a/include/media/hevc-ctrls.h
> +++ b/include/media/hevc-ctrls.h
> @@ -127,15 +127,13 @@ struct v4l2_ctrl_hevc_pps {
>  	__u64	flags;
>  };
>  
> -#define V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_BEFORE	0x01
> -#define V4L2_HEVC_DPB_ENTRY_RPS_ST_CURR_AFTER	0x02
> -#define V4L2_HEVC_DPB_ENTRY_RPS_LT_CURR		0x03
> +#define V4L2_HEVC_DPB_ENTRY_LONG_TERM_REFERENCE	0x01
>  
>  #define V4L2_HEVC_DPB_ENTRIES_NUM_MAX		16
>  
>  struct v4l2_hevc_dpb_entry {
>  	__u64	timestamp;
> -	__u8	rps;
> +	__u8	flags;
>  	__u8	field_pic;
>  	__u16	pic_order_cnt[2];
>  	__u8	padding[2];
> -- 
> 2.30.2
> 
>