From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>,
DRI Development <dri-devel@lists.freedesktop.org>
Cc: "Intel Graphics Development" <intel-gfx@lists.freedesktop.org>,
"Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
"Daniel Vetter" <daniel.vetter@intel.com>,
"Jon Bloomfield" <jon.bloomfield@intel.com>,
"Chris Wilson" <chris@chris-wilson.co.uk>,
"Joonas Lahtinen" <joonas.lahtinen@linux.intel.com>,
"Thomas Hellström" <thomas.hellstrom@linux.intel.com>,
"Matthew Auld" <matthew.auld@intel.com>,
"Lionel Landwerlin" <lionel.g.landwerlin@intel.com>,
"Dave Airlie" <airlied@redhat.com>,
"Jason Ekstrand" <jason@jlekstrand.net>
Subject: Re: [Intel-gfx] [PATCH 05/11] drm/i915: Rename i915_gem_context_get_vm_rcu to i915_gem_context_get_eb_vm
Date: Fri, 3 Sep 2021 09:05:00 +0100 [thread overview]
Message-ID: <2a1f1395-2fe3-3c3d-48a6-bf741f910023@linux.intel.com> (raw)
In-Reply-To: <20210902142057.929669-5-daniel.vetter@ffwll.ch>
On 02/09/2021 15:20, Daniel Vetter wrote:
> The important part isn't so much that this does an rcu lookup - that's
> more an implementation detail, which will also be removed.
>
> The thing that makes this different from other functions is that it's
> gettting you the vm that batchbuffers will run in for that gem
> context, which is either a full ppgtt stored in gem->ctx, or the ggtt.
>
> We'll make more use of this function later on.
>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jon Bloomfield <jon.bloomfield@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Jason Ekstrand <jason@jlekstrand.net>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_context.h | 2 +-
> drivers/gpu/drm/i915/gem/selftests/huge_pages.c | 4 ++--
> drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c | 4 ++--
> drivers/gpu/drm/i915/gt/selftest_execlists.c | 2 +-
> drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 2 +-
> drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 4 ++--
> drivers/gpu/drm/i915/selftests/i915_vma.c | 2 +-
> 7 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h b/drivers/gpu/drm/i915/gem/i915_gem_context.h
> index 18060536b0c2..da6e8b506d96 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h
> @@ -155,7 +155,7 @@ i915_gem_context_vm(struct i915_gem_context *ctx)
> }
>
> static inline struct i915_address_space *
> -i915_gem_context_get_vm_rcu(struct i915_gem_context *ctx)
> +i915_gem_context_get_eb_vm(struct i915_gem_context *ctx)
> {
> struct i915_address_space *vm;
>
> diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> index a094f3ce1a90..6c68fe26bb32 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> @@ -1456,7 +1456,7 @@ static int igt_tmpfs_fallback(void *arg)
> struct i915_gem_context *ctx = arg;
> struct drm_i915_private *i915 = ctx->i915;
> struct vfsmount *gemfs = i915->mm.gemfs;
> - struct i915_address_space *vm = i915_gem_context_get_vm_rcu(ctx);
> + struct i915_address_space *vm = i915_gem_context_get_eb_vm(ctx);
> struct drm_i915_gem_object *obj;
> struct i915_vma *vma;
> u32 *vaddr;
> @@ -1512,7 +1512,7 @@ static int igt_shrink_thp(void *arg)
> {
> struct i915_gem_context *ctx = arg;
> struct drm_i915_private *i915 = ctx->i915;
> - struct i915_address_space *vm = i915_gem_context_get_vm_rcu(ctx);
> + struct i915_address_space *vm = i915_gem_context_get_eb_vm(ctx);
Problem here (and probably elsewhere) is that this test does no "eb",
nor even submits any requests for execution.
More so, execbuf path does currently rely on intel_context->vm which is
always set. So I really wonder how it would look, what I touched on
elsewhere in the thread, if we instead made ctx->vm always point to
something. It would align the rules between intel_context and GEM
context and may end up with a more consistent situation.
Regards,
Tvrtko
> struct drm_i915_gem_object *obj;
> struct i915_gem_engines_iter it;
> struct intel_context *ce;
> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> index 4d2758718d21..fc7fb33a3a52 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> @@ -1528,7 +1528,7 @@ static int write_to_scratch(struct i915_gem_context *ctx,
>
> intel_gt_chipset_flush(engine->gt);
>
> - vm = i915_gem_context_get_vm_rcu(ctx);
> + vm = i915_gem_context_get_eb_vm(ctx);
> vma = i915_vma_instance(obj, vm, NULL);
> if (IS_ERR(vma)) {
> err = PTR_ERR(vma);
> @@ -1607,7 +1607,7 @@ static int read_from_scratch(struct i915_gem_context *ctx,
> if (GRAPHICS_VER(i915) >= 8) {
> const u32 GPR0 = engine->mmio_base + 0x600;
>
> - vm = i915_gem_context_get_vm_rcu(ctx);
> + vm = i915_gem_context_get_eb_vm(ctx);
> vma = i915_vma_instance(obj, vm, NULL);
> if (IS_ERR(vma)) {
> err = PTR_ERR(vma);
> diff --git a/drivers/gpu/drm/i915/gt/selftest_execlists.c b/drivers/gpu/drm/i915/gt/selftest_execlists.c
> index f12ffe797639..b3863abc51f5 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_execlists.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_execlists.c
> @@ -3493,7 +3493,7 @@ static int smoke_submit(struct preempt_smoke *smoke,
> if (batch) {
> struct i915_address_space *vm;
>
> - vm = i915_gem_context_get_vm_rcu(ctx);
> + vm = i915_gem_context_get_eb_vm(ctx);
> vma = i915_vma_instance(batch, vm, NULL);
> i915_vm_put(vm);
> if (IS_ERR(vma))
> diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
> index 2c1ed32ca5ac..8be23e0f9306 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
> @@ -117,7 +117,7 @@ static struct i915_request *
> hang_create_request(struct hang *h, struct intel_engine_cs *engine)
> {
> struct intel_gt *gt = h->gt;
> - struct i915_address_space *vm = i915_gem_context_get_vm_rcu(h->ctx);
> + struct i915_address_space *vm = i915_gem_context_get_eb_vm(h->ctx);
> struct drm_i915_gem_object *obj;
> struct i915_request *rq = NULL;
> struct i915_vma *hws, *vma;
> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
> index f843a5040706..2d60a5a5b065 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
> @@ -1300,7 +1300,7 @@ static int exercise_mock(struct drm_i915_private *i915,
> if (!ctx)
> return -ENOMEM;
>
> - vm = i915_gem_context_get_vm_rcu(ctx);
> + vm = i915_gem_context_get_eb_vm(ctx);
> err = func(vm, 0, min(vm->total, limit), end_time);
> i915_vm_put(vm);
>
> @@ -1848,7 +1848,7 @@ static int igt_cs_tlb(void *arg)
> goto out_unlock;
> }
>
> - vm = i915_gem_context_get_vm_rcu(ctx);
> + vm = i915_gem_context_get_eb_vm(ctx);
> if (i915_is_ggtt(vm))
> goto out_vm;
>
> diff --git a/drivers/gpu/drm/i915/selftests/i915_vma.c b/drivers/gpu/drm/i915/selftests/i915_vma.c
> index dd0607254a95..79ba72da0813 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_vma.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_vma.c
> @@ -118,7 +118,7 @@ static int create_vmas(struct drm_i915_private *i915,
> struct i915_vma *vma;
> int err;
>
> - vm = i915_gem_context_get_vm_rcu(ctx);
> + vm = i915_gem_context_get_eb_vm(ctx);
> vma = checked_vma_instance(obj, vm, NULL);
> i915_vm_put(vm);
> if (IS_ERR(vma))
>
next prev parent reply other threads:[~2021-09-03 8:05 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-02 14:20 [PATCH 01/11] drm/i915: Release i915_gem_context from a worker Daniel Vetter
2021-09-02 14:20 ` [Intel-gfx] " Daniel Vetter
2021-09-02 14:20 ` [PATCH 02/11] drm/i915: Release ctx->syncobj on final put, not on ctx close Daniel Vetter
2021-09-02 14:20 ` [Intel-gfx] " Daniel Vetter
2021-09-02 14:20 ` [PATCH 03/11] drm/i915: Keep gem ctx->vm alive until the final put Daniel Vetter
2021-09-02 14:20 ` [Intel-gfx] " Daniel Vetter
2021-09-02 14:20 ` [PATCH 04/11] drm/i915: Drop code to handle set-vm races from execbuf Daniel Vetter
2021-09-02 14:20 ` [Intel-gfx] " Daniel Vetter
2021-09-02 14:20 ` [PATCH 05/11] drm/i915: Rename i915_gem_context_get_vm_rcu to i915_gem_context_get_eb_vm Daniel Vetter
2021-09-02 14:20 ` [Intel-gfx] " Daniel Vetter
2021-09-03 8:05 ` Tvrtko Ursulin [this message]
2021-09-06 8:51 ` Daniel Vetter
2021-09-07 14:48 ` Tvrtko Ursulin
2021-09-02 14:20 ` [PATCH 06/11] drm/i915: Use i915_gem_context_get_eb_vm in ctx_getparam Daniel Vetter
2021-09-02 14:20 ` [Intel-gfx] " Daniel Vetter
2021-09-02 14:20 ` [PATCH 07/11] drm/i915: Add i915_gem_context_is_full_ppgtt Daniel Vetter
2021-09-02 14:20 ` [Intel-gfx] " Daniel Vetter
2021-09-02 14:54 ` Tvrtko Ursulin
2021-09-02 15:22 ` Daniel Vetter
2021-09-02 16:41 ` Tvrtko Ursulin
2021-09-02 17:05 ` Bloomfield, Jon
2021-09-02 17:05 ` Bloomfield, Jon
2021-09-02 19:49 ` Daniel Vetter
2021-09-02 14:20 ` [PATCH 08/11] drm/i915: Use i915_gem_context_get_eb_vm in intel_context_set_gem Daniel Vetter
2021-09-02 14:20 ` [Intel-gfx] " Daniel Vetter
2021-09-02 14:20 ` [PATCH 09/11] drm/i915: Drop __rcu from gem_context->vm Daniel Vetter
2021-09-02 14:20 ` [Intel-gfx] " Daniel Vetter
2021-09-02 14:20 ` [PATCH 10/11] drm/i915: use xa_lock/unlock for fpriv->vm_xa lookups Daniel Vetter
2021-09-02 14:20 ` [Intel-gfx] " Daniel Vetter
2021-09-02 14:20 ` [PATCH 11/11] drm/i915: Stop rcu support for i915_address_space Daniel Vetter
2021-09-02 14:20 ` [Intel-gfx] " Daniel Vetter
2021-09-02 16:49 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [01/11] drm/i915: Release i915_gem_context from a worker Patchwork
2021-09-02 16:50 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2021-09-02 17:20 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
2021-09-02 18:24 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [01/11] drm/i915: Release i915_gem_context from a worker (rev2) Patchwork
2021-09-02 18:26 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2021-09-02 18:55 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
2021-09-03 6:20 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [01/11] drm/i915: Release i915_gem_context from a worker (rev3) Patchwork
2021-09-03 6:22 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2021-09-03 6:49 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2021-09-03 8:03 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
-- strict thread matches above, loose matches on Subject: below --
2021-08-13 20:30 [PATCH 01/11] drm/i915: Release i915_gem_context from a worker Daniel Vetter
2021-08-13 20:30 ` [Intel-gfx] [PATCH 05/11] drm/i915: Rename i915_gem_context_get_vm_rcu to i915_gem_context_get_eb_vm Daniel Vetter
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=2a1f1395-2fe3-3c3d-48a6-bf741f910023@linux.intel.com \
--to=tvrtko.ursulin@linux.intel.com \
--cc=airlied@redhat.com \
--cc=chris@chris-wilson.co.uk \
--cc=daniel.vetter@ffwll.ch \
--cc=daniel.vetter@intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jason@jlekstrand.net \
--cc=jon.bloomfield@intel.com \
--cc=joonas.lahtinen@linux.intel.com \
--cc=lionel.g.landwerlin@intel.com \
--cc=maarten.lankhorst@linux.intel.com \
--cc=matthew.auld@intel.com \
--cc=thomas.hellstrom@linux.intel.com \
/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: link
Be 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.