Re: [PATCH 08/15] drm/i915: clean up cache coloring

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Matthew Auld <matthew.auld@intel.com> writes:

> To keep the next patch simple, rid the code of any mm.color_adjust
> assumptions to allow adding another flavour of coloring.
>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |  2 +-
>  drivers/gpu/drm/i915/i915_gem.c       |  3 ++-
>  drivers/gpu/drm/i915/i915_gem_evict.c | 12 +++++-------
>  drivers/gpu/drm/i915/i915_gem_gtt.h   |  6 ++++++
>  drivers/gpu/drm/i915/i915_vma.c       | 10 +++++++---
>  5 files changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e45b8d74cebf..aac764b5aad4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3526,7 +3526,7 @@ int i915_perf_open_ioctl(struct drm_device *dev, void *data,
>  /* i915_gem_evict.c */
>  int __must_check i915_gem_evict_something(struct i915_address_space *vm,
>  					  u64 min_size, u64 alignment,
> -					  unsigned cache_level,
> +					  unsigned long color,
>  					  u64 start, u64 end,
>  					  unsigned flags);
>  int __must_check i915_gem_evict_for_node(struct i915_address_space *vm,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0a6ed2c54629..9acf279e5f93 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3392,7 +3392,8 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>  		obj->cache_dirty = true;
>  
>  	list_for_each_entry(vma, &obj->vma_list, obj_link)
> -		vma->node.color = cache_level;
> +		if (i915_uses_cache_coloring(vma->vm))
> +			vma->node.color = cache_level;
>  	obj->cache_level = cache_level;
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> index 2da3a94fc9f3..f9364f917b67 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> @@ -74,7 +74,7 @@ mark_free(struct drm_mm_scan *scan,
>   * @vm: address space to evict from
>   * @min_size: size of the desired free space
>   * @alignment: alignment constraint of the desired free space
> - * @cache_level: cache_level for the desired space
> + * @color: color for the desired space
>   * @start: start (inclusive) of the range from which to evict objects
>   * @end: end (exclusive) of the range from which to evict objects
>   * @flags: additional flags to control the eviction algorithm
> @@ -95,7 +95,7 @@ mark_free(struct drm_mm_scan *scan,
>  int
>  i915_gem_evict_something(struct i915_address_space *vm,
>  			 u64 min_size, u64 alignment,
> -			 unsigned cache_level,
> +			 unsigned long color,
>  			 u64 start, u64 end,
>  			 unsigned flags)
>  {
> @@ -134,7 +134,7 @@ i915_gem_evict_something(struct i915_address_space *vm,
>  	if (flags & PIN_MAPPABLE)
>  		mode = DRM_MM_INSERT_LOW;
>  	drm_mm_scan_init_with_range(&scan, &vm->mm,
> -				    min_size, alignment, cache_level,
> +				    min_size, alignment, color,
>  				    start, end, mode);
>  
>  	/* Retire before we search the active list. Although we have
> @@ -254,7 +254,6 @@ int i915_gem_evict_for_node(struct i915_address_space *vm,
>  	u64 start = target->start;
>  	u64 end = start + target->size;
>  	struct i915_vma *vma, *next;
> -	bool check_color;
>  	int ret = 0;
>  
>  	lockdep_assert_held(&vm->i915->drm.struct_mutex);
> @@ -271,8 +270,7 @@ int i915_gem_evict_for_node(struct i915_address_space *vm,
>  	if (!(flags & PIN_NONBLOCK))
>  		i915_gem_retire_requests(vm->i915);
>  
> -	check_color = vm->mm.color_adjust;
> -	if (check_color) {
> +	if (i915_uses_cache_coloring(vm)) {
>  		/* Expand search to cover neighbouring guard pages (or lack!) */
>  		if (start)
>  			start -= I915_GTT_PAGE_SIZE;
> @@ -298,7 +296,7 @@ int i915_gem_evict_for_node(struct i915_address_space *vm,
>  		 * abutt and conflict. If they are in conflict, then we evict
>  		 * those as well to make room for our guard pages.
>  		 */
> -		if (check_color) {
> +		if (i915_uses_cache_coloring(vm)) {
>  			if (node->start + node->size == target->start) {
>  				if (node->color == target->color)
>  					continue;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 1f51402cf816..8d7436105718 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -346,6 +346,12 @@ struct i915_address_space {
>  #define i915_is_ggtt(V) (!(V)->file)
>  
>  static inline bool
> +i915_uses_cache_coloring(const struct i915_address_space *vm)

i915_vm_uses_color()
i915_vm_has_coloring()

Just trying to think of better naming as this is
vm property.

-Mika

> +{
> +	return vm->mm.color_adjust && i915_is_ggtt(vm);
> +}
> +
> +static inline bool
>  i915_vm_is_48bit(const struct i915_address_space *vm)
>  {
>  	return (vm->total - 1) >> 32;
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 1aba47024656..31e2327492ba 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -390,7 +390,7 @@ bool i915_gem_valid_gtt_space(struct i915_vma *vma, unsigned long cache_level)
>  	 * these constraints apply and set the drm_mm.color_adjust
>  	 * appropriately.
>  	 */
> -	if (vma->vm->mm.color_adjust == NULL)
> +	if (!i915_uses_cache_coloring(vma->vm))
>  		return true;
>  
>  	/* Only valid to be called on an already inserted vma */
> @@ -429,6 +429,7 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
>  	struct drm_i915_gem_object *obj = vma->obj;
>  	u64 start, end;
>  	int ret;
> +	unsigned long color = 0;
>  
>  	GEM_BUG_ON(vma->flags & (I915_VMA_GLOBAL_BIND | I915_VMA_LOCAL_BIND));
>  	GEM_BUG_ON(drm_mm_node_allocated(&vma->node));
> @@ -471,6 +472,9 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
>  	if (ret)
>  		return ret;
>  
> +	if (i915_uses_cache_coloring(vma->vm))
> +		color = obj->cache_level;
> +
>  	if (flags & PIN_OFFSET_FIXED) {
>  		u64 offset = flags & PIN_OFFSET_MASK;
>  		if (!IS_ALIGNED(offset, alignment) ||
> @@ -480,13 +484,13 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
>  		}
>  
>  		ret = i915_gem_gtt_reserve(vma->vm, &vma->node,
> -					   size, offset, obj->cache_level,
> +					   size, offset, color,
>  					   flags);
>  		if (ret)
>  			goto err_unpin;
>  	} else {
>  		ret = i915_gem_gtt_insert(vma->vm, &vma->node,
> -					  size, alignment, obj->cache_level,
> +					  size, alignment, color,
>  					  start, end, flags);
>  		if (ret)
>  			goto err_unpin;
> -- 
> 2.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx