| Message ID | 20200330144421.11632-1-kai.vehmanen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] drm/i915: do AUD_FREQ_CNTRL state save on all gen9+ platforms | expand |
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
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
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
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
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,
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(-)