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

[Emil Velikov]

On 28 January 2018 at 12:46, Miklós Máté <mtmkls at gmail.com> wrote:
> On 24/01/18 17:16, Emil Velikov wrote:
>>
>> 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.
>
> Thanks for the review. My goal was indeed the removal of the static variable
> and the special cases to handle it. You are right that this change will
> increase the cost of GenFragmentShadersATI(large_number), however, the 3
> users of this extension that I know of don't do that. Doom 3 only has one
> shader and uses 255 as the hardcoded id for it, KOTOR generates ids
> one-by-one, and my piglit tests only allocate at most 3 ids at once. Feel
> free to ignore this patch if you feel like it's not useful.
>
FWIW using static [const] dummy instances is fairly common in the codebase.
$ git grep -i static.*dummy | wc -l > 31

If devs are happy with the proposed approach it'll be better to update
everything.

-Emil