* Re: [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock
@ 2011-11-07 18:14 Nicolas Kalkhof
2011-11-07 18:36 ` Daniel Vetter
0 siblings, 1 reply; 16+ messages in thread
From: Nicolas Kalkhof @ 2011-11-07 18:14 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Daniel Vetter, intel-gfx
Hi Daniel,
ok here is the sysprof result (see below). No cpu/gpu intensive apps are running, just the idling desktop. CPU Temperature is low and frequency is throttled down to 800 MHz like it should be. The System is responsive and behaves normal but top shows full cpu load. Honestly I cannot see the reason why X and gkrellm should eat up all the gpu since they clearly don't hog the cpu. Maybe the kernel reports wrong cpu stats? Then again the load goes down when I kill X! Hope this gives you a clue.
If you need more info, please let me know.
Yours,
Nic
[Everything] 0.00% 100.00%
[/usr/bin/X] 0.00% 29.26%
No map [/usr/bin/X] 0.00% 12.95%
select 0.00% 11.79%
- - kernel - - 0.00% 11.79%
_raw_spin_unlock_irqrestore 2.91% 2.91%
unix_poll 1.60% 1.60%
fget_light 1.16% 1.16%
sock_poll 0.73% 0.73%
do_select 0.58% 0.58%
fput 0.44% 0.44%
__pollwait 0.44% 0.44%
get_work_gcwq 0.44% 0.44%
get_page_from_freelist 0.44% 0.44%
evdev_poll 0.29% 0.29%
_raw_spin_lock_irqsave 0.29% 0.29%
finish_task_switch 0.29% 0.29%
copy_user_generic_unrolled 0.29% 0.29%
n_tty_poll 0.15% 0.15%
start_flush_work 0.15% 0.15%
tty_poll 0.15% 0.15%
datagram_poll 0.15% 0.15%
get_signal_to_deliver 0.15% 0.15%
next_zones_zonelist 0.15% 0.15%
__alloc_pages_nodemask 0.15% 0.15%
core_sys_select 0.15% 0.15%
sys_rt_sigreturn 0.15% 0.15%
_cond_resched 0.15% 0.15%
save_i387_xstate 0.15% 0.15%
timerfd_poll 0.15% 0.15%
hrtimer_init 0.15% 0.15%
In file /usr/lib64/xorg/modules/drivers/intel_drv.so 0.73% 0.87%
- - kernel - - 0.00% 0.15%
retint_signal 0.15% 0.15%
__errno_location 0.15% 0.15%
WakeupHandler 0.15% 0.15%
ioctl 0.00% 3.35%
In file /usr/lib64/xorg/modules/drivers/intel_drv.so 2.62% 2.62%
In file /usr/lib64/libpixman-1.so.0.23.9 2.04% 2.04%
__read 0.15% 1.46%
In file /usr/bin/Xorg 1.46% 1.46%
In file /lib64/libc-2.13.so 1.02% 1.16%
In file /usr/lib64/xorg/modules/input/synaptics_drv.so 0.58% 0.87%
fbPixmapToRegion 0.29% 0.44%
fbBlt 0.29% 0.29%
ChangeGC 0.29% 0.29%
ReadRequestFromClient 0.15% 0.15%
mmap 0.00% 0.15%
ProcQueryPointer 0.15% 0.15%
In file [heap] 0.00% 0.15%
fbPolyline32 0.15% 0.15%
In file /usr/lib64/xorg/modules/input/evdev_drv.so 0.15% 0.15%
dixLookupDrawable 0.15% 0.15%
ChangeWindowAttributes 0.15% 0.15%
GetExtensionEntry 0.15% 0.15%
ValidatePicture 0.15% 0.15%
dixLookupResourceByClass 0.15% 0.15%
timerfd_settime 0.15% 0.15%
IsMaster 0.15% 0.15%
malloc 0.15% 0.15%
dixLookupResourceByType 0.15% 0.15%
ConfigureWindow 0.15% 0.15%
[gkrellm] 0.00% 26.49%
read 0.15% 12.66%
- - kernel - - 0.00% 12.52%
format_decode 3.35% 3.35%
vsnprintf 2.04% 2.04%
string.clone.2 1.46% 1.46%
number.clone.1 1.16% 1.16%
seq_printf 0.58% 0.58%
e1000e_update_stats 0.58% 0.58%
show_stat 0.29% 0.29%
put_dec_trunc 0.29% 0.29%
strnlen 0.29% 0.29%
kstat_irqs 0.29% 0.29%
part_round_stats 0.15% 0.15%
all_vm_events 0.15% 0.15%
dev_seq_printf_stats 0.15% 0.15%
acpi_ds_exec_end_op 0.15% 0.15%
put_dec_full 0.15% 0.15%
kref_put 0.15% 0.15%
get_device 0.15% 0.15%
diskstats_show 0.15% 0.15%
seq_read 0.15% 0.15%
irq_to_desc 0.15% 0.15%
vmstat_show 0.15% 0.15%
vmstat_next 0.15% 0.15%
system_call_after_swapgs 0.15% 0.15%
jiffies_to_msecs 0.15% 0.15%
sys_read 0.15% 0.15%
__isoc99_vsscanf 0.00% 1.31%
No map [gkrellm] 0.00% 1.16%
In file /usr/lib64/libcairo.so.2.11000.2 0.87% 0.87%
In file /usr/lib64/libpango-1.0.so.0.2904.0 0.58% 0.58%
In file /usr/lib64/libgdk-x11-2.0.so.0.2400.7 0.44% 0.44%
In file /lib64/libc-2.13.so 0.29% 0.44%
g_type_check_instance_is_a 0.44% 0.44%
pango_default_break 0.44% 0.44%
In file /usr/lib64/libpangocairo-1.0.so.0.2904.0 0.29% 0.29%
g_type_instance_get_private 0.29% 0.29%
In file /usr/lib64/libglib-2.0.so.0.3000.1 0.15% 0.29%
_XUpdateGCCache 0.29% 0.29%
_IO_file_seekoff 0.29% 0.29%
__open64 0.00% 0.29%
g_markup_parse_context_parse 0.29% 0.29%
gdk_gc_get_type 0.29% 0.29%
In file /usr/bin/gkrellm 0.29% 0.29%
In file /usr/lib64/libfreetype.so.6.7.2 0.29% 0.29%
update_host 0.15% 0.15%
In file /usr/lib64/gkrellm2/plugins/cpufreq.so 0.15% 0.15%
g_unichar_type 0.15% 0.15%
g_type_fundamental 0.15% 0.15%
pango_renderer_set_matrix 0.15% 0.15%
g_object_ref 0.15% 0.15%
gdk_draw_drawable 0.00% 0.15%
In file /lib64/libpthread-2.13.so 0.15% 0.15%
free 0.15% 0.15%
poll 0.00% 0.15%
g_type_create_instance 0.15% 0.15%
g_object_unref 0.15% 0.15%
_XFlushGCCache 0.15% 0.15%
g_pointer_bit_unlock 0.15% 0.15%
cairo_close_path 0.15% 0.15%
xcb_writev 0.15% 0.15%
pango_layout_line_get_extents 0.15% 0.15%
gdk_pango_renderer_get_type 0.15% 0.15%
pango_renderer_deactivate 0.15% 0.15%
__pthread_mutex_lock 0.15% 0.15%
In file /usr/lib64/libX11.so.6.3.0 0.15% 0.15%
pango_ot_buffer_output 0.15% 0.15%
g_type_class_peek_static 0.15% 0.15%
cairo_pattern_destroy 0.15% 0.15%
cairo_destroy 0.15% 0.15%
g_unichar_get_script 0.15% 0.15%
pango_script_iter_next 0.15% 0.15%
open 0.00% 0.15%
pango_renderer_set_color 0.15% 0.15%
g_malloc_n 0.15% 0.15%
XSetClipMask 0.15% 0.15%
gkrellm_update_krell 0.15% 0.15%
finite 0.15% 0.15%
g_object_newv 0.15% 0.15%
lseek64 0.00% 0.15%
g_datalist_id_get_data 0.15% 0.15%
[kicker [kdeinit]] 0.00% 8.88%
[/opt/apps/firefox/bin/firefox] 0.00% 6.40%
[sysprof] 0.00% 5.97%
[top] 0.00% 5.68%
[/opt/java/jdk/jre/bin/java] 0.00% 5.24%
[konsole [kdeinit]] 0.00% 5.09%
[kcpuload] 0.00% 2.47%
[/opt/apps/claws-mail/bin/claws-mail] 0.00% 1.75%
[kwin [kdeinit] -session 10d66f6e7a000128602678900000074690000_1320681] 0.00% 0.87%
[kworker/u:2] 0.00% 0.44%
[konsole [kdeinit] -session 10d66f6e7a000132061162800000028890007_1320] 0.00% 0.44%
[konqueror [kdeinit] -session 10d66f6e7a000132061213500000034570005_13] 0.00% 0.29%
[kworker/0:0] 0.00% 0.29%
[kwrapper] 0.00% 0.15%
[kworker/1:0] 0.00% 0.15%
[/usr/bin/wpa_cli] 0.00% 0.15%
-----Ursprüngliche Nachricht-----
Von: "Daniel Vetter" <daniel@ffwll.ch>
Gesendet: Nov 7, 2011 5:56:38 PM
An: "Nicolas Kalkhof" <nkalkhof@web.de>
Betreff: Re: [Intel-gfx] [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock
>On Mon, Nov 07, 2011 at 05:39:44PM +0100, Nicolas Kalkhof wrote:
>> Hi Daniel,
>>
>> confirmed. Linus's linux-next.git shows the same behaviour. Until now
>> I've used a patched 3.1-rc-6+ Kernel from Dave Airlie's branch
>> (git://people.freedesktop.org/~airlied/linux' from Oct 26th.).
>> dmesg/syslog shows nothing special, neither does i915_error_state.
>>
>> Any advice?
>
>If the behaviour-difference is clear, you could try to bisect this. The
>alternative is to use your favourite system profiler and see where the
>cycles get wasted (either sysprof or perf). I suggest you'll try whatever
>approach your more familiar with first and then switch over to the other
>if the first one doesn't yield any clear results. If the difference is
>really clear, I'd start with the bisect (aside: don't restrict the bisect
>to any subdir, the change causing the regression could equally likely be
>somewhere in the core kernel).
>
>Yours, Daniel
>--
>Daniel Vetter
>Mail: daniel@ffwll.ch
>Mobile: +41 (0)79 365 57 48
___________________________________________________________
SMS schreiben mit WEB.DE FreeMail - einfach, schnell und
kostenguenstig. Jetzt gleich testen! http://f.web.de/?mc=021192
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock
2011-11-07 18:14 [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock Nicolas Kalkhof
@ 2011-11-07 18:36 ` Daniel Vetter
0 siblings, 0 replies; 16+ messages in thread
From: Daniel Vetter @ 2011-11-07 18:36 UTC (permalink / raw)
To: Nicolas Kalkhof; +Cc: Daniel Vetter, intel-gfx
On Mon, Nov 07, 2011 at 07:14:35PM +0100, Nicolas Kalkhof wrote:
> ok here is the sysprof result (see below). No cpu/gpu intensive apps are
> running, just the idling desktop. CPU Temperature is low and frequency
> is throttled down to 800 MHz like it should be. The System is responsive
> and behaves normal but top shows full cpu load. Honestly I cannot see
> the reason why X and gkrellm should eat up all the gpu since they
> clearly don't hog the cpu. Maybe the kernel reports wrong cpu stats?
> Then again the load goes down when I kill X! Hope this gives you a clue.
On a quick look it seems that at least X is busy-spinning in the select
loop and gkrellm seems to be eqally busy doing not much. Have you tried
what happens when you kill gkrellm?
Otherwise I think it's time for a bit of bisecting.
-Daniel
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock
2011-11-09 16:22 Nicolas Kalkhof
@ 2011-11-09 16:28 ` Daniel Vetter
0 siblings, 0 replies; 16+ messages in thread
From: Daniel Vetter @ 2011-11-09 16:28 UTC (permalink / raw)
To: Nicolas Kalkhof; +Cc: Daniel Vetter, intel-gfx
On Wed, Nov 09, 2011 at 05:22:00PM +0100, Nicolas Kalkhof wrote:
> Bisecting the kernel proved a little time consuming ;) however with the
> very latest linux.git anf xf86-video-intel the cpu load doesn't seem to
> go up accodring to top. What I can see is that the SNB draws approx
> 10-15 watts more when my system is completely idle. My first thought was
> that rc6 is being disabled but i915_enable_rc6 reports 1. Is there a way
> I can monitor the gpu frequency or its current power state?
cat /sys/kernel/debug/dri/0/i915_cur_delayinfo
atm you can't monitor the rc6 state, our current monitoring code doesn't
work on snb :(
Yours, Daniel
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock
@ 2011-11-09 16:22 Nicolas Kalkhof
2011-11-09 16:28 ` Daniel Vetter
0 siblings, 1 reply; 16+ messages in thread
From: Nicolas Kalkhof @ 2011-11-09 16:22 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Daniel Vetter, intel-gfx
Daniel,
Bisecting the kernel proved a little time consuming ;) however with the very latest linux.git anf xf86-video-intel the cpu load doesn't seem to go up accodring to top. What I can see is that the SNB draws approx 10-15 watts more when my system is completely idle. My first thought was that rc6 is being disabled but i915_enable_rc6 reports 1. Is there a way I can monitor the gpu frequency or its current power state?
Nic
-----Ursprüngliche Nachricht-----
Von: "Daniel Vetter" <daniel@ffwll.ch>
Gesendet: Nov 7, 2011 7:36:28 PM
An: "Nicolas Kalkhof" <nkalkhof@web.de>
Betreff: Re: [Intel-gfx] [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock
>On Mon, Nov 07, 2011 at 07:14:35PM +0100, Nicolas Kalkhof wrote:
>> ok here is the sysprof result (see below). No cpu/gpu intensive apps are
>> running, just the idling desktop. CPU Temperature is low and frequency
>> is throttled down to 800 MHz like it should be. The System is responsive
>> and behaves normal but top shows full cpu load. Honestly I cannot see
>> the reason why X and gkrellm should eat up all the gpu since they
>> clearly don't hog the cpu. Maybe the kernel reports wrong cpu stats?
>> Then again the load goes down when I kill X! Hope this gives you a clue.
>
>On a quick look it seems that at least X is busy-spinning in the select
>loop and gkrellm seems to be eqally busy doing not much. Have you tried
>what happens when you kill gkrellm?
>
>Otherwise I think it's time for a bit of bisecting.
>-Daniel
>--
>Daniel Vetter
>Mail: daniel@ffwll.ch
>Mobile: +41 (0)79 365 57 48
___________________________________________________________
SMS schreiben mit WEB.DE FreeMail - einfach, schnell und
kostenguenstig. Jetzt gleich testen! http://f.web.de/?mc=021192
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock
@ 2011-11-07 17:31 Nicolas Kalkhof
0 siblings, 0 replies; 16+ messages in thread
From: Nicolas Kalkhof @ 2011-11-07 17:31 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Daniel Vetter, intel-gfx
Hi Daniel,
Thanks for your Advice. Since it might take some time for the issue to appear I'll try to use a profiler to speed up the bughunt. I don't have any experience in profiling the kernel so this might take some time.... :(
I'll keep you posted.
Yours
Nic
-----Ursprüngliche Nachricht-----
Von: "Daniel Vetter" <daniel@ffwll.ch>
Gesendet: Nov 7, 2011 5:56:38 PM
An: "Nicolas Kalkhof" <nkalkhof@web.de>
Betreff: Re: [Intel-gfx] [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock
>On Mon, Nov 07, 2011 at 05:39:44PM +0100, Nicolas Kalkhof wrote:
>> Hi Daniel,
>>
>> confirmed. Linus's linux-next.git shows the same behaviour. Until now
>> I've used a patched 3.1-rc-6+ Kernel from Dave Airlie's branch
>> (git://people.freedesktop.org/~airlied/linux' from Oct 26th.).
>> dmesg/syslog shows nothing special, neither does i915_error_state.
>>
>> Any advice?
>
>If the behaviour-difference is clear, you could try to bisect this. The
>alternative is to use your favourite system profiler and see where the
>cycles get wasted (either sysprof or perf). I suggest you'll try whatever
>approach your more familiar with first and then switch over to the other
>if the first one doesn't yield any clear results. If the difference is
>really clear, I'd start with the bisect (aside: don't restrict the bisect
>to any subdir, the change causing the regression could equally likely be
>somewhere in the core kernel).
>
>Yours, Daniel
>--
>Daniel Vetter
>Mail: daniel@ffwll.ch
>Mobile: +41 (0)79 365 57 48
___________________________________________________________
SMS schreiben mit WEB.DE FreeMail - einfach, schnell und
kostenguenstig. Jetzt gleich testen! http://f.web.de/?mc=021192
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock
2011-11-07 16:39 Nicolas Kalkhof
@ 2011-11-07 16:56 ` Daniel Vetter
0 siblings, 0 replies; 16+ messages in thread
From: Daniel Vetter @ 2011-11-07 16:56 UTC (permalink / raw)
To: Nicolas Kalkhof; +Cc: Daniel Vetter, intel-gfx
On Mon, Nov 07, 2011 at 05:39:44PM +0100, Nicolas Kalkhof wrote:
> Hi Daniel,
>
> confirmed. Linus's linux-next.git shows the same behaviour. Until now
> I've used a patched 3.1-rc-6+ Kernel from Dave Airlie's branch
> (git://people.freedesktop.org/~airlied/linux' from Oct 26th.).
> dmesg/syslog shows nothing special, neither does i915_error_state.
>
> Any advice?
If the behaviour-difference is clear, you could try to bisect this. The
alternative is to use your favourite system profiler and see where the
cycles get wasted (either sysprof or perf). I suggest you'll try whatever
approach your more familiar with first and then switch over to the other
if the first one doesn't yield any clear results. If the difference is
really clear, I'd start with the bisect (aside: don't restrict the bisect
to any subdir, the change causing the regression could equally likely be
somewhere in the core kernel).
Yours, Daniel
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock
@ 2011-11-07 16:39 Nicolas Kalkhof
2011-11-07 16:56 ` Daniel Vetter
0 siblings, 1 reply; 16+ messages in thread
From: Nicolas Kalkhof @ 2011-11-07 16:39 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Daniel Vetter, intel-gfx
Hi Daniel,
confirmed. Linus's linux-next.git shows the same behaviour. Until now I've used a patched 3.1-rc-6+ Kernel from Dave Airlie's branch (git://people.freedesktop.org/~airlied/linux' from Oct 26th.). dmesg/syslog shows nothing special, neither does i915_error_state.
Any advice?
Nic
-----Ursprüngliche Nachricht-----
Von: "Daniel Vetter" <daniel@ffwll.ch>
Gesendet: Nov 7, 2011 5:05:12 PM
An: "Nicolas Kalkhof" <nkalkhof@web.de>
Betreff: Re: [Intel-gfx] [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock
>On Mon, Nov 07, 2011 at 02:52:06PM +0100, Nicolas Kalkhof wrote:
>> Hi Daniel,
>>
>> thanks for the git branch. Strange thing is with the patched kernel the
>> cpu load goes up after working with my desktop with no obvious reason.
>> top shows a high user and system load but no process using up all the
>> cpu cores. The cpufreq-daemon clocks the cpu cores down to the minimal
>> frequency (800mhz) when idle while top still shows full cpu load but no
>> process using up the cpu. The cpu load drops to normal after I kill X
>> and immediately goes up again if I restart X.
>>
>> I'm kinda lost here :( Any Ideas?
>
>Step one is to check whether -linus master has the same issues. My branch
>is based upon
>
>commit 37be944a0270402f9cda291a930b0286f6dc92f5
>Merge: ca836a2 1717c0e
>Author: Linus Torvalds <torvalds@linux-foundation.org>
>Date: Fri Oct 28 05:54:23 2011 -0700
>
> Merge branch 'drm-core-next' of
> git://people.freedesktop.org/~airlied/linux'
>
>If that doesn't show the same issue, can you please run a bisect (only 13
>commits) to find out which patch introduced this?
>
>Thanks, Daniel
>--
>Daniel Vetter
>Mail: daniel@ffwll.ch
>Mobile: +41 (0)79 365 57 48
___________________________________________________________
SMS schreiben mit WEB.DE FreeMail - einfach, schnell und
kostenguenstig. Jetzt gleich testen! http://f.web.de/?mc=021192
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock
2011-11-07 13:52 Nicolas Kalkhof
@ 2011-11-07 16:05 ` Daniel Vetter
0 siblings, 0 replies; 16+ messages in thread
From: Daniel Vetter @ 2011-11-07 16:05 UTC (permalink / raw)
To: Nicolas Kalkhof; +Cc: Daniel Vetter, intel-gfx
On Mon, Nov 07, 2011 at 02:52:06PM +0100, Nicolas Kalkhof wrote:
> Hi Daniel,
>
> thanks for the git branch. Strange thing is with the patched kernel the
> cpu load goes up after working with my desktop with no obvious reason.
> top shows a high user and system load but no process using up all the
> cpu cores. The cpufreq-daemon clocks the cpu cores down to the minimal
> frequency (800mhz) when idle while top still shows full cpu load but no
> process using up the cpu. The cpu load drops to normal after I kill X
> and immediately goes up again if I restart X.
>
> I'm kinda lost here :( Any Ideas?
Step one is to check whether -linus master has the same issues. My branch
is based upon
commit 37be944a0270402f9cda291a930b0286f6dc92f5
Merge: ca836a2 1717c0e
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date: Fri Oct 28 05:54:23 2011 -0700
Merge branch 'drm-core-next' of
git://people.freedesktop.org/~airlied/linux'
If that doesn't show the same issue, can you please run a bisect (only 13
commits) to find out which patch introduced this?
Thanks, Daniel
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock
@ 2011-11-07 13:52 Nicolas Kalkhof
2011-11-07 16:05 ` Daniel Vetter
0 siblings, 1 reply; 16+ messages in thread
From: Nicolas Kalkhof @ 2011-11-07 13:52 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Daniel Vetter, intel-gfx
Hi Daniel,
thanks for the git branch. Strange thing is with the patched kernel the cpu load goes up after working with my desktop with no obvious reason. top shows a high user and system load but no process using up all the cpu cores. The cpufreq-daemon clocks the cpu cores down to the minimal frequency (800mhz) when idle while top still shows full cpu load but no process using up the cpu. The cpu load drops to normal after I kill X and immediately goes up again if I restart X.
I'm kinda lost here :( Any Ideas?
Regards
Nic
-----Ursprüngliche Nachricht-----
Von: "Daniel Vetter" <daniel@ffwll.ch>
Gesendet: Nov 6, 2011 6:46:54 PM
An: "Nicolas Kalkhof" <nkalkhof@web.de>
Betreff: Re: [Intel-gfx] [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock
>On Sun, Nov 06, 2011 at 06:42:00PM +0100, Nicolas Kalkhof wrote:
>> is there a specific git branch availble where I can test your patch? I'm
>> also experiencing random forcewake hangs and would like to see if your
>> patch helps.
>
>Please grab the my-next branch from my personal repo:
>
>http://cgit.freedesktop.org/~danvet/drm/log/?h=my-next
>
>Yours, Daniel
>--
>Daniel Vetter
>Mail: daniel@ffwll.ch
>Mobile: +41 (0)79 365 57 48
___________________________________________________________
SMS schreiben mit WEB.DE FreeMail - einfach, schnell und
kostenguenstig. Jetzt gleich testen! http://f.web.de/?mc=021192
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock
2011-11-06 21:01 ` Chris Wilson
@ 2011-11-06 22:06 ` Daniel Vetter
0 siblings, 0 replies; 16+ messages in thread
From: Daniel Vetter @ 2011-11-06 22:06 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter
The problem this patch solves is that the forcewake accounting
necessary for register reads is protected by dev->struct_mutex. But the
hangcheck and error_capture code need to access registers without
grabbing this mutex because we hold it while waiting for the gpu.
So a new lock is required. Because currently the error_state capture
is called from the error irq handler and the hangcheck code runs from
a timer, it needs to be an irqsafe spinlock (note that the registers
used by the irq handler (neglecting the error handling part) only uses
registers that don't need the forcewake dance).
We could tune this down to a normal spinlock when we rework the
error_state capture and hangcheck code to run from a workqueue. But
we don't have any read in a fastpath that needs forcewake, so I've
decided to not care much about overhead.
This prevents tests/gem_hangcheck_forcewake from i-g-t from killing my
snb on recent kernels - something must have slightly changed the
timings. On previous kernels it only trigger a WARN about the broken
locking.
v2: Drop the previous patch for the register writes.
v3: Improve the commit message per Chris Wilson's suggestions.
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_debugfs.c | 8 ++++++--
drivers/gpu/drm/i915/i915_dma.c | 1 +
drivers/gpu/drm/i915/i915_drv.c | 19 +++++++++++++------
drivers/gpu/drm/i915/i915_drv.h | 10 +++++++---
4 files changed, 27 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5ba63ad..397f76c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1320,9 +1320,13 @@ static int i915_gen6_forcewake_count_info(struct seq_file *m, void *data)
struct drm_info_node *node = (struct drm_info_node *) m->private;
struct drm_device *dev = node->minor->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
+ unsigned forcewake_count;
- seq_printf(m, "forcewake count = %d\n",
- atomic_read(&dev_priv->forcewake_count));
+ spin_lock_irq(&dev_priv->gt_lock);
+ forcewake_count = dev_priv->forcewake_count;
+ spin_unlock_irq(&dev_priv->gt_lock);
+
+ seq_printf(m, "forcewake count = %u\n", forcewake_count);
return 0;
}
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 2eac955..ab3a3fd 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2031,6 +2031,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
if (!IS_I945G(dev) && !IS_I945GM(dev))
pci_enable_msi(dev->pdev);
+ spin_lock_init(&dev_priv->gt_lock);
spin_lock_init(&dev_priv->irq_lock);
spin_lock_init(&dev_priv->error_lock);
spin_lock_init(&dev_priv->rps_lock);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 548e04b..bab4e08 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -351,11 +351,12 @@ static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
*/
void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
{
- WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
+ unsigned long irqflags;
- /* Forcewake is atomic in case we get in here without the lock */
- if (atomic_add_return(1, &dev_priv->forcewake_count) == 1)
+ spin_lock_irqsave(&dev_priv->gt_lock, irqflags);
+ if (dev_priv->forcewake_count++ == 0)
__gen6_gt_force_wake_get(dev_priv);
+ spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags);
}
static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
@@ -369,10 +370,13 @@ static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
*/
void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
{
- WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
+ unsigned long irqflags;
- if (atomic_dec_and_test(&dev_priv->forcewake_count))
+ spin_lock_irqsave(&dev_priv->gt_lock, irqflags);
+
+ if (--dev_priv->forcewake_count == 0)
__gen6_gt_force_wake_put(dev_priv);
+ spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags);
}
void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
@@ -603,6 +607,7 @@ int i915_reset(struct drm_device *dev, u8 flags)
* need to
*/
bool need_display = true;
+ unsigned long irqflags;
int ret;
if (!i915_try_reset)
@@ -621,8 +626,10 @@ int i915_reset(struct drm_device *dev, u8 flags)
case 6:
ret = gen6_do_reset(dev, flags);
/* If reset with a user forcewake, try to restore */
- if (atomic_read(&dev_priv->forcewake_count))
+ spin_lock_irqsave(&dev_priv->gt_lock, irqflags);
+ if (dev_priv->forcewake_count)
__gen6_gt_force_wake_get(dev_priv);
+ spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags);
break;
case 5:
ret = ironlake_do_reset(dev, flags);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bd98fb3..25036f5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -276,7 +276,13 @@ typedef struct drm_i915_private {
int relative_constants_mode;
void __iomem *regs;
- u32 gt_fifo_count;
+ /** gt_fifo_count and the subsequent register write are synchronized
+ * with dev->struct_mutex. */
+ unsigned gt_fifo_count;
+ /** forcewake_count is protected by gt_lock */
+ unsigned forcewake_count;
+ /** gt_lock is also taken in irq contexts. */
+ struct spinlock gt_lock;
struct intel_gmbus {
struct i2c_adapter adapter;
@@ -725,8 +731,6 @@ typedef struct drm_i915_private {
struct drm_property *broadcast_rgb_property;
struct drm_property *force_audio_property;
-
- atomic_t forcewake_count;
} drm_i915_private_t;
enum i915_cache_level {
--
1.7.6.3
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock
2011-11-06 12:35 ` Daniel Vetter
@ 2011-11-06 21:01 ` Chris Wilson
2011-11-06 22:06 ` Daniel Vetter
0 siblings, 1 reply; 16+ messages in thread
From: Chris Wilson @ 2011-11-06 21:01 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter
On Sun, 6 Nov 2011 13:35:22 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> The problem this patch solves is that the forcewake accounting
> necessary for register reads is protected by dev->struct_mutex. But the
> hangcheck and error_capture code need to access registers without
> grabbing this mutex because we hold it while waiting for the gpu.
> So a new lock is required. Because currently the error_state capture
> is called from the error irq handler and the hangcheck code runs from
> a timer, it needs to be an irqsafe spinlock (note that the registers
> used by the irq handler (neglecting the error handling part) only uses
> registers that don't need the forcewake dance).
>
> We could tune this down to a normal spinlock when we rework the
> error_state capture and hangcheck code to run from a workqueue. But
> we don't have any read in a fastpath that needs forcewake, so I've
> decided to not care much about overhead.
>
> This prevents tests/gem_hangcheck_forcewake from i-g-t from killing my
> snb on recent kernels - something must have slightly changed the
> timings. On previous kernels it only trigger a WARN about the broken
> locking.
>
> v2: Drop the previous patch for the register writes.
>
> v3: Improve the commit message per Chris Wilson's suggestions.
>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
One minor oddity left ;-)
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 8 ++++++--
> drivers/gpu/drm/i915/i915_dma.c | 1 +
> drivers/gpu/drm/i915/i915_drv.c | 19 +++++++++++++------
> drivers/gpu/drm/i915/i915_drv.h | 10 +++++++---
> 4 files changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 5ba63ad..51b21eb 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1320,9 +1320,13 @@ static int i915_gen6_forcewake_count_info(struct seq_file *m, void *data)
> struct drm_info_node *node = (struct drm_info_node *) m->private;
> struct drm_device *dev = node->minor->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> + unsigned forcewake_count;
>
> - seq_printf(m, "forcewake count = %d\n",
> - atomic_read(&dev_priv->forcewake_count));
> + spin_lock_irq(&dev_priv->gt_lock);
> + forcewake_count = dev_priv->forcewake_count;
> + spin_unlock_irq(&dev_priv->gt_lock);
> +
> + seq_printf(m, "forcewake count = %d\n", forcewake_count);
Is it signed or unsigned? Who cares? ;-)
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock
2011-11-06 17:42 Nicolas Kalkhof
@ 2011-11-06 17:46 ` Daniel Vetter
0 siblings, 0 replies; 16+ messages in thread
From: Daniel Vetter @ 2011-11-06 17:46 UTC (permalink / raw)
To: Nicolas Kalkhof; +Cc: Daniel Vetter, intel-gfx
On Sun, Nov 06, 2011 at 06:42:00PM +0100, Nicolas Kalkhof wrote:
> is there a specific git branch availble where I can test your patch? I'm
> also experiencing random forcewake hangs and would like to see if your
> patch helps.
Please grab the my-next branch from my personal repo:
http://cgit.freedesktop.org/~danvet/drm/log/?h=my-next
Yours, Daniel
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock
@ 2011-11-06 17:42 Nicolas Kalkhof
2011-11-06 17:46 ` Daniel Vetter
0 siblings, 1 reply; 16+ messages in thread
From: Nicolas Kalkhof @ 2011-11-06 17:42 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter
Hi Daniel,
is there a specific git branch availble where I can test your patch? I'm also experiencing random forcewake hangs and would like to see if your patch helps.
Thanks
Nic
-----Ursprüngliche Nachricht-----
Von: "Daniel Vetter" <daniel.vetter@ffwll.ch>
Gesendet: Nov 6, 2011 1:35:22 PM
An: intel-gfx <intel-gfx@lists.freedesktop.org>
Betreff: [Intel-gfx] [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock
>The problem this patch solves is that the forcewake accounting
>necessary for register reads is protected by dev->struct_mutex. But the
>hangcheck and error_capture code need to access registers without
>grabbing this mutex because we hold it while waiting for the gpu.
>So a new lock is required. Because currently the error_state capture
>is called from the error irq handler and the hangcheck code runs from
>a timer, it needs to be an irqsafe spinlock (note that the registers
>used by the irq handler (neglecting the error handling part) only uses
>registers that don't need the forcewake dance).
>
>We could tune this down to a normal spinlock when we rework the
>error_state capture and hangcheck code to run from a workqueue. But
>we don't have any read in a fastpath that needs forcewake, so I've
>decided to not care much about overhead.
>
>This prevents tests/gem_hangcheck_forcewake from i-g-t from killing my
>snb on recent kernels - something must have slightly changed the
>timings. On previous kernels it only trigger a WARN about the broken
>locking.
>
>v2: Drop the previous patch for the register writes.
>
>v3: Improve the commit message per Chris Wilson's suggestions.
>
>Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>---
> drivers/gpu/drm/i915/i915_debugfs.c | 8 ++++++--
> drivers/gpu/drm/i915/i915_dma.c | 1 +
> drivers/gpu/drm/i915/i915_drv.c | 19 +++++++++++++------
> drivers/gpu/drm/i915/i915_drv.h | 10 +++++++---
> 4 files changed, 27 insertions(+), 11 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>index 5ba63ad..51b21eb 100644
>--- a/drivers/gpu/drm/i915/i915_debugfs.c
>+++ b/drivers/gpu/drm/i915/i915_debugfs.c
>@@ -1320,9 +1320,13 @@ static int i915_gen6_forcewake_count_info(struct seq_file *m, void *data)
> struct drm_info_node *node = (struct drm_info_node *) m->private;
> struct drm_device *dev = node->minor->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
>+ unsigned forcewake_count;
>
>- seq_printf(m, "forcewake count = %d\n",
>- atomic_read(&dev_priv->forcewake_count));
>+ spin_lock_irq(&dev_priv->gt_lock);
>+ forcewake_count = dev_priv->forcewake_count;
>+ spin_unlock_irq(&dev_priv->gt_lock);
>+
>+ seq_printf(m, "forcewake count = %d\n", forcewake_count);
>
> return 0;
> }
>diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>index 2eac955..ab3a3fd 100644
>--- a/drivers/gpu/drm/i915/i915_dma.c
>+++ b/drivers/gpu/drm/i915/i915_dma.c
>@@ -2031,6 +2031,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> if (!IS_I945G(dev) && !IS_I945GM(dev))
> pci_enable_msi(dev->pdev);
>
>+ spin_lock_init(&dev_priv->gt_lock);
> spin_lock_init(&dev_priv->irq_lock);
> spin_lock_init(&dev_priv->error_lock);
> spin_lock_init(&dev_priv->rps_lock);
>diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>index 548e04b..bab4e08 100644
>--- a/drivers/gpu/drm/i915/i915_drv.c
>+++ b/drivers/gpu/drm/i915/i915_drv.c
>@@ -351,11 +351,12 @@ static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
> */
> void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
> {
>- WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
>+ unsigned long irqflags;
>
>- /* Forcewake is atomic in case we get in here without the lock */
>- if (atomic_add_return(1, &dev_priv->forcewake_count) == 1)
>+ spin_lock_irqsave(&dev_priv->gt_lock, irqflags);
>+ if (dev_priv->forcewake_count++ == 0)
> __gen6_gt_force_wake_get(dev_priv);
>+ spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags);
> }
>
> static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
>@@ -369,10 +370,13 @@ static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
> */
> void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
> {
>- WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
>+ unsigned long irqflags;
>
>- if (atomic_dec_and_test(&dev_priv->forcewake_count))
>+ spin_lock_irqsave(&dev_priv->gt_lock, irqflags);
>+
>+ if (--dev_priv->forcewake_count == 0)
> __gen6_gt_force_wake_put(dev_priv);
>+ spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags);
> }
>
> void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
>@@ -603,6 +607,7 @@ int i915_reset(struct drm_device *dev, u8 flags)
> * need to
> */
> bool need_display = true;
>+ unsigned long irqflags;
> int ret;
>
> if (!i915_try_reset)
>@@ -621,8 +626,10 @@ int i915_reset(struct drm_device *dev, u8 flags)
> case 6:
> ret = gen6_do_reset(dev, flags);
> /* If reset with a user forcewake, try to restore */
>- if (atomic_read(&dev_priv->forcewake_count))
>+ spin_lock_irqsave(&dev_priv->gt_lock, irqflags);
>+ if (dev_priv->forcewake_count)
> __gen6_gt_force_wake_get(dev_priv);
>+ spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags);
> break;
> case 5:
> ret = ironlake_do_reset(dev, flags);
>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>index bd98fb3..25036f5 100644
>--- a/drivers/gpu/drm/i915/i915_drv.h
>+++ b/drivers/gpu/drm/i915/i915_drv.h
>@@ -276,7 +276,13 @@ typedef struct drm_i915_private {
> int relative_constants_mode;
>
> void __iomem *regs;
>- u32 gt_fifo_count;
>+ /** gt_fifo_count and the subsequent register write are synchronized
>+ * with dev->struct_mutex. */
>+ unsigned gt_fifo_count;
>+ /** forcewake_count is protected by gt_lock */
>+ unsigned forcewake_count;
>+ /** gt_lock is also taken in irq contexts. */
>+ struct spinlock gt_lock;
>
> struct intel_gmbus {
> struct i2c_adapter adapter;
>@@ -725,8 +731,6 @@ typedef struct drm_i915_private {
>
> struct drm_property *broadcast_rgb_property;
> struct drm_property *force_audio_property;
>-
>- atomic_t forcewake_count;
> } drm_i915_private_t;
>
> enum i915_cache_level {
>--
>1.7.6.4
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>http://lists.freedesktop.org/mailman/listinfo/intel-gfx
___________________________________________________________
SMS schreiben mit WEB.DE FreeMail - einfach, schnell und
kostenguenstig. Jetzt gleich testen! http://f.web.de/?mc=021192
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock
2011-11-06 11:57 ` Chris Wilson
@ 2011-11-06 12:35 ` Daniel Vetter
2011-11-06 21:01 ` Chris Wilson
0 siblings, 1 reply; 16+ messages in thread
From: Daniel Vetter @ 2011-11-06 12:35 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter
The problem this patch solves is that the forcewake accounting
necessary for register reads is protected by dev->struct_mutex. But the
hangcheck and error_capture code need to access registers without
grabbing this mutex because we hold it while waiting for the gpu.
So a new lock is required. Because currently the error_state capture
is called from the error irq handler and the hangcheck code runs from
a timer, it needs to be an irqsafe spinlock (note that the registers
used by the irq handler (neglecting the error handling part) only uses
registers that don't need the forcewake dance).
We could tune this down to a normal spinlock when we rework the
error_state capture and hangcheck code to run from a workqueue. But
we don't have any read in a fastpath that needs forcewake, so I've
decided to not care much about overhead.
This prevents tests/gem_hangcheck_forcewake from i-g-t from killing my
snb on recent kernels - something must have slightly changed the
timings. On previous kernels it only trigger a WARN about the broken
locking.
v2: Drop the previous patch for the register writes.
v3: Improve the commit message per Chris Wilson's suggestions.
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/i915_debugfs.c | 8 ++++++--
drivers/gpu/drm/i915/i915_dma.c | 1 +
drivers/gpu/drm/i915/i915_drv.c | 19 +++++++++++++------
drivers/gpu/drm/i915/i915_drv.h | 10 +++++++---
4 files changed, 27 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5ba63ad..51b21eb 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1320,9 +1320,13 @@ static int i915_gen6_forcewake_count_info(struct seq_file *m, void *data)
struct drm_info_node *node = (struct drm_info_node *) m->private;
struct drm_device *dev = node->minor->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
+ unsigned forcewake_count;
- seq_printf(m, "forcewake count = %d\n",
- atomic_read(&dev_priv->forcewake_count));
+ spin_lock_irq(&dev_priv->gt_lock);
+ forcewake_count = dev_priv->forcewake_count;
+ spin_unlock_irq(&dev_priv->gt_lock);
+
+ seq_printf(m, "forcewake count = %d\n", forcewake_count);
return 0;
}
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 2eac955..ab3a3fd 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2031,6 +2031,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
if (!IS_I945G(dev) && !IS_I945GM(dev))
pci_enable_msi(dev->pdev);
+ spin_lock_init(&dev_priv->gt_lock);
spin_lock_init(&dev_priv->irq_lock);
spin_lock_init(&dev_priv->error_lock);
spin_lock_init(&dev_priv->rps_lock);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 548e04b..bab4e08 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -351,11 +351,12 @@ static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
*/
void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
{
- WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
+ unsigned long irqflags;
- /* Forcewake is atomic in case we get in here without the lock */
- if (atomic_add_return(1, &dev_priv->forcewake_count) == 1)
+ spin_lock_irqsave(&dev_priv->gt_lock, irqflags);
+ if (dev_priv->forcewake_count++ == 0)
__gen6_gt_force_wake_get(dev_priv);
+ spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags);
}
static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
@@ -369,10 +370,13 @@ static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
*/
void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
{
- WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
+ unsigned long irqflags;
- if (atomic_dec_and_test(&dev_priv->forcewake_count))
+ spin_lock_irqsave(&dev_priv->gt_lock, irqflags);
+
+ if (--dev_priv->forcewake_count == 0)
__gen6_gt_force_wake_put(dev_priv);
+ spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags);
}
void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
@@ -603,6 +607,7 @@ int i915_reset(struct drm_device *dev, u8 flags)
* need to
*/
bool need_display = true;
+ unsigned long irqflags;
int ret;
if (!i915_try_reset)
@@ -621,8 +626,10 @@ int i915_reset(struct drm_device *dev, u8 flags)
case 6:
ret = gen6_do_reset(dev, flags);
/* If reset with a user forcewake, try to restore */
- if (atomic_read(&dev_priv->forcewake_count))
+ spin_lock_irqsave(&dev_priv->gt_lock, irqflags);
+ if (dev_priv->forcewake_count)
__gen6_gt_force_wake_get(dev_priv);
+ spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags);
break;
case 5:
ret = ironlake_do_reset(dev, flags);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bd98fb3..25036f5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -276,7 +276,13 @@ typedef struct drm_i915_private {
int relative_constants_mode;
void __iomem *regs;
- u32 gt_fifo_count;
+ /** gt_fifo_count and the subsequent register write are synchronized
+ * with dev->struct_mutex. */
+ unsigned gt_fifo_count;
+ /** forcewake_count is protected by gt_lock */
+ unsigned forcewake_count;
+ /** gt_lock is also taken in irq contexts. */
+ struct spinlock gt_lock;
struct intel_gmbus {
struct i2c_adapter adapter;
@@ -725,8 +731,6 @@ typedef struct drm_i915_private {
struct drm_property *broadcast_rgb_property;
struct drm_property *force_audio_property;
-
- atomic_t forcewake_count;
} drm_i915_private_t;
enum i915_cache_level {
--
1.7.6.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock
2011-11-06 11:31 ` [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock Daniel Vetter
@ 2011-11-06 11:57 ` Chris Wilson
2011-11-06 12:35 ` Daniel Vetter
0 siblings, 1 reply; 16+ messages in thread
From: Chris Wilson @ 2011-11-06 11:57 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter
On Sun, 6 Nov 2011 12:31:34 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> We don't have any read in a fastpath that needs forcewake, so I've
> decided to not care much about overhead.
>
> This prevents tests/gem_hangcheck_forcewake from i-g-t from killing my
> snb on recent kernels - something must have slightly changed the
> timings.
Almost there. You just haven't explained the rationale for *this* patch,
which is that hangcheck needs to acquire the forcewake in order to read
the registers and hangcheck must not take the struct_mutex (or else
deadlock with wait_request and a hung GPU).
So there is a choice here: introduce a new locking rule for forcewake,
or use the existing struct_mutex inside hangcheck and therefore drop the
mutex for wait_request. The first definitely feels safer than dropping
struct_mutex on waits, and I haven't thought of any tangible benefits
for doing so (other than concurrent clients might see an improvement).
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock
2011-11-06 10:46 [PATCH 1/2] drm/i915: properly lock gt_fifo_count Daniel Vetter
@ 2011-11-06 11:31 ` Daniel Vetter
2011-11-06 11:57 ` Chris Wilson
0 siblings, 1 reply; 16+ messages in thread
From: Daniel Vetter @ 2011-11-06 11:31 UTC (permalink / raw)
To: intel-gfx; +Cc: Daniel Vetter
We don't have any read in a fastpath that needs forcewake, so I've
decided to not care much about overhead.
This prevents tests/gem_hangcheck_forcewake from i-g-t from killing my
snb on recent kernels - something must have slightly changed the
timings.
v2: Drop the previous patch for the register writes.
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/i915_debugfs.c | 8 ++++++--
drivers/gpu/drm/i915/i915_dma.c | 1 +
drivers/gpu/drm/i915/i915_drv.c | 19 +++++++++++++------
drivers/gpu/drm/i915/i915_drv.h | 10 +++++++---
4 files changed, 27 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5ba63ad..51b21eb 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1320,9 +1320,13 @@ static int i915_gen6_forcewake_count_info(struct seq_file *m, void *data)
struct drm_info_node *node = (struct drm_info_node *) m->private;
struct drm_device *dev = node->minor->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
+ unsigned forcewake_count;
- seq_printf(m, "forcewake count = %d\n",
- atomic_read(&dev_priv->forcewake_count));
+ spin_lock_irq(&dev_priv->gt_lock);
+ forcewake_count = dev_priv->forcewake_count;
+ spin_unlock_irq(&dev_priv->gt_lock);
+
+ seq_printf(m, "forcewake count = %d\n", forcewake_count);
return 0;
}
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 2eac955..ab3a3fd 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -2031,6 +2031,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
if (!IS_I945G(dev) && !IS_I945GM(dev))
pci_enable_msi(dev->pdev);
+ spin_lock_init(&dev_priv->gt_lock);
spin_lock_init(&dev_priv->irq_lock);
spin_lock_init(&dev_priv->error_lock);
spin_lock_init(&dev_priv->rps_lock);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 548e04b..bab4e08 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -351,11 +351,12 @@ static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
*/
void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
{
- WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
+ unsigned long irqflags;
- /* Forcewake is atomic in case we get in here without the lock */
- if (atomic_add_return(1, &dev_priv->forcewake_count) == 1)
+ spin_lock_irqsave(&dev_priv->gt_lock, irqflags);
+ if (dev_priv->forcewake_count++ == 0)
__gen6_gt_force_wake_get(dev_priv);
+ spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags);
}
static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
@@ -369,10 +370,13 @@ static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
*/
void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
{
- WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
+ unsigned long irqflags;
- if (atomic_dec_and_test(&dev_priv->forcewake_count))
+ spin_lock_irqsave(&dev_priv->gt_lock, irqflags);
+
+ if (--dev_priv->forcewake_count == 0)
__gen6_gt_force_wake_put(dev_priv);
+ spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags);
}
void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
@@ -603,6 +607,7 @@ int i915_reset(struct drm_device *dev, u8 flags)
* need to
*/
bool need_display = true;
+ unsigned long irqflags;
int ret;
if (!i915_try_reset)
@@ -621,8 +626,10 @@ int i915_reset(struct drm_device *dev, u8 flags)
case 6:
ret = gen6_do_reset(dev, flags);
/* If reset with a user forcewake, try to restore */
- if (atomic_read(&dev_priv->forcewake_count))
+ spin_lock_irqsave(&dev_priv->gt_lock, irqflags);
+ if (dev_priv->forcewake_count)
__gen6_gt_force_wake_get(dev_priv);
+ spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags);
break;
case 5:
ret = ironlake_do_reset(dev, flags);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bd98fb3..25036f5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -276,7 +276,13 @@ typedef struct drm_i915_private {
int relative_constants_mode;
void __iomem *regs;
- u32 gt_fifo_count;
+ /** gt_fifo_count and the subsequent register write are synchronized
+ * with dev->struct_mutex. */
+ unsigned gt_fifo_count;
+ /** forcewake_count is protected by gt_lock */
+ unsigned forcewake_count;
+ /** gt_lock is also taken in irq contexts. */
+ struct spinlock gt_lock;
struct intel_gmbus {
struct i2c_adapter adapter;
@@ -725,8 +731,6 @@ typedef struct drm_i915_private {
struct drm_property *broadcast_rgb_property;
struct drm_property *force_audio_property;
-
- atomic_t forcewake_count;
} drm_i915_private_t;
enum i915_cache_level {
--
1.7.6.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
end of thread, other threads:[~2011-11-09 16:28 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-11-07 18:14 [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock Nicolas Kalkhof
2011-11-07 18:36 ` Daniel Vetter
-- strict thread matches above, loose matches on Subject: below --
2011-11-09 16:22 Nicolas Kalkhof
2011-11-09 16:28 ` Daniel Vetter
2011-11-07 17:31 Nicolas Kalkhof
2011-11-07 16:39 Nicolas Kalkhof
2011-11-07 16:56 ` Daniel Vetter
2011-11-07 13:52 Nicolas Kalkhof
2011-11-07 16:05 ` Daniel Vetter
2011-11-06 17:42 Nicolas Kalkhof
2011-11-06 17:46 ` Daniel Vetter
2011-11-06 10:46 [PATCH 1/2] drm/i915: properly lock gt_fifo_count Daniel Vetter
2011-11-06 11:31 ` [PATCH] drm/i915: protect force_wake_(get|put) with the gt_lock Daniel Vetter
2011-11-06 11:57 ` Chris Wilson
2011-11-06 12:35 ` Daniel Vetter
2011-11-06 21:01 ` Chris Wilson
2011-11-06 22:06 ` Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.