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

Joonas Lahtinen
2016-07-26T07:08:35Z

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