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: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-02 14:20 [Intel-gfx] [PATCH 01/11] drm/i915: Release i915_gem_context from a worker Daniel Vetter
2021-09-02 14:20 ` [Intel-gfx] [PATCH 02/11] drm/i915: Release ctx->syncobj on final put, not on ctx close Daniel Vetter
2021-09-02 14:20 ` [Intel-gfx] [PATCH 03/11] drm/i915: Keep gem ctx->vm alive until the final put Daniel Vetter
2021-09-02 14:20 ` [Intel-gfx] [PATCH 04/11] drm/i915: Drop code to handle set-vm races from execbuf Daniel Vetter
2021-09-02 14:20 ` [Intel-gfx] [PATCH 05/11] drm/i915: Rename i915_gem_context_get_vm_rcu to i915_gem_context_get_eb_vm 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 ` [Intel-gfx] [PATCH 06/11] drm/i915: Use i915_gem_context_get_eb_vm in ctx_getparam Daniel Vetter
2021-09-02 14:20 ` [Intel-gfx] [PATCH 07/11] drm/i915: Add i915_gem_context_is_full_ppgtt 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 19:49 ` Daniel Vetter
2021-09-02 14:20 ` [Intel-gfx] [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] [PATCH 09/11] drm/i915: Drop __rcu from gem_context->vm Daniel Vetter
2021-09-02 14:20 ` [Intel-gfx] [PATCH 10/11] drm/i915: use xa_lock/unlock for fpriv->vm_xa lookups Daniel Vetter
2021-09-02 14:20 ` [Intel-gfx] [PATCH 11/11] drm/i915: Stop rcu support for i915_address_space 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 [Intel-gfx] [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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox