[Mesa-dev] [PATCH 2/9] i965: Consider surface resolves just before draw

[Pohjolainen, Topi]

On Tue, Dec 20, 2016 at 03:05:14PM +0000, Ben Widawsky wrote:
> On 16-12-20 16:45:30, Topi Pohjolainen wrote:
> > If gl-state remains intact api_validate.c::_mesa_valid_to_render()
> > and brw_try_draw_prims() skip checking if textures and shader
> > images need resolves.
> > This can lead to a case where a surface is left unresolved due to
> > driver writing it internally using blorp. Blorp doesn't trash
> > global gl state but only the internal driver state.
> > 
> > Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> > CC: Kenneth Graunke <kenneth at whitecape.org>
> > CC: Jason Ekstrand <jason at jlekstrand.net>
> > CC: Ben Widawsky <ben at bwidawsk.net>
> > ---
> > src/mesa/drivers/dri/i965/brw_compute.c | 1 +
> > src/mesa/drivers/dri/i965/brw_context.c | 4 ----
> > src/mesa/drivers/dri/i965/brw_draw.c    | 2 ++
> > 3 files changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_compute.c b/src/mesa/drivers/dri/i965/brw_compute.c
> > index 16b5df7..77c056c 100644
> > --- a/src/mesa/drivers/dri/i965/brw_compute.c
> > +++ b/src/mesa/drivers/dri/i965/brw_compute.c
> > @@ -186,6 +186,7 @@ brw_dispatch_compute_common(struct gl_context *ctx)
> >    if (ctx->NewState)
> >       _mesa_update_state(ctx);
> > 
> > +   brw_resolve_surfaces(ctx);
> >    brw_validate_textures(brw);
> > 
> >    const int sampler_state_size = 16; /* 16 bytes */
> > diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c
> > index 367cd9d..0d339ff 100644
> > --- a/src/mesa/drivers/dri/i965/brw_context.c
> > +++ b/src/mesa/drivers/dri/i965/brw_context.c
> > @@ -180,10 +180,6 @@ intel_update_state(struct gl_context * ctx, GLuint new_state)
> > 
> >    brw->NewGLState |= new_state;
> > 
> > -   _mesa_unlock_context_textures(ctx);
> > -   brw_resolve_surfaces(ctx);
> > -   _mesa_lock_context_textures(ctx);
> > -
> 
> I'm surprised this patch doesn't fix a bug. From this patch this lock/unlock
> being removed seems fishy, but I think that might be an issue from the previous
> patch.

Good question, I'll amend the commit message:

It should be noted that the new callers brw_dispatch_compute_common() and
brw_try_draw_prims() are deep in the driver draw logic and shouldn't need
_mesa_unlock_context_textures()/_mesa_lock_context_textures(). Current
caller intel_update_state() in turn implements dd_function_table::UpdateState
and also gets called by _mesa_update_state_locked() which apparently needs the
unlock/lock sequence.

> 
> Reviewed-by: Ben Widawsky <ben at bwidawsk.net>

Thanks!

> 
> >    if (new_state & _NEW_BUFFERS) {
> >       intel_update_framebuffer(ctx, ctx->DrawBuffer);
> >       if (ctx->DrawBuffer != ctx->ReadBuffer)
> > diff --git a/src/mesa/drivers/dri/i965/brw_draw.c b/src/mesa/drivers/dri/i965/brw_draw.c
> > index 2ce782d..5e58f96 100644
> > --- a/src/mesa/drivers/dri/i965/brw_draw.c
> > +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> > @@ -628,6 +628,8 @@ brw_try_draw_prims(struct gl_context *ctx,
> >    if (ctx->NewState)
> >       _mesa_update_state(ctx);
> > 
> > +   brw_resolve_surfaces(ctx);
> > +
> >    /* We have to validate the textures *before* checking for fallbacks;
> >     * otherwise, the software fallback won't be able to rely on the
> >     * texture state, the firstLevel and lastLevel fields won't be
> > -- 
> > 2.5.5
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center