Re: [PATCH] drm/i915: Always pin the default context

[Ben Widawsky <benjamin.widawsky@intel.com>]

On Thu, Jan 23, 2014 at 06:30:02PM +0000, Chris Wilson wrote:
> 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.)

Shouldn't it be:
"Restricted to Baytrail machines, or gen6+ machines which force 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
> 
> Reported-by: Jesse Barnes <jess@virtuousgeek.org>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=73985
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jesse Barnes <jess@virtuousgeek.org>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> ---
>  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..bc12d31517e7 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_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_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_default_ctx) {

I don't know what your base is, but I believe this is wrong.

The below commit would make this apply to all contexts which id == 0,
which is not what we want for the special casing of the real global
default context.

commit 0eea67eb26000657079b7fc41079097942339452
Author: Ben Widawsky <ben@bwidawsk.net>
Date:   Fri Dec 6 14:11:19 2013 -0800

    drm/i915: Create a per file_priv default context

If you like the way the code looks, make it is_global_default_ctx, and
resurrect the old definition. Then I am happy.

>  			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_default_ctx)
> +		i915_gem_object_ggtt_unpin(ctx->obj);
>  err_destroy:
>  	i915_gem_context_unreference(ctx);
>  	return ERR_PTR(ret);
> -- 
> 1.8.5.3
> 

-- 
Ben Widawsky, Intel Open Source Technology Center