[PATCH v2] drm/i915/ilk: Don't disable SSC source if it's in use
Ville Syrjälä
ville.syrjala at linux.intel.com
Tue May 24 13:14:14 UTC 2016
On Mon, May 23, 2016 at 03:56:36PM -0400, Lyude wrote:
> Thanks to Ville Syrjälä for pointing me towards the cause of this issue.
>
> Unfortunately one of the sideaffects of having the refclk for a DPLL set
> to SSC is that as long as it's set to SSC, the GPU will prevent us from
> powering down any of the pipes or transcoders using it. A couple of
> BIOSes enable SSC in both PCH_DREF_CONTROL and in the DPLL
> configurations. This causes issues on the first modeset, since we don't
> expect SSC to be left on and as a result, can't successfully power down
> the pipes or the transcoders using it. Here's an example from this Dell
> OptiPlex 990:
>
> [drm:intel_modeset_init] SSC enabled by BIOS, overriding VBT which says disabled
> [drm:intel_modeset_init] 2 display pipes available.
> [drm:intel_update_cdclk] Current CD clock rate: 400000 kHz
> [drm:intel_update_max_cdclk] Max CD clock rate: 400000 kHz
> [drm:intel_update_max_cdclk] Max dotclock rate: 360000 kHz
> vgaarb: device changed decodes: PCI:0000:00:02.0,olddecodes=io+mem,decodes=io+mem:owns=io+mem
> [drm:intel_crt_reset] crt adpa set to 0xf40000
> [drm:intel_dp_init_connector] Adding DP connector on port C
> [drm:intel_dp_aux_init] registering DPDDC-C bus for card0-DP-1
> [drm:ironlake_init_pch_refclk] has_panel 0 has_lvds 0 has_ck505 0
> [drm:ironlake_init_pch_refclk] Disabling SSC entirely
> … later we try committing the first modeset …
> [drm:intel_dump_pipe_config] [CRTC:26][modeset] config ffff88041b02e800 for pipe A
> [drm:intel_dump_pipe_config] cpu_transcoder: A
> …
> [drm:intel_dump_pipe_config] dpll_hw_state: dpll: 0xc4016001, dpll_md: 0x0, fp0: 0x20e08, fp1: 0x30d07
> [drm:intel_dump_pipe_config] planes on this crtc
> [drm:intel_dump_pipe_config] STANDARD PLANE:23 plane: 0.0 idx: 0 enabled
> [drm:intel_dump_pipe_config] FB:42, fb = 800x600 format = 0x34325258
> [drm:intel_dump_pipe_config] scaler:0 src (0, 0) 800x600 dst (0, 0) 800x600
> [drm:intel_dump_pipe_config] CURSOR PLANE:25 plane: 0.1 idx: 1 disabled, scaler_id = 0
> [drm:intel_dump_pipe_config] STANDARD PLANE:27 plane: 0.1 idx: 2 disabled, scaler_id = 0
> [drm:intel_get_shared_dpll] CRTC:26 allocated PCH DPLL A
> [drm:intel_get_shared_dpll] using PCH DPLL A for pipe A
> [drm:ilk_audio_codec_disable] Disable audio codec on port C, pipe A
> [drm:intel_disable_pipe] disabling pipe A
> ------------[ cut here ]------------
> WARNING: CPU: 1 PID: 130 at drivers/gpu/drm/i915/intel_display.c:1146 intel_disable_pipe+0x297/0x2d0 [i915]
> pipe_off wait timed out
> …
> ---[ end trace 94fc8aa03ae139e8 ]---
> [drm:intel_dp_link_down]
> [drm:ironlake_crtc_disable [i915]] *ERROR* failed to disable transcoder A
>
> Later modesets succeed since they reset the DPLL's configuration anyway,
> but this is enough to get stuck with a big fat warning in dmesg.
>
> A better solution would be to add refcounts for the SSC source, but for
> now leaving the source clock on should suffice.
>
> Changes since v1:
> - Leave the SSC source clock on instead of just shutting it off on all
> of the DPLL configurations.
>
> Cc: stable at vger.kernel.org
> Signed-off-by: Lyude <cpaul at redhat.com>
> ---
> Sorry about that! I misread danvet's suggestion as "disable the SSC" instead of
> "don't disable the SSC".
>
> drivers/gpu/drm/i915/intel_display.c | 47 +++++++++++++++++++++++++++---------
> 1 file changed, 35 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d500e6f..dff8511 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8230,12 +8230,14 @@ static void ironlake_init_pch_refclk(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_encoder *encoder;
> - u32 val, final;
> + int i;
> + u32 val, temp, final;
> bool has_lvds = false;
> bool has_cpu_edp = false;
> bool has_panel = false;
> bool has_ck505 = false;
> bool can_ssc = false;
> + bool using_ssc_source = false;
>
> /* We need to take the global config into account */
> for_each_intel_encoder(dev, encoder) {
> @@ -8283,9 +8285,26 @@ static void ironlake_init_pch_refclk(struct drm_device *dev)
> else
> final |= DREF_NONSPREAD_SOURCE_ENABLE;
>
> - final &= ~DREF_SSC_SOURCE_MASK;
> final &= ~DREF_CPU_SOURCE_OUTPUT_MASK;
> - final &= ~DREF_SSC1_ENABLE;
> +
> + /* Check if any DPLLs are using the SSC source */
> + for (i = 0; i < dev_priv->num_shared_dpll; i++) {
> + temp = I915_READ(PCH_DPLL(i));
> +
> + if (!(temp & DPLL_VCO_ENABLE))
> + continue;
> +
> + if ((temp & PLL_REF_INPUT_MASK) ==
> + PLLB_REF_INPUT_SPREADSPECTRUMIN) {
> + using_ssc_source = true;
> + break;
> + }
> + }
> +
> + if (!using_ssc_source) {
> + final &= ~DREF_SSC_SOURCE_MASK;
> + final &= ~DREF_SSC1_ENABLE;
> + }
>
> if (has_panel) {
> final |= DREF_SSC_SOURCE_ENABLE;
> @@ -8348,7 +8367,7 @@ static void ironlake_init_pch_refclk(struct drm_device *dev)
> POSTING_READ(PCH_DREF_CONTROL);
> udelay(200);
> } else {
> - DRM_DEBUG_KMS("Disabling SSC entirely\n");
> + DRM_DEBUG_KMS("Disabling SSC CPU output\n");
The CPU source isn't necessarily SSC. It can come from SSC1 or from
the non-spread source.
IIRC the clock tree looks something like this:
integrated
\
-> non-spread -> dpll non-spread
/ \
ck505 -> cpu source -> edp pll
/
integrated -> ssc -> ssc1 -> dpll ssc
integrated -> super ssc -> ssc4 -> dpll super ssc
Maybe we should slap that diagram somewhere as a comment?
Anyways, othewise the patch looks OK I think.
As far as the original code goes, I don't actually understand it. For
LVDS it can enable the SSC source but leave SSC1 modulator disabled.
Not sure what's the point in using the SSC source w/ SSC1 disabled.
I'm not sure that's even legal TBH.
For eDP it can select the non-spread source but still leave the SSC
source enabled for some reason.
Also I don't understand why the has_ssc depends on ck505 on IBX. We
only use ck505 as the non-spread source, so not really sure why
has_ssc depends on the presence of ck505. Maybe you can only get
ssc+non-spread sources enabled at the same time if one of them
comes from ck505?
> val &= ~DREF_CPU_SOURCE_OUTPUT_MASK;
>
> @@ -8359,16 +8378,20 @@ static void ironlake_init_pch_refclk(struct drm_device *dev)
> POSTING_READ(PCH_DREF_CONTROL);
> udelay(200);
>
> - /* Turn off the SSC source */
> - val &= ~DREF_SSC_SOURCE_MASK;
> - val |= DREF_SSC_SOURCE_DISABLE;
> + if (!using_ssc_source) {
> + DRM_DEBUG_KMS("Disabling SSC source\n");
>
> - /* Turn off SSC1 */
> - val &= ~DREF_SSC1_ENABLE;
> + /* Turn off the SSC source */
> + val &= ~DREF_SSC_SOURCE_MASK;
> + val |= DREF_SSC_SOURCE_DISABLE;
>
> - I915_WRITE(PCH_DREF_CONTROL, val);
> - POSTING_READ(PCH_DREF_CONTROL);
> - udelay(200);
> + /* Turn off SSC1 */
> + val &= ~DREF_SSC1_ENABLE;
> +
> + I915_WRITE(PCH_DREF_CONTROL, val);
> + POSTING_READ(PCH_DREF_CONTROL);
> + udelay(200);
> + }
> }
>
> BUG_ON(val != final);
> --
> 2.5.5
--
Ville Syrjälä
Intel OTC
More information about the dri-devel
mailing list