[Mesa-dev] [RFC PATCH 52/65] radeonsi: implement ARB_bindless_texture
Samuel Pitoiset
samuel.pitoiset at gmail.com
Wed May 24 16:00:03 UTC 2017
On 05/24/2017 01:25 PM, Marek Olšák wrote:
> 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)
>> +{
>> + 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);
>> + util_memcpy_cpu_to_le32(ptr + desc->offset, desc_list, size);
>> + sscreen->b.ws->buffer_unmap(desc->buffer->buf);
>> +
>> + return desc;
>> +}
>> +
>> +static uint64_t si_create_texture_handle(struct pipe_context *ctx,
>> + struct pipe_resource *texture,
>> + struct pipe_sampler_view *view,
>> + const struct pipe_sampler_state *state)
>> +{
>> + struct si_sampler_view *sview = (struct si_sampler_view *)view;
>> + struct si_context *sctx = (struct si_context *)ctx;
>> + struct si_texture_handle *tex_handle;
>> + struct si_sampler_state *sstate;
>> + uint32_t desc_list[16];
>> + uint64_t handle;
>> +
>> + tex_handle = CALLOC_STRUCT(si_texture_handle);
>> + if (!tex_handle)
>> + return 0;
>> +
>> + memset(desc_list, 0, sizeof(desc_list));
>> + si_init_descriptor_list(&desc_list[0], 16, 1, null_texture_descriptor);
>> +
>> + sstate = ctx->create_sampler_state(ctx, state);
>> + if (!sstate) {
>> + FREE(tex_handle);
>> + return 0;
>> + }
>> +
>> + si_set_sampler_view_desc(sctx, sview, sstate, &desc_list[0]);
>> + ctx->delete_sampler_state(ctx, sstate);
>> +
>> + tex_handle->view = sview;
>> + tex_handle->desc = si_create_resident_descriptor(sctx, desc_list,
>> + sizeof(desc_list));
>> + if (!tex_handle->desc) {
>> + FREE(tex_handle);
>> + return 0;
>> + }
>> +
>> + handle = tex_handle->desc->buffer->gpu_address +
>> + tex_handle->desc->offset;
>> +
>> + if (!_mesa_hash_table_insert(sctx->tex_handles, (void *)handle,
>> + tex_handle)) {
>> + pb_slab_free(&sctx->resident_descriptor_slabs,
>> + &tex_handle->desc->entry);
>> + FREE(tex_handle);
>> + return 0;
>> + }
>> +
>> + return handle;
>
> Here's the reference counting issue. First of all, "texture" isn't
> used, which is weird, because it's part of the interface. Then, the
> "view" pointer is stored in tex_handle without incrementing the
> reference counter, so st/mesa can release it.
>
> It looks like the original idea was to use "texture" as the resource
> and "view" as the view template, while this code ignores "texture" and
> uses "view" as a full sampler view.
Exactly, in my first implementation, the sampler views were created
directly in si_create_texture_handle(). I will remove this useless
"texture" parameter and I will fix the refcount issue.
>
>
>> +}
>> +
>> +static void si_delete_texture_handle(struct pipe_context *ctx, uint64_t handle)
>> +{
>> + struct si_context *sctx = (struct si_context *)ctx;
>> + struct si_texture_handle *tex_handle;
>> + struct hash_entry *entry;
>> +
>> + entry = _mesa_hash_table_search(sctx->tex_handles, (void *)handle);
>> + if (!entry)
>> + return;
>> +
>> + tex_handle = (struct si_texture_handle *)entry->data;
>> +
>> + _mesa_hash_table_remove(sctx->tex_handles, entry);
>> + pb_slab_free(&sctx->resident_descriptor_slabs,
>> + &tex_handle->desc->entry);
>> + FREE(tex_handle);
>> +}
>> +
>> +static void si_make_texture_handle_resident(struct pipe_context *ctx,
>> + uint64_t handle, bool resident)
>> +{
>> + struct si_context *sctx = (struct si_context *)ctx;
>> + struct si_texture_handle *tex_handle;
>> + struct si_sampler_view *sview;
>> + struct hash_entry *entry;
>> +
>> + entry = _mesa_hash_table_search(sctx->tex_handles, (void *)handle);
>> + if (!entry)
>> + return;
>> +
>> + tex_handle = (struct si_texture_handle *)entry->data;
>> + sview = tex_handle->view;
>> +
>> + if (resident) {
>> + si_add_resident_tex_handle(sctx, tex_handle);
>> +
>> + /* Add the buffers to the current CS in case si_begin_new_cs()
>> + * is not going to be called.
>> + */
>> + radeon_add_to_buffer_list(&sctx->b, &sctx->b.gfx,
>> + tex_handle->desc->buffer,
>> + RADEON_USAGE_READ,
>> + RADEON_PRIO_DESCRIPTORS);
>> +
>> + si_sampler_view_add_buffer(sctx, sview->base.texture,
>> + RADEON_USAGE_READ,
>> + sview->is_stencil_sampler, false);
>> + } else {
>> + si_del_resident_tex_handle(sctx, tex_handle);
>> + }
>> +}
>> +
>> +static uint64_t si_create_image_handle(struct pipe_context *ctx,
>> + const struct pipe_image_view *view)
>> +{
>> + struct si_context *sctx = (struct si_context *)ctx;
>> + struct si_image_handle *img_handle;
>> + uint32_t desc_list[16];
>> + uint64_t handle;
>> +
>> + if (!view || !view->resource)
>> + return 0;
>> +
>> + img_handle = CALLOC_STRUCT(si_image_handle);
>> + if (!img_handle)
>> + return 0;
>> +
>> + memset(desc_list, 0, sizeof(desc_list));
>> + si_init_descriptor_list(&desc_list[0], 8, 1, null_image_descriptor);
>> +
>> + si_set_shader_image_desc(sctx, view, false, &desc_list[0]);
>> +
>> + util_copy_image_view(&img_handle->view, view);
>> + img_handle->desc = si_create_resident_descriptor(sctx, desc_list,
>> + sizeof(desc_list));
>> + if (!img_handle->desc) {
>> + FREE(img_handle);
>
> This is a memory leak. util_copy_image_view increments the reference
> counter of view->resource, which isn't cleared before FREE.
>
>> + return 0;
>> + }
>> +
>> + handle = img_handle->desc->buffer->gpu_address +
>> + img_handle->desc->offset;
>> +
>> + if (!_mesa_hash_table_insert(sctx->img_handles, (void *)handle,
>> + img_handle)) {
>> + pb_slab_free(&sctx->resident_descriptor_slabs,
>> + &img_handle->desc->entry);
>> + FREE(img_handle);
>> + return 0;
>> + }
>> +
>> + return handle;
>> +}
>> +
>> +static void si_delete_image_handle(struct pipe_context *ctx, uint64_t handle)
>> +{
>> + struct si_context *sctx = (struct si_context *)ctx;
>> + struct si_image_handle *img_handle;
>> + struct hash_entry *entry;
>> +
>> + entry = _mesa_hash_table_search(sctx->img_handles, (void *)handle);
>> + if (!entry)
>> + return;
>> +
>> + img_handle = (struct si_image_handle *)entry->data;
>> +
>> + _mesa_hash_table_remove(sctx->img_handles, entry);
>> + pb_slab_free(&sctx->resident_descriptor_slabs,
>> + &img_handle->desc->entry);
>> + FREE(img_handle);
>
> Same memory leak here.
>
> I've not yet looked at this patch thoroughly. It's just what I've noticed now.
Good catch, fixed locally. Thanks!
>
> Marek
>
More information about the mesa-dev
mailing list