[Mesa-dev] [RFC PATCH 52/65] radeonsi: implement ARB_bindless_texture
Samuel Pitoiset
samuel.pitoiset at gmail.com
Thu May 25 20:49:12 UTC 2017
On 05/25/2017 10:45 PM, Marek Olšák wrote:
> On Thu, May 25, 2017 at 9:56 PM, Samuel Pitoiset
> <samuel.pitoiset at gmail.com> wrote:
>>
>>
>> On 05/25/2017 07:58 PM, Marek Olšák wrote:
>>>
>>> Hi,
>>>
>>> 1) Patches 48 and 52 are missing code comments. I'd like to see code
>>> comments about how things work and why it was designed like that.
>>
>>
>> Okay, I will add some.
>>
>>>
>>> 2) There is a lot of code duplication regarding managing the resizable
>>> arrays. I'd like to see some util module used here, e.g. u_vector or
>>> you can add new macros for the array structure.
>>
>>
>> That's right. There is a dynarray stuf also, I will have a look.
>>
>>
>>>
>>> See also below.
>>>
>>> On Fri, May 19, 2017 at 6:52 PM, Samuel Pitoiset
>>> <samuel.pitoiset at gmail.com> wrote:
>>>>
>>>> This implements the Gallium interface. Decompression of resident
>>>> textures/images will follow in the next patches.
>>>>
>>>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>>>> ---
>>>> src/gallium/drivers/radeonsi/si_descriptors.c | 340
>>>> ++++++++++++++++++++++++++
>>>> src/gallium/drivers/radeonsi/si_pipe.c | 12 +
>>>> src/gallium/drivers/radeonsi/si_pipe.h | 26 ++
>>>> 3 files changed, 378 insertions(+)
>>>>
>>>> diff --git a/src/gallium/drivers/radeonsi/si_descriptors.c
>>>> b/src/gallium/drivers/radeonsi/si_descriptors.c
>>>> index abe39de583..a687506f7f 100644
>>>> --- a/src/gallium/drivers/radeonsi/si_descriptors.c
>>>> +++ b/src/gallium/drivers/radeonsi/si_descriptors.c
>>>> @@ -60,6 +60,7 @@
>>>> #include "sid.h"
>>>> #include "gfx9d.h"
>>>>
>>>> +#include "util/hash_table.h"
>>>> #include "util/u_format.h"
>>>> #include "util/u_memory.h"
>>>> #include "util/u_upload_mgr.h"
>>>> @@ -2193,6 +2194,339 @@ void si_resident_descriptor_slab_free(void *priv,
>>>> struct pb_slab *pslab)
>>>> FREE(slab);
>>>> }
>>>>
>>>> +static int si_add_resident_tex_handle(struct si_context *sctx,
>>>> + struct si_texture_handle
>>>> *tex_handle)
>>>> +{
>>>> + int idx;
>>>> +
>>>> + /* New resident handle, check if the backing array is large
>>>> enough. */
>>>> + if (sctx->num_resident_tex_handles >=
>>>> sctx->max_resident_tex_handles) {
>>>> + unsigned new_max_handles =
>>>> + MAX2(1, sctx->max_resident_tex_handles * 2);
>>>> + struct si_texture_handle **new_handles =
>>>> + REALLOC(sctx->resident_tex_handles,
>>>> + sctx->num_resident_tex_handles *
>>>> (sizeof(*new_handles)),
>>>> + new_max_handles * sizeof(*new_handles));
>>>> +
>>>> + if (new_handles) {
>>>> + sctx->resident_tex_handles = new_handles;
>>>> + sctx->max_resident_tex_handles = new_max_handles;
>>>> + } else {
>>>> + fprintf(stderr, "si_add_resident_tex_handle: "
>>>> + "allocation failed\n");
>>>> + return -1;
>>>> + }
>>>> + }
>>>> +
>>>> + idx = sctx->num_resident_tex_handles;
>>>> + sctx->resident_tex_handles[idx] = tex_handle;
>>>> + sctx->num_resident_tex_handles++;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void si_del_resident_tex_handle(struct si_context *sctx,
>>>> + struct si_texture_handle
>>>> *tex_handle)
>>>> +{
>>>> + unsigned i;
>>>> + int size;
>>>> +
>>>> + for (i = 0; i < sctx->num_resident_tex_handles; i++) {
>>>> + if (sctx->resident_tex_handles[i] != tex_handle)
>>>> + continue;
>>>> +
>>>> + if (i < sctx->num_resident_tex_handles - 1) {
>>>> + size = sizeof(*sctx->resident_tex_handles) *
>>>> + (sctx->num_resident_tex_handles - 1 - i);
>>>> +
>>>> + memmove(&sctx->resident_tex_handles[i],
>>>> + &sctx->resident_tex_handles[i + 1],
>>>> size);
>>>> + }
>>>> +
>>>> + sctx->num_resident_tex_handles--;
>>>> + return;
>>>> + }
>>>> +}
>>>> +
>>>> +static int si_add_resident_img_handle(struct si_context *sctx,
>>>> + struct si_image_handle *img_handle)
>>>> +{
>>>> + int idx;
>>>> +
>>>> + /* New resident handle, check if the backing array is large
>>>> enough. */
>>>> + if (sctx->num_resident_img_handles >=
>>>> sctx->max_resident_img_handles) {
>>>> + unsigned new_max_handles =
>>>> + MAX2(1, sctx->max_resident_img_handles * 2);
>>>> + struct si_image_handle **new_handles =
>>>> + REALLOC(sctx->resident_img_handles,
>>>> + sctx->num_resident_img_handles *
>>>> (sizeof(*new_handles)),
>>>> + new_max_handles * sizeof(*new_handles));
>>>> +
>>>> + if (new_handles) {
>>>> + sctx->resident_img_handles = new_handles;
>>>> + sctx->max_resident_img_handles = new_max_handles;
>>>> + } else {
>>>> + fprintf(stderr, "si_add_resident_img_handle: "
>>>> + "allocation failed\n");
>>>> + return -1;
>>>> + }
>>>> + }
>>>> +
>>>> + idx = sctx->num_resident_img_handles;
>>>> + sctx->resident_img_handles[idx] = img_handle;
>>>> + sctx->num_resident_img_handles++;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void si_del_resident_img_handle(struct si_context *sctx,
>>>> + struct si_image_handle
>>>> *img_handle)
>>>> +{
>>>> + unsigned i;
>>>> + int size;
>>>> +
>>>> + for (i = 0; i < sctx->num_resident_img_handles; i++) {
>>>> + if (sctx->resident_img_handles[i] != img_handle)
>>>> + continue;
>>>> +
>>>> + if (i < sctx->num_resident_img_handles - 1) {
>>>> + size = sizeof(*sctx->resident_img_handles) *
>>>> + (sctx->num_resident_img_handles - 1 - i);
>>>> +
>>>> + memmove(&sctx->resident_img_handles[i],
>>>> + &sctx->resident_img_handles[i + 1],
>>>> size);
>>>> + }
>>>> +
>>>> + sctx->num_resident_img_handles--;
>>>> + return;
>>>> + }
>>>> +}
>>>> +
>>>> +static struct si_resident_descriptor *
>>>> +si_create_resident_descriptor(struct si_context *sctx, uint32_t
>>>> *desc_list,
>>>> + unsigned size)
>>>
>>>
>>> This name is misleading, because the function is called by
>>> si_create_texture_handle when the handle is not resident. This needs
>>> code comments at least and maybe even some renaming if needed, because
>>> I don't understand why the name "resident_descriptor" is used by
>>> non-resident handles.
>>
>>
>> Mmmh, it creates a descriptor which is going to be resident at some point.
>> Not sure about the function name, maybe si_create_descriptor()?
>> si_create_bindless_descriptor?
>
> "bindless" in the name is better.
>
>>
>>>
>>>> +{
>>>> + struct si_screen *sscreen = sctx->screen;
>>>> + struct si_resident_descriptor *desc;
>>>> + struct pb_slab_entry *entry;
>>>> + void *ptr;
>>>> +
>>>> + /* Sub-allocate the resident descriptor from slabs. */
>>>> + entry = pb_slab_alloc(&sctx->resident_descriptor_slabs, 64, 0);
>>>> + if (!entry)
>>>> + return NULL;
>>>> +
>>>> + desc = NULL;
>>>> + desc = container_of(entry, desc, entry);
>>>> +
>>>> + /* Upload the descriptor. */
>>>> + ptr = sscreen->b.ws->buffer_map(desc->buffer->buf, NULL,
>>>> + PIPE_TRANSFER_WRITE |
>>>> + PIPE_TRANSFER_UNSYNCHRONIZED);
>>>
>>>
>>> This is using UNSYNCHRONIZED, but what guarantees that the buffer
>>> isn't being used by the GPU?
>>>
>>> can_reclaim_slab is only using cs_is_buffer_referenced, which only
>>> guarantees that the buffer is not referenced by an unflushed command
>>> buffer. It doesn't guarantee that the descriptor is not being used by
>>> the GPU.
>>
>>
>> Oh right. Actually, the resident descriptors are never reclaimed... because
>> they are added to every new CS, so can_reclaim_slab never returns TRUE. Yes,
>> it's dumb. :)
>>
>> Though, I would like to find a way to release unused slabs when it's
>> possible.
>
> The code should contain comments mentioning that.
Sure. One solution, though definitely not ideal is to always return
false in can_reclaim_slab(). Maybe we can do that and improve later?
Your call.
>
> Marek
>
More information about the mesa-dev
mailing list