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

[Kenneth Graunke]

On Tuesday, December 20, 2016 5:20:43 PM PST Pohjolainen, Topi wrote:
> 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!

Also, krh had me pretty convinced at one point that the current texture
locking is a joke :(
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161220/a9e6c30e/attachment.sig>