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

[Kenneth Graunke]

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)?

> diff --git a/glamor/glamor_copy.c b/glamor/glamor_copy.c
> index 3320935..2522adf 100644
> --- a/glamor/glamor_copy.c
> +++ b/glamor/glamor_copy.c
> @@ -404,11 +404,9 @@ glamor_copy_fbo_fbo_draw(DrawablePtr src,
>          glDisable(GL_SCISSOR_TEST);
>      glDisableVertexAttribArray(GLAMOR_VERTEX_POS);
> 
> -    glDisable(GL_COLOR_LOGIC_OP);
>      return TRUE;
> 
>  bail_ctx:
> -    glDisable(GL_COLOR_LOGIC_OP);
>      return FALSE;
>  }
> 
> @@ -452,7 +450,6 @@ glamor_copy_fbo_fbo_temp(DrawablePtr src,
> 
>      if (!glamor_set_alu(screen, gc ? gc->alu : GXcopy))
>          goto bail_ctx;
> -    glDisable(GL_COLOR_LOGIC_OP);

While we're looking at this...

Why does this function need to call glamor_set_alu() at all?  It just uses 
glamor_copy_fbo_fbo_draw() to do the actual work, and that already calls 
glamor_set_alu() for us.  Seems redundant to me.

I suppose deleting it would be a separate patch, though.

> 
>      /* Find the size of the area to copy
>       */
> @@ -521,7 +518,6 @@ bail:
>      return FALSE;
> 
>  bail_ctx:
> -    glDisable(GL_COLOR_LOGIC_OP);
>      return FALSE;
>  }