| Message ID | 20240325064102.9278-5-brnkv.i1@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Wave515 decoder IP support | expand |
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. 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
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. > 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. Sure, sounds reasonable. > > Thanks. > Nas. > > >+ > > #define MAX_NUM_INSTANCE 32 > > > > #define W5_MIN_ENC_PIC_WIDTH 256 > >-- > >2.44.0 >
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... > 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 >
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. >> 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 >>
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 > >>
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 ? Thanks. Nas. >> >> >> >> 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 >> >>
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 >>> >>
Hi, Le jeudi 04 avril 2024 à 10:02 +0200, sebastian.fricke@collabora.com a écrit : > > > > > > 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. I feel your statement is a bit ambiguous. Can you clarify what you adhere too ? That we should hardcode fixed sram size in the DT ? When I read earlier today "How about handle sram-size through match_data ?", what I saw was a static C structure in the driver. Like what we do with HW variants usually. I was pretty convince that the right solution is to allocate sram when the driver is used (open() seems appropriate), and drop it when its not used. With the clear benefit that we can change our mind later if we find valid arguments. So with that in mind, dropping (cleaning up) that old code seems helpful to create a clean slate to re-introduce a better version. Nicolas
Hey Nicolas, On 04.04.2024 14:56, Nicolas Dufresne wrote: >Hi, > >Le jeudi 04 avril 2024 à 10:02 +0200, sebastian.fricke@collabora.com a écrit : >> > > > > > 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. > >I feel your statement is a bit ambiguous. Can you clarify what you adhere too ? >That we should hardcode fixed sram size in the DT ? Sorry wasn't aware that my statement is ambiguous, my intention was to say that we do not add the sram size to the DT as it was discussed with the DT maintainer in the thread above, my adherence was pointed towards the statement from Brandon: "then this series can be ignored". > >When I read earlier today "How about handle sram-size through match_data ?", >what I saw was a static C structure in the driver. Like what we do with HW >variants usually. > >I was pretty convince that the right solution is to allocate sram when the >driver is used (open() seems appropriate), and drop it when its not used. With >the clear benefit that we can change our mind later if we find valid arguments. > >So with that in mind, dropping (cleaning up) that old code seems helpful to >create a clean slate to re-introduce a better version. > >Nicolas Greetings, Sebastian
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) + #define MAX_NUM_INSTANCE 32 #define W5_MIN_ENC_PIC_WIDTH 256
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(-)