diff mbox series

[13/17] ASoC: Intel: avs: Dynamic firmware resources management

Message ID 20220207122108.3780926-14-cezary.rojewski@intel.com (mailing list archive)
State Superseded
Headers show
Series ASoC: Intel: AVS - Audio DSP for cAVS | expand

Commit Message

Cezary Rojewski Feb. 7, 2022, 12:21 p.m. UTC 
Wrap elementary DSP-core operations and resource control into more
complex handlers. This is done to reduce the number of invocations of
wrapped operations throughout the driver as order of operations matters -
most flows involve register manipulation and IPCs combined.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/intel/avs/avs.h |  10 +++
 sound/soc/intel/avs/dsp.c | 170 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 180 insertions(+)

Comments

Pierre-Louis Bossart Feb. 25, 2022, 2:02 a.m. UTC | #1 
> +static int avs_dsp_enable(struct avs_dev *adev, u32 core_mask)
> +{
> +	u32 mask;
> +	int ret;
> +
> +	ret = avs_dsp_core_enable(adev, core_mask);
> +	if (ret < 0)
> +		return ret;
> +
> +	mask = core_mask & ~AVS_MAIN_CORE_MASK;

so here BIT(MAIN_CORE) is zero in mask

> +	if (!mask)
> +		/*
> +		 * without main core, fw is dead anyway
> +		 * so setting D0 for it is futile.

I don't get the comment, you explicitly discarded the main core with
your logical AND above, so this test means that all other non-main cores
are disabled.

> +		 */
> +		return 0;
> +
> +	ret = avs_ipc_set_dx(adev, mask, true);
> +	return AVS_IPC_RET(ret);
> +}
> +
> +static int avs_dsp_disable(struct avs_dev *adev, u32 core_mask)
> +{
> +	int ret;
> +
> +	ret = avs_ipc_set_dx(adev, core_mask, false);
> +	if (ret)
> +		return AVS_IPC_RET(ret);
> +
> +	return avs_dsp_core_disable(adev, core_mask);
> +}
> +
> +static int avs_dsp_get_core(struct avs_dev *adev, u32 core_id)
> +{
> +	u32 mask;
> +	int ret;
> +
> +	mask = BIT_MASK(core_id);
> +	if (mask == AVS_MAIN_CORE_MASK)
> +		/* nothing to do for main core */
> +		return 0;
> +	if (core_id >= adev->hw_cfg.dsp_cores) {
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	adev->core_refs[core_id]++;
> +	if (adev->core_refs[core_id] == 1) {
> +		ret = avs_dsp_enable(adev, mask);
> +		if (ret)
> +			goto err_enable_dsp;
> +	}
> +
> +	return 0;
> +
> +err_enable_dsp:
> +	adev->core_refs[core_id]--;
> +err:
> +	dev_err(adev->dev, "get core failed: %d\n", ret);

you should log which core could not be enabled

> +	return ret;
> +}
> +
> +static int avs_dsp_put_core(struct avs_dev *adev, u32 core_id)
> +{
> +	u32 mask;
> +	int ret;
> +
> +	mask = BIT_MASK(core_id);
> +	if (mask == AVS_MAIN_CORE_MASK)
> +		/* nothing to do for main core */
> +		return 0;
> +	if (core_id >= adev->hw_cfg.dsp_cores) {
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	adev->core_refs[core_id]--;
> +	if (!adev->core_refs[core_id]) {
> +		ret = avs_dsp_disable(adev, mask);
> +		if (ret)
> +			goto err;
> +	}
> +
> +	return 0;
> +err:
> +	dev_err(adev->dev, "put core failed: %d\n", ret);

put core %d

> +	return ret;
> +}

>  MODULE_LICENSE("GPL v2");

"GPL"
Cezary Rojewski Feb. 25, 2022, 7:27 p.m. UTC | #2 
On 2022-02-25 3:02 AM, Pierre-Louis Bossart wrote:
> 
>> +static int avs_dsp_enable(struct avs_dev *adev, u32 core_mask)
>> +{
>> +	u32 mask;
>> +	int ret;
>> +
>> +	ret = avs_dsp_core_enable(adev, core_mask);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	mask = core_mask & ~AVS_MAIN_CORE_MASK;
> 
> so here BIT(MAIN_CORE) is zero in mask


What's wrong with AVS_MAIN_CORE_MASK being used explicitly?

>> +	if (!mask)
>> +		/*
>> +		 * without main core, fw is dead anyway
>> +		 * so setting D0 for it is futile.
> 
> I don't get the comment, you explicitly discarded the main core with
> your logical AND above, so this test means that all other non-main cores
> are disabled.

There is no setting D0 for MAIN_CORE as firmware is not operational 
without it. Firmware needs to be notified about D3 -> D0 transitions 
only in case of non-MAIN_COREs.


>> +		 */
>> +		return 0;
>> +
>> +	ret = avs_ipc_set_dx(adev, mask, true);
>> +	return AVS_IPC_RET(ret);
>> +}
>> +
>> +static int avs_dsp_disable(struct avs_dev *adev, u32 core_mask)
>> +{
>> +	int ret;
>> +
>> +	ret = avs_ipc_set_dx(adev, core_mask, false);
>> +	if (ret)
>> +		return AVS_IPC_RET(ret);
>> +
>> +	return avs_dsp_core_disable(adev, core_mask);
>> +}
>> +
>> +static int avs_dsp_get_core(struct avs_dev *adev, u32 core_id)
>> +{
>> +	u32 mask;
>> +	int ret;
>> +
>> +	mask = BIT_MASK(core_id);
>> +	if (mask == AVS_MAIN_CORE_MASK)
>> +		/* nothing to do for main core */
>> +		return 0;
>> +	if (core_id >= adev->hw_cfg.dsp_cores) {
>> +		ret = -EINVAL;
>> +		goto err;
>> +	}
>> +
>> +	adev->core_refs[core_id]++;
>> +	if (adev->core_refs[core_id] == 1) {
>> +		ret = avs_dsp_enable(adev, mask);
>> +		if (ret)
>> +			goto err_enable_dsp;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_enable_dsp:
>> +	adev->core_refs[core_id]--;
>> +err:
>> +	dev_err(adev->dev, "get core failed: %d\n", ret);
> 
> you should log which core could not be enabled


Good catch! Ack.

>> +	return ret;
>> +}
>> +
>> +static int avs_dsp_put_core(struct avs_dev *adev, u32 core_id)
>> +{
>> +	u32 mask;
>> +	int ret;
>> +
>> +	mask = BIT_MASK(core_id);
>> +	if (mask == AVS_MAIN_CORE_MASK)
>> +		/* nothing to do for main core */
>> +		return 0;
>> +	if (core_id >= adev->hw_cfg.dsp_cores) {
>> +		ret = -EINVAL;
>> +		goto err;
>> +	}
>> +
>> +	adev->core_refs[core_id]--;
>> +	if (!adev->core_refs[core_id]) {
>> +		ret = avs_dsp_disable(adev, mask);
>> +		if (ret)
>> +			goto err;
>> +	}
>> +
>> +	return 0;
>> +err:
>> +	dev_err(adev->dev, "put core failed: %d\n", ret);
> 
> put core %d


Ack.

>> +	return ret;
>> +}
> 
>>   MODULE_LICENSE("GPL v2");
> 
> "GPL"


Ack.
Pierre-Louis Bossart Feb. 25, 2022, 8:21 p.m. UTC | #3 
>>> +static int avs_dsp_enable(struct avs_dev *adev, u32 core_mask)
>>> +{
>>> +    u32 mask;
>>> +    int ret;
>>> +
>>> +    ret = avs_dsp_core_enable(adev, core_mask);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    mask = core_mask & ~AVS_MAIN_CORE_MASK;
>>
>> so here BIT(MAIN_CORE) is zero in mask
> 
> 
> What's wrong with AVS_MAIN_CORE_MASK being used explicitly?
> 
>>> +    if (!mask)
>>> +        /*
>>> +         * without main core, fw is dead anyway
>>> +         * so setting D0 for it is futile.
>>
>> I don't get the comment, you explicitly discarded the main core with
>> your logical AND above, so this test means that all other non-main cores
>> are disabled.
> 
> There is no setting D0 for MAIN_CORE as firmware is not operational
> without it. Firmware needs to be notified about D3 -> D0 transitions
> only in case of non-MAIN_COREs.

the comment was about 'without main core'.

This is difficult to follow, because you've discarded the main code in
the if (!mask), so that's an always-true case, which makes the rest of
the explanations not so clear.
Cezary Rojewski Feb. 28, 2022, 3:30 p.m. UTC | #4 
On 2022-02-25 9:21 PM, Pierre-Louis Bossart wrote:
>>>> +static int avs_dsp_enable(struct avs_dev *adev, u32 core_mask)
>>>> +{
>>>> +    u32 mask;
>>>> +    int ret;
>>>> +
>>>> +    ret = avs_dsp_core_enable(adev, core_mask);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    mask = core_mask & ~AVS_MAIN_CORE_MASK;
>>>
>>> so here BIT(MAIN_CORE) is zero in mask
>>
>>
>> What's wrong with AVS_MAIN_CORE_MASK being used explicitly?
>>
>>>> +    if (!mask)
>>>> +        /*
>>>> +         * without main core, fw is dead anyway
>>>> +         * so setting D0 for it is futile.
>>>
>>> I don't get the comment, you explicitly discarded the main core with
>>> your logical AND above, so this test means that all other non-main cores
>>> are disabled.
>>
>> There is no setting D0 for MAIN_CORE as firmware is not operational
>> without it. Firmware needs to be notified about D3 -> D0 transitions
>> only in case of non-MAIN_COREs.
> 
> the comment was about 'without main core'.
> 
> This is difficult to follow, because you've discarded the main code in
> the if (!mask), so that's an always-true case, which makes the rest of
> the explanations not so clear.


I'm afraid, not seeing the problem. It's clearly not always-true 
statement as 'mask' can be non-zero after discarding the MAIN_CORE.
Pierre-Louis Bossart Feb. 28, 2022, 5:20 p.m. UTC | #5 
On 2/28/22 09:30, Cezary Rojewski wrote:
> On 2022-02-25 9:21 PM, Pierre-Louis Bossart wrote:
>>>>> +static int avs_dsp_enable(struct avs_dev *adev, u32 core_mask)
>>>>> +{
>>>>> +    u32 mask;
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = avs_dsp_core_enable(adev, core_mask);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>> +    mask = core_mask & ~AVS_MAIN_CORE_MASK;
>>>>
>>>> so here BIT(MAIN_CORE) is zero in mask
>>>
>>>
>>> What's wrong with AVS_MAIN_CORE_MASK being used explicitly?
>>>
>>>>> +    if (!mask)
>>>>> +        /*
>>>>> +         * without main core, fw is dead anyway
>>>>> +         * so setting D0 for it is futile.
>>>>
>>>> I don't get the comment, you explicitly discarded the main core with
>>>> your logical AND above, so this test means that all other non-main
>>>> cores
>>>> are disabled.
>>>
>>> There is no setting D0 for MAIN_CORE as firmware is not operational
>>> without it. Firmware needs to be notified about D3 -> D0 transitions
>>> only in case of non-MAIN_COREs.
>>
>> the comment was about 'without main core'.
>>
>> This is difficult to follow, because you've discarded the main code in
>> the if (!mask), so that's an always-true case, which makes the rest of
>> the explanations not so clear.
> 
> 
> I'm afraid, not seeing the problem. It's clearly not always-true
> statement as 'mask' can be non-zero after discarding the MAIN_CORE.

mask = core_mask & ~AVS_MAIN_CORE_MASK;

-> main core is ignored.

comment says "without main core, fw is dead anyway"

since you ignored the main core, is fw dead in all cases?
diff mbox series

Patch

diff --git a/sound/soc/intel/avs/avs.h b/sound/soc/intel/avs/avs.h
index d12b19a7299b..8779fe9fd8c3 100644
--- a/sound/soc/intel/avs/avs.h
+++ b/sound/soc/intel/avs/avs.h
@@ -74,6 +74,7 @@  struct avs_dev {
 	struct mutex modres_mutex;
 	struct ida ppl_ida;
 	struct list_head fw_list;
+	int *core_refs;
 
 	struct completion fw_ready;
 };
@@ -183,4 +184,13 @@  void avs_module_id_free(struct avs_dev *adev, u16 module_id, u8 instance_id);
 int avs_request_firmware(struct avs_dev *adev, const struct firmware **fw_p, const char *name);
 void avs_release_firmwares(struct avs_dev *adev);
 
+int avs_dsp_init_module(struct avs_dev *adev, u16 module_id, u8 ppl_instance_id,
+			u8 core_id, u8 domain, void *param, u32 param_size,
+			u16 *instance_id);
+void avs_dsp_delete_module(struct avs_dev *adev, u16 module_id, u16 instance_id,
+			   u8 ppl_instance_id, u8 core_id);
+int avs_dsp_create_pipeline(struct avs_dev *adev, u16 req_size, u8 priority,
+			    bool lp, u16 attributes, u8 *instance_id);
+int avs_dsp_delete_pipeline(struct avs_dev *adev, u8 instance_id);
+
 #endif /* __SOUND_SOC_INTEL_AVS_H */
diff --git a/sound/soc/intel/avs/dsp.c b/sound/soc/intel/avs/dsp.c
index 258544277bbb..c0291516809d 100644
--- a/sound/soc/intel/avs/dsp.c
+++ b/sound/soc/intel/avs/dsp.c
@@ -104,4 +104,174 @@  int avs_dsp_core_disable(struct avs_dev *adev, u32 core_mask)
 	return avs_dsp_op(adev, power, core_mask, false);
 }
 
+static int avs_dsp_enable(struct avs_dev *adev, u32 core_mask)
+{
+	u32 mask;
+	int ret;
+
+	ret = avs_dsp_core_enable(adev, core_mask);
+	if (ret < 0)
+		return ret;
+
+	mask = core_mask & ~AVS_MAIN_CORE_MASK;
+	if (!mask)
+		/*
+		 * without main core, fw is dead anyway
+		 * so setting D0 for it is futile.
+		 */
+		return 0;
+
+	ret = avs_ipc_set_dx(adev, mask, true);
+	return AVS_IPC_RET(ret);
+}
+
+static int avs_dsp_disable(struct avs_dev *adev, u32 core_mask)
+{
+	int ret;
+
+	ret = avs_ipc_set_dx(adev, core_mask, false);
+	if (ret)
+		return AVS_IPC_RET(ret);
+
+	return avs_dsp_core_disable(adev, core_mask);
+}
+
+static int avs_dsp_get_core(struct avs_dev *adev, u32 core_id)
+{
+	u32 mask;
+	int ret;
+
+	mask = BIT_MASK(core_id);
+	if (mask == AVS_MAIN_CORE_MASK)
+		/* nothing to do for main core */
+		return 0;
+	if (core_id >= adev->hw_cfg.dsp_cores) {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	adev->core_refs[core_id]++;
+	if (adev->core_refs[core_id] == 1) {
+		ret = avs_dsp_enable(adev, mask);
+		if (ret)
+			goto err_enable_dsp;
+	}
+
+	return 0;
+
+err_enable_dsp:
+	adev->core_refs[core_id]--;
+err:
+	dev_err(adev->dev, "get core failed: %d\n", ret);
+	return ret;
+}
+
+static int avs_dsp_put_core(struct avs_dev *adev, u32 core_id)
+{
+	u32 mask;
+	int ret;
+
+	mask = BIT_MASK(core_id);
+	if (mask == AVS_MAIN_CORE_MASK)
+		/* nothing to do for main core */
+		return 0;
+	if (core_id >= adev->hw_cfg.dsp_cores) {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	adev->core_refs[core_id]--;
+	if (!adev->core_refs[core_id]) {
+		ret = avs_dsp_disable(adev, mask);
+		if (ret)
+			goto err;
+	}
+
+	return 0;
+err:
+	dev_err(adev->dev, "put core failed: %d\n", ret);
+	return ret;
+}
+
+int avs_dsp_init_module(struct avs_dev *adev, u16 module_id, u8 ppl_instance_id,
+			u8 core_id, u8 domain, void *param, u32 param_size,
+			u16 *instance_id)
+{
+	struct avs_module_entry mentry;
+	int ret, id;
+
+	id = avs_module_id_alloc(adev, module_id);
+	if (id < 0)
+		return id;
+
+	ret = avs_get_module_id_entry(adev, module_id, &mentry);
+	if (ret)
+		goto err_mod_entry;
+
+	ret = avs_dsp_get_core(adev, core_id);
+	if (ret)
+		goto err_mod_entry;
+
+	ret = avs_ipc_init_instance(adev, module_id, id, ppl_instance_id,
+				    core_id, domain, param, param_size);
+	if (ret) {
+		ret = AVS_IPC_RET(ret);
+		goto err_ipc;
+	}
+
+	*instance_id = id;
+	return 0;
+
+err_ipc:
+	avs_dsp_put_core(adev, core_id);
+err_mod_entry:
+	avs_module_id_free(adev, module_id, id);
+	return ret;
+}
+
+void avs_dsp_delete_module(struct avs_dev *adev, u16 module_id, u16 instance_id,
+			   u8 ppl_instance_id, u8 core_id)
+{
+	/* Modules not owned by any pipeline need to be freed explicitly. */
+	if (ppl_instance_id == INVALID_PIPELINE_ID)
+		avs_ipc_delete_instance(adev, module_id, instance_id);
+
+	avs_module_id_free(adev, module_id, instance_id);
+
+	avs_dsp_put_core(adev, core_id);
+}
+
+int avs_dsp_create_pipeline(struct avs_dev *adev, u16 req_size, u8 priority,
+			    bool lp, u16 attributes, u8 *instance_id)
+{
+	struct avs_fw_cfg *fw_cfg = &adev->fw_cfg;
+	int ret, id;
+
+	id = ida_alloc_max(&adev->ppl_ida, fw_cfg->max_ppl_count - 1, GFP_KERNEL);
+	if (id < 0)
+		return id;
+
+	ret = avs_ipc_create_pipeline(adev, req_size, priority, id, lp,
+				      attributes);
+	if (ret) {
+		ida_free(&adev->ppl_ida, id);
+		return AVS_IPC_RET(ret);
+	}
+
+	*instance_id = id;
+	return 0;
+}
+
+int avs_dsp_delete_pipeline(struct avs_dev *adev, u8 instance_id)
+{
+	int ret;
+
+	ret = avs_ipc_delete_pipeline(adev, instance_id);
+	if (ret)
+		ret = AVS_IPC_RET(ret);
+
+	ida_free(&adev->ppl_ida, instance_id);
+	return ret;
+}
+
 MODULE_LICENSE("GPL v2");