Re: [PATCH 1/3] glamor: Just set the logic op to what we want at the start of all rendering.
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 > --- 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; > } ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/3] glamor: Just set the logic op to what we want at the start of all rendering.
Kenneth Graunke writes: > 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 suspect I was just reflecting the existing "conventions" in the code. The most important thing, of course, is to be consistent. Ideally, we'd write down the rules and then construct assertions to validate them at each entry point. For each piece of state, we can either: 1) Set it at startup and never change it. This is easiest to validate; make sure there's only one call to set the state. 2) Set it explicitly everywhere. This makes sense where GL already short-circuits re-writing the same value (scissor rects?) 3) Remember the state and change when necessary. When it's expensive for GL to short-circuit an operation, Glamor could do that itself (logic ops?) 4) Ensure that it always has the same value on entry. This is mostly what we do today, and frankly it sucks, as we're frequently flipping state around for no good reason. It might make sense for infrequently exercised paths that change things which GL can't easily short-circuit though. We can just look at how Mesa deals with each state change that we do and use that as a first approximation of how to handle each value. -keith signature.asc Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/3] glamor: Just set the logic op to what we want at the start of all rendering.
Kenneth Graunke 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 >> --- > > 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 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/3] glamor: Just set the logic op to what we want at the start of all rendering.
Eric Anholt writes: > 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. Reviewed-by: Keith Packard Could this be further optimized by not calling glDisable/glEnable in glamor_set_alu when unnecessary? If those are expensive, keeping track of the current value would seem like a good optimization. > diff --git a/hw/kdrive/ephyr/ephyr_glamor_glx.c > b/hw/kdrive/ephyr/ephyr_glamor_glx.c > index 8fe7516..582e3af 100644 > --- a/hw/kdrive/ephyr/ephyr_glamor_glx.c > +++ b/hw/kdrive/ephyr/ephyr_glamor_glx.c > @@ -214,6 +214,8 @@ ephyr_glamor_damage_redisplay(struct ephyr_glamor *glamor, > glBindFramebuffer(GL_FRAMEBUFFER, 0); > glUseProgram(glamor->texture_shader); > glViewport(0, 0, glamor->width, glamor->height); > +if (!ephyr_glamor_gles2) > +glDisable(GL_COLOR_LOGIC_OP); I'd suggest an unconditional call to glamor_set_alu(screen, GXcopy) here; that way any future optimizations to avoid calling glDisable/glEnable would work without having to remember to fix ephyr too. -- -keith signature.asc Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/3] glamor: Just set the logic op to what we want at the start of all rendering.
Keith Packard writes: > Eric Anholt writes: > >> 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. > > Reviewed-by: Keith Packard > > Could this be further optimized by not calling glDisable/glEnable in > glamor_set_alu when unnecessary? If those are expensive, keeping track > of the current value would seem like a good optimization. If logic op is enabled, blending doesn't happen. > >> diff --git a/hw/kdrive/ephyr/ephyr_glamor_glx.c >> b/hw/kdrive/ephyr/ephyr_glamor_glx.c >> index 8fe7516..582e3af 100644 >> --- a/hw/kdrive/ephyr/ephyr_glamor_glx.c >> +++ b/hw/kdrive/ephyr/ephyr_glamor_glx.c >> @@ -214,6 +214,8 @@ ephyr_glamor_damage_redisplay(struct ephyr_glamor >> *glamor, >> glBindFramebuffer(GL_FRAMEBUFFER, 0); >> glUseProgram(glamor->texture_shader); >> glViewport(0, 0, glamor->width, glamor->height); >> +if (!ephyr_glamor_gles2) >> +glDisable(GL_COLOR_LOGIC_OP); > > I'd suggest an unconditional call to glamor_set_alu(screen, GXcopy) > here; that way any future optimizations to avoid calling > glDisable/glEnable would work without having to remember to fix ephyr too. Yeah, I was a bit bothered by doing this, because it means if we ever get logic ops in gles2, this check would need to be updated. But so far, ephyr isn't using glamor_priv.h. I feel an in-tree use of glamor private functions is fine, though. signature.asc Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/3] glamor: Just set the logic op to what we want at the start of all rendering.
Eric Anholt writes: > If logic op is enabled, blending doesn't happen. Sure, but presumably we're almost always leaving the logic op disabled as we're using either Render or GXcopy. *not* calling glDisable for every operation seems like it might be a good idea. > Yeah, I was a bit bothered by doing this, because it means if we ever > get logic ops in gles2, this check would need to be updated. But so > far, ephyr isn't using glamor_priv.h. I feel an in-tree use of glamor > private functions is fine, though. It would probably be better to expose that function in glamor.h than use glamor_priv.h in ephyr. -- -keith signature.asc Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
