On Fri, Mar 21, 2003 at 08:29:02PM +0000, Keith Whitwell wrote:
José Fonseca wrote:
That is, instead of Mesa acting as the middle man, it should act more as a library. This specificaly means that, instead of (phony names):
userapp.c: glEnable(GL_TEXTURE);
mesa.c: _mesa_Enable(enum) { // Do its thing here if(ctx->Driver.Enable) ctx->Driver.Enable(ctx, enum); }
driver.c: RadeonEnable(ctx, enum) { // Do its thing here }
It would be:
userapp.c: glEnable(GL_TEXTURE);
driver.c: RadeonEnable(enum) { // Do its thing here _mesa_Enable(ctx, enum) // ... or here }
mesa.c: _mesa_Enable(enum) { // Do its thing here }
This was the initial idea for the mesa 3.5 rework (which resulted in things like the swrast,tnl,etc. modules). I didn't go this far as I felt the 3.5 structure was enough to bite off in a single chunk...
Were you thinking going the full way now for the vtxfmt rework? (Sorry for asking and not looking to the code myself, but I don't know exactly what's its state and where to start, and I'm also with my hands full now as you know)
No, it's primarily a rework of the tnl module & it's interfaces. Plus removing some cruft.
Anyway, the slight trouble you get with this is error checking. Who does it?
That's a choice that RadeonEnable has: if it builds upon _mesa_Enable then _mesa_Enable can do that for him. If _mesa_Enable is never called then is its responsability to set the error.
How does RadeonEnable check if _mesa_Enable succeeded?
In many cases you can simply check the error status. If ctx->ErrorValue hasn't enough (or updated) info about the error, we can extend it elsewhere (e.g., ctx->ReturnValue which would be set on zero on success).
Yes - just pointing out one of the things that I'd come across on the same path...
Also, error checking is tied in with the general structure of the function a lot of the time. Anyway, the code in api_validate.c is a fragment of my original thoughts on this, but won't work so well where error checking can't be easily seperated from the rest of the functionality.
I see. In some cases most of the code in a API is validation, and the solution you show in api_validate is the most apropriate, but others are which aren't that simple.
Correct.
Continuing with the Enable() example, it would be nice to duplicating avoid the 'switch' on the enum in both RadeonEnable and _mesa_Enable.
NOTE: I'm thinking out loud in the next paragraphs, so you may want to skim quickly through them and read only the last code snippets.
Actually, I'm trying not to do that with your posts anymore...
The only way I see to avoid the duplication of the switch is to break the glEnable in glEnableTexture, glEnableFoo, ... which is not a bad thing. We could have a function table (in the context or passed as and argument to a function _mesa_do_enable which would do the 'switch') whose the index would be 'enum', and there would only by one implementation of glEnable. RadeonEnableTexture would call _mesa_enable_texture, and so on...
I like this idea too. The ideal division for me would be along attribute group lines. Similarly you can split up glPushAttrib and glPopAttrib. Then (with the init/update-state directions from recent embedded commits) attribute state management starts to look "modular"...
On other places this is way too much trouble for a small 'switch', and
we could simply duplicate the code in Mesa's 'case' statements and
eliminate the call to the Mesa API completly.
Wasn't the idea to reduce duplication?
> Note that the state is
stored in the context already is exported to the drivers, so everytime we change the state we need to check if the drivers aren't broken, but I confess that I'm still not very confortable doing this, since if we add derived state, things will silently be broken.
You mean if we add some new derived state, then the duplicated code in the drivers won't know about it & so won't keep it uptodate?
Things would be easier if all these glEnableFoo were inlined and defined in a header, and _mesa_Enable and RadeonEnable would use them as suitable. There would be duplication of the switch statement in the code, but only one of them would be executed at run-time.
Or if each enable enum were in a header. Or if we just eat the function call.
Overall, if we consider that we need to support new OpenGL extensions everytime, and that we want to do that without having to modify each driver, then the only viable solutions are either in the line of api_validate.c or breaking glEnable in smaller functions. The others suck badily. Passing functions tables in C++ is a PITA if we want to model these functions as methods... so I think api_validate is the way to. Still, in many cases is enough just to check the error code and let Mesa do the validation by itself. That is:
I think you're seeing some of the reasons I didn't go all the way with this in 3.5...
----------- userapp.c: glEnable(GL_TEXTURE);
driver.c: RadeonEnable(enum) {
_mesa_Enable(ctx, enum) if ( !ctx->ReturnValue )
switch() {
case GL_ONLY_A_SUBSET_OF_THE_POSSIBLE_ENUMS:
// Do its thing here
break;
}
}
mesa.c: _mesa_Enable(ctx, enum) { // Do its thing here ctx->ReturnValue = error_condition ? 1 : 0; } -----------
That's not really any better than the current system. I'd rather see (for enable) :
_mesa_enable( enum )
{
get_context();
fn = get_enable_fn_from_hash( enum );
if (fn) fn( enum, GL_TRUE );
else raise_error();
}Then:
bool radeon_enable_texture( enum e, bool b )
{
if (_mesa_enable_texture( e, b )) {
RADEON_FIRE_VERTICES();
}
}bool radeon_enable_polygon_stipple( e, b )
{
...
}void init()
{
register_enable( GL_TEXTURE_1D, radeon_enable_texture );
register_enable( GL_TEXTURE_2D, radeon_enable_texture );
register_enable( GL_POLYGON_STIPPLE, radeon_enable_polygon_stipple );
}This also works nicely with extensions that may or may not be turned on.
Or when we need pre-validation:
----------- userapp.c: glEnable(GL_TEXTURE);
driver.c: RadeonEnable(enum) { if ( !_mesa_validate_Enable(ctx, enum) ) { switch() { case GL_ONLY_A_SUBSET_OF_THE_POSSIBLE_ENUMS: // Do its thing here break; } _mesa_Enable_novalidate(ctx, enum) } }
mesa.c: _mesa_Enable(ctx, enum) { if ( !_mesa_validate_Enable(ctx, enum) ) { // Do its thing here _mesa_Enable_novalidate(ctx, enum) } }
_mesa_Enable_novalidate(ctx, enum) { // No need to validate here switch () { // Do its thing here } }
I think this would turn into a pretty big mess over time...
Keith
------------------------------------------------------- This SF.net email is sponsored by:Crypto Challenge is now open! Get cracking and register here for some mind boggling fun and the chance of winning an Apple iPod: http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0031en _______________________________________________ Dri-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/dri-devel
