Skip to content

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

Closed
plbossart wants to merge 1 commit into
thesofproject:topic/sof-devfrom
plbossart:fix/multi-dai-interfaces
Closed

ASoC: SOF: add support for multi-dai and multi-configs per dai#1785
plbossart wants to merge 1 commit into
thesofproject:topic/sof-devfrom
plbossart:fix/multi-dai-interfaces

Conversation

@plbossart

@plbossart plbossart commented Feb 12, 2020

Copy link
Copy Markdown
Member

Mirror firmware IPC changes to multi-dai/multi-config.

This is just a backwards-compatible code change with no new
functionality.

Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com

@plbossart
ASoC: SOF: add support for multi-dai and multi-configs per dai
65bbfe7 
Mirror firmware IPC changes to multi-dai/multi-config.

This is just a backwards-compatible code change with no new
functionality.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart

plbossart commented Feb 12, 2020

Copy link
Copy Markdown
Member Author

Kernel side version of thesofproject/sof#2363

Still need to figure out compilation warnings

sound/soc/sof/topology.c:2647:2: warning: ‘memset’ writing 64 bytes into a region of size 24 overflows the destination [-Wstringop-overflow=]
 2647 |  memset(&config->dai_data[0].ssp, 0,
      |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 2648 |         sizeof(struct sof_ipc_dai_ssp_params));
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@plbossart plbossart mentioned this pull request Feb 12, 2020
Closed
@ranj063

ranj063 commented Feb 12, 2020

Copy link
Copy Markdown
Collaborator

Kernel side version of thesofproject/sof#2363

Still need to figure out compilation warnings

sound/soc/sof/topology.c:2647:2: warning: ‘memset’ writing 64 bytes into a region of size 24 overflows the destination [-Wstringop-overflow=]
 2647 |  memset(&config->dai_data[0].ssp, 0,
      |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 2648 |         sizeof(struct sof_ipc_dai_ssp_params));
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@plbossart i guess you need to allocate memory for dai_data since it is a flex array?

bardliao
bardliao requested changes Feb 13, 2020
View reviewed changes

@bardliao bardliao left a comment

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 bit size of struct sof_ipc_dai_config {} seems to be inconsistent. Where num_payloads and num_payloads are taken from one reserved[] which is consistent. So, I think we don't need to change the size of reserved16

Comment thread include/sound/sof/dai.h
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.

Comment thread include/sound/sof/dai.h

/* 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.

kv2019i
kv2019i reviewed Feb 13, 2020
View reviewed changes

@kv2019i kv2019i left a comment

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 have one concern about implications of the zero-sized array. As the patch passes CI, I guess it is ok, but just wanted to doublecheck. Otherwise looks good -- the mention that this is backwards-compatible is really important as it's not quite obvious from the patch that it is...

Comment thread sound/soc/sof/intel/hda.c
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...

lyakh
lyakh requested changes Feb 14, 2020
View reviewed changes
Comment thread include/sound/sof/dai-intel.h
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.

Comment thread include/sound/sof/dai.h
* 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.

Comment thread sound/soc/sof/intel/hda.c
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;

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

@plbossart

plbossart commented Feb 20, 2020

Copy link
Copy Markdown
Member Author

closing for now, we'll have to revisit later

@plbossart plbossart closed this Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kv2019i kv2019i kv2019i left review comments
@lyakh lyakh lyakh requested changes
@bardliao bardliao bardliao requested changes
@dbaluta dbaluta Awaiting requested review from dbaluta dbaluta is a code owner
@juimonen juimonen Awaiting requested review from juimonen
@keyonjie keyonjie Awaiting requested review from keyonjie
@lgirdwood lgirdwood Awaiting requested review from lgirdwood lgirdwood is a code owner
@ranj063 ranj063 Awaiting requested review from ranj063 ranj063 is a code owner
@singalsu singalsu Awaiting requested review from singalsu singalsu is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@plbossart @ranj063 @lyakh @bardliao @kv2019i