From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v2 3/3] drm/i915: clean up and simplify i9xx_crtc_mode_set wrt PLL handling Date: Thu, 5 Sep 2013 14:04:24 +0300 Message-ID: <20130905110423.GX11428@intel.com> References: <1378293610-26631-1-git-send-email-jani.nikula@intel.com> <1378293610-26631-3-git-send-email-jani.nikula@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id 8C494E5CF9 for ; Thu, 5 Sep 2013 04:04:27 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1378293610-26631-3-git-send-email-jani.nikula@intel.com> 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: Jani Nikula Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Wed, Sep 04, 2013 at 02:20:10PM +0300, Jani Nikula wrote: > Flat out skip anything to do with PLL if we have a DSI encoder (and thus > DSI PLL). Also skip PLL computation if the encoder has already set > clocks. This allows for some tidying up of the code, including a > superfluous call to intel_limit() for LVDS downclock path. > = > Signed-off-by: Jani Nikula > --- > drivers/gpu/drm/i915/intel_display.c | 44 +++++++++++++++++-----------= ------ > 1 file changed, 22 insertions(+), 22 deletions(-) > = > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/= intel_display.c > index 4939683..0646d14 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4891,9 +4891,12 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crt= c, > num_connectors++; > } > = > - refclk =3D i9xx_get_refclk(crtc, num_connectors); > + if (is_dsi) > + goto skip_dpll; > + > + if (!intel_crtc->config.clock_set) { > + refclk =3D i9xx_get_refclk(crtc, num_connectors); > = > - if (!is_dsi && !intel_crtc->config.clock_set) { > /* > * Returns a set of divisors for the desired target clock with > * the given refclk, or FALSE. The returned values represent > @@ -4904,28 +4907,25 @@ static int i9xx_crtc_mode_set(struct drm_crtc *cr= tc, > ok =3D dev_priv->display.find_dpll(limit, crtc, > intel_crtc->config.port_clock, > refclk, NULL, &clock); > - if (!ok && !intel_crtc->config.clock_set) { > + if (!ok) { > DRM_ERROR("Couldn't find PLL settings for mode!\n"); > return -EINVAL; > } > - } > = > - if (!is_dsi && is_lvds && dev_priv->lvds_downclock_avail) { > - /* > - * Ensure we match the reduced clock's P to the target clock. > - * If the clocks don't match, we can't switch the display clock > - * by using the FP0/FP1. In such case we will disable the LVDS > - * downclock feature. > - */ > - limit =3D intel_limit(crtc, refclk); > - has_reduced_clock =3D > - dev_priv->display.find_dpll(limit, crtc, > - dev_priv->lvds_downclock, > - refclk, &clock, > - &reduced_clock); > - } > - /* Compat-code for transition, will disappear. */ > - if (!intel_crtc->config.clock_set) { > + if (is_lvds && dev_priv->lvds_downclock_avail) { > + /* > + * Ensure we match the reduced clock's P to the target > + * clock. If the clocks don't match, we can't switch > + * the display clock by using the FP0/FP1. In such case > + * we will disable the LVDS downclock feature. > + */ > + has_reduced_clock =3D > + dev_priv->display.find_dpll(limit, crtc, > + dev_priv->lvds_downclock, > + refclk, &clock, > + &reduced_clock); > + } > + /* Compat-code for transition, will disappear. */ > intel_crtc->config.dpll.n =3D clock.n; > intel_crtc->config.dpll.m1 =3D clock.m1; > intel_crtc->config.dpll.m2 =3D clock.m2; > @@ -4938,14 +4938,14 @@ static int i9xx_crtc_mode_set(struct drm_crtc *cr= tc, > has_reduced_clock ? &reduced_clock : NULL, > num_connectors); > } else if (IS_VALLEYVIEW(dev)) { > - if (!is_dsi) > - vlv_update_pll(intel_crtc); > + vlv_update_pll(intel_crtc); > } else { > i9xx_update_pll(intel_crtc, > has_reduced_clock ? &reduced_clock : NULL, > num_connectors); > } This looks all right. But I was thinking that we could just set .clock_set=3Dtrue in DSI code and move the DSI PLL enable here. But we anyway have to move the DSI PLL calculations out from the PLL enable func, so I guess we can leave all that for later. So in the meantime: Reviewed-by: Ville Syrj=E4l=E4 > = > +skip_dpll: > /* Set up the display plane register */ > dspcntr =3D DISPPLANE_GAMMA_ENABLE; > = > -- = > 1.7.9.5 -- = Ville Syrj=E4l=E4 Intel OTC