[Mesa-dev] [PATCH] i965: Ignore glPointSize when GL_POINT_SIZE_ARRAY_OES is enabled
Kenneth Graunke
kenneth at whitecape.org
Tue Mar 1 20:07:46 UTC 2016
On Tuesday, March 1, 2016 6:39:49 PM PST Plamena Manolova wrote:
> When a user defines a point size array and enables it, the point
> size value set via glPointSize should be ignored. To achieve this,
> we can simply omit point size when creating a batch inside
> upload_sf_state for brw, gen6, gen7 and gen8.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=42187
> Signed-off-by: Plamena Manolova <plamena.manolova at intel.com>
Wow! Great work tracking this down!
So...not filling out these fields is equivalent to just setting
"use point size state" to false. We can still set the actual
point size (the big CLAMP expression) if we want.
I think a simpler fix would be to make core Mesa set the
ctx->VertexProgram.PointSizeEnabled boolean whenever
GL_POINT_SIZE_ARRAY_OES is enabled/disabled. Then, no driver changes
should be necessary.
vao->VertexAttrib[VERT_ATTRIB_POINT_SIZE].Enabled means "please read the
point size array". It makes ffvertex_prog.c copy the input point size
to the output varying. But ctx->VertexProgram.PointSizeEnabled essentially
means "use the value of the program's output varying", so I think it
makes sense for drivers to check that, and core Mesa to set it.
Does that seem reasonable?
Thanks for fixing this longstanding bug!
> ---
> src/mesa/drivers/dri/i965/brw_sf_state.c | 19 +++++++++++--------
> src/mesa/drivers/dri/i965/gen6_sf_state.c | 23 +++++++++++++----------
> src/mesa/drivers/dri/i965/gen7_sf_state.c | 17 ++++++++++-------
> src/mesa/drivers/dri/i965/gen8_sf_state.c | 17 ++++++++++-------
> 4 files changed, 44 insertions(+), 32 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_sf_state.c b/src/mesa/drivers/
dri/i965/brw_sf_state.c
> index b126f82..18504b1 100644
> --- a/src/mesa/drivers/dri/i965/brw_sf_state.c
> +++ b/src/mesa/drivers/dri/i965/brw_sf_state.c
> @@ -259,15 +259,18 @@ static void upload_sf_unit( struct brw_context *brw )
>
> /* _NEW_POINT */
> sf->sf7.sprite_point = ctx->Point.PointSprite;
> - sf->sf7.point_size = CLAMP(rintf(CLAMP(ctx->Point.Size,
> - ctx->Point.MinSize,
> - ctx->Point.MaxSize)), 1.0f,
255.0f) *
> - (1<<3);
> - /* _NEW_PROGRAM | _NEW_POINT */
> - sf->sf7.use_point_size_state = !(ctx->VertexProgram.PointSizeEnabled ||
> - ctx->Point._Attenuated);
> - sf->sf7.aa_line_distance_mode = 0;
>
> + if (!ctx->Array.VAO->VertexAttrib[VERT_ATTRIB_POINT_SIZE].Enabled) {
> + sf->sf7.point_size = CLAMP(rintf(CLAMP(ctx->Point.Size,
> + ctx->Point.MinSize,
> + ctx->Point.MaxSize)),
> + 1.0f, 255.0f) * (1<<3);
> + /* _NEW_PROGRAM | _NEW_POINT */
> + sf->sf7.use_point_size_state = !(ctx->VertexProgram.PointSizeEnabled
||
> + ctx->Point._Attenuated);
> + }
> +
> + sf->sf7.aa_line_distance_mode = 0;
> /* might be BRW_NEW_PRIMITIVE if we have to adjust pv for polygons:
> * _NEW_LIGHT
> */
> diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c b/src/mesa/drivers/
dri/i965/gen6_sf_state.c
> index 2634e6b..d506a2b 100644
> --- a/src/mesa/drivers/dri/i965/gen6_sf_state.c
> +++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c
> @@ -406,16 +406,19 @@ 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))
> - dw4 |= GEN6_SF_USE_STATE_POINT_WIDTH;
> -
> - /* Clamp to ARB_point_parameters user limits */
> - point_size = CLAMP(ctx->Point.Size, ctx->Point.MinSize, ctx-
>Point.MaxSize);
> -
> - /* Clamp to the hardware limits and convert to fixed point */
> - dw4 |= U_FIXED(CLAMP(point_size, 0.125f, 255.875f), 3);
> + if (!ctx->Array.VAO->VertexAttrib[VERT_ATTRIB_POINT_SIZE].Enabled) {
> + /* _NEW_PROGRAM | _NEW_POINT */
> + if (!(ctx->VertexProgram.PointSizeEnabled ||
> + ctx->Point._Attenuated))
> + dw4 |= GEN6_SF_USE_STATE_POINT_WIDTH;
> +
> + /* Clamp to ARB_point_parameters user limits */
> + point_size = CLAMP(ctx->Point.Size, ctx->Point.MinSize,
> + ctx->Point.MaxSize);
> +
> + /* Clamp to the hardware limits and convert to fixed point */
> + dw4 |= U_FIXED(CLAMP(point_size, 0.125f, 255.875f), 3);
> + }
>
> /*
> * Window coordinates in an FBO are inverted, which means point
> diff --git a/src/mesa/drivers/dri/i965/gen7_sf_state.c b/src/mesa/drivers/
dri/i965/gen7_sf_state.c
> index b1f13ac..a17e5a2 100644
> --- a/src/mesa/drivers/dri/i965/gen7_sf_state.c
> +++ b/src/mesa/drivers/dri/i965/gen7_sf_state.c
> @@ -213,15 +213,18 @@ 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))
> - dw3 |= GEN6_SF_USE_STATE_POINT_WIDTH;
> + if (!ctx->Array.VAO->VertexAttrib[VERT_ATTRIB_POINT_SIZE].Enabled) {
> + /* _NEW_PROGRAM | _NEW_POINT */
> + if (!(ctx->VertexProgram.PointSizeEnabled || ctx->Point._Attenuated))
> + dw3 |= GEN6_SF_USE_STATE_POINT_WIDTH;
>
> - /* Clamp to ARB_point_parameters user limits */
> - point_size = CLAMP(ctx->Point.Size, ctx->Point.MinSize, ctx-
>Point.MaxSize);
> + /* Clamp to ARB_point_parameters user limits */
> + point_size = CLAMP(ctx->Point.Size, ctx->Point.MinSize,
> + ctx->Point.MaxSize);
>
> - /* Clamp to the hardware limits and convert to fixed point */
> - dw3 |= U_FIXED(CLAMP(point_size, 0.125f, 255.875f), 3);
> + /* Clamp to the hardware limits and convert to fixed point */
> + dw3 |= U_FIXED(CLAMP(point_size, 0.125f, 255.875f), 3);
> + }
>
> /* _NEW_LIGHT */
> if (ctx->Light.ProvokingVertex != GL_FIRST_VERTEX_CONVENTION) {
> diff --git a/src/mesa/drivers/dri/i965/gen8_sf_state.c b/src/mesa/drivers/
dri/i965/gen8_sf_state.c
> index 8b6f31f..0146383 100644
> --- a/src/mesa/drivers/dri/i965/gen8_sf_state.c
> +++ b/src/mesa/drivers/dri/i965/gen8_sf_state.c
> @@ -167,15 +167,18 @@ upload_sf(struct brw_context *brw)
> dw2 |= GEN6_SF_LINE_END_CAP_WIDTH_1_0;
> }
>
> - /* Clamp to ARB_point_parameters user limits */
> - point_size = CLAMP(ctx->Point.Size, ctx->Point.MinSize, ctx-
>Point.MaxSize);
> + if (!ctx->Array.VAO->VertexAttrib[VERT_ATTRIB_POINT_SIZE].Enabled) {
> + /* Clamp to ARB_point_parameters user limits */
> + point_size = CLAMP(ctx->Point.Size, ctx->Point.MinSize,
> + ctx->Point.MaxSize);
>
> - /* Clamp to the hardware limits and convert to fixed point */
> - dw3 |= U_FIXED(CLAMP(point_size, 0.125f, 255.875f), 3);
> + /* 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))
> - dw3 |= GEN6_SF_USE_STATE_POINT_WIDTH;
> + /* _NEW_PROGRAM | _NEW_POINT */
> + if (!(ctx->VertexProgram.PointSizeEnabled || ctx->Point._Attenuated))
> + dw3 |= GEN6_SF_USE_STATE_POINT_WIDTH;
> + }
>
> /* _NEW_POINT | _NEW_MULTISAMPLE */
> if ((ctx->Point.SmoothFlag || ctx->Multisample._Enabled) &&
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160301/0bce37c1/attachment.sig>
More information about the mesa-dev
mailing list