On Fri, 11 Jun 2021 at 15:55, Thomas Hellström
wrote:
>
> After a TTM move we need to update the i915 gem flags and caching
> settings to reflect the new placement.
> Also introduce gpu_binds_iomem() and cpu_maps_iomem() to clean up the
> various ways we previously used to detect this.
> Finally, initialize the TTM object reserved to be able to update
> flags and caching before anyone else gets hold of the object.
Hmm, why do we need to update it after a move? Is it not static? i.e
we just consider the mm.placements/region to determine the correct
domain and cache tracking? Or maybe it doesn't really matter either
way?
>
> Signed-off-by: Thomas Hellström
> ---
> v2:
> - Style fixes (Reported by Matthew Auld)
> ---
> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 112 +++-
> 1 file changed, 90 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index 33ab47f1e05b..45ef1d101937 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -70,6 +70,17 @@ static struct ttm_placement i915_sys_placement = {
> .busy_placement = _sys_placement_flags[1],
> };
>
> +static bool gpu_binds_iomem(struct ttm_resource *mem)
> +{
> + return mem->mem_type != TTM_PL_SYSTEM;
> +}
> +
> +static bool cpu_maps_iomem(struct ttm_resource *mem)
> +{
> + /* Once / if we support GGTT, this is also false for cached ttm_tts */
> + return mem->mem_type != TTM_PL_SYSTEM;
> +}
> +
> static void i915_ttm_adjust_lru(struct drm_i915_gem_object *obj);
>
> static struct ttm_tt *i915_ttm_tt_create(struct ttm_buffer_object *bo,
> @@ -175,6 +186,41 @@ static void i915_ttm_free_cached_io_st(struct
> drm_i915_gem_object *obj)
> obj->ttm.cached_io_st = NULL;
> }
>
> +static void
> +i915_ttm_adjust_domains_after_cpu_move(struct drm_i915_gem_object *obj)
> +{
> + struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
> +
> + if (cpu_maps_iomem(bo->resource) || bo->ttm->caching != ttm_cached) {
> + obj->write_domain = I915_GEM_DOMAIN_WC;
> + obj->read_domains = I915_GEM_DOMAIN_WC;
> + } else {
> + obj->write_domain = I915_GEM_DOMAIN_CPU;
> + obj->read_domains = I915_GEM_DOMAIN_CPU;
> + }
> +}
> +
> +static void i915_ttm_adjust_gem_after_move(struct drm_i915_gem_object *obj)
> +{
> + struct drm_i915_private *i915 = to_i915(obj->base.dev);
> + struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
> + unsigned int cache_level;
> +
> + obj->mem_flags &= ~(I915_BO_FLAG_STRUCT_PAGE | I915_BO_FLAG_IOMEM);
> +
> + obj->mem_flags |= cpu_maps_iomem(bo->resource) ? I915_BO_FLAG_IOMEM :
> + I915_BO_FLAG_STRUCT_PAGE;
> +
> + if ((HAS_LLC(i915) || HAS_SNOOP(i915)) &&
> !gpu_binds_iomem(bo->resource) &&
> + bo->ttm->caching == ttm_cached) {
> + cache_level = I915_CACHE_LLC;
> + } else {
> + cache_level = I915_CACHE_NONE;
> + }
Nit: no need for the braces.
> +
> + i915_gem_object_set_cache_coherency(obj, cache_level);
> +}
> +
> static void i915_ttm_purge(struct drm_i915_gem_object *obj)
> {
> struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
> @@ -190,8 +236,10 @@ static void i915_ttm_purge(struct drm_i915_gem_object
> *obj)
>
> /* TTM's purge interface. Note that we might be reentering. */
> ret = ttm_bo_validate(bo, , );
> -
> if (!ret) {
> + obj->write_domain = 0;
> + obj->read_domains = 0;
> + i915_ttm_adjust_gem_after_move(obj);
> i915_ttm_free_cached_io_st(obj);
> obj->mm.madv = __I915_MADV_PURGED;
> }
> @@ -273,12 +321,15 @@ i915_ttm_resource_get_st(struct drm_i915_gem_object
> *obj,
> struct ttm_resource *res)
> {
> struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
> - struct ttm_resource_manager *man =
> - ttm_manager_type(bo->bdev, res->mem_type);
>
> - if (man->use_tt)
> + if (!gpu_binds_iomem(res))
> return i915_ttm_tt_get_st(bo->ttm);
>
> + /*
> +* If CPU mapping differs, we need to add the ttm_tt pages to
> +* the resulting st. Might make sense for GGTT.
> +*/
> + GEM_WARN_ON(!cpu_maps_iomem(res));
> return intel_region_ttm_node_to_st(obj->mm.region, res);
> }
>
> @@ -290,8 +341,6 @@ static int i915_ttm_move(struct ttm_buffer_object *bo,
> bool evict,
> struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
> struct ttm_resource_manager *dst_man =
> ttm_manager_type(bo->bdev, dst_mem->mem_type);
> - struct ttm_resource_manager *src_man =
> - ttm_manager_type(bo->bdev, bo->resource->mem_type);
> struct intel_memory_region *dst_reg, *src_reg;
> union {
>
