Re: [Dri-devel] Comoditize Mesa's driver interfaces.

[Keith W. <ke...@tu...>]

Jos=E9 Fonseca wrote:
> On Fri, Mar 21, 2003 at 08:29:02PM +0000, Keith Whitwell wrote:
>=20
>>Jos=E9 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=20
>>things like the swrast,tnl,etc. modules).  I didn't go this far as I fe=
lt=20
>>the 3.5 structure was enough to bite off in a single chunk...
>=20
>=20
> 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 no=
w
> as you know)

No, it's primarily a rework of the tnl module & it's interfaces.  Plus=20
removing some cruft.

>=20
>>Anyway, the slight trouble you get with this is error checking.  Who do=
es=20
>>it?=20
>=20
>=20
> 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.
>=20
>=20
>>How does RadeonEnable check if _mesa_Enable succeeded?=20
>=20
>=20
> 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 sam=
e path...

>=20
>>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.
>=20
>=20
> 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 ar=
e
> which aren't that simple.

Correct.

>=20
>>Continuing with the Enable() example, it would be nice to duplicating=20
>>avoid the 'switch' on the enum in both RadeonEnable and _mesa_Enable.
>=20
>=20
> 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=
=20
group lines.  Similarly you can split up glPushAttrib and glPopAttrib.  T=
hen=20
(with the init/update-state directions from recent embedded commits) attr=
ibute=20
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. =20

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 th=
e=20
drivers won't know about it & so won't keep it uptodate?

> Things would be easier if all these glEnableFoo were inlined and define=
d
> 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 th=
is in=20
3.5...


> -----------
> userapp.c:	glEnable(GL_TEXTURE);
>=20
> driver.c:	RadeonEnable(enum) {
> 		  _mesa_Enable(ctx, enum)		 =20
> 		  if ( !ctx->ReturnValue )
> 		    switch() {
> 		      case GL_ONLY_A_SUBSET_OF_THE_POSSIBLE_ENUMS:
> 		         // Do its thing here
> 			 break;
> 		    }
> 		}
>=20
> mesa.c:		_mesa_Enable(ctx, enum) {
> 		  // Do its thing here
> 		  ctx->ReturnValue =3D error_condition ? 1 : 0;
> 		}
> -----------

That's not really any better than the current system.  I'd rather see (fo=
r=20
enable) :

_mesa_enable( enum )
{
	get_context();
	fn =3D 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:
>=20
> -----------
> userapp.c:	glEnable(GL_TEXTURE);
>=20
> 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)
> 		  }
> 		}
>=20
> mesa.c:		_mesa_Enable(ctx, enum) {
> 		  if ( !_mesa_validate_Enable(ctx, enum) ) {
> 		    // Do its thing here
> 		    _mesa_Enable_novalidate(ctx, enum)
> 		  }
> 		}
>=20
> 		_mesa_Enable_novalidate(ctx, enum) {
> 		  // No need to validate here
> 		  switch () {
> 		    // Do its thing here
> 		  }
> 		}
>=20

I think this would turn into a pretty big mess over time...

Keith