Alsa-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [alsa-devel] [PATCH] ASoC: Intel: kbl_rt5663_rt5514_max98927: Add dmic format constraint
@ 2019-09-12  2:27 Yu-Hsuan Hsu
  2019-09-12 13:02 ` Pierre-Louis Bossart
  0 siblings, 1 reply; 6+ messages in thread
From: Yu-Hsuan Hsu @ 2019-09-12  2:27 UTC (permalink / raw)
  To: linux-kernel
  Cc: Cezary Rojewski, Kuninori Morimoto, Takashi Iwai, Jie Yang,
	Pierre-Louis Bossart, Liam Girdwood, Tzung-Bi Shih, Mark Brown,
	Thomas Gleixner, alsa-devel, Yu-Hsuan Hsu

24 bits recording from DMIC is not supported for KBL platform because
the TDM slot between PCH and codec is 16 bits only. We should add a
constraint to remove that unsupported format.

Signed-off-by: Yu-Hsuan Hsu <yuhsuan@chromium.org>
---
 sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
index 74dda8784f1a01..67b276a65a8d2d 100644
--- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
+++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
@@ -400,6 +400,9 @@ static int kabylake_dmic_startup(struct snd_pcm_substream *substream)
 	snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS,
 			dmic_constraints);
 
+	runtime->hw.formats = SNDRV_PCM_FMTBIT_S16_LE;
+	snd_pcm_hw_constraint_msbits(runtime, 0, 16, 16);
+
 	return snd_pcm_hw_constraint_list(substream->runtime, 0,
 			SNDRV_PCM_HW_PARAM_RATE, &constraints_rates);
 }
-- 
2.23.0.162.g0b9fbb3734-goog

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH] ASoC: Intel: kbl_rt5663_rt5514_max98927: Add dmic format constraint
  2019-09-12  2:27 [alsa-devel] [PATCH] ASoC: Intel: kbl_rt5663_rt5514_max98927: Add dmic format constraint Yu-Hsuan Hsu
@ 2019-09-12 13:02 ` Pierre-Louis Bossart
  2019-09-13 14:45   ` Yu-Hsuan Hsu
  2019-09-13 16:01   ` Yu-Hsuan Hsu
  0 siblings, 2 replies; 6+ messages in thread
From: Pierre-Louis Bossart @ 2019-09-12 13:02 UTC (permalink / raw)
  To: Yu-Hsuan Hsu, linux-kernel
  Cc: Cezary Rojewski, Kuninori Morimoto, Jie Yang, Takashi Iwai,
	Liam Girdwood, Tzung-Bi Shih, Mark Brown, Thomas Gleixner,
	alsa-devel

On 9/11/19 9:27 PM, Yu-Hsuan Hsu wrote:
> 24 bits recording from DMIC is not supported for KBL platform because
> the TDM slot between PCH and codec is 16 bits only. We should add a
> constraint to remove that unsupported format.

Humm, when you use DMICs they are directly connected to the PCH with a 
standard 1-bit PDM. There is no notion of TDM or slot.

It could very well be that the firmware/topology only support 16 bit (I 
vaguely recall another case where 24 bits was added), but the 
description in the commit message would need to be modified to make the 
reason for this change clearer.

> 
> Signed-off-by: Yu-Hsuan Hsu <yuhsuan@chromium.org>
> ---
>   sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> index 74dda8784f1a01..67b276a65a8d2d 100644
> --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> @@ -400,6 +400,9 @@ static int kabylake_dmic_startup(struct snd_pcm_substream *substream)
>   	snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS,
>   			dmic_constraints);
>   
> +	runtime->hw.formats = SNDRV_PCM_FMTBIT_S16_LE;
> +	snd_pcm_hw_constraint_msbits(runtime, 0, 16, 16);
> +
>   	return snd_pcm_hw_constraint_list(substream->runtime, 0,
>   			SNDRV_PCM_HW_PARAM_RATE, &constraints_rates);
>   }
> 

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH] ASoC: Intel: kbl_rt5663_rt5514_max98927: Add dmic format constraint
  2019-09-12 13:02 ` Pierre-Louis Bossart
@ 2019-09-13 14:45   ` Yu-Hsuan Hsu
  2019-09-13 17:27     ` Pierre-Louis Bossart
  2019-09-13 16:01   ` Yu-Hsuan Hsu
  1 sibling, 1 reply; 6+ messages in thread
From: Yu-Hsuan Hsu @ 2019-09-13 14:45 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: Cezary Rojewski, Kuninori Morimoto, Takashi Iwai, Jie Yang,
	linux-kernel, Liam Girdwood, Tzung-Bi Shih, Mark Brown,
	Thomas Gleixner, alsa-devel

Thanks for the review! If I'm not mistaken, the microphone is attached to
external codec(rt5514) instead of PCH on Kabylake platform. So there should
be a TDM between DMICs and PCH. We can see in the
kabylake_ssp0_hw_params function,
there are some operations about setting tdm slot_width to 16 bits.
Therefore, I think it only supports S16_LE format for DMICs. Is it correct?

Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> 於 2019年9月12日 週四
下午9:02寫道:

> On 9/11/19 9:27 PM, Yu-Hsuan Hsu wrote:
> > 24 bits recording from DMIC is not supported for KBL platform because
> > the TDM slot between PCH and codec is 16 bits only. We should add a
> > constraint to remove that unsupported format.
>
> Humm, when you use DMICs they are directly connected to the PCH with a
> standard 1-bit PDM. There is no notion of TDM or slot.
>
> It could very well be that the firmware/topology only support 16 bit (I
> vaguely recall another case where 24 bits was added), but the
> description in the commit message would need to be modified to make the
> reason for this change clearer.
>
> >
> > Signed-off-by: Yu-Hsuan Hsu <yuhsuan@chromium.org>
> > ---
> >   sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> > index 74dda8784f1a01..67b276a65a8d2d 100644
> > --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> > +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> > @@ -400,6 +400,9 @@ static int kabylake_dmic_startup(struct
> snd_pcm_substream *substream)
> >       snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS,
> >                       dmic_constraints);
> >
> > +     runtime->hw.formats = SNDRV_PCM_FMTBIT_S16_LE;
> > +     snd_pcm_hw_constraint_msbits(runtime, 0, 16, 16);
> > +
> >       return snd_pcm_hw_constraint_list(substream->runtime, 0,
> >                       SNDRV_PCM_HW_PARAM_RATE, &constraints_rates);
> >   }
> >
>
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH] ASoC: Intel: kbl_rt5663_rt5514_max98927: Add dmic format constraint
  2019-09-12 13:02 ` Pierre-Louis Bossart
  2019-09-13 14:45   ` Yu-Hsuan Hsu
@ 2019-09-13 16:01   ` Yu-Hsuan Hsu
  1 sibling, 0 replies; 6+ messages in thread
From: Yu-Hsuan Hsu @ 2019-09-13 16:01 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: Cezary Rojewski, Kuninori Morimoto, Takashi Iwai, Jie Yang,
	linux-kernel, Liam Girdwood, Tzung-Bi Shih, Mark Brown,
	Thomas Gleixner, alsa-devel

Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> 於
2019年9月12日 週四 下午9:02寫道:
>
> On 9/11/19 9:27 PM, Yu-Hsuan Hsu wrote:
> > 24 bits recording from DMIC is not supported for KBL platform because
> > the TDM slot between PCH and codec is 16 bits only. We should add a
> > constraint to remove that unsupported format.
>
> Humm, when you use DMICs they are directly connected to the PCH with a
> standard 1-bit PDM. There is no notion of TDM or slot.
>
> It could very well be that the firmware/topology only support 16 bit (I
> vaguely recall another case where 24 bits was added), but the
> description in the commit message would need to be modified to make the
> reason for this change clearer.

(I sent it again because the previous email contains HTML subpart.
Sorry for the inconvenience.)

Thanks for the review! If I'm not mistaken, the microphone is attached
to external codec(rt5514) instead of PCH on Kabylake platform. So
there should be a TDM between DMICs and PCH. We can see in the
kabylake_ssp0_hw_params function, there are some operations about
setting tdm slot_width to 16 bits. Therefore, I think it only supports
S16_LE format for DMICs. Is it correct?
>
>
>
> >
> > Signed-off-by: Yu-Hsuan Hsu <yuhsuan@chromium.org>
> > ---
> >   sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> > index 74dda8784f1a01..67b276a65a8d2d 100644
> > --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> > +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> > @@ -400,6 +400,9 @@ static int kabylake_dmic_startup(struct snd_pcm_substream *substream)
> >       snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS,
> >                       dmic_constraints);
> >
> > +     runtime->hw.formats = SNDRV_PCM_FMTBIT_S16_LE;
> > +     snd_pcm_hw_constraint_msbits(runtime, 0, 16, 16);
> > +
> >       return snd_pcm_hw_constraint_list(substream->runtime, 0,
> >                       SNDRV_PCM_HW_PARAM_RATE, &constraints_rates);
> >   }
> >
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH] ASoC: Intel: kbl_rt5663_rt5514_max98927: Add dmic format constraint
  2019-09-13 14:45   ` Yu-Hsuan Hsu
@ 2019-09-13 17:27     ` Pierre-Louis Bossart
  2019-09-14  8:34       ` Yu-Hsuan Hsu
  0 siblings, 1 reply; 6+ messages in thread
From: Pierre-Louis Bossart @ 2019-09-13 17:27 UTC (permalink / raw)
  To: Yu-Hsuan Hsu
  Cc: Cezary Rojewski, Kuninori Morimoto, Takashi Iwai, Jie Yang,
	linux-kernel, Liam Girdwood, Tzung-Bi Shih, Mark Brown,
	Thomas Gleixner, alsa-devel

please don't top-post on public mailing lists, thanks.

On 9/13/19 9:45 AM, Yu-Hsuan Hsu wrote:
> Thanks for the review! If I'm not mistaken, the microphone is attached 
> to external codec(rt5514) instead of PCH on Kabylake platform. So there 
> should be a TDM between DMICs and PCH. We can see in the 
> kabylake_ssp0_hw_params function, there are some operations about 
> setting tdm slot_width to 16 bits. Therefore, I think it only supports 
> S16_LE format for DMICs. Is it correct?

Ah yes, ok. we have other machine drivers where dmic refers to the PCH 
attached case, thanks for the precision.

I am still not clear on the problem, you are adding this constraint to a 
front-end, so in theory the copier element in the firmware should take 
care of converting from 16-bits recorded on the TDM link to the 24 bits 
used by the application. Is this not the case? is this patch based on an 
actual error and if yes can you share more information to help check 
where the problem happens, topology maybe?

> 
> Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com 
> <mailto:pierre-louis.bossart@linux.intel.com>> 於 2019年9月12日 週四 下 
> 午9:02寫道:
> 
>     On 9/11/19 9:27 PM, Yu-Hsuan Hsu wrote:
>      > 24 bits recording from DMIC is not supported for KBL platform because
>      > the TDM slot between PCH and codec is 16 bits only. We should add a
>      > constraint to remove that unsupported format.
> 
>     Humm, when you use DMICs they are directly connected to the PCH with a
>     standard 1-bit PDM. There is no notion of TDM or slot.
> 
>     It could very well be that the firmware/topology only support 16 bit (I
>     vaguely recall another case where 24 bits was added), but the
>     description in the commit message would need to be modified to make the
>     reason for this change clearer.
> 
>      >
>      > Signed-off-by: Yu-Hsuan Hsu <yuhsuan@chromium.org
>     <mailto:yuhsuan@chromium.org>>
>      > ---
>      >   sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c | 3 +++
>      >   1 file changed, 3 insertions(+)
>      >
>      > diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
>     b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
>      > index 74dda8784f1a01..67b276a65a8d2d 100644
>      > --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
>      > +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
>      > @@ -400,6 +400,9 @@ static int kabylake_dmic_startup(struct
>     snd_pcm_substream *substream)
>      >       snd_pcm_hw_constraint_list(runtime, 0,
>     SNDRV_PCM_HW_PARAM_CHANNELS,
>      >                       dmic_constraints);
>      >
>      > +     runtime->hw.formats = SNDRV_PCM_FMTBIT_S16_LE;
>      > +     snd_pcm_hw_constraint_msbits(runtime, 0, 16, 16);
>      > +
>      >       return snd_pcm_hw_constraint_list(substream->runtime, 0,
>      >                       SNDRV_PCM_HW_PARAM_RATE, &constraints_rates);
>      >   }
>      >
> 

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH] ASoC: Intel: kbl_rt5663_rt5514_max98927: Add dmic format constraint
  2019-09-13 17:27     ` Pierre-Louis Bossart
@ 2019-09-14  8:34       ` Yu-Hsuan Hsu
  0 siblings, 0 replies; 6+ messages in thread
From: Yu-Hsuan Hsu @ 2019-09-14  8:34 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: Cezary Rojewski, Kuninori Morimoto, Takashi Iwai, Jie Yang,
	linux-kernel, Liam Girdwood, Tzung-Bi Shih, Mark Brown,
	Thomas Gleixner, alsa-devel

Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> 於
2019年9月14日 週六 上午1:28寫道:
>
> please don't top-post on public mailing lists, thanks.
>
> On 9/13/19 9:45 AM, Yu-Hsuan Hsu wrote:
> > Thanks for the review! If I'm not mistaken, the microphone is attached
> > to external codec(rt5514) instead of PCH on Kabylake platform. So there
> > should be a TDM between DMICs and PCH. We can see in the
> > kabylake_ssp0_hw_params function, there are some operations about
> > setting tdm slot_width to 16 bits. Therefore, I think it only supports
> > S16_LE format for DMICs. Is it correct?
>
> Ah yes, ok. we have other machine drivers where dmic refers to the PCH
> attached case, thanks for the precision.
>
> I am still not clear on the problem, you are adding this constraint to a
> front-end, so in theory the copier element in the firmware should take
> care of converting from 16-bits recorded on the TDM link to the 24 bits
> used by the application. Is this not the case? is this patch based on an
> actual error and if yes can you share more information to help check
> where the problem happens, topology maybe?

If we use 24 bits format on that device to record, the audio samples
it returns are still in 16 bits. So the rate we measured is only the
half of the expected rate. It's a real problem. Apart from the rate,
the audio samples are also wrong if we still decode them with 24 bits
format. Therefore, the better fix is to add a constraint to remove
24bits support.

By the way, we found this issue by "ALSA conformance test", which is a
new tool to verify audio drivers.
(https://chromium.googlesource.com/chromiumos/platform/audiotest/+/master/alsa_conformance_test.md)

>
> >
> > Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com
> > <mailto:pierre-louis.bossart@linux.intel.com>> 於 2019年9月12日 週四 下
> > 午9:02寫道:
> >
> >     On 9/11/19 9:27 PM, Yu-Hsuan Hsu wrote:
> >      > 24 bits recording from DMIC is not supported for KBL platform because
> >      > the TDM slot between PCH and codec is 16 bits only. We should add a
> >      > constraint to remove that unsupported format.
> >
> >     Humm, when you use DMICs they are directly connected to the PCH with a
> >     standard 1-bit PDM. There is no notion of TDM or slot.
> >
> >     It could very well be that the firmware/topology only support 16 bit (I
> >     vaguely recall another case where 24 bits was added), but the
> >     description in the commit message would need to be modified to make the
> >     reason for this change clearer.
> >
> >      >
> >      > Signed-off-by: Yu-Hsuan Hsu <yuhsuan@chromium.org
> >     <mailto:yuhsuan@chromium.org>>
> >      > ---
> >      >   sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c | 3 +++
> >      >   1 file changed, 3 insertions(+)
> >      >
> >      > diff --git a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> >     b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> >      > index 74dda8784f1a01..67b276a65a8d2d 100644
> >      > --- a/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> >      > +++ b/sound/soc/intel/boards/kbl_rt5663_rt5514_max98927.c
> >      > @@ -400,6 +400,9 @@ static int kabylake_dmic_startup(struct
> >     snd_pcm_substream *substream)
> >      >       snd_pcm_hw_constraint_list(runtime, 0,
> >     SNDRV_PCM_HW_PARAM_CHANNELS,
> >      >                       dmic_constraints);
> >      >
> >      > +     runtime->hw.formats = SNDRV_PCM_FMTBIT_S16_LE;
> >      > +     snd_pcm_hw_constraint_msbits(runtime, 0, 16, 16);
> >      > +
> >      >       return snd_pcm_hw_constraint_list(substream->runtime, 0,
> >      >                       SNDRV_PCM_HW_PARAM_RATE, &constraints_rates);
> >      >   }
> >      >
> >
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-12  2:27 [alsa-devel] [PATCH] ASoC: Intel: kbl_rt5663_rt5514_max98927: Add dmic format constraint Yu-Hsuan Hsu
2019-09-12 13:02 ` Pierre-Louis Bossart
2019-09-13 14:45   ` Yu-Hsuan Hsu
2019-09-13 17:27     ` Pierre-Louis Bossart
2019-09-14  8:34       ` Yu-Hsuan Hsu
2019-09-13 16:01   ` Yu-Hsuan Hsu

Alsa-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/alsa-devel/0 alsa-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 alsa-devel alsa-devel/ https://lore.kernel.org/alsa-devel \
		alsa-devel@alsa-project.org alsa-devel@archiver.kernel.org
	public-inbox-index alsa-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.alsa-project.alsa-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox