Re: [PATCH 3/3] drm/i915: Share cdclk code for BDW and BXT

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Oct 27, 2015 at 03:43:03PM +0200, Jani Nikula wrote:
> On Tue, 27 Oct 2015, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > The difference betwen the BXT and BDW cdclk code boils down to two
> > things: claming the cdclk to the maximum supported, and panel fitter
> > downscaling ratio
> >
> > Unifying the the max cdclk clamping is easy, just do it.
> >
> > And as far as the panel fitter is concerned, SKL+ already (ab)use the
> > pch pfit state for its pipe scaler information, so it will compute the
> > adjusted pixel rate correctly.
> >
> > So we can just use the BDW code for BXT, as long as we lift the BXT
> > pixel rate -> cdclk selection into the correct place.
> 
> I don't like the direction taken by this patch.
> 
> We have the platform specific functions to keep stuff platform
> specific. Now you claim bdw and bxt are almost the same, yet the
> functions are filled with conditionals on the platform. I value having
> straightforward and readable platform specific hooks much higher than
> the slight reduction in line count.

I agree that it's not all that nice. The real problem is that the current
cdclk  function pointers are just too high level. They really should
be at the calc_cdclk and set_cdclk level. But fixing that would require
actually dealing with the gmch pfit, and stuffing the power domain
hack (assuming we still need it) and the pfi programming for vlv/chv
into the lower level functions. And looks like no one has bothered to do
any of that.

> 
> BR,
> Jani.
> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 87 +++++++++++++-----------------------
> >  1 file changed, 30 insertions(+), 57 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 978f543..0c782c7 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5932,25 +5932,6 @@ static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv,
> >  		return 200000;
> >  }
> >  
> > -static int broxton_calc_cdclk(struct drm_i915_private *dev_priv,
> > -			      int max_pixclk)
> > -{
> > -	/*
> > -	 * FIXME:
> > -	 * - set 19.2MHz bypass frequency if there are no active pipes
> > -	 */
> > -	if (max_pixclk > 576000)
> > -		return 624000;
> > -	else if (max_pixclk > 384000)
> > -		return 576000;
> > -	else if (max_pixclk > 288000)
> > -		return 384000;
> > -	else if (max_pixclk > 144000)
> > -		return 288000;
> > -	else
> > -		return 144000;
> > -}
> > -
> >  /* Compute the max pixel clock for new configuration. Uses atomic state if
> >   * that's non-NULL, look at current state otherwise. */
> >  static int intel_mode_max_pixclk(struct drm_device *dev,
> > @@ -5990,21 +5971,6 @@ static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  	return 0;
> >  }
> >  
> > -static int broxton_modeset_calc_cdclk(struct drm_atomic_state *state)
> > -{
> > -	struct drm_device *dev = state->dev;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	int max_pixclk = intel_mode_max_pixclk(dev, state);
> > -
> > -	if (max_pixclk < 0)
> > -		return max_pixclk;
> > -
> > -	to_intel_atomic_state(state)->cdclk =
> > -		broxton_calc_cdclk(dev_priv, max_pixclk);
> > -
> > -	return 0;
> > -}
> > -
> >  static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv)
> >  {
> >  	unsigned int credits, default_credits;
> > @@ -9497,14 +9463,6 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv)
> >  	intel_prepare_ddi(dev);
> >  }
> >  
> > -static void broxton_modeset_commit_cdclk(struct drm_atomic_state *old_state)
> > -{
> > -	struct drm_device *dev = old_state->dev;
> > -	unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk;
> > -
> > -	broxton_set_cdclk(dev, req_cdclk);
> > -}
> > -
> >  /* compute the max rate for new configuration */
> >  static int ilk_max_pixel_rate(struct drm_atomic_state *state)
> >  {
> > @@ -9621,14 +9579,31 @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  	 * FIXME should also account for plane ratio
> >  	 * once 64bpp pixel formats are supported.
> >  	 */
> > -	if (max_pixclk > 540000)
> > -		cdclk = 675000;
> > -	else if (max_pixclk > 450000)
> > -		cdclk = 540000;
> > -	else if (max_pixclk > 337500)
> > -		cdclk = 450000;
> > -	else
> > -		cdclk = 337500;
> > +	if (IS_BROXTON(dev_priv)) {
> > +		/*
> > +		 * FIXME:
> > +		 * - set 19.2MHz bypass frequency if there are no active pipes
> > +		 */
> > +		if (max_pixclk > 576000)
> > +			cdclk = 624000;
> > +		else if (max_pixclk > 384000)
> > +			cdclk = 576000;
> > +		else if (max_pixclk > 288000)
> > +			cdclk = 384000;
> > +		else if (max_pixclk > 144000)
> > +			cdclk = 288000;
> > +		else
> > +			cdclk = 144000;
> > +	} else {
> > +		if (max_pixclk > 540000)
> > +			cdclk = 675000;
> > +		else if (max_pixclk > 450000)
> > +			cdclk = 540000;
> > +		else if (max_pixclk > 337500)
> > +			cdclk = 450000;
> > +		else
> > +			cdclk = 337500;
> > +	}
> >  
> >  	/*
> >  	 * FIXME move the cdclk caclulation to
> > @@ -9650,7 +9625,10 @@ static void broadwell_modeset_commit_cdclk(struct drm_atomic_state *old_state)
> >  	struct drm_device *dev = old_state->dev;
> >  	unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk;
> >  
> > -	broadwell_set_cdclk(dev, req_cdclk);
> > +	if (IS_BROXTON(dev))
> > +		broxton_set_cdclk(dev, req_cdclk);
> > +	else
> > +		broadwell_set_cdclk(dev, req_cdclk);
> >  }
> >  
> >  static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
> > @@ -14619,7 +14597,7 @@ static void intel_init_display(struct drm_device *dev)
> >  		dev_priv->display.fdi_link_train = hsw_fdi_link_train;
> >  	}
> >  
> > -	if (IS_BROADWELL(dev)) {
> > +	if (IS_BROADWELL(dev) || IS_BROXTON(dev)) {
> >  		dev_priv->display.modeset_commit_cdclk =
> >  			broadwell_modeset_commit_cdclk;
> >  		dev_priv->display.modeset_calc_cdclk =
> > @@ -14629,11 +14607,6 @@ static void intel_init_display(struct drm_device *dev)
> >  			valleyview_modeset_commit_cdclk;
> >  		dev_priv->display.modeset_calc_cdclk =
> >  			valleyview_modeset_calc_cdclk;
> > -	} else if (IS_BROXTON(dev)) {
> > -		dev_priv->display.modeset_commit_cdclk =
> > -			broxton_modeset_commit_cdclk;
> > -		dev_priv->display.modeset_calc_cdclk =
> > -			broxton_modeset_calc_cdclk;
> >  	}
> >  
> >  	switch (INTEL_INFO(dev)->gen) {
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx