Re: [Mesa-dev] [PATCH 1/4] i965/fs: Use sample interpolation for interpolateAtCentroid in persample mode

[Kenneth Graunke]

On Wednesday, September 14, 2016 10:45:24 AM PDT Jason Ekstrand wrote:
> From the ARB_gpu_shader5 spec:
> 
>The built-in functions interpolateAtCentroid() and interpolateAtSample()
>will sample variables as though they were declared with the "centroid"
>or "sample" qualifiers, respectively.
> 
> When running with persample dispatch forced by the API, we interpolate
> anything that isn't flat as if it's qualified by "sample".  In order to
> keep interpolateAtCentroid() consistent with the "centroid" qualifier, we
> need to make interpolateAtCentroid() do sample interpolation instead.
> Nothing in the GLSL spec guarantees that the result of
> interpolateAtCentroid is uniform across samples in any way, so this is a
> perfectly fine thing to do.
> 
> Fixes 8 of the new dEQP-VK.pipeline.multisample_interpolation.* Vulkan CTS
> tests that specifically validate consistency between the "sample" qualifier
> and interpolateAtSample()
> 
> Signed-off-by: Jason Ekstrand 
> ---
>  src/mesa/drivers/dri/i965/brw_fs.cpp | 26 ++
>  1 file changed, 26 insertions(+)

Series is:
Reviewed-by: Kenneth Graunke 


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 1/4] i965/fs: Use sample interpolation for interpolateAtCentroid in persample mode

[Anuj Phogat]

On Wed, Sep 14, 2016 at 1:31 PM, Jason Ekstrand  wrote:
>
>
> On Wed, Sep 14, 2016 at 1:29 PM, Anuj Phogat  wrote:
>>
>> On Wed, Sep 14, 2016 at 10:45 AM, Jason Ekstrand 
>> wrote:
>> > From the ARB_gpu_shader5 spec:
>> >
>> >The built-in functions interpolateAtCentroid() and
>> > interpolateAtSample()
>> >will sample variables as though they were declared with the
>> > "centroid"
>> >or "sample" qualifiers, respectively.
>> >
>> > When running with persample dispatch forced by the API, we interpolate
>> > anything that isn't flat as if it's qualified by "sample".  In order to
>> > keep interpolateAtCentroid() consistent with the "centroid" qualifier,
>> > we
>> > need to make interpolateAtCentroid() do sample interpolation instead.
>> > Nothing in the GLSL spec guarantees that the result of
>> > interpolateAtCentroid is uniform across samples in any way, so this is a
>> > perfectly fine thing to do.
>> >
>> This explanation sounds good to me. To be consistent with what
>> we do in case of per sample interpolation, shouldn't we do sample
>> interpolation in case of InterpolateAtOffset() too? This series
>> doesn't seem to include it.
>
>
> No.  interpolateAtOffset ask that the input be interpolated at a particular
> offset relative to the pixel center.  I believe we have to respect that.
>
Series is: Reviewed-by: Anuj Phogat 
>>
>> > Fixes 8 of the new dEQP-VK.pipeline.multisample_interpolation.* Vulkan
>> > CTS
>> > tests that specifically validate consistency between the "sample"
>> > qualifier
>> > and interpolateAtSample()
>> >
>> > Signed-off-by: Jason Ekstrand 
>> > ---
>> >  src/mesa/drivers/dri/i965/brw_fs.cpp | 26 ++
>> >  1 file changed, 26 insertions(+)
>> >
>> > diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> > b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> > index 75642d3..9dbb699 100644
>> > --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> > +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> > @@ -6497,6 +6497,32 @@ brw_nir_set_default_interpolation(const struct
>> > gen_device_info *devinfo,
>> >   var->data.sample = false;
>> >}
>> > }
>> > +
>> > +   if (per_sample_interpolation) {
>> > +  nir_foreach_block(block, nir_shader_get_entrypoint(nir)) {
>> > + nir_foreach_instr(instr, block) {
>> > +if (instr->type != nir_instr_type_intrinsic)
>> > +   continue;
>> > +
>> > +nir_intrinsic_instr *intrin =
>> > nir_instr_as_intrinsic(instr);
>> > +if (intrin->intrinsic !=
>> > nir_intrinsic_interp_var_at_centroid)
>> > +   continue;
>> > +
>> > +nir_variable *var = intrin->variables[0]->var;
>> > +if (var->data.interpolation == INTERP_MODE_FLAT)
>> > +   continue;
>> > +
>> > +/* The description of the interpolateAtCentroid intrinsic
>> > is that
>> > + * it interpolates the variable as if it had the "centroid"
>> > + * qualifier.  When executing with
>> > per_sample_interpolation, this
>> > + * is equivalent to having the "sample" qualifier.  Just
>> > convert
>> > + * it to a load_var instead.
>> > + */
>> > +assert(var->data.sample);
>> > +intrin->intrinsic = nir_intrinsic_load_var;
>> > + }
>> > +  }
>> > +   }
>> >  }
>> >
>> >  /**
>> > --
>> > 2.5.0.400.gff86faf
>> >
>> > ___
>> > mesa-dev mailing list
>> > mesa-dev@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/4] i965/fs: Use sample interpolation for interpolateAtCentroid in persample mode

[Jason Ekstrand]

On Wed, Sep 14, 2016 at 1:29 PM, Anuj Phogat  wrote:

> On Wed, Sep 14, 2016 at 10:45 AM, Jason Ekstrand 
> wrote:
> > From the ARB_gpu_shader5 spec:
> >
> >The built-in functions interpolateAtCentroid() and
> interpolateAtSample()
> >will sample variables as though they were declared with the "centroid"
> >or "sample" qualifiers, respectively.
> >
> > When running with persample dispatch forced by the API, we interpolate
> > anything that isn't flat as if it's qualified by "sample".  In order to
> > keep interpolateAtCentroid() consistent with the "centroid" qualifier, we
> > need to make interpolateAtCentroid() do sample interpolation instead.
> > Nothing in the GLSL spec guarantees that the result of
> > interpolateAtCentroid is uniform across samples in any way, so this is a
> > perfectly fine thing to do.
> >
> This explanation sounds good to me. To be consistent with what
> we do in case of per sample interpolation, shouldn't we do sample
> interpolation in case of InterpolateAtOffset() too? This series
> doesn't seem to include it.
>

No.  interpolateAtOffset ask that the input be interpolated at a particular
offset relative to the pixel center.  I believe we have to respect that.


> > Fixes 8 of the new dEQP-VK.pipeline.multisample_interpolation.* Vulkan
> CTS
> > tests that specifically validate consistency between the "sample"
> qualifier
> > and interpolateAtSample()
> >
> > Signed-off-by: Jason Ekstrand 
> > ---
> >  src/mesa/drivers/dri/i965/brw_fs.cpp | 26 ++
> >  1 file changed, 26 insertions(+)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
> b/src/mesa/drivers/dri/i965/brw_fs.cpp
> > index 75642d3..9dbb699 100644
> > --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> > @@ -6497,6 +6497,32 @@ brw_nir_set_default_interpolation(const struct
> gen_device_info *devinfo,
> >   var->data.sample = false;
> >}
> > }
> > +
> > +   if (per_sample_interpolation) {
> > +  nir_foreach_block(block, nir_shader_get_entrypoint(nir)) {
> > + nir_foreach_instr(instr, block) {
> > +if (instr->type != nir_instr_type_intrinsic)
> > +   continue;
> > +
> > +nir_intrinsic_instr *intrin = nir_instr_as_intrinsic(instr);
> > +if (intrin->intrinsic != nir_intrinsic_interp_var_at_
> centroid)
> > +   continue;
> > +
> > +nir_variable *var = intrin->variables[0]->var;
> > +if (var->data.interpolation == INTERP_MODE_FLAT)
> > +   continue;
> > +
> > +/* The description of the interpolateAtCentroid intrinsic
> is that
> > + * it interpolates the variable as if it had the "centroid"
> > + * qualifier.  When executing with
> per_sample_interpolation, this
> > + * is equivalent to having the "sample" qualifier.  Just
> convert
> > + * it to a load_var instead.
> > + */
> > +assert(var->data.sample);
> > +intrin->intrinsic = nir_intrinsic_load_var;
> > + }
> > +  }
> > +   }
> >  }
> >
> >  /**
> > --
> > 2.5.0.400.gff86faf
> >
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/4] i965/fs: Use sample interpolation for interpolateAtCentroid in persample mode

[Anuj Phogat]

On Wed, Sep 14, 2016 at 10:45 AM, Jason Ekstrand  wrote:
> From the ARB_gpu_shader5 spec:
>
>The built-in functions interpolateAtCentroid() and interpolateAtSample()
>will sample variables as though they were declared with the "centroid"
>or "sample" qualifiers, respectively.
>
> When running with persample dispatch forced by the API, we interpolate
> anything that isn't flat as if it's qualified by "sample".  In order to
> keep interpolateAtCentroid() consistent with the "centroid" qualifier, we
> need to make interpolateAtCentroid() do sample interpolation instead.
> Nothing in the GLSL spec guarantees that the result of
> interpolateAtCentroid is uniform across samples in any way, so this is a
> perfectly fine thing to do.
>
This explanation sounds good to me. To be consistent with what
we do in case of per sample interpolation, shouldn't we do sample
interpolation in case of InterpolateAtOffset() too? This series
doesn't seem to include it.

> Fixes 8 of the new dEQP-VK.pipeline.multisample_interpolation.* Vulkan CTS
> tests that specifically validate consistency between the "sample" qualifier
> and interpolateAtSample()
>
> Signed-off-by: Jason Ekstrand 
> ---
>  src/mesa/drivers/dri/i965/brw_fs.cpp | 26 ++
>  1 file changed, 26 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp 
> b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index 75642d3..9dbb699 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -6497,6 +6497,32 @@ brw_nir_set_default_interpolation(const struct 
> gen_device_info *devinfo,
>   var->data.sample = false;
>}
> }
> +
> +   if (per_sample_interpolation) {
> +  nir_foreach_block(block, nir_shader_get_entrypoint(nir)) {
> + nir_foreach_instr(instr, block) {
> +if (instr->type != nir_instr_type_intrinsic)
> +   continue;
> +
> +nir_intrinsic_instr *intrin = nir_instr_as_intrinsic(instr);
> +if (intrin->intrinsic != nir_intrinsic_interp_var_at_centroid)
> +   continue;
> +
> +nir_variable *var = intrin->variables[0]->var;
> +if (var->data.interpolation == INTERP_MODE_FLAT)
> +   continue;
> +
> +/* The description of the interpolateAtCentroid intrinsic is that
> + * it interpolates the variable as if it had the "centroid"
> + * qualifier.  When executing with per_sample_interpolation, this
> + * is equivalent to having the "sample" qualifier.  Just convert
> + * it to a load_var instead.
> + */
> +assert(var->data.sample);
> +intrin->intrinsic = nir_intrinsic_load_var;
> + }
> +  }
> +   }
>  }
>
>  /**
> --
> 2.5.0.400.gff86faf
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 1/4] i965/fs: Use sample interpolation for interpolateAtCentroid in persample mode

[Jason Ekstrand]

From the ARB_gpu_shader5 spec:

   The built-in functions interpolateAtCentroid() and interpolateAtSample()
   will sample variables as though they were declared with the "centroid"
   or "sample" qualifiers, respectively.

When running with persample dispatch forced by the API, we interpolate
anything that isn't flat as if it's qualified by "sample".  In order to
keep interpolateAtCentroid() consistent with the "centroid" qualifier, we
need to make interpolateAtCentroid() do sample interpolation instead.
Nothing in the GLSL spec guarantees that the result of
interpolateAtCentroid is uniform across samples in any way, so this is a
perfectly fine thing to do.

Fixes 8 of the new dEQP-VK.pipeline.multisample_interpolation.* Vulkan CTS
tests that specifically validate consistency between the "sample" qualifier
and interpolateAtSample()

Signed-off-by: Jason Ekstrand 
---
 src/mesa/drivers/dri/i965/brw_fs.cpp | 26 ++
 1 file changed, 26 insertions(+)

diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp 
b/src/mesa/drivers/dri/i965/brw_fs.cpp
index 75642d3..9dbb699 100644
--- a/src/mesa/drivers/dri/i965/brw_fs.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
@@ -6497,6 +6497,32 @@ brw_nir_set_default_interpolation(const struct 
gen_device_info *devinfo,
  var->data.sample = false;
   }
}
+
+   if (per_sample_interpolation) {
+  nir_foreach_block(block, nir_shader_get_entrypoint(nir)) {
+ nir_foreach_instr(instr, block) {
+if (instr->type != nir_instr_type_intrinsic)
+   continue;
+
+nir_intrinsic_instr *intrin = nir_instr_as_intrinsic(instr);
+if (intrin->intrinsic != nir_intrinsic_interp_var_at_centroid)
+   continue;
+
+nir_variable *var = intrin->variables[0]->var;
+if (var->data.interpolation == INTERP_MODE_FLAT)
+   continue;
+
+/* The description of the interpolateAtCentroid intrinsic is that
+ * it interpolates the variable as if it had the "centroid"
+ * qualifier.  When executing with per_sample_interpolation, this
+ * is equivalent to having the "sample" qualifier.  Just convert
+ * it to a load_var instead.
+ */
+assert(var->data.sample);
+intrin->intrinsic = nir_intrinsic_load_var;
+ }
+  }
+   }
 }
 
 /**
-- 
2.5.0.400.gff86faf

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev