[Mesa-dev] [PATCH 4/4] st/mesa: add OES_sample_variables support
Ian Romanick
idr at freedesktop.org
Tue Feb 16 17:02:21 UTC 2016
On 02/15/2016 10:31 PM, Ilia Mirkin wrote:
> Basically the same thing as ARB_sample_shading except that it also needs
> gl_SampleMaskIn support as well as not enable per-sample interpolation
> whenever doing per-sample shading. This is done explicitly in another
> extension.
>
> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> ---
>
> I get 16 failures with dEQP tests, these fall into 2 categories:
>
> - 1-sample multisample surfaces don't behave the way it likes (it considers
> them non-multisampled even though they're created through gl*Multisample*
>
> - gl_SampleMaskIn is reporting the whole pixel's worth of mask rather than
> just the fragment in question. Looking back, it appears that
> ARB_gpu_shader5 also wants it for only the current fragment.
>
> docs/GL3.txt | 2 +-
> src/mesa/state_tracker/st_atom_rasterizer.c | 2 ++
> src/mesa/state_tracker/st_atom_shader.c | 2 ++
> src/mesa/state_tracker/st_extensions.c | 4 ++++
> src/mesa/state_tracker/st_program.c | 5 ++++-
> 5 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/docs/GL3.txt b/docs/GL3.txt
> index 26847b9..ae439f6 100644
> --- a/docs/GL3.txt
> +++ b/docs/GL3.txt
> @@ -248,7 +248,7 @@ GLES3.2, GLSL ES 3.2
> GL_OES_gpu_shader5 not started (based on parts of GL_ARB_gpu_shader5, which is done for some drivers)
> GL_OES_primitive_bounding box not started
> GL_OES_sample_shading not started (based on parts of GL_ARB_sample_shading, which is done for some drivers)
> - GL_OES_sample_variables not started (based on parts of GL_ARB_sample_shading, which is done for some drivers)
> + GL_OES_sample_variables DONE (nvc0, r600, radeonsi)
> GL_OES_shader_image_atomic not started (based on parts of GL_ARB_shader_image_load_store, which is done for some drivers)
> GL_OES_shader_io_blocks not started (based on parts of GLSL 1.50, which is done)
> GL_OES_shader_multisample_interpolation not started (based on parts of GL_ARB_gpu_shader5, which is done)
> diff --git a/src/mesa/state_tracker/st_atom_rasterizer.c b/src/mesa/state_tracker/st_atom_rasterizer.c
> index c20cadf..d42d512 100644
> --- a/src/mesa/state_tracker/st_atom_rasterizer.c
> +++ b/src/mesa/state_tracker/st_atom_rasterizer.c
> @@ -31,6 +31,7 @@
> */
>
> #include "main/macros.h"
> +#include "main/context.h"
> #include "st_context.h"
> #include "st_atom.h"
> #include "st_debug.h"
> @@ -239,6 +240,7 @@ static void update_raster_state( struct st_context *st )
>
> /* _NEW_MULTISAMPLE | _NEW_BUFFERS */
> raster->force_persample_interp =
> + !_mesa_is_gles(ctx) &&
> !st->force_persample_in_shader &&
> ctx->Multisample._Enabled &&
> ctx->Multisample.SampleShading &&
Is this change necessary? I would have thought that
ctx->Multisample.SampleShading couldn't get set without using features
from OES_sample_shading.
> diff --git a/src/mesa/state_tracker/st_atom_shader.c b/src/mesa/state_tracker/st_atom_shader.c
> index a88f035..8cfe756 100644
> --- a/src/mesa/state_tracker/st_atom_shader.c
> +++ b/src/mesa/state_tracker/st_atom_shader.c
> @@ -36,6 +36,7 @@
> */
>
> #include "main/imports.h"
> +#include "main/context.h"
> #include "main/mtypes.h"
> #include "program/program.h"
>
> @@ -76,6 +77,7 @@ update_fp( struct st_context *st )
> * Ignore sample qualifier while computing this flag.
> */
> key.persample_shading =
> + !_mesa_is_gles(st->ctx) &&
> st->force_persample_in_shader &&
> !(stfp->Base.Base.SystemValuesRead & (SYSTEM_BIT_SAMPLE_ID |
> SYSTEM_BIT_SAMPLE_POS)) &&
Is this correct? While not normative, the overview section of the OES
extension does say
This means that where these features are used (gl_SampleID and
gl_SamplePosition), implementations must run the fragment shader
for each sample.
I haven't dug into the body of the spec to find supporting text there.
> diff --git a/src/mesa/state_tracker/st_extensions.c b/src/mesa/state_tracker/st_extensions.c
> index eff3a2d..49d5a2c 100644
> --- a/src/mesa/state_tracker/st_extensions.c
> +++ b/src/mesa/state_tracker/st_extensions.c
> @@ -861,6 +861,10 @@ void st_init_extensions(struct pipe_screen *screen,
> extensions->OES_copy_image = GL_TRUE;
> }
>
> + /* Needs PIPE_CAP_SAMPLE_SHADING + gl_SampleMaskIn.
> + */
> + extensions->OES_sample_variables = extensions->ARB_gpu_shader5;
> +
Just based on the comment... shouldn't this be
extensions->ARB_sample_shading && extensions->ARB_gpu_shader5?
Does any Gallium driver support PIPE_CAP_SAMPLE_SHADING and not support
gl_SampleMaskIn? I wonder if it would be better to extend the meaning
of the cap bit to include gl_SampleMaskIn. Then, if the other fixes
from this patch get moved before patch 2, we could just have
GL_OES_sample_variables extension string depend on the ARB_gpu_shader5 flag.
Let me test it on our CI first... :)
> /* Maximum sample count. */
> {
> enum pipe_format color_formats[] = {
> diff --git a/src/mesa/state_tracker/st_program.c b/src/mesa/state_tracker/st_program.c
> index 2e21d02..de628d7 100644
> --- a/src/mesa/state_tracker/st_program.c
> +++ b/src/mesa/state_tracker/st_program.c
> @@ -32,6 +32,7 @@
>
>
> #include "main/imports.h"
> +#include "main/context.h"
> #include "main/hash.h"
> #include "main/mtypes.h"
> #include "program/prog_parameter.h"
> @@ -573,7 +574,9 @@ st_translate_fragment_program(struct st_context *st,
> else
> interpLocation[slot] = TGSI_INTERPOLATE_LOC_CENTER;
>
> - if (stfp->Base.Base.SystemValuesRead & (SYSTEM_BIT_SAMPLE_ID |
> + /* GLES specifies that only the sample keyword alters interpolation */
> + if (!_mesa_is_gles(st->ctx) &&
> + stfp->Base.Base.SystemValuesRead & (SYSTEM_BIT_SAMPLE_ID |
> SYSTEM_BIT_SAMPLE_POS))
> interpLocation[slot] = TGSI_INTERPOLATE_LOC_SAMPLE;
>
>
More information about the mesa-dev
mailing list