[PATCH] drm/i915: Acquire intel_runtime_pm for HD-Audio registers

[PATCH] drm/i915: Acquire intel_runtime_pm for HD-Audio registers

[Chris Wilson]

On Haswell/Broadwell, the HD-Audio block is inside the HDMI/display
power well and so the sna-hda audio codec acquires the display power
well while it is operational. However, Skylake separates the powerwells
again, and so we must remember to acquire the rpm wakeref for ourselves
whilst tweaking the registers.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96214
Fixes: 03b135cebc47 "ALSA: hda - remove dependency on i915 power well for SKL")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Libin Yang <libin.yang@intel.com>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Marius Vlad <marius.c.vlad@intel.com>
---
 drivers/gpu/drm/i915/intel_audio.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 5d5f6bc10e85..eedcce6478ef 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -600,6 +600,8 @@ static void i915_audio_component_codec_wake_override(struct device *dev,
 	if (!IS_SKYLAKE(dev_priv) && !IS_KABYLAKE(dev_priv))
 		return;
 
+	intel_runtime_pm_get(dev_priv);
+
 	/*
 	 * Enable/disable generating the codec wake signal, overriding the
 	 * internal logic to generate the codec wake to controller.
@@ -615,6 +617,8 @@ static void i915_audio_component_codec_wake_override(struct device *dev,
 		I915_WRITE(HSW_AUD_CHICKENBIT, tmp);
 		usleep_range(1000, 1500);
 	}
+
+	intel_runtime_pm_put(dev_priv);
 }
 
 /* Get CDCLK in kHz  */
@@ -648,6 +652,8 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
 	    !IS_HASWELL(dev_priv))
 		return 0;
 
+	intel_runtime_pm_get(dev_priv);
+
 	mutex_lock(&dev_priv->av_mutex);
 	/* 1. get the pipe */
 	intel_encoder = dev_priv->dig_port_map[port];
@@ -698,6 +704,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
 
  unlock:
 	mutex_unlock(&dev_priv->av_mutex);
+	intel_runtime_pm_put(dev_priv);
 	return err;
 }
 
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Ro.CI.BAT: failure for drm/i915: Acquire intel_runtime_pm for HD-Audio registers

[Patchwork]

== Series Details ==

Series: drm/i915: Acquire intel_runtime_pm for HD-Audio registers
URL   : https://patchwork.freedesktop.org/series/9689/
State : failure

== Summary ==

Series 9689v1 drm/i915: Acquire intel_runtime_pm for HD-Audio registers
http://patchwork.freedesktop.org/api/1.0/series/9689/revisions/1/mbox

Test vgem_basic:
        Subgroup debugfs:
                pass       -> FAIL       (ro-bdw-i7-5557U)
                pass       -> FAIL       (ro-ilk1-i5-650)
                pass       -> FAIL       (ro-hsw-i3-4010u)
                pass       -> FAIL       (ro-bdw-i7-5600u)
                pass       -> FAIL       (ro-snb-i7-2620M)
                pass       -> FAIL       (ro-bdw-i5-5250u)
                pass       -> FAIL       (ro-hsw-i7-4770r)
                pass       -> FAIL       (ro-ivb-i7-3770)
                pass       -> FAIL       (ro-byt-n2820)
                pass       -> FAIL       (ro-skl3-i5-6260u)

