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.pohjolai...@intel.com>
> > CC: Kenneth Graunke <kenn...@whitecape.org>
> > CC: Jason Ekstrand <ja...@jlekstrand.net>
> > CC: Ben Widawsky <b...@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 <b...@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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
> --
> Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
