All of lore.kernel.org
 help / color / mirror / Atom feed
From: "sebastian.fricke@collabora.com" <sebastian.fricke@collabora.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: [PATCH v2 4/5] media: chips-media: wave5: drop "sram-size" DT prop
Date: Thu, 4 Apr 2024 10:02:35 +0200	[thread overview]
Message-ID: <20240404080235.kab6taeimsxpjtr3@basti-XPS-13-9310> (raw)
In-Reply-To: <SL2P216MB1246597E5880A5A590CE1DB2FB3F2@SL2P216MB1246.KORP216.PROD.OUTLOOK.COM>

Hey Nas,

On 01.04.2024 09:28, Nas Chung wrote:
>Hi, Sebastian.
>
>>-----Original Message-----
>>From: Ivan Bornyakov <brnkv.i1@gmail.com>
>>Sent: Thursday, March 28, 2024 8:36 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: RE: [PATCH v2 4/5] media: chips-media: wave5: drop
>>"sram-size" DT prop
>>
>>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.
>
>I proposed using match_data to allocate different sram size for each device.
>Do you think this is a reasonable approach ?

As discussed here:
https://patchwork.linuxtv.org/project/linux-media/patch/20240201184238.2542695-1-b-brnich@ti.com/

To quote Brandon Brnich from TI:

If static SRAM allocation is the correct method to go, then this series
can be ignored and I will add section in device tree and remove check
for parameter in driver as that will now be a bug.

I would like to adhere to that.

>
>Thanks.
>Nas.

Greetings,
Sebastian

>
>>> >>
>>> >> 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
>>> >>

  reply	other threads:[~2024-04-04  8:02 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-25  6:40 [PATCH v2 0/5] Wave515 decoder IP support Ivan Bornyakov
2024-03-25  6:40 ` [PATCH v2 1/5] media: chips-media: wave5: support decoding HEVC Main10 profile Ivan Bornyakov
2024-03-25  6:40 ` [PATCH v2 2/5] media: chips-media: wave5: support reset lines Ivan Bornyakov
2024-04-05 13:23   ` Sebastian Fricke
2024-03-25  6:40 ` [PATCH v2 3/5] media: chips-media: wave5: separate irq setup routine Ivan Bornyakov
2024-04-05  9:29   ` Sebastian Fricke
2024-03-25  6:40 ` [PATCH v2 4/5] media: chips-media: wave5: drop "sram-size" DT prop Ivan Bornyakov
2024-03-27 10:27   ` Nas Chung
2024-03-27 11:18     ` Ivan Bornyakov
2024-03-27 12:26     ` Ivan Bornyakov
2024-03-28 10:16       ` Nas Chung
2024-03-28 11:36         ` Ivan Bornyakov
2024-04-01  9:28           ` Nas Chung
2024-04-04  8:02             ` sebastian.fricke [this message]
2024-04-04 18:56               ` Nicolas Dufresne
2024-04-05  8:56                 ` sebastian.fricke
2024-03-25  6:41 ` [PATCH v2 5/5] media: chips-media: wave5: support Wave515 decoder Ivan Bornyakov
2024-03-27 10:34   ` Nas Chung
2024-03-27 11:29     ` Ivan Bornyakov
2024-03-28 16:18   ` Sebastian Fricke
2024-04-01  9:15     ` Nas Chung

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240404080235.kab6taeimsxpjtr3@basti-XPS-13-9310 \
    --to=sebastian.fricke@collabora.com \
    --cc=jackson.lee@chipsnmedia.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@kernel.org \
    --cc=nas.chung@chipsnmedia.com \
    --cc=p.zabel@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.