From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lyude Paul Subject: Re: [PATCH v2] drm/i915/ilk: Don't disable SSC source if it's in use Date: Tue, 24 May 2016 10:08:47 -0400 Message-ID: <1464098927.14789.1.camel@redhat.com> References: <20160523191635.GZ4329@intel.com> <1464033396-27649-1-git-send-email-cpaul@redhat.com> <20160524131414.GE4329@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <20160524131414.GE4329@intel.com> Sender: stable-owner@vger.kernel.org To: Ville =?ISO-8859-1?Q?Syrj=E4l=E4?= Cc: intel-gfx@lists.freedesktop.org, "open list:INTEL DRM DRIVERS (excluding Poulsbo, Moorestow...), linux-kernel@vger.kernel.org (open list)" , Daniel Vetter , stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org Huh=E2=80=A6 so I'm going to have to take back what I said before about= this. On further observation, it looks like the "Don't disable SSC source if it's in use= " patch actually got rid of the underruns entirely, with or without the wait fo= r a vblank here. On Tue, 2016-05-24 at 16:14 +0300, Ville Syrj=C3=A4l=C3=A4 wrote: > On Mon, May 23, 2016 at 03:56:36PM -0400, Lyude wrote: > >=20 > > Thanks to Ville Syrj=C3=A4l=C3=A4 for pointing me towards the cause= of this issue. > >=20 > > Unfortunately one of the sideaffects of having the refclk for a DPL= L 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 d= on'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: > >=20 > > [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=3Dio+mem,decodes=3Dio+mem:owns=3Dio+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 > > =E2=80=A6 later we try committing the first modeset =E2=80=A6 > > [drm:intel_dump_pipe_config] [CRTC:26][modeset] config ffff88041b02= e800 for > > pipe A > > [drm:intel_dump_pipe_config] cpu_transcoder: A > > =E2=80=A6 > > [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 en= abled > > [drm:intel_dump_pipe_config]=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0FB:42, fb= =3D 800x600 format =3D 0x34325258 > > [drm:intel_dump_pipe_config]=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0scaler:0 = src (0, 0) 800x600 dst (0, 0) > > 800x600 > > [drm:intel_dump_pipe_config] CURSOR PLANE:25 plane: 0.1 idx: 1 disa= bled, > > scaler_id =3D 0 > > [drm:intel_dump_pipe_config] STANDARD PLANE:27 plane: 0.1 idx: 2 di= sabled, > > scaler_id =3D 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:11= 46 > > intel_disable_pipe+0x297/0x2d0 [i915] > > pipe_off wait timed out > > =E2=80=A6 > > ---[ end trace 94fc8aa03ae139e8 ]--- > > [drm:intel_dp_link_down] > > [drm:ironlake_crtc_disable [i915]] *ERROR* failed to disable transc= oder A > >=20 > > Later modesets succeed since they reset the DPLL's configuration an= yway, > > but this is enough to get stuck with a big fat warning in dmesg. > >=20 > > A better solution would be to add refcounts for the SSC source, but= for > > now leaving the source clock on should suffice. > >=20 > > Changes since v1: > > =C2=A0- Leave the SSC source clock on instead of just shutting it o= ff on all > > =C2=A0=C2=A0=C2=A0of the DPLL configurations. > >=20 > > Cc: stable@vger.kernel.org > > Signed-off-by: Lyude > > --- > > Sorry about that! I misread danvet's suggestion as "disable the SSC= " instead > > of > > "don't disable the SSC". > >=20 > > =C2=A0drivers/gpu/drm/i915/intel_display.c | 47 +++++++++++++++++++= ++++++++---- > > ----- > > =C2=A01 file changed, 35 insertions(+), 12 deletions(-) > >=20 > > 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) > > =C2=A0{ > > =C2=A0 struct drm_i915_private *dev_priv =3D dev->dev_private; > > =C2=A0 struct intel_encoder *encoder; > > - u32 val, final; > > + int i; > > + u32 val, temp, final; > > =C2=A0 bool has_lvds =3D false; > > =C2=A0 bool has_cpu_edp =3D false; > > =C2=A0 bool has_panel =3D false; > > =C2=A0 bool has_ck505 =3D false; > > =C2=A0 bool can_ssc =3D false; > > + bool using_ssc_source =3D false; > > =C2=A0 > > =C2=A0 /* We need to take the global config into account */ > > =C2=A0 for_each_intel_encoder(dev, encoder) { > > @@ -8283,9 +8285,26 @@ static void ironlake_init_pch_refclk(struct > > drm_device *dev) > > =C2=A0 else > > =C2=A0 final |=3D DREF_NONSPREAD_SOURCE_ENABLE; > > =C2=A0 > > - final &=3D ~DREF_SSC_SOURCE_MASK; > > =C2=A0 final &=3D ~DREF_CPU_SOURCE_OUTPUT_MASK; > > - final &=3D ~DREF_SSC1_ENABLE; > > + > > + /* Check if any DPLLs are using the SSC source */ > > + for (i =3D 0; i < dev_priv->num_shared_dpll; i++) { > > + temp =3D I915_READ(PCH_DPLL(i)); > > + > > + if (!(temp & DPLL_VCO_ENABLE)) > > + continue; > > + > > + if ((temp & PLL_REF_INPUT_MASK) =3D=3D > > + =C2=A0=C2=A0=C2=A0=C2=A0PLLB_REF_INPUT_SPREADSPECTRUMIN) { > > + using_ssc_source =3D true; > > + break; > > + } > > + } > > + > > + if (!using_ssc_source) { > > + final &=3D ~DREF_SSC_SOURCE_MASK; > > + final &=3D ~DREF_SSC1_ENABLE; > > + } > > =C2=A0 > > =C2=A0 if (has_panel) { > > =C2=A0 final |=3D DREF_SSC_SOURCE_ENABLE; > > @@ -8348,7 +8367,7 @@ static void ironlake_init_pch_refclk(struct d= rm_device > > *dev) > > =C2=A0 POSTING_READ(PCH_DREF_CONTROL); > > =C2=A0 udelay(200); > > =C2=A0 } 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. >=20 > IIRC the clock tree looks something like this: >=20 > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0integrated > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0-> non-spread -> dpll non-spread > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0\ > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ck5= 05=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0-> cpu source -> edp pll > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/ > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0integrated -> ssc -> ssc1 -> dpll= ssc >=20 > integrated -> super ssc -> ssc4 -> dpll super ssc >=20 > Maybe we should slap that diagram somewhere as a comment? >=20 >=20 > Anyways, othewise the patch looks OK I think. >=20 > 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. >=20 > For eDP it can select the non-spread source but still leave the SSC > source enabled for some reason. >=20 > 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? >=20 > >=20 > > =C2=A0 val &=3D ~DREF_CPU_SOURCE_OUTPUT_MASK; > > =C2=A0 > > @@ -8359,16 +8378,20 @@ static void ironlake_init_pch_refclk(struct > > drm_device *dev) > > =C2=A0 POSTING_READ(PCH_DREF_CONTROL); > > =C2=A0 udelay(200); > > =C2=A0 > > - /* Turn off the SSC source */ > > - val &=3D ~DREF_SSC_SOURCE_MASK; > > - val |=3D DREF_SSC_SOURCE_DISABLE; > > + if (!using_ssc_source) { > > + DRM_DEBUG_KMS("Disabling SSC source\n"); > > =C2=A0 > > - /* Turn off SSC1 */ > > - val &=3D ~DREF_SSC1_ENABLE; > > + /* Turn off the SSC source */ > > + val &=3D ~DREF_SSC_SOURCE_MASK; > > + val |=3D DREF_SSC_SOURCE_DISABLE; > > =C2=A0 > > - I915_WRITE(PCH_DREF_CONTROL, val); > > - POSTING_READ(PCH_DREF_CONTROL); > > - udelay(200); > > + /* Turn off SSC1 */ > > + val &=3D ~DREF_SSC1_ENABLE; > > + > > + I915_WRITE(PCH_DREF_CONTROL, val); > > + POSTING_READ(PCH_DREF_CONTROL); > > + udelay(200); > > + } > > =C2=A0 } > > =C2=A0 > > =C2=A0 BUG_ON(val !=3D final); > > --=C2=A0 > > 2.5.5