linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function
@ 2020-05-13 19:20 Lukasz Majczak
  2020-05-14 12:47 ` Pierre-Louis Bossart
  0 siblings, 1 reply; 8+ messages in thread
From: Lukasz Majczak @ 2020-05-13 19:20 UTC (permalink / raw)
  To: Pierre-Louis Bossart, Liam Girdwood, Jie Yang
  Cc: Bob Brandt, Alex Levin, Ross Zwisler, Marcin Wojtas,
	Radoslaw Biernacki, Lukasz Majczak, alsa-devel, linux-kernel

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.

Signed-off-by: Lukasz Majczak <lma@semihalf.com>
---
 .../intel/boards/kbl_rt5663_rt5514_max98927.c | 68 +++++++++++--------
 1 file changed, 39 insertions(+), 29 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..2e0ae724122c 100644
--- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
+++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
@@ -328,46 +328,55 @@ 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_cap_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;
+}
 static int kabylake_rt5663_hw_params(struct snd_pcm_substream *substream,
 	struct snd_pcm_hw_params *params)
 {
@@ -582,6 +591,7 @@ static struct snd_soc_dai_link kabylake_dais[] = {
 		.dpcm_capture = 1,
 		.nonatomic = 1,
 		.dynamic = 1,
+		.be_hw_params_fixup = kabylake_dmic_cap_fixup,
 		.ops = &kabylake_dmic_ops,
 		SND_SOC_DAILINK_REG(dmic, dummy, platform),
 	},
@@ -618,7 +628,7 @@ 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,
@@ -632,7 +642,7 @@ 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,
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function
  2020-05-13 19:20 [PATCH] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function Lukasz Majczak
@ 2020-05-14 12:47 ` Pierre-Louis Bossart
  2020-05-14 13:56   ` Guenter Roeck
  0 siblings, 1 reply; 8+ messages in thread
From: Pierre-Louis Bossart @ 2020-05-14 12:47 UTC (permalink / raw)
  To: Lukasz Majczak, Liam Girdwood, Jie Yang
  Cc: alsa-devel, Radoslaw Biernacki, Ross Zwisler, linux-kernel,
	Bob Brandt, Marcin Wojtas, Alex Levin, Guenter Roeck



On 5/13/20 2:20 PM, 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.

This looks like a nice/welcome change, we've had these unexplained 
crashes on KBL since 4.19 (reported by Guenter and me). I thought they 
were topology related.

If indeed this fixes the issue, it might be worth applying in to all 
stable releases?

Since we have the same code structure for the other kbl drivers, would 
it also make sense to apply the same fixes there:

kbl_da7219_max98927.c:  struct snd_soc_dpcm *dpcm = container_of(
kbl_rt5663_max98927.c:  struct snd_soc_dpcm *dpcm = container_of(


> 
> Signed-off-by: Lukasz Majczak <lma@semihalf.com>
> ---
>   .../intel/boards/kbl_rt5663_rt5514_max98927.c | 68 +++++++++++--------
>   1 file changed, 39 insertions(+), 29 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..2e0ae724122c 100644
> --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> @@ -328,46 +328,55 @@ 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_cap_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;
> +}
>   static int kabylake_rt5663_hw_params(struct snd_pcm_substream *substream,
>   	struct snd_pcm_hw_params *params)
>   {
> @@ -582,6 +591,7 @@ static struct snd_soc_dai_link kabylake_dais[] = {
>   		.dpcm_capture = 1,
>   		.nonatomic = 1,
>   		.dynamic = 1,
> +		.be_hw_params_fixup = kabylake_dmic_cap_fixup,
>   		.ops = &kabylake_dmic_ops,
>   		SND_SOC_DAILINK_REG(dmic, dummy, platform),
>   	},
> @@ -618,7 +628,7 @@ 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,
> @@ -632,7 +642,7 @@ 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,
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function
  2020-05-14 12:47 ` Pierre-Louis Bossart
