Through a twisty and circuituous path it is possible to currently trick the code into creating a default context and forgetting to pin it immediately into the GGTT. (This requires a system using contexts without an aliasing ppgtt, which is currently restricted to Baytrails machines manually specifying a module parameter to force enable contexts, or on Sandybridge and later that manually disable the aliasing ppgtt.) The consequence is that during module unload we attempt to unpin the default context twice and encounter a BUG remonstrating that we attempt to unpin an unbound object. [ 161.002869] Kernel BUG at f84861f8 [verbose debug info unavailable] [ 161.002875] invalid opcode: 0000 [#1] SMP [ 161.002882] Modules linked in: coretemp kvm_intel kvm crc32_pclmul aesni_intel aes_i586 xts lrw gf128mul ablk_helper cryptd hid_sensor_accel_3d hid_sensor_gyro_3d hid_sensor_magn_3d hid_sensor_trigger industrialio_triggered_buffer kfifo_buf industrialio hid_sensor_iio_common snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_intel snd_hda_codec snd_hwdep snd_pcm snd_page_alloc snd_seq_midi snd_seq_midi_event dm_multipath scsi_dh asix ppdev usbnet snd_rawmidi mii hid_sensor_hub microcode snd_seq rfcomm bnep snd_seq_device bluetooth snd_timer snd parport_pc binfmt_misc soundcore dw_dmac_pci dw_dmac_core mac_hid lp parport dm_mirror dm_region_hash dm_log hid_generic usbhid hid i915(O-) drm_kms_helper(O) igb dca ptp pps_core i2c_algo_bit drm(O) ahci libahci video [ 161.002991] CPU: 0 PID: 2114 Comm: rmmod Tainted: G W O 3.13.0-rc8+ #2 [ 161.002997] Hardware name: NEXCOM VTC1010/Aptio CRB, BIOS 5.6.5 09/24/2013 [ 161.003004] task: dbdd6800 ti: dbe0e000 task.ti: dbe0e000 [ 161.003010] EIP: 0060:[<f84861f8>] EFLAGS: 00010246 CPU: 0 [ 161.003044] EIP is at i915_gem_object_ggtt_unpin+0x88/0x90 [i915] [ 161.003050] EAX: dfce3840 EBX: 00000000 ECX: dfafd690 EDX: dfce3874 [ 161.003056] ESI: c0086b40 EDI: df962e00 EBP: dbe0fe1c ESP: dbe0fe0c [ 161.003062] DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 [ 161.003068] CR0: 8005003b CR2: b7718000 CR3: 1bec0000 CR4: 001007f0 [ 161.003076] Stack: [ 161.003081] 00afc014 00000004 c0086b40 dfafc000 dbe0fe38 f8487e5a dfaa5400 c0086b40 [ 161.003099] dfafc000 dfaa5400 dfaa5414 dbe0fe58 f84741aa 00000000 f89c34b9 dfaa5414 [ 161.003117] dfaa5400 dfaa5400 f644b000 dbe0fe6c f89a5443 dfaa5400 f8505000 f644b000 [ 161.003134] Call Trace: [ 161.003169] [<f8487e5a>] i915_gem_context_fini+0xba/0x1c0 [i915] [ 161.003202] [<f84741aa>] i915_driver_unload+0x1fa/0x2f0 [i915] [ 161.003232] [<f89a5443>] drm_dev_unregister+0x23/0x90 [drm] [ 161.003259] [<f89a54ed>] drm_put_dev+0x3d/0x70 [drm] [ 161.003294] [<f8470615>] i915_pci_remove+0x15/0x20 [i915] [ 161.003306] [<c1338a6f>] pci_device_remove+0x2f/0xa0 [ 161.003317] [<c140c871>] __device_release_driver+0x61/0xc0 [ 161.003328] [<c140d12f>] driver_detach+0x8f/0xa0 [ 161.003341] [<c140c54f>] bus_remove_driver+0x4f/0xc0 [ 161.003353] [<c140d708>] driver_unregister+0x28/0x60 [ 161.003362] [<c10cee42>] ? stop_cpus+0x32/0x40 [ 161.003372] [<c10bd510>] ? module_refcount+0x90/0x90 [ 161.003383] [<c13378c5>] pci_unregister_driver+0x15/0x60 [ 161.003413] [<f89a739f>] drm_pci_exit+0x9f/0xb0 [drm] [ 161.003458] [<f84e624a>] i915_exit+0x1b/0x1d [i915] [ 161.003468] [<c10bf8a8>] SyS_delete_module+0x158/0x1f0 [ 161.003480] [<c1173d5d>] ? ____fput+0xd/0x10 [ 161.003488] [<c106f0fe>] ? task_work_run+0x7e/0xb0 [ 161.003499] [<c165a68d>] sysenter_do_call+0x12/0x28 [ 161.003505] Code: 0f b6 4d f3 8d 51 0f 83 e1 f0 83 e2 0f 09 d1 84 d2 88 48 54 75 07 80 a7 91 00 00 00 7f 83 c4 04 5b 5e 5f 5d c3 8d b6 00 00 00 00 <0f> 0b 8d b6 00 00 00 00 55 89 e5 57 56 53 83 ec 64 3e 8d 74 26 [ 161.003586] EIP: [<f84861f8>] i915_gem_object_ggtt_unpin+0x88/0x90 [i915] SS:ESP 0068:dbe0fe0c v2: Rename the local variable (is_default_ctx) to avoid confusion with the function is_default_ctx(). And correct Jesse's email address. Reported-by: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=73985 Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> Cc: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx> Cc: Ben Widawsky <benjamin.widawsky@xxxxxxxxx> Tested-by: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx> Reviewed-by: Ben Widawsky <benjamin.widawsky@xxxxxxxxx> --- drivers/gpu/drm/i915/i915_gem_context.c | 41 ++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index fed9aaf14465..6a61e1d1a7ce 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -252,6 +252,7 @@ i915_gem_create_context(struct drm_device *dev, struct drm_i915_file_private *file_priv, bool create_vm) { + const bool is_global_default_ctx = file_priv == NULL; struct drm_i915_private *dev_priv = dev->dev_private; struct i915_hw_context *ctx; int ret = 0; @@ -262,6 +263,22 @@ i915_gem_create_context(struct drm_device *dev, if (IS_ERR(ctx)) return ctx; + if (is_global_default_ctx) { + /* We may need to do things with the shrinker which + * require us to immediately switch back to the default + * context. This can cause a problem as pinning the + * default context also requires GTT space which may not + * be available. To avoid this we always pin the default + * context. + */ + ret = i915_gem_obj_ggtt_pin(ctx->obj, + get_context_alignment(dev), 0); + if (ret) { + DRM_DEBUG_DRIVER("Couldn't pin %d\n", ret); + goto err_destroy; + } + } + if (create_vm) { struct i915_hw_ppgtt *ppgtt = create_vm_for_ctx(dev, ctx); @@ -269,34 +286,19 @@ i915_gem_create_context(struct drm_device *dev, DRM_DEBUG_DRIVER("PPGTT setup failed (%ld)\n", PTR_ERR(ppgtt)); ret = PTR_ERR(ppgtt); - goto err_destroy; + goto err_unpin; } else ctx->vm = &ppgtt->base; /* This case is reserved for the global default context and * should only happen once. */ - if (!file_priv) { + if (is_global_default_ctx) { if (WARN_ON(dev_priv->mm.aliasing_ppgtt)) { ret = -EEXIST; - goto err_destroy; + goto err_unpin; } dev_priv->mm.aliasing_ppgtt = ppgtt; - - /* We may need to do things with the shrinker which - * require us to immediately switch back to the default - * context. This can cause a problem as pinning the - * default context also requires GTT space which may not - * be available. To avoid this we always pin the default - * context. - */ - ret = i915_gem_obj_ggtt_pin(ctx->obj, - get_context_alignment(dev), 0); - if (ret) { - DRM_DEBUG_DRIVER("Couldn't pin %d\n", ret); - goto err_destroy; - } - ctx->vm = &dev_priv->mm.aliasing_ppgtt->base; } } else if (USES_ALIASING_PPGTT(dev)) { @@ -309,6 +311,9 @@ i915_gem_create_context(struct drm_device *dev, return ctx; +err_unpin: + if (is_global_default_ctx) + i915_gem_object_ggtt_unpin(ctx->obj); err_destroy: i915_gem_context_unreference(ctx); return ERR_PTR(ret); -- 1.8.5.3 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
