| Message ID | 87sgsnfjge.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: modern dai_link style support | expand |
Hi Morimoto-san, One question inline: On Thu, Jun 6, 2019 at 8:26 AM Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > > > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > ASoC is now supporting modern style dai_link > (= snd_soc_dai_link_component) for CPU/Codec/Platform. > This patch switches to use it. > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > sound/soc/sof/nocodec.c | 21 +++++++++++++++++---- > sound/soc/sof/topology.c | 20 +++++++++----------- > 2 files changed, 26 insertions(+), 15 deletions(-) > > diff --git a/sound/soc/sof/nocodec.c b/sound/soc/sof/nocodec.c > index f84b434..3d128e5 100644 > --- a/sound/soc/sof/nocodec.c > +++ b/sound/soc/sof/nocodec.c > @@ -21,6 +21,7 @@ static int sof_nocodec_bes_setup(struct device *dev, > struct snd_soc_dai_link *links, > int link_num, struct snd_soc_card *card) > { > + struct snd_soc_dai_link_component *dlc; > int i; > > if (!ops || !links || !card) > @@ -28,17 +29,29 @@ static int sof_nocodec_bes_setup(struct device *dev, > > /* set up BE dai_links */ > for (i = 0; i < link_num; i++) { > + dlc = devm_kzalloc(dev, 3 * sizeof(*dlc), GFP_KERNEL); > + if (!dlc) > + return -ENOMEM; > + > links[i].name = devm_kasprintf(dev, GFP_KERNEL, > "NoCodec-%d", i); > if (!links[i].name) > return -ENOMEM; > > + links[i].cpus = &dlc[0]; > + links[i].codecs = &dlc[1]; > + links[i].platforms = &dlc[2]; > + > + links[i].num_cpus = 1; > + links[i].num_codecs = 1; > + links[i].num_platforms = 1; > + > links[i].id = i; > links[i].no_pcm = 1; > - links[i].cpu_dai_name = ops->drv[i].name; > - links[i].platform_name = dev_name(dev); > - links[i].codec_dai_name = "snd-soc-dummy-dai"; > - links[i].codec_name = "snd-soc-dummy"; > + links[i].cpus->dai_name = ops->drv[i].name; > + links[i].platforms->name = dev_name(dev); > + links[i].codecs->dai_name = "snd-soc-dummy-dai"; > + links[i].codecs->name = "snd-soc-dummy"; > links[i].dpcm_playback = 1; > links[i].dpcm_capture = 1; > } > diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c > index b969686f..a13233a 100644 > --- a/sound/soc/sof/topology.c > +++ b/sound/soc/sof/topology.c > @@ -2639,7 +2639,6 @@ static int sof_link_hda_load(struct snd_soc_component *scomp, int index, > struct sof_ipc_dai_config *config) > { > struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(scomp); > - struct snd_soc_dai_link_component dai_component; > struct snd_soc_tplg_private *private = &cfg->priv; > struct snd_soc_dai *dai; > u32 size = sizeof(*config); > @@ -2650,7 +2649,6 @@ static int sof_link_hda_load(struct snd_soc_component *scomp, int index, > int ret; > > /* init IPC */ > - memset(&dai_component, 0, sizeof(dai_component)); > memset(&config->hda, 0, sizeof(struct sof_ipc_dai_hda_params)); > config->hdr.size = size; > > @@ -2664,11 +2662,10 @@ static int sof_link_hda_load(struct snd_soc_component *scomp, int index, > return ret; > } > > - dai_component.dai_name = link->cpu_dai_name; > - dai = snd_soc_find_dai(&dai_component); > + dai = snd_soc_find_dai(link->cpus); > if (!dai) { > dev_err(sdev->dev, "error: failed to find dai %s in %s", > - dai_component.dai_name, __func__); > + link->cpus->dai_name, __func__); > return -EINVAL; > } > > @@ -2708,7 +2705,11 @@ static int sof_link_load(struct snd_soc_component *scomp, int index, > int ret; > int i = 0; > > - link->platform_name = dev_name(sdev->dev); > + if (!link->platforms) { > + dev_err(sdev->dev, "error: no platforms\n"); > + return -EINVAL; Why do we need this check? With linux-next this check fails for me. I do also have some local changes so take this question with a grain of salt. This check fails for FE links as they are created as follows: /* create the FE DAI link */ static int soc_tplg_fe_link_create => link->cpus» = &dlc[0]; => link->codecs» = &dlc[1]; => link->num_cpus» = 1; => link->num_codecs = 1; So, there is no platforms set up for FE links. I could get rid of the failure with the following patch: diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index f485f7f751a1..ee73318135fc 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -1883,7 +1883,7 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, int ret; /* link + cpu + codec */ - link = kzalloc(sizeof(*link) + (2 * sizeof(*dlc)), GFP_KERNEL); + link = kzalloc(sizeof(*link) + (3 * sizeof(*dlc)), GFP_KERNEL); if (link == NULL) return -ENOMEM; @@ -1891,9 +1891,11 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, link->cpus = &dlc[0]; link->codecs = &dlc[1]; + link->platforms = &dlc[2]; link->num_cpus = 1; link->num_codecs = 1; + link->num_platforms = 1; Can you please help me figure this out? thanks, Daniel.
> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c > index f485f7f751a1..ee73318135fc 100644 > --- a/sound/soc/soc-topology.c > +++ b/sound/soc/soc-topology.c > @@ -1883,7 +1883,7 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, > int ret; > > /* link + cpu + codec */ > - link = kzalloc(sizeof(*link) + (2 * sizeof(*dlc)), GFP_KERNEL); > + link = kzalloc(sizeof(*link) + (3 * sizeof(*dlc)), GFP_KERNEL); > if (link == NULL) > return -ENOMEM; > > @@ -1891,9 +1891,11 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, > > link->cpus = &dlc[0]; > link->codecs = &dlc[1]; > + link->platforms = &dlc[2]; > > link->num_cpus = 1; > link->num_codecs = 1; > + link->num_platforms = 1; > > Can you please help me figure this out? Isn't this fixed by my patch "ASoC: soc-topology: fix modern dai link style" applied on 6/13? Looks like the same issue to me.
Hi Daniel Thank you for feedback and sorry for bother you > > @@ -2708,7 +2705,11 @@ static int sof_link_load(struct snd_soc_component *scomp, int index, > > int ret; > > int i = 0; > > > > - link->platform_name = dev_name(sdev->dev); > > + if (!link->platforms) { > > + dev_err(sdev->dev, "error: no platforms\n"); > > + return -EINVAL; > > Why do we need this check? With linux-next this check fails for me. I don't remember but some sof might use without platform it I thought. But, current ALSA SoC can handle NULL platform today. Maybe/Maybe not support timing issue ? > diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c > index f485f7f751a1..ee73318135fc 100644 > --- a/sound/soc/soc-topology.c > +++ b/sound/soc/soc-topology.c > @@ -1883,7 +1883,7 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, > int ret; > > /* link + cpu + codec */ > - link = kzalloc(sizeof(*link) + (2 * sizeof(*dlc)), GFP_KERNEL); > + link = kzalloc(sizeof(*link) + (3 * sizeof(*dlc)), GFP_KERNEL); > if (link == NULL) > return -ENOMEM; > > @@ -1891,9 +1891,11 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, > > link->cpus = &dlc[0]; > link->codecs = &dlc[1]; > + link->platforms = &dlc[2]; > > link->num_cpus = 1; > link->num_codecs = 1; > + link->num_platforms = 1; > > Can you please help me figure this out? I think this is same as 3e6de89409bf7ad149bfb05dd0dce6c5678ea0a8 ("ASoC: soc-topology: fix modern dai link style") Above one is also OK, but now we can use NULL platform. I'm not familiar with SOF, but maybe like this code instead of having dummy platform can salve your issue ? if (link->platforms) { link->platforms->name = dev_name(sdev->dev); }
Hi Morimoto-san, <replying again now also including all Cc's> On Thu, Jun 27, 2019 at 3:52 AM Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > > > Hi Daniel > > Thank you for feedback and sorry for bother you > > > > @@ -2708,7 +2705,11 @@ static int sof_link_load(struct snd_soc_component *scomp, int index, > > > int ret; > > > int i = 0; > > > > > > - link->platform_name = dev_name(sdev->dev); > > > + if (!link->platforms) { > > > + dev_err(sdev->dev, "error: no platforms\n"); > > > + return -EINVAL; > > > > Why do we need this check? With linux-next this check fails for me. > > I don't remember but some sof might use without platform it I thought. > But, current ALSA SoC can handle NULL platform today. > Maybe/Maybe not support timing issue ? > > > diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c > > index f485f7f751a1..ee73318135fc 100644 > > --- a/sound/soc/soc-topology.c > > +++ b/sound/soc/soc-topology.c > > @@ -1883,7 +1883,7 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, > > int ret; > > > > /* link + cpu + codec */ > > - link = kzalloc(sizeof(*link) + (2 * sizeof(*dlc)), GFP_KERNEL); > > + link = kzalloc(sizeof(*link) + (3 * sizeof(*dlc)), GFP_KERNEL); > > if (link == NULL) > > return -ENOMEM; > > > > @@ -1891,9 +1891,11 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, > > > > link->cpus = &dlc[0]; > > link->codecs = &dlc[1]; > > + link->platforms = &dlc[2]; > > > > link->num_cpus = 1; > > link->num_codecs = 1; > > + link->num_platforms = 1; > > > > Can you please help me figure this out? > > I think this is same as > > 3e6de89409bf7ad149bfb05dd0dce6c5678ea0a8 > ("ASoC: soc-topology: fix modern dai link style") > Yes, it is! Sorry, I'm doing development on 5.2-rc3 - not latest sources. > Above one is also OK, but now we can use NULL platform. > I'm not familiar with SOF, but maybe like this code instead > of having dummy platform can salve your issue ? > > if (link->platforms) { > link->platforms->name = dev_name(sdev->dev); > } > Yes, this also seems to work.
Hi Daniel > > > diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c > > > index f485f7f751a1..ee73318135fc 100644 > > > --- a/sound/soc/soc-topology.c > > > +++ b/sound/soc/soc-topology.c > > > @@ -1883,7 +1883,7 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, > > > int ret; > > > > > > /* link + cpu + codec */ > > > - link = kzalloc(sizeof(*link) + (2 * sizeof(*dlc)), GFP_KERNEL); > > > + link = kzalloc(sizeof(*link) + (3 * sizeof(*dlc)), GFP_KERNEL); > > > if (link == NULL) > > > return -ENOMEM; > > > > > > @@ -1891,9 +1891,11 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, > > > > > > link->cpus = &dlc[0]; > > > link->codecs = &dlc[1]; > > > + link->platforms = &dlc[2]; > > > > > > link->num_cpus = 1; > > > link->num_codecs = 1; > > > + link->num_platforms = 1; (snip) > > Above one is also OK, but now we can use NULL platform. > > I'm not familiar with SOF, but maybe like this code instead > > of having dummy platform can salve your issue ? > > > > if (link->platforms) { > > link->platforms->name = dev_name(sdev->dev); > > } > > > Yes, this also seems to work. Thanks, Nice to know !! For SOF future, NULL platform support is nice idea, I think. Thank you for your help !! Best regards --- Kuninori Morimoto
On Wed, 2019-06-26 at 22:58 +0200, Pierre-Louis Bossart wrote: > > diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c > > index f485f7f751a1..ee73318135fc 100644 > > --- a/sound/soc/soc-topology.c > > +++ b/sound/soc/soc-topology.c > > @@ -1883,7 +1883,7 @@ static int soc_tplg_fe_link_create(struct > > soc_tplg *tplg, > > int ret; > > > > /* link + cpu + codec */ > > - link = kzalloc(sizeof(*link) + (2 * sizeof(*dlc)), > > GFP_KERNEL); > > + link = kzalloc(sizeof(*link) + (3 * sizeof(*dlc)), > > GFP_KERNEL); > > if (link == NULL) > > return -ENOMEM; > > > > @@ -1891,9 +1891,11 @@ static int soc_tplg_fe_link_create(struct > > soc_tplg *tplg, > > > > link->cpus = &dlc[0]; > > link->codecs = &dlc[1]; > > + link->platforms = &dlc[2]; > > > > link->num_cpus = 1; > > link->num_codecs = 1; > > + link->num_platforms = 1; > > > > Can you please help me figure this out? > > Isn't this fixed by my patch "ASoC: soc-topology: fix modern dai > link > style" applied on 6/13? Looks like the same issue to me. Yes, this is the same thing. My tree with all NXP i.MX8QXP patches is a little bit older and did not contain your patch. Sorry for the noise, everything is looking good now! :)
Hi Pierre-Louis > > @@ -1883,7 +1883,7 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, > > int ret; > > > > /* link + cpu + codec */ > > - link = kzalloc(sizeof(*link) + (2 * sizeof(*dlc)), GFP_KERNEL); > > + link = kzalloc(sizeof(*link) + (3 * sizeof(*dlc)), GFP_KERNEL); > > if (link == NULL) > > return -ENOMEM; > > > > @@ -1891,9 +1891,11 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, > > > > link->cpus = &dlc[0]; > > link->codecs = &dlc[1]; > > + link->platforms = &dlc[2]; > > > > link->num_cpus = 1; > > link->num_codecs = 1; > > + link->num_platforms = 1; > > > > Can you please help me figure this out? > > Isn't this fixed by my patch "ASoC: soc-topology: fix modern dai link > style" applied on 6/13? Looks like the same issue to me. This is very impertinent comment, but it is possible to allow NULL platform instead of dummy platform by this or similar code ? I guess it is nice for SOF future. I can't test and not familiar with SOF thought... if (link->platforms) link->platforms->name = dev_name(sdev->dev); Thank you for your help !! Best regards --- Kuninori Morimoto
On 6/28/19 4:06 AM, Kuninori Morimoto wrote: > > Hi Pierre-Louis > >>> @@ -1883,7 +1883,7 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, >>> int ret; >>> >>> /* link + cpu + codec */ >>> - link = kzalloc(sizeof(*link) + (2 * sizeof(*dlc)), GFP_KERNEL); >>> + link = kzalloc(sizeof(*link) + (3 * sizeof(*dlc)), GFP_KERNEL); >>> if (link == NULL) >>> return -ENOMEM; >>> >>> @@ -1891,9 +1891,11 @@ static int soc_tplg_fe_link_create(struct soc_tplg *tplg, >>> >>> link->cpus = &dlc[0]; >>> link->codecs = &dlc[1]; >>> + link->platforms = &dlc[2]; >>> >>> link->num_cpus = 1; >>> link->num_codecs = 1; >>> + link->num_platforms = 1; >>> >>> Can you please help me figure this out? >> >> Isn't this fixed by my patch "ASoC: soc-topology: fix modern dai link >> style" applied on 6/13? Looks like the same issue to me. > > This is very impertinent comment, but it is possible to allow NULL platform > instead of dummy platform by this or similar code ? > I guess it is nice for SOF future. > I can't test and not familiar with SOF thought... > > if (link->platforms) > link->platforms->name = dev_name(sdev->dev); It's a good question. To be honest I don't fully understand what this 'platform' field is needed for... I was just trying to maintain 'as-is' functionality. If anyone has a good explanation on when this field might be required and for what purpose, and when it can be made optional, I am all ears.
Hi Pierre-Louis > > This is very impertinent comment, but it is possible to allow NULL platform > > instead of dummy platform by this or similar code ? > > I guess it is nice for SOF future. > > I can't test and not familiar with SOF thought... > > > > if (link->platforms) > > link->platforms->name = dev_name(sdev->dev); > > It's a good question. To be honest I don't fully understand what this > 'platform' field is needed for... I was just trying to maintain > 'as-is' functionality. If anyone has a good explanation on when this > field might be required and for what purpose, and when it can be made > optional, I am all ears. I can try to explain. Originally "Platform" component is for "DMA" transfer (But I'm not sure detail. It had been exist when I started to work for ALSA SoC...) But in many SoC, "CPU" component is doing it. Some SoC needs special "Platform" in my understanding. Before, if Card didn't select "Platform", ALSA SoC had selects "dummy platform" automatically. Today, if Card didn't select it, ALSA SoC will just igore it. I guess you added "dummy platform" patch because SOF had this code if (!link->platforms) { dev_err(...); ... } This come from my patch, but it is just wrong guess. I don't remember why I did it, but I thought SOF has it. I'm not familiar with SOF, but it can accept NULL Platform if we can fix above code to like this ? if (link->platforms) link->platforms-> ... But, selecting dummy Platforms itself is not wrong idea. # 100% wrong idea was my patch orz Thank you for your help !! Best regards --- Kuninori Morimoto
On Fri, Jun 28, 2019 at 03:43:47PM +0900, Kuninori Morimoto wrote: > Originally "Platform" component is for "DMA" transfer > (But I'm not sure detail. It had been exist when I started to work for ALSA SoC...) > But in many SoC, "CPU" component is doing it. > Some SoC needs special "Platform" in my understanding. Right, most systems needed a separate driver for DMA historically - they still do but for a lot of them it's now done less visibly by the dmaengine integration.
diff --git a/sound/soc/sof/nocodec.c b/sound/soc/sof/nocodec.c index f84b434..3d128e5 100644 --- a/sound/soc/sof/nocodec.c +++ b/sound/soc/sof/nocodec.c @@ -21,6 +21,7 @@ static int sof_nocodec_bes_setup(struct device *dev, struct snd_soc_dai_link *links, int link_num, struct snd_soc_card *card) { + struct snd_soc_dai_link_component *dlc; int i; if (!ops || !links || !card) @@ -28,17 +29,29 @@ static int sof_nocodec_bes_setup(struct device *dev, /* set up BE dai_links */ for (i = 0; i < link_num; i++) { + dlc = devm_kzalloc(dev, 3 * sizeof(*dlc), GFP_KERNEL); + if (!dlc) + return -ENOMEM; + links[i].name = devm_kasprintf(dev, GFP_KERNEL, "NoCodec-%d", i); if (!links[i].name) return -ENOMEM; + links[i].cpus = &dlc[0]; + links[i].codecs = &dlc[1]; + links[i].platforms = &dlc[2]; + + links[i].num_cpus = 1; + links[i].num_codecs = 1; + links[i].num_platforms = 1; + links[i].id = i; links[i].no_pcm = 1; - links[i].cpu_dai_name = ops->drv[i].name; - links[i].platform_name = dev_name(dev); - links[i].codec_dai_name = "snd-soc-dummy-dai"; - links[i].codec_name = "snd-soc-dummy"; + links[i].cpus->dai_name = ops->drv[i].name; + links[i].platforms->name = dev_name(dev); + links[i].codecs->dai_name = "snd-soc-dummy-dai"; + links[i].codecs->name = "snd-soc-dummy"; links[i].dpcm_playback = 1; links[i].dpcm_capture = 1; } diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c index b969686f..a13233a 100644 --- a/sound/soc/sof/topology.c +++ b/sound/soc/sof/topology.c @@ -2639,7 +2639,6 @@ static int sof_link_hda_load(struct snd_soc_component *scomp, int index, struct sof_ipc_dai_config *config) { struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(scomp); - struct snd_soc_dai_link_component dai_component; struct snd_soc_tplg_private *private = &cfg->priv; struct snd_soc_dai *dai; u32 size = sizeof(*config); @@ -2650,7 +2649,6 @@ static int sof_link_hda_load(struct snd_soc_component *scomp, int index, int ret; /* init IPC */ - memset(&dai_component, 0, sizeof(dai_component)); memset(&config->hda, 0, sizeof(struct sof_ipc_dai_hda_params)); config->hdr.size = size; @@ -2664,11 +2662,10 @@ static int sof_link_hda_load(struct snd_soc_component *scomp, int index, return ret; } - dai_component.dai_name = link->cpu_dai_name; - dai = snd_soc_find_dai(&dai_component); + dai = snd_soc_find_dai(link->cpus); if (!dai) { dev_err(sdev->dev, "error: failed to find dai %s in %s", - dai_component.dai_name, __func__); + link->cpus->dai_name, __func__); return -EINVAL; } @@ -2708,7 +2705,11 @@ static int sof_link_load(struct snd_soc_component *scomp, int index, int ret; int i = 0; - link->platform_name = dev_name(sdev->dev); + if (!link->platforms) { + dev_err(sdev->dev, "error: no platforms\n"); + return -EINVAL; + } + link->platforms->name = dev_name(sdev->dev); /* * Set nonatomic property for FE dai links as their trigger action @@ -2801,16 +2802,13 @@ static int sof_link_load(struct snd_soc_component *scomp, int index, static int sof_link_hda_unload(struct snd_sof_dev *sdev, struct snd_soc_dai_link *link) { - struct snd_soc_dai_link_component dai_component; struct snd_soc_dai *dai; int ret = 0; - memset(&dai_component, 0, sizeof(dai_component)); - dai_component.dai_name = link->cpu_dai_name; - dai = snd_soc_find_dai(&dai_component); + dai = snd_soc_find_dai(link->cpus); if (!dai) { dev_err(sdev->dev, "error: failed to find dai %s in %s", - dai_component.dai_name, __func__); + link->cpus->dai_name, __func__); return -EINVAL; }