Re: [PATCH 08/16] drm/i915: remove unused restore_gtt_mappings optimization during suspend

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Sep 10, 2014 at 06:17:01PM +0300, Imre Deak wrote:
> The logic to skip restoring GTT mappings was added to speed up
> suspend/resume, but not on old GENs where not restoring them caused
> problems. The check for old GENs is based on the existance of OpRegion,
> but this doesn't work since opregion is initialized only after
> the check. So we end up always restoring the mappings.
> 
> On my BYT - which has OpRegion - skipping restoring the mappings during
> suspend doesn't work, I get a GPU hang after resume. Also the logic of
> when to allow the optimization during S4 is reversed: we should allow it
> during S4 thaw but not during S4 restore, but atm we have it the other
> way around in the code.
> 
> Since correctness wins over optimal code and since the optimization
> wasn't used anyway I decided not to try to fix it at this point, but
> just remove it. This allows us to unify the S3 and S4 handlers in the
> following patches.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Adding Jesse. Also he claimed that it actually helped back in

commit 1abd02e2dd7e0bd577000301fb2fd47780637387
Author: Jesse Barnes <jbarnes@virtuousgeek.org>
Date:   Fri Nov 2 11:14:02 2012 -0700

    drm/i915: don't rewrite the GTT on resume v4

dunno where exactly this broke.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index a3addc2..5e25283 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -667,12 +667,11 @@ static int i915_drm_thaw_early(struct drm_device *dev)
>  	return ret;
>  }
>  
> -static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
> +static int __i915_drm_thaw(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	if (drm_core_check_feature(dev, DRIVER_MODESET) &&
> -	    restore_gtt_mappings) {
> +	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		mutex_lock(&dev->struct_mutex);
>  		i915_gem_restore_gtt_mappings(dev);
>  		mutex_unlock(&dev->struct_mutex);
> @@ -740,7 +739,7 @@ static int i915_drm_thaw(struct drm_device *dev)
>  	if (drm_core_check_feature(dev, DRIVER_MODESET))
>  		i915_check_and_clear_faults(dev);
>  
> -	return __i915_drm_thaw(dev, true);
> +	return __i915_drm_thaw(dev);
>  }
>  
>  static int i915_resume_early(struct drm_device *dev)
> @@ -767,15 +766,9 @@ static int i915_resume_early(struct drm_device *dev)
>  
>  static int i915_drm_resume(struct drm_device *dev)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret;
>  
> -	/*
> -	 * Platforms with opregion should have sane BIOS, older ones (gen3 and
> -	 * earlier) need to restore the GTT mappings since the BIOS might clear
> -	 * all our scratch PTEs.
> -	 */
> -	ret = __i915_drm_thaw(dev, !dev_priv->opregion.header);
> +	ret = __i915_drm_thaw(dev);
>  	if (ret)
>  		return ret;
>  
> -- 
> 1.8.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch