[Mesa-dev] [PATCH 4/4] st/mesa: add OES_sample_variables support
Ilia Mirkin
imirkin at alum.mit.edu
Tue Feb 16 17:12:18 UTC 2016
On Tue, Feb 16, 2016 at 12:02 PM, Ian Romanick <idr at 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 <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.
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;
>>
>>
>
More information about the mesa-dev
mailing list