On Tue, 2015-06-09 at 01:49 -0700, Kenneth Graunke wrote: > On Tuesday, June 09, 2015 08:49:54 AM Iago Toral Quiroga wrote: > > In commit fe74fee8fa721a we rounded the line width to the nearest integer to > > match the GLES3 spec requirements stated in section 13.4.2.1, but that seems > > to break a dEQP test that renders wide lines in some multisampling > > scenarios. > > > > Ian noted that the Open 4.4 spec has the following similar text: > > > > "The actual width of non-antialiased lines is determined by rounding the > > supplied width to the nearest integer, then clamping it to the > > implementation-dependent maximum non-antialiased line width." > > > > and suggested that when ES removed antialiased lines, they removed > > "non-antialised" from that paragraph but probably should not have. > > > > Going by that note, this patch restricts the quantization implemented in > > fe74fee8fa721a only to regular aliased lines. This seems to fix the broken > > test without causing regressions in any of the dEQP line rasterization tests > > (dEQP-GLES3.functional.rasterization.*line*). > > > > Fixes: > > dEQP-GLES3.functional.rasterization.fbo.rbo_multisample_max.primitives.lines_wide > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90749 > > --- > > src/mesa/drivers/dri/i965/gen6_sf_state.c | 5 +++-- > > src/mesa/drivers/dri/i965/gen7_sf_state.c | 6 ++++-- > > src/mesa/drivers/dri/i965/gen8_sf_state.c | 5 +++-- > > 3 files changed, 10 insertions(+), 6 deletions(-) > > > > diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c > > b/src/mesa/drivers/dri/i965/gen6_sf_state.c > > index e445ce2..b674268 100644 > > --- a/src/mesa/drivers/dri/i965/gen6_sf_state.c > > +++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c > > @@ -364,8 +364,9 @@ upload_sf_state(struct brw_context *brw) > > /* OpenGL dictates that line width should be rounded to the nearest > > * integer > > */ > > - float line_width = > > - roundf(CLAMP(ctx->Line.Width, 0.0, ctx->Const.MaxLineWidth)); > > + float line_width = CLAMP(ctx->Line.Width, 0.0, > > ctx->Const.MaxLineWidth); > > + if (!ctx->Multisample._Enabled && !ctx->Line.SmoothFlag) > > + line_width = CLAMP(roundf(line_width), 0.0, > > ctx->Const.MaxLineWidth); > > uint32_t line_width_u3_7 = U_FIXED(line_width, 7); > > > > /* Line width of 0 is not allowed when MSAA enabled */ > > diff --git a/src/mesa/drivers/dri/i965/gen7_sf_state.c > > b/src/mesa/drivers/dri/i965/gen7_sf_state.c > > index 58e3337..07b4394 100644 > > --- a/src/mesa/drivers/dri/i965/gen7_sf_state.c > > +++ b/src/mesa/drivers/dri/i965/gen7_sf_state.c > > @@ -195,9 +195,11 @@ upload_sf_state(struct brw_context *brw) > > /* OpenGL dictates that line width should be rounded to the nearest > > * integer > > */ > > - float line_width = > > - roundf(CLAMP(ctx->Line.Width, 0.0, ctx->Const.MaxLineWidth)); > > + float line_width = CLAMP(ctx->Line.Width, 0.0, > > ctx->Const.MaxLineWidth); > > + if (!ctx->Multisample._Enabled && !ctx->Line.SmoothFlag) > > + line_width = CLAMP(roundf(line_width), 0.0, > > ctx->Const.MaxLineWidth); > > uint32_t line_width_u3_7 = U_FIXED(line_width, 7); > > + > > /* Line width of 0 is not allowed when MSAA enabled */ > > if (ctx->Multisample._Enabled) { > > if (line_width_u3_7 == 0) > > diff --git a/src/mesa/drivers/dri/i965/gen8_sf_state.c > > b/src/mesa/drivers/dri/i965/gen8_sf_state.c > > index 52a21b6..6c765f0 100644 > > --- a/src/mesa/drivers/dri/i965/gen8_sf_state.c > > +++ b/src/mesa/drivers/dri/i965/gen8_sf_state.c > > @@ -157,8 +157,9 @@ upload_sf(struct brw_context *brw) > > /* OpenGL dictates that line width should be rounded to the nearest > > * integer > > */ > > - float line_width = > > - roundf(CLAMP(ctx->Line.Width, 0.0, ctx->Const.MaxLineWidth)); > > + float line_width = CLAMP(ctx->Line.Width, 0.0, ctx->Const.MaxLineWidth); > > + if (!ctx->Multisample._Enabled && !ctx->Line.SmoothFlag) > > + line_width = CLAMP(roundf(line_width), 0.0, ctx->Const.MaxLineWidth); > > uint32_t line_width_u3_7 = U_FIXED(line_width, 7); > > if (line_width_u3_7 == 0) > > line_width_u3_7 = 1; > > > > Why not drop the second CLAMP? Given our current MaxLineWidth values, > roundf() of a value clamped to [0, MaxLineWidth] will remain in-range.
I added the second CLAMP precisely to avoid enforcing the rule you suggest below because it is a bit artificial (in the sense that is not in the GL spec, which makes a explicit reference to the second clamp). I don't see a strong reason for going one way or the other though, so if you like this better I am fine with dropping the second clamp and adding the assert. > To enforce this, it would probably be good to add an assertion to > brw_initialize_context_constants(): > > /* For non-antialiased lines, we have to round the line width to the > * nearest whole number. Make sure that we don't advertise a line > * width that, when rounded, will be beyond the actual hardware > * maximum. > */ > assert(roundf(ctx->Const.MaxLineWidth) <= ctx->Const.MaxLineWidth)); > > With the second CLAMP dropped (or the suggestion refuted), > Reviewed-by: Kenneth Graunke <kenn...@whitecape.org> > > Thanks for fixing this, Iago! Thanks for clarifying the spec, Ian! _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev