Re: [PATCH v3 1/2] drm/i915/gtt: Do not initialize drm_mm twice.

[Imre Deak <imre.deak@intel.com>]

On ke, 2015-09-16 at 11:49 +0200, Michał Winiarski wrote:
> It would be initialized just moments later by i915_init_vm.
> 
> v2: Commit msg update,
>     s/i915_init_vm/i915_address_space_init, move to i915_gem_gtt.c,
>     init address_space during i915_gem_setup_global_gtt for ggtt.
> v3: Do not init global_link - we are adding it to vm_list moments later,
>     make i915_address_space_init static, use OOP style parameter order.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>

With this patch I get the oops below when booting. Looking at it it's
caused by dev_priv->gtt being initialized now too late for
i915_gem_object_create_stolen_for_preallocated().

[   13.547091] BUG: unable to handle kernel NULL pointer dereference at           (null)
[   13.555857] IP: [<ffffffffa0243f09>] i915_gem_object_create_stolen_for_preallocated+0x1e9/0x320 [i915]
[   13.566380] PGD 0 
[   13.568634] Oops: 0002 [#1] PREEMPT SMP 
[   13.573035] Modules linked in: snd_hda_codec_hdmi snd_hda_intel i915(O+) snd_hda_codec snd_hwdep snd_hda_core snd_pcm snd_seq_dummy snd_seq_oss kvm_intel i2c_algo_bit snd_seq_midi drm_kms_helper kvm snd_rawmidi syscopyarea sysfillrect sysimgblt snd_seq_midi_event crc32c_intel fb_sys_fops microcode snd_seq drm snd_seq_device snd_timer efivars intel_gtt snd agpgart fuse
[   13.609814] CPU: 1 PID: 329 Comm: systemd-udevd Tainted: G          IO    4.3.0-rc2-bxt+ #177
[   13.619401] Hardware name: Intel Corp. BROXTON A1 PLATFORM/TABLET, BIOS BXTM_IFWI_X64_R_2015_36_4_00 09/02/2015
[   13.630681] task: ffff88007674cc80 ti: ffff880075c1c000 task.ti: ffff880075c1c000
[   13.639049] RIP: 0010:[<ffffffffa0243f09>]  [<ffffffffa0243f09>] i915_gem_object_create_stolen_for_preallocated+0x1e9/0x320 [i915]
[   13.652219] RSP: 0018:ffff880075c1f810  EFLAGS: 00010286
[   13.658154] RAX: 0000000000000000 RBX: 00000000008ca000 RCX: ffff880072e61ca8
[   13.666129] RDX: ffff880072e61c00 RSI: ffff88006de4a0c8 RDI: ffff88007674d4c8
[   13.674103] RBP: ffff880075c1f848 R08: 00000000000196f0 R09: ffff880072e61c00
[   13.682078] R10: 0000000000000001 R11: 0000000000000001 R12: ffff88006de49f18
[   13.690053] R13: ffff88006df00000 R14: ffff8800754154e0 R15: 0000000000000000
[   13.698034] FS:  00007f4d61c0f880(0000) GS:ffff880079480000(0000) knlGS:0000000000000000
[   13.707089] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   13.713513] CR2: 0000000000000000 CR3: 0000000073304000 CR4: 00000000003406e0
[   13.721494] Stack:
[   13.723740]  ffff88006de4a048 ffff88006de49e18 ffff880075c1f928 ffff880075c1f858
[   13.732044]  ffff880072cba540 ffff880076061000 ffff880076061000 ffff880075c1f8e8
[   13.740345]  ffffffffa027bdbc 0000000000000000 0000000000000000
[   13.745411] sdhci-pci 0000:00:1c.0: power state changed by ACPI to D3cold
[   13.754373]  0000000000000000

[   13.757903] Call Trace:
[   13.760709]  [<ffffffffa027bdbc>] intel_alloc_initial_plane_obj.isra.56+0x7c/0x190 [i915]
[   13.769930]  [<ffffffffa0287654>] intel_modeset_init+0xbb4/0x1a10 [i915]
[   13.777495]  [<ffffffffa02c03fc>] i915_driver_load+0xeec/0x14e0 [i915]
[   13.784831]  [<ffffffffa0069569>] drm_dev_register+0xa9/0xc0 [drm]
[   13.791762]  [<ffffffffa006c37d>] drm_get_pci_dev+0x8d/0x1e0 [drm]
[   13.798682]  [<ffffffff818c7e42>] ? _raw_spin_unlock_irqrestore+0x42/0x70
[   13.806321]  [<ffffffffa0201234>] i915_pci_probe+0x34/0x50 [i915]
[   13.813144]  [<ffffffff81452cbc>] pci_device_probe+0x8c/0x100
[   13.819577]  [<ffffffff8152a5a9>] driver_probe_device+0x169/0x4a0
[   13.826393]  [<ffffffff8152a968>] __driver_attach+0x88/0x90
[   13.832628]  [<ffffffff8152a8e0>] ? driver_probe_device+0x4a0/0x4a0
[   13.839643]  [<ffffffff81528126>] bus_for_each_dev+0x66/0xa0
[   13.845976]  [<ffffffff81529f5e>] driver_attach+0x1e/0x20
[   13.852012]  [<ffffffff815299b4>] bus_add_driver+0x1f4/0x290
[   13.858343]  [<ffffffff8152b540>] driver_register+0x60/0xe0
[   13.864577]  [<ffffffff814513b0>] __pci_register_driver+0x60/0x70
[   13.871410]  [<ffffffffa006c5b0>] drm_pci_init+0xe0/0x110 [drm]
[   13.878034]  [<ffffffff810d024d>] ? trace_hardirqs_on+0xd/0x10
[   13.884560]  [<ffffffffa032b000>] ? 0xffffffffa032b000
[   13.890350]  [<ffffffffa032b094>] i915_init+0x94/0x9b [i915]
[   13.896685]  [<ffffffff810002f3>] do_one_initcall+0xb3/0x1d0
[   13.903016]  [<ffffffff810ed246>] ? rcu_read_lock_sched_held+0x86/0x90
[   13.910323]  [<ffffffff811e21f1>] ? kmem_cache_alloc_trace+0x1c1/0x270
[   13.917626]  [<ffffffff81192893>] ? do_init_module+0x27/0x1ea
[   13.924052]  [<ffffffff811928cc>] do_init_module+0x60/0x1ea
[   13.930288]  [<ffffffff81114b02>] load_module+0x2012/0x2580
[   13.936521]  [<ffffffff811109a0>] ? store_uevent+0x40/0x40
[   13.942659]  [<ffffffff81111246>] ? copy_module_from_fd.isra.36+0xf6/0x140
[   13.950350]  [<ffffffff81115270>] SyS_finit_module+0x80/0xb0
[   13.956681]  [<ffffffff818c86db>] entry_SYSCALL_64_fastpath+0x16/0x73


