[Mesa-dev] [PATCH] meta: Remove dither bit from blit paths that do not need it

[Neil Roberts]

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.pohjolainen at 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 <kenneth at 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-stable at lists.freedesktop.org>
>> >     Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
>> >     Reviewed-by: Anuj Phogat <anuj.phogat at gmail.com>
>> >     Reviewed-by: Eric Anholt <eric at 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.pohjolainen at intel.com>
>> > CC: Neil Roberts <neil at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev