[Mesa-dev] [PATCH v2] i965: Fix point size with tessellation/geometry shaders in GLES.

Michael Schellenberger Costa mschellenbergercosta at googlemail.com
Fri Jun 3 08:13:37 UTC 2016


Hi Kenneth,

Am 03.06.2016 um 10:08 schrieb Kenneth Graunke:
> Our previous code worked for desktop GL, and ES without geometry or
> tessellation shaders.  But those features require fancier point size
> handling.  Fortunately, we can use one rule for all APIs.
> 
> Fixes a number of dEQP tests with EXT_tessellation_shader enabled:
> dEQP-GLES31.functional.tessellation_geometry_interaction.point_size.*
> 
> Cc: Ilia Mirkin <imirkin at alum.mit.edu>
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/drivers/dri/i965/brw_state.h     | 49 +++++++++++++++++++++++++++++++
>  src/mesa/drivers/dri/i965/gen6_sf_state.c |  5 ++--
>  src/mesa/drivers/dri/i965/gen7_sf_state.c |  7 +++--
>  src/mesa/drivers/dri/i965/gen8_sf_state.c |  7 +++--
>  4 files changed, 59 insertions(+), 9 deletions(-)
> 
> Hey Ilia,
> 
> Thanks for helping me figure out these rules on IRC yesterday.
> I think this version should work...
> 
>  --Ken
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_state.h b/src/mesa/drivers/dri/i965/brw_state.h
> index 0a4c21f..be7f6ce 100644
> --- a/src/mesa/drivers/dri/i965/brw_state.h
> +++ b/src/mesa/drivers/dri/i965/brw_state.h
> @@ -473,6 +473,55 @@ is_drawing_lines(const struct brw_context *brw)
>     return false;
>  }
>  
> +static inline bool
> +use_state_point_size(const struct brw_context *brw)
> +{
> +   const struct gl_context *ctx = &brw->ctx;
> +
> +   /* Section 14.4 (Points) of the OpenGL 4.5 specification says:
> +    *
> +    *    "If program point size mode is enabled, the derived point size is
> +    *     taken from the (potentially clipped) shader built-in gl_PointSize
> +    *     written by:
> +    *
> +    *        * the geometry shader, if active;
> +    *        * the tessellation evaluation shader, if active and no
> +    *          geometry shader is active;
> +    *        * the vertex shader, otherwise
> +    *
> +    *    and clamped to the implementation-dependent point size range.  If
> +    *    the value written to gl_PointSize is less than or equal to zero,
> +    *    or if no value was written to gl_PointSize, results are undefined.
> +    *    If program point size mode is disabled, the derived point size is
> +    *    specified with the command
> +    *
> +    *       void PointSize(float size);
> +    *
> +    *    size specifies the requested size of a point.  The default value
> +    *    is 1.0."
> +    *
> +    * The rules for GLES come from the ES 3.2, OES_geometry_point_size, and
> +    * OES_tessellation_point_size specifications.  To summarize: if the last
> +    * stage before rasterization is a GS or TES, then use gl_PointSize from
> +    * the shader if written.  Otherwise, use 1.0.  If the last stage is a
> +    * vertex shader, use gl_PointSize, or it is undefined.
> +    *
> +    * We can combine these rules into a single condition for both APIs.
> +    * Using the state point size when the last shader stage doesn't write
> +    * gl_PointSize satisfies GL's requirements, as it's undefined.  Because
> +    * ES doesn't have a PointSize() command, the state point size will
> +    * remain 1.0, satisfying the ES default value in the GS/TES case, and
> +    * the VS case (1.0 works for "undefined").  Mesa sets the program point
> +    * mode flag to always-enabled in ES, so we can safely check that, and
> +    * it'll be ignored for ES.
> +    *
> +    * _NEW_PROGRAM | _NEW_POINT
> +    * BRW_NEW_VUE_MAP_GEOM_OUT
> +    */
> +   return (!ctx->VertexProgram.PointSizeEnabled && !ctx->Point._Attenuated) ||
> +          (brw->vue_map_geom_out.slots_valid & VARYING_BIT_PSIZ) == 0;
That last condition looks decidedly weird. Are those brackets correct?
--Michael
> +}
> +
>  
>  #ifdef __cplusplus
>  }
> diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c b/src/mesa/drivers/dri/i965/gen6_sf_state.c
> index 0538ab7..94731e0 100644
> --- a/src/mesa/drivers/dri/i965/gen6_sf_state.c
> +++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c
> @@ -373,9 +373,8 @@ upload_sf_state(struct brw_context *brw)
>     if (multisampled_fbo && ctx->Multisample.Enabled)
>        dw3 |= GEN6_SF_MSRAST_ON_PATTERN;
>  
> -   /* _NEW_PROGRAM | _NEW_POINT */
> -   if (!(ctx->VertexProgram.PointSizeEnabled ||
> -	 ctx->Point._Attenuated))
> +   /* _NEW_PROGRAM | _NEW_POINT, BRW_NEW_VUE_MAP_GEOM_OUT */
> +   if (use_state_point_size(brw))
>        dw4 |= GEN6_SF_USE_STATE_POINT_WIDTH;
>  
>     /* _NEW_POINT - Clamp to ARB_point_parameters user limits */
> diff --git a/src/mesa/drivers/dri/i965/gen7_sf_state.c b/src/mesa/drivers/dri/i965/gen7_sf_state.c
> index d3a658c..8d49e24 100644
> --- a/src/mesa/drivers/dri/i965/gen7_sf_state.c
> +++ b/src/mesa/drivers/dri/i965/gen7_sf_state.c
> @@ -213,8 +213,8 @@ upload_sf_state(struct brw_context *brw)
>  
>     dw3 = GEN6_SF_LINE_AA_MODE_TRUE;
>  
> -   /* _NEW_PROGRAM | _NEW_POINT */
> -   if (!(ctx->VertexProgram.PointSizeEnabled || ctx->Point._Attenuated))
> +   /* _NEW_PROGRAM | _NEW_POINT, BRW_NEW_VUE_MAP_GEOM_OUT */
> +   if (use_state_point_size(brw))
>        dw3 |= GEN6_SF_USE_STATE_POINT_WIDTH;
>  
>     /* _NEW_POINT - Clamp to ARB_point_parameters user limits */
> @@ -256,7 +256,8 @@ const struct brw_tracked_state gen7_sf_state = {
>                 _NEW_SCISSOR,
>        .brw   = BRW_NEW_BLORP |
>                 BRW_NEW_CONTEXT |
> -               BRW_NEW_PRIMITIVE,
> +               BRW_NEW_PRIMITIVE |
> +               BRW_NEW_VUE_MAP_GEOM_OUT,
>     },
>     .emit = upload_sf_state,
>  };
> diff --git a/src/mesa/drivers/dri/i965/gen8_sf_state.c b/src/mesa/drivers/dri/i965/gen8_sf_state.c
> index d854b6d..0c4f1df 100644
> --- a/src/mesa/drivers/dri/i965/gen8_sf_state.c
> +++ b/src/mesa/drivers/dri/i965/gen8_sf_state.c
> @@ -172,8 +172,8 @@ upload_sf(struct brw_context *brw)
>     /* Clamp to the hardware limits and convert to fixed point */
>     dw3 |= U_FIXED(CLAMP(point_size, 0.125f, 255.875f), 3);
>  
> -   /* _NEW_PROGRAM | _NEW_POINT */
> -   if (!(ctx->VertexProgram.PointSizeEnabled || ctx->Point._Attenuated))
> +   /* _NEW_PROGRAM | _NEW_POINT, BRW_NEW_VUE_MAP_GEOM_OUT */
> +   if (use_state_point_size(brw))
>        dw3 |= GEN6_SF_USE_STATE_POINT_WIDTH;
>  
>     /* _NEW_POINT | _NEW_MULTISAMPLE */
> @@ -209,7 +209,8 @@ const struct brw_tracked_state gen8_sf_state = {
>                 _NEW_MULTISAMPLE |
>                 _NEW_POINT,
>        .brw   = BRW_NEW_BLORP |
> -               BRW_NEW_CONTEXT,
> +               BRW_NEW_CONTEXT |
> +               BRW_NEW_VUE_MAP_GEOM_OUT,
>     },
>     .emit = upload_sf,
>  };
> 


More information about the mesa-dev mailing list