On Thu, May 25, 2017 at 9:56 PM, Samuel Pitoiset
<samuel.pitoi...@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.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?
"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.
Marek
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
