ASoC: SOF: mediatek: Add PCM stream callback for mt8195 #3190

yaochunhung · 2021-10-04T12:53:08Z

This PR is to add module to support ALSA pcm stream for hw_params and hw_free.
For mt8195(or other mediatek ICs with inside DSP), DSP can access dram memory. There is MPU(memory protection unit) configuration on DSP. MPU can be used to define the access right for different memory regions.
To avoid that DSP overwrite invalid region of DRAM used by AP side. We need to pre-define the DRAM memory range will be used by DSP then use MPU to protect.
At AP side, we reserve dram for DSP via device tree
e.g. reserve dram 0x60000000 - 0x60FFFFFF for dsp system code/data/audio data...etc, 0x61000000 - 0 0x610FFFFF for dma
reference : 8f38f99#diff-78282555906c4a4a913fd0bbd23e9541cb0e2dbdd6432f7c2f49f79f15602d8fR119

	reserved_memory: reserved-memory {
		#address-cells = <2>;
		#size-cells = <2>;
		ranges;

		adsp_mem_reserved: adsp_mem_region {
			compatible = "mediatek,adsp-reserved-memory";
			no-map;
			reg = <0 0x60000000 0 0x1000000>;
		};

		adsp_dma_mem_reserved: adsp_dma_mem_region {
			compatible = "shared-dma-pool";
			reg = <0 0x61000000 0 0x0100000>;
			no-map;
		};
	};
       memory-region = <&adsp_dma_mem_reserved>,
                       <&adsp_mem_reserved>;

At DSP side, we define MPU table as below,
https://github.com/thesofproject/sof/blob/main/src/platform/mt8195/platform.c#L139

const struct xthal_MPU_entry __xt_mpu_init_table[] __attribute__((section(".ResetVector.text"))) = {
	XTHAL_MPU_ENTRY(0x00000000, 1, XTHAL_AR_NONE,
			XTHAL_MEM_DEVICE), /* illegal: no access */
	XTHAL_MPU_ENTRY(0x0C000000, 1, XTHAL_AR_RWXrwx,
			XTHAL_MEM_DEVICE), /* MCU & DBG Registers read/write/execute */
	XTHAL_MPU_ENTRY(0x0F000000, 1, XTHAL_AR_NONE,
			XTHAL_MEM_DEVICE), /* illegal: no access */
	XTHAL_MPU_ENTRY(0x10000000, 1, XTHAL_AR_RWXrwx,
			XTHAL_MEM_DEVICE), /* DSP register: read/write/execute */
	XTHAL_MPU_ENTRY(0x12000000, 1, XTHAL_AR_NONE,
			XTHAL_MEM_DEVICE), /* illegal: no access */
	XTHAL_MPU_ENTRY(0x40000000, 1, XTHAL_AR_RWXrwx,
			XTHAL_MEM_WRITEBACK), /* DSP SRAM: read/write/execute writeback */
	XTHAL_MPU_ENTRY(0x40040000, 1, XTHAL_AR_NONE,
			XTHAL_MEM_DEVICE), /* illegal: no access */
	XTHAL_MPU_ENTRY(0x60000000, 1, XTHAL_AR_RWXrwx,
			XTHAL_MEM_WRITEBACK), /* DRAM: read/write/execute writeback */
	XTHAL_MPU_ENTRY(0x61000000, 1, XTHAL_AR_RWXrwx,
			XTHAL_MEM_DEVICE), /* DMA: read/write/execute writeback */
	XTHAL_MPU_ENTRY(0x61100000, 1, XTHAL_AR_NONE,
			XTHAL_MEM_DEVICE), /* illegal: no access */
};

Where XTHAL_MPU_ENTY is #define XTHAL_MPU_ENTRY(vaddr, valid, access, memtype). first 32 field first parameter is "vaddr". it implies the start address for this regions, and the end address will be the start address minus one in the next region. In this case, the start address of first region is 0x00000000, and the end address of first region is 0x0C000000-1=0x0BFFFFFF (0x0C000000 is the start address of second region). The second parameter "valid" means enable. it is always set as one to enable MPU. The third parameter 3"access" means MPU access rights(AR) : NONE for no access, following upper cases means access right for kernel space and lower cases means user space. "R/r" means read, "W/w" means write, and "X/x" means execute. The forth parameter "memtype" means memory type, it could be DEVICE/NON_CACHE/WRITETHROUGH/WRITEBACK. For last region , the start address of the last region is 0x6110000 and the end address of the last region will be 0xFFFFFFFF. we also set it as none When DSP access this region, exception interrupt will be triggered. Then we can know there is a invalid access to dram.

To ensure this mechanism can work well, we cannot use dynamic allocation memory for DSP because the address of dynamic allocated memory is unknow and it will be changed at run time therefore DSP cannot set MPU region in the boot stage. Therefore, even page table has already been allocated in sof/pcm.c, we still use the reserved memory to create a memory pool for audio data in the mt8195 probe function(adsp_genpool_destroy in remove function)

static int mt8195_dsp_probe(struct snd_sof_dev *sdev)
{
    adsp_genpool_create(&priv->mem_pool, &share_dram);
}
static int mt8195_dsp_remove(struct snd_sof_dev *sdev)
{
    adsp_genpool_destroy(&priv->mem_pool);
}

Then allocate memory in the reserved memory(pa_shared_dram) in mtk_adsp_pcm_hw_params and free it in mtk_adsp_pcm_hw_free. Besides, we also backup original page table information in mtk_adsp_pcm_hw_params and restore it in mtk_adsp_pcm_hw_free to avoid influencing original flow. Thanks.

Add module to support ALSA pcm stream for hw_params and hw_free Signed-off-by: YC Hung <yc.hung@mediatek.com>

plbossart

one nit-pick and one fundamental ask to document the memory management before asking us to review, thanks!

plbossart · 2021-10-04T13:45:15Z

sound/soc/sof/mediatek/adsp_pcm.c

+	if (runtime->buffer_changed && substream->managed_buffer_alloc) {
+		/* free dma pages allocated by common layer
+		 * if page table private data is not NULL,
+		 * this hw_params may be trigged by PCM xrun


xrun will result in .prepare being called, not hw_params. There must be a confusion here?

plbossart · 2021-10-04T13:47:36Z

sound/soc/sof/mediatek/adsp_pcm.c

+		dmab->addr = priv->ap2adsp_addr(dmab->addr, priv->adsp);
+		snd_pcm_set_runtime_buffer(substream, dmab);
+		snd_sof_create_page_table(dev, dmab,
+					  pg_table->area, dma_bytes);


You really need to add more comments and better explanations in the commit message, this seems to duplicated what is already done at the SOF level in pcm.c

In addition this is the first use of the genpool library/subsystem and the ALSA memory management changed recently so it's not clear who does what and why.

@plbossart I added more information in the commit message. Please help to check it. Thanks.

Thanks @yaochunhung I was talking about the GIT commit message, not the GitHub introduction. You really want the information added to the GIT commit so that when this reaches upstream reviewers can have a trace of your design.

@plbossart Got it. I will keep the introduction and also add more information in git commit message. Thanks.

ranj063 · 2021-10-06T03:01:11Z

sound/soc/sof/mediatek/adsp_pcm.c

+
+		dmab->addr = priv->ap2adsp_addr(dmab->addr, priv->adsp);
+		snd_pcm_set_runtime_buffer(substream, dmab);
+		snd_sof_create_page_table(dev, dmab,


will fit in one line

ranj063 · 2021-10-06T03:02:24Z

sound/soc/sof/mediatek/adsp_pcm.c

+		 * if page table private data is not NULL,
+		 * this hw_params may be trigged by PCM xrun
+		 */
+		if (pg_table->private_data)


may I ask why this check needs to be inside the if (runtime->buffer_changed && substream->managed_buffer_alloc) block?

while I'm at it can you also please comment what "if (runtime->buffer_changed && substream->managed_buffer_alloc)" really checks for?

ranj063 · 2021-10-06T03:05:12Z

sound/soc/sof/mediatek/mt8195/mt8195.c

+	if (ret) {
+		dev_err(sdev->dev, "adsp_genpool_create fail!\n");
+		ret = -ENOMEM;
+		goto err_adsp_sram_power_off;


this goto seems unnecessary. Is the intention to power off the adsp sram in case of a probe success also?

ranj063 · 2021-10-06T03:06:17Z

sound/soc/sof/mediatek/mt8195/mt8195.c

+	struct adsp_priv *priv = sdev->pdata->hw_pdata;

-	return adsp_sram_power_on(&pdev->dev, false);
+	adsp_sram_power_on(&pdev->dev, false);


if this can fail, you need to check for error right?

ranj063 · 2021-10-06T03:06:28Z

sound/soc/sof/mediatek/mt8195/mt8195.c


-	return adsp_sram_power_on(&pdev->dev, false);
+	adsp_sram_power_on(&pdev->dev, false);
+	adsp_genpool_destroy(&priv->mem_pool);


same with this one too

dbaluta · 2021-10-06T07:11:27Z

sound/soc/sof/mediatek/adsp_pcm.h

+/* adsp share memory manage */
+int adsp_genpool_create(struct adsp_mem_pool *mem_pool,
+			struct adsp_mem *mem_info);
+


adsp is a pretty generic prefix. Please use mtk or something more specific.

dbaluta · 2021-10-06T07:13:10Z

sound/soc/sof/mediatek/Makefile

@@ -1,2 +1,5 @@
 # SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
+snd-sof-mtk-adsp-objs := adsp_pcm.o
+


We definitely need more explanations about this commit in the commit message.

yaochunhung · 2021-10-06T13:24:28Z

@plbossart @dbaluta @ranj063
Thanks for comments. Since gen pool is not a general use for sound/soc and we also have the of_reserved_mem_device_init to reserve memory for device. I try to enlarge reserved dma memory and don't implement hw_params/hw_free function. It seems work fine. I think this patch maybe can be pending and I will do more verification on new method. Sorry for inconvenient and thanks for reviews.

yaochunhung requested review from bardliao, dbaluta, kv2019i, lgirdwood, plbossart and ranj063 as code owners October 4, 2021 12:53

yaochunhung force-pushed the mt8195-pcm branch from 9f6289c to b6f7dea Compare October 4, 2021 13:18

ASoC: SOF: mediatek: Add PCM stream callback for mt8195

c3dece5

Add module to support ALSA pcm stream for hw_params and hw_free Signed-off-by: YC Hung <yc.hung@mediatek.com>

yaochunhung force-pushed the mt8195-pcm branch from b6f7dea to c3dece5 Compare October 4, 2021 13:28

plbossart requested changes Oct 4, 2021

View reviewed changes

yaochunhung mentioned this pull request Oct 5, 2021

Add support for Mediatek mt8195 platform #3122

Closed

ranj063 reviewed Oct 6, 2021

View reviewed changes

dbaluta reviewed Oct 6, 2021

View reviewed changes

yaochunhung closed this Oct 7, 2021

		@@ -1,2 +1,5 @@
		# SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
		snd-sof-mtk-adsp-objs := adsp_pcm.o

ASoC: SOF: mediatek: Add PCM stream callback for mt8195 #3190

ASoC: SOF: mediatek: Add PCM stream callback for mt8195 #3190

Uh oh!

Conversation

yaochunhung commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaochunhung commented Oct 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yaochunhung commented Oct 4, 2021 •

edited

Loading