| Message ID | 1442396940-15855-1-git-send-email-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 16, 2015 at 11:49:00AM +0200, Micha? Winiarski wrote: Rearrange the code such that i915_init_vm() is next to its callers inside i915_gem_gtt (and so we can make it static). After removing the dance around the files, it is clear that we are repeating some work inside the initializers (such as calling drm_mm_init() multiple times), so take advantage of the refactor to also remove some redundant code and clean up the interface. > 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> Reviewed-by Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Thu, Sep 17, 2015 at 11:59:19AM +0100, Chris Wilson wrote: > On Wed, Sep 16, 2015 at 11:49:00AM +0200, Micha? Winiarski wrote: > > Rearrange the code such that i915_init_vm() is next to its callers > inside i915_gem_gtt (and so we can make it static). After removing the > dance around the files, it is clear that we are repeating some work > inside the initializers (such as calling drm_mm_init() multiple times), > so take advantage of the refactor to also remove some redundant code and > clean up the interface. Added to the commit message. > > 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> > Reviewed-by Chris Wilson <chris@chris-wilson.co.uk> Queued for -next, thanks for the patch. -Daniel
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) {
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. > > [ 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
On 09/23/2015 11:55 AM, 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. > I see this too on BYT. Can we get this reverted or get a fix? Michal? Thanks, Jesse
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 > > >
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) {
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> --- 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(-)