On 2017-03-23 08:02 AM, Nicolai Hähnle wrote:
> On 17.03.2017 19:52, 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 | 15 ++++++++++--
>> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 33
>> +++++++++++++++++++++------
>> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 4 ++--
>> 3 files changed, 41 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..2cffb0e 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_queue_mgr.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_queue_mgr.c
>> @@ -108,24 +108,35 @@ 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;
>> int ring_type = amdgpu_hw_ip_to_ring_type(mapper->hw_ip);
>> + int ring_blacklist[AMDGPU_MAX_RINGS];
>> + struct amdgpu_ring *ring;
>> +
>> + for (i = 0; i < AMDGPU_MAX_RINGS; i++) {
>> + ring = mapper->queue_map[i];
>> + if (!ring)
>> + ring_blacklist[i] = -1;
>> + else
>> + ring_blacklist[i] = ring->idx;
>> + }
>
> Given how ring_blacklist is used, I'd suggest to "compress" its
> entries instead of introducing -1 gaps.
>
> The rest of the patch looks good to me.
>
> Cheers,
> Nicolai
>
Can do.
>
>>
>> - r = amdgpu_ring_lru_get(adev, ring_type, out_ring);
>> + r = amdgpu_ring_lru_get(adev, ring_type, ring_blacklist,
>> + AMDGPU_MAX_RINGS, out_ring);
>> 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..0db07b0 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; i++) {
>> + 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
>>
>
>
