[Mesa-dev] [PATCH 1/3] gallium: add TGSI_SEMANTIC_VERTEXID_ZEROBASE and TGSI_SEMANTIC_BASEVERTEX

Brian Paul brianp at vmware.com
Wed Dec 10 16:23:31 PST 2014


On 12/10/2014 05:09 PM, Roland Scheidegger wrote:
> Am 11.12.2014 um 00:53 schrieb Brian Paul:
>> On 12/10/2014 02:22 PM, sroland at vmware.com wrote:
>>> From: Roland Scheidegger <sroland at vmware.com>
>>>
>>> Plus a new PIPE_CAP_VERTEXID_NOOFFSET query. The idea is that drivers not
>>> supporting vertex ids with base vertex offset applied (so, only support
>>> d3d10-style vertex ids) will get such a d3d10-style vertex id instead -
>>> with the caveat they'll also need to handle the basevertex system value
>>> too (this follows what core mesa already does).
>>> Additionally, this is also useful for other state trackers (for instance
>>> llvmpipe / draw right now implement the d3d10 behavior on purpose, but
>>> with different semantics it can just do both).
>>> Doesn't do anything yet.
>>> And fix up the docs wrt similar values.
>>> ---
>>>    src/gallium/auxiliary/tgsi/tgsi_scan.c           |  6 ++++
>>>    src/gallium/auxiliary/tgsi/tgsi_scan.h           |  2 ++
>>>    src/gallium/auxiliary/tgsi/tgsi_strings.c        |  2 ++
>>>    src/gallium/docs/source/screen.rst               |  6 ++++
>>>    src/gallium/docs/source/tgsi.rst                 | 36
>>> ++++++++++++++++++++++++
>>>    src/gallium/drivers/freedreno/freedreno_screen.c |  1 +
>>>    src/gallium/drivers/i915/i915_screen.c           |  1 +
>>>    src/gallium/drivers/ilo/ilo_screen.c             |  2 ++
>>>    src/gallium/drivers/llvmpipe/lp_screen.c         |  2 ++
>>>    src/gallium/drivers/nouveau/nv30/nv30_screen.c   |  1 +
>>>    src/gallium/drivers/nouveau/nv50/nv50_screen.c   |  1 +
>>>    src/gallium/drivers/r300/r300_screen.c           |  1 +
>>>    src/gallium/drivers/r600/r600_pipe.c             |  1 +
>>>    src/gallium/drivers/radeonsi/si_pipe.c           |  1 +
>>>    src/gallium/drivers/softpipe/sp_screen.c         |  2 ++
>>>    src/gallium/drivers/svga/svga_screen.c           |  1 +
>>>    src/gallium/drivers/vc4/vc4_screen.c             |  1 +
>>>    src/gallium/include/pipe/p_defines.h             |  1 +
>>>    src/gallium/include/pipe/p_shader_tokens.h       |  4 ++-
>>>    19 files changed, 71 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/gallium/auxiliary/tgsi/tgsi_scan.c
>>> b/src/gallium/auxiliary/tgsi/tgsi_scan.c
>>> index 649c327..954eb10 100644
>>> --- a/src/gallium/auxiliary/tgsi/tgsi_scan.c
>>> +++ b/src/gallium/auxiliary/tgsi/tgsi_scan.c
>>> @@ -213,6 +213,12 @@ tgsi_scan_shader(const struct tgsi_token *tokens,
>>>                      else if (semName == TGSI_SEMANTIC_VERTEXID) {
>>>                         info->uses_vertexid = TRUE;
>>>                      }
>>> +                  else if (semName == TGSI_SEMANTIC_VERTEXID_ZEROBASE) {
>>> +                     info->uses_vertexid_zerobase = TRUE;
>>> +                  }
>>> +                  else if (semName == TGSI_SEMANTIC_BASEVERTEX) {
>>> +                     info->uses_basevertex = TRUE;
>>> +                  }
>>>                      else if (semName == TGSI_SEMANTIC_PRIMID) {
>>>                         info->uses_primid = TRUE;
>>>                      }
>>
>> Tangential, but I wonder if we could someday replace the instances of:
>>
>> if (semName == TGSI_SEMANTIC_FOO) {
>>     info->uses_foo = TRUE;
>> }
>>
>> with something like:
>>
>> info->semantics_used |= (1 << TGSI_SEMANTIC_FOO);
>>
>> Maybe separate bitfields for read vs. written registers.
> Yes that sounds good to me.
>
>>
>>
>>
>>> diff --git a/src/gallium/auxiliary/tgsi/tgsi_scan.h
>>> b/src/gallium/auxiliary/tgsi/tgsi_scan.h
>>> index 61ce813..b353097 100644
>>> --- a/src/gallium/auxiliary/tgsi/tgsi_scan.h
>>> +++ b/src/gallium/auxiliary/tgsi/tgsi_scan.h
>>> @@ -74,6 +74,8 @@ struct tgsi_shader_info
>>>       boolean uses_kill;  /**< KILL or KILL_IF instruction used? */
>>>       boolean uses_instanceid;
>>>       boolean uses_vertexid;
>>> +   boolean uses_vertexid_zerobase;
>>> +   boolean uses_basevertex;
>>>       boolean uses_primid;
>>>       boolean uses_frontface;
>>>       boolean writes_psize;
>>> diff --git a/src/gallium/auxiliary/tgsi/tgsi_strings.c
>>> b/src/gallium/auxiliary/tgsi/tgsi_strings.c
>>> index 01fa5a9..7a45e2d 100644
>>> --- a/src/gallium/auxiliary/tgsi/tgsi_strings.c
>>> +++ b/src/gallium/auxiliary/tgsi/tgsi_strings.c
>>> @@ -86,6 +86,8 @@ const char *tgsi_semantic_names[TGSI_SEMANTIC_COUNT] =
>>>       "SAMPLEPOS",
>>>       "SAMPLEMASK",
>>>       "INVOCATIONID",
>>> +   "VERTEXID_ZEROBASE",
>>> +   "BASEVERTEX",
>>>    };
>>>
>>>    const char *tgsi_texture_names[TGSI_TEXTURE_COUNT] =
>>> diff --git a/src/gallium/docs/source/screen.rst
>>> b/src/gallium/docs/source/screen.rst
>>> index e711ad4..6ccbe5b 100644
>>> --- a/src/gallium/docs/source/screen.rst
>>> +++ b/src/gallium/docs/source/screen.rst
>>> @@ -233,6 +233,12 @@ The integer capabilities:
>>>    * ``PIPE_CAP_CLIP_HALFZ``: Whether the driver supports the
>>>      pipe_rasterizer_state::clip_halfz being set to true. This is required
>>>      for enabling ARB_clip_control.
>>> +* ``PIPE_CAP_VERTEXID_NOOFFSET``: Whether the driver needs lowering
>>> of the
>>> +  vertex id to not include the basevertex offset. If so vertex id will
>>> +  use TGSI_SEMANTIC_VERTEXID_ZEROBASE and TGSI_SEMANTIC_BASEVERTEX
>>> instead
>>> +  of TGSI_SEMANTIC_VERTEXID. Only relevant if geometry shaders are
>>> supported.
>>> +  (Currently not possible to query availability of these two
>>> semantics outside
>>> +  of this).
>>
>> The first sentence is a little hard to parse (at least to me).  How about:
>>
>> "If true, the driver only supports TGSI_SEMANTIC_VERTEXID_ZEROBASE (and
>> not TGSI_SEMANTIC_VERTEXID).  This means state trackers which need base
>> offsetting will need to lower TGSI_SEMANTIC_VERTEXID to
>> TGSI_SEMANTIC_VERTEXID_ZEROBASE and TGSI_SEMANTIC_BASEVERTEX."  ...
> Sounds better indeed.
>
>>
>>>
>>>    .. _pipe_capf:
>>> diff --git a/src/gallium/docs/source/tgsi.rst
>>> b/src/gallium/docs/source/tgsi.rst
>>> index cbb8f74..c12858d 100644
>>> --- a/src/gallium/docs/source/tgsi.rst
>>> +++ b/src/gallium/docs/source/tgsi.rst
>>> @@ -2723,6 +2723,42 @@ For geometry shaders, this semantic label
>>> indicates that a system value
>>>    contains the current invocation id (i.e. gl_InvocationID). Only the
>>> X value is
>>>    used.
>>>
>>> +TGSI_SEMANTIC_INSTANCEID
>>> +""""""""""""""""""""""""
>>> +
>>> +For vertex shaders, this semantic label indicates that a system value
>>> contains
>>> +the current instance id (i.e. gl_InstanceID). It does not include the
>>> base
>>> +instance. Only the X value is used.
>>> +
>>> +TGSI_SEMANTIC_VERTEXID
>>> +""""""""""""""""""""""
>>> +
>>> +For vertex shaders, this semantic label indicates that a system value
>>> contains
>>> +the current vertex id (i.e. gl_VertexID). It does (unlike in d3d10)
>>> include the
>>> +base vertex. Only the X value is used.
>>> +
>>> +TGSI_SEMANTIC_VERTEXID_ZEROBASE
>>> +"""""""""""""""""""""""""""""""
>>> +
>>> +For vertex shaders, this semantic label indicates that a system value
>>> contains
>>> +the current vertex id without including the base vertex (this
>>> corresponds to
>>> +d3d10 vertex id, so TGSI_SEMANTIC_VERTEXID_ZEROBASE +
>>> TGSI_SEMANTIC_BASEVERTEX
>>> +== TGSI_SEMANTIC_VERTEXID). Only the X value is used.
>>
>> I wonder if those two semantics should be renamed:
>>
>> TGSI_SEMANTIC_VERTEXID  ->  TGSI_SEMANTIC_VERTEXID_WITH_BASE
>> TGSI_SEMANTIC_VERTEXID_ZEROBASE  ->  TGSI_SEMANTIC_VERTEXID
>>
>> This would make TGSI_SEMANTIC_VERTEXID consistent with
>> TGSI_SEMANTIC_INSTANCEID so they're both zero-based (do not include the
>> base value).
> I'm not sure, I was just going by the names similar to what core mesa is
> using - SYSTEM_VALUE_VERTEX_ID / SYSTEM_VALUE_VERTEX_ID_ZERO_BASE.

OK, I didn't realize that core mesa already has these terms.  In that 
case, I don't have a strong opinion.



> I don't mind renaming the existing VERTEXID one though imho if renaming
> we should avoid calling the one without base vertex just VERTEXID.
> Maybe VERTEXID_WITH_BASE and VERTEXID_NO_BASE?

Well, that would certainly make things clear to the reader.  Your call, 
as far as I'm concerned.


>
>>
>> Also, should we be explicit and say these values are integers?
> Yes.
>
>>
>>
>>> +
>>> +TGSI_SEMANTIC_BASEVERTEX
>>> +""""""""""""""""""""""""
>>> +
>>> +For vertex shaders, this semantic label indicates that a system value
>>> contains
>>> +the base vertex (i.e. gl_BaseVertex). Only the X value is used.
>>> +
>>> +TGSI_SEMANTIC_PRIMID
>>> +""""""""""""""""""""
>>> +
>>> +For geometry and fragment shaders, this semantic label indicates the
>>> value
>>> +contains the primitive id (i.e. gl_PrimitiveID). Only the X value is
>>> used.
>>> +FIXME: This right now can be either a ordinary input or a system
>>> value...
>>> +
>>> +
>>>    Declaration Interpolate
>>>    ^^^^^^^^^^^^^^^^^^^^^^^
>>>
>>> diff --git a/src/gallium/drivers/freedreno/freedreno_screen.c
>>> b/src/gallium/drivers/freedreno/freedreno_screen.c
>>> index ce105b8..43f5366 100644
>>> --- a/src/gallium/drivers/freedreno/freedreno_screen.c
>>> +++ b/src/gallium/drivers/freedreno/freedreno_screen.c
>>> @@ -228,6 +228,7 @@ fd_screen_get_param(struct pipe_screen *pscreen,
>>> enum pipe_cap param)
>>>        case PIPE_CAP_CONDITIONAL_RENDER_INVERTED:
>>>        case PIPE_CAP_SAMPLER_VIEW_TARGET:
>>>        case PIPE_CAP_CLIP_HALFZ:
>>> +    case PIPE_CAP_VERTEXID_NOOFSET:
>>
>> How about NO_OFFSET instead of NOOFFSET?
> Ahh your eyes are as sharp as Marek's ;-). Apparently this doesn't get
> compiled in my setup...
>
>>
>>
>>>            return 0;
>>>
>>>        case PIPE_CAP_MAX_VIEWPORTS:
>>> diff --git a/src/gallium/drivers/i915/i915_screen.c
>>> b/src/gallium/drivers/i915/i915_screen.c
>>> index 1c60499..dedc7a8 100644
>>> --- a/src/gallium/drivers/i915/i915_screen.c
>>> +++ b/src/gallium/drivers/i915/i915_screen.c
>>> @@ -226,6 +226,7 @@ i915_get_param(struct pipe_screen *screen, enum
>>> pipe_cap cap)
>>>       case PIPE_CAP_TGSI_VS_WINDOW_SPACE_POSITION:
>>>       case PIPE_CAP_CONDITIONAL_RENDER_INVERTED:
>>>       case PIPE_CAP_CLIP_HALFZ:
>>> +   case PIPE_CAP_VERTEXID_NOOFFSET:
>>>          return 0;
>>>
>>>       case PIPE_CAP_MAX_DUAL_SOURCE_RENDER_TARGETS:
>>> diff --git a/src/gallium/drivers/ilo/ilo_screen.c
>>> b/src/gallium/drivers/ilo/ilo_screen.c
>>> index 06aa973..cbf1083 100644
>>> --- a/src/gallium/drivers/ilo/ilo_screen.c
>>> +++ b/src/gallium/drivers/ilo/ilo_screen.c
>>> @@ -495,6 +495,8 @@ ilo_get_param(struct pipe_screen *screen, enum
>>> pipe_cap param)
>>>          return true;
>>>       case PIPE_CAP_CLIP_HALFZ:
>>>          return true;
>>> +   case PIPE_CAP_VERTEXID_NOOFFSET:
>>> +      return false;
>>>
>>>       default:
>>>          return 0;
>>> diff --git a/src/gallium/drivers/llvmpipe/lp_screen.c
>>> b/src/gallium/drivers/llvmpipe/lp_screen.c
>>> index 6af43cc..a394fa7 100644
>>> --- a/src/gallium/drivers/llvmpipe/lp_screen.c
>>> +++ b/src/gallium/drivers/llvmpipe/lp_screen.c
>>> @@ -282,6 +282,8 @@ llvmpipe_get_param(struct pipe_screen *screen,
>>> enum pipe_cap param)
>>>          return 0;
>>>       case PIPE_CAP_CLIP_HALFZ:
>>>          return 1;
>>> +   case PIPE_CAP_VERTEXID_NOOFFSET:
>>> +      return 0;
>>>       }
>>>       /* should only get here on unhandled cases */
>>>       debug_printf("Unexpected PIPE_CAP %d query\n", param);
>>> diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>>> b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>>> index 2b65f8c..6ab5203 100644
>>> --- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>>> +++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>>> @@ -157,6 +157,7 @@ nv30_screen_get_param(struct pipe_screen *pscreen,
>>> enum pipe_cap param)
>>>       case PIPE_CAP_CONDITIONAL_RENDER_INVERTED:
>>>       case PIPE_CAP_SAMPLER_VIEW_TARGET:
>>>       case PIPE_CAP_CLIP_HALFZ:
>>> +   case PIPE_CAP_VERTEXID_NOOFFSET:
>>>          return 0;
>>>
>>>       case PIPE_CAP_VENDOR_ID:
>>> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>>> b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>>> index fcf0098..16cedc9 100644
>>> --- a/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>>> +++ b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>>> @@ -205,6 +205,7 @@ nv50_screen_get_param(struct pipe_screen *pscreen,
>>> enum pipe_cap param)
>>>       case PIPE_CAP_TGSI_VS_WINDOW_SPACE_POSITION:
>>>       case PIPE_CAP_COMPUTE:
>>>       case PIPE_CAP_DRAW_INDIRECT:
>>> +   case PIPE_CAP_VERTEXID_NOOFFSET:
>>>          return 0;
>>>
>>>       case PIPE_CAP_VENDOR_ID:
>>> diff --git a/src/gallium/drivers/r300/r300_screen.c
>>> b/src/gallium/drivers/r300/r300_screen.c
>>> index e653eab..bb6ba32 100644
>>> --- a/src/gallium/drivers/r300/r300_screen.c
>>> +++ b/src/gallium/drivers/r300/r300_screen.c
>>> @@ -181,6 +181,7 @@ static int r300_get_param(struct pipe_screen*
>>> pscreen, enum pipe_cap param)
>>>            case PIPE_CAP_TGSI_FS_FINE_DERIVATIVE:
>>>            case PIPE_CAP_CONDITIONAL_RENDER_INVERTED:
>>>            case PIPE_CAP_SAMPLER_VIEW_TARGET:
>>> +        case PIPE_CAP_VERTEXID_NOOFFSET:
>>>                return 0;
>>>
>>>            /* SWTCL-only features. */
>>> diff --git a/src/gallium/drivers/r600/r600_pipe.c
>>> b/src/gallium/drivers/r600/r600_pipe.c
>>> index 0b571e4..1be037f 100644
>>> --- a/src/gallium/drivers/r600/r600_pipe.c
>>> +++ b/src/gallium/drivers/r600/r600_pipe.c
>>> @@ -325,6 +325,7 @@ static int r600_get_param(struct pipe_screen*
>>> pscreen, enum pipe_cap param)
>>>        case PIPE_CAP_DRAW_INDIRECT:
>>>        case PIPE_CAP_CONDITIONAL_RENDER_INVERTED:
>>>        case PIPE_CAP_SAMPLER_VIEW_TARGET:
>>> +    case PIPE_CAP_VERTEXID_NOOFFSET:
>>>            return 0;
>>>
>>>        /* Stream output. */
>>> diff --git a/src/gallium/drivers/radeonsi/si_pipe.c
>>> b/src/gallium/drivers/radeonsi/si_pipe.c
>>> index 8fc5c19..e86a593 100644
>>> --- a/src/gallium/drivers/radeonsi/si_pipe.c
>>> +++ b/src/gallium/drivers/radeonsi/si_pipe.c
>>> @@ -253,6 +253,7 @@ static int si_get_param(struct pipe_screen*
>>> pscreen, enum pipe_cap param)
>>>        case PIPE_CAP_TGSI_FS_FINE_DERIVATIVE:
>>>        case PIPE_CAP_CONDITIONAL_RENDER_INVERTED:
>>>        case PIPE_CAP_SAMPLER_VIEW_TARGET:
>>> +    case PIPE_CAP_VERTEXID_NOOFFSET:
>>>            return 0;
>>>
>>>        case PIPE_CAP_TEXTURE_BORDER_COLOR_QUIRK:
>>> diff --git a/src/gallium/drivers/softpipe/sp_screen.c
>>> b/src/gallium/drivers/softpipe/sp_screen.c
>>> index 57cd9b6..296c94a 100644
>>> --- a/src/gallium/drivers/softpipe/sp_screen.c
>>> +++ b/src/gallium/drivers/softpipe/sp_screen.c
>>> @@ -231,6 +231,8 @@ softpipe_get_param(struct pipe_screen *screen,
>>> enum pipe_cap param)
>>>          return 1;
>>>       case PIPE_CAP_CLIP_HALFZ:
>>>          return 1;
>>> +   case PIPE_CAP_VERTEXID_NOOFFSET:
>>> +      return 0;
>>>       }
>>>       /* should only get here on unhandled cases */
>>>       debug_printf("Unexpected PIPE_CAP %d query\n", param);
>>> diff --git a/src/gallium/drivers/svga/svga_screen.c
>>> b/src/gallium/drivers/svga/svga_screen.c
>>> index 691d9df..9bcfb3c 100644
>>> --- a/src/gallium/drivers/svga/svga_screen.c
>>> +++ b/src/gallium/drivers/svga/svga_screen.c
>>> @@ -282,6 +282,7 @@ svga_get_param(struct pipe_screen *screen, enum
>>> pipe_cap param)
>>>       case PIPE_CAP_CONDITIONAL_RENDER_INVERTED:
>>>       case PIPE_CAP_SAMPLER_VIEW_TARGET:
>>>       case PIPE_CAP_CLIP_HALFZ:
>>> +   case PIPE_CAP_VERTEXID_NOOFFSET:
>>>          return 0;
>>>       case PIPE_CAP_MIN_MAP_BUFFER_ALIGNMENT:
>>>          return 64;
>>> diff --git a/src/gallium/drivers/vc4/vc4_screen.c
>>> b/src/gallium/drivers/vc4/vc4_screen.c
>>> index 18451bd..0ead51e 100644
>>> --- a/src/gallium/drivers/vc4/vc4_screen.c
>>> +++ b/src/gallium/drivers/vc4/vc4_screen.c
>>> @@ -167,6 +167,7 @@ vc4_screen_get_param(struct pipe_screen *pscreen,
>>> enum pipe_cap param)
>>>            case PIPE_CAP_CONDITIONAL_RENDER_INVERTED:
>>>            case PIPE_CAP_SAMPLER_VIEW_TARGET:
>>>            case PIPE_CAP_CLIP_HALFZ:
>>> +        case PIPE_CAP_VERTEXID_NOOFFSET:
>>>                    return 0;
>>>
>>>                    /* Stream output. */
>>> diff --git a/src/gallium/include/pipe/p_defines.h
>>> b/src/gallium/include/pipe/p_defines.h
>>> index 8c4e415..c5ee2e8 100644
>>> --- a/src/gallium/include/pipe/p_defines.h
>>> +++ b/src/gallium/include/pipe/p_defines.h
>>> @@ -572,6 +572,7 @@ enum pipe_cap {
>>>       PIPE_CAP_MAX_VERTEX_ATTRIB_STRIDE = 109,
>>>       PIPE_CAP_SAMPLER_VIEW_TARGET = 110,
>>>       PIPE_CAP_CLIP_HALFZ = 111,
>>> +   PIPE_CAP_VERTEXID_NOOFFSET,
>>>    };
>>>
>>>    #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 98f0670..c8bf448 100644
>>> --- a/src/gallium/include/pipe/p_shader_tokens.h
>>> +++ b/src/gallium/include/pipe/p_shader_tokens.h
>>> @@ -176,7 +176,9 @@ struct tgsi_declaration_interp
>>>    #define TGSI_SEMANTIC_SAMPLEPOS  25
>>>    #define TGSI_SEMANTIC_SAMPLEMASK 26
>>>    #define TGSI_SEMANTIC_INVOCATIONID 27
>>> -#define TGSI_SEMANTIC_COUNT      28 /**< number of semantic values */
>>> +#define TGSI_SEMANTIC_VERTEXID_ZEROBASE 28
>>> +#define TGSI_SEMANTIC_BASEVERTEX 29
>>> +#define TGSI_SEMANTIC_COUNT      30 /**< number of semantic values */
>>>
>>>    struct tgsi_declaration_semantic
>>>    {
>>>
>>
>> Otherwise, everything else looks OK to me.
>>
>> Reviewed-by: Brian Paul <brianp at vmware.com>
>>
>



More information about the mesa-dev mailing list