[Mesa-dev] [PATCH 08/14] gallium/radeon: clean up (domain, flags) <-> (slab heap) translations

[Marek Olšák]

On Mon, Jul 3, 2017 at 9:47 PM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
> On 29.06.2017 21:47, Marek Olšák wrote:
>>
>> From: Marek Olšák <marek.olsak at amd.com>
>>
>> This is cleaner, and we are down to 4 slabs.
>> ---
>>   src/gallium/drivers/radeon/radeon_winsys.h        | 62
>> +++++++++++++++++++++++
>>   src/gallium/winsys/amdgpu/drm/amdgpu_bo.c         | 44 +++-------------
>>   src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c     |  2 +-
>>   src/gallium/winsys/radeon/drm/radeon_drm_bo.c     | 44 +++-------------
>>   src/gallium/winsys/radeon/drm/radeon_drm_winsys.c |  2 +-
>>   5 files changed, 80 insertions(+), 74 deletions(-)
>>
>> diff --git a/src/gallium/drivers/radeon/radeon_winsys.h
>> b/src/gallium/drivers/radeon/radeon_winsys.h
>> index 1be94f7..95543bb 100644
>> --- a/src/gallium/drivers/radeon/radeon_winsys.h
>> +++ b/src/gallium/drivers/radeon/radeon_winsys.h
>> @@ -651,11 +651,73 @@ static inline void radeon_emit(struct
>> radeon_winsys_cs *cs, uint32_t value)
>>       cs->current.buf[cs->current.cdw++] = value;
>>   }
>>     static inline void radeon_emit_array(struct radeon_winsys_cs *cs,
>>                                      const uint32_t *values, unsigned
>> count)
>>   {
>>       memcpy(cs->current.buf + cs->current.cdw, values, count * 4);
>>       cs->current.cdw += count;
>>   }
>>   +enum radeon_heap {
>> +    RADEON_HEAP_VRAM,
>> +    RADEON_HEAP_VRAM_GTT, /* combined heaps */
>> +    RADEON_HEAP_GTT_WC,
>> +    RADEON_HEAP_GTT,
>> +    RADEON_MAX_SLAB_HEAPS,
>> +};
>> +
>> +static inline enum radeon_bo_domain radeon_domain_from_heap(enum
>> radeon_heap heap)
>> +{
>> +    switch (heap) {
>> +    case RADEON_HEAP_VRAM:
>> +        return RADEON_DOMAIN_VRAM;
>> +    case RADEON_HEAP_VRAM_GTT:
>> +        return RADEON_DOMAIN_VRAM_GTT;
>> +    case RADEON_HEAP_GTT_WC:
>> +    case RADEON_HEAP_GTT:
>> +        return RADEON_DOMAIN_GTT;
>> +    default:
>> +        assert(0);
>> +        return 0;
>> +    }
>> +}
>> +
>> +static inline unsigned radeon_flags_from_heap(enum radeon_heap heap)
>> +{
>> +    switch (heap) {
>> +    case RADEON_HEAP_VRAM:
>> +    case RADEON_HEAP_VRAM_GTT:
>> +    case RADEON_HEAP_GTT_WC:
>> +        return RADEON_FLAG_GTT_WC;
>> +    case RADEON_HEAP_GTT:
>> +    default:
>> +        return 0;
>> +    }
>> +}
>> +
>> +/* Return the heap index for winsys allocators, or -1 on failure. */
>> +static inline int radeon_get_heap_index(enum radeon_bo_domain domain,
>> +                                        enum radeon_bo_flag flags)
>> +{
>> +    /* VRAM implies WC (write combining) */
>> +    assert(!(domain & RADEON_DOMAIN_VRAM) || flags & RADEON_FLAG_GTT_WC);
>> +
>> +    /* Unsupported flags: NO_CPU_ACCESS, NO_SUBALLOC, SPARSE. */
>> +    if (flags & ~RADEON_FLAG_GTT_WC)
>> +        return -1;
>> +
>> +    switch (domain) {
>> +    case RADEON_DOMAIN_VRAM:
>> +        return RADEON_HEAP_VRAM;
>> +    case RADEON_DOMAIN_VRAM_GTT:
>> +        return RADEON_HEAP_VRAM_GTT;
>> +    case RADEON_DOMAIN_GTT:
>> +        if (flags & RADEON_FLAG_GTT_WC)
>> +            return RADEON_HEAP_GTT_WC;
>> +        else
>> +            return RADEON_HEAP_GTT;
>> +    }
>> +    return -1;
>> +}
>> +
>>   #endif
>> diff --git a/src/gallium/winsys/amdgpu/drm/amdgpu_bo.c
>> b/src/gallium/winsys/amdgpu/drm/amdgpu_bo.c
>> index 9736f44a..5ebe59f 100644
>> --- a/src/gallium/winsys/amdgpu/drm/amdgpu_bo.c
>> +++ b/src/gallium/winsys/amdgpu/drm/amdgpu_bo.c
>> @@ -488,43 +488,27 @@ static const struct pb_vtbl
>> amdgpu_winsys_bo_slab_vtbl = {
>>      amdgpu_bo_slab_destroy
>>      /* other functions are never called */
>>   };
>>     struct pb_slab *amdgpu_bo_slab_alloc(void *priv, unsigned heap,
>>                                        unsigned entry_size,
>>                                        unsigned group_index)
>>   {
>>      struct amdgpu_winsys *ws = priv;
>>      struct amdgpu_slab *slab = CALLOC_STRUCT(amdgpu_slab);
>> -   enum radeon_bo_domain domains;
>> -   enum radeon_bo_flag flags = 0;
>> +   enum radeon_bo_domain domains = radeon_domain_from_heap(heap);
>> +   enum radeon_bo_flag flags = radeon_flags_from_heap(heap);
>>      uint32_t base_id;
>>        if (!slab)
>>         return NULL;
>>   -   if (heap & 1)
>> -      flags |= RADEON_FLAG_GTT_WC;
>> -
>> -   switch (heap >> 2) {
>> -   case 0:
>> -      domains = RADEON_DOMAIN_VRAM;
>> -      break;
>> -   default:
>> -   case 1:
>> -      domains = RADEON_DOMAIN_VRAM_GTT;
>> -      break;
>> -   case 2:
>> -      domains = RADEON_DOMAIN_GTT;
>> -      break;
>> -   }
>> -
>>      slab->buffer = amdgpu_winsys_bo(amdgpu_bo_create(&ws->base,
>>                                                       64 * 1024, 64 *
>> 1024,
>>                                                       domains, flags));
>>      if (!slab->buffer)
>>         goto fail;
>>        assert(slab->buffer->bo);
>>        slab->base.num_entries = slab->buffer->base.size / entry_size;
>>      slab->base.num_free = slab->base.num_entries;
>> @@ -1144,45 +1128,33 @@ static struct pb_buffer *
>>   amdgpu_bo_create(struct radeon_winsys *rws,
>>                    uint64_t size,
>>                    unsigned alignment,
>>                    enum radeon_bo_domain domain,
>>                    enum radeon_bo_flag flags)
>>   {
>>      struct amdgpu_winsys *ws = amdgpu_winsys(rws);
>>      struct amdgpu_winsys_bo *bo;
>>      unsigned usage = 0, pb_cache_bucket;
>>   +   /* VRAM implies WC. This is not optional. */
>> +   if (domain & RADEON_DOMAIN_VRAM)
>> +      flags |= RADEON_FLAG_GTT_WC;
>
>
> I'd prefer this as an assert. Otherwise callers might be confused about
> being able to leave the flag clear.
>
> With that changed, patches 1-8 are
>
> Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>

I guess you mean amdgpu only. I can't add the assertion for radeon
because then I'd have to test r300.

Marek