[Mesa-dev] [PATCH 4/4] st/mesa: add OES_sample_variables support
Ian Romanick
idr at freedesktop.org
Tue Feb 16 18:42:57 UTC 2016
On 02/16/2016 09:12 AM, Ilia Mirkin wrote:
> 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.
I think that's correct. GL_ARB_sample_shading also says:
If the fragment shader is being evaluated at
any frequency other than per-framgent, bits of the sample mask not
corresponding to the current fragment shader invocation are ignored.
>>> 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.
With just OES_sample_shading, this is true. Based on the existing
expression, you can only get per-sample interpolation if
ctx->Multisample.SampleShading is set. That can only be set by
glEnable(GL_SAMPLE_SHADING), and that is gated on 'if
(!_mesa_is_desktop_gl(ctx)) goto invalid_enum_error;' We'll need to
change that when we add support for GL_OES_sample_shading. :)
So... I'm pretty sure that it's already impossible for
raster->force_persample_interp to get set to true in a GLES context.
This hunk just adds some code that will have to be removed when we
(you?) add GL_OES_sample_shading support.
>>> 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).
I may not understand what key.persample_shading does, then. Based on
the name, I assumed it caused the fragment shader to execute once per
sample instead of once per fragment. The spec seems to indicate that
each invocation will get the same inputs except gl_SampleID and
gl_SamplePosition (and this is what motivates the previous hunk).
>>> 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.
That's just because of the way the state tracker sets the flags,
right? Both GL_ARB_sample_shading and GL_ARB_gpu_shader5 are required
for OpenGL 4.0, but they're not required for each other. I'm pretty
sure there was some point in time when the i965 driver only supported
one. I don't recall which was first.
>> 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 meant to change that to ARB_sample_shading. Oops.
> 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.
That's possible. I didn't remember the OES extensions deviating from
the ARB semantics. Of course, that doesn't necessarily mean that they
don't. :)
I just spent quite a bit of time looking at the 4.5 (API and GLSL)
specs, ARB_gpu_shader5, ARB_sample_shading, and the OES extensions.
The OES extensions are a bit more clear because they split things into
separate extension with clear intentions. However, they all use nearly
identical words. I /think/ they should all behave the same: you only
get per-sample interpolation of GL_SAMPLE_SHADING is set and
GL_MIN_SAMPLE_SHADING_VALUE is >0.0. The spec could be more clear
here, so I have submitted
https://www.khronos.org/bugzilla/show_bug.cgi?id=1462.
>> 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