[Mesa-dev] [RFC PATCH 52/65] radeonsi: implement ARB_bindless_texture
Samuel Pitoiset
samuel.pitoiset at gmail.com
Thu May 25 19:56:32 UTC 2017
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?
>
>> +{
>> + 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
>
More information about the mesa-dev
mailing list