Bug 93599

I forgot to mention that my CPU/GPU is an Intel(R) Core(TM) i7-4770R CPU, Iris Pro

Kind regards
Darius

(In reply to Darius Spitznagel from comment #0)
> <<<
> 1500: message: major shader compiler error 4: 0:16(1): error: #extension
> directive is not allowed in the middle of a shader
> 1500 @0 glCompileShader(shader = 440)
> 1500: warning: compilation failed
> 1500: warning: 0:16(1): error: #extension directive is not allowed in the
> middle of a shader
> >>>

Oh bother. :(  This is an application bug, and it can be worked around by setting the environment variable allow_glsl_extension_directive_midshader=true.  You can also set it in your driconf.

> Later...
> 
> <<<
> 334307: message: shader compiler issue 17597: VS vec4 shader: 46 inst, 0
> loops, 204 cycles, compacted 736 to 624 bytes.
> 334348 @0 glLinkProgram(program = 6704)
> 334348: warning: link failed
> 334348: warning: error: linking with uncompiled shader
> >>>

And this is surely caused by the previous message.  The shader that generated the '#extension directives..." message didn't compile, and this error is triggered when the application tries to link the failed shader.  The work-around for the previous problem should eliminate this message.

Once this is resolved, if the green flashes persist, can you see if you can reproduce the problem on upstream kernels?  If you can, is there any chance you could bisect?

> More later...
> 
> <<<
> 342445: message: api performance issue 17829: Exceeded state cache size
> limit.  Clearing the set of compiled programs, which will trigger recompiles
> >>>

This and the other messages are just performance hints for developers (application and driver).  You can ignore these.

(In reply to Ian Romanick from comment #2)
Hello Ian,
you are right. Exporting allow_glsl_extension_directive_midshader=true resolved the first two problems/errors, but it didn't solve the green flashes nor the black glitches.

I took your advice and installed newest kernel as of today (it's 4.4-rc8), but this also didn't solve it.

In the meantime I compiled Mesa 11.1.0 and tried both games on kernel 4.3.3 and 4.4-rc8 again...
> No green flashes nor black glitches > AHA!

OK, I have now a point where I can start to bisect.
I will do what I can to find the bad commit(s).

Thank you so far.

228d5a3f75086e92aaa01619a55d6c8ac7841e0e is the first bad commit
commit 228d5a3f75086e92aaa01619a55d6c8ac7841e0e
Author: Francisco Jerez <currojerez@riseup.net>
Date:   Tue Dec 8 18:53:57 2015 +0200

    i965: Hook up L3 partitioning state atom.
    
    Reviewed-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
    Reviewed-by: Kristian Høgsberg  <krh@bitplanet.net>

:040000 040000 529ab4c31e8341f54ab57b6b0ae13617cb6af42e e85f211790f17b9b3285c304e144abaf7cb44092 M	src

Hope this helps!

Hello Ian,

I dug a little bit more into this and found at the end of file src/mesa/drivers/dri/i965/gen7_l3_state.c this hack...

/**
 * Hack to restore the default L3 configuration.
 *
 * This will be called at the end of every batch in order to reset the L3
 * configuration to the default values for the time being until the kernel is
 * fixed.  Until kernel commit 6702cf16e0ba8b0129f5aa1b6609d4e9c70bc13b
 * (included in v4.1) we would set the MI_RESTORE_INHIBIT bit when submitting
 * batch buffers for the default context used by the DDX, which meant that any
 * context state changed by the GL would leak into the DDX, the assumption
 * being that the DDX would initialize any state it cares about manually.  The
 * DDX is however not careful enough to program an L3 configuration
 * explicitly, and it makes assumptions about it (URB size) which won't hold
 * and cause it to misrender if we let our L3 set-up to leak into the DDX.
 *
 * Since v4.1 of the Linux kernel the default context is saved and restored
 * normally, so it's far less likely for our L3 programming to interfere with
 * other contexts -- In fact restoring the default L3 configuration at the end
 * of the batch will be redundant most of the time.  A kind of state leak is
 * still possible though if the context making assumptions about L3 state is
 * created immediately after our context was active (e.g. without the DDX
 * default context being scheduled in between) because at present the DRM
 * doesn't fully initialize the contents of newly created contexts and instead
 * sets the MI_RESTORE_INHIBIT flag causing it to inherit the state from the
 * last active context.
 *
 * It's possible to realize such a scenario if, say, an X server (or a GL
 * application using an outdated non-L3-aware Mesa version) is started while
 * another GL application is running and happens to have modified the L3
 * configuration, or if no X server is running at all and a GL application
 * using a non-L3-aware Mesa version is started after another GL application
 * ran and modified the L3 configuration -- The latter situation can actually
 * be reproduced easily on IVB in our CI system.
 */
void
gen7_restore_default_l3_config(struct brw_context *brw)
{
   const struct brw_device_info *devinfo = brw->intelScreen->devinfo;
   /* For efficiency assume that the first entry of the array matches the
    * default configuration.
    */
   const struct brw_l3_config *const cfg = get_l3_configs(devinfo);
   assert(cfg == get_l3_config(devinfo,
                               get_default_l3_weights(devinfo, false, false)));

   if (cfg != brw->l3.config && brw->can_do_pipelined_register_writes) {
      setup_l3_config(brw, cfg);
      update_urb_size(brw, cfg);
      brw->l3.config = cfg;
   }
}

... when I change this to...

void
gen7_restore_default_l3_config(struct brw_context *brw) {}

...both games render correct with kernel 4.3.3 (tested).

But I am not sure if I disabled "L3 partitioning" at whole with this one or the hack which was/is my intension.
If I am right, maybe the autor (Francisco Jerez) could check this for correctness?
Otherwise I'm sorry.

Reassigning to Curro, since he wrote the patch. :)

Created attachment 120998 [details] [review]
gen7_stall_before_ro_invalidation.patch

Does the attached patch fix the corruption for you?

Thanks.

(In reply to Francisco Jerez from comment #7)
> Created attachment 120998 [details] [review] [review]
> gen7_stall_before_ro_invalidation.patch
> 
> Does the attached patch fix the corruption for you?
> 
> Thanks.

Yes, it does! Please push it to master before 11.2.0 final.
Thank you very much.

I would also appreciate it when both games would be added to Mesas included drirc like Dead Island, Second Life, etc. (see Comment 2 from Ian > allow_glsl_extension_directive_midshader=true).

Created attachment 121012 [details] [review]
gen7_stall_before_ro_invalidation-2.patch

Thanks for testing.  I'm attaching a (marginally) more optimal fix which avoids a redundant CS stall and HDC flush from the next pipe control, can you confirm whether it still gets rid of the corruption?

Thanks.

(In reply to Francisco Jerez from comment #9)
> Created attachment 121012 [details] [review] [review]
> gen7_stall_before_ro_invalidation-2.patch
> 
> Thanks for testing.  I'm attaching a (marginally) more optimal fix which
> avoids a redundant CS stall and HDC flush from the next pipe control, can
> you confirm whether it still gets rid of the corruption?
> 
> Thanks.

This optimized patch also works - there is no corruption!

Thanks again.

(In reply to Darius Spitznagel from comment #10)
> (In reply to Francisco Jerez from comment #9)
> > Created attachment 121012 [details] [review] [review] [review]
> > gen7_stall_before_ro_invalidation-2.patch
> > 
> > Thanks for testing.  I'm attaching a (marginally) more optimal fix which
> > avoids a redundant CS stall and HDC flush from the next pipe control, can
> > you confirm whether it still gets rid of the corruption?
> > 
> > Thanks.
> 
> This optimized patch also works - there is no corruption!
> 
> Thanks again.

Cool, I just added your tested-by tag and sent the fix for review to the mesa-dev mailing list:

http://lists.freedesktop.org/archives/mesa-dev/2016-January/104968.html

*** This bug has been marked as a duplicate of bug 93540 ***

Created attachment 121065 [details]
attachment-13944-0.html

On 01/15/2016 03:44 AM, bugzilla-daemon@freedesktop.org wrote:
>
> *Comment # 11 <https://bugs.freedesktop.org/show_bug.cgi?id=93599#c11> 
> on bug 93599 <https://bugs.freedesktop.org/show_bug.cgi?id=93599> from 
> Francisco Jerez <mailto:currojerez@riseup.net> *
> (In reply to Darius Spitznagel fromcomment #10 <show_bug.cgi?id=93599#c10>)
> > (In reply to Francisco Jerez fromcomment #9 <show_bug.cgi?id=93599#c9>) > > Created attachment 121012 [details] [review] 
> <attachment.cgi?id=121012> [details] 
> <attachment.cgi?id=121012&action=edit> [review] 
> <page.cgi?id=splinter.html&bug=93599&attachment=121012> [review] 
> [review] > > gen7_stall_before_ro_invalidation-2.patch > > > > Thanks 
> for testing. I'm attaching a (marginally) more optimal fix which > > 
> avoids a redundant CS stall and HDC flush from the next pipe control, 
> can > > you confirm whether it still gets rid of the corruption? > > > 
> > Thanks. > > This optimized patch also works - there is no 
> corruption! > > Thanks again.
>
> Cool, I just added your tested-by tag and sent the fix for review to the
> mesa-dev mailing list:
>
> http://lists.freedesktop.org/archives/mesa-dev/2016-January/104968.html
> ------------------------------------------------------------------------
> You are receiving this mail because:
>
>   * You reported the bug.
>
Hello Francisco,
I think you forgot the rename PIPE_CONTROL_DATA_CACHE_INVALIDATE also in 
"src/mesa/drivers/dri/i965/brw_misc_state.c".

(In reply to Darius Spitznagel from comment #13)
> Created attachment 121065 [details]
> attachment-13944-0.html
> 
> On 01/15/2016 03:44 AM, bugzilla-daemon@freedesktop.org wrote:
> >
> > *Comment # 11 <https://bugs.freedesktop.org/show_bug.cgi?id=93599#c11> 
> > on bug 93599 <https://bugs.freedesktop.org/show_bug.cgi?id=93599> from 
> > Francisco Jerez <mailto:currojerez@riseup.net> *
> > (In reply to Darius Spitznagel fromcomment #10 <show_bug.cgi?id=93599#c10>)
> > > (In reply to Francisco Jerez fromcomment #9 <show_bug.cgi?id=93599#c9>) > > Created attachment 121012 [details] [review] [review] 
> > <attachment.cgi?id=121012> [details] 
> > <attachment.cgi?id=121012&action=edit> [review] 
> > <page.cgi?id=splinter.html&bug=93599&attachment=121012> [review] 
> > [review] > > gen7_stall_before_ro_invalidation-2.patch > > > > Thanks 
> > for testing. I'm attaching a (marginally) more optimal fix which > > 
> > avoids a redundant CS stall and HDC flush from the next pipe control, 
> > can > > you confirm whether it still gets rid of the corruption? > > > 
> > > Thanks. > > This optimized patch also works - there is no 
> > corruption! > > Thanks again.
> >
> > Cool, I just added your tested-by tag and sent the fix for review to the
> > mesa-dev mailing list:
> >
> > http://lists.freedesktop.org/archives/mesa-dev/2016-January/104968.html
> > ------------------------------------------------------------------------
> > You are receiving this mail because:
> >
> >   * You reported the bug.
> >
> Hello Francisco,
> I think you forgot the rename PIPE_CONTROL_DATA_CACHE_INVALIDATE also in 
> "src/mesa/drivers/dri/i965/brw_misc_state.c".

I committed the patch that introduces that invalidation yesterday, after I had written and sent for review the series fixing for your problem, so it wasn't there at that point -- But thanks for reminding me to update it ;)

(In reply to Francisco Jerez from comment #14)
> I committed the patch that introduces that invalidation yesterday, after I
> had written and sent for review the series fixing for your problem, so it
> wasn't there at that point -- But thanks for reminding me to update it ;)
Yes you are right.

I wonder why your patches are still not in master;)

(In reply to Darius Spitznagel from comment #15)
> (In reply to Francisco Jerez from comment #14)
> > I committed the patch that introduces that invalidation yesterday, after I
> > had written and sent for review the series fixing for your problem, so it
> > wasn't there at that point -- But thanks for reminding me to update it ;)
> Yes you are right.
> 
> I wonder why your patches are still not in master;)

The patches are still missing a Reviewed-by, feel free to send a ping on the mailing list :)