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

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

On Fri, Oct 30, 2015 at 05:06:27PM +0100, Daniel Vetter wrote:
> On Tue, Oct 27, 2015 at 04:31:56PM +0200, Ville Syrjälä wrote:
> > 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.
> 
> vfunc hooks imo should be high-level, with common code shared by
> extracting helpers. Trying to make vfuncs super flexible is imo an
> approach which often just leads to big trouble. We probably should rip out
> a pile of the current cdlock vfunc hooks ... Unfortunately atomic is still
> in-flight a lot, and it's also lacking some really important top-level
> vfuncs for handling modesetting.

The top level cdclk logic is (or rather should be) the same for all
platforms. Shoveling it into vfuncs doesn't make sense to me. Only
the low level details differ.

> -Daniel
> 
> > 
> > > 
> > > 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
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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