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

[Jani Nikula <jani.nikula@intel.com>]

On Wed, 15 Mar 2017, 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>
> Link: http://patchwork.freedesktop.org/patch/msgid/20170220140443.30891-1-ville.syrjala@linux.intel.com
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks, pushed to drm-intel-fixes.

BR,
Jani.


> ---
>  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 01341670738f..70be773b3e70 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -16696,12 +16696,11 @@ int intel_modeset_init(struct drm_device *dev)
>  		}
>  	}
>  
> -	intel_update_czclk(dev_priv);
> -	intel_update_cdclk(dev_priv);
> -	dev_priv->atomic_cdclk_freq = dev_priv->cdclk_freq;
> -
>  	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);
>  
> @@ -17258,8 +17257,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);
>  }

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx