Re: [PATCH 2/2] drm/i915: Move Valleyview DP DPLL divisor calc to intel_dp_set_clock

["Lee, Chon Ming" <chon.ming.lee@intel.com>]

On 08/30 11:00, Jani Nikula wrote:
> 
> [Okay, I missed Daniel's review, and noticed I hadn't actually hit send
> on this one either... but here goes anyway...]
> 
> On Fri, 30 Aug 2013, Chon Ming Lee <chon.ming.lee@intel.com> wrote:
> > For DP pll settings, there is only two golden configs.  Instead of running
> > through the algorithm to determine it, hardcode the value and get it
> > determine in intel_dp_set_clock.
> >
> > Signed-off-by: Chon Ming Lee <chon.ming.lee@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |   22 ++++------------------
> >  drivers/gpu/drm/i915/intel_dp.c      |   12 +++++++++++-
> >  2 files changed, 15 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index f526ea9..453fa16 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -339,19 +339,6 @@ static const intel_limit_t intel_limits_vlv_hdmi = {
> >  		.p2_slow = 2, .p2_fast = 20 },
> >  };
> >  
> > -static const intel_limit_t intel_limits_vlv_dp = {
> > -	.dot = { .min = 25000, .max = 270000 },
> > -	.vco = { .min = 4000000, .max = 6000000 },
> > -	.n = { .min = 1, .max = 7 },
> > -	.m = { .min = 22, .max = 450 },
> > -	.m1 = { .min = 2, .max = 3 },
> > -	.m2 = { .min = 11, .max = 156 },
> > -	.p = { .min = 10, .max = 30 },
> > -	.p1 = { .min = 1, .max = 3 },
> > -	.p2 = { .dot_limit = 270000,
> > -		.p2_slow = 2, .p2_fast = 20 },
> > -};
> > -
> >  static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc,
> >  						int refclk)
> >  {
> > @@ -414,10 +401,8 @@ static const intel_limit_t *intel_limit(struct drm_crtc *crtc, int refclk)
> >  	} else if (IS_VALLEYVIEW(dev)) {
> >  		if (intel_pipe_has_type(crtc, INTEL_OUTPUT_ANALOG))
> >  			limit = &intel_limits_vlv_dac;
> > -		else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))
> > +		else 
> >  			limit = &intel_limits_vlv_hdmi;
> > -		else
> > -			limit = &intel_limits_vlv_dp;
> >  	} else if (!IS_GEN2(dev)) {
> >  		if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS))
> >  			limit = &intel_limits_i9xx_lvds;
> > @@ -4889,15 +4874,16 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> >  	}
> >  
> >  	refclk = i9xx_get_refclk(crtc, num_connectors);
> > +		
> > +	limit = intel_limit(crtc, refclk);
> 
> Did you move this here just to avoid the warning about uninitialized
> limit? It's a bit ugly... but then again the the whole is_dsi vs. not is
> rather ugly already. *shrug*.
> 
Yes, correct.  I can reverse it but, just have to add another limit =
intel_limit(crtc, refclk); in this if statement.

if (!is_dsi && is_lvds && dev_priv->lvds_downclock_avail) {

Will it more make sense?

> >  
> > -	if (!is_dsi) {
> > +	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
> >  		 * the clock equation: reflck * (5 * (m1 + 2) + (m2 + 2)) / (n +
> >  		 * 2) / p1 / p2.
> >  		 */
> > -		limit = intel_limit(crtc, refclk);
> >  		ok = dev_priv->display.find_dpll(limit, crtc,
> >  						 intel_crtc->config.port_clock,
> >  						 refclk, NULL, &clock);
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index ab8a5ff..89a2606 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -51,6 +51,10 @@ static const struct dp_link_dpll pch_dpll[] =
> >  				{{ DP_LINK_BW_1_62, {1,12,9,2,10,0,0,0,0}},
> >  				{ DP_LINK_BW_2_7,  {2,14,8,1,10,0,0,0,0}}};
> >  
> > +static const struct dp_link_dpll vlv_dpll[] =
> > +				{{ DP_LINK_BW_1_62, {5,3,81,3,2,0,0,0,0}},
> > +				{ DP_LINK_BW_2_7, {1,2,27,2,2,0,0,0,0}}};
> > +
> >  /**
> >   * is_edp - is the given port attached to an eDP panel (either CPU or PCH)
> >   * @intel_dp: DP struct
> > @@ -683,7 +687,13 @@ intel_dp_set_clock(struct intel_encoder *encoder,
> >  		}
> >  		pipe_config->clock_set = true;
> >  	} else if (IS_VALLEYVIEW(dev)) {
> > -		/* FIXME: Need to figure out optimized DP clocks for vlv. */
> > +		for(i = 0; i < sizeof(vlv_dpll) / sizeof(struct dp_link_dpll); i++) {
> > +			if (link_bw == vlv_dpll[i].link_bw){ 
> > +				pipe_config->dpll = vlv_dpll[i].dpll;
> > +				break;
> > +			}
> > +		}
> > +		pipe_config->clock_set = true;
> 
> You now have three similar loops in the function. A follow-up patch
> could pick the table to use in the if branches, and have a single loop
> at the end. You could handle the array size by having .link_bw = 0 in
> the last entry as a stop condition, and using that as the fallback entry
> too (see my review of patch 1 about unknown link_bw values).
> 

Will make this change.  

> BR,
> Jani.
> 
> 
> >  	}
> >  }
> >  
> > -- 
> > 1.7.7.6
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center