Re: [r300] partly working fragment.position patch
Brian Paul wrote: > Rune Petersen wrote: >> Brian Paul wrote: > >>> It seems to me that in ctx->Driver.ProgramStringNotify() you can add any >>> extra parameters you need to the program's parameters list. >> >> I would prefer to make driver specific version of >> _mesa_add_state_reference() for internal state vars. >> >> No matter how this is done, the driver needs to call add_parameter() to >> safely add to the parameter list. >> >> Would it be all right if I made add_parameter non static? > > You can't use _mesa_add_state_reference()? Perhaps a new > _mesa_add_driver_state() function otherwise? > Turns out I was just being silly, yes _mesa_add_driver_state() will do just fine. > >>> Then, during state validation in the driver you can load/update any >>> parameters you might have added. >> >> I think it is a good idea. >> >> >>> In _mesa_fetch_state() we could change the default case for switch >>> (state[i]) to be silent when it sees any state tokens it doesn't >>> understand (rather than call _mesa_problem()). >> Provided it is only done for STATE_INTERNAL, it should be pretty safe. >> Also I was thinking adding something like STATE_INTERNAL_DRIVER or some >> such so the drivers have a safe place to start adding there own vars. >> >>> Does that sound feasible? >> yes, and it would be less intrusive. > > OK. I think the next step would be for you to post a patch with > whatever you need changed. Then we'll understand the details better. The program.[ch] changes should be straight forward, the only sore point is the new state index name (STATE_INTERNAL_DRIVER). The r300_state.c changes is incomplete, but shows how I intend to update the state vars. _mesa_add_driver_state() will be used to add them to the parameter list. Rune Petersen Index: r300_state.c === RCS file: /cvs/mesa/Mesa/src/mesa/drivers/dri/r300/r300_state.c,v retrieving revision 1.166 diff -u -r1.166 r300_state.c --- r300_state.c15 Oct 2006 18:22:28 - 1.166 +++ r300_state.c4 Nov 2006 20:07:14 - @@ -1045,6 +1045,50 @@ #endif } +static void r300FetchStateParameter(GLcontext *ctx, const enum state_index state[], + GLfloat *value) +{ + switch(state[0]) + { + case STATE_INTERNAL: + switch(state[1]) + { + case STATE_R300_WINDOW_DIMENSION: + break; + default:; + } + default:; + } +} + +/** + * Update R300's own internal state parameters. + * For now just STATE_R300_WINDOW_DIMENSION + */ +static void r300UpdateStateParameters(GLcontext * ctx, GLuint new_state) +{ + struct r300_vertex_program_cont *vpc; + struct gl_program_parameter_list *paramList; + GLuint i; + + if(!(new_state & _NEW_BUFFERS)) + return; + + vpc = (struct r300_vertex_program_cont *)ctx->VertexProgram._Current; + paramList = vpc->mesa_program.Base.Parameters; + + if (!paramList) + return; + + for (i = 0; i < paramList->NumParameters; i++) { + if (paramList->Parameters[i].Type == PROGRAM_STATE_VAR){ + r300FetchStateParameter(ctx, + paramList->Parameters[i].StateIndexes, + paramList->ParameterValues[i]); + } + } +} + /* = * Polygon state */ @@ -1813,6 +1872,9 @@ if (new_state & (_NEW_BUFFERS | _NEW_COLOR | _NEW_PIXEL)) { r300UpdateDrawBuffer(ctx); } + + r300UpdateStateParameters(ctx, new_state); + #ifndef CB_DPATH /* Go inefficiency! */ r300ResetHwState(r300); Index: program.c === RCS file: /cvs/mesa/Mesa/src/mesa/shader/program.c,v retrieving revision 1.59 diff -u -r1.59 program.c --- program.c 10 Oct 2006 22:45:50 - 1.59 +++ program.c 4 Nov 2006 19:03:48 - @@ -927,7 +927,9 @@ break; } default: - _mesa_problem(ctx, "Bad state switch in _mesa_fetch_state()"); + /* unknown state indexes are silently ignored + * should be handled by the driver. + */ return; } } @@ -1000,7 +1002,9 @@ case STATE_TEXRECT_SCALE: return _NEW_TEXTURE; default: - _mesa_problem(NULL, "unexpected int. state in make_state_flags()"); + /* unknown state indexes are silently ignored and + * no flag set, since it is handled by the driver. + */ return 0; } @@ -1272,7 +1276,7 @@ case STATE_INTERNAL: break; default: - _mesa_problem(NULL, "Invalid state in maka_state_string"); + _mesa_problem(NULL, "Invalid state in make_state_string"); break; } Index
Re: [r300] partly working fragment.position patch
Rune Petersen wrote: > Brian Paul wrote: >>It seems to me that in ctx->Driver.ProgramStringNotify() you can add any >>extra parameters you need to the program's parameters list. > > > I would prefer to make driver specific version of > _mesa_add_state_reference() for internal state vars. > > No matter how this is done, the driver needs to call add_parameter() to > safely add to the parameter list. > > Would it be all right if I made add_parameter non static? You can't use _mesa_add_state_reference()? Perhaps a new _mesa_add_driver_state() function otherwise? >>Then, during state validation in the driver you can load/update any >>parameters you might have added. > > > I think it is a good idea. > > >>In _mesa_fetch_state() we could change the default case for switch >>(state[i]) to be silent when it sees any state tokens it doesn't >>understand (rather than call _mesa_problem()). > > Provided it is only done for STATE_INTERNAL, it should be pretty safe. > Also I was thinking adding something like STATE_INTERNAL_DRIVER or some > such so the drivers have a safe place to start adding there own vars. > >>Does that sound feasible? > > yes, and it would be less intrusive. OK. I think the next step would be for you to post a patch with whatever you need changed. Then we'll understand the details better. -Brian - Using Tomcat but need to do more? Need to support web services, security? Get stuff done quickly with pre-integrated technology to make your job easier Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642 -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [r300] partly working fragment.position patch
Brian Paul wrote: > Rune Petersen wrote: >> Keith Whitwell wrote: >>> Rune Petersen wrote: >>> >>> Rune, >>> >>> I don't quite understand what you want to do here. Can you show me >>> the code you'd like to have (ignoring the ctx argument issue)? I >>> would have thought that we could determine the state statically and >>> just rely on the driver to set that state in ctx->NewState when >>> necessary. >>> >> >> >> I am trying to make generic state vars that the drivers can use. >> >> the way I read these functions: >> make_state_flags() - returns the state flags should trigger an update >> of the state var. >> >> _mesa_fetch_state() - fetches the state var. >> >> In order to make generic state vars. >> - I need to get the flags via a callback to the driver from >> make_state_flags(). >> >> I need to fetch the vars via a callback to the driver from >> _mesa_fetch_state(). >> >> >> make_state_flags() >> { >> . >> case STATE_INTERNAL: >> { >> switch (state[1]) { >> case STATE_NORMAL_SCALE: >> . >>break; >> case STATE_TEXRECT_SCALE: >> . >>break; >> case STATE_GENERIC1: >>assert(ctx->Driver.FetchGenericState); >>ctx->Driver.FetchGenericState(ctx, state, value); >>break; >> } >> } >> } >> >> _mesa_fetch_state() >> { >> . >> case STATE_INTERNAL: >> switch (state[1]) { >> case STATE_NORMAL_SCALE: >> return _NEW_MODELVIEW; >> case STATE_TEXRECT_SCALE: >> return _NEW_TEXTURE; >> case STATE_GENERIC1: >> assert(ctx->Driver.GetGenericStateFlags); >> return ctx->Driver.GetGenericStateFlags(state); >> } >> >> } > > It seems to me that in ctx->Driver.ProgramStringNotify() you can add any > extra parameters you need to the program's parameters list. I would prefer to make driver specific version of _mesa_add_state_reference() for internal state vars. No matter how this is done, the driver needs to call add_parameter() to safely add to the parameter list. Would it be all right if I made add_parameter non static? > > Then, during state validation in the driver you can load/update any > parameters you might have added. I think it is a good idea. > In _mesa_fetch_state() we could change the default case for switch > (state[i]) to be silent when it sees any state tokens it doesn't > understand (rather than call _mesa_problem()). Provided it is only done for STATE_INTERNAL, it should be pretty safe. Also I was thinking adding something like STATE_INTERNAL_DRIVER or some such so the drivers have a safe place to start adding there own vars. > > Does that sound feasible? yes, and it would be less intrusive. Rune Petersen - Using Tomcat but need to do more? Need to support web services, security? Get stuff done quickly with pre-integrated technology to make your job easier Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642 -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [r300] partly working fragment.position patch
Keith Whitwell wrote: > Rune Petersen wrote: >> Keith Whitwell wrote: >>> Rune Petersen wrote: Keith Whitwell wrote: > Roland Scheidegger wrote: >> Keith Whitwell wrote: >> I think Rune is rather refering to the fact that you can't change >> (not >> with "legal" means at least) the constant you got with >> _mesa_add_unnamed_constant. > Ah right. I missed that. > >> I think there exist at least 2 solutions for that. The clean way >> would >> probably be to add some more INTERNAL_STATE (like i965 driver >> uses) so >> you use _mesa_add_state_reference instead, in this case mesa's shader >> code would need to update program parameter based on the drawable >> information - I'm not sure if accessing a driver's drawable >> information there would get messy). The easier solution would >> probably >> be to just directly manipulate the ParameterValues entry associated >> with the constant you added, easy though it might be considered >> somewhat hackish. Just don't forget you not only have to update the >> "constant" within r300UpdateWindow (if the currently bound fp >> requires >> it), but also when the active fp is switched to another one (and make >> sure that a parameter upload is actually triggered if it not already >> is upon drawable changes). > I think the parameter approach is probably the right one. This would > require that there be a callback into the driver to get this state, > and > more importantly, the driver would have to set a bit in ctx->NewState > (perhaps _NEW_BUFFERS) to indicate that a statechange has occurred > which > would affect that internal state atom. Thank you. I've hit a bit of a problem: I was planning to have state flags returned from a callback make_state_flags(). something like: ctx->Driver.GetGenericStateFlags(state); The problem being that the context ctx is not a parameter in make_state_flags(). Is there smart way of solving this? >>> Rune, >>> >>> I don't quite understand what you want to do here. Can you show me >>> the code you'd like to have (ignoring the ctx argument issue)? I >>> would have thought that we could determine the state statically and >>> just rely on the driver to set that state in ctx->NewState when >>> necessary. >>> >> >> I am trying to make generic state vars that the drivers can use. >> >> the way I read these functions: >> make_state_flags() - returns the state flags should trigger an update >> of the state var. >> >> _mesa_fetch_state() - fetches the state var. >> >> In order to make generic state vars. >> - I need to get the flags via a callback to the driver from >> make_state_flags(). >> >> I need to fetch the vars via a callback to the driver from >> _mesa_fetch_state(). >> >> >> make_state_flags() >> { >> . >> case STATE_INTERNAL: >> { >> switch (state[1]) { >> case STATE_NORMAL_SCALE: >> . >>break; >> case STATE_TEXRECT_SCALE: >> . >>break; >> case STATE_GENERIC1: >>assert(ctx->Driver.FetchGenericState); >>ctx->Driver.FetchGenericState(ctx, state, value); >>break; >> } >> } >> } >> >> _mesa_fetch_state() >> { >> . >> case STATE_INTERNAL: >> switch (state[1]) { >> case STATE_NORMAL_SCALE: >> return _NEW_MODELVIEW; >> case STATE_TEXRECT_SCALE: >> return _NEW_TEXTURE; >> case STATE_GENERIC1: >> assert(ctx->Driver.GetGenericStateFlags); >> return ctx->Driver.GetGenericStateFlags(state); >> } >> >> } > > I guess what I'm wondering is whether the flags you want to put into the > driver as generics are actually things which are universal and should be > supported across Mesa and the other drivers - is it just stuff like > window position? I think it would be better to create a new > STATE_WINDOW_POSITION keyed off something like _NEW_BUFFERS for that. It > would still be the driver's responsibility to set _NEW_BUFFERS on window > position changes though. > That would solve the problem with the callback in make_state_flags(), but I was told that only the driver knows the window dimensions (not position =). So I'll still need to add a callback to _mesa_fetch_state(). Rune Petersen - Using Tomcat but need to do more? Need to support web services, security? Get stuff done quickly with pre-integrated technology to make your job easier Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642 -- ___ Dri-devel mailing list Dri-de
Re: [r300] partly working fragment.position patch
Rune Petersen wrote: > Keith Whitwell wrote: >> Rune Petersen wrote: >>> Keith Whitwell wrote: Roland Scheidegger wrote: > Keith Whitwell wrote: > I think Rune is rather refering to the fact that you can't change (not > with "legal" means at least) the constant you got with > _mesa_add_unnamed_constant. Ah right. I missed that. > I think there exist at least 2 solutions for that. The clean way would > probably be to add some more INTERNAL_STATE (like i965 driver uses) so > you use _mesa_add_state_reference instead, in this case mesa's shader > code would need to update program parameter based on the drawable > information - I'm not sure if accessing a driver's drawable > information there would get messy). The easier solution would probably > be to just directly manipulate the ParameterValues entry associated > with the constant you added, easy though it might be considered > somewhat hackish. Just don't forget you not only have to update the > "constant" within r300UpdateWindow (if the currently bound fp requires > it), but also when the active fp is switched to another one (and make > sure that a parameter upload is actually triggered if it not already > is upon drawable changes). I think the parameter approach is probably the right one. This would require that there be a callback into the driver to get this state, and more importantly, the driver would have to set a bit in ctx->NewState (perhaps _NEW_BUFFERS) to indicate that a statechange has occurred which would affect that internal state atom. >>> Thank you. >>> >>> >>> I've hit a bit of a problem: >>> I was planning to have state flags returned from a callback >>> make_state_flags(). >>> something like: >>> ctx->Driver.GetGenericStateFlags(state); >>> >>> The problem being that the context ctx is not a parameter in >>> make_state_flags(). >>> >>> Is there smart way of solving this? >> Rune, >> >> I don't quite understand what you want to do here. Can you show me the >> code you'd like to have (ignoring the ctx argument issue)? I would have >> thought that we could determine the state statically and just rely on >> the driver to set that state in ctx->NewState when necessary. >> > > I am trying to make generic state vars that the drivers can use. > > the way I read these functions: > make_state_flags() - returns the state flags should trigger an update > of the state var. > > _mesa_fetch_state() - fetches the state var. > > In order to make generic state vars. > - I need to get the flags via a callback to the driver from > make_state_flags(). > > I need to fetch the vars via a callback to the driver from > _mesa_fetch_state(). > > > make_state_flags() > { > . > case STATE_INTERNAL: > { > switch (state[1]) { > case STATE_NORMAL_SCALE: > . >break; > case STATE_TEXRECT_SCALE: > . >break; > case STATE_GENERIC1: >assert(ctx->Driver.FetchGenericState); >ctx->Driver.FetchGenericState(ctx, state, value); >break; > } > } > } > > _mesa_fetch_state() > { > . > case STATE_INTERNAL: > switch (state[1]) { > case STATE_NORMAL_SCALE: > return _NEW_MODELVIEW; > case STATE_TEXRECT_SCALE: > return _NEW_TEXTURE; > case STATE_GENERIC1: > assert(ctx->Driver.GetGenericStateFlags); > return ctx->Driver.GetGenericStateFlags(state); > } > > } I guess what I'm wondering is whether the flags you want to put into the driver as generics are actually things which are universal and should be supported across Mesa and the other drivers - is it just stuff like window position? I think it would be better to create a new STATE_WINDOW_POSITION keyed off something like _NEW_BUFFERS for that. It would still be the driver's responsibility to set _NEW_BUFFERS on window position changes though. Keith - Using Tomcat but need to do more? Need to support web services, security? Get stuff done quickly with pre-integrated technology to make your job easier Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642 -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [r300] partly working fragment.position patch
Rune Petersen wrote: > Keith Whitwell wrote: > >>Rune Petersen wrote: >> >>>Keith Whitwell wrote: >>> Roland Scheidegger wrote: >Keith Whitwell wrote: >I think Rune is rather refering to the fact that you can't change (not >with "legal" means at least) the constant you got with >_mesa_add_unnamed_constant. Ah right. I missed that. >I think there exist at least 2 solutions for that. The clean way would >probably be to add some more INTERNAL_STATE (like i965 driver uses) so >you use _mesa_add_state_reference instead, in this case mesa's shader >code would need to update program parameter based on the drawable >information - I'm not sure if accessing a driver's drawable >information there would get messy). The easier solution would probably >be to just directly manipulate the ParameterValues entry associated >with the constant you added, easy though it might be considered >somewhat hackish. Just don't forget you not only have to update the >"constant" within r300UpdateWindow (if the currently bound fp requires >it), but also when the active fp is switched to another one (and make >sure that a parameter upload is actually triggered if it not already >is upon drawable changes). I think the parameter approach is probably the right one. This would require that there be a callback into the driver to get this state, and more importantly, the driver would have to set a bit in ctx->NewState (perhaps _NEW_BUFFERS) to indicate that a statechange has occurred which would affect that internal state atom. >>> >>>Thank you. >>> >>> >>>I've hit a bit of a problem: >>>I was planning to have state flags returned from a callback >>>make_state_flags(). >>>something like: >>>ctx->Driver.GetGenericStateFlags(state); >>> >>>The problem being that the context ctx is not a parameter in >>>make_state_flags(). >>> >>>Is there smart way of solving this? >> >>Rune, >> >>I don't quite understand what you want to do here. Can you show me the >>code you'd like to have (ignoring the ctx argument issue)? I would have >>thought that we could determine the state statically and just rely on >>the driver to set that state in ctx->NewState when necessary. >> > > > I am trying to make generic state vars that the drivers can use. > > the way I read these functions: > make_state_flags() - returns the state flags should trigger an update > of the state var. > > _mesa_fetch_state() - fetches the state var. > > In order to make generic state vars. > - I need to get the flags via a callback to the driver from > make_state_flags(). > > I need to fetch the vars via a callback to the driver from > _mesa_fetch_state(). > > > make_state_flags() > { > . > case STATE_INTERNAL: > { > switch (state[1]) { > case STATE_NORMAL_SCALE: > . >break; > case STATE_TEXRECT_SCALE: > . >break; > case STATE_GENERIC1: >assert(ctx->Driver.FetchGenericState); >ctx->Driver.FetchGenericState(ctx, state, value); >break; > } > } > } > > _mesa_fetch_state() > { > . > case STATE_INTERNAL: > switch (state[1]) { > case STATE_NORMAL_SCALE: > return _NEW_MODELVIEW; > case STATE_TEXRECT_SCALE: > return _NEW_TEXTURE; > case STATE_GENERIC1: > assert(ctx->Driver.GetGenericStateFlags); > return ctx->Driver.GetGenericStateFlags(state); > } > > } It seems to me that in ctx->Driver.ProgramStringNotify() you can add any extra parameters you need to the program's parameters list. Then, during state validation in the driver you can load/update any parameters you might have added. In _mesa_fetch_state() we could change the default case for switch (state[i]) to be silent when it sees any state tokens it doesn't understand (rather than call _mesa_problem()). Does that sound feasible? -Brian - Using Tomcat but need to do more? Need to support web services, security? Get stuff done quickly with pre-integrated technology to make your job easier Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642 -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [r300] partly working fragment.position patch
Keith Whitwell wrote: > Rune Petersen wrote: >> Keith Whitwell wrote: >>> Roland Scheidegger wrote: Keith Whitwell wrote: I think Rune is rather refering to the fact that you can't change (not with "legal" means at least) the constant you got with _mesa_add_unnamed_constant. >>> Ah right. I missed that. >>> I think there exist at least 2 solutions for that. The clean way would probably be to add some more INTERNAL_STATE (like i965 driver uses) so you use _mesa_add_state_reference instead, in this case mesa's shader code would need to update program parameter based on the drawable information - I'm not sure if accessing a driver's drawable information there would get messy). The easier solution would probably be to just directly manipulate the ParameterValues entry associated with the constant you added, easy though it might be considered somewhat hackish. Just don't forget you not only have to update the "constant" within r300UpdateWindow (if the currently bound fp requires it), but also when the active fp is switched to another one (and make sure that a parameter upload is actually triggered if it not already is upon drawable changes). >>> I think the parameter approach is probably the right one. This would >>> require that there be a callback into the driver to get this state, and >>> more importantly, the driver would have to set a bit in ctx->NewState >>> (perhaps _NEW_BUFFERS) to indicate that a statechange has occurred which >>> would affect that internal state atom. >> Thank you. >> >> >> I've hit a bit of a problem: >> I was planning to have state flags returned from a callback >> make_state_flags(). >> something like: >> ctx->Driver.GetGenericStateFlags(state); >> >> The problem being that the context ctx is not a parameter in >> make_state_flags(). >> >> Is there smart way of solving this? > > Rune, > > I don't quite understand what you want to do here. Can you show me the > code you'd like to have (ignoring the ctx argument issue)? I would have > thought that we could determine the state statically and just rely on > the driver to set that state in ctx->NewState when necessary. > I am trying to make generic state vars that the drivers can use. the way I read these functions: make_state_flags() - returns the state flags should trigger an update of the state var. _mesa_fetch_state() - fetches the state var. In order to make generic state vars. - I need to get the flags via a callback to the driver from make_state_flags(). I need to fetch the vars via a callback to the driver from _mesa_fetch_state(). make_state_flags() { . case STATE_INTERNAL: { switch (state[1]) { case STATE_NORMAL_SCALE: . break; case STATE_TEXRECT_SCALE: . break; case STATE_GENERIC1: assert(ctx->Driver.FetchGenericState); ctx->Driver.FetchGenericState(ctx, state, value); break; } } } _mesa_fetch_state() { . case STATE_INTERNAL: switch (state[1]) { case STATE_NORMAL_SCALE: return _NEW_MODELVIEW; case STATE_TEXRECT_SCALE: return _NEW_TEXTURE; case STATE_GENERIC1: assert(ctx->Driver.GetGenericStateFlags); return ctx->Driver.GetGenericStateFlags(state); } } Rune Petersen - Using Tomcat but need to do more? Need to support web services, security? Get stuff done quickly with pre-integrated technology to make your job easier Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642 -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [r300] partly working fragment.position patch
Rune Petersen wrote: > Keith Whitwell wrote: >> Roland Scheidegger wrote: >>> Keith Whitwell wrote: >>> I think Rune is rather refering to the fact that you can't change (not >>> with "legal" means at least) the constant you got with >>> _mesa_add_unnamed_constant. >> Ah right. I missed that. >> >>> I think there exist at least 2 solutions for that. The clean way would >>> probably be to add some more INTERNAL_STATE (like i965 driver uses) so >>> you use _mesa_add_state_reference instead, in this case mesa's shader >>> code would need to update program parameter based on the drawable >>> information - I'm not sure if accessing a driver's drawable >>> information there would get messy). The easier solution would probably >>> be to just directly manipulate the ParameterValues entry associated >>> with the constant you added, easy though it might be considered >>> somewhat hackish. Just don't forget you not only have to update the >>> "constant" within r300UpdateWindow (if the currently bound fp requires >>> it), but also when the active fp is switched to another one (and make >>> sure that a parameter upload is actually triggered if it not already >>> is upon drawable changes). >> I think the parameter approach is probably the right one. This would >> require that there be a callback into the driver to get this state, and >> more importantly, the driver would have to set a bit in ctx->NewState >> (perhaps _NEW_BUFFERS) to indicate that a statechange has occurred which >> would affect that internal state atom. > > Thank you. > > > I've hit a bit of a problem: > I was planning to have state flags returned from a callback > make_state_flags(). > something like: > ctx->Driver.GetGenericStateFlags(state); > > The problem being that the context ctx is not a parameter in > make_state_flags(). > > Is there smart way of solving this? Rune, I don't quite understand what you want to do here. Can you show me the code you'd like to have (ignoring the ctx argument issue)? I would have thought that we could determine the state statically and just rely on the driver to set that state in ctx->NewState when necessary. Keith - Using Tomcat but need to do more? Need to support web services, security? Get stuff done quickly with pre-integrated technology to make your job easier Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642 -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [r300] partly working fragment.position patch
Keith Whitwell wrote: > Roland Scheidegger wrote: >> Keith Whitwell wrote: >> I think Rune is rather refering to the fact that you can't change (not >> with "legal" means at least) the constant you got with >> _mesa_add_unnamed_constant. > > Ah right. I missed that. > >> I think there exist at least 2 solutions for that. The clean way would >> probably be to add some more INTERNAL_STATE (like i965 driver uses) so >> you use _mesa_add_state_reference instead, in this case mesa's shader >> code would need to update program parameter based on the drawable >> information - I'm not sure if accessing a driver's drawable >> information there would get messy). The easier solution would probably >> be to just directly manipulate the ParameterValues entry associated >> with the constant you added, easy though it might be considered >> somewhat hackish. Just don't forget you not only have to update the >> "constant" within r300UpdateWindow (if the currently bound fp requires >> it), but also when the active fp is switched to another one (and make >> sure that a parameter upload is actually triggered if it not already >> is upon drawable changes). > > I think the parameter approach is probably the right one. This would > require that there be a callback into the driver to get this state, and > more importantly, the driver would have to set a bit in ctx->NewState > (perhaps _NEW_BUFFERS) to indicate that a statechange has occurred which > would affect that internal state atom. Thank you. I've hit a bit of a problem: I was planning to have state flags returned from a callback make_state_flags(). something like: ctx->Driver.GetGenericStateFlags(state); The problem being that the context ctx is not a parameter in make_state_flags(). Is there smart way of solving this? Rune Petersen - Using Tomcat but need to do more? Need to support web services, security? Get stuff done quickly with pre-integrated technology to make your job easier Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642 -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [r300] partly working fragment.position patch
Roland Scheidegger wrote: > Keith Whitwell wrote: >>> Now I remember why I can't use radeon->dri.drawable, at least not >>> directly when the shader code is added: >>> >>> When the window size changes the constants have to be updated. >>> >>> Is there a way for the driver to update a constant after construction? >>> >> >> This is an age-old dilemma... The i965 driver gets around this by >> locking the hardware before validating and emitting state and drawing >> commands and unlocking again afterwards - so the window can't change >> size in the meantime. Other drivers tend to just deal with the >> occasional incorrectness. >> >> In general this is something we need to get a bit better at. API's >> like DX9 and GL/ES do away with frontbuffer rendering which gives the >> drivers a lot more flexibility in terms of dealing with window moves >> and resizes, allowing them to pick a time to respond to a resize. >> With private backbuffers we might get the same benefits at least in >> the common case. > > I think Rune is rather refering to the fact that you can't change (not > with "legal" means at least) the constant you got with > _mesa_add_unnamed_constant. Ah right. I missed that. > I think there exist at least 2 solutions for that. The clean way would > probably be to add some more INTERNAL_STATE (like i965 driver uses) so > you use _mesa_add_state_reference instead, in this case mesa's shader > code would need to update program parameter based on the drawable > information - I'm not sure if accessing a driver's drawable information > there would get messy). The easier solution would probably be to just > directly manipulate the ParameterValues entry associated with the > constant you added, easy though it might be considered somewhat hackish. > Just don't forget you not only have to update the "constant" within > r300UpdateWindow (if the currently bound fp requires it), but also when > the active fp is switched to another one (and make sure that a parameter > upload is actually triggered if it not already is upon drawable changes). I think the parameter approach is probably the right one. This would require that there be a callback into the driver to get this state, and more importantly, the driver would have to set a bit in ctx->NewState (perhaps _NEW_BUFFERS) to indicate that a statechange has occurred which would affect that internal state atom. Keith - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys -- and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [r300] partly working fragment.position patch
Keith Whitwell wrote: >> Now I remember why I can't use radeon->dri.drawable, at least not >> directly when the shader code is added: >> >> When the window size changes the constants have to be updated. >> >> Is there a way for the driver to update a constant after construction? >> > > This is an age-old dilemma... The i965 driver gets around this by > locking the hardware before validating and emitting state and drawing > commands and unlocking again afterwards - so the window can't change > size in the meantime. Other drivers tend to just deal with the > occasional incorrectness. > > In general this is something we need to get a bit better at. API's like > DX9 and GL/ES do away with frontbuffer rendering which gives the drivers > a lot more flexibility in terms of dealing with window moves and > resizes, allowing them to pick a time to respond to a resize. With > private backbuffers we might get the same benefits at least in the > common case. I think Rune is rather refering to the fact that you can't change (not with "legal" means at least) the constant you got with _mesa_add_unnamed_constant. I think there exist at least 2 solutions for that. The clean way would probably be to add some more INTERNAL_STATE (like i965 driver uses) so you use _mesa_add_state_reference instead, in this case mesa's shader code would need to update program parameter based on the drawable information - I'm not sure if accessing a driver's drawable information there would get messy). The easier solution would probably be to just directly manipulate the ParameterValues entry associated with the constant you added, easy though it might be considered somewhat hackish. Just don't forget you not only have to update the "constant" within r300UpdateWindow (if the currently bound fp requires it), but also when the active fp is switched to another one (and make sure that a parameter upload is actually triggered if it not already is upon drawable changes). Roland - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys -- and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [r300] partly working fragment.position patch
Rune Petersen wrote: > Keith Whitwell wrote: >> Rune Petersen wrote: >>> Rune Petersen wrote: Roland Scheidegger wrote: > Rune Petersen wrote: >> I hit a problem constructing this: >> >> - In order to do range mapping in the vertex shader (if I so choose) >> I will need a constant (0.5), but how to add it? > I think this might work similar to what is used for position invariant > programs, instead of using _mesa_add_state_reference you could try > _mesa_add_named_parameter. Otherwise, you could always "construct" 0.5 > in the shader itself, since you always have the constants 0 and 1 > available thanks to the powerful swizzling capabilities, though > surprsingly it seems somewhat complicated. Either use 2 instructions > (ADD 1+1, RCP). Or try EX2/EXP though I'm not sure about performance of > these, but I guess the approximated EXP should do (2^-1). > This math in this patch appear sound. the doom3-demo issue appear unrelated to fragment.position. This version makes use of existing instructions to calculate result.position. split into 2 parts: - select_vertex_shader changes - The actual fragment.position changes This patch assumes that: - That the temp used to calculate result.position is safe to use (true for std. use). - That fragment.position.x & y wont be used (mostly true, except for exotic programs.) In order to fix this, I'll need to know the window size, but how? >> Surely it's right there in radeon->dri.drawable ? >> > > Now I remember why I can't use radeon->dri.drawable, at least not > directly when the shader code is added: > > When the window size changes the constants have to be updated. > > Is there a way for the driver to update a constant after construction? > This is an age-old dilemma... The i965 driver gets around this by locking the hardware before validating and emitting state and drawing commands and unlocking again afterwards - so the window can't change size in the meantime. Other drivers tend to just deal with the occasional incorrectness. In general this is something we need to get a bit better at. API's like DX9 and GL/ES do away with frontbuffer rendering which gives the drivers a lot more flexibility in terms of dealing with window moves and resizes, allowing them to pick a time to respond to a resize. With private backbuffers we might get the same benefits at least in the common case. Keith Keith - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys -- and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [r300] partly working fragment.position patch
Keith Whitwell wrote: > Rune Petersen wrote: >> Rune Petersen wrote: >>> Roland Scheidegger wrote: Rune Petersen wrote: > I hit a problem constructing this: > > - In order to do range mapping in the vertex shader (if I so choose) > I will need a constant (0.5), but how to add it? I think this might work similar to what is used for position invariant programs, instead of using _mesa_add_state_reference you could try _mesa_add_named_parameter. Otherwise, you could always "construct" 0.5 in the shader itself, since you always have the constants 0 and 1 available thanks to the powerful swizzling capabilities, though surprsingly it seems somewhat complicated. Either use 2 instructions (ADD 1+1, RCP). Or try EX2/EXP though I'm not sure about performance of these, but I guess the approximated EXP should do (2^-1). >>> This math in this patch appear sound. the doom3-demo issue appear >>> unrelated to fragment.position. >>> >>> This version makes use of existing instructions to calculate >>> result.position. >>> >>> split into 2 parts: >>> - select_vertex_shader changes >>> - The actual fragment.position changes >>> >>> This patch assumes that: >>> >>> - That the temp used to calculate result.position is safe to use (true >>> for std. use). >>> >>> - That fragment.position.x & y wont be used (mostly true, except for >>> exotic programs.) >>>In order to fix this, I'll need to know the window size, but how? > > Surely it's right there in radeon->dri.drawable ? > Now I remember why I can't use radeon->dri.drawable, at least not directly when the shader code is added: When the window size changes the constants have to be updated. Is there a way for the driver to update a constant after construction? Rune Petersen - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys -- and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [r300] partly working fragment.position patch
Rune Petersen wrote: > Keith Whitwell wrote: >> Rune Petersen wrote: >>> Rune Petersen wrote: Roland Scheidegger wrote: > Rune Petersen wrote: >> I hit a problem constructing this: >> >> - In order to do range mapping in the vertex shader (if I so choose) >> I will need a constant (0.5), but how to add it? > I think this might work similar to what is used for position invariant > programs, instead of using _mesa_add_state_reference you could try > _mesa_add_named_parameter. Otherwise, you could always "construct" 0.5 > in the shader itself, since you always have the constants 0 and 1 > available thanks to the powerful swizzling capabilities, though > surprsingly it seems somewhat complicated. Either use 2 instructions > (ADD 1+1, RCP). Or try EX2/EXP though I'm not sure about performance of > these, but I guess the approximated EXP should do (2^-1). > This math in this patch appear sound. the doom3-demo issue appear unrelated to fragment.position. This version makes use of existing instructions to calculate result.position. split into 2 parts: - select_vertex_shader changes - The actual fragment.position changes This patch assumes that: - That the temp used to calculate result.position is safe to use (true for std. use). - That fragment.position.x & y wont be used (mostly true, except for exotic programs.) In order to fix this, I'll need to know the window size, but how? >> Surely it's right there in radeon->dri.drawable ? >> >> > Sure it's is, I just managed to miss it, I'm still new at this =) > > It is pretty solid now. position x & y are correct. And no regressions > > After some extensive testing, I found that a doom3-demo vertex shader > and the select_vertex_shader code uncovered a bug in the vertex shader. > > We can't output result.* unless the fragment shader expects the input. > The fix is to change the unused outputs to an unused temp. > Of cause I manage to attach the right patch Rune Petersen ? depend ? server Index: r300_context.h === RCS file: /cvs/mesa/Mesa/src/mesa/drivers/dri/r300/r300_context.h,v retrieving revision 1.104 diff -u -r1.104 r300_context.h --- r300_context.h 12 Sep 2006 18:34:43 - 1.104 +++ r300_context.h 7 Oct 2006 12:55:47 - @@ -592,7 +592,8 @@ extern int hw_tcl_on; -#define CURRENT_VERTEX_SHADER(ctx) (ctx->VertexProgram._Current) +//#define CURRENT_VERTEX_SHADER(ctx) (ctx->VertexProgram._Current) +#define CURRENT_VERTEX_SHADER(ctx) (R300_CONTEXT(ctx)->selected_vp) /* Should but doesnt work */ //#define CURRENT_VERTEX_SHADER(ctx) (R300_CONTEXT(ctx)->curr_vp) @@ -607,12 +608,18 @@ /* r300_vertex_shader_state and r300_vertex_program should probably be merged together someday. * Keeping them them seperate for now should ensure fixed pipeline keeps functioning properly. */ + +struct r300_vertex_program_key { + GLuint InputsRead; + GLuint OutputsWritten; +}; + struct r300_vertex_program { - struct gl_vertex_program mesa_program; /* Must be first */ + struct r300_vertex_program *next; + struct r300_vertex_program_key key; int translated; struct r300_vertex_shader_fragment program; - struct r300_vertex_shader_fragment params; int pos_end; int num_temporaries; /* Number of temp vars used by program */ @@ -623,6 +630,12 @@ int use_ref_count; }; +struct r300_vertex_program_cont { + struct gl_vertex_program mesa_program; /* Must be first */ + struct r300_vertex_shader_fragment params; + struct r300_vertex_program *progs; +}; + #define PFS_MAX_ALU_INST 64 #define PFS_MAX_TEX_INST 64 #define PFS_MAX_TEX_INDIRECT 4 @@ -797,6 +810,7 @@ struct r300_cmdbuf cmdbuf; struct r300_state state; struct gl_vertex_program *curr_vp; + struct r300_vertex_program *selected_vp; /* Vertex buffers */ @@ -854,9 +868,9 @@ extern int r300_get_num_verts(r300ContextPtr rmesa, int num_verts, int prim); -void r300_translate_vertex_shader(struct r300_vertex_program *vp); +extern void r300_select_vertex_shader(r300ContextPtr r300); extern void r300InitShaderFuncs(struct dd_function_table *functions); -extern int r300VertexProgUpdateParams(GLcontext *ctx, struct r300_vertex_program *vp, float *dst); +extern int r300VertexProgUpdateParams(GLcontext *ctx, struct r300_vertex_program_cont *vp, float *dst); extern int r300Fallback(GLcontext *ctx); extern void radeon_vb_to_rvb(r300ContextPtr rmesa, struct radeon_vertex_buffer *rvb, struct vertex_buffer *vb); Index: r300_maos.c === RCS file: /cvs/mesa/Mesa/src/mesa/drivers/dri/r300/r300_maos.c,v retrieving revision 1.39 diff -u -r1.39 r300
Re: [r300] partly working fragment.position patch
Keith Whitwell wrote: > Rune Petersen wrote: >> Rune Petersen wrote: >>> Roland Scheidegger wrote: Rune Petersen wrote: > I hit a problem constructing this: > > - In order to do range mapping in the vertex shader (if I so choose) > I will need a constant (0.5), but how to add it? I think this might work similar to what is used for position invariant programs, instead of using _mesa_add_state_reference you could try _mesa_add_named_parameter. Otherwise, you could always "construct" 0.5 in the shader itself, since you always have the constants 0 and 1 available thanks to the powerful swizzling capabilities, though surprsingly it seems somewhat complicated. Either use 2 instructions (ADD 1+1, RCP). Or try EX2/EXP though I'm not sure about performance of these, but I guess the approximated EXP should do (2^-1). >>> This math in this patch appear sound. the doom3-demo issue appear >>> unrelated to fragment.position. >>> >>> This version makes use of existing instructions to calculate >>> result.position. >>> >>> split into 2 parts: >>> - select_vertex_shader changes >>> - The actual fragment.position changes >>> >>> This patch assumes that: >>> >>> - That the temp used to calculate result.position is safe to use (true >>> for std. use). >>> >>> - That fragment.position.x & y wont be used (mostly true, except for >>> exotic programs.) >>>In order to fix this, I'll need to know the window size, but how? > > Surely it's right there in radeon->dri.drawable ? > > Sure it's is, I just managed to miss it, I'm still new at this =) It is pretty solid now. position x & y are correct. And no regressions After some extensive testing, I found that a doom3-demo vertex shader and the select_vertex_shader code uncovered a bug in the vertex shader. We can't output result.* unless the fragment shader expects the input. The fix is to change the unused outputs to an unused temp. Rune Petersen diff -Naur -x '*.o' -x '*.so' -x Entries -x depend -x depend.bak r300-dev/r300_context.h r300-dev2/r300_context.h --- r300-dev/r300_context.h 2006-09-19 18:52:45.0 +0200 +++ r300-dev2/r300_context.h2006-10-06 22:00:40.0 +0200 @@ -623,6 +623,8 @@ int pos_end; int num_temporaries; /* Number of temp vars used by program */ + int wpos_idx; + const __DRIdrawablePrivate * dPriv; int inputs[VERT_ATTRIB_MAX]; int outputs[VERT_RESULT_MAX]; int native; diff -Naur -x '*.o' -x '*.so' -x Entries -x depend -x depend.bak r300-dev/r300_fragprog.c r300-dev2/r300_fragprog.c --- r300-dev/r300_fragprog.c2006-09-17 13:49:38.0 +0200 +++ r300-dev2/r300_fragprog.c 2006-10-05 18:59:57.0 +0200 @@ -1400,6 +1400,13 @@ } InputsRead &= ~FRAG_BITS_TEX_ANY; + /* fragment position treated as a texcoord */ + if (InputsRead & FRAG_BIT_WPOS) { + cs->inputs[FRAG_ATTRIB_WPOS].refcount = 0; + cs->inputs[FRAG_ATTRIB_WPOS].reg = get_hw_temp(rp); + } + InputsRead &= ~FRAG_BIT_WPOS; + /* Then primary colour */ if (InputsRead & FRAG_BIT_COL0) { cs->inputs[FRAG_ATTRIB_COL0].refcount = 0; diff -Naur -x '*.o' -x '*.so' -x Entries -x depend -x depend.bak r300-dev/r300_state.c r300-dev2/r300_state.c --- r300-dev/r300_state.c 2006-09-17 13:49:36.0 +0200 +++ r300-dev2/r300_state.c 2006-10-04 22:58:14.0 +0200 @@ -1305,6 +1305,20 @@ r300->hw.rr.cmd[R300_RR_ROUTE_1] = 0; + if (InputsRead & FRAG_BIT_WPOS){ + for (i = 0; i < ctx->Const.MaxTextureUnits; i++) + if (!(InputsRead & (FRAG_BIT_TEX0 << i))) + break; + + if(i == ctx->Const.MaxTextureUnits){ + fprintf(stderr, "\tno free texcoord found...\n"); + exit(0); + } + + InputsRead |= (FRAG_BIT_TEX0 << i); + InputsRead &= ~FRAG_BIT_WPOS; + } + for (i=0;iConst.MaxTextureUnits;i++) { r300->hw.ri.cmd[R300_RI_INTERP_0+i] = 0 | R300_RS_INTERP_USED diff -Naur -x '*.o' -x '*.so' -x Entries -x depend -x depend.bak r300-dev/r300_vertexprog.c r300-dev2/r300_vertexprog.c --- r300-dev/r300_vertexprog.c 2006-10-07 14:10:14.0 +0200 +++ r300-dev2/r300_vertexprog.c 2006-10-07 14:10:23.0 +0200 @@ -955,17 +955,160 @@ assert(vpi->Opcode == OPCODE_END); } +static void insert_wpos(struct r300_vertex_program *vp, + struct gl_program *prog, + GLint pos) +{ + struct prog_instruction *vpi; + struct prog_instruction *vpi_insert; + GLuint temp_index; + GLfloat window_values[4]; + GLuint const_window_index; + int i = 0; + + vpi = malloc((prog->NumInstructions + 5) * sizeof(struct prog
Re: [r300] partly working fragment.position patch
Rune Petersen wrote: > Rune Petersen wrote: >> Roland Scheidegger wrote: >>> Rune Petersen wrote: I hit a problem constructing this: - In order to do range mapping in the vertex shader (if I so choose) I will need a constant (0.5), but how to add it? >>> I think this might work similar to what is used for position invariant >>> programs, instead of using _mesa_add_state_reference you could try >>> _mesa_add_named_parameter. Otherwise, you could always "construct" 0.5 >>> in the shader itself, since you always have the constants 0 and 1 >>> available thanks to the powerful swizzling capabilities, though >>> surprsingly it seems somewhat complicated. Either use 2 instructions >>> (ADD 1+1, RCP). Or try EX2/EXP though I'm not sure about performance of >>> these, but I guess the approximated EXP should do (2^-1). >>> >> This math in this patch appear sound. the doom3-demo issue appear >> unrelated to fragment.position. >> >> This version makes use of existing instructions to calculate >> result.position. >> >> split into 2 parts: >> - select_vertex_shader changes >> - The actual fragment.position changes >> >> This patch assumes that: >> >> - That the temp used to calculate result.position is safe to use (true >> for std. use). >> >> - That fragment.position.x & y wont be used (mostly true, except for >> exotic programs.) >>In order to fix this, I'll need to know the window size, but how? Surely it's right there in radeon->dri.drawable ? Keith - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys -- and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [r300] partly working fragment.position patch
Rune Petersen wrote: > Roland Scheidegger wrote: >> Rune Petersen wrote: >>> I hit a problem constructing this: >>> >>> - In order to do range mapping in the vertex shader (if I so choose) >>> I will need a constant (0.5), but how to add it? >> I think this might work similar to what is used for position invariant >> programs, instead of using _mesa_add_state_reference you could try >> _mesa_add_named_parameter. Otherwise, you could always "construct" 0.5 >> in the shader itself, since you always have the constants 0 and 1 >> available thanks to the powerful swizzling capabilities, though >> surprsingly it seems somewhat complicated. Either use 2 instructions >> (ADD 1+1, RCP). Or try EX2/EXP though I'm not sure about performance of >> these, but I guess the approximated EXP should do (2^-1). >> > > This math in this patch appear sound. the doom3-demo issue appear > unrelated to fragment.position. > > This version makes use of existing instructions to calculate > result.position. > > split into 2 parts: > - select_vertex_shader changes > - The actual fragment.position changes > > This patch assumes that: > > - That the temp used to calculate result.position is safe to use (true > for std. use). > > - That fragment.position.x & y wont be used (mostly true, except for > exotic programs.) >In order to fix this, I'll need to know the window size, but how? > > > - That select_vertex_shader changes (by Aapo Tahkola) is acceptable for > the r300 driver. > > > Rune Petersen > oops.. the select_vertex_shader patch included unrelated changes new version attached. Rune Petersen ? server Index: r300_context.h === RCS file: /cvs/mesa/Mesa/src/mesa/drivers/dri/r300/r300_context.h,v retrieving revision 1.104 diff -u -r1.104 r300_context.h --- r300_context.h 12 Sep 2006 18:34:43 - 1.104 +++ r300_context.h 5 Oct 2006 19:10:19 - @@ -592,7 +592,8 @@ extern int hw_tcl_on; -#define CURRENT_VERTEX_SHADER(ctx) (ctx->VertexProgram._Current) +//#define CURRENT_VERTEX_SHADER(ctx) (ctx->VertexProgram._Current) +#define CURRENT_VERTEX_SHADER(ctx) (R300_CONTEXT(ctx)->selected_vp) /* Should but doesnt work */ //#define CURRENT_VERTEX_SHADER(ctx) (R300_CONTEXT(ctx)->curr_vp) @@ -607,12 +608,18 @@ /* r300_vertex_shader_state and r300_vertex_program should probably be merged together someday. * Keeping them them seperate for now should ensure fixed pipeline keeps functioning properly. */ + +struct r300_vertex_program_key { + GLuint InputsRead; + GLuint OutputsWritten; +}; + struct r300_vertex_program { - struct gl_vertex_program mesa_program; /* Must be first */ + struct r300_vertex_program *next; + struct r300_vertex_program_key key; int translated; struct r300_vertex_shader_fragment program; - struct r300_vertex_shader_fragment params; int pos_end; int num_temporaries; /* Number of temp vars used by program */ @@ -623,6 +630,12 @@ int use_ref_count; }; +struct r300_vertex_program_cont { + struct gl_vertex_program mesa_program; /* Must be first */ + struct r300_vertex_shader_fragment params; + struct r300_vertex_program *progs; +}; + #define PFS_MAX_ALU_INST 64 #define PFS_MAX_TEX_INST 64 #define PFS_MAX_TEX_INDIRECT 4 @@ -797,6 +810,7 @@ struct r300_cmdbuf cmdbuf; struct r300_state state; struct gl_vertex_program *curr_vp; + struct r300_vertex_program *selected_vp; /* Vertex buffers */ @@ -854,9 +868,9 @@ extern int r300_get_num_verts(r300ContextPtr rmesa, int num_verts, int prim); -void r300_translate_vertex_shader(struct r300_vertex_program *vp); +extern void r300_select_vertex_shader(r300ContextPtr r300); extern void r300InitShaderFuncs(struct dd_function_table *functions); -extern int r300VertexProgUpdateParams(GLcontext *ctx, struct r300_vertex_program *vp, float *dst); +extern int r300VertexProgUpdateParams(GLcontext *ctx, struct r300_vertex_program_cont *vp, float *dst); extern int r300Fallback(GLcontext *ctx); extern void radeon_vb_to_rvb(r300ContextPtr rmesa, struct radeon_vertex_buffer *rvb, struct vertex_buffer *vb); Index: r300_maos.c === RCS file: /cvs/mesa/Mesa/src/mesa/drivers/dri/r300/r300_maos.c,v retrieving revision 1.39 diff -u -r1.39 r300_maos.c --- r300_maos.c 14 Sep 2006 16:17:06 - 1.39 +++ r300_maos.c 5 Oct 2006 19:10:21 - @@ -407,8 +407,8 @@ if (hw_tcl_on) { struct r300_vertex_program *prog=(struct r300_vertex_program *)CURRENT_VERTEX_SHADER(ctx); inputs = prog->inputs; - InputsRead = CURRENT_VERTEX_SHADER(ctx)->Base.InputsRead; - OutputsWritten = CURRENT_VERTEX_SHADER(ctx)->Base.OutputsWritten; + InputsRead = CURRENT_
Re: [r300] partly working fragment.position patch
Roland Scheidegger wrote: > Rune Petersen wrote: >> I hit a problem constructing this: >> >> - In order to do range mapping in the vertex shader (if I so choose) >> I will need a constant (0.5), but how to add it? > I think this might work similar to what is used for position invariant > programs, instead of using _mesa_add_state_reference you could try > _mesa_add_named_parameter. Otherwise, you could always "construct" 0.5 > in the shader itself, since you always have the constants 0 and 1 > available thanks to the powerful swizzling capabilities, though > surprsingly it seems somewhat complicated. Either use 2 instructions > (ADD 1+1, RCP). Or try EX2/EXP though I'm not sure about performance of > these, but I guess the approximated EXP should do (2^-1). > This math in this patch appear sound. the doom3-demo issue appear unrelated to fragment.position. This version makes use of existing instructions to calculate result.position. split into 2 parts: - select_vertex_shader changes - The actual fragment.position changes This patch assumes that: - That the temp used to calculate result.position is safe to use (true for std. use). - That fragment.position.x & y wont be used (mostly true, except for exotic programs.) In order to fix this, I'll need to know the window size, but how? - That select_vertex_shader changes (by Aapo Tahkola) is acceptable for the r300 driver. Rune Petersen diff -Naur -x '*.o' -x '*.so' -x Entries -x depend r300-dev/r300_fragprog.c r300_fragprog.c --- r300_fragprog.c 2006-09-17 13:49:38.0 +0200 +++ r300_fragprog.c 2006-10-05 18:59:57.0 +0200 @@ -1400,6 +1400,13 @@ } InputsRead &= ~FRAG_BITS_TEX_ANY; + /* fragment position treated as a texcoord */ + if (InputsRead & FRAG_BIT_WPOS) { + cs->inputs[FRAG_ATTRIB_WPOS].refcount = 0; + cs->inputs[FRAG_ATTRIB_WPOS].reg = get_hw_temp(rp); + } + InputsRead &= ~FRAG_BIT_WPOS; + /* Then primary colour */ if (InputsRead & FRAG_BIT_COL0) { cs->inputs[FRAG_ATTRIB_COL0].refcount = 0; diff -Naur -x '*.o' -x '*.so' -x Entries -x depend r300_state.c r300_state.c --- r300_state.c2006-09-17 13:49:36.0 +0200 +++ r300_state.c2006-10-04 22:58:14.0 +0200 @@ -1305,6 +1305,20 @@ r300->hw.rr.cmd[R300_RR_ROUTE_1] = 0; + if (InputsRead & FRAG_BIT_WPOS){ + for (i = 0; i < ctx->Const.MaxTextureUnits; i++) + if (!(InputsRead & (FRAG_BIT_TEX0 << i))) + break; + + if(i == ctx->Const.MaxTextureUnits){ + fprintf(stderr, "\tno free texcoord found...\n"); + exit(0); + } + + InputsRead |= (FRAG_BIT_TEX0 << i); + InputsRead &= ~FRAG_BIT_WPOS; + } + for (i=0;iConst.MaxTextureUnits;i++) { r300->hw.ri.cmd[R300_RI_INTERP_0+i] = 0 | R300_RS_INTERP_USED diff -Naur -x '*.o' -x '*.so' -x Entries -x depend r300_vertexprog.c r300_vertexprog.c --- r300_vertexprog.c 2006-10-05 19:04:22.0 +0200 +++ r300_vertexprog.c 2006-10-05 19:05:10.0 +0200 @@ -950,6 +950,133 @@ //_mesa_print_program(&mesa_vp->Base); } +int wpos_idx; + +static void insert_pos(struct gl_program *prog, int pos) +{ + struct prog_instruction *vpi; + struct prog_instruction *vpi_insert; + GLuint temp_index, temp_index2; + int i = 0; + + vpi = malloc((prog->NumInstructions + 5) * sizeof(struct prog_instruction)); + memcpy(vpi, prog->Instructions, (pos+1) * sizeof(struct prog_instruction)); + + vpi_insert = &vpi[pos]; + + /* make a copy before outputting VERT_RESULT_HPOS */ + vpi_insert->DstReg.File = vpi_insert->SrcReg[2].File; + vpi_insert->DstReg.Index = temp_index = vpi_insert->SrcReg[2].Index; + + vpi_insert++; + memset(vpi_insert, 0, 5 * sizeof(struct prog_instruction)); + + vpi_insert[i].Opcode = OPCODE_MOV; + + vpi_insert[i].DstReg.File = PROGRAM_OUTPUT; + vpi_insert[i].DstReg.Index = VERT_RESULT_HPOS; + vpi_insert[i].DstReg.WriteMask = WRITEMASK_XYZW; + vpi_insert[i].DstReg.CondMask = COND_TR; + + vpi_insert[i].SrcReg[0].File = PROGRAM_TEMPORARY; + vpi_insert[i].SrcReg[0].Index = temp_index; + vpi_insert[i].SrcReg[0].Swizzle = MAKE_SWIZZLE4(SWIZZLE_X, SWIZZLE_Y, SWIZZLE_Z, SWIZZLE_W); + i++; + + /* perspective divide */ + vpi_insert[i].Opcode = OPCODE_RCP; + + vpi_insert[i].DstReg.File = PROGRAM_TEMPORARY; + vpi_insert[i].DstReg.Index = temp_index; + vpi_insert[i].DstReg.WriteMask = WRITEMASK_W; + vpi_insert[i].DstReg.CondMask = COND_TR; + + vpi_insert[i].SrcReg[0].File = PROGRAM_TEMPORARY; + vpi_insert[i].SrcReg[0].Index = temp_index; + vpi_
Re: [r300] partly working fragment.position patch
Roland Scheidegger wrote: > Rune Petersen wrote: >> I hit a problem constructing this: >> >> - In order to do range mapping in the vertex shader (if I so choose) >> I will need a constant (0.5), but how to add it? > I think this might work similar to what is used for position invariant > programs, instead of using _mesa_add_state_reference you could try > _mesa_add_named_parameter. Otherwise, you could always "construct" 0.5 > in the shader itself, since you always have the constants 0 and 1 > available thanks to the powerful swizzling capabilities, though > surprsingly it seems somewhat complicated. Either use 2 instructions > (ADD 1+1, RCP). Or try EX2/EXP though I'm not sure about performance of > these, but I guess the approximated EXP should do (2^-1). Thank you. EXP works fine: fp/tri-depth & tri-depth2 now look correct. I tries Doom-demo (arb2) still looks bad... Nexiuz is unstable on my system (for some unknown reason) Are there other games that use fragment.position? Rune Petersen ? depend ? server Index: r300_context.h === RCS file: /cvs/mesa/Mesa/src/mesa/drivers/dri/r300/r300_context.h,v retrieving revision 1.104 diff -u -r1.104 r300_context.h --- r300_context.h 12 Sep 2006 18:34:43 - 1.104 +++ r300_context.h 23 Sep 2006 21:33:49 - @@ -592,7 +592,8 @@ extern int hw_tcl_on; -#define CURRENT_VERTEX_SHADER(ctx) (ctx->VertexProgram._Current) +//#define CURRENT_VERTEX_SHADER(ctx) (ctx->VertexProgram._Current) +#define CURRENT_VERTEX_SHADER(ctx) (R300_CONTEXT(ctx)->selected_vp) /* Should but doesnt work */ //#define CURRENT_VERTEX_SHADER(ctx) (R300_CONTEXT(ctx)->curr_vp) @@ -607,12 +608,18 @@ /* r300_vertex_shader_state and r300_vertex_program should probably be merged together someday. * Keeping them them seperate for now should ensure fixed pipeline keeps functioning properly. */ + +struct r300_vertex_program_key { + GLuint InputsRead; + GLuint OutputsWritten; +}; + struct r300_vertex_program { - struct gl_vertex_program mesa_program; /* Must be first */ + struct r300_vertex_program *next; + struct r300_vertex_program_key key; int translated; struct r300_vertex_shader_fragment program; - struct r300_vertex_shader_fragment params; int pos_end; int num_temporaries; /* Number of temp vars used by program */ @@ -623,6 +630,12 @@ int use_ref_count; }; +struct r300_vertex_program_cont { + struct gl_vertex_program mesa_program; /* Must be first */ + struct r300_vertex_shader_fragment params; + struct r300_vertex_program *progs; +}; + #define PFS_MAX_ALU_INST 64 #define PFS_MAX_TEX_INST 64 #define PFS_MAX_TEX_INDIRECT 4 @@ -797,6 +810,7 @@ struct r300_cmdbuf cmdbuf; struct r300_state state; struct gl_vertex_program *curr_vp; + struct r300_vertex_program *selected_vp; /* Vertex buffers */ @@ -854,9 +868,9 @@ extern int r300_get_num_verts(r300ContextPtr rmesa, int num_verts, int prim); -void r300_translate_vertex_shader(struct r300_vertex_program *vp); +extern void r300_select_vertex_shader(r300ContextPtr r300); extern void r300InitShaderFuncs(struct dd_function_table *functions); -extern int r300VertexProgUpdateParams(GLcontext *ctx, struct r300_vertex_program *vp, float *dst); +extern int r300VertexProgUpdateParams(GLcontext *ctx, struct r300_vertex_program_cont *vp, float *dst); extern int r300Fallback(GLcontext *ctx); extern void radeon_vb_to_rvb(r300ContextPtr rmesa, struct radeon_vertex_buffer *rvb, struct vertex_buffer *vb); Index: r300_fragprog.c === RCS file: /cvs/mesa/Mesa/src/mesa/drivers/dri/r300/r300_fragprog.c,v retrieving revision 1.29 diff -u -r1.29 r300_fragprog.c --- r300_fragprog.c 31 Aug 2006 18:19:50 - 1.29 +++ r300_fragprog.c 23 Sep 2006 21:33:50 - @@ -1400,6 +1400,13 @@ } InputsRead &= ~FRAG_BITS_TEX_ANY; + /* fragment position treated as a texcoord */ + if (InputsRead & FRAG_BIT_WPOS) { + cs->inputs[FRAG_ATTRIB_WPOS].refcount = 0; + cs->inputs[FRAG_ATTRIB_WPOS].reg = get_hw_temp(rp); + } + InputsRead &= ~FRAG_BIT_WPOS; + /* Then primary colour */ if (InputsRead & FRAG_BIT_COL0) { cs->inputs[FRAG_ATTRIB_COL0].refcount = 0; Index: r300_maos.c === RCS file: /cvs/mesa/Mesa/src/mesa/drivers/dri/r300/r300_maos.c,v retrieving revision 1.39 diff -u -r1.39 r300_maos.c --- r300_maos.c 14 Sep 2006 16:17:06 - 1.39 +++ r300_maos.c 23 Sep 2006 21:33:51 - @@ -407,8 +407,8 @@ if (hw_tcl_on) { struct r300_vertex_program *prog=(struct r300_vertex_program *)CURRENT_VERTEX_SHADER(ctx); inputs = prog
Re: [r300] partly working fragment.position patch
Rune Petersen wrote: > I hit a problem constructing this: > > - In order to do range mapping in the vertex shader (if I so choose) > I will need a constant (0.5), but how to add it? I think this might work similar to what is used for position invariant programs, instead of using _mesa_add_state_reference you could try _mesa_add_named_parameter. Otherwise, you could always "construct" 0.5 in the shader itself, since you always have the constants 0 and 1 available thanks to the powerful swizzling capabilities, though surprsingly it seems somewhat complicated. Either use 2 instructions (ADD 1+1, RCP). Or try EX2/EXP though I'm not sure about performance of these, but I guess the approximated EXP should do (2^-1). Roland - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys -- and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [r300] partly working fragment.position patch
Rune Petersen wrote: > Keith Whitwell wrote: >> Rune Petersen wrote: >>> It turns out I missed something obvious... >>> >>> The parameters are passed correctly, I have just not transformed the >>> vertex.position to the fragment.position >> I guess that's the viewport transformation, or maybe a perspective >> divide followed by viewport transformation. > > I did do a viewport transformation, but I didn't map the z component > from a range of [-1 1] to [0 1]. > Perspective divide is also needed, but not in my test app (w=1) > > ATI appears to do perspective divide in the fragment shader. > > I hit a problem constructing this: > > - In order to do range mapping in the vertex shader (if I so > choose) I will need a constant (0.5), but how to add it? > > - If I do perspective divide in the fragment shader, I will need to > remap the WPOS from an INPUT to a TEMP. > >> But I think there's a bigger problem here -- somehow you're going to >> have to arrange for that value to be interpolated over the triangle so >> that each fragment ends up with the correct position. >> >> Maybe they are being interpolated already? I guess it then depends on >> whether the interpolation is perspective correct so that once >> transformed you really get the right pixel coordinates rather than just >> a linear interpolation across the triangle. > > Is there a way to visually verify this? Yes of course - once you've got it working, emit the position as fragment.color and have a test program read it back. If it is correct on triangles that are 'flat' but incorrect on ones that are angled away from the viewer, then it is wrong. My guess is it'll probably be fine. Keith - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys -- and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [r300] partly working fragment.position patch
Keith Whitwell wrote: > Rune Petersen wrote: >> It turns out I missed something obvious... >> >> The parameters are passed correctly, I have just not transformed the >> vertex.position to the fragment.position > > I guess that's the viewport transformation, or maybe a perspective > divide followed by viewport transformation. I did do a viewport transformation, but I didn't map the z component from a range of [-1 1] to [0 1]. Perspective divide is also needed, but not in my test app (w=1) ATI appears to do perspective divide in the fragment shader. I hit a problem constructing this: - In order to do range mapping in the vertex shader (if I so choose) I will need a constant (0.5), but how to add it? - If I do perspective divide in the fragment shader, I will need to remap the WPOS from an INPUT to a TEMP. > > But I think there's a bigger problem here -- somehow you're going to > have to arrange for that value to be interpolated over the triangle so > that each fragment ends up with the correct position. > > Maybe they are being interpolated already? I guess it then depends on > whether the interpolation is perspective correct so that once > transformed you really get the right pixel coordinates rather than just > a linear interpolation across the triangle. Is there a way to visually verify this? Rune Petersen - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys -- and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [r300] partly working fragment.position patch
Rune Petersen wrote: > It turns out I missed something obvious... > > The parameters are passed correctly, I have just not transformed the > vertex.position to the fragment.position I guess that's the viewport transformation, or maybe a perspective divide followed by viewport transformation. But I think there's a bigger problem here -- somehow you're going to have to arrange for that value to be interpolated over the triangle so that each fragment ends up with the correct position. Maybe they are being interpolated already? I guess it then depends on whether the interpolation is perspective correct so that once transformed you really get the right pixel coordinates rather than just a linear interpolation across the triangle. Keith - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys -- and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [r300] partly working fragment.position patch
It turns out I missed something obvious... The parameters are passed correctly, I have just not transformed the vertex.position to the fragment.position Now I just need to figure out how =) Rune Petersen Rune Petersen wrote: > Hi, > > Found some time to have a look at routing fragment.position from the > vertex shader. > > Patch notes: > x & y appear correct, but z is 0 % w is 1. > z appears to be 0 in the vertex shader, because swizzling Z to > position.x is is also 0. > > Most of the patch is the select_vertex_shader changes by Aapo Tahkola. > of interest is in r300_state.c and the bottom of r300_vertexprog.c > > color0 is now always passed from the vertex to the fragment, otherwise > the fragment input registers wont be correctly aligned... > > > I would be grateful for any suggestions. > > Rune Petersen - Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys -- and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel