Received-SPF: pass (google.com: domain of linux-kernel+bounces-122837-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 28 Mar 2024 14:36:10 +0300
From: Ivan Bornyakov <brnkv.i1@gmail.com>
To: Nas Chung <nas.chung@chipsnmedia.com>
Cc: "linux-media@vger.kernel.org" <linux-media@vger.kernel.org>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "jackson.lee" <jackson.lee@chipsnmedia.com>, 
	Mauro Carvalho Chehab <mchehab@kernel.org>, Philipp Zabel <p.zabel@pengutronix.de>
Subject: Re: RE: RE: [PATCH v2 4/5] media: chips-media: wave5: drop
 "sram-size" DT prop
Message-ID: <7lro7ryzjajpzvkjdj5cndkmzmjkqvhts6dmer4pkkt7dluiii@wp7ael4ae7jd>
References: <20240325064102.9278-1-brnkv.i1@gmail.com>
 <20240325064102.9278-5-brnkv.i1@gmail.com>
 <SL2P216MB1246537DD623B813453B28F9FB342@SL2P216MB1246.KORP216.PROD.OUTLOOK.COM>
 <5hd7duzqhgdxpmvom3opkhwxkq55dmitk4gwdl4dy46q662in6@xxkmvdj6plqb>
 <SL2P216MB1246499CC9FED9BFB5B11DA3FB3B2@SL2P216MB1246.KORP216.PROD.OUTLOOK.COM>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <SL2P216MB1246499CC9FED9BFB5B11DA3FB3B2@SL2P216MB1246.KORP216.PROD.OUTLOOK.COM>

On Thu, Mar 28, 2024 at 10:16:53AM +0000, Nas Chung wrote:
> Hi, Ivan.
> 
> >-----Original Message-----
> >From: Ivan Bornyakov <brnkv.i1@gmail.com>
> >Sent: Wednesday, March 27, 2024 9:27 PM
> >To: Nas Chung <nas.chung@chipsnmedia.com>
> >Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> >jackson.lee <jackson.lee@chipsnmedia.com>; Mauro Carvalho Chehab
> ><mchehab@kernel.org>; Philipp Zabel <p.zabel@pengutronix.de>
> >Subject: Re: RE: [PATCH v2 4/5] media: chips-media: wave5: drop "sram-
> >size" DT prop
> >
> >On Wed, Mar 27, 2024 at 10:27:19AM +0000, Nas Chung wrote:
> >> Hi, Ivan.
> >>
> >> >-----Original Message-----
> >> >From: Ivan Bornyakov <brnkv.i1@gmail.com>
> >> >Sent: Monday, March 25, 2024 3:41 PM
> >> >To: Nas Chung <nas.chung@chipsnmedia.com>; jackson.lee
> >> ><jackson.lee@chipsnmedia.com>; Mauro Carvalho Chehab
> ><mchehab@kernel.org>;
> >> >Philipp Zabel <p.zabel@pengutronix.de>
> >> >Cc: Ivan Bornyakov <brnkv.i1@gmail.com>; linux-media@vger.kernel.org;
> >> >linux-kernel@vger.kernel.org
> >> >Subject: [PATCH v2 4/5] media: chips-media: wave5: drop "sram-size" DT
> >> >prop
> >> >
> >> >Use all available SRAM memory up to WAVE5_MAX_SRAM_SIZE. Remove
> >> >excessive "sram-size" device-tree property as genalloc is already able
> >> >to determine available memory.
> >> >
> >> >Signed-off-by: Ivan Bornyakov <brnkv.i1@gmail.com>
> >> >---
> >> > .../platform/chips-media/wave5/wave5-vdi.c    | 21 ++++++++++---------
> >> > .../platform/chips-media/wave5/wave5-vpu.c    |  7 -------
> >> > .../platform/chips-media/wave5/wave5-vpuapi.h |  1 -
> >> > .../chips-media/wave5/wave5-vpuconfig.h       |  2 ++
> >> > 4 files changed, 13 insertions(+), 18 deletions(-)
> >> >
> >> >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> >> >b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> >> >index 3809f70bc0b4..a63fffed55e9 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 = min_t(size_t, WAVE5_MAX_SRAM_SIZE, gen_pool_avail(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.c
> >> >b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> >> >index 1e631da58e15..2a972cddf4a6 100644
> >> >--- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> >> >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> >> >@@ -177,13 +177,6 @@ static int wave5_vpu_probe(struct platform_device
> >> >*pdev)
> >> > 		goto err_reset_assert;
> >> > 	}
> >> >
> >> >-	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");
> >> >diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> >> >b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> >> >index da530fd98964..975d96b22191 100644
> >> >--- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> >> >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
> >> >@@ -750,7 +750,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;
> >> >diff --git a/drivers/media/platform/chips-media/wave5/wave5-
> >vpuconfig.h
> >> >b/drivers/media/platform/chips-media/wave5/wave5-vpuconfig.h
> >> >index d9751eedb0f9..9d99afb78c89 100644
> >> >--- a/drivers/media/platform/chips-media/wave5/wave5-vpuconfig.h
> >> >+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuconfig.h
> >> >@@ -28,6 +28,8 @@
> >> > #define WAVE521ENC_WORKBUF_SIZE         (128 * 1024)      //HEVC 128K,
> >AVC
> >> >40K
> >> > #define WAVE521DEC_WORKBUF_SIZE         (1784 * 1024)
> >> >
> >> >+#define WAVE5_MAX_SRAM_SIZE		(64 * 1024)
> >>
> >> WAVE521 can support 8K stream decoding/encoding.
> >> So, I suggest the MAX_SRAME_SIZE to 128 * 1024 (128KB).
> >>
> >> And, Current driver always enable sec_axi_info option if sram buffer is
> >allocated.
> >> But, we have to enable/disable the sec_axi_info option after checking
> >the allocated sram size is enough to decode/encode current bitstream
> >resolution.
> >
> >Do we really? As an experiment I tried to provide to Wave515 1KB of SRAM
> >memory and decoded 4k sample file was fine...
> >
> 
> You can think It seems like driver works fine.
> But, This is not the behavior we expect.
> There is a possibility that unexpected problems may occur.
> 

Ok, then we either

 1) don't try to allocate any availible SRAM memory up to
    match_data->sram_size, but allocate exact match_data->sram_size

or

 2) allocate any available SRAM memory up to match_data->sram_size, but
    check for allocated size before writing to registers W5_USE_SEC_AXI
    and W5_CMD_ENC_PIC_USE_SEC_AXI

With second variant I won't be able to add said check for Wave521, as I
don't know its memory requirements.

Also would this check be SoC specific or would it be common for any SoC
with same Wave5xx IP?

> >> Wave5 can enable/disable the sec_axi_info option for each instance.
> >>
> >> How about handle sram-size through match_data ?
> >> I can find some drivers which use match_data to configure the sram size.
> >>
> >> We can use current "ti,k3-j721s2-wave521c" device as a 4K supported
> >device.
> >> - .sram_size = (64 * 1024);
> >> Driver just allocate the sram-size for max supported resolution of each
> >device, and we don't need to check the sram-size is enough or not.
> >>
> >> Thanks.
> >> Nas.
> >>
> >> >+
> >> > #define MAX_NUM_INSTANCE                32
> >> >
> >> > #define W5_MIN_ENC_PIC_WIDTH            256
> >> >--
> >> >2.44.0
> >>