@ 2020-05-14 13:56   ` Guenter Roeck
  0 siblings, 0 replies; 8+ messages in thread
From: Guenter Roeck @ 2020-05-14 13:56 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: Lukasz Majczak, Liam Girdwood, Jie Yang, alsa-devel,
	Radoslaw Biernacki, Ross Zwisler, linux-kernel, Bob Brandt,
	Marcin Wojtas, Alex Levin, Guenter Roeck

On Thu, May 14, 2020 at 5:47 AM Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
>
>
>
> On 5/13/20 2:20 PM, 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.
>
> This looks like a nice/welcome change, we've had these unexplained
> crashes on KBL since 4.19 (reported by Guenter and me). I thought they
> were topology related.
>

Not entirely unexplained. See
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1600868,
which fixes the problem for all three affected drivers.

Someone had told me, though, that the problem is no longer seen. Guess
that was wrong.

Guenter

> If indeed this fixes the issue, it might be worth applying in to all
> stable releases?
>
> Since we have the same code structure for the other kbl drivers, would
> it also make sense to apply the same fixes there:
>
> kbl_da7219_max98927.c:  struct snd_soc_dpcm *dpcm = container_of(
> kbl_rt5663_max98927.c:  struct snd_soc_dpcm *dpcm = container_of(
>
>
> >
> > Signed-off-by: Lukasz Majczak <lma@semihalf.com>
> > ---
> >   .../intel/boards/kbl_rt5663_rt5514_max98927.c | 68 +++++++++++--------
> >   1 file changed, 39 insertions(+), 29 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..2e0ae724122c 100644
> > --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> > +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> > @@ -328,46 +328,55 @@ 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_cap_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;
> > +}
> >   static int kabylake_rt5663_hw_params(struct snd_pcm_substream *substream,
> >       struct snd_pcm_hw_params *params)
> >   {
> > @@ -582,6 +591,7 @@ static struct snd_soc_dai_link kabylake_dais[] = {
> >               .dpcm_capture = 1,
> >               .nonatomic = 1,
> >               .dynamic = 1,
> > +             .be_hw_params_fixup = kabylake_dmic_cap_fixup,
> >               .ops = &kabylake_dmic_ops,
> >               SND_SOC_DAILINK_REG(dmic, dummy, platform),
> >       },
> > @@ -618,7 +628,7 @@ 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,
> > @@ -632,7 +642,7 @@ 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,
> >

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function
  2020-05-21 14:43     ` Łukasz Majczak
@ 2020-05-21 15:14       ` Pierre-Louis Bossart
  0 siblings, 0 replies; 8+ messages in thread
From: Pierre-Louis Bossart @ 2020-05-21 15:14 UTC (permalink / raw)
  To: Łukasz Majczak
  Cc: Liam Girdwood, Jie Yang, alsa-devel, Radoslaw Biernacki,
	Ross Zwisler, linux-kernel, Bob Brandt, Marcin Wojtas,
	Alex Levin



On 5/21/20 9:43 AM, Łukasz Majczak wrote:
> Pierre
> 
> Sorry, I didn't get it for the first time, you are suggesting to not
> use DMIC01 as it is used by PCH-attached
> dmics by other drivers.

Yes, it's confusing and it also took me time during the initial reviews 
of this driver to figure out it's different from the others.

My suggestion was indeed to avoid using DMIC01 but instead keep the 
traditional SSP0-based RX routes. You can still use the dmic fixup to 
constrain the word size/ channel count, but this should not add 
confusion on how the physical connections are handled.

> 
> Best regards,
> Lukasz
> 
> czw., 21 maj 2020 o 16:36 Łukasz Majczak <lma@semihalf.com> napisał(a):
>>
>> Yes, my bad
>> it should be:
>> +       { "codec1_in", NULL, "DMIC01 Rx" },
>> +       { "DMIC01 Rx", NULL, "AIF1 Capture" },
>>
>> The whole idea of taking the mic for SSP0 definition is that each BE
>> should have its own fixup. Before there was one fixup function, which
>> checked inside which BE is connected to which FE and applied the
>> proper fix, it was using the fact that "params" were part of
>> snd_soc_dpcm. That has changed and now params are "orphaned" so each
>> BE has to apply a specific fixup for itself.
>>
>> Best regards,
>> Lukasz
>>
>>
>> czw., 21 maj 2020 o 16:25 Pierre-Louis Bossart
>> <pierre-louis.bossart@linux.intel.com> napisał(a):
>>>
>>>
>>>
>>> On 5/21/20 8:47 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.
>>>>
>>>> Signed-off-by: Lukasz Majczak <lma@semihalf.com>
>>>> ---
>>>>    .../intel/boards/kbl_rt5663_rt5514_max98927.c | 130 ++++++++++++------
>>>>    1 file changed, 85 insertions(+), 45 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..12a9983979e0 100644
>>>> --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
>>>> +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
>>>> @@ -171,8 +171,8 @@ static const struct snd_soc_dapm_route kabylake_map[] = {
>>>>        { "hs_in", NULL, "ssp1 Rx" },
>>>>        { "ssp1 Rx", NULL, "AIF Capture" },
>>>>
>>>> -     { "codec1_in", NULL, "ssp0 Rx" },
>>>> -     { "ssp0 Rx", NULL, "AIF1 Capture" },
>>>> +     { "codec1_in", NULL, "DMIC01 Rx" },
>>>> +     { "DMIC01 Rx", NULL, "AIF1 Capture" },
>>>
>>> This doesn't seem right. This board uses DMICs attached to the codec so
>>> we should not make references to routes that are used for PCH-attached
>>> dmics in all other machine drivers:
>>>
>>>          { "dmic01_hifi", NULL, "DMIC01 Rx" },
>>>          { "DMIC01 Rx", NULL, "DMIC AIF" },
>>>
>>>>
>>>>        /* IV feedback path */
>>>>        { "codec0_fb_in", NULL, "ssp0 Rx"},
>>>> @@ -328,42 +328,52 @@ 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")) {
>>>> +
>>>> +     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)
>>>> -                     chan->min = chan->max = 2;
>>>> +                     channels->min = channels->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);
>>>> +                     channels->min = channels->max = 4;
>>>>
>>>>        return 0;
>>>>    }
>>>> @@ -400,20 +410,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 +429,37 @@ 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 +530,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 +646,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 +659,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
>>>>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function
  2020-05-21 14:36   ` Łukasz Majczak
