Hi Kenneth, 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!
On Tue, Mar 1, 2016 at 10:07 PM, Kenneth Graunke <kenn...@whitecape.org> wrote: > 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.manol...@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) && > > > >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
