All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/7] media: hantro: add Allwinner H6 support
@ 2021-11-22 18:46 ` Jernej Skrabec
  0 siblings, 0 replies; 40+ messages in thread
From: Jernej Skrabec @ 2021-11-22 18:46 UTC (permalink / raw)
  To: linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, andrzej.p, gregkh, devicetree, linux-arm-kernel,
	linux-sunxi, linux-kernel, linux-staging, Jernej Skrabec

Hi everyone!

Here is patchset which adds support for Hantro G2 core found in Allwinner
H6 SoC. It is slightly older core, so it needs few quirks to be
implemented:
1. It uses slightly different register layout in some cases. However, those
   differences are small, so it makes sense only to add quirks.
2. It doesn't use ring buffer for bitstream as newer variants.
3. It needs double buffering to be enabled in order to work correctly.
4. postproc must be enabled at the end of the job. It seems that core has
   some issues with latching register values if postproc registers are set
   at the beginning of the job

legacy_regs quirk could be split into 3, like legacy_regs, ring_buffer and
late_postproc, but I didn't see the need for that. I examined vendor
sources at [1] and it suggests that legacy_regs implies no ring buffer.

It's also unclear if core supports HEVC decoding or not. This can be
implemented later. VP9 10-bit decoding support is mentioned in manual, but
it doesn't work at the moment. This will be addressed later.

Please take a look.

Best regards,
Jernej

[1] https://github.com/CliveLau1990/imx-vpu-hantro

Jernej Skrabec (7):
  media: hantro: add support for reset lines
  media: hantro: vp9: use double buffering if needed
  media: hantro: vp9: add support for legacy register set
  media: hantro: move postproc enablement for old cores
  media: dt-bindings: allwinner: document H6 Hantro G2 binding
  media: hantro: Add support for Allwinner H6
  arm64: dts: allwinner: h6: Add Hantro G2 node

 .../media/allwinner,sun50i-h6-vpu-g2.yaml     |  64 +++++++++++
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi  |   9 ++
 drivers/staging/media/hantro/Kconfig          |  10 +-
 drivers/staging/media/hantro/Makefile         |   3 +
 drivers/staging/media/hantro/hantro.h         |   7 ++
 drivers/staging/media/hantro/hantro_drv.c     |  27 ++++-
 drivers/staging/media/hantro/hantro_g2_regs.h |  20 ++++
 .../staging/media/hantro/hantro_g2_vp9_dec.c  |  76 ++++++++++---
 drivers/staging/media/hantro/hantro_hw.h      |   1 +
 drivers/staging/media/hantro/sunxi_vpu_hw.c   | 104 ++++++++++++++++++
 10 files changed, 301 insertions(+), 20 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/allwinner,sun50i-h6-vpu-g2.yaml
 create mode 100644 drivers/staging/media/hantro/sunxi_vpu_hw.c

-- 
2.34.0


^ permalink raw reply	[flat|nested] 40+ messages in thread
* [PATCH 0/7] media: hantro: add Allwinner H6 support
@ 2021-11-22 18:46 ` Jernej Skrabec
  0 siblings, 0 replies; 40+ messages in thread
From: Jernej Skrabec @ 2021-11-22 18:46 UTC (permalink / raw)
  To: linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, andrzej.p, gregkh, devicetree, linux-arm-kernel,
	linux-sunxi, linux-kernel, linux-staging, Jernej Skrabec

Hi everyone!

Here is patchset which adds support for Hantro G2 core found in Allwinner
H6 SoC. It is slightly older core, so it needs few quirks to be
implemented:
1. It uses slightly different register layout in some cases. However, those
   differences are small, so it makes sense only to add quirks.
2. It doesn't use ring buffer for bitstream as newer variants.
3. It needs double buffering to be enabled in order to work correctly.
4. postproc must be enabled at the end of the job. It seems that core has
   some issues with latching register values if postproc registers are set
   at the beginning of the job

legacy_regs quirk could be split into 3, like legacy_regs, ring_buffer and
late_postproc, but I didn't see the need for that. I examined vendor
sources at [1] and it suggests that legacy_regs implies no ring buffer.

It's also unclear if core supports HEVC decoding or not. This can be
implemented later. VP9 10-bit decoding support is mentioned in manual, but
it doesn't work at the moment. This will be addressed later.

Please take a look.

Best regards,
Jernej

[1] https://github.com/CliveLau1990/imx-vpu-hantro

Jernej Skrabec (7):
  media: hantro: add support for reset lines
  media: hantro: vp9: use double buffering if needed
  media: hantro: vp9: add support for legacy register set
  media: hantro: move postproc enablement for old cores
  media: dt-bindings: allwinner: document H6 Hantro G2 binding
  media: hantro: Add support for Allwinner H6
  arm64: dts: allwinner: h6: Add Hantro G2 node

 .../media/allwinner,sun50i-h6-vpu-g2.yaml     |  64 +++++++++++
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi  |   9 ++
 drivers/staging/media/hantro/Kconfig          |  10 +-
 drivers/staging/media/hantro/Makefile         |   3 +
 drivers/staging/media/hantro/hantro.h         |   7 ++
 drivers/staging/media/hantro/hantro_drv.c     |  27 ++++-
 drivers/staging/media/hantro/hantro_g2_regs.h |  20 ++++
 .../staging/media/hantro/hantro_g2_vp9_dec.c  |  76 ++++++++++---
 drivers/staging/media/hantro/hantro_hw.h      |   1 +
 drivers/staging/media/hantro/sunxi_vpu_hw.c   | 104 ++++++++++++++++++
 10 files changed, 301 insertions(+), 20 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/allwinner,sun50i-h6-vpu-g2.yaml
 create mode 100644 drivers/staging/media/hantro/sunxi_vpu_hw.c

-- 
2.34.0


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

^ permalink raw reply	[flat|nested] 40+ messages in thread
* [PATCH 1/7] media: hantro: add support for reset lines
  2021-11-22 18:46 ` Jernej Skrabec
@ 2021-11-22 18:46   ` Jernej Skrabec
  -1 siblings, 0 replies; 40+ messages in thread
From: Jernej Skrabec @ 2021-11-22 18:46 UTC (permalink / raw)
  To: linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, andrzej.p, gregkh, devicetree, linux-arm-kernel,
	linux-sunxi, linux-kernel, linux-staging, Jernej Skrabec

Some SoCs like Allwinner H6 use reset lines for resetting Hantro G2. Add
support for them.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/staging/media/hantro/hantro.h     |  3 +++
 drivers/staging/media/hantro/hantro_drv.c | 15 ++++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h
index 7da23f7f207a..33eb3e092cc1 100644
--- a/drivers/staging/media/hantro/hantro.h
+++ b/drivers/staging/media/hantro/hantro.h
@@ -16,6 +16,7 @@
 #include <linux/videodev2.h>
 #include <linux/wait.h>
 #include <linux/clk.h>
+#include <linux/reset.h>
 
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
@@ -171,6 +172,7 @@ hantro_vdev_to_func(struct video_device *vdev)
  * @dev:		Pointer to device for convenient logging using
  *			dev_ macros.
  * @clocks:		Array of clock handles.
+ * @resets:		Array of reset handles.
  * @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.
@@ -190,6 +192,7 @@ struct hantro_dev {
 	struct platform_device *pdev;
 	struct device *dev;
 	struct clk_bulk_data *clocks;
+	struct reset_control *resets;
 	void __iomem **reg_bases;
 	void __iomem *enc_base;
 	void __iomem *dec_base;
diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index ab2467998d29..8c3de31f51b3 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -905,6 +905,10 @@ static int hantro_probe(struct platform_device *pdev)
 			return PTR_ERR(vpu->clocks[0].clk);
 	}
 
+	vpu->resets = devm_reset_control_array_get(&pdev->dev, false, true);
+	if (IS_ERR(vpu->resets))
+		return PTR_ERR(vpu->resets);
+
 	num_bases = vpu->variant->num_regs ?: 1;
 	vpu->reg_bases = devm_kcalloc(&pdev->dev, num_bases,
 				      sizeof(*vpu->reg_bases), GFP_KERNEL);
@@ -978,10 +982,16 @@ static int hantro_probe(struct platform_device *pdev)
 	pm_runtime_use_autosuspend(vpu->dev);
 	pm_runtime_enable(vpu->dev);
 
+	ret = reset_control_deassert(vpu->resets);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to deassert resets\n");
+		return ret;
+	}
+
 	ret = clk_bulk_prepare(vpu->variant->num_clocks, vpu->clocks);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to prepare clocks\n");
-		return ret;
+		goto err_rst_assert;
 	}
 
 	ret = v4l2_device_register(&pdev->dev, &vpu->v4l2_dev);
@@ -1037,6 +1047,8 @@ static int hantro_probe(struct platform_device *pdev)
 	v4l2_device_unregister(&vpu->v4l2_dev);
 err_clk_unprepare:
 	clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
+err_rst_assert:
+	reset_control_assert(vpu->resets);
 	pm_runtime_dont_use_autosuspend(vpu->dev);
 	pm_runtime_disable(vpu->dev);
 	return ret;
@@ -1055,6 +1067,7 @@ static int hantro_remove(struct platform_device *pdev)
 	v4l2_m2m_release(vpu->m2m_dev);
 	v4l2_device_unregister(&vpu->v4l2_dev);
 	clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
+	reset_control_assert(vpu->resets);
 	pm_runtime_dont_use_autosuspend(vpu->dev);
 	pm_runtime_disable(vpu->dev);
 	return 0;
-- 
2.34.0


^ permalink raw reply related	[flat|nested] 40+ messages in thread
* [PATCH 1/7] media: hantro: add support for reset lines
@ 2021-11-22 18:46   ` Jernej Skrabec
  0 siblings, 0 replies; 40+ messages in thread
From: Jernej Skrabec @ 2021-11-22 18:46 UTC (permalink / raw)
  To: linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, andrzej.p, gregkh, devicetree, linux-arm-kernel,
	linux-sunxi, linux-kernel, linux-staging, Jernej Skrabec

Some SoCs like Allwinner H6 use reset lines for resetting Hantro G2. Add
support for them.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/staging/media/hantro/hantro.h     |  3 +++
 drivers/staging/media/hantro/hantro_drv.c | 15 ++++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h
index 7da23f7f207a..33eb3e092cc1 100644
--- a/drivers/staging/media/hantro/hantro.h
+++ b/drivers/staging/media/hantro/hantro.h
@@ -16,6 +16,7 @@
 #include <linux/videodev2.h>
 #include <linux/wait.h>
 #include <linux/clk.h>
+#include <linux/reset.h>
 
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
@@ -171,6 +172,7 @@ hantro_vdev_to_func(struct video_device *vdev)
  * @dev:		Pointer to device for convenient logging using
  *			dev_ macros.
  * @clocks:		Array of clock handles.
+ * @resets:		Array of reset handles.
  * @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.
@@ -190,6 +192,7 @@ struct hantro_dev {
 	struct platform_device *pdev;
 	struct device *dev;
 	struct clk_bulk_data *clocks;
+	struct reset_control *resets;
 	void __iomem **reg_bases;
 	void __iomem *enc_base;
 	void __iomem *dec_base;
diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index ab2467998d29..8c3de31f51b3 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -905,6 +905,10 @@ static int hantro_probe(struct platform_device *pdev)
 			return PTR_ERR(vpu->clocks[0].clk);
 	}
 
+	vpu->resets = devm_reset_control_array_get(&pdev->dev, false, true);
+	if (IS_ERR(vpu->resets))
+		return PTR_ERR(vpu->resets);
+
 	num_bases = vpu->variant->num_regs ?: 1;
 	vpu->reg_bases = devm_kcalloc(&pdev->dev, num_bases,
 				      sizeof(*vpu->reg_bases), GFP_KERNEL);
@@ -978,10 +982,16 @@ static int hantro_probe(struct platform_device *pdev)
 	pm_runtime_use_autosuspend(vpu->dev);
 	pm_runtime_enable(vpu->dev);
 
+	ret = reset_control_deassert(vpu->resets);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to deassert resets\n");
+		return ret;
+	}
+
 	ret = clk_bulk_prepare(vpu->variant->num_clocks, vpu->clocks);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to prepare clocks\n");
-		return ret;
+		goto err_rst_assert;
 	}
 
 	ret = v4l2_device_register(&pdev->dev, &vpu->v4l2_dev);
@@ -1037,6 +1047,8 @@ static int hantro_probe(struct platform_device *pdev)
 	v4l2_device_unregister(&vpu->v4l2_dev);
 err_clk_unprepare:
 	clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
+err_rst_assert:
+	reset_control_assert(vpu->resets);
 	pm_runtime_dont_use_autosuspend(vpu->dev);
 	pm_runtime_disable(vpu->dev);
 	return ret;
@@ -1055,6 +1067,7 @@ static int hantro_remove(struct platform_device *pdev)
 	v4l2_m2m_release(vpu->m2m_dev);
 	v4l2_device_unregister(&vpu->v4l2_dev);
 	clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
+	reset_control_assert(vpu->resets);
 	pm_runtime_dont_use_autosuspend(vpu->dev);
 	pm_runtime_disable(vpu->dev);
 	return 0;
-- 
2.34.0


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

^ permalink raw reply related	[flat|nested] 40+ messages in thread
* [PATCH 2/7] media: hantro: vp9: use double buffering if needed
  2021-11-22 18:46 ` Jernej Skrabec
@ 2021-11-22 18:46   ` Jernej Skrabec
  -1 siblings, 0 replies; 40+ messages in thread
From: Jernej Skrabec @ 2021-11-22 18:46 UTC (permalink / raw)
  To: linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, andrzej.p, gregkh, devicetree, linux-arm-kernel,
	linux-sunxi, linux-kernel, linux-staging, Jernej Skrabec

Some G2 variants need double buffering to be enabled in order to work
correctly, like that found in Allwinner H6 SoC.

Add platform quirk for that.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/staging/media/hantro/hantro.h            | 2 ++
 drivers/staging/media/hantro/hantro_g2_regs.h    | 1 +
 drivers/staging/media/hantro/hantro_g2_vp9_dec.c | 2 ++
 3 files changed, 5 insertions(+)

diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h
index 33eb3e092cc1..d03824fa3222 100644
--- a/drivers/staging/media/hantro/hantro.h
+++ b/drivers/staging/media/hantro/hantro.h
@@ -73,6 +73,7 @@ struct hantro_irq {
  * @num_clocks:			number of clocks in the array
  * @reg_names:			array of register range names
  * @num_regs:			number of register range names in the array
+ * @double_buffer:		core needs double buffering
  */
 struct hantro_variant {
 	unsigned int enc_offset;
@@ -94,6 +95,7 @@ struct hantro_variant {
 	int num_clocks;
 	const char * const *reg_names;
 	int num_regs;
+	unsigned int double_buffer : 1;
 };
 
 /**
diff --git a/drivers/staging/media/hantro/hantro_g2_regs.h b/drivers/staging/media/hantro/hantro_g2_regs.h
index 9c857dd1ad9b..15a391a4650e 100644
--- a/drivers/staging/media/hantro/hantro_g2_regs.h
+++ b/drivers/staging/media/hantro/hantro_g2_regs.h
@@ -270,6 +270,7 @@
 #define g2_apf_threshold	G2_DEC_REG(55, 0, 0xffff)
 
 #define g2_clk_gate_e		G2_DEC_REG(58, 16, 0x1)
+#define g2_double_buffer_e	G2_DEC_REG(58, 15, 0x1)
 #define g2_buswidth		G2_DEC_REG(58, 8,  0x7)
 #define g2_max_burst		G2_DEC_REG(58, 0,  0xff)
 
diff --git a/drivers/staging/media/hantro/hantro_g2_vp9_dec.c b/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
index e04242d10fa2..d4fc649a4da1 100644
--- a/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
+++ b/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
@@ -847,6 +847,8 @@ config_registers(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_frame *dec_p
 	hantro_reg_write(ctx->dev, &g2_clk_gate_e, 1);
 	hantro_reg_write(ctx->dev, &g2_max_cb_size, 6);
 	hantro_reg_write(ctx->dev, &g2_min_cb_size, 3);
+	if (ctx->dev->variant->double_buffer)
+		hantro_reg_write(ctx->dev, &g2_double_buffer_e, 1);
 
 	config_output(ctx, dst, dec_params);
 
-- 
2.34.0


^ permalink raw reply related	[flat|nested] 40+ messages in thread
* [PATCH 2/7] media: hantro: vp9: use double buffering if needed
@ 2021-11-22 18:46   ` Jernej Skrabec
  0 siblings, 0 replies; 40+ messages in thread
From: Jernej Skrabec @ 2021-11-22 18:46 UTC (permalink / raw)
  To: linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, andrzej.p, gregkh, devicetree, linux-arm-kernel,
	linux-sunxi, linux-kernel, linux-staging, Jernej Skrabec

Some G2 variants need double buffering to be enabled in order to work
correctly, like that found in Allwinner H6 SoC.

Add platform quirk for that.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/staging/media/hantro/hantro.h            | 2 ++
 drivers/staging/media/hantro/hantro_g2_regs.h    | 1 +
 drivers/staging/media/hantro/hantro_g2_vp9_dec.c | 2 ++
 3 files changed, 5 insertions(+)

diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h
index 33eb3e092cc1..d03824fa3222 100644
--- a/drivers/staging/media/hantro/hantro.h
+++ b/drivers/staging/media/hantro/hantro.h
@@ -73,6 +73,7 @@ struct hantro_irq {
  * @num_clocks:			number of clocks in the array
  * @reg_names:			array of register range names
  * @num_regs:			number of register range names in the array
+ * @double_buffer:		core needs double buffering
  */
 struct hantro_variant {
 	unsigned int enc_offset;
@@ -94,6 +95,7 @@ struct hantro_variant {
 	int num_clocks;
 	const char * const *reg_names;
 	int num_regs;
+	unsigned int double_buffer : 1;
 };
 
 /**
diff --git a/drivers/staging/media/hantro/hantro_g2_regs.h b/drivers/staging/media/hantro/hantro_g2_regs.h
index 9c857dd1ad9b..15a391a4650e 100644
--- a/drivers/staging/media/hantro/hantro_g2_regs.h
+++ b/drivers/staging/media/hantro/hantro_g2_regs.h
@@ -270,6 +270,7 @@
 #define g2_apf_threshold	G2_DEC_REG(55, 0, 0xffff)
 
 #define g2_clk_gate_e		G2_DEC_REG(58, 16, 0x1)
+#define g2_double_buffer_e	G2_DEC_REG(58, 15, 0x1)
 #define g2_buswidth		G2_DEC_REG(58, 8,  0x7)
 #define g2_max_burst		G2_DEC_REG(58, 0,  0xff)
 
diff --git a/drivers/staging/media/hantro/hantro_g2_vp9_dec.c b/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
index e04242d10fa2..d4fc649a4da1 100644
--- a/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
+++ b/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
@@ -847,6 +847,8 @@ config_registers(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_frame *dec_p
 	hantro_reg_write(ctx->dev, &g2_clk_gate_e, 1);
 	hantro_reg_write(ctx->dev, &g2_max_cb_size, 6);
 	hantro_reg_write(ctx->dev, &g2_min_cb_size, 3);
+	if (ctx->dev->variant->double_buffer)
+		hantro_reg_write(ctx->dev, &g2_double_buffer_e, 1);
 
 	config_output(ctx, dst, dec_params);
 
-- 
2.34.0


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

^ permalink raw reply related	[flat|nested] 40+ messages in thread
* [PATCH 3/7] media: hantro: vp9: add support for legacy register set
  2021-11-22 18:46 ` Jernej Skrabec
@ 2021-11-22 18:46   ` Jernej Skrabec
  -1 siblings, 0 replies; 40+ messages in thread
From: Jernej Skrabec @ 2021-11-22 18:46 UTC (permalink / raw)
  To: linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, andrzej.p, gregkh, devicetree, linux-arm-kernel,
	linux-sunxi, linux-kernel, linux-staging, Jernej Skrabec

Some older G2 cores uses slightly different register set for HEVC and
VP9. Since vast majority of registers and logic is the same, it doesn't
make sense to introduce another drivers.

Add legacy_regs quirk and implement only VP9 changes for now. HEVC
changes will be introduced later, if needed.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/staging/media/hantro/hantro.h         |  2 +
 drivers/staging/media/hantro/hantro_g2_regs.h | 19 +++++
 .../staging/media/hantro/hantro_g2_vp9_dec.c  | 74 ++++++++++++++-----
 3 files changed, 78 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h
index d03824fa3222..83ed25d9657b 100644
--- a/drivers/staging/media/hantro/hantro.h
+++ b/drivers/staging/media/hantro/hantro.h
@@ -74,6 +74,7 @@ struct hantro_irq {
  * @reg_names:			array of register range names
  * @num_regs:			number of register range names in the array
  * @double_buffer:		core needs double buffering
+ * @legacy_regs:		core uses legacy register set
  */
 struct hantro_variant {
 	unsigned int enc_offset;
@@ -96,6 +97,7 @@ struct hantro_variant {
 	const char * const *reg_names;
 	int num_regs;
 	unsigned int double_buffer : 1;
+	unsigned int legacy_regs : 1;
 };
 
 /**
diff --git a/drivers/staging/media/hantro/hantro_g2_regs.h b/drivers/staging/media/hantro/hantro_g2_regs.h
index 15a391a4650e..d7c2ff05208e 100644
--- a/drivers/staging/media/hantro/hantro_g2_regs.h
+++ b/drivers/staging/media/hantro/hantro_g2_regs.h
@@ -37,6 +37,13 @@
 
 #define g2_strm_swap		G2_DEC_REG(2, 28, 0xf)
 #define g2_dirmv_swap		G2_DEC_REG(2, 20, 0xf)
+/* used on older variants */
+#define g2_strm_swap_old	G2_DEC_REG(2, 27, 0x1f)
+#define g2_pic_swap		G2_DEC_REG(2, 22, 0x1f)
+#define g2_dirmv_swap_old	G2_DEC_REG(2, 17, 0x1f)
+#define g2_tab0_swap		G2_DEC_REG(2, 12, 0x1f)
+#define g2_tab1_swap		G2_DEC_REG(2, 7, 0x1f)
+#define g2_tab2_swap		G2_DEC_REG(2, 2, 0x1f)
 
 #define g2_mode			G2_DEC_REG(3, 27, 0x1f)
 #define g2_compress_swap	G2_DEC_REG(3, 20, 0xf)
@@ -45,6 +52,8 @@
 #define g2_out_dis		G2_DEC_REG(3, 15, 0x1)
 #define g2_out_filtering_dis	G2_DEC_REG(3, 14, 0x1)
 #define g2_write_mvs_e		G2_DEC_REG(3, 12, 0x1)
+#define g2_tab3_swap		G2_DEC_REG(3, 7, 0x1f)
+#define g2_rscan_swap		G2_DEC_REG(3, 2, 0x1f)
 
 #define g2_pic_width_in_cbs	G2_DEC_REG(4, 19, 0x1fff)
 #define g2_pic_height_in_cbs	G2_DEC_REG(4, 6,  0x1fff)
@@ -58,6 +67,7 @@
 #define g2_tempor_mvp_e		G2_DEC_REG(5, 11, 0x1)
 #define g2_max_cu_qpd_depth	G2_DEC_REG(5, 5,  0x3f)
 #define g2_cu_qpd_e		G2_DEC_REG(5, 4,  0x1)
+#define g2_pix_shift		G2_DEC_REG(5, 0,  0xf)
 
 #define g2_stream_len		G2_DEC_REG(6, 0,  0xffffffff)
 
@@ -80,6 +90,8 @@
 
 #define g2_const_intra_e	G2_DEC_REG(8, 31, 0x1)
 #define g2_filt_ctrl_pres	G2_DEC_REG(8, 30, 0x1)
+#define g2_bit_depth_y		G2_DEC_REG(8, 21, 0xf)
+#define g2_bit_depth_c		G2_DEC_REG(8, 17, 0xf)
 #define g2_idr_pic_e		G2_DEC_REG(8, 16, 0x1)
 #define g2_bit_depth_pcm_y	G2_DEC_REG(8, 12, 0xf)
 #define g2_bit_depth_pcm_c	G2_DEC_REG(8, 8,  0xf)
@@ -87,6 +99,9 @@
 #define g2_bit_depth_c_minus8	G2_DEC_REG(8, 4,  0x3)
 #define g2_output_8_bits	G2_DEC_REG(8, 3,  0x1)
 #define g2_output_format	G2_DEC_REG(8, 0,  0x7)
+/* used on older variants */
+#define g2_rs_out_bit_depth	G2_DEC_REG(8, 4,  0xf)
+#define g2_pp_pix_shift		G2_DEC_REG(8, 0,  0xf)
 
 #define g2_refidx1_active	G2_DEC_REG(9, 19, 0x1f)
 #define g2_refidx0_active	G2_DEC_REG(9, 14, 0x1f)
@@ -98,6 +113,10 @@
 #define g2_num_tile_rows	G2_DEC_REG(10, 14, 0x1f)
 #define g2_tile_e		G2_DEC_REG(10, 1,  0x1)
 #define g2_entropy_sync_e	G2_DEC_REG(10, 0,  0x1)
+/* used on older variants */
+#define g2_init_qp_old		G2_DEC_REG(10, 25, 0x3f)
+#define g2_num_tile_cols_old	G2_DEC_REG(10, 20, 0x1f)
+#define g2_num_tile_rows_old	G2_DEC_REG(10, 15, 0x1f)
 
 #define vp9_transform_mode	G2_DEC_REG(11, 27, 0x7)
 #define vp9_filt_sharpness	G2_DEC_REG(11, 21, 0x7)
diff --git a/drivers/staging/media/hantro/hantro_g2_vp9_dec.c b/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
index d4fc649a4da1..5aac32700cd0 100644
--- a/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
+++ b/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
@@ -150,7 +150,8 @@ static void config_output(struct hantro_ctx *ctx,
 	dma_addr_t luma_addr, chroma_addr, mv_addr;
 
 	hantro_reg_write(ctx->dev, &g2_out_dis, 0);
-	hantro_reg_write(ctx->dev, &g2_output_format, 0);
+	if (!ctx->dev->variant->legacy_regs)
+		hantro_reg_write(ctx->dev, &g2_output_format, 0);
 
 	luma_addr = hantro_get_dec_buf_addr(ctx, &dst->base.vb.vb2_buf);
 	hantro_write_addr(ctx->dev, G2_OUT_LUMA_ADDR, luma_addr);
@@ -327,6 +328,7 @@ config_tiles(struct hantro_ctx *ctx,
 	struct hantro_aux_buf *tile_edge = &vp9_ctx->tile_edge;
 	dma_addr_t addr;
 	unsigned short *tile_mem;
+	unsigned int rows, cols;
 
 	addr = misc->dma + vp9_ctx->tile_info_offset;
 	hantro_write_addr(ctx->dev, G2_TILE_SIZES_ADDR, addr);
@@ -344,17 +346,24 @@ config_tiles(struct hantro_ctx *ctx,
 
 		fill_tile_info(ctx, tile_r, tile_c, sbs_r, sbs_c, tile_mem);
 
+		cols = tile_c;
+		rows = tile_r;
 		hantro_reg_write(ctx->dev, &g2_tile_e, 1);
-		hantro_reg_write(ctx->dev, &g2_num_tile_cols, tile_c);
-		hantro_reg_write(ctx->dev, &g2_num_tile_rows, tile_r);
-
 	} else {
 		tile_mem[0] = hantro_vp9_num_sbs(dst->vp9.width);
 		tile_mem[1] = hantro_vp9_num_sbs(dst->vp9.height);
 
+		cols = 1;
+		rows = 1;
 		hantro_reg_write(ctx->dev, &g2_tile_e, 0);
-		hantro_reg_write(ctx->dev, &g2_num_tile_cols, 1);
-		hantro_reg_write(ctx->dev, &g2_num_tile_rows, 1);
+	}
+
+	if (ctx->dev->variant->legacy_regs) {
+		hantro_reg_write(ctx->dev, &g2_num_tile_cols_old, cols);
+		hantro_reg_write(ctx->dev, &g2_num_tile_rows_old, rows);
+	} else {
+		hantro_reg_write(ctx->dev, &g2_num_tile_cols, cols);
+		hantro_reg_write(ctx->dev, &g2_num_tile_rows, rows);
 	}
 
 	/* provide aux buffers even if no tiles are used */
@@ -505,8 +514,22 @@ static void config_picture_dimensions(struct hantro_ctx *ctx, struct hantro_deco
 static void
 config_bit_depth(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_frame *dec_params)
 {
-	hantro_reg_write(ctx->dev, &g2_bit_depth_y_minus8, dec_params->bit_depth - 8);
-	hantro_reg_write(ctx->dev, &g2_bit_depth_c_minus8, dec_params->bit_depth - 8);
+	if (ctx->dev->variant->legacy_regs) {
+		u8 pp_shift = 0;
+
+		hantro_reg_write(ctx->dev, &g2_bit_depth_y, dec_params->bit_depth);
+		hantro_reg_write(ctx->dev, &g2_bit_depth_c, dec_params->bit_depth);
+		hantro_reg_write(ctx->dev, &g2_rs_out_bit_depth, dec_params->bit_depth);
+
+		if (dec_params->bit_depth > 8)
+			pp_shift = 16 - dec_params->bit_depth;
+
+		hantro_reg_write(ctx->dev, &g2_pp_pix_shift, pp_shift);
+		hantro_reg_write(ctx->dev, &g2_pix_shift, 0);
+	} else {
+		hantro_reg_write(ctx->dev, &g2_bit_depth_y_minus8, dec_params->bit_depth - 8);
+		hantro_reg_write(ctx->dev, &g2_bit_depth_c_minus8, dec_params->bit_depth - 8);
+	}
 }
 
 static inline bool is_lossless(const struct v4l2_vp9_quantization *quant)
@@ -784,9 +807,13 @@ config_source(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_frame *dec_para
 		     + dec_params->compressed_header_size;
 
 	stream_base = vb2_dma_contig_plane_dma_addr(&vb2_src->vb2_buf, 0);
-	hantro_write_addr(ctx->dev, G2_STREAM_ADDR, stream_base);
 
 	tmp_addr = stream_base + headres_size;
+	if (ctx->dev->variant->legacy_regs)
+		hantro_write_addr(ctx->dev, G2_STREAM_ADDR, (tmp_addr & ~0xf));
+	else
+		hantro_write_addr(ctx->dev, G2_STREAM_ADDR, stream_base);
+
 	start_bit = (tmp_addr & 0xf) * 8;
 	hantro_reg_write(ctx->dev, &g2_start_bit, start_bit);
 
@@ -794,10 +821,12 @@ config_source(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_frame *dec_para
 	src_len += start_bit / 8 - headres_size;
 	hantro_reg_write(ctx->dev, &g2_stream_len, src_len);
 
-	tmp_addr &= ~0xf;
-	hantro_reg_write(ctx->dev, &g2_strm_start_offset, tmp_addr - stream_base);
-	src_buf_len = vb2_plane_size(&vb2_src->vb2_buf, 0);
-	hantro_reg_write(ctx->dev, &g2_strm_buffer_len, src_buf_len);
+	if (!ctx->dev->variant->legacy_regs) {
+		tmp_addr &= ~0xf;
+		hantro_reg_write(ctx->dev, &g2_strm_start_offset, tmp_addr - stream_base);
+		src_buf_len = vb2_plane_size(&vb2_src->vb2_buf, 0);
+		hantro_reg_write(ctx->dev, &g2_strm_buffer_len, src_buf_len);
+	}
 }
 
 static void
@@ -837,13 +866,24 @@ config_registers(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_frame *dec_p
 
 	/* configure basic registers */
 	hantro_reg_write(ctx->dev, &g2_mode, VP9_DEC_MODE);
-	hantro_reg_write(ctx->dev, &g2_strm_swap, 0xf);
-	hantro_reg_write(ctx->dev, &g2_dirmv_swap, 0xf);
-	hantro_reg_write(ctx->dev, &g2_compress_swap, 0xf);
+	if (!ctx->dev->variant->legacy_regs) {
+		hantro_reg_write(ctx->dev, &g2_strm_swap, 0xf);
+		hantro_reg_write(ctx->dev, &g2_dirmv_swap, 0xf);
+		hantro_reg_write(ctx->dev, &g2_compress_swap, 0xf);
+		hantro_reg_write(ctx->dev, &g2_ref_compress_bypass, 1);
+	} else {
+		hantro_reg_write(ctx->dev, &g2_strm_swap_old, 0x1f);
+		hantro_reg_write(ctx->dev, &g2_pic_swap, 0x10);
+		hantro_reg_write(ctx->dev, &g2_dirmv_swap_old, 0x10);
+		hantro_reg_write(ctx->dev, &g2_tab0_swap, 0x10);
+		hantro_reg_write(ctx->dev, &g2_tab1_swap, 0x10);
+		hantro_reg_write(ctx->dev, &g2_tab2_swap, 0x10);
+		hantro_reg_write(ctx->dev, &g2_tab3_swap, 0x10);
+		hantro_reg_write(ctx->dev, &g2_rscan_swap, 0x10);
+	}
 	hantro_reg_write(ctx->dev, &g2_buswidth, BUS_WIDTH_128);
 	hantro_reg_write(ctx->dev, &g2_max_burst, 16);
 	hantro_reg_write(ctx->dev, &g2_apf_threshold, 8);
-	hantro_reg_write(ctx->dev, &g2_ref_compress_bypass, 1);
 	hantro_reg_write(ctx->dev, &g2_clk_gate_e, 1);
 	hantro_reg_write(ctx->dev, &g2_max_cb_size, 6);
 	hantro_reg_write(ctx->dev, &g2_min_cb_size, 3);
-- 
2.34.0


^ permalink raw reply related	[flat|nested] 40+ messages in thread
* [PATCH 3/7] media: hantro: vp9: add support for legacy register set
@ 2021-11-22 18:46   ` Jernej Skrabec
  0 siblings, 0 replies; 40+ messages in thread
From: Jernej Skrabec @ 2021-11-22 18:46 UTC (permalink / raw)
  To: linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, andrzej.p, gregkh, devicetree, linux-arm-kernel,
	linux-sunxi, linux-kernel, linux-staging, Jernej Skrabec

Some older G2 cores uses slightly different register set for HEVC and
VP9. Since vast majority of registers and logic is the same, it doesn't
make sense to introduce another drivers.

Add legacy_regs quirk and implement only VP9 changes for now. HEVC
changes will be introduced later, if needed.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/staging/media/hantro/hantro.h         |  2 +
 drivers/staging/media/hantro/hantro_g2_regs.h | 19 +++++
 .../staging/media/hantro/hantro_g2_vp9_dec.c  | 74 ++++++++++++++-----
 3 files changed, 78 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h
index d03824fa3222..83ed25d9657b 100644
--- a/drivers/staging/media/hantro/hantro.h
+++ b/drivers/staging/media/hantro/hantro.h
@@ -74,6 +74,7 @@ struct hantro_irq {
  * @reg_names:			array of register range names
  * @num_regs:			number of register range names in the array
  * @double_buffer:		core needs double buffering
+ * @legacy_regs:		core uses legacy register set
  */
 struct hantro_variant {
 	unsigned int enc_offset;
@@ -96,6 +97,7 @@ struct hantro_variant {
 	const char * const *reg_names;
 	int num_regs;
 	unsigned int double_buffer : 1;
+	unsigned int legacy_regs : 1;
 };
 
 /**
diff --git a/drivers/staging/media/hantro/hantro_g2_regs.h b/drivers/staging/media/hantro/hantro_g2_regs.h
index 15a391a4650e..d7c2ff05208e 100644
--- a/drivers/staging/media/hantro/hantro_g2_regs.h
+++ b/drivers/staging/media/hantro/hantro_g2_regs.h
@@ -37,6 +37,13 @@
 
 #define g2_strm_swap		G2_DEC_REG(2, 28, 0xf)
 #define g2_dirmv_swap		G2_DEC_REG(2, 20, 0xf)
+/* used on older variants */
+#define g2_strm_swap_old	G2_DEC_REG(2, 27, 0x1f)
+#define g2_pic_swap		G2_DEC_REG(2, 22, 0x1f)
+#define g2_dirmv_swap_old	G2_DEC_REG(2, 17, 0x1f)
+#define g2_tab0_swap		G2_DEC_REG(2, 12, 0x1f)
+#define g2_tab1_swap		G2_DEC_REG(2, 7, 0x1f)
+#define g2_tab2_swap		G2_DEC_REG(2, 2, 0x1f)
 
 #define g2_mode			G2_DEC_REG(3, 27, 0x1f)
 #define g2_compress_swap	G2_DEC_REG(3, 20, 0xf)
@@ -45,6 +52,8 @@
 #define g2_out_dis		G2_DEC_REG(3, 15, 0x1)
 #define g2_out_filtering_dis	G2_DEC_REG(3, 14, 0x1)
 #define g2_write_mvs_e		G2_DEC_REG(3, 12, 0x1)
+#define g2_tab3_swap		G2_DEC_REG(3, 7, 0x1f)
+#define g2_rscan_swap		G2_DEC_REG(3, 2, 0x1f)
 
 #define g2_pic_width_in_cbs	G2_DEC_REG(4, 19, 0x1fff)
 #define g2_pic_height_in_cbs	G2_DEC_REG(4, 6,  0x1fff)
@@ -58,6 +67,7 @@
 #define g2_tempor_mvp_e		G2_DEC_REG(5, 11, 0x1)
 #define g2_max_cu_qpd_depth	G2_DEC_REG(5, 5,  0x3f)
 #define g2_cu_qpd_e		G2_DEC_REG(5, 4,  0x1)
+#define g2_pix_shift		G2_DEC_REG(5, 0,  0xf)
 
 #define g2_stream_len		G2_DEC_REG(6, 0,  0xffffffff)
 
@@ -80,6 +90,8 @@
 
 #define g2_const_intra_e	G2_DEC_REG(8, 31, 0x1)
 #define g2_filt_ctrl_pres	G2_DEC_REG(8, 30, 0x1)
+#define g2_bit_depth_y		G2_DEC_REG(8, 21, 0xf)
+#define g2_bit_depth_c		G2_DEC_REG(8, 17, 0xf)
 #define g2_idr_pic_e		G2_DEC_REG(8, 16, 0x1)
 #define g2_bit_depth_pcm_y	G2_DEC_REG(8, 12, 0xf)
 #define g2_bit_depth_pcm_c	G2_DEC_REG(8, 8,  0xf)
@@ -87,6 +99,9 @@
 #define g2_bit_depth_c_minus8	G2_DEC_REG(8, 4,  0x3)
 #define g2_output_8_bits	G2_DEC_REG(8, 3,  0x1)
 #define g2_output_format	G2_DEC_REG(8, 0,  0x7)
+/* used on older variants */
+#define g2_rs_out_bit_depth	G2_DEC_REG(8, 4,  0xf)
+#define g2_pp_pix_shift		G2_DEC_REG(8, 0,  0xf)
 
 #define g2_refidx1_active	G2_DEC_REG(9, 19, 0x1f)
 #define g2_refidx0_active	G2_DEC_REG(9, 14, 0x1f)
@@ -98,6 +113,10 @@
 #define g2_num_tile_rows	G2_DEC_REG(10, 14, 0x1f)
 #define g2_tile_e		G2_DEC_REG(10, 1,  0x1)
 #define g2_entropy_sync_e	G2_DEC_REG(10, 0,  0x1)
+/* used on older variants */
+#define g2_init_qp_old		G2_DEC_REG(10, 25, 0x3f)
+#define g2_num_tile_cols_old	G2_DEC_REG(10, 20, 0x1f)
+#define g2_num_tile_rows_old	G2_DEC_REG(10, 15, 0x1f)
 
 #define vp9_transform_mode	G2_DEC_REG(11, 27, 0x7)
 #define vp9_filt_sharpness	G2_DEC_REG(11, 21, 0x7)
diff --git a/drivers/staging/media/hantro/hantro_g2_vp9_dec.c b/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
index d4fc649a4da1..5aac32700cd0 100644
--- a/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
+++ b/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
@@ -150,7 +150,8 @@ static void config_output(struct hantro_ctx *ctx,
 	dma_addr_t luma_addr, chroma_addr, mv_addr;
 
 	hantro_reg_write(ctx->dev, &g2_out_dis, 0);
-	hantro_reg_write(ctx->dev, &g2_output_format, 0);
+	if (!ctx->dev->variant->legacy_regs)
+		hantro_reg_write(ctx->dev, &g2_output_format, 0);
 
 	luma_addr = hantro_get_dec_buf_addr(ctx, &dst->base.vb.vb2_buf);
 	hantro_write_addr(ctx->dev, G2_OUT_LUMA_ADDR, luma_addr);
@@ -327,6 +328,7 @@ config_tiles(struct hantro_ctx *ctx,
 	struct hantro_aux_buf *tile_edge = &vp9_ctx->tile_edge;
 	dma_addr_t addr;
 	unsigned short *tile_mem;
+	unsigned int rows, cols;
 
 	addr = misc->dma + vp9_ctx->tile_info_offset;
 	hantro_write_addr(ctx->dev, G2_TILE_SIZES_ADDR, addr);
@@ -344,17 +346,24 @@ config_tiles(struct hantro_ctx *ctx,
 
 		fill_tile_info(ctx, tile_r, tile_c, sbs_r, sbs_c, tile_mem);
 
+		cols = tile_c;
+		rows = tile_r;
 		hantro_reg_write(ctx->dev, &g2_tile_e, 1);
-		hantro_reg_write(ctx->dev, &g2_num_tile_cols, tile_c);
-		hantro_reg_write(ctx->dev, &g2_num_tile_rows, tile_r);
-
 	} else {
 		tile_mem[0] = hantro_vp9_num_sbs(dst->vp9.width);
 		tile_mem[1] = hantro_vp9_num_sbs(dst->vp9.height);
 
+		cols = 1;
+		rows = 1;
 		hantro_reg_write(ctx->dev, &g2_tile_e, 0);
-		hantro_reg_write(ctx->dev, &g2_num_tile_cols, 1);
-		hantro_reg_write(ctx->dev, &g2_num_tile_rows, 1);
+	}
+
+	if (ctx->dev->variant->legacy_regs) {
+		hantro_reg_write(ctx->dev, &g2_num_tile_cols_old, cols);
+		hantro_reg_write(ctx->dev, &g2_num_tile_rows_old, rows);
+	} else {
+		hantro_reg_write(ctx->dev, &g2_num_tile_cols, cols);
+		hantro_reg_write(ctx->dev, &g2_num_tile_rows, rows);
 	}
 
 	/* provide aux buffers even if no tiles are used */
@@ -505,8 +514,22 @@ static void config_picture_dimensions(struct hantro_ctx *ctx, struct hantro_deco
 static void
 config_bit_depth(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_frame *dec_params)
 {
-	hantro_reg_write(ctx->dev, &g2_bit_depth_y_minus8, dec_params->bit_depth - 8);
-	hantro_reg_write(ctx->dev, &g2_bit_depth_c_minus8, dec_params->bit_depth - 8);
+	if (ctx->dev->variant->legacy_regs) {
+		u8 pp_shift = 0;
+
+		hantro_reg_write(ctx->dev, &g2_bit_depth_y, dec_params->bit_depth);
+		hantro_reg_write(ctx->dev, &g2_bit_depth_c, dec_params->bit_depth);
+		hantro_reg_write(ctx->dev, &g2_rs_out_bit_depth, dec_params->bit_depth);
+
+		if (dec_params->bit_depth > 8)
+			pp_shift = 16 - dec_params->bit_depth;
+
+		hantro_reg_write(ctx->dev, &g2_pp_pix_shift, pp_shift);
+		hantro_reg_write(ctx->dev, &g2_pix_shift, 0);
+	} else {
+		hantro_reg_write(ctx->dev, &g2_bit_depth_y_minus8, dec_params->bit_depth - 8);
+		hantro_reg_write(ctx->dev, &g2_bit_depth_c_minus8, dec_params->bit_depth - 8);
+	}
 }
 
 static inline bool is_lossless(const struct v4l2_vp9_quantization *quant)
@@ -784,9 +807,13 @@ config_source(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_frame *dec_para
 		     + dec_params->compressed_header_size;
 
 	stream_base = vb2_dma_contig_plane_dma_addr(&vb2_src->vb2_buf, 0);
-	hantro_write_addr(ctx->dev, G2_STREAM_ADDR, stream_base);
 
 	tmp_addr = stream_base + headres_size;
+	if (ctx->dev->variant->legacy_regs)
+		hantro_write_addr(ctx->dev, G2_STREAM_ADDR, (tmp_addr & ~0xf));
+	else
+		hantro_write_addr(ctx->dev, G2_STREAM_ADDR, stream_base);
+
 	start_bit = (tmp_addr & 0xf) * 8;
 	hantro_reg_write(ctx->dev, &g2_start_bit, start_bit);
 
@@ -794,10 +821,12 @@ config_source(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_frame *dec_para
 	src_len += start_bit / 8 - headres_size;
 	hantro_reg_write(ctx->dev, &g2_stream_len, src_len);
 
-	tmp_addr &= ~0xf;
-	hantro_reg_write(ctx->dev, &g2_strm_start_offset, tmp_addr - stream_base);
-	src_buf_len = vb2_plane_size(&vb2_src->vb2_buf, 0);
-	hantro_reg_write(ctx->dev, &g2_strm_buffer_len, src_buf_len);
+	if (!ctx->dev->variant->legacy_regs) {
+		tmp_addr &= ~0xf;
+		hantro_reg_write(ctx->dev, &g2_strm_start_offset, tmp_addr - stream_base);
+		src_buf_len = vb2_plane_size(&vb2_src->vb2_buf, 0);
+		hantro_reg_write(ctx->dev, &g2_strm_buffer_len, src_buf_len);
+	}
 }
 
 static void
@@ -837,13 +866,24 @@ config_registers(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_frame *dec_p
 
 	/* configure basic registers */
 	hantro_reg_write(ctx->dev, &g2_mode, VP9_DEC_MODE);
-	hantro_reg_write(ctx->dev, &g2_strm_swap, 0xf);
-	hantro_reg_write(ctx->dev, &g2_dirmv_swap, 0xf);
-	hantro_reg_write(ctx->dev, &g2_compress_swap, 0xf);
+	if (!ctx->dev->variant->legacy_regs) {
+		hantro_reg_write(ctx->dev, &g2_strm_swap, 0xf);
+		hantro_reg_write(ctx->dev, &g2_dirmv_swap, 0xf);
+		hantro_reg_write(ctx->dev, &g2_compress_swap, 0xf);
+		hantro_reg_write(ctx->dev, &g2_ref_compress_bypass, 1);
+	} else {
+		hantro_reg_write(ctx->dev, &g2_strm_swap_old, 0x1f);
+		hantro_reg_write(ctx->dev, &g2_pic_swap, 0x10);
+		hantro_reg_write(ctx->dev, &g2_dirmv_swap_old, 0x10);
+		hantro_reg_write(ctx->dev, &g2_tab0_swap, 0x10);
+		hantro_reg_write(ctx->dev, &g2_tab1_swap, 0x10);
+		hantro_reg_write(ctx->dev, &g2_tab2_swap, 0x10);
+		hantro_reg_write(ctx->dev, &g2_tab3_swap, 0x10);
+		hantro_reg_write(ctx->dev, &g2_rscan_swap, 0x10);
+	}
 	hantro_reg_write(ctx->dev, &g2_buswidth, BUS_WIDTH_128);
 	hantro_reg_write(ctx->dev, &g2_max_burst, 16);
 	hantro_reg_write(ctx->dev, &g2_apf_threshold, 8);
-	hantro_reg_write(ctx->dev, &g2_ref_compress_bypass, 1);
 	hantro_reg_write(ctx->dev, &g2_clk_gate_e, 1);
 	hantro_reg_write(ctx->dev, &g2_max_cb_size, 6);
 	hantro_reg_write(ctx->dev, &g2_min_cb_size, 3);
-- 
2.34.0


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

^ permalink raw reply related	[flat|nested] 40+ messages in thread
* [PATCH 4/7] media: hantro: move postproc enablement for old cores
  2021-11-22 18:46 ` Jernej Skrabec
@ 2021-11-22 18:46   ` Jernej Skrabec
  -1 siblings, 0 replies; 40+ messages in thread
From: Jernej Skrabec @ 2021-11-22 18:46 UTC (permalink / raw)
  To: linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, andrzej.p, gregkh, devicetree, linux-arm-kernel,
	linux-sunxi, linux-kernel, linux-staging, Jernej Skrabec

Older G2 cores, like that in Allwinner H6, seem to have issue with
latching postproc register values if this is first thing done in job.
Moving that to the end solves the issue.

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

diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index 8c3de31f51b3..530994ab3024 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -130,7 +130,7 @@ void hantro_start_prepare_run(struct hantro_ctx *ctx)
 	v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
 				&ctx->ctrl_handler);
 
-	if (!ctx->is_encoder) {
+	if (!ctx->is_encoder && !ctx->dev->variant->legacy_regs) {
 		if (hantro_needs_postproc(ctx, ctx->vpu_dst_fmt))
 			hantro_postproc_enable(ctx);
 		else
@@ -142,6 +142,13 @@ void hantro_end_prepare_run(struct hantro_ctx *ctx)
 {
 	struct vb2_v4l2_buffer *src_buf;
 
+	if (ctx->dev->variant->legacy_regs && !ctx->is_encoder) {
+		if (hantro_needs_postproc(ctx, ctx->vpu_dst_fmt))
+			hantro_postproc_enable(ctx);
+		else
+			hantro_postproc_disable(ctx);
+	}
+
 	src_buf = hantro_get_src_buf(ctx);
 	v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
 				   &ctx->ctrl_handler);
-- 
2.34.0


^ permalink raw reply related	[flat|nested] 40+ messages in thread
* [PATCH 4/7] media: hantro: move postproc enablement for old cores
@ 2021-11-22 18:46   ` Jernej Skrabec
  0 siblings, 0 replies; 40+ messages in thread
From: Jernej Skrabec @ 2021-11-22 18:46 UTC (permalink / raw)
  To: linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, andrzej.p, gregkh, devicetree, linux-arm-kernel,
	linux-sunxi, linux-kernel, linux-staging, Jernej Skrabec

Older G2 cores, like that in Allwinner H6, seem to have issue with
latching postproc register values if this is first thing done in job.
Moving that to the end solves the issue.

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

diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index 8c3de31f51b3..530994ab3024 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -130,7 +130,7 @@ void hantro_start_prepare_run(struct hantro_ctx *ctx)
 	v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
 				&ctx->ctrl_handler);
 
-	if (!ctx->is_encoder) {
+	if (!ctx->is_encoder && !ctx->dev->variant->legacy_regs) {
 		if (hantro_needs_postproc(ctx, ctx->vpu_dst_fmt))
 			hantro_postproc_enable(ctx);
 		else
@@ -142,6 +142,13 @@ void hantro_end_prepare_run(struct hantro_ctx *ctx)
 {
 	struct vb2_v4l2_buffer *src_buf;
 
+	if (ctx->dev->variant->legacy_regs && !ctx->is_encoder) {
+		if (hantro_needs_postproc(ctx, ctx->vpu_dst_fmt))
+			hantro_postproc_enable(ctx);
+		else
+			hantro_postproc_disable(ctx);
+	}
+
 	src_buf = hantro_get_src_buf(ctx);
 	v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
 				   &ctx->ctrl_handler);
-- 
2.34.0


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

^ permalink raw reply related	[flat|nested] 40+ messages in thread
* [PATCH 5/7] media: dt-bindings: allwinner: document H6 Hantro G2 binding
  2021-11-22 18:46 ` Jernej Skrabec
@ 2021-11-22 18:47   ` Jernej Skrabec
  -1 siblings, 0 replies; 40+ messages in thread
From: Jernej Skrabec @ 2021-11-22 18:47 UTC (permalink / raw)
  To: linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, andrzej.p, gregkh, devicetree, linux-arm-kernel,
	linux-sunxi, linux-kernel, linux-staging, Jernej Skrabec

Allwinner H6 contains older Hantro G2 core, primarly used for VP9 video
decoding. It's unclear for now if HEVC is also supported.

Describe it's binding.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 .../media/allwinner,sun50i-h6-vpu-g2.yaml     | 64 +++++++++++++++++++
 1 file changed, 64 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/allwinner,sun50i-h6-vpu-g2.yaml

diff --git a/Documentation/devicetree/bindings/media/allwinner,sun50i-h6-vpu-g2.yaml b/Documentation/devicetree/bindings/media/allwinner,sun50i-h6-vpu-g2.yaml
new file mode 100644
index 000000000000..24d7bf21499e
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/allwinner,sun50i-h6-vpu-g2.yaml
@@ -0,0 +1,64 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/media/allwinner,sun50i-h6-vpu-g2.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Hantro G2 VPU codec implemented on Allwinner H6 SoC
+
+maintainers:
+  - Jernej Skrabec <jernej.skrabec@gmail.com>
+
+description:
+  Hantro G2 video decode accelerator present on Allwinner H6 SoC.
+
+properties:
+  compatible:
+    const: allwinner,sun50i-h6-vpu-g2
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: Bus Clock
+      - description: Module Clock
+
+  clock-names:
+    items:
+      - const: bus
+      - const: mod
+
+  resets:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - resets
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/sun50i-h6-ccu.h>
+    #include <dt-bindings/reset/sun50i-h6-ccu.h>
+
+    video-codec-g2@1c00000 {
+        compatible = "allwinner,sun50i-h6-vpu-g2";
+        reg = <0x01c00000 0x1000>;
+        interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&ccu CLK_BUS_VP9>, <&ccu CLK_VP9>;
+        clock-names = "bus", "mod";
+        resets = <&ccu RST_BUS_VP9>;
+    };
+
+...
-- 
2.34.0


^ permalink raw reply related	[flat|nested] 40+ messages in thread
* [PATCH 5/7] media: dt-bindings: allwinner: document H6 Hantro G2 binding
@ 2021-11-22 18:47   ` Jernej Skrabec
  0 siblings, 0 replies; 40+ messages in thread
From: Jernej Skrabec @ 2021-11-22 18:47 UTC (permalink / raw)
  To: linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, andrzej.p, gregkh, devicetree, linux-arm-kernel,
	linux-sunxi, linux-kernel, linux-staging, Jernej Skrabec

Allwinner H6 contains older Hantro G2 core, primarly used for VP9 video
decoding. It's unclear for now if HEVC is also supported.

Describe it's binding.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 .../media/allwinner,sun50i-h6-vpu-g2.yaml     | 64 +++++++++++++++++++
 1 file changed, 64 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/allwinner,sun50i-h6-vpu-g2.yaml

diff --git a/Documentation/devicetree/bindings/media/allwinner,sun50i-h6-vpu-g2.yaml b/Documentation/devicetree/bindings/media/allwinner,sun50i-h6-vpu-g2.yaml
new file mode 100644
index 000000000000..24d7bf21499e
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/allwinner,sun50i-h6-vpu-g2.yaml
@@ -0,0 +1,64 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/media/allwinner,sun50i-h6-vpu-g2.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Hantro G2 VPU codec implemented on Allwinner H6 SoC
+
+maintainers:
+  - Jernej Skrabec <jernej.skrabec@gmail.com>
+
+description:
+  Hantro G2 video decode accelerator present on Allwinner H6 SoC.
+
+properties:
+  compatible:
+    const: allwinner,sun50i-h6-vpu-g2
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: Bus Clock
+      - description: Module Clock
+
+  clock-names:
+    items:
+      - const: bus
+      - const: mod
+
+  resets:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - resets
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/sun50i-h6-ccu.h>
+    #include <dt-bindings/reset/sun50i-h6-ccu.h>
+
+    video-codec-g2@1c00000 {
+        compatible = "allwinner,sun50i-h6-vpu-g2";
+        reg = <0x01c00000 0x1000>;
+        interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&ccu CLK_BUS_VP9>, <&ccu CLK_VP9>;
+        clock-names = "bus", "mod";
+        resets = <&ccu RST_BUS_VP9>;
+    };
+
+...
-- 
2.34.0


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

^ permalink raw reply related	[flat|nested] 40+ messages in thread
* [PATCH 6/7] media: hantro: Add support for Allwinner H6
  2021-11-22 18:46 ` Jernej Skrabec
@ 2021-11-22 18:47   ` Jernej Skrabec
  -1 siblings, 0 replies; 40+ messages in thread
From: Jernej Skrabec @ 2021-11-22 18:47 UTC (permalink / raw)
  To: linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, andrzej.p, gregkh, devicetree, linux-arm-kernel,
	linux-sunxi, linux-kernel, linux-staging, Jernej Skrabec

Allwinner H6 has a Hantro G2 core used for VP9 decoding. It's not clear
at this time if HEVC is also supported or not.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/staging/media/hantro/Kconfig        |  10 +-
 drivers/staging/media/hantro/Makefile       |   3 +
 drivers/staging/media/hantro/hantro_drv.c   |   3 +
 drivers/staging/media/hantro/hantro_hw.h    |   1 +
 drivers/staging/media/hantro/sunxi_vpu_hw.c | 104 ++++++++++++++++++++
 5 files changed, 120 insertions(+), 1 deletion(-)
 create mode 100644 drivers/staging/media/hantro/sunxi_vpu_hw.c

diff --git a/drivers/staging/media/hantro/Kconfig b/drivers/staging/media/hantro/Kconfig
index 00a57d88c92e..3c5d833322c8 100644
--- a/drivers/staging/media/hantro/Kconfig
+++ b/drivers/staging/media/hantro/Kconfig
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 config VIDEO_HANTRO
 	tristate "Hantro VPU driver"
-	depends on ARCH_MXC || ARCH_ROCKCHIP || ARCH_AT91 || COMPILE_TEST
+	depends on ARCH_MXC || ARCH_ROCKCHIP || ARCH_AT91 || ARCH_SUNXI || COMPILE_TEST
 	depends on VIDEO_DEV && VIDEO_V4L2
 	select MEDIA_CONTROLLER
 	select MEDIA_CONTROLLER_REQUEST_API
@@ -40,3 +40,11 @@ config VIDEO_HANTRO_ROCKCHIP
 	default y
 	help
 	  Enable support for RK3288, RK3328, and RK3399 SoCs.
+
+config VIDEO_HANTRO_SUNXI
+	bool "Hantro VPU Allwinner support"
+	depends on VIDEO_HANTRO
+	depends on ARCH_SUNXI || COMPILE_TEST
+	default y
+	help
+	  Enable support for H6 SoC.
diff --git a/drivers/staging/media/hantro/Makefile b/drivers/staging/media/hantro/Makefile
index 28af0a1ee4bf..ebd5ede7bef7 100644
--- a/drivers/staging/media/hantro/Makefile
+++ b/drivers/staging/media/hantro/Makefile
@@ -33,3 +33,6 @@ hantro-vpu-$(CONFIG_VIDEO_HANTRO_SAMA5D4) += \
 
 hantro-vpu-$(CONFIG_VIDEO_HANTRO_ROCKCHIP) += \
 		rockchip_vpu_hw.o
+
+hantro-vpu-$(CONFIG_VIDEO_HANTRO_SUNXI) += \
+		sunxi_vpu_hw.o
diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index 530994ab3024..54f34644ecdf 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -620,6 +620,9 @@ static const struct of_device_id of_hantro_match[] = {
 #endif
 #ifdef CONFIG_VIDEO_HANTRO_SAMA5D4
 	{ .compatible = "microchip,sama5d4-vdec", .data = &sama5d4_vdec_variant, },
+#endif
+#ifdef CONFIG_VIDEO_HANTRO_SUNXI
+	{ .compatible = "allwinner,sun50i-h6-vpu-g2", .data = &sunxi_vpu_variant, },
 #endif
 	{ /* sentinel */ }
 };
diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
index dbe51303724b..0676989b986b 100644
--- a/drivers/staging/media/hantro/hantro_hw.h
+++ b/drivers/staging/media/hantro/hantro_hw.h
@@ -308,6 +308,7 @@ extern const struct hantro_variant rk3288_vpu_variant;
 extern const struct hantro_variant rk3328_vpu_variant;
 extern const struct hantro_variant rk3399_vpu_variant;
 extern const struct hantro_variant sama5d4_vdec_variant;
+extern const struct hantro_variant sunxi_vpu_variant;
 
 extern const struct hantro_postproc_ops hantro_g1_postproc_ops;
 extern const struct hantro_postproc_ops hantro_g2_postproc_ops;
diff --git a/drivers/staging/media/hantro/sunxi_vpu_hw.c b/drivers/staging/media/hantro/sunxi_vpu_hw.c
new file mode 100644
index 000000000000..05e964dc0499
--- /dev/null
+++ b/drivers/staging/media/hantro/sunxi_vpu_hw.c
@@ -0,0 +1,104 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Allwinner Hantro G2 VPU codec driver
+ *
+ * Copyright (C) 2021 Jernej Skrabec <jernej.skrabec@gmail.com>
+ */
+
+#include <linux/clk.h>
+
+#include "hantro.h"
+#include "hantro_g2_regs.h"
+
+static const struct hantro_fmt sunxi_vpu_postproc_fmts[] = {
+	{
+		.fourcc = V4L2_PIX_FMT_NV12,
+		.codec_mode = HANTRO_MODE_NONE,
+		.postprocessed = true,
+	},
+};
+
+static const struct hantro_fmt sunxi_vpu_dec_fmts[] = {
+	{
+		.fourcc = V4L2_PIX_FMT_NV12_4L4,
+		.codec_mode = HANTRO_MODE_NONE,
+	},
+	{
+		.fourcc = V4L2_PIX_FMT_VP9_FRAME,
+		.codec_mode = HANTRO_MODE_VP9_DEC,
+		.max_depth = 2,
+		.frmsize = {
+			.min_width = 48,
+			.max_width = 3840,
+			.step_width = MB_DIM,
+			.min_height = 48,
+			.max_height = 2160,
+			.step_height = MB_DIM,
+		},
+	},
+};
+
+static irqreturn_t sunxi_vpu_irq(int irq, void *dev_id)
+{
+	struct hantro_dev *vpu = dev_id;
+	enum vb2_buffer_state state;
+	u32 status;
+
+	status = vdpu_read(vpu, G2_REG_INTERRUPT);
+	state = (status & G2_REG_INTERRUPT_DEC_RDY_INT) ?
+		 VB2_BUF_STATE_DONE : VB2_BUF_STATE_ERROR;
+
+	vdpu_write(vpu, 0, G2_REG_INTERRUPT);
+	vdpu_write(vpu, G2_REG_CONFIG_DEC_CLK_GATE_E, G2_REG_CONFIG);
+
+	hantro_irq_done(vpu, state);
+
+	return IRQ_HANDLED;
+}
+
+static int sunxi_vpu_hw_init(struct hantro_dev *vpu)
+{
+	clk_set_rate(vpu->clocks[0].clk, 300000000);
+
+	return 0;
+}
+
+static void sunxi_vpu_reset(struct hantro_ctx *ctx)
+{
+	struct hantro_dev *vpu = ctx->dev;
+
+	reset_control_reset(vpu->resets);
+}
+
+static const struct hantro_codec_ops sunxi_vpu_codec_ops[] = {
+	[HANTRO_MODE_VP9_DEC] = {
+		.run = hantro_g2_vp9_dec_run,
+		.done = hantro_g2_vp9_dec_done,
+		.reset = sunxi_vpu_reset,
+		.init = hantro_vp9_dec_init,
+		.exit = hantro_vp9_dec_exit,
+	},
+};
+
+static const struct hantro_irq sunxi_irqs[] = {
+	{ NULL, sunxi_vpu_irq },
+};
+
+static const char * const sunxi_clk_names[] = { "mod", "bus" };
+
+const struct hantro_variant sunxi_vpu_variant = {
+	.dec_fmts = sunxi_vpu_dec_fmts,
+	.num_dec_fmts = ARRAY_SIZE(sunxi_vpu_dec_fmts),
+	.postproc_fmts = sunxi_vpu_postproc_fmts,
+	.num_postproc_fmts = ARRAY_SIZE(sunxi_vpu_postproc_fmts),
+	.postproc_ops = &hantro_g2_postproc_ops,
+	.codec = HANTRO_VP9_DECODER,
+	.codec_ops = sunxi_vpu_codec_ops,
+	.init = sunxi_vpu_hw_init,
+	.irqs = sunxi_irqs,
+	.num_irqs = ARRAY_SIZE(sunxi_irqs),
+	.clk_names = sunxi_clk_names,
+	.num_clocks = ARRAY_SIZE(sunxi_clk_names),
+	.double_buffer = 1,
+	.legacy_regs = 1,
+};
-- 
2.34.0


^ permalink raw reply related	[flat|nested] 40+ messages in thread
* [PATCH 6/7] media: hantro: Add support for Allwinner H6
@ 2021-11-22 18:47   ` Jernej Skrabec
  0 siblings, 0 replies; 40+ messages in thread
From: Jernej Skrabec @ 2021-11-22 18:47 UTC (permalink / raw)
  To: linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, andrzej.p, gregkh, devicetree, linux-arm-kernel,
	linux-sunxi, linux-kernel, linux-staging, Jernej Skrabec

Allwinner H6 has a Hantro G2 core used for VP9 decoding. It's not clear
at this time if HEVC is also supported or not.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/staging/media/hantro/Kconfig        |  10 +-
 drivers/staging/media/hantro/Makefile       |   3 +
 drivers/staging/media/hantro/hantro_drv.c   |   3 +
 drivers/staging/media/hantro/hantro_hw.h    |   1 +
 drivers/staging/media/hantro/sunxi_vpu_hw.c | 104 ++++++++++++++++++++
 5 files changed, 120 insertions(+), 1 deletion(-)
 create mode 100644 drivers/staging/media/hantro/sunxi_vpu_hw.c

diff --git a/drivers/staging/media/hantro/Kconfig b/drivers/staging/media/hantro/Kconfig
index 00a57d88c92e..3c5d833322c8 100644
--- a/drivers/staging/media/hantro/Kconfig
+++ b/drivers/staging/media/hantro/Kconfig
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 config VIDEO_HANTRO
 	tristate "Hantro VPU driver"
-	depends on ARCH_MXC || ARCH_ROCKCHIP || ARCH_AT91 || COMPILE_TEST
+	depends on ARCH_MXC || ARCH_ROCKCHIP || ARCH_AT91 || ARCH_SUNXI || COMPILE_TEST
 	depends on VIDEO_DEV && VIDEO_V4L2
 	select MEDIA_CONTROLLER
 	select MEDIA_CONTROLLER_REQUEST_API
@@ -40,3 +40,11 @@ config VIDEO_HANTRO_ROCKCHIP
 	default y
 	help
 	  Enable support for RK3288, RK3328, and RK3399 SoCs.
+
+config VIDEO_HANTRO_SUNXI
+	bool "Hantro VPU Allwinner support"
+	depends on VIDEO_HANTRO
+	depends on ARCH_SUNXI || COMPILE_TEST
+	default y
+	help
+	  Enable support for H6 SoC.
diff --git a/drivers/staging/media/hantro/Makefile b/drivers/staging/media/hantro/Makefile
index 28af0a1ee4bf..ebd5ede7bef7 100644
--- a/drivers/staging/media/hantro/Makefile
+++ b/drivers/staging/media/hantro/Makefile
@@ -33,3 +33,6 @@ hantro-vpu-$(CONFIG_VIDEO_HANTRO_SAMA5D4) += \
 
 hantro-vpu-$(CONFIG_VIDEO_HANTRO_ROCKCHIP) += \
 		rockchip_vpu_hw.o
+
+hantro-vpu-$(CONFIG_VIDEO_HANTRO_SUNXI) += \
+		sunxi_vpu_hw.o
diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index 530994ab3024..54f34644ecdf 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -620,6 +620,9 @@ static const struct of_device_id of_hantro_match[] = {
 #endif
 #ifdef CONFIG_VIDEO_HANTRO_SAMA5D4
 	{ .compatible = "microchip,sama5d4-vdec", .data = &sama5d4_vdec_variant, },
+#endif
+#ifdef CONFIG_VIDEO_HANTRO_SUNXI
+	{ .compatible = "allwinner,sun50i-h6-vpu-g2", .data = &sunxi_vpu_variant, },
 #endif
 	{ /* sentinel */ }
 };
diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
index dbe51303724b..0676989b986b 100644
--- a/drivers/staging/media/hantro/hantro_hw.h
+++ b/drivers/staging/media/hantro/hantro_hw.h
@@ -308,6 +308,7 @@ extern const struct hantro_variant rk3288_vpu_variant;
 extern const struct hantro_variant rk3328_vpu_variant;
 extern const struct hantro_variant rk3399_vpu_variant;
 extern const struct hantro_variant sama5d4_vdec_variant;
+extern const struct hantro_variant sunxi_vpu_variant;
 
 extern const struct hantro_postproc_ops hantro_g1_postproc_ops;
 extern const struct hantro_postproc_ops hantro_g2_postproc_ops;
diff --git a/drivers/staging/media/hantro/sunxi_vpu_hw.c b/drivers/staging/media/hantro/sunxi_vpu_hw.c
new file mode 100644
index 000000000000..05e964dc0499
--- /dev/null
+++ b/drivers/staging/media/hantro/sunxi_vpu_hw.c
@@ -0,0 +1,104 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Allwinner Hantro G2 VPU codec driver
+ *
+ * Copyright (C) 2021 Jernej Skrabec <jernej.skrabec@gmail.com>
+ */
+
+#include <linux/clk.h>
+
+#include "hantro.h"
+#include "hantro_g2_regs.h"
+
+static const struct hantro_fmt sunxi_vpu_postproc_fmts[] = {
+	{
+		.fourcc = V4L2_PIX_FMT_NV12,
+		.codec_mode = HANTRO_MODE_NONE,
+		.postprocessed = true,
+	},
+};
+
+static const struct hantro_fmt sunxi_vpu_dec_fmts[] = {
+	{
+		.fourcc = V4L2_PIX_FMT_NV12_4L4,
+		.codec_mode = HANTRO_MODE_NONE,
+	},
+	{
+		.fourcc = V4L2_PIX_FMT_VP9_FRAME,
+		.codec_mode = HANTRO_MODE_VP9_DEC,
+		.max_depth = 2,
+		.frmsize = {
+			.min_width = 48,
+			.max_width = 3840,
+			.step_width = MB_DIM,
+			.min_height = 48,
+			.max_height = 2160,
+			.step_height = MB_DIM,
+		},
+	},
+};
+
+static irqreturn_t sunxi_vpu_irq(int irq, void *dev_id)
+{
+	struct hantro_dev *vpu = dev_id;
+	enum vb2_buffer_state state;
+	u32 status;
+
+	status = vdpu_read(vpu, G2_REG_INTERRUPT);
+	state = (status & G2_REG_INTERRUPT_DEC_RDY_INT) ?
+		 VB2_BUF_STATE_DONE : VB2_BUF_STATE_ERROR;
+
+	vdpu_write(vpu, 0, G2_REG_INTERRUPT);
+	vdpu_write(vpu, G2_REG_CONFIG_DEC_CLK_GATE_E, G2_REG_CONFIG);
+
+	hantro_irq_done(vpu, state);
+
+	return IRQ_HANDLED;
+}
+
+static int sunxi_vpu_hw_init(struct hantro_dev *vpu)
+{
+	clk_set_rate(vpu->clocks[0].clk, 300000000);
+
+	return 0;
+}
+
+static void sunxi_vpu_reset(struct hantro_ctx *ctx)
+{
+	struct hantro_dev *vpu = ctx->dev;
+
+	reset_control_reset(vpu->resets);
+}
+
+static const struct hantro_codec_ops sunxi_vpu_codec_ops[] = {
+	[HANTRO_MODE_VP9_DEC] = {
+		.run = hantro_g2_vp9_dec_run,
+		.done = hantro_g2_vp9_dec_done,
+		.reset = sunxi_vpu_reset,
+		.init = hantro_vp9_dec_init,
+		.exit = hantro_vp9_dec_exit,
+	},
+};
+
+static const struct hantro_irq sunxi_irqs[] = {
+	{ NULL, sunxi_vpu_irq },
+};
+
+static const char * const sunxi_clk_names[] = { "mod", "bus" };
+
+const struct hantro_variant sunxi_vpu_variant = {
+	.dec_fmts = sunxi_vpu_dec_fmts,
+	.num_dec_fmts = ARRAY_SIZE(sunxi_vpu_dec_fmts),
+	.postproc_fmts = sunxi_vpu_postproc_fmts,
+	.num_postproc_fmts = ARRAY_SIZE(sunxi_vpu_postproc_fmts),
+	.postproc_ops = &hantro_g2_postproc_ops,
+	.codec = HANTRO_VP9_DECODER,
+	.codec_ops = sunxi_vpu_codec_ops,
+	.init = sunxi_vpu_hw_init,
+	.irqs = sunxi_irqs,
+	.num_irqs = ARRAY_SIZE(sunxi_irqs),
+	.clk_names = sunxi_clk_names,
+	.num_clocks = ARRAY_SIZE(sunxi_clk_names),
+	.double_buffer = 1,
+	.legacy_regs = 1,
+};
-- 
2.34.0


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

^ permalink raw reply related	[flat|nested] 40+ messages in thread
* [PATCH 7/7] arm64: dts: allwinner: h6: Add Hantro G2 node
  2021-11-22 18:46 ` Jernej Skrabec
@ 2021-11-22 18:47   ` Jernej Skrabec
  -1 siblings, 0 replies; 40+ messages in thread
From: Jernej Skrabec @ 2021-11-22 18:47 UTC (permalink / raw)
  To: linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, andrzej.p, gregkh, devicetree, linux-arm-kernel,
	linux-sunxi, linux-kernel, linux-staging, Jernej Skrabec

H6 SoC has a second VPU, dedicated to VP9 decoding. It's a slightly
older design, though.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
index 4c4547f7d0c7..878061e75098 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
@@ -153,6 +153,15 @@ mixer0_out_tcon_top_mixer0: endpoint {
 			};
 		};
 
+		video-codec-g2@1c00000 {
+			compatible = "allwinner,sun50i-h6-vpu-g2";
+			reg = <0x01c00000 0x1000>;
+			interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_VP9>, <&ccu CLK_VP9>;
+			clock-names = "bus", "mod";
+			resets = <&ccu RST_BUS_VP9>;
+		};
+
 		video-codec@1c0e000 {
 			compatible = "allwinner,sun50i-h6-video-engine";
 			reg = <0x01c0e000 0x2000>;
-- 
2.34.0


^ permalink raw reply related	[flat|nested] 40+ messages in thread
* [PATCH 7/7] arm64: dts: allwinner: h6: Add Hantro G2 node
@ 2021-11-22 18:47   ` Jernej Skrabec
  0 siblings, 0 replies; 40+ messages in thread
From: Jernej Skrabec @ 2021-11-22 18:47 UTC (permalink / raw)
  To: linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, andrzej.p, gregkh, devicetree, linux-arm-kernel,
	linux-sunxi, linux-kernel, linux-staging, Jernej Skrabec

H6 SoC has a second VPU, dedicated to VP9 decoding. It's a slightly
older design, though.

Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
index 4c4547f7d0c7..878061e75098 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
@@ -153,6 +153,15 @@ mixer0_out_tcon_top_mixer0: endpoint {
 			};
 		};
 
+		video-codec-g2@1c00000 {
+			compatible = "allwinner,sun50i-h6-vpu-g2";
+			reg = <0x01c00000 0x1000>;
+			interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_VP9>, <&ccu CLK_VP9>;
+			clock-names = "bus", "mod";
+			resets = <&ccu RST_BUS_VP9>;
+		};
+
 		video-codec@1c0e000 {
 			compatible = "allwinner,sun50i-h6-video-engine";
 			reg = <0x01c0e000 0x2000>;
-- 
2.34.0


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

^ permalink raw reply related	[flat|nested] 40+ messages in thread
* Re: [PATCH 0/7] media: hantro: add Allwinner H6 support
  2021-11-22 18:46 ` Jernej Skrabec
@ 2021-11-22 18:51   ` Jernej Škrabec
  -1 siblings, 0 replies; 40+ messages in thread
From: Jernej Škrabec @ 2021-11-22 18:51 UTC (permalink / raw)
  To: linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, andrzej.p, gregkh, devicetree, linux-arm-kernel,
	linux-sunxi, linux-kernel, linux-staging

Dne ponedeljek, 22. november 2021 ob 19:46:55 CET je Jernej Skrabec 
napisal(a):
> Hi everyone!
> 
> Here is patchset which adds support for Hantro G2 core found in Allwinner
> H6 SoC. It is slightly older core, so it needs few quirks to be
> implemented:
> 1. It uses slightly different register layout in some cases. However, those
>    differences are small, so it makes sense only to add quirks.
> 2. It doesn't use ring buffer for bitstream as newer variants.
> 3. It needs double buffering to be enabled in order to work correctly.
> 4. postproc must be enabled at the end of the job. It seems that core has
>    some issues with latching register values if postproc registers are set
>    at the beginning of the job
> 
> legacy_regs quirk could be split into 3, like legacy_regs, ring_buffer and
> late_postproc, but I didn't see the need for that. I examined vendor
> sources at [1] and it suggests that legacy_regs implies no ring buffer.
> 
> It's also unclear if core supports HEVC decoding or not. This can be
> implemented later. VP9 10-bit decoding support is mentioned in manual, but
> it doesn't work at the moment. This will be addressed later.
> 
> Please take a look.
> 
> Best regards,
> Jernej
> 
> [1] https://github.com/CliveLau1990/imx-vpu-hantro

Forgot to add, this series is based on top of:
https://www.spinics.net/lists/linux-media/msg202448.html

Regards,
Jernej

> 
> Jernej Skrabec (7):
>   media: hantro: add support for reset lines
>   media: hantro: vp9: use double buffering if needed
>   media: hantro: vp9: add support for legacy register set
>   media: hantro: move postproc enablement for old cores
>   media: dt-bindings: allwinner: document H6 Hantro G2 binding
>   media: hantro: Add support for Allwinner H6
>   arm64: dts: allwinner: h6: Add Hantro G2 node
> 
>  .../media/allwinner,sun50i-h6-vpu-g2.yaml     |  64 +++++++++++
>  arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi  |   9 ++
>  drivers/staging/media/hantro/Kconfig          |  10 +-
>  drivers/staging/media/hantro/Makefile         |   3 +
>  drivers/staging/media/hantro/hantro.h         |   7 ++
>  drivers/staging/media/hantro/hantro_drv.c     |  27 ++++-
>  drivers/staging/media/hantro/hantro_g2_regs.h |  20 ++++
>  .../staging/media/hantro/hantro_g2_vp9_dec.c  |  76 ++++++++++---
>  drivers/staging/media/hantro/hantro_hw.h      |   1 +
>  drivers/staging/media/hantro/sunxi_vpu_hw.c   | 104 ++++++++++++++++++
>  10 files changed, 301 insertions(+), 20 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/media/
allwinner,sun50i-h6-vpu-g2.yaml
>  create mode 100644 drivers/staging/media/hantro/sunxi_vpu_hw.c
> 
> -- 
> 2.34.0
> 
> 



^ permalink raw reply	[flat|nested] 40+ messages in thread
* Re: [PATCH 0/7] media: hantro: add Allwinner H6 support
@ 2021-11-22 18:51   ` Jernej Škrabec
  0 siblings, 0 replies; 40+ messages in thread
From: Jernej Škrabec @ 2021-11-22 18:51 UTC (permalink / raw)
  To: linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, andrzej.p, gregkh, devicetree, linux-arm-kernel,
	linux-sunxi, linux-kernel, linux-staging

Dne ponedeljek, 22. november 2021 ob 19:46:55 CET je Jernej Skrabec 
napisal(a):
> Hi everyone!
> 
> Here is patchset which adds support for Hantro G2 core found in Allwinner
> H6 SoC. It is slightly older core, so it needs few quirks to be
> implemented:
> 1. It uses slightly different register layout in some cases. However, those
>    differences are small, so it makes sense only to add quirks.
> 2. It doesn't use ring buffer for bitstream as newer variants.
> 3. It needs double buffering to be enabled in order to work correctly.
> 4. postproc must be enabled at the end of the job. It seems that core has
>    some issues with latching register values if postproc registers are set
>    at the beginning of the job
> 
> legacy_regs quirk could be split into 3, like legacy_regs, ring_buffer and
> late_postproc, but I didn't see the need for that. I examined vendor
> sources at [1] and it suggests that legacy_regs implies no ring buffer.
> 
> It's also unclear if core supports HEVC decoding or not. This can be
> implemented later. VP9 10-bit decoding support is mentioned in manual, but
> it doesn't work at the moment. This will be addressed later.
> 
> Please take a look.
> 
> Best regards,
> Jernej
> 
> [1] https://github.com/CliveLau1990/imx-vpu-hantro

Forgot to add, this series is based on top of:
https://www.spinics.net/lists/linux-media/msg202448.html

Regards,
Jernej

> 
> Jernej Skrabec (7):
>   media: hantro: add support for reset lines
>   media: hantro: vp9: use double buffering if needed
>   media: hantro: vp9: add support for legacy register set
>   media: hantro: move postproc enablement for old cores
>   media: dt-bindings: allwinner: document H6 Hantro G2 binding
>   media: hantro: Add support for Allwinner H6
>   arm64: dts: allwinner: h6: Add Hantro G2 node
> 
>  .../media/allwinner,sun50i-h6-vpu-g2.yaml     |  64 +++++++++++
>  arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi  |   9 ++
>  drivers/staging/media/hantro/Kconfig          |  10 +-
>  drivers/staging/media/hantro/Makefile         |   3 +
>  drivers/staging/media/hantro/hantro.h         |   7 ++
>  drivers/staging/media/hantro/hantro_drv.c     |  27 ++++-
>  drivers/staging/media/hantro/hantro_g2_regs.h |  20 ++++
>  .../staging/media/hantro/hantro_g2_vp9_dec.c  |  76 ++++++++++---
>  drivers/staging/media/hantro/hantro_hw.h      |   1 +
>  drivers/staging/media/hantro/sunxi_vpu_hw.c   | 104 ++++++++++++++++++
>  10 files changed, 301 insertions(+), 20 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/media/
allwinner,sun50i-h6-vpu-g2.yaml
>  create mode 100644 drivers/staging/media/hantro/sunxi_vpu_hw.c
> 
> -- 
> 2.34.0
> 
> 



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

^ permalink raw reply	[flat|nested] 40+ messages in thread
* Re: [PATCH 1/7] media: hantro: add support for reset lines
  2021-11-22 18:46   ` Jernej Skrabec
@ 2021-11-23 11:09     ` Andrzej Pietrasiewicz
  -1 siblings, 0 replies; 40+ messages in thread
From: Andrzej Pietrasiewicz @ 2021-11-23 11:09 UTC (permalink / raw)
  To: Jernej Skrabec, linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, gregkh, devicetree, linux-arm-kernel, linux-sunxi,
	linux-kernel, linux-staging

Hi Jernej,

Thanks for the patch.

W dniu 22.11.2021 o 19:46, Jernej Skrabec pisze:
> Some SoCs like Allwinner H6 use reset lines for resetting Hantro G2. Add
> support for them.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>   drivers/staging/media/hantro/hantro.h     |  3 +++
>   drivers/staging/media/hantro/hantro_drv.c | 15 ++++++++++++++-
>   2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h
> index 7da23f7f207a..33eb3e092cc1 100644
> --- a/drivers/staging/media/hantro/hantro.h
> +++ b/drivers/staging/media/hantro/hantro.h
> @@ -16,6 +16,7 @@
>   #include <linux/videodev2.h>
>   #include <linux/wait.h>
>   #include <linux/clk.h>
> +#include <linux/reset.h>
>   
>   #include <media/v4l2-ctrls.h>
>   #include <media/v4l2-device.h>
> @@ -171,6 +172,7 @@ hantro_vdev_to_func(struct video_device *vdev)
>    * @dev:		Pointer to device for convenient logging using
>    *			dev_ macros.
>    * @clocks:		Array of clock handles.
> + * @resets:		Array of reset handles.
>    * @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.
> @@ -190,6 +192,7 @@ struct hantro_dev {
>   	struct platform_device *pdev;
>   	struct device *dev;
>   	struct clk_bulk_data *clocks;
> +	struct reset_control *resets;
>   	void __iomem **reg_bases;
>   	void __iomem *enc_base;
>   	void __iomem *dec_base;
> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> index ab2467998d29..8c3de31f51b3 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -905,6 +905,10 @@ static int hantro_probe(struct platform_device *pdev)
>   			return PTR_ERR(vpu->clocks[0].clk);
>   	}
>   
> +	vpu->resets = devm_reset_control_array_get(&pdev->dev, false, true);
> +	if (IS_ERR(vpu->resets))
> +		return PTR_ERR(vpu->resets);
> +
>   	num_bases = vpu->variant->num_regs ?: 1;
>   	vpu->reg_bases = devm_kcalloc(&pdev->dev, num_bases,
>   				      sizeof(*vpu->reg_bases), GFP_KERNEL);
> @@ -978,10 +982,16 @@ static int hantro_probe(struct platform_device *pdev)
>   	pm_runtime_use_autosuspend(vpu->dev);
>   	pm_runtime_enable(vpu->dev);
>   
> +	ret = reset_control_deassert(vpu->resets);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to deassert resets\n");
> +		return ret;
> +	}
> +
>   	ret = clk_bulk_prepare(vpu->variant->num_clocks, vpu->clocks);
>   	if (ret) {
>   		dev_err(&pdev->dev, "Failed to prepare clocks\n");
> -		return ret;
> +		goto err_rst_assert;

Before your patch is applied if clk_bulk_prepare() fails, we
simply return on the spot. After the patch is applied not only
do you...

>   	}
>   
>   	ret = v4l2_device_register(&pdev->dev, &vpu->v4l2_dev);
> @@ -1037,6 +1047,8 @@ static int hantro_probe(struct platform_device *pdev)
>   	v4l2_device_unregister(&vpu->v4l2_dev);
>   err_clk_unprepare:
>   	clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
> +err_rst_assert:
> +	reset_control_assert(vpu->resets);

...revert the effect of reset_control_deassert(), you also...

>   	pm_runtime_dont_use_autosuspend(vpu->dev);
>   	pm_runtime_disable(vpu->dev);

... do pm_*() stuff. Is there any reason why this is needed?

Andrzej

>   	return ret;
> @@ -1055,6 +1067,7 @@ static int hantro_remove(struct platform_device *pdev)
>   	v4l2_m2m_release(vpu->m2m_dev);
>   	v4l2_device_unregister(&vpu->v4l2_dev);
>   	clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
> +	reset_control_assert(vpu->resets);
>   	pm_runtime_dont_use_autosuspend(vpu->dev);
>   	pm_runtime_disable(vpu->dev);
>   	return 0;
> 


^ permalink raw reply	[flat|nested] 40+ messages in thread
* Re: [PATCH 1/7] media: hantro: add support for reset lines
@ 2021-11-23 11:09     ` Andrzej Pietrasiewicz
  0 siblings, 0 replies; 40+ messages in thread
From: Andrzej Pietrasiewicz @ 2021-11-23 11:09 UTC (permalink / raw)
  To: Jernej Skrabec, linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, gregkh, devicetree, linux-arm-kernel, linux-sunxi,
	linux-kernel, linux-staging

Hi Jernej,

Thanks for the patch.

W dniu 22.11.2021 o 19:46, Jernej Skrabec pisze:
> Some SoCs like Allwinner H6 use reset lines for resetting Hantro G2. Add
> support for them.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>   drivers/staging/media/hantro/hantro.h     |  3 +++
>   drivers/staging/media/hantro/hantro_drv.c | 15 ++++++++++++++-
>   2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h
> index 7da23f7f207a..33eb3e092cc1 100644
> --- a/drivers/staging/media/hantro/hantro.h
> +++ b/drivers/staging/media/hantro/hantro.h
> @@ -16,6 +16,7 @@
>   #include <linux/videodev2.h>
>   #include <linux/wait.h>
>   #include <linux/clk.h>
> +#include <linux/reset.h>
>   
>   #include <media/v4l2-ctrls.h>
>   #include <media/v4l2-device.h>
> @@ -171,6 +172,7 @@ hantro_vdev_to_func(struct video_device *vdev)
>    * @dev:		Pointer to device for convenient logging using
>    *			dev_ macros.
>    * @clocks:		Array of clock handles.
> + * @resets:		Array of reset handles.
>    * @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.
> @@ -190,6 +192,7 @@ struct hantro_dev {
>   	struct platform_device *pdev;
>   	struct device *dev;
>   	struct clk_bulk_data *clocks;
> +	struct reset_control *resets;
>   	void __iomem **reg_bases;
>   	void __iomem *enc_base;
>   	void __iomem *dec_base;
> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> index ab2467998d29..8c3de31f51b3 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -905,6 +905,10 @@ static int hantro_probe(struct platform_device *pdev)
>   			return PTR_ERR(vpu->clocks[0].clk);
>   	}
>   
> +	vpu->resets = devm_reset_control_array_get(&pdev->dev, false, true);
> +	if (IS_ERR(vpu->resets))
> +		return PTR_ERR(vpu->resets);
> +
>   	num_bases = vpu->variant->num_regs ?: 1;
>   	vpu->reg_bases = devm_kcalloc(&pdev->dev, num_bases,
>   				      sizeof(*vpu->reg_bases), GFP_KERNEL);
> @@ -978,10 +982,16 @@ static int hantro_probe(struct platform_device *pdev)
>   	pm_runtime_use_autosuspend(vpu->dev);
>   	pm_runtime_enable(vpu->dev);
>   
> +	ret = reset_control_deassert(vpu->resets);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to deassert resets\n");
> +		return ret;
> +	}
> +
>   	ret = clk_bulk_prepare(vpu->variant->num_clocks, vpu->clocks);
>   	if (ret) {
>   		dev_err(&pdev->dev, "Failed to prepare clocks\n");
> -		return ret;
> +		goto err_rst_assert;

Before your patch is applied if clk_bulk_prepare() fails, we
simply return on the spot. After the patch is applied not only
do you...

>   	}
>   
>   	ret = v4l2_device_register(&pdev->dev, &vpu->v4l2_dev);
> @@ -1037,6 +1047,8 @@ static int hantro_probe(struct platform_device *pdev)
>   	v4l2_device_unregister(&vpu->v4l2_dev);
>   err_clk_unprepare:
>   	clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
> +err_rst_assert:
> +	reset_control_assert(vpu->resets);

...revert the effect of reset_control_deassert(), you also...

>   	pm_runtime_dont_use_autosuspend(vpu->dev);
>   	pm_runtime_disable(vpu->dev);

... do pm_*() stuff. Is there any reason why this is needed?

Andrzej

>   	return ret;
> @@ -1055,6 +1067,7 @@ static int hantro_remove(struct platform_device *pdev)
>   	v4l2_m2m_release(vpu->m2m_dev);
>   	v4l2_device_unregister(&vpu->v4l2_dev);
>   	clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
> +	reset_control_assert(vpu->resets);
>   	pm_runtime_dont_use_autosuspend(vpu->dev);
>   	pm_runtime_disable(vpu->dev);
>   	return 0;
> 


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

^ permalink raw reply	[flat|nested] 40+ messages in thread
* Re: [PATCH 2/7] media: hantro: vp9: use double buffering if needed
  2021-11-22 18:46   ` Jernej Skrabec
@ 2021-11-23 11:22     ` Andrzej Pietrasiewicz
  -1 siblings, 0 replies; 40+ messages in thread
From: Andrzej Pietrasiewicz @ 2021-11-23 11:22 UTC (permalink / raw)
  To: Jernej Skrabec, linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, gregkh, devicetree, linux-arm-kernel, linux-sunxi,
	linux-kernel, linux-staging

W dniu 22.11.2021 o 19:46, Jernej Skrabec pisze:
> Some G2 variants need double buffering to be enabled in order to work
> correctly, like that found in Allwinner H6 SoC.
> 
> Add platform quirk for that.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>

> ---
>   drivers/staging/media/hantro/hantro.h            | 2 ++
>   drivers/staging/media/hantro/hantro_g2_regs.h    | 1 +
>   drivers/staging/media/hantro/hantro_g2_vp9_dec.c | 2 ++
>   3 files changed, 5 insertions(+)
> 
> diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h
> index 33eb3e092cc1..d03824fa3222 100644
> --- a/drivers/staging/media/hantro/hantro.h
> +++ b/drivers/staging/media/hantro/hantro.h
> @@ -73,6 +73,7 @@ struct hantro_irq {
>    * @num_clocks:			number of clocks in the array
>    * @reg_names:			array of register range names
>    * @num_regs:			number of register range names in the array
> + * @double_buffer:		core needs double buffering
>    */
>   struct hantro_variant {
>   	unsigned int enc_offset;
> @@ -94,6 +95,7 @@ struct hantro_variant {
>   	int num_clocks;
>   	const char * const *reg_names;
>   	int num_regs;
> +	unsigned int double_buffer : 1;
>   };
>   
>   /**
> diff --git a/drivers/staging/media/hantro/hantro_g2_regs.h b/drivers/staging/media/hantro/hantro_g2_regs.h
> index 9c857dd1ad9b..15a391a4650e 100644
> --- a/drivers/staging/media/hantro/hantro_g2_regs.h
> +++ b/drivers/staging/media/hantro/hantro_g2_regs.h
> @@ -270,6 +270,7 @@
>   #define g2_apf_threshold	G2_DEC_REG(55, 0, 0xffff)
>   
>   #define g2_clk_gate_e		G2_DEC_REG(58, 16, 0x1)
> +#define g2_double_buffer_e	G2_DEC_REG(58, 15, 0x1)
>   #define g2_buswidth		G2_DEC_REG(58, 8,  0x7)
>   #define g2_max_burst		G2_DEC_REG(58, 0,  0xff)
>   
> diff --git a/drivers/staging/media/hantro/hantro_g2_vp9_dec.c b/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
> index e04242d10fa2..d4fc649a4da1 100644
> --- a/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
> @@ -847,6 +847,8 @@ config_registers(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_frame *dec_p
>   	hantro_reg_write(ctx->dev, &g2_clk_gate_e, 1);
>   	hantro_reg_write(ctx->dev, &g2_max_cb_size, 6);
>   	hantro_reg_write(ctx->dev, &g2_min_cb_size, 3);
> +	if (ctx->dev->variant->double_buffer)
> +		hantro_reg_write(ctx->dev, &g2_double_buffer_e, 1);
>   
>   	config_output(ctx, dst, dec_params);
>   
> 


^ permalink raw reply	[flat|nested] 40+ messages in thread
* Re: [PATCH 2/7] media: hantro: vp9: use double buffering if needed
@ 2021-11-23 11:22     ` Andrzej Pietrasiewicz
  0 siblings, 0 replies; 40+ messages in thread
From: Andrzej Pietrasiewicz @ 2021-11-23 11:22 UTC (permalink / raw)
  To: Jernej Skrabec, linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, gregkh, devicetree, linux-arm-kernel, linux-sunxi,
	linux-kernel, linux-staging

W dniu 22.11.2021 o 19:46, Jernej Skrabec pisze:
> Some G2 variants need double buffering to be enabled in order to work
> correctly, like that found in Allwinner H6 SoC.
> 
> Add platform quirk for that.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>

> ---
>   drivers/staging/media/hantro/hantro.h            | 2 ++
>   drivers/staging/media/hantro/hantro_g2_regs.h    | 1 +
>   drivers/staging/media/hantro/hantro_g2_vp9_dec.c | 2 ++
>   3 files changed, 5 insertions(+)
> 
> diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h
> index 33eb3e092cc1..d03824fa3222 100644
> --- a/drivers/staging/media/hantro/hantro.h
> +++ b/drivers/staging/media/hantro/hantro.h
> @@ -73,6 +73,7 @@ struct hantro_irq {
>    * @num_clocks:			number of clocks in the array
>    * @reg_names:			array of register range names
>    * @num_regs:			number of register range names in the array
> + * @double_buffer:		core needs double buffering
>    */
>   struct hantro_variant {
>   	unsigned int enc_offset;
> @@ -94,6 +95,7 @@ struct hantro_variant {
>   	int num_clocks;
>   	const char * const *reg_names;
>   	int num_regs;
> +	unsigned int double_buffer : 1;
>   };
>   
>   /**
> diff --git a/drivers/staging/media/hantro/hantro_g2_regs.h b/drivers/staging/media/hantro/hantro_g2_regs.h
> index 9c857dd1ad9b..15a391a4650e 100644
> --- a/drivers/staging/media/hantro/hantro_g2_regs.h
> +++ b/drivers/staging/media/hantro/hantro_g2_regs.h
> @@ -270,6 +270,7 @@
>   #define g2_apf_threshold	G2_DEC_REG(55, 0, 0xffff)
>   
>   #define g2_clk_gate_e		G2_DEC_REG(58, 16, 0x1)
> +#define g2_double_buffer_e	G2_DEC_REG(58, 15, 0x1)
>   #define g2_buswidth		G2_DEC_REG(58, 8,  0x7)
>   #define g2_max_burst		G2_DEC_REG(58, 0,  0xff)
>   
> diff --git a/drivers/staging/media/hantro/hantro_g2_vp9_dec.c b/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
> index e04242d10fa2..d4fc649a4da1 100644
> --- a/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
> @@ -847,6 +847,8 @@ config_registers(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_frame *dec_p
>   	hantro_reg_write(ctx->dev, &g2_clk_gate_e, 1);
>   	hantro_reg_write(ctx->dev, &g2_max_cb_size, 6);
>   	hantro_reg_write(ctx->dev, &g2_min_cb_size, 3);
> +	if (ctx->dev->variant->double_buffer)
> +		hantro_reg_write(ctx->dev, &g2_double_buffer_e, 1);
>   
>   	config_output(ctx, dst, dec_params);
>   
> 


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

^ permalink raw reply	[flat|nested] 40+ messages in thread
* Re: [PATCH 3/7] media: hantro: vp9: add support for legacy register set
  2021-11-22 18:46   ` Jernej Skrabec
@ 2021-11-23 12:06     ` Andrzej Pietrasiewicz
  -1 siblings, 0 replies; 40+ messages in thread
From: Andrzej Pietrasiewicz @ 2021-11-23 12:06 UTC (permalink / raw)
  To: Jernej Skrabec, linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, gregkh, devicetree, linux-arm-kernel, linux-sunxi,
	linux-kernel, linux-staging

Hi Jernej,

W dniu 22.11.2021 o 19:46, Jernej Skrabec pisze:
> Some older G2 cores uses slightly different register set for HEVC and
> VP9. Since vast majority of registers and logic is the same, it doesn't
> make sense to introduce another drivers.
> 
> Add legacy_regs quirk and implement only VP9 changes for now. HEVC
> changes will be introduced later, if needed.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>   drivers/staging/media/hantro/hantro.h         |  2 +
>   drivers/staging/media/hantro/hantro_g2_regs.h | 19 +++++
>   .../staging/media/hantro/hantro_g2_vp9_dec.c  | 74 ++++++++++++++-----
>   3 files changed, 78 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h
> index d03824fa3222..83ed25d9657b 100644
> --- a/drivers/staging/media/hantro/hantro.h
> +++ b/drivers/staging/media/hantro/hantro.h
> @@ -74,6 +74,7 @@ struct hantro_irq {
>    * @reg_names:			array of register range names
>    * @num_regs:			number of register range names in the array
>    * @double_buffer:		core needs double buffering
> + * @legacy_regs:		core uses legacy register set
>    */
>   struct hantro_variant {
>   	unsigned int enc_offset;
> @@ -96,6 +97,7 @@ struct hantro_variant {
>   	const char * const *reg_names;
>   	int num_regs;
>   	unsigned int double_buffer : 1;
> +	unsigned int legacy_regs : 1;
>   };
>   
>   /**
> diff --git a/drivers/staging/media/hantro/hantro_g2_regs.h b/drivers/staging/media/hantro/hantro_g2_regs.h
> index 15a391a4650e..d7c2ff05208e 100644
> --- a/drivers/staging/media/hantro/hantro_g2_regs.h
> +++ b/drivers/staging/media/hantro/hantro_g2_regs.h
> @@ -37,6 +37,13 @@
>   
>   #define g2_strm_swap		G2_DEC_REG(2, 28, 0xf)
>   #define g2_dirmv_swap		G2_DEC_REG(2, 20, 0xf)
> +/* used on older variants */
> +#define g2_strm_swap_old	G2_DEC_REG(2, 27, 0x1f)
> +#define g2_pic_swap		G2_DEC_REG(2, 22, 0x1f)
> +#define g2_dirmv_swap_old	G2_DEC_REG(2, 17, 0x1f)
> +#define g2_tab0_swap		G2_DEC_REG(2, 12, 0x1f)
> +#define g2_tab1_swap		G2_DEC_REG(2, 7, 0x1f)
> +#define g2_tab2_swap		G2_DEC_REG(2, 2, 0x1f)\

Please rename g2_tab[0-2]_swap to g2_tab[0-2]_swap_old. Similar names
exist in newer variants (even if not used at the moment).

It is rather difficult to come up with a consistent rule with regard
to in what sequence to arrange these register definitions. It seems
to me that you use a hybrid approach: if the definitions being added
fall "out of order" then you annotate with a comment about older variants,
and if they are "in order" then you simply fold them into their place.

I don't have a very strong opinion, but maybe they all should be
just "in bitfield order" and without comments?

>   
>   #define g2_mode			G2_DEC_REG(3, 27, 0x1f)
>   #define g2_compress_swap	G2_DEC_REG(3, 20, 0xf)
> @@ -45,6 +52,8 @@
>   #define g2_out_dis		G2_DEC_REG(3, 15, 0x1)
>   #define g2_out_filtering_dis	G2_DEC_REG(3, 14, 0x1)
>   #define g2_write_mvs_e		G2_DEC_REG(3, 12, 0x1)
> +#define g2_tab3_swap		G2_DEC_REG(3, 7, 0x1f)

g2_tab3_swap_old

With all the above addressed you can add my

Reviewied-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>

> +#define g2_rscan_swap		G2_DEC_REG(3, 2, 0x1f)
>   
>   #define g2_pic_width_in_cbs	G2_DEC_REG(4, 19, 0x1fff)
>   #define g2_pic_height_in_cbs	G2_DEC_REG(4, 6,  0x1fff)
> @@ -58,6 +67,7 @@
>   #define g2_tempor_mvp_e		G2_DEC_REG(5, 11, 0x1)
>   #define g2_max_cu_qpd_depth	G2_DEC_REG(5, 5,  0x3f)
>   #define g2_cu_qpd_e		G2_DEC_REG(5, 4,  0x1)
> +#define g2_pix_shift		G2_DEC_REG(5, 0,  0xf)
>   
>   #define g2_stream_len		G2_DEC_REG(6, 0,  0xffffffff)
>   
> @@ -80,6 +90,8 @@
>   
>   #define g2_const_intra_e	G2_DEC_REG(8, 31, 0x1)
>   #define g2_filt_ctrl_pres	G2_DEC_REG(8, 30, 0x1)
> +#define g2_bit_depth_y		G2_DEC_REG(8, 21, 0xf)
> +#define g2_bit_depth_c		G2_DEC_REG(8, 17, 0xf)
>   #define g2_idr_pic_e		G2_DEC_REG(8, 16, 0x1)
>   #define g2_bit_depth_pcm_y	G2_DEC_REG(8, 12, 0xf)
>   #define g2_bit_depth_pcm_c	G2_DEC_REG(8, 8,  0xf)
> @@ -87,6 +99,9 @@
>   #define g2_bit_depth_c_minus8	G2_DEC_REG(8, 4,  0x3)
>   #define g2_output_8_bits	G2_DEC_REG(8, 3,  0x1)
>   #define g2_output_format	G2_DEC_REG(8, 0,  0x7)
> +/* used on older variants */
> +#define g2_rs_out_bit_depth	G2_DEC_REG(8, 4,  0xf)
> +#define g2_pp_pix_shift		G2_DEC_REG(8, 0,  0xf)
>   
>   #define g2_refidx1_active	G2_DEC_REG(9, 19, 0x1f)
>   #define g2_refidx0_active	G2_DEC_REG(9, 14, 0x1f)
> @@ -98,6 +113,10 @@
>   #define g2_num_tile_rows	G2_DEC_REG(10, 14, 0x1f)
>   #define g2_tile_e		G2_DEC_REG(10, 1,  0x1)
>   #define g2_entropy_sync_e	G2_DEC_REG(10, 0,  0x1)
> +/* used on older variants */
> +#define g2_init_qp_old		G2_DEC_REG(10, 25, 0x3f)
> +#define g2_num_tile_cols_old	G2_DEC_REG(10, 20, 0x1f)
> +#define g2_num_tile_rows_old	G2_DEC_REG(10, 15, 0x1f)
>   
>   #define vp9_transform_mode	G2_DEC_REG(11, 27, 0x7)
>   #define vp9_filt_sharpness	G2_DEC_REG(11, 21, 0x7)
> diff --git a/drivers/staging/media/hantro/hantro_g2_vp9_dec.c b/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
> index d4fc649a4da1..5aac32700cd0 100644
> --- a/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
> @@ -150,7 +150,8 @@ static void config_output(struct hantro_ctx *ctx,
>   	dma_addr_t luma_addr, chroma_addr, mv_addr;
>   
>   	hantro_reg_write(ctx->dev, &g2_out_dis, 0);
> -	hantro_reg_write(ctx->dev, &g2_output_format, 0);
> +	if (!ctx->dev->variant->legacy_regs)
> +		hantro_reg_write(ctx->dev, &g2_output_format, 0);
>   
>   	luma_addr = hantro_get_dec_buf_addr(ctx, &dst->base.vb.vb2_buf);
>   	hantro_write_addr(ctx->dev, G2_OUT_LUMA_ADDR, luma_addr);
> @@ -327,6 +328,7 @@ config_tiles(struct hantro_ctx *ctx,
>   	struct hantro_aux_buf *tile_edge = &vp9_ctx->tile_edge;
>   	dma_addr_t addr;
>   	unsigned short *tile_mem;
> +	unsigned int rows, cols;
>   
>   	addr = misc->dma + vp9_ctx->tile_info_offset;
>   	hantro_write_addr(ctx->dev, G2_TILE_SIZES_ADDR, addr);
> @@ -344,17 +346,24 @@ config_tiles(struct hantro_ctx *ctx,
>   
>   		fill_tile_info(ctx, tile_r, tile_c, sbs_r, sbs_c, tile_mem);
>   
> +		cols = tile_c;
> +		rows = tile_r;
>   		hantro_reg_write(ctx->dev, &g2_tile_e, 1);
> -		hantro_reg_write(ctx->dev, &g2_num_tile_cols, tile_c);
> -		hantro_reg_write(ctx->dev, &g2_num_tile_rows, tile_r);
> -
>   	} else {
>   		tile_mem[0] = hantro_vp9_num_sbs(dst->vp9.width);
>   		tile_mem[1] = hantro_vp9_num_sbs(dst->vp9.height);
>   
> +		cols = 1;
> +		rows = 1;
>   		hantro_reg_write(ctx->dev, &g2_tile_e, 0);
> -		hantro_reg_write(ctx->dev, &g2_num_tile_cols, 1);
> -		hantro_reg_write(ctx->dev, &g2_num_tile_rows, 1);
> +	}
> +
> +	if (ctx->dev->variant->legacy_regs) {
> +		hantro_reg_write(ctx->dev, &g2_num_tile_cols_old, cols);
> +		hantro_reg_write(ctx->dev, &g2_num_tile_rows_old, rows);
> +	} else {
> +		hantro_reg_write(ctx->dev, &g2_num_tile_cols, cols);
> +		hantro_reg_write(ctx->dev, &g2_num_tile_rows, rows);
>   	}
>   
>   	/* provide aux buffers even if no tiles are used */
> @@ -505,8 +514,22 @@ static void config_picture_dimensions(struct hantro_ctx *ctx, struct hantro_deco
>   static void
>   config_bit_depth(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_frame *dec_params)
>   {
> -	hantro_reg_write(ctx->dev, &g2_bit_depth_y_minus8, dec_params->bit_depth - 8);
> -	hantro_reg_write(ctx->dev, &g2_bit_depth_c_minus8, dec_params->bit_depth - 8);
> +	if (ctx->dev->variant->legacy_regs) {
> +		u8 pp_shift = 0;
> +
> +		hantro_reg_write(ctx->dev, &g2_bit_depth_y, dec_params->bit_depth);
> +		hantro_reg_write(ctx->dev, &g2_bit_depth_c, dec_params->bit_depth);
> +		hantro_reg_write(ctx->dev, &g2_rs_out_bit_depth, dec_params->bit_depth);
> +
> +		if (dec_params->bit_depth > 8)
> +			pp_shift = 16 - dec_params->bit_depth;
> +
> +		hantro_reg_write(ctx->dev, &g2_pp_pix_shift, pp_shift);
> +		hantro_reg_write(ctx->dev, &g2_pix_shift, 0);
> +	} else {
> +		hantro_reg_write(ctx->dev, &g2_bit_depth_y_minus8, dec_params->bit_depth - 8);
> +		hantro_reg_write(ctx->dev, &g2_bit_depth_c_minus8, dec_params->bit_depth - 8);
> +	}
>   }
>   
>   static inline bool is_lossless(const struct v4l2_vp9_quantization *quant)
> @@ -784,9 +807,13 @@ config_source(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_frame *dec_para
>   		     + dec_params->compressed_header_size;
>   
>   	stream_base = vb2_dma_contig_plane_dma_addr(&vb2_src->vb2_buf, 0);
> -	hantro_write_addr(ctx->dev, G2_STREAM_ADDR, stream_base);
>   
>   	tmp_addr = stream_base + headres_size;
> +	if (ctx->dev->variant->legacy_regs)
> +		hantro_write_addr(ctx->dev, G2_STREAM_ADDR, (tmp_addr & ~0xf));
> +	else
> +		hantro_write_addr(ctx->dev, G2_STREAM_ADDR, stream_base);
> +
>   	start_bit = (tmp_addr & 0xf) * 8;
>   	hantro_reg_write(ctx->dev, &g2_start_bit, start_bit);
>   
> @@ -794,10 +821,12 @@ config_source(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_frame *dec_para
>   	src_len += start_bit / 8 - headres_size;
>   	hantro_reg_write(ctx->dev, &g2_stream_len, src_len);
>   
> -	tmp_addr &= ~0xf;
> -	hantro_reg_write(ctx->dev, &g2_strm_start_offset, tmp_addr - stream_base);
> -	src_buf_len = vb2_plane_size(&vb2_src->vb2_buf, 0);
> -	hantro_reg_write(ctx->dev, &g2_strm_buffer_len, src_buf_len);
> +	if (!ctx->dev->variant->legacy_regs) {
> +		tmp_addr &= ~0xf;
> +		hantro_reg_write(ctx->dev, &g2_strm_start_offset, tmp_addr - stream_base);
> +		src_buf_len = vb2_plane_size(&vb2_src->vb2_buf, 0);
> +		hantro_reg_write(ctx->dev, &g2_strm_buffer_len, src_buf_len);
> +	}
>   }
>   
>   static void
> @@ -837,13 +866,24 @@ config_registers(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_frame *dec_p
>   
>   	/* configure basic registers */
>   	hantro_reg_write(ctx->dev, &g2_mode, VP9_DEC_MODE);
> -	hantro_reg_write(ctx->dev, &g2_strm_swap, 0xf);
> -	hantro_reg_write(ctx->dev, &g2_dirmv_swap, 0xf);
> -	hantro_reg_write(ctx->dev, &g2_compress_swap, 0xf);
> +	if (!ctx->dev->variant->legacy_regs) {
> +		hantro_reg_write(ctx->dev, &g2_strm_swap, 0xf);
> +		hantro_reg_write(ctx->dev, &g2_dirmv_swap, 0xf);
> +		hantro_reg_write(ctx->dev, &g2_compress_swap, 0xf);
> +		hantro_reg_write(ctx->dev, &g2_ref_compress_bypass, 1);
> +	} else {
> +		hantro_reg_write(ctx->dev, &g2_strm_swap_old, 0x1f);
> +		hantro_reg_write(ctx->dev, &g2_pic_swap, 0x10);
> +		hantro_reg_write(ctx->dev, &g2_dirmv_swap_old, 0x10);
> +		hantro_reg_write(ctx->dev, &g2_tab0_swap, 0x10);
> +		hantro_reg_write(ctx->dev, &g2_tab1_swap, 0x10);
> +		hantro_reg_write(ctx->dev, &g2_tab2_swap, 0x10);
> +		hantro_reg_write(ctx->dev, &g2_tab3_swap, 0x10);
> +		hantro_reg_write(ctx->dev, &g2_rscan_swap, 0x10);
> +	}
>   	hantro_reg_write(ctx->dev, &g2_buswidth, BUS_WIDTH_128);
>   	hantro_reg_write(ctx->dev, &g2_max_burst, 16);
>   	hantro_reg_write(ctx->dev, &g2_apf_threshold, 8);
> -	hantro_reg_write(ctx->dev, &g2_ref_compress_bypass, 1);
>   	hantro_reg_write(ctx->dev, &g2_clk_gate_e, 1);
>   	hantro_reg_write(ctx->dev, &g2_max_cb_size, 6);
>   	hantro_reg_write(ctx->dev, &g2_min_cb_size, 3);
> 


^ permalink raw reply	[flat|nested] 40+ messages in thread
* Re: [PATCH 3/7] media: hantro: vp9: add support for legacy register set
@ 2021-11-23 12:06     ` Andrzej Pietrasiewicz
  0 siblings, 0 replies; 40+ messages in thread
From: Andrzej Pietrasiewicz @ 2021-11-23 12:06 UTC (permalink / raw)
  To: Jernej Skrabec, linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, gregkh, devicetree, linux-arm-kernel, linux-sunxi,
	linux-kernel, linux-staging

Hi Jernej,

W dniu 22.11.2021 o 19:46, Jernej Skrabec pisze:
> Some older G2 cores uses slightly different register set for HEVC and
> VP9. Since vast majority of registers and logic is the same, it doesn't
> make sense to introduce another drivers.
> 
> Add legacy_regs quirk and implement only VP9 changes for now. HEVC
> changes will be introduced later, if needed.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>   drivers/staging/media/hantro/hantro.h         |  2 +
>   drivers/staging/media/hantro/hantro_g2_regs.h | 19 +++++
>   .../staging/media/hantro/hantro_g2_vp9_dec.c  | 74 ++++++++++++++-----
>   3 files changed, 78 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro.h b/drivers/staging/media/hantro/hantro.h
> index d03824fa3222..83ed25d9657b 100644
> --- a/drivers/staging/media/hantro/hantro.h
> +++ b/drivers/staging/media/hantro/hantro.h
> @@ -74,6 +74,7 @@ struct hantro_irq {
>    * @reg_names:			array of register range names
>    * @num_regs:			number of register range names in the array
>    * @double_buffer:		core needs double buffering
> + * @legacy_regs:		core uses legacy register set
>    */
>   struct hantro_variant {
>   	unsigned int enc_offset;
> @@ -96,6 +97,7 @@ struct hantro_variant {
>   	const char * const *reg_names;
>   	int num_regs;
>   	unsigned int double_buffer : 1;
> +	unsigned int legacy_regs : 1;
>   };
>   
>   /**
> diff --git a/drivers/staging/media/hantro/hantro_g2_regs.h b/drivers/staging/media/hantro/hantro_g2_regs.h
> index 15a391a4650e..d7c2ff05208e 100644
> --- a/drivers/staging/media/hantro/hantro_g2_regs.h
> +++ b/drivers/staging/media/hantro/hantro_g2_regs.h
> @@ -37,6 +37,13 @@
>   
>   #define g2_strm_swap		G2_DEC_REG(2, 28, 0xf)
>   #define g2_dirmv_swap		G2_DEC_REG(2, 20, 0xf)
> +/* used on older variants */
> +#define g2_strm_swap_old	G2_DEC_REG(2, 27, 0x1f)
> +#define g2_pic_swap		G2_DEC_REG(2, 22, 0x1f)
> +#define g2_dirmv_swap_old	G2_DEC_REG(2, 17, 0x1f)
> +#define g2_tab0_swap		G2_DEC_REG(2, 12, 0x1f)
> +#define g2_tab1_swap		G2_DEC_REG(2, 7, 0x1f)
> +#define g2_tab2_swap		G2_DEC_REG(2, 2, 0x1f)\

Please rename g2_tab[0-2]_swap to g2_tab[0-2]_swap_old. Similar names
exist in newer variants (even if not used at the moment).

It is rather difficult to come up with a consistent rule with regard
to in what sequence to arrange these register definitions. It seems
to me that you use a hybrid approach: if the definitions being added
fall "out of order" then you annotate with a comment about older variants,
and if they are "in order" then you simply fold them into their place.

I don't have a very strong opinion, but maybe they all should be
just "in bitfield order" and without comments?

>   
>   #define g2_mode			G2_DEC_REG(3, 27, 0x1f)
>   #define g2_compress_swap	G2_DEC_REG(3, 20, 0xf)
> @@ -45,6 +52,8 @@
>   #define g2_out_dis		G2_DEC_REG(3, 15, 0x1)
>   #define g2_out_filtering_dis	G2_DEC_REG(3, 14, 0x1)
>   #define g2_write_mvs_e		G2_DEC_REG(3, 12, 0x1)
> +#define g2_tab3_swap		G2_DEC_REG(3, 7, 0x1f)

g2_tab3_swap_old

With all the above addressed you can add my

Reviewied-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>

> +#define g2_rscan_swap		G2_DEC_REG(3, 2, 0x1f)
>   
>   #define g2_pic_width_in_cbs	G2_DEC_REG(4, 19, 0x1fff)
>   #define g2_pic_height_in_cbs	G2_DEC_REG(4, 6,  0x1fff)
> @@ -58,6 +67,7 @@
>   #define g2_tempor_mvp_e		G2_DEC_REG(5, 11, 0x1)
>   #define g2_max_cu_qpd_depth	G2_DEC_REG(5, 5,  0x3f)
>   #define g2_cu_qpd_e		G2_DEC_REG(5, 4,  0x1)
> +#define g2_pix_shift		G2_DEC_REG(5, 0,  0xf)
>   
>   #define g2_stream_len		G2_DEC_REG(6, 0,  0xffffffff)
>   
> @@ -80,6 +90,8 @@
>   
>   #define g2_const_intra_e	G2_DEC_REG(8, 31, 0x1)
>   #define g2_filt_ctrl_pres	G2_DEC_REG(8, 30, 0x1)
> +#define g2_bit_depth_y		G2_DEC_REG(8, 21, 0xf)
> +#define g2_bit_depth_c		G2_DEC_REG(8, 17, 0xf)
>   #define g2_idr_pic_e		G2_DEC_REG(8, 16, 0x1)
>   #define g2_bit_depth_pcm_y	G2_DEC_REG(8, 12, 0xf)
>   #define g2_bit_depth_pcm_c	G2_DEC_REG(8, 8,  0xf)
> @@ -87,6 +99,9 @@
>   #define g2_bit_depth_c_minus8	G2_DEC_REG(8, 4,  0x3)
>   #define g2_output_8_bits	G2_DEC_REG(8, 3,  0x1)
>   #define g2_output_format	G2_DEC_REG(8, 0,  0x7)
> +/* used on older variants */
> +#define g2_rs_out_bit_depth	G2_DEC_REG(8, 4,  0xf)
> +#define g2_pp_pix_shift		G2_DEC_REG(8, 0,  0xf)
>   
>   #define g2_refidx1_active	G2_DEC_REG(9, 19, 0x1f)
>   #define g2_refidx0_active	G2_DEC_REG(9, 14, 0x1f)
> @@ -98,6 +113,10 @@
>   #define g2_num_tile_rows	G2_DEC_REG(10, 14, 0x1f)
>   #define g2_tile_e		G2_DEC_REG(10, 1,  0x1)
>   #define g2_entropy_sync_e	G2_DEC_REG(10, 0,  0x1)
> +/* used on older variants */
> +#define g2_init_qp_old		G2_DEC_REG(10, 25, 0x3f)
> +#define g2_num_tile_cols_old	G2_DEC_REG(10, 20, 0x1f)
> +#define g2_num_tile_rows_old	G2_DEC_REG(10, 15, 0x1f)
>   
>   #define vp9_transform_mode	G2_DEC_REG(11, 27, 0x7)
>   #define vp9_filt_sharpness	G2_DEC_REG(11, 21, 0x7)
> diff --git a/drivers/staging/media/hantro/hantro_g2_vp9_dec.c b/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
> index d4fc649a4da1..5aac32700cd0 100644
> --- a/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
> +++ b/drivers/staging/media/hantro/hantro_g2_vp9_dec.c
> @@ -150,7 +150,8 @@ static void config_output(struct hantro_ctx *ctx,
>   	dma_addr_t luma_addr, chroma_addr, mv_addr;
>   
>   	hantro_reg_write(ctx->dev, &g2_out_dis, 0);
> -	hantro_reg_write(ctx->dev, &g2_output_format, 0);
> +	if (!ctx->dev->variant->legacy_regs)
> +		hantro_reg_write(ctx->dev, &g2_output_format, 0);
>   
>   	luma_addr = hantro_get_dec_buf_addr(ctx, &dst->base.vb.vb2_buf);
>   	hantro_write_addr(ctx->dev, G2_OUT_LUMA_ADDR, luma_addr);
> @@ -327,6 +328,7 @@ config_tiles(struct hantro_ctx *ctx,
>   	struct hantro_aux_buf *tile_edge = &vp9_ctx->tile_edge;
>   	dma_addr_t addr;
>   	unsigned short *tile_mem;
> +	unsigned int rows, cols;
>   
>   	addr = misc->dma + vp9_ctx->tile_info_offset;
>   	hantro_write_addr(ctx->dev, G2_TILE_SIZES_ADDR, addr);
> @@ -344,17 +346,24 @@ config_tiles(struct hantro_ctx *ctx,
>   
>   		fill_tile_info(ctx, tile_r, tile_c, sbs_r, sbs_c, tile_mem);
>   
> +		cols = tile_c;
> +		rows = tile_r;
>   		hantro_reg_write(ctx->dev, &g2_tile_e, 1);
> -		hantro_reg_write(ctx->dev, &g2_num_tile_cols, tile_c);
> -		hantro_reg_write(ctx->dev, &g2_num_tile_rows, tile_r);
> -
>   	} else {
>   		tile_mem[0] = hantro_vp9_num_sbs(dst->vp9.width);
>   		tile_mem[1] = hantro_vp9_num_sbs(dst->vp9.height);
>   
> +		cols = 1;
> +		rows = 1;
>   		hantro_reg_write(ctx->dev, &g2_tile_e, 0);
> -		hantro_reg_write(ctx->dev, &g2_num_tile_cols, 1);
> -		hantro_reg_write(ctx->dev, &g2_num_tile_rows, 1);
> +	}
> +
> +	if (ctx->dev->variant->legacy_regs) {
> +		hantro_reg_write(ctx->dev, &g2_num_tile_cols_old, cols);
> +		hantro_reg_write(ctx->dev, &g2_num_tile_rows_old, rows);
> +	} else {
> +		hantro_reg_write(ctx->dev, &g2_num_tile_cols, cols);
> +		hantro_reg_write(ctx->dev, &g2_num_tile_rows, rows);
>   	}
>   
>   	/* provide aux buffers even if no tiles are used */
> @@ -505,8 +514,22 @@ static void config_picture_dimensions(struct hantro_ctx *ctx, struct hantro_deco
>   static void
>   config_bit_depth(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_frame *dec_params)
>   {
> -	hantro_reg_write(ctx->dev, &g2_bit_depth_y_minus8, dec_params->bit_depth - 8);
> -	hantro_reg_write(ctx->dev, &g2_bit_depth_c_minus8, dec_params->bit_depth - 8);
> +	if (ctx->dev->variant->legacy_regs) {
> +		u8 pp_shift = 0;
> +
> +		hantro_reg_write(ctx->dev, &g2_bit_depth_y, dec_params->bit_depth);
> +		hantro_reg_write(ctx->dev, &g2_bit_depth_c, dec_params->bit_depth);
> +		hantro_reg_write(ctx->dev, &g2_rs_out_bit_depth, dec_params->bit_depth);
> +
> +		if (dec_params->bit_depth > 8)
> +			pp_shift = 16 - dec_params->bit_depth;
> +
> +		hantro_reg_write(ctx->dev, &g2_pp_pix_shift, pp_shift);
> +		hantro_reg_write(ctx->dev, &g2_pix_shift, 0);
> +	} else {
> +		hantro_reg_write(ctx->dev, &g2_bit_depth_y_minus8, dec_params->bit_depth - 8);
> +		hantro_reg_write(ctx->dev, &g2_bit_depth_c_minus8, dec_params->bit_depth - 8);
> +	}
>   }
>   
>   static inline bool is_lossless(const struct v4l2_vp9_quantization *quant)
> @@ -784,9 +807,13 @@ config_source(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_frame *dec_para
>   		     + dec_params->compressed_header_size;
>   
>   	stream_base = vb2_dma_contig_plane_dma_addr(&vb2_src->vb2_buf, 0);
> -	hantro_write_addr(ctx->dev, G2_STREAM_ADDR, stream_base);
>   
>   	tmp_addr = stream_base + headres_size;
> +	if (ctx->dev->variant->legacy_regs)
> +		hantro_write_addr(ctx->dev, G2_STREAM_ADDR, (tmp_addr & ~0xf));
> +	else
> +		hantro_write_addr(ctx->dev, G2_STREAM_ADDR, stream_base);
> +
>   	start_bit = (tmp_addr & 0xf) * 8;
>   	hantro_reg_write(ctx->dev, &g2_start_bit, start_bit);
>   
> @@ -794,10 +821,12 @@ config_source(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_frame *dec_para
>   	src_len += start_bit / 8 - headres_size;
>   	hantro_reg_write(ctx->dev, &g2_stream_len, src_len);
>   
> -	tmp_addr &= ~0xf;
> -	hantro_reg_write(ctx->dev, &g2_strm_start_offset, tmp_addr - stream_base);
> -	src_buf_len = vb2_plane_size(&vb2_src->vb2_buf, 0);
> -	hantro_reg_write(ctx->dev, &g2_strm_buffer_len, src_buf_len);
> +	if (!ctx->dev->variant->legacy_regs) {
> +		tmp_addr &= ~0xf;
> +		hantro_reg_write(ctx->dev, &g2_strm_start_offset, tmp_addr - stream_base);
> +		src_buf_len = vb2_plane_size(&vb2_src->vb2_buf, 0);
> +		hantro_reg_write(ctx->dev, &g2_strm_buffer_len, src_buf_len);
> +	}
>   }
>   
>   static void
> @@ -837,13 +866,24 @@ config_registers(struct hantro_ctx *ctx, const struct v4l2_ctrl_vp9_frame *dec_p
>   
>   	/* configure basic registers */
>   	hantro_reg_write(ctx->dev, &g2_mode, VP9_DEC_MODE);
> -	hantro_reg_write(ctx->dev, &g2_strm_swap, 0xf);
> -	hantro_reg_write(ctx->dev, &g2_dirmv_swap, 0xf);
> -	hantro_reg_write(ctx->dev, &g2_compress_swap, 0xf);
> +	if (!ctx->dev->variant->legacy_regs) {
> +		hantro_reg_write(ctx->dev, &g2_strm_swap, 0xf);
> +		hantro_reg_write(ctx->dev, &g2_dirmv_swap, 0xf);
> +		hantro_reg_write(ctx->dev, &g2_compress_swap, 0xf);
> +		hantro_reg_write(ctx->dev, &g2_ref_compress_bypass, 1);
> +	} else {
> +		hantro_reg_write(ctx->dev, &g2_strm_swap_old, 0x1f);
> +		hantro_reg_write(ctx->dev, &g2_pic_swap, 0x10);
> +		hantro_reg_write(ctx->dev, &g2_dirmv_swap_old, 0x10);
> +		hantro_reg_write(ctx->dev, &g2_tab0_swap, 0x10);
> +		hantro_reg_write(ctx->dev, &g2_tab1_swap, 0x10);
> +		hantro_reg_write(ctx->dev, &g2_tab2_swap, 0x10);
> +		hantro_reg_write(ctx->dev, &g2_tab3_swap, 0x10);
> +		hantro_reg_write(ctx->dev, &g2_rscan_swap, 0x10);
> +	}
>   	hantro_reg_write(ctx->dev, &g2_buswidth, BUS_WIDTH_128);
>   	hantro_reg_write(ctx->dev, &g2_max_burst, 16);
>   	hantro_reg_write(ctx->dev, &g2_apf_threshold, 8);
> -	hantro_reg_write(ctx->dev, &g2_ref_compress_bypass, 1);
>   	hantro_reg_write(ctx->dev, &g2_clk_gate_e, 1);
>   	hantro_reg_write(ctx->dev, &g2_max_cb_size, 6);
>   	hantro_reg_write(ctx->dev, &g2_min_cb_size, 3);
> 


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

^ permalink raw reply	[flat|nested] 40+ messages in thread
* Re: [PATCH 4/7] media: hantro: move postproc enablement for old cores
  2021-11-22 18:46   ` Jernej Skrabec
@ 2021-11-23 12:12     ` Andrzej Pietrasiewicz
  -1 siblings, 0 replies; 40+ messages in thread
From: Andrzej Pietrasiewicz @ 2021-11-23 12:12 UTC (permalink / raw)
  To: Jernej Skrabec, linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, gregkh, devicetree, linux-arm-kernel, linux-sunxi,
	linux-kernel, linux-staging

Hi Jernej,

W dniu 22.11.2021 o 19:46, Jernej Skrabec pisze:
> Older G2 cores, like that in Allwinner H6, seem to have issue with
> latching postproc register values if this is first thing done in job.
> Moving that to the end solves the issue.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>   drivers/staging/media/hantro/hantro_drv.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> index 8c3de31f51b3..530994ab3024 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -130,7 +130,7 @@ void hantro_start_prepare_run(struct hantro_ctx *ctx)
>   	v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
>   				&ctx->ctrl_handler);
>   
> -	if (!ctx->is_encoder) {
> +	if (!ctx->is_encoder && !ctx->dev->variant->legacy_regs) {
>   		if (hantro_needs_postproc(ctx, ctx->vpu_dst_fmt))
>   			hantro_postproc_enable(ctx);
>   		else
> @@ -142,6 +142,13 @@ void hantro_end_prepare_run(struct hantro_ctx *ctx)
>   {
>   	struct vb2_v4l2_buffer *src_buf;
>   
> +	if (ctx->dev->variant->legacy_regs && !ctx->is_encoder) {

My personal preference would be to alter the order to match what is
in hantro_start_prepare_run(), and add a comment:

+       /*
+        * Older G2 cores, like that in Allwinner H6, seem to have issue with
+	 * latching postproc register values if this is first thing done in job.
+	 * Moving that to the end solves the issue.
+	 */
+	if (!ctx->is_encoder && ctx->dev->variant->legacy_regs) {

With that changed,

Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>

> +		if (hantro_needs_postproc(ctx, ctx->vpu_dst_fmt))
> +			hantro_postproc_enable(ctx);
> +		else
> +			hantro_postproc_disable(ctx);
> +	}
> +
>   	src_buf = hantro_get_src_buf(ctx);
>   	v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
>   				   &ctx->ctrl_handler);
> 


^ permalink raw reply	[flat|nested] 40+ messages in thread
* Re: [PATCH 4/7] media: hantro: move postproc enablement for old cores
@ 2021-11-23 12:12     ` Andrzej Pietrasiewicz
  0 siblings, 0 replies; 40+ messages in thread
From: Andrzej Pietrasiewicz @ 2021-11-23 12:12 UTC (permalink / raw)
  To: Jernej Skrabec, linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, gregkh, devicetree, linux-arm-kernel, linux-sunxi,
	linux-kernel, linux-staging

Hi Jernej,

W dniu 22.11.2021 o 19:46, Jernej Skrabec pisze:
> Older G2 cores, like that in Allwinner H6, seem to have issue with
> latching postproc register values if this is first thing done in job.
> Moving that to the end solves the issue.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>   drivers/staging/media/hantro/hantro_drv.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> index 8c3de31f51b3..530994ab3024 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -130,7 +130,7 @@ void hantro_start_prepare_run(struct hantro_ctx *ctx)
>   	v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
>   				&ctx->ctrl_handler);
>   
> -	if (!ctx->is_encoder) {
> +	if (!ctx->is_encoder && !ctx->dev->variant->legacy_regs) {
>   		if (hantro_needs_postproc(ctx, ctx->vpu_dst_fmt))
>   			hantro_postproc_enable(ctx);
>   		else
> @@ -142,6 +142,13 @@ void hantro_end_prepare_run(struct hantro_ctx *ctx)
>   {
>   	struct vb2_v4l2_buffer *src_buf;
>   
> +	if (ctx->dev->variant->legacy_regs && !ctx->is_encoder) {

My personal preference would be to alter the order to match what is
in hantro_start_prepare_run(), and add a comment:

+       /*
+        * Older G2 cores, like that in Allwinner H6, seem to have issue with
+	 * latching postproc register values if this is first thing done in job.
+	 * Moving that to the end solves the issue.
+	 */
+	if (!ctx->is_encoder && ctx->dev->variant->legacy_regs) {

With that changed,

Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>

> +		if (hantro_needs_postproc(ctx, ctx->vpu_dst_fmt))
> +			hantro_postproc_enable(ctx);
> +		else
> +			hantro_postproc_disable(ctx);
> +	}
> +
>   	src_buf = hantro_get_src_buf(ctx);
>   	v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
>   				   &ctx->ctrl_handler);
> 


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

^ permalink raw reply	[flat|nested] 40+ messages in thread
* Re: [PATCH 6/7] media: hantro: Add support for Allwinner H6
  2021-11-22 18:47   ` Jernej Skrabec
@ 2021-11-23 12:23     ` Andrzej Pietrasiewicz
  -1 siblings, 0 replies; 40+ messages in thread
From: Andrzej Pietrasiewicz @ 2021-11-23 12:23 UTC (permalink / raw)
  To: Jernej Skrabec, linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, gregkh, devicetree, linux-arm-kernel, linux-sunxi,
	linux-kernel, linux-staging

Hi Jernej,

W dniu 22.11.2021 o 19:47, Jernej Skrabec pisze:
> Allwinner H6 has a Hantro G2 core used for VP9 decoding. It's not clear
> at this time if HEVC is also supported or not.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>   drivers/staging/media/hantro/Kconfig        |  10 +-
>   drivers/staging/media/hantro/Makefile       |   3 +
>   drivers/staging/media/hantro/hantro_drv.c   |   3 +
>   drivers/staging/media/hantro/hantro_hw.h    |   1 +
>   drivers/staging/media/hantro/sunxi_vpu_hw.c | 104 ++++++++++++++++++++
>   5 files changed, 120 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/staging/media/hantro/sunxi_vpu_hw.c
> 
> diff --git a/drivers/staging/media/hantro/Kconfig b/drivers/staging/media/hantro/Kconfig
> index 00a57d88c92e..3c5d833322c8 100644
> --- a/drivers/staging/media/hantro/Kconfig
> +++ b/drivers/staging/media/hantro/Kconfig
> @@ -1,7 +1,7 @@
>   # SPDX-License-Identifier: GPL-2.0
>   config VIDEO_HANTRO
>   	tristate "Hantro VPU driver"
> -	depends on ARCH_MXC || ARCH_ROCKCHIP || ARCH_AT91 || COMPILE_TEST
> +	depends on ARCH_MXC || ARCH_ROCKCHIP || ARCH_AT91 || ARCH_SUNXI || COMPILE_TEST
>   	depends on VIDEO_DEV && VIDEO_V4L2
>   	select MEDIA_CONTROLLER
>   	select MEDIA_CONTROLLER_REQUEST_API
> @@ -40,3 +40,11 @@ config VIDEO_HANTRO_ROCKCHIP
>   	default y
>   	help
>   	  Enable support for RK3288, RK3328, and RK3399 SoCs.
> +
> +config VIDEO_HANTRO_SUNXI
> +	bool "Hantro VPU Allwinner support"
> +	depends on VIDEO_HANTRO
> +	depends on ARCH_SUNXI || COMPILE_TEST
> +	default y
> +	help
> +	  Enable support for H6 SoC.
> diff --git a/drivers/staging/media/hantro/Makefile b/drivers/staging/media/hantro/Makefile
> index 28af0a1ee4bf..ebd5ede7bef7 100644
> --- a/drivers/staging/media/hantro/Makefile
> +++ b/drivers/staging/media/hantro/Makefile
> @@ -33,3 +33,6 @@ hantro-vpu-$(CONFIG_VIDEO_HANTRO_SAMA5D4) += \
>   
>   hantro-vpu-$(CONFIG_VIDEO_HANTRO_ROCKCHIP) += \
>   		rockchip_vpu_hw.o
> +
> +hantro-vpu-$(CONFIG_VIDEO_HANTRO_SUNXI) += \
> +		sunxi_vpu_hw.o
> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> index 530994ab3024..54f34644ecdf 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -620,6 +620,9 @@ static const struct of_device_id of_hantro_match[] = {
>   #endif
>   #ifdef CONFIG_VIDEO_HANTRO_SAMA5D4
>   	{ .compatible = "microchip,sama5d4-vdec", .data = &sama5d4_vdec_variant, },
> +#endif
> +#ifdef CONFIG_VIDEO_HANTRO_SUNXI
> +	{ .compatible = "allwinner,sun50i-h6-vpu-g2", .data = &sunxi_vpu_variant, },
>   #endif
>   	{ /* sentinel */ }
>   };
> diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
> index dbe51303724b..0676989b986b 100644
> --- a/drivers/staging/media/hantro/hantro_hw.h
> +++ b/drivers/staging/media/hantro/hantro_hw.h
> @@ -308,6 +308,7 @@ extern const struct hantro_variant rk3288_vpu_variant;
>   extern const struct hantro_variant rk3328_vpu_variant;
>   extern const struct hantro_variant rk3399_vpu_variant;
>   extern const struct hantro_variant sama5d4_vdec_variant;
> +extern const struct hantro_variant sunxi_vpu_variant;
>   
>   extern const struct hantro_postproc_ops hantro_g1_postproc_ops;
>   extern const struct hantro_postproc_ops hantro_g2_postproc_ops;
> diff --git a/drivers/staging/media/hantro/sunxi_vpu_hw.c b/drivers/staging/media/hantro/sunxi_vpu_hw.c
> new file mode 100644
> index 000000000000..05e964dc0499
> --- /dev/null
> +++ b/drivers/staging/media/hantro/sunxi_vpu_hw.c
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Allwinner Hantro G2 VPU codec driver
> + *
> + * Copyright (C) 2021 Jernej Skrabec <jernej.skrabec@gmail.com>
> + */
> +
> +#include <linux/clk.h>
> +
> +#include "hantro.h"
> +#include "hantro_g2_regs.h"
> +
> +static const struct hantro_fmt sunxi_vpu_postproc_fmts[] = {
> +	{
> +		.fourcc = V4L2_PIX_FMT_NV12,
> +		.codec_mode = HANTRO_MODE_NONE,
> +		.postprocessed = true,
> +	},
> +};
> +
> +static const struct hantro_fmt sunxi_vpu_dec_fmts[] = {
> +	{
> +		.fourcc = V4L2_PIX_FMT_NV12_4L4,
> +		.codec_mode = HANTRO_MODE_NONE,
> +	},
> +	{
> +		.fourcc = V4L2_PIX_FMT_VP9_FRAME,
> +		.codec_mode = HANTRO_MODE_VP9_DEC,
> +		.max_depth = 2,
> +		.frmsize = {
> +			.min_width = 48,
> +			.max_width = 3840,
> +			.step_width = MB_DIM,
> +			.min_height = 48,
> +			.max_height = 2160,
> +			.step_height = MB_DIM,
> +		},
> +	},
> +};
> +
> +static irqreturn_t sunxi_vpu_irq(int irq, void *dev_id)
> +{
> +	struct hantro_dev *vpu = dev_id;
> +	enum vb2_buffer_state state;
> +	u32 status;
> +
> +	status = vdpu_read(vpu, G2_REG_INTERRUPT);
> +	state = (status & G2_REG_INTERRUPT_DEC_RDY_INT) ?
> +		 VB2_BUF_STATE_DONE : VB2_BUF_STATE_ERROR;
> +
> +	vdpu_write(vpu, 0, G2_REG_INTERRUPT);
> +	vdpu_write(vpu, G2_REG_CONFIG_DEC_CLK_GATE_E, G2_REG_CONFIG);
> +
> +	hantro_irq_done(vpu, state);
> +
> +	return IRQ_HANDLED;
> +}
> +

This function is a verbatim copy of imx8m_vpu_g2_irq(), modulo
the function name. Perhaps the two can be factored out into hantro_g2.c?

Andrzej

> +static int sunxi_vpu_hw_init(struct hantro_dev *vpu)
> +{
> +	clk_set_rate(vpu->clocks[0].clk, 300000000);
> +
> +	return 0;
> +}
> +
> +static void sunxi_vpu_reset(struct hantro_ctx *ctx)
> +{
> +	struct hantro_dev *vpu = ctx->dev;
> +
> +	reset_control_reset(vpu->resets);
> +}
> +
> +static const struct hantro_codec_ops sunxi_vpu_codec_ops[] = {
> +	[HANTRO_MODE_VP9_DEC] = {
> +		.run = hantro_g2_vp9_dec_run,
> +		.done = hantro_g2_vp9_dec_done,
> +		.reset = sunxi_vpu_reset,
> +		.init = hantro_vp9_dec_init,
> +		.exit = hantro_vp9_dec_exit,
> +	},
> +};
> +
> +static const struct hantro_irq sunxi_irqs[] = {
> +	{ NULL, sunxi_vpu_irq },
> +};
> +
> +static const char * const sunxi_clk_names[] = { "mod", "bus" };
> +
> +const struct hantro_variant sunxi_vpu_variant = {
> +	.dec_fmts = sunxi_vpu_dec_fmts,
> +	.num_dec_fmts = ARRAY_SIZE(sunxi_vpu_dec_fmts),
> +	.postproc_fmts = sunxi_vpu_postproc_fmts,
> +	.num_postproc_fmts = ARRAY_SIZE(sunxi_vpu_postproc_fmts),
> +	.postproc_ops = &hantro_g2_postproc_ops,
> +	.codec = HANTRO_VP9_DECODER,
> +	.codec_ops = sunxi_vpu_codec_ops,
> +	.init = sunxi_vpu_hw_init,
> +	.irqs = sunxi_irqs,
> +	.num_irqs = ARRAY_SIZE(sunxi_irqs),
> +	.clk_names = sunxi_clk_names,
> +	.num_clocks = ARRAY_SIZE(sunxi_clk_names),
> +	.double_buffer = 1,
> +	.legacy_regs = 1,
> +};
> 


^ permalink raw reply	[flat|nested] 40+ messages in thread
* Re: [PATCH 6/7] media: hantro: Add support for Allwinner H6
@ 2021-11-23 12:23     ` Andrzej Pietrasiewicz
  0 siblings, 0 replies; 40+ messages in thread
From: Andrzej Pietrasiewicz @ 2021-11-23 12:23 UTC (permalink / raw)
  To: Jernej Skrabec, linux-media
  Cc: ezequiel, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, gregkh, devicetree, linux-arm-kernel, linux-sunxi,
	linux-kernel, linux-staging

Hi Jernej,

W dniu 22.11.2021 o 19:47, Jernej Skrabec pisze:
> Allwinner H6 has a Hantro G2 core used for VP9 decoding. It's not clear
> at this time if HEVC is also supported or not.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>   drivers/staging/media/hantro/Kconfig        |  10 +-
>   drivers/staging/media/hantro/Makefile       |   3 +
>   drivers/staging/media/hantro/hantro_drv.c   |   3 +
>   drivers/staging/media/hantro/hantro_hw.h    |   1 +
>   drivers/staging/media/hantro/sunxi_vpu_hw.c | 104 ++++++++++++++++++++
>   5 files changed, 120 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/staging/media/hantro/sunxi_vpu_hw.c
> 
> diff --git a/drivers/staging/media/hantro/Kconfig b/drivers/staging/media/hantro/Kconfig
> index 00a57d88c92e..3c5d833322c8 100644
> --- a/drivers/staging/media/hantro/Kconfig
> +++ b/drivers/staging/media/hantro/Kconfig
> @@ -1,7 +1,7 @@
>   # SPDX-License-Identifier: GPL-2.0
>   config VIDEO_HANTRO
>   	tristate "Hantro VPU driver"
> -	depends on ARCH_MXC || ARCH_ROCKCHIP || ARCH_AT91 || COMPILE_TEST
> +	depends on ARCH_MXC || ARCH_ROCKCHIP || ARCH_AT91 || ARCH_SUNXI || COMPILE_TEST
>   	depends on VIDEO_DEV && VIDEO_V4L2
>   	select MEDIA_CONTROLLER
>   	select MEDIA_CONTROLLER_REQUEST_API
> @@ -40,3 +40,11 @@ config VIDEO_HANTRO_ROCKCHIP
>   	default y
>   	help
>   	  Enable support for RK3288, RK3328, and RK3399 SoCs.
> +
> +config VIDEO_HANTRO_SUNXI
> +	bool "Hantro VPU Allwinner support"
> +	depends on VIDEO_HANTRO
> +	depends on ARCH_SUNXI || COMPILE_TEST
> +	default y
> +	help
> +	  Enable support for H6 SoC.
> diff --git a/drivers/staging/media/hantro/Makefile b/drivers/staging/media/hantro/Makefile
> index 28af0a1ee4bf..ebd5ede7bef7 100644
> --- a/drivers/staging/media/hantro/Makefile
> +++ b/drivers/staging/media/hantro/Makefile
> @@ -33,3 +33,6 @@ hantro-vpu-$(CONFIG_VIDEO_HANTRO_SAMA5D4) += \
>   
>   hantro-vpu-$(CONFIG_VIDEO_HANTRO_ROCKCHIP) += \
>   		rockchip_vpu_hw.o
> +
> +hantro-vpu-$(CONFIG_VIDEO_HANTRO_SUNXI) += \
> +		sunxi_vpu_hw.o
> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> index 530994ab3024..54f34644ecdf 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -620,6 +620,9 @@ static const struct of_device_id of_hantro_match[] = {
>   #endif
>   #ifdef CONFIG_VIDEO_HANTRO_SAMA5D4
>   	{ .compatible = "microchip,sama5d4-vdec", .data = &sama5d4_vdec_variant, },
> +#endif
> +#ifdef CONFIG_VIDEO_HANTRO_SUNXI
> +	{ .compatible = "allwinner,sun50i-h6-vpu-g2", .data = &sunxi_vpu_variant, },
>   #endif
>   	{ /* sentinel */ }
>   };
> diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h
> index dbe51303724b..0676989b986b 100644
> --- a/drivers/staging/media/hantro/hantro_hw.h
> +++ b/drivers/staging/media/hantro/hantro_hw.h
> @@ -308,6 +308,7 @@ extern const struct hantro_variant rk3288_vpu_variant;
>   extern const struct hantro_variant rk3328_vpu_variant;
>   extern const struct hantro_variant rk3399_vpu_variant;
>   extern const struct hantro_variant sama5d4_vdec_variant;
> +extern const struct hantro_variant sunxi_vpu_variant;
>   
>   extern const struct hantro_postproc_ops hantro_g1_postproc_ops;
>   extern const struct hantro_postproc_ops hantro_g2_postproc_ops;
> diff --git a/drivers/staging/media/hantro/sunxi_vpu_hw.c b/drivers/staging/media/hantro/sunxi_vpu_hw.c
> new file mode 100644
> index 000000000000..05e964dc0499
> --- /dev/null
> +++ b/drivers/staging/media/hantro/sunxi_vpu_hw.c
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Allwinner Hantro G2 VPU codec driver
> + *
> + * Copyright (C) 2021 Jernej Skrabec <jernej.skrabec@gmail.com>
> + */
> +
> +#include <linux/clk.h>
> +
> +#include "hantro.h"
> +#include "hantro_g2_regs.h"
> +
> +static const struct hantro_fmt sunxi_vpu_postproc_fmts[] = {
> +	{
> +		.fourcc = V4L2_PIX_FMT_NV12,
> +		.codec_mode = HANTRO_MODE_NONE,
> +		.postprocessed = true,
> +	},
> +};
> +
> +static const struct hantro_fmt sunxi_vpu_dec_fmts[] = {
> +	{
> +		.fourcc = V4L2_PIX_FMT_NV12_4L4,
> +		.codec_mode = HANTRO_MODE_NONE,
> +	},
> +	{
> +		.fourcc = V4L2_PIX_FMT_VP9_FRAME,
> +		.codec_mode = HANTRO_MODE_VP9_DEC,
> +		.max_depth = 2,
> +		.frmsize = {
> +			.min_width = 48,
> +			.max_width = 3840,
> +			.step_width = MB_DIM,
> +			.min_height = 48,
> +			.max_height = 2160,
> +			.step_height = MB_DIM,
> +		},
> +	},
> +};
> +
> +static irqreturn_t sunxi_vpu_irq(int irq, void *dev_id)
> +{
> +	struct hantro_dev *vpu = dev_id;
> +	enum vb2_buffer_state state;
> +	u32 status;
> +
> +	status = vdpu_read(vpu, G2_REG_INTERRUPT);
> +	state = (status & G2_REG_INTERRUPT_DEC_RDY_INT) ?
> +		 VB2_BUF_STATE_DONE : VB2_BUF_STATE_ERROR;
> +
> +	vdpu_write(vpu, 0, G2_REG_INTERRUPT);
> +	vdpu_write(vpu, G2_REG_CONFIG_DEC_CLK_GATE_E, G2_REG_CONFIG);
> +
> +	hantro_irq_done(vpu, state);
> +
> +	return IRQ_HANDLED;
> +}
> +

This function is a verbatim copy of imx8m_vpu_g2_irq(), modulo
the function name. Perhaps the two can be factored out into hantro_g2.c?

Andrzej

> +static int sunxi_vpu_hw_init(struct hantro_dev *vpu)
> +{
> +	clk_set_rate(vpu->clocks[0].clk, 300000000);
> +
> +	return 0;
> +}
> +
> +static void sunxi_vpu_reset(struct hantro_ctx *ctx)
> +{
> +	struct hantro_dev *vpu = ctx->dev;
> +
> +	reset_control_reset(vpu->resets);
> +}
> +
> +static const struct hantro_codec_ops sunxi_vpu_codec_ops[] = {
> +	[HANTRO_MODE_VP9_DEC] = {
> +		.run = hantro_g2_vp9_dec_run,
> +		.done = hantro_g2_vp9_dec_done,
> +		.reset = sunxi_vpu_reset,
> +		.init = hantro_vp9_dec_init,
> +		.exit = hantro_vp9_dec_exit,
> +	},
> +};
> +
> +static const struct hantro_irq sunxi_irqs[] = {
> +	{ NULL, sunxi_vpu_irq },
> +};
> +
> +static const char * const sunxi_clk_names[] = { "mod", "bus" };
> +
> +const struct hantro_variant sunxi_vpu_variant = {
> +	.dec_fmts = sunxi_vpu_dec_fmts,
> +	.num_dec_fmts = ARRAY_SIZE(sunxi_vpu_dec_fmts),
> +	.postproc_fmts = sunxi_vpu_postproc_fmts,
> +	.num_postproc_fmts = ARRAY_SIZE(sunxi_vpu_postproc_fmts),
> +	.postproc_ops = &hantro_g2_postproc_ops,
> +	.codec = HANTRO_VP9_DECODER,
> +	.codec_ops = sunxi_vpu_codec_ops,
> +	.init = sunxi_vpu_hw_init,
> +	.irqs = sunxi_irqs,
> +	.num_irqs = ARRAY_SIZE(sunxi_irqs),
> +	.clk_names = sunxi_clk_names,
> +	.num_clocks = ARRAY_SIZE(sunxi_clk_names),
> +	.double_buffer = 1,
> +	.legacy_regs = 1,
> +};
> 


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

^ permalink raw reply	[flat|nested] 40+ messages in thread
* Re: [PATCH 1/7] media: hantro: add support for reset lines
  2021-11-23 11:09     ` Andrzej Pietrasiewicz
@ 2021-11-23 14:59       ` Dan Carpenter
  -1 siblings, 0 replies; 40+ messages in thread
From: Dan Carpenter @ 2021-11-23 14:59 UTC (permalink / raw)
  To: Andrzej Pietrasiewicz
  Cc: Jernej Skrabec, linux-media, ezequiel, nicolas.dufresne, mchehab,
	robh+dt, mripard, wens, p.zabel, gregkh, devicetree,
	linux-arm-kernel, linux-sunxi, linux-kernel, linux-staging

On Tue, Nov 23, 2021 at 12:09:03PM +0100, Andrzej Pietrasiewicz wrote:
> > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> > index ab2467998d29..8c3de31f51b3 100644
> > --- a/drivers/staging/media/hantro/hantro_drv.c
> > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > @@ -905,6 +905,10 @@ static int hantro_probe(struct platform_device *pdev)
> >   			return PTR_ERR(vpu->clocks[0].clk);
> >   	}
> > +	vpu->resets = devm_reset_control_array_get(&pdev->dev, false, true);
> > +	if (IS_ERR(vpu->resets))
> > +		return PTR_ERR(vpu->resets);
> > +
> >   	num_bases = vpu->variant->num_regs ?: 1;
> >   	vpu->reg_bases = devm_kcalloc(&pdev->dev, num_bases,
> >   				      sizeof(*vpu->reg_bases), GFP_KERNEL);
> > @@ -978,10 +982,16 @@ static int hantro_probe(struct platform_device *pdev)
> >   	pm_runtime_use_autosuspend(vpu->dev);
> >   	pm_runtime_enable(vpu->dev);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^
It looks like this is the pm stuff that we have to unwind on error

> > +	ret = reset_control_deassert(vpu->resets);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to deassert resets\n");
> > +		return ret;
                ^^^^^^^^^^
So this return should be a goto undo_pm_stuff


> > +	}
> > +
> >   	ret = clk_bulk_prepare(vpu->variant->num_clocks, vpu->clocks);
> >   	if (ret) {
> >   		dev_err(&pdev->dev, "Failed to prepare clocks\n");
> > -		return ret;

And this return should also have been a goto so it's a bug in the
original code.

> > +		goto err_rst_assert;
> 
> Before your patch is applied if clk_bulk_prepare() fails, we
> simply return on the spot. After the patch is applied not only
> do you...
> 
> >   	}
> >   	ret = v4l2_device_register(&pdev->dev, &vpu->v4l2_dev);
> > @@ -1037,6 +1047,8 @@ static int hantro_probe(struct platform_device *pdev)
> >   	v4l2_device_unregister(&vpu->v4l2_dev);
> >   err_clk_unprepare:
> >   	clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
> > +err_rst_assert:
> > +	reset_control_assert(vpu->resets);
> 
> ...revert the effect of reset_control_deassert(), you also...
> 
> >   	pm_runtime_dont_use_autosuspend(vpu->dev);
> >   	pm_runtime_disable(vpu->dev);
> 
> ... do pm_*() stuff. Is there any reason why this is needed?

So, yes, it's needed, but you're correct to spot that it's not
consistent.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 40+ messages in thread
* Re: [PATCH 1/7] media: hantro: add support for reset lines
@ 2021-11-23 14:59       ` Dan Carpenter
  0 siblings, 0 replies; 40+ messages in thread
From: Dan Carpenter @ 2021-11-23 14:59 UTC (permalink / raw)
  To: Andrzej Pietrasiewicz
  Cc: Jernej Skrabec, linux-media, ezequiel, nicolas.dufresne, mchehab,
	robh+dt, mripard, wens, p.zabel, gregkh, devicetree,
	linux-arm-kernel, linux-sunxi, linux-kernel, linux-staging

On Tue, Nov 23, 2021 at 12:09:03PM +0100, Andrzej Pietrasiewicz wrote:
> > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> > index ab2467998d29..8c3de31f51b3 100644
> > --- a/drivers/staging/media/hantro/hantro_drv.c
> > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > @@ -905,6 +905,10 @@ static int hantro_probe(struct platform_device *pdev)
> >   			return PTR_ERR(vpu->clocks[0].clk);
> >   	}
> > +	vpu->resets = devm_reset_control_array_get(&pdev->dev, false, true);
> > +	if (IS_ERR(vpu->resets))
> > +		return PTR_ERR(vpu->resets);
> > +
> >   	num_bases = vpu->variant->num_regs ?: 1;
> >   	vpu->reg_bases = devm_kcalloc(&pdev->dev, num_bases,
> >   				      sizeof(*vpu->reg_bases), GFP_KERNEL);
> > @@ -978,10 +982,16 @@ static int hantro_probe(struct platform_device *pdev)
> >   	pm_runtime_use_autosuspend(vpu->dev);
> >   	pm_runtime_enable(vpu->dev);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^
It looks like this is the pm stuff that we have to unwind on error

> > +	ret = reset_control_deassert(vpu->resets);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to deassert resets\n");
> > +		return ret;
                ^^^^^^^^^^
So this return should be a goto undo_pm_stuff


> > +	}
> > +
> >   	ret = clk_bulk_prepare(vpu->variant->num_clocks, vpu->clocks);
> >   	if (ret) {
> >   		dev_err(&pdev->dev, "Failed to prepare clocks\n");
> > -		return ret;

And this return should also have been a goto so it's a bug in the
original code.

> > +		goto err_rst_assert;
> 
> Before your patch is applied if clk_bulk_prepare() fails, we
> simply return on the spot. After the patch is applied not only
> do you...
> 
> >   	}
> >   	ret = v4l2_device_register(&pdev->dev, &vpu->v4l2_dev);
> > @@ -1037,6 +1047,8 @@ static int hantro_probe(struct platform_device *pdev)
> >   	v4l2_device_unregister(&vpu->v4l2_dev);
> >   err_clk_unprepare:
> >   	clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
> > +err_rst_assert:
> > +	reset_control_assert(vpu->resets);
> 
> ...revert the effect of reset_control_deassert(), you also...
> 
> >   	pm_runtime_dont_use_autosuspend(vpu->dev);
> >   	pm_runtime_disable(vpu->dev);
> 
> ... do pm_*() stuff. Is there any reason why this is needed?

So, yes, it's needed, but you're correct to spot that it's not
consistent.

regards,
dan carpenter


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

^ permalink raw reply	[flat|nested] 40+ messages in thread
* Re: [PATCH 1/7] media: hantro: add support for reset lines
  2021-11-23 14:59       ` Dan Carpenter
@ 2021-11-23 16:36         ` Andrzej Pietrasiewicz
  -1 siblings, 0 replies; 40+ messages in thread
From: Andrzej Pietrasiewicz @ 2021-11-23 16:36 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Jernej Skrabec, linux-media, ezequiel, nicolas.dufresne, mchehab,
	robh+dt, mripard, wens, p.zabel, gregkh, devicetree,
	linux-arm-kernel, linux-sunxi, linux-kernel, linux-staging

Hi Dan, hi Jernej,

W dniu 23.11.2021 o 15:59, Dan Carpenter pisze:
> On Tue, Nov 23, 2021 at 12:09:03PM +0100, Andrzej Pietrasiewicz wrote:
>>> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
>>> index ab2467998d29..8c3de31f51b3 100644
>>> --- a/drivers/staging/media/hantro/hantro_drv.c
>>> +++ b/drivers/staging/media/hantro/hantro_drv.c
>>> @@ -905,6 +905,10 @@ static int hantro_probe(struct platform_device *pdev)
>>>    			return PTR_ERR(vpu->clocks[0].clk);
>>>    	}
>>> +	vpu->resets = devm_reset_control_array_get(&pdev->dev, false, true);
>>> +	if (IS_ERR(vpu->resets))
>>> +		return PTR_ERR(vpu->resets);
>>> +
>>>    	num_bases = vpu->variant->num_regs ?: 1;
>>>    	vpu->reg_bases = devm_kcalloc(&pdev->dev, num_bases,
>>>    				      sizeof(*vpu->reg_bases), GFP_KERNEL);
>>> @@ -978,10 +982,16 @@ static int hantro_probe(struct platform_device *pdev)
>>>    	pm_runtime_use_autosuspend(vpu->dev);
>>>    	pm_runtime_enable(vpu->dev);
>          ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> It looks like this is the pm stuff that we have to unwind on error
> 
>>> +	ret = reset_control_deassert(vpu->resets);
>>> +	if (ret) {
>>> +		dev_err(&pdev->dev, "Failed to deassert resets\n");
>>> +		return ret;
>                  ^^^^^^^^^^
> So this return should be a goto undo_pm_stuff
> 
> 
>>> +	}
>>> +
>>>    	ret = clk_bulk_prepare(vpu->variant->num_clocks, vpu->clocks);
>>>    	if (ret) {
>>>    		dev_err(&pdev->dev, "Failed to prepare clocks\n");
>>> -		return ret;
> 
> And this return should also have been a goto so it's a bug in the
> original code.

So we probably want a separate patch addressing that first, and then
the series proper on top of that.

Regards,

Andrzej

> 
>>> +		goto err_rst_assert;
>>
>> Before your patch is applied if clk_bulk_prepare() fails, we
>> simply return on the spot. After the patch is applied not only
>> do you...
>>
>>>    	}
>>>    	ret = v4l2_device_register(&pdev->dev, &vpu->v4l2_dev);
>>> @@ -1037,6 +1047,8 @@ static int hantro_probe(struct platform_device *pdev)
>>>    	v4l2_device_unregister(&vpu->v4l2_dev);
>>>    err_clk_unprepare:
>>>    	clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
>>> +err_rst_assert:
>>> +	reset_control_assert(vpu->resets);
>>
>> ...revert the effect of reset_control_deassert(), you also...
>>
>>>    	pm_runtime_dont_use_autosuspend(vpu->dev);
>>>    	pm_runtime_disable(vpu->dev);
>>
>> ... do pm_*() stuff. Is there any reason why this is needed?
> 
> So, yes, it's needed, but you're correct to spot that it's not
> consistent.
> 
> regards,
> dan carpenter
> 


^ permalink raw reply	[flat|nested] 40+ messages in thread
* Re: [PATCH 1/7] media: hantro: add support for reset lines
@ 2021-11-23 16:36         ` Andrzej Pietrasiewicz
  0 siblings, 0 replies; 40+ messages in thread
From: Andrzej Pietrasiewicz @ 2021-11-23 16:36 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Jernej Skrabec, linux-media, ezequiel, nicolas.dufresne, mchehab,
	robh+dt, mripard, wens, p.zabel, gregkh, devicetree,
	linux-arm-kernel, linux-sunxi, linux-kernel, linux-staging

Hi Dan, hi Jernej,

W dniu 23.11.2021 o 15:59, Dan Carpenter pisze:
> On Tue, Nov 23, 2021 at 12:09:03PM +0100, Andrzej Pietrasiewicz wrote:
>>> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
>>> index ab2467998d29..8c3de31f51b3 100644
>>> --- a/drivers/staging/media/hantro/hantro_drv.c
>>> +++ b/drivers/staging/media/hantro/hantro_drv.c
>>> @@ -905,6 +905,10 @@ static int hantro_probe(struct platform_device *pdev)
>>>    			return PTR_ERR(vpu->clocks[0].clk);
>>>    	}
>>> +	vpu->resets = devm_reset_control_array_get(&pdev->dev, false, true);
>>> +	if (IS_ERR(vpu->resets))
>>> +		return PTR_ERR(vpu->resets);
>>> +
>>>    	num_bases = vpu->variant->num_regs ?: 1;
>>>    	vpu->reg_bases = devm_kcalloc(&pdev->dev, num_bases,
>>>    				      sizeof(*vpu->reg_bases), GFP_KERNEL);
>>> @@ -978,10 +982,16 @@ static int hantro_probe(struct platform_device *pdev)
>>>    	pm_runtime_use_autosuspend(vpu->dev);
>>>    	pm_runtime_enable(vpu->dev);
>          ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> It looks like this is the pm stuff that we have to unwind on error
> 
>>> +	ret = reset_control_deassert(vpu->resets);
>>> +	if (ret) {
>>> +		dev_err(&pdev->dev, "Failed to deassert resets\n");
>>> +		return ret;
>                  ^^^^^^^^^^
> So this return should be a goto undo_pm_stuff
> 
> 
>>> +	}
>>> +
>>>    	ret = clk_bulk_prepare(vpu->variant->num_clocks, vpu->clocks);
>>>    	if (ret) {
>>>    		dev_err(&pdev->dev, "Failed to prepare clocks\n");
>>> -		return ret;
> 
> And this return should also have been a goto so it's a bug in the
> original code.

So we probably want a separate patch addressing that first, and then
the series proper on top of that.

Regards,

Andrzej

> 
>>> +		goto err_rst_assert;
>>
>> Before your patch is applied if clk_bulk_prepare() fails, we
>> simply return on the spot. After the patch is applied not only
>> do you...
>>
>>>    	}
>>>    	ret = v4l2_device_register(&pdev->dev, &vpu->v4l2_dev);
>>> @@ -1037,6 +1047,8 @@ static int hantro_probe(struct platform_device *pdev)
>>>    	v4l2_device_unregister(&vpu->v4l2_dev);
>>>    err_clk_unprepare:
>>>    	clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
>>> +err_rst_assert:
>>> +	reset_control_assert(vpu->resets);
>>
>> ...revert the effect of reset_control_deassert(), you also...
>>
>>>    	pm_runtime_dont_use_autosuspend(vpu->dev);
>>>    	pm_runtime_disable(vpu->dev);
>>
>> ... do pm_*() stuff. Is there any reason why this is needed?
> 
> So, yes, it's needed, but you're correct to spot that it's not
> consistent.
> 
> regards,
> dan carpenter
> 


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

^ permalink raw reply	[flat|nested] 40+ messages in thread
* Re: Re: [PATCH 1/7] media: hantro: add support for reset lines
  2021-11-23 16:36         ` Andrzej Pietrasiewicz
@ 2021-11-23 16:46           ` Jernej Škrabec
  -1 siblings, 0 replies; 40+ messages in thread
From: Jernej Škrabec @ 2021-11-23 16:46 UTC (permalink / raw)
  To: Dan Carpenter, Andrzej Pietrasiewicz
  Cc: linux-media, ezequiel, nicolas.dufresne, mchehab, robh+dt,
	mripard, wens, p.zabel, gregkh, devicetree, linux-arm-kernel,
	linux-sunxi, linux-kernel, linux-staging

Hi all,

Dne torek, 23. november 2021 ob 17:36:57 CET je Andrzej Pietrasiewicz 
napisal(a):
> Hi Dan, hi Jernej,
> 
> W dniu 23.11.2021 o 15:59, Dan Carpenter pisze:
> > On Tue, Nov 23, 2021 at 12:09:03PM +0100, Andrzej Pietrasiewicz wrote:
> >>> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/
media/hantro/hantro_drv.c
> >>> index ab2467998d29..8c3de31f51b3 100644
> >>> --- a/drivers/staging/media/hantro/hantro_drv.c
> >>> +++ b/drivers/staging/media/hantro/hantro_drv.c
> >>> @@ -905,6 +905,10 @@ static int hantro_probe(struct platform_device 
*pdev)
> >>>    			return PTR_ERR(vpu->clocks[0].clk);
> >>>    	}
> >>> +	vpu->resets = devm_reset_control_array_get(&pdev->dev, false, 
true);
> >>> +	if (IS_ERR(vpu->resets))
> >>> +		return PTR_ERR(vpu->resets);
> >>> +
> >>>    	num_bases = vpu->variant->num_regs ?: 1;
> >>>    	vpu->reg_bases = devm_kcalloc(&pdev->dev, num_bases,
> >>>    				      sizeof(*vpu->reg_bases), 
GFP_KERNEL);
> >>> @@ -978,10 +982,16 @@ static int hantro_probe(struct platform_device 
*pdev)
> >>>    	pm_runtime_use_autosuspend(vpu->dev);
> >>>    	pm_runtime_enable(vpu->dev);
> >          ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > It looks like this is the pm stuff that we have to unwind on error
> > 
> >>> +	ret = reset_control_deassert(vpu->resets);
> >>> +	if (ret) {
> >>> +		dev_err(&pdev->dev, "Failed to deassert resets\n");
> >>> +		return ret;
> >                  ^^^^^^^^^^
> > So this return should be a goto undo_pm_stuff
> > 
> > 
> >>> +	}
> >>> +
> >>>    	ret = clk_bulk_prepare(vpu->variant->num_clocks, vpu->clocks);
> >>>    	if (ret) {
> >>>    		dev_err(&pdev->dev, "Failed to prepare clocks\n");
> >>> -		return ret;
> > 
> > And this return should also have been a goto so it's a bug in the
> > original code.
> 
> So we probably want a separate patch addressing that first, and then
> the series proper on top of that.

I was just about to suggest that.

Other drivers usually enable PM last, so they don't have PM calls in unwind 
code. However, I think current approach is just as valid (with a fix).

Best regards,
Jernej

> 
> Regards,
> 
> Andrzej
> 
> > 
> >>> +		goto err_rst_assert;
> >>
> >> Before your patch is applied if clk_bulk_prepare() fails, we
> >> simply return on the spot. After the patch is applied not only
> >> do you...
> >>
> >>>    	}
> >>>    	ret = v4l2_device_register(&pdev->dev, &vpu->v4l2_dev);
> >>> @@ -1037,6 +1047,8 @@ static int hantro_probe(struct platform_device 
*pdev)
> >>>    	v4l2_device_unregister(&vpu->v4l2_dev);
> >>>    err_clk_unprepare:
> >>>    	clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
> >>> +err_rst_assert:
> >>> +	reset_control_assert(vpu->resets);
> >>
> >> ...revert the effect of reset_control_deassert(), you also...
> >>
> >>>    	pm_runtime_dont_use_autosuspend(vpu->dev);
> >>>    	pm_runtime_disable(vpu->dev);
> >>
> >> ... do pm_*() stuff. Is there any reason why this is needed?
> > 
> > So, yes, it's needed, but you're correct to spot that it's not
> > consistent.
> > 
> > regards,
> > dan carpenter
> > 
> 
> 



^ permalink raw reply	[flat|nested] 40+ messages in thread
* Re: Re: [PATCH 1/7] media: hantro: add support for reset lines
@ 2021-11-23 16:46           ` Jernej Škrabec
  0 siblings, 0 replies; 40+ messages in thread
From: Jernej Škrabec @ 2021-11-23 16:46 UTC (permalink / raw)
  To: Dan Carpenter, Andrzej Pietrasiewicz
  Cc: linux-media, ezequiel, nicolas.dufresne, mchehab, robh+dt,
	mripard, wens, p.zabel, gregkh, devicetree, linux-arm-kernel,
	linux-sunxi, linux-kernel, linux-staging

Hi all,

Dne torek, 23. november 2021 ob 17:36:57 CET je Andrzej Pietrasiewicz 
napisal(a):
> Hi Dan, hi Jernej,
> 
> W dniu 23.11.2021 o 15:59, Dan Carpenter pisze:
> > On Tue, Nov 23, 2021 at 12:09:03PM +0100, Andrzej Pietrasiewicz wrote:
> >>> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/
media/hantro/hantro_drv.c
> >>> index ab2467998d29..8c3de31f51b3 100644
> >>> --- a/drivers/staging/media/hantro/hantro_drv.c
> >>> +++ b/drivers/staging/media/hantro/hantro_drv.c
> >>> @@ -905,6 +905,10 @@ static int hantro_probe(struct platform_device 
*pdev)
> >>>    			return PTR_ERR(vpu->clocks[0].clk);
> >>>    	}
> >>> +	vpu->resets = devm_reset_control_array_get(&pdev->dev, false, 
true);
> >>> +	if (IS_ERR(vpu->resets))
> >>> +		return PTR_ERR(vpu->resets);
> >>> +
> >>>    	num_bases = vpu->variant->num_regs ?: 1;
> >>>    	vpu->reg_bases = devm_kcalloc(&pdev->dev, num_bases,
> >>>    				      sizeof(*vpu->reg_bases), 
GFP_KERNEL);
> >>> @@ -978,10 +982,16 @@ static int hantro_probe(struct platform_device 
*pdev)
> >>>    	pm_runtime_use_autosuspend(vpu->dev);
> >>>    	pm_runtime_enable(vpu->dev);
> >          ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > It looks like this is the pm stuff that we have to unwind on error
> > 
> >>> +	ret = reset_control_deassert(vpu->resets);
> >>> +	if (ret) {
> >>> +		dev_err(&pdev->dev, "Failed to deassert resets\n");
> >>> +		return ret;
> >                  ^^^^^^^^^^
> > So this return should be a goto undo_pm_stuff
> > 
> > 
> >>> +	}
> >>> +
> >>>    	ret = clk_bulk_prepare(vpu->variant->num_clocks, vpu->clocks);
> >>>    	if (ret) {
> >>>    		dev_err(&pdev->dev, "Failed to prepare clocks\n");
> >>> -		return ret;
> > 
> > And this return should also have been a goto so it's a bug in the
> > original code.
> 
> So we probably want a separate patch addressing that first, and then
> the series proper on top of that.

I was just about to suggest that.

Other drivers usually enable PM last, so they don't have PM calls in unwind 
code. However, I think current approach is just as valid (with a fix).

Best regards,
Jernej

> 
> Regards,
> 
> Andrzej
> 
> > 
> >>> +		goto err_rst_assert;
> >>
> >> Before your patch is applied if clk_bulk_prepare() fails, we
> >> simply return on the spot. After the patch is applied not only
> >> do you...
> >>
> >>>    	}
> >>>    	ret = v4l2_device_register(&pdev->dev, &vpu->v4l2_dev);
> >>> @@ -1037,6 +1047,8 @@ static int hantro_probe(struct platform_device 
*pdev)
> >>>    	v4l2_device_unregister(&vpu->v4l2_dev);
> >>>    err_clk_unprepare:
> >>>    	clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
> >>> +err_rst_assert:
> >>> +	reset_control_assert(vpu->resets);
> >>
> >> ...revert the effect of reset_control_deassert(), you also...
> >>
> >>>    	pm_runtime_dont_use_autosuspend(vpu->dev);
> >>>    	pm_runtime_disable(vpu->dev);
> >>
> >> ... do pm_*() stuff. Is there any reason why this is needed?
> > 
> > So, yes, it's needed, but you're correct to spot that it's not
> > consistent.
> > 
> > regards,
> > dan carpenter
> > 
> 
> 



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

^ permalink raw reply	[flat|nested] 40+ messages in thread
* Re: [PATCH 4/7] media: hantro: move postproc enablement for old cores
  2021-11-22 18:46   ` Jernej Skrabec
@ 2021-11-25 12:00     ` Ezequiel Garcia
  -1 siblings, 0 replies; 40+ messages in thread
From: Ezequiel Garcia @ 2021-11-25 12:00 UTC (permalink / raw)
  To: Jernej Skrabec
  Cc: linux-media, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, andrzej.p, gregkh, devicetree, linux-arm-kernel,
	linux-sunxi, linux-kernel, linux-staging

Hi Jernej,

On Mon, Nov 22, 2021 at 07:46:59PM +0100, Jernej Skrabec wrote:
> Older G2 cores, like that in Allwinner H6, seem to have issue with
> latching postproc register values if this is first thing done in job.
> Moving that to the end solves the issue.
> 

Any idea what exact register should be written before the post-processor
is enabled, for H6 to work? Also, which of the PP registers need
to be written "at the end"?

> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  drivers/staging/media/hantro/hantro_drv.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> index 8c3de31f51b3..530994ab3024 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -130,7 +130,7 @@ void hantro_start_prepare_run(struct hantro_ctx *ctx)
>  	v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
>  				&ctx->ctrl_handler);
>  
> -	if (!ctx->is_encoder) {
> +	if (!ctx->is_encoder && !ctx->dev->variant->legacy_regs) {

To make this less fragile, do you think it would make sense to
have a dedicated quirk flag, something like "legacy_post_proc",
instead of overloading the meaning of legacy_regs.

What do you think?

Thanks,
Ezequiel

>  		if (hantro_needs_postproc(ctx, ctx->vpu_dst_fmt))
>  			hantro_postproc_enable(ctx);
>  		else
> @@ -142,6 +142,13 @@ void hantro_end_prepare_run(struct hantro_ctx *ctx)
>  {
>  	struct vb2_v4l2_buffer *src_buf;
>  
> +	if (ctx->dev->variant->legacy_regs && !ctx->is_encoder) {
> +		if (hantro_needs_postproc(ctx, ctx->vpu_dst_fmt))
> +			hantro_postproc_enable(ctx);
> +		else
> +			hantro_postproc_disable(ctx);
> +	}
> +
>  	src_buf = hantro_get_src_buf(ctx);
>  	v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
>  				   &ctx->ctrl_handler);
> -- 
> 2.34.0
> 

^ permalink raw reply	[flat|nested] 40+ messages in thread
* Re: [PATCH 4/7] media: hantro: move postproc enablement for old cores
@ 2021-11-25 12:00     ` Ezequiel Garcia
  0 siblings, 0 replies; 40+ messages in thread
From: Ezequiel Garcia @ 2021-11-25 12:00 UTC (permalink / raw)
  To: Jernej Skrabec
  Cc: linux-media, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, andrzej.p, gregkh, devicetree, linux-arm-kernel,
	linux-sunxi, linux-kernel, linux-staging

Hi Jernej,

On Mon, Nov 22, 2021 at 07:46:59PM +0100, Jernej Skrabec wrote:
> Older G2 cores, like that in Allwinner H6, seem to have issue with
> latching postproc register values if this is first thing done in job.
> Moving that to the end solves the issue.
> 

Any idea what exact register should be written before the post-processor
is enabled, for H6 to work? Also, which of the PP registers need
to be written "at the end"?

> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  drivers/staging/media/hantro/hantro_drv.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> index 8c3de31f51b3..530994ab3024 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -130,7 +130,7 @@ void hantro_start_prepare_run(struct hantro_ctx *ctx)
>  	v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
>  				&ctx->ctrl_handler);
>  
> -	if (!ctx->is_encoder) {
> +	if (!ctx->is_encoder && !ctx->dev->variant->legacy_regs) {

To make this less fragile, do you think it would make sense to
have a dedicated quirk flag, something like "legacy_post_proc",
instead of overloading the meaning of legacy_regs.

What do you think?

Thanks,
Ezequiel

>  		if (hantro_needs_postproc(ctx, ctx->vpu_dst_fmt))
>  			hantro_postproc_enable(ctx);
>  		else
> @@ -142,6 +142,13 @@ void hantro_end_prepare_run(struct hantro_ctx *ctx)
>  {
>  	struct vb2_v4l2_buffer *src_buf;
>  
> +	if (ctx->dev->variant->legacy_regs && !ctx->is_encoder) {
> +		if (hantro_needs_postproc(ctx, ctx->vpu_dst_fmt))
> +			hantro_postproc_enable(ctx);
> +		else
> +			hantro_postproc_disable(ctx);
> +	}
> +
>  	src_buf = hantro_get_src_buf(ctx);
>  	v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
>  				   &ctx->ctrl_handler);
> -- 
> 2.34.0
> 

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

^ permalink raw reply	[flat|nested] 40+ messages in thread
* Re: [PATCH 4/7] media: hantro: move postproc enablement for old cores
  2021-11-25 12:00     ` Ezequiel Garcia
@ 2021-11-25 19:21       ` Jernej Škrabec
  -1 siblings, 0 replies; 40+ messages in thread
From: Jernej Škrabec @ 2021-11-25 19:21 UTC (permalink / raw)
  To: Ezequiel Garcia
  Cc: linux-media, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, andrzej.p, gregkh, devicetree, linux-arm-kernel,
	linux-sunxi, linux-kernel, linux-staging

Hi Ezequiel,

Dne četrtek, 25. november 2021 ob 13:00:24 CET je Ezequiel Garcia napisal(a):
> Hi Jernej,
> 
> On Mon, Nov 22, 2021 at 07:46:59PM +0100, Jernej Skrabec wrote:
> > Older G2 cores, like that in Allwinner H6, seem to have issue with
> > latching postproc register values if this is first thing done in job.
> > Moving that to the end solves the issue.
> 
> Any idea what exact register should be written before the post-processor
> is enabled, for H6 to work? Also, which of the PP registers need
> to be written "at the end"?

No, there is too much registers to determine this exactly. Vendor library 
actually stores register values in buffer and write them all at once in 
increasing order. This is probably the reason why HDL engineers missed this 
issue...

> 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > ---
> > 
> >  drivers/staging/media/hantro/hantro_drv.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_drv.c
> > b/drivers/staging/media/hantro/hantro_drv.c index
> > 8c3de31f51b3..530994ab3024 100644
> > --- a/drivers/staging/media/hantro/hantro_drv.c
> > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > @@ -130,7 +130,7 @@ void hantro_start_prepare_run(struct hantro_ctx *ctx)
> > 
> >  	v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
> >  	
> >  				&ctx->ctrl_handler);
> > 
> > -	if (!ctx->is_encoder) {
> > +	if (!ctx->is_encoder && !ctx->dev->variant->legacy_regs) {
> 
> To make this less fragile, do you think it would make sense to
> have a dedicated quirk flag, something like "legacy_post_proc",
> instead of overloading the meaning of legacy_regs.

Sure, it can be done :) But then I suggest "late_post_proc" - it better 
describes what it does.

Best regards,
Jernej

> 
> What do you think?
> 
> Thanks,
> Ezequiel
> 
> >  		if (hantro_needs_postproc(ctx, ctx->vpu_dst_fmt))
> >  		
> >  			hantro_postproc_enable(ctx);
> >  		
> >  		else
> > 
> > @@ -142,6 +142,13 @@ void hantro_end_prepare_run(struct hantro_ctx *ctx)
> > 
> >  {
> >  
> >  	struct vb2_v4l2_buffer *src_buf;
> > 
> > +	if (ctx->dev->variant->legacy_regs && !ctx->is_encoder) {
> > +		if (hantro_needs_postproc(ctx, ctx->vpu_dst_fmt))
> > +			hantro_postproc_enable(ctx);
> > +		else
> > +			hantro_postproc_disable(ctx);
> > +	}
> > +
> > 
> >  	src_buf = hantro_get_src_buf(ctx);
> >  	v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
> >  	
> >  				   &ctx->ctrl_handler);





^ permalink raw reply	[flat|nested] 40+ messages in thread
* Re: [PATCH 4/7] media: hantro: move postproc enablement for old cores
@ 2021-11-25 19:21       ` Jernej Škrabec
  0 siblings, 0 replies; 40+ messages in thread
From: Jernej Škrabec @ 2021-11-25 19:21 UTC (permalink / raw)
  To: Ezequiel Garcia
  Cc: linux-media, nicolas.dufresne, mchehab, robh+dt, mripard, wens,
	p.zabel, andrzej.p, gregkh, devicetree, linux-arm-kernel,
	linux-sunxi, linux-kernel, linux-staging

Hi Ezequiel,

Dne četrtek, 25. november 2021 ob 13:00:24 CET je Ezequiel Garcia napisal(a):
> Hi Jernej,
> 
> On Mon, Nov 22, 2021 at 07:46:59PM +0100, Jernej Skrabec wrote:
> > Older G2 cores, like that in Allwinner H6, seem to have issue with
> > latching postproc register values if this is first thing done in job.
> > Moving that to the end solves the issue.
> 
> Any idea what exact register should be written before the post-processor
> is enabled, for H6 to work? Also, which of the PP registers need
> to be written "at the end"?

No, there is too much registers to determine this exactly. Vendor library 
actually stores register values in buffer and write them all at once in 
increasing order. This is probably the reason why HDL engineers missed this 
issue...

> 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > ---
> > 
> >  drivers/staging/media/hantro/hantro_drv.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_drv.c
> > b/drivers/staging/media/hantro/hantro_drv.c index
> > 8c3de31f51b3..530994ab3024 100644
> > --- a/drivers/staging/media/hantro/hantro_drv.c
> > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > @@ -130,7 +130,7 @@ void hantro_start_prepare_run(struct hantro_ctx *ctx)
> > 
> >  	v4l2_ctrl_request_setup(src_buf->vb2_buf.req_obj.req,
> >  	
> >  				&ctx->ctrl_handler);
> > 
> > -	if (!ctx->is_encoder) {
> > +	if (!ctx->is_encoder && !ctx->dev->variant->legacy_regs) {
> 
> To make this less fragile, do you think it would make sense to
> have a dedicated quirk flag, something like "legacy_post_proc",
> instead of overloading the meaning of legacy_regs.

Sure, it can be done :) But then I suggest "late_post_proc" - it better 
describes what it does.

Best regards,
Jernej

> 
> What do you think?
> 
> Thanks,
> Ezequiel
> 
> >  		if (hantro_needs_postproc(ctx, ctx->vpu_dst_fmt))
> >  		
> >  			hantro_postproc_enable(ctx);
> >  		
> >  		else
> > 
> > @@ -142,6 +142,13 @@ void hantro_end_prepare_run(struct hantro_ctx *ctx)
> > 
> >  {
> >  
> >  	struct vb2_v4l2_buffer *src_buf;
> > 
> > +	if (ctx->dev->variant->legacy_regs && !ctx->is_encoder) {
> > +		if (hantro_needs_postproc(ctx, ctx->vpu_dst_fmt))
> > +			hantro_postproc_enable(ctx);
> > +		else
> > +			hantro_postproc_disable(ctx);
> > +	}
> > +
> > 
> >  	src_buf = hantro_get_src_buf(ctx);
> >  	v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req,
> >  	
> >  				   &ctx->ctrl_handler);





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

^ permalink raw reply	[flat|nested] 40+ messages in thread
* Re: [PATCH 1/7] media: hantro: add support for reset lines
       [not found]           ` <CAAEAJfD3d4zjwvbv967+oe6awnAkZiGNKroYF5jvoTy=0sn+Pw@mail.gmail.com>
@ 2021-11-30 10:39               ` Andrzej Pietrasiewicz
  0 siblings, 0 replies; 40+ messages in thread
From: Andrzej Pietrasiewicz @ 2021-11-30 10:39 UTC (permalink / raw)
  To: Ezequiel Garcia, Jernej Škrabec
  Cc: Dan Carpenter, linux-media, Nicolas Dufresne,
	Mauro Carvalho Chehab, Rob Herring, Maxime Ripard, Chen-Yu Tsai,
	Philipp Zabel, Greg KH, devicetree, linux-arm-kernel,
	linux-sunxi, Linux Kernel Mailing List,
	open list:STAGING SUBSYSTEM, Robert Beckett

Hi Ezequiel,

W dniu 23.11.2021 o 19:07, Ezequiel Garcia pisze:
> Hi all,
> 
> Reset logic tends to be highly integration-specific, so it  could be more robust 
> to deal  with this in  the machine specific file. I have some vague recollection 
> of our experience here when we  integrated vc8000 last year, but I cannot recall 
> the outcome.
> 

Do you mean vpu->variant->init()?

That is the very first thing we do after the devm_*() calls. So any subsequent
initialization that fails would want vpu->variant->deinit(). Maybe at this
moment handling the resets at the common level is simpler? Existing drivers
will get NULL anyway from devm_reset_control_array_get().

Regards,

Andrzej

> I'm Ccing Bob who might remember better.
> 
> Thanks,
> Ezequiel
> 
> 
> 
> El mar., nov. 23, 2021 1:46 PM, Jernej Škrabec <jernej.skrabec@gmail.com 
> <mailto:jernej.skrabec@gmail.com>> escribió:
> 
>     Hi all,
> 
>     Dne torek, 23. november 2021 ob 17:36:57 CET je Andrzej Pietrasiewicz
>     napisal(a):
>      > Hi Dan, hi Jernej,
>      >
>      > W dniu 23.11.2021 o 15:59, Dan Carpenter pisze:
>      > > On Tue, Nov 23, 2021 at 12:09:03PM +0100, Andrzej Pietrasiewicz wrote:
>      > >>> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/
>     media/hantro/hantro_drv.c
>      > >>> index ab2467998d29..8c3de31f51b3 100644
>      > >>> --- a/drivers/staging/media/hantro/hantro_drv.c
>      > >>> +++ b/drivers/staging/media/hantro/hantro_drv.c
>      > >>> @@ -905,6 +905,10 @@ static int hantro_probe(struct platform_device
>     *pdev)
>      > >>>                           return PTR_ERR(vpu->clocks[0].clk);
>      > >>>           }
>      > >>> + vpu->resets = devm_reset_control_array_get(&pdev->dev, false,
>     true);
>      > >>> + if (IS_ERR(vpu->resets))
>      > >>> +         return PTR_ERR(vpu->resets);
>      > >>> +
>      > >>>           num_bases = vpu->variant->num_regs ?: 1;
>      > >>>           vpu->reg_bases = devm_kcalloc(&pdev->dev, num_bases,
>      > >>>                                         sizeof(*vpu->reg_bases),
>     GFP_KERNEL);
>      > >>> @@ -978,10 +982,16 @@ static int hantro_probe(struct platform_device
>     *pdev)
>      > >>>           pm_runtime_use_autosuspend(vpu->dev);
>      > >>>           pm_runtime_enable(vpu->dev);
>      > >          ^^^^^^^^^^^^^^^^^^^^^^^^^^^
>      > > It looks like this is the pm stuff that we have to unwind on error
>      > >
>      > >>> + ret = reset_control_deassert(vpu->resets);
>      > >>> + if (ret) {
>      > >>> +         dev_err(&pdev->dev, "Failed to deassert resets\n");
>      > >>> +         return ret;
>      > >                  ^^^^^^^^^^
>      > > So this return should be a goto undo_pm_stuff
>      > >
>      > >
>      > >>> + }
>      > >>> +
>      > >>>           ret = clk_bulk_prepare(vpu->variant->num_clocks, vpu->clocks);
>      > >>>           if (ret) {
>      > >>>                   dev_err(&pdev->dev, "Failed to prepare clocks\n");
>      > >>> -         return ret;
>      > >
>      > > And this return should also have been a goto so it's a bug in the
>      > > original code.
>      >
>      > So we probably want a separate patch addressing that first, and then
>      > the series proper on top of that.
> 
>     I was just about to suggest that.
> 
>     Other drivers usually enable PM last, so they don't have PM calls in unwind
>     code. However, I think current approach is just as valid (with a fix).
> 
>     Best regards,
>     Jernej
> 
>      >
>      > Regards,
>      >
>      > Andrzej
>      >
>      > >
>      > >>> +         goto err_rst_assert;
>      > >>
>      > >> Before your patch is applied if clk_bulk_prepare() fails, we
>      > >> simply return on the spot. After the patch is applied not only
>      > >> do you...
>      > >>
>      > >>>           }
>      > >>>           ret = v4l2_device_register(&pdev->dev, &vpu->v4l2_dev);
>      > >>> @@ -1037,6 +1047,8 @@ static int hantro_probe(struct platform_device
>     *pdev)
>      > >>>           v4l2_device_unregister(&vpu->v4l2_dev);
>      > >>>    err_clk_unprepare:
>      > >>>           clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
>      > >>> +err_rst_assert:
>      > >>> + reset_control_assert(vpu->resets);
>      > >>
>      > >> ...revert the effect of reset_control_deassert(), you also...
>      > >>
>      > >>>           pm_runtime_dont_use_autosuspend(vpu->dev);
>      > >>>           pm_runtime_disable(vpu->dev);
>      > >>
>      > >> ... do pm_*() stuff. Is there any reason why this is needed?
>      > >
>      > > So, yes, it's needed, but you're correct to spot that it's not
>      > > consistent.
>      > >
>      > > regards,
>      > > dan carpenter
>      > >
>      >
>      >
> 
> 


^ permalink raw reply	[flat|nested] 40+ messages in thread
* Re: [PATCH 1/7] media: hantro: add support for reset lines
@ 2021-11-30 10:39               ` Andrzej Pietrasiewicz
  0 siblings, 0 replies; 40+ messages in thread
From: Andrzej Pietrasiewicz @ 2021-11-30 10:39 UTC (permalink / raw)
  To: Ezequiel Garcia, Jernej Škrabec
  Cc: Dan Carpenter, linux-media, Nicolas Dufresne,
	Mauro Carvalho Chehab, Rob Herring, Maxime Ripard, Chen-Yu Tsai,
	Philipp Zabel, Greg KH, devicetree, linux-arm-kernel,
	linux-sunxi, Linux Kernel Mailing List,
	open list:STAGING SUBSYSTEM, Robert Beckett

Hi Ezequiel,

W dniu 23.11.2021 o 19:07, Ezequiel Garcia pisze:
> Hi all,
> 
> Reset logic tends to be highly integration-specific, so it  could be more robust 
> to deal  with this in  the machine specific file. I have some vague recollection 
> of our experience here when we  integrated vc8000 last year, but I cannot recall 
> the outcome.
> 

Do you mean vpu->variant->init()?

That is the very first thing we do after the devm_*() calls. So any subsequent
initialization that fails would want vpu->variant->deinit(). Maybe at this
moment handling the resets at the common level is simpler? Existing drivers
will get NULL anyway from devm_reset_control_array_get().

Regards,

Andrzej

> I'm Ccing Bob who might remember better.
> 
> Thanks,
> Ezequiel
> 
> 
> 
> El mar., nov. 23, 2021 1:46 PM, Jernej Škrabec <jernej.skrabec@gmail.com 
> <mailto:jernej.skrabec@gmail.com>> escribió:
> 
>     Hi all,
> 
>     Dne torek, 23. november 2021 ob 17:36:57 CET je Andrzej Pietrasiewicz
>     napisal(a):
>      > Hi Dan, hi Jernej,
>      >
>      > W dniu 23.11.2021 o 15:59, Dan Carpenter pisze:
>      > > On Tue, Nov 23, 2021 at 12:09:03PM +0100, Andrzej Pietrasiewicz wrote:
>      > >>> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/
>     media/hantro/hantro_drv.c
>      > >>> index ab2467998d29..8c3de31f51b3 100644
>      > >>> --- a/drivers/staging/media/hantro/hantro_drv.c
>      > >>> +++ b/drivers/staging/media/hantro/hantro_drv.c
>      > >>> @@ -905,6 +905,10 @@ static int hantro_probe(struct platform_device
>     *pdev)
>      > >>>                           return PTR_ERR(vpu->clocks[0].clk);
>      > >>>           }
>      > >>> + vpu->resets = devm_reset_control_array_get(&pdev->dev, false,
>     true);
>      > >>> + if (IS_ERR(vpu->resets))
>      > >>> +         return PTR_ERR(vpu->resets);
>      > >>> +
>      > >>>           num_bases = vpu->variant->num_regs ?: 1;
>      > >>>           vpu->reg_bases = devm_kcalloc(&pdev->dev, num_bases,
>      > >>>                                         sizeof(*vpu->reg_bases),
>     GFP_KERNEL);
>      > >>> @@ -978,10 +982,16 @@ static int hantro_probe(struct platform_device
>     *pdev)
>      > >>>           pm_runtime_use_autosuspend(vpu->dev);
>      > >>>           pm_runtime_enable(vpu->dev);
>      > >          ^^^^^^^^^^^^^^^^^^^^^^^^^^^
>      > > It looks like this is the pm stuff that we have to unwind on error
>      > >
>      > >>> + ret = reset_control_deassert(vpu->resets);
>      > >>> + if (ret) {
>      > >>> +         dev_err(&pdev->dev, "Failed to deassert resets\n");
>      > >>> +         return ret;
>      > >                  ^^^^^^^^^^
>      > > So this return should be a goto undo_pm_stuff
>      > >
>      > >
>      > >>> + }
>      > >>> +
>      > >>>           ret = clk_bulk_prepare(vpu->variant->num_clocks, vpu->clocks);
>      > >>>           if (ret) {
>      > >>>                   dev_err(&pdev->dev, "Failed to prepare clocks\n");
>      > >>> -         return ret;
>      > >
>      > > And this return should also have been a goto so it's a bug in the
>      > > original code.
>      >
>      > So we probably want a separate patch addressing that first, and then
>      > the series proper on top of that.
> 
>     I was just about to suggest that.
> 
>     Other drivers usually enable PM last, so they don't have PM calls in unwind
>     code. However, I think current approach is just as valid (with a fix).
> 
>     Best regards,
>     Jernej
> 
>      >
>      > Regards,
>      >
>      > Andrzej
>      >
>      > >
>      > >>> +         goto err_rst_assert;
>      > >>
>      > >> Before your patch is applied if clk_bulk_prepare() fails, we
>      > >> simply return on the spot. After the patch is applied not only
>      > >> do you...
>      > >>
>      > >>>           }
>      > >>>           ret = v4l2_device_register(&pdev->dev, &vpu->v4l2_dev);
>      > >>> @@ -1037,6 +1047,8 @@ static int hantro_probe(struct platform_device
>     *pdev)
>      > >>>           v4l2_device_unregister(&vpu->v4l2_dev);
>      > >>>    err_clk_unprepare:
>      > >>>           clk_bulk_unprepare(vpu->variant->num_clocks, vpu->clocks);
>      > >>> +err_rst_assert:
>      > >>> + reset_control_assert(vpu->resets);
>      > >>
>      > >> ...revert the effect of reset_control_deassert(), you also...
>      > >>
>      > >>>           pm_runtime_dont_use_autosuspend(vpu->dev);
>      > >>>           pm_runtime_disable(vpu->dev);
>      > >>
>      > >> ... do pm_*() stuff. Is there any reason why this is needed?
>      > >
>      > > So, yes, it's needed, but you're correct to spot that it's not
>      > > consistent.
>      > >
>      > > regards,
>      > > dan carpenter
>      > >
>      >
>      >
> 
> 


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

^ permalink raw reply	[flat|nested] 40+ messages in thread
end of thread, other threads:[~2021-11-30 10:40 UTC | newest]

Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-22 18:46 [PATCH 0/7] media: hantro: add Allwinner H6 support Jernej Skrabec
2021-11-22 18:46 ` Jernej Skrabec
2021-11-22 18:46 ` [PATCH 1/7] media: hantro: add support for reset lines Jernej Skrabec
2021-11-22 18:46   ` Jernej Skrabec
2021-11-23 11:09   ` Andrzej Pietrasiewicz
2021-11-23 11:09     ` Andrzej Pietrasiewicz
2021-11-23 14:59     ` Dan Carpenter
2021-11-23 14:59       ` Dan Carpenter
2021-11-23 16:36       ` Andrzej Pietrasiewicz
2021-11-23 16:36         ` Andrzej Pietrasiewicz
2021-11-23 16:46         ` Jernej Škrabec
2021-11-23 16:46           ` Jernej Škrabec
     [not found]           ` <CAAEAJfD3d4zjwvbv967+oe6awnAkZiGNKroYF5jvoTy=0sn+Pw@mail.gmail.com>
2021-11-30 10:39             ` Andrzej Pietrasiewicz
2021-11-30 10:39               ` Andrzej Pietrasiewicz
2021-11-22 18:46 ` [PATCH 2/7] media: hantro: vp9: use double buffering if needed Jernej Skrabec
2021-11-22 18:46   ` Jernej Skrabec
2021-11-23 11:22   ` Andrzej Pietrasiewicz
2021-11-23 11:22     ` Andrzej Pietrasiewicz
2021-11-22 18:46 ` [PATCH 3/7] media: hantro: vp9: add support for legacy register set Jernej Skrabec
2021-11-22 18:46   ` Jernej Skrabec
2021-11-23 12:06   ` Andrzej Pietrasiewicz
2021-11-23 12:06     ` Andrzej Pietrasiewicz
2021-11-22 18:46 ` [PATCH 4/7] media: hantro: move postproc enablement for old cores Jernej Skrabec
2021-11-22 18:46   ` Jernej Skrabec
2021-11-23 12:12   ` Andrzej Pietrasiewicz
2021-11-23 12:12     ` Andrzej Pietrasiewicz
2021-11-25 12:00   ` Ezequiel Garcia
2021-11-25 12:00     ` Ezequiel Garcia
2021-11-25 19:21     ` Jernej Škrabec
2021-11-25 19:21       ` Jernej Škrabec
2021-11-22 18:47 ` [PATCH 5/7] media: dt-bindings: allwinner: document H6 Hantro G2 binding Jernej Skrabec
2021-11-22 18:47   ` Jernej Skrabec
2021-11-22 18:47 ` [PATCH 6/7] media: hantro: Add support for Allwinner H6 Jernej Skrabec
2021-11-22 18:47   ` Jernej Skrabec
2021-11-23 12:23   ` Andrzej Pietrasiewicz
2021-11-23 12:23     ` Andrzej Pietrasiewicz
2021-11-22 18:47 ` [PATCH 7/7] arm64: dts: allwinner: h6: Add Hantro G2 node Jernej Skrabec
2021-11-22 18:47   ` Jernej Skrabec
2021-11-22 18:51 ` [PATCH 0/7] media: hantro: add Allwinner H6 support Jernej Škrabec
2021-11-22 18:51   ` Jernej Škrabec

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.