From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 46A48C35646 for ; Fri, 21 Feb 2020 13:31:50 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 23DEE222C4 for ; Fri, 21 Feb 2020 13:31:50 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 23DEE222C4 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=intel-gfx-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id A3D086E2BE; Fri, 21 Feb 2020 13:31:49 +0000 (UTC) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTPS id 7E9026E2BE for ; Fri, 21 Feb 2020 13:31:47 +0000 (UTC) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 21 Feb 2020 05:31:46 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,468,1574150400"; d="scan'208";a="230437268" Received: from gumm-mobl2.ger.corp.intel.com (HELO [10.252.39.181]) ([10.252.39.181]) by fmsmga008.fm.intel.com with ESMTP; 21 Feb 2020 05:31:45 -0800 To: Chris Wilson , intel-gfx@lists.freedesktop.org References: <20200221094002.2481703-3-chris@chris-wilson.co.uk> <20200221131529.2742129-1-chris@chris-wilson.co.uk> From: Maarten Lankhorst Message-ID: <63f2f9e9-a1fc-3a03-9501-e3be7d3a7e6c@linux.intel.com> Date: Fri, 21 Feb 2020 14:31:44 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: <20200221131529.2742129-1-chris@chris-wilson.co.uk> Content-Language: en-US Subject: Re: [Intel-gfx] [PATCH] drm/i915/gem: Only call eb_lookup_vma once during execbuf ioctl X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" Op 21-02-2020 om 14:15 schreef Chris Wilson: > As we no longer stash anything inside i915_vma under the exclusive > protection of struct_mutex, we do not need to revoke the i915_vma > stashes before dropping struct_mutex to handle pagefaults. Knowing that > we must drop the struct_mutex while keeping the eb->vma around, means > that we are required to hold onto to the object reference until we have > marked the vma as active. > > Fixes: 155ab8836caa ("drm/i915: Move object close under its own lock") > Signed-off-by: Chris Wilson > Cc: Maarten Lankhorst > --- > .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 109 +++++++----------- > 1 file changed, 43 insertions(+), 66 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > index 3ccd3a096486..e4e50155145e 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > @@ -48,17 +48,15 @@ enum { > #define DBG_FORCE_RELOC 0 /* choose one of the above! */ > }; > > -#define __EXEC_OBJECT_HAS_REF BIT(31) > -#define __EXEC_OBJECT_HAS_PIN BIT(30) > -#define __EXEC_OBJECT_HAS_FENCE BIT(29) > -#define __EXEC_OBJECT_NEEDS_MAP BIT(28) > -#define __EXEC_OBJECT_NEEDS_BIAS BIT(27) > -#define __EXEC_OBJECT_INTERNAL_FLAGS (~0u << 27) /* all of the above */ > +#define __EXEC_OBJECT_HAS_PIN BIT(31) > +#define __EXEC_OBJECT_HAS_FENCE BIT(30) > +#define __EXEC_OBJECT_NEEDS_MAP BIT(29) > +#define __EXEC_OBJECT_NEEDS_BIAS BIT(28) > +#define __EXEC_OBJECT_INTERNAL_FLAGS (~0u << 28) /* all of the above */ > #define __EXEC_OBJECT_RESERVED (__EXEC_OBJECT_HAS_PIN | __EXEC_OBJECT_HAS_FENCE) > > #define __EXEC_HAS_RELOC BIT(31) > -#define __EXEC_VALIDATED BIT(30) > -#define __EXEC_INTERNAL_FLAGS (~0u << 30) > +#define __EXEC_INTERNAL_FLAGS (~0u << 31) > #define UPDATE PIN_OFFSET_FIXED > > #define BATCH_OFFSET_BIAS (256*1024) > @@ -473,24 +471,17 @@ eb_validate_vma(struct i915_execbuffer *eb, > return 0; > } > > -static int > +static void > eb_add_vma(struct i915_execbuffer *eb, > unsigned int i, unsigned batch_idx, > struct i915_vma *vma) > { > struct drm_i915_gem_exec_object2 *entry = &eb->exec[i]; > struct eb_vma *ev = &eb->vma[i]; > - int err; > > GEM_BUG_ON(i915_vma_is_closed(vma)); > > - if (!(eb->args->flags & __EXEC_VALIDATED)) { > - err = eb_validate_vma(eb, entry, vma); > - if (unlikely(err)) > - return err; > - } > - > - ev->vma = vma; > + ev->vma = i915_vma_get(vma); > ev->exec = entry; > ev->flags = entry->flags; > > @@ -523,20 +514,14 @@ eb_add_vma(struct i915_execbuffer *eb, > eb->batch = ev; > } > > - err = 0; > if (eb_pin_vma(eb, entry, ev)) { > if (entry->offset != vma->node.start) { > entry->offset = vma->node.start | UPDATE; > eb->args->flags |= __EXEC_HAS_RELOC; > } > } else { > - eb_unreserve_vma(ev); > - > list_add_tail(&ev->bind_link, &eb->unbound); > - if (drm_mm_node_allocated(&vma->node)) > - err = i915_vma_unbind(vma); > } > - return err; > } > > static inline int use_cpu_reloc(const struct reloc_cache *cache, > @@ -585,6 +570,14 @@ static int eb_reserve_vma(const struct i915_execbuffer *eb, struct eb_vma *ev) > pin_flags |= BATCH_OFFSET_BIAS | PIN_OFFSET_BIAS; > } > > + if (drm_mm_node_allocated(&vma->node) && > + eb_vma_misplaced(entry, vma, ev->flags)) { > + eb_unreserve_vma(ev); > + err = i915_vma_unbind(vma); > + if (err) > + return err; > + } > + > err = i915_vma_pin(vma, > entry->pad_to_size, entry->alignment, > pin_flags); > @@ -643,7 +636,7 @@ static int eb_reserve(struct i915_execbuffer *eb) > if (err) > break; > } > - if (err != -ENOSPC) > + if (!(err == -ENOSPC || err == -EAGAIN)) > return err; > > /* Resort *all* the objects into priority order */ > @@ -673,6 +666,11 @@ static int eb_reserve(struct i915_execbuffer *eb) > } > list_splice_tail(&last, &eb->unbound); > > + if (err == -EAGAIN) { > + flush_workqueue(eb->i915->mm.userptr_wq); > + continue; > + } > + > switch (pass++) { > case 0: > break; > @@ -726,17 +724,14 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb) > unsigned int i, batch; > int err; > > + if (unlikely(i915_gem_context_is_closed(eb->gem_context))) > + return -ENOENT; > + > INIT_LIST_HEAD(&eb->relocs); > INIT_LIST_HEAD(&eb->unbound); > > batch = eb_batch_index(eb); > > - mutex_lock(&eb->gem_context->mutex); > - if (unlikely(i915_gem_context_is_closed(eb->gem_context))) { > - err = -ENOENT; > - goto err_ctx; > - } > - > for (i = 0; i < eb->buffer_count; i++) { > u32 handle = eb->exec[i].handle; > struct i915_lut_handle *lut; > @@ -781,25 +776,19 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb) > i915_gem_object_unlock(obj); > > add_vma: > - err = eb_add_vma(eb, i, batch, vma); > + err = eb_validate_vma(eb, &eb->exec[i], vma); > if (unlikely(err)) > goto err_vma; > > - GEM_BUG_ON(drm_mm_node_allocated(&vma->node) && > - eb_vma_misplaced(&eb->exec[i], vma, eb->vma[i].flags)); > + eb_add_vma(eb, i, batch, vma); > } > > - mutex_unlock(&eb->gem_context->mutex); > - > - eb->args->flags |= __EXEC_VALIDATED; > - return eb_reserve(eb); > + return 0; > > err_obj: > i915_gem_object_put(obj); > err_vma: > eb->vma[i].vma = NULL; > -err_ctx: > - mutex_unlock(&eb->gem_context->mutex); > return err; > } > > @@ -840,19 +829,10 @@ static void eb_release_vmas(const struct i915_execbuffer *eb) > if (ev->flags & __EXEC_OBJECT_HAS_PIN) > __eb_unreserve_vma(vma, ev->flags); > > - if (ev->flags & __EXEC_OBJECT_HAS_REF) > - i915_vma_put(vma); > + i915_vma_put(vma); > } > } > > -static void eb_reset_vmas(const struct i915_execbuffer *eb) > -{ > - eb_release_vmas(eb); > - if (eb->lut_size > 0) > - memset(eb->buckets, 0, > - sizeof(struct hlist_head) << eb->lut_size); > -} > - > static void eb_destroy(const struct i915_execbuffer *eb) > { > GEM_BUG_ON(eb->reloc_cache.rq); > @@ -1661,8 +1641,6 @@ static noinline int eb_relocate_slow(struct i915_execbuffer *eb) > goto out; > } > > - /* We may process another execbuffer during the unlock... */ > - eb_reset_vmas(eb); > mutex_unlock(&dev->struct_mutex); > > /* > @@ -1701,11 +1679,6 @@ static noinline int eb_relocate_slow(struct i915_execbuffer *eb) > goto out; > } > > - /* reacquire the objects */ > - err = eb_lookup_vmas(eb); > - if (err) > - goto err; > - > GEM_BUG_ON(!eb->batch); > > list_for_each_entry(ev, &eb->relocs, reloc_link) { > @@ -1756,8 +1729,17 @@ static noinline int eb_relocate_slow(struct i915_execbuffer *eb) > > static int eb_relocate(struct i915_execbuffer *eb) > { > - if (eb_lookup_vmas(eb)) > - goto slow; > + int err; > + > + mutex_lock(&eb->gem_context->mutex); > + err = eb_lookup_vmas(eb); > + mutex_unlock(&eb->gem_context->mutex); > + if (err) > + return err; > + > + err = eb_reserve(eb); > + if (err) > + return err; > > /* The objects are in their final locations, apply the relocations. */ > if (eb->args->flags & __EXEC_HAS_RELOC) { > @@ -1765,14 +1747,11 @@ static int eb_relocate(struct i915_execbuffer *eb) > > list_for_each_entry(ev, &eb->relocs, reloc_link) { > if (eb_relocate_vma(eb, ev)) > - goto slow; > + return eb_relocate_slow(eb); > } > } > > return 0; > - > -slow: > - return eb_relocate_slow(eb); > } > > static int eb_move_to_gpu(struct i915_execbuffer *eb) > @@ -1854,8 +1833,7 @@ static int eb_move_to_gpu(struct i915_execbuffer *eb) > i915_vma_unlock(vma); > > __eb_unreserve_vma(vma, flags); > - if (unlikely(flags & __EXEC_OBJECT_HAS_REF)) > - i915_vma_put(vma); > + i915_vma_put(vma); > > ev->vma = NULL; > } > @@ -2115,8 +2093,7 @@ static int eb_parse(struct i915_execbuffer *eb) > goto err_trampoline; > > eb->vma[eb->buffer_count].vma = i915_vma_get(shadow); > - eb->vma[eb->buffer_count].flags = > - __EXEC_OBJECT_HAS_PIN | __EXEC_OBJECT_HAS_REF; > + eb->vma[eb->buffer_count].flags = __EXEC_OBJECT_HAS_PIN; > eb->batch = &eb->vma[eb->buffer_count++]; > > eb->trampoline = trampoline; Did you steal this from me? :P I Ihave a similar patch in my series.. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx