Re: [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 +, 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 
> > > CC: Kenneth Graunke 
> > > CC: Jason Ekstrand 
> > > CC: Ben Widawsky 
> > > ---
> > > 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 
> 
> Thanks!

Also, krh had me pretty convinced at one point that the current texture
locking is a joke :(


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Pohjolainen, Topi]

On Tue, Dec 20, 2016 at 03:05:14PM +, 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 
> > CC: Kenneth Graunke 
> > CC: Jason Ekstrand 
> > CC: Ben Widawsky 
> > ---
> > 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 

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

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

[Ben Widawsky]

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 
CC: Kenneth Graunke 
CC: Jason Ekstrand 
CC: Ben Widawsky 
---
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.

Reviewed-by: Ben Widawsky 


   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

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

[Topi Pohjolainen]

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 
CC: Kenneth Graunke 
CC: Jason Ekstrand 
CC: Ben Widawsky 
---
 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);
-
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