> ---
>  drivers/gpu/drm/i915/i915_drv.h     |  2 --
>  drivers/gpu/drm/i915/i915_gem.c     | 14 --------------
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 26 ++++++++++++++++++--------
>  3 files changed, 18 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3bf8a9b..039227d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2804,8 +2804,6 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
>  						  size_t size);
>  struct drm_i915_gem_object *i915_gem_object_create_from_data(
>  		struct drm_device *dev, const void *data, size_t size);
> -void i915_init_vm(struct drm_i915_private *dev_priv,
> -		  struct i915_address_space *vm);
>  void i915_gem_free_object(struct drm_gem_object *obj);
>  void i915_gem_vma_destroy(struct i915_vma *vma);
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index cb0df7e..4811f8a 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4844,18 +4844,6 @@ init_ring_lists(struct intel_engine_cs *ring)
>  	INIT_LIST_HEAD(&ring->request_list);
>  }
>  
> -void i915_init_vm(struct drm_i915_private *dev_priv,
> -		  struct i915_address_space *vm)
> -{
> -	if (!i915_is_ggtt(vm))
> -		drm_mm_init(&vm->mm, vm->start, vm->total);
> -	vm->dev = dev_priv->dev;
> -	INIT_LIST_HEAD(&vm->active_list);
> -	INIT_LIST_HEAD(&vm->inactive_list);
> -	INIT_LIST_HEAD(&vm->global_link);
> -	list_add_tail(&vm->global_link, &dev_priv->vm_list);
> -}
> -
>  void
>  i915_gem_load(struct drm_device *dev)
>  {
> @@ -4879,8 +4867,6 @@ i915_gem_load(struct drm_device *dev)
>  				  NULL);
>  
>  	INIT_LIST_HEAD(&dev_priv->vm_list);
> -	i915_init_vm(dev_priv, &dev_priv->gtt.base);
> -
>  	INIT_LIST_HEAD(&dev_priv->context_list);
>  	INIT_LIST_HEAD(&dev_priv->mm.unbound_list);
>  	INIT_LIST_HEAD(&dev_priv->mm.bound_list);
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 8786281..01f3521 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2121,6 +2121,16 @@ static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
>  		return gen8_ppgtt_init(ppgtt);
>  }
>  
> +static void i915_address_space_init(struct i915_address_space *vm,
> +				    struct drm_i915_private *dev_priv)
> +{
> +	drm_mm_init(&vm->mm, vm->start, vm->total);
> +	vm->dev = dev_priv->dev;
> +	INIT_LIST_HEAD(&vm->active_list);
> +	INIT_LIST_HEAD(&vm->inactive_list);
> +	list_add_tail(&vm->global_link, &dev_priv->vm_list);
> +}
> +
>  int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -2129,9 +2139,7 @@ int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
>  	ret = __hw_ppgtt_init(dev, ppgtt);
>  	if (ret == 0) {
>  		kref_init(&ppgtt->ref);
> -		drm_mm_init(&ppgtt->base.mm, ppgtt->base.start,
> -			    ppgtt->base.total);
> -		i915_init_vm(dev_priv, &ppgtt->base);
> +		i915_address_space_init(&ppgtt->base, dev_priv);
>  	}
>  
>  	return ret;
> @@ -2618,11 +2626,13 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
>  
>  	BUG_ON(mappable_end > end);
>  
> -	/* Subtract the guard page ... */
> -	drm_mm_init(&ggtt_vm->mm, start, end - start - PAGE_SIZE);
> +	ggtt_vm->start = start;
>  
> -	dev_priv->gtt.base.start = start;
> -	dev_priv->gtt.base.total = end - start;
> +	/* Subtract the guard page before address space initialization to
> +	 * shrink the range used by drm_mm */
> +	ggtt_vm->total = end - start - PAGE_SIZE;
> +	i915_address_space_init(ggtt_vm, dev_priv);
> +	ggtt_vm->total += PAGE_SIZE;
>  
>  	if (intel_vgpu_active(dev)) {
>  		ret = intel_vgt_balloon(dev);
> @@ -2631,7 +2641,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
>  	}
>  
>  	if (!HAS_LLC(dev))
> -		dev_priv->gtt.base.mm.color_adjust = i915_gtt_color_adjust;
> +		ggtt_vm->mm.color_adjust = i915_gtt_color_adjust;
>  
>  	/* Mark any preallocated objects as occupied */
>  	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx