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;
+}
