[Mesa-dev] [PATCH] i965: do not round line width when multisampling or antialiasing are enabled
Kenneth Graunke
kenneth at whitecape.org
Tue Jun 9 10:38:31 PDT 2015
On Tuesday, June 09, 2015 11:29:48 AM Iago Toral wrote:
> 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.
I'm a bit confused, then. Didn't you already enforce that artificial
rule in fe74fee8fa721a42448470e063870d24f9453dab, where you reduced
MaxLineWidth from 7.875 to 7.375 so that roundf(MaxLineWidth) would be
7.0 (in range) and not 8.0 (out of range)? Didn't tests expect it to
round to the nearest integer (8.0) and not be clamped to 7.875?
What I meant is that (on IVB) these two properties must hold:
assert(ctx->Const.MaxLineWidthAA <= 7.9921875);
assert(roundf(ctx->Const.MaxLineWidth) <= 7.9921875);
i.e. whatever we advertise can't cause us to go over the hardware limit.
> > 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 <kenneth at whitecape.org>
> >
> > Thanks for fixing this, Iago! Thanks for clarifying the spec, Ian!
>
>
-------------- 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: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150609/f82e27d5/attachment.sig>
More information about the mesa-dev
mailing list