<div dir="ltr">Hi Kenneth,<div>You're absolutely right, just setting ctx->VertexProgram.PointSizeEnabled would be much simpler and cleaner, I'll go ahead and update the patch. Thank you for reviewing!</div></div><div class="gmail_extra"> <div class="gmail_quote">On Tue, Mar 1, 2016 at 10:07 PM, Kenneth Graunke <<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=42187" rel="noreferrer" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=42187</a> > Signed-off-by: Plamena Manolova <<a href="mailto:plamena.manolova@intel.com">plamena.manolova@intel.com</a>> 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! <div class="HOEnZb"><div class="h5"> > --- > 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) && > </div></div></blockquote></div> </div>