Re: [PATCH 32/55] drm/i915: Split early global GTT initialisation

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On ma, 2016-07-25 at 18:32 +0100, Chris Wilson wrote:
> -/**
> - * i915_gem_init_ggtt - Initialize GEM for Global GTT
> - * @dev: DRM device
> - */
> -void i915_gem_init_ggtt(struct drm_device *dev)
> 
> +static void init_global_gtt(struct drm_i915_private *dev_priv)

Why not s/global_gtt/ggtt/ for easier grepping.

>  {
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> +	struct i915_address_space *ggtt = &dev_priv->ggtt.base;

Nope, I did quite a job making sure every ggtt variable is of
i915_ggtt. And we agreed (a compromise :P) on ggtt->base for the
address space

> +
> +	INIT_LIST_HEAD(&dev_priv->vm_list);
>  
> -	i915_gem_setup_global_gtt(dev, 0, ggtt->mappable_end, ggtt->base.total);
> +	/* Subtract the guard page before address space initialization to
> +	 * shrink the range used by drm_mm.
> +	 */
> +	ggtt->total -= PAGE_SIZE;
> +	i915_address_space_init(ggtt, dev_priv);
> +	ggtt->total += PAGE_SIZE;
> +
> +	if (!HAS_LLC(dev_priv))
> +		ggtt->mm.color_adjust = i915_gtt_color_adjust;
>  }
>  
>  /**
> @@ -2890,6 +2862,9 @@ void i915_ggtt_cleanup_hw(struct drm_device *dev)
>  	}
>  
>  	ggtt->base.cleanup(&ggtt->base);
> +
> +	arch_phys_wc_del(ggtt->mtrr);
> +	io_mapping_free(ggtt->mappable);
>  }
>  
>  static unsigned int gen6_get_total_gtt_size(u16 snb_gmch_ctl)
> @@ -3250,21 +3225,14 @@ int i915_ggtt_init_hw(struct drm_device *dev)
>  	if (ret)
>  		return ret;
>  
> -	if ((ggtt->base.total - 1) >> 32) {
> -		DRM_ERROR("We never expected a Global GTT with more than 32bits"
> -			  "of address space! Found %lldM!\n",
> -			  ggtt->base.total >> 20);
> -		ggtt->base.total = 1ULL << 32;
> -		ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total);
> -	}
> +	init_global_gtt(dev_priv);
>  
> -	/*
> -	 * Initialise stolen early so that we may reserve preallocated
> -	 * objects for the BIOS to KMS transition.
> -	 */
> -	ret = i915_gem_init_stolen(dev);
> -	if (ret)
> -		goto out_gtt_cleanup;
> +	ggtt->mappable =
> +		io_mapping_create_wc(ggtt->mappable_base, ggtt->mappable_end);

Splitting between arguments might look leaner.

Otherwise looks OK, plenty of code motion so not super easy to track.
With above addressed;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx