| Message ID | 1380322949-2568-1-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Sep 27, 2013 at 04:02:29PM -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, 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. > > v2: re-add bits in vlv_update_pll to keep from confusing the state checker > v3: use enum pipe checks (Daniel) > set CRI clock source early (Ville) > consistently set CRI clock source everywhere (Ville) Btw do we care about the additional power consumption of having that clock running all the time? My long-term plan/idea for these display refclocks was to enable/disable them in the ->modeset_global_resources callback so that we only ever enable what we actually need. Haswell has this somewhat implemented already implicitly through the pc8+ power refcounting. Just a "have you thought about this" comment. > 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@virtuousgeek.org> > > int clock Dropped some paste garbage by accident? > --- > drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++++++++++++++--- > 1 file changed, 33 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 9a83236..1c76a26 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -1387,6 +1387,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 the integrated clock source is enabled */ > + if (crtc->pipe == PIPE_B) > + dpll |= DPLL_INTEGRATED_CRI_CLK_VLV; > + else > + I915_WRITE(DPLL(1), I915_READ(DPLL(1)) | s/1/PIPE_B/g > + DPLL_INTEGRATED_CRI_CLK_VLV); > + > I915_WRITE(reg, dpll); > POSTING_READ(reg); > udelay(150); > @@ -1477,6 +1484,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 */ > + if (pipe == PIPE_B) > + val = DPLL_INTEGRATED_CRI_CLK_VLV; > + I915_WRITE(DPLL(pipe), val); > + POSTING_READ(DPLL(pipe)); > +} > + > void vlv_wait_port_ready(struct drm_i915_private *dev_priv, int port) > { > u32 port_mask; > @@ -3887,7 +3908,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) > if (encoder->post_disable) > encoder->post_disable(encoder); > > - if (!intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI)) > + if (IS_VALLEYVIEW(dev) && !intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI)) > + vlv_disable_pll(dev_priv, pipe); > + else if (!IS_VALLEYVIEW(dev)) > i9xx_disable_pll(dev_priv, pipe); > > intel_crtc->active = false; > @@ -4627,9 +4650,9 @@ static void vlv_update_pll(struct intel_crtc *crtc) > /* Enable DPIO clock input */ > dpll = DPLL_EXT_BUFFER_ENABLE_VLV | DPLL_REFA_CLK_ENABLE_VLV | > DPLL_VGA_MODE_DIS | DPLL_INTEGRATED_CLOCK_VLV; > - if (pipe) > + /* We should never disable this, set it here for state tracking */ > + if (pipe == PIPE_B) > dpll |= DPLL_INTEGRATED_CRI_CLK_VLV; > - > dpll |= DPLL_VCO_ENABLE; > crtc->config.dpll_hw_state.dpll = dpll; > > @@ -10296,8 +10319,15 @@ void i915_disable_vga_mem(struct drm_device *dev) > > void intel_modeset_init_hw(struct drm_device *dev) > { > + struct drm_i915_private *dev_priv = dev->dev_private; > + > intel_prepare_ddi(dev); > > + /* Enable the CRI clock source so we can get at the display */ > + if (IS_VALLEYVIEW(dev)) > + I915_WRITE(DPLL(1), I915_READ(DPLL(1)) | s/1/PIPE_B/g Also there's this --in-reply-to switch for git send-email to keep discussions on the m-l neatly tied up ;-) > + DPLL_INTEGRATED_CRI_CLK_VLV); > + > intel_init_dpio(dev); > > intel_init_clock_gating(dev); > -- > 1.8.3.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Sat, 28 Sep 2013 11:54:21 +0200 Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Sep 27, 2013 at 04:02:29PM -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, 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. > > > > v2: re-add bits in vlv_update_pll to keep from confusing the state checker > > v3: use enum pipe checks (Daniel) > > set CRI clock source early (Ville) > > consistently set CRI clock source everywhere (Ville) > > Btw do we care about the additional power consumption of having that clock > running all the time? My long-term plan/idea for these display refclocks > was to enable/disable them in the ->modeset_global_resources callback so > that we only ever enable what we actually need. Haswell has this somewhat > implemented already implicitly through the pc8+ power refcounting. > > Just a "have you thought about this" comment. Yes I have. But I don't think this actually enables a clock, just allows it to flow through to the PLLs and DPIO. Shutting down the display power well will probably take care of it though, so I'm not too worried about it now (plus like Ville said, this may be required to access some display reg anyway). > > > 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@virtuousgeek.org> > > > > int clock > > Dropped some paste garbage by accident? Oh you don't think it add anything to the changelog? :p > > > --- > > drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++++++++++++++--- > > 1 file changed, 33 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 9a83236..1c76a26 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -1387,6 +1387,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 the integrated clock source is enabled */ > > + if (crtc->pipe == PIPE_B) > > + dpll |= DPLL_INTEGRATED_CRI_CLK_VLV; > > + else > > + I915_WRITE(DPLL(1), I915_READ(DPLL(1)) | > > s/1/PIPE_B/g I call "too late" on this one. :) You wanted me to avoid enum cast to bool in your previous reply, but didn't suggest that I use enums as ints here. Plus I like this way better... so you can change it when you apply if you feel strongly about it. > Also there's this --in-reply-to switch for git send-email to keep > discussions on the m-l neatly tied up ;-) Yeah need to fix my send-email on this new machine, it doesn't prompt me for anything anymore, which is a pain (and why that other patch got sent out).
On Sat, Sep 28, 2013 at 08:05:14AM -0700, Jesse Barnes wrote: > On Sat, 28 Sep 2013 11:54:21 +0200 > Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Fri, Sep 27, 2013 at 04:02:29PM -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, 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. > > > > > > v2: re-add bits in vlv_update_pll to keep from confusing the state checker > > > v3: use enum pipe checks (Daniel) > > > set CRI clock source early (Ville) > > > consistently set CRI clock source everywhere (Ville) > > > > Btw do we care about the additional power consumption of having that clock > > running all the time? My long-term plan/idea for these display refclocks > > was to enable/disable them in the ->modeset_global_resources callback so > > that we only ever enable what we actually need. Haswell has this somewhat > > implemented already implicitly through the pc8+ power refcounting. > > > > Just a "have you thought about this" comment. > > Yes I have. But I don't think this actually enables a clock, just > allows it to flow through to the PLLs and DPIO. Shutting down the > display power well will probably take care of it though, so I'm not too > worried about it now (plus like Ville said, this may be required to > access some display reg anyway). Yeah I think this doesn't actually enable the clock. IIRC there's some DPIO refclock stuff in ccu, so we may be able to turn it off there if needed. But what happened to the idea to set this bit in init_hw or somesuch place and just preserving it for pipe B in the PLL funcs? > > > > > > 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@virtuousgeek.org> > > > > > > int clock > > > > Dropped some paste garbage by accident? > > Oh you don't think it add anything to the changelog? :p > > > > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++++++++++++++--- > > > 1 file changed, 33 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index 9a83236..1c76a26 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -1387,6 +1387,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 the integrated clock source is enabled */ > > > + if (crtc->pipe == PIPE_B) > > > + dpll |= DPLL_INTEGRATED_CRI_CLK_VLV; > > > + else > > > + I915_WRITE(DPLL(1), I915_READ(DPLL(1)) | > > > > s/1/PIPE_B/g > > I call "too late" on this one. :) You wanted me to avoid enum cast to > bool in your previous reply, but didn't suggest that I use enums as > ints here. Plus I like this way better... so you can change it when > you apply if you feel strongly about it. > > > Also there's this --in-reply-to switch for git send-email to keep > > discussions on the m-l neatly tied up ;-) > > Yeah need to fix my send-email on this new machine, it doesn't prompt > me for anything anymore, which is a pain (and why that other patch got > sent out). > > -- > Jesse Barnes, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 1 Oct 2013 00:19:55 +0300 Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Sat, Sep 28, 2013 at 08:05:14AM -0700, Jesse Barnes wrote: > > On Sat, 28 Sep 2013 11:54:21 +0200 > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > On Fri, Sep 27, 2013 at 04:02:29PM -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, 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. > > > > > > > > v2: re-add bits in vlv_update_pll to keep from confusing the state checker > > > > v3: use enum pipe checks (Daniel) > > > > set CRI clock source early (Ville) > > > > consistently set CRI clock source everywhere (Ville) > > > > > > Btw do we care about the additional power consumption of having that clock > > > running all the time? My long-term plan/idea for these display refclocks > > > was to enable/disable them in the ->modeset_global_resources callback so > > > that we only ever enable what we actually need. Haswell has this somewhat > > > implemented already implicitly through the pc8+ power refcounting. > > > > > > Just a "have you thought about this" comment. > > > > Yes I have. But I don't think this actually enables a clock, just > > allows it to flow through to the PLLs and DPIO. Shutting down the > > display power well will probably take care of it though, so I'm not too > > worried about it now (plus like Ville said, this may be required to > > access some display reg anyway). > > Yeah I think this doesn't actually enable the clock. IIRC there's some > DPIO refclock stuff in ccu, so we may be able to turn it off there if > needed. > > But what happened to the idea to set this bit in init_hw or somesuch > place and just preserving it for pipe B in the PLL funcs? It's being set early and preserved in the v3 version (well overwritten, but that's the same number of lines). Daniel and I briefly discussed having a global settings mask for the PLL checker and global_resources too, but that would be even more lines than this simple approach. Jesse
On Mon, Sep 30, 2013 at 03:20:44PM -0700, Jesse Barnes wrote: > On Tue, 1 Oct 2013 00:19:55 +0300 > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > On Sat, Sep 28, 2013 at 08:05:14AM -0700, Jesse Barnes wrote: > > > On Sat, 28 Sep 2013 11:54:21 +0200 > > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > > On Fri, Sep 27, 2013 at 04:02:29PM -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, 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. > > > > > > > > > > v2: re-add bits in vlv_update_pll to keep from confusing the state checker > > > > > v3: use enum pipe checks (Daniel) > > > > > set CRI clock source early (Ville) > > > > > consistently set CRI clock source everywhere (Ville) > > > > > > > > Btw do we care about the additional power consumption of having that clock > > > > running all the time? My long-term plan/idea for these display refclocks > > > > was to enable/disable them in the ->modeset_global_resources callback so > > > > that we only ever enable what we actually need. Haswell has this somewhat > > > > implemented already implicitly through the pc8+ power refcounting. > > > > > > > > Just a "have you thought about this" comment. > > > > > > Yes I have. But I don't think this actually enables a clock, just > > > allows it to flow through to the PLLs and DPIO. Shutting down the > > > display power well will probably take care of it though, so I'm not too > > > worried about it now (plus like Ville said, this may be required to > > > access some display reg anyway). > > > > Yeah I think this doesn't actually enable the clock. IIRC there's some > > DPIO refclock stuff in ccu, so we may be able to turn it off there if > > needed. > > > > But what happened to the idea to set this bit in init_hw or somesuch > > place and just preserving it for pipe B in the PLL funcs? > > It's being set early and preserved in the v3 version (well overwritten, > but that's the same number of lines). Ah so it is. But you're also still setting it in vlv_enable_pll(), which is no longer necessary. > > Daniel and I briefly discussed having a global settings mask for the > PLL checker and global_resources too, but that would be even more lines > than this simple approach. > > Jesse
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 9a83236..1c76a26 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -1387,6 +1387,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 the integrated clock source is enabled */ + if (crtc->pipe == PIPE_B) + dpll |= DPLL_INTEGRATED_CRI_CLK_VLV; + else + I915_WRITE(DPLL(1), I915_READ(DPLL(1)) | + DPLL_INTEGRATED_CRI_CLK_VLV); + I915_WRITE(reg, dpll); POSTING_READ(reg); udelay(150); @@ -1477,6 +1484,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 */ + if (pipe == PIPE_B) + val = DPLL_INTEGRATED_CRI_CLK_VLV; + I915_WRITE(DPLL(pipe), val); + POSTING_READ(DPLL(pipe)); +} + void vlv_wait_port_ready(struct drm_i915_private *dev_priv, int port) { u32 port_mask; @@ -3887,7 +3908,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) if (encoder->post_disable) encoder->post_disable(encoder); - if (!intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI)) + if (IS_VALLEYVIEW(dev) && !intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI)) + vlv_disable_pll(dev_priv, pipe); + else if (!IS_VALLEYVIEW(dev)) i9xx_disable_pll(dev_priv, pipe); intel_crtc->active = false; @@ -4627,9 +4650,9 @@ static void vlv_update_pll(struct intel_crtc *crtc) /* Enable DPIO clock input */ dpll = DPLL_EXT_BUFFER_ENABLE_VLV | DPLL_REFA_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS | DPLL_INTEGRATED_CLOCK_VLV; - if (pipe) + /* We should never disable this, set it here for state tracking */ + if (pipe == PIPE_B) dpll |= DPLL_INTEGRATED_CRI_CLK_VLV; - dpll |= DPLL_VCO_ENABLE; crtc->config.dpll_hw_state.dpll = dpll; @@ -10296,8 +10319,15 @@ void i915_disable_vga_mem(struct drm_device *dev) void intel_modeset_init_hw(struct drm_device *dev) { + struct drm_i915_private *dev_priv = dev->dev_private; + intel_prepare_ddi(dev); + /* Enable the CRI clock source so we can get at the display */ + if (IS_VALLEYVIEW(dev)) + I915_WRITE(DPLL(1), I915_READ(DPLL(1)) | + DPLL_INTEGRATED_CRI_CLK_VLV); + intel_init_dpio(dev); intel_init_clock_gating(dev);
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, 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. v2: re-add bits in vlv_update_pll to keep from confusing the state checker v3: use enum pipe checks (Daniel) set CRI clock source early (Ville) consistently set CRI clock source everywhere (Ville) 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@virtuousgeek.org> int clock --- drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-)