[PATCH 1/3] glamor: Just set the logic op to what we want at the start of all rendering.

[Eric Anholt]

Kenneth Graunke <kenneth at whitecape.org> writes:

> On Tuesday 30 December 2014 14:54:27 Eric Anholt wrote:
>> By dropping the unconditional logic op disable at the end of
>> rendering, this fixes GL errors being thrown in GLES2 contexts (which
>> don't have logic ops).  On desktop, this also means a little less
>> overhead per draw call from taking one less trip through the
>> glEnable/glDisable switch statement of doom in Mesa.
>> 
>> The exchange here is that we end up taking a trip through it in the
>> XV, Render, and gradient-generation paths.  If the glEnable() is
>> actually costly, we should probably cache our logic op state in our
>> screen, since there's no way the GL could make that switch statement
>> as cheap as the caller caching it would be.
>> 
>> Signed-off-by: Eric Anholt <eric at anholt.net>
>> ---
>
> Hi Eric,
>
>> In some of these cases, we've now got gotos to just a return FALSE in
>> the error case.  Do we want to just dump the gotos in this patch?  Or
>> leave them in for consistency and in case we end up adding some sort
>> of other cleanup later?
>
> The gotos do seem a bit silly.  One suggestion would be to leave "goto 
> bail_ctx" in this patch, for less churn, leaving an empty label:
>
> bail_ctx:
> bail:
>     return FALSE;
>
> then in a second patch, replace both "goto bail_ctx" and "goto bail" with 
> return statements.
>
> But, you've written it this way; not sure it's worth changing now.
>
>>  glamor/glamor_copy.c     |  4 ----
>>  glamor/glamor_dash.c     | 13 +++++--------
>>  glamor/glamor_glyphblt.c | 10 ++--------
>>  glamor/glamor_gradient.c |  4 ++++
>>  glamor/glamor_lines.c    |  5 +----
>>  glamor/glamor_pixmap.c   |  3 +++
>>  glamor/glamor_points.c   |  9 +++------
>>  glamor/glamor_rects.c    |  7 ++-----
>>  glamor/glamor_render.c   |  1 +
>>  glamor/glamor_segs.c     |  2 --
>>  glamor/glamor_spans.c    |  7 ++-----
>>  glamor/glamor_text.c     |  8 +-------
>>  glamor/glamor_xv.c       |  1 +
>>  13 files changed, 25 insertions(+), 49 deletions(-)
>
> I like this patch.
>
> I remember commenting about this to Keith at one point, and I seem to recall 
> him preferring idempotency - each operation alters some state, then puts it 
> back when it's done.  Except that we're really just mashing it on then mashing 
> it off, not saving/restoring state.  It's sort of "putting it back", but only 
> because we have the global convention that logic operations should be disabled 
> at the start/end of each Glamor operation.
>
> I do prefer having each drawing operation program the state it needs, without
> assuming anything about the value coming in (or worrying about putting it 
> back).
>
> It looks to me like the core rendering code is missing glamor_set_alu() calls 
> though - all of these files draw, but don't appear to be setting up logic ops:
>
> - glamor_dash.c
> - glamor_segs.c
> - glamor_lines.c
> - glamor_glyphblt.c
> - glamor_points.c
> - glamor_rects.c
> - glamor_spans.c
> - glamor_text.c
>
> Isn't it necessary to set or disable logic ops in those cases (or in a "start 
> core rendering" central location)?

glamor_use_program_fill makes it happen (see the call in
glamor_transform.c, and what references it).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20150204/071d7777/attachment.sig>