From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 08/31] drm/i915: hw state readout for shared pch plls Date: Fri, 7 Jun 2013 20:23:33 +0300 Message-ID: <20130607172333.GU5004@intel.com> References: <1370432073-27634-1-git-send-email-daniel.vetter@ffwll.ch> <1370432073-27634-9-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 62167E5C7A for ; Fri, 7 Jun 2013 10:23:37 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1370432073-27634-9-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Daniel Vetter Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Wed, Jun 05, 2013 at 01:34:10PM +0200, Daniel Vetter wrote: > Well, the first step of a long road at least, it only reads out > the pipe -> shared dpll association thus far. Other state which needs > to follow: > = > - hw state of the dpll (on/off + dpll registers). Currently we just > read that out from the hw state, but that doesn't work too well when > the dpll is in use, but not yet fully enabled. We get away since > most likely it already has been enabled and so the correct state is > left behind in the registers. But that doesn't hold for atomic > modesets when we want to enable all pipes at once. > = > - Refcount reconstruction for each dpll. > = > - Cross-checking of all the above. For that we need to keep the dpll > register state both in the pipe and in the shared_dpll struct, so > that we can check that every pipe is still connected to a correctly > configured dpll. > = > Note that since the refcount resconstruction isn't done yet this will > spill a few WARNs at boot-up while trying to disable pch plls which > have bogus refcounts. But since there's still a pile of refactoring to > do I'd like to lock down the state handling as soon as possible hence > decided against reordering the patches to quiet these WARNs - after > all the issues they're complaining about have existed since forever, > as Jesse can testify by having pch pll states blow up consistently in > his fastboot patches ... > = > v2: We need to preserve the old shared_dpll since currently the > shared dpll refcount dropping/getting is done in ->mode_set. With > the usual pipe_config infrastructure the old dpll id is already lost > at that point, hence preserve it in the new config. Ah so here is the missing link. The correct place for this would appear to be patch 6. > v3: Rebase on top of the ips patch from Paulo. > = > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/i915/intel_display.c | 34 ++++++++++++++++++++++++++++--= ---- > 1 file changed, 28 insertions(+), 6 deletions(-) > = > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/= intel_display.c > index a44c43c..20a933f 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4020,12 +4020,11 @@ static void hsw_compute_ips_config(struct intel_c= rtc *crtc, > pipe_config->pipe_bpp =3D=3D 24; > } > = > -static int intel_crtc_compute_config(struct drm_crtc *crtc, > +static int intel_crtc_compute_config(struct intel_crtc *crtc, > struct intel_crtc_config *pipe_config) > { > - struct drm_device *dev =3D crtc->dev; > + struct drm_device *dev =3D crtc->base.dev; > struct drm_display_mode *adjusted_mode =3D &pipe_config->adjusted_mode; > - struct intel_crtc *intel_crtc =3D to_intel_crtc(crtc); > = > if (HAS_PCH_SPLIT(dev)) { > /* FDI link clock is fixed at 2.7G */ > @@ -4056,10 +4055,16 @@ static int intel_crtc_compute_config(struct drm_c= rtc *crtc, > } > = > if (IS_HASWELL(dev)) > - hsw_compute_ips_config(intel_crtc, pipe_config); > + hsw_compute_ips_config(crtc, pipe_config); > + > + /* XXX: PCH clock sharing is done in ->mode_set, so make sure the old > + * clock survives for now. */ > + if (pipe_config->has_pch_encoder && > + (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))) > + pipe_config->shared_dpll =3D crtc->config.shared_dpll; > = > if (pipe_config->has_pch_encoder) > - return ironlake_fdi_compute_config(intel_crtc, pipe_config); > + return ironlake_fdi_compute_config(crtc, pipe_config); > = > return 0; > } > @@ -4934,6 +4939,7 @@ static bool i9xx_get_pipe_config(struct intel_crtc = *crtc, > uint32_t tmp; > = > pipe_config->cpu_transcoder =3D crtc->pipe; > + pipe_config->shared_dpll =3D DPLL_ID_PRIVATE; > = > tmp =3D I915_READ(PIPECONF(crtc->pipe)); > if (!(tmp & PIPECONF_ENABLE)) > @@ -5810,6 +5816,7 @@ static bool ironlake_get_pipe_config(struct intel_c= rtc *crtc, > uint32_t tmp; > = > pipe_config->cpu_transcoder =3D crtc->pipe; > + pipe_config->shared_dpll =3D DPLL_ID_PRIVATE; > = > tmp =3D I915_READ(PIPECONF(crtc->pipe)); > if (!(tmp & PIPECONF_ENABLE)) > @@ -5827,6 +5834,16 @@ static bool ironlake_get_pipe_config(struct intel_= crtc *crtc, > /* XXX: Can't properly read out the pch dpll pixel multiplier > * since we don't have state tracking for pch clocks yet. */ > pipe_config->pixel_multiplier =3D 1; > + > + if (HAS_PCH_IBX(dev_priv->dev)) { > + pipe_config->shared_dpll =3D crtc->pipe; Slightly magic, but all right. > + } else { > + tmp =3D I915_READ(PCH_DPLL_SEL); > + if (tmp & TRANS_DPLLB_SEL(crtc->pipe)) > + pipe_config->shared_dpll =3D DPLL_ID_PCH_PLL_B; > + else > + pipe_config->shared_dpll =3D DPLL_ID_PCH_PLL_A; > + } > } else { > pipe_config->pixel_multiplier =3D 1; > } > @@ -5907,6 +5924,8 @@ static bool haswell_get_pipe_config(struct intel_cr= tc *crtc, > uint32_t tmp; > = > pipe_config->cpu_transcoder =3D crtc->pipe; > + pipe_config->shared_dpll =3D DPLL_ID_PRIVATE; > + > tmp =3D I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_EDP)); > if (tmp & TRANS_DDI_FUNC_ENABLE) { > enum pipe trans_edp_pipe; > @@ -7768,6 +7787,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, > drm_mode_copy(&pipe_config->adjusted_mode, mode); > drm_mode_copy(&pipe_config->requested_mode, mode); > pipe_config->cpu_transcoder =3D to_intel_crtc(crtc)->pipe; > + pipe_config->shared_dpll =3D DPLL_ID_PRIVATE; Do we want to put this here, or should we have some DPLL_ID_INVALID, so that we'd always catch cases where we forgot to really think about which PLL should be used? > = > /* Compute a starting value for pipe_config->pipe_bpp taking the source > * plane pixel format and any sink constraints into account. Returns the > @@ -7816,7 +7836,7 @@ encoder_retry: > if (!pipe_config->port_clock) > pipe_config->port_clock =3D pipe_config->adjusted_mode.clock; > = > - ret =3D intel_crtc_compute_config(crtc, pipe_config); > + ret =3D intel_crtc_compute_config(to_intel_crtc(crtc), pipe_config); > if (ret < 0) { > DRM_DEBUG_KMS("CRTC fixup failed\n"); > goto fail; > @@ -8079,6 +8099,8 @@ intel_pipe_config_compare(struct drm_device *dev, > = > PIPE_CONF_CHECK_I(ips_enabled); > = > + PIPE_CONF_CHECK_I(shared_dpll); > + > #undef PIPE_CONF_CHECK_I > #undef PIPE_CONF_CHECK_FLAGS > = > -- = > 1.7.11.7 > = > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- = Ville Syrj=E4l=E4 Intel OTC