[PATCH 06/10] glamor: Add infrastructure for generating shaders on the fly

[Keith Packard]

Eric Anholt <eric at anholt.net> writes:


> The rest of the code doesn't do spaces between function name and args,
> let's be consistent.

Fixed.

> 7 space indents in your shader code?  :P

Yeah, I just hit 'tab' in the editor and that's what I got. Not exactly
sure what to do about this, other than live with it; the alternative is
to hand-indent every shader fragment, and that seems worse...

> trailing whitespace
> stray whitespace

Fixed.

> It would be nice on pre-GL3 drivers to avoid trying to compile 1.30
> shaders, failing, and spewing a complaint to ErrorF.  We don't have a
> handy function for checking GLSL versions, but this would cover the case
> used in this code:
>
> if (version >= 130) {
>    if (glamor_priv->gl_flavor != GLAMOR_GL_DESKTOP ||
>        glamor_priv->glamor_get_gl_version() < GLAMOR_GL_VERSION_ENCODE(3, 0)) {
>        goto fail
>    }
> }

I've stuck this right after the version computation.

> I'd rather not have glGetError()s littered around.

Removed. I suspect we'll need to change the glamor compiler functions to
return status instead of silently failing or crashing though.

> You could just always call this function, if you wanted to simplify this
> code:

What it could do is set the locations to -1 that aren't used so that
we'll catch errors in programs a bit better?

> I think you want to glDeleteShader() your shaders you attached at this
> point, since they won't be freed until context destroy otherwise, and
> you've got code to clean up similar objects from the CloseScreen chain
> in glamor_delete_programs().

Done.

> I'm not sure whether we want glamor to bother with cleaning up its GL
> objects at screen close -- any driver is going to delete the context at
> CloseScreen anyway, right?  If so, then freeing those going-to-be-freed
> things and clearing the going-to-be-freed screen private seems like just
> a chance for bugs.

That's a nice clean up. I've removed the delete functions from the API.

> (Also possibly of interest: "If the value of location is -1, the
> Uniform* commands will silently ignore the data passed in, and the
> current uniform values will not be changed.").
>
> Going further, you could optionally rely on the GL dropping unused
> uniforms, and just always stuff all the location_vars at the top of
> every program.

I don't think I want to count on GL compilers being that smart :-)

What I've done for now is to create a glamor_get_uniform function that
checks the bit and sets the uniform to -2 if it shouldn't be
available. That should catch errors nicely, while not cluttering the
code as much as the sequence of conditionals (and associated #ifdef DBG
bits).

> I think we'll just delete yInverted and not worry about it any more.

Awesome.

> Without a space, "-1.0f" read to me as if it was a literal constant
> -1.

Fixed.

> I think we do need to do the conditional handling of pixel center offset
> for points vs polygons, and I'll hold off on r-b of this patch for
> that.

I've added a 'center_offset' boolean to the
glamor_set_destination_drawable function for poly_point which works
fine.

> Patch 2-5 are:
>
> Reviewed-by: Eric Anholt <eric at anholt.net>

Tnx. I've added your review, removed my patch 1 and merged in changes
cascading from the above changes into my tree.

Here's the new version of the infrastructure patch. I'll send out
another infrastructure patch in a minute; the changes to the rest of the
rendering operations are mechanical, so I won't send those out, although
they'll be in my branch of course.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-glamor-Add-infrastructure-for-generating-shaders-on-.patch
Type: text/x-diff
Size: 29389 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20140315/4b150bab/attachment-0001.patch>
-------------- next part --------------

--
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 810 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20140315/4b150bab/attachment-0001.sig>