RE: [PATCH v3] ASoC: Intel: sof_rt5682: Add ALC1015Q-VB speaker amp support
> > The code is looks fine, but Jack Yu added a separate patch that makes > RTL1019 equivalent to RTL1015, so should this patch also handle the > RTL1019 case? The topology used by this machine driver is using 48k, 64fs I2S format. RT1019 needs to support this configuration. Not sure if RT1019 could support that. Regards, Brent
RE: [PATCH v2] ASoC: Intel: sof_rt5682: Add ALC1015Q-VB speaker amp support
> > +/*
> > + * rt1015: i2c mode driver for ALC1015 and ALC1015Q
> > + * rt1015p: auto-mode driver for ALC1015, ALC1015Q, and ALC1015Q-VB
> > +*/ static const struct snd_soc_dapm_route rt1015p_1dev_dapm_routes[]
> > += {
> > + /* speaker */
> > + { "Left Spk", NULL, "Speaker" },
> > + { "Right Spk", NULL, "Speaker" },
> > +};
> > +
> > +static const struct snd_soc_dapm_route rt1015p_2dev_dapm_routes[] = {
> > + /* speaker */
> > + { "Left Spk", NULL, "Left Speaker" },
> > + { "Right Spk", NULL, "Right Speaker" }, };
>
> I am confused by these routes...
>
> is this a result of using the codec confs below only when there are 2 amps
> with
> their own enable pin?
Yes, we need to use the prefix in codec confs when there are two device
instances.
>
> You still have 2 amps even in the 1dev case, so I want to make sure the code
> has
> enough comments so that we don't lose track of the design.
>
> The rest of the code looks fine.
>
There are always two amps on the board for stereo output. If the two amps
sharing
one enable pin, ODM will implement only one device instance in the ACPI table
because there is only one GPIO to control in codec driver.
I'll upload a v3 with more comment.
Regards,
Brent
RE: [PATCH] ASoC: Intel: sof_rt5682: Add rt1015p speaker amp support
> > could we not handle this quirk inside one of the two dai_link or codec_conf > functions above? > The machine driver does not care about this RT1015P_SHARE_EN_SPK quirk, > it's only used in those two functions so should be set in this scope. > There's no need to make it visible to the machine driver, is there? > Thank you for the comment; this patch looks terrible... I will improve the code and upload a v2 patch. Regards, Brent
RE: [PATCH] ASoC: Intel: sof_rt5682: Add rt1015p speaker amp support
> > This patch adds jsl_rt5682_rt1015p which supports the > RT5682 headset codec and RT1015P speaker amplifier combination on > JasperLake platform. > > Signed-off-by: Brent Lu > --- > sound/soc/codecs/rt1015p.c| 10 ++ > sound/soc/intel/boards/Kconfig| 1 + > sound/soc/intel/boards/sof_realtek_common.c | 94 > +++ > sound/soc/intel/boards/sof_realtek_common.h | 8 ++ > sound/soc/intel/boards/sof_rt5682.c | 31 +- > .../intel/common/soc-acpi-intel-jsl-match.c | 13 +++ > 6 files changed, 155 insertions(+), 2 deletions(-) > Sorry it contains unnecessary patch for codec driver. I'll remove it and upload a version 2. I'm also wondering if it's a good idea to use acpi_dev_present() function in driver probe to determine the number of amp device instance. Without using it, we will need to use quirk for those devices using one EN pin for two amps. Regards, Brent
RE: [PATCH] ASoC: intel: sof_rt5682: Add support for tgl_rt1011_rt5682
> > +struct {
> > + unsigned int tx;
> > + unsigned int rx;
> > +} rt1011_tdm_mask[] = {
> > + {.tx = 0x4, .rx = 0x1},
> > + {.tx = 0x8, .rx = 0x2},
> > +};
>
> as noted in the GitHub review this should be static and possibly const.
Will fix in v2. Also add const to structures in soc-acpi-intel-tgl-match.c
and include file in sof_realtek_common.c
Regards,
Brent
RE: [PATCH 2/2] ASoC: intel: sof_rt5682: Add quirk for Dooly
, Brent Lu wrote:
> > This DMI product family string of this board is "Google_Hatch" so the
> > DMI quirk will take place. However, this board is using rt1015 speaker
> > amp instead of max98357a specified in the quirk. Therefore, we need an
> > new DMI quirk for this board.
>
> Do you actually need a DMI quirk for this platform?
>
> the .driver_data below uses the exact same settings as what you would use
> with the generic solution based on ACPI IDs, see below.
>
> Wondering if patch1 would be enough?
>
Dooly has DMI family string " Google_Hatch" so the DMI quirk will overwrite the
driver_data. I asked google but they prefer not removing this string so it
seems to
me that one extra DMI quirk is needed.
{
.callback = sof_rt5682_quirk_cb,
.matches = {
DMI_MATCH(DMI_PRODUCT_FAMILY,
"Google_Hatch"),
},
.driver_data = (void *)(SOF_RT5682_MCLK_EN |
SOF_RT5682_MCLK_24MHZ |
SOF_RT5682_SSP_CODEC(0) |
SOF_SPEAKER_AMP_PRESENT |
SOF_RT5682_SSP_AMP(1)),
},
The other way is using acpi_dev_present() in probe to update the quirk with
correct
codec setting. Which one do you think is better?
Regards,
Brent
RE: [PATCH] ASoC: Intel: sof_rt5682: update quirk for cml boards
>
> That setting is not wrong, but is it sufficient?
>
> see e.g. what we set for existing platforms which need 24 Mhz in this
> driver:
>
> DMI quirks:
>
> {
> .callback = sof_rt5682_quirk_cb,
> .matches = {
> DMI_MATCH(DMI_PRODUCT_FAMILY,
> "Google_Hatch"),
> },
> .driver_data = (void *)(SOF_RT5682_MCLK_EN |
> SOF_RT5682_MCLK_24MHZ |
> SOF_RT5682_SSP_CODEC(0) |
> SOF_SPEAKER_AMP_PRESENT |
> SOF_RT5682_SSP_AMP(1)),
> },
>
Hi Pierre,
CML chromebox is using DMI quirk so we don't need this patch. Sorry
for not noticing it.
Handle 0x0001, DMI type 1, 27 bytes
System Information
Manufacturer: Google
Product Name: Duffy
Version: rev3
Serial Number: 123456789
UUID: Not Settable
Wake-up Type: Reserved
SKU Number: sku33554439
Family: Google_Hatch
RE: [PATCH v3] ASoC: hdac_hdmi: support 'ELD' mixer
> > Please don't send new patches in the middle of existing threads, it makes it > hard to keep track fo what is going on. Sorry for the problem. Does it mean I should avoid using " --in-reply-to=" when sending new patch set? Regards, Brent
RE: [PATCH v2] ASoC: hdac_hdmi: support 'ELD' mixer
> > This is a bit iffy part. If same PCM is connected to multiple receivers, you > return ELD data for the first one and ignore the rest. OTOH, this is inline > with > comment in hdac_hdmi_get_port_from_cvt() in that this pcm-to-many > routing is not really supported by the driver now. But jack status reporting > is > done a port basis, not per PCM/CVTs, so this is not fully aligned. > > Hmm. Given the proposed patch is aligned with the user-space interface > exposed by patch_hdmi.c, I'm ok to go with this. Can you add an explicit > comment to explain what is happening above? > > Br, Kai Fix it in v3. Thank you for the review. Regards, Brent
RE: [PATCH] ASoC: hdac_hdmi: support 'ELD' mixer
> > + /* add control for ELD Bytes */
> > + err = hdac_hdmi_create_eld_ctl(component, pcm);
> > + if (err < 0) {
> > + dev_err(>dev,
> > + "eld control add failed with err: %d for pcm: %d\n",
> > + err, device);
> > + kfree(pcm);
>
> pcm is allocated vida devm_kzalloc(), hence you shoudn't free it explicitly.
Will fix it in v2. Thanks.
Brent
>
>
> thanks,
>
> Takashi
RE: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board
> > > > > > CRAS calls snd_pcm_hw_params_set_buffer_size_max() to use as large > > > buffer as possible. So the period size is an arbitrary number in > > > different platforms. Atom SST platform happens to be 256, and CML > > > SOF platform is 1056 for example. > > > > ok, but earlier in this thread it was mentioned that values such as > > 432 are not suitable. the statement above seems to mean the period > > actual value is a "don't care", so I don't quite see why this specific > > patch2 restricting the value to 240 is necessary. Patch1 is needed for > > sure, > > Patch2 is where Takashi and I are not convinced. > > I have downloaded the patch1 but it does not work. After applying patch1, > the default period size changes to 320. However, it also has the same issue > with period size 320. (It can be verified by aplay.) The period_size is related to the audio latency so it's decided by application according to the use case it's running. That's why there are concerns about patch 2 and also you cannot find similar constraints in other machine driver. Another problem is the buffer size. Too large buffer is not just wasting memories. It also creates problems to memory allocator since continuous pages are not always there. Using a small period_count like 2 or 4 should be sufficient for audio data transfer. buffer_size = period_size * period_count * 100 / sample_rate; snd_pcm_hw_params_set_buffer_time_near(mPcmDevice, params, _size, NULL); And one more problem here: you need to decide period_size and period_count first in order to calculate the buffer size... Regards, Brent
RE: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board
> > > > I also wonder what's really missing, too :) > > > > BTW, I took a look back at the thread, and CRAS seems using a very > > large buffer, namely: > > [ 52.434791] sound pcmC1D0p: PERIOD_SIZE [240:240] > > [ 52.434802] sound pcmC1D0p: BUFFER_SIZE [204480:204480] > > yes, that's 852 periods and 4.260 seconds. Never seen such values :-) CRAS calls snd_pcm_hw_params_set_buffer_size_max() to use as large buffer as possible. So the period size is an arbitrary number in different platforms. Atom SST platform happens to be 256, and CML SOF platform is 1056 for example. Regards, Brent
RE: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board
>
> Sorry for the late reply. CRAS does not set the period size when using it.
> The default period size is 256, which consumes the samples quickly(about 49627
> fps when the rate is 48000 fps) at the beginning of the playback.
> Since CRAS write samples with the fixed frequency, it triggers underruns
> immidiately.
>
> According to Brent, the DSP is using 240 period regardless the hw_param. If
> the
> period size is 256, DSP will read 256 samples each time but only consume 240
> samples until the ring buffer of DSP is full. This behavior makes the samples
> in
> the ring buffer of kernel consumed quickly. (Not sure whether the explanation
> is
> correct. Need Brent to confirm it.)
>
> Unfortunately, we can not change the behavior of DSP. After some experiments,
> we found that the issue can be fixed if we set the period size to 240. With
> the
> same frequency as the DSP, the samples are consumed stably. Because everyone
> can trigger this issue when using the driver without setting the period size,
> we
> think it is a general issue that should be fixed in the kernel.
I check the code and just realized CRAS does nothing but request maximum buffer
size. As I know the application needs to decide the buffer time and period time
so
ALSA could generate a hw_param structure with proper period size instead of
using
fixed constraint in machine driver because driver has no idea about the latency
you
want.
You can use snd_pcm_hw_params_set_buffer_time_near() and
snd_pcm_hw_params_set_period_time_near() to get a proper configuration of
buffer and period parameters according to the latency requirement. In the CRAS
code, there is a UCM variable to support this: DmaPeriodMicrosecs. I tested it
on
Celes and it looks quite promising. It seems to me that adding constraint in
machine
driver is not necessary.
SectionDevice."Speaker".0 {
Value {
PlaybackPCM "hw:chtrt5650,0"
DmaPeriodMicrosecs "5000"
...
[ 52.434761] sound pcmC1D0p: hw_param
[ 52.434767] sound pcmC1D0p: ACCESS 0x1
[ 52.434770] sound pcmC1D0p: FORMAT 0x4
[ 52.434772] sound pcmC1D0p: SUBFORMAT 0x1
[ 52.434776] sound pcmC1D0p: SAMPLE_BITS [16:16]
[ 52.434779] sound pcmC1D0p: FRAME_BITS [32:32]
[ 52.434782] sound pcmC1D0p: CHANNELS [2:2]
[ 52.434785] sound pcmC1D0p: RATE [48000:48000]
[ 52.434788] sound pcmC1D0p: PERIOD_TIME [5000:5000]
[ 52.434791] sound pcmC1D0p: PERIOD_SIZE [240:240]
[ 52.434794] sound pcmC1D0p: PERIOD_BYTES [960:960]
[ 52.434797] sound pcmC1D0p: PERIODS [852:852]
[ 52.434799] sound pcmC1D0p: BUFFER_TIME [426:426]
[ 52.434802] sound pcmC1D0p: BUFFER_SIZE [204480:204480]
[ 52.434805] sound pcmC1D0p: BUFFER_BYTES [817920:817920]
[ 52.434808] sound pcmC1D0p: TICK_TIME [0:0]
Regards,
Brent
>
> Thanks,
> Yu-Hsuan
RE: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board
> > I don't get this. If the platform driver already stated 240 and 960 samples > why > would 432 be chosen? Doesn't this mean the constraint is not applied? Hi Pierre, Sorry for late reply. I used following constraints in V3 patch so any period which aligns 1ms would be accepted. + /* +* Make sure the period to be multiple of 1ms to align the +* design of firmware. Apply same rule to buffer size to make +* sure alsa could always find a value for period size +* regardless the buffer size given by user space. +*/ + snd_pcm_hw_constraint_step(substream->runtime, 0, + SNDRV_PCM_HW_PARAM_PERIOD_SIZE, 48); + snd_pcm_hw_constraint_step(substream->runtime, 0, + SNDRV_PCM_HW_PARAM_BUFFER_SIZE, 48); Regards, Brent > > > [ 52.011146] sound pcmC1D0p: hw_param > > [ 52.011152] sound pcmC1D0p: ACCESS 0x1 > > [ 52.011155] sound pcmC1D0p: FORMAT 0x4 > > [ 52.011158] sound pcmC1D0p: SUBFORMAT 0x1 > > [ 52.011161] sound pcmC1D0p: SAMPLE_BITS [16:16] > > [ 52.011164] sound pcmC1D0p: FRAME_BITS [32:32] > > [ 52.011167] sound pcmC1D0p: CHANNELS [2:2] > > [ 52.011170] sound pcmC1D0p: RATE [48000:48000] > > [ 52.011173] sound pcmC1D0p: PERIOD_TIME [9000:9000] > > [ 52.011176] sound pcmC1D0p: PERIOD_SIZE [432:432] > > [ 52.011179] sound pcmC1D0p: PERIOD_BYTES [1728:1728] > > [ 52.011182] sound pcmC1D0p: PERIODS [474:474] > > [ 52.011185] sound pcmC1D0p: BUFFER_TIME [4266000:4266000] > > [ 52.011188] sound pcmC1D0p: BUFFER_SIZE [204768:204768] > > [ 52.011191] sound pcmC1D0p: BUFFER_BYTES [819072:819072] > > [ 52.011194] sound pcmC1D0p: TICK_TIME [0:0] > > > > Regards, > > Brent > > > >> > >> > >> Takashi > > > >
RE: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board
> > For avoid further misunderstanding: it's fine that CRAS *uses* such a short > period. It's often required for achieving a short latency. > > However, the question is whether the driver can set *only* this value for > making it working. IOW, if we don't have this constraint, what actually > happens? If the driver gives the period size alignment, wouldn't CRAS > choose 240? It won't. Without the constraint it becomes 432. Actually CRAS does not set period size specifically so the value depends on the constraint rules. [ 52.011146] sound pcmC1D0p: hw_param [ 52.011152] sound pcmC1D0p: ACCESS 0x1 [ 52.011155] sound pcmC1D0p: FORMAT 0x4 [ 52.011158] sound pcmC1D0p: SUBFORMAT 0x1 [ 52.011161] sound pcmC1D0p: SAMPLE_BITS [16:16] [ 52.011164] sound pcmC1D0p: FRAME_BITS [32:32] [ 52.011167] sound pcmC1D0p: CHANNELS [2:2] [ 52.011170] sound pcmC1D0p: RATE [48000:48000] [ 52.011173] sound pcmC1D0p: PERIOD_TIME [9000:9000] [ 52.011176] sound pcmC1D0p: PERIOD_SIZE [432:432] [ 52.011179] sound pcmC1D0p: PERIOD_BYTES [1728:1728] [ 52.011182] sound pcmC1D0p: PERIODS [474:474] [ 52.011185] sound pcmC1D0p: BUFFER_TIME [4266000:4266000] [ 52.011188] sound pcmC1D0p: BUFFER_SIZE [204768:204768] [ 52.011191] sound pcmC1D0p: BUFFER_BYTES [819072:819072] [ 52.011194] sound pcmC1D0p: TICK_TIME [0:0] Regards, Brent > > > Takashi
RE: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board
> > Hi Takashi, > > > > I've double checked with google. It's a must for Chromebooks due to > > low latency use case. > > I wonder if there's a misunderstanding here? > > I believe Takashi's question was "is this a must to ONLY accept 240 samples > for the period size", there was no pushback on the value itself. > Are those boards broken with e.g. 960 samples? I've added google people to discuss directly. Hi Yuhsuan, Would you explain why CRAS needs to use such short period size? Thanks. Regards, Brent
RE: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board
> > > > > > Again, is this fixed 240 is a must? Or is this also an alignment issue? > > Hi Takashi, > > > > I think it's a must for Chromebooks. Google found this value works > > best with their CRAS server running on their BSW products. They > > offered this patch for their own Chromebooks. > > Hrm, but it's likely a worse choice on other sound backends. > > Please double-check whether this fixed small period is a must, or it's an > alignment issue. Hi Takashi, I've double checked with google. It's a must for Chromebooks due to low latency use case. Regards, Brent > > > Takashi
RE: [PATCH v3 2/2] ASoC: Intel: Add period size constraint on strago board
> > Again, is this fixed 240 is a must? Or is this also an alignment issue? Hi Takashi, I think it's a must for Chromebooks. Google found this value works best with their CRAS server running on their BSW products. They offered this patch for their own Chromebooks. > > > thanks, > > Takashi
RE: [PATCH 2/2] ASoC: Intel: Add period size constraint on strago board
> > If the 1ms alignment is the condition, it can be better with a different > hw_params constraint. We can use > snd_pcm_hw_constraint_step() for such a purpose. Will fix. Thanks. Regards, Brent > > > thanks, > > Takashi
RE: [PATCH 2/2] ASoC: Intel: Add period size constraint on strago board
> >> > >> Yes or alsa will select 320 as default period size for it. > > > > ok, then that's a miss in your patch1. 320 samples is a multiple of > > 1ms > > typo: is NOT > > > for 48kHz rates. I think it was valid only for the 16kHz VoIP paths > > used in some versions of Android, but that we don't support in the > > upstream code. > > > > To build on Takashi's answer, the real ask here is to require that the > > period be a multiple of 1ms, because that's the fundamental > > design/limitation of firmware. It doesn't matter if it's 48, 96, 192, > > 240, 480, 960 samples. Yes 320 is for VoIP and the rates in CPU DAI are 8/16KHz. It's a mistake. Regards, Brent
RE: [PATCH v2 0/2] Add period size constraint for Atom Chromebook
> > > > Two different constraints are implemented: one is in platform's CPU > > DAI to enforce period sizes which are already used in Android BSP. The > > other is in Atom Chromebook's machine driver to use 240 as period size. > > > > Changes since v1: > > -Add comma at the end of media_period_size array declaration. > > Is it a hardware restriction? Unless it's a must for some hardware issues, > enforcing such a small period size is nothing but a drawback for a system > without CRAS. Hi Takashi, This patch is cherry-picked from Chrome's branch which is originally implemented by google. They found this value works best with CRAS and these two machine drivers are for Chromebooks. Other BSW machine drivers are untouched. Regards, Brent > > > thanks, > > Takashi > > > > > Brent Lu (1): > > ASoC: intel: atom: Add period size constraint > > > > Yu-Hsuan Hsu (1): > > ASoC: Intel: Add period size constraint on strago board > > > > sound/soc/intel/atom/sst-mfld-platform-pcm.c | 15 +++ > > sound/soc/intel/boards/cht_bsw_max98090_ti.c | 14 +- > > sound/soc/intel/boards/cht_bsw_rt5645.c | 14 +- > > 3 files changed, 41 insertions(+), 2 deletions(-) > > > > -- > > 2.7.4 > >
RE: [PATCH v2 2/2] ASoC: Intel: Add period size constraint on strago board
> On Thu, Jul 30, 2020 at 04:13:35PM +0800, Brent Lu wrote: > > From: Yu-Hsuan Hsu > > > > The CRAS server does not set the period size in hw_param so ALSA will > > calculate a value for period size which is based on the buffer size > > and other parameters. The value may not always be aligned with Atom's > > dsp design so a constraint is added to make sure the board always has > > a good value. > > > > Cyan uses chtmax98090 and others(banon, celes, edgar, kefka...) use > > rt5650. > > Actually one more comment here. > Can you split per machine driver? > It adds constraints on BSW Chromebooks for same purpose. I don't see the benefit to split it. Regards, Brent > > sound/soc/intel/boards/cht_bsw_max98090_ti.c | 14 +- > > sound/soc/intel/boards/cht_bsw_rt5645.c | 14 +- > > -- > With Best Regards, > Andy Shevchenko >
RE: [PATCH 2/2] ASoC: Intel: Add period size constraint on strago board
> > Is this patch required if you've already constrained the period sizes for the > platform driver in patch1? Yes or alsa will select 320 as default period size for it. Regards, Brent
RE: [PATCH] ASoC: Intel: Atom: use hardware counter to update hw_ptr
> > So if there are already quirks in atom machine drivers to change the period > size, why is this patch necessary? > The story is: google implemented the constraint but doesn't know why it works so asked us to explain. After checking the two counters I realized the increase of ring buffer pointer follows the period size setting in hw_param (256) but the period of interrupt is always 5ms instead of 5.33 so it's running little bit too fast. It seems the LPE keeps tracking the difference of two counters. When the difference exceeds 2160 samples, the next interrupt will be canceled so the hardware counter could catch up a little. [ 43.208299] intel_sst_acpi 808622A8:00: mrfld ring_buffer_counter 107520 hardware_counter 98880 pcm delay 8640 (in bytes) [ 43.208306] intel_sst_acpi 808622A8:00: buffer ptr 26880 pcm_delay rep: 2160 [ 43.208321] sound pcmC1D0p: [Q] pos 26880 hw_ptr 26880 appl_ptr 4 avail 191680 => one interrupt is skipped. [ 43.218299] intel_sst_acpi 808622A8:00: mrfld ring_buffer_counter 108544 hardware_counter 100800 pcm delay 7744 (in bytes) [ 43.218307] intel_sst_acpi 808622A8:00: buffer ptr 27136 pcm_delay rep: 1936 [ 43.218336] sound pcmC1D0p: [Q] pos 27136 hw_ptr 27136 appl_ptr 4 avail 191936 So I think why not using the hardware counter? It increases 240 samples every 5ms perfectly match the 48000 sample rate. The test result is good but I know there must be a reason for the original designer to use ring buffer counter instead of hardware counter. I uploaded this patch to see if anyone still remember the reason and share some insight with me. I totally agree that we shouldn't touch this part of design. Do you think it make sense to add a constraint to enforce the period size in machine driver? If yes then I would upload patches for Chrome atom machines for google. Regards, Brent > > I'm curious why not just using hardware counter to update hw_ptr and > > get rid of the period setting in hw_param? It seems to me the ring > > buffer counter does not reflect the real status. > > I don't recall precisely what this hardware counter does. I vaguely recall > it's > tied to the 19.2MHz external timer which is also used to schedule the 1ms > SBA mixer and the SSP IOs. And by comparing with the ring buffer pointer > you can infer the delay inside the DSP. I think you are also making an > assumption that all streams are tied to the output rate, but that's most > likely > a bad assumption. The hard-coded topology supported media, speech and > compressed data and the consumption rate on the DMA side could be faster > with some buffering happening in the DSP. > It's not a passthrough DMA in all cases. > > This is really legacy code that no one really fully understands nor plans on > improving, it'd be a bad idea to change the pcm pointer reports now, 6 years > after the initial code release and after all initial contributors moved on. > It's > what it is. >
RE: [PATCH] ASoC: Intel: Atom: use hardware counter to update hw_ptr
> > All the Atom firmware assumes data chunks in multiples of 1ms (typically 5, > 10 or 20ms). I have never seen anyone use 256 frames, that's asking for > trouble really. > > it's actually the same with Skylake and SOF in most cases. > > Is this a 'real' problem or a problem detected by the Chrome ALSA > compliance tests, in the latter case that would hint at a too generic value of > min_period. > I've told them 240 is more reasonable since the sample rate is 48000 and our android bsp also uses 240 for multimedia use case for many years but they don't want to change the CRAS setting for some reason. Google says it's a real issue for them: "The driver consumes frames quickly at the beginning will make CRAS underrun because CRAS fills samples in the fixed rate." Currently they implement constraint in machine driver of atom machines to force 240 period size so CRAS is using 240 for atom platforms and 256 for other big cores. I'm curious why not just using hardware counter to update hw_ptr and get rid of the period setting in hw_param? It seems to me the ring buffer counter does not reflect the real status. Regards, Brent > > and that seems also wrong? Why would the delay be zero? > info->pcm_delay is the difference between ring buffer counter and hardware counter. Because the ring buffer counter (hw_ptr) is running faster then it should, so we add the info->pcm_delay to substream->runtime->delay as compensation. Therefore, application could use snd_pcm_delay() to get the actual frame number which are still in buffer. snd_pcm_delay() = buffer_size - snd_pcm_avail() + runtime->delay We don't need pcm_delay to compensate anything if using hardware counter. > > - info->pcm_delay = delay_frames; > > dev_dbg(ctx->dev, "buffer ptr %llu pcm_delay rep: %llu\n", > > info->buffer_ptr, info->pcm_delay); > > return 0; > >
RE: [PATCH] ASoC: hdac_hdmi: remove cancel_work_sync in runtime suspend
> A deadlock is identified when there are three contexts running at the same > time: > - a HDMI jack work which is calling snd_soc_dapm_sync(). > - user space is calling snd_pcm_release() to close pcm device. > - pm is calling runtime suspend function of HDMI codec driver. > > By removing the clear_dapm_works() invocation in the > hdac_hdmi_runtime_suspend() function, the snd_pcm_release() could > always returns from dapm_power_widgets() function call without blocking > the hdac_hdmi_jack_dapm_work() work thread or being blocked by the > hdac_hdmi_runtime_suspend() function. The purpose of the jack work is to > enable/disable the dapm jack pin so it's not necessary to cancel the work in > runtime suspend function which is usually called when pcm device is closed. > > Signed-off-by: Brent Lu > --- It is found on CML Chromebox 'Puff' which is using Chrome-v4.19 branch. Kernel resets itself because tasks getting stuck in mutex for 2 mins. Following is the reproduce steps: 1. Connect to external HDMI display. 2. Open web page "https://bitmovin.com/demos/drm; 3. Reload the web page multiple times until device reboot snd_pcm_release() - In dapm_power_widgets() waitng for dapm_pre_sequence_async() to complete. - Holding the card->dapm_mutex when calling snd_soc_dapm_stream_event() function. - Call trace: [ 1352.228063] __schedule+0x4b8/0xf5c [ 1352.228074] schedule+0x3f/0x7a [ 1352.228083] async_synchronize_cookie_domain+0xb5/0x11c [ 1352.228091] ? init_wait_entry+0x2e/0x2e [ 1352.228101] dapm_power_widgets+0x1eb/0x3b5 [ 1352.228111] snd_soc_dapm_stream_event+0x80/0x92 [ 1352.228121] dpcm_dapm_stream_event+0x58/0x7c [ 1352.228130] dpcm_fe_dai_close+0xa3/0x118 [ 1352.228141] snd_pcm_release_substream+0x72/0xbc [ 1352.228148] snd_pcm_release+0x3e/0x9b [ 1352.228156] __fput+0xfa/0x1ef [ 1352.228166] task_work_run+0x7f/0xa7 [ 1352.228174] exit_to_usermode_loop+0xa5/0xa7 [ 1352.228182] do_syscall_64+0xfb/0x14b [ 1352.228190] entry_SYSCALL_64_after_hwframe+0x44/0xa9 dapm_pre_sequence_async() - In rpm_resume() waiting for hdac_hdmi_runtime_suspend() to complete. - Call trace: [ 1352.227903] Workqueue: events_unbound async_run_entry_fn [ 1352.227908] Call Trace: [ 1352.227917] __schedule+0x4b8/0xf5c [ 1352.227926] schedule+0x3f/0x7a [ 1352.227934] rpm_resume+0x16e/0x5ee [ 1352.227944] ? init_wait_entry+0x2e/0x2e [ 1352.227951] __pm_runtime_resume+0x6b/0x83 [ 1352.227959] dapm_pre_sequence_async+0x2c/0x8f [ 1352.227967] async_run_entry_fn+0x3d/0xd1 [ 1352.227975] process_one_work+0x19b/0x4c9 [ 1352.227983] worker_thread+0x10d/0x284 [ 1352.227991] kthread+0x138/0x140 [ 1352.227998] ? process_one_work+0x4c9/0x4c9 [ 1352.228006] ? kthread_associate_blkcg+0xbe/0xbe [ 1352.228012] ret_from_fork+0x1f/0x40 hdac_hdmi_runtime_suspend() - In clear_dapm_works() waiting for hdac_hdmi_jack_dapm_work() to complete. - Call trace: [ 1352.228359] Workqueue: pm pm_runtime_work [ 1352.228379] Call Trace: [ 1352.228396] __schedule+0x4b8/0xf5c [ 1352.228410] ? delete_node+0x74/0x1ab [ 1352.228426] schedule+0x3f/0x7a [ 1352.228442] schedule_timeout+0x11f/0x176 [ 1352.228461] do_wait_for_common+0x118/0x164 [ 1352.228477] ? console_conditional_schedule+0x28/0x28 [ 1352.228494] ? queue_core_balance+0x75/0x75 [ 1352.228509] wait_for_completion+0x4c/0x58 [ 1352.228526] __flush_work+0x112/0x1a1 [ 1352.228540] ? insert_wq_barrier+0x118/0x118 [ 1352.228556] __cancel_work_timer+0x9f/0x1ad [ 1352.228569] ? __switch_to_asm+0x41/0x70 [ 1352.228582] ? __switch_to_asm+0x35/0x70 [ 1352.228596] ? __switch_to_asm+0x41/0x70 [ 1352.228613] clear_dapm_works+0x3f/0x60 [snd_soc_hdac_hdmi] [ 1352.228627] ? hdmi_codec_resume+0x40/0x40 [snd_soc_hdac_hdmi] [ 1352.228642] hdac_hdmi_runtime_suspend+0x20/0xac [snd_soc_hdac_hdmi] [ 1352.228654] __rpm_callback+0x8a/0xf3 [ 1352.228669] ? hdmi_codec_resume+0x40/0x40 [snd_soc_hdac_hdmi] [ 1352.228680] rpm_suspend+0x3b4/0x73e [ 1352.228696] pm_runtime_work+0x5d/0xa0 [ 1352.228708] process_one_work+0x19b/0x4c9 [ 1352.228717] worker_thread+0x10d/0x284 [ 1352.228727] kthread+0x138/0x140 [ 1352.228736] ? process_one_work+0x4c9/0x4c9 [ 1352.228746] ? kthread_associate_blkcg+0xbe/0xbe [ 1352.228761] ret_from_fork+0x1f/0x40 hdac_hdmi_jack_dapm_work() - In snd_soc_dapm_sync() waiting for waiting for card->dapm_mutex to be released. - The mutex is held by snd_pcm_release(), deadlock here. - Call trace: [ 1352.228835] Workqueue: events hdac_hdmi_jack_dapm_work [snd_soc_hdac_hdmi] [ 1352.228845] Call Trace: [ 1352.228860] __schedule+0x4b8/0xf5c [ 1352.228878] ? mutex_spin_on_owner+0x86/0x94 [ 1352.228896] schedule_preempt_disabled+0x47/0x8a [ 1352.228911] __mutex_lock_common+0x25c/0x52c [ 1352.228929] __mutex_lock_slowpath+0x20/0x26 [ 1352.228946] snd_soc_dapm_sync+0x1b/0x4d [ 1352.228960] process_one_work+0x19b/0x4c9 [ 1352.228976] worker_thread+0x1e4/0x284 [ 1352.228994]
RE: [PATCH] ASoC: Intel: bxt-da7219-max98357a: support MAX98390 speaker amp
>
> this doesn't look too good, the only difference is the addition of
> MAX98090 which should be added in
> SND_SOC_INTEL_DA7219_MAX98357A_GENERIC
> above.
Will do
>
> i don't think you need a different top-level config, just extend the existing
> one to say MAX98357A or MAX98390.
>
> [...]
>
OK. I'll modify the description for MAX98390
>
> these look like unrelated changes?
>
>
Will revert those changes.
>
> That looks just wrong?
>
> It would be really odd to list two devices as prerequisites for loading a
> driver,
> when in practice they are mutually exclusive? Something's broken in
> coreboot if both are present.
>
> see below what we used for JSL:
>
> see static struct snd_soc_acpi_codecs jsl_7219_98373_codecs = {
> .num_codecs = 1,
> .codecs = {"MX98373"}
> };
>
> static struct snd_soc_acpi_codecs mx98360a_spk = {
> .num_codecs = 1,
> .codecs = {"MX98360A"}
> };
Err... I misread the snd_soc_acpi_codec_list function and made this error. Will
fix it in the V2 patch. I will also fix the Chrome v4.19 branch. Thank you for
the
review.
Regards,
Brent
RE: [PATCH] ASoC: Intel: bxt-da7219-max98357a: support MAX98390 speaker amp
>
> Support MAX98390 speaker amplifier on cometlake platform. Driver now
> detects amplifier type in the probe function and installs corresponding
> controls and DAPM widgets/routes in the late_probe function.
>
> Signed-off-by: Brent Lu
This patch is from Chrome-v4.19 branch to support the combination of DA7219
and MAX98390 on a CML Chromebook. It reuses the topology file
sof-cml-da7219-max98357a.tplg
> ---
> sound/soc/intel/boards/Kconfig| 20
> sound/soc/intel/boards/bxt_da7219_max98357a.c | 129
> --
> sound/soc/intel/common/soc-acpi-intel-cml-match.c | 4 +-
> 3 files changed, 139 insertions(+), 14 deletions(-)
>
> diff --git a/sound/soc/intel/boards/Kconfig
> b/sound/soc/intel/boards/Kconfig index 3d820e1..b3b863e 100644
> --- a/sound/soc/intel/boards/Kconfig
> +++ b/sound/soc/intel/boards/Kconfig
> @@ -291,9 +291,17 @@ config
> SND_SOC_INTEL_DA7219_MAX98357A_GENERIC
> select SND_SOC_DMIC
> select SND_SOC_HDAC_HDMI
>
> +config SND_SOC_INTEL_DA7219_MAX98390_GENERIC
> + tristate
> + select SND_SOC_DA7219
> + select SND_SOC_MAX98390
> + select SND_SOC_DMIC
> + select SND_SOC_HDAC_HDMI
> +
> config SND_SOC_INTEL_BXT_DA7219_MAX98357A_COMMON
> tristate
> select SND_SOC_INTEL_DA7219_MAX98357A_GENERIC
> + select SND_SOC_INTEL_DA7219_MAX98390_GENERIC
>
> if SND_SOC_INTEL_APL
>
> @@ -309,6 +317,18 @@ config
> SND_SOC_INTEL_BXT_DA7219_MAX98357A_MACH
> Say Y or m if you have such a device. This is a recommended option.
> If unsure select "N".
>
> +config SND_SOC_INTEL_BXT_DA7219_MAX98390_MACH
> + tristate "Broxton with DA7219 and MAX98390 in I2S Mode"
> + depends on I2C && ACPI && GPIOLIB
> + depends on MFD_INTEL_LPSS || COMPILE_TEST
> + depends on SND_HDA_CODEC_HDMI
> + select SND_SOC_INTEL_BXT_DA7219_MAX98357A_COMMON
> + help
> +This adds support for ASoC machine driver for Broxton-P platforms
> +with DA7219 + MAX98390 I2S audio codec.
> +Say Y or m if you have such a device. This is a recommended option.
> +If unsure select "N".
> +
> config SND_SOC_INTEL_BXT_RT298_MACH
> tristate "Broxton with RT298 I2S mode"
> depends on I2C && ACPI && GPIOLIB
> diff --git a/sound/soc/intel/boards/bxt_da7219_max98357a.c
> b/sound/soc/intel/boards/bxt_da7219_max98357a.c
> index 44016c1..12f07e1 100644
> --- a/sound/soc/intel/boards/bxt_da7219_max98357a.c
> +++ b/sound/soc/intel/boards/bxt_da7219_max98357a.c
> @@ -25,9 +25,14 @@
>
> #define BXT_DIALOG_CODEC_DAI "da7219-hifi"
> #define BXT_MAXIM_CODEC_DAI "HiFi"
> +#define MAX98390_DEV0_NAME "i2c-MX98390:00"
> +#define MAX98390_DEV1_NAME "i2c-MX98390:01"
> #define DUAL_CHANNEL 2
> #define QUAD_CHANNEL 4
>
> +#define SPKAMP_MAX98357A 1
> +#define SPKAMP_MAX98390 2
> +
> static struct snd_soc_jack broxton_headset; static struct snd_soc_jack
> broxton_hdmi[3];
>
> @@ -40,6 +45,7 @@ struct bxt_hdmi_pcm {
> struct bxt_card_private {
> struct list_head hdmi_pcm_list;
> bool common_hdmi_codec_drv;
> + int spkamp;
> };
>
> enum {
> @@ -85,13 +91,20 @@ static int platform_clock_control(struct
> snd_soc_dapm_widget *w, static const struct snd_kcontrol_new
> broxton_controls[] = {
> SOC_DAPM_PIN_SWITCH("Headphone Jack"),
> SOC_DAPM_PIN_SWITCH("Headset Mic"),
> +};
> +
> +static const struct snd_kcontrol_new max98357a_controls[] = {
> SOC_DAPM_PIN_SWITCH("Spk"),
> };
>
> +static const struct snd_kcontrol_new max98390_controls[] = {
> + SOC_DAPM_PIN_SWITCH("Left Spk"),
> + SOC_DAPM_PIN_SWITCH("Right Spk"),
> +};
> +
> static const struct snd_soc_dapm_widget broxton_widgets[] = {
> SND_SOC_DAPM_HP("Headphone Jack", NULL),
> SND_SOC_DAPM_MIC("Headset Mic", NULL),
> - SND_SOC_DAPM_SPK("Spk", NULL),
> SND_SOC_DAPM_MIC("SoC DMIC", NULL),
> SND_SOC_DAPM_SPK("HDMI1", NULL),
> SND_SOC_DAPM_SPK("HDMI2", NULL),
> @@ -100,14 +113,20 @@ static const struct snd_soc_dapm_widget
> broxton_widgets[] = {
> platform_clock_control,
> SND_SOC_DAPM_POST_PMD|SND_SOC_DAPM_PRE_PMU),
> };
>
> +static const struct snd_soc_dapm_widget max98357a_widgets[] = {
> + SND_SOC_DAPM_SPK("Spk", NULL),
> +};
> +
> +static const struct snd_soc_dapm_widget max98390_widgets[] = {
> + SND_SOC_DAPM_SPK("Left Spk", NULL),
> + SND_SOC_DAPM_SPK("Right Spk", NULL),
> +};
> +
> static const struct snd_soc_dapm_route audio_map[] = {
> /* HP jack connectors - unknown if we have jack detection */
> {"Headphone Jack", NULL, "HPL"},
> {"Headphone Jack", NULL, "HPR"},
>
> - /* speaker */
> - {"Spk", NULL, "Speaker"},
> -
> /* other jacks */
> {"MIC", NULL, "Headset Mic"},
>
> @@ -134,6 +153,17 @@ static const struct snd_soc_dapm_route audio_map[]
> = {
> { "Headset Mic", NULL, "Platform Clock" }, };
>
> +static const struct
RE: [PATCH v2] ASoC: SOF: Intel: hda: unsolicited RIRB response
> > better to align the logic, so I'm ok to take this patch to SOF. > Can you fix the summary though: > > - ASoC: SOF: Intel: hda: unsolicited RIRB response > + ASoC: SOF: Intel: hda: Clear RIRB status before reading WP > > Br, Kai A v3 patch is uploaded. Thanks for review. Regards, Brent
RE: [PATCH] ASoC: SOF: Intel: hda: unsolicited RIRB response
> > Now I noticed that the legacy driver already addressed it recently via commit > 6d011d5057ff > ALSA: hda: Clear RIRB status before reading WP > > We should have checked SOF at the same time, too... > > > thanks, > > Takashi Hi Takashi-san, Yes you are correct. I tested Chrome v5.4 on a CML Chromebook 'hatch' and realize the SOF does no suffer from this issue because the 'sync write' feature is enabled in hda_init. Soon I can reproduce the issue after turning it off. So I think it's still worthy to have this fix in case we need to disable 'sync write' someday. Regards, Brent
RE: [PATCH] ASoC: SOF: Intel: hda: unsolicited RIRB response
> > IIRC we added this loop before merging all interrupt handling in one thread, > somehow the MSI mode never worked reliably without this change, so > maybe we don't need this loop any longer. > > I'd really prefer it if we didn't tie the RIRB handing change to this loop > change, > removing the loop should only be done with *a lot of testing*. The reason I removed the loop because I thought it's for the unsolicited response, apparently it's not. I'd like to port the commit 6d011d5057ff ALSA: hda: Clear RIRB status before reading WP to SOF driver and upload a version 2. Thanks. Regards, Brent
RE: [PATCH] ASoC: SOF: Intel: hda: unsolicited RIRB response
> Hi Brent, > > Thanks for the patch. Is this fix for a specific issue you're seeing? > If so, could you please give us some details about it? > > Thanks, > Ranjani Hi Ranjani, It's reported to happen on GLK Chromebook 'Fleex' that sometimes it cannot output the audio stream to external display. The kernel is Chrome v4.14 branch. Following is the reproduce step provided by ODM but I could reproduce it simply running aplay or cras_test_client so I think it's not about the cable plug/unplug handling. What steps will reproduce the problem? 1. Play YouTube video on Chromebook and connect it to external monitor with Type C to DP dongle 2. Press monitor power button to turn off the monitor 3. Press monitor power button again to turn on the monitor 4. Continue to play YouTube video and check audio playback 5. No sound comes out from built-in speaker of external monitor when turn on external monitor I added debug messages to print the RIRBWP register and realize that response could come between the read of RIRBWP in the snd_hdac_bus_update_rirb() function and the interrupt clear in the hda_dsp_stream_interrupt() function. The response is not handled but the interrupt is already cleared. It will cause timeout unless more responses coming to RIRB. [ 69.173507] sof-audio-pci :00:0e.0: snd_hdac_bus_get_response: addr 0x2 [ 69.173567] sof-audio-pci :00:0e.0: snd_hdac_bus_update_rirb: cmds 1 res 0 rp 21 wp 21 => handle the response in slot 21 [ 69.173570] sof-audio-pci :00:0e.0: snd_hdac_bus_update_rirb: updated wp 22 => new response in slot 22 but not handled [ 70.174089] sof-audio-pci :00:0e.0: snd_hdac_bus_get_response: timeout, wp 22 [ 70.174106] HDMI HDA Codec ehdaudio0D2: codec_read: fail to read codec I found there is a commit addressing this issue and cherry-pick it to the Chrome v4.14 but the issue is still there. I think more loop does not help because eventually there will be response coming in the snd_hdac_bus_update_rirb() function and become unhandled response in the last loop. commit 6297a0dc4c14a62bea5a9137ceef280cb7a80665 Author: Ranjani Sridharan Date: Wed Jun 12 12:23:40 2019 -0500 ASoC: SOF: Intel: hda: modify stream interrupt handler Modify the stream interrupt handler to always wake up the IRQ thread if the status register is valid. The IRQ thread performs the check for stream interrupts and RIRB interrupts in a loop to handle the case of missed interrupts when an unsolicited response from the codec is received just before the stream interrupt handler is completed. Regards, Brent
RE: [PATCH] ALSA: pcm: fix incorrect hw_base increase
> > I tried to imagine a negative impact for this hw_ptr_jiffies update when the > DMA position is not updated from the driver and I haven't found any so far. > > Let's apply this and we'll see in future :-) > > And yes, the patch description should be improved (DMA ptr is not updated / > streaming is inactive). > > Reviewed-by: Jaroslav Kysela Hi Jaroslav and Takashi, Thank you for the review. Patch V2 has been uploaded. Regards, Brent > > > > > > > thanks, > > > > Takashi > > > > > -- > Jaroslav Kysela > Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
RE: [PATCH] ALSA: pcm: fix incorrect hw_base increase
> > Updating hw_ptr_jiffies at that code path looks correct, but it still leaves > the > question why this condition happens. It means that the actual hwptr isn't > changed and yet only jiffies increase significantly; it means that the > hardware > can't report proper pointer, and it should have set > SNDRV_PCM_INFO_BATCH flag, then the jiffies check is skipped. > > With which hardware and under which situation did it happen (and the patch > fixed)? > > > thanks, > > Takashi > >From time to time we got questions from google about why sometimes the snd_pcm_avail() returns a value larger than buffer size. Recently we finally found reliable reproduce steps: it's on Intel GLK Chromebook Fleex with SOF firmware. There is a 1/20 chance the audio playback to HDMI fails. >From the FW side we observe a buffer runderrun, the FW is not able to recover it for some reason and stops the pipe. >From the Linux side we see the pos does not increase because the FW stops receiving data but suddenly the hw_prt is increased by buffer_size (16368). It could make snd_pcm_avail() reporting a false underrun to the caller like following log: 2020-05-09T16:08:32.712042+08:00 DEBUG kernel: [ 418.510086] sound pcmC0D5p: pos 96 hw_ptr 96 appl_ptr 4096 avail 12368 2020-05-09T16:08:32.712043+08:00 DEBUG kernel: [ 418.510149] sound pcmC0D5p: pos 96 hw_ptr 96 appl_ptr 6910 avail 9554 ... 2020-05-09T16:08:32.883095+08:00 DEBUG kernel: [ 418.680868] sound pcmC0D5p: pos 96 hw_ptr 96 appl_ptr 15102 avail 1362 2020-05-09T16:08:32.883104+08:00 DEBUG kernel: [ 418.681052] sound pcmC0D5p: pos 96 hw_ptr 96 appl_ptr 15102 avail 1362 2020-05-09T16:08:32.883109+08:00 DEBUG kernel: [ 418.681130] sound pcmC0D5p: pos 96 hw_ptr 96 appl_ptr 16464 avail 0 2020-05-09T16:08:32.929330+08:00 DEBUG kernel: [ 418.726515] sound pcmC0D5p: pos 96 hw_ptr 16464 appl_ptr 16464 avail 16368 2020-05-09T16:08:32.929512+08:00 DEBUG kernel: [ 418.727041] sound pcmC0D5p: pos 96 hw_ptr 16464 appl_ptr 16464 avail 16368 Or it could make snd_pcm_avail() returns an invalid number and confuses the Caller like following log: 2020-05-09T16:08:33.054039+08:00 DEBUG kernel: [ 418.851755] sound pcmC0D5p: pos 96 hw_ptr 16464 appl_ptr 27390 avail 5442 2020-05-09T16:08:33.129552+08:00 DEBUG kernel: [ 418.926491] sound pcmC0D5p: pos 96 hw_ptr 32832 appl_ptr 27390 avail 21810 2020-05-09T16:08:33.131746+08:00 ERR cras_server[1907]: pcm_avail returned frames larger than buf_size: sof-glkda7219max: :0,5: 21810 > 16368 I've submitted a patch to fix the issue in SOF side. However, I think it's also good to fix the incorrect hw_base increasement in Linux side. https://github.com/thesofproject/sof/pull/2926 Regards, Brent
RE: [PATCH] ALSA: pcm: fix incorrect hw_base increase
> > Is this a bugfix needed for older kernels as well? When did this issue show > up? > > thanks, > > greg k-h It happens when DMA stop moving data from host to DSP/DAI for a long time (> half of buffer time). I know host driver should do something about it. But if not, the HWSYNC will keep increasing the hw_base and hw_ptr and confuses user space program. Regards, Brent
RE: [PATCH] ASoC: Intel: sst: ipc command timeout
> > Looks there is race between the previous stream stop and the current > stream start here. Can you help try changing the > sst_byt_stream_start/stop() from 'nowait' mode to 'wait' mode, and see if > the issue can be reproduced with it? Hi Keyon, Kernel panic if the mode is changed. The defect could happen to the first ALLOC_STREAM command after bootstrap so I think the START/DROP_STRAM may not work. Regards, Brent > > > Thanks, > ~Keyon >
RE: [PATCH] ASoC: Intel: sst: ipc command timeout
>
> Hi,
>
> That's why I would suggest trying with readq, it should also generate one
> instruction read x86_64 platforms, I looked a bit more and there is fallback
> to
> generate two 32 bit reads on 32bit platforms, so my previous concern about
> having to write separate handling for those platforms was unneeded. So I
> would recommend checking using it.
Hi,
The readq/writeq works. Code is optimized unlike memcpy_fromio/memcpy_toio
and the defect is not able to reproduce.
(gdb) disas sst_shim32_read64
Dump of assembler code for function sst_shim32_read64:
0x096c <+0>: call 0x971
0x0971 <+5>: push rbp
0x0972 <+6>: movrbp,rsp
0x0975 <+9>: moveax,esi
0x0977 <+11>:movrax,QWORD PTR [rdi+rax*1]
0x097b <+15>:poprbp
0x097c <+16>:ret
End of assembler dump.
(gdb) disas sst_shim32_write64
Dump of assembler code for function sst_shim32_write64:
0x095b <+0>: call 0x960
0x0960 <+5>: push rbp
0x0961 <+6>: movrbp,rsp
0x0964 <+9>: moveax,esi
0x0966 <+11>:movQWORD PTR [rdi+rax*1],rdx
0x096a <+15>:poprbp
0x096b <+16>:ret
End of assembler dump.
Regards,
Brent
>
> diff --git a/sound/soc/intel/common/sst-dsp.c
> b/sound/soc/intel/common/sst-dsp.c
> index ec66be269b69..e96f636387d9 100644
> --- a/sound/soc/intel/common/sst-dsp.c
> +++ b/sound/soc/intel/common/sst-dsp.c
> @@ -34,16 +34,13 @@ EXPORT_SYMBOL_GPL(sst_shim32_read);
>
> void sst_shim32_write64(void __iomem *addr, u32 offset, u64 value)
> {
> - memcpy_toio(addr + offset, , sizeof(value));
> + writeq(value, addr + offset);
> }
> EXPORT_SYMBOL_GPL(sst_shim32_write64);
>
> u64 sst_shim32_read64(void __iomem *addr, u32 offset)
> {
> - u64 val;
> -
> - memcpy_fromio(, addr + offset, sizeof(val));
> - return val;
> + return readq(addr + offset);
> }
> EXPORT_SYMBOL_GPL(sst_shim32_read64);
>
>
> Thanks,
> Amadeusz
RE: [PATCH] ASoC: Intel: sst: ipc command timeout
> > Hi, > yes that seems bit weird. It is bit better as it does not modify common code, > but still... Maybe going back to your original idea of replacing memcpy, try > replacing it with readq? It should generate one instruction read (although it > is > only for x64_64, for 32 bit kernel we would still need to do something else). > > Thanks, > Amadeusz Hi, I've compared the assembly to see if there is clue. Both kernels are using 64-bit mov to read register and the only difference is optimized or not. Both implementations are looking good to me. Currently I don't have answer why slower kernel hits the problem while optimized one survived. 1. Old kernel. Code is optimized and not able to reproduce the issue on this kernel. (gdb) disas sst_shim32_read64 Dump of assembler code for function sst_shim32_read64: 0x096c <+0>: call 0x971 => call __fentry__ 0x0971 <+5>: push rbp 0x0972 <+6>: movrbp,rsp 0x0975 <+9>: moveax,esi 0x0977 <+11>:movrax,QWORD PTR [rdi+rax*1] => perform 64-bit mov 0x097b <+15>:poprbp 0x097c <+16>:ret End of assembler dump. 2. New kernel: obviously optimization is disabled and it calls memcpy to do the read operation. (gdb) disas sst_shim32_read64 Dump of assembler code for function sst_shim32_read64: 0x09a8 <+0>: call 0x9ad => call __fentry__ 0x09ad <+5>: push rbp 0x09ae <+6>: movrbp,rsp 0x09b1 <+9>: push rbx 0x09b2 <+10>:subrsp,0x10 0x09b6 <+14>:movrax,QWORD PTR gs:0x28 0x09bf <+23>:movQWORD PTR [rbp-0x10],rax 0x09c3 <+27>:movabs rax,0x 0x09cd <+37>:learbx,[rbp-0x18] 0x09d1 <+41>:movQWORD PTR [rbx],rax 0x09d4 <+44>:movesi,esi 0x09d6 <+46>:addrsi,rdi 0x09d9 <+49>:movedx,0x8 0x09de <+54>:movrdi,rbx 0x09e1 <+57>:call 0x9e6 => call memcpy The memcpy is implemented in arch/x86/lib/memcpy_64.S (gdb) disas memcpy Dump of assembler code for function memcpy: 0x813519c0 <+0>: jmp0x813519f0 => jump to memcpy_orig function X86_FEATURE_ERMS is disabled so it jumps to memcpy_orig (gdb) disas memcpy_orig Dump of assembler code for function memcpy_orig: 0x813519f0 <+0>: movrax,rdi 0x813519f3 <+3>: cmprdx,0x20 0x813519f7 <+7>: jb 0x81351a77 => jump because our read size is 8 ... 0x81351a77 <+135>: cmpedx,0x10 0x81351a7a <+138>: jb 0x81351aa0 => jump because our read size is 8 ... 0x81351aa0 <+176>: cmpedx,0x8 0x81351aa3 <+179>: jb 0x81351ac0 0x81351aa5 <+181>: movr8,QWORD PTR [rsi] 0x81351aa8 <+184>: movr9,QWORD PTR [rsi+rdx*1-0x8] 0x81351aad <+189>: movQWORD PTR [rdi],r8 0x81351ab0 <+192>: movQWORD PTR [rdi+rdx*1-0x8],r9 => perform 64-bit mov twice over same address (rdx=0x8) 0x81351ab5 <+197>: ret Regards, Brent
RE: [PATCH] ASoC: Intel: sst: ipc command timeout
>
> I've looked at the code and byt_is_dsp_busy seems suspicious to me.
> Can you check if following change fixes problem for you:
>
> diff --git a/sound/soc/intel/baytrail/sst-baytrail-ipc.c
> b/sound/soc/intel/baytrail/sst-baytrail-ipc.c
> index 74274bd38f7a..34746fd871b0 100644
> --- a/sound/soc/intel/baytrail/sst-baytrail-ipc.c
> +++ b/sound/soc/intel/baytrail/sst-baytrail-ipc.c
> @@ -666,8 +666,8 @@ static bool byt_is_dsp_busy(struct sst_dsp *dsp)
> {
> u64 ipcx;
>
> - ipcx = sst_dsp_shim_read_unlocked(dsp, SST_IPCX);
> - return (ipcx & (SST_IPCX_BUSY | SST_IPCX_DONE));
> + ipcx = sst_dsp_shim_read64_unlocked(dsp, SST_IPCX);
> + return (ipcx & (SST_BYT_IPCX_BUSY | SST_BYT_IPCX_DONE));
> }
>
> int sst_byt_dsp_init(struct device *dev, struct sst_pdata *pdata)
>
> We seem to treat SST_IPCX as 32 bit register instead of 64 one, which may
> explain wrong behaviour. (Specification says it is 64 bit register).
>
> Thanks,
> Amadeusz
Hi Amadeusz,
The patch does not work but I managed to create a workaround just for
reference. Still don't know why first read in sst_byt_irq_thread returns
incorrect value.
diff --git a/sound/soc/intel/baytrail/sst-baytrail-ipc.c
b/sound/soc/intel/baytrail/sst-baytrail-ipc.c
index 5bbaa667bec1..56c557cb722d 100644
--- a/sound/soc/intel/baytrail/sst-baytrail-ipc.c
+++ b/sound/soc/intel/baytrail/sst-baytrail-ipc.c
@@ -12,6 +12,7 @@
* more details.
*/
+#define DEBUG
#include
#include
#include
@@ -313,7 +314,7 @@ static irqreturn_t sst_byt_irq_thread(int irq, void
*context)
struct sst_dsp *sst = (struct sst_dsp *) context;
struct sst_byt *byt = sst_dsp_get_thread_context(sst);
struct sst_generic_ipc *ipc = >ipc;
- u64 header;
+ u64 header, mask, old, new;
unsigned long flags;
spin_lock_irqsave(>spinlock, flags);
@@ -332,10 +333,32 @@ static irqreturn_t sst_byt_irq_thread(int irq, void
*context)
* processed the message and can accept new. Clear data part
* of the header
*/
- sst_dsp_shim_update_bits64_unlocked(sst, SST_IPCD,
- SST_BYT_IPCD_DONE | SST_BYT_IPCD_BUSY |
- IPC_HEADER_DATA(IPC_HEADER_DATA_MASK),
- SST_BYT_IPCD_DONE);
+ /* inline the sst_dsp_shim_update_bits64_unlocked function */
+ mask = SST_BYT_IPCD_DONE | SST_BYT_IPCD_BUSY |
+ IPC_HEADER_DATA(IPC_HEADER_DATA_MASK);
+ old = sst_dsp_shim_read64_unlocked(sst, SST_IPCD);
+ new = (old & (~mask)) | (SST_BYT_IPCD_DONE & mask);
+
+ if (old != new)
+ sst_dsp_shim_write64_unlocked(sst, SST_IPCD, new);
+
+ /*
+* workaround, give it a second chance if the SST_IPCD
+* changes
+*/
+ if (old != header) {
+ dev_dbg(ipc->dev, "%s: header 0x%llx old 0x%llx\n",
+ __func__, header, old);
+ if (old & SST_BYT_IPCD_BUSY) {
+ if (old & IPC_NOTIFICATION) {
+ /* message from ADSP */
+ sst_byt_process_notification(byt,
);
+ } else {
+ /* reply from ADSP */
+ sst_byt_process_reply(byt, old);
+ }
+ }
+ }
/* unmask message request interrupts */
sst_dsp_shim_update_bits64_unlocked(sst, SST_IMRX,
SST_BYT_IMRX_REQUEST, 0);
RE: [PATCH] ASoC: bdw-rt5677: enable runtime channel merge
> Subject: [PATCH] ASoC: bdw-rt5677: enable runtime channel merge > > In the DAI link "Capture PCM", the FE DAI "Capture Pin" supports 4-channel > capture but the BE DAI supports only 2-channel capture. To fix the channel > mismatch, we need to enable the runtime channel merge for this DAI link. > Hi Pierre, This patch is for the same issue discussed in the following thread: https://patchwork.kernel.org/patch/11134167/ We enable the runtime channel merge for the DMIC DAI instead of adding a machine driver constraint. It's working good on chrome's 3.14 branch (which requires some backport for the runtime channel merge feature). Please let me know if this implementation is correct for the FE/BE mismatch problem. Thanks. Regards, Brent
RE: [alsa-devel] [PATCH] ASoC: bdw-rt5677: channel constraint support
> > > > It's not only the mismatch but also the design limitation. According > > to the information from google, the board (samus) only uses two > > microphone so > > 3 or 4 channel recording are not supported. That's the reason we > > leverage the constraint from other machine driver (like > > kbl_da7219_max98357a.c) to remove the 3 and 4 channel recording option. > > The design limitation is already handled by the fact that the SSP operates in > 2ch mode, so it's a different case from KBL where indeed the DMIC-based > back-end can support 4 channels. > > > > > The difference after the constraint is implemented is that the > > snd_pcm_hw_params_set_channels() function will return error (Invalid > > argument) when channel number is 3 or 4 so the application knows the > > configuration is not supported. > > I get the error, I am just wondering if the fix is at the right location. > I'll look > into it, give me until tomorrow. I think I got your point. I cherry-pick these commits back to v3.14 branch to fix the FE/BE mismatch without adding constraint in machine driver. Thanks. b073ed4e ASoC: soc-pcm: DPCM cares BE format f4c277b8 ASoC: soc-pcm: DPCM cares BE channel constraint 4f2bd18b ASoC: dpcm: extend channel merging to the backend cpu dai 435ffb76 ASoC: dpcm: rework runtime stream merge baacd8d1 ASoC: dpcm: add rate merge to the BE stream merge
RE: [alsa-devel] [PATCH] ASoC: bdw-rt5677: channel constraint support
> > > > The story is Chrome has a tool called alsa_conformance_test which runs > > capture or playback against a PCM port with all possible > > configurations (channel, format, rate) then measure if the sample rate > > is correct. Since the channel max number reported is 4, it tests the > > 4-channel 48K capture and reports the actual sample rate is 24000 > > instead of 48000. That's the reason we want to add a constraint in > > machine driver to avoid user space programs trying to do 4 channel > recording since this machine does not support it in the beginning. > > ok, that helps get context, thanks for the details. > > I would have expected some error to be returned if there's a front-end > opened with 4 channels and the back-end only supports two. Adding the > constraint seems like a work-around to avoid dealing with the mismatch > between FE and BE. I don't understand DPCM enough to suggest an > alternative though. Ranjani, can you help on this one? > > And even if we agree with this solution, it'd be nice to apply it for the > Broadwell machine driver for consistency. It's not only the mismatch but also the design limitation. According to the information from google, the board (samus) only uses two microphone so 3 or 4 channel recording are not supported. That's the reason we leverage the constraint from other machine driver (like kbl_da7219_max98357a.c) to remove the 3 and 4 channel recording option. The difference after the constraint is implemented is that the snd_pcm_hw_params_set_channels() function will return error (Invalid argument) when channel number is 3 or 4 so the application knows the configuration is not supported. Regards, Brent
RE: [alsa-devel] [PATCH] ASoC: bdw-rt5677: channel constraint support
> -Original Message-
> From: Pierre-Louis Bossart [mailto:pierre-louis.boss...@linux.intel.com]
> Sent: Tuesday, September 10, 2019 1:53 AM
> To: Lu, Brent ; alsa-de...@alsa-project.org
> Cc: Rojewski, Cezary ;
> kuninori.morimoto...@renesas.com; linux-kernel@vger.kernel.org;
> yang@linux.intel.com; ti...@suse.com; liam.r.girdw...@linux.intel.com;
> broo...@kernel.org; t...@linutronix.de
> Subject: Re: [alsa-devel] [PATCH] ASoC: bdw-rt5677: channel constraint
> support
>
> Please don't top-post on public mailing lists.
>
> > We are working on a backport 3.14 branch for Chrome projects based on
> BDW platform. In the branch 4-channel capture is supported on some
> platforms but not all. So we need to add a constraint in the machine driver
> for machines don't support this feature.
> >
> > I checked the for-next branch in the broonie repo. The channels_max of
> HSW_PCM_DAI_ID_SYSTEM is 4 for capture stream so I think it would have
> same issue like google's backport tree. I didn't find any constraint for this
> dai.
> Would you point out where it is?
> >
> > .capture = {
> > .stream_name = "Analog Capture",
> > .channels_min = 2,
> > .channels_max = 4,
> > .rates = SNDRV_PCM_RATE_48000,
> > .formats = SNDRV_PCM_FMTBIT_S24_LE |
> SNDRV_PCM_FMTBIT_S16_LE,
> > },
>
> ok, I missed this capture case indeed, but when I look at the Chrome
> 3.14 code I don't see this constraint being added, and we already have an
> ssp0_fixup function where 2 channels only are used.
>
> /* The ADSP will covert the FE rate to 48k, stereo */
> rate->min = rate->max = 48000;
> channels->min = channels->max = 2;
Yes I noticed it, but the channel max number returned by
snd_pcm_hw_params_any()
is still 4 for the port "Capture PCM" so I add a constraint to the FE dai.
>
> I also don't see any case where we support 4 channels in any broadwell
> machine driver?
It's the bdw-rt5650.c which only exists in chrome's 3.14 branch supporting
Buddy
project. They submitted the machine driver but not yet merged.
https://patchwork.kernel.org/patch/11050985/
>
> So again can you point me to an issue or existing backport that requires the
> patch below. Not trying to be obtuse but we should only change older
> platforms when there is evidence that a change is needed.
The story is Chrome has a tool called alsa_conformance_test which runs capture
or
playback against a PCM port with all possible configurations (channel, format,
rate)
then measure if the sample rate is correct. Since the channel max number
reported
is 4, it tests the 4-channel 48K capture and reports the actual sample rate is
24000
instead of 48000. That's the reason we want to add a constraint in machine
driver to
avoid user space programs trying to do 4 channel recording since this machine
does
not support it in the beginning.
>
> -Pierre
>
> >
> > Regards,
> > Brent Lu
> >
> > -Original Message-
> > From: Pierre-Louis Bossart
> > [mailto:pierre-louis.boss...@linux.intel.com]
> > Sent: Friday, September 6, 2019 10:21 PM
> > To: Lu, Brent ; alsa-de...@alsa-project.org
> > Cc: Rojewski, Cezary ;
> > liam.r.girdw...@linux.intel.com; yang@linux.intel.com;
> > broo...@kernel.org; pe...@perex.cz; ti...@suse.com;
> > kuninori.morimoto...@renesas.com; t...@linutronix.de;
> > linux-kernel@vger.kernel.org
> > Subject: Re: [alsa-devel] [PATCH] ASoC: bdw-rt5677: channel constraint
> > support
> >
> > On 9/5/19 8:24 PM, Brent Lu wrote:
> >> BDW boards using this machine driver supports only stereo capture and
> >> playback. Implement a constraint to enforce it.
> >
> > Humm, can you clarify what problem/error this patch fixes?
> >
> > There are already constraints on the hsw_dais[] where the channels are
> stereo only.
> >
> > Thanks
> > -Pierre
> >
> >>
> >> Signed-off-by: Brent Lu
> >> ---
> >>sound/soc/intel/boards/bdw-rt5677.c | 33
> +
> >>1 file changed, 33 insertions(+)
> >>
> >> diff --git a/sound/soc/intel/boards/bdw-rt5677.c
> >> b/sound/soc/intel/boards/bdw-rt5677.c
> >> index 4a4d335..a312b55 100644
> >> --- a/sound/soc/intel/boards/bdw-rt5677.c
> >> +++ b/sound/soc/intel/boards/bdw-rt5677.c
> >> @@ -22,6 +22,8 @@
> >>
> >>#include "../../codecs/rt5677.h"
> >>
> >> +#define DUAL_CHANNEL 2
> >> +
>
RE: [alsa-devel] [PATCH] ASoC: bdw-rt5677: channel constraint support
Hi Pierre
We are working on a backport 3.14 branch for Chrome projects based on BDW
platform. In the branch 4-channel capture is supported on some platforms but
not all. So we need to add a constraint in the machine driver for machines
don't support this feature.
I checked the for-next branch in the broonie repo. The channels_max of
HSW_PCM_DAI_ID_SYSTEM is 4 for capture stream so I think it would have same
issue like google's backport tree. I didn't find any constraint for this dai.
Would you point out where it is?
.capture = {
.stream_name = "Analog Capture",
.channels_min = 2,
.channels_max = 4,
.rates = SNDRV_PCM_RATE_48000,
.formats = SNDRV_PCM_FMTBIT_S24_LE |
SNDRV_PCM_FMTBIT_S16_LE,
},
Regards,
Brent Lu
-Original Message-
From: Pierre-Louis Bossart [mailto:pierre-louis.boss...@linux.intel.com]
Sent: Friday, September 6, 2019 10:21 PM
To: Lu, Brent ; alsa-de...@alsa-project.org
Cc: Rojewski, Cezary ;
liam.r.girdw...@linux.intel.com; yang@linux.intel.com; broo...@kernel.org;
pe...@perex.cz; ti...@suse.com; kuninori.morimoto...@renesas.com;
t...@linutronix.de; linux-kernel@vger.kernel.org
Subject: Re: [alsa-devel] [PATCH] ASoC: bdw-rt5677: channel constraint support
On 9/5/19 8:24 PM, Brent Lu wrote:
> BDW boards using this machine driver supports only stereo capture and
> playback. Implement a constraint to enforce it.
Humm, can you clarify what problem/error this patch fixes?
There are already constraints on the hsw_dais[] where the channels are stereo
only.
Thanks
-Pierre
>
> Signed-off-by: Brent Lu
> ---
> sound/soc/intel/boards/bdw-rt5677.c | 33 +
> 1 file changed, 33 insertions(+)
>
> diff --git a/sound/soc/intel/boards/bdw-rt5677.c
> b/sound/soc/intel/boards/bdw-rt5677.c
> index 4a4d335..a312b55 100644
> --- a/sound/soc/intel/boards/bdw-rt5677.c
> +++ b/sound/soc/intel/boards/bdw-rt5677.c
> @@ -22,6 +22,8 @@
>
> #include "../../codecs/rt5677.h"
>
> +#define DUAL_CHANNEL 2
> +
> struct bdw_rt5677_priv {
> struct gpio_desc *gpio_hp_en;
> struct snd_soc_component *component; @@ -245,6 +247,36 @@ static
> int bdw_rt5677_init(struct snd_soc_pcm_runtime *rtd)
> return 0;
> }
>
> +static const unsigned int channels[] = {
> + DUAL_CHANNEL,
> +};
> +
> +static const struct snd_pcm_hw_constraint_list constraints_channels = {
> + .count = ARRAY_SIZE(channels),
> + .list = channels,
> + .mask = 0,
> +};
> +
> +static int bdw_fe_startup(struct snd_pcm_substream *substream) {
> + struct snd_pcm_runtime *runtime = substream->runtime;
> +
> + /*
> + * On this platform for PCM device we support,
> + * stereo
> + */
> +
> + runtime->hw.channels_max = DUAL_CHANNEL;
> + snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS,
> +_channels);
> +
> + return 0;
> +}
> +
> +static const struct snd_soc_ops bdw_rt5677_fe_ops = {
> + .startup = bdw_fe_startup,
> +};
> +
> /* broadwell digital audio interface glue - connects codec <--> CPU */
> SND_SOC_DAILINK_DEF(dummy,
> DAILINK_COMP_ARRAY(COMP_DUMMY()));
> @@ -273,6 +305,7 @@ static struct snd_soc_dai_link bdw_rt5677_dais[] = {
> },
> .dpcm_capture = 1,
> .dpcm_playback = 1,
> + .ops = _rt5677_fe_ops,
> SND_SOC_DAILINK_REG(fe, dummy, platform),
> },
>
>
