diff mbox series

[v3,4/5] media: chips-media: wave5: drop "sram-size" DT prop

Message ID 20240405164112.24571-5-brnkv.i1@gmail.com (mailing list archive)
State New, archived
Headers show
Series Wave515 decoder IP support | expand

Commit Message

Ivan Bornyakov April 5, 2024, 4:41 p.m. UTC
Move excessive "sram-size" device-tree property to device match data.
Also change SRAM memory allocation strategy: instead of allocation exact
sram_size bytes, allocate all available SRAM memory up to sram_size.
Add placeholders wave5_vpu_dec_validate_sec_axi() and
wave5_vpu_enc_validate_sec_axi() for validation that allocated SRAM
memory is enough to decode/encode bitstream of given resolution.

Signed-off-by: Ivan Bornyakov <brnkv.i1@gmail.com>
---
 .../platform/chips-media/wave5/wave5-hw.c     | 62 +++++++++++++++++--
 .../platform/chips-media/wave5/wave5-vdi.c    | 21 ++++---
 .../platform/chips-media/wave5/wave5-vpu.c    | 11 ++--
 3 files changed, 72 insertions(+), 22 deletions(-)

Comments

Jackson.lee April 9, 2024, 4:50 a.m. UTC | #1
Hey Ivan

> -----Original Message-----
> From: Ivan Bornyakov <brnkv.i1@gmail.com>
> Sent: Saturday, April 6, 2024 1:41 AM
> 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>; Sebastian Fricke
> <sebastian.fricke@collabora.com>
> Cc: Ivan Bornyakov <brnkv.i1@gmail.com>; linux-media@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: [PATCH v3 4/5] media: chips-media: wave5: drop "sram-size" DT
> prop
> 
> Move excessive "sram-size" device-tree property to device match data.
> Also change SRAM memory allocation strategy: instead of allocation exact
> sram_size bytes, allocate all available SRAM memory up to sram_size.
> Add placeholders wave5_vpu_dec_validate_sec_axi() and
> wave5_vpu_enc_validate_sec_axi() for validation that allocated SRAM memory
> is enough to decode/encode bitstream of given resolution.
> 
> Signed-off-by: Ivan Bornyakov <brnkv.i1@gmail.com>
> ---
>  .../platform/chips-media/wave5/wave5-hw.c     | 62 +++++++++++++++++--
>  .../platform/chips-media/wave5/wave5-vdi.c    | 21 ++++---
>  .../platform/chips-media/wave5/wave5-vpu.c    | 11 ++--
>  3 files changed, 72 insertions(+), 22 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..36f2fc818013 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-hw.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-hw.c
> @@ -843,6 +843,36 @@ int wave5_vpu_dec_register_framebuffer(struct
> vpu_instance *inst, struct frame_b
>  	return ret;
>  }
> 
> +static u32 wave5_vpu_dec_validate_sec_axi(struct vpu_instance *inst) {
> +	struct dec_info *p_dec_info = &inst->codec_info->dec_info;
> +	u32 bit_size = 0, ip_size = 0, lf_size = 0, ret = 0;

The bit_size, ip_size and 1f_size is always 0? If so, why are you using them ?


> +	u32 sram_size = inst->dev->sram_size;
> +
> +	if (!sram_size)
> +		return 0;
> +
> +	/*
> +	 * TODO: calculate bit_size, ip_size, lf_size from inst-
> >src_fmt.width
> +	 * and inst->codec_info->dec_info.initial_info.luma_bitdepth
> +	 */
> +
> +	if (p_dec_info->sec_axi_info.use_bit_enable && sram_size >=
> bit_size) {
> +		ret |= BIT(0);
> +		sram_size -= bit_size;
> +	}
> +
> +	if (p_dec_info->sec_axi_info.use_ip_enable && sram_size >= ip_size)
> {
> +		ret |= BIT(9);
> +		sram_size -= ip_size;
> +	}
> +
> +	if (p_dec_info->sec_axi_info.use_lf_row_enable && sram_size >=
> lf_size)
> +		ret |= BIT(15);
> +
> +	return ret;
> +}
> +
>  int wave5_vpu_decode(struct vpu_instance *inst, u32 *fail_res)  {
>  	u32 reg_val;
> @@ -855,9 +885,7 @@ int wave5_vpu_decode(struct vpu_instance *inst, u32
> *fail_res)
>  	vpu_write_reg(inst->dev, W5_BS_OPTION,
> get_bitstream_options(p_dec_info));
> 
>  	/* secondary AXI */
> -	reg_val = p_dec_info->sec_axi_info.use_bit_enable |
> -		(p_dec_info->sec_axi_info.use_ip_enable << 9) |
> -		(p_dec_info->sec_axi_info.use_lf_row_enable << 15);
> +	reg_val = wave5_vpu_dec_validate_sec_axi(inst);
>  	vpu_write_reg(inst->dev, W5_USE_SEC_AXI, reg_val);
> 
>  	/* set attributes of user buffer */
> @@ -1938,6 +1966,31 @@ int wave5_vpu_enc_register_framebuffer(struct
> device *dev, struct vpu_instance *
>  	return ret;
>  }
> 
> +static u32 wave5_vpu_enc_validate_sec_axi(struct vpu_instance *inst) {
> +	struct enc_info *p_enc_info = &inst->codec_info->enc_info;
> +	u32 rdo_size = 0, lf_size = 0, ret = 0;

The rdo_size and 1f_size is always 0? If so, why are you using them ?

> +	u32 sram_size = inst->dev->sram_size;
> +
> +	if (!sram_size)
> +		return 0;
> +
> +	/*
> +	 * TODO: calculate rdo_size and lf_size from inst->src_fmt.width
> and
> +	 * inst->codec_info-
> >enc_info.open_param.wave_param.internal_bit_depth
> +	 */
> +
> +	if (p_enc_info->sec_axi_info.use_enc_rdo_enable && sram_size >=
> rdo_size) {
> +		ret |= BIT(11);
> +		sram_size -= rdo_size;
> +	}
> +
> +	if (p_enc_info->sec_axi_info.use_enc_lf_enable && sram_size >=
> lf_size)
> +		ret |= BIT(15);
> +
> +	return ret;
> +}
> +
>  int wave5_vpu_encode(struct vpu_instance *inst, struct enc_param *option,
> u32 *fail_res)  {
>  	u32 src_frame_format;
> @@ -1959,8 +2012,7 @@ int wave5_vpu_encode(struct vpu_instance *inst,
> struct enc_param *option, u32 *f
> 
>  	vpu_write_reg(inst->dev, W5_CMD_ENC_PIC_SRC_AXI_SEL,
> DEFAULT_SRC_AXI);
>  	/* secondary AXI */
> -	reg_val = (p_enc_info->sec_axi_info.use_enc_rdo_enable << 11) |
> -		(p_enc_info->sec_axi_info.use_enc_lf_enable << 15);
> +	reg_val = wave5_vpu_enc_validate_sec_axi(inst);
>  	vpu_write_reg(inst->dev, W5_CMD_ENC_PIC_USE_SEC_AXI, reg_val);
> 
>  	vpu_write_reg(inst->dev, W5_CMD_ENC_PIC_REPORT_PARAM, 0); diff --
> git a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> index 3809f70bc0b4..556de2f043fe 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vdi.c


The below code is not based on the current upstream code. Where did you get the original code ?

> @@ -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, vpu_dev->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..9e93969ab6db 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> @@ -25,6 +25,7 @@
>  struct wave5_match_data {
>  	int flags;
>  	const char *fw_name;
> +	u32 sram_size;
>  };
> 
>  int wave5_vpu_wait_interrupt(struct vpu_instance *inst, unsigned int
> timeout) @@ -177,17 +178,12 @@ 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");
> 
> +	dev->sram_size = match_data->sram_size;
> +
>  	dev->product_code = wave5_vdi_read_register(dev,
> VPU_PRODUCT_CODE_REGISTER);
>  	ret = wave5_vdi_init(&pdev->dev);
>  	if (ret < 0) {
> @@ -281,6 +277,7 @@ static void wave5_vpu_remove(struct platform_device
> *pdev)  static const struct wave5_match_data ti_wave521c_data = {
>  	.flags = WAVE5_IS_ENC | WAVE5_IS_DEC,
>  	.fw_name = "cnm/wave521c_k3_codec_fw.bin",
> +	.sram_size = (64 * 1024),
>  };
> 
>  static const struct of_device_id wave5_dt_ids[] = {
> --
> 2.44.0
Ivan Bornyakov April 9, 2024, 8:12 a.m. UTC | #2
Hi, Jackson

On Tue, Apr 09, 2024 at 04:50:15AM +0000, jackson.lee wrote:
> Hey Ivan
> 
> > -----Original Message-----
> > From: Ivan Bornyakov <brnkv.i1@gmail.com>
> > Sent: Saturday, April 6, 2024 1:41 AM
> > 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>; Sebastian Fricke
> > <sebastian.fricke@collabora.com>
> > Cc: Ivan Bornyakov <brnkv.i1@gmail.com>; linux-media@vger.kernel.org;
> > linux-kernel@vger.kernel.org
> > Subject: [PATCH v3 4/5] media: chips-media: wave5: drop "sram-size" DT
> > prop
> > 
> > Move excessive "sram-size" device-tree property to device match data.
> > Also change SRAM memory allocation strategy: instead of allocation exact
> > sram_size bytes, allocate all available SRAM memory up to sram_size.
> > Add placeholders wave5_vpu_dec_validate_sec_axi() and
> > wave5_vpu_enc_validate_sec_axi() for validation that allocated SRAM memory
> > is enough to decode/encode bitstream of given resolution.
> > 
> > Signed-off-by: Ivan Bornyakov <brnkv.i1@gmail.com>
> > ---
> >  .../platform/chips-media/wave5/wave5-hw.c     | 62 +++++++++++++++++--
> >  .../platform/chips-media/wave5/wave5-vdi.c    | 21 ++++---
> >  .../platform/chips-media/wave5/wave5-vpu.c    | 11 ++--
> >  3 files changed, 72 insertions(+), 22 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..36f2fc818013 100644
> > --- a/drivers/media/platform/chips-media/wave5/wave5-hw.c
> > +++ b/drivers/media/platform/chips-media/wave5/wave5-hw.c
> > @@ -843,6 +843,36 @@ int wave5_vpu_dec_register_framebuffer(struct
> > vpu_instance *inst, struct frame_b
> >  	return ret;
> >  }
> > 
> > +static u32 wave5_vpu_dec_validate_sec_axi(struct vpu_instance *inst) {
> > +	struct dec_info *p_dec_info = &inst->codec_info->dec_info;
> > +	u32 bit_size = 0, ip_size = 0, lf_size = 0, ret = 0;
> 
> The bit_size, ip_size and 1f_size is always 0? If so, why are you using them ?
> 

Since I don't have documentation on Wave521, this is a placeholder for
someone who have documentation to write proper SRAM size validation,
hence TODO comment.

In the next patch "media: chips-media: wave5: support Wave515 decoder"
I added validation of SRAM usage for Wave515, for which I do have
documentation.

> 
> > +	u32 sram_size = inst->dev->sram_size;
> > +
> > +	if (!sram_size)
> > +		return 0;
> > +
> > +	/*
> > +	 * TODO: calculate bit_size, ip_size, lf_size from inst-
> > >src_fmt.width
> > +	 * and inst->codec_info->dec_info.initial_info.luma_bitdepth
> > +	 */
> > +
> > +	if (p_dec_info->sec_axi_info.use_bit_enable && sram_size >=
> > bit_size) {
> > +		ret |= BIT(0);
> > +		sram_size -= bit_size;
> > +	}
> > +
> > +	if (p_dec_info->sec_axi_info.use_ip_enable && sram_size >= ip_size)
> > {
> > +		ret |= BIT(9);
> > +		sram_size -= ip_size;
> > +	}
> > +
> > +	if (p_dec_info->sec_axi_info.use_lf_row_enable && sram_size >=
> > lf_size)
> > +		ret |= BIT(15);
> > +
> > +	return ret;
> > +}
> > +
> >  int wave5_vpu_decode(struct vpu_instance *inst, u32 *fail_res)  {
> >  	u32 reg_val;
> > @@ -855,9 +885,7 @@ int wave5_vpu_decode(struct vpu_instance *inst, u32
> > *fail_res)
> >  	vpu_write_reg(inst->dev, W5_BS_OPTION,
> > get_bitstream_options(p_dec_info));
> > 
> >  	/* secondary AXI */
> > -	reg_val = p_dec_info->sec_axi_info.use_bit_enable |
> > -		(p_dec_info->sec_axi_info.use_ip_enable << 9) |
> > -		(p_dec_info->sec_axi_info.use_lf_row_enable << 15);
> > +	reg_val = wave5_vpu_dec_validate_sec_axi(inst);
> >  	vpu_write_reg(inst->dev, W5_USE_SEC_AXI, reg_val);
> > 
> >  	/* set attributes of user buffer */
> > @@ -1938,6 +1966,31 @@ int wave5_vpu_enc_register_framebuffer(struct
> > device *dev, struct vpu_instance *
> >  	return ret;
> >  }
> > 
> > +static u32 wave5_vpu_enc_validate_sec_axi(struct vpu_instance *inst) {
> > +	struct enc_info *p_enc_info = &inst->codec_info->enc_info;
> > +	u32 rdo_size = 0, lf_size = 0, ret = 0;
> 
> The rdo_size and 1f_size is always 0? If so, why are you using them ?
>

Same as above. It is a placeholder for someone else to implement these.

> > +	u32 sram_size = inst->dev->sram_size;
> > +
> > +	if (!sram_size)
> > +		return 0;
> > +
> > +	/*
> > +	 * TODO: calculate rdo_size and lf_size from inst->src_fmt.width
> > and
> > +	 * inst->codec_info-
> > >enc_info.open_param.wave_param.internal_bit_depth
> > +	 */
> > +
> > +	if (p_enc_info->sec_axi_info.use_enc_rdo_enable && sram_size >=
> > rdo_size) {
> > +		ret |= BIT(11);
> > +		sram_size -= rdo_size;
> > +	}
> > +
> > +	if (p_enc_info->sec_axi_info.use_enc_lf_enable && sram_size >=
> > lf_size)
> > +		ret |= BIT(15);
> > +
> > +	return ret;
> > +}
> > +
> >  int wave5_vpu_encode(struct vpu_instance *inst, struct enc_param *option,
> > u32 *fail_res)  {
> >  	u32 src_frame_format;
> > @@ -1959,8 +2012,7 @@ int wave5_vpu_encode(struct vpu_instance *inst,
> > struct enc_param *option, u32 *f
> > 
> >  	vpu_write_reg(inst->dev, W5_CMD_ENC_PIC_SRC_AXI_SEL,
> > DEFAULT_SRC_AXI);
> >  	/* secondary AXI */
> > -	reg_val = (p_enc_info->sec_axi_info.use_enc_rdo_enable << 11) |
> > -		(p_enc_info->sec_axi_info.use_enc_lf_enable << 15);
> > +	reg_val = wave5_vpu_enc_validate_sec_axi(inst);
> >  	vpu_write_reg(inst->dev, W5_CMD_ENC_PIC_USE_SEC_AXI, reg_val);
> > 
> >  	vpu_write_reg(inst->dev, W5_CMD_ENC_PIC_REPORT_PARAM, 0); diff --
> > git a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> > b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> > index 3809f70bc0b4..556de2f043fe 100644
> > --- a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> > +++ b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> 
> 
> The below code is not based on the current upstream code. Where did you get the original code ?
> 

What do you mean? This patch series is based on the latest linux-next.

> > @@ -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, vpu_dev->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..9e93969ab6db 100644
> > --- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > @@ -25,6 +25,7 @@
> >  struct wave5_match_data {
> >  	int flags;
> >  	const char *fw_name;
> > +	u32 sram_size;
> >  };
> > 
> >  int wave5_vpu_wait_interrupt(struct vpu_instance *inst, unsigned int
> > timeout) @@ -177,17 +178,12 @@ 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");
> > 
> > +	dev->sram_size = match_data->sram_size;
> > +
> >  	dev->product_code = wave5_vdi_read_register(dev,
> > VPU_PRODUCT_CODE_REGISTER);
> >  	ret = wave5_vdi_init(&pdev->dev);
> >  	if (ret < 0) {
> > @@ -281,6 +277,7 @@ static void wave5_vpu_remove(struct platform_device
> > *pdev)  static const struct wave5_match_data ti_wave521c_data = {
> >  	.flags = WAVE5_IS_ENC | WAVE5_IS_DEC,
> >  	.fw_name = "cnm/wave521c_k3_codec_fw.bin",
> > +	.sram_size = (64 * 1024),
> >  };
> > 
> >  static const struct of_device_id wave5_dt_ids[] = {
> > --
> > 2.44.0
>
Jackson.lee April 11, 2024, 8:11 a.m. UTC | #3
Hi Ivan

> -----Original Message-----
> From: Ivan Bornyakov <brnkv.i1@gmail.com>
> Sent: Tuesday, April 9, 2024 5:12 PM
> To: jackson.lee <jackson.lee@chipsnmedia.com>
> Cc: Nas Chung <nas.chung@chipsnmedia.com>; Mauro Carvalho Chehab
> <mchehab@kernel.org>; Philipp Zabel <p.zabel@pengutronix.de>; Sebastian
> Fricke <sebastian.fricke@collabora.com>; linux-media@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: RE: [PATCH v3 4/5] media: chips-media: wave5: drop "sram-
> size" DT prop
> 
> Hi, Jackson
> 
> On Tue, Apr 09, 2024 at 04:50:15AM +0000, jackson.lee wrote:
> > Hey Ivan
> >
> > > -----Original Message-----
> > > From: Ivan Bornyakov <brnkv.i1@gmail.com>
> > > Sent: Saturday, April 6, 2024 1:41 AM
> > > 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>;
> > > Sebastian Fricke <sebastian.fricke@collabora.com>
> > > Cc: Ivan Bornyakov <brnkv.i1@gmail.com>;
> > > linux-media@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: [PATCH v3 4/5] media: chips-media: wave5: drop "sram-size"
> > > DT prop
> > >
> > > Move excessive "sram-size" device-tree property to device match data.
> > > Also change SRAM memory allocation strategy: instead of allocation
> > > exact sram_size bytes, allocate all available SRAM memory up to
> sram_size.
> > > Add placeholders wave5_vpu_dec_validate_sec_axi() and
> > > wave5_vpu_enc_validate_sec_axi() for validation that allocated SRAM
> > > memory is enough to decode/encode bitstream of given resolution.
> > >
> > > Signed-off-by: Ivan Bornyakov <brnkv.i1@gmail.com>
> > > ---
> > >  .../platform/chips-media/wave5/wave5-hw.c     | 62 +++++++++++++++++--
> > >  .../platform/chips-media/wave5/wave5-vdi.c    | 21 ++++---
> > >  .../platform/chips-media/wave5/wave5-vpu.c    | 11 ++--
> > >  3 files changed, 72 insertions(+), 22 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..36f2fc818013 100644
> > > --- a/drivers/media/platform/chips-media/wave5/wave5-hw.c
> > > +++ b/drivers/media/platform/chips-media/wave5/wave5-hw.c
> > > @@ -843,6 +843,36 @@ int wave5_vpu_dec_register_framebuffer(struct
> > > vpu_instance *inst, struct frame_b
> > >  	return ret;
> > >  }
> > >
> > > +static u32 wave5_vpu_dec_validate_sec_axi(struct vpu_instance *inst)
> {
> > > +	struct dec_info *p_dec_info = &inst->codec_info->dec_info;
> > > +	u32 bit_size = 0, ip_size = 0, lf_size = 0, ret = 0;
> >
> > The bit_size, ip_size and 1f_size is always 0? If so, why are you using
> them ?
> >
> 
> Since I don't have documentation on Wave521, this is a placeholder for
> someone who have documentation to write proper SRAM size validation, hence
> TODO comment.
> 
> In the next patch "media: chips-media: wave5: support Wave515 decoder"
> I added validation of SRAM usage for Wave515, for which I do have
> documentation.
> 
> >
> > > +	u32 sram_size = inst->dev->sram_size;
> > > +
> > > +	if (!sram_size)
> > > +		return 0;
> > > +
> > > +	/*
> > > +	 * TODO: calculate bit_size, ip_size, lf_size from inst-
> > > >src_fmt.width
> > > +	 * and inst->codec_info->dec_info.initial_info.luma_bitdepth
> > > +	 */
> > > +
> > > +	if (p_dec_info->sec_axi_info.use_bit_enable && sram_size >=
> > > bit_size) {
> > > +		ret |= BIT(0);
> > > +		sram_size -= bit_size;
> > > +	}
> > > +
> > > +	if (p_dec_info->sec_axi_info.use_ip_enable && sram_size >=
> > > +ip_size)
> > > {
> > > +		ret |= BIT(9);
> > > +		sram_size -= ip_size;
> > > +	}
> > > +
> > > +	if (p_dec_info->sec_axi_info.use_lf_row_enable && sram_size >=
> > > lf_size)
> > > +		ret |= BIT(15);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >  int wave5_vpu_decode(struct vpu_instance *inst, u32 *fail_res)  {
> > >  	u32 reg_val;
> > > @@ -855,9 +885,7 @@ int wave5_vpu_decode(struct vpu_instance *inst,
> > > u32
> > > *fail_res)
> > >  	vpu_write_reg(inst->dev, W5_BS_OPTION,
> > > get_bitstream_options(p_dec_info));
> > >
> > >  	/* secondary AXI */
> > > -	reg_val = p_dec_info->sec_axi_info.use_bit_enable |
> > > -		(p_dec_info->sec_axi_info.use_ip_enable << 9) |
> > > -		(p_dec_info->sec_axi_info.use_lf_row_enable << 15);
> > > +	reg_val = wave5_vpu_dec_validate_sec_axi(inst);
> > >  	vpu_write_reg(inst->dev, W5_USE_SEC_AXI, reg_val);
> > >
> > >  	/* set attributes of user buffer */ @@ -1938,6 +1966,31 @@ int
> > > wave5_vpu_enc_register_framebuffer(struct
> > > device *dev, struct vpu_instance *
> > >  	return ret;
> > >  }
> > >
> > > +static u32 wave5_vpu_enc_validate_sec_axi(struct vpu_instance *inst)
> {
> > > +	struct enc_info *p_enc_info = &inst->codec_info->enc_info;
> > > +	u32 rdo_size = 0, lf_size = 0, ret = 0;
> >
> > The rdo_size and 1f_size is always 0? If so, why are you using them ?
> >
> 
> Same as above. It is a placeholder for someone else to implement these.
> 
> > > +	u32 sram_size = inst->dev->sram_size;
> > > +
> > > +	if (!sram_size)
> > > +		return 0;
> > > +
> > > +	/*
> > > +	 * TODO: calculate rdo_size and lf_size from inst->src_fmt.width
> > > and
> > > +	 * inst->codec_info-
> > > >enc_info.open_param.wave_param.internal_bit_depth
> > > +	 */
> > > +
> > > +	if (p_enc_info->sec_axi_info.use_enc_rdo_enable && sram_size >=
> > > rdo_size) {
> > > +		ret |= BIT(11);
> > > +		sram_size -= rdo_size;
> > > +	}
> > > +
> > > +	if (p_enc_info->sec_axi_info.use_enc_lf_enable && sram_size >=
> > > lf_size)
> > > +		ret |= BIT(15);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >  int wave5_vpu_encode(struct vpu_instance *inst, struct enc_param
> > > *option,
> > > u32 *fail_res)  {
> > >  	u32 src_frame_format;
> > > @@ -1959,8 +2012,7 @@ int wave5_vpu_encode(struct vpu_instance
> > > *inst, struct enc_param *option, u32 *f
> > >
> > >  	vpu_write_reg(inst->dev, W5_CMD_ENC_PIC_SRC_AXI_SEL,
> > > DEFAULT_SRC_AXI);
> > >  	/* secondary AXI */
> > > -	reg_val = (p_enc_info->sec_axi_info.use_enc_rdo_enable << 11) |
> > > -		(p_enc_info->sec_axi_info.use_enc_lf_enable << 15);
> > > +	reg_val = wave5_vpu_enc_validate_sec_axi(inst);
> > >  	vpu_write_reg(inst->dev, W5_CMD_ENC_PIC_USE_SEC_AXI, reg_val);
> > >
> > >  	vpu_write_reg(inst->dev, W5_CMD_ENC_PIC_REPORT_PARAM, 0); diff --
> > > git a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> > > b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> > > index 3809f70bc0b4..556de2f043fe 100644
> > > --- a/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> > > +++ b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
> >
> >
> > The below code is not based on the current upstream code. Where did you
> get the original code ?
> >
> 
> What do you mean? This patch series is based on the latest linux-next.


I was confused, please ignore the above comment.

> 
> > > @@ -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, vpu_dev->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..9e93969ab6db 100644
> > > --- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > > +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > > @@ -25,6 +25,7 @@
> > >  struct wave5_match_data {
> > >  	int flags;
> > >  	const char *fw_name;
> > > +	u32 sram_size;
> > >  };
> > >
> > >  int wave5_vpu_wait_interrupt(struct vpu_instance *inst, unsigned
> > > int
> > > timeout) @@ -177,17 +178,12 @@ 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");
> > >
> > > +	dev->sram_size = match_data->sram_size;
> > > +
> > >  	dev->product_code = wave5_vdi_read_register(dev,
> > > VPU_PRODUCT_CODE_REGISTER);
> > >  	ret = wave5_vdi_init(&pdev->dev);
> > >  	if (ret < 0) {
> > > @@ -281,6 +277,7 @@ static void wave5_vpu_remove(struct
> > > platform_device
> > > *pdev)  static const struct wave5_match_data ti_wave521c_data = {
> > >  	.flags = WAVE5_IS_ENC | WAVE5_IS_DEC,
> > >  	.fw_name = "cnm/wave521c_k3_codec_fw.bin",
> > > +	.sram_size = (64 * 1024),
> > >  };
> > >
> > >  static const struct of_device_id wave5_dt_ids[] = {
> > > --
> > > 2.44.0
> >
diff mbox series

Patch

diff --git a/drivers/media/platform/chips-media/wave5/wave5-hw.c b/drivers/media/platform/chips-media/wave5/wave5-hw.c
index cdd0a0948a94..36f2fc818013 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-hw.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-hw.c
@@ -843,6 +843,36 @@  int wave5_vpu_dec_register_framebuffer(struct vpu_instance *inst, struct frame_b
 	return ret;
 }
 
+static u32 wave5_vpu_dec_validate_sec_axi(struct vpu_instance *inst)
+{
+	struct dec_info *p_dec_info = &inst->codec_info->dec_info;
+	u32 bit_size = 0, ip_size = 0, lf_size = 0, ret = 0;
+	u32 sram_size = inst->dev->sram_size;
+
+	if (!sram_size)
+		return 0;
+
+	/*
+	 * TODO: calculate bit_size, ip_size, lf_size from inst->src_fmt.width
+	 * and inst->codec_info->dec_info.initial_info.luma_bitdepth
+	 */
+
+	if (p_dec_info->sec_axi_info.use_bit_enable && sram_size >= bit_size) {
+		ret |= BIT(0);
+		sram_size -= bit_size;
+	}
+
+	if (p_dec_info->sec_axi_info.use_ip_enable && sram_size >= ip_size) {
+		ret |= BIT(9);
+		sram_size -= ip_size;
+	}
+
+	if (p_dec_info->sec_axi_info.use_lf_row_enable && sram_size >= lf_size)
+		ret |= BIT(15);
+
+	return ret;
+}
+
 int wave5_vpu_decode(struct vpu_instance *inst, u32 *fail_res)
 {
 	u32 reg_val;
@@ -855,9 +885,7 @@  int wave5_vpu_decode(struct vpu_instance *inst, u32 *fail_res)
 	vpu_write_reg(inst->dev, W5_BS_OPTION, get_bitstream_options(p_dec_info));
 
 	/* secondary AXI */
-	reg_val = p_dec_info->sec_axi_info.use_bit_enable |
-		(p_dec_info->sec_axi_info.use_ip_enable << 9) |
-		(p_dec_info->sec_axi_info.use_lf_row_enable << 15);
+	reg_val = wave5_vpu_dec_validate_sec_axi(inst);
 	vpu_write_reg(inst->dev, W5_USE_SEC_AXI, reg_val);
 
 	/* set attributes of user buffer */
@@ -1938,6 +1966,31 @@  int wave5_vpu_enc_register_framebuffer(struct device *dev, struct vpu_instance *
 	return ret;
 }
 
+static u32 wave5_vpu_enc_validate_sec_axi(struct vpu_instance *inst)
+{
+	struct enc_info *p_enc_info = &inst->codec_info->enc_info;
+	u32 rdo_size = 0, lf_size = 0, ret = 0;
+	u32 sram_size = inst->dev->sram_size;
+
+	if (!sram_size)
+		return 0;
+
+	/*
+	 * TODO: calculate rdo_size and lf_size from inst->src_fmt.width and
+	 * inst->codec_info->enc_info.open_param.wave_param.internal_bit_depth
+	 */
+
+	if (p_enc_info->sec_axi_info.use_enc_rdo_enable && sram_size >= rdo_size) {
+		ret |= BIT(11);
+		sram_size -= rdo_size;
+	}
+
+	if (p_enc_info->sec_axi_info.use_enc_lf_enable && sram_size >= lf_size)
+		ret |= BIT(15);
+
+	return ret;
+}
+
 int wave5_vpu_encode(struct vpu_instance *inst, struct enc_param *option, u32 *fail_res)
 {
 	u32 src_frame_format;
@@ -1959,8 +2012,7 @@  int wave5_vpu_encode(struct vpu_instance *inst, struct enc_param *option, u32 *f
 
 	vpu_write_reg(inst->dev, W5_CMD_ENC_PIC_SRC_AXI_SEL, DEFAULT_SRC_AXI);
 	/* secondary AXI */
-	reg_val = (p_enc_info->sec_axi_info.use_enc_rdo_enable << 11) |
-		(p_enc_info->sec_axi_info.use_enc_lf_enable << 15);
+	reg_val = wave5_vpu_enc_validate_sec_axi(inst);
 	vpu_write_reg(inst->dev, W5_CMD_ENC_PIC_USE_SEC_AXI, reg_val);
 
 	vpu_write_reg(inst->dev, W5_CMD_ENC_PIC_REPORT_PARAM, 0);
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vdi.c b/drivers/media/platform/chips-media/wave5/wave5-vdi.c
index 3809f70bc0b4..556de2f043fe 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, vpu_dev->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..9e93969ab6db 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
@@ -25,6 +25,7 @@ 
 struct wave5_match_data {
 	int flags;
 	const char *fw_name;
+	u32 sram_size;
 };
 
 int wave5_vpu_wait_interrupt(struct vpu_instance *inst, unsigned int timeout)
@@ -177,17 +178,12 @@  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");
 
+	dev->sram_size = match_data->sram_size;
+
 	dev->product_code = wave5_vdi_read_register(dev, VPU_PRODUCT_CODE_REGISTER);
 	ret = wave5_vdi_init(&pdev->dev);
 	if (ret < 0) {
@@ -281,6 +277,7 @@  static void wave5_vpu_remove(struct platform_device *pdev)
 static const struct wave5_match_data ti_wave521c_data = {
 	.flags = WAVE5_IS_ENC | WAVE5_IS_DEC,
 	.fw_name = "cnm/wave521c_k3_codec_fw.bin",
+	.sram_size = (64 * 1024),
 };
 
 static const struct of_device_id wave5_dt_ids[] = {