[Mesa-dev] [PATCH] mesa: remove DummyShader from atifragshader.c

[Emil Velikov]

On 20 December 2017 at 16:33, Miklós Máté <mtmkls at gmail.com> wrote:
> It's much cleaner to allocate a normal shader struct when
> GenFragmentShadersATI is called.
>
> Signed-off-by: Miklós Máté <mtmkls at gmail.com>
> ---
>  src/mesa/main/atifragshader.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/src/mesa/main/atifragshader.c b/src/mesa/main/atifragshader.c
> index 6b636f1dc7..0a5ba26310 100644
> --- a/src/mesa/main/atifragshader.c
> +++ b/src/mesa/main/atifragshader.c
> @@ -34,8 +34,6 @@
>
>  #define MESA_DEBUG_ATI_FS 0
>
> -static struct ati_fragment_shader DummyShader;
> -
>
>  /**
>   * Allocate and initialize a new ATI fragment shader object.
> @@ -61,9 +59,6 @@ _mesa_delete_ati_fragment_shader(struct gl_context *ctx, struct ati_fragment_sha
>  {
>     GLuint i;
>
> -   if (s == &DummyShader)
> -      return;
> -
>     for (i = 0; i < MAX_NUM_PASSES_ATI; i++) {
>        free(s->Instructions[i]);
>        free(s->SetupInst[i]);
> @@ -205,7 +200,13 @@ _mesa_GenFragmentShadersATI(GLuint range)
>
>     first = _mesa_HashFindFreeKeyBlock(ctx->Shared->ATIShaders, range);
>     for (i = 0; i < range; i++) {
> -      _mesa_HashInsertLocked(ctx->Shared->ATIShaders, first + i, &DummyShader);
> +      struct ati_fragment_shader *newProg = _mesa_new_ati_fragment_shader(ctx, first + i);
> +      if (!newProg) {
> +         _mesa_error(ctx, GL_OUT_OF_MEMORY, "glBindFragmentShaderATI");
> +         _mesa_HashUnlockMutex(ctx->Shared->ATIShaders);
> +         return;
> +      }
> +      _mesa_HashInsertLocked(ctx->Shared->ATIShaders, first + i, newProg);
Static variables are fiddly, but this doesn't seems like an
improvement. Currently we'll allocate a struct only as needed.
Whereas with the patch we'll allocate the whole "range" even if not
all are utilised.

Feel free to ignore my comments if others approve with the patch goal.

>     }
>
>     _mesa_HashUnlockMutex(ctx->Shared->ATIShaders);
> @@ -246,7 +247,7 @@ _mesa_BindFragmentShaderATI(GLuint id)
>     else {
>        newProg = (struct ati_fragment_shader *)
>           _mesa_HashLookup(ctx->Shared->ATIShaders, id);
> -      if (!newProg || newProg == &DummyShader) {
> +      if (!newProg) {
>          /* allocate a new program now */
>          newProg = _mesa_new_ati_fragment_shader(ctx, id);
>          if (!newProg) {
> @@ -279,10 +280,8 @@ _mesa_DeleteFragmentShaderATI(GLuint id)
>     if (id != 0) {
>        struct ati_fragment_shader *prog = (struct ati_fragment_shader *)
>          _mesa_HashLookup(ctx->Shared->ATIShaders, id);
> -      if (prog == &DummyShader) {
> -        _mesa_HashRemove(ctx->Shared->ATIShaders, id);
> -      }
> -      else if (prog) {
> +
> +      if (prog) {
>          if (ctx->ATIFragmentShader.Current &&
>              ctx->ATIFragmentShader.Current->Id == id) {
>              FLUSH_VERTICES(ctx, _NEW_PROGRAM);
The existing _mesa_DeleteFragmentShaderATI can be simplified as below.
It makes things easier to parse over here, feel free to reuse if you
agree.
Unrelated: there's no need cast _mesa_HashLookup - void * is good for
anything C [but not C++].

-Emil

diff --git a/src/mesa/main/atifragshader.c b/src/mesa/main/atifragshader.c
index 6b636f1dc74..e18393fb827 100644
--- a/src/mesa/main/atifragshader.c
+++ b/src/mesa/main/atifragshader.c
@@ -276,29 +276,27 @@ _mesa_DeleteFragmentShaderATI(GLuint id)
       return;
    }

-   if (id != 0) {
-      struct ati_fragment_shader *prog = (struct ati_fragment_shader *)
- _mesa_HashLookup(ctx->Shared->ATIShaders, id);
-      if (prog == &DummyShader) {
- _mesa_HashRemove(ctx->Shared->ATIShaders, id);
-      }
-      else if (prog) {
- if (ctx->ATIFragmentShader.Current &&
-      ctx->ATIFragmentShader.Current->Id == id) {
-      FLUSH_VERTICES(ctx, _NEW_PROGRAM);
-     _mesa_BindFragmentShaderATI(0);
- }
-      }
+   /* As of spec revision: 1.6 non-existent or default shaders do not generate
+    * an error */
+   if (id == 0)
+      return;

-      /* The ID is immediately available for re-use now */
-      _mesa_HashRemove(ctx->Shared->ATIShaders, id);
-      if (prog) {
- prog->RefCount--;
- if (prog->RefCount <= 0) {
-            _mesa_delete_ati_fragment_shader(ctx, prog);
- }
-      }
+   struct ati_fragment_shader *prog =
_mesa_HashLookup(ctx->Shared->ATIShaders, id);
+   if (!prog)
+      return;
+
+   if (prog != &DummyShader &&
+       ctx->ATIFragmentShader.Current &&
+       ctx->ATIFragmentShader.Current->Id == id) {
+      FLUSH_VERTICES(ctx, _NEW_PROGRAM);
+      _mesa_BindFragmentShaderATI(0);
    }
+
+   _mesa_HashRemove(ctx->Shared->ATIShaders, id);
+
+   prog->RefCount--;
+   if (prog->RefCount <= 0)
+      _mesa_delete_ati_fragment_shader(ctx, prog);
 }