@ 2020-05-21 14:43     ` Łukasz Majczak
  2020-05-21 15:14       ` Pierre-Louis Bossart
  0 siblings, 1 reply; 8+ messages in thread
From: Łukasz Majczak @ 2020-05-21 14:43 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: Liam Girdwood, Jie Yang, alsa-devel, Radoslaw Biernacki,
	Ross Zwisler, linux-kernel, Bob Brandt, Marcin Wojtas,
	Alex Levin

Pierre

Sorry, I didn't get it for the first time, you are suggesting to not
use DMIC01 as it is used by PCH-attached
dmics by other drivers.

Best regards,
Lukasz

czw., 21 maj 2020 o 16:36 Łukasz Majczak <lma@semihalf.com> napisał(a):
>
> Yes, my bad
> it should be:
> +       { "codec1_in", NULL, "DMIC01 Rx" },
> +       { "DMIC01 Rx", NULL, "AIF1 Capture" },
>
> The whole idea of taking the mic for SSP0 definition is that each BE
> should have its own fixup. Before there was one fixup function, which
> checked inside which BE is connected to which FE and applied the
> proper fix, it was using the fact that "params" were part of
> snd_soc_dpcm. That has changed and now params are "orphaned" so each
> BE has to apply a specific fixup for itself.
>
> Best regards,
> Lukasz
>
>
> czw., 21 maj 2020 o 16:25 Pierre-Louis Bossart
> <pierre-louis.bossart@linux.intel.com> napisał(a):
> >
> >
> >
> > On 5/21/20 8:47 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.
> > >
> > > Signed-off-by: Lukasz Majczak <lma@semihalf.com>
> > > ---
> > >   .../intel/boards/kbl_rt5663_rt5514_max98927.c | 130 ++++++++++++------
> > >   1 file changed, 85 insertions(+), 45 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..12a9983979e0 100644
> > > --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> > > +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> > > @@ -171,8 +171,8 @@ static const struct snd_soc_dapm_route kabylake_map[] = {
> > >       { "hs_in", NULL, "ssp1 Rx" },
> > >       { "ssp1 Rx", NULL, "AIF Capture" },
> > >
> > > -     { "codec1_in", NULL, "ssp0 Rx" },
> > > -     { "ssp0 Rx", NULL, "AIF1 Capture" },
> > > +     { "codec1_in", NULL, "DMIC01 Rx" },
> > > +     { "DMIC01 Rx", NULL, "AIF1 Capture" },
> >
> > This doesn't seem right. This board uses DMICs attached to the codec so
> > we should not make references to routes that are used for PCH-attached
> > dmics in all other machine drivers:
> >
> >         { "dmic01_hifi", NULL, "DMIC01 Rx" },
> >         { "DMIC01 Rx", NULL, "DMIC AIF" },
> >
> > >
> > >       /* IV feedback path */
> > >       { "codec0_fb_in", NULL, "ssp0 Rx"},
> > > @@ -328,42 +328,52 @@ 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")) {
> > > +
> > > +     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)
> > > -                     chan->min = chan->max = 2;
> > > +                     channels->min = channels->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);
> > > +                     channels->min = channels->max = 4;
> > >
> > >       return 0;
> > >   }
> > > @@ -400,20 +410,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 +429,37 @@ 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 +530,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 +646,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 +659,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
> > >

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function
  2020-05-21 14:25 ` Pierre-Louis Bossart
@ 2020-05-21 14:36   ` Łukasz Majczak
  2020-05-21 14:43     ` Łukasz Majczak
  0 siblings, 1 reply; 8+ messages in thread
From: Łukasz Majczak @ 2020-05-21 14:36 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: Liam Girdwood, Jie Yang, alsa-devel, Radoslaw Biernacki,
	Ross Zwisler, linux-kernel, Bob Brandt, Marcin Wojtas,
	Alex Levin

Yes, my bad
it should be:
+       { "codec1_in", NULL, "DMIC01 Rx" },
+       { "DMIC01 Rx", NULL, "AIF1 Capture" },

The whole idea of taking the mic for SSP0 definition is that each BE
should have its own fixup. Before there was one fixup function, which
checked inside which BE is connected to which FE and applied the
proper fix, it was using the fact that "params" were part of
snd_soc_dpcm. That has changed and now params are "orphaned" so each
BE has to apply a specific fixup for itself.

Best regards,
Lukasz


czw., 21 maj 2020 o 16:25 Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> napisał(a):
>
>
>
> On 5/21/20 8:47 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.
> >
> > Signed-off-by: Lukasz Majczak <lma@semihalf.com>
> > ---
> >   .../intel/boards/kbl_rt5663_rt5514_max98927.c | 130 ++++++++++++------
> >   1 file changed, 85 insertions(+), 45 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..12a9983979e0 100644
> > --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> > +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> > @@ -171,8 +171,8 @@ static const struct snd_soc_dapm_route kabylake_map[] = {
> >       { "hs_in", NULL, "ssp1 Rx" },
> >       { "ssp1 Rx", NULL, "AIF Capture" },
> >
> > -     { "codec1_in", NULL, "ssp0 Rx" },
> > -     { "ssp0 Rx", NULL, "AIF1 Capture" },
> > +     { "codec1_in", NULL, "DMIC01 Rx" },
> > +     { "DMIC01 Rx", NULL, "AIF1 Capture" },
>
> This doesn't seem right. This board uses DMICs attached to the codec so
> we should not make references to routes that are used for PCH-attached
> dmics in all other machine drivers:
>
>         { "dmic01_hifi", NULL, "DMIC01 Rx" },
>         { "DMIC01 Rx", NULL, "DMIC AIF" },
>
> >
> >       /* IV feedback path */
> >       { "codec0_fb_in", NULL, "ssp0 Rx"},
> > @@ -328,42 +328,52 @@ 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")) {
> > +
> > +     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)
> > -                     chan->min = chan->max = 2;
> > +                     channels->min = channels->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);
> > +                     channels->min = channels->max = 4;
> >
> >       return 0;
> >   }
> > @@ -400,20 +410,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 +429,37 @@ 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 +530,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 +646,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 +659,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
> >

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function
  2020-05-21 13:47 Lukasz Majczak
@ 2020-05-21 14:25 ` Pierre-Louis Bossart
  2020-05-21 14:36   ` Łukasz Majczak
  0 siblings, 1 reply; 8+ messages in thread
From: Pierre-Louis Bossart @ 2020-05-21 14:25 UTC (permalink / raw)
  To: Lukasz Majczak, Liam Girdwood, Jie Yang
  Cc: alsa-devel, Radoslaw Biernacki, Ross Zwisler, linux-kernel,
	Bob Brandt, Marcin Wojtas, Alex Levin



On 5/21/20 8:47 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.
> 
> Signed-off-by: Lukasz Majczak <lma@semihalf.com>
> ---
>   .../intel/boards/kbl_rt5663_rt5514_max98927.c | 130 ++++++++++++------
>   1 file changed, 85 insertions(+), 45 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..12a9983979e0 100644
> --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> @@ -171,8 +171,8 @@ static const struct snd_soc_dapm_route kabylake_map[] = {
>   	{ "hs_in", NULL, "ssp1 Rx" },
>   	{ "ssp1 Rx", NULL, "AIF Capture" },
>   
> -	{ "codec1_in", NULL, "ssp0 Rx" },
> -	{ "ssp0 Rx", NULL, "AIF1 Capture" },
> +	{ "codec1_in", NULL, "DMIC01 Rx" },
> +	{ "DMIC01 Rx", NULL, "AIF1 Capture" },

This doesn't seem right. This board uses DMICs attached to the codec so 
we should not make references to routes that are used for PCH-attached 
dmics in all other machine drivers:

	{ "dmic01_hifi", NULL, "DMIC01 Rx" },
	{ "DMIC01 Rx", NULL, "DMIC AIF" },

>   
>   	/* IV feedback path */
>   	{ "codec0_fb_in", NULL, "ssp0 Rx"},
> @@ -328,42 +328,52 @@ 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")) {
> +
> +	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)
> -			chan->min = chan->max = 2;
> +			channels->min = channels->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);
> +			channels->min = channels->max = 4;
>   
>   	return 0;
>   }
> @@ -400,20 +410,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 +429,37 @@ 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 +530,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 +646,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 +659,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
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function
@ 2020-05-21 13:47 Lukasz Majczak
  2020-05-21 14:25 ` Pierre-Louis Bossart
  0 siblings, 1 reply; 8+ messages in thread
From: Lukasz Majczak @ 2020-05-21 13:47 UTC (permalink / raw)
  To: Pierre-Louis Bossart, Liam Girdwood, Jie Yang
  Cc: Bob Brandt, Alex Levin, Ross Zwisler, Marcin Wojtas,
	Radoslaw Biernacki, Lukasz Majczak, alsa-devel, linux-kernel

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.

Signed-off-by: Lukasz Majczak <lma@semihalf.com>
---
 .../intel/boards/kbl_rt5663_rt5514_max98927.c | 130 ++++++++++++------
 1 file changed, 85 insertions(+), 45 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..12a9983979e0 100644
--- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
+++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
@@ -171,8 +171,8 @@ static const struct snd_soc_dapm_route kabylake_map[] = {
 	{ "hs_in", NULL, "ssp1 Rx" },
 	{ "ssp1 Rx", NULL, "AIF Capture" },
 
-	{ "codec1_in", NULL, "ssp0 Rx" },
-	{ "ssp0 Rx", NULL, "AIF1 Capture" },
+	{ "codec1_in", NULL, "DMIC01 Rx" },
+	{ "DMIC01 Rx", NULL, "AIF1 Capture" },
 
 	/* IV feedback path */
 	{ "codec0_fb_in", NULL, "ssp0 Rx"},
@@ -328,42 +328,52 @@ 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")) {
+
+	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)
-			chan->min = chan->max = 2;
+			channels->min = channels->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);
+			channels->min = channels->max = 4;
 
 	return 0;
 }
@@ -400,20 +410,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 +429,37 @@ 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 +530,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 +646,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 +659,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
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-05-21 15:14 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-13 19:20 [PATCH] ASoC: Intel: kbl_rt5663_rt5514_max98927: Split be_hw_params_fixup function Lukasz Majczak
2020-05-14 12:47 ` Pierre-Louis Bossart
2020-05-14 13:56   ` Guenter Roeck
2020-05-21 13:47 Lukasz Majczak
2020-05-21 14:25 ` Pierre-Louis Bossart
2020-05-21 14:36   ` Łukasz Majczak
2020-05-21 14:43     ` Łukasz Majczak
2020-05-21 15:14       ` Pierre-Louis Bossart

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).