On Tue, Feb 16, 2016 at 12:02 PM, Ian Romanick <i...@freedesktop.org> wrote: > 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 <imir...@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.
We don't want per-sample interp when sample shading, as far as I can tell, based on the various OES text. > >> 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. 'Correct' is a strong word to use. Not sure. But I think so :) Based on my reading of the various OES texts (and note that I haven't dug into the EXT or NV/whatever ones), GL ES does away with the whole auto-interpolate-to-sample when sample shading. Basically the point of this persample_shading shader key is to force the interpolation position to be 'sample' for drivers that can't auto-do it via the rasterizer setting (above). > >> 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? gs5 requires sample shading, I'm assuming that no driver is devious enough in its caps settings to produce an invalid configuration. > > 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 The DX10.1 GPUs supported by nv50 (i.e. the GT21x chips) support per-sample shading, but don't, currently, support gl_SampleMaskIn. I could have a look to see if I can figure out how to get that. Although as it stands, apparently nvc0 doesn't properly support gl_SampleMaskIn either, so I have some work to do =/ At least the tests fail. > 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. I thought that was a bit harsh. For example freedreno might want to support per-sample shading without worrying about all the junk that gs5 brings in. So I figured I'd have a separate enable, but leave it set to gs5. Also if my interpretation of the per-sample interpolation changes is correct, i965 will need some fixes too. > > 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; >> >> > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev