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

Winiarski, Michal
2015-09-24T11:21:09Z

On Wed, 2015-09-23 at 15:55 -0300, Paulo Zanoni wrote:
> 2015-09-23 13:48 GMT-03:00 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().
>
> My BDW machine stopped booting, and git bisect tells me this patch is
> the cause. I didn't setup anything to help me grab the logs before
> the
> crash, so I can't confirm the backtrace below.
>
> Something interesting is that if I boot with i915.modeset=0, then
> later "rmmod i915; modprobe i915 modeset=1", the driver loads
> successfully.

drm/i915: Defer adding preallocated stolen objects to the VM list
from Chris fixes this. I missed it because I usually just reload i915
module after the machine boots - sorry for breaking your setups :(

-Michał

> >
> > [ 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