Bug 77742

Created attachment 98113 [details] [review]
Patch

This patch adds support for instruction compaction to ILK and previous architectures (G45 and i965).

I have access to a testing machine with ILK. I have tested it with a simple program which runs this new code without any visual difference. However I don't know this is enough or if there is another method to check whether this compaction code is correctly working or not by using something like a piglit test or other.

(In reply to comment #1)
> Created attachment 98113 [details] [review] [review]
> Patch
> 
> This patch adds support for instruction compaction to ILK and previous
> architectures (G45 and i965).
> 
> I have access to a testing machine with ILK. I have tested it with a simple
> program which runs this new code without any visual difference. However I
> don't know this is enough or if there is another method to check whether
> this compaction code is correctly working or not by using something like a
> piglit test or other.

Great. Thanks! That was quick.

It's unclear from the i965 docs whether it actually supports instruction compaction (the docs in those days often shipped after changes started being made for the next chip). Eric's initial instruction compaction commit says GM45 and newer support it. We should probably change the gen4_ prefix to gm45_ and then do two small fix ups to only allow compaction on gen4 iff brw->is_g4x is true.

With those changes and a passing piglit run on ILK, I think the patch would be good to go. Feel free to send to the list. We'll find a GM45 system to test around the office.

From there we should test whether the original gen4 supports instruction compaction.

Comment on attachment 98113 [details] [review]
Patch

The patch is wrong. I found a couple of things: first there is one value on the first struct that was wrong. Second that even with the fix, piglit tests gave me dmesg errors related to the render ring after a couple of thousands of tests.

I'm going to investigate more this issue.

Thanks for your tips, Matt!

Created attachment 98676 [details] [review]
Patch

This is the patch I had developed with my errors fixed... but I couldn't make it work on Ironlake. As I said in the previous comment, I found that in Ironlake it outputs these kernel messages when I run piglit tests (after some thousands of them):

[...]
[757604.292530] [drm] stuck on render ring
[757604.292581] [drm] GPU crash dump saved to /sys/class/drm/card0/error
[757604.292583] [drm] GPU hangs can indicate a bug anywhere in the entire gfx stack, including userspace.
[757604.292585] [drm] Please file a _new_ bug report on bugs.freedesktop.org against DRI -> DRM/Intel
[757604.292587] [drm] drm/i915 developers can then reassign to the right component if it's not a kernel issue.
[757604.292588] [drm] The gpu crash dump is required to analyze gpu hangs, so please always attach it.
[757606.269708] [drm] stuck on render ring
[757608.270874] [drm] stuck on render ring
[...]

I don't know if in G45 architecture this patch would work (the struct values are copied from its docs) because I don't access to a testing machine that has it.

It might be that Ironlake needs different values or that my patch is directly wrong. If you want I can submit the GPU crash dump file if needed.

Thanks

I've sent patches for this.

Patches finally committed:

commit 14e44f896f10f9ce1533e149d634137a3b6b807d
Author: Matt Turner <mattst88@gmail.com>
Date:   Wed Aug 20 11:43:29 2014 -0700

    i965/compaction: Add support for G45.
    
    Acked-by: Kenneth Graunke <kenneth@whitecape.org>
    Reviewed-by: Jason Ekstrand <jason.ekstrand@intel.com>


commit 64c0f62018d0182ccf65e58ac53318b986986288
Author: Matt Turner <mattst88@gmail.com>
Date:   Thu Apr 24 10:02:35 2014 +0200

    i965/compaction: Add support for Gen5.
    
    Acked-by: Kenneth Graunke <kenneth@whitecape.org>
    Reviewed-by: Jason Ekstrand <jason.ekstrand@intel.com>
    Acked-by: Ian Romanick <ian.d.romanick@intel.com>

(In reply to comment #6)
> Patches finally committed:
> 
> commit 14e44f896f10f9ce1533e149d634137a3b6b807d
> Author: Matt Turner <mattst88@gmail.com>
> Date:   Wed Aug 20 11:43:29 2014 -0700
> 
>     i965/compaction: Add support for G45.
>     
>     Acked-by: Kenneth Graunke <kenneth@whitecape.org>
>     Reviewed-by: Jason Ekstrand <jason.ekstrand@intel.com>
> 
> 
> commit 64c0f62018d0182ccf65e58ac53318b986986288
> Author: Matt Turner <mattst88@gmail.com>
> Date:   Thu Apr 24 10:02:35 2014 +0200
> 
>     i965/compaction: Add support for Gen5.
>     
>     Acked-by: Kenneth Graunke <kenneth@whitecape.org>
>     Reviewed-by: Jason Ekstrand <jason.ekstrand@intel.com>
>     Acked-by: Ian Romanick <ian.d.romanick@intel.com>


Ironlake:

This segfaults for me in brw_eu_compact.c:1407 when compiled with -m32.
64bit is working fine.

(In reply to comment #7)
> (In reply to comment #6)
> > Patches finally committed:
> > 
> > commit 14e44f896f10f9ce1533e149d634137a3b6b807d
> > Author: Matt Turner <mattst88@gmail.com>
> > Date:   Wed Aug 20 11:43:29 2014 -0700
> > 
> >     i965/compaction: Add support for G45.
> >     
> >     Acked-by: Kenneth Graunke <kenneth@whitecape.org>
> >     Reviewed-by: Jason Ekstrand <jason.ekstrand@intel.com>
> > 
> > 
> > commit 64c0f62018d0182ccf65e58ac53318b986986288
> > Author: Matt Turner <mattst88@gmail.com>
> > Date:   Thu Apr 24 10:02:35 2014 +0200
> > 
> >     i965/compaction: Add support for Gen5.
> >     
> >     Acked-by: Kenneth Graunke <kenneth@whitecape.org>
> >     Reviewed-by: Jason Ekstrand <jason.ekstrand@intel.com>
> >     Acked-by: Ian Romanick <ian.d.romanick@intel.com>
> 
> 
> Ironlake:
> 
> This segfaults for me in brw_eu_compact.c:1407 when compiled with -m32.
> 64bit is working fine.

Thanks for the report. Can you post a backtrace of the segfault?

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > Patches finally committed:
> > > 
> > > commit 14e44f896f10f9ce1533e149d634137a3b6b807d
> > > Author: Matt Turner <mattst88@gmail.com>
> > > Date:   Wed Aug 20 11:43:29 2014 -0700
> > > 
> > >     i965/compaction: Add support for G45.
> > >     
> > >     Acked-by: Kenneth Graunke <kenneth@whitecape.org>
> > >     Reviewed-by: Jason Ekstrand <jason.ekstrand@intel.com>
> > > 
> > > 
> > > commit 64c0f62018d0182ccf65e58ac53318b986986288
> > > Author: Matt Turner <mattst88@gmail.com>
> > > Date:   Thu Apr 24 10:02:35 2014 +0200
> > > 
> > >     i965/compaction: Add support for Gen5.
> > >     
> > >     Acked-by: Kenneth Graunke <kenneth@whitecape.org>
> > >     Reviewed-by: Jason Ekstrand <jason.ekstrand@intel.com>
> > >     Acked-by: Ian Romanick <ian.d.romanick@intel.com>
> > 
> > 
> > Ironlake:
> > 
> > This segfaults for me in brw_eu_compact.c:1407 when compiled with -m32.
> > 64bit is working fine.
> 
> Thanks for the report. Can you post a backtrace of the segfault?

#0  0xf77c7607 in brw_compact_instructions (p=0xffffa994, start_offset=0, num_annotations=0, annotation=0x0) at brw_eu_compact.c:1453
#1  0xf77b0cbf in compile_clip_prog (key=<optimized out>, brw=<optimized out>) at brw_clip.c:112
#2  brw_upload_clip_prog (brw=0xf7fb501c) at brw_clip.c:254
#3  0xf780e981 in brw_upload_state (brw=0xf7fb501c) at brw_state_upload.c:653
#4  0xf77c3231 in brw_try_draw_prims (indirect=<optimized out>, max_index=<optimized out>, min_index=<optimized out>, ib=<optimized out>, nr_prims=<optimized out>, 
    prims=<optimized out>, arrays=<optimized out>, ctx=<optimized out>) at brw_draw.c:468
#5  brw_draw_prims (ctx=0xffffad88, prims=0xffffad88, nr_prims=1, ib=0x0, index_bounds_valid=1 '\001', min_index=0, max_index=3, unused_tfb_object=0x0, indirect=0x0)
    at brw_draw.c:568
#6  0xf760f8fa in vbo_draw_arrays (ctx=ctx@entry=0xf7fb501c, mode=mode@entry=6, start=0, count=4, numInstances=1, baseInstance=0)
    at ../../src/mesa/vbo/vbo_exec_array.c:657
#7  0xf760fb47 in vbo_exec_DrawArrays (mode=6, start=0, count=4) at ../../src/mesa/vbo/vbo_exec_array.c:819
#8  0xf76a88d6 in meta_clear (ctx=0xf7fb501c, buffers=4294944032, glsl=true) at ../../src/mesa/drivers/common/meta.c:1804
#9  0xf77b065f in brw_clear (ctx=0xf7fb501c, mask=0) at brw_clear.c:263
#10 0xf74e619a in _mesa_Clear (mask=16640) at ../../src/mesa/main/clear.c:225
#11 0x08049efd in ?? ()
#12 0x080496ba in ?? ()
#13 0xf7bdbe5e in __libc_start_main () from /usr/lib32/libc.so.6
#14 0x08049ce3 in ?? ()