Skip to content

ASoC: SOF: add support for multi-dai and multi-configs per dai #1785

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 1 commit into from
Closed

ASoC: SOF: add support for multi-dai and multi-configs per dai #1785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 9 additions & 5 deletions include/sound/sof/dai-intel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@

/* SSP Configuration Request - SOF_IPC_DAI_SSP_CONFIG */
struct sof_ipc_dai_ssp_params {
struct sof_ipc_hdr hdr;
uint32_t subdai_index; /* only used in multi-dai usages */
uint32_t reserved0;

@lyakh lyakh Feb 14, 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.

Do I understand this correctly, that hdr can be removed from this structs because these structs are never sent as a complete IPC message but only always embedded in larger structs with their own header? So, having this header in them was actually pointless from the beginning? This is a good and significant change. I'd say this change deserves a separate patch if this my understanding is correct...

@plbossart plbossart Feb 14, 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.

yes indeed, it was useless. This used to be a separate patch, not sure what I did with git.

uint16_t reserved1;
uint16_t mclk_id;

Expand Down Expand Up @@ -83,13 +84,15 @@ struct sof_ipc_dai_ssp_params {

/* HDA Configuration Request - SOF_IPC_DAI_HDA_CONFIG */
struct sof_ipc_dai_hda_params {
struct sof_ipc_hdr hdr;
uint32_t subdai_index; /* only used in multi-dai usages */
uint32_t reserved0;
uint32_t link_dma_ch;
} __packed;

/* ALH Configuration Request - SOF_IPC_DAI_ALH_CONFIG */
struct sof_ipc_dai_alh_params {
struct sof_ipc_hdr hdr;
uint32_t subdai_index; /* only used in multi-dai usages */
uint32_t reserved0;
uint32_t stream_id;

/* reserved for future use */
Expand All @@ -116,7 +119,7 @@ struct sof_ipc_dai_alh_params {
* data integrity problems.
*/
struct sof_ipc_dai_dmic_pdm_ctrl {
struct sof_ipc_hdr hdr;
uint32_t reserved0[2];
uint16_t id; /**< PDM controller ID */

uint16_t enable_mic_a; /**< Use A (left) channel mic (0 or 1)*/
Expand Down Expand Up @@ -161,7 +164,8 @@ struct sof_ipc_dai_dmic_pdm_ctrl {
* treated as an error.
*/
struct sof_ipc_dai_dmic_params {
struct sof_ipc_hdr hdr;
uint32_t subdai_index; /* only used in multi-dai usages */
uint32_t reserved0;
uint32_t driver_ipc_version; /**< Version (1..N) */

uint32_t pdmclk_min; /**< Minimum microphone clock in Hz (100000..N) */
Expand Down
29 changes: 22 additions & 7 deletions include/sound/sof/dai.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#ifndef __INCLUDE_SOUND_SOF_DAI_H__
#define __INCLUDE_SOUND_SOF_DAI_H__

#include <sound/sof/channel_map.h>
#include <sound/sof/header.h>
#include <sound/sof/dai-intel.h>
#include <sound/sof/dai-imx.h>
Expand Down Expand Up @@ -50,23 +51,31 @@ enum sof_ipc_dai_type {
SOF_DAI_INTEL_SSP, /**< Intel SSP */
SOF_DAI_INTEL_DMIC, /**< Intel DMIC */
SOF_DAI_INTEL_HDA, /**< Intel HD/A */
SOF_DAI_INTEL_ALH, /**< Intel ALH */
SOF_DAI_IMX_SAI, /**< i.MX SAI */
SOF_DAI_IMX_ESAI, /**< i.MX ESAI */
SOF_DAI_INTEL_ALH, /**< Intel ALH */
SOF_DAI_IMX_SAI, /**< i.MX SAI */
SOF_DAI_IMX_ESAI, /**< i.MX ESAI */

@bardliao bardliao 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 like whitespace is changed.

@plbossart plbossart Feb 13, 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.

will fix.

SOF_DAI_MULTIDAI = 0xFFFF /**< config with multiple elements of the same dai type */
};

/* general purpose DAI configuration */
struct sof_ipc_dai_config {
struct sof_ipc_cmd_hdr hdr;
uint32_t type; /**< DAI type - enum sof_ipc_dai_type */

uint16_t type; /**< DAI type - enum sof_ipc_dai_type */
uint16_t subdai_type; /**< only used in multi-dai cases */
uint32_t dai_index; /**< index of this type dai */

/* physical protocol and clocking */
uint16_t format; /**< SOF_DAI_FMT_ */
uint16_t reserved16; /**< alignment */

/* we can either have multiple sub-DAIs or multiple configs, not both */
uint16_t num_payloads; /**< number of configs or number of sub-DAIs */

uint16_t config_index; /**< index in dai_data */

/* reserved for future use */
uint32_t reserved[8];
uint32_t reserved16;

@bardliao bardliao 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.

Should it be uint16_t reserved16?

@plbossart plbossart Feb 13, 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.

Gah, yes. merge mistake.

uint32_t reserved[7];

/* HW specific data */
union {
Expand All @@ -76,7 +85,13 @@ struct sof_ipc_dai_config {
struct sof_ipc_dai_alh_params alh;
struct sof_ipc_dai_esai_params esai;
struct sof_ipc_dai_sai_params sai;
};
} dai_data[0];

/*
* multi-dai stream maps. The ext_id used needs to match subdai_index
* in each _params structure above
*/
struct sof_ipc_stream_map stream_map;

@lyakh lyakh Feb 14, 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.

I'm confused. This struct sof_ipc_dai_config is a part of the ABI, right? Isn't this change changing the size of the struct in some unclear way? Before we had a union above - i.e. the size that it took was equal to that of the largest member. Now it's a 0-size array, so, that size is 0. And instead you're adding a new struct after that... And 0-size arrays in the middle of a struct can have weird consequences... I'm really confused.

@plbossart plbossart Feb 14, 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.

ack, this is completely broken indeed. i started a parallel thread to remove all variable arrays from IPC structs.

} __packed;

#endif
2 changes: 1 addition & 1 deletion sound/soc/sof/intel/hda-dai.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ static int hda_link_config_ipc(struct sof_intel_hda_stream *hda_stream,
}

/* update config with stream tag */
config->hda.link_dma_ch = channel;
config->dai_data[0].hda.link_dma_ch = channel;

/* send IPC */
ret = sof_ipc_tx_message(hda_stream->sdev->ipc,
Expand Down
4 changes: 2 additions & 2 deletions sound/soc/sof/intel/hda.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ static int sdw_params_stream(struct device *dev,
config.hdr.cmd = SOF_IPC_GLB_DAI_MSG | SOF_IPC_DAI_CONFIG;
config.type = SOF_DAI_INTEL_ALH;
config.dai_index = (link_id << 8) | (d->id);
config.alh.stream_id = alh_stream_id;
config.dai_data[0].alh.stream_id = alh_stream_id;

@kv2019i kv2019i 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.

@plbossart Hmm, is this really safe? I mean definition now has a zero-length array and config size is set with sizeof(). I would except the sizeof to change with this patch.

@lyakh lyakh Feb 14, 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.

agree with @kv2019i - very strange and unclear what you're trying to do with that 0-size array of unions in the middle of a struct...


/* send message to DSP */
ret = sof_ipc_tx_message(sdev->ipc,
Expand Down Expand Up @@ -98,7 +98,7 @@ static int sdw_free_stream(struct device *dev,
config.hdr.cmd = SOF_IPC_GLB_DAI_MSG | SOF_IPC_DAI_CONFIG;
config.type = SOF_DAI_INTEL_ALH;
config.dai_index = (link_id << 8) | d->id;
config.alh.stream_id = 0xFFFF; /* invalid value on purpose */
config.dai_data[0].alh.stream_id = 0xFFFF; /* invalid value on purpose */

/* send message to DSP */
ret = sof_ipc_tx_message(sdev->ipc,
Expand Down
16 changes: 8 additions & 8 deletions sound/soc/sof/pcm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,10 +679,10 @@ static int sof_pcm_dai_link_fixup(struct snd_soc_pcm_runtime *rtd,
/* read rate and channels from topology */
switch (dai->dai_config->type) {
case SOF_DAI_INTEL_SSP:
rate->min = dai->dai_config->ssp.fsync_rate;
rate->max = dai->dai_config->ssp.fsync_rate;
channels->min = dai->dai_config->ssp.tdm_slots;
channels->max = dai->dai_config->ssp.tdm_slots;
rate->min = dai->dai_config->dai_data[0].ssp.fsync_rate;
rate->max = dai->dai_config->dai_data[0].ssp.fsync_rate;
channels->min = dai->dai_config->dai_data[0].ssp.tdm_slots;
channels->max = dai->dai_config->dai_data[0].ssp.tdm_slots;

dev_dbg(component->dev,
"rate_min: %d rate_max: %d\n", rate->min, rate->max);
Expand All @@ -707,16 +707,16 @@ static int sof_pcm_dai_link_fixup(struct snd_soc_pcm_runtime *rtd,
/* do nothing for ALH dai_link */
break;
case SOF_DAI_IMX_ESAI:
channels->min = dai->dai_config->esai.tdm_slots;
channels->max = dai->dai_config->esai.tdm_slots;
channels->min = dai->dai_config->dai_data[0].esai.tdm_slots;
channels->max = dai->dai_config->dai_data[0].esai.tdm_slots;

dev_dbg(component->dev,
"channels_min: %d channels_max: %d\n",
channels->min, channels->max);
break;
case SOF_DAI_IMX_SAI:
channels->min = dai->dai_config->sai.tdm_slots;
channels->max = dai->dai_config->sai.tdm_slots;
channels->min = dai->dai_config->dai_data[0].sai.tdm_slots;
channels->max = dai->dai_config->dai_data[0].sai.tdm_slots;

dev_dbg(component->dev,
"channels_min: %d channels_max: %d\n",
Expand Down
2 changes: 1 addition & 1 deletion sound/soc/sof/sof-audio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ int sof_restore_pipelines(struct device *dev)
* the dai config in the DSP.
*/
if (config->type == SOF_DAI_INTEL_HDA)
config->hda.link_dma_ch = DMA_CHAN_INVALID;
config->dai_data[0].hda.link_dma_ch = DMA_CHAN_INVALID;

ret = sof_ipc_tx_message(sdev->ipc,
config->hdr.cmd, config,
Expand Down
Loading