[Mesa-dev] [PATCH 1/3] svga: keep a list of contexts for the screen

[Jose Fonseca]

I have few doubts/questions.

On 05/03/2019 23:57, Brian Paul wrote:
> This will allow us to query whether a context is valid.
> 
> In addition to keeping a list of contexts, we need to give each
> context we create a unique ID which is never re-used.  The screen
> also contains a bitmask to track which IDs are valid.  We need the
> ID since a context pointer could be recycled by the memory allocator.
> ---
>   src/gallium/drivers/svga/svga_context.c |  7 +++
>   src/gallium/drivers/svga/svga_context.h |  6 +++
>   src/gallium/drivers/svga/svga_screen.c  | 86 +++++++++++++++++++++++++++++++++
>   src/gallium/drivers/svga/svga_screen.h  | 23 +++++++++
>   4 files changed, 122 insertions(+)
> 
> diff --git a/src/gallium/drivers/svga/svga_context.c b/src/gallium/drivers/svga/svga_context.c
> index 7b3e9e8..1284d2f 100644
> --- a/src/gallium/drivers/svga/svga_context.c
> +++ b/src/gallium/drivers/svga/svga_context.c
> @@ -57,6 +57,7 @@ DEBUG_GET_ONCE_BOOL_OPTION(force_hw_line_stipple, "SVGA_FORCE_HW_LINE_STIPPLE",
>   static void
>   svga_destroy(struct pipe_context *pipe)
>   {
> +   struct svga_screen *svgascreen = svga_screen(pipe->screen);
>      struct svga_context *svga = svga_context(pipe);
>      unsigned shader, i;
>   
> @@ -97,6 +98,9 @@ svga_destroy(struct pipe_context *pipe)
>   
>      svga->swc->destroy(svga->swc);
>   
> +   /* remove this context from the screen's list */
> +   svga_screen_remove_context(svgascreen, svga);
> +
>      util_bitmask_destroy(svga->blend_object_id_bm);
>      util_bitmask_destroy(svga->ds_object_id_bm);
>      util_bitmask_destroy(svga->input_element_object_id_bm);
> @@ -300,6 +304,9 @@ svga_context_create(struct pipe_screen *screen, void *priv, unsigned flags)
>      svga->pred.query_id = SVGA3D_INVALID_ID;
>      svga->disable_rasterizer = FALSE;
>   
> +   /* add this context to the screen's list */
> +   svga_screen_save_context(svgascreen, svga);
> +
>      goto done;
>   
>   cleanup:
> diff --git a/src/gallium/drivers/svga/svga_context.h b/src/gallium/drivers/svga/svga_context.h
> index fc63ec3..2ec6b3f 100644
> --- a/src/gallium/drivers/svga/svga_context.h
> +++ b/src/gallium/drivers/svga/svga_context.h
> @@ -441,6 +441,12 @@ struct svga_context
>      struct u_upload_mgr *const0_upload;
>      struct u_upload_mgr *tex_upload;
>   
> +   /** used for svga_screen's list of contexts */
> +   struct list_head context_node;
> +
> +   /** A per-context ID which is never reused */
> +   unsigned context_id;
> +
>      struct {
>         boolean no_swtnl;
>         boolean force_swtnl;
> diff --git a/src/gallium/drivers/svga/svga_screen.c b/src/gallium/drivers/svga/svga_screen.c
> index 6cb5a14..6f4e8fc 100644
> --- a/src/gallium/drivers/svga/svga_screen.c
> +++ b/src/gallium/drivers/svga/svga_screen.c
> @@ -24,6 +24,7 @@
>    **********************************************************/
>   
>   #include "git_sha1.h" /* For MESA_GIT_SHA1 */
> +#include "util/u_bitmask.h"
>   #include "util/u_format.h"
>   #include "util/u_memory.h"
>   #include "util/u_inlines.h"
> @@ -1129,6 +1130,11 @@ svga_screen_create(struct svga_winsys_screen *sws)
>         debug_printf("svga: msaa samples mask: 0x%x\n", svgascreen->ms_samples);
>      }
>   
> +   LIST_INITHEAD(&svgascreen->contexts);
> +   mtx_init(&svgascreen->contexts_mutex, mtx_plain);
> +
> +   svgascreen->context_id_bm = util_bitmask_create();
> +
>      (void) mtx_init(&svgascreen->tex_mutex, mtx_plain);
>      (void) mtx_init(&svgascreen->swc_mutex, mtx_recursive);
>   
> @@ -1144,6 +1150,86 @@ error1:
>   }
>   
>   
> +/*
> + * Add the given context to the screen's list.
> + * This should be done once when a context is created.
> + */
> +void
> +svga_screen_save_context(struct svga_screen *svgascreen,
> +                         struct svga_context *svga)
> +{
> +   /* This context should not already be in the list */
> +   assert(!svga_screen_context_exists(svgascreen, svga));
> +
> +   /* the context ID should not be set yet */
> +   assert(svga->context_id == 0);
> +
> +   mtx_lock(&svgascreen->contexts_mutex);
> +
> +   /* Assign a unique ID to the svga context.  The ID is never reused */
> +   svga->context_id = svgascreen->context_counter++;

post-increment doesn't look right, because context_counter is never 
initialied to 1 anywhere.   So it seems to me the first context will 
have a context_id of zero, which shouldn't happen.

I think we should rename `context_counter` to `last_context_id` and use 
pre-increment.

> +
> +   util_bitmask_set(svgascreen->context_id_bm, svga->context_id);
> +
> +   LIST_ADDTAIL(&svga->context_node, &svgascreen->contexts);
> +
> +   mtx_unlock(&svgascreen->contexts_mutex);
> +}
> +
> +
> +/*
> + * Remove the given context from the screen's list.
> + * This should be done once when a context is destroyed;
> + */
> +void
> +svga_screen_remove_context(struct svga_screen *svgascreen,
> +                           struct svga_context *svga)
> +{
> +   /* This context should be in the list */
> +   assert(svga_screen_context_exists(svgascreen, svga));
> +
> +   mtx_lock(&svgascreen->contexts_mutex);
> +
> +   /* remove the ID from the bitmask */
> +   util_bitmask_clear(svgascreen->context_id_bm, svga->context_id);
> +
> +   LIST_DEL(&svga->context_node);
> +
> +   mtx_unlock(&svgascreen->contexts_mutex);
> +}
> +
> +
> +/*
> + * Return true if the context exists in the screen's list and its ID
> + * is valid, false otherwise.
> + */
> +boolean
> +svga_screen_context_exists(struct svga_screen *svgascreen,
> +                           const struct svga_context *svga)
> +{
> +   struct svga_context *ctx;
> +   boolean found = FALSE;
> +
> +   mtx_lock(&svgascreen->contexts_mutex);
> +
> +   LIST_FOR_EACH_ENTRY(ctx, &svgascreen->contexts, context_node) {
> +      if (ctx == svga) {
> +         /* Also check that the context ID is valid since a context
> +          * pointer could be recycled by the memory allocator.
> +          */
> +         if (util_bitmask_get(svgascreen->context_id_bm, svga->context_id)) {

At a glance context_id_bm seems overkill.  Why not remove the context 
from svgascreen->contexts immediately?

It's not clear what these three things mean:
- belonging to svgascreen->contexts
- belonging to svgascreen->context_id_bm
- have non zero svga->context_id

> +            found = TRUE;
> +            break;
> +         }
> +      }
> +   }
> +
> +   mtx_unlock(&svgascreen->contexts_mutex);
> +
> +   return found;
> +}
> +
> +
>   struct svga_winsys_screen *
>   svga_winsys_screen(struct pipe_screen *screen)
>   {
> diff --git a/src/gallium/drivers/svga/svga_screen.h b/src/gallium/drivers/svga/svga_screen.h
> index 12b9346..5ada452 100644
> --- a/src/gallium/drivers/svga/svga_screen.h
> +++ b/src/gallium/drivers/svga/svga_screen.h
> @@ -45,6 +45,15 @@ struct svga_screen
>      struct pipe_screen screen;
>      struct svga_winsys_screen *sws;
>   
> +   /** list of svga_contexts */
> +   struct list_head contexts;
> +   mtx_t contexts_mutex;
> +

I'd put everything that needs to be protected by the mutex into a 
structure, like


    struct {
       mtx_t mutex;
       struct list_head list;

       ...

    } contexts;

Because it's not immedaitely obvious that all recently added members 
need the mutex.

> +   /** Bitmask used to allocate context IDs */
> +   struct util_bitmask *context_id_bm;

I don't see context_id_bm being freed anywhere.

> +
> +   unsigned context_counter;
> +
>      SVGA3dHardwareVersion hw_version;
>   
>      /** Device caps */
> @@ -102,4 +111,18 @@ struct svga_screen *
>   svga_screen(struct pipe_screen *screen);
>   #endif
>   
> +
> +void
> +svga_screen_save_context(struct svga_screen *screen,
> +                         struct svga_context *svga);
> +
> +void
> +svga_screen_remove_context(struct svga_screen *screen,
> +                           struct svga_context *svga);
> +
> +boolean
> +svga_screen_context_exists(struct svga_screen *screen,
> +                           const struct svga_context *svga);
> +
> +
>   #endif /* SVGA_SCREEN_H */
>