Skip to content

Add support for dynamic loading of module libraries for IPC4#3720

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ranj063 wants to merge 10 commits into from
Closed

Add support for dynamic loading of module libraries for IPC4 #3720

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
88f33a7
ASoC: SOF: loader: Set complete state before post_fw_run op
ranj063 Jun 25, 2022
0d166a2
ASoC: SOF: IPC4: Introduce 2 new fields in struct sof_ipc4_fw_data
ranj063 Jun 25, 2022
49b52be
ASoC: SOF: loader: Modify the signature of parse_ext_manifest op
ranj063 Jun 25, 2022
775cb46
ASoC: SOF: Add new DSP op for module library loading
ranj063 Jun 25, 2022
9ff992b
ASoC: SOF: Intel: MTL: set the load_library op
ranj063 Jul 20, 2022
4bbbe9a
ASoC: SOF: Intel: APL: set the load_library op
ranj063 Jul 20, 2022
9d62fff
ASoC: SOF: Intel: TGL: set the load_library op
ranj063 Jul 20, 2022
8348e34
ASoC: SOF: Intel: CNL: set the load_library op
ranj063 Jul 20, 2022
b0aae2b
ASoC: SOF: Add load_library op to struct sof_ipc_fw_loader_ops
ranj063 Jun 25, 2022
fc391aa
ASoC: SOF: Add a new fw loader op to reload_module libraries
ranj063 Jun 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions include/sound/sof/ipc4/header.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -185,6 +185,10 @@ enum sof_ipc4_pipeline_state {
#define SOF_IPC4_GLB_PIPE_STATE_MASK GENMASK(15, 0)
#define SOF_IPC4_GLB_PIPE_STATE(x) ((x) << SOF_IPC4_GLB_PIPE_STATE_SHIFT)

/* load library ipc msg */
#define SOF_IPC4_GLB_LOAD_LIBRARY_LIB_ID_SHIFT 16
#define SOF_IPC4_GLB_LOAD_LIBRARY_LIB_ID(x) ((x) << SOF_IPC4_GLB_LOAD_LIBRARY_LIB_ID_SHIFT)
Comment thread
plbossart marked this conversation as resolved.

Comment thread
ranj063 marked this conversation as resolved.
enum sof_ipc4_channel_config {
/* one channel only. */
SOF_IPC4_CHANNEL_CONFIG_MONO,
Expand Down
3 changes: 3 additions & 0 deletions sound/soc/sof/intel/apl.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -64,6 +64,9 @@ int sof_apl_ops_init(struct snd_sof_dev *sdev)

/* ipc */
sof_apl_ops.send_msg = hda_dsp_ipc4_send_msg;

/* module library load */
sof_apl_ops.load_library = hda_dsp_ipc4_load_library;
}

/* set DAI driver ops */
Expand Down
3 changes: 3 additions & 0 deletions sound/soc/sof/intel/cnl.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -370,6 +370,9 @@ int sof_cnl_ops_init(struct snd_sof_dev *sdev)

/* ipc */
sof_cnl_ops.send_msg = cnl_ipc4_send_msg;

/* module library load */
sof_cnl_ops.load_library = hda_dsp_ipc4_load_library;
}

/* set DAI driver ops */
Expand Down
70 changes: 69 additions & 1 deletion sound/soc/sof/intel/hda-loader.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -19,6 +19,7 @@
#include <sound/hdaudio_ext.h>
#include <sound/hda_register.h>
#include <sound/sof.h>
#include <sound/sof/ipc4/header.h>
#include "ext_manifest.h"
#include "../ops.h"
#include "../sof-priv.h"
Expand DownExpand Up@@ -382,6 +383,64 @@ static int hda_dsp_boot_imr(struct snd_sof_dev *sdev)
return ret;
}

