Skip to content

alternate multi-dai and multi-config proposal #2363

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
plbossart wants to merge 2 commits into from
Closed

alternate multi-dai and multi-config proposal #2363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
735ad88
ipc: dai: remove unused hdr field in params
plbossart Feb 10, 2020
c360ffa
dai: add multi-dai configuration
plbossart Feb 10, 2020
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
12 changes: 6 additions & 6 deletions src/audio/dai.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -688,7 +688,7 @@ static int dai_config(struct comp_dev *dev, struct sof_ipc_dai_config *config)
switch (config->type) {

@dbaluta dbaluta Feb 12, 2020 •
edited
Loading

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.

@plbossart the commit message looks very good. I think would be a nice addition to explain the current supported scenario and WHY do you need multi-dai/multi-config. The description must be made in natural language so that can be understood even by the users of SOF.

So, as far as I understand now once a topology is loaded we can support just 1 DAI with 1 configuration. For example, we can support playback over SSP for PCM streams with rate 48KHz, channels 2, format S16_LE.

Is that right?

Now, if we want to play a file with 4 channels for example this is not really possible due to limited topology?

Going forward: With this change would be somehow possible to specify a topology that supports multiple DAIs like for example on i.MX8 -> ESAI and SAI, and also support multiple configurations (like for example we can support playing files at 44.1Khz / 48Khz) dynamically without the need to reload the topology?

Is my understanding correct? If so, this is a great feature!

@plbossart plbossart Feb 12, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The multi-dai is really an 'aggregation' feature, so that a stream can be rendered on two separate interfaces. For SoundWire, we have e.g. the left amplifier on one link and the right amplifier on the second. The multi-dai support provides you with the ability to build a 'compound' dai from existing components. From the kernel side you only see one DAI, but the firmware will program and synchronize the individual components. It's an alternative to the 'multi-cpu dai' approach at the kernel level, which could have been a solution as well but might be more complicated to handle at the firmware level.

The multi-configuration is unrelated, it's rather to support different modes of operation. We need this for Bluetooth support, where we can either use 48kHz 2ch 24 bits or 16kHz mono 16 bits. There is no dynamic switch between configs, and it may not be usable by e.g. PulseAudio, but a dedicated Bluetooth app or stack would know what to do.

I'll update the commit messages, for now I am in 'don't break anything' validation mode.

@dbaluta dbaluta Feb 12, 2020

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.

@plbossart I see. I'm very interested about multi-configuration stuff. What do you understand by dynamic switching between configs?

Ideally, I'm wishing this scenario to work:

  1. Run aplay -Dhw:0,0 -f S32_LE -c 2 -r 48000 sample.wav

At some point this will call hw_params() in Linux kernel which in turn will set the correct configuration on FW.

Then:

  1. Run aplay -Dhw:0,0 -f S32_LE -c 4 -r 44100 sample.wav

Similarly, this will call hw_params() in Linux kernel which in turn will set the correct configuration on FW.

AFAIK this use case is not really supported because the topology on FW is created at the same time when the sound card is created on Linux kernel.

@plbossart plbossart Feb 12, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

that's exactly what @ranj063 and I were talking about earlier, we'd need the kernel to send the config IPC at hw_params time for all DAIs (currently it's only done for HDaudio and ALH). the kernel would select which config_index is desired and the firrmware woud apply configuration1 for 48 kHZ and configuration2 for 44.1kHz.

The point about switching is that you have to close the PCM and shutdown the DAI before you can change configurations. So you can still do run-time changes, but not after the hw_params were selected - if you do you have to go back to square1.

@dbaluta dbaluta Feb 12, 2020 •
edited
Loading

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.

@plbossart Ok, we are on the same page. I only need to switch configuration when PCM is opened and that's all. If i need another configuration I will close the PCM and re-open it with new params.

This will work for us for PCM streams. We will see how things will be going with the compress interface where there I think there is a need to change the params without reopening the interface. But that's another story.

My concern now is about the initial configuration of the pipeline. Because on FW the pipeline is created just only once when the sound card is created on the Linux kernel side. When pipeline is created one needs to know how big the buffers will be, for example.

So, when sound card is created the kernel will look at PCM card capabilities and extract the maximum possible parameters.

If the sound card on the Linux kernel is advertised to support the following config:

rate: 44100, 48000
chanels: 2, 4, 16
rate: S16_LE, S24_LE, S32_LE

Then the kernel needs to figure out that, it needs to support:
maxrate: 48000
maxchan: 16
maxformat: S32_LE

and then create the pipeline on the firmware with exact these parameters.

@plbossart plbossart Feb 12, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

On the PCM/host side, you can have a lot more variations as long as you have the proper converter for the DAI format. For the DAIs you should really have 3 or 4 configs max.

Note that the pipeline creation on startup is what we do today, at some point we should really need to create the pipeline when it's needed. That would really allow for more optimal uses of the hardware resources.

case SOF_DAI_INTEL_SSP:
/* set dma burst elems to slot number */
dd->config.burst_elems = config->ssp.tdm_slots;
dd->config.burst_elems = config->dai_data[0].ssp.tdm_slots;
break;
case SOF_DAI_INTEL_DMIC:
tracev_dai_comp_with_ids(dev, "dai_config(), config->type = SOF_DAI_INTEL_DMIC");
Expand All @@ -697,11 +697,11 @@ static int dai_config(struct comp_dev *dev, struct sof_ipc_dai_config *config)
dd->config.burst_elems = 8;

trace_dai_comp_with_ids(dev, "dai_config(), config->dmic.fifo_bits = %u config->dmic.num_pdm_active = %u",
config->dmic.fifo_bits,
config->dmic.num_pdm_active);
config->dai_data[0].dmic.fifo_bits,
config->dai_data[0].dmic.num_pdm_active);
break;
case SOF_DAI_INTEL_HDA:
channel = config->hda.link_dma_ch;
channel = config->dai_data[0].hda.link_dma_ch;
trace_dai_comp_with_ids(dev, "dai_config(), channel = %d",
channel);

Expand Down Expand Up @@ -729,8 +729,8 @@ static int dai_config(struct comp_dev *dev, struct sof_ipc_dai_config *config)
/* As with HDA, the DMA channel is assigned in runtime,
* not during topology parsing.
*/
channel = config->alh.stream_id;
dd->stream_id = config->alh.stream_id;
channel = config->dai_data[0].alh.stream_id;
dd->stream_id = config->dai_data[0].alh.stream_id;
trace_dai_comp_with_ids(dev, "dai_config(), channel = %d",
channel);
break;
Expand Down
64 changes: 39 additions & 25 deletions src/drivers/intel/baytrail/ssp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ static int ssp_set_config(struct dai *dai,
sspsp = SSPSP_ETDS; /* last value (bit 0) */

ssp->config = *config;
ssp->params = config->ssp;
ssp->params = config->dai_data[0].ssp;

@ranj063 ranj063 Feb 12, 2020

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.

@plbossart should we change this all to config->dai_data[config->config_index] instead of 0 to make it ready when the config_index would be something other than 0?

@plbossart plbossart Feb 12, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

i wanted to do this in follow-up patches when we have support for this at the kernel level. For now it's simpler.
And I am not sure that we want to have multiple configurations for all DAI types anyways, so it's rather a case-by-case change, not a blanket one.

@ranj063 ranj063 Feb 13, 2020

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.

look good in that case @plbossart . we've still got some build failures on a couple of platforms


/* clock masters */
/*
Expand Down Expand Up @@ -281,15 +281,17 @@ static int ssp_set_config(struct dai *dai,
}

/* BCLK is generated from MCLK - must be divisable */
if (config->ssp.mclk_rate % config->ssp.bclk_rate) {
if (config->dai_data[0].ssp.mclk_rate %
config->dai_data[0].ssp.bclk_rate) {
trace_ssp_error("ssp_set_config() error: "
"MCLK is not divisable");
ret = -EINVAL;
goto out;
}

/* divisor must be within SCR range */
mdiv = (config->ssp.mclk_rate / config->ssp.bclk_rate) - 1;
mdiv = (config->dai_data[0].ssp.mclk_rate /
config->dai_data[0].ssp.bclk_rate) - 1;
if (mdiv > (SSCR0_SCR_MASK >> 8)) {
trace_ssp_error("ssp_set_config() error: "
"divisor is not within SCR range");
Expand All @@ -301,24 +303,26 @@ static int ssp_set_config(struct dai *dai,
sscr0 |= SSCR0_SCR(mdiv);

/* calc frame width based on BCLK and rate - must be divisable */
if (config->ssp.bclk_rate % config->ssp.fsync_rate) {
if (config->dai_data[0].ssp.bclk_rate %
config->dai_data[0].ssp.fsync_rate) {
trace_ssp_error("ssp_set_config() error: "
"BLCK is not divisable");
ret = -EINVAL;
goto out;
}

/* must be enouch BCLKs for data */
bdiv = config->ssp.bclk_rate / config->ssp.fsync_rate;
if (bdiv < config->ssp.tdm_slot_width *
config->ssp.tdm_slots) {
bdiv = config->dai_data[0].ssp.bclk_rate /
config->dai_data[0].ssp.fsync_rate;
if (bdiv < config->dai_data[0].ssp.tdm_slot_width *
config->dai_data[0].ssp.tdm_slots) {
trace_ssp_error("ssp_set_config() error: not enough BCLKs");
ret = -EINVAL;
goto out;
}

/* tdm_slot_width must be <= 38 for SSP */
if (config->ssp.tdm_slot_width > 38) {
if (config->dai_data[0].ssp.tdm_slot_width > 38) {
trace_ssp_error("ssp_set_config() error: tdm_slot_width > 38");
ret = -EINVAL;
goto out;
Expand All @@ -334,7 +338,7 @@ static int ssp_set_config(struct dai *dai,
sscr3 |= SSCR3_I2S_MODE_EN | SSCR3_I2S_TX_EN | SSCR3_I2S_RX_EN;

/* set asserted frame length */
frame_len = config->ssp.tdm_slot_width;
frame_len = config->dai_data[0].ssp.tdm_slot_width;

/* handle frame polarity, I2S default is falling/active low */
sspsp |= SSPSP_SFRMP(!inverted_frame);
Expand Down Expand Up @@ -364,7 +368,7 @@ static int ssp_set_config(struct dai *dai,
sscr3 |= SSCR3_I2S_MODE_EN | SSCR3_I2S_TX_EN | SSCR3_I2S_RX_EN;

/* set asserted frame length */
frame_len = config->ssp.tdm_slot_width;
frame_len = config->dai_data[0].ssp.tdm_slot_width;

/* LEFT_J default is rising/active high, opposite of I2S */
sspsp |= SSPSP_SFRMP(inverted_frame);
Expand All @@ -389,7 +393,8 @@ static int ssp_set_config(struct dai *dai,

start_delay = 1;

sscr0 |= SSCR0_MOD | SSCR0_FRDC(config->ssp.tdm_slots);
sscr0 |= SSCR0_MOD |
SSCR0_FRDC(config->dai_data[0].ssp.tdm_slots);

/* set asserted frame length */
frame_len = 1;
Expand All @@ -405,19 +410,24 @@ static int ssp_set_config(struct dai *dai,
* total frame period (both asserted and
* deasserted time of frame)
*/
if (cbs)
sscr4 |= SSCR4_TOT_FRM_PRD(config->ssp.tdm_slots *
config->ssp.tdm_slot_width);
if (cbs) {
int tdm_slots = config->dai_data[0].ssp.tdm_slots;
int tdm_slot_width =
config->dai_data[0].ssp.tdm_slot_width;

sscr4 |= SSCR4_TOT_FRM_PRD(tdm_slots * tdm_slot_width);
}

active_tx_slots = popcount(config->ssp.tx_slots);
active_rx_slots = popcount(config->ssp.rx_slots);
active_tx_slots = popcount(config->dai_data[0].ssp.tx_slots);
active_rx_slots = popcount(config->dai_data[0].ssp.rx_slots);

break;
case SOF_DAI_FMT_DSP_B:

start_delay = 0;

sscr0 |= SSCR0_MOD | SSCR0_FRDC(config->ssp.tdm_slots);
sscr0 |= SSCR0_MOD |
SSCR0_FRDC(config->dai_data[0].ssp.tdm_slots);

/* set asserted frame length */
frame_len = 1;
Expand All @@ -433,12 +443,16 @@ static int ssp_set_config(struct dai *dai,
* total frame period (both asserted and
* deasserted time of frame
*/
if (cbs)
sscr4 |= SSCR4_TOT_FRM_PRD(config->ssp.tdm_slots *
config->ssp.tdm_slot_width);
if (cbs) {
int tdm_slots = config->dai_data[0].ssp.tdm_slots;
int tdm_slot_width =
config->dai_data[0].ssp.tdm_slot_width;

sscr4 |= SSCR4_TOT_FRM_PRD(tdm_slots * tdm_slot_width);
}

active_tx_slots = popcount(config->ssp.tx_slots);
active_rx_slots = popcount(config->ssp.rx_slots);
active_tx_slots = popcount(config->dai_data[0].ssp.tx_slots);
active_rx_slots = popcount(config->dai_data[0].ssp.rx_slots);

break;
default:
Expand All @@ -452,7 +466,7 @@ static int ssp_set_config(struct dai *dai,
sspsp |= SSPSP_SFRMWDTH(frame_len);
sscr5 |= SSCR5_FRM_ASRT_CLOCKS(frame_len);

data_size = config->ssp.sample_valid_bits;
data_size = config->dai_data[0].ssp.sample_valid_bits;

if (data_size > 16)
sscr0 |= (SSCR0_EDSS | SSCR0_DSIZE(data_size - 16));
Expand All @@ -473,8 +487,8 @@ static int ssp_set_config(struct dai *dai,
ssp_write(dai, SSCR5, sscr5);
ssp_write(dai, SSPSP, sspsp);
ssp_write(dai, SFIFOTT, sfifott);
ssp_write(dai, SSTSA, config->ssp.tx_slots);
ssp_write(dai, SSRSA, config->ssp.rx_slots);
ssp_write(dai, SSTSA, config->dai_data[0].ssp.tx_slots);
ssp_write(dai, SSRSA, config->dai_data[0].ssp.rx_slots);

ssp->state[DAI_DIR_PLAYBACK] = COMP_STATE_PREPARE;
ssp->state[DAI_DIR_CAPTURE] = COMP_STATE_PREPARE;
Expand Down
22 changes: 12 additions & 10 deletions src/drivers/intel/cavs/dmic.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1077,16 +1077,16 @@ static int dmic_set_config(struct dai *dai, struct sof_ipc_dai_config *config)

trace_dmic_id(dai, "dmic_set_config()");

if (config->dmic.driver_ipc_version != DMIC_IPC_VERSION) {
if (config->dai_data[0].dmic.driver_ipc_version != DMIC_IPC_VERSION) {
trace_error_dmic_id(dai, "dmic_set_config() error: wrong ipc version");
return -EINVAL;
}

/* Compute unmute ramp gain update coefficient. Use the value from
* topology if it is non-zero, otherwise use default length.
*/
if (config->dmic.unmute_ramp_time)
unmute_ramp_time_ms = config->dmic.unmute_ramp_time;
if (config->dai_data[0].dmic.unmute_ramp_time)
unmute_ramp_time_ms = config->dai_data[0].dmic.unmute_ramp_time;
else
unmute_ramp_time_ms = LOGRAMP_TIME_MS;

Expand All @@ -1102,7 +1102,7 @@ static int dmic_set_config(struct dai *dai, struct sof_ipc_dai_config *config)
return -EINVAL;
}

if (config->dmic.num_pdm_active > DMIC_HW_CONTROLLERS) {
if (config->dai_data[0].dmic.num_pdm_active > DMIC_HW_CONTROLLERS) {
trace_error_dmic_id(dai, "dmic_set_config() error: the requested PDM controllers count exceeds platform capability");
return -EINVAL;
}
Expand Down Expand Up @@ -1136,28 +1136,30 @@ static int dmic_set_config(struct dai *dai, struct sof_ipc_dai_config *config)
/* Copy the new DMIC params to persistent. The last request
* determines the parameters.
*/
ret = memcpy_s(dmic_prm[di], sizeof(*dmic_prm[di]), &config->dmic,
ret = memcpy_s(dmic_prm[di], sizeof(*dmic_prm[di]),
&config->dai_data[0].dmic,
sizeof(struct sof_ipc_dai_dmic_params));
assert(!ret);

/* copy the pdm controller params from ipc */
for (i = 0; i < DMIC_HW_CONTROLLERS; i++) {
dmic_prm[di]->pdm[i].id = i;
for (j = 0; j < config->dmic.num_pdm_active; j++) {
for (j = 0; j < config->dai_data[0].dmic.num_pdm_active; j++) {
/* copy the pdm controller params id the id's match */
if (dmic_prm[di]->pdm[i].id == config->dmic.pdm[j].id) {
if (dmic_prm[di]->pdm[i].id ==
config->dai_data[0].dmic.pdm[j].id) {
ret = memcpy_s(&dmic_prm[di]->pdm[i],
sizeof(dmic_prm[di]->pdm[i]),
&config->dmic.pdm[j],
&config->dai_data[0].dmic.pdm[j],
sizeof(
struct sof_ipc_dai_dmic_pdm_ctrl));
assert(!ret);
}
}
}

trace_dmic_id(dai, "dmic_set_config(), prm config->dmic.num_pdm_active = %u",
config->dmic.num_pdm_active);
trace_dmic_id(dai, "dmic_set_config(), prm config->dai_data[0].dmic.num_pdm_active = %u",
config->dai_data[0].dmic.num_pdm_active);
trace_dmic_id(dai, "dmic_set_config(), prm pdmclk_min = %u, pdmclk_max = %u",
dmic_prm[di]->pdmclk_min, dmic_prm[di]->pdmclk_max);
trace_dmic_id(dai, "dmic_set_config(), prm duty_min = %u, duty_max = %u",
Expand Down
Loading