[PATCH 1/2] drm/amdgpu: guarantee bijective mapping of ring ids for LRU

[Nicolai Hähnle]

On 28.03.2017 00:36, Andres Rodriguez wrote:
> Depending on usage patterns, the current LRU policy may create a
> non-injective mapping between userspace ring ids and kernel rings.
>
> This behaviour is undesired as apps that attempt to fill all HW blocks
> would be unable to reach some of them.
>
> This change forces the LRU policy to create bijective mappings only.
>
> Signed-off-by: Andres Rodriguez <andresx7 at gmail.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_queue_mgr.c | 16 +++++++++++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c      | 33 +++++++++++++++++++++------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h      |  4 ++--
>  3 files changed, 42 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_queue_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_queue_mgr.c
> index 054d750..c6275b4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_queue_mgr.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_queue_mgr.c
> @@ -108,24 +108,36 @@ static enum amdgpu_ring_type amdgpu_hw_ip_to_ring_type(int hw_ip)
>  		DRM_ERROR("Invalid HW IP specified %d\n", hw_ip);
>  		return -1;
>  	}
>  }
>
>  static int amdgpu_lru_map(struct amdgpu_device *adev,
>  			  struct amdgpu_queue_mapper *mapper,
>  			  int user_ring,
>  			  struct amdgpu_ring **out_ring)
>  {
> -	int r;
> +	int r, i, j;
>  	int ring_type = amdgpu_hw_ip_to_ring_type(mapper->hw_ip);
> +	int ring_blacklist[AMDGPU_MAX_RINGS];
> +	struct amdgpu_ring *ring;
>
> -	r = amdgpu_ring_lru_get(adev, ring_type, out_ring);
> +	/* 0 is a valid ring index, so initialize to -1 */
> +	memset(ring_blacklist, 0xff, sizeof(ring_blacklist));

Unless I'm mistaken, you don't need this if ...


> +
> +	for (i = 0, j = 0; i < AMDGPU_MAX_RINGS; i++) {
> +		ring = mapper->queue_map[i];
> +		if (ring)
> +			ring_blacklist[j++] = ring->idx;
> +	}
> +
> +	r = amdgpu_ring_lru_get(adev, ring_type, ring_blacklist,
> +				AMDGPU_MAX_RINGS, out_ring);

... you s/AMDGPU_MAX_RINGS/j/ here.


>  	if (r)
>  		return r;
>
>  	return amdgpu_update_cached_map(mapper, user_ring, *out_ring);
>  }
>
>  /**
>   * amdgpu_queue_mgr_init - init an amdgpu_queue_mgr struct
>   *
>   * @adev: amdgpu_device pointer
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> index ca41b3a..22c5b28 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> @@ -393,46 +393,65 @@ void amdgpu_ring_fini(struct amdgpu_ring *ring)
>  	ring->adev->rings[ring->idx] = NULL;
>  }
>
>  static void amdgpu_ring_lru_touch_locked(struct amdgpu_device *adev,
>  					 struct amdgpu_ring *ring)
>  {
>  	/* list_move_tail handles the case where ring isn't part of the list */
>  	list_move_tail(&ring->lru_list, &adev->ring_lru_list);
>  }
>
> +static bool amdgpu_ring_is_blacklisted(struct amdgpu_ring *ring,
> +				       int *blacklist, int num_blacklist)
> +{
> +	int i;
> +
> +	for (i = 0; i < num_blacklist && blacklist[i] != -1; i++) {

... and then you can also drop the blacklist[i] != -1 here.

With those changes:

Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>


> +		if (ring->idx == blacklist[i])
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  /**
>   * amdgpu_ring_lru_get - get the least recently used ring for a HW IP block
>   *
>   * @adev: amdgpu_device pointer
>   * @type: amdgpu_ring_type enum
> + * @blacklist: blacklisted ring ids array
> + * @num_blacklist: number of entries in @blacklist
>   * @ring: output ring
>   *
>   * Retrieve the amdgpu_ring structure for the least recently used ring of
>   * a specific IP block (all asics).
>   * Returns 0 on success, error on failure.
>   */
> -int amdgpu_ring_lru_get(struct amdgpu_device *adev, int type,
> -			struct amdgpu_ring **ring)
> +int amdgpu_ring_lru_get(struct amdgpu_device *adev, int type, int *blacklist,
> +			int num_blacklist, struct amdgpu_ring **ring)
>  {
>  	struct amdgpu_ring *entry;
>
>  	/* List is sorted in LRU order, find first entry corresponding
>  	 * to the desired HW IP */
>  	*ring = NULL;
>  	spin_lock(&adev->ring_lru_list_lock);
>  	list_for_each_entry(entry, &adev->ring_lru_list, lru_list) {
> -		if (entry->funcs->type == type) {
> -			*ring = entry;
> -			amdgpu_ring_lru_touch_locked(adev, *ring);
> -			break;
> -		}
> +		if (entry->funcs->type != type)
> +			continue;
> +
> +		if (amdgpu_ring_is_blacklisted(entry, blacklist, num_blacklist))
> +			continue;
> +
> +		*ring = entry;
> +		amdgpu_ring_lru_touch_locked(adev, *ring);
> +		break;
>  	}
>  	spin_unlock(&adev->ring_lru_list_lock);
>
>  	if (!*ring) {
>  		DRM_ERROR("Ring LRU contains no entries for ring type:%d\n", type);
>  		return -EINVAL;
>  	}
>
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> index 2c801a5..811d76a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> @@ -195,15 +195,15 @@ void amdgpu_ring_generic_pad_ib(struct amdgpu_ring *ring, struct amdgpu_ib *ib);
>  void amdgpu_ring_commit(struct amdgpu_ring *ring);
>  void amdgpu_ring_undo(struct amdgpu_ring *ring);
>  int amdgpu_ring_priority_get(struct amdgpu_ring *ring,
>  			     enum amd_sched_priority priority);
>  void amdgpu_ring_priority_put(struct amdgpu_ring *ring,
>  			      enum amd_sched_priority priority);
>  int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
>  		     unsigned ring_size, struct amdgpu_irq_src *irq_src,
>  		     unsigned irq_type);
>  void amdgpu_ring_fini(struct amdgpu_ring *ring);
> -int amdgpu_ring_lru_get(struct amdgpu_device *adev, int hw_ip,
> -			struct amdgpu_ring **ring);
> +int amdgpu_ring_lru_get(struct amdgpu_device *adev, int type, int *blacklist,
> +			int num_blacklist, struct amdgpu_ring **ring);
>  void amdgpu_ring_lru_touch(struct amdgpu_device *adev, struct amdgpu_ring *ring);
>
>  #endif
>


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.