Re: [Mesa-dev] [PATCH] st/atifs: merge gl_program and ati_fragment_shader
On 11/13/2017 10:49 PM, Timothy Arceri wrote:
>
>
> On 14/11/17 13:30, Ian Romanick wrote:
>> On 11/10/2017 02:35 AM, Timothy Arceri wrote:
>>> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>>> index 6b5c5bbb36..7c357b07ee 100644
>>> --- a/src/mesa/main/mtypes.h
>>> +++ b/src/mesa/main/mtypes.h
>>> @@ -2352,58 +2376,29 @@ struct gl_fragment_program_state
>>> */
>>> struct gl_compute_program_state
>>> {
>>> /** Currently enabled and valid program (including internal
>>> programs
>>> * and compiled shader programs).
>>> */
>>> struct gl_program *_Current;
>>> };
>>> -/**
>>> - * ATI_fragment_shader runtime state
>>> - */
>>> -
>>> -struct atifs_instruction;
>>> -struct atifs_setupinst;
>>> -
>>> -/**
>>> - * ATI fragment shader
>>> - */
>>> -struct ati_fragment_shader
>>> -{
>>> - GLuint Id;
>>> - GLint RefCount;
>>> - struct atifs_instruction *Instructions[2];
>>> - struct atifs_setupinst *SetupInst[2];
>>> - GLfloat Constants[8][4];
>>> - GLbitfield LocalConstDef; /**< Indicates which constants have
>>> been set */
>>> - GLubyte numArithInstr[2];
>>> - GLubyte regsAssigned[2];
>>> - GLubyte NumPasses; /**< 1 or 2 */
>>> - GLubyte cur_pass;
>>> - GLubyte last_optype;
>>> - GLboolean interpinp1;
>>> - GLboolean isValid;
>>> - GLuint swizzlerq;
>>> - struct gl_program *Program;
>>> -};
>>> -
>>
>> Other places in Mesa would do this by C-style subclassing of gl_program:
>>
>> struct ati_fragment_shader {
>> struct gl_program Base;
>> struct atifs_instruction *Instructions[2];
>> struct atifs_setupinst *SetupInst[2];
>> GLfloat Constants[8][4];
>> GLbitfield LocalConstDef; /**< Indicates which constants have
>> been set */
>> GLubyte numArithInstr[2];
>> GLubyte regsAssigned[2];
>> GLubyte NumPasses; /**< 1 or 2 */
>> GLubyte cur_pass;
>> GLubyte last_optype;
>> GLboolean interpinp1;
>> GLboolean isValid;
>> GLuint swizzlerq;
>> };
>>
>> Is there a reason to not do that here?
>
> The st already does a C-style subclassing of gl_program, so it would
> start getting messy (which is what this patch tries to avoid in the
> first place). Also that type of subclassing is generally used for adding
> driver specific additional fields, not for specifying the basics of the
> program as we are doing here.
>
> Having these fields inside gl_program simplifies things a bunch. Is
> there something you don't like? IMO this makes things more consistent as
> its how we handle arb programs. The final step is to make this all a
> union but that requires a bit of work on i915 and swrast,
There wasn't anything in particular that I didn't like, I was just
trying to understand the design choice. What I mostly didn't like was
the old way ati_fragment_shader was implemented. :) I had forgotten that
ARB programs were also in gl_program, so this makes a bit more sense now.
>>> /**
>>> * Context state for GL_ATI_fragment_shader
>>> */
>>> struct gl_ati_fragment_shader_state
>>> {
>>> GLboolean Enabled;
>>> GLboolean Compiling;
>>> GLfloat GlobalConstants[8][4];
>>> - struct ati_fragment_shader *Current;
>>> + struct gl_program *Current;
>>> };
>>> /**
>>> * Shader subroutine function definition
>>> */
>>> struct gl_subroutine_function
>>> {
>>> char *name;
>>> int index;
>>> int num_compat_types;
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] st/atifs: merge gl_program and ati_fragment_shader
On 14/11/17 13:30, Ian Romanick wrote:
On 11/10/2017 02:35 AM, Timothy Arceri wrote:
diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
index 6b5c5bbb36..7c357b07ee 100644
--- a/src/mesa/main/mtypes.h
+++ b/src/mesa/main/mtypes.h
@@ -2352,58 +2376,29 @@ struct gl_fragment_program_state
*/
struct gl_compute_program_state
{
/** Currently enabled and valid program (including internal programs
* and compiled shader programs).
*/
struct gl_program *_Current;
};
-/**
- * ATI_fragment_shader runtime state
- */
-
-struct atifs_instruction;
-struct atifs_setupinst;
-
-/**
- * ATI fragment shader
- */
-struct ati_fragment_shader
-{
- GLuint Id;
- GLint RefCount;
- struct atifs_instruction *Instructions[2];
- struct atifs_setupinst *SetupInst[2];
- GLfloat Constants[8][4];
- GLbitfield LocalConstDef; /**< Indicates which constants have been set */
- GLubyte numArithInstr[2];
- GLubyte regsAssigned[2];
- GLubyte NumPasses; /**< 1 or 2 */
- GLubyte cur_pass;
- GLubyte last_optype;
- GLboolean interpinp1;
- GLboolean isValid;
- GLuint swizzlerq;
- struct gl_program *Program;
-};
-
Other places in Mesa would do this by C-style subclassing of gl_program:
struct ati_fragment_shader {
struct gl_program Base;
struct atifs_instruction *Instructions[2];
struct atifs_setupinst *SetupInst[2];
GLfloat Constants[8][4];
GLbitfield LocalConstDef; /**< Indicates which constants have been set */
GLubyte numArithInstr[2];
GLubyte regsAssigned[2];
GLubyte NumPasses; /**< 1 or 2 */
GLubyte cur_pass;
GLubyte last_optype;
GLboolean interpinp1;
GLboolean isValid;
GLuint swizzlerq;
};
Is there a reason to not do that here?
The st already does a C-style subclassing of gl_program, so it would
start getting messy (which is what this patch tries to avoid in the
first place). Also that type of subclassing is generally used for adding
driver specific additional fields, not for specifying the basics of the
program as we are doing here.
Having these fields inside gl_program simplifies things a bunch. Is
there something you don't like? IMO this makes things more consistent as
its how we handle arb programs. The final step is to make this all a
union but that requires a bit of work on i915 and swrast,
/**
* Context state for GL_ATI_fragment_shader
*/
struct gl_ati_fragment_shader_state
{
GLboolean Enabled;
GLboolean Compiling;
GLfloat GlobalConstants[8][4];
- struct ati_fragment_shader *Current;
+ struct gl_program *Current;
};
/**
* Shader subroutine function definition
*/
struct gl_subroutine_function
{
char *name;
int index;
int num_compat_types;
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] st/atifs: merge gl_program and ati_fragment_shader
On 11/10/2017 02:35 AM, Timothy Arceri wrote:
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index 6b5c5bbb36..7c357b07ee 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -2352,58 +2376,29 @@ struct gl_fragment_program_state
> */
> struct gl_compute_program_state
> {
> /** Currently enabled and valid program (including internal programs
> * and compiled shader programs).
> */
> struct gl_program *_Current;
> };
>
>
> -/**
> - * ATI_fragment_shader runtime state
> - */
> -
> -struct atifs_instruction;
> -struct atifs_setupinst;
> -
> -/**
> - * ATI fragment shader
> - */
> -struct ati_fragment_shader
> -{
> - GLuint Id;
> - GLint RefCount;
> - struct atifs_instruction *Instructions[2];
> - struct atifs_setupinst *SetupInst[2];
> - GLfloat Constants[8][4];
> - GLbitfield LocalConstDef; /**< Indicates which constants have been set */
> - GLubyte numArithInstr[2];
> - GLubyte regsAssigned[2];
> - GLubyte NumPasses; /**< 1 or 2 */
> - GLubyte cur_pass;
> - GLubyte last_optype;
> - GLboolean interpinp1;
> - GLboolean isValid;
> - GLuint swizzlerq;
> - struct gl_program *Program;
> -};
> -
Other places in Mesa would do this by C-style subclassing of gl_program:
struct ati_fragment_shader {
struct gl_program Base;
struct atifs_instruction *Instructions[2];
struct atifs_setupinst *SetupInst[2];
GLfloat Constants[8][4];
GLbitfield LocalConstDef; /**< Indicates which constants have been set */
GLubyte numArithInstr[2];
GLubyte regsAssigned[2];
GLubyte NumPasses; /**< 1 or 2 */
GLubyte cur_pass;
GLubyte last_optype;
GLboolean interpinp1;
GLboolean isValid;
GLuint swizzlerq;
};
Is there a reason to not do that here?
> /**
> * Context state for GL_ATI_fragment_shader
> */
> struct gl_ati_fragment_shader_state
> {
> GLboolean Enabled;
> GLboolean Compiling;
> GLfloat GlobalConstants[8][4];
> - struct ati_fragment_shader *Current;
> + struct gl_program *Current;
> };
>
> /**
> * Shader subroutine function definition
> */
> struct gl_subroutine_function
> {
> char *name;
> int index;
> int num_compat_types;
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] st/atifs: merge gl_program and ati_fragment_shader
Miklós Máté writes: > On 12/11/17 00:06, Timothy Arceri wrote: >> On 12/11/17 04:10, Miklós Máté wrote: >>> Hi, >>> >>> this breaks a few things. The patch below gets rid of the assertion >>> failures, but the reference counting needs a proper fix, and swrast >>> draws blackness when ATIfs is enabled. >> >> Thanks for testing :) Is there something freely available I can test >> this with? Are your piglit tests in a state where I could pull the >> repo and use what you have? > My piglit tests are about 25% complete. My progress is slow, because as > I go along I also fix the problems the tests find in Mesa. I can send > you a targz of what I have so far (mostly API sanity checks and a couple > of very simple render tests). I know of two games that use this > extension: KOTOR is not free, but Doom3 has a demo version. Set > r_renderer=r200 and image_useNormalCompression=2. I'd recommend submitting tests for review as soon as you can -- you'll probably get feedback about general structure of writing tests that will mean less work when writing your later tests. :) signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] st/atifs: merge gl_program and ati_fragment_shader
On 12/11/17 00:06, Timothy Arceri wrote: On 12/11/17 04:10, Miklós Máté wrote: Hi, this breaks a few things. The patch below gets rid of the assertion failures, but the reference counting needs a proper fix, and swrast draws blackness when ATIfs is enabled. Thanks for testing :) Is there something freely available I can test this with? Are your piglit tests in a state where I could pull the repo and use what you have? My piglit tests are about 25% complete. My progress is slow, because as I go along I also fix the problems the tests find in Mesa. I can send you a targz of what I have so far (mostly API sanity checks and a couple of very simple render tests). I know of two games that use this extension: KOTOR is not free, but Doom3 has a demo version. Set r_renderer=r200 and image_useNormalCompression=2. MM ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] st/atifs: merge gl_program and ati_fragment_shader
On 12/11/17 04:10, Miklós Máté wrote:
Hi,
this breaks a few things. The patch below gets rid of the assertion
failures, but the reference counting needs a proper fix, and swrast
draws blackness when ATIfs is enabled.
Thanks for testing :) Is there something freely available I can test
this with? Are your piglit tests in a state where I could pull the repo
and use what you have?
MM
diff --git a/src/mesa/program/program.c b/src/mesa/program/program.c
index faf5b7fa28..b71917d7b1 100644
--- a/src/mesa/program/program.c
+++ b/src/mesa/program/program.c
@@ -258,7 +258,6 @@ _mesa_delete_program(struct gl_context *ctx, struct
gl_program *prog)
{
(void) ctx;
assert(prog);
- assert(prog->RefCount==0);
if (prog == &_mesa_DummyProgram)
return;
@@ -320,7 +319,8 @@ _mesa_reference_program_(struct gl_context *ctx,
assert(prog->Target == GL_VERTEX_PROGRAM_ARB);
else if ((*ptr)->Target == GL_FRAGMENT_PROGRAM_ARB)
assert(prog->Target == GL_FRAGMENT_PROGRAM_ARB ||
- prog->Target == GL_FRAGMENT_PROGRAM_NV);
+ prog->Target == GL_FRAGMENT_PROGRAM_NV ||
+ prog->Target == GL_FRAGMENT_SHADER_ATI);
else if ((*ptr)->Target == GL_GEOMETRY_PROGRAM_NV)
assert(prog->Target == GL_GEOMETRY_PROGRAM_NV);
}
diff --git a/src/mesa/state_tracker/st_program.c
b/src/mesa/state_tracker/st_program.c
index e3649a8b7c..fe0724d331 100644
--- a/src/mesa/state_tracker/st_program.c
+++ b/src/mesa/state_tracker/st_program.c
@@ -1843,6 +1843,7 @@ destroy_program_variants(struct st_context *st,
struct gl_program *target)
}
}
break;
+ case GL_FRAGMENT_SHADER_ATI:
case GL_FRAGMENT_PROGRAM_ARB:
{
struct st_fragment_program *stfp =
@@ -2029,6 +2030,7 @@ st_precompile_shader_variant(struct st_context *st,
break;
}
+ case GL_FRAGMENT_SHADER_ATI:
case GL_FRAGMENT_PROGRAM_ARB: {
struct st_fragment_program *p = (struct st_fragment_program *)prog;
struct st_fp_variant_key key;
diff --git a/src/mesa/swrast/s_context.c b/src/mesa/swrast/s_context.c
index 9f3d21f91d..b8160509c7 100644
--- a/src/mesa/swrast/s_context.c
+++ b/src/mesa/swrast/s_context.c
@@ -251,7 +251,7 @@ _swrast_update_fog_state( struct gl_context *ctx )
SWcontext *swrast = SWRAST_CONTEXT(ctx);
const struct gl_program *fp = ctx->FragmentProgram._Current;
- assert(fp == NULL || fp->Target == GL_FRAGMENT_PROGRAM_ARB);
+ assert(fp == NULL || fp->Target == GL_FRAGMENT_PROGRAM_ARB ||
fp->Target == GL_FRAGMENT_SHADER_ATI);
(void) fp; /* silence unused var warning */
/* determine if fog is needed, and if so, which fog mode */
On 10/11/17 11:35, Timothy Arceri wrote:
This increases the size of gl_program but in future a union
can be used to offset this increase in memory use. Combining
the two reduces code and make it easier to follow.
Cc: Miklós Máté
---
NOTE: compile tested only.
src/mesa/drivers/common/driverfuncs.c | 3 -
src/mesa/drivers/dri/r200/r200_context.h | 2 +-
src/mesa/drivers/dri/r200/r200_fragshader.c | 86 ++---
src/mesa/drivers/dri/r200/r200_state_init.c | 2 +-
src/mesa/drivers/dri/r200/r200_vertprog.c | 1 +
src/mesa/main/atifragshader.c | 193
+---
src/mesa/main/atifragshader.h | 4 +-
src/mesa/main/dd.h | 6 +-
src/mesa/main/mtypes.h | 57
src/mesa/main/shared.c | 2 +-
src/mesa/main/state.c | 4 +-
src/mesa/main/state.h | 2 +-
src/mesa/program/program.c | 3 +-
src/mesa/state_tracker/st_atifs_to_tgsi.c | 28 ++--
src/mesa/state_tracker/st_atifs_to_tgsi.h | 2 -
src/mesa/state_tracker/st_atom_constbuf.c | 9 +-
src/mesa/state_tracker/st_atom_shader.c | 8 +-
src/mesa/state_tracker/st_cb_program.c | 20 +--
src/mesa/state_tracker/st_program.c | 7 +-
src/mesa/state_tracker/st_program.h | 1 -
src/mesa/swrast/s_atifragshader.c | 18 +--
21 files changed, 206 insertions(+), 252 deletions(-)
diff --git a/src/mesa/drivers/common/driverfuncs.c
b/src/mesa/drivers/common/driverfuncs.c
index ddb4bb6d6a..a8b4d9857f 100644
--- a/src/mesa/drivers/common/driverfuncs.c
+++ b/src/mesa/drivers/common/driverfuncs.c
@@ -110,23 +110,20 @@ _mesa_init_driver_functions(struct
dd_function_table *driver)
driver->AllocTextureImageBuffer =
_swrast_alloc_texture_image_buffer;
driver->FreeTextureImageBuffer = _swrast_free_texture_image_buffer;
driver->MapTextureImage = _swrast_map_teximage;
driver->UnmapTextureImage = _swrast_unmap_teximage;
driver->DrawTex = _mesa_meta_DrawTex;
/* Vertex/fragment programs */
driver->NewProgram = _mesa_new_program;
driver->DeleteProgram = _mesa_delete_program;
-
Re: [Mesa-dev] [PATCH] st/atifs: merge gl_program and ati_fragment_shader
Hi,
this breaks a few things. The patch below gets rid of the assertion
failures, but the reference counting needs a proper fix, and swrast
draws blackness when ATIfs is enabled.
MM
diff --git a/src/mesa/program/program.c b/src/mesa/program/program.c
index faf5b7fa28..b71917d7b1 100644
--- a/src/mesa/program/program.c
+++ b/src/mesa/program/program.c
@@ -258,7 +258,6 @@ _mesa_delete_program(struct gl_context *ctx, struct
gl_program *prog)
{
(void) ctx;
assert(prog);
- assert(prog->RefCount==0);
if (prog == &_mesa_DummyProgram)
return;
@@ -320,7 +319,8 @@ _mesa_reference_program_(struct gl_context *ctx,
assert(prog->Target == GL_VERTEX_PROGRAM_ARB);
else if ((*ptr)->Target == GL_FRAGMENT_PROGRAM_ARB)
assert(prog->Target == GL_FRAGMENT_PROGRAM_ARB ||
- prog->Target == GL_FRAGMENT_PROGRAM_NV);
+ prog->Target == GL_FRAGMENT_PROGRAM_NV ||
+ prog->Target == GL_FRAGMENT_SHADER_ATI);
else if ((*ptr)->Target == GL_GEOMETRY_PROGRAM_NV)
assert(prog->Target == GL_GEOMETRY_PROGRAM_NV);
}
diff --git a/src/mesa/state_tracker/st_program.c
b/src/mesa/state_tracker/st_program.c
index e3649a8b7c..fe0724d331 100644
--- a/src/mesa/state_tracker/st_program.c
+++ b/src/mesa/state_tracker/st_program.c
@@ -1843,6 +1843,7 @@ destroy_program_variants(struct st_context *st,
struct gl_program *target)
}
}
break;
+ case GL_FRAGMENT_SHADER_ATI:
case GL_FRAGMENT_PROGRAM_ARB:
{
struct st_fragment_program *stfp =
@@ -2029,6 +2030,7 @@ st_precompile_shader_variant(struct st_context *st,
break;
}
+ case GL_FRAGMENT_SHADER_ATI:
case GL_FRAGMENT_PROGRAM_ARB: {
struct st_fragment_program *p = (struct st_fragment_program *)prog;
struct st_fp_variant_key key;
diff --git a/src/mesa/swrast/s_context.c b/src/mesa/swrast/s_context.c
index 9f3d21f91d..b8160509c7 100644
--- a/src/mesa/swrast/s_context.c
+++ b/src/mesa/swrast/s_context.c
@@ -251,7 +251,7 @@ _swrast_update_fog_state( struct gl_context *ctx )
SWcontext *swrast = SWRAST_CONTEXT(ctx);
const struct gl_program *fp = ctx->FragmentProgram._Current;
- assert(fp == NULL || fp->Target == GL_FRAGMENT_PROGRAM_ARB);
+ assert(fp == NULL || fp->Target == GL_FRAGMENT_PROGRAM_ARB ||
fp->Target == GL_FRAGMENT_SHADER_ATI);
(void) fp; /* silence unused var warning */
/* determine if fog is needed, and if so, which fog mode */
On 10/11/17 11:35, Timothy Arceri wrote:
This increases the size of gl_program but in future a union
can be used to offset this increase in memory use. Combining
the two reduces code and make it easier to follow.
Cc: Miklós Máté
---
NOTE: compile tested only.
src/mesa/drivers/common/driverfuncs.c | 3 -
src/mesa/drivers/dri/r200/r200_context.h| 2 +-
src/mesa/drivers/dri/r200/r200_fragshader.c | 86 ++---
src/mesa/drivers/dri/r200/r200_state_init.c | 2 +-
src/mesa/drivers/dri/r200/r200_vertprog.c | 1 +
src/mesa/main/atifragshader.c | 193 +---
src/mesa/main/atifragshader.h | 4 +-
src/mesa/main/dd.h | 6 +-
src/mesa/main/mtypes.h | 57
src/mesa/main/shared.c | 2 +-
src/mesa/main/state.c | 4 +-
src/mesa/main/state.h | 2 +-
src/mesa/program/program.c | 3 +-
src/mesa/state_tracker/st_atifs_to_tgsi.c | 28 ++--
src/mesa/state_tracker/st_atifs_to_tgsi.h | 2 -
src/mesa/state_tracker/st_atom_constbuf.c | 9 +-
src/mesa/state_tracker/st_atom_shader.c | 8 +-
src/mesa/state_tracker/st_cb_program.c | 20 +--
src/mesa/state_tracker/st_program.c | 7 +-
src/mesa/state_tracker/st_program.h | 1 -
src/mesa/swrast/s_atifragshader.c | 18 +--
21 files changed, 206 insertions(+), 252 deletions(-)
diff --git a/src/mesa/drivers/common/driverfuncs.c
b/src/mesa/drivers/common/driverfuncs.c
index ddb4bb6d6a..a8b4d9857f 100644
--- a/src/mesa/drivers/common/driverfuncs.c
+++ b/src/mesa/drivers/common/driverfuncs.c
@@ -110,23 +110,20 @@ _mesa_init_driver_functions(struct dd_function_table
*driver)
driver->AllocTextureImageBuffer = _swrast_alloc_texture_image_buffer;
driver->FreeTextureImageBuffer = _swrast_free_texture_image_buffer;
driver->MapTextureImage = _swrast_map_teximage;
driver->UnmapTextureImage = _swrast_unmap_teximage;
driver->DrawTex = _mesa_meta_DrawTex;
/* Vertex/fragment programs */
driver->NewProgram = _mesa_new_program;
driver->DeleteProgram = _mesa_delete_program;
- /* ATI_fragment_shader */
- driver->NewATIfs = NULL;
-
/* simple state commands */
driver->AlphaFunc = NULL;
driver->BlendColor = NULL;
driver->BlendEquationSeparate = NULL;
driver->BlendFuncSeparate = NUL
