Re: [PATCH] drm/i915: Do .init_clock_gating() earlier to avoid it clobbering watermarks

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

On Mon, Feb 20, 2017 at 04:04:43PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> 
> Currently ILK-BDW explicitly disable LP1+ watermarks from their
> .init_clock_gating() hooks. Unfortunately that hook gets called way too
> late since by that time we've already initialized all the watermark
> state tracking which then gets out of sync with the hardware state.
> 
> We may eventually want to consider killing off the explicit LP1+
> disable from .init_clock_gating(). In the meantime however, we can
> avoid the problem by reordering the init sequence such that
> intel_modeset_init_hw()->intel_init_clock_gating() gets called
> prior to the hardware state takeover.
> 
> I suppose prior to the two stage watermark programming we were
> magically saved by something that forced the watermarks to be
> reprogrammed fully after .init_clock_gating() got called. But
> now that no longer happens.
> 
> Note that the diff might look a bit odd as it kills off one
> call of intel_update_cdclk(), but that's fine because
> intel_modeset_init_hw() does the exact same thing. Previously
> we just did it twice.
> 
> Actually even this new init sequence is pretty bogus as
> .init_clock_gating() really should be called before any gem
> hardware init since it can  configure various clock gating
> workarounds and whatnot that affect the GT side as well. Also
> intel_modeset_init() really should get split up into better
> defined init stages. Another "fun" detail is that
> intel_modeset_gem_init() is where RPS/RC6 gets configured.
> Why that is done from the display code is beyond me. I've
> decided to leave all this be for now, and just try to fix
> the init sequence enough for watermarks to work.
> 
> Cc: stable@vger.kernel.org
> Cc: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> Cc: David Purton <dcpurton@marshwiggle.net>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reported-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> Reported-by: David Purton <dcpurton@marshwiggle.net>
> Tested-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96645
> Fixes: ed4a6a7ca853 ("drm/i915: Add two-stage ILK-style watermark programming (v11)")
> Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>

Pushed to dinq with Daniel's irc r-b. Thanks.

19:22 < vsyrjala> anyone care to review https://patchwork.freedesktop.org/patch/139975/ ? would be one less bug to 
                  worry about...
19:28 < danvet> vsyrjala, r-b

> ---
>  drivers/gpu/drm/i915/intel_display.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 730aee755c80..0466db16f193 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15028,12 +15028,11 @@ int intel_modeset_init(struct drm_device *dev)
>  		}
>  	}
>  
> -	intel_update_czclk(dev_priv);
> -	intel_update_cdclk(dev_priv);
> -	dev_priv->cdclk.logical = dev_priv->cdclk.actual = dev_priv->cdclk.hw;
> -
>  	intel_shared_dpll_init(dev);
>  
> +	intel_update_czclk(dev_priv);
> +	intel_modeset_init_hw(dev);
> +
>  	if (dev_priv->max_cdclk_freq == 0)
>  		intel_update_max_cdclk(dev_priv);
>  
> @@ -15591,8 +15590,6 @@ void intel_modeset_gem_init(struct drm_device *dev)
>  
>  	intel_init_gt_powersave(dev_priv);
>  
> -	intel_modeset_init_hw(dev);
> -
>  	intel_setup_overlay(dev_priv);
>  }
>  
> -- 
> 2.10.2

-- 
Ville Syrj�l�
Intel OTC