Kenneth Graunke <[email protected]> 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 <[email protected]> >> --- > > 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).
signature.asc
Description: PGP signature
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
