[Mesa-dev] [PATCH] i965: Ignore glPointSize when GL_POINT_SIZE_ARRAY_OES is enabled
Manolova, Plamena
plamena.manolova at intel.com
Wed Mar 2 08:44:46 UTC 2016
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 <kenneth at 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.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 --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160302/31343a04/attachment.html>
More information about the mesa-dev
mailing list