[PATCH 5.1 stable v2] drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled
From: Ville Syrjälä ville.syrjala@linux.intel.com
commit 905801fe72377b4dc53c6e13eea1a91c6a4aa0c4 upstream.
CDCLK has to be at least twice the BLCK regardless of audio. Audio driver has to probe using this hook and increase the clock even in absence of any display.
v2: Use atomic refcount for get_power, put_power so that we can call each once(Abhay). v3: Reset power well 2 to avoid any transaction on iDisp link during cdclk change(Abhay). v4: Remove Power well 2 reset workaround(Ville). v5: Remove unwanted Power well 2 register defined in v4(Abhay). v6: - Use a dedicated flag instead of state->modeset for min CDCLK changes - Make get/put audio power domain symmetric - Rebased on top of intel_wakeref tracking changes.
[jian-hong@endlessm.com: Rediff to keep i915_audio_component_get_power and i915_audio_component_put_power for Linux stable 5.1.x] Cc: stable@vger.kernel.org # 5.1.x Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Abhay Kumar abhay.kumar@intel.com Tested-by: Abhay Kumar abhay.kumar@intel.com Signed-off-by: Imre Deak imre.deak@intel.com Reviewed-by: Clint Taylor Clinton.A.Taylor@intel.com Link: https://patchwork.freedesktop.org/patch/msgid/20190320135439.12201-1-imre.de... Signed-off-by: Jian-Hong Pan jian-hong@endlessm.com --- v2: Use intel_display_power_put_unchecked() instead of intel_display_power_put_power() in i915_audio_component_put_power() for balance.
drivers/gpu/drm/i915/i915_drv.h | 3 ++ drivers/gpu/drm/i915/intel_audio.c | 62 ++++++++++++++++++++++++++++++++++-- drivers/gpu/drm/i915/intel_cdclk.c | 30 ++++++----------- drivers/gpu/drm/i915/intel_display.c | 9 +++++- drivers/gpu/drm/i915/intel_drv.h | 3 ++ 5 files changed, 83 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index a67a63b5aa84..5c9bb1b2d1f3 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1622,6 +1622,8 @@ struct drm_i915_private { struct intel_cdclk_state actual; /* The current hardware cdclk state */ struct intel_cdclk_state hw; + + int force_min_cdclk; } cdclk;
/** @@ -1741,6 +1743,7 @@ struct drm_i915_private { * */ struct mutex av_mutex; + int audio_power_refcount;
struct { struct mutex mutex; diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 5104c6bbd66f..2f3fd5bf0f11 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -741,15 +741,71 @@ void intel_init_audio_hooks(struct drm_i915_private *dev_priv) } }
+static void glk_force_audio_cdclk(struct drm_i915_private *dev_priv, + bool enable) +{ + struct drm_modeset_acquire_ctx ctx; + struct drm_atomic_state *state; + int ret; + + drm_modeset_acquire_init(&ctx, 0); + state = drm_atomic_state_alloc(&dev_priv->drm); + if (WARN_ON(!state)) + return; + + state->acquire_ctx = &ctx; + +retry: + to_intel_atomic_state(state)->cdclk.force_min_cdclk_changed = true; + to_intel_atomic_state(state)->cdclk.force_min_cdclk = + enable ? 2 * 96000 : 0; + + /* + * Protects dev_priv->cdclk.force_min_cdclk + * Need to lock this here in case we have no active pipes + * and thus wouldn't lock it during the commit otherwise. + */ + ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, + &ctx); + if (!ret) + ret = drm_atomic_commit(state); + + if (ret == -EDEADLK) { + drm_atomic_state_clear(state); + drm_modeset_backoff(&ctx); + goto retry; + } + + WARN_ON(ret); + + drm_atomic_state_put(state); + + drm_modeset_drop_locks(&ctx); + drm_modeset_acquire_fini(&ctx); +} + static void i915_audio_component_get_power(struct device *kdev) { - intel_display_power_get(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO); + struct drm_i915_private *dev_priv = kdev_to_i915(kdev); + + intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO); + + /* Force CDCLK to 2*BCLK as long as we need audio to be powered. */ + if (dev_priv->audio_power_refcount++ == 0) + if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) + glk_force_audio_cdclk(dev_priv, true); }
static void i915_audio_component_put_power(struct device *kdev) { - intel_display_power_put_unchecked(kdev_to_i915(kdev), - POWER_DOMAIN_AUDIO); + struct drm_i915_private *dev_priv = kdev_to_i915(kdev); + + /* Stop forcing CDCLK to 2*BCLK if no need for audio to be powered. */ + if (--dev_priv->audio_power_refcount == 0) + if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) + glk_force_audio_cdclk(dev_priv, false); + + intel_display_power_put_unchecked(dev_priv, POWER_DOMAIN_AUDIO); }
static void i915_audio_component_codec_wake_override(struct device *kdev, diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c index 15ba950dee00..553f57ff60f4 100644 --- a/drivers/gpu/drm/i915/intel_cdclk.c +++ b/drivers/gpu/drm/i915/intel_cdclk.c @@ -2187,19 +2187,8 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state) /* * According to BSpec, "The CD clock frequency must be at least twice * the frequency of the Azalia BCLK." and BCLK is 96 MHz by default. - * - * FIXME: Check the actual, not default, BCLK being used. - * - * FIXME: This does not depend on ->has_audio because the higher CDCLK - * is required for audio probe, also when there are no audio capable - * displays connected at probe time. This leads to unnecessarily high - * CDCLK when audio is not required. - * - * FIXME: This limit is only applied when there are displays connected - * at probe time. If we probe without displays, we'll still end up using - * the platform minimum CDCLK, failing audio probe. */ - if (INTEL_GEN(dev_priv) >= 9) + if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9) min_cdclk = max(2 * 96000, min_cdclk);
/* @@ -2239,7 +2228,7 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state) intel_state->min_cdclk[i] = min_cdclk; }
- min_cdclk = 0; + min_cdclk = intel_state->cdclk.force_min_cdclk; for_each_pipe(dev_priv, pipe) min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk);
@@ -2300,7 +2289,8 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state) vlv_calc_voltage_level(dev_priv, cdclk);
if (!intel_state->active_crtcs) { - cdclk = vlv_calc_cdclk(dev_priv, 0); + cdclk = vlv_calc_cdclk(dev_priv, + intel_state->cdclk.force_min_cdclk);
intel_state->cdclk.actual.cdclk = cdclk; intel_state->cdclk.actual.voltage_level = @@ -2333,7 +2323,7 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state) bdw_calc_voltage_level(cdclk);
if (!intel_state->active_crtcs) { - cdclk = bdw_calc_cdclk(0); + cdclk = bdw_calc_cdclk(intel_state->cdclk.force_min_cdclk);
intel_state->cdclk.actual.cdclk = cdclk; intel_state->cdclk.actual.voltage_level = @@ -2405,7 +2395,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state) skl_calc_voltage_level(cdclk);
if (!intel_state->active_crtcs) { - cdclk = skl_calc_cdclk(0, vco); + cdclk = skl_calc_cdclk(intel_state->cdclk.force_min_cdclk, vco);
intel_state->cdclk.actual.vco = vco; intel_state->cdclk.actual.cdclk = cdclk; @@ -2444,10 +2434,10 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
if (!intel_state->active_crtcs) { if (IS_GEMINILAKE(dev_priv)) { - cdclk = glk_calc_cdclk(0); + cdclk = glk_calc_cdclk(intel_state->cdclk.force_min_cdclk); vco = glk_de_pll_vco(dev_priv, cdclk); } else { - cdclk = bxt_calc_cdclk(0); + cdclk = bxt_calc_cdclk(intel_state->cdclk.force_min_cdclk); vco = bxt_de_pll_vco(dev_priv, cdclk); }
@@ -2483,7 +2473,7 @@ static int cnl_modeset_calc_cdclk(struct drm_atomic_state *state) cnl_compute_min_voltage_level(intel_state));
if (!intel_state->active_crtcs) { - cdclk = cnl_calc_cdclk(0); + cdclk = cnl_calc_cdclk(intel_state->cdclk.force_min_cdclk); vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
intel_state->cdclk.actual.vco = vco; @@ -2519,7 +2509,7 @@ static int icl_modeset_calc_cdclk(struct drm_atomic_state *state) cnl_compute_min_voltage_level(intel_state));
if (!intel_state->active_crtcs) { - cdclk = icl_calc_cdclk(0, ref); + cdclk = icl_calc_cdclk(intel_state->cdclk.force_min_cdclk, ref); vco = icl_calc_cdclk_pll_vco(dev_priv, cdclk);
intel_state->cdclk.actual.vco = vco; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 421aac80a838..ebbac873b8f4 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12769,6 +12769,11 @@ static int intel_modeset_checks(struct drm_atomic_state *state) return -EINVAL; }
+ /* keep the current setting */ + if (!intel_state->cdclk.force_min_cdclk_changed) + intel_state->cdclk.force_min_cdclk = + dev_priv->cdclk.force_min_cdclk; + intel_state->modeset = true; intel_state->active_crtcs = dev_priv->active_crtcs; intel_state->cdclk.logical = dev_priv->cdclk.logical; @@ -12864,7 +12869,7 @@ static int intel_atomic_check(struct drm_device *dev, struct drm_crtc *crtc; struct drm_crtc_state *old_crtc_state, *crtc_state; int ret, i; - bool any_ms = false; + bool any_ms = intel_state->cdclk.force_min_cdclk_changed;
/* Catch I915_MODE_FLAG_INHERITED */ for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, @@ -13456,6 +13461,8 @@ static int intel_atomic_commit(struct drm_device *dev, dev_priv->active_crtcs = intel_state->active_crtcs; dev_priv->cdclk.logical = intel_state->cdclk.logical; dev_priv->cdclk.actual = intel_state->cdclk.actual; + dev_priv->cdclk.force_min_cdclk = + intel_state->cdclk.force_min_cdclk; }
drm_atomic_state_get(state); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index d5660ac1b0d6..539caca05da4 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -480,6 +480,9 @@ struct intel_atomic_state { * state only when all crtc's are DPMS off. */ struct intel_cdclk_state actual; + + int force_min_cdclk; + bool force_min_cdclk_changed; } cdclk;
bool dpll_set, modeset;
Hi,
After apply the commit "drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled", it induces the screen to flicker when the CDCLK changes on the laptop like ASUS E406MA. [1]
So, we need these commits to prevent that: commit 48d9f87ddd21 drm/i915: Save the old CDCLK atomic state commit 2b21dfbeee72 drm/i915: Remove redundant store of logical CDCLK state commit 59f9e9cab3a1 drm/i915: Skip modeset for cdclk changes if possible
[1]: https://bugzilla.kernel.org/show_bug.cgi?id=203623#c12
Jian-Hong Pan
Imre Deak (2): drm/i915: Save the old CDCLK atomic state drm/i915: Remove redundant store of logical CDCLK state
Ville Syrjälä (1): drm/i915: Skip modeset for cdclk changes if possible
drivers/gpu/drm/i915/i915_drv.h | 3 +- drivers/gpu/drm/i915/intel_cdclk.c | 155 ++++++++++++++++++++++----- drivers/gpu/drm/i915/intel_display.c | 48 +++++++-- drivers/gpu/drm/i915/intel_drv.h | 18 +++- 4 files changed, 186 insertions(+), 38 deletions(-)
On Mon, Jun 10, 2019 at 02:01:39PM +0800, Jian-Hong Pan wrote:
Hi,
After apply the commit "drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled", it induces the screen to flicker when the CDCLK changes on the laptop like ASUS E406MA. [1]
So, we need these commits to prevent that: commit 48d9f87ddd21 drm/i915: Save the old CDCLK atomic state commit 2b21dfbeee72 drm/i915: Remove redundant store of logical CDCLK state commit 59f9e9cab3a1 drm/i915: Skip modeset for cdclk changes if possible
Jian-Hong Pan
Imre Deak (2): drm/i915: Save the old CDCLK atomic state drm/i915: Remove redundant store of logical CDCLK state
Ville Syrjälä (1): drm/i915: Skip modeset for cdclk changes if possible
drivers/gpu/drm/i915/i915_drv.h | 3 +- drivers/gpu/drm/i915/intel_cdclk.c | 155 ++++++++++++++++++++++----- drivers/gpu/drm/i915/intel_display.c | 48 +++++++-- drivers/gpu/drm/i915/intel_drv.h | 18 +++- 4 files changed, 186 insertions(+), 38 deletions(-)
These are all big patches, I would like to get an ack from the i915 developer(s) that these are acceptable for the stable tree before applying them...
thanks,
greg k-h
Greg KH gregkh@linuxfoundation.org 於 2019年6月13日 週四 下午3:52寫道：
On Mon, Jun 10, 2019 at 02:01:39PM +0800, Jian-Hong Pan wrote:
Hi,
After apply the commit "drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled", it induces the screen to flicker when the CDCLK changes on the laptop like ASUS E406MA. [1]
So, we need these commits to prevent that: commit 48d9f87ddd21 drm/i915: Save the old CDCLK atomic state commit 2b21dfbeee72 drm/i915: Remove redundant store of logical CDCLK state commit 59f9e9cab3a1 drm/i915: Skip modeset for cdclk changes if possible
Jian-Hong Pan
Imre Deak (2): drm/i915: Save the old CDCLK atomic state drm/i915: Remove redundant store of logical CDCLK state
Ville Syrjälä (1): drm/i915: Skip modeset for cdclk changes if possible
drivers/gpu/drm/i915/i915_drv.h | 3 +- drivers/gpu/drm/i915/intel_cdclk.c | 155 ++++++++++++++++++++++----- drivers/gpu/drm/i915/intel_display.c | 48 +++++++-- drivers/gpu/drm/i915/intel_drv.h | 18 +++- 4 files changed, 186 insertions(+), 38 deletions(-)
These are all big patches, I would like to get an ack from the i915 developer(s) that these are acceptable for the stable tree before applying them...
thanks,
greg k-h
Hi Intel friends,
We have laptops like ASUS E406MA, which hits the issue: The audio playback does not work anymore after suspend & resume [1] Thanks for your contribution. We found the commit "drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled" can fix it. But, it induces the screen to flicker when the CDCLK changes on. So, we need these commits to be back ported to Linux stable 5.1.x tree to prevent flickering:
59f9e9cab3a1 drm/i915: Skip modeset for cdclk changes if possible 2b21dfbeee72 drm/i915: Remove redundant store of logical CDCLK state 48d9f87ddd21 drm/i915: Save the old CDCLK atomic state 905801fe7237 drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled
[1] https://bugzilla.kernel.org/show_bug.cgi?id=203623
May we have your comment or ack for the back port patches? https://www.spinics.net/lists/stable/msg308491.html https://www.spinics.net/lists/stable/msg310121.html https://www.spinics.net/lists/stable/msg310122.html https://www.spinics.net/lists/stable/msg310124.html https://www.spinics.net/lists/stable/msg310125.html
Thank you, Jian-Hong Pan
On Thu, Jun 13, 2019 at 04:37:48PM +0800, Jian-Hong Pan wrote:
Greg KH gregkh@linuxfoundation.org 於 2019年6月13日 週四 下午3:52寫道：
On Mon, Jun 10, 2019 at 02:01:39PM +0800, Jian-Hong Pan wrote:
Hi,
After apply the commit "drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled", it induces the screen to flicker when the CDCLK changes on the laptop like ASUS E406MA. [1]
So, we need these commits to prevent that: commit 48d9f87ddd21 drm/i915: Save the old CDCLK atomic state commit 2b21dfbeee72 drm/i915: Remove redundant store of logical CDCLK state commit 59f9e9cab3a1 drm/i915: Skip modeset for cdclk changes if possible
Jian-Hong Pan
Imre Deak (2): drm/i915: Save the old CDCLK atomic state drm/i915: Remove redundant store of logical CDCLK state
Ville Syrjälä (1): drm/i915: Skip modeset for cdclk changes if possible
drivers/gpu/drm/i915/i915_drv.h | 3 +- drivers/gpu/drm/i915/intel_cdclk.c | 155 ++++++++++++++++++++++----- drivers/gpu/drm/i915/intel_display.c | 48 +++++++-- drivers/gpu/drm/i915/intel_drv.h | 18 +++- 4 files changed, 186 insertions(+), 38 deletions(-)
These are all big patches, I would like to get an ack from the i915 developer(s) that these are acceptable for the stable tree before applying them...
thanks,
greg k-h
Hi Intel friends,
We have laptops like ASUS E406MA, which hits the issue: The audio playback does not work anymore after suspend & resume [1] Thanks for your contribution. We found the commit "drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled" can fix it. But, it induces the screen to flicker when the CDCLK changes on. So, we need these commits to be back ported to Linux stable 5.1.x tree to prevent flickering:
Pardon the delay.
Now that I refreshed my memory a bit I can't really see how this could fix anything, except by luck. Before these patches we always forced cdclk to be >=2*96 MHz so audio should never have hit this particular problem.
The reason for adding this extra complexity was to claw back a few milliwatts of power by allowing cdclk to drop below that magic limit when audio isn't needed.
I *think* these patches should probably work in 5.1, but for now I don't see this as anything more than bandaid for an unknown issue somewhere else. So I'd rather like a new bug filed at https://bugs.freedesktop.org/enter_bug.cgi?product=DRI&component=DRM/Int... with a full dmesg with drm.debug=0xe (+ some audio debug knob(s) which show when it's trying to poke the hw during suspend/resume) attached.
59f9e9cab3a1 drm/i915: Skip modeset for cdclk changes if possible 2b21dfbeee72 drm/i915: Remove redundant store of logical CDCLK state 48d9f87ddd21 drm/i915: Save the old CDCLK atomic state 905801fe7237 drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled
[1] https://bugzilla.kernel.org/show_bug.cgi?id=203623
May we have your comment or ack for the back port patches? https://www.spinics.net/lists/stable/msg308491.html https://www.spinics.net/lists/stable/msg310121.html https://www.spinics.net/lists/stable/msg310122.html https://www.spinics.net/lists/stable/msg310124.html https://www.spinics.net/lists/stable/msg310125.html
Thank you, Jian-Hong Pan
Ville Syrjälä ville.syrjala@linux.intel.com 於 2019年6月13日 週四 下午6:05寫道：
On Thu, Jun 13, 2019 at 04:37:48PM +0800, Jian-Hong Pan wrote:
Greg KH gregkh@linuxfoundation.org 於 2019年6月13日 週四 下午3:52寫道：
On Mon, Jun 10, 2019 at 02:01:39PM +0800, Jian-Hong Pan wrote:
Hi,
After apply the commit "drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled", it induces the screen to flicker when the CDCLK changes on the laptop like ASUS E406MA. [1]
So, we need these commits to prevent that: commit 48d9f87ddd21 drm/i915: Save the old CDCLK atomic state commit 2b21dfbeee72 drm/i915: Remove redundant store of logical CDCLK state commit 59f9e9cab3a1 drm/i915: Skip modeset for cdclk changes if possible
Jian-Hong Pan
Imre Deak (2): drm/i915: Save the old CDCLK atomic state drm/i915: Remove redundant store of logical CDCLK state
Ville Syrjälä (1): drm/i915: Skip modeset for cdclk changes if possible
drivers/gpu/drm/i915/i915_drv.h | 3 +- drivers/gpu/drm/i915/intel_cdclk.c | 155 ++++++++++++++++++++++----- drivers/gpu/drm/i915/intel_display.c | 48 +++++++-- drivers/gpu/drm/i915/intel_drv.h | 18 +++- 4 files changed, 186 insertions(+), 38 deletions(-)
These are all big patches, I would like to get an ack from the i915 developer(s) that these are acceptable for the stable tree before applying them...
thanks,
greg k-h
Hi Intel friends,
We have laptops like ASUS E406MA, which hits the issue: The audio playback does not work anymore after suspend & resume [1] Thanks for your contribution. We found the commit "drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled" can fix it. But, it induces the screen to flicker when the CDCLK changes on. So, we need these commits to be back ported to Linux stable 5.1.x tree to prevent flickering:
Pardon the delay.
Now that I refreshed my memory a bit I can't really see how this could fix anything, except by luck. Before these patches we always forced cdclk to be >=2*96 MHz so audio should never have hit this particular problem.
The reason for adding this extra complexity was to claw back a few milliwatts of power by allowing cdclk to drop below that magic limit when audio isn't needed.
I *think* these patches should probably work in 5.1, but for now I don't see this as anything more than bandaid for an unknown issue somewhere else. So I'd rather like a new bug filed at https://bugs.freedesktop.org/enter_bug.cgi?product=DRI&component=DRM/Int... with a full dmesg with drm.debug=0xe (+ some audio debug knob(s) which show when it's trying to poke the hw during suspend/resume) attached.
Thanks for your attention! I have filed the bug on freedesktop's bugzilla. https://bugs.freedesktop.org/show_bug.cgi?id=110916
I can upload more log information with guidance.
Jian-Hong Pan
59f9e9cab3a1 drm/i915: Skip modeset for cdclk changes if possible 2b21dfbeee72 drm/i915: Remove redundant store of logical CDCLK state 48d9f87ddd21 drm/i915: Save the old CDCLK atomic state 905801fe7237 drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled
[1] https://bugzilla.kernel.org/show_bug.cgi?id=203623
May we have your comment or ack for the back port patches? https://www.spinics.net/lists/stable/msg308491.html https://www.spinics.net/lists/stable/msg310121.html https://www.spinics.net/lists/stable/msg310122.html https://www.spinics.net/lists/stable/msg310124.html https://www.spinics.net/lists/stable/msg310125.html
Thank you, Jian-Hong Pan
-- Ville Syrjälä Intel
On Fri, Jun 14, 2019 at 01:59:51PM +0800, Jian-Hong Pan wrote:
Ville Syrjälä ville.syrjala@linux.intel.com 於 2019年6月13日 週四 下午6:05寫道：
On Thu, Jun 13, 2019 at 04:37:48PM +0800, Jian-Hong Pan wrote:
Greg KH gregkh@linuxfoundation.org 於 2019年6月13日 週四 下午3:52寫道：
On Mon, Jun 10, 2019 at 02:01:39PM +0800, Jian-Hong Pan wrote:
Hi,
After apply the commit "drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled", it induces the screen to flicker when the CDCLK changes on the laptop like ASUS E406MA. [1]
So, we need these commits to prevent that: commit 48d9f87ddd21 drm/i915: Save the old CDCLK atomic state commit 2b21dfbeee72 drm/i915: Remove redundant store of logical CDCLK state commit 59f9e9cab3a1 drm/i915: Skip modeset for cdclk changes if possible
Jian-Hong Pan
Imre Deak (2): drm/i915: Save the old CDCLK atomic state drm/i915: Remove redundant store of logical CDCLK state
Ville Syrjälä (1): drm/i915: Skip modeset for cdclk changes if possible
drivers/gpu/drm/i915/i915_drv.h | 3 +- drivers/gpu/drm/i915/intel_cdclk.c | 155 ++++++++++++++++++++++----- drivers/gpu/drm/i915/intel_display.c | 48 +++++++-- drivers/gpu/drm/i915/intel_drv.h | 18 +++- 4 files changed, 186 insertions(+), 38 deletions(-)
These are all big patches, I would like to get an ack from the i915 developer(s) that these are acceptable for the stable tree before applying them...
thanks,
greg k-h
Hi Intel friends,
We have laptops like ASUS E406MA, which hits the issue: The audio playback does not work anymore after suspend & resume [1] Thanks for your contribution. We found the commit "drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled" can fix it. But, it induces the screen to flicker when the CDCLK changes on. So, we need these commits to be back ported to Linux stable 5.1.x tree to prevent flickering:
Pardon the delay.
Now that I refreshed my memory a bit I can't really see how this could fix anything, except by luck. Before these patches we always forced cdclk to be >=2*96 MHz so audio should never have hit this particular problem.
The reason for adding this extra complexity was to claw back a few milliwatts of power by allowing cdclk to drop below that magic limit when audio isn't needed.
I *think* these patches should probably work in 5.1, but for now I don't see this as anything more than bandaid for an unknown issue somewhere else. So I'd rather like a new bug filed at https://bugs.freedesktop.org/enter_bug.cgi?product=DRI&component=DRM/Int... with a full dmesg with drm.debug=0xe (+ some audio debug knob(s) which show when it's trying to poke the hw during suspend/resume) attached.
Thanks for your attention! I have filed the bug on freedesktop's bugzilla. https://bugs.freedesktop.org/show_bug.cgi?id=110916
I can upload more log information with guidance.
OK, I think I see what's going on. We would just need to change a few lines of code to fix that (basically just s/0/2*96000/ in two places) but we don't want that upstream anymore due to these patches.
So I guess we need to go ahead with the backport. I read through them and checked the 5.1 baseline and couldn't spot any problems. So for the backport to 5.1: Acked-by: Ville Syrjälä ville.syrjala@linux.intel.com
From: Imre Deak imre.deak@intel.com
commit 48d9f87ddd2108663fd866b254e05d422243cc56 upstream.
The old state will be needed by an upcoming patch to determine if the commit increases or decreases CDCLK, so move the old state to the atomic state (while keeping the new one in dev_priv). cdclk.logical and cdclk.actual in the atomic state isn't used atm anywhere after the atomic check phase, so this should be safe.
v2: - Use swap() instead of opencoding it. (Ville)
Suggested-by: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Imre Deak imre.deak@intel.com Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com Link: https://patchwork.freedesktop.org/patch/msgid/20190320135439.12201-2-imre.de... Cc: stable@vger.kernel.org # 5.1.x Signed-off-by: Jian-Hong Pan jian-hong@endlessm.com --- drivers/gpu/drm/i915/intel_cdclk.c | 20 ++++++++++++++++++++ drivers/gpu/drm/i915/intel_display.c | 4 ++-- drivers/gpu/drm/i915/intel_drv.h | 1 + 3 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c index 553f57ff60f4..9814a6f2b3c4 100644 --- a/drivers/gpu/drm/i915/intel_cdclk.c +++ b/drivers/gpu/drm/i915/intel_cdclk.c @@ -2100,6 +2100,26 @@ bool intel_cdclk_changed(const struct intel_cdclk_state *a, a->voltage_level != b->voltage_level; }
+/** + * intel_cdclk_swap_state - make atomic CDCLK configuration effective + * @state: atomic state + * + * This is the CDCLK version of drm_atomic_helper_swap_state() since the + * helper does not handle driver-specific global state. + * + * Similarly to the atomic helpers this function does a complete swap, + * i.e. it also puts the old state into @state. This is used by the commit + * code to determine how CDCLK has changed (for instance did it increase or + * decrease). + */ +void intel_cdclk_swap_state(struct intel_atomic_state *state) +{ + struct drm_i915_private *dev_priv = to_i915(state->base.dev); + + swap(state->cdclk.logical, dev_priv->cdclk.logical); + swap(state->cdclk.actual, dev_priv->cdclk.actual); +} + void intel_dump_cdclk_state(const struct intel_cdclk_state *cdclk_state, const char *context) { diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index ebbac873b8f4..dd1a059cf850 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13459,10 +13459,10 @@ static int intel_atomic_commit(struct drm_device *dev, intel_state->min_voltage_level, sizeof(intel_state->min_voltage_level)); dev_priv->active_crtcs = intel_state->active_crtcs; - dev_priv->cdclk.logical = intel_state->cdclk.logical; - dev_priv->cdclk.actual = intel_state->cdclk.actual; dev_priv->cdclk.force_min_cdclk = intel_state->cdclk.force_min_cdclk; + + intel_cdclk_swap_state(intel_state); }
drm_atomic_state_get(state); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 539caca05da4..a9183579700d 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1597,6 +1597,7 @@ bool intel_cdclk_needs_modeset(const struct intel_cdclk_state *a, const struct intel_cdclk_state *b); bool intel_cdclk_changed(const struct intel_cdclk_state *a, const struct intel_cdclk_state *b); +void intel_cdclk_swap_state(struct intel_atomic_state *state); void intel_set_cdclk(struct drm_i915_private *dev_priv, const struct intel_cdclk_state *cdclk_state); void intel_dump_cdclk_state(const struct intel_cdclk_state *cdclk_state,
On Mon, Jun 10, 2019 at 02:01:41PM +0800, Jian-Hong Pan wrote:
From: Imre Deak imre.deak@intel.com
commit 48d9f87ddd2108663fd866b254e05d422243cc56 upstream.
The old state will be needed by an upcoming patch to determine if the commit increases or decreases CDCLK, so move the old state to the atomic state (while keeping the new one in dev_priv). cdclk.logical and cdclk.actual in the atomic state isn't used atm anywhere after the atomic check phase, so this should be safe.
v2:
- Use swap() instead of opencoding it. (Ville)
Suggested-by: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Imre Deak imre.deak@intel.com Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com Link: https://patchwork.freedesktop.org/patch/msgid/20190320135439.12201-2-imre.de... Cc: stable@vger.kernel.org # 5.1.x Signed-off-by: Jian-Hong Pan jian-hong@endlessm.com
drivers/gpu/drm/i915/intel_cdclk.c | 20 ++++++++++++++++++++ drivers/gpu/drm/i915/intel_display.c | 4 ++-- drivers/gpu/drm/i915/intel_drv.h | 1 + 3 files changed, 23 insertions(+), 2 deletions(-)
Does not apply against the latest 5.1.y tree. Can you rebase this series and resend please?
thanks,
greg k-h
Hi,
To make it more clearly, I re-send this patch series again.
To fix "the audio playback does not work anymore after suspend & resume" on ASUS E406MA, we need rediffed commit "drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled". However, after apply the commit "drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled", it induces the screen to flicker when the CDCLK changes on the laptop. [1]
So, we need these commits to prevent that:
commit 48d9f87ddd21 drm/i915: Save the old CDCLK atomic state commit 2b21dfbeee72 drm/i915: Remove redundant store of logical CDCLK state commit 59f9e9cab3a1 drm/i915: Skip modeset for cdclk changes if possible
This issue is also reconfirmed on the list. [2][3]
[1] https://bugzilla.kernel.org/show_bug.cgi?id=203623 [2] https://bugs.freedesktop.org/show_bug.cgi?id=110916 [3] https://www.spinics.net/lists/stable/msg310910.html
Thank you, Jian-Hong Pan
Imre Deak (2): drm/i915: Save the old CDCLK atomic state drm/i915: Remove redundant store of logical CDCLK state
Ville Syrjälä (2): drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled drm/i915: Skip modeset for cdclk changes if possible
drivers/gpu/drm/i915/i915_drv.h | 6 +- drivers/gpu/drm/i915/intel_audio.c | 62 ++++++++- drivers/gpu/drm/i915/intel_cdclk.c | 185 ++++++++++++++++++++------- drivers/gpu/drm/i915/intel_display.c | 57 +++++++-- drivers/gpu/drm/i915/intel_drv.h | 21 ++- 5 files changed, 269 insertions(+), 62 deletions(-)
From: Ville Syrjälä ville.syrjala@linux.intel.com
commit 905801fe72377b4dc53c6e13eea1a91c6a4aa0c4 upstream.
CDCLK has to be at least twice the BLCK regardless of audio. Audio driver has to probe using this hook and increase the clock even in absence of any display.
v2: Use atomic refcount for get_power, put_power so that we can call each once(Abhay). v3: Reset power well 2 to avoid any transaction on iDisp link during cdclk change(Abhay). v4: Remove Power well 2 reset workaround(Ville). v5: Remove unwanted Power well 2 register defined in v4(Abhay). v6: - Use a dedicated flag instead of state->modeset for min CDCLK changes - Make get/put audio power domain symmetric - Rebased on top of intel_wakeref tracking changes.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Abhay Kumar abhay.kumar@intel.com Tested-by: Abhay Kumar abhay.kumar@intel.com Signed-off-by: Imre Deak imre.deak@intel.com Reviewed-by: Clint Taylor Clinton.A.Taylor@intel.com Link: https://patchwork.freedesktop.org/patch/msgid/20190320135439.12201-1-imre.de... [jian-hong@endlessm.com: Rediff to keep i915_audio_component_get_power and i915_audio_component_put_power for Linux stable 5.1.x] Cc: stable@vger.kernel.org # 5.1.x Signed-off-by: Jian-Hong Pan jian-hong@endlessm.com Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=203623 Buglink: https://bugs.freedesktop.org/show_bug.cgi?id=110916 Link: https://www.spinics.net/lists/stable/msg310910.html --- drivers/gpu/drm/i915/i915_drv.h | 3 ++ drivers/gpu/drm/i915/intel_audio.c | 62 ++++++++++++++++++++++++++-- drivers/gpu/drm/i915/intel_cdclk.c | 30 +++++--------- drivers/gpu/drm/i915/intel_display.c | 9 +++- drivers/gpu/drm/i915/intel_drv.h | 3 ++ 5 files changed, 83 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index a67a63b5aa84..5c9bb1b2d1f3 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1622,6 +1622,8 @@ struct drm_i915_private { struct intel_cdclk_state actual; /* The current hardware cdclk state */ struct intel_cdclk_state hw; + + int force_min_cdclk; } cdclk;
/** @@ -1741,6 +1743,7 @@ struct drm_i915_private { * */ struct mutex av_mutex; + int audio_power_refcount;
struct { struct mutex mutex; diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 5104c6bbd66f..2f3fd5bf0f11 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -741,15 +741,71 @@ void intel_init_audio_hooks(struct drm_i915_private *dev_priv) } }
+static void glk_force_audio_cdclk(struct drm_i915_private *dev_priv, + bool enable) +{ + struct drm_modeset_acquire_ctx ctx; + struct drm_atomic_state *state; + int ret; + + drm_modeset_acquire_init(&ctx, 0); + state = drm_atomic_state_alloc(&dev_priv->drm); + if (WARN_ON(!state)) + return; + + state->acquire_ctx = &ctx; + +retry: + to_intel_atomic_state(state)->cdclk.force_min_cdclk_changed = true; + to_intel_atomic_state(state)->cdclk.force_min_cdclk = + enable ? 2 * 96000 : 0; + + /* + * Protects dev_priv->cdclk.force_min_cdclk + * Need to lock this here in case we have no active pipes + * and thus wouldn't lock it during the commit otherwise. + */ + ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, + &ctx); + if (!ret) + ret = drm_atomic_commit(state); + + if (ret == -EDEADLK) { + drm_atomic_state_clear(state); + drm_modeset_backoff(&ctx); + goto retry; + } + + WARN_ON(ret); + + drm_atomic_state_put(state); + + drm_modeset_drop_locks(&ctx); + drm_modeset_acquire_fini(&ctx); +} + static void i915_audio_component_get_power(struct device *kdev) { - intel_display_power_get(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO); + struct drm_i915_private *dev_priv = kdev_to_i915(kdev); + + intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO); + + /* Force CDCLK to 2*BCLK as long as we need audio to be powered. */ + if (dev_priv->audio_power_refcount++ == 0) + if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) + glk_force_audio_cdclk(dev_priv, true); }
static void i915_audio_component_put_power(struct device *kdev) { - intel_display_power_put_unchecked(kdev_to_i915(kdev), - POWER_DOMAIN_AUDIO); + struct drm_i915_private *dev_priv = kdev_to_i915(kdev); + + /* Stop forcing CDCLK to 2*BCLK if no need for audio to be powered. */ + if (--dev_priv->audio_power_refcount == 0) + if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) + glk_force_audio_cdclk(dev_priv, false); + + intel_display_power_put_unchecked(dev_priv, POWER_DOMAIN_AUDIO); }
static void i915_audio_component_codec_wake_override(struct device *kdev, diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c index 15ba950dee00..553f57ff60f4 100644 --- a/drivers/gpu/drm/i915/intel_cdclk.c +++ b/drivers/gpu/drm/i915/intel_cdclk.c @@ -2187,19 +2187,8 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state) /* * According to BSpec, "The CD clock frequency must be at least twice * the frequency of the Azalia BCLK." and BCLK is 96 MHz by default. - * - * FIXME: Check the actual, not default, BCLK being used. - * - * FIXME: This does not depend on ->has_audio because the higher CDCLK - * is required for audio probe, also when there are no audio capable - * displays connected at probe time. This leads to unnecessarily high - * CDCLK when audio is not required. - * - * FIXME: This limit is only applied when there are displays connected - * at probe time. If we probe without displays, we'll still end up using - * the platform minimum CDCLK, failing audio probe. */ - if (INTEL_GEN(dev_priv) >= 9) + if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9) min_cdclk = max(2 * 96000, min_cdclk);
/* @@ -2239,7 +2228,7 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state) intel_state->min_cdclk[i] = min_cdclk; }
- min_cdclk = 0; + min_cdclk = intel_state->cdclk.force_min_cdclk; for_each_pipe(dev_priv, pipe) min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk);
@@ -2300,7 +2289,8 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state) vlv_calc_voltage_level(dev_priv, cdclk);
if (!intel_state->active_crtcs) { - cdclk = vlv_calc_cdclk(dev_priv, 0); + cdclk = vlv_calc_cdclk(dev_priv, + intel_state->cdclk.force_min_cdclk);
intel_state->cdclk.actual.cdclk = cdclk; intel_state->cdclk.actual.voltage_level = @@ -2333,7 +2323,7 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state) bdw_calc_voltage_level(cdclk);
if (!intel_state->active_crtcs) { - cdclk = bdw_calc_cdclk(0); + cdclk = bdw_calc_cdclk(intel_state->cdclk.force_min_cdclk);
intel_state->cdclk.actual.cdclk = cdclk; intel_state->cdclk.actual.voltage_level = @@ -2405,7 +2395,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state) skl_calc_voltage_level(cdclk);
if (!intel_state->active_crtcs) { - cdclk = skl_calc_cdclk(0, vco); + cdclk = skl_calc_cdclk(intel_state->cdclk.force_min_cdclk, vco);
intel_state->cdclk.actual.vco = vco; intel_state->cdclk.actual.cdclk = cdclk; @@ -2444,10 +2434,10 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
if (!intel_state->active_crtcs) { if (IS_GEMINILAKE(dev_priv)) { - cdclk = glk_calc_cdclk(0); + cdclk = glk_calc_cdclk(intel_state->cdclk.force_min_cdclk); vco = glk_de_pll_vco(dev_priv, cdclk); } else { - cdclk = bxt_calc_cdclk(0); + cdclk = bxt_calc_cdclk(intel_state->cdclk.force_min_cdclk); vco = bxt_de_pll_vco(dev_priv, cdclk); }
@@ -2483,7 +2473,7 @@ static int cnl_modeset_calc_cdclk(struct drm_atomic_state *state) cnl_compute_min_voltage_level(intel_state));
if (!intel_state->active_crtcs) { - cdclk = cnl_calc_cdclk(0); + cdclk = cnl_calc_cdclk(intel_state->cdclk.force_min_cdclk); vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
intel_state->cdclk.actual.vco = vco; @@ -2519,7 +2509,7 @@ static int icl_modeset_calc_cdclk(struct drm_atomic_state *state) cnl_compute_min_voltage_level(intel_state));
if (!intel_state->active_crtcs) { - cdclk = icl_calc_cdclk(0, ref); + cdclk = icl_calc_cdclk(intel_state->cdclk.force_min_cdclk, ref); vco = icl_calc_cdclk_pll_vco(dev_priv, cdclk);
intel_state->cdclk.actual.vco = vco; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index cd8a22d6370e..1d7f750ad809 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12773,6 +12773,11 @@ static int intel_modeset_checks(struct drm_atomic_state *state) return -EINVAL; }
+ /* keep the current setting */ + if (!intel_state->cdclk.force_min_cdclk_changed) + intel_state->cdclk.force_min_cdclk = + dev_priv->cdclk.force_min_cdclk; + intel_state->modeset = true; intel_state->active_crtcs = dev_priv->active_crtcs; intel_state->cdclk.logical = dev_priv->cdclk.logical; @@ -12868,7 +12873,7 @@ static int intel_atomic_check(struct drm_device *dev, struct drm_crtc *crtc; struct drm_crtc_state *old_crtc_state, *crtc_state; int ret, i; - bool any_ms = false; + bool any_ms = intel_state->cdclk.force_min_cdclk_changed;
/* Catch I915_MODE_FLAG_INHERITED */ for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, @@ -13460,6 +13465,8 @@ static int intel_atomic_commit(struct drm_device *dev, dev_priv->active_crtcs = intel_state->active_crtcs; dev_priv->cdclk.logical = intel_state->cdclk.logical; dev_priv->cdclk.actual = intel_state->cdclk.actual; + dev_priv->cdclk.force_min_cdclk = + intel_state->cdclk.force_min_cdclk; }
drm_atomic_state_get(state); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 18e17b422701..824935b87a12 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -480,6 +480,9 @@ struct intel_atomic_state { * state only when all crtc's are DPMS off. */ struct intel_cdclk_state actual; + + int force_min_cdclk; + bool force_min_cdclk_changed; } cdclk;
bool dpll_set, modeset;
From: Imre Deak imre.deak@intel.com
commit 48d9f87ddd2108663fd866b254e05d422243cc56 upstream.
The old state will be needed by an upcoming patch to determine if the commit increases or decreases CDCLK, so move the old state to the atomic state (while keeping the new one in dev_priv). cdclk.logical and cdclk.actual in the atomic state isn't used atm anywhere after the atomic check phase, so this should be safe.
v2: - Use swap() instead of opencoding it. (Ville)
Suggested-by: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Imre Deak imre.deak@intel.com Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com Link: https://patchwork.freedesktop.org/patch/msgid/20190320135439.12201-2-imre.de... Cc: stable@vger.kernel.org # 5.1.x Signed-off-by: Jian-Hong Pan jian-hong@endlessm.com --- drivers/gpu/drm/i915/intel_cdclk.c | 20 ++++++++++++++++++++ drivers/gpu/drm/i915/intel_display.c | 4 ++-- drivers/gpu/drm/i915/intel_drv.h | 1 + 3 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c index 553f57ff60f4..9814a6f2b3c4 100644 --- a/drivers/gpu/drm/i915/intel_cdclk.c +++ b/drivers/gpu/drm/i915/intel_cdclk.c @@ -2100,6 +2100,26 @@ bool intel_cdclk_changed(const struct intel_cdclk_state *a, a->voltage_level != b->voltage_level; }
+/** + * intel_cdclk_swap_state - make atomic CDCLK configuration effective + * @state: atomic state + * + * This is the CDCLK version of drm_atomic_helper_swap_state() since the + * helper does not handle driver-specific global state. + * + * Similarly to the atomic helpers this function does a complete swap, + * i.e. it also puts the old state into @state. This is used by the commit + * code to determine how CDCLK has changed (for instance did it increase or + * decrease). + */ +void intel_cdclk_swap_state(struct intel_atomic_state *state) +{ + struct drm_i915_private *dev_priv = to_i915(state->base.dev); + + swap(state->cdclk.logical, dev_priv->cdclk.logical); + swap(state->cdclk.actual, dev_priv->cdclk.actual); +} + void intel_dump_cdclk_state(const struct intel_cdclk_state *cdclk_state, const char *context) { diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 1d7f750ad809..fb2a23830325 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13463,10 +13463,10 @@ static int intel_atomic_commit(struct drm_device *dev, intel_state->min_voltage_level, sizeof(intel_state->min_voltage_level)); dev_priv->active_crtcs = intel_state->active_crtcs; - dev_priv->cdclk.logical = intel_state->cdclk.logical; - dev_priv->cdclk.actual = intel_state->cdclk.actual; dev_priv->cdclk.force_min_cdclk = intel_state->cdclk.force_min_cdclk; + + intel_cdclk_swap_state(intel_state); }
drm_atomic_state_get(state); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 824935b87a12..2a11dfc28a2a 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1597,6 +1597,7 @@ bool intel_cdclk_needs_modeset(const struct intel_cdclk_state *a, const struct intel_cdclk_state *b); bool intel_cdclk_changed(const struct intel_cdclk_state *a, const struct intel_cdclk_state *b); +void intel_cdclk_swap_state(struct intel_atomic_state *state); void intel_set_cdclk(struct drm_i915_private *dev_priv, const struct intel_cdclk_state *cdclk_state); void intel_dump_cdclk_state(const struct intel_cdclk_state *cdclk_state,
From: Imre Deak imre.deak@intel.com
commit 2b21dfbeee725778daed2c3dd45a3fc808176feb upstream.
We copied the original state into the atomic state already earlier in the function, so no need to do it a second time.
Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Imre Deak imre.deak@intel.com Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com Link: https://patchwork.freedesktop.org/patch/msgid/20190320135439.12201-3-imre.de... Cc: stable@vger.kernel.org # 5.1.x Signed-off-by: Jian-Hong Pan jian-hong@endlessm.com --- drivers/gpu/drm/i915/intel_display.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index fb2a23830325..d8fabe4643d9 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12831,8 +12831,6 @@ static int intel_modeset_checks(struct drm_atomic_state *state) DRM_DEBUG_KMS("New voltage level calculated to be logical %u, actual %u\n", intel_state->cdclk.logical.voltage_level, intel_state->cdclk.actual.voltage_level); - } else { - to_intel_atomic_state(state)->cdclk.logical = dev_priv->cdclk.logical; }
intel_modeset_clear_plls(state);
From: Ville Syrjälä ville.syrjala@linux.intel.com
commit 59f9e9cab3a1e6762fb707d0d829b982930f1349 upstream.
If we have only a single active pipe and the cdclk change only requires the cd2x divider to be updated bxt+ can do the update with forcing a full modeset on the pipe. Try to hook that up.
v2: - Wait for vblank after an optimized CDCLK change. - Avoid optimization if the pipe needs a modeset (or was disabled). - Split CDCLK change to a pre/post plane update step. v3: - Use correct version of CDCLK state as old state. (Ville) - Remove unused intel_cdclk_can_skip_modeset() v4: - For consistency call intel_set_cdclk_post_plane_update() only during modesets (and not fastsets). v5: - Remove the logic to update the CD2X divider on-the-fly on ICL, since only a divider of 1 is supported there. Clint also noticed that the pipe select bits in CDCLK_CTL are oddly defined on ICL, it's not clear yet whether that's only an error in the specification.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Abhay Kumar abhay.kumar@intel.com Tested-by: Abhay Kumar abhay.kumar@intel.com Signed-off-by: Imre Deak imre.deak@intel.com Reviewed-by: Clint Taylor Clinton.A.Taylor@intel.com Link: https://patchwork.freedesktop.org/patch/msgid/20190327101321.3095-1-imre.dea... Cc: stable@vger.kernel.org # 5.1.x Signed-off-by: Jian-Hong Pan jian-hong@endlessm.com --- drivers/gpu/drm/i915/i915_drv.h | 3 +- drivers/gpu/drm/i915/intel_cdclk.c | 135 +++++++++++++++++++++------ drivers/gpu/drm/i915/intel_display.c | 42 ++++++++- drivers/gpu/drm/i915/intel_drv.h | 17 +++- 4 files changed, 163 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 5c9bb1b2d1f3..0c4a76bca5c6 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -280,7 +280,8 @@ struct drm_i915_display_funcs { void (*get_cdclk)(struct drm_i915_private *dev_priv, struct intel_cdclk_state *cdclk_state); void (*set_cdclk)(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state); + const struct intel_cdclk_state *cdclk_state, + enum pipe pipe); int (*get_fifo_size)(struct drm_i915_private *dev_priv, enum i9xx_plane_id i9xx_plane); int (*compute_pipe_wm)(struct intel_crtc_state *cstate); diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c index 9814a6f2b3c4..00f76261924c 100644 --- a/drivers/gpu/drm/i915/intel_cdclk.c +++ b/drivers/gpu/drm/i915/intel_cdclk.c @@ -516,7 +516,8 @@ static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv) }
static void vlv_set_cdclk(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state) + const struct intel_cdclk_state *cdclk_state, + enum pipe pipe) { int cdclk = cdclk_state->cdclk; u32 val, cmd = cdclk_state->voltage_level; @@ -598,7 +599,8 @@ static void vlv_set_cdclk(struct drm_i915_private *dev_priv, }
static void chv_set_cdclk(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state) + const struct intel_cdclk_state *cdclk_state, + enum pipe pipe) { int cdclk = cdclk_state->cdclk; u32 val, cmd = cdclk_state->voltage_level; @@ -697,7 +699,8 @@ static void bdw_get_cdclk(struct drm_i915_private *dev_priv, }
static void bdw_set_cdclk(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state) + const struct intel_cdclk_state *cdclk_state, + enum pipe pipe) { int cdclk = cdclk_state->cdclk; u32 val; @@ -987,7 +990,8 @@ static void skl_dpll0_disable(struct drm_i915_private *dev_priv) }
static void skl_set_cdclk(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state) + const struct intel_cdclk_state *cdclk_state, + enum pipe pipe) { int cdclk = cdclk_state->cdclk; int vco = cdclk_state->vco; @@ -1158,7 +1162,7 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv) cdclk_state.cdclk = skl_calc_cdclk(0, cdclk_state.vco); cdclk_state.voltage_level = skl_calc_voltage_level(cdclk_state.cdclk);
- skl_set_cdclk(dev_priv, &cdclk_state); + skl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE); }
/** @@ -1176,7 +1180,7 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv) cdclk_state.vco = 0; cdclk_state.voltage_level = skl_calc_voltage_level(cdclk_state.cdclk);
- skl_set_cdclk(dev_priv, &cdclk_state); + skl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE); }
static int bxt_calc_cdclk(int min_cdclk) @@ -1355,7 +1359,8 @@ static void bxt_de_pll_enable(struct drm_i915_private *dev_priv, int vco) }
static void bxt_set_cdclk(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state) + const struct intel_cdclk_state *cdclk_state, + enum pipe pipe) { int cdclk = cdclk_state->cdclk; int vco = cdclk_state->vco; @@ -1408,11 +1413,10 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv, bxt_de_pll_enable(dev_priv, vco);
val = divider | skl_cdclk_decimal(cdclk); - /* - * FIXME if only the cd2x divider needs changing, it could be done - * without shutting off the pipe (if only one pipe is active). - */ - val |= BXT_CDCLK_CD2X_PIPE_NONE; + if (pipe == INVALID_PIPE) + val |= BXT_CDCLK_CD2X_PIPE_NONE; + else + val |= BXT_CDCLK_CD2X_PIPE(pipe); /* * Disable SSA Precharge when CD clock frequency < 500 MHz, * enable otherwise. @@ -1421,6 +1425,9 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv, val |= BXT_CDCLK_SSA_PRECHARGE_ENABLE; I915_WRITE(CDCLK_CTL, val);
+ if (pipe != INVALID_PIPE) + intel_wait_for_vblank(dev_priv, pipe); + mutex_lock(&dev_priv->pcu_lock); /* * The timeout isn't specified, the 2ms used here is based on @@ -1525,7 +1532,7 @@ void bxt_init_cdclk(struct drm_i915_private *dev_priv) } cdclk_state.voltage_level = bxt_calc_voltage_level(cdclk_state.cdclk);
- bxt_set_cdclk(dev_priv, &cdclk_state); + bxt_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE); }
/** @@ -1543,7 +1550,7 @@ void bxt_uninit_cdclk(struct drm_i915_private *dev_priv) cdclk_state.vco = 0; cdclk_state.voltage_level = bxt_calc_voltage_level(cdclk_state.cdclk);
- bxt_set_cdclk(dev_priv, &cdclk_state); + bxt_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE); }
static int cnl_calc_cdclk(int min_cdclk) @@ -1663,7 +1670,8 @@ static void cnl_cdclk_pll_enable(struct drm_i915_private *dev_priv, int vco) }
static void cnl_set_cdclk(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state) + const struct intel_cdclk_state *cdclk_state, + enum pipe pipe) { int cdclk = cdclk_state->cdclk; int vco = cdclk_state->vco; @@ -1704,13 +1712,15 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv, cnl_cdclk_pll_enable(dev_priv, vco);
val = divider | skl_cdclk_decimal(cdclk); - /* - * FIXME if only the cd2x divider needs changing, it could be done - * without shutting off the pipe (if only one pipe is active). - */ - val |= BXT_CDCLK_CD2X_PIPE_NONE; + if (pipe == INVALID_PIPE) + val |= BXT_CDCLK_CD2X_PIPE_NONE; + else + val |= BXT_CDCLK_CD2X_PIPE(pipe); I915_WRITE(CDCLK_CTL, val);
+ if (pipe != INVALID_PIPE) + intel_wait_for_vblank(dev_priv, pipe); + /* inform PCU of the change */ mutex_lock(&dev_priv->pcu_lock); sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL, @@ -1847,7 +1857,8 @@ static int icl_calc_cdclk_pll_vco(struct drm_i915_private *dev_priv, int cdclk) }
static void icl_set_cdclk(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state) + const struct intel_cdclk_state *cdclk_state, + enum pipe pipe) { unsigned int cdclk = cdclk_state->cdclk; unsigned int vco = cdclk_state->vco; @@ -1872,6 +1883,11 @@ static void icl_set_cdclk(struct drm_i915_private *dev_priv, if (dev_priv->cdclk.hw.vco != vco) cnl_cdclk_pll_enable(dev_priv, vco);
+ /* + * On ICL CD2X_DIV can only be 1, so we'll never end up changing the + * divider here synchronized to a pipe while CDCLK is on, nor will we + * need the corresponding vblank wait. + */ I915_WRITE(CDCLK_CTL, ICL_CDCLK_CD2X_PIPE_NONE | skl_cdclk_decimal(cdclk));
@@ -2002,7 +2018,7 @@ void icl_init_cdclk(struct drm_i915_private *dev_priv) sanitized_state.voltage_level = icl_calc_voltage_level(sanitized_state.cdclk);
- icl_set_cdclk(dev_priv, &sanitized_state); + icl_set_cdclk(dev_priv, &sanitized_state, INVALID_PIPE); }
/** @@ -2020,7 +2036,7 @@ void icl_uninit_cdclk(struct drm_i915_private *dev_priv) cdclk_state.vco = 0; cdclk_state.voltage_level = icl_calc_voltage_level(cdclk_state.cdclk);
- icl_set_cdclk(dev_priv, &cdclk_state); + icl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE); }
/** @@ -2048,7 +2064,7 @@ void cnl_init_cdclk(struct drm_i915_private *dev_priv) cdclk_state.vco = cnl_cdclk_pll_vco(dev_priv, cdclk_state.cdclk); cdclk_state.voltage_level = cnl_calc_voltage_level(cdclk_state.cdclk);
- cnl_set_cdclk(dev_priv, &cdclk_state); + cnl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE); }
/** @@ -2066,7 +2082,7 @@ void cnl_uninit_cdclk(struct drm_i915_private *dev_priv) cdclk_state.vco = 0; cdclk_state.voltage_level = cnl_calc_voltage_level(cdclk_state.cdclk);
- cnl_set_cdclk(dev_priv, &cdclk_state); + cnl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE); }
/** @@ -2085,6 +2101,27 @@ bool intel_cdclk_needs_modeset(const struct intel_cdclk_state *a, a->ref != b->ref; }
+/** + * intel_cdclk_needs_cd2x_update - Determine if two CDCLK states require a cd2x divider update + * @a: first CDCLK state + * @b: second CDCLK state + * + * Returns: + * True if the CDCLK states require just a cd2x divider update, false if not. + */ +bool intel_cdclk_needs_cd2x_update(struct drm_i915_private *dev_priv, + const struct intel_cdclk_state *a, + const struct intel_cdclk_state *b) +{ + /* Older hw doesn't have the capability */ + if (INTEL_GEN(dev_priv) < 10 && !IS_GEN9_LP(dev_priv)) + return false; + + return a->cdclk != b->cdclk && + a->vco == b->vco && + a->ref == b->ref; +} + /** * intel_cdclk_changed - Determine if two CDCLK states are different * @a: first CDCLK state @@ -2133,12 +2170,14 @@ void intel_dump_cdclk_state(const struct intel_cdclk_state *cdclk_state, * intel_set_cdclk - Push the CDCLK state to the hardware * @dev_priv: i915 device * @cdclk_state: new CDCLK state + * @pipe: pipe with which to synchronize the update * * Program the hardware based on the passed in CDCLK state, * if necessary. */ -void intel_set_cdclk(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state) +static void intel_set_cdclk(struct drm_i915_private *dev_priv, + const struct intel_cdclk_state *cdclk_state, + enum pipe pipe) { if (!intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_state)) return; @@ -2148,7 +2187,7 @@ void intel_set_cdclk(struct drm_i915_private *dev_priv,
intel_dump_cdclk_state(cdclk_state, "Changing CDCLK to");
- dev_priv->display.set_cdclk(dev_priv, cdclk_state); + dev_priv->display.set_cdclk(dev_priv, cdclk_state, pipe);
if (WARN(intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_state), "cdclk state doesn't match!\n")) { @@ -2157,6 +2196,46 @@ void intel_set_cdclk(struct drm_i915_private *dev_priv, } }
+/** + * intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware + * @dev_priv: i915 device + * @old_state: old CDCLK state + * @new_state: new CDCLK state + * @pipe: pipe with which to synchronize the update + * + * Program the hardware before updating the HW plane state based on the passed + * in CDCLK state, if necessary. + */ +void +intel_set_cdclk_pre_plane_update(struct drm_i915_private *dev_priv, + const struct intel_cdclk_state *old_state, + const struct intel_cdclk_state *new_state, + enum pipe pipe) +{ + if (pipe == INVALID_PIPE || old_state->cdclk <= new_state->cdclk) + intel_set_cdclk(dev_priv, new_state, pipe); +} + +/** + * intel_set_cdclk_post_plane_update - Push the CDCLK state to the hardware + * @dev_priv: i915 device + * @old_state: old CDCLK state + * @new_state: new CDCLK state + * @pipe: pipe with which to synchronize the update + * + * Program the hardware after updating the HW plane state based on the passed + * in CDCLK state, if necessary. + */ +void +intel_set_cdclk_post_plane_update(struct drm_i915_private *dev_priv, + const struct intel_cdclk_state *old_state, + const struct intel_cdclk_state *new_state, + enum pipe pipe) +{ + if (pipe != INVALID_PIPE && old_state->cdclk > new_state->cdclk) + intel_set_cdclk(dev_priv, new_state, pipe); +} + static int intel_pixel_rate_to_cdclk(struct drm_i915_private *dev_priv, int pixel_rate) { diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d8fabe4643d9..d7c05c7750ae 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12782,6 +12782,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state) intel_state->active_crtcs = dev_priv->active_crtcs; intel_state->cdclk.logical = dev_priv->cdclk.logical; intel_state->cdclk.actual = dev_priv->cdclk.actual; + intel_state->cdclk.pipe = INVALID_PIPE;
for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { if (new_crtc_state->active) @@ -12801,6 +12802,8 @@ static int intel_modeset_checks(struct drm_atomic_state *state) * adjusted_mode bits in the crtc directly. */ if (dev_priv->display.modeset_calc_cdclk) { + enum pipe pipe; + ret = dev_priv->display.modeset_calc_cdclk(state); if (ret < 0) return ret; @@ -12817,12 +12820,36 @@ static int intel_modeset_checks(struct drm_atomic_state *state) return ret; }
+ if (is_power_of_2(intel_state->active_crtcs)) { + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; + + pipe = ilog2(intel_state->active_crtcs); + crtc = &intel_get_crtc_for_pipe(dev_priv, pipe)->base; + crtc_state = drm_atomic_get_new_crtc_state(state, crtc); + if (crtc_state && needs_modeset(crtc_state)) + pipe = INVALID_PIPE; + } else { + pipe = INVALID_PIPE; + } + /* All pipes must be switched off while we change the cdclk. */ - if (intel_cdclk_needs_modeset(&dev_priv->cdclk.actual, - &intel_state->cdclk.actual)) { + if (pipe != INVALID_PIPE && + intel_cdclk_needs_cd2x_update(dev_priv, + &dev_priv->cdclk.actual, + &intel_state->cdclk.actual)) { + ret = intel_lock_all_pipes(state); + if (ret < 0) + return ret; + + intel_state->cdclk.pipe = pipe; + } else if (intel_cdclk_needs_modeset(&dev_priv->cdclk.actual, + &intel_state->cdclk.actual)) { ret = intel_modeset_all_pipes(state); if (ret < 0) return ret; + + intel_state->cdclk.pipe = INVALID_PIPE; }
DRM_DEBUG_KMS("New cdclk calculated to be logical %u kHz, actual %u kHz\n", @@ -13231,7 +13258,10 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) if (intel_state->modeset) { drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
- intel_set_cdclk(dev_priv, &dev_priv->cdclk.actual); + intel_set_cdclk_pre_plane_update(dev_priv, + &intel_state->cdclk.actual, + &dev_priv->cdclk.actual, + intel_state->cdclk.pipe);
/* * SKL workaround: bspec recommends we disable the SAGV when we @@ -13260,6 +13290,12 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) /* Now enable the clocks, plane, pipe, and connectors that we set up. */ dev_priv->display.update_crtcs(state);
+ if (intel_state->modeset) + intel_set_cdclk_post_plane_update(dev_priv, + &intel_state->cdclk.actual, + &dev_priv->cdclk.actual, + intel_state->cdclk.pipe); + /* FIXME: We should call drm_atomic_helper_commit_hw_done() here * already, but still need the state for the delayed optimization. To * fix this: diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 2a11dfc28a2a..ceaa05dfa0d0 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -483,6 +483,8 @@ struct intel_atomic_state {
int force_min_cdclk; bool force_min_cdclk_changed; + /* pipe to which cd2x update is synchronized */ + enum pipe pipe; } cdclk;
bool dpll_set, modeset; @@ -1593,13 +1595,24 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv); void intel_update_max_cdclk(struct drm_i915_private *dev_priv); void intel_update_cdclk(struct drm_i915_private *dev_priv); void intel_update_rawclk(struct drm_i915_private *dev_priv); +bool intel_cdclk_needs_cd2x_update(struct drm_i915_private *dev_priv, + const struct intel_cdclk_state *a, + const struct intel_cdclk_state *b); bool intel_cdclk_needs_modeset(const struct intel_cdclk_state *a, const struct intel_cdclk_state *b); bool intel_cdclk_changed(const struct intel_cdclk_state *a, const struct intel_cdclk_state *b); void intel_cdclk_swap_state(struct intel_atomic_state *state); -void intel_set_cdclk(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state); +void +intel_set_cdclk_pre_plane_update(struct drm_i915_private *dev_priv, + const struct intel_cdclk_state *old_state, + const struct intel_cdclk_state *new_state, + enum pipe pipe); +void +intel_set_cdclk_post_plane_update(struct drm_i915_private *dev_priv, + const struct intel_cdclk_state *old_state, + const struct intel_cdclk_state *new_state, + enum pipe pipe); void intel_dump_cdclk_state(const struct intel_cdclk_state *cdclk_state, const char *context);
On Fri, Jun 21, 2019 at 06:07:12PM +0800, Jian-Hong Pan wrote:
Hi,
To make it more clearly, I re-send this patch series again.
To fix "the audio playback does not work anymore after suspend & resume" on ASUS E406MA, we need rediffed commit "drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled". However, after apply the commit "drm/i915: Force 2*96 MHz cdclk on glk/cnl when audio power is enabled", it induces the screen to flicker when the CDCLK changes on the laptop. [1]
So, we need these commits to prevent that:
commit 48d9f87ddd21 drm/i915: Save the old CDCLK atomic state commit 2b21dfbeee72 drm/i915: Remove redundant store of logical CDCLK state commit 59f9e9cab3a1 drm/i915: Skip modeset for cdclk changes if possible
This issue is also reconfirmed on the list. [2][3]
[1] https://bugzilla.kernel.org/show_bug.cgi?id=203623 [2] https://bugs.freedesktop.org/show_bug.cgi?id=110916 [3] https://www.spinics.net/lists/stable/msg310910.html
Now queued up, thanks.
greg k-h
From: Imre Deak imre.deak@intel.com
commit 2b21dfbeee725778daed2c3dd45a3fc808176feb upstream.
We copied the original state into the atomic state already earlier in the function, so no need to do it a second time.
Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Imre Deak imre.deak@intel.com Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com Link: https://patchwork.freedesktop.org/patch/msgid/20190320135439.12201-3-imre.de... Cc: stable@vger.kernel.org # 5.1.x Signed-off-by: Jian-Hong Pan jian-hong@endlessm.com --- drivers/gpu/drm/i915/intel_display.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index dd1a059cf850..aa16804687c2 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12827,8 +12827,6 @@ static int intel_modeset_checks(struct drm_atomic_state *state) DRM_DEBUG_KMS("New voltage level calculated to be logical %u, actual %u\n", intel_state->cdclk.logical.voltage_level, intel_state->cdclk.actual.voltage_level); - } else { - to_intel_atomic_state(state)->cdclk.logical = dev_priv->cdclk.logical; }
intel_modeset_clear_plls(state);
From: Ville Syrjälä ville.syrjala@linux.intel.com
commit 59f9e9cab3a1e6762fb707d0d829b982930f1349 upstream.
If we have only a single active pipe and the cdclk change only requires the cd2x divider to be updated bxt+ can do the update with forcing a full modeset on the pipe. Try to hook that up.
v2: - Wait for vblank after an optimized CDCLK change. - Avoid optimization if the pipe needs a modeset (or was disabled). - Split CDCLK change to a pre/post plane update step. v3: - Use correct version of CDCLK state as old state. (Ville) - Remove unused intel_cdclk_can_skip_modeset() v4: - For consistency call intel_set_cdclk_post_plane_update() only during modesets (and not fastsets). v5: - Remove the logic to update the CD2X divider on-the-fly on ICL, since only a divider of 1 is supported there. Clint also noticed that the pipe select bits in CDCLK_CTL are oddly defined on ICL, it's not clear yet whether that's only an error in the specification.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Abhay Kumar abhay.kumar@intel.com Tested-by: Abhay Kumar abhay.kumar@intel.com Signed-off-by: Imre Deak imre.deak@intel.com Reviewed-by: Clint Taylor Clinton.A.Taylor@intel.com Link: https://patchwork.freedesktop.org/patch/msgid/20190327101321.3095-1-imre.dea... Cc: stable@vger.kernel.org # 5.1.x Signed-off-by: Jian-Hong Pan jian-hong@endlessm.com --- drivers/gpu/drm/i915/i915_drv.h | 3 +- drivers/gpu/drm/i915/intel_cdclk.c | 135 +++++++++++++++++++++------ drivers/gpu/drm/i915/intel_display.c | 42 ++++++++- drivers/gpu/drm/i915/intel_drv.h | 17 +++- 4 files changed, 163 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 5c9bb1b2d1f3..0c4a76bca5c6 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -280,7 +280,8 @@ struct drm_i915_display_funcs { void (*get_cdclk)(struct drm_i915_private *dev_priv, struct intel_cdclk_state *cdclk_state); void (*set_cdclk)(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state); + const struct intel_cdclk_state *cdclk_state, + enum pipe pipe); int (*get_fifo_size)(struct drm_i915_private *dev_priv, enum i9xx_plane_id i9xx_plane); int (*compute_pipe_wm)(struct intel_crtc_state *cstate); diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c index 9814a6f2b3c4..00f76261924c 100644 --- a/drivers/gpu/drm/i915/intel_cdclk.c +++ b/drivers/gpu/drm/i915/intel_cdclk.c @@ -516,7 +516,8 @@ static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv) }
static void vlv_set_cdclk(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state) + const struct intel_cdclk_state *cdclk_state, + enum pipe pipe) { int cdclk = cdclk_state->cdclk; u32 val, cmd = cdclk_state->voltage_level; @@ -598,7 +599,8 @@ static void vlv_set_cdclk(struct drm_i915_private *dev_priv, }
static void chv_set_cdclk(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state) + const struct intel_cdclk_state *cdclk_state, + enum pipe pipe) { int cdclk = cdclk_state->cdclk; u32 val, cmd = cdclk_state->voltage_level; @@ -697,7 +699,8 @@ static void bdw_get_cdclk(struct drm_i915_private *dev_priv, }
static void bdw_set_cdclk(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state) + const struct intel_cdclk_state *cdclk_state, + enum pipe pipe) { int cdclk = cdclk_state->cdclk; u32 val; @@ -987,7 +990,8 @@ static void skl_dpll0_disable(struct drm_i915_private *dev_priv) }
static void skl_set_cdclk(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state) + const struct intel_cdclk_state *cdclk_state, + enum pipe pipe) { int cdclk = cdclk_state->cdclk; int vco = cdclk_state->vco; @@ -1158,7 +1162,7 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv) cdclk_state.cdclk = skl_calc_cdclk(0, cdclk_state.vco); cdclk_state.voltage_level = skl_calc_voltage_level(cdclk_state.cdclk);
- skl_set_cdclk(dev_priv, &cdclk_state); + skl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE); }
/** @@ -1176,7 +1180,7 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv) cdclk_state.vco = 0; cdclk_state.voltage_level = skl_calc_voltage_level(cdclk_state.cdclk);
- skl_set_cdclk(dev_priv, &cdclk_state); + skl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE); }
static int bxt_calc_cdclk(int min_cdclk) @@ -1355,7 +1359,8 @@ static void bxt_de_pll_enable(struct drm_i915_private *dev_priv, int vco) }
static void bxt_set_cdclk(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state) + const struct intel_cdclk_state *cdclk_state, + enum pipe pipe) { int cdclk = cdclk_state->cdclk; int vco = cdclk_state->vco; @@ -1408,11 +1413,10 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv, bxt_de_pll_enable(dev_priv, vco);
val = divider | skl_cdclk_decimal(cdclk); - /* - * FIXME if only the cd2x divider needs changing, it could be done - * without shutting off the pipe (if only one pipe is active). - */ - val |= BXT_CDCLK_CD2X_PIPE_NONE; + if (pipe == INVALID_PIPE) + val |= BXT_CDCLK_CD2X_PIPE_NONE; + else + val |= BXT_CDCLK_CD2X_PIPE(pipe); /* * Disable SSA Precharge when CD clock frequency < 500 MHz, * enable otherwise. @@ -1421,6 +1425,9 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv, val |= BXT_CDCLK_SSA_PRECHARGE_ENABLE; I915_WRITE(CDCLK_CTL, val);
+ if (pipe != INVALID_PIPE) + intel_wait_for_vblank(dev_priv, pipe); + mutex_lock(&dev_priv->pcu_lock); /* * The timeout isn't specified, the 2ms used here is based on @@ -1525,7 +1532,7 @@ void bxt_init_cdclk(struct drm_i915_private *dev_priv) } cdclk_state.voltage_level = bxt_calc_voltage_level(cdclk_state.cdclk);
- bxt_set_cdclk(dev_priv, &cdclk_state); + bxt_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE); }
/** @@ -1543,7 +1550,7 @@ void bxt_uninit_cdclk(struct drm_i915_private *dev_priv) cdclk_state.vco = 0; cdclk_state.voltage_level = bxt_calc_voltage_level(cdclk_state.cdclk);
- bxt_set_cdclk(dev_priv, &cdclk_state); + bxt_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE); }
static int cnl_calc_cdclk(int min_cdclk) @@ -1663,7 +1670,8 @@ static void cnl_cdclk_pll_enable(struct drm_i915_private *dev_priv, int vco) }
static void cnl_set_cdclk(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state) + const struct intel_cdclk_state *cdclk_state, + enum pipe pipe) { int cdclk = cdclk_state->cdclk; int vco = cdclk_state->vco; @@ -1704,13 +1712,15 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv, cnl_cdclk_pll_enable(dev_priv, vco);
val = divider | skl_cdclk_decimal(cdclk); - /* - * FIXME if only the cd2x divider needs changing, it could be done - * without shutting off the pipe (if only one pipe is active). - */ - val |= BXT_CDCLK_CD2X_PIPE_NONE; + if (pipe == INVALID_PIPE) + val |= BXT_CDCLK_CD2X_PIPE_NONE; + else + val |= BXT_CDCLK_CD2X_PIPE(pipe); I915_WRITE(CDCLK_CTL, val);
+ if (pipe != INVALID_PIPE) + intel_wait_for_vblank(dev_priv, pipe); + /* inform PCU of the change */ mutex_lock(&dev_priv->pcu_lock); sandybridge_pcode_write(dev_priv, SKL_PCODE_CDCLK_CONTROL, @@ -1847,7 +1857,8 @@ static int icl_calc_cdclk_pll_vco(struct drm_i915_private *dev_priv, int cdclk) }
static void icl_set_cdclk(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state) + const struct intel_cdclk_state *cdclk_state, + enum pipe pipe) { unsigned int cdclk = cdclk_state->cdclk; unsigned int vco = cdclk_state->vco; @@ -1872,6 +1883,11 @@ static void icl_set_cdclk(struct drm_i915_private *dev_priv, if (dev_priv->cdclk.hw.vco != vco) cnl_cdclk_pll_enable(dev_priv, vco);
+ /* + * On ICL CD2X_DIV can only be 1, so we'll never end up changing the + * divider here synchronized to a pipe while CDCLK is on, nor will we + * need the corresponding vblank wait. + */ I915_WRITE(CDCLK_CTL, ICL_CDCLK_CD2X_PIPE_NONE | skl_cdclk_decimal(cdclk));
@@ -2002,7 +2018,7 @@ void icl_init_cdclk(struct drm_i915_private *dev_priv) sanitized_state.voltage_level = icl_calc_voltage_level(sanitized_state.cdclk);
- icl_set_cdclk(dev_priv, &sanitized_state); + icl_set_cdclk(dev_priv, &sanitized_state, INVALID_PIPE); }
/** @@ -2020,7 +2036,7 @@ void icl_uninit_cdclk(struct drm_i915_private *dev_priv) cdclk_state.vco = 0; cdclk_state.voltage_level = icl_calc_voltage_level(cdclk_state.cdclk);
- icl_set_cdclk(dev_priv, &cdclk_state); + icl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE); }
/** @@ -2048,7 +2064,7 @@ void cnl_init_cdclk(struct drm_i915_private *dev_priv) cdclk_state.vco = cnl_cdclk_pll_vco(dev_priv, cdclk_state.cdclk); cdclk_state.voltage_level = cnl_calc_voltage_level(cdclk_state.cdclk);
- cnl_set_cdclk(dev_priv, &cdclk_state); + cnl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE); }
/** @@ -2066,7 +2082,7 @@ void cnl_uninit_cdclk(struct drm_i915_private *dev_priv) cdclk_state.vco = 0; cdclk_state.voltage_level = cnl_calc_voltage_level(cdclk_state.cdclk);
- cnl_set_cdclk(dev_priv, &cdclk_state); + cnl_set_cdclk(dev_priv, &cdclk_state, INVALID_PIPE); }
/** @@ -2085,6 +2101,27 @@ bool intel_cdclk_needs_modeset(const struct intel_cdclk_state *a, a->ref != b->ref; }
+/** + * intel_cdclk_needs_cd2x_update - Determine if two CDCLK states require a cd2x divider update + * @a: first CDCLK state + * @b: second CDCLK state + * + * Returns: + * True if the CDCLK states require just a cd2x divider update, false if not. + */ +bool intel_cdclk_needs_cd2x_update(struct drm_i915_private *dev_priv, + const struct intel_cdclk_state *a, + const struct intel_cdclk_state *b) +{ + /* Older hw doesn't have the capability */ + if (INTEL_GEN(dev_priv) < 10 && !IS_GEN9_LP(dev_priv)) + return false; + + return a->cdclk != b->cdclk && + a->vco == b->vco && + a->ref == b->ref; +} + /** * intel_cdclk_changed - Determine if two CDCLK states are different * @a: first CDCLK state @@ -2133,12 +2170,14 @@ void intel_dump_cdclk_state(const struct intel_cdclk_state *cdclk_state, * intel_set_cdclk - Push the CDCLK state to the hardware * @dev_priv: i915 device * @cdclk_state: new CDCLK state + * @pipe: pipe with which to synchronize the update * * Program the hardware based on the passed in CDCLK state, * if necessary. */ -void intel_set_cdclk(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state) +static void intel_set_cdclk(struct drm_i915_private *dev_priv, + const struct intel_cdclk_state *cdclk_state, + enum pipe pipe) { if (!intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_state)) return; @@ -2148,7 +2187,7 @@ void intel_set_cdclk(struct drm_i915_private *dev_priv,
intel_dump_cdclk_state(cdclk_state, "Changing CDCLK to");
- dev_priv->display.set_cdclk(dev_priv, cdclk_state); + dev_priv->display.set_cdclk(dev_priv, cdclk_state, pipe);
if (WARN(intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_state), "cdclk state doesn't match!\n")) { @@ -2157,6 +2196,46 @@ void intel_set_cdclk(struct drm_i915_private *dev_priv, } }
+/** + * intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware + * @dev_priv: i915 device + * @old_state: old CDCLK state + * @new_state: new CDCLK state + * @pipe: pipe with which to synchronize the update + * + * Program the hardware before updating the HW plane state based on the passed + * in CDCLK state, if necessary. + */ +void +intel_set_cdclk_pre_plane_update(struct drm_i915_private *dev_priv, + const struct intel_cdclk_state *old_state, + const struct intel_cdclk_state *new_state, + enum pipe pipe) +{ + if (pipe == INVALID_PIPE || old_state->cdclk <= new_state->cdclk) + intel_set_cdclk(dev_priv, new_state, pipe); +} + +/** + * intel_set_cdclk_post_plane_update - Push the CDCLK state to the hardware + * @dev_priv: i915 device + * @old_state: old CDCLK state + * @new_state: new CDCLK state + * @pipe: pipe with which to synchronize the update + * + * Program the hardware after updating the HW plane state based on the passed + * in CDCLK state, if necessary. + */ +void +intel_set_cdclk_post_plane_update(struct drm_i915_private *dev_priv, + const struct intel_cdclk_state *old_state, + const struct intel_cdclk_state *new_state, + enum pipe pipe) +{ + if (pipe != INVALID_PIPE && old_state->cdclk > new_state->cdclk) + intel_set_cdclk(dev_priv, new_state, pipe); +} + static int intel_pixel_rate_to_cdclk(struct drm_i915_private *dev_priv, int pixel_rate) { diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index aa16804687c2..044c9235a86d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12778,6 +12778,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state) intel_state->active_crtcs = dev_priv->active_crtcs; intel_state->cdclk.logical = dev_priv->cdclk.logical; intel_state->cdclk.actual = dev_priv->cdclk.actual; + intel_state->cdclk.pipe = INVALID_PIPE;
for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { if (new_crtc_state->active) @@ -12797,6 +12798,8 @@ static int intel_modeset_checks(struct drm_atomic_state *state) * adjusted_mode bits in the crtc directly. */ if (dev_priv->display.modeset_calc_cdclk) { + enum pipe pipe; + ret = dev_priv->display.modeset_calc_cdclk(state); if (ret < 0) return ret; @@ -12813,12 +12816,36 @@ static int intel_modeset_checks(struct drm_atomic_state *state) return ret; }
+ if (is_power_of_2(intel_state->active_crtcs)) { + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; + + pipe = ilog2(intel_state->active_crtcs); + crtc = &intel_get_crtc_for_pipe(dev_priv, pipe)->base; + crtc_state = drm_atomic_get_new_crtc_state(state, crtc); + if (crtc_state && needs_modeset(crtc_state)) + pipe = INVALID_PIPE; + } else { + pipe = INVALID_PIPE; + } + /* All pipes must be switched off while we change the cdclk. */ - if (intel_cdclk_needs_modeset(&dev_priv->cdclk.actual, - &intel_state->cdclk.actual)) { + if (pipe != INVALID_PIPE && + intel_cdclk_needs_cd2x_update(dev_priv, + &dev_priv->cdclk.actual, + &intel_state->cdclk.actual)) { + ret = intel_lock_all_pipes(state); + if (ret < 0) + return ret; + + intel_state->cdclk.pipe = pipe; + } else if (intel_cdclk_needs_modeset(&dev_priv->cdclk.actual, + &intel_state->cdclk.actual)) { ret = intel_modeset_all_pipes(state); if (ret < 0) return ret; + + intel_state->cdclk.pipe = INVALID_PIPE; }
DRM_DEBUG_KMS("New cdclk calculated to be logical %u kHz, actual %u kHz\n", @@ -13227,7 +13254,10 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) if (intel_state->modeset) { drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
- intel_set_cdclk(dev_priv, &dev_priv->cdclk.actual); + intel_set_cdclk_pre_plane_update(dev_priv, + &intel_state->cdclk.actual, + &dev_priv->cdclk.actual, + intel_state->cdclk.pipe);
/* * SKL workaround: bspec recommends we disable the SAGV when we @@ -13256,6 +13286,12 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) /* Now enable the clocks, plane, pipe, and connectors that we set up. */ dev_priv->display.update_crtcs(state);
+ if (intel_state->modeset) + intel_set_cdclk_post_plane_update(dev_priv, + &intel_state->cdclk.actual, + &dev_priv->cdclk.actual, + intel_state->cdclk.pipe); + /* FIXME: We should call drm_atomic_helper_commit_hw_done() here * already, but still need the state for the delayed optimization. To * fix this: diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index a9183579700d..18b60016f0ed 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -483,6 +483,8 @@ struct intel_atomic_state {
int force_min_cdclk; bool force_min_cdclk_changed; + /* pipe to which cd2x update is synchronized */ + enum pipe pipe; } cdclk;
bool dpll_set, modeset; @@ -1593,13 +1595,24 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv); void intel_update_max_cdclk(struct drm_i915_private *dev_priv); void intel_update_cdclk(struct drm_i915_private *dev_priv); void intel_update_rawclk(struct drm_i915_private *dev_priv); +bool intel_cdclk_needs_cd2x_update(struct drm_i915_private *dev_priv, + const struct intel_cdclk_state *a, + const struct intel_cdclk_state *b); bool intel_cdclk_needs_modeset(const struct intel_cdclk_state *a, const struct intel_cdclk_state *b); bool intel_cdclk_changed(const struct intel_cdclk_state *a, const struct intel_cdclk_state *b); void intel_cdclk_swap_state(struct intel_atomic_state *state); -void intel_set_cdclk(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state); +void +intel_set_cdclk_pre_plane_update(struct drm_i915_private *dev_priv, + const struct intel_cdclk_state *old_state, + const struct intel_cdclk_state *new_state, + enum pipe pipe); +void +intel_set_cdclk_post_plane_update(struct drm_i915_private *dev_priv, + const struct intel_cdclk_state *old_state, + const struct intel_cdclk_state *new_state, + enum pipe pipe); void intel_dump_cdclk_state(const struct intel_cdclk_state *cdclk_state, const char *context);
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Jian-Hong Pan -
Ville Syrjälä