From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915/vlv: untangle integrated clock source handling v3 Date: Tue, 1 Oct 2013 09:42:01 +0300 Message-ID: <20131001064201.GK9395@intel.com> References: <1380322949-2568-1-git-send-email-jbarnes@virtuousgeek.org> <20130928095421.GK26592@phenom.ffwll.local> <20130928080514.587a32b3@jbarnes-desktop> <20130930211954.GJ9395@intel.com> <20130930152044.550ba1a0@jbarnes-t420> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 45BF5E62CB for ; Mon, 30 Sep 2013 23:42:09 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20130930152044.550ba1a0@jbarnes-t420> 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: Jesse Barnes Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Mon, Sep 30, 2013 at 03:20:44PM -0700, Jesse Barnes wrote: > On Tue, 1 Oct 2013 00:19:55 +0300 > Ville Syrj=E4l=E4 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 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 when= ever > > > > > 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 disab= ling it > > > > > when pipe B shuts down. > > > > > = > > > > > I'm guessing on the references here, I expect this to bite any co= nfig > > > > > where multiple displays are active or displays are moved from pip= e to > > > > > pipe. > > > > > = > > > > > v2: re-add bits in vlv_update_pll to keep from confusing the stat= e 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 tha= t clock > > > > running all the time? My long-term plan/idea for these display refc= locks > > > > was to enable/disable them in the ->modeset_global_resources callba= ck so > > > > that we only ever enable what we actually need. Haswell has this so= mewhat > > > > 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 t= oo > > > 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 -- = Ville Syrj=E4l=E4 Intel OTC