int hda_dsp_ipc4_load_library(struct snd_sof_dev *sdev,
struct snd_sof_module_library_info *lib_info)
{
struct hdac_ext_stream *hext_stream;
struct firmware stripped_firmware;
struct snd_dma_buffer dmab;
struct sof_ipc4_msg msg = {};
int ret, ret1;

stripped_firmware.data = lib_info->fw->data + lib_info->fw_offset;
stripped_firmware.size = lib_info->fw->size - lib_info->fw_offset;

@plbossartplbossartJul 21, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe check that the offset is smaller than a size?

@ranj063ranj063Jul 21, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the parse_ext_manifest op already takes of it isnt it?

@plbossartplbossartJul 22, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no idea, a comment wouldn't hurt :-)


/* prepare DMA for code loader stream */
hext_stream = hda_cl_stream_prepare(sdev, HDA_CL_STREAM_FORMAT,
stripped_firmware.size,
&dmab, SNDRV_PCM_STREAM_PLAYBACK);
if (IS_ERR(hext_stream)) {
dev_err(sdev->dev, "DMA prepare failed\n");
return PTR_ERR(hext_stream);
}

memcpy(dmab.area, stripped_firmware.data, stripped_firmware.size);

msg.primary = hext_stream->hstream.stream_tag - 1;
msg.primary |= SOF_IPC4_MSG_TYPE_SET(SOF_IPC4_GLB_LOAD_LIBRARY);
msg.primary |= SOF_IPC4_MSG_DIR(SOF_IPC4_MSG_REQUEST);
msg.primary |= SOF_IPC4_MSG_TARGET(SOF_IPC4_FW_GEN_MSG);
msg.primary |= SOF_IPC4_GLB_LOAD_LIBRARY_LIB_ID(lib_info->id);
Comment thread
plbossart marked this conversation as resolved.

@bardliaobardliaoJun 29, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move those IPC4 stuff to a new helper function in ipc4.c?

@ranj063ranj063Jul 20, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean the entire load_library function? and this is HDA specific no? how can I move to the common code outside the intel directory?

@ujfalusiujfalusiAug 12, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is highly HDA specific message and sequence, let's add a comment and think about the abstraction if the need ever arise

ret = cl_trigger(sdev, hext_stream, SNDRV_PCM_TRIGGER_START);
if (ret < 0) {
dev_err(sdev->dev, "DMA trigger start failed\n");
goto cleanup;
}

ret = sof_ipc_tx_message(sdev->ipc, &msg, 0, NULL, 0);

ret1 = cl_trigger(sdev, hext_stream, SNDRV_PCM_TRIGGER_STOP);

@ujfalusiujfalusiJun 28, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the reply is only going to be received when the module is loaded by the DMA or they go in parallel? Is there a race here? If the module is 'big' we might have a timeout or we stop the channel before the whole module is sent down?

@ranj063ranj063Jul 20, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ujfalusi I'd assume the reply would be recieved after the module library is loaded. And if the module is big and takes too long and we don't receive the reply, the time will timeout.

if (ret1 < 0) {
dev_err(sdev->dev, "DMA trigger stop failed\n");
if (!ret)
ret = ret1;
}

cleanup:
/* clean up even in case of error and return the first error */
ret1 = hda_cl_cleanup(sdev, &dmab, hext_stream);
if (ret1 < 0) {
dev_err(sdev->dev, "Code loader DSP cleanup failed\n");

/* set return value to indicate cleanup failure */
if (!ret)
ret = ret1;
}

return ret;
}

