Created attachment 117270 [details] set the same bclk_m/bclk_n for 675000 as for 337500 System Environment ======= CRB:SawTooth Peak Platform: Broadwell-U CPU: Broadwell U F0 Chipset PCH: Wildcat Point –LP B0 Audio card: ALC286S BIOS: BDW-E1R1.86C.0093.R00.1409071941 Kernel:4.2.0-rc1_drm-intel-nightly Tree: git://anongit.freedesktop.org/drm-intel Branch:Drm-intel-nightly Commit:b42fa27abff5970649ff07b0ce1691f6464097f3 Bug detailed description ======= Play sound clip through DP or HDMI port. The DP output is distorted, sounds like be played in half sample rate; The HDMI output is muted. Analog output is OK. Reproduce Steps ============== 1. boot up 2. assume DP/HDMI playback device is hw:0,3, run $ speaker-test -D hw:0,3 -c 2 -t wav Expected Result ============= Output repeated voice "Front Left; Front Right" Actual Result =========== DP output repeated slow voice "F-r-o-n-t--L-e-f-t, F-r-o-n-t--R-i-g-h-t"; HDMI output cannot be heard.
The issue can be reproduced in X mode and text mode. In normal case, cd_clock is 337500; In abnormal case, cd_clock is 675000; I added a patch that set the same bclk_m/bclk_n value for cd_clock 675000 as for 337500, and the distort disappeared. I suspect the cd_clock was actually 337500 but somehow be passed to audio driver 675000 by mistake.
We can check the value directly using intel_reg read 0x46200 (CDCLK_FREQ is the reg to look for in the bspec); that should give you the cdclk freq from hardware. Maybe the kernel driver is reporting a stale value?
Hi Jesse, This issue is easy to reproduce on one BDW RVP but not easy to reproduce on other platforms. It may not an universal issue, yet we are confused about how incorrect cd_clock be passed to audio. (in normal case cbclk should be 337500 right?) My test result with intel_reg: # ./intel_reg read 0x46200 (0x00046200): 0x000002a2 from Bspec I'm not sure what this value stands for; # ./intel_reg read 0x130040 LCPLL_CTL (0x00130040): 0x48000000 for Bspec it looks like 337.5MHz and X2 CD clock be set? and from dmesg: # dmesg | grep lh [ 1.386837] lh: hda need i915_power?[1] [ 1.386840] lh: cbclk: 675000 my debug patch in kernel, FYI: diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index ca151b4..66ae510 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -820,10 +820,12 @@ static void haswell_set_bclk(struct hda_intel *hda) int cdclk_freq; unsigned int bclk_m, bclk_n; + printk("lh: hda need i915_power?[%d]\n", hda->need_i915_power); if (!hda->need_i915_power) return; cdclk_freq = snd_hdac_get_display_clk(azx_bus(chip)); + printk("lh: cbclk: %d\n", cdclk_freq); switch (cdclk_freq) { case 337500: bclk_m = 16;
This issue is reproduced again on the same BDW RVP with different SSD. and I got the same register value: # ./intel_reg read 0x46200 (0x00046200): 0x000002a2 # ./intel_reg read 0x130040 LCPLL_CTL (0x00130040): 0x48000000
Is this the same as bug 90776?
I think it's different from 90776, the issue is only reproduced on BDW, not on SKL. I have only 1 BDW RVP3 so I'm not sure if it reproduced on other BDW though.
For long time the issue is not reproduced on BDW NUC and BDW WSB platforms, so close it.
I can reproduce this issue on Lenovo Carbon X1 gen3 laptop.
(In reply to Lu, Han from comment #4) > This issue is reproduced again on the same BDW RVP with different SSD. and I > got the same register value: > # ./intel_reg read 0x46200 > (0x00046200): 0x000002a2 > # ./intel_reg read 0x130040 > LCPLL_CTL (0x00130040): 0x48000000 Regs contents are same, snd-hda-intel believes that cd_clock is 675000, setting same bclk_m/bclk_n as for 337500 workarounds issue for me.
Issue happens on both linux-4.4.5 and linux-4.5.1
Created attachment 122889 [details] [review] a patch for diagnosing The patch prints the cdclk received by hda audio
Hi Vasily, The issue is reproduced in our BDW NUC too. I applied the "diagnose.patch" and check the dmesg, the cdclock received by audio changes when issue happens: in normal case, cdclock = 337.5MHz; when issue happened, cdclock = 540MHz. Could you please double check on your platform, through either intel_reg read or dmesg, to see if we observed the same issue? Sorry I have no Lenovo Carbon X1 gen3 laptop on my hand.
Rise the priority as regression.
System Environment CRB:Intel NUC5i3RYH Platform: BroadWell CPU: Hasewell i5-4250U @ 1.30GHz Audio card: ALC283 BIOS:BDW-E1R1.86C.0093.R00.1409071941 Kernel:4.5.0 drm-intel-nightly Tree: git://anongit.freedesktop.org/drm-intel Branch: drm-intel-nightly Commit: 3e5ecc8c5ff80cb1fb635ce1cf16b7cd4cfb1979 Regression? YES Bug detailed description display audio playback speed is very slow after resumed from S3. Reproduce Steps aplay -D plughw:0,3 /$Music/3_WAV_PCM_44100Hz_mono_16bit.wav & aplay -D plughw:0,7 /$Music/18_WAV_48000Hz_stereo_16bit.wav & sleep 3 ; rtcwake -m mem -s 10 Expected Result Display audio playback normally after resumed from S3. Actual Result playback speed is very slow.
(In reply to Lu, Han from comment #14) > Regression? > YES Can you bisect please?
(In reply to Jani Nikula from comment #15) > (In reply to Lu, Han from comment #14) > > Regression? > > YES > > Can you bisect please? We can ask our QA to help bisect. Also could GFX expert involve to give a quick review please? With the diagnose patch, when the issue be reproduced, it looks like the register LCPLL_CTL was not changed but the return value of callback get_display_clock_speed() changed from 337.5M to 540M: 1. no patch, playback OK; (1) ./intel_reg read 0x46200 -> (0x00046200): 0x0000021b (2) ./intel_reg read 0x130040 -> LCPLL_CTL (0x00130040): 0x48000000 (3) dmesg | grep lh -> None 2. no patch, playback slow; (1) ./intel_reg read 0x46200 -> (0x00046200): 0x0000021b (2) ./intel_reg read 0x130040 -> LCPLL_CTL (0x00130040): 0x48000000 (3) dmesg | grep lh -> None 3. apply "diagnose.patch", playback OK; (1) ./intel_reg read 0x46200 -> (0x00046200): 0x0000021b (2) ./intel_reg read 0x130040 -> LCPLL_CTL (0x00130040): 0x48000000 (3) dmesg | grep lh -> cdclk: 337500 4.apply "diagnose.patch", playback slow; (1) ./intel_reg read 0x46200 -> (0x00046200): 0x0000021b (2) ./intel_reg read 0x130040 -> LCPLL_CTL (0x00130040): 0x48000000 (3) dmesg | grep lh -> cdclk: 540000
Created attachment 122916 [details] [review] a patch for diagnosing fix typo on patch
(In reply to Lu, Han from comment #12) > Hi Vasily, > > The issue is reproduced in our BDW NUC too. > I applied the "diagnose.patch" and check the dmesg, the cdclock received by > audio changes when issue happens: > in normal case, cdclock = 337.5MHz; > when issue happened, cdclock = 540MHz. > > Could you please double check on your platform, through either intel_reg > read or dmesg, to see if we observed the same issue? Sorry I have no Lenovo > Carbon X1 gen3 laptop on my hand. Hi Lu, I always get cdclock = 675000. It always fails for me, it doesn't depend on whether system was suspended or not. Probably snd_hdac_get_display_clk() returns incorrect value?
(In reply to Vasily Khoruzhick from comment #18) > (In reply to Lu, Han from comment #12) > > Hi Vasily, > > > > The issue is reproduced in our BDW NUC too. > > I applied the "diagnose.patch" and check the dmesg, the cdclock received by > > audio changes when issue happens: > > in normal case, cdclock = 337.5MHz; > > when issue happened, cdclock = 540MHz. > > > > Could you please double check on your platform, through either intel_reg > > read or dmesg, to see if we observed the same issue? Sorry I have no Lenovo > > Carbon X1 gen3 laptop on my hand. > > Hi Lu, > > I always get cdclock = 675000. It always fails for me, it doesn't depend on > whether system was suspended or not. > > Probably snd_hdac_get_display_clk() returns incorrect value? # intel_reg read 0x130040 (0x00130040): 0x48000000 (0x48000000 & LCPLL_CLK_FREQ_MASK) = 0x8000000 = LCPLL_CLK_FREQ_337_5_BDW How come it returns 675000 from broadwell_get_display_clock_speed()?: https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/drivers/gpu/drm/i915/intel_display.c?h=linux-4.4.y#n6788
Created attachment 122963 [details] [review] [PATCH] drm/i915: Update CDCLK_FREQ register on BDW after changing cdclk frequency I found at least one thing missing from out BDW cdclk code. This should add it.
Created attachment 122964 [details] [review] [PATCH] drm/i915: Use cached cdclk value in i915_audio_component_get_cdclk_freq() This shouldn't actually change anything, but avoids doing a pointless cdclk readout from hw.
(In reply to Ville Syrjala from comment #20) > Created attachment 122963 [details] [review] [review] > [PATCH] drm/i915: Update CDCLK_FREQ register on BDW after changing cdclk > frequency > > I found at least one thing missing from out BDW cdclk code. This should add > it. Doesn't fix the issue. Looks like audio driver uses stale value: [ 2644.305055] broadwell_get_display_clock_speed 4c000000 [ 2644.305057] i915_audio_component_get_cdclk_freq 675000 [ 2644.322469] broadwell_get_display_clock_speed 4c000000 [ 2644.325669] broadwell_get_display_clock_speed 48000000
(In reply to Vasily Khoruzhick from comment #22) > (In reply to Ville Syrjala from comment #20) > > Created attachment 122963 [details] [review] [review] [review] > > [PATCH] drm/i915: Update CDCLK_FREQ register on BDW after changing cdclk > > frequency > > > > I found at least one thing missing from out BDW cdclk code. This should add > > it. > > Doesn't fix the issue. Looks like audio driver uses stale value: > > [ 2644.305055] broadwell_get_display_clock_speed 4c000000 > [ 2644.305057] i915_audio_component_get_cdclk_freq 675000 > [ 2644.322469] broadwell_get_display_clock_speed 4c000000 > [ 2644.325669] broadwell_get_display_clock_speed 48000000 There seems to be a race between i915 and snd-hda-intel. Latter seems to call snd_hdac_get_display_clk() only once, and if i915 didn't set cdclk before the call, snd-hda-intel gets incorrect value. It's easy to enough to reproduce by either loading snd-hda-intel and i915 at the same time (it's the case for Archlinux if i915 is not in initramfs), or by suspending and resuming device.
I'm not so sure that this is a race per se. From the brief look I took at the HDA driver, it seems that the audio code believes that CDCLK stays at a fixed value and thus it only confirms the value during its initial init and when a resume operation occurs. Since CDCLK can be changed at various times on some platforms, it seems like what we need is some sort of a callback mechanism to notify the audio driver when CDCLK gets set to a new value so that they can update their M/N values appropriately. Alternately, although I'd leave this determination to folks that are more familiar with the audio driver, if there's a good spot where they can call snd_hdac_get_display_clk() periodically and update their M/N values if the new value differs from the previous value that they have then that would work too. Thoughts?
(In reply to Jim Bride from comment #24) > I'm not so sure that this is a race per se. From the brief look I took at > the HDA driver, it seems that the audio code believes that CDCLK stays at a > fixed value and thus it only confirms the value during its initial init and > when a resume operation occurs. Since CDCLK can be changed at various times > on some platforms, it seems like what we need is some sort of a callback > mechanism to notify the audio driver when CDCLK gets set to a new value so > that they can update their M/N values appropriately. Alternately, although > I'd leave this determination to folks that are more familiar with the audio > driver, if there's a good spot where they can call > snd_hdac_get_display_clk() periodically and update their M/N values if the > new value differs from the previous value that they have then that would > work too. Thoughts? We have the .pin_eld_notify() hook which could trigger the audio driver to re-query the cdclk. If the port is enabled, cdclk is currently guaranteed to stay at a fixed frequency between the .pin_eld_notify() calls from the port enable and disable.
We have the function to query cdclk frequency because the audio registers em4/5 are lost if the display power domains go to power save. If my memory serves me right the function to query cdclk frequency predates the ability to change cdclk frequency. It's likely the audio driver only queries cdclk whenever the em4/5 registers may have been lost, and we don't communicate cdclk changes. Sadly, there's no dmesg with drm.debug=14 to see if there actually are any cdclk changes in the cases in question, or if that's just another theoretical case I came up in addition! I guess we should also do diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 56ba8765816e..1063108a9bab 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -624,17 +624,11 @@ static void i915_audio_component_codec_wake_override(struct device *dev, static int i915_audio_component_get_cdclk_freq(struct device *dev) { struct drm_i915_private *dev_priv = dev_to_i915(dev); - int ret; if (WARN_ON_ONCE(!HAS_DDI(dev_priv))) return -ENODEV; - intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO); - ret = dev_priv->display.get_display_clock_speed(dev_priv->dev); - - intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO); - - return ret; + return dev_priv->cdclk_freq; } static int i915_audio_component_sync_audio_rate(struct device *dev,
(In reply to Jani Nikula from comment #26) > + return dev_priv->cdclk_freq; > } > > static int i915_audio_component_sync_audio_rate(struct device *dev, Is it initialized early enough to allow audio driver querying a clock?
(In reply to Vasily Khoruzhick from comment #27) > (In reply to Jani Nikula from comment #26) > > > + return dev_priv->cdclk_freq; > > } > > > > static int i915_audio_component_sync_audio_rate(struct device *dev, > > Is it initialized early enough to allow audio driver querying a clock? I think the more interesting question is whether or not the HDA driver is trying to get the CDCLK information too early, which I think it is for the case of rendering sound to an audio output device that is built into an external monitor. Currently the HDA driver is getting CDCLK during it's initial initialization (azx_first_init()) and only updates it when processing a resume event (azx_resume() and azx_runtime_resume().) These three cases make sense to me, but I think there's something missing. Since CDCLK can be changed in the process of initializing a particular port, I wonder if, as Ville suggests, we wouldn't be better off setting the CDCLK-related state (EM4 & EM5) as a part of pin_eld_notify() as well.
(In reply to Jani Nikula from comment #26) > We have the function to query cdclk frequency because the audio registers > em4/5 are lost if the display power domains go to power save. If my memory > serves me right the function to query cdclk frequency predates the ability > to change cdclk frequency. It's likely the audio driver only queries cdclk > whenever the em4/5 registers may have been lost, and we don't communicate > cdclk changes. > > Sadly, there's no dmesg with drm.debug=14 to see if there actually are any > cdclk changes in the cases in question, or if that's just another > theoretical case I came up in addition! > > I guess we should also do > > diff --git a/drivers/gpu/drm/i915/intel_audio.c > b/drivers/gpu/drm/i915/intel_audio.c > index 56ba8765816e..1063108a9bab 100644 > --- a/drivers/gpu/drm/i915/intel_audio.c > +++ b/drivers/gpu/drm/i915/intel_audio.c > @@ -624,17 +624,11 @@ static void > i915_audio_component_codec_wake_override(struct device *dev, > static int i915_audio_component_get_cdclk_freq(struct device *dev) > { > struct drm_i915_private *dev_priv = dev_to_i915(dev); > - int ret; > > if (WARN_ON_ONCE(!HAS_DDI(dev_priv))) > return -ENODEV; > > - intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO); > - ret = dev_priv->display.get_display_clock_speed(dev_priv->dev); > - > - intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO); > - > - return ret; > + return dev_priv->cdclk_freq; > } > > static int i915_audio_component_sync_audio_rate(struct device *dev, That is exactly my patch from comment 21 ;)
(In reply to Vasily Khoruzhick from comment #27) > (In reply to Jani Nikula from comment #26) > > > + return dev_priv->cdclk_freq; > > } > > > > static int i915_audio_component_sync_audio_rate(struct device *dev, > > Is it initialized early enough to allow audio driver querying a clock? It should be set before we expose the component interface for the audio driver query.
(In reply to Jani Nikula from comment #30) > (In reply to Vasily Khoruzhick from comment #27) > > (In reply to Jani Nikula from comment #26) > > > > > + return dev_priv->cdclk_freq; > > > } > > > > > > static int i915_audio_component_sync_audio_rate(struct device *dev, > > > > Is it initialized early enough to allow audio driver querying a clock? > > It should be set before we expose the component interface for the audio > driver query. Jani, Ville, do we have any patch available to re-test?
(In reply to yann from comment #31) > (In reply to Jani Nikula from comment #30) > > (In reply to Vasily Khoruzhick from comment #27) > > > (In reply to Jani Nikula from comment #26) > > > > > > > + return dev_priv->cdclk_freq; > > > > } > > > > > > > > static int i915_audio_component_sync_audio_rate(struct device *dev, > > > > > > Is it initialized early enough to allow audio driver querying a clock? > > > > It should be set before we expose the component interface for the audio > > driver query. > > Jani, Ville, do we have any patch available to re-test? The audio team needs to modify hdac_hdmi_eld_notify_cb() in their driver to query CDCLK and update their M/N registers appropriately. I'm not sure that there's anything on the graphics side that is needed.
(In reply to Jim Bride from comment #32) > (In reply to yann from comment #31) > > (In reply to Jani Nikula from comment #30) > > > (In reply to Vasily Khoruzhick from comment #27) > > > > (In reply to Jani Nikula from comment #26) > > > > > > > > > + return dev_priv->cdclk_freq; > > > > > } > > > > > > > > > > static int i915_audio_component_sync_audio_rate(struct device *dev, > > > > > > > > Is it initialized early enough to allow audio driver querying a clock? > > > > > > It should be set before we expose the component interface for the audio > > > driver query. > > > > Jani, Ville, do we have any patch available to re-test? > > The audio team needs to modify hdac_hdmi_eld_notify_cb() in their driver to > query CDCLK and update their M/N registers appropriately. I'm not sure that > there's anything on the graphics side that is needed. We'll want the CDCLK_FREQ patch in any case, but I don't know if it will have any effect on audio.
(In reply to Jim Bride from comment #32) > The audio team needs to modify hdac_hdmi_eld_notify_cb() in their driver to > query CDCLK and update their M/N registers appropriately. hdac_hdmi_eld_notify_cb() is ASoC SKL driver, so it must be irrelevant. The relevant code is in patch_hdmi.c for legacy HDA driver. I cooked up a patch quickly. The untested patch is attached below. Let me know if it works.
Created attachment 123122 [details] [review] [PATCH] ALSA: hda - Move i915 HSW/BDW BCLK setup to a common function
Created attachment 123130 [details] [review] [PATCH] ALSA: hda - Update BCLK also at hotplug for i915 HSW/BDW Revised patch.
Focus, please try Takashi's patch and update accordingly the bug. thanks
After applying Takashi's patch, this issue cannot be reproduced anymore, both analog audio and display audio are work well. Thanks~
Thanks Keqiao, once this patch set is merged do a final re-test and then close the bug
Good to hear. Now I merged the patch to for-linus branch.
Takashi's patch is merged in drm-intel and this issue is gone after verified, so let's close this bug. Thanks~
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.