Re: [Mesa-dev] [PATCH] i965: Use sample barycentric coordinates with per sample shading
On Fri, Jan 10, 2014 at 5:25 PM, Anuj Phogat anuj.pho...@gmail.com wrote: On Thu, Jan 9, 2014 at 4:34 PM, Chris Forbes chr...@ijw.co.nz wrote: Hi Anuj, There's one fiddly interaction that I don't think this handles quite right, although I think it does conform. Suppose we have this fragment shader: #version 330 #extension ARB_gpu_shader5: require sample in vec4 a; in vec4 b; ... Then `b` is being evaluated at the sample position as well. This is allowed by my reading of the spec, but probably not what the author expected. Good catch. From the ARB_gpu_shader5 spec, emphasis mine: (11) Should we support per-sample interpolation of attributes? If so, how? RESOLVED. Yes. When multisample rasterization is enabled, qualifying one or more fragment shader inputs with sample will force per-sample interpolation of those attributes. If the same shader includes other fragment inputs not qualified with sample, those attributes _may_ be interpolated per-pixel (i.e., all samples get the same values, likely evaluated at the pixel center). What do you think? I agree with your interpretation. Spec seems to be flexible about it. I'll check what NVIDIA does in this case. This should be easy to fix if we need to. I verified that NVIDIA doesn't evaluate variable 'b' at sample position. I'll send out an updated patch to match this behavior. -- Chris ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: Use sample barycentric coordinates with per sample shading
On Mon, Jan 13, 2014 at 1:06 PM, Anuj Phogat anuj.pho...@gmail.com wrote: On Fri, Jan 10, 2014 at 5:25 PM, Anuj Phogat anuj.pho...@gmail.com wrote: On Thu, Jan 9, 2014 at 4:34 PM, Chris Forbes chr...@ijw.co.nz wrote: Hi Anuj, There's one fiddly interaction that I don't think this handles quite right, although I think it does conform. Suppose we have this fragment shader: #version 330 #extension ARB_gpu_shader5: require sample in vec4 a; in vec4 b; ... Then `b` is being evaluated at the sample position as well. This is allowed by my reading of the spec, but probably not what the author expected. Good catch. From the ARB_gpu_shader5 spec, emphasis mine: (11) Should we support per-sample interpolation of attributes? If so, how? RESOLVED. Yes. When multisample rasterization is enabled, qualifying one or more fragment shader inputs with sample will force per-sample interpolation of those attributes. If the same shader includes other fragment inputs not qualified with sample, those attributes _may_ be interpolated per-pixel (i.e., all samples get the same values, likely evaluated at the pixel center). What do you think? I agree with your interpretation. Spec seems to be flexible about it. I'll check what NVIDIA does in this case. This should be easy to fix if we need to. I verified that NVIDIA doesn't evaluate variable 'b' at sample position. I'll send out an updated patch to match this behavior. Chris, I identified another case not very well defined by OpenGL specs: /* Enable sample shading using OpenGL API */ glEnable(GL_SAMPLE_SHADING); glMinSampleShading(1.0); fragment shader: #version 130 in vec4 a; centroid in vec4 b; ... Variable 'a' will be interpolated at sample location. What interpolation should we use for variable 'b' ? ARB_sample_shading says: Should there be an option to specify that all fragment shader inputs be interpolated at per-sample frequency? If so, how? RESOLVED: Yes. Via the enable When the sample shading fraction is 1.0, a separate set of colors and other associated data are evaluated for each sample, each set of values are evaluated at the sample location. If we follow ARB_sample_shading 'b' should be interpolated at sample position. But GLSL 400 (and previous versions) spec says that: When an interpolation qualifier is used, it overrides settings established through the OpenGL API. This text got deleted in later versions of GLSL. If we follow GLSL 400 (or earlier) 'b' should use centroid interpolation. For later versions of GLSL 'b' should be interpolated at sample position. NVIDIA's and AMD's proprietary linux drivers (at OpenGL 4.3) interpolates at sample position. I think we should also stick to this behavior. Any views? -- Chris ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: Use sample barycentric coordinates with per sample shading
I would have expected explicit qualifiers to trump everything. I wonder why that was removed -- Ian? It seems there's a clear precedent established by the other drivers, though -- so I think we should stick to it. Bonus for us: since our centroid support is a bit bogus and requires workarounds, we get to emit slightly better code this way :) -- Chris ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: Use sample barycentric coordinates with per sample shading
On Thu, Jan 9, 2014 at 4:34 PM, Chris Forbes chr...@ijw.co.nz wrote: Hi Anuj, There's one fiddly interaction that I don't think this handles quite right, although I think it does conform. Suppose we have this fragment shader: #version 330 #extension ARB_gpu_shader5: require sample in vec4 a; in vec4 b; ... Then `b` is being evaluated at the sample position as well. This is allowed by my reading of the spec, but probably not what the author expected. Good catch. From the ARB_gpu_shader5 spec, emphasis mine: (11) Should we support per-sample interpolation of attributes? If so, how? RESOLVED. Yes. When multisample rasterization is enabled, qualifying one or more fragment shader inputs with sample will force per-sample interpolation of those attributes. If the same shader includes other fragment inputs not qualified with sample, those attributes _may_ be interpolated per-pixel (i.e., all samples get the same values, likely evaluated at the pixel center). What do you think? I agree with your interpretation. Spec seems to be flexible about it. I'll check what NVIDIA does in this case. This should be easy to fix if we need to. -- Chris ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] i965: Use sample barycentric coordinates with per sample shading
Current implementation of arb_sample_shading doesn't set 'Barycentric
Interpolation Mode' correctly. We use pixel barycentric coordinates
for per sample shading. Instead we should select perspective sample
or non-perspective sample barycentric coordinates.
It also enables using sample barycentric coordinates in case of a
fragment shader variable declared with 'sample' qualifier.
e.g. sample in vec4 pos;
A piglit test to verify the implementation has been posted on piglit
mailing list for review.
Signed-off-by: Anuj Phogat anuj.pho...@gmail.com
Cc: Chris Forbes chr...@ijw.co.nz
Cc: mesa-sta...@lists.freedesktop.org
---
src/mesa/drivers/dri/i965/brw_fs.cpp | 13 ++---
src/mesa/drivers/dri/i965/brw_fs.h | 2 +-
src/mesa/drivers/dri/i965/brw_wm.c | 18 --
src/mesa/drivers/dri/i965/brw_wm.h | 1 +
4 files changed, 28 insertions(+), 6 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
b/src/mesa/drivers/dri/i965/brw_fs.cpp
index baf9220..a85646f 100644
--- a/src/mesa/drivers/dri/i965/brw_fs.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
@@ -998,7 +998,7 @@ fs_visitor::emit_fragcoord_interpolation(ir_variable *ir)
fs_inst *
fs_visitor::emit_linterp(const fs_reg attr, const fs_reg interp,
glsl_interp_qualifier interpolation_mode,
- bool is_centroid)
+ bool is_centroid, bool is_sample)
{
brw_wm_barycentric_interp_mode barycoord_mode;
if (brw-gen = 6) {
@@ -1007,6 +1007,11 @@ fs_visitor::emit_linterp(const fs_reg attr, const
fs_reg interp,
barycoord_mode = BRW_WM_PERSPECTIVE_CENTROID_BARYCENTRIC;
else
barycoord_mode = BRW_WM_NONPERSPECTIVE_CENTROID_BARYCENTRIC;
+ } else if (is_sample) {
+ if (interpolation_mode == INTERP_QUALIFIER_SMOOTH)
+barycoord_mode = BRW_WM_PERSPECTIVE_SAMPLE_BARYCENTRIC;
+ else
+barycoord_mode = BRW_WM_NONPERSPECTIVE_SAMPLE_BARYCENTRIC;
} else {
if (interpolation_mode == INTERP_QUALIFIER_SMOOTH)
barycoord_mode = BRW_WM_PERSPECTIVE_PIXEL_BARYCENTRIC;
@@ -1084,7 +1089,8 @@ fs_visitor::emit_general_interpolation(ir_variable *ir)
*/
struct brw_reg interp = interp_reg(location, k);
emit_linterp(attr, fs_reg(interp), interpolation_mode,
-ir-data.centroid);
+ir-data.centroid,
+ir-data.sample || c-key.per_sample_shade);
if (brw-needs_unlit_centroid_workaround ir-data.centroid) {
/* Get the pixel/sample mask into f0 so that we know
* which pixels are lit. Then, for each channel that is
@@ -1093,7 +1099,8 @@ fs_visitor::emit_general_interpolation(ir_variable *ir)
*/
emit(FS_OPCODE_MOV_DISPATCH_TO_FLAGS);
fs_inst *inst = emit_linterp(attr, fs_reg(interp),
- interpolation_mode, false);
+ interpolation_mode,
+ false, false);
inst-predicate = BRW_PREDICATE_NORMAL;
inst-predicate_inverse = true;
}
diff --git a/src/mesa/drivers/dri/i965/brw_fs.h
b/src/mesa/drivers/dri/i965/brw_fs.h
index 9bef07c..b5656bf 100644
--- a/src/mesa/drivers/dri/i965/brw_fs.h
+++ b/src/mesa/drivers/dri/i965/brw_fs.h
@@ -336,7 +336,7 @@ public:
fs_reg *emit_fragcoord_interpolation(ir_variable *ir);
fs_inst *emit_linterp(const fs_reg attr, const fs_reg interp,
glsl_interp_qualifier interpolation_mode,
- bool is_centroid);
+ bool is_centroid, bool is_sample);
fs_reg *emit_frontfacing_interpolation(ir_variable *ir);
fs_reg *emit_samplepos_setup(ir_variable *ir);
fs_reg *emit_sampleid_setup(ir_variable *ir);
diff --git a/src/mesa/drivers/dri/i965/brw_wm.c
b/src/mesa/drivers/dri/i965/brw_wm.c
index 6739a91..89830a4 100644
--- a/src/mesa/drivers/dri/i965/brw_wm.c
+++ b/src/mesa/drivers/dri/i965/brw_wm.c
@@ -52,6 +52,7 @@ brw_compute_barycentric_interp_modes(struct brw_context *brw,
const struct gl_fragment_program *fprog)
{
unsigned barycentric_interp_modes = 0;
+ struct gl_context *ctx = brw-ctx;
int attr;
/* Loop through all fragment shader inputs to figure out what interpolation
@@ -62,6 +63,8 @@ brw_compute_barycentric_interp_modes(struct brw_context *brw,
enum glsl_interp_qualifier interp_qualifier =
fprog-InterpQualifier[attr];
bool is_centroid = fprog-IsCentroid BITFIELD64_BIT(attr);
+ bool is_sample = (fprog-IsSample BITFIELD64_BIT(attr)) ||
+ _mesa_get_min_invocations_per_fragment(ctx, fprog) 1;
bool is_gl_Color = attr == VARYING_SLOT_COL0 || attr ==
Re: [Mesa-dev] [PATCH] i965: Use sample barycentric coordinates with per sample shading
Hi Anuj, There's one fiddly interaction that I don't think this handles quite right, although I think it does conform. Suppose we have this fragment shader: #version 330 #extension ARB_gpu_shader5: require sample in vec4 a; in vec4 b; ... Then `b` is being evaluated at the sample position as well. This is allowed by my reading of the spec, but probably not what the author expected. From the ARB_gpu_shader5 spec, emphasis mine: (11) Should we support per-sample interpolation of attributes? If so, how? RESOLVED. Yes. When multisample rasterization is enabled, qualifying one or more fragment shader inputs with sample will force per-sample interpolation of those attributes. If the same shader includes other fragment inputs not qualified with sample, those attributes _may_ be interpolated per-pixel (i.e., all samples get the same values, likely evaluated at the pixel center). What do you think? -- Chris ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
