RE: RE: [PATCH v2 4/5] media: chips-media: wave5: drop "sram-size" DT prop

From: Nas Chung
Date: Thu Mar 28 2024 - 06:17:49 EST

Hi, Ivan.

>-----Original Message-----
>From: Ivan Bornyakov <brnkv.i1@xxxxxxxxx>
>Sent: Wednesday, March 27, 2024 9:27 PM
>To: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>
>Cc: linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>jackson.lee <jackson.lee@xxxxxxxxxxxxxxx>; Mauro Carvalho Chehab
><mchehab@xxxxxxxxxx>; Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
>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@xxxxxxxxx>
>> >Sent: Monday, March 25, 2024 3:41 PM
>> >To: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>; jackson.lee
>> ><jackson.lee@xxxxxxxxxxxxxxx>; Mauro Carvalho Chehab
><mchehab@xxxxxxxxxx>;
>> >Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
>> >Cc: Ivan Bornyakov <brnkv.i1@xxxxxxxxx>; linux-media@xxxxxxxxxxxxxxx;
>> >linux-kernel@xxxxxxxxxxxxxxx
>> >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@xxxxxxxxx>
>> >---
>> > .../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.

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