| Message ID | 20220426172346.3508411-2-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: Intel: avs: Driver core and PCM operations | expand |
On 4/26/22 12:23, Cezary Rojewski wrote: > Not all modules are part of base firmware. Some are part of loadable > libraries. These need to be loaded after base firmware reports ready > status through FW_READY notification. > > Their loading process is similar to the base firmware's one. Request the > binary file, verify and strip the manifest and load the actual code into > DSP memory with help of CLDMA or HD-Audio render stream, depending on > audio device generation. > > List of libraries needed for loading is obtained through the topology - > vendor sections specifying the name of firmware files to request. > > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> > --- > sound/soc/intel/avs/avs.h | 3 ++ > sound/soc/intel/avs/loader.c | 79 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 82 insertions(+) > > diff --git a/sound/soc/intel/avs/avs.h b/sound/soc/intel/avs/avs.h > index c57a07a18d8e..14b4a780a91c 100644 > --- a/sound/soc/intel/avs/avs.h > +++ b/sound/soc/intel/avs/avs.h > @@ -19,6 +19,8 @@ > > struct avs_dev; > struct avs_tplg; > +struct avs_tplg_library; > +struct avs_soc_component; > > /* > * struct avs_dsp_ops - Platform-specific DSP operations > @@ -241,6 +243,7 @@ void avs_hda_clock_gating_enable(struct avs_dev *adev, bool enable); > void avs_hda_power_gating_enable(struct avs_dev *adev, bool enable); > void avs_hda_l1sen_enable(struct avs_dev *adev, bool enable); > > +int avs_dsp_load_libraries(struct avs_dev *adev, struct avs_tplg_library *libs, u32 num_libs); > int avs_dsp_boot_firmware(struct avs_dev *adev, bool purge); > int avs_dsp_first_boot_firmware(struct avs_dev *adev); > > diff --git a/sound/soc/intel/avs/loader.c b/sound/soc/intel/avs/loader.c > index c47f85161d95..de98f4c3adf8 100644 > --- a/sound/soc/intel/avs/loader.c > +++ b/sound/soc/intel/avs/loader.c > @@ -15,6 +15,7 @@ > #include "cldma.h" > #include "messages.h" > #include "registers.h" > +#include "topology.h" > > #define AVS_ROM_STS_MASK 0xFF > #define AVS_ROM_INIT_DONE 0x1 > @@ -466,6 +467,70 @@ int avs_hda_transfer_modules(struct avs_dev *adev, bool load, > return 0; > } > > +int avs_dsp_load_libraries(struct avs_dev *adev, struct avs_tplg_library *libs, u32 num_libs) > +{ > + int start, id, i = 0; > + int ret; > + > + /* Calculate the id to assign for the next lib. */ > + for (id = 0; id < adev->fw_cfg.max_libs_count; id++) > + if (adev->lib_names[id][0] == '\0') > + break; > + if (id + num_libs >= adev->fw_cfg.max_libs_count) > + return -EINVAL; use ida_alloc_max() ? > + > + start = id; > + while (i < num_libs) { > + struct avs_fw_manifest *man; > + const struct firmware *fw; > + struct firmware stripped_fw; > + char *filename; > + int j; > + > + filename = kasprintf(GFP_KERNEL, "%s/%s/%s", AVS_ROOT_DIR, adev->spec->name, > + libs[i].name); > + if (!filename) > + return -ENOMEM; > + > + ret = avs_request_firmware(adev, &fw, filename); > + kfree(filename); > + if (ret < 0) > + return ret; > + > + stripped_fw = *fw; > + ret = avs_fw_manifest_strip_verify(adev, &stripped_fw, NULL); > + if (ret) { > + dev_err(adev->dev, "invalid library data: %d\n", ret); > + goto release_fw; > + } > + > + ret = avs_fw_manifest_offset(&stripped_fw); > + if (ret < 0) > + goto release_fw; > + man = (struct avs_fw_manifest *)(stripped_fw.data + ret); > + > + /* Don't load anything that's already in DSP memory. */ > + for (j = 0; j < id; j++) > + if (!strncmp(adev->lib_names[j], man->name, AVS_LIB_NAME_SIZE)) > + goto next_lib; > + > + ret = avs_dsp_op(adev, load_lib, &stripped_fw, id); > + if (ret) > + goto release_fw; > + > + strncpy(adev->lib_names[id], man->name, AVS_LIB_NAME_SIZE); > + id++; > +next_lib: > + i++; > + } > + > + return start == id ? 1 : 0; > + > +release_fw: > + avs_release_last_firmware(adev); why release only the last library and not all the ones that were previous loaded? or why bother to even release the last since at this point you probably need to restart completely? > + return ret; > +} > + > static int avs_dsp_load_basefw(struct avs_dev *adev) > { > const struct avs_fw_version *min_req; > @@ -519,6 +584,7 @@ static int avs_dsp_load_basefw(struct avs_dev *adev) > > int avs_dsp_boot_firmware(struct avs_dev *adev, bool purge) > { > + struct avs_soc_component *acomp; > int ret, i; > > /* Forgo full boot if flash from IMR succeeds. */ > @@ -538,7 +604,20 @@ int avs_dsp_boot_firmware(struct avs_dev *adev, bool purge) > avs_hda_l1sen_enable(adev, false); > > ret = avs_dsp_load_basefw(adev); > + if (ret) > + goto reenable_gating; > + > + mutex_lock(&adev->comp_list_mutex); > + list_for_each_entry(acomp, &adev->comp_list, node) { > + struct avs_tplg *tplg = acomp->tplg; > + > + ret = avs_dsp_load_libraries(adev, tplg->libs, tplg->num_libs); > + if (ret < 0) > + break; > + } > + mutex_unlock(&adev->comp_list_mutex); > > +reenable_gating: > avs_hda_l1sen_enable(adev, true); > avs_hda_clock_gating_enable(adev, true); >
On 2022-04-26 11:21 PM, Pierre-Louis Bossart wrote: > On 4/26/22 12:23, Cezary Rojewski wrote: ... >> diff --git a/sound/soc/intel/avs/loader.c b/sound/soc/intel/avs/loader.c >> index c47f85161d95..de98f4c3adf8 100644 >> --- a/sound/soc/intel/avs/loader.c >> +++ b/sound/soc/intel/avs/loader.c >> @@ -15,6 +15,7 @@ >> #include "cldma.h" >> #include "messages.h" >> #include "registers.h" >> +#include "topology.h" >> >> #define AVS_ROM_STS_MASK 0xFF >> #define AVS_ROM_INIT_DONE 0x1 >> @@ -466,6 +467,70 @@ int avs_hda_transfer_modules(struct avs_dev *adev, bool load, >> return 0; >> } >> >> +int avs_dsp_load_libraries(struct avs_dev *adev, struct avs_tplg_library *libs, u32 num_libs) >> +{ >> + int start, id, i = 0; >> + int ret; >> + >> + /* Calculate the id to assign for the next lib. */ >> + for (id = 0; id < adev->fw_cfg.max_libs_count; id++) >> + if (adev->lib_names[id][0] == '\0') >> + break; >> + if (id + num_libs >= adev->fw_cfg.max_libs_count) >> + return -EINVAL; > > use ida_alloc_max() ? After reading this one couple of times I'm keen to agree that IDA should have been used for library ID allocation and a at the same time, surprised it has't done that already. Till now we used IDA 'only' when allocating pipeline IDs and module instance IDs. Pipeline allocation is good comparison here - makes use of ida_alloc_max() already - library one should follow. This finding is much appreciated, Pierre. >> + >> + start = id; >> + while (i < num_libs) { >> + struct avs_fw_manifest *man; >> + const struct firmware *fw; >> + struct firmware stripped_fw; >> + char *filename; >> + int j; >> + >> + filename = kasprintf(GFP_KERNEL, "%s/%s/%s", AVS_ROOT_DIR, adev->spec->name, >> + libs[i].name); >> + if (!filename) >> + return -ENOMEM; >> + >> + ret = avs_request_firmware(adev, &fw, filename); >> + kfree(filename); >> + if (ret < 0) >> + return ret; >> + >> + stripped_fw = *fw; >> + ret = avs_fw_manifest_strip_verify(adev, &stripped_fw, NULL); >> + if (ret) { >> + dev_err(adev->dev, "invalid library data: %d\n", ret); >> + goto release_fw; >> + } >> + >> + ret = avs_fw_manifest_offset(&stripped_fw); >> + if (ret < 0) >> + goto release_fw; >> + man = (struct avs_fw_manifest *)(stripped_fw.data + ret); >> + >> + /* Don't load anything that's already in DSP memory. */ >> + for (j = 0; j < id; j++) >> + if (!strncmp(adev->lib_names[j], man->name, AVS_LIB_NAME_SIZE)) >> + goto next_lib; >> + >> + ret = avs_dsp_op(adev, load_lib, &stripped_fw, id); >> + if (ret) >> + goto release_fw; >> + >> + strncpy(adev->lib_names[id], man->name, AVS_LIB_NAME_SIZE); >> + id++; >> +next_lib: >> + i++; >> + } >> + >> + return start == id ? 1 : 0; >> + >> +release_fw: >> + avs_release_last_firmware(adev); > > why release only the last library and not all the ones that were previous loaded? > or why bother to even release the last since at this point you probably need to restart completely? Yes, avs_release_last_firmware() is used to clean 'locally' and indeed, failing to load a library will most likely end-up is complete restart. I'll provide an internal build with this part removed and have validation do some testing first as performing avs_release_last_firmware() keeps things sane i.e. no invalid entries in the ->fw_list, no unnecessarily occupied memory for the already invalid entry and such. In regard to *why is only last one released*, it's tied to how base firmware behaves. Assuming a scenario where two libraries need to be loaded, if the loading procedure for the second fails, base firmware will NOT unload/unmap allocated memory and resources for the first one. At the same time, there is no capability to unload library on demand. In order to rollback the transaction, DSP has to be shut down entirely, just like if we were talking about firmware exception or IPC timeout. So, by doing what we do here, driver reflects firmware side (MODULES_INFO) basically 1:1. Another good one, Pierre. >> + return ret; >> +}
On 2022-05-01 11:45, Cezary Rojewski wrote: > On 2022-04-26 11:21 PM, Pierre-Louis Bossart wrote: >> On 4/26/22 12:23, Cezary Rojewski wrote: > > ... > >>> diff --git a/sound/soc/intel/avs/loader.c >>> b/sound/soc/intel/avs/loader.c >>> index c47f85161d95..de98f4c3adf8 100644 >>> --- a/sound/soc/intel/avs/loader.c >>> +++ b/sound/soc/intel/avs/loader.c >>> @@ -15,6 +15,7 @@ >>> #include "cldma.h" >>> #include "messages.h" >>> #include "registers.h" >>> +#include "topology.h" >>> #define AVS_ROM_STS_MASK 0xFF >>> #define AVS_ROM_INIT_DONE 0x1 >>> @@ -466,6 +467,70 @@ int avs_hda_transfer_modules(struct avs_dev >>> *adev, bool load, >>> return 0; >>> } >>> +int avs_dsp_load_libraries(struct avs_dev *adev, struct >>> avs_tplg_library *libs, u32 num_libs) >>> +{ >>> + int start, id, i = 0; >>> + int ret; >>> + >>> + /* Calculate the id to assign for the next lib. */ >>> + for (id = 0; id < adev->fw_cfg.max_libs_count; id++) >>> + if (adev->lib_names[id][0] == '\0') >>> + break; >>> + if (id + num_libs >= adev->fw_cfg.max_libs_count) >>> + return -EINVAL; >> >> use ida_alloc_max() ? > > > After reading this one couple of times I'm keen to agree that IDA > should have been used for library ID allocation and a at the same > time, surprised it has't done that already. Till now we used IDA > 'only' when allocating pipeline IDs and module instance IDs. Pipeline > allocation is good comparison here - makes use of ida_alloc_max() > already - library one should follow. > > This finding is much appreciated, Pierre. I think that using ida here is a bit of an overkill. Ida works fine when there can be both id allocation and freeing and that's how it work with pipelines and modules IDs in avs. However there is no mechanism for unloading libraries in cAVS firmware, therefore ida would be used here only to increase the ID, so it needlessly complicates the code while not giving much of a benefit. Also our approach to check if we can load all libraries before the loop makes it problematic with ida because we would need to allocate an id at start and calculate if all libs would fit and then either free it instantly or complicate the loop to use id allocated before. Therefore I suggest to leave this code unchanged. I've synced with Cezary on this and provided explanation convinced him too. > >>> + >>> + start = id; >>> + while (i < num_libs) { >>> + struct avs_fw_manifest *man; >>> + const struct firmware *fw; >>> + struct firmware stripped_fw; >>> + char *filename; >>> + int j; >>> + >>> + filename = kasprintf(GFP_KERNEL, "%s/%s/%s", AVS_ROOT_DIR, >>> adev->spec->name, >>> + libs[i].name); >>> + if (!filename) >>> + return -ENOMEM; >>> + >>> + ret = avs_request_firmware(adev, &fw, filename); >>> + kfree(filename); >>> + if (ret < 0) >>> + return ret; >>> + >>> + stripped_fw = *fw; >>> + ret = avs_fw_manifest_strip_verify(adev, &stripped_fw, NULL); >>> + if (ret) { >>> + dev_err(adev->dev, "invalid library data: %d\n", ret); >>> + goto release_fw; >>> + } >>> + >>> + ret = avs_fw_manifest_offset(&stripped_fw); >>> + if (ret < 0) >>> + goto release_fw; >>> + man = (struct avs_fw_manifest *)(stripped_fw.data + ret); >>> + >>> + /* Don't load anything that's already in DSP memory. */ >>> + for (j = 0; j < id; j++) >>> + if (!strncmp(adev->lib_names[j], man->name, >>> AVS_LIB_NAME_SIZE)) >>> + goto next_lib; >>> + >>> + ret = avs_dsp_op(adev, load_lib, &stripped_fw, id); >>> + if (ret) >>> + goto release_fw; >>> + >>> + strncpy(adev->lib_names[id], man->name, AVS_LIB_NAME_SIZE); >>> + id++; >>> +next_lib: >>> + i++; >>> + } >>> + >>> + return start == id ? 1 : 0; >>> + >>> +release_fw: >>> + avs_release_last_firmware(adev); >> >> why release only the last library and not all the ones that were >> previous loaded? >> or why bother to even release the last since at this point you >> probably need to restart completely? > > > Yes, avs_release_last_firmware() is used to clean 'locally' and > indeed, failing to load a library will most likely end-up is complete > restart. > > I'll provide an internal build with this part removed and have > validation do some testing first as performing > avs_release_last_firmware() keeps things sane i.e. no invalid entries > in the ->fw_list, no unnecessarily occupied memory for the already > invalid entry and such. > > In regard to *why is only last one released*, it's tied to how base > firmware behaves. Assuming a scenario where two libraries need to be > loaded, if the loading procedure for the second fails, base firmware > will NOT unload/unmap allocated memory and resources for the first > one. At the same time, there is no capability to unload library on > demand. In order to rollback the transaction, DSP has to be shut down > entirely, just like if we were talking about firmware exception or IPC > timeout. > > So, by doing what we do here, driver reflects firmware side > (MODULES_INFO) basically 1:1. > > Another good one, Pierre. > >>> + return ret; >>> +}
>>>> +int avs_dsp_load_libraries(struct avs_dev *adev, struct >>>> avs_tplg_library *libs, u32 num_libs) >>>> +{ >>>> + int start, id, i = 0; >>>> + int ret; >>>> + >>>> + /* Calculate the id to assign for the next lib. */ >>>> + for (id = 0; id < adev->fw_cfg.max_libs_count; id++) >>>> + if (adev->lib_names[id][0] == '\0') >>>> + break; >>>> + if (id + num_libs >= adev->fw_cfg.max_libs_count) >>>> + return -EINVAL; >>> >>> use ida_alloc_max() ? >> >> >> After reading this one couple of times I'm keen to agree that IDA >> should have been used for library ID allocation and a at the same >> time, surprised it has't done that already. Till now we used IDA >> 'only' when allocating pipeline IDs and module instance IDs. Pipeline >> allocation is good comparison here - makes use of ida_alloc_max() >> already - library one should follow. >> >> This finding is much appreciated, Pierre. > > I think that using ida here is a bit of an overkill. Ida works fine when > there can be both id allocation and freeing and that's how it work with > pipelines and modules IDs in avs. However there is no mechanism for > unloading libraries in cAVS firmware, therefore ida would be used here > only to increase the ID, so it needlessly complicates the code while not > giving much of a benefit. Also our approach to check if we can load all > libraries before the loop makes it problematic with ida because we would > need to allocate an id at start and calculate if all libs would fit and > then either free it instantly or complicate the loop to use id allocated > before. Therefore I suggest to leave this code unchanged. I've synced > with Cezary on this and provided explanation convinced him too. That's fine, you should however capture this design decision with a comment or a clarification in the commit message. "libraries" mean different things to different people, and it's hard to review without context.
diff --git a/sound/soc/intel/avs/avs.h b/sound/soc/intel/avs/avs.h index c57a07a18d8e..14b4a780a91c 100644 --- a/sound/soc/intel/avs/avs.h +++ b/sound/soc/intel/avs/avs.h @@ -19,6 +19,8 @@ struct avs_dev; struct avs_tplg; +struct avs_tplg_library; +struct avs_soc_component; /* * struct avs_dsp_ops - Platform-specific DSP operations @@ -241,6 +243,7 @@ void avs_hda_clock_gating_enable(struct avs_dev *adev, bool enable); void avs_hda_power_gating_enable(struct avs_dev *adev, bool enable); void avs_hda_l1sen_enable(struct avs_dev *adev, bool enable); +int avs_dsp_load_libraries(struct avs_dev *adev, struct avs_tplg_library *libs, u32 num_libs); int avs_dsp_boot_firmware(struct avs_dev *adev, bool purge); int avs_dsp_first_boot_firmware(struct avs_dev *adev); diff --git a/sound/soc/intel/avs/loader.c b/sound/soc/intel/avs/loader.c index c47f85161d95..de98f4c3adf8 100644 --- a/sound/soc/intel/avs/loader.c +++ b/sound/soc/intel/avs/loader.c @@ -15,6 +15,7 @@ #include "cldma.h" #include "messages.h" #include "registers.h" +#include "topology.h" #define AVS_ROM_STS_MASK 0xFF #define AVS_ROM_INIT_DONE 0x1 @@ -466,6 +467,70 @@ int avs_hda_transfer_modules(struct avs_dev *adev, bool load, return 0; } +int avs_dsp_load_libraries(struct avs_dev *adev, struct avs_tplg_library *libs, u32 num_libs) +{ + int start, id, i = 0; + int ret; + + /* Calculate the id to assign for the next lib. */ + for (id = 0; id < adev->fw_cfg.max_libs_count; id++) + if (adev->lib_names[id][0] == '\0') + break; + if (id + num_libs >= adev->fw_cfg.max_libs_count) + return -EINVAL; + + start = id; + while (i < num_libs) { + struct avs_fw_manifest *man; + const struct firmware *fw; + struct firmware stripped_fw; + char *filename; + int j; + + filename = kasprintf(GFP_KERNEL, "%s/%s/%s", AVS_ROOT_DIR, adev->spec->name, + libs[i].name); + if (!filename) + return -ENOMEM; + + ret = avs_request_firmware(adev, &fw, filename); + kfree(filename); + if (ret < 0) + return ret; + + stripped_fw = *fw; + ret = avs_fw_manifest_strip_verify(adev, &stripped_fw, NULL); + if (ret) { + dev_err(adev->dev, "invalid library data: %d\n", ret); + goto release_fw; + } + + ret = avs_fw_manifest_offset(&stripped_fw); + if (ret < 0) + goto release_fw; + man = (struct avs_fw_manifest *)(stripped_fw.data + ret); + + /* Don't load anything that's already in DSP memory. */ + for (j = 0; j < id; j++) + if (!strncmp(adev->lib_names[j], man->name, AVS_LIB_NAME_SIZE)) + goto next_lib; + + ret = avs_dsp_op(adev, load_lib, &stripped_fw, id); + if (ret) + goto release_fw; + + strncpy(adev->lib_names[id], man->name, AVS_LIB_NAME_SIZE); + id++; +next_lib: + i++; + } + + return start == id ? 1 : 0; + +release_fw: + avs_release_last_firmware(adev); + return ret; +} + static int avs_dsp_load_basefw(struct avs_dev *adev) { const struct avs_fw_version *min_req; @@ -519,6 +584,7 @@ static int avs_dsp_load_basefw(struct avs_dev *adev) int avs_dsp_boot_firmware(struct avs_dev *adev, bool purge) { + struct avs_soc_component *acomp; int ret, i; /* Forgo full boot if flash from IMR succeeds. */ @@ -538,7 +604,20 @@ int avs_dsp_boot_firmware(struct avs_dev *adev, bool purge) avs_hda_l1sen_enable(adev, false); ret = avs_dsp_load_basefw(adev); + if (ret) + goto reenable_gating; + + mutex_lock(&adev->comp_list_mutex); + list_for_each_entry(acomp, &adev->comp_list, node) { + struct avs_tplg *tplg = acomp->tplg; + + ret = avs_dsp_load_libraries(adev, tplg->libs, tplg->num_libs); + if (ret < 0) + break; + } + mutex_unlock(&adev->comp_list_mutex); +reenable_gating: avs_hda_l1sen_enable(adev, true); avs_hda_clock_gating_enable(adev, true);
Not all modules are part of base firmware. Some are part of loadable libraries. These need to be loaded after base firmware reports ready status through FW_READY notification. Their loading process is similar to the base firmware's one. Request the binary file, verify and strip the manifest and load the actual code into DSP memory with help of CLDMA or HD-Audio render stream, depending on audio device generation. List of libraries needed for loading is obtained through the topology - vendor sections specifying the name of firmware files to request. Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> --- sound/soc/intel/avs/avs.h | 3 ++ sound/soc/intel/avs/loader.c | 79 ++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+)