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.
}
_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
Your version seems clearer. If there will be a v2 of this patch, I'll
reuse it.
MM
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);
}
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
