[PATCH v3] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function
Split be_hw_params_fixup function for different codecs as current common function, leads to crash while trying to get snd_soc_dpcm with container_of() macro in kabylake_ssp_fixup(). The crash call path looks as below: soc_pcm_hw_params() snd_soc_dai_hw_params(codec_dai, substream, &codec_params); rtd->dai_link->be_hw_params_fixup(rtd, params) kabylake_ssp_fixup() In this case, codec_params is just a copy of an internal structure and is not embedded into struct snd_soc_dpcm thus we cannot use container_of() on it.
v1 -> v2: - Extract dmic from SSP0 as every BE should have own fixup function. v2 -> v3: - Restore naming in the dapm route table to not confuse with other drivers - Fixed indentations
Signed-off-by: Lukasz Majczak lma@semihalf.com --- .../intel/boards/kbl_rt5663_rt5514_max98927.c | 130 +++++++++++------- 1 file changed, 84 insertions(+), 46 deletions(-)
diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c index 1b1f8d7a4ea3..b9596cf3a6c4 100644 --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c @@ -328,42 +328,51 @@ static const struct snd_soc_ops kabylake_rt5663_fe_ops = { .startup = kbl_fe_startup, };
-static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime *rtd, - struct snd_pcm_hw_params *params) +static void kabylake_ssp_fixup(struct snd_soc_pcm_runtime *rtd, + struct snd_pcm_hw_params *params, snd_pcm_format_t pcm_fmt) { struct snd_interval *rate = hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE); - struct snd_interval *chan = hw_param_interval(params, + struct snd_interval *channels = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS); struct snd_mask *fmt = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT); - struct snd_soc_dpcm *dpcm = container_of( - params, struct snd_soc_dpcm, hw_params); - struct snd_soc_dai_link *fe_dai_link = dpcm->fe->dai_link; - struct snd_soc_dai_link *be_dai_link = dpcm->be->dai_link;
/* * The ADSP will convert the FE rate to 48k, stereo, 24 bit */ - if (!strcmp(fe_dai_link->name, "Kbl Audio Port") || - !strcmp(fe_dai_link->name, "Kbl Audio Headset Playback") || - !strcmp(fe_dai_link->name, "Kbl Audio Capture Port")) { - rate->min = rate->max = 48000; - chan->min = chan->max = 2; - snd_mask_none(fmt); - snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S24_LE); - } else if (!strcmp(fe_dai_link->name, "Kbl Audio DMIC cap")) { - if (params_channels(params) == 2 || - DMIC_CH(dmic_constraints) == 2) - chan->min = chan->max = 2; - else - chan->min = chan->max = 4; - } - /* - * The speaker on the SSP0 supports S16_LE and not S24_LE. - * thus changing the mask here - */ - if (!strcmp(be_dai_link->name, "SSP0-Codec")) - snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S16_LE); + + rate->min = rate->max = 48000; + channels->min = channels->max = 2; + + snd_mask_none(fmt); + snd_mask_set_format(fmt, pcm_fmt); +} + +static int kabylake_ssp0_fixup(struct snd_soc_pcm_runtime *rtd, + struct snd_pcm_hw_params *params) +{ + kabylake_ssp_fixup(rtd, params, SNDRV_PCM_FORMAT_S16_LE); + return 0; +} + +static int kabylake_ssp1_fixup(struct snd_soc_pcm_runtime *rtd, + struct snd_pcm_hw_params *params) +{ + + kabylake_ssp_fixup(rtd, params, SNDRV_PCM_FORMAT_S24_LE); + return 0; +} + +static int kabylake_dmic_fixup(struct snd_soc_pcm_runtime *rtd, + struct snd_pcm_hw_params *params) +{ + struct snd_interval *channels = hw_param_interval(params, + SNDRV_PCM_HW_PARAM_CHANNELS); + + if (params_channels(params) == 2 || DMIC_CH(dmic_constraints) == 2) + channels->min = channels->max = 2; + else + channels->min = channels->max = 4;
return 0; } @@ -400,20 +409,6 @@ static int kabylake_ssp0_hw_params(struct snd_pcm_substream *substream, int ret = 0, j;
for_each_rtd_codec_dais(rtd, j, codec_dai) { - if (!strcmp(codec_dai->component->name, RT5514_DEV_NAME)) { - ret = snd_soc_dai_set_tdm_slot(codec_dai, 0xF, 0, 8, 16); - if (ret < 0) { - dev_err(rtd->dev, "set TDM slot err:%d\n", ret); - return ret; - } - - ret = snd_soc_dai_set_sysclk(codec_dai, - RT5514_SCLK_S_MCLK, 24576000, SND_SOC_CLOCK_IN); - if (ret < 0) { - dev_err(rtd->dev, "set sysclk err: %d\n", ret); - return ret; - } - } if (!strcmp(codec_dai->component->name, MAXIM_DEV0_NAME)) { ret = snd_soc_dai_set_tdm_slot(codec_dai, 0x30, 3, 8, 16); if (ret < 0) { @@ -433,10 +428,36 @@ static int kabylake_ssp0_hw_params(struct snd_pcm_substream *substream, return ret; }
+static int kabylake_dmic01_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + int ret = 0; + + ret = snd_soc_dai_set_tdm_slot(rtd->codec_dai, 0xF, 0, 8, 16); + if (ret < 0) { + dev_err(rtd->dev, "set TDM slot err:%d\n", ret); + return ret; + } + + ret = snd_soc_dai_set_sysclk(rtd->codec_dai, + RT5514_SCLK_S_MCLK, 24576000, SND_SOC_CLOCK_IN); + if (ret < 0) { + dev_err(rtd->dev, "set sysclk err: %d\n", ret); + return ret; + } + + return ret; +} + static struct snd_soc_ops kabylake_ssp0_ops = { .hw_params = kabylake_ssp0_hw_params, };
+static struct snd_soc_ops kabylake_dmic01_ops = { + .hw_params = kabylake_dmic01_hw_params, +}; + static const unsigned int channels_dmic[] = { 4, }; @@ -507,14 +528,19 @@ SND_SOC_DAILINK_DEF(ssp0_pin, SND_SOC_DAILINK_DEF(ssp0_codec, DAILINK_COMP_ARRAY( /* Left */ COMP_CODEC(MAXIM_DEV0_NAME, KBL_MAXIM_CODEC_DAI), - /* Right */COMP_CODEC(MAXIM_DEV1_NAME, KBL_MAXIM_CODEC_DAI), - /* dmic */ COMP_CODEC(RT5514_DEV_NAME, KBL_REALTEK_DMIC_CODEC_DAI))); + /* Right */COMP_CODEC(MAXIM_DEV1_NAME, KBL_MAXIM_CODEC_DAI)));
SND_SOC_DAILINK_DEF(ssp1_pin, DAILINK_COMP_ARRAY(COMP_CPU("SSP1 Pin"))); SND_SOC_DAILINK_DEF(ssp1_codec, DAILINK_COMP_ARRAY(COMP_CODEC(RT5663_DEV_NAME, KBL_REALTEK_CODEC_DAI)));
+SND_SOC_DAILINK_DEF(dmic01_pin, + DAILINK_COMP_ARRAY(COMP_CPU("DMIC01 Pin"))); +SND_SOC_DAILINK_DEF(dmic01_codec, + DAILINK_COMP_ARRAY( + COMP_CODEC(RT5514_DEV_NAME, KBL_REALTEK_DMIC_CODEC_DAI))); + SND_SOC_DAILINK_DEF(idisp1_pin, DAILINK_COMP_ARRAY(COMP_CPU("iDisp1 Pin"))); SND_SOC_DAILINK_DEF(idisp1_codec, @@ -618,9 +644,8 @@ static struct snd_soc_dai_link kabylake_dais[] = { SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS, .ignore_pmdown_time = 1, - .be_hw_params_fixup = kabylake_ssp_fixup, + .be_hw_params_fixup = kabylake_ssp0_fixup, .dpcm_playback = 1, - .dpcm_capture = 1, .ops = &kabylake_ssp0_ops, SND_SOC_DAILINK_REG(ssp0_pin, ssp0_codec, platform), }, @@ -632,12 +657,25 @@ static struct snd_soc_dai_link kabylake_dais[] = { .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS, .ignore_pmdown_time = 1, - .be_hw_params_fixup = kabylake_ssp_fixup, + .be_hw_params_fixup = kabylake_ssp1_fixup, .ops = &kabylake_rt5663_ops, .dpcm_playback = 1, .dpcm_capture = 1, SND_SOC_DAILINK_REG(ssp1_pin, ssp1_codec, platform), }, + { + .name = "dmic01", + .id = 2, + .no_pcm = 1, + .dai_fmt = SND_SOC_DAIFMT_DSP_B | + SND_SOC_DAIFMT_NB_NF | + SND_SOC_DAIFMT_CBS_CFS, + .ignore_pmdown_time = 1, + .be_hw_params_fixup = kabylake_dmic_fixup, + .dpcm_capture = 1, + .ops = &kabylake_dmic01_ops, + SND_SOC_DAILINK_REG(dmic01_pin, dmic01_codec, platform), + }, { .name = "iDisp1", .id = 3,
base-commit: a4f6fc98cd2fa1774bcaeb248c67156ef9402a56
On 5/21/20 11:25 AM, Lukasz Majczak wrote:
Split be_hw_params_fixup function for different codecs as current common function, leads to crash while trying to get snd_soc_dpcm with container_of() macro in kabylake_ssp_fixup(). The crash call path looks as below: soc_pcm_hw_params() snd_soc_dai_hw_params(codec_dai, substream, &codec_params); rtd->dai_link->be_hw_params_fixup(rtd, params) kabylake_ssp_fixup() In this case, codec_params is just a copy of an internal structure and is not embedded into struct snd_soc_dpcm thus we cannot use container_of() on it.
v1 -> v2:
- Extract dmic from SSP0 as every BE should have own fixup function.
v2 -> v3:
- Restore naming in the dapm route table to not confuse with other
drivers
- Fixed indentations
you need the changelog to be below the --- marker two lines down.
Signed-off-by: Lukasz Majczak lma@semihalf.com
[...]
+static int kabylake_dmic01_hw_params(struct snd_pcm_substream *substream,
- struct snd_pcm_hw_params *params)
+{
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
- int ret = 0;
- ret = snd_soc_dai_set_tdm_slot(rtd->codec_dai, 0xF, 0, 8, 16);
- if (ret < 0) {
dev_err(rtd->dev, "set TDM slot err:%d\n", ret);return ret;- }
- ret = snd_soc_dai_set_sysclk(rtd->codec_dai,
RT5514_SCLK_S_MCLK, 24576000, SND_SOC_CLOCK_IN);- if (ret < 0) {
dev_err(rtd->dev, "set sysclk err: %d\n", ret);return ret;- }
- return ret;
+}
- static struct snd_soc_ops kabylake_ssp0_ops = { .hw_params = kabylake_ssp0_hw_params, };
+static struct snd_soc_ops kabylake_dmic01_ops = {
- .hw_params = kabylake_dmic01_hw_params,
+};
- static const unsigned int channels_dmic[] = { 4, };
@@ -507,14 +528,19 @@ SND_SOC_DAILINK_DEF(ssp0_pin, SND_SOC_DAILINK_DEF(ssp0_codec, DAILINK_COMP_ARRAY( /* Left */ COMP_CODEC(MAXIM_DEV0_NAME, KBL_MAXIM_CODEC_DAI),
- /* Right */COMP_CODEC(MAXIM_DEV1_NAME, KBL_MAXIM_CODEC_DAI),
- /* dmic */ COMP_CODEC(RT5514_DEV_NAME, KBL_REALTEK_DMIC_CODEC_DAI)));
/* Right */COMP_CODEC(MAXIM_DEV1_NAME, KBL_MAXIM_CODEC_DAI)));
SND_SOC_DAILINK_DEF(ssp1_pin, DAILINK_COMP_ARRAY(COMP_CPU("SSP1 Pin"))); SND_SOC_DAILINK_DEF(ssp1_codec, DAILINK_COMP_ARRAY(COMP_CODEC(RT5663_DEV_NAME, KBL_REALTEK_CODEC_DAI)));
+SND_SOC_DAILINK_DEF(dmic01_pin,
- DAILINK_COMP_ARRAY(COMP_CPU("DMIC01 Pin")));
+SND_SOC_DAILINK_DEF(dmic01_codec,
- DAILINK_COMP_ARRAY(
COMP_CODEC(RT5514_DEV_NAME, KBL_REALTEK_DMIC_CODEC_DAI)));- SND_SOC_DAILINK_DEF(idisp1_pin, DAILINK_COMP_ARRAY(COMP_CPU("iDisp1 Pin"))); SND_SOC_DAILINK_DEF(idisp1_codec,
@@ -618,9 +644,8 @@ static struct snd_soc_dai_link kabylake_dais[] = { SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS, .ignore_pmdown_time = 1,
.be_hw_params_fixup = kabylake_ssp_fixup,
.be_hw_params_fixup = kabylake_ssp0_fixup,
.dpcm_capture = 1,@@ -632,12 +657,25 @@ static struct snd_soc_dai_link kabylake_dais[] = { .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS, .ignore_pmdown_time = 1,
.be_hw_params_fixup = kabylake_ssp_fixup,
.be_hw_params_fixup = kabylake_ssp1_fixup,- {
.name = "dmic01",.id = 2,.no_pcm = 1,.dai_fmt = SND_SOC_DAIFMT_DSP_B |SND_SOC_DAIFMT_NB_NF |SND_SOC_DAIFMT_CBS_CFS,.ignore_pmdown_time = 1,.be_hw_params_fixup = kabylake_dmic_fixup,.dpcm_capture = 1,.ops = &kabylake_dmic01_ops,SND_SOC_DAILINK_REG(dmic01_pin, dmic01_codec, platform),- },
don't add a new dailink, this is not right.
{ .name = "iDisp1", .id = 3,
base-commit: a4f6fc98cd2fa1774bcaeb248c67156ef9402a56
On 5/21/20 12:08 PM, Łukasz Majczak wrote:
don't add a new dailink, this is not right.
Can you advise a better solution how to assign different fixup functions to mic and to speakers? I was looking at "dmic01" dailink in skl_nau88l25_max98357a.c as an example.
I am not sure I follow. the DMICs are handled on a shared SSP, so how would one set a different fixup? The word length have to be the same.
Hi Pierre
If you will take a look at the original kabylake_ssp_fixup() you will see that it is checking whether the related FE is "Kbl Audio Port", "Kbl Audio Headset Playback", "Kbl Audio Capture Port" or "Kbl Audio DMIC cap" - then for the first 3 cases it sets min/max channels to 2 while for the "Kbl DMIC cap" it can be 2 or 4, that's is why I'm trying to split this, but maybe I'm missing here something.
Best regards, Lukasz
czw., 21 maj 2020 o 19:17 Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com napisał(a):
On 5/21/20 12:08 PM, Łukasz Majczak wrote:
don't add a new dailink, this is not right.
Can you advise a better solution how to assign different fixup functions to mic and to speakers? I was looking at "dmic01" dailink in skl_nau88l25_max98357a.c as an example.
I am not sure I follow. the DMICs are handled on a shared SSP, so how would one set a different fixup? The word length have to be the same.
On 5/21/20 12:30 PM, Łukasz Majczak wrote:
Hi Pierre
If you will take a look at the original kabylake_ssp_fixup() you will see that it is checking whether the related FE is "Kbl Audio Port", "Kbl Audio Headset Playback", "Kbl Audio Capture Port" or "Kbl Audio DMIC cap" - then for the first 3 cases it sets min/max channels to 2 while for the "Kbl DMIC cap" it can be 2 or 4, that's is why I'm trying to split this, but maybe I'm missing here something.
I don't understand this code either.
I believe the intent is that for all SSP1-RT5663 usages, we should use
rate->min = rate->max = 48000; chan->min = chan->max = 2; snd_mask_none(fmt); snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S24_LE);
That is pretty easy to move to a dedicated ssp1 fixup.
for SSP0, we have RT5514 for capture and max98927 for playback, but the existing code does not explicitly deal with rate/channels/format for all cases, so it's not clear what should happen.
Harsha, can you help here?
Best regards, Lukasz
czw., 21 maj 2020 o 19:17 Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com napisał(a):
On 5/21/20 12:08 PM, Łukasz Majczak wrote:
don't add a new dailink, this is not right.
Can you advise a better solution how to assign different fixup functions to mic and to speakers? I was looking at "dmic01" dailink in skl_nau88l25_max98357a.c as an example.
I am not sure I follow. the DMICs are handled on a shared SSP, so how would one set a different fixup? The word length have to be the same.
Hi Harsha,
We would like to continue the work on this, could you please suggest the correct approach.
Best regards, Lukasz
czw., 21 maj 2020 o 20:10 Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com napisał(a):
On 5/21/20 12:30 PM, Łukasz Majczak wrote:
Hi Pierre
If you will take a look at the original kabylake_ssp_fixup() you will see that it is checking whether the related FE is "Kbl Audio Port", "Kbl Audio Headset Playback", "Kbl Audio Capture Port" or "Kbl Audio DMIC cap" - then for the first 3 cases it sets min/max channels to 2 while for the "Kbl DMIC cap" it can be 2 or 4, that's is why I'm trying to split this, but maybe I'm missing here something.
I don't understand this code either.
I believe the intent is that for all SSP1-RT5663 usages, we should use
rate->min = rate->max = 48000; chan->min = chan->max = 2; snd_mask_none(fmt); snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S24_LE);That is pretty easy to move to a dedicated ssp1 fixup.
for SSP0, we have RT5514 for capture and max98927 for playback, but the existing code does not explicitly deal with rate/channels/format for all cases, so it's not clear what should happen.
Harsha, can you help here?
Best regards, Lukasz
czw., 21 maj 2020 o 19:17 Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com napisał(a):
On 5/21/20 12:08 PM, Łukasz Majczak wrote:
don't add a new dailink, this is not right.
Can you advise a better solution how to assign different fixup functions to mic and to speakers? I was looking at "dmic01" dailink in skl_nau88l25_max98357a.c as an example.
I am not sure I follow. the DMICs are handled on a shared SSP, so how would one set a different fixup? The word length have to be the same.
-----Original Message----- From: Łukasz Majczak lma@semihalf.com Sent: Monday, June 29, 2020 4:11 AM To: N, Harshapriya harshapriya.n@intel.com Cc: alsa-devel@alsa-project.org; Pierre-Louis Bossart <pierre- louis.bossart@linux.intel.com>; Jie Yang yang.jie@linux.intel.com; Radoslaw Biernacki rad@semihalf.com; Ross Zwisler zwisler@google.com; linux- kernel@vger.kernel.org; Liam Girdwood liam.r.girdwood@linux.intel.com; Bob Brandt brndt@google.com; Marcin Wojtas mw@semihalf.com; Alex Levin levinale@chromium.org Subject: Re: [PATCH v3] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function
Hi Harsha,
We would like to continue the work on this, could you please suggest the correct approach.
Best regards, Lukasz
czw., 21 maj 2020 o 20:10 Pierre-Louis Bossart <pierre- louis.bossart@linux.intel.com> napisał(a):
On 5/21/20 12:30 PM, Łukasz Majczak wrote:
Hi Pierre
If you will take a look at the original kabylake_ssp_fixup() you will see that it is checking whether the related FE is "Kbl Audio Port", "Kbl Audio Headset Playback", "Kbl Audio Capture Port" or "Kbl Audio DMIC cap" - then for the first 3 cases it sets min/max channels to 2 while for the "Kbl DMIC cap" it can be 2 or 4, that's is why I'm trying to split this, but maybe I'm missing here something.
I don't understand this code either.
I believe the intent is that for all SSP1-RT5663 usages, we should use
rate->min = rate->max = 48000; chan->min = chan->max = 2; snd_mask_none(fmt); snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S24_LE);That is pretty easy to move to a dedicated ssp1 fixup.
for SSP0, we have RT5514 for capture and max98927 for playback, but the existing code does not explicitly deal with rate/channels/format for all cases, so it's not clear what should happen.
Harsha, can you help here?
Apologies for missing the email I had to respond to.
SSP0 - has the speakers SSP1 - has headset and dmic For speakers and headsets its 48Khz, 2 ch and 24 bit and this setting is done based on the front-end dai For speakers, however support only 16 bit, so we set it back to 16 bit If the front end dai is dmic, then the channel is set to either 2 or 4 dmic_constraints. No other formats need to be set.
All the SSP1 usages do not have the same parameters (as dmic is on SSP1 and its different as given above) Most parameters are same for speakers and headset which are on different SSP. This is the reason we had a single fixup function.
Is there a reason why the fixup function needs to be split?
Best regards, Lukasz
czw., 21 maj 2020 o 19:17 Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com napisał(a):
On 5/21/20 12:08 PM, Łukasz Majczak wrote:
don't add a new dailink, this is not right.
Can you advise a better solution how to assign different fixup functions to mic and to speakers? I was looking at "dmic01" dailink in skl_nau88l25_max98357a.c as an example.
I am not sure I follow. the DMICs are handled on a shared SSP, so how would one set a different fixup? The word length have to be the same.
Hi Harsha,
I've put the reason for this change in the commit message. I had to split be_hw_params_fixup function for different codecs because current approach (made for kernel 4.4) used in kernel 5.4, leads to crash while trying to get snd_soc_dpcm with container_of() macro in kabylake_ssp_fixup(). The crash call path looks as below: soc_pcm_hw_params() snd_soc_dai_hw_params(codec_dai, substream, &codec_params); rtd->dai_link->be_hw_params_fixup(rtd, params) kabylake_ssp_fixup() In this case, codec_params is just a copy of an internal structure and is not embedded into struct snd_soc_dpcm thus we cannot use container_of() on it.
Best regards, Lukasz
pon., 29 cze 2020 o 18:51 N, Harshapriya harshapriya.n@intel.com napisał(a):
-----Original Message----- From: Łukasz Majczak lma@semihalf.com Sent: Monday, June 29, 2020 4:11 AM To: N, Harshapriya harshapriya.n@intel.com Cc: alsa-devel@alsa-project.org; Pierre-Louis Bossart <pierre- louis.bossart@linux.intel.com>; Jie Yang yang.jie@linux.intel.com; Radoslaw Biernacki rad@semihalf.com; Ross Zwisler zwisler@google.com; linux- kernel@vger.kernel.org; Liam Girdwood liam.r.girdwood@linux.intel.com; Bob Brandt brndt@google.com; Marcin Wojtas mw@semihalf.com; Alex Levin levinale@chromium.org Subject: Re: [PATCH v3] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function
Hi Harsha,
We would like to continue the work on this, could you please suggest the correct approach.
Best regards, Lukasz
czw., 21 maj 2020 o 20:10 Pierre-Louis Bossart <pierre- louis.bossart@linux.intel.com> napisał(a):
On 5/21/20 12:30 PM, Łukasz Majczak wrote:
Hi Pierre
If you will take a look at the original kabylake_ssp_fixup() you will see that it is checking whether the related FE is "Kbl Audio Port", "Kbl Audio Headset Playback", "Kbl Audio Capture Port" or "Kbl Audio DMIC cap" - then for the first 3 cases it sets min/max channels to 2 while for the "Kbl DMIC cap" it can be 2 or 4, that's is why I'm trying to split this, but maybe I'm missing here something.
I don't understand this code either.
I believe the intent is that for all SSP1-RT5663 usages, we should use
rate->min = rate->max = 48000; chan->min = chan->max = 2; snd_mask_none(fmt); snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S24_LE);That is pretty easy to move to a dedicated ssp1 fixup.
for SSP0, we have RT5514 for capture and max98927 for playback, but the existing code does not explicitly deal with rate/channels/format for all cases, so it's not clear what should happen.
Harsha, can you help here?
Apologies for missing the email I had to respond to.
SSP0 - has the speakers SSP1 - has headset and dmic For speakers and headsets its 48Khz, 2 ch and 24 bit and this setting is done based on the front-end dai For speakers, however support only 16 bit, so we set it back to 16 bit If the front end dai is dmic, then the channel is set to either 2 or 4 dmic_constraints. No other formats need to be set.
All the SSP1 usages do not have the same parameters (as dmic is on SSP1 and its different as given above) Most parameters are same for speakers and headset which are on different SSP. This is the reason we had a single fixup function.
Is there a reason why the fixup function needs to be split?
Best regards, Lukasz
czw., 21 maj 2020 o 19:17 Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com napisał(a):
On 5/21/20 12:08 PM, Łukasz Majczak wrote:
> > don't add a new dailink, this is not right. > Can you advise a better solution how to assign different fixup functions to mic and to speakers? I was looking at "dmic01" dailink in skl_nau88l25_max98357a.c as an example.
I am not sure I follow. the DMICs are handled on a shared SSP, so how would one set a different fixup? The word length have to be the same.
Hi Harsha,
I've put the reason for this change in the commit message. I had to split be_hw_params_fixup function for different codecs because current approach (made for kernel 4.4) used in kernel 5.4, leads to crash while trying to get snd_soc_dpcm with container_of() macro in kabylake_ssp_fixup(). The crash call path looks as below: soc_pcm_hw_params() snd_soc_dai_hw_params(codec_dai, substream, &codec_params); rtd->dai_link->be_hw_params_fixup(rtd, params) kabylake_ssp_fixup() In this case, codec_params is just a copy of an internal structure and is not embedded into struct snd_soc_dpcm thus we cannot use container_of() on it.
addressing it below to keep the context
Best regards, Lukasz
pon., 29 cze 2020 o 18:51 N, Harshapriya harshapriya.n@intel.com napisał(a):
-----Original Message----- From: Łukasz Majczak lma@semihalf.com Sent: Monday, June 29, 2020 4:11 AM To: N, Harshapriya harshapriya.n@intel.com Cc: alsa-devel@alsa-project.org; Pierre-Louis Bossart <pierre- louis.bossart@linux.intel.com>; Jie Yang yang.jie@linux.intel.com; Radoslaw Biernacki rad@semihalf.com; Ross Zwisler zwisler@google.com; linux- kernel@vger.kernel.org; Liam Girdwood liam.r.girdwood@linux.intel.com; Bob Brandt brndt@google.com; Marcin Wojtas mw@semihalf.com;
Alex
Levin levinale@chromium.org Subject: Re: [PATCH v3] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function
Hi Harsha,
We would like to continue the work on this, could you please suggest the correct approach.
Best regards, Lukasz
czw., 21 maj 2020 o 20:10 Pierre-Louis Bossart <pierre- louis.bossart@linux.intel.com> napisał(a):
On 5/21/20 12:30 PM, Łukasz Majczak wrote:
Hi Pierre
If you will take a look at the original kabylake_ssp_fixup() you will see that it is checking whether the related FE is "Kbl Audio Port", "Kbl Audio Headset Playback", "Kbl Audio Capture Port" or "Kbl Audio DMIC cap" - then for the first 3 cases it sets min/max channels to 2 while for the "Kbl DMIC cap" it can be 2 or 4, that's is why I'm trying to split this, but maybe I'm missing
here something.
I don't understand this code either.
I believe the intent is that for all SSP1-RT5663 usages, we should use
rate->min = rate->max = 48000; chan->min = chan->max = 2; snd_mask_none(fmt); snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S24_LE);That is pretty easy to move to a dedicated ssp1 fixup.
Agree. SSP1 can be moved to a separate function as it only hosts headset codec
for SSP0, we have RT5514 for capture and max98927 for playback, but the existing code does not explicitly deal with rate/channels/format for all cases, so it's not clear what should happen.
Harsha, can you help here?
Apologies for missing the email I had to respond to.
SSP0 - has the speakers SSP1 - has headset and dmic
Correction dmic is on SSP0
For speakers and headsets its 48Khz, 2 ch and 24 bit and this setting is done based on the front-end dai For speakers, however support only 16 bit, so we set it back to 16 bit If the front end dai is dmic, then the channel
is set to either 2 or 4 dmic_constraints. No other formats need to be set.
All the SSP1 usages do not have the same parameters (as dmic is on SSP1 and its different as given above) Most parameters are same for
speakers and headset which are on different SSP. This is the reason we had a single fixup function.
On SSP1, for dmic we need to fix the channels which is derived from dmic_num of the snd_soc_acpi_mach structure based on the number of dmic on the board. The channel is something that might be different from speakers. We might not want to constraint the dmic capture to always be 48Khz as well. Given this, there seems to me, 2 ways to set it: 1. Derive if the fixup is being called for dmic or speaker 2. Having a new dailink
If #2 is not preferred (going by Pierre's comments), can we use rtd->cpu_dai/codec_dai->name to figure out if its for dmic or speaker? I can test this and get back to you.
Is there a reason why the fixup function needs to be split?
Best regards, Lukasz
czw., 21 maj 2020 o 19:17 Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com napisał(a):
On 5/21/20 12:08 PM, Łukasz Majczak wrote: >> >> don't add a new dailink, this is not right. >> > Can you advise a better solution how to assign different fixup > functions to mic and to speakers? I was looking at "dmic01" > dailink in skl_nau88l25_max98357a.c as an example.
I am not sure I follow. the DMICs are handled on a shared SSP, so how would one set a different fixup? The word length have to be the
same.
For speakers and headsets its 48Khz, 2 ch and 24 bit and this setting is done based on the front-end dai For speakers, however support only 16 bit, so we set it back to 16 bit If the front end dai is dmic, then the channel
is set to either 2 or 4 dmic_constraints. No other formats need to be set.
All the SSP1 usages do not have the same parameters (as dmic is on SSP1 and its different as given above) Most parameters are same for
speakers and headset which are on different SSP. This is the reason we had a single fixup function.
On SSP1, for dmic we need to fix the channels which is derived from dmic_num of the snd_soc_acpi_mach structure based on the number of dmic on the board. The channel is something that might be different from speakers. We might not want to constraint the dmic capture to always be 48Khz as well. Given this, there seems to me, 2 ways to set it:
- Derive if the fixup is being called for dmic or speaker 2. Having a new dailink
If #2 is not preferred (going by Pierre's comments), can we use rtd-
cpu_dai/codec_dai->name to figure out if its for dmic or speaker?
I can test this and get back to you.
Tested and the following is something we can use without creating a new dailink. struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0); if (!strcmp(codec_dai->name, KBL_REALTEK_DMIC_CODEC_DAI)) { if (params_channels(params) == 2 || DMIC_CH(dmic_constraints) == 2) channels->min = channels->max = 2; else channels->min = channels->max = 4; } else { rate->min = rate->max = 48000; channels->min = channels->max = 2; snd_mask_none(fmt); snd_mask_set_format(fmt, pcm_fmt); }
Pierre, thoughts?
On 7/1/20 11:37 AM, N, Harshapriya wrote:
For speakers and headsets its 48Khz, 2 ch and 24 bit and this setting is done based on the front-end dai For speakers, however support only 16 bit, so we set it back to 16 bit If the front end dai is dmic, then the channel
is set to either 2 or 4 dmic_constraints. No other formats need to be set.
All the SSP1 usages do not have the same parameters (as dmic is on SSP1 and its different as given above) Most parameters are same for
speakers and headset which are on different SSP. This is the reason we had a single fixup function.
On SSP1, for dmic we need to fix the channels which is derived from dmic_num of the snd_soc_acpi_mach structure based on the number of dmic on the board. The channel is something that might be different from speakers. We might not want to constraint the dmic capture to always be 48Khz as well. Given this, there seems to me, 2 ways to set it:
- Derive if the fixup is being called for dmic or speaker 2. Having a new dailink
If #2 is not preferred (going by Pierre's comments), can we use rtd-
cpu_dai/codec_dai->name to figure out if its for dmic or speaker?
I can test this and get back to you.
Tested and the following is something we can use without creating a new dailink. struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0); if (!strcmp(codec_dai->name, KBL_REALTEK_DMIC_CODEC_DAI)) { if (params_channels(params) == 2 || DMIC_CH(dmic_constraints) == 2) channels->min = channels->max = 2; else channels->min = channels->max = 4; } else { rate->min = rate->max = 48000; channels->min = channels->max = 2; snd_mask_none(fmt); snd_mask_set_format(fmt, pcm_fmt); }
Pierre, thoughts?
thanks Harsha, that looks like what I had in mind, but my earlier question was why we deal with the rates and formats only in the last case?
For speakers and headsets its 48Khz, 2 ch and 24 bit and this setting is done based on the front-end dai For speakers, however support only 16 bit, so we set it back to 16 bit If the front end dai is dmic, then the channel
is set to either 2 or 4 dmic_constraints. No other formats need to be set.
All the SSP1 usages do not have the same parameters (as dmic is on SSP1 and its different as given above) Most parameters are same for
speakers and headset which are on different SSP. This is the reason we had a single fixup function.
On SSP1, for dmic we need to fix the channels which is derived from dmic_num of the snd_soc_acpi_mach structure based on the number of dmic on the board. The channel is something that might be different from speakers. We might not want to constraint the dmic capture to always be 48Khz as
well.
Given this, there seems to me, 2 ways to set it:
- Derive if the fixup is being called for dmic or speaker 2. Having
a new dailink
If #2 is not preferred (going by Pierre's comments), can we use rtd-
cpu_dai/codec_dai->name to figure out if its for dmic or speaker?
I can test this and get back to you.
Tested and the following is something we can use without creating a new
dailink.
struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);if (!strcmp(codec_dai->name, KBL_REALTEK_DMIC_CODEC_DAI)) { if (params_channels(params) == 2 ||
DMIC_CH(dmic_constraints) == 2)
channels->min = channels->max = 2; else channels->min = channels->max = 4;} else { rate->min = rate->max = 48000; channels->min = channels->max = 2; snd_mask_none(fmt); snd_mask_set_format(fmt, pcm_fmt); }
Pierre, thoughts?
thanks Harsha, that looks like what I had in mind, but my earlier question was why we deal with the rates and formats only in the last case?
The speaker codec supported only 16 bit. (Vendor mentioned) For playback on this platform, only 48Khz was used.
Tested and the following is something we can use without creating a new
dailink.
struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);if (!strcmp(codec_dai->name, KBL_REALTEK_DMIC_CODEC_DAI)) { if (params_channels(params) == 2 ||
DMIC_CH(dmic_constraints) == 2)
channels->min = channels->max = 2; else channels->min = channels->max = 4;} else { rate->min = rate->max = 48000; channels->min = channels->max = 2; snd_mask_none(fmt); snd_mask_set_format(fmt, pcm_fmt); }
Pierre, thoughts?
thanks Harsha, that looks like what I had in mind, but my earlier question was why we deal with the rates and formats only in the last case?
The speaker codec supported only 16 bit. (Vendor mentioned) For playback on this platform, only 48Khz was used.
ok then, as long as Harsha and Lukasz are aligned I'm fine. Thanks!
Hi,
I am also ok with Harsha patch. I have checked it on my Eve and it looks ok.
Best regards,
Lukasz
śr., 1 lip 2020 o 19:08 Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com napisał(a):
Tested and the following is something we can use without creating a new
dailink.
struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0); if (!strcmp(codec_dai->name, KBL_REALTEK_DMIC_CODEC_DAI)) { if (params_channels(params) == 2 ||DMIC_CH(dmic_constraints) == 2)
channels->min = channels->max = 2; else channels->min = channels->max = 4; } else { rate->min = rate->max = 48000; channels->min = channels->max = 2; snd_mask_none(fmt); snd_mask_set_format(fmt, pcm_fmt); }Pierre, thoughts?
thanks Harsha, that looks like what I had in mind, but my earlier question was why we deal with the rates and formats only in the last case?
The speaker codec supported only 16 bit. (Vendor mentioned) For playback on this platform, only 48Khz was used.
ok then, as long as Harsha and Lukasz are aligned I'm fine. Thanks!
Fix kabylake_ssp_fixup function to distinguish codecs DAIs by names, as current approach, leads to crash while trying to get snd_soc_dpcm with container_of() macro in kabylake_ssp_fixup(). The crash call path looks as below: soc_pcm_hw_params() snd_soc_dai_hw_params(codec_dai, substream, &codec_params); rtd->dai_link->be_hw_params_fixup(rtd, params) kabylake_ssp_fixup() In this case, codec_params is just a copy of an internal structure and is not embedded into struct snd_soc_dpcm thus we cannot use container_of() on it.
v1 -> v2: - Extract dmic from SSP0 as every BE should have own fixup function. v2 -> v3: - Restore naming in the dapm route table to not confuse with other drivers - Fixed indentations v3 -> v4: - Updated code and commit description according to solution proposed by Harsha
Signed-off-by: Lukasz Majczak lma@semihalf.com Signed-off-by: Harsha Priya harshapriya.n@intel.com --- .../intel/boards/kbl_rt5663_rt5514_max98927.c | 28 ++++++++----------- 1 file changed, 12 insertions(+), 16 deletions(-)
diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c index b34cf6cf11395..df454de40739a 100644 --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c @@ -333,36 +333,32 @@ static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime *rtd, { struct snd_interval *rate = hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE); - struct snd_interval *chan = hw_param_interval(params, + struct snd_interval *channels = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS); struct snd_mask *fmt = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT); - struct snd_soc_dpcm *dpcm = container_of( - params, struct snd_soc_dpcm, hw_params); - struct snd_soc_dai_link *fe_dai_link = dpcm->fe->dai_link; - struct snd_soc_dai_link *be_dai_link = dpcm->be->dai_link; + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
/* * The ADSP will convert the FE rate to 48k, stereo, 24 bit */ - if (!strcmp(fe_dai_link->name, "Kbl Audio Port") || - !strcmp(fe_dai_link->name, "Kbl Audio Headset Playback") || - !strcmp(fe_dai_link->name, "Kbl Audio Capture Port")) { + + if (!strcmp(codec_dai->name, KBL_REALTEK_DMIC_CODEC_DAI)) { + if (params_channels(params) == 2 || + DMIC_CH(dmic_constraints) == 2) + channels->min = channels->max = 2; + else + channels->min = channels->max = 4; + } else { rate->min = rate->max = 48000; - chan->min = chan->max = 2; + channels->min = channels->max = 2; snd_mask_none(fmt); snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S24_LE); - } else if (!strcmp(fe_dai_link->name, "Kbl Audio DMIC cap")) { - if (params_channels(params) == 2 || - DMIC_CH(dmic_constraints) == 2) - chan->min = chan->max = 2; - else - chan->min = chan->max = 4; } /* * The speaker on the SSP0 supports S16_LE and not S24_LE. * thus changing the mask here */ - if (!strcmp(be_dai_link->name, "SSP0-Codec")) + if (!strcmp(codec_dai->name, KBL_MAXIM_CODEC_DAI)) snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S16_LE);
return 0;
On 7/3/20 7:16 AM, Lukasz Majczak wrote:
Fix kabylake_ssp_fixup function to distinguish codecs DAIs by names, as current approach, leads to crash while trying to get snd_soc_dpcm with container_of() macro in kabylake_ssp_fixup(). The crash call path looks as below: soc_pcm_hw_params() snd_soc_dai_hw_params(codec_dai, substream, &codec_params); rtd->dai_link->be_hw_params_fixup(rtd, params) kabylake_ssp_fixup() In this case, codec_params is just a copy of an internal structure and is not embedded into struct snd_soc_dpcm thus we cannot use container_of() on it.
v1 -> v2:
- Extract dmic from SSP0 as every BE should have own fixup function.
v2 -> v3:
- Restore naming in the dapm route table to not confuse with other
drivers
- Fixed indentations
v3 -> v4:
- Updated code and commit description according to
solution proposed by Harsha
Looks good Lukasz but you need to move the information on changes below the --- marker (~4 lines below).
Signed-off-by: Lukasz Majczak lma@semihalf.com Signed-off-by: Harsha Priya harshapriya.n@intel.com
.../intel/boards/kbl_rt5663_rt5514_max98927.c | 28 ++++++++----------- 1 file changed, 12 insertions(+), 16 deletions(-)
diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c index b34cf6cf11395..df454de40739a 100644 --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c @@ -333,36 +333,32 @@ static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime *rtd, { struct snd_interval *rate = hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE);
- struct snd_interval *chan = hw_param_interval(params,
- struct snd_interval *channels = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS); struct snd_mask *fmt = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT);
- struct snd_soc_dpcm *dpcm = container_of(
params, struct snd_soc_dpcm, hw_params);- struct snd_soc_dai_link *fe_dai_link = dpcm->fe->dai_link;
- struct snd_soc_dai_link *be_dai_link = dpcm->be->dai_link;
struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
/*
- The ADSP will convert the FE rate to 48k, stereo, 24 bit
*/
- if (!strcmp(fe_dai_link->name, "Kbl Audio Port") ||
!strcmp(fe_dai_link->name, "Kbl Audio Headset Playback") ||!strcmp(fe_dai_link->name, "Kbl Audio Capture Port")) {
- if (!strcmp(codec_dai->name, KBL_REALTEK_DMIC_CODEC_DAI)) {
if (params_channels(params) == 2 ||DMIC_CH(dmic_constraints) == 2)channels->min = channels->max = 2;elsechannels->min = channels->max = 4;- } else { rate->min = rate->max = 48000;
chan->min = chan->max = 2;
channels->min = channels->max = 2;
- } else if (!strcmp(fe_dai_link->name, "Kbl Audio DMIC cap")) {
if (params_channels(params) == 2 ||DMIC_CH(dmic_constraints) == 2)chan->min = chan->max = 2;elsechan->min = chan->max = 4;*/
- The speaker on the SSP0 supports S16_LE and not S24_LE.
- thus changing the mask here
- if (!strcmp(be_dai_link->name, "SSP0-Codec"))
if (!strcmp(codec_dai->name, KBL_MAXIM_CODEC_DAI)) snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S16_LE);
return 0;
Fix kabylake_ssp_fixup function to distinguish codecs DAIs by names, as current approach, leads to crash while trying to get snd_soc_dpcm with container_of() macro in kabylake_ssp_fixup(). The crash call path looks as below: soc_pcm_hw_params() snd_soc_dai_hw_params(codec_dai, substream, &codec_params); rtd->dai_link->be_hw_params_fixup(rtd, params) kabylake_ssp_fixup() In this case, codec_params is just a copy of an internal structure and is not embedded into struct snd_soc_dpcm thus we cannot use container_of() on it.
Signed-off-by: Lukasz Majczak lma@semihalf.com Signed-off-by: Harsha Priya harshapriya.n@intel.com --- v1 -> v2: - Extract dmic from SSP0 as every BE should have own fixup function. v2 -> v3: - Restore naming in the dapm route table to not confuse with other drivers - Fixed indentations v3 -> v4: - Updated code and commit description according to solution proposed by Harsha --- .../intel/boards/kbl_rt5663_rt5514_max98927.c | 28 ++++++++----------- 1 file changed, 12 insertions(+), 16 deletions(-)
diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c index b34cf6cf11395..df454de40739a 100644 --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c @@ -333,36 +333,32 @@ static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime *rtd, { struct snd_interval *rate = hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE); - struct snd_interval *chan = hw_param_interval(params, + struct snd_interval *channels = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS); struct snd_mask *fmt = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT); - struct snd_soc_dpcm *dpcm = container_of( - params, struct snd_soc_dpcm, hw_params); - struct snd_soc_dai_link *fe_dai_link = dpcm->fe->dai_link; - struct snd_soc_dai_link *be_dai_link = dpcm->be->dai_link; + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
/* * The ADSP will convert the FE rate to 48k, stereo, 24 bit */ - if (!strcmp(fe_dai_link->name, "Kbl Audio Port") || - !strcmp(fe_dai_link->name, "Kbl Audio Headset Playback") || - !strcmp(fe_dai_link->name, "Kbl Audio Capture Port")) { + + if (!strcmp(codec_dai->name, KBL_REALTEK_DMIC_CODEC_DAI)) { + if (params_channels(params) == 2 || + DMIC_CH(dmic_constraints) == 2) + channels->min = channels->max = 2; + else + channels->min = channels->max = 4; + } else { rate->min = rate->max = 48000; - chan->min = chan->max = 2; + channels->min = channels->max = 2; snd_mask_none(fmt); snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S24_LE); - } else if (!strcmp(fe_dai_link->name, "Kbl Audio DMIC cap")) { - if (params_channels(params) == 2 || - DMIC_CH(dmic_constraints) == 2) - chan->min = chan->max = 2; - else - chan->min = chan->max = 4; } /* * The speaker on the SSP0 supports S16_LE and not S24_LE. * thus changing the mask here */ - if (!strcmp(be_dai_link->name, "SSP0-Codec")) + if (!strcmp(codec_dai->name, KBL_MAXIM_CODEC_DAI)) snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S16_LE);
return 0;
On 7/7/20 3:34 AM, Lukasz Majczak wrote:
Fix kabylake_ssp_fixup function to distinguish codecs DAIs by names, as current approach, leads to crash while trying to get snd_soc_dpcm with container_of() macro in kabylake_ssp_fixup(). The crash call path looks as below: soc_pcm_hw_params() snd_soc_dai_hw_params(codec_dai, substream, &codec_params); rtd->dai_link->be_hw_params_fixup(rtd, params) kabylake_ssp_fixup() In this case, codec_params is just a copy of an internal structure and is not embedded into struct snd_soc_dpcm thus we cannot use container_of() on it.
Signed-off-by: Lukasz Majczak lma@semihalf.com Signed-off-by: Harsha Priya harshapriya.n@intel.com
Acked-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
v1 -> v2:
- Extract dmic from SSP0 as every BE should have own fixup function.
v2 -> v3:
- Restore naming in the dapm route table to not confuse with other
drivers
- Fixed indentations
v3 -> v4:
- Updated code and commit description according to
solution proposed by Harsha
.../intel/boards/kbl_rt5663_rt5514_max98927.c | 28 ++++++++----------- 1 file changed, 12 insertions(+), 16 deletions(-)
diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c index b34cf6cf11395..df454de40739a 100644 --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c @@ -333,36 +333,32 @@ static int kabylake_ssp_fixup(struct snd_soc_pcm_runtime *rtd, { struct snd_interval *rate = hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE);
- struct snd_interval *chan = hw_param_interval(params,
- struct snd_interval *channels = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS); struct snd_mask *fmt = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT);
- struct snd_soc_dpcm *dpcm = container_of(
params, struct snd_soc_dpcm, hw_params);- struct snd_soc_dai_link *fe_dai_link = dpcm->fe->dai_link;
- struct snd_soc_dai_link *be_dai_link = dpcm->be->dai_link;
struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
/*
- The ADSP will convert the FE rate to 48k, stereo, 24 bit
*/
- if (!strcmp(fe_dai_link->name, "Kbl Audio Port") ||
!strcmp(fe_dai_link->name, "Kbl Audio Headset Playback") ||!strcmp(fe_dai_link->name, "Kbl Audio Capture Port")) {
- if (!strcmp(codec_dai->name, KBL_REALTEK_DMIC_CODEC_DAI)) {
if (params_channels(params) == 2 ||DMIC_CH(dmic_constraints) == 2)channels->min = channels->max = 2;elsechannels->min = channels->max = 4;- } else { rate->min = rate->max = 48000;
chan->min = chan->max = 2;
channels->min = channels->max = 2;
- } else if (!strcmp(fe_dai_link->name, "Kbl Audio DMIC cap")) {
if (params_channels(params) == 2 ||DMIC_CH(dmic_constraints) == 2)chan->min = chan->max = 2;elsechan->min = chan->max = 4;*/
- The speaker on the SSP0 supports S16_LE and not S24_LE.
- thus changing the mask here
- if (!strcmp(be_dai_link->name, "SSP0-Codec"))
if (!strcmp(codec_dai->name, KBL_MAXIM_CODEC_DAI)) snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S16_LE);
return 0;
On 7/7/20 3:34 AM, Lukasz Majczak wrote:
Fix kabylake_ssp_fixup function to distinguish codecs DAIs by names, as current approach, leads to crash while trying to get snd_soc_dpcm with container_of() macro in kabylake_ssp_fixup(). The crash call path looks as below: soc_pcm_hw_params() snd_soc_dai_hw_params(codec_dai, substream, &codec_params); rtd->dai_link->be_hw_params_fixup(rtd, params) kabylake_ssp_fixup() In this case, codec_params is just a copy of an internal structure and is not embedded into struct snd_soc_dpcm thus we cannot use container_of() on it.
Signed-off-by: Lukasz Majczak lma@semihalf.com Signed-off-by: Harsha Priya harshapriya.n@intel.com
Acked-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
This fix is not complete as the asoc_rtd_to_codec(rtd,0) returns the codec instance at 0th location. Instead we need to use the dpcm in the snd_pcm_runtime structure. I will need another version of this patch.
v1 -> v2:
- Extract dmic from SSP0 as every BE should have own fixup function.
v2 -> v3:
- Restore naming in the dapm route table to not confuse with other
drivers
- Fixed indentations
v3 -> v4:
- Updated code and commit description according to solution proposed
by Harsha
.../intel/boards/kbl_rt5663_rt5514_max98927.c | 28 ++++++++----------- 1 file changed, 12 insertions(+), 16 deletions(-)
diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c index b34cf6cf11395..df454de40739a 100644 --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c @@ -333,36 +333,32 @@ static int kabylake_ssp_fixup(struct
snd_soc_pcm_runtime *rtd,
{ struct snd_interval *rate = hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE);
- struct snd_interval *chan = hw_param_interval(params,
- struct snd_interval *channels = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS); struct snd_mask *fmt = hw_param_mask(params,
SNDRV_PCM_HW_PARAM_FORMAT);
- struct snd_soc_dpcm *dpcm = container_of(
params, struct snd_soc_dpcm, hw_params);- struct snd_soc_dai_link *fe_dai_link = dpcm->fe->dai_link;
- struct snd_soc_dai_link *be_dai_link = dpcm->be->dai_link;
struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
/*
- The ADSP will convert the FE rate to 48k, stereo, 24 bit
*/
- if (!strcmp(fe_dai_link->name, "Kbl Audio Port") ||
!strcmp(fe_dai_link->name, "Kbl Audio Headset Playback") ||!strcmp(fe_dai_link->name, "Kbl Audio Capture Port")) {
- if (!strcmp(codec_dai->name, KBL_REALTEK_DMIC_CODEC_DAI)) {
if (params_channels(params) == 2 ||DMIC_CH(dmic_constraints) == 2)channels->min = channels->max = 2;elsechannels->min = channels->max = 4;- } else { rate->min = rate->max = 48000;
chan->min = chan->max = 2;
channels->min = channels->max = 2;
- } else if (!strcmp(fe_dai_link->name, "Kbl Audio DMIC cap")) {
if (params_channels(params) == 2 ||DMIC_CH(dmic_constraints) == 2)chan->min = chan->max = 2;elsechan->min = chan->max = 4;*/
- The speaker on the SSP0 supports S16_LE and not S24_LE.
- thus changing the mask here
- if (!strcmp(be_dai_link->name, "SSP0-Codec"))
if (!strcmp(codec_dai->name, KBL_MAXIM_CODEC_DAI)) snd_mask_set_format(fmt, SNDRV_PCM_FORMAT_S16_LE);
return 0;
participants (4)
-
Lukasz Majczak -
N, Harshapriya -
Pierre-Louis Bossart -
Łukasz Majczak