From: Chris Wilson <chris@chris-wilson.co.uk> To: Andi Shyti <andi@etezian.org> Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org, andi@etezian.org, andi.shyti@intel.com Subject: Re: [Intel-gfx] [PATCH 01/23] drm/i915: Drop vm.ref for duplicate vma on construction Date: Thu, 02 Jul 2020 21:38:41 +0100 [thread overview] Message-ID: <159372232179.22925.7779642345726039521@build.alporthouse.com> (raw) In-Reply-To: <20200702202545.GA1969@jack.zhora.eu> Quoting Andi Shyti (2020-07-02 21:25:45) > Hi Chris, > > > diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c > > index 1f63c4a1f055..7fe1f317cd2b 100644 > > --- a/drivers/gpu/drm/i915/i915_vma.c > > +++ b/drivers/gpu/drm/i915/i915_vma.c > > @@ -198,6 +198,7 @@ vma_create(struct drm_i915_gem_object *obj, > > cmp = i915_vma_compare(pos, vm, view); > > if (cmp == 0) { > > spin_unlock(&obj->vma.lock); > > + i915_vm_put(vm); > > i915_vma_free(vma); > > You are forgettin one return without dereferencing it. > > would this be a solution: > > @@ -106,6 +106,7 @@ vma_create(struct drm_i915_gem_object *obj, > { > struct i915_vma *vma; > struct rb_node *rb, **p; > + struct i915_vma *pos = ERR_PTR(-E2BIG); > > /* The aliasing_ppgtt should never be used directly! */ > GEM_BUG_ON(vm == &vm->gt->ggtt->alias->vm); > @@ -185,7 +186,6 @@ vma_create(struct drm_i915_gem_object *obj, > rb = NULL; > p = &obj->vma.tree.rb_node; > while (*p) { > - struct i915_vma *pos; > long cmp; > > rb = *p; > @@ -197,12 +197,8 @@ vma_create(struct drm_i915_gem_object *obj, > * and dispose of ours. > */ > cmp = i915_vma_compare(pos, vm, view); > - if (cmp == 0) { > - spin_unlock(&obj->vma.lock); > - i915_vm_put(vm); > - i915_vma_free(vma); > - return pos; > - } > + if (!cmp) > + goto err_unlock; Yeah, but you might as well do if (cmp < 0) p = right; else if (cmp > 0) p = left; else goto err_unlock; > if (cmp < 0) > p = &rb->rb_right; > @@ -230,8 +226,9 @@ vma_create(struct drm_i915_gem_object *obj, > err_unlock: > spin_unlock(&obj->vma.lock); > err_vma: > + i915_vm_put(vm); > i915_vma_free(vma); > - return ERR_PTR(-E2BIG); > + return pos; > } > > Andi Ta, going to send that as a patch? -Chris
WARNING: multiple messages have this Message-ID (diff)
From: Chris Wilson <chris@chris-wilson.co.uk> To: Andi Shyti <andi@etezian.org> Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org Subject: Re: [Intel-gfx] [PATCH 01/23] drm/i915: Drop vm.ref for duplicate vma on construction Date: Thu, 02 Jul 2020 21:38:41 +0100 [thread overview] Message-ID: <159372232179.22925.7779642345726039521@build.alporthouse.com> (raw) In-Reply-To: <20200702202545.GA1969@jack.zhora.eu> Quoting Andi Shyti (2020-07-02 21:25:45) > Hi Chris, > > > diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c > > index 1f63c4a1f055..7fe1f317cd2b 100644 > > --- a/drivers/gpu/drm/i915/i915_vma.c > > +++ b/drivers/gpu/drm/i915/i915_vma.c > > @@ -198,6 +198,7 @@ vma_create(struct drm_i915_gem_object *obj, > > cmp = i915_vma_compare(pos, vm, view); > > if (cmp == 0) { > > spin_unlock(&obj->vma.lock); > > + i915_vm_put(vm); > > i915_vma_free(vma); > > You are forgettin one return without dereferencing it. > > would this be a solution: > > @@ -106,6 +106,7 @@ vma_create(struct drm_i915_gem_object *obj, > { > struct i915_vma *vma; > struct rb_node *rb, **p; > + struct i915_vma *pos = ERR_PTR(-E2BIG); > > /* The aliasing_ppgtt should never be used directly! */ > GEM_BUG_ON(vm == &vm->gt->ggtt->alias->vm); > @@ -185,7 +186,6 @@ vma_create(struct drm_i915_gem_object *obj, > rb = NULL; > p = &obj->vma.tree.rb_node; > while (*p) { > - struct i915_vma *pos; > long cmp; > > rb = *p; > @@ -197,12 +197,8 @@ vma_create(struct drm_i915_gem_object *obj, > * and dispose of ours. > */ > cmp = i915_vma_compare(pos, vm, view); > - if (cmp == 0) { > - spin_unlock(&obj->vma.lock); > - i915_vm_put(vm); > - i915_vma_free(vma); > - return pos; > - } > + if (!cmp) > + goto err_unlock; Yeah, but you might as well do if (cmp < 0) p = right; else if (cmp > 0) p = left; else goto err_unlock; > if (cmp < 0) > p = &rb->rb_right; > @@ -230,8 +226,9 @@ vma_create(struct drm_i915_gem_object *obj, > err_unlock: > spin_unlock(&obj->vma.lock); > err_vma: > + i915_vm_put(vm); > i915_vma_free(vma); > - return ERR_PTR(-E2BIG); > + return pos; > } > > Andi Ta, going to send that as a patch? -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2020-07-02 20:38 UTC|newest] Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-02 8:32 [PATCH 01/23] drm/i915: Drop vm.ref for duplicate vma on construction Chris Wilson 2020-07-02 8:32 ` [Intel-gfx] " Chris Wilson 2020-07-02 8:32 ` [Intel-gfx] [PATCH 02/23] drm/i915/gem: Split the context's obj:vma lut into its own mutex Chris Wilson 2020-07-02 22:09 ` Andi Shyti 2020-07-02 22:14 ` Chris Wilson 2020-07-02 8:32 ` [Intel-gfx] [PATCH 03/23] drm/i915/gem: Drop forced struct_mutex from shrinker_taints_mutex Chris Wilson 2020-07-02 22:24 ` Andi Shyti 2020-07-02 8:32 ` [Intel-gfx] [PATCH 04/23] drm/i915/gem: Only revoke mmap handlers if active Chris Wilson 2020-07-02 12:35 ` Tvrtko Ursulin 2020-07-02 12:47 ` Chris Wilson 2020-07-02 12:54 ` Chris Wilson 2020-07-02 8:32 ` [Intel-gfx] [PATCH 05/23] drm/i915: Export ppgtt_bind_vma Chris Wilson 2020-07-03 10:09 ` Andi Shyti 2020-07-02 8:32 ` [Intel-gfx] [PATCH 06/23] drm/i915: Preallocate stashes for vma page-directories Chris Wilson 2020-07-03 16:47 ` Tvrtko Ursulin 2020-07-02 8:32 ` [Intel-gfx] [PATCH 07/23] drm/i915: Switch to object allocations for page directories Chris Wilson 2020-07-03 8:44 ` Tvrtko Ursulin 2020-07-03 9:00 ` Chris Wilson 2020-07-03 9:24 ` Tvrtko Ursulin 2020-07-03 9:49 ` Chris Wilson 2020-07-03 16:34 ` Tvrtko Ursulin 2020-07-03 16:36 ` Tvrtko Ursulin 2020-07-02 8:32 ` [Intel-gfx] [PATCH 08/23] drm/i915/gem: Don't drop the timeline lock during execbuf Chris Wilson 2020-07-02 8:32 ` [Intel-gfx] [PATCH 09/23] drm/i915/gem: Rename execbuf.bind_link to unbound_link Chris Wilson 2020-07-02 8:32 ` [Intel-gfx] [PATCH 10/23] drm/i915/gem: Break apart the early i915_vma_pin from execbuf object lookup Chris Wilson 2020-07-02 8:32 ` [Intel-gfx] [PATCH 11/23] drm/i915/gem: Remove the call for no-evict i915_vma_pin Chris Wilson 2020-07-03 8:59 ` Tvrtko Ursulin 2020-07-03 9:23 ` Chris Wilson 2020-07-06 14:43 ` Tvrtko Ursulin 2020-07-02 8:32 ` [Intel-gfx] [PATCH 12/23] drm/i915: Add list_for_each_entry_safe_continue_reverse Chris Wilson 2020-07-02 8:32 ` [Intel-gfx] [PATCH 13/23] drm/i915: Always defer fenced work to the worker Chris Wilson 2020-07-02 8:32 ` [Intel-gfx] [PATCH 14/23] drm/i915/gem: Assign context id for async work Chris Wilson 2020-07-02 8:32 ` [Intel-gfx] [PATCH 15/23] drm/i915: Export a preallocate variant of i915_active_acquire() Chris Wilson 2020-07-02 8:32 ` [Intel-gfx] [PATCH 16/23] drm/i915/gem: Separate the ww_mutex walker into its own list Chris Wilson 2020-07-02 8:32 ` [Intel-gfx] [PATCH 17/23] drm/i915/gem: Asynchronous GTT unbinding Chris Wilson 2020-07-05 22:01 ` Andi Shyti 2020-07-05 22:07 ` Chris Wilson 2020-07-02 8:32 ` [Intel-gfx] [PATCH 18/23] drm/i915/gem: Bind the fence async for execbuf Chris Wilson 2020-07-02 8:32 ` [Intel-gfx] [PATCH 19/23] drm/i915/gem: Include cmdparser in common execbuf pinning Chris Wilson 2020-07-02 8:32 ` [Intel-gfx] [PATCH 20/23] drm/i915/gem: Include secure batch " Chris Wilson 2020-07-02 8:32 ` [Intel-gfx] [PATCH 21/23] drm/i915/gem: Reintroduce multiple passes for reloc processing Chris Wilson 2020-07-02 8:32 ` [Intel-gfx] [PATCH 22/23] drm/i915: Add an implementation for i915_gem_ww_ctx locking, v2 Chris Wilson 2020-07-02 22:32 ` kernel test robot 2020-07-02 8:32 ` [Intel-gfx] [PATCH 23/23] drm/i915/gem: Pull execbuf dma resv under a single critical section Chris Wilson 2020-07-02 9:17 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [01/23] drm/i915: Drop vm.ref for duplicate vma on construction Patchwork 2020-07-02 9:18 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork 2020-07-02 9:40 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork 2020-07-02 12:27 ` [Intel-gfx] [PATCH 01/23] " Tvrtko Ursulin 2020-07-02 12:27 ` Tvrtko Ursulin 2020-07-02 13:08 ` [Intel-gfx] ✗ Fi.CI.IGT: failure for series starting with [01/23] " Patchwork 2020-07-02 20:25 ` [Intel-gfx] [PATCH 01/23] " Andi Shyti 2020-07-02 20:25 ` Andi Shyti 2020-07-02 20:38 ` Chris Wilson [this message] 2020-07-02 20:38 ` Chris Wilson 2020-07-02 20:56 ` Andi Shyti 2020-07-02 20:56 ` Andi Shyti
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=159372232179.22925.7779642345726039521@build.alporthouse.com \ --to=chris@chris-wilson.co.uk \ --cc=andi.shyti@intel.com \ --cc=andi@etezian.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=stable@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.