fi-kbl-qkkr      total:237  pass:168  dwarn:28  dfail:2   fail:5   skip:34 
fi-skl-i5-6260u  total:237  pass:211  dwarn:0   dfail:1   fail:5   skip:20 
fi-skl-i7-6700k  total:237  pass:197  dwarn:0   dfail:1   fail:5   skip:34 
fi-snb-i7-2600   total:237  pass:183  dwarn:0   dfail:1   fail:5   skip:48 
ro-bdw-i5-5250u  total:237  pass:206  dwarn:1   dfail:1   fail:5   skip:24 
ro-bdw-i7-5557U  total:237  pass:206  dwarn:2   dfail:1   fail:5   skip:23 
ro-bdw-i7-5600u  total:237  pass:192  dwarn:0   dfail:1   fail:5   skip:39 
ro-bsw-n3050     total:217  pass:170  dwarn:0   dfail:0   fail:4   skip:42 
ro-byt-n2820     total:237  pass:182  dwarn:0   dfail:1   fail:8   skip:46 
ro-hsw-i3-4010u  total:237  pass:199  dwarn:0   dfail:1   fail:5   skip:32 
ro-hsw-i7-4770r  total:237  pass:199  dwarn:0   dfail:1   fail:5   skip:32 
ro-ilk-i7-620lm  total:237  pass:159  dwarn:0   dfail:1   fail:6   skip:71 
ro-ilk1-i5-650   total:232  pass:159  dwarn:0   dfail:1   fail:6   skip:66 
ro-ivb-i7-3770   total:237  pass:190  dwarn:0   dfail:1   fail:5   skip:41 
ro-skl3-i5-6260u total:237  pass:210  dwarn:1   dfail:1   fail:5   skip:20 
ro-snb-i7-2620M  total:237  pass:181  dwarn:0   dfail:1   fail:6   skip:49 

Results at /archive/results/CI_IGT_test/RO_Patchwork_1458/

689b286 drm-intel-nightly: 2016y-07m-08d-12h-37m-09s UTC integration manifest
fc31f3e drm/i915: Acquire intel_runtime_pm for HD-Audio registers

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Acquire intel_runtime_pm for HD-Audio registers

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> On Haswell/Broadwell, the HD-Audio block is inside the HDMI/display
> power well and so the sna-hda audio codec acquires the display power
> well while it is operational. However, Skylake separates the powerwells
> again, and so we must remember to acquire the rpm wakeref for ourselves
> whilst tweaking the registers.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96214
> Fixes: 03b135cebc47 "ALSA: hda - remove dependency on i915 power well for SKL")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> Cc: Libin Yang <libin.yang@intel.com>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Marius Vlad <marius.c.vlad@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_audio.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 5d5f6bc10e85..eedcce6478ef 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -600,6 +600,8 @@ static void i915_audio_component_codec_wake_override(struct device *dev,
>  	if (!IS_SKYLAKE(dev_priv) && !IS_KABYLAKE(dev_priv))
>  		return;
>  
> +	intel_runtime_pm_get(dev_priv);
> +
>  	/*
>  	 * Enable/disable generating the codec wake signal, overriding the
>  	 * internal logic to generate the codec wake to controller.
> @@ -615,6 +617,8 @@ static void i915_audio_component_codec_wake_override(struct device *dev,
>  		I915_WRITE(HSW_AUD_CHICKENBIT, tmp);
>  		usleep_range(1000, 1500);
>  	}
> +
> +	intel_runtime_pm_put(dev_priv);
>  }
>  
>  /* Get CDCLK in kHz  */
> @@ -648,6 +652,8 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
>  	    !IS_HASWELL(dev_priv))
>  		return 0;
>  
> +	intel_runtime_pm_get(dev_priv);
> +
>  	mutex_lock(&dev_priv->av_mutex);
>  	/* 1. get the pipe */
>  	intel_encoder = dev_priv->dig_port_map[port];
> @@ -698,6 +704,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
>  
>   unlock:
>  	mutex_unlock(&dev_priv->av_mutex);
> +	intel_runtime_pm_put(dev_priv);
>  	return err;
>  }
>  
> -- 
> 2.8.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Acquire intel_runtime_pm for HD-Audio registers

[Chris Wilson]

On Tue, Jul 12, 2016 at 04:10:22PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > On Haswell/Broadwell, the HD-Audio block is inside the HDMI/display
> > power well and so the sna-hda audio codec acquires the display power
> > well while it is operational. However, Skylake separates the powerwells
> > again, and so we must remember to acquire the rpm wakeref for ourselves
> > whilst tweaking the registers.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96214
> > Fixes: 03b135cebc47 "ALSA: hda - remove dependency on i915 power well for SKL")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
> 
> > Cc: Libin Yang <libin.yang@intel.com>
> > Cc: Takashi Iwai <tiwai@suse.de>
> > Cc: Marius Vlad <marius.c.vlad@intel.com>

