Skip to content

ASoC: SOF: ipc4-topology: Improve playback dai copier in/out format selection #5396

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

Open
ujfalusi wants to merge 2 commits into
base: topic/sof-dev
Choose a base branch
from
+53 −25
Open

ASoC: SOF: ipc4-topology: Improve playback dai copier in/out format selection #5396

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ce5e4f
ASoC: SOF: ipc4-topology: Propagate change caused by NHLT data for SSP
ujfalusi Apr 29, 2025
26f4500
ASoC: SOF: ipc4-topology: Improve playback dai copier in/out format s…
ujfalusi Apr 29, 2025
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
78 changes: 53 additions & 25 deletions sound/soc/sof/ipc4-topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1805,6 +1805,12 @@ snd_sof_get_nhlt_endpoint_data(struct snd_sof_dev *sdev, struct snd_sof_dai *dai
dai_index);
if (dev_type < 0)
return dev_type;

if (params_width(params) != bit_depth) {
format_change = true;
dev_dbg(sdev->dev, "SSP sample width change from %d to %d\n",
params_width(params), bit_depth);
}
Copy link
Copy Markdown
Collaborator

@kv2019i kv2019i May 20, 2026

Choose a reason for hiding this comment

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

I'm wondering if this safe for the BT offload case where SSP DAI supports multiple formats and we should be not using NHLT info for this. I guess this is. On current devices where we support BT offload, there is no NHLT in BIOS, so I guess this is good, but some potential for confusing bugs.

Copy link
Copy Markdown
Collaborator Author

@ujfalusi ujfalusi May 20, 2026

Choose a reason for hiding this comment

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

at this point we did not queried NHLT at all. This is all topology. The topology contains 'hw_config' array of supported configurations on the copier, this corresponds with the copier formats (and if topology have NHLT, then the blobs).

If the copier does not support the sample width of the params, then we need to pick the one which is supported to force a conversion.

Later we will ask the NHLT blob for the configuration that is actually supported by the copier.

Copy link
Copy Markdown
Collaborator Author

@ujfalusi ujfalusi May 20, 2026

Choose a reason for hiding this comment

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

@kv2019i, the issue is that if we don't do this then we would have silent incorrect configuration as described by the commit message. copier running in 16 bit mode while the SSP is configured in 32 bit mode or other permutation.
The nice thing is that the NHLT blob and copier format / hw_config all comes from different source and it is a topology bug if they don't match.

break;
default:
return 0;
Expand Down Expand Up @@ -2169,28 +2175,32 @@ sof_ipc4_prepare_copier_module(struct snd_sof_widget *swidget,
copier_data = &ipc4_copier->data;
available_fmt = &ipc4_copier->available_fmt;
Copy link
Copy Markdown
Collaborator

@kv2019i kv2019i May 20, 2026

Choose a reason for hiding this comment

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

The commit message "When using a nocodec passthrough topology (which is a bug) " is a bit confusing. What is the bug here, the fact we are using nocodec passthrough topology? Not sure what is the bug. It seems legal to only provide S32 blobs and a copier configuration that supports at least S32.

Copy link
Copy Markdown
Collaborator Author

@ujfalusi ujfalusi May 20, 2026

Choose a reason for hiding this comment

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

OK, I will drop the note. I added it after a long internal discussion that using the nocodec passthrough is really something which is not to be used, it is even more experimental than nocodec and it has been not tested at all (I happened to needed it for testing something else)


/*
* Use the fe_params as a base for the copier configuration.
* The ref_params might get updated to reflect what format is
* supported by the copier on the DAI side.
*
* In case of capture the ref_params returned will be used to
* find the input configuration of the copier.
*/
ref_params = kmemdup(fe_params, sizeof(*ref_params), GFP_KERNEL);
if (!ref_params)
return -ENOMEM;

ret = sof_ipc4_prepare_dai_copier(sdev, dai, ref_params, dir);
if (ret < 0)
return ret;
if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
/*
* For playback the pipeline_params needs to be used to
* find the input configuration of the copier.
*/
ref_params = kmemdup(pipeline_params, sizeof(*ref_params),
GFP_KERNEL);
if (!ref_params)
return -ENOMEM;
} else {
/*
* For capture the adjusted fe_params needs to be used
* to find the input configuration of the copier.
*
* The params might be updated in
* sof_ipc4_prepare_dai_copier() to reflect the supported
* input formats by the copier/dai.
*/
ref_params = kmemdup(fe_params, sizeof(*ref_params), GFP_KERNEL);
if (!ref_params)
return -ENOMEM;

/*
* For playback the pipeline_params needs to be used to find the
* input configuration of the copier.
*/
if (dir == SNDRV_PCM_STREAM_PLAYBACK)
memcpy(ref_params, pipeline_params, sizeof(*ref_params));
ret = sof_ipc4_prepare_dai_copier(sdev, dai, ref_params, dir);
if (ret < 0)
Copy link
Copy Markdown
Collaborator

@ranj063 ranj063 Mar 24, 2026

Choose a reason for hiding this comment

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

@ujfalusi do we have a memory leak for the cllocated memory for ref_params here and possibly all error paths below?

Copy link
Copy Markdown
Collaborator Author

@ujfalusi ujfalusi May 19, 2026

Choose a reason for hiding this comment

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

No, we don't because of:

struct snd_pcm_hw_params *ref_params __free(kfree) = NULL;

it will be freed on function exit.

return ret;
}

break;
}
Expand Down Expand Up @@ -2245,15 +2255,33 @@ sof_ipc4_prepare_copier_module(struct snd_sof_widget *swidget,
}
case snd_soc_dapm_aif_out:
case snd_soc_dapm_dai_in:
out_ref_rate = params_rate(fe_params);
out_ref_channels = params_channels(fe_params);
ret = sof_ipc4_get_sample_type(sdev, fe_params);
/*
* For capture the fe_params needs to be used to find the output
* configuration of the copier.
*
* For playback the adjusted fe_params needs to be used
* to find the output configuration of the copier.
*
* The params might be updated in
* sof_ipc4_prepare_dai_copier() to reflect the supported
* output formats by the copier/dai.
*/
memcpy(ref_params, fe_params, sizeof(*ref_params));
if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
ret = sof_ipc4_prepare_dai_copier(sdev, dai, ref_params, dir);
if (ret < 0)
return ret;
}

out_ref_rate = params_rate(ref_params);
out_ref_channels = params_channels(ref_params);
ret = sof_ipc4_get_sample_type(sdev, ref_params);
if (ret < 0)
return ret;
out_ref_type = (u32)ret;

if (!single_output_bitdepth) {
out_ref_valid_bits = sof_ipc4_get_valid_bits(sdev, fe_params);
out_ref_valid_bits = sof_ipc4_get_valid_bits(sdev, ref_params);
if (out_ref_valid_bits < 0)
return out_ref_valid_bits;
}
Expand Down
Loading