| Message ID | 1529492057-32627-4-git-send-email-shreyas.nc@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/20/2018 05:54 AM, Shreyas NC wrote: > DAPM handles DAIs during soc_dapm_stream_event() and during addition > and creation of DAI widgets i.e., dapm_add_valid_dai_widget() and > dapm_connect_dai_link_widgets(). can you split this patch in two, one where you add dapm_add_valid_dai_widget() and the second one where you add the multi-cpu stuff? the current diff format is really hard to read with the two changes lumped together. > + for (i = 0; i < rtd->num_codecs; i++) { > + struct snd_soc_dai *codec_dai = rtd->codec_dais[i]; > + > + for (j = 0; j < rtd->num_cpu_dai; j++) { > + cpu_dai = rtd->cpu_dais[j]; > + > + dapm_add_valid_dai_widget(card, rtd, > + codec_dai, cpu_dai); I didn't click on this earlier, but what makes you think all codec_dais are connected to all cpu_dais? That doesn't seem quite right. For the multi-codec case, all the codec_dais hang from a single cpu_dai. it's a stretch for me to have a full M:N connectivity. And that's clearly not the case for SoundWire stream in the multi-link case. Can't we use the dai_link information here to only connect cpu_ and codec_dais that are related? > } > } > } > @@ -4211,7 +4230,9 @@ static void soc_dapm_stream_event(struct snd_soc_pcm_runtime *rtd, int stream, > { > int i; > > - soc_dapm_dai_stream_event(rtd->cpu_dai, stream, event); > + for (i = 0; i < rtd->num_cpu_dai; i++) > + soc_dapm_dai_stream_event(rtd->cpu_dais[i], stream, event); > + > for (i = 0; i < rtd->num_codecs; i++) > soc_dapm_dai_stream_event(rtd->codec_dais[i], stream, event); >
On Thu, Jun 21, 2018 at 09:55:54PM -0500, Pierre-Louis Bossart wrote: > > > On 06/20/2018 05:54 AM, Shreyas NC wrote: > >DAPM handles DAIs during soc_dapm_stream_event() and during addition > >and creation of DAI widgets i.e., dapm_add_valid_dai_widget() and > >dapm_connect_dai_link_widgets(). > can you split this patch in two, one where you add > dapm_add_valid_dai_widget() and the second one where you add the multi-cpu > stuff? the current diff format is really hard to read with the two changes > lumped together. As I had replied earlier, the change is really moving the same code from one function to a new one. So, a patch split would mean we would have the same duplicated code in one patch which surely is not desirable. I just realized that in my earlier reply I had excluded the list and replied only to you :) > > >+ for (i = 0; i < rtd->num_codecs; i++) { > >+ struct snd_soc_dai *codec_dai = rtd->codec_dais[i]; > >+ > >+ for (j = 0; j < rtd->num_cpu_dai; j++) { > >+ cpu_dai = rtd->cpu_dais[j]; > >+ > >+ dapm_add_valid_dai_widget(card, rtd, > >+ codec_dai, cpu_dai); > > I didn't click on this earlier, but what makes you think all codec_dais are > connected to all cpu_dais? Yes, there need not be a M:N connectivity. But, how do you find that out ? > That doesn't seem quite right. > For the multi-codec case, all the codec_dais hang from a single cpu_dai. > it's a stretch for me to have a full M:N connectivity. And that's clearly > not the case for SoundWire stream in the multi-link case. I mostly do not disagree with you here.. > Can't we use the dai_link information here to only connect cpu_ and > codec_dais that are related? Which DAI Link information are you referring to here ? Other than the machine driver which sets the audio route, I am unable to figure out how we will find out the connected cpu_dai and codec_dais at ASoC core level. May be I am missing something :( --Shreyas
On 6/22/18 12:53 AM, Shreyas NC wrote: > On Thu, Jun 21, 2018 at 09:55:54PM -0500, Pierre-Louis Bossart wrote: >> >> >> On 06/20/2018 05:54 AM, Shreyas NC wrote: >>> DAPM handles DAIs during soc_dapm_stream_event() and during addition >>> and creation of DAI widgets i.e., dapm_add_valid_dai_widget() and >>> dapm_connect_dai_link_widgets(). >> can you split this patch in two, one where you add >> dapm_add_valid_dai_widget() and the second one where you add the multi-cpu >> stuff? the current diff format is really hard to read with the two changes >> lumped together. > > As I had replied earlier, the change is really moving the same code from one > function to a new one. So, a patch split would mean we would have the same > duplicated code in one patch which surely is not desirable. I don't understand your answer. It's fine to have a small preparation patch that just moves one piece of code to another function, and they change how that function is called. > I just realized that in my earlier reply I had excluded the list and replied > only to you :) > >> >>> + for (i = 0; i < rtd->num_codecs; i++) { >>> + struct snd_soc_dai *codec_dai = rtd->codec_dais[i]; >>> + >>> + for (j = 0; j < rtd->num_cpu_dai; j++) { >>> + cpu_dai = rtd->cpu_dais[j]; >>> + >>> + dapm_add_valid_dai_widget(card, rtd, >>> + codec_dai, cpu_dai); >> >> I didn't click on this earlier, but what makes you think all codec_dais are >> connected to all cpu_dais? > > Yes, there need not be a M:N connectivity. But, how do you find that out ? > >> That doesn't seem quite right. >> For the multi-codec case, all the codec_dais hang from a single cpu_dai. >> it's a stretch for me to have a full M:N connectivity. And that's clearly >> not the case for SoundWire stream in the multi-link case. > > I mostly do not disagree with you here.. > >> Can't we use the dai_link information here to only connect cpu_ and >> codec_dais that are related? > > Which DAI Link information are you referring to here ? > Other than the machine driver which sets the audio route, I am unable to > figure out how we will find out the connected cpu_dai and codec_dais at > ASoC core level. > > May be I am missing something :( How is it different from the multi-codec support? You must have a description somewhere that tells you how the cpu_dai is connected to various codec_dais. Maybe we should start with the examples you provided for Soundwire and describe how the dailinks would be represented. With the M:N connectivity you'd end-up having spurious events with non-existent connections, it's not necessarily fatal but certainly not elegant and may or may not work depending on state management in codec drivers.
> >>>DAPM handles DAIs during soc_dapm_stream_event() and during addition > >>>and creation of DAI widgets i.e., dapm_add_valid_dai_widget() and > >>>dapm_connect_dai_link_widgets(). > >>can you split this patch in two, one where you add > >>dapm_add_valid_dai_widget() and the second one where you add the multi-cpu > >>stuff? the current diff format is really hard to read with the two changes > >>lumped together. > > > >As I had replied earlier, the change is really moving the same code from one > >function to a new one. So, a patch split would mean we would have the same > >duplicated code in one patch which surely is not desirable. > > I don't understand your answer. It's fine to have a small preparation patch > that just moves one piece of code to another function, and they change how > that function is called. > Ok, I will give it a try :) > >I just realized that in my earlier reply I had excluded the list and replied > >only to you :) > > > >> > >>>+ for (i = 0; i < rtd->num_codecs; i++) { > >>>+ struct snd_soc_dai *codec_dai = rtd->codec_dais[i]; > >>>+ > >>>+ for (j = 0; j < rtd->num_cpu_dai; j++) { > >>>+ cpu_dai = rtd->cpu_dais[j]; > >>>+ > >>>+ dapm_add_valid_dai_widget(card, rtd, > >>>+ codec_dai, cpu_dai); > >> > >>I didn't click on this earlier, but what makes you think all codec_dais are > >>connected to all cpu_dais? > > > >Yes, there need not be a M:N connectivity. But, how do you find that out ? > > > >>That doesn't seem quite right. > >>For the multi-codec case, all the codec_dais hang from a single cpu_dai. > >>it's a stretch for me to have a full M:N connectivity. And that's clearly > >>not the case for SoundWire stream in the multi-link case. > > > >I mostly do not disagree with you here.. > > > >>Can't we use the dai_link information here to only connect cpu_ and > >>codec_dais that are related? > > > >Which DAI Link information are you referring to here ? > >Other than the machine driver which sets the audio route, I am unable to > >figure out how we will find out the connected cpu_dai and codec_dais at > >ASoC core level. > > > >May be I am missing something :( > > How is it different from the multi-codec support? You must have a > description somewhere that tells you how the cpu_dai is connected to various > codec_dais. > No, there is no such description that I could find. My reference was skl_nau88l25_ssm4567.c machine driver: /* Back End DAI links */ { /* SSP0 - Codec */ .name = "SSP0-Codec", .id = 0, .cpu_dai_name = "SSP0 Pin", .platform_name = "0000:00:1f.3", .no_pcm = 1, .codecs = ssm4567_codec_components, .num_codecs = ARRAY_SIZE(ssm4567_codec_components), .. }, The only way I could infer the connection was from DAPM route: { /* CODEC BE connections */ { "Left Playback", NULL, "ssp0 Tx"}, { "Right Playback", NULL, "ssp0 Tx"}, { "ssp0 Tx", NULL, "codec0_out"}, } > Maybe we should start with the examples you provided for Soundwire and > describe how the dailinks would be represented. > Ok , sure. Let me describe a case where we would want to play a 2 ch stream and we have two Masters "int-sdw.1" and "int-sdw.2" So, DAI Link would look this way: Here I specify the multi CPU DAIs static struct snd_soc_dai_link_component sdw_multi_cpu_comp[] = { { /* Left */ .name = "int-sdw.1", .dai_name = "SDW1 Pin0", }, { /* Right */ .name = "int-sdw.2", .dai_name = "SDW2 Pin0", }, }; static struct snd_soc_codec_conf rt700_codec_conf[] = { { .dev_name = "sdw:1:25d:700:0:0", .name_prefix = "Left", }, { .dev_name = "sdw:2:25d:701:0:0", .name_prefix = "Right", }, }; And the DAI Link as: struct snd_soc_dai_link cnl_rt700_msic_dailink[] = { { ... { .name = "SDW0-Codec", .cpu_dai = sdw_multi_cpu_comp, .num_cpu_dai = ARRAY_SIZE(sdw_multi_cpu_comp), .platform_name = "0000:00:1f.3", .codecs = sdw_multi_codec_comp, .num_codecs = ARRAY_SIZE(sdw_multi_codec_comp), .. }, } And it is in the DAPM route that I would specify : { ... { "Left DP1 Playback", NULL, "SDW1 Tx0"}, { "SDW1 Tx0", NULL, "codec0_out"}, { "Right DP1 Playback", NULL, "SDW2 Tx0"}, { "SDW2 Tx0", NULL, "codec0_out"}, ... } > With the M:N connectivity you'd end-up having spurious events with > non-existent connections, it's not necessarily fatal but certainly not > elegant and may or may not work depending on state management in codec > drivers. > What are the events that you are referring to here? The reason I ask that is because my understanding is that we will get these events only for those widgets marked connected by DAPM after parsing the DAPM route map. I will check further if you can let me know of the events you are suspecting :) Sorry for the longish mail. I thought it would be better to put in the details rather than go back and forth over them :) --Shreyas >
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index a099c3e..865c47f 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -4113,38 +4113,57 @@ int snd_soc_dapm_link_dai_widgets(struct snd_soc_card *card) return 0; } -static void dapm_connect_dai_link_widgets(struct snd_soc_card *card, - struct snd_soc_pcm_runtime *rtd) +static void dapm_add_valid_dai_widget(struct snd_soc_card *card, + struct snd_soc_pcm_runtime *rtd, + struct snd_soc_dai *codec_dai, + struct snd_soc_dai *cpu_dai) { - struct snd_soc_dai *cpu_dai = rtd->cpu_dai; struct snd_soc_dapm_widget *sink, *source; - int i; - for (i = 0; i < rtd->num_codecs; i++) { - struct snd_soc_dai *codec_dai = rtd->codec_dais[i]; + /* connect BE DAI playback if widgets are valid */ + if (codec_dai->playback_widget && cpu_dai->playback_widget) { + source = cpu_dai->playback_widget; + sink = codec_dai->playback_widget; + dev_err(rtd->dev, "connected DAI link %s:%s -> %s:%s\n", + cpu_dai->component->name, + source->name, + codec_dai->component->name, + sink->name); + + snd_soc_dapm_add_path(&card->dapm, source, sink, + NULL, NULL); + } - /* connect BE DAI playback if widgets are valid */ - if (codec_dai->playback_widget && cpu_dai->playback_widget) { - source = cpu_dai->playback_widget; - sink = codec_dai->playback_widget; - dev_dbg(rtd->dev, "connected DAI link %s:%s -> %s:%s\n", - cpu_dai->component->name, source->name, - codec_dai->component->name, sink->name); + /* connect BE DAI capture if widgets are valid */ + if (codec_dai->capture_widget && cpu_dai->capture_widget) { + source = codec_dai->capture_widget; + sink = cpu_dai->capture_widget; + dev_err(rtd->dev, "connected DAI link %s:%s -> %s:%s\n", + codec_dai->component->name, + source->name, + cpu_dai->component->name, + sink->name); - snd_soc_dapm_add_path(&card->dapm, source, sink, + snd_soc_dapm_add_path(&card->dapm, source, sink, NULL, NULL); - } + } + +} - /* connect BE DAI capture if widgets are valid */ - if (codec_dai->capture_widget && cpu_dai->capture_widget) { - source = codec_dai->capture_widget; - sink = cpu_dai->capture_widget; - dev_dbg(rtd->dev, "connected DAI link %s:%s -> %s:%s\n", - codec_dai->component->name, source->name, - cpu_dai->component->name, sink->name); +static void dapm_connect_dai_link_widgets(struct snd_soc_card *card, + struct snd_soc_pcm_runtime *rtd) +{ + struct snd_soc_dai *cpu_dai; + int i, j; - snd_soc_dapm_add_path(&card->dapm, source, sink, - NULL, NULL); + for (i = 0; i < rtd->num_codecs; i++) { + struct snd_soc_dai *codec_dai = rtd->codec_dais[i]; + + for (j = 0; j < rtd->num_cpu_dai; j++) { + cpu_dai = rtd->cpu_dais[j]; + + dapm_add_valid_dai_widget(card, rtd, + codec_dai, cpu_dai); } } } @@ -4211,7 +4230,9 @@ static void soc_dapm_stream_event(struct snd_soc_pcm_runtime *rtd, int stream, { int i; - soc_dapm_dai_stream_event(rtd->cpu_dai, stream, event); + for (i = 0; i < rtd->num_cpu_dai; i++) + soc_dapm_dai_stream_event(rtd->cpu_dais[i], stream, event); + for (i = 0; i < rtd->num_codecs; i++) soc_dapm_dai_stream_event(rtd->codec_dais[i], stream, event);