Marius, could you provide a tested by?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Acquire intel_runtime_pm for HD-Audio registers

[Marius Vlad]

[-- Attachment #1.1: Type: text/plain, Size: 1254 bytes --]

Did try when you submitted the patch...but can't seem to replicate with
latest nightly on other SKLs, and currently do not have access on
the machine that caused it.

On Tue, Jul 12, 2016 at 02:16:50PM +0100, Chris Wilson wrote:
> On Tue, Jul 12, 2016 at 04:10:22PM +0300, Mika Kuoppala wrote:
> > Chris Wilson <chris@chris-wilson.co.uk> writes:
> > 
> > > On Haswell/Broadwell, the HD-Audio block is inside the HDMI/display
> > > power well and so the sna-hda audio codec acquires the display power
> > > well while it is operational. However, Skylake separates the powerwells
> > > again, and so we must remember to acquire the rpm wakeref for ourselves
> > > whilst tweaking the registers.
> > >
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96214
> > > Fixes: 03b135cebc47 "ALSA: hda - remove dependency on i915 power well for SKL")
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
> > 
> > > Cc: Libin Yang <libin.yang@intel.com>
> > > Cc: Takashi Iwai <tiwai@suse.de>
> > > Cc: Marius Vlad <marius.c.vlad@intel.com>
> 
> Marius, could you provide a tested by?
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Acquire intel_runtime_pm for HD-Audio registers

[Chris Wilson]

On Wed, Jul 13, 2016 at 05:01:02PM +0300, Marius Vlad wrote:
> Did try when you submitted the patch...but can't seem to replicate with
> latest nightly on other SKLs, and currently do not have access on
> the machine that caused it.

So fwiw, Hans de Geode confirmed that only reverting 03b135cebc47 (which
enables the display powerwell as well) was enough.

Libin, do you want to handle the revert or explain the mistake in

commit 03b135cebc47d75ea2dc346770374ab741966955
Author: Libin Yang <libin.yang@intel.com>
Date:   Wed Jun 3 09:30:15 2015 +0800

    ALSA: hda - remove controller dependency on i915 power well for SKL

-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Acquire intel_runtime_pm for HD-Audio registers

[Yang, Libin]

Hi Wilson,

> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Thursday, July 14, 2016 10:34 PM
> To: Mika Kuoppala <mika.kuoppala@linux.intel.com>; intel-
> gfx@lists.freedesktop.org; Takashi Iwai <tiwai@suse.de>; Yang, Libin
> <libin.yang@intel.com>
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Acquire intel_runtime_pm for HD-
> Audio registers
> 
> On Wed, Jul 13, 2016 at 05:01:02PM +0300, Marius Vlad wrote:
> > Did try when you submitted the patch...but can't seem to replicate
> > with latest nightly on other SKLs, and currently do not have access on
> > the machine that caused it.
> 
> So fwiw, Hans de Geode confirmed that only reverting 03b135cebc47 (which
> enables the display powerwell as well) was enough.
> 
> Libin, do you want to handle the revert or explain the mistake in

What's the problem with the patch? I can find the details from the email thread.

HSW/BSW need acquire the power because it is inside the gpu. So we 
limited need i915 power for controller to HSW/BDW. 

> 
> commit 03b135cebc47d75ea2dc346770374ab741966955
> Author: Libin Yang <libin.yang@intel.com>
> Date:   Wed Jun 3 09:30:15 2015 +0800
> 
>     ALSA: hda - remove controller dependency on i915 power well for SKL
> 
> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Acquire intel_runtime_pm for HD-Audio registers

[Chris Wilson]

On Fri, Jul 15, 2016 at 02:40:42AM +0000, Yang, Libin wrote:
> Hi Wilson,
> 
> > -----Original Message-----
> > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > Sent: Thursday, July 14, 2016 10:34 PM
> > To: Mika Kuoppala <mika.kuoppala@linux.intel.com>; intel-
> > gfx@lists.freedesktop.org; Takashi Iwai <tiwai@suse.de>; Yang, Libin
> > <libin.yang@intel.com>
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Acquire intel_runtime_pm for HD-
> > Audio registers
> > 
> > On Wed, Jul 13, 2016 at 05:01:02PM +0300, Marius Vlad wrote:
> > > Did try when you submitted the patch...but can't seem to replicate
> > > with latest nightly on other SKLs, and currently do not have access on
> > > the machine that caused it.
> > 
> > So fwiw, Hans de Geode confirmed that only reverting 03b135cebc47 (which
> > enables the display powerwell as well) was enough.
> > 
> > Libin, do you want to handle the revert or explain the mistake in
> 
> What's the problem with the patch? I can find the details from the email thread.
> 
> HSW/BSW need acquire the power because it is inside the gpu. So we 
> limited need i915 power for controller to HSW/BDW. 

Please see
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96214

Judging from the reports, Skylake uses the same display powerwell as
HSW/BDW.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Acquire intel_runtime_pm for HD-Audio registers

[Yang, Libin]

Hi Wilson,

Thanks for tell the details.

SKL is different from HSW/BDW.

HSW/BDW has 2 audio controllers. One is in PCH and another is in gfx.
SKL has one controller, which is in PCH.

Both HSW/BDW and SKL has display audio codec in gfx.

For HSW/BDW, to use the controller inside gfx, we must get the gfx power.
For SKL, to use the controller, we don't need to get the gfx power.
For both HSW/BDW and SKL, to use display audio codec, we must get the gfx power.

Without the patch, all the audio controllers except BYT/BSW will get the gfx power.
However this is not true, SKL and APL audio controller don't need this power.
-               /* Baytral/Braswell controllers don't need this power */
-               if (pci->device != 0x0f04 && pci->device != 0x2284)
+               /* HSW/BDW controllers need this power */
+               if (CONTROLLER_IN_GPU(pci))
                        hda->need_i915_power = 1;

For skl, it need to get gfx power only when display audio codec is used, this is
set in the patch:
+       if (is_valleyview_plus(codec) || is_skylake(codec))
                codec->core.link_power_control = 1;

Without the patch, SKL may consume gfx power even it doesn't need.

This patch has been tested by our full test cases for about one year,
it shows the right result. Do you think maybe there is another recent
patch cause this issue? I will debug the Bugzilla next week on audio side.

Regards,
Libin


> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Friday, July 15, 2016 4:45 PM
> To: Yang, Libin <libin.yang@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>; intel-
> gfx@lists.freedesktop.org; Takashi Iwai <tiwai@suse.de>
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Acquire intel_runtime_pm for HD-
> Audio registers
> 
> On Fri, Jul 15, 2016 at 02:40:42AM +0000, Yang, Libin wrote:
> > Hi Wilson,
> >
> > > -----Original Message-----
> > > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > > Sent: Thursday, July 14, 2016 10:34 PM
> > > To: Mika Kuoppala <mika.kuoppala@linux.intel.com>; intel-
> > > gfx@lists.freedesktop.org; Takashi Iwai <tiwai@suse.de>; Yang, Libin
> > > <libin.yang@intel.com>
> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Acquire intel_runtime_pm
> > > for HD- Audio registers
> > >
> > > On Wed, Jul 13, 2016 at 05:01:02PM +0300, Marius Vlad wrote:
> > > > Did try when you submitted the patch...but can't seem to replicate
> > > > with latest nightly on other SKLs, and currently do not have
> > > > access on the machine that caused it.
> > >
> > > So fwiw, Hans de Geode confirmed that only reverting 03b135cebc47
> > > (which enables the display powerwell as well) was enough.
> > >
> > > Libin, do you want to handle the revert or explain the mistake in
> >
> > What's the problem with the patch? I can find the details from the email
> thread.
> >
> > HSW/BSW need acquire the power because it is inside the gpu. So we
> > limited need i915 power for controller to HSW/BDW.
> 
> Please see
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96214
> 
> Judging from the reports, Skylake uses the same display powerwell as
> HSW/BDW.
> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx