[RESEND] drm/i915: do AUD_FREQ_CNTRL state save on all gen9+ platforms
diff mbox series

Message ID 20200330144421.11632-1-kai.vehmanen@linux.intel.com
State New
Headers show
Series
  • [RESEND] drm/i915: do AUD_FREQ_CNTRL state save on all gen9+ platforms
Related show

Commit Message

Kai Vehmanen March 30, 2020, 2:44 p.m. UTC
Replace the TGL/ICL specific platform checks with a more generic check
using INTEL_GEN(). Fixes bug with broken audio after S3 resume on JSL
platforms.

An initial version of state save and restore of AUD_FREQ_CNTRL register
was added for subset of platforms in commit 87c1694533c9
("drm/i915: save AUD_FREQ_CNTRL state at audio domain suspend"). The state
save has proven to work well and it is needed in newer platforms, so needs
to be extended. Although the logic is not in practise needed on GEN9/10
systems, follow the hardware specification and apply state and restore on
all gen9+ platforms.

Bspec: 49281
Link: https://github.com/thesofproject/linux/issues/1719
Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_audio.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Kai Vehmanen April 9, 2020, 2:14 p.m. UTC | #1
Hey,

On Mon, 30 Mar 2020, Kai Vehmanen wrote:

> Replace the TGL/ICL specific platform checks with a more generic check
> using INTEL_GEN(). Fixes bug with broken audio after S3 resume on JSL
> platforms.

I would be (gently) beaten with a stick on alsa-devel for sending this
type of content free ping, but I still dare to seek your input on what is 
the proper way to get attention to a patch that are seemingly forever 
stuck on the review sideline.

I've sent this on 13.3., resend on 30.3.. Should I just keep on sending 
resends and let the system work (this is the alsa-devel practise), or 
should I start to contact potential reviewers with more direct asks?

Tests seem to all pass and this is pretty important for anyone using JSL 
platforms (you lose HDMI/DP audio after first S3 suspend otherwise):
https://patchwork.freedesktop.org/series/74664/

Br, Kai
Ville Syrjala April 9, 2020, 6:04 p.m. UTC | #2
On Thu, Apr 09, 2020 at 05:14:01PM +0300, Kai Vehmanen wrote:
> Hey,
> 
> On Mon, 30 Mar 2020, Kai Vehmanen wrote:
> 
> > Replace the TGL/ICL specific platform checks with a more generic check
> > using INTEL_GEN(). Fixes bug with broken audio after S3 resume on JSL
> > platforms.
> 
> I would be (gently) beaten with a stick on alsa-devel for sending this
> type of content free ping, but I still dare to seek your input on what is 
> the proper way to get attention to a patch that are seemingly forever 
> stuck on the review sideline.

And what is this?
https://patchwork.freedesktop.org/patch/347148/?series=71527&rev=1

> 
> I've sent this on 13.3., resend on 30.3.. Should I just keep on sending 
> resends and let the system work (this is the alsa-devel practise), or 
> should I start to contact potential reviewers with more direct asks?

Just ping on original patch or ping someone on irc. Resending
the same patch over and over does no good. At least my brain just
ignores anything that looks like it's just a resend w/o any clear
justification.

> 
> Tests seem to all pass and this is pretty important for anyone using JSL 
> platforms (you lose HDMI/DP audio after first S3 suspend otherwise):
> https://patchwork.freedesktop.org/series/74664/
> 
> Br, Kai
Kai Vehmanen April 14, 2020, 10:32 a.m. UTC | #3
Hey,

On Thu, 9 Apr 2020, Ville Syrjälä wrote:

> On Thu, Apr 09, 2020 at 05:14:01PM +0300, Kai Vehmanen wrote:
> > type of content free ping, but I still dare to seek your input on what is 
> > the proper way to get attention to a patch that are seemingly forever 
> > stuck on the review sideline.
> 
> And what is this?
> https://patchwork.freedesktop.org/patch/347148/?series=71527&rev=1

that's a lost child I'm afraid. It's essentially the same patch I 
submitted late last year. It got review ok from Matt, and I thought it was 
going to be merged and went on to do other things -- i.e. I failed to 
follow-up on this. Back then I did not know about any actual bugs this 
would fix -- this was a generic change to align with hw specs.

Fast forward two months. I had forgotten about that previous patch, and 
I ended up recreating the same patch to fix an actual bug. I.e. the of 
this thread:
https://patchwork.freedesktop.org/series/74664/

I only very recently noticed the old patch. But alas, the new attempt 
is probably the one that should be merged as it has more information in 
the commit message (we now know about actual issues on JSL).

> Just ping on original patch or ping someone on irc. Resending
> the same patch over and over does no good. At least my brain just
> ignores anything that looks like it's just a resend w/o any clear
> justification.

Ack, thanks a lot, this clarifies. In any case, patch author should own 
the follow-up and I definitely dropped the ball on the older #347148 .

Br, Kai
Ville Syrjala April 14, 2020, 4:54 p.m. UTC | #4
On Tue, Apr 14, 2020 at 01:32:49PM +0300, Kai Vehmanen wrote:
> Hey,
> 
> On Thu, 9 Apr 2020, Ville Syrjälä wrote:
> 
> > On Thu, Apr 09, 2020 at 05:14:01PM +0300, Kai Vehmanen wrote:
> > > type of content free ping, but I still dare to seek your input on what is 
> > > the proper way to get attention to a patch that are seemingly forever 
> > > stuck on the review sideline.
> > 
> > And what is this?
> > https://patchwork.freedesktop.org/patch/347148/?series=71527&rev=1
> 
> that's a lost child I'm afraid. It's essentially the same patch I 
> submitted late last year. It got review ok from Matt, and I thought it was 
> going to be merged and went on to do other things -- i.e. I failed to 
> follow-up on this. Back then I did not know about any actual bugs this 
> would fix -- this was a generic change to align with hw specs.
> 
> Fast forward two months. I had forgotten about that previous patch, and 
> I ended up recreating the same patch to fix an actual bug. I.e. the of 
> this thread:
> https://patchwork.freedesktop.org/series/74664/

OK. I sucked in the rb from the old patch and pushed the new version.
Thanks.

> 
> I only very recently noticed the old patch. But alas, the new attempt 
> is probably the one that should be merged as it has more information in 
> the commit message (we now know about actual issues on JSL).
> 
> > Just ping on original patch or ping someone on irc. Resending
> > the same patch over and over does no good. At least my brain just
> > ignores anything that looks like it's just a resend w/o any clear
> > justification.
> 
> Ack, thanks a lot, this clarifies. In any case, patch author should own 
> the follow-up and I definitely dropped the ball on the older #347148 .
> 
> Br, Kai

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c
index 19bf206037c2..f4ed3acddc07 100644
--- a/drivers/gpu/drm/i915/display/intel_audio.c
+++ b/drivers/gpu/drm/i915/display/intel_audio.c
@@ -883,7 +883,7 @@  static unsigned long i915_audio_component_get_power(struct device *kdev)
 	ret = intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
 
 	if (dev_priv->audio_power_refcount++ == 0) {
-		if (IS_TIGERLAKE(dev_priv) || IS_ICELAKE(dev_priv)) {
+		if (INTEL_GEN(dev_priv) >= 9) {
 			intel_de_write(dev_priv, AUD_FREQ_CNTRL,
 				       dev_priv->audio_freq_cntrl);
 			drm_dbg_kms(&dev_priv->drm,
@@ -1165,7 +1165,7 @@  static void i915_audio_component_init(struct drm_i915_private *dev_priv)
 		return;
 	}
 
-	if (IS_TIGERLAKE(dev_priv) || IS_ICELAKE(dev_priv)) {
+	if (INTEL_GEN(dev_priv) >= 9) {
 		dev_priv->audio_freq_cntrl = intel_de_read(dev_priv,
 							   AUD_FREQ_CNTRL);
 		drm_dbg_kms(&dev_priv->drm,