[Mesa-dev] [RFC PATCH 52/65] radeonsi: implement ARB_bindless_texture

[Marek Olšák]

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.

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.

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.

> +{
> +       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.

Marek