| Message ID | 20220207122108.3780926-14-cezary.rojewski@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [01/17] ALSA: hda: Add helper macros for DSP capable devices | expand |
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.
>>> +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.
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.
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 --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");