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

[Nicolai Hähnle]

On 03.07.2017 22:09, Marek Olšák wrote:
> 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.

Yes.

Cheers,
Nicolai

> 
> Marek
> 


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