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.pitoi...@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.pitoi...@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?
+{ + 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.
Marek
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
