I'm not sure about either of these patches. It looks like according to the GL 4.4 spec dithering is not supposed to affect glBlitFramebuffer:
“When values are written to the draw buffers, blit operations bypass most of the fragment pipeline. The only fragment operations which affect a blit are the pixel ownership test, the scissor test, and sRGB conversion (see section 17.3.9). Color, depth, and stencil masks (see section 17.4.2) are ignored.” I assume dithering is part of the ‘fragment operations’ because it is listed under the section ‘17.3 Per-Fragment Operations’ in the spec. In that case _mesa_meta_BlitFramebuffer should always act as if dithering is disabled. Therefore we had a bug before and after my patch. Before the patch dithering would be enabled or not depending on what the application set it to, which would usually be left as GL_TRUE. After the patch it is forced to GL_TRUE. It looks like the bug was hidden before my patch because the test suite explicitly disables dithering before running any tests. I think the right thing to do would be to make _mesa_meta_BlitFramebuffer use the MESA_META_DITHER state flag but then explicitly disable dithering. I think it would be weird if we didn't set dithering to TRUE when saving the state for MESA_META_DITHER because as far as I understand _mesa_meta_begin is expected to reset all of the selected state to the default values and the default for dithering is TRUE. - Neil "Pohjolainen, Topi" <topi.pohjolai...@intel.com> writes: > On Sun, Aug 03, 2014 at 06:32:54PM -0700, Kenneth Graunke wrote: >> On Friday, August 01, 2014 08:37:53 PM Topi Pohjolainen wrote: >> > Similarly to an older patch. This is the minimum needed to fix the >> > bug but there are other meta-paths remaining that have the bit >> > since its introduction but not before. >> >> Why not just drop the >> >> _mesa_set_enable(ctx, GL_DITHER, GL_TRUE); >> >> in _mesa_meta_begin(), which was introduced in commit >> 05b52efbc977311c96ba31ac5fa2c95fe525d85e? >> >> Neil's new glClearTexSubImage path only wants it to be saved/restored so he >> can /disable/ it. I see no reason to explicitly turn it /on/ for all >> paths... >> >> That should fix the bug. > > I'm fine with this. But just to mention, we discussed this a little when Neil > introduded the original logic. The spec for glEnable() says that by default > all the capabilities are disabled except GL_DITHER which is TRUE. To me it is > not entirely clear if meta paths should be using the default state or the > current. > >> >> > >> > commit e526ebf35c113d59eb0b860e492c1308d3862ee9 >> > Author: Kenneth Graunke <kenn...@whitecape.org> >> > Date: Mon May 5 14:03:46 2014 -0700 >> > >> > meta: Add a new MESA_META_DRAW_BUFFERS bit. >> > >> > This will be used for saving/restoring the glDrawBuffers state. >> > For now, make sure that existing users of MESA_META_ALL don't get >> > the new bit, since they probably won't want it. >> > >> > Cc: "10.2" <mesa-sta...@lists.freedesktop.org> >> > Signed-off-by: Kenneth Graunke <kenn...@whitecape.org> >> > Reviewed-by: Anuj Phogat <anuj.pho...@gmail.com> >> > Reviewed-by: Eric Anholt <e...@anholt.net> >> >> Quoting this commit message is a bit confusing - usually, when people quote >> other commits, it's referring to a regression...and the regression was from >> 05b52efbc977311c96ba31ac5fa2c95fe525d85e. Masking off unnecessary >> save/restore bits is a pretty common thing to do. >> >> > Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=81828. >> > >> > Signed-off-by: Topi Pohjolainen <topi.pohjolai...@intel.com> >> > CC: Neil Roberts <n...@linux.intel.com> >> > --- >> > src/mesa/drivers/common/meta.c | 3 ++- >> > src/mesa/drivers/common/meta_blit.c | 3 ++- >> > 2 files changed, 4 insertions(+), 2 deletions(-) >> > >> > diff --git a/src/mesa/drivers/common/meta.c >> > b/src/mesa/drivers/common/meta.c >> > index 604bc21..b4f26d0 100644 >> > --- a/src/mesa/drivers/common/meta.c >> > +++ b/src/mesa/drivers/common/meta.c >> > @@ -2798,7 +2798,8 @@ copytexsubimage_using_blit_framebuffer(struct >> > gl_context *ctx, GLuint dims, >> > >> > _mesa_unlock_texture(ctx, texObj); >> > >> > - _mesa_meta_begin(ctx, MESA_META_ALL & ~MESA_META_DRAW_BUFFERS); >> > + _mesa_meta_begin(ctx, MESA_META_ALL & ~(MESA_META_DRAW_BUFFERS | >> > + MESA_META_DITHER)); >> > >> > _mesa_GenFramebuffers(1, &fbo); >> > _mesa_BindFramebuffer(GL_DRAW_FRAMEBUFFER, fbo); >> > diff --git a/src/mesa/drivers/common/meta_blit.c >> > b/src/mesa/drivers/common/meta_blit.c >> > index bbf0c3c..d150e8a 100644 >> > --- a/src/mesa/drivers/common/meta_blit.c >> > +++ b/src/mesa/drivers/common/meta_blit.c >> > @@ -707,7 +707,8 @@ _mesa_meta_BlitFramebuffer(struct gl_context *ctx, >> > /* Only scissor affects blit, but we're doing to set a custom scissor >> > if >> > * necessary anyway, so save/clear state. >> > */ >> > - _mesa_meta_begin(ctx, MESA_META_ALL & ~MESA_META_DRAW_BUFFERS); >> > + _mesa_meta_begin(ctx, MESA_META_ALL & ~(MESA_META_DRAW_BUFFERS | >> > + MESA_META_DITHER)); >> > >> > /* If the clipping earlier changed the destination rect at all, then >> > * enable the scissor to clip to it. >> > > > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev