Re: [PATCH 19/19] drm/i915: Modeset global_pipes() update

[Mika Kahola <mika.kahola@intel.com>]

On Tue, Mar 31, 2015 at 05:45:56PM +0300, Ville Syrjälä wrote:
> On Tue, Mar 31, 2015 at 02:14:23PM +0300, Mika Kahola wrote:
> > Combined Valleyview, Haswell and Broadwell '*_modeset_global_pipes()'
> > into one function 'intel_modeset_global_pipes()'
> > 
> > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 89 +++++++++++++++++-------------------
> >  1 file changed, 41 insertions(+), 48 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 5ed40df..7180d2b 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5209,38 +5209,6 @@ static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv,
> >  		return 200000;
> >  }
> >  
> > -/* compute the max pixel clock for new configuration */
> > -static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv)
> > -{
> > -	struct drm_device *dev = dev_priv->dev;
> > -	struct intel_crtc *intel_crtc;
> > -	int max_pixclk = 0;
> > -
> > -	for_each_intel_crtc(dev, intel_crtc) {
> > -		if (intel_crtc->new_enabled)
> > -			max_pixclk = max(max_pixclk,
> > -					 intel_crtc->new_config->base.adjusted_mode.crtc_clock);
> > -	}
> > -
> > -	return max_pixclk;
> > -}
> > -
> > -static void valleyview_modeset_global_pipes(struct drm_device *dev,
> > -					    unsigned *prepare_pipes)
> > -{
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	struct intel_crtc *intel_crtc;
> > -	int max_pixclk = intel_mode_max_pixclk(dev_priv);
> > -
> > -	if (valleyview_calc_cdclk(dev_priv, max_pixclk) == dev_priv->cdclk_freq)
> > -		return;
> > -
> > -	/* disable/enable all currently active pipes while we change cdclk */
> > -	for_each_intel_crtc(dev, intel_crtc)
> > -		if (intel_crtc->base.state->enable)
> > -			*prepare_pipes |= (1 << intel_crtc->pipe);
> > -}
> > -
> >  static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv)
> >  {
> >  	unsigned int credits, default_credits;
> > @@ -5277,6 +5245,22 @@ static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv)
> >  	WARN_ON(I915_READ(GCI_CONTROL) & PFI_CREDIT_RESEND);
> >  }
> >  
> > +/* compute the max pixel clock for new configuration */
> > +static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv)
> > +{
> > +	struct drm_device *dev = dev_priv->dev;
> > +	struct intel_crtc *intel_crtc;
> > +	int max_pixclk = 0;
> > +
> > +	for_each_intel_crtc(dev, intel_crtc) {
> > +		if (intel_crtc->new_enabled)
> > +			max_pixclk = max(max_pixclk,
> > +					 intel_crtc->new_config->base.adjusted_mode.crtc_clock);
> > +	}
> > +
> > +	return max_pixclk;
> > +}
> > +
> >  static void valleyview_modeset_global_resources(struct drm_atomic_state *state)
> >  {
> >  	struct drm_device *dev = state->dev;
> > @@ -8973,21 +8957,38 @@ static void broadwell_set_cdclk(struct drm_device *dev, int cdclk)
> >  	     cdclk, dev_priv->cdclk_freq);
> >  }
> >  
> > -static void haswell_modeset_global_pipes(struct drm_device *dev,
> > -					 unsigned *prepare_pipes)
> > +static void intel_modeset_global_pipes(struct drm_device *dev,
> > +				       unsigned *prepare_pipes,
> > +				       unsigned *disable_pipes)
> 
> You don't modify disable_pipes, so no need to pass as pointer.
> 
> I do think passing disable_pipes into intel_modeset_global_pipes() does
> make sense however, as it makes it clearer why we need to clear out the
> disable_pipes when the code is all in one place like this.
That's a good point.

> 
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_crtc *crtc;
> > -	int max_pixel_rate = ilk_max_pixel_rate(dev_priv);
> > +	int max_pixclk;
> >  
> > -	if (haswell_calc_cdclk(dev_priv, max_pixel_rate) ==
> > -	    dev_priv->cdclk_freq)
> > +	/* this modeset is valid only for VLV, HSW, and BDW */
> > +	if (!IS_VALLEYVIEW(dev) && !IS_HASWELL(dev) && !IS_BROADWELL(dev))
> >  		return;
> >  
> > +	if (IS_VALLEYVIEW(dev)) {
> > +		max_pixclk = intel_mode_max_pixclk(dev_priv);
> > +		if (valleyview_calc_cdclk(dev_priv, max_pixclk) ==
> > +		    dev_priv->cdclk_freq)
> > +			return;
> > +	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> > +		max_pixclk = ilk_max_pixel_rate(dev_priv);
> > +		if (haswell_calc_cdclk(dev_priv, max_pixclk) ==
> > +		    dev_priv->cdclk_freq)
> > +			return;
> > +
> > +	}
> 
> Maybe move the current vs. newly computed pixclk comparison out of
> the if ladder, so we don't have to duplicate it for each platform?
> It should also get rid of the ugly line length issue we have here.
> 
> Eventually we should get rid of the platform specifics here, but
> that requires that we sort out the pfit pixel rate mess for all
> platforms in the same way. Currently we just ignore the pfit
> scaling ratio for gmch (and gen9 IIRC) platforms.
Indeed, line break issue does look ugly. I throw a revised patch for this.

Thanks for the review!

> 
> > +
> >  	/* disable/enable all currently active pipes while we change cdclk */
> >  	for_each_intel_crtc(dev, crtc)
> > -		if (crtc->base.enabled)
> > -			*prepare_pipes |= 1 << crtc->pipe;
> > +		if (crtc->base.state->enable)
> > +			*prepare_pipes |= (1 << crtc->pipe);
> > +
> > +	/* may have added more to prepare_pipes than we should */
> > +	*prepare_pipes &= ~*disable_pipes;
> >  }
> >  
> >  static void haswell_modeset_global_resources(struct drm_atomic_state *state)
> > @@ -12120,15 +12121,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
> >  	 * mode set on this crtc.  For other crtcs we need to use the
> >  	 * adjusted_mode bits in the crtc directly.
> >  	 */
> > -	if (IS_VALLEYVIEW(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> > -		if (IS_VALLEYVIEW(dev))
> > -			valleyview_modeset_global_pipes(dev, &prepare_pipes);
> > -		else
> > -			haswell_modeset_global_pipes(dev, &prepare_pipes);
> > -
> > -		/* may have added more to prepare_pipes than we should */
> > -		prepare_pipes &= ~disable_pipes;
> > -	}
> > +	intel_modeset_global_pipes(dev, &prepare_pipes, &disable_pipes);
> >  
> >  	ret = __intel_set_mode_setup_plls(dev, modeset_pipes, disable_pipes);
> >  	if (ret)
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Mika Kahola, Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx