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

[Andres Rodriguez <andresx7-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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@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
>>
>
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx