[Mesa-dev] [PATCH v2] i965: Fix point size with tessellation/geometry shaders in GLES.
Ilia Mirkin
imirkin at alum.mit.edu
Fri Jun 3 20:40:46 UTC 2016
On Fri, Jun 3, 2016 at 4:08 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> 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;
I believe that this is "overly" nice to buggy programs, but not
incorrect. The cases where this has to return true:
(a) point size is explicitly disabled
(b) ES2 context, and point size isn't emitted in TES/GS
And it has to return false when point size is enabled && "last stage"
emits a point size. In all other cases, you can do whatever. I believe
you're choosing to be nice and use the fixed function point size if a
point size isn't being emitted, which is fine but unnecessary.
Dunno what the _Attenuated thing is, but it was like that before too,
so I assume you know what you're doing.
In principle this makes sense to me, but I don't know what
vue_map_geom_out actually is, so I'm going to leave the actual review
to someone who knows the driver better.
-ilia
> +}
> +
>
> #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,
> };
> --
> 2.8.3
>
More information about the mesa-dev
mailing list