[Mesa-dev] [PATCH 1/3] gallium: add basic support for ARB_sample_shading

Roland Scheidegger sroland at vmware.com
Mon Mar 31 16:42:51 PDT 2014


I've got some ideas but I'm not sure the design would be really better -
I'd like to hear some other opinions.

Am 31.03.2014 02:52, schrieb Ilia Mirkin:
> ---
>  src/gallium/auxiliary/tgsi/tgsi_strings.c  |  5 ++++-
>  src/gallium/docs/source/context.rst        |  3 +++
>  src/gallium/docs/source/screen.rst         |  3 +++
>  src/gallium/docs/source/tgsi.rst           | 20 ++++++++++++++++++++
>  src/gallium/include/pipe/p_context.h       |  3 +++
>  src/gallium/include/pipe/p_defines.h       |  3 ++-
>  src/gallium/include/pipe/p_shader_tokens.h |  5 ++++-
>  7 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/src/gallium/auxiliary/tgsi/tgsi_strings.c b/src/gallium/auxiliary/tgsi/tgsi_strings.c
> index b0ba3ef..2be726c 100644
> --- a/src/gallium/auxiliary/tgsi/tgsi_strings.c
> +++ b/src/gallium/auxiliary/tgsi/tgsi_strings.c
> @@ -81,7 +81,10 @@ const char *tgsi_semantic_names[TGSI_SEMANTIC_COUNT] =
>     "PCOORD",
>     "VIEWPORT_INDEX",
>     "LAYER",
> -   "CULLDIST"
> +   "CULLDIST",
> +   "SAMPLEID",
> +   "SAMPLEPOS",
> +   "SAMPLEMASK"
>  };
>  
>  const char *tgsi_texture_names[TGSI_TEXTURE_COUNT] =
> diff --git a/src/gallium/docs/source/context.rst b/src/gallium/docs/source/context.rst
> index 8e14522..505995f 100644
> --- a/src/gallium/docs/source/context.rst
> +++ b/src/gallium/docs/source/context.rst
> @@ -67,6 +67,9 @@ objects. They all follow simple, one-method binding calls, e.g.
>    which are used as comparison values in stencil test.
>  * ``set_blend_color``
>  * ``set_sample_mask``
> +* ``set_sample_shading`` sets the minimum percentage of samples that must be
> +  run. 0 indicates per-fragment shading, while 1 should guarantee full
> +  per-sample shading.
>  * ``set_clip_state``
>  * ``set_polygon_stipple``
>  * ``set_scissor_states`` sets the bounds for the scissor test, which culls
> diff --git a/src/gallium/docs/source/screen.rst b/src/gallium/docs/source/screen.rst
> index 09f809d..8530eb4 100644
> --- a/src/gallium/docs/source/screen.rst
> +++ b/src/gallium/docs/source/screen.rst
> @@ -191,6 +191,9 @@ The integer capabilities:
>  * ``PIPE_CAP_BUFFER_MAP_PERSISTENT_COHERENT``: Whether
>    PIPE_TRANSFER_PERSISTENT and PIPE_TRANSFER_COHERENT are supported
>    for buffers.
> +* ``PIPE_CAP_SAMPLE_SHADING``: Whether there is support for per-sample
> +  shading. The context->set_sample_shading function will be expected to be
> +  implemented.
>  
>  
>  .. _pipe_capf:
> diff --git a/src/gallium/docs/source/tgsi.rst b/src/gallium/docs/source/tgsi.rst
> index e184b2f..fa30d08 100644
> --- a/src/gallium/docs/source/tgsi.rst
> +++ b/src/gallium/docs/source/tgsi.rst
> @@ -2600,6 +2600,26 @@ distances and by the PIPE_MAX_CLIP_OR_CULL_DISTANCE_ELEMENT_COUNT
>  which specifies the maximum number of registers which can be
>  annotated with those semantics.
>  
> +TGSI_SEMANTIC_SAMPLEID
> +""""""""""""""""""""""
> +
> +For fragment shaders, this semantic label indicates that a system value
> +contains the current sample id (i.e. gl_SampleID). Only the X value is used.
> +
> +TGSI_SEMANTIC_SAMPLEPOS
> +"""""""""""""""""""""""
> +
> +For fragment shaders, this semantic label indicates that a system value
> +contains the current sample's position (i.e. gl_SamplePosition). Only the X
> +and Y values are used.
> +
> +TGSI_SEMANTIC_SAMPLEMASK
> +""""""""""""""""""""""""
> +
> +For fragment shaders, this semantic label indicates that an output contains
> +the sample mask used to disable further sample processing
> +(i.e. gl_SampleMask). Only the X value is used, up to 32x MS.
> +
>  
>  Declaration Interpolate
>  ^^^^^^^^^^^^^^^^^^^^^^^
> diff --git a/src/gallium/include/pipe/p_context.h b/src/gallium/include/pipe/p_context.h
> index fe3045a..b753def 100644
> --- a/src/gallium/include/pipe/p_context.h
> +++ b/src/gallium/include/pipe/p_context.h
> @@ -190,6 +190,9 @@ struct pipe_context {
>     void (*set_sample_mask)( struct pipe_context *,
>                              unsigned sample_mask );
>  
> +   void (*set_sample_shading)( struct pipe_context *,
> +                               float samples );
I don't like this being called "samples" since you're saying it's a
value between 0.0 and 1.0. Something like shade_frequency or some such
would be more appropriate imho. That said, since we're actually
translating float sample coverage values away should probably do the
same here too and just translate to a value between 0 (or 1 if you want)
and the number of samples in the fbo. Your hw certainly doesn't allow to
set a percentage number...
I'm not entirely happy with "wasting" a whole function for a fringe
feature but I don't see any good alternatives (it's not exactly
rasterizer state, theoretically it's a shader property but in this case
you'd have to use a percentage value again since otherwise it's
obviously going to be dependent on fbo's number of samples - a shader
property though would probably allow this to be merged easier with the
code dealing with the "sample" input qualifier which also cause you to
run at higher frequency - though I still don't know how these
interactions work...).

> +
>     void (*set_clip_state)( struct pipe_context *,
>                              const struct pipe_clip_state * );
>  
> diff --git a/src/gallium/include/pipe/p_defines.h b/src/gallium/include/pipe/p_defines.h
> index a220de0..eb3bb98 100644
> --- a/src/gallium/include/pipe/p_defines.h
> +++ b/src/gallium/include/pipe/p_defines.h
> @@ -551,7 +551,8 @@ enum pipe_cap {
>     PIPE_CAP_MAX_GEOMETRY_TOTAL_OUTPUT_COMPONENTS = 89,
>     PIPE_CAP_MAX_TEXTURE_GATHER_COMPONENTS = 90,
>     PIPE_CAP_TEXTURE_GATHER_SM5 = 91,
> -   PIPE_CAP_BUFFER_MAP_PERSISTENT_COHERENT = 92
> +   PIPE_CAP_BUFFER_MAP_PERSISTENT_COHERENT = 92,
> +   PIPE_CAP_SAMPLE_SHADING = 93
>  };
>  
>  #define PIPE_QUIRK_TEXTURE_BORDER_COLOR_SWIZZLE_NV50 (1 << 0)
> diff --git a/src/gallium/include/pipe/p_shader_tokens.h b/src/gallium/include/pipe/p_shader_tokens.h
> index 8fa6a0a..2ce8b96 100644
> --- a/src/gallium/include/pipe/p_shader_tokens.h
> +++ b/src/gallium/include/pipe/p_shader_tokens.h
> @@ -167,7 +167,10 @@ struct tgsi_declaration_interp
>  #define TGSI_SEMANTIC_VIEWPORT_INDEX 21 /**< viewport index */
>  #define TGSI_SEMANTIC_LAYER      22 /**< layer (rendertarget index) */
>  #define TGSI_SEMANTIC_CULLDIST   23
> -#define TGSI_SEMANTIC_COUNT      24 /**< number of semantic values */
> +#define TGSI_SEMANTIC_SAMPLEID   24
> +#define TGSI_SEMANTIC_SAMPLEPOS  25
Hmm I'm not sure this is the right way to go. Note that d3d10.1+
actually has an instruction for getting sample positions (you just feed
it the sample index from the sample_id value if you want the current
sample position, but of course this also allows you to query OTHER
sample positions, something which this does not). From a quick look it
seems your nv50 hardware operates that way anyway, so if that's the case
for other hw too could just as well use that for gallium.
(There are instructions TGSI_OPCODE_SAMPLEINFO and TGSI_OPCODE_SAMPLEPOS
for such purposes already but they are rather unimplemented. I'm not
sure you could use them easily, since d3d10.1+ actually uses them to
query the properties of resources too not just the rasterizer, you
either give it a "rasterizer reg" or a resource reg, but resource regs
aren't used in GL because you always use sampler regs, and so far
rasterizer regs aren't implemented in gallium, so I guess there'd be
some work to do, I'm not sure it's the right direction neither.)

> +#define TGSI_SEMANTIC_SAMPLEMASK 26
> +#define TGSI_SEMANTIC_COUNT      27 /**< number of semantic values */
>  
>  struct tgsi_declaration_semantic
>  {
> 

Otherwise seems reasonable


More information about the mesa-dev mailing list