On Thu, May 25, 2017 at 10:49 PM, Samuel Pitoiset
<samuel.pitoi...@gmail.com> wrote:
>
>
> On 05/25/2017 10:45 PM, Marek Olšák wrote:
>>
>> 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.
>
>
> Sure. One solution, though definitely not ideal is to always return false in
> can_reclaim_slab(). Maybe we can do that and improve later? Your call.
Yeah that would also make things clearer.
Marek
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
