On Fri, 27 Sep 2013 23:09:26 +0300 Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > On Fri, Sep 27, 2013 at 12:22:11PM -0700, Jesse Barnes wrote: > > The global integrated clock source bit resides in DPLL B on VLV, but we > > were treating it as a per-pipe resource. It needs to be set whenever > > any PLL is active, > > Actually AFAIU the cri clock has to be running even if we just attempt > to do any register access to the phy. Ah hm, that explains the comment below. So yeah stuffing this into init_hw would be good, and just keeping it on all the time. > > > so pull setting the bit out of vlv_update_pll and > > into vlv_enable_pll. Also add a vlv_disable_pll to prevent disabling it > > when pipe B shuts down. > > > > I'm guessing on the references here, I expect this to bite any config > > where multiple displays are active or displays are moved from pipe to > > pipe. > > > > References: https://bugs.freedesktop.org/show_bug.cgi?id=67245 > > References: https://bugs.freedesktop.org/show_bug.cgi?id=69693 > > Signed-off-by: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx> > > --- > > drivers/gpu/drm/i915/intel_display.c | 27 ++++++++++++++++++++++++--- > > 1 file changed, 24 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 0eeba84..def2473 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -1386,6 +1386,13 @@ static void vlv_enable_pll(struct intel_crtc *crtc) > > if (IS_MOBILE(dev_priv->dev) && !IS_I830(dev_priv->dev)) > > assert_panel_unlocked(dev_priv, crtc->pipe); > > > > + /* Make sure to use the integrated clock source */ > > + if (!crtc->pipe) > > + I915_WRITE(DPLL(1), I915_READ(DPLL(1)) | > > + DPLL_INTEGRATED_CRI_CLK_VLV); > > We may need the cri clock before this, so I would just put this part > into some modeset hw init function. > > > + else > > + dpll |= DPLL_INTEGRATED_CRI_CLK_VLV; > > And obviously this part has to stay here, or we could do a > 'I915_READ(DPLL(1)) & DPLL_INTEGRATED_CRI_CLK_VLV)' to extract the > current setting from the hardware like you do in vlv_disable_pll(). > In any case I think doing it the same way for both enable and > disable would be good. > > Or we could leave setting of this bit up to vlv_update_pll() like it was > before. > > > + > > I915_WRITE(reg, dpll); > > POSTING_READ(reg); > > udelay(150); > > @@ -1476,6 +1483,20 @@ static void i9xx_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe) > > POSTING_READ(DPLL(pipe)); > > } > > > > +static void vlv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe) > > +{ > > + u32 val = 0; > > + > > + /* Make sure the pipe isn't still relying on us */ > > + assert_pipe_disabled(dev_priv, pipe); > > + > > + /* Leave integrated clock source enabled for the other pipe */ > > + if (pipe) > > + val = I915_READ(DPLL(pipe)) & DPLL_INTEGRATED_CRI_CLK_VLV; > > Right. As stated we need to either hardcode the bit on (if we assume 1 > is the right answer always) or we read it back like you do. Yeah I'll just keep it enabled all the time. The other bit I was missing is that the hw state tracking needs to have the bit set, or it'll complain about a DPLL mismatch. I'll post a fixed up one with both your and Daniel's changes applied. -- Jesse Barnes, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
