On 28 January 2018 at 12:46, Miklós Máté <mtm...@gmail.com> wrote:
> On 24/01/18 17:16, Emil Velikov wrote:
>>
>> On 20 December 2017 at 16:33, Miklós Máté <mtm...@gmail.com> wrote:
>>>
>>> It's much cleaner to allocate a normal shader struct when
>>> GenFragmentShadersATI is called.
>>>
>>> Signed-off-by: Miklós Máté <mtm...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
