Bug 78679

The code needs to be fixed:

On an IronLake laptop here I can make the current implementation break badly  producing a global system freeze. To reproduce the problem the requirements are:

- Enable line smoothing: glEnable(GL_LINE_SMOOTH)
- Enable line rendering mode for front-faces: glPolygonMode(GL_FRONT, GL_LINE)
- Render a triangle

Long explanation:

The setup I describe above the makes the code in brw_wm_populate_key() set key->line_aa = AA_SOMETIMES, which will then make c->runtime_check_aads_emit be TRUE.

The original code removed with the mentioned commit checked c->runtime_check_aads_emit and did different things depending on its value. I think the current code for this, which now seems to live in fs_visitor::emit_fb_writes() handles the original case where runtime_check_aads_emit is FALSE, but does not consider the other case.

This works in Gen >= 6 in all cases because as far as I can see, the code in brw_wm_populate_key() will never set line_aa to anything other than AA_NEVER. The reason for this is that brw_wm_populate_key() checks the value of brw->reduced_primitive to decide the value of line_aa but this field is only set in Gen <= 5 (Gen >= 6 uses brw->primitive). I suppose these checks against reduced_primitive for the value of line_aa are only meaningful in Gen < 6 (otherwise this code is not correct for Gen >= 6).

I think the work to do here involves adding the code that handles the case were runtime_check_aads_emit is TRUE. The old code doing this was this:

-   else {
-      struct brw_reg v1_null_ud = vec1(retype(brw_null_reg(), BRW_REGISTER_TYPE_UD));
-      struct brw_reg ip = brw_ip_reg();
-      int jmp;
-      
-      brw_set_compression_control(p, BRW_COMPRESSION_NONE);
-      brw_set_conditionalmod(p, BRW_CONDITIONAL_Z);
-      brw_AND(p, 
-             v1_null_ud, 
-             get_element_ud(brw_vec8_grf(1,0), 6), 
-             brw_imm_ud(1<<26)); 
-
-      jmp = brw_JMPI(p, ip, ip, brw_imm_w(0)) - p->store;
-      {
-        emit_aa(c, arg1, 2);
-        fire_fb_write(c, 0, nr, target, eot);
-        /* note - thread killed in subroutine */
-      }
-      brw_land_fwd_jump(p, jmp);
-
-      /* ELSE: Shuffle up one register to fill in the hole left for AA:
-       */
-      fire_fb_write(c, 1, nr-1, target, eot);
-   }

Unfortunately, the code base has changed quite a bit since this code was removed so it is not as easy as putting the code back, many of the functions used do not exist any more and the code . I'll have to see if I can figure how to get an up-to-date version of this code in place.

I'll attach a sample program that reproduces the problem too.

Created attachment 99081 [details]
Sample program

Reproduces the problem freeze on IronLake

This is weird, but for some reason I cannot reproduce the GPU hang any more on IronLake... I guess the hang was being caused for some other reason.

Anyway, I have spent some time implementing the removed code paths used when runtime_check_aads_emit was set to TRUE and that are activated with the attached example on IronLake and I only get GPU hangs.

I have checked the generated assembly code with INTEL_DEBUG="wm"  and I don't see anything wrong with the generated program so my guess is that IronLake is not intended to be used like that (*). On the other hand, with the current code the AA cases all seem to work fine in IronLake: whether I render antialiased lines or wire-frame polygons it seems to be doing  antialiasing properly so I suspect runtime_check_aads_emit was only relevant in Gen <= 4 but Gen 5 is okay without it.

To support this hypothesis even further there is the fact that the runtime_check_aads_emit flag was only checked in brw_wm_emit.c, which as far as I can see handles Gen4 code generation only.

Unfortunately, I don't have any Gen4 hardware here so I cannot verify this.

(*) Actually, I found that SIMD16 code generated was incorrect due to wrong assignment of GRF registers. I could fix this for my tests by altering fs_visitor::run() to use assign_regs_trivial() instead of assign_regs() though, but I wonder if there is a bug lurking in there anyway. I will send an e-mail to mesa-dev describing this situation in more detail.

Created attachment 99498 [details]
Sample program

This new version of the sample program does show a problem in IronLake.

Although the front face of the polygon (rendered in line mode) seems to be fine, the back face of the polygon does not render properly. This program showcases this by rotating the triangle to show both faces.

Interestingly, this problem only happens in the cases that activate the runtime_check_aads_emit flag (line antialiasing + one polygon face in line mode), however, I also tested this example with a checkout from before the commit mentioned in the bug report that still had the old code and the problem persists, so this issue has always existed in IronLake.

Created attachment 99623 [details] [review]
Patch for testing

This patch provides two different solutions for the problem that can be selected via an environment variable. One uses the JMPI instruction as in the original code but at least in my IronLake hardware it does not produce correct rendering of the GL_FILL face of the polygon. The other one uses IF/ELSE/ENDIF instructions and does produce correct results in IronLake. I don't know why only the latter works, maybe someone can look at the code and point out the problem.

To test, run the sample program (line_aa) attached to this bug report like this:

# Test solution with IF/END/ELSE
./line_aa

# Test solution with JMPI
INTEL_TRY_AA_JMPI=1 ./line_aa

The correct result should show the front face of the triangle rendered in GL_LINE mode with antialiased lines and the back face rendered in GL_FILL mode with smooth interpolation of the red, green and blue vertices.

I am particularly interested in having this tested in Gen < 5 since I don't have that hardware.

> # Test solution with JMPI
> INTEL_TRY_AA_JMPI=1 ./line_aa

Actually, this should be:
INTEL_AA_TRY_JMPI=1 ./line_aa

Created attachment 99676 [details]
gm45 line_aa test

(In reply to comment #4)
> Created attachment 99498 [details]
> Sample program
> 
> This new version of the sample program does show a problem in IronLake.
> 
> Although the front face of the polygon (rendered in line mode) seems to be
> fine, the back face of the polygon does not render properly. This program
> showcases this by rotating the triangle to show both faces.
> 
> Interestingly, this problem only happens in the cases that activate the
> runtime_check_aads_emit flag (line antialiasing + one polygon face in line
> mode), however, I also tested this example with a checkout from before the
> commit mentioned in the bug report that still had the old code and the
> problem persists, so this issue has always existed in IronLake.

Hi Iago

I can test on gm45, if you update the patch, since "struct brw_wm_compile (better known as 'c')" is gone.

I've attached a screenchot with the back face rendering on gm45.

Regards Sergej

(In reply to comment #8)
> Hi Iago
> 
> I can test on gm45, if you update the patch, since "struct brw_wm_compile
> (better known as 'c')" is gone.

Great, I'll update the patch and attach the new version here on Monday.

> I've attached a screenchot with the back face rendering on gm45.

As expected the problem exists also on gm45. Hopefully the patch will fix this for both gens then.

Thanks Sergej!

Created attachment 99840 [details] [review]
Updated patch for testing

Ugh, unfortunately the changes made to master affected my patch in various ways besides the removal of 'c': brw_JMPI is now private to sf so I can't use it from the generator code... since I don't know the reasons for that change for now I'll just drop the JMPI solution (which did not work in ironlake anyway) and go with the IF/ELSE/ENDIF solution only, so no more environment variables required to test.

This patch is a quick rework and it is not ready for submission to the mailing list, if this works on Gen4 I'll rework it properly before sending to the mailing list for review.

Created attachment 99856 [details] [review]
Updated patch for testing (visitor version)

This patch implements the same behavior as the previous one, but moves the implementation from the generator to the visitor, which changes a few things, but I think is the right thing to do in this case since we need to acquire a temporary register.

Created attachment 99865 [details] [review]
Updated patch for testing (visitor version, final)

Mostly the same as the last patch, but this one is ready to be sent to the mailing list for review if it works in Gen4.

With the updated patch I am getting a solid blue back face.

PS:
I didn't pay attention to it last time, but color interpolation is broken for the lines(front face) too, independent of your patch. The lower edge flips from red to green during the rotation. I'll attach the screenshots just in case...

Created attachment 99875 [details]
line_aa interpolation

Created attachment 99876 [details]
unpatched interpolation

(In reply to comment #13)

Thanks for testing:

> With the updated patch I am getting a solid blue back face.

Mmm... we got rid of the noise but the back face should be color-interpolated so there is still something wrong.  I'd need to narrow the problem down to some of the features we are using here:

Could you try the following?

1. Disable AA only: Remove glEnable(GL_LINE_SMOOTH)
2. Disable blending only: remove glEnable(GL_BLEND)
3. Disable AA and blending
4. Do 1-3 again but removing also glPolygonMode(GL_FRONT, GL_LINE)

In the meantime, considering that the original code back in the day used JMPI to implement this in gm45 I'll try to cook a patch that uses this. It did not work for me in ironlake, but maybe it does for gm45, otherwise I guess it might have always been broken like this.

> PS:
> I didn't pay attention to it last time, but color interpolation is broken
> for the lines(front face) too, independent of your patch. The lower edge
> flips from red to green during the rotation. I'll attach the screenshots
> just in case...

Yes, that happens in ironlake too, but as you say this was independent from the patch and is a different problem.

(In reply to comment #16)
> (In reply to comment #13)
> Could you try the following?
> 
> 1. Disable AA only: Remove glEnable(GL_LINE_SMOOTH)
> 2. Disable blending only: remove glEnable(GL_BLEND)
> 3. Disable AA and blending
> 4. Do 1-3 again but removing also glPolygonMode(GL_FRONT, GL_LINE)

0. Patched (original): Upper edges interpolate, back face is blue.

1. Disable AA only: No interpolation for upper edges, they are blue as the back face.

2. Disable blending only: Same as 0 (has no effect).

3. Disable AA and blending: Same as 1.

4. Disable line front: Interpolates correctly on both sides independent of other settings.

Also

5. Enable line front and back: Same as 1 for both sides.

6. Disable line front, enable line back: Front face is blue, shorter upper edge is blue, longer upper edge still interpolates.

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #13)
> > Could you try the following?
> > 
> > 1. Disable AA only: Remove glEnable(GL_LINE_SMOOTH)
> > 2. Disable blending only: remove glEnable(GL_BLEND)
> > 3. Disable AA and blending
> > 4. Do 1-3 again but removing also glPolygonMode(GL_FRONT, GL_LINE)
> 
> 0. Patched (original): Upper edges interpolate, back face is blue.

In ironlake only one of the upper edges interpolates, back face is properly interpolated.

> 1. Disable AA only: No interpolation for upper edges, they are blue as the
> back face.

Same in ironlake, except for the back face that is interpolated.

> 2. Disable blending only: Same as 0 (has no effect).
> 
> 3. Disable AA and blending: Same as 1.

Same as ironlake (in the sense that they don't seem to affect the results obtained in 0 and 1 respectively)

> 4. Disable line front: Interpolates correctly on both sides independent of
> other settings.

Interesting, looks like the flat rendering of the back face in Gen4 is unaffected by AA and is only related to the selected polygon modes.

> Also
> 
> 5. Enable line front and back: Same as 1 for both sides.
> 
> 6. Disable line front, enable line back: Front face is blue, shorter upper
> edge is blue, longer upper edge still interpolates.

Based on these results there are two issues:

1) In gen4-5: wrong color interpolation of the GL_LINE face of the polygons.
2) In gen4: wrong color interpolation of the GL_FILL face of the polygon when the other face is GL_LINE.

These issues seem to exist with and without AA settings in general and seem to be related mostly with the polygon modes.

With this info I think I'll send the patches to the mailing list for review since they seem to fix some problems and we should probably leave this bug open to track the remaining problems (or maybe open a new bug since I don't know if these issues existed before 098acf6c84333edbb7b1228545e4bdb2572ee0cd too).

Thanks Sergej, that was useful.

Fixed by dc2d3a7f5c217a7cee92380fbf503924a9591bea?

Iago fixed a bunch of the problems, but I think he uncovered a few related bugs that aren't yet fixed.  We should probably update the bug title to reflect those.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/mesa/mesa/issues/1443.