[PATCH 09/12] glamor: Replace glamor_get/put_context() with just glamor_make_current().

[Keith Packard]

Adam Jackson <ajax at nwnk.net> writes:

> On Fri, 2014-04-18 at 11:40 -0700, Eric Anholt wrote:
>
>> @@ -545,9 +541,8 @@ _glamor_copy_n_to_n(DrawablePtr src, 
>>   fail:
>> -    glamor_get_context(glamor_priv);
>> +    glamor_make_current(glamor_priv);
>>      glamor_set_alu(screen, GXcopy);
>> -    glamor_put_context(glamor_priv);
>>  
>>      if (ok)
>>          return TRUE;
>
> This made me double-take.  I mean, it can't be wrong, but I'm not sure
> what it'd protect against; glamor_create_pixmap() can't change to a
> proxy GLX context, right?  Or are we assuming that the driver could be
> using another GL context below glamor itself?  That'd be super
> weird...

Yeah, I think that for this case, the obvious mechanical edit isn't
right; every path to 'fail' has already been through glamor_make_current

There are other redundant calls to glamor_make_current:

	_glamor_download_sub_pixmap_to_cpu
        glamor_fill_spans_gl
        
I think glamor could use some rules about when you need to call
glamor_make_current; right now it's very haphazard and error prone. Here
are two possibilities:

 * Call glamor_make_current before the first actual gl call in any
   function.

 * Call glamor_make_current at the top of every external glamor
   function.

I think either of these would be pretty easy to audit; right now, all I
can do is verify that you've replaced all of the old calls to
glamor_get_context with glamor_make_current.

I reviewed the whole glamor set of patches in this series as a complete
set. I have questions about several of the intermediate states about
keeping the get_count correct, but I don't care about those given that
the goal is to get here.

[PATCH 01/12] glamor: Fix a missing set of the GL context.
[PATCH 07/12] glamor: Use lastGLContext to coordinate the context with GLX.
[PATCH 08/12] glamor: Stop unsetting the EGL context in put_context().
[PATCH 09/12] glamor: Replace glamor_get/put_context() with just glamor_make_current().
[PATCH 10/12] glamor: Explain the weird EGL_NO_CONTEXT code.
[PATCH 11/12] glamor: Do the same MakeCurrent(None) for GLX as we do for EGL.
[PATCH 12/12] glamor: Move a make_current before the first GL call entrypoint.

Reviewed-by: Keith Packard <keithp at keithp.com>

-- 
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/20140422/74d6abf6/attachment.sig>