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