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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxx wrote:
> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > >
> > > 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.
-Daniel

> 
> > 
> > BR,
> > Jani.
> > 
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > ---
> > >  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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux