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

[Pohjolainen, Topi]

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.
> >