Re: [Mesa-dev] [PATCH 6/7] i965: ensure execution of fragment shader when fragment shader has atomic buffer access
On Fri, Apr 24, 2015 at 07:45:29PM +0300, Rogovin, Kevin wrote: I'll check with Jordan and others. I have a faint recollection that compute shaders have similar needs. I think your change is fine though, I just want to understand the bigger picture first. I do not think compute shaders have similar needs. These flags are for making sure the rasterizer-wm thingy in Gen spawns the fragment shader threads. Compute kernels are not (I believe) spawned by the raster-wm thing, as they do not actually use the pipeline (rather they use L3, samplers and EU's only essentially). Yes, you are correct, cs has its own state packets entirely. I realized that when reading Jordan's series. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 6/7] i965: ensure execution of fragment shader when fragment shader has atomic buffer access
On Friday, April 24, 2015 08:21:36 PM Rogovin, Kevin wrote: Checking brw-ctx.Shader._CurrentFragmentProgram != NULL is unnecessary. There is always a valid pixel shader. (If the application is using fixed-function, we supply a fragment shader for them.) Please drop that check. Without this check(in the Gen7 function/code), about 30 crashes are induced on piglit tests for Gen7; the tests are all using GL fixed function pipeline. I have not run piglit without this check on Gen8 though. My mistake - we always have a pixel shader, but I think only GLSL programs have a _CurrentFragmentProgram pointer in place. Certainly ARB programs don't have a gl_shader_program, so you're right, this check is necessary. I thought that UAVs were essentially for Images...I'm not clear why this is needed. Perhaps Curro can confirm one way or another. The essential reason is to guarantee that the pixel shader gets invoked by Gen even when all render target surfaces are NULL surfaces. There are other flags one can use, but the UAV seems (to me) the most natural. -Kevin Yeah, that makes sense. Seems fine. signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 6/7] i965: ensure execution of fragment shader when fragment shader has atomic buffer access
From: Kevin Rogovin kevin.rogo...@intel.com
Ensure that the GPU spawns the fragment shader thread for those
fragment shaders with atomic buffer access.
---
src/mesa/drivers/dri/i965/gen7_wm_state.c | 7 +++
src/mesa/drivers/dri/i965/gen8_ps_state.c | 4
2 files changed, 11 insertions(+)
diff --git a/src/mesa/drivers/dri/i965/gen7_wm_state.c
b/src/mesa/drivers/dri/i965/gen7_wm_state.c
index 82e116c..fa04221 100644
--- a/src/mesa/drivers/dri/i965/gen7_wm_state.c
+++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c
@@ -77,6 +77,13 @@ upload_wm_state(struct brw_context *brw)
dw1 |= GEN7_WM_KILL_ENABLE;
}
+ /* pixel shader must run if it has side-effects
+*/
+ if (brw-ctx.Shader._CurrentFragmentProgram!=NULL
+ brw-ctx.Shader._CurrentFragmentProgram-NumAtomicBuffers 0) {
+ dw1 |= GEN7_WM_DISPATCH_ENABLE;
+ }
+
/* _NEW_BUFFERS | _NEW_COLOR */
if (brw_color_buffer_write_enabled(brw) || writes_depth ||
dw1 GEN7_WM_KILL_ENABLE) {
diff --git a/src/mesa/drivers/dri/i965/gen8_ps_state.c
b/src/mesa/drivers/dri/i965/gen8_ps_state.c
index 5f39e12..614bc9b 100644
--- a/src/mesa/drivers/dri/i965/gen8_ps_state.c
+++ b/src/mesa/drivers/dri/i965/gen8_ps_state.c
@@ -62,6 +62,10 @@ upload_ps_extra(struct brw_context *brw)
if (prog_data-uses_omask)
dw1 |= GEN8_PSX_OMASK_TO_RENDER_TARGET;
+ if (brw-ctx.Shader._CurrentFragmentProgram!=NULL
+ brw-ctx.Shader._CurrentFragmentProgram-NumAtomicBuffers 0)
+ dw1 |= GEN8_PSX_SHADER_HAS_UAV;
+
BEGIN_BATCH(2);
OUT_BATCH(_3DSTATE_PS_EXTRA 16 | (2 - 2));
OUT_BATCH(dw1);
--
1.9.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 6/7] i965: ensure execution of fragment shader when fragment shader has atomic buffer access
Reviewed-by: Tapani Pälli tapani.pa...@intel.com
On 04/24/2015 09:59 AM, kevin.rogo...@intel.com wrote:
From: Kevin Rogovin kevin.rogo...@intel.com
Ensure that the GPU spawns the fragment shader thread for those
fragment shaders with atomic buffer access.
---
src/mesa/drivers/dri/i965/gen7_wm_state.c | 7 +++
src/mesa/drivers/dri/i965/gen8_ps_state.c | 4
2 files changed, 11 insertions(+)
diff --git a/src/mesa/drivers/dri/i965/gen7_wm_state.c
b/src/mesa/drivers/dri/i965/gen7_wm_state.c
index 82e116c..fa04221 100644
--- a/src/mesa/drivers/dri/i965/gen7_wm_state.c
+++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c
@@ -77,6 +77,13 @@ upload_wm_state(struct brw_context *brw)
dw1 |= GEN7_WM_KILL_ENABLE;
}
+ /* pixel shader must run if it has side-effects
+*/
+ if (brw-ctx.Shader._CurrentFragmentProgram!=NULL
+ brw-ctx.Shader._CurrentFragmentProgram-NumAtomicBuffers 0) {
+ dw1 |= GEN7_WM_DISPATCH_ENABLE;
+ }
+
/* _NEW_BUFFERS | _NEW_COLOR */
if (brw_color_buffer_write_enabled(brw) || writes_depth ||
dw1 GEN7_WM_KILL_ENABLE) {
diff --git a/src/mesa/drivers/dri/i965/gen8_ps_state.c
b/src/mesa/drivers/dri/i965/gen8_ps_state.c
index 5f39e12..614bc9b 100644
--- a/src/mesa/drivers/dri/i965/gen8_ps_state.c
+++ b/src/mesa/drivers/dri/i965/gen8_ps_state.c
@@ -62,6 +62,10 @@ upload_ps_extra(struct brw_context *brw)
if (prog_data-uses_omask)
dw1 |= GEN8_PSX_OMASK_TO_RENDER_TARGET;
+ if (brw-ctx.Shader._CurrentFragmentProgram!=NULL
+ brw-ctx.Shader._CurrentFragmentProgram-NumAtomicBuffers 0)
+ dw1 |= GEN8_PSX_SHADER_HAS_UAV;
+
BEGIN_BATCH(2);
OUT_BATCH(_3DSTATE_PS_EXTRA 16 | (2 - 2));
OUT_BATCH(dw1);
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 6/7] i965: ensure execution of fragment shader when fragment shader has atomic buffer access
On Friday, April 24, 2015 09:59:09 AM kevin.rogo...@intel.com wrote:
From: Kevin Rogovin kevin.rogo...@intel.com
Ensure that the GPU spawns the fragment shader thread for those
fragment shaders with atomic buffer access.
---
src/mesa/drivers/dri/i965/gen7_wm_state.c | 7 +++
src/mesa/drivers/dri/i965/gen8_ps_state.c | 4
2 files changed, 11 insertions(+)
diff --git a/src/mesa/drivers/dri/i965/gen7_wm_state.c
b/src/mesa/drivers/dri/i965/gen7_wm_state.c
index 82e116c..fa04221 100644
--- a/src/mesa/drivers/dri/i965/gen7_wm_state.c
+++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c
@@ -77,6 +77,13 @@ upload_wm_state(struct brw_context *brw)
dw1 |= GEN7_WM_KILL_ENABLE;
}
+ /* pixel shader must run if it has side-effects
+*/
+ if (brw-ctx.Shader._CurrentFragmentProgram!=NULL
+ brw-ctx.Shader._CurrentFragmentProgram-NumAtomicBuffers 0) {
+ dw1 |= GEN7_WM_DISPATCH_ENABLE;
+ }
+
Hi Kevin,
Checking brw-ctx.Shader._CurrentFragmentProgram != NULL is unnecessary.
There is always a valid pixel shader. (If the application is using
fixed-function, we supply a fragment shader for them.) Please drop
that check.
Also, this patch conflicts with Curro's ARB_image_load_store series - he
was also setting the UAV bits. We'll have to sort out which should land
first. Yours is smaller, but I think he did this in a more complete
manner...
/* _NEW_BUFFERS | _NEW_COLOR */
if (brw_color_buffer_write_enabled(brw) || writes_depth ||
dw1 GEN7_WM_KILL_ENABLE) {
diff --git a/src/mesa/drivers/dri/i965/gen8_ps_state.c
b/src/mesa/drivers/dri/i965/gen8_ps_state.c
index 5f39e12..614bc9b 100644
--- a/src/mesa/drivers/dri/i965/gen8_ps_state.c
+++ b/src/mesa/drivers/dri/i965/gen8_ps_state.c
@@ -62,6 +62,10 @@ upload_ps_extra(struct brw_context *brw)
if (prog_data-uses_omask)
dw1 |= GEN8_PSX_OMASK_TO_RENDER_TARGET;
+ if (brw-ctx.Shader._CurrentFragmentProgram!=NULL
+ brw-ctx.Shader._CurrentFragmentProgram-NumAtomicBuffers 0)
+ dw1 |= GEN8_PSX_SHADER_HAS_UAV;
+
I thought that UAVs were essentially for Images...I'm not clear why this
is needed. Perhaps Curro can confirm one way or another.
BEGIN_BATCH(2);
OUT_BATCH(_3DSTATE_PS_EXTRA 16 | (2 - 2));
OUT_BATCH(dw1);
signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 6/7] i965: ensure execution of fragment shader when fragment shader has atomic buffer access
On Fri, Apr 24, 2015 at 09:59:09AM +0300, kevin.rogo...@intel.com wrote:
From: Kevin Rogovin kevin.rogo...@intel.com
Ensure that the GPU spawns the fragment shader thread for those
fragment shaders with atomic buffer access.
---
src/mesa/drivers/dri/i965/gen7_wm_state.c | 7 +++
src/mesa/drivers/dri/i965/gen8_ps_state.c | 4
2 files changed, 11 insertions(+)
diff --git a/src/mesa/drivers/dri/i965/gen7_wm_state.c
b/src/mesa/drivers/dri/i965/gen7_wm_state.c
index 82e116c..fa04221 100644
--- a/src/mesa/drivers/dri/i965/gen7_wm_state.c
+++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c
@@ -77,6 +77,13 @@ upload_wm_state(struct brw_context *brw)
dw1 |= GEN7_WM_KILL_ENABLE;
}
+ /* pixel shader must run if it has side-effects
+*/
One line comment style:
/* pixel shader must run if it has side-effects */
+ if (brw-ctx.Shader._CurrentFragmentProgram!=NULL
Add spaces around !=
+ brw-ctx.Shader._CurrentFragmentProgram-NumAtomicBuffers 0) {
+ dw1 |= GEN7_WM_DISPATCH_ENABLE;
+ }
+
/* _NEW_BUFFERS | _NEW_COLOR */
if (brw_color_buffer_write_enabled(brw) || writes_depth ||
dw1 GEN7_WM_KILL_ENABLE) {
diff --git a/src/mesa/drivers/dri/i965/gen8_ps_state.c
b/src/mesa/drivers/dri/i965/gen8_ps_state.c
index 5f39e12..614bc9b 100644
--- a/src/mesa/drivers/dri/i965/gen8_ps_state.c
+++ b/src/mesa/drivers/dri/i965/gen8_ps_state.c
@@ -62,6 +62,10 @@ upload_ps_extra(struct brw_context *brw)
if (prog_data-uses_omask)
dw1 |= GEN8_PSX_OMASK_TO_RENDER_TARGET;
+ if (brw-ctx.Shader._CurrentFragmentProgram!=NULL
Same here.
I'll check with Jordan and others. I have a faint recollection that
compute shaders have similar needs. I think your change is fine though, I
just want to understand the bigger picture first.
+ brw-ctx.Shader._CurrentFragmentProgram-NumAtomicBuffers 0)
+ dw1 |= GEN8_PSX_SHADER_HAS_UAV;
+
BEGIN_BATCH(2);
OUT_BATCH(_3DSTATE_PS_EXTRA 16 | (2 - 2));
OUT_BATCH(dw1);
--
1.9.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 6/7] i965: ensure execution of fragment shader when fragment shader has atomic buffer access
I'll check with Jordan and others. I have a faint recollection that compute shaders have similar needs. I think your change is fine though, I just want to understand the bigger picture first. I do not think compute shaders have similar needs. These flags are for making sure the rasterizer-wm thingy in Gen spawns the fragment shader threads. Compute kernels are not (I believe) spawned by the raster-wm thing, as they do not actually use the pipeline (rather they use L3, samplers and EU's only essentially). -Kevin ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 6/7] i965: ensure execution of fragment shader when fragment shader has atomic buffer access
Kenneth Graunke kenn...@whitecape.org writes:
On Friday, April 24, 2015 09:59:09 AM kevin.rogo...@intel.com wrote:
From: Kevin Rogovin kevin.rogo...@intel.com
Ensure that the GPU spawns the fragment shader thread for those
fragment shaders with atomic buffer access.
---
src/mesa/drivers/dri/i965/gen7_wm_state.c | 7 +++
src/mesa/drivers/dri/i965/gen8_ps_state.c | 4
2 files changed, 11 insertions(+)
diff --git a/src/mesa/drivers/dri/i965/gen7_wm_state.c
b/src/mesa/drivers/dri/i965/gen7_wm_state.c
index 82e116c..fa04221 100644
--- a/src/mesa/drivers/dri/i965/gen7_wm_state.c
+++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c
@@ -77,6 +77,13 @@ upload_wm_state(struct brw_context *brw)
dw1 |= GEN7_WM_KILL_ENABLE;
}
+ /* pixel shader must run if it has side-effects
+*/
+ if (brw-ctx.Shader._CurrentFragmentProgram!=NULL
+ brw-ctx.Shader._CurrentFragmentProgram-NumAtomicBuffers 0) {
+ dw1 |= GEN7_WM_DISPATCH_ENABLE;
+ }
+
Hi Kevin,
Checking brw-ctx.Shader._CurrentFragmentProgram != NULL is unnecessary.
There is always a valid pixel shader. (If the application is using
fixed-function, we supply a fragment shader for them.) Please drop
that check.
Also, this patch conflicts with Curro's ARB_image_load_store series - he
was also setting the UAV bits. We'll have to sort out which should land
first. Yours is smaller, but I think he did this in a more complete
manner...
Meh. I'm OK with resolving the conflicts if this patch lands first. I
haven't merged my patch yet (even though it has your R-b) because it
depends on some other patches in the same series that haven't been
reviewed yet.
/* _NEW_BUFFERS | _NEW_COLOR */
if (brw_color_buffer_write_enabled(brw) || writes_depth ||
dw1 GEN7_WM_KILL_ENABLE) {
diff --git a/src/mesa/drivers/dri/i965/gen8_ps_state.c
b/src/mesa/drivers/dri/i965/gen8_ps_state.c
index 5f39e12..614bc9b 100644
--- a/src/mesa/drivers/dri/i965/gen8_ps_state.c
+++ b/src/mesa/drivers/dri/i965/gen8_ps_state.c
@@ -62,6 +62,10 @@ upload_ps_extra(struct brw_context *brw)
if (prog_data-uses_omask)
dw1 |= GEN8_PSX_OMASK_TO_RENDER_TARGET;
+ if (brw-ctx.Shader._CurrentFragmentProgram!=NULL
+ brw-ctx.Shader._CurrentFragmentProgram-NumAtomicBuffers 0)
+ dw1 |= GEN8_PSX_SHADER_HAS_UAV;
+
I thought that UAVs were essentially for Images...I'm not clear why this
is needed. Perhaps Curro can confirm one way or another.
Yeah. I told him to enable this bit because it influences the
calculation of the WM_INT::ThreadDispatchEnable signal on BDW [it also
influences the cross-draw UAV coherency stuff but we don't currently
need that]. Technically atomic counters are implemented using the
hardware support for UAVs so it seems reasonable to me to set the bit.
Another possibility would be to enable Force Thread Dispatch in
3DSTATE_WM which according to the B-Spec must always be set to Normal,
except for driver debug -- Sounds like it may not have been properly
validated?
BEGIN_BATCH(2);
OUT_BATCH(_3DSTATE_PS_EXTRA 16 | (2 - 2));
OUT_BATCH(dw1);
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 6/7] i965: ensure execution of fragment shader when fragment shader has atomic buffer access
One more comment, that I neglected to add: there are other checks for _CurrentFragmentProgram to be non-NULL, indeed function brw_upload_wm_abo_surface() [file brw_wm_surface_state.c], also has a check for it being non-NULL. That function is the emit for the atom brw_wm_abo_surfaces which is present in both gen7_atoms and gen8_atoms. I would argue that _CurrentFragmentProgram can be NULL, given that other places check for it and that without the check piglit gets about 30 more crashes. Sorry for not posting this in the first reply. -Kevin -Original Message- From: Rogovin, Kevin Sent: Friday, April 24, 2015 11:22 PM To: 'Kenneth Graunke'; mesa-dev@lists.freedesktop.org Cc: mesa-...@freedesktop.org; curroje...@riseup.net Subject: RE: [Mesa-dev] [PATCH 6/7] i965: ensure execution of fragment shader when fragment shader has atomic buffer access Checking brw-ctx.Shader._CurrentFragmentProgram != NULL is unnecessary. There is always a valid pixel shader. (If the application is using fixed-function, we supply a fragment shader for them.) Please drop that check. Without this check(in the Gen7 function/code), about 30 crashes are induced on piglit tests for Gen7; the tests are all using GL fixed function pipeline. I have not run piglit without this check on Gen8 though. I thought that UAVs were essentially for Images...I'm not clear why this is needed. Perhaps Curro can confirm one way or another. The essential reason is to guarantee that the pixel shader gets invoked by Gen even when all render target surfaces are NULL surfaces. There are other flags one can use, but the UAV seems (to me) the most natural. -Kevin ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 6/7] i965: ensure execution of fragment shader when fragment shader has atomic buffer access
Checking brw-ctx.Shader._CurrentFragmentProgram != NULL is unnecessary. There is always a valid pixel shader. (If the application is using fixed-function, we supply a fragment shader for them.) Please drop that check. Without this check(in the Gen7 function/code), about 30 crashes are induced on piglit tests for Gen7; the tests are all using GL fixed function pipeline. I have not run piglit without this check on Gen8 though. I thought that UAVs were essentially for Images...I'm not clear why this is needed. Perhaps Curro can confirm one way or another. The essential reason is to guarantee that the pixel shader gets invoked by Gen even when all render target surfaces are NULL surfaces. There are other flags one can use, but the UAV seems (to me) the most natural. -Kevin ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
