| Message ID | 1451577127-3197-1-git-send-email-gabriel.feceoru@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hey, Op 31-12-15 om 16:52 schreef Gabriel Feceoru: > This gets rid of errors like: > > [ 906.286213] ------------[ cut here ]------------ > [ 906.286233] WARNING: CPU: 0 PID: 12252 at drivers/gpu/drm/i915/intel_drv.h:1457 gen6_read32+0x1ca/0x1e0 [i915]() > [ 906.286234] RPM wakelock ref not held during HW access > [ 906.286235] Modules linked in: > [ 906.286236] snd_hda_intel i915 drm_kms_helper drm msr snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic snd_hda_codec snd_hwdep x86_pkg_temp_thermal snd_hda_core i2c_algo_bit ehci_pci syscopyarea sysfillrect sysimgblt fb_sys_fops ehci_hcd r8169 xhci_pci mii xhci_hcd video [last unloaded: drm] > [ 906.286246] CPU: 0 PID: 12252 Comm: kms_pipe_crc_ba Tainted: G U W 4.4.0-rc6+ #45 > [ 906.286247] Hardware name: Dell Inc. Inspiron 3847/088DT1 , BIOS A06 01/15/2015 > [ 906.286248] ffffffffc022c098 ffff880210dbbae0 ffffffff813e0e4f ffff880210dbbb28 > [ 906.286250] ffff880210dbbb18 ffffffff8105f5f2 ffff8801fff40000 0000000000046040 > [ 906.286251] ffff8801fff49170 ffff8801fff49170 ffff8801fff40000 ffff880210dbbb78 > [ 906.286253] Call Trace: > [ 906.286256] [<ffffffff813e0e4f>] dump_stack+0x44/0x55 > [ 906.286259] [<ffffffff8105f5f2>] warn_slowpath_common+0x82/0xc0 > [ 906.286261] [<ffffffff8105f67c>] warn_slowpath_fmt+0x4c/0x50 > [ 906.286270] [<ffffffffc007e29c>] ? drm_property_free_blob+0x8c/0xb0 [drm] > [ 906.286280] [<ffffffffc01a32ea>] gen6_read32+0x1ca/0x1e0 [i915] > [ 906.286283] [<ffffffff8172cd12>] ? mutex_lock+0x12/0x30 > [ 906.286294] [<ffffffffc01e0ef0>] hsw_ddi_wrpll_get_hw_state+0x40/0x50 [i915] > [ 906.286304] [<ffffffffc01c3fa1>] intel_atomic_commit+0xd41/0x1740 [i915] > [ 906.286312] [<ffffffffc008c597>] drm_atomic_commit+0x37/0x60 [drm] > [ 906.286316] [<ffffffffc01373e6>] drm_atomic_helper_set_config+0x76/0xb0 [drm_kms_helper] > [ 906.286323] [<ffffffffc008ac0a>] ? drm_modeset_lock_all_ctx+0x9a/0xb0 [drm] > [ 906.286329] [<ffffffffc007b852>] drm_mode_set_config_internal+0x62/0x100 [drm] > [ 906.286335] [<ffffffffc007fcfd>] drm_mode_setcrtc+0x3cd/0x4e0 [drm] > [ 906.286339] [<ffffffffc0071762>] drm_ioctl+0x152/0x540 [drm] > [ 906.286341] [<ffffffff8109d914>] ? __wake_up+0x44/0x50 > [ 906.286346] [<ffffffffc007f930>] ? drm_mode_setplane+0x1b0/0x1b0 [drm] > [ 906.286348] [<ffffffff811dc844>] ? mntput+0x24/0x40 > [ 906.286350] [<ffffffff811bfe22>] ? __fput+0x172/0x1e0 > [ 906.286352] [<ffffffff811d0758>] do_vfs_ioctl+0x288/0x460 > [ 906.286353] [<ffffffff811bfece>] ? ____fput+0xe/0x10 > > v2: > Fixed return value in skl_ddi_pll_get_hw_state > > Signed-off-by: Gabriel Feceoru <gabriel.feceoru@intel.com> > Why do this in the get_hw_state functions? Seems more like it could be held by intel_modeset_check_state. ~Maarten
On Thu, Dec 31, 2015 at 05:52:07PM +0200, Gabriel Feceoru wrote: > This gets rid of errors like: > > [ 906.286213] ------------[ cut here ]------------ > [ 906.286233] WARNING: CPU: 0 PID: 12252 at drivers/gpu/drm/i915/intel_drv.h:1457 gen6_read32+0x1ca/0x1e0 [i915]() > [ 906.286234] RPM wakelock ref not held during HW access > [ 906.286235] Modules linked in: > [ 906.286236] snd_hda_intel i915 drm_kms_helper drm msr snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic snd_hda_codec snd_hwdep x86_pkg_temp_thermal snd_hda_core i2c_algo_bit ehci_pci syscopyarea sysfillrect sysimgblt fb_sys_fops ehci_hcd r8169 xhci_pci mii xhci_hcd video [last unloaded: drm] > [ 906.286246] CPU: 0 PID: 12252 Comm: kms_pipe_crc_ba Tainted: G U W 4.4.0-rc6+ #45 > [ 906.286247] Hardware name: Dell Inc. Inspiron 3847/088DT1 , BIOS A06 01/15/2015 > [ 906.286248] ffffffffc022c098 ffff880210dbbae0 ffffffff813e0e4f ffff880210dbbb28 > [ 906.286250] ffff880210dbbb18 ffffffff8105f5f2 ffff8801fff40000 0000000000046040 > [ 906.286251] ffff8801fff49170 ffff8801fff49170 ffff8801fff40000 ffff880210dbbb78 > [ 906.286253] Call Trace: > [ 906.286256] [<ffffffff813e0e4f>] dump_stack+0x44/0x55 > [ 906.286259] [<ffffffff8105f5f2>] warn_slowpath_common+0x82/0xc0 > [ 906.286261] [<ffffffff8105f67c>] warn_slowpath_fmt+0x4c/0x50 > [ 906.286270] [<ffffffffc007e29c>] ? drm_property_free_blob+0x8c/0xb0 [drm] > [ 906.286280] [<ffffffffc01a32ea>] gen6_read32+0x1ca/0x1e0 [i915] > [ 906.286283] [<ffffffff8172cd12>] ? mutex_lock+0x12/0x30 > [ 906.286294] [<ffffffffc01e0ef0>] hsw_ddi_wrpll_get_hw_state+0x40/0x50 [i915] > [ 906.286304] [<ffffffffc01c3fa1>] intel_atomic_commit+0xd41/0x1740 [i915] > [ 906.286312] [<ffffffffc008c597>] drm_atomic_commit+0x37/0x60 [drm] > [ 906.286316] [<ffffffffc01373e6>] drm_atomic_helper_set_config+0x76/0xb0 [drm_kms_helper] > [ 906.286323] [<ffffffffc008ac0a>] ? drm_modeset_lock_all_ctx+0x9a/0xb0 [drm] > [ 906.286329] [<ffffffffc007b852>] drm_mode_set_config_internal+0x62/0x100 [drm] > [ 906.286335] [<ffffffffc007fcfd>] drm_mode_setcrtc+0x3cd/0x4e0 [drm] > [ 906.286339] [<ffffffffc0071762>] drm_ioctl+0x152/0x540 [drm] > [ 906.286341] [<ffffffff8109d914>] ? __wake_up+0x44/0x50 > [ 906.286346] [<ffffffffc007f930>] ? drm_mode_setplane+0x1b0/0x1b0 [drm] > [ 906.286348] [<ffffffff811dc844>] ? mntput+0x24/0x40 > [ 906.286350] [<ffffffff811bfe22>] ? __fput+0x172/0x1e0 > [ 906.286352] [<ffffffff811d0758>] do_vfs_ioctl+0x288/0x460 > [ 906.286353] [<ffffffff811bfece>] ? ____fput+0xe/0x10 > > v2: > Fixed return value in skl_ddi_pll_get_hw_state > > Signed-off-by: Gabriel Feceoru <gabriel.feceoru@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 50 +++++++++++++++++++++++++++++----------- > 1 file changed, 37 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index e6408e5..e92ed7a 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2014,15 +2014,18 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder, > enum intel_display_power_domain power_domain; > u32 tmp; > int i; > + bool ret = false; > > power_domain = intel_display_port_power_domain(encoder); > if (!intel_display_power_is_enabled(dev_priv, power_domain)) > - return false; > + return ret; I think what we really need here is some kind of intel_display_power_get_unless_zero() thing. We need to make sure not only that the rpm reference is held when reading out the state, but also that the relevant power well(s) remain enabled while we're reading out the state. > + > + intel_runtime_pm_get(dev_priv); > > tmp = I915_READ(DDI_BUF_CTL(port)); > > if (!(tmp & DDI_BUF_CTL_ENABLE)) > - return false; > + goto out; > > if (port == PORT_A) { > tmp = I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_EDP)); > @@ -2040,7 +2043,8 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder, > break; > } > > - return true; > + ret = true; > + goto out; > } else { > for (i = TRANSCODER_A; i <= TRANSCODER_C; i++) { > tmp = I915_READ(TRANS_DDI_FUNC_CTL(i)); > @@ -2048,17 +2052,19 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder, > if ((tmp & TRANS_DDI_PORT_MASK) > == TRANS_DDI_SELECT_PORT(port)) { > if ((tmp & TRANS_DDI_MODE_SELECT_MASK) == TRANS_DDI_MODE_SELECT_DP_MST) > - return false; > + goto out; > > *pipe = i; > - return true; > + ret = true; > + goto out; > } > } > } > > DRM_DEBUG_KMS("No pipe for ddi port %c found\n", port_name(port)); > - > - return false; > +out: > + intel_runtime_pm_put(dev_priv); > + return ret; > } > > void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc) > @@ -2510,7 +2516,10 @@ static bool hsw_ddi_wrpll_get_hw_state(struct drm_i915_private *dev_priv, > if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS)) > return false; > > + intel_runtime_pm_get(dev_priv); > val = I915_READ(WRPLL_CTL(pll->id)); > + intel_runtime_pm_put(dev_priv); > + > hw_state->wrpll = val; > > return val & WRPLL_PLL_ENABLE; > @@ -2525,7 +2534,10 @@ static bool hsw_ddi_spll_get_hw_state(struct drm_i915_private *dev_priv, > if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS)) > return false; > > + intel_runtime_pm_get(dev_priv); > val = I915_READ(SPLL_CTL); > + intel_runtime_pm_put(dev_priv); > + > hw_state->spll = val; > > return val & SPLL_PLL_ENABLE; > @@ -2644,16 +2656,19 @@ static bool skl_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv, > uint32_t val; > unsigned int dpll; > const struct skl_dpll_regs *regs = skl_dpll_regs; > + bool ret = false; > > if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS)) > - return false; > + return ret; > > /* DPLL0 is not part of the shared DPLLs, so pll->id is 0 for DPLL1 */ > dpll = pll->id + 1; > > + intel_runtime_pm_get(dev_priv); > + > val = I915_READ(regs[pll->id].ctl); > if (!(val & LCPLL_PLL_ENABLE)) > - return false; > + goto out; > > val = I915_READ(DPLL_CTRL1); > hw_state->ctrl1 = (val >> (dpll * 6)) & 0x3f; > @@ -2664,7 +2679,10 @@ static bool skl_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv, > hw_state->cfgcr2 = I915_READ(regs[pll->id].cfgcr2); > } > > - return true; > + ret = true; > +out: > + intel_runtime_pm_put(dev_priv); > + return ret; > } > > static void skl_shared_dplls_init(struct drm_i915_private *dev_priv) > @@ -2931,13 +2949,16 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv, > { > enum port port = (enum port)pll->id; /* 1:1 port->PLL mapping */ > uint32_t val; > + bool ret = false; > > if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS)) > - return false; > + return ret; > + > + intel_runtime_pm_get(dev_priv); > > val = I915_READ(BXT_PORT_PLL_ENABLE(port)); > if (!(val & PORT_PLL_ENABLE)) > - return false; > + goto out; > > hw_state->ebb0 = I915_READ(BXT_PORT_PLL_EBB_0(port)); > hw_state->ebb0 &= PORT_PLL_P1_MASK | PORT_PLL_P2_MASK; > @@ -2984,7 +3005,10 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv, > I915_READ(BXT_PORT_PCS_DW12_LN23(port))); > hw_state->pcsdw12 &= LANE_STAGGER_MASK | LANESTAGGER_STRAP_OVRD; > > - return true; > + ret = true; > +out: > + intel_runtime_pm_put(dev_priv); > + return ret; > } > > static void bxt_shared_dplls_init(struct drm_i915_private *dev_priv) > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Jan 4, 2016 at 4:05 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > I think what we really need here is some kind of > intel_display_power_get_unless_zero() thing. We need to make sure not > only that the rpm reference is held when reading out the state, but also > that the relevant power well(s) remain enabled while we're reading out > the state. Yeah, we need to check whether a given piece of hw is enabled or not. All the get_hw_state funcs do that, using intel_display_power_is_enabled. But it looks like intel_ddi_get_hw_state partially gets this wrong. That would also address the same backtrace in module unload. -Daniel
On Tue, Jan 05, 2016 at 02:54:31PM +0100, Daniel Vetter wrote: > On Mon, Jan 4, 2016 at 4:05 PM, Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > I think what we really need here is some kind of > > intel_display_power_get_unless_zero() thing. We need to make sure not > > only that the rpm reference is held when reading out the state, but also > > that the relevant power well(s) remain enabled while we're reading out > > the state. > > Yeah, we need to check whether a given piece of hw is enabled or not. > All the get_hw_state funcs do that, using > intel_display_power_is_enabled. But it looks like > intel_ddi_get_hw_state partially gets this wrong. That would also > address the same backtrace in module unload. Tried __only__ this patch but it doesn't fix the trace in driver unload, had the same hunch.... > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On ma, 2016-01-04 at 17:05 +0200, Ville Syrjälä wrote: > On Thu, Dec 31, 2015 at 05:52:07PM +0200, Gabriel Feceoru wrote <SNIP> > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -2014,15 +2014,18 @@ bool intel_ddi_get_hw_state(struct > > intel_encoder *encoder, > > enum intel_display_power_domain power_domain; > > u32 tmp; > > int i; > > + bool ret = false; > > > > power_domain = intel_display_port_power_domain(encoder); > > if (!intel_display_power_is_enabled(dev_priv, > > power_domain)) > > - return false; > > + return ret; > > I think what we really need here is some kind of > intel_display_power_get_unless_zero() thing. We need to make sure not > only that the rpm reference is held when reading out the state, but > also > that the relevant power well(s) remain enabled while we're reading > out > the state. > Once this gets merged to our trees, should be rather easy to add: PM / runtime: Add new helper for conditional usage count incrementation https://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit /?h=bleeding-edge&id=a436b6a19f57656a6557439523923d89eb4a880d Regards, Joonas > > > + > > + intel_runtime_pm_get(dev_priv); > > > > tmp = I915_READ(DDI_BUF_CTL(port)); > > > > if (!(tmp & DDI_BUF_CTL_ENABLE)) > > - return false; > > + goto out; > > > > if (port == PORT_A) { > > tmp = > > I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_EDP)); > > @@ -2040,7 +2043,8 @@ bool intel_ddi_get_hw_state(struct > > intel_encoder *encoder, > > break; > > } > > > > - return true; > > + ret = true; > > + goto out; > > } else { > > for (i = TRANSCODER_A; i <= TRANSCODER_C; i++) { > > tmp = I915_READ(TRANS_DDI_FUNC_CTL(i)); > > @@ -2048,17 +2052,19 @@ bool intel_ddi_get_hw_state(struct > > intel_encoder *encoder, > > if ((tmp & TRANS_DDI_PORT_MASK) > > == TRANS_DDI_SELECT_PORT(port)) { > > if ((tmp & > > TRANS_DDI_MODE_SELECT_MASK) == TRANS_DDI_MODE_SELECT_DP_MST) > > - return false; > > + goto out; > > > > *pipe = i; > > - return true; > > + ret = true; > > + goto out; > > } > > } > > } > > > > DRM_DEBUG_KMS("No pipe for ddi port %c found\n", > > port_name(port)); > > - > > - return false; > > +out: > > + intel_runtime_pm_put(dev_priv); > > + return ret; > > } > > > > void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc) > > @@ -2510,7 +2516,10 @@ static bool > > hsw_ddi_wrpll_get_hw_state(struct drm_i915_private *dev_priv, > > if (!intel_display_power_is_enabled(dev_priv, > > POWER_DOMAIN_PLLS)) > > return false; > > > > + intel_runtime_pm_get(dev_priv); > > val = I915_READ(WRPLL_CTL(pll->id)); > > + intel_runtime_pm_put(dev_priv); > > + > > hw_state->wrpll = val; > > > > return val & WRPLL_PLL_ENABLE; > > @@ -2525,7 +2534,10 @@ static bool hsw_ddi_spll_get_hw_state(struct > > drm_i915_private *dev_priv, > > if (!intel_display_power_is_enabled(dev_priv, > > POWER_DOMAIN_PLLS)) > > return false; > > > > + intel_runtime_pm_get(dev_priv); > > val = I915_READ(SPLL_CTL); > > + intel_runtime_pm_put(dev_priv); > > + > > hw_state->spll = val; > > > > return val & SPLL_PLL_ENABLE; > > @@ -2644,16 +2656,19 @@ static bool skl_ddi_pll_get_hw_state(struct > > drm_i915_private *dev_priv, > > uint32_t val; > > unsigned int dpll; > > const struct skl_dpll_regs *regs = skl_dpll_regs; > > + bool ret = false; > > > > if (!intel_display_power_is_enabled(dev_priv, > > POWER_DOMAIN_PLLS)) > > - return false; > > + return ret; > > > > /* DPLL0 is not part of the shared DPLLs, so pll->id is 0 > > for DPLL1 */ > > dpll = pll->id + 1; > > > > + intel_runtime_pm_get(dev_priv); > > + > > val = I915_READ(regs[pll->id].ctl); > > if (!(val & LCPLL_PLL_ENABLE)) > > - return false; > > + goto out; > > > > val = I915_READ(DPLL_CTRL1); > > hw_state->ctrl1 = (val >> (dpll * 6)) & 0x3f; > > @@ -2664,7 +2679,10 @@ static bool skl_ddi_pll_get_hw_state(struct > > drm_i915_private *dev_priv, > > hw_state->cfgcr2 = I915_READ(regs[pll > > ->id].cfgcr2); > > } > > > > - return true; > > + ret = true; > > +out: > > + intel_runtime_pm_put(dev_priv); > > + return ret; > > } > > > > static void skl_shared_dplls_init(struct drm_i915_private > > *dev_priv) > > @@ -2931,13 +2949,16 @@ static bool bxt_ddi_pll_get_hw_state(struct > > drm_i915_private *dev_priv, > > { > > enum port port = (enum port)pll->id; /* 1:1 port > > ->PLL mapping */ > > uint32_t val; > > + bool ret = false; > > > > if (!intel_display_power_is_enabled(dev_priv, > > POWER_DOMAIN_PLLS)) > > - return false; > > + return ret; > > + > > + intel_runtime_pm_get(dev_priv); > > > > val = I915_READ(BXT_PORT_PLL_ENABLE(port)); > > if (!(val & PORT_PLL_ENABLE)) > > - return false; > > + goto out; > > > > hw_state->ebb0 = I915_READ(BXT_PORT_PLL_EBB_0(port)); > > hw_state->ebb0 &= PORT_PLL_P1_MASK | PORT_PLL_P2_MASK; > > @@ -2984,7 +3005,10 @@ static bool bxt_ddi_pll_get_hw_state(struct > > drm_i915_private *dev_priv, > > I915_READ(BXT_PORT_PCS_DW12_LN23( > > port))); > > hw_state->pcsdw12 &= LANE_STAGGER_MASK | > > LANESTAGGER_STRAP_OVRD; > > > > - return true; > > + ret = true; > > +out: > > + intel_runtime_pm_put(dev_priv); > > + return ret; > > } > > > > static void bxt_shared_dplls_init(struct drm_i915_private > > *dev_priv) > > -- > > 1.9.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index e6408e5..e92ed7a 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2014,15 +2014,18 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder, enum intel_display_power_domain power_domain; u32 tmp; int i; + bool ret = false; power_domain = intel_display_port_power_domain(encoder); if (!intel_display_power_is_enabled(dev_priv, power_domain)) - return false; + return ret; + + intel_runtime_pm_get(dev_priv); tmp = I915_READ(DDI_BUF_CTL(port)); if (!(tmp & DDI_BUF_CTL_ENABLE)) - return false; + goto out; if (port == PORT_A) { tmp = I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_EDP)); @@ -2040,7 +2043,8 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder, break; } - return true; + ret = true; + goto out; } else { for (i = TRANSCODER_A; i <= TRANSCODER_C; i++) { tmp = I915_READ(TRANS_DDI_FUNC_CTL(i)); @@ -2048,17 +2052,19 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder, if ((tmp & TRANS_DDI_PORT_MASK) == TRANS_DDI_SELECT_PORT(port)) { if ((tmp & TRANS_DDI_MODE_SELECT_MASK) == TRANS_DDI_MODE_SELECT_DP_MST) - return false; + goto out; *pipe = i; - return true; + ret = true; + goto out; } } } DRM_DEBUG_KMS("No pipe for ddi port %c found\n", port_name(port)); - - return false; +out: + intel_runtime_pm_put(dev_priv); + return ret; } void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc) @@ -2510,7 +2516,10 @@ static bool hsw_ddi_wrpll_get_hw_state(struct drm_i915_private *dev_priv, if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS)) return false; + intel_runtime_pm_get(dev_priv); val = I915_READ(WRPLL_CTL(pll->id)); + intel_runtime_pm_put(dev_priv); + hw_state->wrpll = val; return val & WRPLL_PLL_ENABLE; @@ -2525,7 +2534,10 @@ static bool hsw_ddi_spll_get_hw_state(struct drm_i915_private *dev_priv, if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS)) return false; + intel_runtime_pm_get(dev_priv); val = I915_READ(SPLL_CTL); + intel_runtime_pm_put(dev_priv); + hw_state->spll = val; return val & SPLL_PLL_ENABLE; @@ -2644,16 +2656,19 @@ static bool skl_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv, uint32_t val; unsigned int dpll; const struct skl_dpll_regs *regs = skl_dpll_regs; + bool ret = false; if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS)) - return false; + return ret; /* DPLL0 is not part of the shared DPLLs, so pll->id is 0 for DPLL1 */ dpll = pll->id + 1; + intel_runtime_pm_get(dev_priv); + val = I915_READ(regs[pll->id].ctl); if (!(val & LCPLL_PLL_ENABLE)) - return false; + goto out; val = I915_READ(DPLL_CTRL1); hw_state->ctrl1 = (val >> (dpll * 6)) & 0x3f; @@ -2664,7 +2679,10 @@ static bool skl_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv, hw_state->cfgcr2 = I915_READ(regs[pll->id].cfgcr2); } - return true; + ret = true; +out: + intel_runtime_pm_put(dev_priv); + return ret; } static void skl_shared_dplls_init(struct drm_i915_private *dev_priv) @@ -2931,13 +2949,16 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv, { enum port port = (enum port)pll->id; /* 1:1 port->PLL mapping */ uint32_t val; + bool ret = false; if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS)) - return false; + return ret; + + intel_runtime_pm_get(dev_priv); val = I915_READ(BXT_PORT_PLL_ENABLE(port)); if (!(val & PORT_PLL_ENABLE)) - return false; + goto out; hw_state->ebb0 = I915_READ(BXT_PORT_PLL_EBB_0(port)); hw_state->ebb0 &= PORT_PLL_P1_MASK | PORT_PLL_P2_MASK; @@ -2984,7 +3005,10 @@ static bool bxt_ddi_pll_get_hw_state(struct drm_i915_private *dev_priv, I915_READ(BXT_PORT_PCS_DW12_LN23(port))); hw_state->pcsdw12 &= LANE_STAGGER_MASK | LANESTAGGER_STRAP_OVRD; - return true; + ret = true; +out: + intel_runtime_pm_put(dev_priv); + return ret; } static void bxt_shared_dplls_init(struct drm_i915_private *dev_priv)
This gets rid of errors like: [ 906.286213] ------------[ cut here ]------------ [ 906.286233] WARNING: CPU: 0 PID: 12252 at drivers/gpu/drm/i915/intel_drv.h:1457 gen6_read32+0x1ca/0x1e0 [i915]() [ 906.286234] RPM wakelock ref not held during HW access [ 906.286235] Modules linked in: [ 906.286236] snd_hda_intel i915 drm_kms_helper drm msr snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic snd_hda_codec snd_hwdep x86_pkg_temp_thermal snd_hda_core i2c_algo_bit ehci_pci syscopyarea sysfillrect sysimgblt fb_sys_fops ehci_hcd r8169 xhci_pci mii xhci_hcd video [last unloaded: drm] [ 906.286246] CPU: 0 PID: 12252 Comm: kms_pipe_crc_ba Tainted: G U W 4.4.0-rc6+ #45 [ 906.286247] Hardware name: Dell Inc. Inspiron 3847/088DT1 , BIOS A06 01/15/2015 [ 906.286248] ffffffffc022c098 ffff880210dbbae0 ffffffff813e0e4f ffff880210dbbb28 [ 906.286250] ffff880210dbbb18 ffffffff8105f5f2 ffff8801fff40000 0000000000046040 [ 906.286251] ffff8801fff49170 ffff8801fff49170 ffff8801fff40000 ffff880210dbbb78 [ 906.286253] Call Trace: [ 906.286256] [<ffffffff813e0e4f>] dump_stack+0x44/0x55 [ 906.286259] [<ffffffff8105f5f2>] warn_slowpath_common+0x82/0xc0 [ 906.286261] [<ffffffff8105f67c>] warn_slowpath_fmt+0x4c/0x50 [ 906.286270] [<ffffffffc007e29c>] ? drm_property_free_blob+0x8c/0xb0 [drm] [ 906.286280] [<ffffffffc01a32ea>] gen6_read32+0x1ca/0x1e0 [i915] [ 906.286283] [<ffffffff8172cd12>] ? mutex_lock+0x12/0x30 [ 906.286294] [<ffffffffc01e0ef0>] hsw_ddi_wrpll_get_hw_state+0x40/0x50 [i915] [ 906.286304] [<ffffffffc01c3fa1>] intel_atomic_commit+0xd41/0x1740 [i915] [ 906.286312] [<ffffffffc008c597>] drm_atomic_commit+0x37/0x60 [drm] [ 906.286316] [<ffffffffc01373e6>] drm_atomic_helper_set_config+0x76/0xb0 [drm_kms_helper] [ 906.286323] [<ffffffffc008ac0a>] ? drm_modeset_lock_all_ctx+0x9a/0xb0 [drm] [ 906.286329] [<ffffffffc007b852>] drm_mode_set_config_internal+0x62/0x100 [drm] [ 906.286335] [<ffffffffc007fcfd>] drm_mode_setcrtc+0x3cd/0x4e0 [drm] [ 906.286339] [<ffffffffc0071762>] drm_ioctl+0x152/0x540 [drm] [ 906.286341] [<ffffffff8109d914>] ? __wake_up+0x44/0x50 [ 906.286346] [<ffffffffc007f930>] ? drm_mode_setplane+0x1b0/0x1b0 [drm] [ 906.286348] [<ffffffff811dc844>] ? mntput+0x24/0x40 [ 906.286350] [<ffffffff811bfe22>] ? __fput+0x172/0x1e0 [ 906.286352] [<ffffffff811d0758>] do_vfs_ioctl+0x288/0x460 [ 906.286353] [<ffffffff811bfece>] ? ____fput+0xe/0x10 v2: Fixed return value in skl_ddi_pll_get_hw_state Signed-off-by: Gabriel Feceoru <gabriel.feceoru@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 50 +++++++++++++++++++++++++++++----------- 1 file changed, 37 insertions(+), 13 deletions(-)