[Mesa-dev] [PATCH] i965: do not round line width when multisampling or antialiasing are enabled

[Iago Toral]

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 <kenneth at whitecape.org>
> 
> Thanks for fixing this, Iago!  Thanks for clarifying the spec, Ian!