[PATCH 0/6] Wave515 decoder IP support

[PATCH 0/6] Wave515 decoder IP support

[Ivan Bornyakov]

Initial support for Wave515 multi-decoder IP among other refinements.
This was tested on FPGA prototype, so wave5_dt_ids[] was not expanded.

Ivan Bornyakov (6):
  media: chips-media: wave5: support decoding higher bit-depth profiles
  media: chips-media: wave5: support reset lines
  dt-bindings: media: cnm,wave521c: drop resets restriction
  media: chips-media: wave5: separate irq setup routine
  media: chips-media: wave5: refine SRAM usage
  media: chips-media: wave5: support Wave515 decoder

 .../bindings/media/cnm,wave521c.yaml          |   3 +-
 .../platform/chips-media/wave5/wave5-helper.c |   3 -
 .../platform/chips-media/wave5/wave5-hw.c     | 293 +++++++++++++-----
 .../chips-media/wave5/wave5-regdefine.h       |   5 +
 .../platform/chips-media/wave5/wave5-vdi.c    |  27 +-
 .../chips-media/wave5/wave5-vpu-dec.c         |  17 +-
 .../chips-media/wave5/wave5-vpu-enc.c         |   2 -
 .../platform/chips-media/wave5/wave5-vpu.c    |  33 +-
 .../platform/chips-media/wave5/wave5-vpuapi.h |   3 +-
 .../chips-media/wave5/wave5-vpuconfig.h       |   9 +-
 .../media/platform/chips-media/wave5/wave5.h  |   1 +
 11 files changed, 272 insertions(+), 124 deletions(-)

-- 
2.44.0

[PATCH 1/6] media: chips-media: wave5: support decoding higher bit-depth profiles

[Ivan Bornyakov]

Add support for decoding higher than 8 bit-depth profiles by scaling FBC
buffer stride and size by the factor of (bitdepth / 8).

Signed-off-by: Ivan Bornyakov <brnkv.i1@gmail.com>
---
 .../platform/chips-media/wave5/wave5-vpu-dec.c    | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
index ef227af72348..aa0401f35d32 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
@@ -1055,6 +1055,7 @@ static int wave5_prepare_fb(struct vpu_instance *inst)
 	int ret, i;
 	struct v4l2_m2m_buffer *buf, *n;
 	struct v4l2_m2m_ctx *m2m_ctx = inst->v4l2_fh.m2m_ctx;
+	u32 bitdepth = inst->codec_info->dec_info.initial_info.luma_bitdepth;
 
 	linear_num = v4l2_m2m_num_dst_bufs_ready(m2m_ctx);
 	non_linear_num = inst->fbc_buf_count;
@@ -1063,7 +1064,7 @@ static int wave5_prepare_fb(struct vpu_instance *inst)
 		struct frame_buffer *frame = &inst->frame_buf[i];
 		struct vpu_buf *vframe = &inst->frame_vbuf[i];
 
-		fb_stride = inst->dst_fmt.width;
+		fb_stride = ALIGN(inst->dst_fmt.width * bitdepth / 8, 32);
 		fb_height = ALIGN(inst->dst_fmt.height, 32);
 		luma_size = fb_stride * fb_height;
 
@@ -1408,22 +1409,10 @@ static int wave5_vpu_dec_start_streaming(struct vb2_queue *q, unsigned int count
 		if (ret)
 			goto free_bitstream_vbuf;
 	} else if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
-		struct dec_initial_info *initial_info =
-			&inst->codec_info->dec_info.initial_info;
-
 		if (inst->state == VPU_INST_STATE_STOP)
 			ret = switch_state(inst, VPU_INST_STATE_INIT_SEQ);
 		if (ret)
 			goto return_buffers;
-
-		if (inst->state == VPU_INST_STATE_INIT_SEQ) {
-			if (initial_info->luma_bitdepth != 8) {
-				dev_info(inst->dev->dev, "%s: no support for %d bit depth",
-					 __func__, initial_info->luma_bitdepth);
-				ret = -EINVAL;
-				goto return_buffers;
-			}
-		}
 	}
 
 	return ret;
-- 
2.44.0

[PATCH 2/6] media: chips-media: wave5: support reset lines

[Ivan Bornyakov]

Add initial support for optional reset lines. For now, simply deassert
resets on driver probe and assert them back on driver remove.

Signed-off-by: Ivan Bornyakov <brnkv.i1@gmail.com>
---
 .../media/platform/chips-media/wave5/wave5-vpu.c    | 13 +++++++++++++
 .../media/platform/chips-media/wave5/wave5-vpuapi.h |  1 +
 2 files changed, 14 insertions(+)

diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.c b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
index 1b3df5b04249..f3ecadefd37a 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
@@ -10,6 +10,7 @@
 #include <linux/clk.h>
 #include <linux/firmware.h>
 #include <linux/interrupt.h>
+#include <linux/reset.h>
 #include "wave5-vpu.h"
 #include "wave5-regdefine.h"
 #include "wave5-vpuconfig.h"
@@ -151,6 +152,17 @@ static int wave5_vpu_probe(struct platform_device *pdev)
 	dev_set_drvdata(&pdev->dev, dev);
 	dev->dev = &pdev->dev;
 
+	dev->resets = devm_reset_control_array_get_optional_exclusive(&pdev->dev);
+	if (IS_ERR(dev->resets)) {
+		return dev_err_probe(&pdev->dev, PTR_ERR(dev->resets),
+				     "Failed to get reset control\n");
+	} else {
+		ret = reset_control_deassert(dev->resets);
+		if (ret)
+			return dev_err_probe(&pdev->dev, ret,
+					     "Failed to deassert resets\n");
+	}
+
 	ret = devm_clk_bulk_get_all(&pdev->dev, &dev->clks);
 
 	/* continue without clock, assume externally managed */
@@ -256,6 +268,7 @@ static void wave5_vpu_remove(struct platform_device *pdev)
 
 	mutex_destroy(&dev->dev_lock);
 	mutex_destroy(&dev->hw_lock);
+	reset_control_assert(dev->resets);
 	clk_bulk_disable_unprepare(dev->num_clks, dev->clks);
 	wave5_vpu_enc_unregister_device(dev);
 	wave5_vpu_dec_unregister_device(dev);
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
index 352f6e904e50..fa62a85080b5 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
@@ -757,6 +757,7 @@ struct vpu_device {
 	struct ida inst_ida;
 	struct clk_bulk_data *clks;
 	int num_clks;
+	struct reset_control *resets;
 };
 
 struct vpu_instance;
-- 
2.44.0

[PATCH 3/6] dt-bindings: media: cnm,wave521c: drop resets restriction

[Ivan Bornyakov]

Different designs may have different amount of routed reset signals.
Drop maxItems restriction, add a small description instead.

Signed-off-by: Ivan Bornyakov <brnkv.i1@gmail.com>
---
 Documentation/devicetree/bindings/media/cnm,wave521c.yaml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/cnm,wave521c.yaml b/Documentation/devicetree/bindings/media/cnm,wave521c.yaml
index 6a11c1d11fb5..2cb4f68d27ac 100644
--- a/Documentation/devicetree/bindings/media/cnm,wave521c.yaml
+++ b/Documentation/devicetree/bindings/media/cnm,wave521c.yaml
@@ -34,7 +34,8 @@ properties:
     maxItems: 1
 
   resets:
-    maxItems: 1
+    items:
+      - description: Optional reset lines
 
   sram:
     $ref: /schemas/types.yaml#/definitions/phandle
-- 
2.44.0

[PATCH 4/6] media: chips-media: wave5: separate irq setup routine

[Ivan Bornyakov]

Separate interrupts setup routine to reduce code duplication. Also
enable interrupts based on vpu_attr->support_encoders and
vpu_attr->support_decoders fields to facilitate other Wave5xx IPs
support, because not all of them are both encoders and decoders.

Signed-off-by: Ivan Bornyakov <brnkv.i1@gmail.com>
---
 .../platform/chips-media/wave5/wave5-hw.c     | 53 +++++++++----------
 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/drivers/media/platform/chips-media/wave5/wave5-hw.c b/drivers/media/platform/chips-media/wave5/wave5-hw.c
index 2d82791f575e..cdd0a0948a94 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-hw.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-hw.c
@@ -299,6 +299,27 @@ static int wave5_send_query(struct vpu_device *vpu_dev, struct vpu_instance *ins
 	return wave5_vpu_firmware_command_queue_error_check(vpu_dev, NULL);
 }
 
+static void setup_wave5_interrupts(struct vpu_device *vpu_dev)
+{
+	u32 reg_val = 0;
+
+	if (vpu_dev->attr.support_encoders) {
+		/* Encoder interrupt */
+		reg_val |= BIT(INT_WAVE5_ENC_SET_PARAM);
+		reg_val |= BIT(INT_WAVE5_ENC_PIC);
+		reg_val |= BIT(INT_WAVE5_BSBUF_FULL);
+	}
+
+	if (vpu_dev->attr.support_decoders) {
+		/* Decoder interrupt */
+		reg_val |= BIT(INT_WAVE5_INIT_SEQ);
+		reg_val |= BIT(INT_WAVE5_DEC_PIC);
+		reg_val |= BIT(INT_WAVE5_BSBUF_EMPTY);
+	}
+
+	return vpu_write_reg(vpu_dev, W5_VPU_VINT_ENABLE, reg_val);
+}
+
 static int setup_wave5_properties(struct device *dev)
 {
 	struct vpu_device *vpu_dev = dev_get_drvdata(dev);
@@ -340,6 +361,8 @@ static int setup_wave5_properties(struct device *dev)
 	p_attr->support_vcpu_backbone = FIELD_GET(FEATURE_VCPU_BACKBONE, hw_config_def0);
 	p_attr->support_vcore_backbone = FIELD_GET(FEATURE_VCORE_BACKBONE, hw_config_def0);
 
+	setup_wave5_interrupts(vpu_dev);
+
 	return 0;
 }
 
@@ -417,16 +440,6 @@ int wave5_vpu_init(struct device *dev, u8 *fw, size_t size)
 	wave5_fio_writel(vpu_dev, W5_BACKBONE_AXI_PARAM, 0);
 	vpu_write_reg(vpu_dev, W5_SEC_AXI_PARAM, 0);
 
-	/* Encoder interrupt */
-	reg_val = BIT(INT_WAVE5_ENC_SET_PARAM);
-	reg_val |= BIT(INT_WAVE5_ENC_PIC);
-	reg_val |= BIT(INT_WAVE5_BSBUF_FULL);
-	/* Decoder interrupt */
-	reg_val |= BIT(INT_WAVE5_INIT_SEQ);
-	reg_val |= BIT(INT_WAVE5_DEC_PIC);
-	reg_val |= BIT(INT_WAVE5_BSBUF_EMPTY);
-	vpu_write_reg(vpu_dev, W5_VPU_VINT_ENABLE, reg_val);
-
 	reg_val = vpu_read_reg(vpu_dev, W5_VPU_RET_VPU_CONFIG0);
 	if (FIELD_GET(FEATURE_BACKBONE, reg_val)) {
 		reg_val = ((WAVE5_PROC_AXI_ID << 28) |
@@ -1034,16 +1047,6 @@ int wave5_vpu_re_init(struct device *dev, u8 *fw, size_t size)
 		wave5_fio_writel(vpu_dev, W5_BACKBONE_AXI_PARAM, 0);
 		vpu_write_reg(vpu_dev, W5_SEC_AXI_PARAM, 0);
 
-		/* Encoder interrupt */
-		reg_val = BIT(INT_WAVE5_ENC_SET_PARAM);
-		reg_val |= BIT(INT_WAVE5_ENC_PIC);
-		reg_val |= BIT(INT_WAVE5_BSBUF_FULL);
-		/* Decoder interrupt */
-		reg_val |= BIT(INT_WAVE5_INIT_SEQ);
-		reg_val |= BIT(INT_WAVE5_DEC_PIC);
-		reg_val |= BIT(INT_WAVE5_BSBUF_EMPTY);
-		vpu_write_reg(vpu_dev, W5_VPU_VINT_ENABLE, reg_val);
-
 		reg_val = vpu_read_reg(vpu_dev, W5_VPU_RET_VPU_CONFIG0);
 		if (FIELD_GET(FEATURE_BACKBONE, reg_val)) {
 			reg_val = ((WAVE5_PROC_AXI_ID << 28) |
@@ -1134,15 +1137,7 @@ static int wave5_vpu_sleep_wake(struct device *dev, bool i_sleep_wake, const uin
 		wave5_fio_writel(vpu_dev, W5_BACKBONE_AXI_PARAM, 0);
 		vpu_write_reg(vpu_dev, W5_SEC_AXI_PARAM, 0);
 
-		/* Encoder interrupt */
-		reg_val = BIT(INT_WAVE5_ENC_SET_PARAM);
-		reg_val |= BIT(INT_WAVE5_ENC_PIC);
-		reg_val |= BIT(INT_WAVE5_BSBUF_FULL);
-		/* Decoder interrupt */
-		reg_val |= BIT(INT_WAVE5_INIT_SEQ);
-		reg_val |= BIT(INT_WAVE5_DEC_PIC);
-		reg_val |= BIT(INT_WAVE5_BSBUF_EMPTY);
-		vpu_write_reg(vpu_dev, W5_VPU_VINT_ENABLE, reg_val);
+		setup_wave5_interrupts(vpu_dev);
 
 		reg_val = vpu_read_reg(vpu_dev, W5_VPU_RET_VPU_CONFIG0);
 		if (FIELD_GET(FEATURE_BACKBONE, reg_val)) {
-- 
2.44.0

[PATCH 5/6] media: chips-media: wave5: refine SRAM usage

[Ivan Bornyakov]

Allocate SRAM memory on module probe, free on remove. There is no need
to allocate on device open, free on close, the memory is the same every
time.

Also use gen_pool_size() to determine SRAM memory size to be allocated
instead of separate "sram-size" DT property to reduce duplication.

Signed-off-by: Ivan Bornyakov <brnkv.i1@gmail.com>
---
 .../platform/chips-media/wave5/wave5-helper.c |  3 ---
 .../platform/chips-media/wave5/wave5-vdi.c    | 21 ++++++++++---------
 .../chips-media/wave5/wave5-vpu-dec.c         |  2 --
 .../chips-media/wave5/wave5-vpu-enc.c         |  2 --
 .../platform/chips-media/wave5/wave5-vpu.c    | 12 +++++------
 .../platform/chips-media/wave5/wave5-vpuapi.h |  1 -
 6 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/drivers/media/platform/chips-media/wave5/wave5-helper.c b/drivers/media/platform/chips-media/wave5/wave5-helper.c
index 8433ecab230c..ec710b838dfe 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-helper.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-helper.c
@@ -29,9 +29,6 @@ void wave5_cleanup_instance(struct vpu_instance *inst)
 {
 	int i;
 
-	if (list_is_singular(&inst->list))
-		wave5_vdi_free_sram(inst->dev);
-
 	for (i = 0; i < inst->fbc_buf_count; i++)
 		wave5_vpu_dec_reset_framebuffer(inst, i);
 
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vdi.c b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
index 3809f70bc0b4..ee671f5a2f37 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
@@ -174,16 +174,19 @@ int wave5_vdi_allocate_array(struct vpu_device *vpu_dev, struct vpu_buf *array,
 void wave5_vdi_allocate_sram(struct vpu_device *vpu_dev)
 {
 	struct vpu_buf *vb = &vpu_dev->sram_buf;
+	dma_addr_t daddr;
+	void *vaddr;
+	size_t size;
 
-	if (!vpu_dev->sram_pool || !vpu_dev->sram_size)
+	if (!vpu_dev->sram_pool || vb->vaddr)
 		return;
 
-	if (!vb->vaddr) {
-		vb->size = vpu_dev->sram_size;
-		vb->vaddr = gen_pool_dma_alloc(vpu_dev->sram_pool, vb->size,
-					       &vb->daddr);
-		if (!vb->vaddr)
-			vb->size = 0;
+	size = gen_pool_size(vpu_dev->sram_pool);
+	vaddr = gen_pool_dma_alloc(vpu_dev->sram_pool, size, &daddr);
+	if (vaddr) {
+		vb->vaddr = vaddr;
+		vb->daddr = daddr;
+		vb->size = size;
 	}
 
 	dev_dbg(vpu_dev->dev, "%s: sram daddr: %pad, size: %zu, vaddr: 0x%p\n",
@@ -197,9 +200,7 @@ void wave5_vdi_free_sram(struct vpu_device *vpu_dev)
 	if (!vb->size || !vb->vaddr)
 		return;
 
-	if (vb->vaddr)
-		gen_pool_free(vpu_dev->sram_pool, (unsigned long)vb->vaddr,
-			      vb->size);
+	gen_pool_free(vpu_dev->sram_pool, (unsigned long)vb->vaddr, vb->size);
 
 	memset(vb, 0, sizeof(*vb));
 }
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
index aa0401f35d32..84dbe56216ad 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
@@ -1854,8 +1854,6 @@ static int wave5_vpu_open_dec(struct file *filp)
 		goto cleanup_inst;
 	}
 
-	wave5_vdi_allocate_sram(inst->dev);
-
 	return 0;
 
 cleanup_inst:
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
index 8bbf9d10b467..86ddcb82443b 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
@@ -1727,8 +1727,6 @@ static int wave5_vpu_open_enc(struct file *filp)
 		goto cleanup_inst;
 	}
 
-	wave5_vdi_allocate_sram(inst->dev);
-
 	return 0;
 
 cleanup_inst:
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.c b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
index f3ecadefd37a..2a0a70dd7062 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
@@ -178,16 +178,11 @@ static int wave5_vpu_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = of_property_read_u32(pdev->dev.of_node, "sram-size",
-				   &dev->sram_size);
-	if (ret) {
-		dev_warn(&pdev->dev, "sram-size not found\n");
-		dev->sram_size = 0;
-	}
-
 	dev->sram_pool = of_gen_pool_get(pdev->dev.of_node, "sram", 0);
 	if (!dev->sram_pool)
 		dev_warn(&pdev->dev, "sram node not found\n");
+	else
+		wave5_vdi_allocate_sram(dev);
 
 	dev->product_code = wave5_vdi_read_register(dev, VPU_PRODUCT_CODE_REGISTER);
 	ret = wave5_vdi_init(&pdev->dev);
@@ -259,6 +254,8 @@ static int wave5_vpu_probe(struct platform_device *pdev)
 err_clk_dis:
 	clk_bulk_disable_unprepare(dev->num_clks, dev->clks);
 
+	wave5_vdi_free_sram(dev);
+
 	return ret;
 }
 
@@ -275,6 +272,7 @@ static void wave5_vpu_remove(struct platform_device *pdev)
 	v4l2_device_unregister(&dev->v4l2_dev);
 	wave5_vdi_release(&pdev->dev);
 	ida_destroy(&dev->inst_ida);
+	wave5_vdi_free_sram(dev);
 }
 
 static const struct wave5_match_data ti_wave521c_data = {
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
index fa62a85080b5..8d88381ac55e 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
@@ -749,7 +749,6 @@ struct vpu_device {
 	struct vpu_attr attr;
 	struct vpu_buf common_mem;
 	u32 last_performance_cycles;
-	u32 sram_size;
 	struct gen_pool *sram_pool;
 	struct vpu_buf sram_buf;
 	void __iomem *vdb_register;
-- 
2.44.0

[PATCH 6/6] media: chips-media: wave5: support Wave515 decoder

[Ivan Bornyakov]

Add initial support for Wave515 multi-decoder IP. For now it is only
able to decode HEVC Main/Main10 profile videos.

This was tested on FPGA prototype, so wave5_dt_ids[] was not expanded.
Users of the real hardware with Wave515 IP will have to
 * provide firmware specific to their SoC
 * add struct wave5_match_data like this:

	static const struct wave5_match_data platform_name_wave515_data = {
		.flags = WAVE5_IS_DEC,
		.fw_name = "cnm/wave515_platform_name_fw.bin",
	};

 * add item to wave5_dt_ids[] like this:

	{
		.compatible = "vendor,soc-wave515",
		.data = &platform_name_wave515_data
	},

 * describe new compatible in
   Documentation/devicetree/bindings/media/cnm,wave521c.yaml

Signed-off-by: Ivan Bornyakov <brnkv.i1@gmail.com>
---
 .../platform/chips-media/wave5/wave5-hw.c     | 242 ++++++++++++++----
 .../chips-media/wave5/wave5-regdefine.h       |   5 +
 .../platform/chips-media/wave5/wave5-vdi.c    |   6 +-
 .../platform/chips-media/wave5/wave5-vpu.c    |   8 +-
 .../platform/chips-media/wave5/wave5-vpuapi.h |   1 +
 .../chips-media/wave5/wave5-vpuconfig.h       |   9 +-
 .../media/platform/chips-media/wave5/wave5.h  |   1 +
 7 files changed, 215 insertions(+), 57 deletions(-)

diff --git a/drivers/media/platform/chips-media/wave5/wave5-hw.c b/drivers/media/platform/chips-media/wave5/wave5-hw.c
index cdd0a0948a94..6d74a93cc5a2 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-hw.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-hw.c
@@ -26,6 +26,7 @@
 /* Decoder support fields */
 #define FEATURE_AVC_DECODER		BIT(3)
 #define FEATURE_HEVC_DECODER		BIT(2)
+#define W515_FEATURE_HEVC_DECODER	BIT(0)
 
 #define FEATURE_BACKBONE		BIT(16)
 #define FEATURE_VCORE_BACKBONE		BIT(22)
@@ -155,6 +156,8 @@ static int wave5_wait_bus_busy(struct vpu_device *vpu_dev, unsigned int addr)
 {
 	u32 gdi_status_check_value = 0x3f;
 
+	if (vpu_dev->product_code == WAVE515_CODE)
+		gdi_status_check_value = 0x0738;
 	if (vpu_dev->product_code == WAVE521C_CODE ||
 	    vpu_dev->product_code == WAVE521_CODE ||
 	    vpu_dev->product_code == WAVE521E1_CODE)
@@ -186,6 +189,8 @@ unsigned int wave5_vpu_get_product_id(struct vpu_device *vpu_dev)
 	u32 val = vpu_read_reg(vpu_dev, W5_PRODUCT_NUMBER);
 
 	switch (val) {
+	case WAVE515_CODE:
+		return PRODUCT_ID_515;
 	case WAVE521C_CODE:
 		return PRODUCT_ID_521;
 	case WAVE521_CODE:
@@ -349,17 +354,31 @@ static int setup_wave5_properties(struct device *dev)
 	hw_config_def1 = vpu_read_reg(vpu_dev, W5_RET_STD_DEF1);
 	hw_config_feature = vpu_read_reg(vpu_dev, W5_RET_CONF_FEATURE);
 
-	p_attr->support_hevc10bit_enc = FIELD_GET(FEATURE_HEVC10BIT_ENC, hw_config_feature);
-	p_attr->support_avc10bit_enc = FIELD_GET(FEATURE_AVC10BIT_ENC, hw_config_feature);
-
-	p_attr->support_decoders = FIELD_GET(FEATURE_AVC_DECODER, hw_config_def1) << STD_AVC;
-	p_attr->support_decoders |= FIELD_GET(FEATURE_HEVC_DECODER, hw_config_def1) << STD_HEVC;
-	p_attr->support_encoders = FIELD_GET(FEATURE_AVC_ENCODER, hw_config_def1) << STD_AVC;
-	p_attr->support_encoders |= FIELD_GET(FEATURE_HEVC_ENCODER, hw_config_def1) << STD_HEVC;
-
-	p_attr->support_backbone = FIELD_GET(FEATURE_BACKBONE, hw_config_def0);
-	p_attr->support_vcpu_backbone = FIELD_GET(FEATURE_VCPU_BACKBONE, hw_config_def0);
-	p_attr->support_vcore_backbone = FIELD_GET(FEATURE_VCORE_BACKBONE, hw_config_def0);
+	if (vpu_dev->product_code == WAVE515_CODE) {
+		p_attr->support_decoders = FIELD_GET(W515_FEATURE_HEVC_DECODER,
+						     hw_config_def1) << STD_HEVC;
+	} else {
+		p_attr->support_hevc10bit_enc = FIELD_GET(FEATURE_HEVC10BIT_ENC,
+							  hw_config_feature);
+		p_attr->support_avc10bit_enc = FIELD_GET(FEATURE_AVC10BIT_ENC,
+							 hw_config_feature);
+
+		p_attr->support_decoders = FIELD_GET(FEATURE_AVC_DECODER,
+						     hw_config_def1) << STD_AVC;
+		p_attr->support_decoders |= FIELD_GET(FEATURE_HEVC_DECODER,
+						      hw_config_def1) << STD_HEVC;
+		p_attr->support_encoders = FIELD_GET(FEATURE_AVC_ENCODER,
+						     hw_config_def1) << STD_AVC;
+		p_attr->support_encoders |= FIELD_GET(FEATURE_HEVC_ENCODER,
+						      hw_config_def1) << STD_HEVC;
+
+		p_attr->support_backbone = FIELD_GET(FEATURE_BACKBONE,
+						     hw_config_def0);
+		p_attr->support_vcpu_backbone = FIELD_GET(FEATURE_VCPU_BACKBONE,
+							  hw_config_def0);
+		p_attr->support_vcore_backbone = FIELD_GET(FEATURE_VCORE_BACKBONE,
+							   hw_config_def0);
+	}
 
 	setup_wave5_interrupts(vpu_dev);
 
@@ -403,12 +422,18 @@ int wave5_vpu_init(struct device *dev, u8 *fw, size_t size)
 	common_vb = &vpu_dev->common_mem;
 
 	code_base = common_vb->daddr;
+
+	if (vpu_dev->product_code == WAVE515_CODE)
+		code_size = WAVE515_MAX_CODE_BUF_SIZE;
+	else
+		code_size = WAVE5_MAX_CODE_BUF_SIZE;
+
 	/* ALIGN TO 4KB */
-	code_size = (WAVE5_MAX_CODE_BUF_SIZE & ~0xfff);
+	code_size &= ~0xfff;
 	if (code_size < size * 2)
 		return -EINVAL;
 
-	temp_base = common_vb->daddr + WAVE5_TEMPBUF_OFFSET;
+	temp_base = code_base + code_size;
 	temp_size = WAVE5_TEMPBUF_SIZE;
 
 	ret = wave5_vdi_write_memory(vpu_dev, common_vb, 0, fw, size);
@@ -436,9 +461,12 @@ int wave5_vpu_init(struct device *dev, u8 *fw, size_t size)
 
 	/* These register must be reset explicitly */
 	vpu_write_reg(vpu_dev, W5_HW_OPTION, 0);
-	wave5_fio_writel(vpu_dev, W5_BACKBONE_PROC_EXT_ADDR, 0);
-	wave5_fio_writel(vpu_dev, W5_BACKBONE_AXI_PARAM, 0);
-	vpu_write_reg(vpu_dev, W5_SEC_AXI_PARAM, 0);
+
+	if (vpu_dev->product_code != WAVE515_CODE) {
+		wave5_fio_writel(vpu_dev, W5_BACKBONE_PROC_EXT_ADDR, 0);
+		wave5_fio_writel(vpu_dev, W5_BACKBONE_AXI_PARAM, 0);
+		vpu_write_reg(vpu_dev, W5_SEC_AXI_PARAM, 0);
+	}
 
 	reg_val = vpu_read_reg(vpu_dev, W5_VPU_RET_VPU_CONFIG0);
 	if (FIELD_GET(FEATURE_BACKBONE, reg_val)) {
@@ -453,6 +481,24 @@ int wave5_vpu_init(struct device *dev, u8 *fw, size_t size)
 		wave5_fio_writel(vpu_dev, W5_BACKBONE_PROG_AXI_ID, reg_val);
 	}
 
+	if (vpu_dev->product_code == WAVE515_CODE) {
+		dma_addr_t task_buf_base;
+
+		vpu_write_reg(vpu_dev, W5_CMD_INIT_NUM_TASK_BUF, WAVE515_COMMAND_QUEUE_DEPTH);
+		vpu_write_reg(vpu_dev, W5_CMD_INIT_TASK_BUF_SIZE, WAVE515_ONE_TASKBUF_SIZE);
+
+		for (i = 0; i < WAVE515_COMMAND_QUEUE_DEPTH; i++) {
+			task_buf_base = temp_base + temp_size +
+					(i * WAVE515_ONE_TASKBUF_SIZE);
+			vpu_write_reg(vpu_dev,
+				      W5_CMD_INIT_ADDR_TASK_BUF0 + (i * 4),
+				      task_buf_base);
+		}
+
+		vpu_write_reg(vpu_dev, W515_CMD_ADDR_SEC_AXI, vpu_dev->sram_buf.daddr);
+		vpu_write_reg(vpu_dev, W515_CMD_SEC_AXI_SIZE, vpu_dev->sram_buf.size);
+	}
+
 	vpu_write_reg(vpu_dev, W5_VPU_BUSY_STATUS, 1);
 	vpu_write_reg(vpu_dev, W5_COMMAND, W5_INIT_VPU);
 	vpu_write_reg(vpu_dev, W5_VPU_REMAP_CORE_START, 1);
@@ -493,29 +539,39 @@ int wave5_vpu_build_up_dec_param(struct vpu_instance *inst,
 		return -EINVAL;
 	}
 
-	p_dec_info->vb_work.size = WAVE521DEC_WORKBUF_SIZE;
+	if (vpu_dev->product == PRODUCT_ID_515)
+		p_dec_info->vb_work.size = WAVE515DEC_WORKBUF_SIZE;
+	else
+		p_dec_info->vb_work.size = WAVE521DEC_WORKBUF_SIZE;
+
 	ret = wave5_vdi_allocate_dma_memory(inst->dev, &p_dec_info->vb_work);
 	if (ret)
 		return ret;
 
-	vpu_write_reg(inst->dev, W5_CMD_DEC_VCORE_INFO, 1);
+	if (inst->dev->product_code != WAVE515_CODE)
+		vpu_write_reg(inst->dev, W5_CMD_DEC_VCORE_INFO, 1);
 
 	wave5_vdi_clear_memory(inst->dev, &p_dec_info->vb_work);
 
 	vpu_write_reg(inst->dev, W5_ADDR_WORK_BASE, p_dec_info->vb_work.daddr);
 	vpu_write_reg(inst->dev, W5_WORK_SIZE, p_dec_info->vb_work.size);
 
-	vpu_write_reg(inst->dev, W5_CMD_ADDR_SEC_AXI, vpu_dev->sram_buf.daddr);
-	vpu_write_reg(inst->dev, W5_CMD_SEC_AXI_SIZE, vpu_dev->sram_buf.size);
+	if (inst->dev->product_code != WAVE515_CODE) {
+		vpu_write_reg(inst->dev, W5_CMD_ADDR_SEC_AXI, vpu_dev->sram_buf.daddr);
+		vpu_write_reg(inst->dev, W5_CMD_SEC_AXI_SIZE, vpu_dev->sram_buf.size);
+	}
 
 	vpu_write_reg(inst->dev, W5_CMD_DEC_BS_START_ADDR, p_dec_info->stream_buf_start_addr);
 	vpu_write_reg(inst->dev, W5_CMD_DEC_BS_SIZE, p_dec_info->stream_buf_size);
 
 	/* NOTE: SDMA reads MSB first */
 	vpu_write_reg(inst->dev, W5_CMD_BS_PARAM, BITSTREAM_ENDIANNESS_BIG_ENDIAN);
-	/* This register must be reset explicitly */
-	vpu_write_reg(inst->dev, W5_CMD_EXT_ADDR, 0);
-	vpu_write_reg(inst->dev, W5_CMD_NUM_CQ_DEPTH_M1, (COMMAND_QUEUE_DEPTH - 1));
+
+	if (inst->dev->product_code != WAVE515_CODE) {
+		/* This register must be reset explicitly */
+		vpu_write_reg(inst->dev, W5_CMD_EXT_ADDR, 0);
+		vpu_write_reg(inst->dev, W5_CMD_NUM_CQ_DEPTH_M1, (COMMAND_QUEUE_DEPTH - 1));
+	}
 
 	ret = send_firmware_command(inst, W5_CREATE_INSTANCE, true, NULL, NULL);
 	if (ret) {
@@ -566,7 +622,7 @@ static u32 get_bitstream_options(struct dec_info *info)
 int wave5_vpu_dec_init_seq(struct vpu_instance *inst)
 {
 	struct dec_info *p_dec_info = &inst->codec_info->dec_info;
-	u32 cmd_option = INIT_SEQ_NORMAL;
+	u32 bs_option, cmd_option = INIT_SEQ_NORMAL;
 	u32 reg_val, fail_res;
 	int ret;
 
@@ -576,7 +632,12 @@ int wave5_vpu_dec_init_seq(struct vpu_instance *inst)
 	vpu_write_reg(inst->dev, W5_BS_RD_PTR, p_dec_info->stream_rd_ptr);
 	vpu_write_reg(inst->dev, W5_BS_WR_PTR, p_dec_info->stream_wr_ptr);
 
-	vpu_write_reg(inst->dev, W5_BS_OPTION, get_bitstream_options(p_dec_info));
+	bs_option = get_bitstream_options(p_dec_info);
+
+	if (inst->dev->product_code == WAVE515_CODE)
+		bs_option |= BSOPTION_RD_PTR_VALID_FLAG;
+
+	vpu_write_reg(inst->dev, W5_BS_OPTION, bs_option);
 
 	vpu_write_reg(inst->dev, W5_COMMAND_OPTION, cmd_option);
 	vpu_write_reg(inst->dev, W5_CMD_DEC_USER_MASK, p_dec_info->user_data_enable);
@@ -642,10 +703,12 @@ static void wave5_get_dec_seq_result(struct vpu_instance *inst, struct dec_initi
 		info->profile = FIELD_GET(SEQ_PARAM_PROFILE_MASK, reg_val);
 	}
 
-	info->vlc_buf_size = vpu_read_reg(inst->dev, W5_RET_VLC_BUF_SIZE);
-	info->param_buf_size = vpu_read_reg(inst->dev, W5_RET_PARAM_BUF_SIZE);
-	p_dec_info->vlc_buf_size = info->vlc_buf_size;
-	p_dec_info->param_buf_size = info->param_buf_size;
+	if (inst->dev->product_code != WAVE515_CODE) {
+		info->vlc_buf_size = vpu_read_reg(inst->dev, W5_RET_VLC_BUF_SIZE);
+		info->param_buf_size = vpu_read_reg(inst->dev, W5_RET_PARAM_BUF_SIZE);
+		p_dec_info->vlc_buf_size = info->vlc_buf_size;
+		p_dec_info->param_buf_size = info->param_buf_size;
+	}
 }
 
 int wave5_vpu_dec_get_seq_info(struct vpu_instance *inst, struct dec_initial_info *info)
@@ -747,22 +810,27 @@ int wave5_vpu_dec_register_framebuffer(struct vpu_instance *inst, struct frame_b
 
 		pic_size = (init_info->pic_width << 16) | (init_info->pic_height);
 
-		vb_buf.size = (p_dec_info->vlc_buf_size * VLC_BUF_NUM) +
+		if (inst->dev->product_code != WAVE515_CODE) {
+			vb_buf.size = (p_dec_info->vlc_buf_size * VLC_BUF_NUM) +
 				(p_dec_info->param_buf_size * COMMAND_QUEUE_DEPTH);
-		vb_buf.daddr = 0;
+			vb_buf.daddr = 0;
 
-		if (vb_buf.size != p_dec_info->vb_task.size) {
-			wave5_vdi_free_dma_memory(inst->dev, &p_dec_info->vb_task);
-			ret = wave5_vdi_allocate_dma_memory(inst->dev, &vb_buf);
-			if (ret)
-				goto free_fbc_c_tbl_buffers;
+			if (vb_buf.size != p_dec_info->vb_task.size) {
+				wave5_vdi_free_dma_memory(inst->dev,
+							  &p_dec_info->vb_task);
+				ret = wave5_vdi_allocate_dma_memory(inst->dev,
+								    &vb_buf);
+				if (ret)
+					goto free_fbc_c_tbl_buffers;
 
-			p_dec_info->vb_task = vb_buf;
-		}
+				p_dec_info->vb_task = vb_buf;
+			}
 
-		vpu_write_reg(inst->dev, W5_CMD_SET_FB_ADDR_TASK_BUF,
-			      p_dec_info->vb_task.daddr);
-		vpu_write_reg(inst->dev, W5_CMD_SET_FB_TASK_BUF_SIZE, vb_buf.size);
+			vpu_write_reg(inst->dev, W5_CMD_SET_FB_ADDR_TASK_BUF,
+				      p_dec_info->vb_task.daddr);
+			vpu_write_reg(inst->dev, W5_CMD_SET_FB_TASK_BUF_SIZE,
+				      vb_buf.size);
+		}
 	} else {
 		pic_size = (init_info->pic_width << 16) | (init_info->pic_height);
 
@@ -999,17 +1067,24 @@ int wave5_vpu_re_init(struct device *dev, u8 *fw, size_t size)
 	dma_addr_t code_base, temp_base;
 	dma_addr_t old_code_base, temp_size;
 	u32 code_size, reason_code;
-	u32 reg_val;
+	u32 i, reg_val;
 	struct vpu_device *vpu_dev = dev_get_drvdata(dev);
 
 	common_vb = &vpu_dev->common_mem;
 
 	code_base = common_vb->daddr;
+
+	if (vpu_dev->product_code == WAVE515_CODE)
+		code_size = WAVE515_MAX_CODE_BUF_SIZE;
+	else
+		code_size = WAVE5_MAX_CODE_BUF_SIZE;
+
 	/* ALIGN TO 4KB */
-	code_size = (WAVE5_MAX_CODE_BUF_SIZE & ~0xfff);
+	code_size &= ~0xfff;
 	if (code_size < size * 2)
 		return -EINVAL;
-	temp_base = common_vb->daddr + WAVE5_TEMPBUF_OFFSET;
+
+	temp_base = code_base + code_size;
 	temp_size = WAVE5_TEMPBUF_SIZE;
 
 	old_code_base = vpu_read_reg(vpu_dev, W5_VPU_REMAP_PADDR);
@@ -1043,9 +1118,12 @@ int wave5_vpu_re_init(struct device *dev, u8 *fw, size_t size)
 
 		/* These register must be reset explicitly */
 		vpu_write_reg(vpu_dev, W5_HW_OPTION, 0);
-		wave5_fio_writel(vpu_dev, W5_BACKBONE_PROC_EXT_ADDR, 0);
-		wave5_fio_writel(vpu_dev, W5_BACKBONE_AXI_PARAM, 0);
-		vpu_write_reg(vpu_dev, W5_SEC_AXI_PARAM, 0);
+
+		if (vpu_dev->product_code != WAVE515_CODE) {
+			wave5_fio_writel(vpu_dev, W5_BACKBONE_PROC_EXT_ADDR, 0);
+			wave5_fio_writel(vpu_dev, W5_BACKBONE_AXI_PARAM, 0);
+			vpu_write_reg(vpu_dev, W5_SEC_AXI_PARAM, 0);
+		}
 
 		reg_val = vpu_read_reg(vpu_dev, W5_VPU_RET_VPU_CONFIG0);
 		if (FIELD_GET(FEATURE_BACKBONE, reg_val)) {
@@ -1060,6 +1138,28 @@ int wave5_vpu_re_init(struct device *dev, u8 *fw, size_t size)
 			wave5_fio_writel(vpu_dev, W5_BACKBONE_PROG_AXI_ID, reg_val);
 		}
 
+		if (vpu_dev->product_code == WAVE515_CODE) {
+			dma_addr_t task_buf_base;
+
+			vpu_write_reg(vpu_dev, W5_CMD_INIT_NUM_TASK_BUF,
+				      WAVE515_COMMAND_QUEUE_DEPTH);
+			vpu_write_reg(vpu_dev, W5_CMD_INIT_TASK_BUF_SIZE,
+				      WAVE515_ONE_TASKBUF_SIZE);
+
+			for (i = 0; i < WAVE515_COMMAND_QUEUE_DEPTH; i++) {
+				task_buf_base = temp_base + temp_size +
+						(i * WAVE515_ONE_TASKBUF_SIZE);
+				vpu_write_reg(vpu_dev,
+					      W5_CMD_INIT_ADDR_TASK_BUF0 + (i * 4),
+					      task_buf_base);
+			}
+
+			vpu_write_reg(vpu_dev, W515_CMD_ADDR_SEC_AXI,
+				      vpu_dev->sram_buf.daddr);
+			vpu_write_reg(vpu_dev, W515_CMD_SEC_AXI_SIZE,
+				      vpu_dev->sram_buf.size);
+		}
+
 		vpu_write_reg(vpu_dev, W5_VPU_BUSY_STATUS, 1);
 		vpu_write_reg(vpu_dev, W5_COMMAND, W5_INIT_VPU);
 		vpu_write_reg(vpu_dev, W5_VPU_REMAP_CORE_START, 1);
@@ -1081,10 +1181,10 @@ int wave5_vpu_re_init(struct device *dev, u8 *fw, size_t size)
 static int wave5_vpu_sleep_wake(struct device *dev, bool i_sleep_wake, const uint16_t *code,
 				size_t size)
 {
-	u32 reg_val;
+	u32 i, reg_val;
 	struct vpu_buf *common_vb;
-	dma_addr_t code_base;
-	u32 code_size, reason_code;
+	dma_addr_t code_base, temp_base;
+	u32 code_size, temp_size, reason_code;
 	struct vpu_device *vpu_dev = dev_get_drvdata(dev);
 	int ret;
 
@@ -1114,13 +1214,22 @@ static int wave5_vpu_sleep_wake(struct device *dev, bool i_sleep_wake, const uin
 		common_vb = &vpu_dev->common_mem;
 
 		code_base = common_vb->daddr;
+
+		if (vpu_dev->product_code == WAVE515_CODE)
+			code_size = WAVE515_MAX_CODE_BUF_SIZE;
+		else
+			code_size = WAVE5_MAX_CODE_BUF_SIZE;
+
 		/* ALIGN TO 4KB */
-		code_size = (WAVE5_MAX_CODE_BUF_SIZE & ~0xfff);
+		code_size &= ~0xfff;
 		if (code_size < size * 2) {
 			dev_err(dev, "size too small\n");
 			return -EINVAL;
 		}
 
+		temp_base = code_base + code_size;
+		temp_size = WAVE5_TEMPBUF_SIZE;
+
 		/* Power on without DEBUG mode */
 		vpu_write_reg(vpu_dev, W5_PO_CONF, 0);
 
@@ -1133,9 +1242,12 @@ static int wave5_vpu_sleep_wake(struct device *dev, bool i_sleep_wake, const uin
 
 		/* These register must be reset explicitly */
 		vpu_write_reg(vpu_dev, W5_HW_OPTION, 0);
-		wave5_fio_writel(vpu_dev, W5_BACKBONE_PROC_EXT_ADDR, 0);
-		wave5_fio_writel(vpu_dev, W5_BACKBONE_AXI_PARAM, 0);
-		vpu_write_reg(vpu_dev, W5_SEC_AXI_PARAM, 0);
+
+		if (vpu_dev->product_code != WAVE515_CODE) {
+			wave5_fio_writel(vpu_dev, W5_BACKBONE_PROC_EXT_ADDR, 0);
+			wave5_fio_writel(vpu_dev, W5_BACKBONE_AXI_PARAM, 0);
+			vpu_write_reg(vpu_dev, W5_SEC_AXI_PARAM, 0);
+		}
 
 		setup_wave5_interrupts(vpu_dev);
 
@@ -1152,6 +1264,28 @@ static int wave5_vpu_sleep_wake(struct device *dev, bool i_sleep_wake, const uin
 			wave5_fio_writel(vpu_dev, W5_BACKBONE_PROG_AXI_ID, reg_val);
 		}
 
+		if (vpu_dev->product_code == WAVE515_CODE) {
+			dma_addr_t task_buf_base;
+
+			vpu_write_reg(vpu_dev, W5_CMD_INIT_NUM_TASK_BUF,
+				      WAVE515_COMMAND_QUEUE_DEPTH);
+			vpu_write_reg(vpu_dev, W5_CMD_INIT_TASK_BUF_SIZE,
+				      WAVE515_ONE_TASKBUF_SIZE);
+
+			for (i = 0; i < WAVE515_COMMAND_QUEUE_DEPTH; i++) {
+				task_buf_base = temp_base + temp_size +
+						(i * WAVE515_ONE_TASKBUF_SIZE);
+				vpu_write_reg(vpu_dev,
+					      W5_CMD_INIT_ADDR_TASK_BUF0 + (i * 4),
+					      task_buf_base);
+			}
+
+			vpu_write_reg(vpu_dev, W515_CMD_ADDR_SEC_AXI,
+				      vpu_dev->sram_buf.daddr);
+			vpu_write_reg(vpu_dev, W515_CMD_SEC_AXI_SIZE,
+				      vpu_dev->sram_buf.size);
+		}
+
 		vpu_write_reg(vpu_dev, W5_VPU_BUSY_STATUS, 1);
 		vpu_write_reg(vpu_dev, W5_COMMAND, W5_WAKEUP_VPU);
 		/* Start VPU after settings */
diff --git a/drivers/media/platform/chips-media/wave5/wave5-regdefine.h b/drivers/media/platform/chips-media/wave5/wave5-regdefine.h
index a15c6b2c3d8b..557344754c4c 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-regdefine.h
+++ b/drivers/media/platform/chips-media/wave5/wave5-regdefine.h
@@ -205,6 +205,9 @@ enum query_opt {
 #define W5_ADDR_TEMP_BASE                       (W5_REG_BASE + 0x011C)
 #define W5_TEMP_SIZE                            (W5_REG_BASE + 0x0120)
 #define W5_HW_OPTION                            (W5_REG_BASE + 0x012C)
+#define W5_CMD_INIT_NUM_TASK_BUF		(W5_REG_BASE + 0x0134)
+#define W5_CMD_INIT_ADDR_TASK_BUF0		(W5_REG_BASE + 0x0138)
+#define W5_CMD_INIT_TASK_BUF_SIZE		(W5_REG_BASE + 0x0178)
 #define W5_SEC_AXI_PARAM                        (W5_REG_BASE + 0x0180)
 
 /************************************************************************/
@@ -216,7 +219,9 @@ enum query_opt {
 #define W5_CMD_DEC_BS_SIZE                      (W5_REG_BASE + 0x0120)
 #define W5_CMD_BS_PARAM                         (W5_REG_BASE + 0x0124)
 #define W5_CMD_ADDR_SEC_AXI                     (W5_REG_BASE + 0x0130)
+#define W515_CMD_ADDR_SEC_AXI			(W5_REG_BASE + 0x0124)
 #define W5_CMD_SEC_AXI_SIZE                     (W5_REG_BASE + 0x0134)
+#define W515_CMD_SEC_AXI_SIZE			(W5_REG_BASE + 0x0128)
 #define W5_CMD_EXT_ADDR                         (W5_REG_BASE + 0x0138)
 #define W5_CMD_NUM_CQ_DEPTH_M1                  (W5_REG_BASE + 0x013C)
 #define W5_CMD_ERR_CONCEAL                      (W5_REG_BASE + 0x0140)
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vdi.c b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
index ee671f5a2f37..1b1baf5caed9 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
@@ -18,7 +18,11 @@ static int wave5_vdi_allocate_common_memory(struct device *dev)
 	if (!vpu_dev->common_mem.vaddr) {
 		int ret;
 
-		vpu_dev->common_mem.size = SIZE_COMMON;
+		if (vpu_dev->product_code == WAVE515_CODE)
+			vpu_dev->common_mem.size = WAVE515_SIZE_COMMON;
+		else
+			vpu_dev->common_mem.size = SIZE_COMMON;
+
 		ret = wave5_vdi_allocate_dma_memory(vpu_dev, &vpu_dev->common_mem);
 		if (ret) {
 			dev_err(dev, "unable to allocate common buffer\n");
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.c b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
index 2a0a70dd7062..99deaabc0bcc 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
@@ -60,7 +60,13 @@ static irqreturn_t wave5_vpu_irq_thread(int irq, void *dev_id)
 
 			if (irq_reason & BIT(INT_WAVE5_INIT_SEQ) ||
 			    irq_reason & BIT(INT_WAVE5_ENC_SET_PARAM)) {
-				if (seq_done & BIT(inst->id)) {
+				if ((dev->product_code == WAVE515_CODE) &&
+				    (cmd_done & BIT(inst->id))) {
+					cmd_done &= ~BIT(inst->id);
+					wave5_vdi_write_register(dev, W5_RET_QUEUE_CMD_DONE_INST,
+								 cmd_done);
+					complete(&inst->irq_done);
+				} else if (seq_done & BIT(inst->id)) {
 					seq_done &= ~BIT(inst->id);
 					wave5_vdi_write_register(dev, W5_RET_SEQ_DONE_INSTANCE_INFO,
 								 seq_done);
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
index 8d88381ac55e..d0e135c65bf6 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
@@ -18,6 +18,7 @@
 #include "wave5-vdi.h"
 
 enum product_id {
+	PRODUCT_ID_515,
 	PRODUCT_ID_521,
 	PRODUCT_ID_511,
 	PRODUCT_ID_517,
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuconfig.h b/drivers/media/platform/chips-media/wave5/wave5-vpuconfig.h
index d9751eedb0f9..b435630633b5 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpuconfig.h
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuconfig.h
@@ -8,6 +8,7 @@
 #ifndef _VPU_CONFIG_H_
 #define _VPU_CONFIG_H_
 
+#define WAVE515_CODE			0x5150
 #define WAVE517_CODE                    0x5170
 #define WAVE537_CODE                    0x5370
 #define WAVE511_CODE                    0x5110
@@ -21,12 +22,13 @@
 		((c) == WAVE517_CODE ||	(c) == WAVE537_CODE ||		\
 		 (c) == WAVE511_CODE || (c) == WAVE521_CODE ||		\
 		 (c) == WAVE521E1_CODE || (c) == WAVE521C_CODE ||	\
-		 (c) == WAVE521C_DUAL_CODE);				\
+		 (c) == WAVE521C_DUAL_CODE) || (c) == WAVE515_CODE;	\
 })
 
 #define WAVE517_WORKBUF_SIZE            (2 * 1024 * 1024)
 #define WAVE521ENC_WORKBUF_SIZE         (128 * 1024)      //HEVC 128K, AVC 40K
 #define WAVE521DEC_WORKBUF_SIZE         (1784 * 1024)
+#define WAVE515DEC_WORKBUF_SIZE		(2 * 1024 * 1024)
 
 #define MAX_NUM_INSTANCE                32
 
@@ -50,16 +52,21 @@
 #define VLC_BUF_NUM                     (2)
 
 #define COMMAND_QUEUE_DEPTH             (2)
+#define WAVE515_COMMAND_QUEUE_DEPTH	(4)
 
 #define W5_REMAP_INDEX0                 0
 #define W5_REMAP_INDEX1                 1
 #define W5_REMAP_MAX_SIZE               (1024 * 1024)
 
 #define WAVE5_MAX_CODE_BUF_SIZE         (2 * 1024 * 1024)
+#define WAVE515_MAX_CODE_BUF_SIZE	(1024 * 1024)
 #define WAVE5_TEMPBUF_OFFSET            WAVE5_MAX_CODE_BUF_SIZE
 #define WAVE5_TEMPBUF_SIZE              (1024 * 1024)
+#define WAVE515_TASKBUF_OFFSET		(WAVE515_MAX_CODE_BUF_SIZE + WAVE5_TEMPBUF_SIZE)
 
 #define SIZE_COMMON                 (WAVE5_MAX_CODE_BUF_SIZE + WAVE5_TEMPBUF_SIZE)
+#define WAVE515_ONE_TASKBUF_SIZE	(8 * 1024 * 1024)
+#define WAVE515_SIZE_COMMON	(WAVE515_TASKBUF_OFFSET + WAVE515_COMMAND_QUEUE_DEPTH * WAVE515_ONE_TASKBUF_SIZE)
 
 //=====4. VPU REPORT MEMORY  ======================//
 
diff --git a/drivers/media/platform/chips-media/wave5/wave5.h b/drivers/media/platform/chips-media/wave5/wave5.h
index 063028eccd3b..57b00e182b6e 100644
--- a/drivers/media/platform/chips-media/wave5/wave5.h
+++ b/drivers/media/platform/chips-media/wave5/wave5.h
@@ -22,6 +22,7 @@
  */
 #define BSOPTION_ENABLE_EXPLICIT_END		BIT(0)
 #define BSOPTION_HIGHLIGHT_STREAM_END		BIT(1)
+#define BSOPTION_RD_PTR_VALID_FLAG		BIT(31)
 
 /*
  * Currently the driver only supports hardware with little endian but for source
-- 
2.44.0

Re: [PATCH 2/6] media: chips-media: wave5: support reset lines

[Philipp Zabel]

Hi Ivan,

On Mo, 2024-03-18 at 17:42 +0300, Ivan Bornyakov wrote:
> Add initial support for optional reset lines. For now, simply deassert
> resets on driver probe and assert them back on driver remove.
> 
> Signed-off-by: Ivan Bornyakov <brnkv.i1@gmail.com>
> ---
>  .../media/platform/chips-media/wave5/wave5-vpu.c    | 13 +++++++++++++
>  .../media/platform/chips-media/wave5/wave5-vpuapi.h |  1 +
>  2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.c b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> index 1b3df5b04249..f3ecadefd37a 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> @@ -10,6 +10,7 @@
>  #include <linux/clk.h>
>  #include <linux/firmware.h>
>  #include <linux/interrupt.h>
> +#include <linux/reset.h>
>  #include "wave5-vpu.h"
>  #include "wave5-regdefine.h"
>  #include "wave5-vpuconfig.h"
> @@ -151,6 +152,17 @@ static int wave5_vpu_probe(struct platform_device *pdev)
>  	dev_set_drvdata(&pdev->dev, dev);
>  	dev->dev = &pdev->dev;
>  
> +	dev->resets = devm_reset_control_array_get_optional_exclusive(&pdev->dev);
> +	if (IS_ERR(dev->resets)) {
> +		return dev_err_probe(&pdev->dev, PTR_ERR(dev->resets),
> +				     "Failed to get reset control\n");
> +	} else {
> +		ret = reset_control_deassert(dev->resets);
> +		if (ret)
> +			return dev_err_probe(&pdev->dev, ret,
> +					     "Failed to deassert resets\n");

This doesn't have to be in an else block, the error path above returns.

> +	}
> +
>  	ret = devm_clk_bulk_get_all(&pdev->dev, &dev->clks);

I'd expect the reset control to be asserted in the error path.


regards
Philipp

Re: [PATCH 3/6] dt-bindings: media: cnm,wave521c: drop resets restriction

[Krzysztof Kozlowski]

On 18/03/2024 15:42, Ivan Bornyakov wrote:
> Different designs may have different amount of routed reset signals.

You changed nothing. This commit msg does not match code at all. I don't
understand this.

And your subject is entirely wrong - again, you did not drop any
restriction.

But in general: we expect restrictions (constraints).

> Drop maxItems restriction, add a small description instead.


Best regards,
Krzysztof

RE: [PATCH 1/6] media: chips-media: wave5: support decoding higher bit-depth profiles

[Nas Chung]

Hi, Ivan.

>-----Original Message-----
>From: Ivan Bornyakov <brnkv.i1@gmail.com>
>Sent: Monday, March 18, 2024 11:42 PM
>To: Nas Chung <nas.chung@chipsnmedia.com>; jackson.lee
><jackson.lee@chipsnmedia.com>; Mauro Carvalho Chehab <mchehab@kernel.org>
>Cc: Ivan Bornyakov <brnkv.i1@gmail.com>; Philipp Zabel
><p.zabel@pengutronix.de>; Rob Herring <robh@kernel.org>; Krzysztof
>Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
><conor+dt@kernel.org>; linux-media@vger.kernel.org; linux-
>kernel@vger.kernel.org; devicetree@vger.kernel.org
>Subject: [PATCH 1/6] media: chips-media: wave5: support decoding higher
>bit-depth profiles
>
>Add support for decoding higher than 8 bit-depth profiles by scaling FBC
>buffer stride and size by the factor of (bitdepth / 8).
>
>Signed-off-by: Ivan Bornyakov <brnkv.i1@gmail.com>
>---
> .../platform/chips-media/wave5/wave5-vpu-dec.c    | 15 ++-------------
> 1 file changed, 2 insertions(+), 13 deletions(-)
>
>diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
>b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
>index ef227af72348..aa0401f35d32 100644
>--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
>+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
>@@ -1055,6 +1055,7 @@ static int wave5_prepare_fb(struct vpu_instance
>*inst)
> 	int ret, i;
> 	struct v4l2_m2m_buffer *buf, *n;
> 	struct v4l2_m2m_ctx *m2m_ctx = inst->v4l2_fh.m2m_ctx;
>+	u32 bitdepth = inst->codec_info-
>>dec_info.initial_info.luma_bitdepth;
>
> 	linear_num = v4l2_m2m_num_dst_bufs_ready(m2m_ctx);
> 	non_linear_num = inst->fbc_buf_count;
>@@ -1063,7 +1064,7 @@ static int wave5_prepare_fb(struct vpu_instance
>*inst)
> 		struct frame_buffer *frame = &inst->frame_buf[i];
> 		struct vpu_buf *vframe = &inst->frame_vbuf[i];
>
>-		fb_stride = inst->dst_fmt.width;
>+		fb_stride = ALIGN(inst->dst_fmt.width * bitdepth / 8, 32);
> 		fb_height = ALIGN(inst->dst_fmt.height, 32);
> 		luma_size = fb_stride * fb_height;
>
>@@ -1408,22 +1409,10 @@ static int wave5_vpu_dec_start_streaming(struct
>vb2_queue *q, unsigned int count
> 		if (ret)
> 			goto free_bitstream_vbuf;
> 	} else if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
>-		struct dec_initial_info *initial_info =
>-			&inst->codec_info->dec_info.initial_info;
>-
> 		if (inst->state == VPU_INST_STATE_STOP)
> 			ret = switch_state(inst, VPU_INST_STATE_INIT_SEQ);
> 		if (ret)
> 			goto return_buffers;
>-
>-		if (inst->state == VPU_INST_STATE_INIT_SEQ) {
>-			if (initial_info->luma_bitdepth != 8) {
>-				dev_info(inst->dev->dev, "%s: no support for %d
>bit depth",
>-					 __func__, initial_info->luma_bitdepth);
>-				ret = -EINVAL;
>-				goto return_buffers;
>-			}
>-		}
> 	}
>

TI wave521C version cannot support 10bit decoding.
So, We need above error checking codes.
How about adding support_10bit flag in wave5_match_data like below ?
static const struct wave5_match_data ti_wave521c_data = {
	.support_10bit = false;
};

> 	return ret;
>--
>2.44.0

RE: [PATCH 5/6] media: chips-media: wave5: refine SRAM usage

[Nas Chung]

Hi, Ivan.

>-----Original Message-----
>From: Ivan Bornyakov <brnkv.i1@gmail.com>
>Sent: Monday, March 18, 2024 11:42 PM
>To: Nas Chung <nas.chung@chipsnmedia.com>; jackson.lee
><jackson.lee@chipsnmedia.com>; Mauro Carvalho Chehab <mchehab@kernel.org>
>Cc: Ivan Bornyakov <brnkv.i1@gmail.com>; Philipp Zabel
><p.zabel@pengutronix.de>; Rob Herring <robh@kernel.org>; Krzysztof
>Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
><conor+dt@kernel.org>; linux-media@vger.kernel.org; linux-
>kernel@vger.kernel.org; devicetree@vger.kernel.org
>Subject: [PATCH 5/6] media: chips-media: wave5: refine SRAM usage
>
>Allocate SRAM memory on module probe, free on remove. There is no need
>to allocate on device open, free on close, the memory is the same every
>time.

If there is no decoder/encoder instance, driver don't need to allocate SRAM memory.
The main reason of allocating the memory in open() is to allow other modules to
use more SRAM memory, if wave5 is not working.

>
>Also use gen_pool_size() to determine SRAM memory size to be allocated
>instead of separate "sram-size" DT property to reduce duplication.
>
>Signed-off-by: Ivan Bornyakov <brnkv.i1@gmail.com>
>---
> .../platform/chips-media/wave5/wave5-helper.c |  3 ---
> .../platform/chips-media/wave5/wave5-vdi.c    | 21 ++++++++++---------
> .../chips-media/wave5/wave5-vpu-dec.c         |  2 --
> .../chips-media/wave5/wave5-vpu-enc.c         |  2 --
> .../platform/chips-media/wave5/wave5-vpu.c    | 12 +++++------
> .../platform/chips-media/wave5/wave5-vpuapi.h |  1 -
> 6 files changed, 16 insertions(+), 25 deletions(-)
>
>diff --git a/drivers/media/platform/chips-media/wave5/wave5-helper.c
>b/drivers/media/platform/chips-media/wave5/wave5-helper.c
>index 8433ecab230c..ec710b838dfe 100644
>--- a/drivers/media/platform/chips-media/wave5/wave5-helper.c
>+++ b/drivers/media/platform/chips-media/wave5/wave5-helper.c
>@@ -29,9 +29,6 @@ void wave5_cleanup_instance(struct vpu_instance *inst)
> {
> 	int i;
>
>-	if (list_is_singular(&inst->list))
>-		wave5_vdi_free_sram(inst->dev);
>-
> 	for (i = 0; i < inst->fbc_buf_count; i++)
> 		wave5_vpu_dec_reset_framebuffer(inst, i);
>
>diff --git a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
>b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
>index 3809f70bc0b4..ee671f5a2f37 100644
>--- a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
>+++ b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
>@@ -174,16 +174,19 @@ int wave5_vdi_allocate_array(struct vpu_device
>*vpu_dev, struct vpu_buf *array,
> void wave5_vdi_allocate_sram(struct vpu_device *vpu_dev)
> {
> 	struct vpu_buf *vb = &vpu_dev->sram_buf;
>+	dma_addr_t daddr;
>+	void *vaddr;
>+	size_t size;
>
>-	if (!vpu_dev->sram_pool || !vpu_dev->sram_size)
>+	if (!vpu_dev->sram_pool || vb->vaddr)
> 		return;
>
>-	if (!vb->vaddr) {
>-		vb->size = vpu_dev->sram_size;
>-		vb->vaddr = gen_pool_dma_alloc(vpu_dev->sram_pool, vb->size,
>-					       &vb->daddr);
>-		if (!vb->vaddr)
>-			vb->size = 0;
>+	size = gen_pool_size(vpu_dev->sram_pool);
>+	vaddr = gen_pool_dma_alloc(vpu_dev->sram_pool, size, &daddr);
>+	if (vaddr) {
>+		vb->vaddr = vaddr;
>+		vb->daddr = daddr;
>+		vb->size = size;
> 	}
>
> 	dev_dbg(vpu_dev->dev, "%s: sram daddr: %pad, size: %zu, vaddr:
>0x%p\n",
>@@ -197,9 +200,7 @@ void wave5_vdi_free_sram(struct vpu_device *vpu_dev)
> 	if (!vb->size || !vb->vaddr)
> 		return;
>
>-	if (vb->vaddr)
>-		gen_pool_free(vpu_dev->sram_pool, (unsigned long)vb->vaddr,
>-			      vb->size);
>+	gen_pool_free(vpu_dev->sram_pool, (unsigned long)vb->vaddr, vb-
>>size);
>
> 	memset(vb, 0, sizeof(*vb));
> }
>diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
>b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
>index aa0401f35d32..84dbe56216ad 100644
>--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
>+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
>@@ -1854,8 +1854,6 @@ static int wave5_vpu_open_dec(struct file *filp)
> 		goto cleanup_inst;
> 	}
>
>-	wave5_vdi_allocate_sram(inst->dev);
>-
> 	return 0;
>
> cleanup_inst:
>diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
>b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
>index 8bbf9d10b467..86ddcb82443b 100644
>--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
>+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
>@@ -1727,8 +1727,6 @@ static int wave5_vpu_open_enc(struct file *filp)
> 		goto cleanup_inst;
> 	}
>
>-	wave5_vdi_allocate_sram(inst->dev);
>-
> 	return 0;
>
> cleanup_inst:
>diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
>b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
>index f3ecadefd37a..2a0a70dd7062 100644
>--- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
>+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
>@@ -178,16 +178,11 @@ static int wave5_vpu_probe(struct platform_device
>*pdev)
> 		return ret;
> 	}
>
>-	ret = of_property_read_u32(pdev->dev.of_node, "sram-size",
>-				   &dev->sram_size);
>-	if (ret) {
>-		dev_warn(&pdev->dev, "sram-size not found\n");
>-		dev->sram_size = 0;
>-	}
>-

Required SRAM size is different from each wave5 product.
And, SoC vendor also can configure the different SRAM size
depend on target SoC specification even they use the same wave5 product.

Thanks.
Nas.

> 	dev->sram_pool = of_gen_pool_get(pdev->dev.of_node, "sram", 0);
> 	if (!dev->sram_pool)
> 		dev_warn(&pdev->dev, "sram node not found\n");
>+	else
>+		wave5_vdi_allocate_sram(dev);
>
> 	dev->product_code = wave5_vdi_read_register(dev,
>VPU_PRODUCT_CODE_REGISTER);
> 	ret = wave5_vdi_init(&pdev->dev);
>@@ -259,6 +254,8 @@ static int wave5_vpu_probe(struct platform_device
>*pdev)
> err_clk_dis:
> 	clk_bulk_disable_unprepare(dev->num_clks, dev->clks);
>
>+	wave5_vdi_free_sram(dev);
>+
> 	return ret;
> }
>
>@@ -275,6 +272,7 @@ static void wave5_vpu_remove(struct platform_device
>*pdev)
> 	v4l2_device_unregister(&dev->v4l2_dev);
> 	wave5_vdi_release(&pdev->dev);
> 	ida_destroy(&dev->inst_ida);
>+	wave5_vdi_free_sram(dev);
> }
>
> static const struct wave5_match_data ti_wave521c_data = {
>diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
>b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
>index fa62a85080b5..8d88381ac55e 100644
>--- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
>+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
>@@ -749,7 +749,6 @@ struct vpu_device {
> 	struct vpu_attr attr;
> 	struct vpu_buf common_mem;
> 	u32 last_performance_cycles;
>-	u32 sram_size;
> 	struct gen_pool *sram_pool;
> 	struct vpu_buf sram_buf;
> 	void __iomem *vdb_register;
>--
>2.44.0

Re: RE: [PATCH 5/6] media: chips-media: wave5: refine SRAM usage

[Ivan Bornyakov]

Hello, Nas

On Tue, Mar 19, 2024 at 10:56:22AM +0000, Nas Chung wrote:
> Hi, Ivan.
> 
> >-----Original Message-----
> >From: Ivan Bornyakov <brnkv.i1@gmail.com>
> >Sent: Monday, March 18, 2024 11:42 PM
> >To: Nas Chung <nas.chung@chipsnmedia.com>; jackson.lee
> ><jackson.lee@chipsnmedia.com>; Mauro Carvalho Chehab <mchehab@kernel.org>
> >Cc: Ivan Bornyakov <brnkv.i1@gmail.com>; Philipp Zabel
> ><p.zabel@pengutronix.de>; Rob Herring <robh@kernel.org>; Krzysztof
> >Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> ><conor+dt@kernel.org>; linux-media@vger.kernel.org; linux-
> >kernel@vger.kernel.org; devicetree@vger.kernel.org
> >Subject: [PATCH 5/6] media: chips-media: wave5: refine SRAM usage
> >
> >Allocate SRAM memory on module probe, free on remove. There is no need
> >to allocate on device open, free on close, the memory is the same every
> >time.
> 
> If there is no decoder/encoder instance, driver don't need to allocate SRAM memory.
> The main reason of allocating the memory in open() is to allow other modules to
> use more SRAM memory, if wave5 is not working.
> 
> >
> >Also use gen_pool_size() to determine SRAM memory size to be allocated
> >instead of separate "sram-size" DT property to reduce duplication.
> >
> >Signed-off-by: Ivan Bornyakov <brnkv.i1@gmail.com>
> >---
> > .../platform/chips-media/wave5/wave5-helper.c |  3 ---
> > .../platform/chips-media/wave5/wave5-vdi.c    | 21 ++++++++++---------
> > .../chips-media/wave5/wave5-vpu-dec.c         |  2 --
> > .../chips-media/wave5/wave5-vpu-enc.c         |  2 --
> > .../platform/chips-media/wave5/wave5-vpu.c    | 12 +++++------
> > .../platform/chips-media/wave5/wave5-vpuapi.h |  1 -
> > 6 files changed, 16 insertions(+), 25 deletions(-)
> >
> >diff --git a/drivers/media/platform/chips-media/wave5/wave5-helper.c
> >b/drivers/media/platform/chips-media/wave5/wave5-helper.c
> >index 8433ecab230c..ec710b838dfe 100644
> >--- a/drivers/media/platform/chips-media/wave5/wave5-helper.c
> >+++ b/drivers/media/platform/chips-media/wave5/wave5-helper.c
> >@@ -29,9 +29,6 @@ void wave5_cleanup_instance(struct vpu_instance *inst)
> > {
> > 	int i;
> >
> >-	if (list_is_singular(&inst->list))
> >-		wave5_vdi_free_sram(inst->dev);
> >-
> > 	for (i = 0; i < inst->fbc_buf_count; i++)
> > 		wave5_vpu_dec_reset_framebuffer(inst, i);
> >
> >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> >b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> >index 3809f70bc0b4..ee671f5a2f37 100644
> >--- a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> >+++ b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> >@@ -174,16 +174,19 @@ int wave5_vdi_allocate_array(struct vpu_device
> >*vpu_dev, struct vpu_buf *array,
> > void wave5_vdi_allocate_sram(struct vpu_device *vpu_dev)
> > {
> > 	struct vpu_buf *vb = &vpu_dev->sram_buf;
> >+	dma_addr_t daddr;
> >+	void *vaddr;
> >+	size_t size;
> >
> >-	if (!vpu_dev->sram_pool || !vpu_dev->sram_size)
> >+	if (!vpu_dev->sram_pool || vb->vaddr)
> > 		return;
> >
> >-	if (!vb->vaddr) {
> >-		vb->size = vpu_dev->sram_size;
> >-		vb->vaddr = gen_pool_dma_alloc(vpu_dev->sram_pool, vb->size,
> >-					       &vb->daddr);
> >-		if (!vb->vaddr)
> >-			vb->size = 0;
> >+	size = gen_pool_size(vpu_dev->sram_pool);
> >+	vaddr = gen_pool_dma_alloc(vpu_dev->sram_pool, size, &daddr);
> >+	if (vaddr) {
> >+		vb->vaddr = vaddr;
> >+		vb->daddr = daddr;
> >+		vb->size = size;
> > 	}
> >
> > 	dev_dbg(vpu_dev->dev, "%s: sram daddr: %pad, size: %zu, vaddr:
> >0x%p\n",
> >@@ -197,9 +200,7 @@ void wave5_vdi_free_sram(struct vpu_device *vpu_dev)
> > 	if (!vb->size || !vb->vaddr)
> > 		return;
> >
> >-	if (vb->vaddr)
> >-		gen_pool_free(vpu_dev->sram_pool, (unsigned long)vb->vaddr,
> >-			      vb->size);
> >+	gen_pool_free(vpu_dev->sram_pool, (unsigned long)vb->vaddr, vb-
> >>size);
> >
> > 	memset(vb, 0, sizeof(*vb));
> > }
> >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> >b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> >index aa0401f35d32..84dbe56216ad 100644
> >--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> >@@ -1854,8 +1854,6 @@ static int wave5_vpu_open_dec(struct file *filp)
> > 		goto cleanup_inst;
> > 	}
> >
> >-	wave5_vdi_allocate_sram(inst->dev);
> >-
> > 	return 0;
> >
> > cleanup_inst:
> >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> >b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> >index 8bbf9d10b467..86ddcb82443b 100644
> >--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> >@@ -1727,8 +1727,6 @@ static int wave5_vpu_open_enc(struct file *filp)
> > 		goto cleanup_inst;
> > 	}
> >
> >-	wave5_vdi_allocate_sram(inst->dev);
> >-
> > 	return 0;
> >
> > cleanup_inst:
> >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> >b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> >index f3ecadefd37a..2a0a70dd7062 100644
> >--- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> >@@ -178,16 +178,11 @@ static int wave5_vpu_probe(struct platform_device
> >*pdev)
> > 		return ret;
> > 	}
> >
> >-	ret = of_property_read_u32(pdev->dev.of_node, "sram-size",
> >-				   &dev->sram_size);
> >-	if (ret) {
> >-		dev_warn(&pdev->dev, "sram-size not found\n");
> >-		dev->sram_size = 0;
> >-	}
> >-
> 
> Required SRAM size is different from each wave5 product.
> And, SoC vendor also can configure the different SRAM size
> depend on target SoC specification even they use the same wave5 product.
> 

One can limit iomem address range in SRAM node. Here is the example of
how I setup Wave515 with SRAM:

	sram@2000000 {
		compatible = "mmio-sram";
		reg = <0x0 0x2000000 0x0 0x80000>;
		#address-cells = <1>;
		#size-cells = <1>;
		ranges = <0x0 0x0 0x2000000 0x80000>;
		
		wave515_vpu_sram: wave515-vpu-sram@0 {
			reg = <0x0 0x80000>;
			pool;
		};
	};

	wave515@410000 {
		compatible = "cnm,wave515";
		reg = <0x0 0x410000 0x0 0x10000>;
		clocks = <&clk_ref1>;
		clock-names = "videc";
		interrupt-parent = <&wave515_intc>;
		interrupts = <16 IRQ_TYPE_LEVEL_HIGH>;
		resets = <&wave515_reset 0>,
			 <&wave515_reset 4>,
			 <&wave515_reset 8>,
			 <&wave515_reset 12>;
		sram = <&wave515_vpu_sram>;
	};

gen_pool_size() returns size of wave515_vpu_sram, no need for extra
"sram-size" property.

> Thanks.
> Nas.
> 
> > 	dev->sram_pool = of_gen_pool_get(pdev->dev.of_node, "sram", 0);
> > 	if (!dev->sram_pool)
> > 		dev_warn(&pdev->dev, "sram node not found\n");
> >+	else
> >+		wave5_vdi_allocate_sram(dev);
> >
> > 	dev->product_code = wave5_vdi_read_register(dev,
> >VPU_PRODUCT_CODE_REGISTER);
> > 	ret = wave5_vdi_init(&pdev->dev);
> >@@ -259,6 +254,8 @@ static int wave5_vpu_probe(struct platform_device
> >*pdev)
> > err_clk_dis:
> > 	clk_bulk_disable_unprepare(dev->num_clks, dev->clks);
> >
> >+	wave5_vdi_free_sram(dev);
> >+
> > 	return ret;
> > }
> >
> >@@ -275,6 +272,7 @@ static void wave5_vpu_remove(struct platform_device
> >*pdev)
> > 	v4l2_device_unregister(&dev->v4l2_dev);
> > 	wave5_vdi_release(&pdev->dev);
> > 	ida_destroy(&dev->inst_ida);
> >+	wave5_vdi_free_sram(dev);
> > }
> >
> > static const struct wave5_match_data ti_wave521c_data = {
> >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> >b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> >index fa62a85080b5..8d88381ac55e 100644
> >--- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> >@@ -749,7 +749,6 @@ struct vpu_device {
> > 	struct vpu_attr attr;
> > 	struct vpu_buf common_mem;
> > 	u32 last_performance_cycles;
> >-	u32 sram_size;
> > 	struct gen_pool *sram_pool;
> > 	struct vpu_buf sram_buf;
> > 	void __iomem *vdb_register;
> >--
> >2.44.0
> 

Re: [PATCH 5/6] media: chips-media: wave5: refine SRAM usage

[Brandon Brnich]

Hi Ivan, 

On 14:24-20240319, Ivan Bornyakov wrote:
> Hello, Nas
> 
> On Tue, Mar 19, 2024 at 10:56:22AM +0000, Nas Chung wrote:
> > Hi, Ivan.
> > 
> > >
> > >Allocate SRAM memory on module probe, free on remove. There is no need
> > >to allocate on device open, free on close, the memory is the same every
> > >time.
> > 
> > If there is no decoder/encoder instance, driver don't need to allocate SRAM memory.
> > The main reason of allocating the memory in open() is to allow other modules to
> > use more SRAM memory, if wave5 is not working.

I have to agree with this statement. Moving allocation to probe results
in wasting SRAM when VPU is not in use. VPU should only be allocating SRAM
when a stream instance is running and free that back once all instances
close.
 
> > >
> > >Also use gen_pool_size() to determine SRAM memory size to be allocated
> > >instead of separate "sram-size" DT property to reduce duplication.
> > >
> > >Signed-off-by: Ivan Bornyakov <brnkv.i1@gmail.com>
> > >---
> > > .../platform/chips-media/wave5/wave5-helper.c |  3 ---
> > > .../platform/chips-media/wave5/wave5-vdi.c    | 21 ++++++++++---------
> > > .../chips-media/wave5/wave5-vpu-dec.c         |  2 --
> > > .../chips-media/wave5/wave5-vpu-enc.c         |  2 --
> > > .../platform/chips-media/wave5/wave5-vpu.c    | 12 +++++------
> > > .../platform/chips-media/wave5/wave5-vpuapi.h |  1 -
> > > 6 files changed, 16 insertions(+), 25 deletions(-)
> > >
> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-helper.c
> > >b/drivers/media/platform/chips-media/wave5/wave5-helper.c
> > >index 8433ecab230c..ec710b838dfe 100644
> > >--- a/drivers/media/platform/chips-media/wave5/wave5-helper.c
> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-helper.c
> > >@@ -29,9 +29,6 @@ void wave5_cleanup_instance(struct vpu_instance *inst)
> > > {
> > > 	int i;
> > >
> > >-	if (list_is_singular(&inst->list))
> > >-		wave5_vdi_free_sram(inst->dev);
> > >-
> > > 	for (i = 0; i < inst->fbc_buf_count; i++)
> > > 		wave5_vpu_dec_reset_framebuffer(inst, i);
> > >
> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> > >b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> > >index 3809f70bc0b4..ee671f5a2f37 100644
> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> > >@@ -174,16 +174,19 @@ int wave5_vdi_allocate_array(struct vpu_device
> > >*vpu_dev, struct vpu_buf *array,
> > > void wave5_vdi_allocate_sram(struct vpu_device *vpu_dev)
> > > {
> > > 	struct vpu_buf *vb = &vpu_dev->sram_buf;
> > >+	dma_addr_t daddr;
> > >+	void *vaddr;
> > >+	size_t size;
> > >
> > >-	if (!vpu_dev->sram_pool || !vpu_dev->sram_size)
> > >+	if (!vpu_dev->sram_pool || vb->vaddr)
> > > 		return;
> > >
> > >-	if (!vb->vaddr) {
> > >-		vb->size = vpu_dev->sram_size;
> > >-		vb->vaddr = gen_pool_dma_alloc(vpu_dev->sram_pool, vb->size,
> > >-					       &vb->daddr);
> > >-		if (!vb->vaddr)
> > >-			vb->size = 0;
> > >+	size = gen_pool_size(vpu_dev->sram_pool);
> > >+	vaddr = gen_pool_dma_alloc(vpu_dev->sram_pool, size, &daddr);
> > >+	if (vaddr) {
> > >+		vb->vaddr = vaddr;
> > >+		vb->daddr = daddr;
> > >+		vb->size = size;
> > > 	}
> > >
> > > 	dev_dbg(vpu_dev->dev, "%s: sram daddr: %pad, size: %zu, vaddr:
> > >0x%p\n",
> > >@@ -197,9 +200,7 @@ void wave5_vdi_free_sram(struct vpu_device *vpu_dev)
> > > 	if (!vb->size || !vb->vaddr)
> > > 		return;
> > >
> > >-	if (vb->vaddr)
> > >-		gen_pool_free(vpu_dev->sram_pool, (unsigned long)vb->vaddr,
> > >-			      vb->size);
> > >+	gen_pool_free(vpu_dev->sram_pool, (unsigned long)vb->vaddr, vb-
> > >>size);
> > >
> > > 	memset(vb, 0, sizeof(*vb));
> > > }
> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> > >b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> > >index aa0401f35d32..84dbe56216ad 100644
> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> > >@@ -1854,8 +1854,6 @@ static int wave5_vpu_open_dec(struct file *filp)
> > > 		goto cleanup_inst;
> > > 	}
> > >
> > >-	wave5_vdi_allocate_sram(inst->dev);
> > >-
> > > 	return 0;
> > >
> > > cleanup_inst:
> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > >b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > >index 8bbf9d10b467..86ddcb82443b 100644
> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > >@@ -1727,8 +1727,6 @@ static int wave5_vpu_open_enc(struct file *filp)
> > > 		goto cleanup_inst;
> > > 	}
> > >
> > >-	wave5_vdi_allocate_sram(inst->dev);
> > >-
> > > 	return 0;
> > >
> > > cleanup_inst:
> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > >b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > >index f3ecadefd37a..2a0a70dd7062 100644
> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > >@@ -178,16 +178,11 @@ static int wave5_vpu_probe(struct platform_device
> > >*pdev)
> > > 		return ret;
> > > 	}
> > >
> > >-	ret = of_property_read_u32(pdev->dev.of_node, "sram-size",
> > >-				   &dev->sram_size);
> > >-	if (ret) {
> > >-		dev_warn(&pdev->dev, "sram-size not found\n");
> > >-		dev->sram_size = 0;
> > >-	}
> > >-
> > 
> > Required SRAM size is different from each wave5 product.
> > And, SoC vendor also can configure the different SRAM size
> > depend on target SoC specification even they use the same wave5 product.
> > 
> 
> One can limit iomem address range in SRAM node. Here is the example of
> how I setup Wave515 with SRAM:
> 
> 	sram@2000000 {
> 		compatible = "mmio-sram";
> 		reg = <0x0 0x2000000 0x0 0x80000>;
> 		#address-cells = <1>;
> 		#size-cells = <1>;
> 		ranges = <0x0 0x0 0x2000000 0x80000>;
> 		
> 		wave515_vpu_sram: wave515-vpu-sram@0 {
> 			reg = <0x0 0x80000>;
> 			pool;
> 		};
> 	};
> 
> 	wave515@410000 {
> 		compatible = "cnm,wave515";
> 		reg = <0x0 0x410000 0x0 0x10000>;
> 		clocks = <&clk_ref1>;
> 		clock-names = "videc";
> 		interrupt-parent = <&wave515_intc>;
> 		interrupts = <16 IRQ_TYPE_LEVEL_HIGH>;
> 		resets = <&wave515_reset 0>,
> 			 <&wave515_reset 4>,
> 			 <&wave515_reset 8>,
> 			 <&wave515_reset 12>;
> 		sram = <&wave515_vpu_sram>;
> 	};
> 
> gen_pool_size() returns size of wave515_vpu_sram, no need for extra
> "sram-size" property.

"sram-size" property does need to be removed, as this was the consensus
gathered from my patch[0]. However, I think your method is still taking
a more static approach. One of the recommendations in my thread[1] was
making a list of known SRAM sizes given typical resolutions and
iterating through until a valid allocation is done. I don't think this
is the correct approach either based on Nas's comment that each Wave5
has different SRAM size requirement. It would clutter up the file too
much if each wave5 product had its own SRAM size mapping.

Could another approach be to change Wave5 dts node to have property set
as "sram = <&sram>;" in your example, then driver calls
gen_pool_availble to get size remaining? From there, a check could be 
put in place to make sure an unnecessary amount is not being allocated.


[0]: https://lore.kernel.org/lkml/99bf4d6d988d426492fffc8de9015751c323bd8a.camel@ndufresne.ca/
[1]: https://lore.kernel.org/lkml/9c5b7b2c-8a66-4173-dfe9-5724ec5f733d@ti.com/

Thanks,
Brandon
> 
> > Thanks.
> > Nas.
> > 
> > > 	dev->sram_pool = of_gen_pool_get(pdev->dev.of_node, "sram", 0);
> > > 	if (!dev->sram_pool)
> > > 		dev_warn(&pdev->dev, "sram node not found\n");
> > >+	else
> > >+		wave5_vdi_allocate_sram(dev);
> > >
> > > 	dev->product_code = wave5_vdi_read_register(dev,
> > >VPU_PRODUCT_CODE_REGISTER);
> > > 	ret = wave5_vdi_init(&pdev->dev);
> > >@@ -259,6 +254,8 @@ static int wave5_vpu_probe(struct platform_device
> > >*pdev)
> > > err_clk_dis:
> > > 	clk_bulk_disable_unprepare(dev->num_clks, dev->clks);
> > >
> > >+	wave5_vdi_free_sram(dev);
> > >+
> > > 	return ret;
> > > }
> > >
> > >@@ -275,6 +272,7 @@ static void wave5_vpu_remove(struct platform_device
> > >*pdev)
> > > 	v4l2_device_unregister(&dev->v4l2_dev);
> > > 	wave5_vdi_release(&pdev->dev);
> > > 	ida_destroy(&dev->inst_ida);
> > >+	wave5_vdi_free_sram(dev);
> > > }
> > >
> > > static const struct wave5_match_data ti_wave521c_data = {
> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> > >b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> > >index fa62a85080b5..8d88381ac55e 100644
> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> > >@@ -749,7 +749,6 @@ struct vpu_device {
> > > 	struct vpu_attr attr;
> > > 	struct vpu_buf common_mem;
> > > 	u32 last_performance_cycles;
> > >-	u32 sram_size;
> > > 	struct gen_pool *sram_pool;
> > > 	struct vpu_buf sram_buf;
> > > 	void __iomem *vdb_register;
> > >--
> > >2.44.0
> > 
> 

Re: [PATCH 3/6] dt-bindings: media: cnm,wave521c: drop resets restriction

[Rob Herring]

On Mon, Mar 18, 2024 at 05:42:18PM +0300, Ivan Bornyakov wrote:
> Different designs may have different amount of routed reset signals.
> Drop maxItems restriction, add a small description instead.
> 
> Signed-off-by: Ivan Bornyakov <brnkv.i1@gmail.com>
> ---
>  Documentation/devicetree/bindings/media/cnm,wave521c.yaml | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/cnm,wave521c.yaml b/Documentation/devicetree/bindings/media/cnm,wave521c.yaml
> index 6a11c1d11fb5..2cb4f68d27ac 100644
> --- a/Documentation/devicetree/bindings/media/cnm,wave521c.yaml
> +++ b/Documentation/devicetree/bindings/media/cnm,wave521c.yaml
> @@ -34,7 +34,8 @@ properties:
>      maxItems: 1
>  
>    resets:
> -    maxItems: 1
> +    items:
> +      - description: Optional reset lines

Your change is a NOP. In fact, the tools will translate an 'items' list 
with no constraints back into maxItems/minItems.

So it seems whatever change you think you needed, you did not validate 
the change.

Rob

RE: [PATCH 5/6] media: chips-media: wave5: refine SRAM usage

[Nas Chung]

Hi, Ivan and Brandon.

>-----Original Message-----
>From: Brandon Brnich <b-brnich@ti.com>
>Sent: Wednesday, March 20, 2024 6:01 AM
>To: Ivan Bornyakov <brnkv.i1@gmail.com>
>Cc: Nas Chung <nas.chung@chipsnmedia.com>; Philipp Zabel
><p.zabel@pengutronix.de>; Rob Herring <robh@kernel.org>; Krzysztof
>Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
><conor+dt@kernel.org>; linux-media@vger.kernel.org; linux-
>kernel@vger.kernel.org; devicetree@vger.kernel.org; jackson.lee
><jackson.lee@chipsnmedia.com>; Mauro Carvalho Chehab <mchehab@kernel.org>
>Subject: Re: [PATCH 5/6] media: chips-media: wave5: refine SRAM usage
>
>Hi Ivan,
>
>On 14:24-20240319, Ivan Bornyakov wrote:
>> Hello, Nas
>>
>> On Tue, Mar 19, 2024 at 10:56:22AM +0000, Nas Chung wrote:
>> > Hi, Ivan.
>> >
>> > >
>> > >Allocate SRAM memory on module probe, free on remove. There is no
>need
>> > >to allocate on device open, free on close, the memory is the same
>every
>> > >time.
>> >
>> > If there is no decoder/encoder instance, driver don't need to
>allocate SRAM memory.
>> > The main reason of allocating the memory in open() is to allow other
>modules to
>> > use more SRAM memory, if wave5 is not working.
>
>I have to agree with this statement. Moving allocation to probe results
>in wasting SRAM when VPU is not in use. VPU should only be allocating
>SRAM
>when a stream instance is running and free that back once all instances
>close.
>
>> > >
>> > >Also use gen_pool_size() to determine SRAM memory size to be
>allocated
>> > >instead of separate "sram-size" DT property to reduce duplication.
>> > >
>> > >Signed-off-by: Ivan Bornyakov <brnkv.i1@gmail.com>
>> > >---
>> > > .../platform/chips-media/wave5/wave5-helper.c |  3 ---
>> > > .../platform/chips-media/wave5/wave5-vdi.c    | 21 ++++++++++-------
>--
>> > > .../chips-media/wave5/wave5-vpu-dec.c         |  2 --
>> > > .../chips-media/wave5/wave5-vpu-enc.c         |  2 --
>> > > .../platform/chips-media/wave5/wave5-vpu.c    | 12 +++++------
>> > > .../platform/chips-media/wave5/wave5-vpuapi.h |  1 -
>> > > 6 files changed, 16 insertions(+), 25 deletions(-)
>> > >
>> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-helper.c
>> > >b/drivers/media/platform/chips-media/wave5/wave5-helper.c
>> > >index 8433ecab230c..ec710b838dfe 100644
>> > >--- a/drivers/media/platform/chips-media/wave5/wave5-helper.c
>> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-helper.c
>> > >@@ -29,9 +29,6 @@ void wave5_cleanup_instance(struct vpu_instance
>*inst)
>> > > {
>> > > 	int i;
>> > >
>> > >-	if (list_is_singular(&inst->list))
>> > >-		wave5_vdi_free_sram(inst->dev);
>> > >-
>> > > 	for (i = 0; i < inst->fbc_buf_count; i++)
>> > > 		wave5_vpu_dec_reset_framebuffer(inst, i);
>> > >
>> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
>> > >b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
>> > >index 3809f70bc0b4..ee671f5a2f37 100644
>> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
>> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
>> > >@@ -174,16 +174,19 @@ int wave5_vdi_allocate_array(struct vpu_device
>> > >*vpu_dev, struct vpu_buf *array,
>> > > void wave5_vdi_allocate_sram(struct vpu_device *vpu_dev)
>> > > {
>> > > 	struct vpu_buf *vb = &vpu_dev->sram_buf;
>> > >+	dma_addr_t daddr;
>> > >+	void *vaddr;
>> > >+	size_t size;
>> > >
>> > >-	if (!vpu_dev->sram_pool || !vpu_dev->sram_size)
>> > >+	if (!vpu_dev->sram_pool || vb->vaddr)
>> > > 		return;
>> > >
>> > >-	if (!vb->vaddr) {
>> > >-		vb->size = vpu_dev->sram_size;
>> > >-		vb->vaddr = gen_pool_dma_alloc(vpu_dev->sram_pool, vb->size,
>> > >-					       &vb->daddr);
>> > >-		if (!vb->vaddr)
>> > >-			vb->size = 0;
>> > >+	size = gen_pool_size(vpu_dev->sram_pool);
>> > >+	vaddr = gen_pool_dma_alloc(vpu_dev->sram_pool, size, &daddr);
>> > >+	if (vaddr) {
>> > >+		vb->vaddr = vaddr;
>> > >+		vb->daddr = daddr;
>> > >+		vb->size = size;
>> > > 	}
>> > >
>> > > 	dev_dbg(vpu_dev->dev, "%s: sram daddr: %pad, size: %zu, vaddr:
>> > >0x%p\n",
>> > >@@ -197,9 +200,7 @@ void wave5_vdi_free_sram(struct vpu_device
>*vpu_dev)
>> > > 	if (!vb->size || !vb->vaddr)
>> > > 		return;
>> > >
>> > >-	if (vb->vaddr)
>> > >-		gen_pool_free(vpu_dev->sram_pool, (unsigned long)vb->vaddr,
>> > >-			      vb->size);
>> > >+	gen_pool_free(vpu_dev->sram_pool, (unsigned long)vb->vaddr, vb-
>> > >>size);
>> > >
>> > > 	memset(vb, 0, sizeof(*vb));
>> > > }
>> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-
>dec.c
>> > >b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
>> > >index aa0401f35d32..84dbe56216ad 100644
>> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
>> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
>> > >@@ -1854,8 +1854,6 @@ static int wave5_vpu_open_dec(struct file
>*filp)
>> > > 		goto cleanup_inst;
>> > > 	}
>> > >
>> > >-	wave5_vdi_allocate_sram(inst->dev);
>> > >-
>> > > 	return 0;
>> > >
>> > > cleanup_inst:
>> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-
>enc.c
>> > >b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
>> > >index 8bbf9d10b467..86ddcb82443b 100644
>> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
>> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
>> > >@@ -1727,8 +1727,6 @@ static int wave5_vpu_open_enc(struct file
>*filp)
>> > > 		goto cleanup_inst;
>> > > 	}
>> > >
>> > >-	wave5_vdi_allocate_sram(inst->dev);
>> > >-
>> > > 	return 0;
>> > >
>> > > cleanup_inst:
>> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
>> > >b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
>> > >index f3ecadefd37a..2a0a70dd7062 100644
>> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
>> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
>> > >@@ -178,16 +178,11 @@ static int wave5_vpu_probe(struct
>platform_device
>> > >*pdev)
>> > > 		return ret;
>> > > 	}
>> > >
>> > >-	ret = of_property_read_u32(pdev->dev.of_node, "sram-size",
>> > >-				   &dev->sram_size);
>> > >-	if (ret) {
>> > >-		dev_warn(&pdev->dev, "sram-size not found\n");
>> > >-		dev->sram_size = 0;
>> > >-	}
>> > >-
>> >
>> > Required SRAM size is different from each wave5 product.
>> > And, SoC vendor also can configure the different SRAM size
>> > depend on target SoC specification even they use the same wave5
>product.
>> >
>>
>> One can limit iomem address range in SRAM node. Here is the example of
>> how I setup Wave515 with SRAM:
>>
>> 	sram@2000000 {
>> 		compatible = "mmio-sram";
>> 		reg = <0x0 0x2000000 0x0 0x80000>;
>> 		#address-cells = <1>;
>> 		#size-cells = <1>;
>> 		ranges = <0x0 0x0 0x2000000 0x80000>;
>>
>> 		wave515_vpu_sram: wave515-vpu-sram@0 {
>> 			reg = <0x0 0x80000>;
>> 			pool;
>> 		};
>> 	};
>>
>> 	wave515@410000 {
>> 		compatible = "cnm,wave515";
>> 		reg = <0x0 0x410000 0x0 0x10000>;
>> 		clocks = <&clk_ref1>;
>> 		clock-names = "videc";
>> 		interrupt-parent = <&wave515_intc>;
>> 		interrupts = <16 IRQ_TYPE_LEVEL_HIGH>;
>> 		resets = <&wave515_reset 0>,
>> 			 <&wave515_reset 4>,
>> 			 <&wave515_reset 8>,
>> 			 <&wave515_reset 12>;
>> 		sram = <&wave515_vpu_sram>;
>> 	};
>>
>> gen_pool_size() returns size of wave515_vpu_sram, no need for extra
>> "sram-size" property.

Thanks for sharing the example.
I agree that the "sram-size" property is not needed.

>
>"sram-size" property does need to be removed, as this was the consensus
>gathered from my patch[0]. However, I think your method is still taking

I missed the previous consensus for the sram-size property.
Thanks for letting me know.

>a more static approach. One of the recommendations in my thread[1] was
>making a list of known SRAM sizes given typical resolutions and
>iterating through until a valid allocation is done. I don't think this
>is the correct approach either based on Nas's comment that each Wave5
>has different SRAM size requirement. It would clutter up the file too
>much if each wave5 product had its own SRAM size mapping.
>
>Could another approach be to change Wave5 dts node to have property set
>as "sram = <&sram>;" in your example, then driver calls
>gen_pool_availble to get size remaining? From there, a check could be
>put in place to make sure an unnecessary amount is not being allocated.

Ivan's approach looks good to me.
It is similar to your first patch, which adds the sram-size property
to configure different SRAM sizes for each device.
And, Driver won't know unnecessary amount is allocated before parsing
bitstream header.

>
>
>[0]:
>https://lore.kernel.org/lkml/99bf4d6d988d426492fffc8de9015751c323bd8a.cam
>el@ndufresne.ca/
>[1]: https://lore.kernel.org/lkml/9c5b7b2c-8a66-4173-dfe9-
>5724ec5f733d@ti.com/
>
>Thanks,
>Brandon
>>
>> > Thanks.
>> > Nas.
>> >
>> > > 	dev->sram_pool = of_gen_pool_get(pdev->dev.of_node, "sram", 0);
>> > > 	if (!dev->sram_pool)
>> > > 		dev_warn(&pdev->dev, "sram node not found\n");
>> > >+	else
>> > >+		wave5_vdi_allocate_sram(dev);
>> > >
>> > > 	dev->product_code = wave5_vdi_read_register(dev,
>> > >VPU_PRODUCT_CODE_REGISTER);
>> > > 	ret = wave5_vdi_init(&pdev->dev);
>> > >@@ -259,6 +254,8 @@ static int wave5_vpu_probe(struct
>platform_device
>> > >*pdev)
>> > > err_clk_dis:
>> > > 	clk_bulk_disable_unprepare(dev->num_clks, dev->clks);
>> > >
>> > >+	wave5_vdi_free_sram(dev);
>> > >+
>> > > 	return ret;
>> > > }
>> > >
>> > >@@ -275,6 +272,7 @@ static void wave5_vpu_remove(struct
>platform_device
>> > >*pdev)
>> > > 	v4l2_device_unregister(&dev->v4l2_dev);
>> > > 	wave5_vdi_release(&pdev->dev);
>> > > 	ida_destroy(&dev->inst_ida);
>> > >+	wave5_vdi_free_sram(dev);
>> > > }
>> > >
>> > > static const struct wave5_match_data ti_wave521c_data = {
>> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
>> > >b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
>> > >index fa62a85080b5..8d88381ac55e 100644
>> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
>> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
>> > >@@ -749,7 +749,6 @@ struct vpu_device {
>> > > 	struct vpu_attr attr;
>> > > 	struct vpu_buf common_mem;
>> > > 	u32 last_performance_cycles;
>> > >-	u32 sram_size;
>> > > 	struct gen_pool *sram_pool;
>> > > 	struct vpu_buf sram_buf;
>> > > 	void __iomem *vdb_register;
>> > >--
>> > >2.44.0
>> >
>>

Re: RE: [PATCH 5/6] media: chips-media: wave5: refine SRAM usage

[Ivan Bornyakov]

Hi!

On Thu, Mar 21, 2024 at 09:29:04AM +0000, Nas Chung wrote:
> Hi, Ivan and Brandon.
> 
> >-----Original Message-----
> >From: Brandon Brnich <b-brnich@ti.com>
> >Sent: Wednesday, March 20, 2024 6:01 AM
> >To: Ivan Bornyakov <brnkv.i1@gmail.com>
> >Cc: Nas Chung <nas.chung@chipsnmedia.com>; Philipp Zabel
> ><p.zabel@pengutronix.de>; Rob Herring <robh@kernel.org>; Krzysztof
> >Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> ><conor+dt@kernel.org>; linux-media@vger.kernel.org; linux-
> >kernel@vger.kernel.org; devicetree@vger.kernel.org; jackson.lee
> ><jackson.lee@chipsnmedia.com>; Mauro Carvalho Chehab <mchehab@kernel.org>
> >Subject: Re: [PATCH 5/6] media: chips-media: wave5: refine SRAM usage
> >
> >Hi Ivan,
> >
> >On 14:24-20240319, Ivan Bornyakov wrote:
> >> Hello, Nas
> >>
> >> On Tue, Mar 19, 2024 at 10:56:22AM +0000, Nas Chung wrote:
> >> > Hi, Ivan.
> >> >
> >> > >
> >> > >Allocate SRAM memory on module probe, free on remove. There is no
> >need
> >> > >to allocate on device open, free on close, the memory is the same
> >every
> >> > >time.
> >> >
> >> > If there is no decoder/encoder instance, driver don't need to
> >allocate SRAM memory.
> >> > The main reason of allocating the memory in open() is to allow other
> >modules to
> >> > use more SRAM memory, if wave5 is not working.
> >
> >I have to agree with this statement. Moving allocation to probe results
> >in wasting SRAM when VPU is not in use. VPU should only be allocating
> >SRAM
> >when a stream instance is running and free that back once all instances
> >close.
> >
> >> > >
> >> > >Also use gen_pool_size() to determine SRAM memory size to be
> >allocated
> >> > >instead of separate "sram-size" DT property to reduce duplication.
> >> > >
> >> > >Signed-off-by: Ivan Bornyakov <brnkv.i1@gmail.com>
> >> > >---
> >> > > .../platform/chips-media/wave5/wave5-helper.c |  3 ---
> >> > > .../platform/chips-media/wave5/wave5-vdi.c    | 21 ++++++++++-------
> >--
> >> > > .../chips-media/wave5/wave5-vpu-dec.c         |  2 --
> >> > > .../chips-media/wave5/wave5-vpu-enc.c         |  2 --
> >> > > .../platform/chips-media/wave5/wave5-vpu.c    | 12 +++++------
> >> > > .../platform/chips-media/wave5/wave5-vpuapi.h |  1 -
> >> > > 6 files changed, 16 insertions(+), 25 deletions(-)
> >> > >
> >> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-helper.c
> >> > >b/drivers/media/platform/chips-media/wave5/wave5-helper.c
> >> > >index 8433ecab230c..ec710b838dfe 100644
> >> > >--- a/drivers/media/platform/chips-media/wave5/wave5-helper.c
> >> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-helper.c
> >> > >@@ -29,9 +29,6 @@ void wave5_cleanup_instance(struct vpu_instance
> >*inst)
> >> > > {
> >> > > 	int i;
> >> > >
> >> > >-	if (list_is_singular(&inst->list))
> >> > >-		wave5_vdi_free_sram(inst->dev);
> >> > >-
> >> > > 	for (i = 0; i < inst->fbc_buf_count; i++)
> >> > > 		wave5_vpu_dec_reset_framebuffer(inst, i);
> >> > >
> >> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> >> > >b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> >> > >index 3809f70bc0b4..ee671f5a2f37 100644
> >> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> >> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> >> > >@@ -174,16 +174,19 @@ int wave5_vdi_allocate_array(struct vpu_device
> >> > >*vpu_dev, struct vpu_buf *array,
> >> > > void wave5_vdi_allocate_sram(struct vpu_device *vpu_dev)
> >> > > {
> >> > > 	struct vpu_buf *vb = &vpu_dev->sram_buf;
> >> > >+	dma_addr_t daddr;
> >> > >+	void *vaddr;
> >> > >+	size_t size;
> >> > >
> >> > >-	if (!vpu_dev->sram_pool || !vpu_dev->sram_size)
> >> > >+	if (!vpu_dev->sram_pool || vb->vaddr)
> >> > > 		return;
> >> > >
> >> > >-	if (!vb->vaddr) {
> >> > >-		vb->size = vpu_dev->sram_size;
> >> > >-		vb->vaddr = gen_pool_dma_alloc(vpu_dev->sram_pool, vb->size,
> >> > >-					       &vb->daddr);
> >> > >-		if (!vb->vaddr)
> >> > >-			vb->size = 0;
> >> > >+	size = gen_pool_size(vpu_dev->sram_pool);
> >> > >+	vaddr = gen_pool_dma_alloc(vpu_dev->sram_pool, size, &daddr);
> >> > >+	if (vaddr) {
> >> > >+		vb->vaddr = vaddr;
> >> > >+		vb->daddr = daddr;
> >> > >+		vb->size = size;
> >> > > 	}
> >> > >
> >> > > 	dev_dbg(vpu_dev->dev, "%s: sram daddr: %pad, size: %zu, vaddr:
> >> > >0x%p\n",
> >> > >@@ -197,9 +200,7 @@ void wave5_vdi_free_sram(struct vpu_device
> >*vpu_dev)
> >> > > 	if (!vb->size || !vb->vaddr)
> >> > > 		return;
> >> > >
> >> > >-	if (vb->vaddr)
> >> > >-		gen_pool_free(vpu_dev->sram_pool, (unsigned long)vb->vaddr,
> >> > >-			      vb->size);
> >> > >+	gen_pool_free(vpu_dev->sram_pool, (unsigned long)vb->vaddr, vb-
> >> > >>size);
> >> > >
> >> > > 	memset(vb, 0, sizeof(*vb));
> >> > > }
> >> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-
> >dec.c
> >> > >b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> >> > >index aa0401f35d32..84dbe56216ad 100644
> >> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> >> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> >> > >@@ -1854,8 +1854,6 @@ static int wave5_vpu_open_dec(struct file
> >*filp)
> >> > > 		goto cleanup_inst;
> >> > > 	}
> >> > >
> >> > >-	wave5_vdi_allocate_sram(inst->dev);
> >> > >-
> >> > > 	return 0;
> >> > >
> >> > > cleanup_inst:
> >> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-
> >enc.c
> >> > >b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> >> > >index 8bbf9d10b467..86ddcb82443b 100644
> >> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> >> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> >> > >@@ -1727,8 +1727,6 @@ static int wave5_vpu_open_enc(struct file
> >*filp)
> >> > > 		goto cleanup_inst;
> >> > > 	}
> >> > >
> >> > >-	wave5_vdi_allocate_sram(inst->dev);
> >> > >-
> >> > > 	return 0;
> >> > >
> >> > > cleanup_inst:
> >> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> >> > >b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> >> > >index f3ecadefd37a..2a0a70dd7062 100644
> >> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> >> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> >> > >@@ -178,16 +178,11 @@ static int wave5_vpu_probe(struct
> >platform_device
> >> > >*pdev)
> >> > > 		return ret;
> >> > > 	}
> >> > >
> >> > >-	ret = of_property_read_u32(pdev->dev.of_node, "sram-size",
> >> > >-				   &dev->sram_size);
> >> > >-	if (ret) {
> >> > >-		dev_warn(&pdev->dev, "sram-size not found\n");
> >> > >-		dev->sram_size = 0;
> >> > >-	}
> >> > >-
> >> >
> >> > Required SRAM size is different from each wave5 product.
> >> > And, SoC vendor also can configure the different SRAM size
> >> > depend on target SoC specification even they use the same wave5
> >product.
> >> >
> >>
> >> One can limit iomem address range in SRAM node. Here is the example of
> >> how I setup Wave515 with SRAM:
> >>
> >> 	sram@2000000 {
> >> 		compatible = "mmio-sram";
> >> 		reg = <0x0 0x2000000 0x0 0x80000>;
> >> 		#address-cells = <1>;
> >> 		#size-cells = <1>;
> >> 		ranges = <0x0 0x0 0x2000000 0x80000>;
> >>
> >> 		wave515_vpu_sram: wave515-vpu-sram@0 {
> >> 			reg = <0x0 0x80000>;
> >> 			pool;
> >> 		};
> >> 	};
> >>
> >> 	wave515@410000 {
> >> 		compatible = "cnm,wave515";
> >> 		reg = <0x0 0x410000 0x0 0x10000>;
> >> 		clocks = <&clk_ref1>;
> >> 		clock-names = "videc";
> >> 		interrupt-parent = <&wave515_intc>;
> >> 		interrupts = <16 IRQ_TYPE_LEVEL_HIGH>;
> >> 		resets = <&wave515_reset 0>,
> >> 			 <&wave515_reset 4>,
> >> 			 <&wave515_reset 8>,
> >> 			 <&wave515_reset 12>;
> >> 		sram = <&wave515_vpu_sram>;
> >> 	};
> >>
> >> gen_pool_size() returns size of wave515_vpu_sram, no need for extra
> >> "sram-size" property.
> 
> Thanks for sharing the example.
> I agree that the "sram-size" property is not needed.
> 
> >
> >"sram-size" property does need to be removed, as this was the consensus
> >gathered from my patch[0]. However, I think your method is still taking
> 
> I missed the previous consensus for the sram-size property.
> Thanks for letting me know.
> 
> >a more static approach. One of the recommendations in my thread[1] was
> >making a list of known SRAM sizes given typical resolutions and
> >iterating through until a valid allocation is done. I don't think this
> >is the correct approach either based on Nas's comment that each Wave5
> >has different SRAM size requirement. It would clutter up the file too
> >much if each wave5 product had its own SRAM size mapping.
> >
> >Could another approach be to change Wave5 dts node to have property set
> >as "sram = <&sram>;" in your example, then driver calls
> >gen_pool_availble to get size remaining? From there, a check could be
> >put in place to make sure an unnecessary amount is not being allocated.
> 
> Ivan's approach looks good to me.
> It is similar to your first patch, which adds the sram-size property
> to configure different SRAM sizes for each device.
> And, Driver won't know unnecessary amount is allocated before parsing
> bitstream header.
> 

To sum up, there is 2 favourable approaches:

1) to have dedicated SRAM partition for Wave5 VPU as suggested in this
patchset. In this approach SoC vendor can setup address range of said
partition to their needs, but other devices won't be able to use SRAM
memory reserved for Wave5 VPU, unless other device's SRAM memory needs
don't exceed the size of reserved partition.

Therefore it is sensible to substitute alloc/free on open/close with
alloc/free on open/close.

Advantages: driver code is simpler, no need for platform-specific defines
or DT properties. Wave5 is guaranteed to get SRAM memory.

Disadvantage: waste of SRAM memory while VPU is not in use

2) allocate all available SRAM memory on open (free on close) from the
common SRAM pool, but limit maximum amount with SoC-specific define.

Advantage: less memory waste

Disadvantages: still need SoC-specific define or DT property, not much
differ from current state. Wave5 is not guaranteed to get SRAM memory.

Which of these approaches would be preferable?

> >
> >
> >[0]:
> >https://lore.kernel.org/lkml/99bf4d6d988d426492fffc8de9015751c323bd8a.cam
> >el@ndufresne.ca/
> >[1]: https://lore.kernel.org/lkml/9c5b7b2c-8a66-4173-dfe9-
> >5724ec5f733d@ti.com/
> >
> >Thanks,
> >Brandon
> >>
> >> > Thanks.
> >> > Nas.
> >> >
> >> > > 	dev->sram_pool = of_gen_pool_get(pdev->dev.of_node, "sram", 0);
> >> > > 	if (!dev->sram_pool)
> >> > > 		dev_warn(&pdev->dev, "sram node not found\n");
> >> > >+	else
> >> > >+		wave5_vdi_allocate_sram(dev);
> >> > >
> >> > > 	dev->product_code = wave5_vdi_read_register(dev,
> >> > >VPU_PRODUCT_CODE_REGISTER);
> >> > > 	ret = wave5_vdi_init(&pdev->dev);
> >> > >@@ -259,6 +254,8 @@ static int wave5_vpu_probe(struct
> >platform_device
> >> > >*pdev)
> >> > > err_clk_dis:
> >> > > 	clk_bulk_disable_unprepare(dev->num_clks, dev->clks);
> >> > >
> >> > >+	wave5_vdi_free_sram(dev);
> >> > >+
> >> > > 	return ret;
> >> > > }
> >> > >
> >> > >@@ -275,6 +272,7 @@ static void wave5_vpu_remove(struct
> >platform_device
> >> > >*pdev)
> >> > > 	v4l2_device_unregister(&dev->v4l2_dev);
> >> > > 	wave5_vdi_release(&pdev->dev);
> >> > > 	ida_destroy(&dev->inst_ida);
> >> > >+	wave5_vdi_free_sram(dev);
> >> > > }
> >> > >
> >> > > static const struct wave5_match_data ti_wave521c_data = {
> >> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> >> > >b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> >> > >index fa62a85080b5..8d88381ac55e 100644
> >> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> >> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> >> > >@@ -749,7 +749,6 @@ struct vpu_device {
> >> > > 	struct vpu_attr attr;
> >> > > 	struct vpu_buf common_mem;
> >> > > 	u32 last_performance_cycles;
> >> > >-	u32 sram_size;
> >> > > 	struct gen_pool *sram_pool;
> >> > > 	struct vpu_buf sram_buf;
> >> > > 	void __iomem *vdb_register;
> >> > >--
> >> > >2.44.0
> >> >
> >>

Re: Re: RE: [PATCH 5/6] media: chips-media: wave5: refine SRAM usage

[Ivan Bornyakov]

On Thu, Mar 21, 2024 at 01:52:03PM +0300, Ivan Bornyakov wrote:
> Hi!
> 
> To sum up, there is 2 favourable approaches:
> 
> 1) to have dedicated SRAM partition for Wave5 VPU as suggested in this
> patchset. In this approach SoC vendor can setup address range of said
> partition to their needs, but other devices won't be able to use SRAM
> memory reserved for Wave5 VPU, unless other device's SRAM memory needs
> don't exceed the size of reserved partition.
> 
> Therefore it is sensible to substitute alloc/free on open/close with
> alloc/free on open/close.
> 
> Advantages: driver code is simpler, no need for platform-specific defines
> or DT properties. Wave5 is guaranteed to get SRAM memory.
> 
> Disadvantage: waste of SRAM memory while VPU is not in use
> 
> 2) allocate all available SRAM memory on open (free on close) from the
> common SRAM pool, but limit maximum amount with SoC-specific define.
> 
> Advantage: less memory waste
> 
> Disadvantages: still need SoC-specific define or DT property, not much
> differ from current state. Wave5 is not guaranteed to get SRAM memory.
> 
> Which of these approaches would be preferable?
> 

Personaly I would say, let's stick with simpler code while there are not
too much mainline users. When someone runs into SRAM insufficiency
because of Wave5 VPU driver, their patches will be welcomed :)

Re: [PATCH 5/6] media: chips-media: wave5: refine SRAM usage

[Brandon Brnich]

Hi Ivan, 

On 13:52-20240321, Ivan Bornyakov wrote:
> Hi!
> 
> On Thu, Mar 21, 2024 at 09:29:04AM +0000, Nas Chung wrote:
> > Hi, Ivan and Brandon.
> > 
> > >-----Original Message-----
> > >On 14:24-20240319, Ivan Bornyakov wrote:
> > >> Hello, Nas
> > >>
> > >> On Tue, Mar 19, 2024 at 10:56:22AM +0000, Nas Chung wrote:
> > >> > Hi, Ivan.
> > >> >
> > >> > >
> > >> > >Allocate SRAM memory on module probe, free on remove. There is no
> > >need
> > >> > >to allocate on device open, free on close, the memory is the same
> > >every
> > >> > >time.
> > >> >
> > >> > If there is no decoder/encoder instance, driver don't need to
> > >allocate SRAM memory.
> > >> > The main reason of allocating the memory in open() is to allow other
> > >modules to
> > >> > use more SRAM memory, if wave5 is not working.
> > >
> > >I have to agree with this statement. Moving allocation to probe results
> > >in wasting SRAM when VPU is not in use. VPU should only be allocating
> > >SRAM
> > >when a stream instance is running and free that back once all instances
> > >close.
> > >
> > >> > >
> > >> > >Also use gen_pool_size() to determine SRAM memory size to be
> > >allocated
> > >> > >instead of separate "sram-size" DT property to reduce duplication.
> > >> > >
> > >> > >Signed-off-by: Ivan Bornyakov <brnkv.i1@gmail.com>
> > >> > >---
> > >> > > .../platform/chips-media/wave5/wave5-helper.c |  3 ---
> > >> > > .../platform/chips-media/wave5/wave5-vdi.c    | 21 ++++++++++-------
> > >--
> > >> > > .../chips-media/wave5/wave5-vpu-dec.c         |  2 --
> > >> > > .../chips-media/wave5/wave5-vpu-enc.c         |  2 --
> > >> > > .../platform/chips-media/wave5/wave5-vpu.c    | 12 +++++------
> > >> > > .../platform/chips-media/wave5/wave5-vpuapi.h |  1 -
> > >> > > 6 files changed, 16 insertions(+), 25 deletions(-)
> > >> > >
> > >> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-helper.c
> > >> > >b/drivers/media/platform/chips-media/wave5/wave5-helper.c
> > >> > >index 8433ecab230c..ec710b838dfe 100644
> > >> > >--- a/drivers/media/platform/chips-media/wave5/wave5-helper.c
> > >> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-helper.c
> > >> > >@@ -29,9 +29,6 @@ void wave5_cleanup_instance(struct vpu_instance
> > >*inst)
> > >> > > {
> > >> > > 	int i;
> > >> > >
> > >> > >-	if (list_is_singular(&inst->list))
> > >> > >-		wave5_vdi_free_sram(inst->dev);
> > >> > >-
> > >> > > 	for (i = 0; i < inst->fbc_buf_count; i++)
> > >> > > 		wave5_vpu_dec_reset_framebuffer(inst, i);
> > >> > >
> > >> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> > >> > >b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> > >> > >index 3809f70bc0b4..ee671f5a2f37 100644
> > >> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> > >> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> > >> > >@@ -174,16 +174,19 @@ int wave5_vdi_allocate_array(struct vpu_device
> > >> > >*vpu_dev, struct vpu_buf *array,
> > >> > > void wave5_vdi_allocate_sram(struct vpu_device *vpu_dev)
> > >> > > {
> > >> > > 	struct vpu_buf *vb = &vpu_dev->sram_buf;
> > >> > >+	dma_addr_t daddr;
> > >> > >+	void *vaddr;
> > >> > >+	size_t size;
> > >> > >
> > >> > >-	if (!vpu_dev->sram_pool || !vpu_dev->sram_size)
> > >> > >+	if (!vpu_dev->sram_pool || vb->vaddr)
> > >> > > 		return;
> > >> > >
> > >> > >-	if (!vb->vaddr) {
> > >> > >-		vb->size = vpu_dev->sram_size;
> > >> > >-		vb->vaddr = gen_pool_dma_alloc(vpu_dev->sram_pool, vb->size,
> > >> > >-					       &vb->daddr);
> > >> > >-		if (!vb->vaddr)
> > >> > >-			vb->size = 0;
> > >> > >+	size = gen_pool_size(vpu_dev->sram_pool);
> > >> > >+	vaddr = gen_pool_dma_alloc(vpu_dev->sram_pool, size, &daddr);
> > >> > >+	if (vaddr) {
> > >> > >+		vb->vaddr = vaddr;
> > >> > >+		vb->daddr = daddr;
> > >> > >+		vb->size = size;
> > >> > > 	}
> > >> > >
> > >> > > 	dev_dbg(vpu_dev->dev, "%s: sram daddr: %pad, size: %zu, vaddr:
> > >> > >0x%p\n",
> > >> > >@@ -197,9 +200,7 @@ void wave5_vdi_free_sram(struct vpu_device
> > >*vpu_dev)
> > >> > > 	if (!vb->size || !vb->vaddr)
> > >> > > 		return;
> > >> > >
> > >> > >-	if (vb->vaddr)
> > >> > >-		gen_pool_free(vpu_dev->sram_pool, (unsigned long)vb->vaddr,
> > >> > >-			      vb->size);
> > >> > >+	gen_pool_free(vpu_dev->sram_pool, (unsigned long)vb->vaddr, vb-
> > >> > >>size);
> > >> > >
> > >> > > 	memset(vb, 0, sizeof(*vb));
> > >> > > }
> > >> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-
> > >dec.c
> > >> > >b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> > >> > >index aa0401f35d32..84dbe56216ad 100644
> > >> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> > >> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> > >> > >@@ -1854,8 +1854,6 @@ static int wave5_vpu_open_dec(struct file
> > >*filp)
> > >> > > 		goto cleanup_inst;
> > >> > > 	}
> > >> > >
> > >> > >-	wave5_vdi_allocate_sram(inst->dev);
> > >> > >-
> > >> > > 	return 0;
> > >> > >
> > >> > > cleanup_inst:
> > >> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-
> > >enc.c
> > >> > >b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > >> > >index 8bbf9d10b467..86ddcb82443b 100644
> > >> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > >> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > >> > >@@ -1727,8 +1727,6 @@ static int wave5_vpu_open_enc(struct file
> > >*filp)
> > >> > > 		goto cleanup_inst;
> > >> > > 	}
> > >> > >
> > >> > >-	wave5_vdi_allocate_sram(inst->dev);
> > >> > >-
> > >> > > 	return 0;
> > >> > >
> > >> > > cleanup_inst:
> > >> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > >> > >b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > >> > >index f3ecadefd37a..2a0a70dd7062 100644
> > >> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > >> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > >> > >@@ -178,16 +178,11 @@ static int wave5_vpu_probe(struct
> > >platform_device
> > >> > >*pdev)
> > >> > > 		return ret;
> > >> > > 	}
> > >> > >
> > >> > >-	ret = of_property_read_u32(pdev->dev.of_node, "sram-size",
> > >> > >-				   &dev->sram_size);
> > >> > >-	if (ret) {
> > >> > >-		dev_warn(&pdev->dev, "sram-size not found\n");
> > >> > >-		dev->sram_size = 0;
> > >> > >-	}
> > >> > >-
> > >> >
> > >> > Required SRAM size is different from each wave5 product.
> > >> > And, SoC vendor also can configure the different SRAM size
> > >> > depend on target SoC specification even they use the same wave5
> > >product.
> > >> >
> > >>
> > >> One can limit iomem address range in SRAM node. Here is the example of
> > >> how I setup Wave515 with SRAM:
> > >>
> > >> 	sram@2000000 {
> > >> 		compatible = "mmio-sram";
> > >> 		reg = <0x0 0x2000000 0x0 0x80000>;
> > >> 		#address-cells = <1>;
> > >> 		#size-cells = <1>;
> > >> 		ranges = <0x0 0x0 0x2000000 0x80000>;
> > >>
> > >> 		wave515_vpu_sram: wave515-vpu-sram@0 {
> > >> 			reg = <0x0 0x80000>;
> > >> 			pool;
> > >> 		};
> > >> 	};
> > >>
> > >> 	wave515@410000 {
> > >> 		compatible = "cnm,wave515";
> > >> 		reg = <0x0 0x410000 0x0 0x10000>;
> > >> 		clocks = <&clk_ref1>;
> > >> 		clock-names = "videc";
> > >> 		interrupt-parent = <&wave515_intc>;
> > >> 		interrupts = <16 IRQ_TYPE_LEVEL_HIGH>;
> > >> 		resets = <&wave515_reset 0>,
> > >> 			 <&wave515_reset 4>,
> > >> 			 <&wave515_reset 8>,
> > >> 			 <&wave515_reset 12>;
> > >> 		sram = <&wave515_vpu_sram>;
> > >> 	};
> > >>
> > >> gen_pool_size() returns size of wave515_vpu_sram, no need for extra
> > >> "sram-size" property.
> > 
> > Thanks for sharing the example.
> > I agree that the "sram-size" property is not needed.
> > 
> > >
> > >"sram-size" property does need to be removed, as this was the consensus
> > >gathered from my patch[0]. However, I think your method is still taking
> > 
> > I missed the previous consensus for the sram-size property.
> > Thanks for letting me know.
> > 
> > >a more static approach. One of the recommendations in my thread[1] was
> > >making a list of known SRAM sizes given typical resolutions and
> > >iterating through until a valid allocation is done. I don't think this
> > >is the correct approach either based on Nas's comment that each Wave5
> > >has different SRAM size requirement. It would clutter up the file too
> > >much if each wave5 product had its own SRAM size mapping.
> > >
> > >Could another approach be to change Wave5 dts node to have property set
> > >as "sram = <&sram>;" in your example, then driver calls
> > >gen_pool_availble to get size remaining? From there, a check could be
> > >put in place to make sure an unnecessary amount is not being allocated.
> > 
> > Ivan's approach looks good to me.
> > It is similar to your first patch, which adds the sram-size property
> > to configure different SRAM sizes for each device.
> > And, Driver won't know unnecessary amount is allocated before parsing
> > bitstream header.

I am aware of this, I should have been more specific. By unnecessary
amount, I meant something greater than the max use case for device.
Could we populate some macros that have max SRAM required for 4K stream?
There's never a need to allocate more SRAM than that for a particular
instance. If the amount available is less than that, then fine. But it
should never be greater.

> > 
> 
> To sum up, there is 2 favourable approaches:
> 
> 1) to have dedicated SRAM partition for Wave5 VPU as suggested in this
> patchset. In this approach SoC vendor can setup address range of said
> partition to their needs, but other devices won't be able to use SRAM
> memory reserved for Wave5 VPU, unless other device's SRAM memory needs
> don't exceed the size of reserved partition.
> 
> Therefore it is sensible to substitute alloc/free on open/close with
> alloc/free on open/close.

Not sure what you mean here. Were you trying to refer to your
substitution of alloc/free from open/close to probe/remove?

If that is what you mean, and the decision is a specific carveout for
SRAM, then I don't see a point in having allocation in open and close
either since Wave5 would be the only IP that could use the pool.

> 
> Advantages: driver code is simpler, no need for platform-specific defines
> or DT properties. Wave5 is guaranteed to get SRAM memory.
> 
> Disadvantage: waste of SRAM memory while VPU is not in use
> 
> 2) allocate all available SRAM memory on open (free on close) from the
> common SRAM pool, but limit maximum amount with SoC-specific define.
> 

Why does it have to be on SoC specific define? Max size required for
SRAM in a 4K case is known. A call can be made to get the size of the
pool and from there the driver can take a portion. Just make sure that
portion is less than known value for 4K. 

> Advantage: less memory waste
> 
> Disadvantages: still need SoC-specific define or DT property, not much
> differ from current state. Wave5 is not guaranteed to get SRAM memory.
> 

Wave5 does not need SRAM to function properly so it doesn't have to be
guaranteed. 

> Which of these approaches would be preferable?
> 
> > >
> > >
> > >[0]:
> > >https://lore.kernel.org/lkml/99bf4d6d988d426492fffc8de9015751c323bd8a.cam
> > >el@ndufresne.ca/
> > >[1]: https://lore.kernel.org/lkml/9c5b7b2c-8a66-4173-dfe9-
> > >5724ec5f733d@ti.com/
> > >
> > >Thanks,
> > >Brandon
> > >>
> > >> > Thanks.
> > >> > Nas.
> > >> >
> > >> > > 	dev->sram_pool = of_gen_pool_get(pdev->dev.of_node, "sram", 0);
> > >> > > 	if (!dev->sram_pool)
> > >> > > 		dev_warn(&pdev->dev, "sram node not found\n");
> > >> > >+	else
> > >> > >+		wave5_vdi_allocate_sram(dev);
> > >> > >
> > >> > > 	dev->product_code = wave5_vdi_read_register(dev,
> > >> > >VPU_PRODUCT_CODE_REGISTER);
> > >> > > 	ret = wave5_vdi_init(&pdev->dev);
> > >> > >@@ -259,6 +254,8 @@ static int wave5_vpu_probe(struct
> > >platform_device
> > >> > >*pdev)
> > >> > > err_clk_dis:
> > >> > > 	clk_bulk_disable_unprepare(dev->num_clks, dev->clks);
> > >> > >
> > >> > >+	wave5_vdi_free_sram(dev);
> > >> > >+
> > >> > > 	return ret;
> > >> > > }
> > >> > >
> > >> > >@@ -275,6 +272,7 @@ static void wave5_vpu_remove(struct
> > >platform_device
> > >> > >*pdev)
> > >> > > 	v4l2_device_unregister(&dev->v4l2_dev);
> > >> > > 	wave5_vdi_release(&pdev->dev);
> > >> > > 	ida_destroy(&dev->inst_ida);
> > >> > >+	wave5_vdi_free_sram(dev);
> > >> > > }
> > >> > >
> > >> > > static const struct wave5_match_data ti_wave521c_data = {
> > >> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> > >> > >b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> > >> > >index fa62a85080b5..8d88381ac55e 100644
> > >> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> > >> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> > >> > >@@ -749,7 +749,6 @@ struct vpu_device {
> > >> > > 	struct vpu_attr attr;
> > >> > > 	struct vpu_buf common_mem;
> > >> > > 	u32 last_performance_cycles;
> > >> > >-	u32 sram_size;
> > >> > > 	struct gen_pool *sram_pool;
> > >> > > 	struct vpu_buf sram_buf;
> > >> > > 	void __iomem *vdb_register;
> > >> > >--
> > >> > >2.44.0
> > >> >
> > >>

Re: Re: [PATCH 5/6] media: chips-media: wave5: refine SRAM usage

[Ivan Bornyakov]

On Thu, Mar 21, 2024 at 11:14:05AM -0500, Brandon Brnich wrote:
> Hi Ivan, 
> 
> On 13:52-20240321, Ivan Bornyakov wrote:
> > Hi!
> > 
> > On Thu, Mar 21, 2024 at 09:29:04AM +0000, Nas Chung wrote:
> > > Hi, Ivan and Brandon.
> > > 
> > > >-----Original Message-----
> > > >On 14:24-20240319, Ivan Bornyakov wrote:
> > > >> Hello, Nas
> > > >>
> > > >> On Tue, Mar 19, 2024 at 10:56:22AM +0000, Nas Chung wrote:
> > > >> > Hi, Ivan.
> > > >> >
> > > >> > >
> > > >> > >Allocate SRAM memory on module probe, free on remove. There is no
> > > >need
> > > >> > >to allocate on device open, free on close, the memory is the same
> > > >every
> > > >> > >time.
> > > >> >
> > > >> > If there is no decoder/encoder instance, driver don't need to
> > > >allocate SRAM memory.
> > > >> > The main reason of allocating the memory in open() is to allow other
> > > >modules to
> > > >> > use more SRAM memory, if wave5 is not working.
> > > >
> > > >I have to agree with this statement. Moving allocation to probe results
> > > >in wasting SRAM when VPU is not in use. VPU should only be allocating
> > > >SRAM
> > > >when a stream instance is running and free that back once all instances
> > > >close.
> > > >
> > > >> > >
> > > >> > >Also use gen_pool_size() to determine SRAM memory size to be
> > > >allocated
> > > >> > >instead of separate "sram-size" DT property to reduce duplication.
> > > >> > >
> > > >> > >Signed-off-by: Ivan Bornyakov <brnkv.i1@gmail.com>
> > > >> > >---
> > > >> > > .../platform/chips-media/wave5/wave5-helper.c |  3 ---
> > > >> > > .../platform/chips-media/wave5/wave5-vdi.c    | 21 ++++++++++-------
> > > >--
> > > >> > > .../chips-media/wave5/wave5-vpu-dec.c         |  2 --
> > > >> > > .../chips-media/wave5/wave5-vpu-enc.c         |  2 --
> > > >> > > .../platform/chips-media/wave5/wave5-vpu.c    | 12 +++++------
> > > >> > > .../platform/chips-media/wave5/wave5-vpuapi.h |  1 -
> > > >> > > 6 files changed, 16 insertions(+), 25 deletions(-)
> > > >> > >
> > > >> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-helper.c
> > > >> > >b/drivers/media/platform/chips-media/wave5/wave5-helper.c
> > > >> > >index 8433ecab230c..ec710b838dfe 100644
> > > >> > >--- a/drivers/media/platform/chips-media/wave5/wave5-helper.c
> > > >> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-helper.c
> > > >> > >@@ -29,9 +29,6 @@ void wave5_cleanup_instance(struct vpu_instance
> > > >*inst)
> > > >> > > {
> > > >> > > 	int i;
> > > >> > >
> > > >> > >-	if (list_is_singular(&inst->list))
> > > >> > >-		wave5_vdi_free_sram(inst->dev);
> > > >> > >-
> > > >> > > 	for (i = 0; i < inst->fbc_buf_count; i++)
> > > >> > > 		wave5_vpu_dec_reset_framebuffer(inst, i);
> > > >> > >
> > > >> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> > > >> > >b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> > > >> > >index 3809f70bc0b4..ee671f5a2f37 100644
> > > >> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> > > >> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> > > >> > >@@ -174,16 +174,19 @@ int wave5_vdi_allocate_array(struct vpu_device
> > > >> > >*vpu_dev, struct vpu_buf *array,
> > > >> > > void wave5_vdi_allocate_sram(struct vpu_device *vpu_dev)
> > > >> > > {
> > > >> > > 	struct vpu_buf *vb = &vpu_dev->sram_buf;
> > > >> > >+	dma_addr_t daddr;
> > > >> > >+	void *vaddr;
> > > >> > >+	size_t size;
> > > >> > >
> > > >> > >-	if (!vpu_dev->sram_pool || !vpu_dev->sram_size)
> > > >> > >+	if (!vpu_dev->sram_pool || vb->vaddr)
> > > >> > > 		return;
> > > >> > >
> > > >> > >-	if (!vb->vaddr) {
> > > >> > >-		vb->size = vpu_dev->sram_size;
> > > >> > >-		vb->vaddr = gen_pool_dma_alloc(vpu_dev->sram_pool, vb->size,
> > > >> > >-					       &vb->daddr);
> > > >> > >-		if (!vb->vaddr)
> > > >> > >-			vb->size = 0;
> > > >> > >+	size = gen_pool_size(vpu_dev->sram_pool);
> > > >> > >+	vaddr = gen_pool_dma_alloc(vpu_dev->sram_pool, size, &daddr);
> > > >> > >+	if (vaddr) {
> > > >> > >+		vb->vaddr = vaddr;
> > > >> > >+		vb->daddr = daddr;
> > > >> > >+		vb->size = size;
> > > >> > > 	}
> > > >> > >
> > > >> > > 	dev_dbg(vpu_dev->dev, "%s: sram daddr: %pad, size: %zu, vaddr:
> > > >> > >0x%p\n",
> > > >> > >@@ -197,9 +200,7 @@ void wave5_vdi_free_sram(struct vpu_device
> > > >*vpu_dev)
> > > >> > > 	if (!vb->size || !vb->vaddr)
> > > >> > > 		return;
> > > >> > >
> > > >> > >-	if (vb->vaddr)
> > > >> > >-		gen_pool_free(vpu_dev->sram_pool, (unsigned long)vb->vaddr,
> > > >> > >-			      vb->size);
> > > >> > >+	gen_pool_free(vpu_dev->sram_pool, (unsigned long)vb->vaddr, vb-
> > > >> > >>size);
> > > >> > >
> > > >> > > 	memset(vb, 0, sizeof(*vb));
> > > >> > > }
> > > >> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-
> > > >dec.c
> > > >> > >b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> > > >> > >index aa0401f35d32..84dbe56216ad 100644
> > > >> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> > > >> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> > > >> > >@@ -1854,8 +1854,6 @@ static int wave5_vpu_open_dec(struct file
> > > >*filp)
> > > >> > > 		goto cleanup_inst;
> > > >> > > 	}
> > > >> > >
> > > >> > >-	wave5_vdi_allocate_sram(inst->dev);
> > > >> > >-
> > > >> > > 	return 0;
> > > >> > >
> > > >> > > cleanup_inst:
> > > >> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-
> > > >enc.c
> > > >> > >b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > > >> > >index 8bbf9d10b467..86ddcb82443b 100644
> > > >> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > > >> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > > >> > >@@ -1727,8 +1727,6 @@ static int wave5_vpu_open_enc(struct file
> > > >*filp)
> > > >> > > 		goto cleanup_inst;
> > > >> > > 	}
> > > >> > >
> > > >> > >-	wave5_vdi_allocate_sram(inst->dev);
> > > >> > >-
> > > >> > > 	return 0;
> > > >> > >
> > > >> > > cleanup_inst:
> > > >> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > > >> > >b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > > >> > >index f3ecadefd37a..2a0a70dd7062 100644
> > > >> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > > >> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > > >> > >@@ -178,16 +178,11 @@ static int wave5_vpu_probe(struct
> > > >platform_device
> > > >> > >*pdev)
> > > >> > > 		return ret;
> > > >> > > 	}
> > > >> > >
> > > >> > >-	ret = of_property_read_u32(pdev->dev.of_node, "sram-size",
> > > >> > >-				   &dev->sram_size);
> > > >> > >-	if (ret) {
> > > >> > >-		dev_warn(&pdev->dev, "sram-size not found\n");
> > > >> > >-		dev->sram_size = 0;
> > > >> > >-	}
> > > >> > >-
> > > >> >
> > > >> > Required SRAM size is different from each wave5 product.
> > > >> > And, SoC vendor also can configure the different SRAM size
> > > >> > depend on target SoC specification even they use the same wave5
> > > >product.
> > > >> >
> > > >>
> > > >> One can limit iomem address range in SRAM node. Here is the example of
> > > >> how I setup Wave515 with SRAM:
> > > >>
> > > >> 	sram@2000000 {
> > > >> 		compatible = "mmio-sram";
> > > >> 		reg = <0x0 0x2000000 0x0 0x80000>;
> > > >> 		#address-cells = <1>;
> > > >> 		#size-cells = <1>;
> > > >> 		ranges = <0x0 0x0 0x2000000 0x80000>;
> > > >>
> > > >> 		wave515_vpu_sram: wave515-vpu-sram@0 {
> > > >> 			reg = <0x0 0x80000>;
> > > >> 			pool;
> > > >> 		};
> > > >> 	};
> > > >>
> > > >> 	wave515@410000 {
> > > >> 		compatible = "cnm,wave515";
> > > >> 		reg = <0x0 0x410000 0x0 0x10000>;
> > > >> 		clocks = <&clk_ref1>;
> > > >> 		clock-names = "videc";
> > > >> 		interrupt-parent = <&wave515_intc>;
> > > >> 		interrupts = <16 IRQ_TYPE_LEVEL_HIGH>;
> > > >> 		resets = <&wave515_reset 0>,
> > > >> 			 <&wave515_reset 4>,
> > > >> 			 <&wave515_reset 8>,
> > > >> 			 <&wave515_reset 12>;
> > > >> 		sram = <&wave515_vpu_sram>;
> > > >> 	};
> > > >>
> > > >> gen_pool_size() returns size of wave515_vpu_sram, no need for extra
> > > >> "sram-size" property.
> > > 
> > > Thanks for sharing the example.
> > > I agree that the "sram-size" property is not needed.
> > > 
> > > >
> > > >"sram-size" property does need to be removed, as this was the consensus
> > > >gathered from my patch[0]. However, I think your method is still taking
> > > 
> > > I missed the previous consensus for the sram-size property.
> > > Thanks for letting me know.
> > > 
> > > >a more static approach. One of the recommendations in my thread[1] was
> > > >making a list of known SRAM sizes given typical resolutions and
> > > >iterating through until a valid allocation is done. I don't think this
> > > >is the correct approach either based on Nas's comment that each Wave5
> > > >has different SRAM size requirement. It would clutter up the file too
> > > >much if each wave5 product had its own SRAM size mapping.
> > > >
> > > >Could another approach be to change Wave5 dts node to have property set
> > > >as "sram = <&sram>;" in your example, then driver calls
> > > >gen_pool_availble to get size remaining? From there, a check could be
> > > >put in place to make sure an unnecessary amount is not being allocated.
> > > 
> > > Ivan's approach looks good to me.
> > > It is similar to your first patch, which adds the sram-size property
> > > to configure different SRAM sizes for each device.
> > > And, Driver won't know unnecessary amount is allocated before parsing
> > > bitstream header.
> 
> I am aware of this, I should have been more specific. By unnecessary
> amount, I meant something greater than the max use case for device.
> Could we populate some macros that have max SRAM required for 4K stream?
> There's never a need to allocate more SRAM than that for a particular
> instance. If the amount available is less than that, then fine. But it
> should never be greater.
> 
> > > 
> > 
> > To sum up, there is 2 favourable approaches:
> > 
> > 1) to have dedicated SRAM partition for Wave5 VPU as suggested in this
> > patchset. In this approach SoC vendor can setup address range of said
> > partition to their needs, but other devices won't be able to use SRAM
> > memory reserved for Wave5 VPU, unless other device's SRAM memory needs
> > don't exceed the size of reserved partition.
> > 
> > Therefore it is sensible to substitute alloc/free on open/close with
> > alloc/free on open/close.
> 
> Not sure what you mean here. Were you trying to refer to your
> substitution of alloc/free from open/close to probe/remove?
> 
> If that is what you mean, and the decision is a specific carveout for
> SRAM, then I don't see a point in having allocation in open and close
> either since Wave5 would be the only IP that could use the pool.
> 
> > 
> > Advantages: driver code is simpler, no need for platform-specific defines
> > or DT properties. Wave5 is guaranteed to get SRAM memory.
> > 
> > Disadvantage: waste of SRAM memory while VPU is not in use
> > 
> > 2) allocate all available SRAM memory on open (free on close) from the
> > common SRAM pool, but limit maximum amount with SoC-specific define.
> > 
> 
> Why does it have to be on SoC specific define?

Well, if I understood correctly, in [1] Nas said that SRAM usage is
SoC-specific even with same Wave5 IP.

[1] https://lore.kernel.org/linux-media/SL2P216MB1246F7FA7E95896AA2409C90FB2C2@SL2P216MB1246.KORP216.PROD.OUTLOOK.COM/

> Max size required for SRAM in a 4K case is known.

From docs I have for Wave515 it's _seems_ to be about 64K, but it's not
too clear.

> A call can be made to get the size of the
> pool and from there the driver can take a portion. Just make sure that
> portion is less than known value for 4K. 
> 

Yeah, I did exactly that in v2, was about to send, until I got
"Ivan's approach looks good to me" :)

> > Advantage: less memory waste
> > 
> > Disadvantages: still need SoC-specific define or DT property, not much
> > differ from current state. Wave5 is not guaranteed to get SRAM memory.
> > 
> 
> Wave5 does not need SRAM to function properly so it doesn't have to be
> guaranteed. 
> 

True.

> > Which of these approaches would be preferable?
> > 
> > > >
> > > >
> > > >[0]:
> > > >https://lore.kernel.org/lkml/99bf4d6d988d426492fffc8de9015751c323bd8a.cam
> > > >el@ndufresne.ca/
> > > >[1]: https://lore.kernel.org/lkml/9c5b7b2c-8a66-4173-dfe9-
> > > >5724ec5f733d@ti.com/
> > > >
> > > >Thanks,
> > > >Brandon
> > > >>
> > > >> > Thanks.
> > > >> > Nas.
> > > >> >
> > > >> > > 	dev->sram_pool = of_gen_pool_get(pdev->dev.of_node, "sram", 0);
> > > >> > > 	if (!dev->sram_pool)
> > > >> > > 		dev_warn(&pdev->dev, "sram node not found\n");
> > > >> > >+	else
> > > >> > >+		wave5_vdi_allocate_sram(dev);
> > > >> > >
> > > >> > > 	dev->product_code = wave5_vdi_read_register(dev,
> > > >> > >VPU_PRODUCT_CODE_REGISTER);
> > > >> > > 	ret = wave5_vdi_init(&pdev->dev);
> > > >> > >@@ -259,6 +254,8 @@ static int wave5_vpu_probe(struct
> > > >platform_device
> > > >> > >*pdev)
> > > >> > > err_clk_dis:
> > > >> > > 	clk_bulk_disable_unprepare(dev->num_clks, dev->clks);
> > > >> > >
> > > >> > >+	wave5_vdi_free_sram(dev);
> > > >> > >+
> > > >> > > 	return ret;
> > > >> > > }
> > > >> > >
> > > >> > >@@ -275,6 +272,7 @@ static void wave5_vpu_remove(struct
> > > >platform_device
> > > >> > >*pdev)
> > > >> > > 	v4l2_device_unregister(&dev->v4l2_dev);
> > > >> > > 	wave5_vdi_release(&pdev->dev);
> > > >> > > 	ida_destroy(&dev->inst_ida);
> > > >> > >+	wave5_vdi_free_sram(dev);
> > > >> > > }
> > > >> > >
> > > >> > > static const struct wave5_match_data ti_wave521c_data = {
> > > >> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> > > >> > >b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> > > >> > >index fa62a85080b5..8d88381ac55e 100644
> > > >> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> > > >> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> > > >> > >@@ -749,7 +749,6 @@ struct vpu_device {
> > > >> > > 	struct vpu_attr attr;
> > > >> > > 	struct vpu_buf common_mem;
> > > >> > > 	u32 last_performance_cycles;
> > > >> > >-	u32 sram_size;
> > > >> > > 	struct gen_pool *sram_pool;
> > > >> > > 	struct vpu_buf sram_buf;
> > > >> > > 	void __iomem *vdb_register;
> > > >> > >--
> > > >> > >2.44.0
> > > >> >
> > > >>

Re: [PATCH 5/6] media: chips-media: wave5: refine SRAM usage

[Brandon Brnich]

On 19:41-20240321, Ivan Bornyakov wrote:
> On Thu, Mar 21, 2024 at 11:14:05AM -0500, Brandon Brnich wrote:
> > Hi Ivan, 
> > 
> > On 13:52-20240321, Ivan Bornyakov wrote:
> > > Hi!
> > > 
> > > On Thu, Mar 21, 2024 at 09:29:04AM +0000, Nas Chung wrote:
> > > > Hi, Ivan and Brandon.
> > > > 
> > > > >-----Original Message-----
> > > > >On 14:24-20240319, Ivan Bornyakov wrote:
> > > > >> Hello, Nas
> > > > >>
> > > > >> On Tue, Mar 19, 2024 at 10:56:22AM +0000, Nas Chung wrote:
> > > > >> > Hi, Ivan.
> > > > >> >
> > > > >> > >
> > > > >> > >Allocate SRAM memory on module probe, free on remove. There is no
> > > > >need
> > > > >> > >to allocate on device open, free on close, the memory is the same
> > > > >every
> > > > >> > >time.
> > > > >> >
> > > > >> > If there is no decoder/encoder instance, driver don't need to
> > > > >allocate SRAM memory.
> > > > >> > The main reason of allocating the memory in open() is to allow other
> > > > >modules to
> > > > >> > use more SRAM memory, if wave5 is not working.
> > > > >
> > > > >I have to agree with this statement. Moving allocation to probe results
> > > > >in wasting SRAM when VPU is not in use. VPU should only be allocating
> > > > >SRAM
> > > > >when a stream instance is running and free that back once all instances
> > > > >close.
> > > > >
> > > > >> > >
> > > > >> > >Also use gen_pool_size() to determine SRAM memory size to be
> > > > >allocated
> > > > >> > >instead of separate "sram-size" DT property to reduce duplication.
> > > > >> > >
> > > > >> > >Signed-off-by: Ivan Bornyakov <brnkv.i1@gmail.com>
> > > > >> > >---
> > > > >> > > .../platform/chips-media/wave5/wave5-helper.c |  3 ---
> > > > >> > > .../platform/chips-media/wave5/wave5-vdi.c    | 21 ++++++++++-------
> > > > >--
> > > > >> > > .../chips-media/wave5/wave5-vpu-dec.c         |  2 --
> > > > >> > > .../chips-media/wave5/wave5-vpu-enc.c         |  2 --
> > > > >> > > .../platform/chips-media/wave5/wave5-vpu.c    | 12 +++++------
> > > > >> > > .../platform/chips-media/wave5/wave5-vpuapi.h |  1 -
> > > > >> > > 6 files changed, 16 insertions(+), 25 deletions(-)
> > > > >> > >
> > > > >> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-helper.c
> > > > >> > >b/drivers/media/platform/chips-media/wave5/wave5-helper.c
> > > > >> > >index 8433ecab230c..ec710b838dfe 100644
> > > > >> > >--- a/drivers/media/platform/chips-media/wave5/wave5-helper.c
> > > > >> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-helper.c
> > > > >> > >@@ -29,9 +29,6 @@ void wave5_cleanup_instance(struct vpu_instance
> > > > >*inst)
> > > > >> > > {
> > > > >> > > 	int i;
> > > > >> > >
> > > > >> > >-	if (list_is_singular(&inst->list))
> > > > >> > >-		wave5_vdi_free_sram(inst->dev);
> > > > >> > >-
> > > > >> > > 	for (i = 0; i < inst->fbc_buf_count; i++)
> > > > >> > > 		wave5_vpu_dec_reset_framebuffer(inst, i);
> > > > >> > >
> > > > >> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> > > > >> > >b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> > > > >> > >index 3809f70bc0b4..ee671f5a2f37 100644
> > > > >> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> > > > >> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> > > > >> > >@@ -174,16 +174,19 @@ int wave5_vdi_allocate_array(struct vpu_device
> > > > >> > >*vpu_dev, struct vpu_buf *array,
> > > > >> > > void wave5_vdi_allocate_sram(struct vpu_device *vpu_dev)
> > > > >> > > {
> > > > >> > > 	struct vpu_buf *vb = &vpu_dev->sram_buf;
> > > > >> > >+	dma_addr_t daddr;
> > > > >> > >+	void *vaddr;
> > > > >> > >+	size_t size;
> > > > >> > >
> > > > >> > >-	if (!vpu_dev->sram_pool || !vpu_dev->sram_size)
> > > > >> > >+	if (!vpu_dev->sram_pool || vb->vaddr)
> > > > >> > > 		return;
> > > > >> > >
> > > > >> > >-	if (!vb->vaddr) {
> > > > >> > >-		vb->size = vpu_dev->sram_size;
> > > > >> > >-		vb->vaddr = gen_pool_dma_alloc(vpu_dev->sram_pool, vb->size,
> > > > >> > >-					       &vb->daddr);
> > > > >> > >-		if (!vb->vaddr)
> > > > >> > >-			vb->size = 0;
> > > > >> > >+	size = gen_pool_size(vpu_dev->sram_pool);
> > > > >> > >+	vaddr = gen_pool_dma_alloc(vpu_dev->sram_pool, size, &daddr);
> > > > >> > >+	if (vaddr) {
> > > > >> > >+		vb->vaddr = vaddr;
> > > > >> > >+		vb->daddr = daddr;
> > > > >> > >+		vb->size = size;
> > > > >> > > 	}
> > > > >> > >
> > > > >> > > 	dev_dbg(vpu_dev->dev, "%s: sram daddr: %pad, size: %zu, vaddr:
> > > > >> > >0x%p\n",
> > > > >> > >@@ -197,9 +200,7 @@ void wave5_vdi_free_sram(struct vpu_device
> > > > >*vpu_dev)
> > > > >> > > 	if (!vb->size || !vb->vaddr)
> > > > >> > > 		return;
> > > > >> > >
> > > > >> > >-	if (vb->vaddr)
> > > > >> > >-		gen_pool_free(vpu_dev->sram_pool, (unsigned long)vb->vaddr,
> > > > >> > >-			      vb->size);
> > > > >> > >+	gen_pool_free(vpu_dev->sram_pool, (unsigned long)vb->vaddr, vb-
> > > > >> > >>size);
> > > > >> > >
> > > > >> > > 	memset(vb, 0, sizeof(*vb));
> > > > >> > > }
> > > > >> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-
> > > > >dec.c
> > > > >> > >b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> > > > >> > >index aa0401f35d32..84dbe56216ad 100644
> > > > >> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> > > > >> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> > > > >> > >@@ -1854,8 +1854,6 @@ static int wave5_vpu_open_dec(struct file
> > > > >*filp)
> > > > >> > > 		goto cleanup_inst;
> > > > >> > > 	}
> > > > >> > >
> > > > >> > >-	wave5_vdi_allocate_sram(inst->dev);
> > > > >> > >-
> > > > >> > > 	return 0;
> > > > >> > >
> > > > >> > > cleanup_inst:
> > > > >> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-
> > > > >enc.c
> > > > >> > >b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > > > >> > >index 8bbf9d10b467..86ddcb82443b 100644
> > > > >> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > > > >> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > > > >> > >@@ -1727,8 +1727,6 @@ static int wave5_vpu_open_enc(struct file
> > > > >*filp)
> > > > >> > > 		goto cleanup_inst;
> > > > >> > > 	}
> > > > >> > >
> > > > >> > >-	wave5_vdi_allocate_sram(inst->dev);
> > > > >> > >-
> > > > >> > > 	return 0;
> > > > >> > >
> > > > >> > > cleanup_inst:
> > > > >> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > > > >> > >b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > > > >> > >index f3ecadefd37a..2a0a70dd7062 100644
> > > > >> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > > > >> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > > > >> > >@@ -178,16 +178,11 @@ static int wave5_vpu_probe(struct
> > > > >platform_device
> > > > >> > >*pdev)
> > > > >> > > 		return ret;
> > > > >> > > 	}
> > > > >> > >
> > > > >> > >-	ret = of_property_read_u32(pdev->dev.of_node, "sram-size",
> > > > >> > >-				   &dev->sram_size);
> > > > >> > >-	if (ret) {
> > > > >> > >-		dev_warn(&pdev->dev, "sram-size not found\n");
> > > > >> > >-		dev->sram_size = 0;
> > > > >> > >-	}
> > > > >> > >-
> > > > >> >
> > > > >> > Required SRAM size is different from each wave5 product.
> > > > >> > And, SoC vendor also can configure the different SRAM size
> > > > >> > depend on target SoC specification even they use the same wave5
> > > > >product.
> > > > >> >
> > > > >>
> > > > >> One can limit iomem address range in SRAM node. Here is the example of
> > > > >> how I setup Wave515 with SRAM:
> > > > >>
> > > > >> 	sram@2000000 {
> > > > >> 		compatible = "mmio-sram";
> > > > >> 		reg = <0x0 0x2000000 0x0 0x80000>;
> > > > >> 		#address-cells = <1>;
> > > > >> 		#size-cells = <1>;
> > > > >> 		ranges = <0x0 0x0 0x2000000 0x80000>;
> > > > >>
> > > > >> 		wave515_vpu_sram: wave515-vpu-sram@0 {
> > > > >> 			reg = <0x0 0x80000>;
> > > > >> 			pool;
> > > > >> 		};
> > > > >> 	};
> > > > >>
> > > > >> 	wave515@410000 {
> > > > >> 		compatible = "cnm,wave515";
> > > > >> 		reg = <0x0 0x410000 0x0 0x10000>;
> > > > >> 		clocks = <&clk_ref1>;
> > > > >> 		clock-names = "videc";
> > > > >> 		interrupt-parent = <&wave515_intc>;
> > > > >> 		interrupts = <16 IRQ_TYPE_LEVEL_HIGH>;
> > > > >> 		resets = <&wave515_reset 0>,
> > > > >> 			 <&wave515_reset 4>,
> > > > >> 			 <&wave515_reset 8>,
> > > > >> 			 <&wave515_reset 12>;
> > > > >> 		sram = <&wave515_vpu_sram>;
> > > > >> 	};
> > > > >>
> > > > >> gen_pool_size() returns size of wave515_vpu_sram, no need for extra
> > > > >> "sram-size" property.
> > > > 
> > > > Thanks for sharing the example.
> > > > I agree that the "sram-size" property is not needed.
> > > > 
> > > > >
> > > > >"sram-size" property does need to be removed, as this was the consensus
> > > > >gathered from my patch[0]. However, I think your method is still taking
> > > > 
> > > > I missed the previous consensus for the sram-size property.
> > > > Thanks for letting me know.
> > > > 
> > > > >a more static approach. One of the recommendations in my thread[1] was
> > > > >making a list of known SRAM sizes given typical resolutions and
> > > > >iterating through until a valid allocation is done. I don't think this
> > > > >is the correct approach either based on Nas's comment that each Wave5
> > > > >has different SRAM size requirement. It would clutter up the file too
> > > > >much if each wave5 product had its own SRAM size mapping.
> > > > >
> > > > >Could another approach be to change Wave5 dts node to have property set
> > > > >as "sram = <&sram>;" in your example, then driver calls
> > > > >gen_pool_availble to get size remaining? From there, a check could be
> > > > >put in place to make sure an unnecessary amount is not being allocated.
> > > > 
> > > > Ivan's approach looks good to me.
> > > > It is similar to your first patch, which adds the sram-size property
> > > > to configure different SRAM sizes for each device.
> > > > And, Driver won't know unnecessary amount is allocated before parsing
> > > > bitstream header.
> > 
> > I am aware of this, I should have been more specific. By unnecessary
> > amount, I meant something greater than the max use case for device.
> > Could we populate some macros that have max SRAM required for 4K stream?
> > There's never a need to allocate more SRAM than that for a particular
> > instance. If the amount available is less than that, then fine. But it
> > should never be greater.
> > 
> > > > 
> > > 
> > > To sum up, there is 2 favourable approaches:
> > > 
> > > 1) to have dedicated SRAM partition for Wave5 VPU as suggested in this
> > > patchset. In this approach SoC vendor can setup address range of said
> > > partition to their needs, but other devices won't be able to use SRAM
> > > memory reserved for Wave5 VPU, unless other device's SRAM memory needs
> > > don't exceed the size of reserved partition.
> > > 
> > > Therefore it is sensible to substitute alloc/free on open/close with
> > > alloc/free on open/close.
> > 
> > Not sure what you mean here. Were you trying to refer to your
> > substitution of alloc/free from open/close to probe/remove?
> > 
> > If that is what you mean, and the decision is a specific carveout for
> > SRAM, then I don't see a point in having allocation in open and close
> > either since Wave5 would be the only IP that could use the pool.
> > 
> > > 
> > > Advantages: driver code is simpler, no need for platform-specific defines
> > > or DT properties. Wave5 is guaranteed to get SRAM memory.
> > > 
> > > Disadvantage: waste of SRAM memory while VPU is not in use
> > > 
> > > 2) allocate all available SRAM memory on open (free on close) from the
> > > common SRAM pool, but limit maximum amount with SoC-specific define.
> > > 
> > 
> > Why does it have to be on SoC specific define?
> 
> Well, if I understood correctly, in [1] Nas said that SRAM usage is
> SoC-specific even with same Wave5 IP.
>

I interpreted this as different Wave5 variants have varying SRAM
requirements. For ex, Wave521lc vs Wave515. If two SoCs have same
variant, the required SRAM won't change from Wave5 perspective. The size
would only really change based on how much SRAM is available on that
particular SoC. 
 
> [1] https://lore.kernel.org/linux-media/SL2P216MB1246F7FA7E95896AA2409C90FB2C2@SL2P216MB1246.KORP216.PROD.OUTLOOK.COM/
> 
> > Max size required for SRAM in a 4K case is known.
> 
> From docs I have for Wave515 it's _seems_ to be about 64K, but it's not
> too clear.

I will let Nas comment on this, but 64K also sounds familiar to me.

> 
> > A call can be made to get the size of the
> > pool and from there the driver can take a portion. Just make sure that
> > portion is less than known value for 4K. 
> > 
> 
> Yeah, I did exactly that in v2, was about to send, until I got
> "Ivan's approach looks good to me" :)
> 
> > > Advantage: less memory waste
> > > 
> > > Disadvantages: still need SoC-specific define or DT property, not much
> > > differ from current state. Wave5 is not guaranteed to get SRAM memory.
> > > 
> > 
> > Wave5 does not need SRAM to function properly so it doesn't have to be
> > guaranteed. 
> > 
> 
> True.
> 
> > > Which of these approaches would be preferable?
> > > 
> > > > >
> > > > >
> > > > >[0]:
> > > > >https://lore.kernel.org/lkml/99bf4d6d988d426492fffc8de9015751c323bd8a.cam
> > > > >el@ndufresne.ca/
> > > > >[1]: https://lore.kernel.org/lkml/9c5b7b2c-8a66-4173-dfe9-
> > > > >5724ec5f733d@ti.com/
> > > > >
> > > > >Thanks,
> > > > >Brandon
> > > > >>
> > > > >> > Thanks.
> > > > >> > Nas.
> > > > >> >
> > > > >> > > 	dev->sram_pool = of_gen_pool_get(pdev->dev.of_node, "sram", 0);
> > > > >> > > 	if (!dev->sram_pool)
> > > > >> > > 		dev_warn(&pdev->dev, "sram node not found\n");
> > > > >> > >+	else
> > > > >> > >+		wave5_vdi_allocate_sram(dev);
> > > > >> > >
> > > > >> > > 	dev->product_code = wave5_vdi_read_register(dev,
> > > > >> > >VPU_PRODUCT_CODE_REGISTER);
> > > > >> > > 	ret = wave5_vdi_init(&pdev->dev);
> > > > >> > >@@ -259,6 +254,8 @@ static int wave5_vpu_probe(struct
> > > > >platform_device
> > > > >> > >*pdev)
> > > > >> > > err_clk_dis:
> > > > >> > > 	clk_bulk_disable_unprepare(dev->num_clks, dev->clks);
> > > > >> > >
> > > > >> > >+	wave5_vdi_free_sram(dev);
> > > > >> > >+
> > > > >> > > 	return ret;
> > > > >> > > }
> > > > >> > >
> > > > >> > >@@ -275,6 +272,7 @@ static void wave5_vpu_remove(struct
> > > > >platform_device
> > > > >> > >*pdev)
> > > > >> > > 	v4l2_device_unregister(&dev->v4l2_dev);
> > > > >> > > 	wave5_vdi_release(&pdev->dev);
> > > > >> > > 	ida_destroy(&dev->inst_ida);
> > > > >> > >+	wave5_vdi_free_sram(dev);
> > > > >> > > }
> > > > >> > >
> > > > >> > > static const struct wave5_match_data ti_wave521c_data = {
> > > > >> > >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> > > > >> > >b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> > > > >> > >index fa62a85080b5..8d88381ac55e 100644
> > > > >> > >--- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> > > > >> > >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> > > > >> > >@@ -749,7 +749,6 @@ struct vpu_device {
> > > > >> > > 	struct vpu_attr attr;
> > > > >> > > 	struct vpu_buf common_mem;
> > > > >> > > 	u32 last_performance_cycles;
> > > > >> > >-	u32 sram_size;
> > > > >> > > 	struct gen_pool *sram_pool;
> > > > >> > > 	struct vpu_buf sram_buf;
> > > > >> > > 	void __iomem *vdb_register;
> > > > >> > >--
> > > > >> > >2.44.0
> > > > >> >
> > > > >>