int hda_dsp_cl_boot_firmware(struct snd_sof_dev *sdev)
{
struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
Expand DownExpand Up@@ -479,7 +538,6 @@ int hda_dsp_cl_boot_firmware(struct snd_sof_dev *sdev)
ret = hda_cl_copy_fw(sdev, hext_stream);
if (!ret) {
dev_dbg(sdev->dev, "Firmware download successful, booting...\n");
hda->skip_imr_boot = false;
} else {
snd_sof_dsp_dbg_dump(sdev, "Firmware download failed",
SOF_DBG_DUMP_PCI | SOF_DBG_DUMP_MBOX);
Expand DownExpand Up@@ -525,6 +583,7 @@ int hda_dsp_pre_fw_run(struct snd_sof_dev *sdev)
/* post fw run operations */
int hda_dsp_post_fw_run(struct snd_sof_dev *sdev)
{
struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
int ret;

if (sdev->first_boot) {
Expand All@@ -542,8 +601,17 @@ int hda_dsp_post_fw_run(struct snd_sof_dev *sdev)
(sdev->fw_ready.flags & SOF_IPC_INFO_D3_PERSISTENT ||
sdev->pdata->ipc_type == SOF_INTEL_IPC4))
hdev->imrboot_supported = true;
} else {
/* reload all 3rd party module libraries during cold boot */
if (!hda->imrboot_supported || hda->skip_imr_boot) {
ret = sdev->ipc->ops->fw_loader->reload_libraries(sdev);

@ujfalusiujfalusiJun 28, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be done in higher level?
Adding a new flag hda flag to indicate that we have cold booted (or imr booted) and skip the sof_ops(sdev)->load_library in Intel arch code if there is no need for the reload?

@ranj063ranj063Jul 20, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

load_library happens during topology parsing and it will be not done during resume. So we need an explicit way to do this either during resume or here inpost_fw_run

@ujfalusiujfalusiAug 12, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The callback should be validated, this will Oops with IPC3 after a firmware crash or when waking up from hibernation, no?

if (ret < 0)
return ret;
}
}

hda->skip_imr_boot = false;

hda_sdw_int_enable(sdev, true);

/* re-enable clock gating and power gating */
Expand Down
2 changes: 2 additions & 0 deletions sound/soc/sof/intel/hda.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -851,5 +851,7 @@ int cnl_ipc4_send_msg(struct snd_sof_dev *sdev, struct snd_sof_ipc_msg *msg);
irqreturn_t hda_dsp_ipc4_irq_thread(int irq, void *context);
int hda_dsp_ipc4_send_msg(struct snd_sof_dev *sdev, struct snd_sof_ipc_msg *msg);
extern struct sdw_intel_ops sdw_callback;
int hda_dsp_ipc4_load_library(struct snd_sof_dev *sdev,
struct snd_sof_module_library_info *lib_info);

#endif
3 changes: 3 additions & 0 deletions sound/soc/sof/intel/mtl.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -749,6 +749,9 @@ int sof_mtl_ops_init(struct snd_sof_dev *sdev)
/* parse platform specific extended manifest */
sof_mtl_ops.parse_platform_ext_manifest = NULL;

/* module library load */
sof_mtl_ops.load_library = hda_dsp_ipc4_load_library;

/* dsp core get/put */
/* TODO: add core_get and core_put */

Expand Down
3 changes: 3 additions & 0 deletions sound/soc/sof/intel/tgl.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -87,6 +87,9 @@ int sof_tgl_ops_init(struct snd_sof_dev *sdev)

/* ipc */
sof_tgl_ops.send_msg = cnl_ipc4_send_msg;

/* module library load */
sof_tgl_ops.load_library = hda_dsp_ipc4_load_library;
}

/* set DAI driver ops */
Expand Down
5 changes: 2 additions & 3 deletions sound/soc/sof/ipc3-loader.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -136,10 +136,9 @@ static ssize_t ipc3_fw_ext_man_size(struct snd_sof_dev *sdev, const struct firmw
return 0;
}

static size_t sof_ipc3_fw_parse_ext_man(struct snd_sof_dev *sdev)
Comment thread
ranj063 marked this conversation as resolved.
static size_t sof_ipc3_fw_parse_ext_man(struct snd_sof_dev *sdev, const struct firmware *fw,
u32 lib_index, u32 flags)
{
struct snd_sof_pdata *plat_data = sdev->pdata;
const struct firmware *fw = plat_data->fw;
const struct sof_ext_man_elem_header *elem_hdr;
const struct sof_ext_man_header *head;
ssize_t ext_man_size;
Expand Down
Loading