From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [Intel-gfx] [PATCH] drm/i915: Allow PCH DPLL sharing regardless of DPLL_SDVO_HIGH_SPEED Date: Wed, 28 Sep 2016 17:18:55 +0300 Message-ID: <20160928141855.GO4329@intel.com> References: <1474878646-17711-1-git-send-email-ville.syrjala@linux.intel.com> <1475062339.5813.5.camel@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Return-path: Content-Disposition: inline In-Reply-To: <1475062339.5813.5.camel@gmail.com> Sender: stable-owner@vger.kernel.org To: Ander Conselvan De Oliveira Cc: intel-gfx@lists.freedesktop.org, Nick Yamane , Daniel Vetter , stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org On Wed, Sep 28, 2016 at 02:32:19PM +0300, Ander Conselvan De Oliveira wrote: > On Mon, 2016-09-26 at 11:30 +0300, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä > > > > DPLL_SDVO_HIGH_SPEED must be set for SDVO/HDMI/DP, but nowhere is it > > forbidden to set it for LVDS/CRT as well. So let's also set it on > > CRT to make it possible to share the DPLL between HDMI and CRT. > > > > What that bit apparently does is enable the x5 clock to the port, > > which then pumps out the bits on both edges of the clock. The DAC > > doesn't need that clock since it's not pumping out bits, but I don't > > think it hurts to have the DPLL output that clock anyway. > > > > This is fairly important on IVB since it has only two DPLLs with three > > pipes. So trying to drive three or more PCH ports with three pipes > > is only possible when at least one of the DPLLs gets shared between > > two of the pipes. > > > > SNB doesn't really need to do this since it has only two pipes. It could > > be done to avoid enabling the second DPLL at all in certain cases, but > > I'm not sure that's such a huge win. So let's not do it for SNB, at > > least for now. On ILK it never makes sense as the DPLLs can't be shared. > > > > v2: Just always enable the high speed clock to keep things simple (Daniel) > >     Beef up the commit message a bit (Daniel) > > > > Cc: Nick Yamane > > Cc: Daniel Vetter > > Cc: stable@vger.kernel.org > > Tested-by: Nick Yamane > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97204 > > Signed-off-by: Ville Syrjälä > > --- > >  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++ > >  1 file changed, 18 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index e5ad1010c8b1..45ff5007544c 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -9512,6 +9512,24 @@ static void ironlake_compute_dpll(struct intel_crtc > > *intel_crtc, > >   if (intel_crtc_has_dp_encoder(crtc_state)) > >   dpll |= DPLL_SDVO_HIGH_SPEED; > >   > > + /* > > +  * The high speed IO clock is only really required for > > +  * SDVO/HDMI/DP, but we also enable it for CRT to make it > > +  * possible to share the DPLL between CRT and HDMI. Enabling > > +  * the clock needlessly does no real harm, except use up a > > +  * bit of power potentially. > > I guess we could have a smarter way to check if two configurations are > compatible than the current memcmp(). I.e., a platform hook that takes two PLL > configs and either returns a merged configuration that satisfy both or signal > failure. That way we could only enable the high speed clock for CRT if really > necessary. > > But meh, sounds like too much work for very little gain. > > The documentation indeed doesn't say anything about not enabling this with CRT, > so > > Reviewed-by: Ander Conselvan de Oliveira Thanks. Pushed to dinq. > > > +  * > > +  * We'll limit this to IVB with 3 pipes, since it has only two > > +  * DPLLs and so DPLL sharing is the only way to get three pipes > > +  * driving PCH ports at the same time. On SNB we could do this, > > +  * and potentially avoid enabling the second DPLL, but it's not > > +  * clear if it''s a win or loss power wise. No point in doing > > +  * this on ILK at all since it has a fixed DPLL<->pipe mapping. > > +  */ > > + if (INTEL_INFO(dev_priv)->num_pipes == 3 && > > +     intel_crtc_has_type(crtc_state, INTEL_OUTPUT_ANALOG)) > > + dpll |= DPLL_SDVO_HIGH_SPEED; > > + > >   /* compute bitmask from p1 value */ > >   dpll |= (1 << (crtc_state->dpll.p1 - 1)) << > > DPLL_FPA01_P1_POST_DIV_SHIFT; > >   /* also FPA1 */ -- Ville Syrjälä Intel OTC From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com ([192.55.52.88]:8154 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932474AbcI1OTE (ORCPT ); Wed, 28 Sep 2016 10:19:04 -0400 Date: Wed, 28 Sep 2016 17:18:55 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Ander Conselvan De Oliveira Cc: intel-gfx@lists.freedesktop.org, Nick Yamane , Daniel Vetter , stable@vger.kernel.org Subject: Re: [Intel-gfx] [PATCH] drm/i915: Allow PCH DPLL sharing regardless of DPLL_SDVO_HIGH_SPEED Message-ID: <20160928141855.GO4329@intel.com> References: <1474878646-17711-1-git-send-email-ville.syrjala@linux.intel.com> <1475062339.5813.5.camel@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1475062339.5813.5.camel@gmail.com> Sender: stable-owner@vger.kernel.org List-ID: On Wed, Sep 28, 2016 at 02:32:19PM +0300, Ander Conselvan De Oliveira wrote: > On Mon, 2016-09-26 at 11:30 +0300, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrj�l� > > > > DPLL_SDVO_HIGH_SPEED must be set for SDVO/HDMI/DP, but nowhere is it > > forbidden to set it for LVDS/CRT as well. So let's also set it on > > CRT to make it possible to share the DPLL between HDMI and CRT. > > > > What that bit apparently does is enable the x5 clock to the port, > > which then pumps out the bits on both edges of the clock. The DAC > > doesn't need that clock since it's not pumping out bits, but I don't > > think it hurts to have the DPLL output that clock anyway. > > > > This is fairly important on IVB since it has only two DPLLs with three > > pipes. So trying to drive three or more PCH ports with three pipes > > is only possible when at least one of the DPLLs gets shared between > > two of the pipes. > > > > SNB doesn't really need to do this since it has only two pipes. It could > > be done to avoid enabling the second DPLL at all in certain cases, but > > I'm not sure that's such a huge win. So let's not do it for SNB, at > > least for now. On ILK it never makes sense as the DPLLs can't be shared. > > > > v2: Just always enable the high speed clock to keep things simple (Daniel) > > ����Beef up the commit message a bit (Daniel) > > > > Cc: Nick Yamane > > Cc: Daniel Vetter > > Cc: stable@vger.kernel.org > > Tested-by: Nick Yamane > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97204 > > Signed-off-by: Ville Syrj�l� > > --- > > �drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++ > > �1 file changed, 18 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index e5ad1010c8b1..45ff5007544c 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -9512,6 +9512,24 @@ static void ironlake_compute_dpll(struct intel_crtc > > *intel_crtc, > > � if (intel_crtc_has_dp_encoder(crtc_state)) > > � dpll |= DPLL_SDVO_HIGH_SPEED; > > � > > + /* > > + �* The high speed IO clock is only really required for > > + �* SDVO/HDMI/DP, but we also enable it for CRT to make it > > + �* possible to share the DPLL between CRT and HDMI. Enabling > > + �* the clock needlessly does no real harm, except use up a > > + �* bit of power potentially. > > I guess we could have a smarter way to check if two configurations are > compatible than the current memcmp(). I.e., a platform hook that takes two PLL > configs and either returns a merged configuration that satisfy both or signal > failure. That way we could only enable the high speed clock for CRT if really > necessary. > > But meh, sounds like too much work for very little gain. > > The documentation indeed doesn't say anything about not enabling this with CRT, > so > > Reviewed-by: Ander Conselvan de Oliveira Thanks. Pushed to dinq. > > > + �* > > + �* We'll limit this to IVB with 3 pipes, since it has only two > > + �* DPLLs and so DPLL sharing is the only way to get three pipes > > + �* driving PCH ports at the same time. On SNB we could do this, > > + �* and potentially avoid enabling the second DPLL, but it's not > > + �* clear if it''s a win or loss power wise. No point in doing > > + �* this on ILK at all since it has a fixed DPLL<->pipe mapping. > > + �*/ > > + if (INTEL_INFO(dev_priv)->num_pipes == 3 && > > + ����intel_crtc_has_type(crtc_state, INTEL_OUTPUT_ANALOG)) > > + dpll |= DPLL_SDVO_HIGH_SPEED; > > + > > � /* compute bitmask from p1 value */ > > � dpll |= (1 << (crtc_state->dpll.p1 - 1)) << > > DPLL_FPA01_P1_POST_DIV_SHIFT; > > � /* also FPA1 */ -- Ville Syrj�l� Intel OTC