[Mesa-dev] [PATCH 3/3] mesa: add gl_context::NewDriverState and use it for vertex arrays

[Brian Paul]

Looks good, but it took me a while to understand exactly what's going
on.  Suggestions for more/improved comments below.

On Tue, Apr 24, 2012 at 4:00 PM, Marek Olšák <maraeo at gmail.com> wrote:
> The vbo module recomputes its states if _NEW_ARRAY is set, so it shouldn't use
> the same flag to notify the driver. Since we've run out of bits in NewState
> and NewState is for core Mesa anyway, we need to find another way.
>
> This patch is the first to start decoupling the state flags meant only
> for core Mesa and those only for drivers.
>
> The idea is to have two flag sets:
> - gl_context::NewState - used by core Mesa only
> - gl_context::NewDriverState - used by drivers only

"used by drivers only.  The flags defined by the driver and
opaque/meaningless to core Mesa."


>
> It makes perfect sense to use NewState|=_NEW_ARRAY to notify the vbo module
> that the user changed vertex arrays, and the vbo module in turn sets
> a driver-specific flag to notify the driver that it should update its vertex
> array bindings.
>
> The driver decides which bits of NewDriverState should be set and stores them
> in gl_context::DriverFlags. Then, Core Mesa can do this:
> ctx->NewDriverState |= ctx->DriverFlags.NewArray;
>
> This patch implements this behavior and adapts st/mesa.
> DriverFlags.NewArray is set to ST_NEW_VERTEX_ARRAYS.
>
> Core Mesa only sets NewDriverState. It's the driver's responsibility to read
> it whenever it wants and reset it to 0.
> ---
>  src/mesa/main/context.c                  |    2 ++
>  src/mesa/main/mtypes.h                   |   14 ++++++++++++++
>  src/mesa/state_tracker/st_cb_rasterpos.c |    5 ++++-
>  src/mesa/state_tracker/st_context.c      |    6 ++++++
>  src/mesa/state_tracker/st_context.h      |    1 +
>  src/mesa/state_tracker/st_draw.c         |   12 +++++++++---
>  src/mesa/vbo/vbo_context.h               |    2 +-
>  src/mesa/vbo/vbo_exec_array.c            |    2 +-
>  src/mesa/vbo/vbo_exec_draw.c             |    2 +-
>  src/mesa/vbo/vbo_rebase.c                |    2 ++
>  src/mesa/vbo/vbo_save_draw.c             |    2 +-
>  src/mesa/vbo/vbo_split_copy.c            |    2 ++
>  src/mesa/vbo/vbo_split_inplace.c         |    2 ++
>  13 files changed, 46 insertions(+), 8 deletions(-)
>
> diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
> index d75351c..7e2ac98 100644
> --- a/src/mesa/main/context.c
> +++ b/src/mesa/main/context.c
> @@ -792,6 +792,7 @@ init_attrib_groups(struct gl_context *ctx)
>
>    /* Miscellaneous */
>    ctx->NewState = _NEW_ALL;
> +   ctx->NewDriverState = ~0;
>    ctx->ErrorValue = (GLenum) GL_NO_ERROR;
>    ctx->ResetStatus = (GLenum) GL_NO_ERROR;
>    ctx->varying_vp_inputs = VERT_BIT_ALL;
> @@ -1290,6 +1291,7 @@ _mesa_copy_context( const struct gl_context *src, struct gl_context *dst,
>    /* XXX FIXME:  Call callbacks?
>     */
>    dst->NewState = _NEW_ALL;
> +   dst->NewDriverState = ~0;
>  }
>  #endif
>
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index eb103ad..7f01514 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -3255,6 +3255,17 @@ typedef enum
>    API_OPENGLES2
>  } gl_api;
>
> +/**
> + * Driver-specific state flags.
> + *
> + * These are or'd with gl_context::NewDriverState to notify a driver about
> + * a state change. The driver gets to decide what bits should be set through
> + * this structure.

Just to be clear (and add to the comment), the bits here are set once
by the driver during context creation and never changed, right?  Also,
could you make it a bit more clear that the values of the flags are
defined by the driver and opaque to core Mesa?


> + */
> +struct gl_driver_flags
> +{
> +   GLbitfield NewArray;             /**< Vertex array state */
> +};
>
[...]


Reviewed-by: Brian Paul <brianp at vmware.com>