[Intel-gfx] [PATCH 3/3] drm/i915: Take rpm wakelock for releasing the fence on unbind

Mon Mar 6 14:03:26 UTC 2017

On 06/03/2017 09:29, Chris Wilson wrote:
> Unbind the vma may happen at any time, outside of the normal GT wakeref.
> As such it relies on having a wakeref of its own. However, we can forgo
> clearing the register whilst the device is asleep and just mark it as
> unused - so that when we do wake up the device, we will clear the unused
> fence register (see i915_gem_restore_fences).
>
> [22423.944631] WARNING: CPU: 3 PID: 26178 at drivers/gpu/drm/i915/intel_drv.h:1739 i915_vma_put_fence+0xf3/0x100 [i915]
> [22423.946053] RPM wakelock ref not held during HW access
> [22423.946056] Modules linked in: vgem(E) i915(E) nls_ascii(E) nls_cp437(E) vfat(E) fat(E) x86_pkg_temp_thermal(E) crct10dif_pclmul(E) crc32_pclmul(E) crc32c_intel(E) ghash_clmulni_intel(E) intel_gtt(E) i2c_algo_bit(E) drm_kms_helper(E) syscopyarea(E) sysfillrect(E) evdev(E) aesni_intel(E) aes_x86_64(E) crypto_simd(E) cryptd(E) glue_helper(E) sysimgblt(E) fb_sys_fops(E) prime_numbers(E) drm(E) efivars(E) mei_me(E) lpc_ich(E) mei(E) mfd_core(E) battery(E) video(E) acpi_pad(E) button(E) tpm_tis(E) tpm_tis_core(E) tpm(E) autofs4(E) i2c_i801(E) thermal(E) fan(E) i2c_designware_platform(E) i2c_designware_core(E)
> [22423.946438] CPU: 2 PID: 26178 Comm: gem_concurrent_ Tainted: G            E   4.10.0+ #101
> [22423.946513] Hardware name: ��������������������������������� ���������������������������������/���������������������������������, BIOS RYBDWi35.86A.0246.2
> [22423.946600] Call Trace:
> [22423.946641]  dump_stack+0x68/0x9f
> [22423.946703]  __warn+0x107/0x130
> [22423.946763]  warn_slowpath_fmt+0xa8/0xe0
> [22423.946825]  ? __warn+0x130/0x130
> [22423.946868]  ? free_hot_cold_page_list+0x53/0x70
> [22423.946942]  ? mark_lock+0xcc/0x7f0
> [22423.946997]  ? __lock_is_held+0x84/0x100
> [22423.947115]  ? i915_vma_put_fence+0x64/0x100 [i915]
> [22423.947224]  i915_vma_put_fence+0xf3/0x100 [i915]
> [22423.947335]  i915_vma_unbind+0x4da/0x560 [i915]
> [22423.947387]  ? rb_erase+0x812/0x8a0
> [22423.947439]  ? kfree+0xa2/0xd0
> [22423.947562]  i915_vma_close+0x159/0x180 [i915]
> [22423.947674]  intel_ring_free+0x31/0x50 [i915]
> [22423.947776]  i915_gem_context_free+0x1ff/0x3d0 [i915]
> [22423.947887]  context_close+0x106/0x110 [i915]
> [22423.947989]  context_idr_cleanup+0xc/0x10 [i915]
> [22423.948041]  idr_for_each+0x14d/0x1d0
> [22423.948158]  ? context_close+0x110/0x110 [i915]
> [22423.948206]  ? get_from_free_list+0x70/0x70
> [22423.948261]  ? __lock_is_held+0x84/0x100
> [22423.948325]  ? __mutex_unlock_slowpath+0xd4/0x400
> [22423.948448]  i915_gem_context_close+0x4b/0x90 [i915]
> [22423.948544]  i915_driver_preclose+0x28/0x50 [i915]
> [22423.948620]  drm_release+0x175/0x690 [drm]
> [22423.948681]  ? fcntl_setlk+0x5e0/0x5e0
> [22423.948746]  __fput+0x17d/0x300
> [22423.948807]  ____fput+0x9/0x10
> [22423.948859]  task_work_run+0xa7/0xe0
> [22423.948924]  do_exit+0x4d2/0x13e0
> [22423.948986]  ? mm_update_next_owner+0x320/0x320
> [22423.949051]  ? __do_page_fault+0x209/0x5c0
> [22423.949110]  ? mark_held_locks+0x23/0xc0
> [22423.949166]  ? entry_SYSCALL_64_fastpath+0x5/0xb1
> [22423.949232]  do_group_exit+0x93/0x160
> [22423.949289]  SyS_exit_group+0x18/0x20
> [22423.949350]  entry_SYSCALL_64_fastpath+0x1c/0xb1
> [22423.949403] RIP: 0033:0x7f9cc2e154c8
> [22423.949484] RSP: 002b:00007ffd7e81b448 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
> [22423.949557] RAX: ffffffffffffffda RBX: ffffffff810ef1f0 RCX: 00007f9cc2e154c8
> [22423.949617] RDX: 0000000000000000 RSI: 000000000000003c RDI: 0000000000000000
> [22423.949677] RBP: ffff880367e9ff98 R08: 00000000000000e7 R09: ffffffffffffff88
> [22423.949741] R10: 00007f9cc1d5c000 R11: 0000000000000246 R12: 00007f9cc30f6c30
> [22423.949798] R13: 0000000000000000 R14: 00007f9cc30f6c20 R15: 0000000000000003
> [22423.949868]  ? trace_hardirqs_off_caller+0xc0/0x110
>
> v2: Move the rpm check down a layer so that we still perform the
> vma/fence update required for the deferred mmio write on resume.
> v3: Don't touch i915_gem_object_set_cache_level() and leave the rpm to
> the low level routines (such as i915_vma_put_fence).
> v4: vma may be null in fence_write, so extract drm_i915_private from
> fence->i915
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_fence_reg.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
> index fadbe8f4c745..5fe2cd8c8f28 100644
> --- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
> +++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
> @@ -248,7 +248,14 @@ static int fence_update(struct drm_i915_fence_reg *fence,
>  		list_move(&fence->link, &fence->i915->mm.fence_list);
>  	}
>
> -	fence_write(fence, vma);
> +	/* We only need to update the register itself if the device is awake.
> +	 * If the device is currently powered down, we will defer the write
> +	 * to the runtime resume, see i915_gem_restore_fences().
> +	 */
> +	if (intel_runtime_pm_get_if_in_use(fence->i915)) {
> +		fence_write(fence, vma);
> +		intel_runtime_pm_put(fence->i915);
> +	}
>
>  	if (vma) {
>  		if (fence->vma != vma) {
> @@ -278,8 +285,6 @@ i915_vma_put_fence(struct i915_vma *vma)
>  {
>  	struct drm_i915_fence_reg *fence = vma->fence;
>
> -	assert_rpm_wakelock_held(vma->vm->i915);
> -
>  	if (!fence)
>  		return 0;
>
>

Looks OK. At least my power management untrained eyes. :) But I've 
traced the code paths and all and it looks simple.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko