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 <[email protected]>
> > ---
> > 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
> > [email protected]
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
--
Mika Kahola, Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
