From: Matthew Auld <matthew.auld@intel.com>
To: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>,
intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v3 2/6] drm/i915: Add support for asynchronous moving fence waiting
Date: Mon, 15 Nov 2021 12:36:47 +0000 [thread overview]
Message-ID: <7a20547f-0189-1072-ec90-9fdb9e3f5e04@intel.com> (raw)
In-Reply-To: <20211114111218.623138-3-thomas.hellstrom@linux.intel.com>
On 14/11/2021 11:12, Thomas Hellström wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
> For now, we will only allow async migration when TTM is used,
> so the paths we care about are related to TTM.
>
> The mmap path is handled by having the fence in ttm_bo->moving,
> when pinning, the binding only becomes available after the moving
> fence is signaled, and pinning a cpu map will only work after
> the moving fence signals.
>
> This should close all holes where userspace can read a buffer
> before it's fully migrated.
>
> v2:
> - Fix a couple of SPARSE warnings
> v3:
> - Fix a NULL pointer dereference
>
> Co-developed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_fbdev.c | 7 ++--
> drivers/gpu/drm/i915/display/intel_overlay.c | 2 +-
> drivers/gpu/drm/i915/gem/i915_gem_pages.c | 6 +++
> .../i915/gem/selftests/i915_gem_coherency.c | 4 +-
> .../drm/i915/gem/selftests/i915_gem_mman.c | 22 ++++++-----
> drivers/gpu/drm/i915/i915_vma.c | 39 ++++++++++++++++++-
> drivers/gpu/drm/i915/i915_vma.h | 3 ++
> drivers/gpu/drm/i915/selftests/i915_vma.c | 4 +-
> 8 files changed, 69 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index adc3a81be9f7..5902ad0c2bd8 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -265,11 +265,12 @@ static int intelfb_create(struct drm_fb_helper *helper,
> info->fix.smem_len = vma->node.size;
> }
>
> - vaddr = i915_vma_pin_iomap(vma);
> + vaddr = i915_vma_pin_iomap_unlocked(vma);
> if (IS_ERR(vaddr)) {
> - drm_err(&dev_priv->drm,
> - "Failed to remap framebuffer into virtual memory\n");
> ret = PTR_ERR(vaddr);
> + if (ret != -EINTR && ret != -ERESTARTSYS)
> + drm_err(&dev_priv->drm,
> + "Failed to remap framebuffer into virtual memory\n");
> goto out_unpin;
> }
> info->screen_base = vaddr;
> diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c b/drivers/gpu/drm/i915/display/intel_overlay.c
> index 7e3f5c6ca484..21593f3f2664 100644
> --- a/drivers/gpu/drm/i915/display/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/display/intel_overlay.c
> @@ -1357,7 +1357,7 @@ static int get_registers(struct intel_overlay *overlay, bool use_phys)
> overlay->flip_addr = sg_dma_address(obj->mm.pages->sgl);
> else
> overlay->flip_addr = i915_ggtt_offset(vma);
> - overlay->regs = i915_vma_pin_iomap(vma);
> + overlay->regs = i915_vma_pin_iomap_unlocked(vma);
> i915_vma_unpin(vma);
>
> if (IS_ERR(overlay->regs)) {
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> index c4f684b7cc51..49c6e55c68ce 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> @@ -418,6 +418,12 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj,
> }
>
> if (!ptr) {
> + err = i915_gem_object_wait_moving_fence(obj, true);
> + if (err) {
> + ptr = ERR_PTR(err);
> + goto err_unpin;
> + }
> +
> if (GEM_WARN_ON(type == I915_MAP_WC &&
> !static_cpu_has(X86_FEATURE_PAT)))
> ptr = ERR_PTR(-ENODEV);
> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c
> index 13b088cc787e..067c512961ba 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c
> @@ -101,7 +101,7 @@ static int gtt_set(struct context *ctx, unsigned long offset, u32 v)
>
> intel_gt_pm_get(vma->vm->gt);
>
> - map = i915_vma_pin_iomap(vma);
> + map = i915_vma_pin_iomap_unlocked(vma);
> i915_vma_unpin(vma);
> if (IS_ERR(map)) {
> err = PTR_ERR(map);
> @@ -134,7 +134,7 @@ static int gtt_get(struct context *ctx, unsigned long offset, u32 *v)
>
> intel_gt_pm_get(vma->vm->gt);
>
> - map = i915_vma_pin_iomap(vma);
> + map = i915_vma_pin_iomap_unlocked(vma);
> i915_vma_unpin(vma);
> if (IS_ERR(map)) {
> err = PTR_ERR(map);
> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> index 6d30cdfa80f3..5d54181c2145 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
> @@ -125,12 +125,13 @@ static int check_partial_mapping(struct drm_i915_gem_object *obj,
> n = page - view.partial.offset;
> GEM_BUG_ON(n >= view.partial.size);
>
> - io = i915_vma_pin_iomap(vma);
> + io = i915_vma_pin_iomap_unlocked(vma);
> i915_vma_unpin(vma);
> if (IS_ERR(io)) {
> - pr_err("Failed to iomap partial view: offset=%lu; err=%d\n",
> - page, (int)PTR_ERR(io));
> err = PTR_ERR(io);
> + if (err != -EINTR && err != -ERESTARTSYS)
> + pr_err("Failed to iomap partial view: offset=%lu; err=%d\n",
> + page, err);
> goto out;
> }
>
> @@ -219,12 +220,15 @@ static int check_partial_mappings(struct drm_i915_gem_object *obj,
> n = page - view.partial.offset;
> GEM_BUG_ON(n >= view.partial.size);
>
> - io = i915_vma_pin_iomap(vma);
> + io = i915_vma_pin_iomap_unlocked(vma);
> i915_vma_unpin(vma);
> if (IS_ERR(io)) {
> - pr_err("Failed to iomap partial view: offset=%lu; err=%d\n",
> - page, (int)PTR_ERR(io));
> - return PTR_ERR(io);
> + int err = PTR_ERR(io);
> +
> + if (err != -EINTR && err != -ERESTARTSYS)
> + pr_err("Failed to iomap partial view: offset=%lu; err=%d\n",
> + page, err);
> + return err;
> }
>
> iowrite32(page, io + n * PAGE_SIZE / sizeof(*io));
> @@ -773,7 +777,7 @@ static int gtt_set(struct drm_i915_gem_object *obj)
> return PTR_ERR(vma);
>
> intel_gt_pm_get(vma->vm->gt);
> - map = i915_vma_pin_iomap(vma);
> + map = i915_vma_pin_iomap_unlocked(vma);
> i915_vma_unpin(vma);
> if (IS_ERR(map)) {
> err = PTR_ERR(map);
> @@ -799,7 +803,7 @@ static int gtt_check(struct drm_i915_gem_object *obj)
> return PTR_ERR(vma);
>
> intel_gt_pm_get(vma->vm->gt);
> - map = i915_vma_pin_iomap(vma);
> + map = i915_vma_pin_iomap_unlocked(vma);
> i915_vma_unpin(vma);
> if (IS_ERR(map)) {
> err = PTR_ERR(map);
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 8781c4f61952..069f22b3cd48 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -431,6 +431,13 @@ int i915_vma_bind(struct i915_vma *vma,
> work->pinned = i915_gem_object_get(vma->obj);
> }
> } else {
> + if (vma->obj) {
> + int ret;
> +
> + ret = i915_gem_object_wait_moving_fence(vma->obj, true);
> + if (ret)
> + return ret;
> + }
> vma->ops->bind_vma(vma->vm, NULL, vma, cache_level, bind_flags);
> }
>
> @@ -455,6 +462,10 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma *vma)
>
> ptr = READ_ONCE(vma->iomap);
> if (ptr == NULL) {
> + err = i915_gem_object_wait_moving_fence(vma->obj, true);
> + if (err)
> + goto err;
> +
> /*
> * TODO: consider just using i915_gem_object_pin_map() for lmem
> * instead, which already supports mapping non-contiguous chunks
> @@ -496,6 +507,25 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma *vma)
> return IO_ERR_PTR(err);
> }
>
> +void __iomem *i915_vma_pin_iomap_unlocked(struct i915_vma *vma)
> +{
> + struct i915_gem_ww_ctx ww;
> + void __iomem *map;
> + int err;
> +
> + for_i915_gem_ww(&ww, err, true) {
> + err = i915_gem_object_lock(vma->obj, &ww);
> + if (err)
> + continue;
> +
> + map = i915_vma_pin_iomap(vma);
> + }
> + if (err)
> + map = IO_ERR_PTR(err);
> +
> + return map;
> +}
What is the reason for this change? Is this strictly related to this
series/commit?
> +
> void i915_vma_flush_writes(struct i915_vma *vma)
> {
> if (i915_vma_unset_ggtt_write(vma))
> @@ -870,6 +900,7 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
> u64 size, u64 alignment, u64 flags)
> {
> struct i915_vma_work *work = NULL;
> + struct dma_fence *moving = NULL;
> intel_wakeref_t wakeref = 0;
> unsigned int bound;
> int err;
> @@ -895,7 +926,8 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
> if (flags & PIN_GLOBAL)
> wakeref = intel_runtime_pm_get(&vma->vm->i915->runtime_pm);
>
> - if (flags & vma->vm->bind_async_flags) {
> + moving = vma->obj ? i915_gem_object_get_moving_fence(vma->obj) : NULL;
> + if (flags & vma->vm->bind_async_flags || moving) {
> /* lock VM */
> err = i915_vm_lock_objects(vma->vm, ww);
> if (err)
> @@ -909,6 +941,8 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
>
> work->vm = i915_vm_get(vma->vm);
>
> + dma_fence_work_chain(&work->base, moving);
> +
> /* Allocate enough page directories to used PTE */
> if (vma->vm->allocate_va_range) {
> err = i915_vm_alloc_pt_stash(vma->vm,
> @@ -1013,7 +1047,10 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
> err_rpm:
> if (wakeref)
> intel_runtime_pm_put(&vma->vm->i915->runtime_pm, wakeref);
> + if (moving)
> + dma_fence_put(moving);
> vma_put_pages(vma);
> +
> return err;
> }
>
> diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
> index 648dbe744c96..1812b2904a31 100644
> --- a/drivers/gpu/drm/i915/i915_vma.h
> +++ b/drivers/gpu/drm/i915/i915_vma.h
> @@ -326,6 +326,9 @@ static inline bool i915_node_color_differs(const struct drm_mm_node *node,
> * Returns a valid iomapped pointer or ERR_PTR.
> */
> void __iomem *i915_vma_pin_iomap(struct i915_vma *vma);
> +
> +void __iomem *i915_vma_pin_iomap_unlocked(struct i915_vma *vma);
> +
> #define IO_ERR_PTR(x) ((void __iomem *)ERR_PTR(x))
>
> /**
> diff --git a/drivers/gpu/drm/i915/selftests/i915_vma.c b/drivers/gpu/drm/i915/selftests/i915_vma.c
> index 1f10fe36619b..85f43b209890 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_vma.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_vma.c
> @@ -1005,7 +1005,7 @@ static int igt_vma_remapped_gtt(void *arg)
>
> GEM_BUG_ON(vma->ggtt_view.type != *t);
>
> - map = i915_vma_pin_iomap(vma);
> + map = i915_vma_pin_iomap_unlocked(vma);
> i915_vma_unpin(vma);
> if (IS_ERR(map)) {
> err = PTR_ERR(map);
> @@ -1036,7 +1036,7 @@ static int igt_vma_remapped_gtt(void *arg)
>
> GEM_BUG_ON(vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL);
>
> - map = i915_vma_pin_iomap(vma);
> + map = i915_vma_pin_iomap_unlocked(vma);
> i915_vma_unpin(vma);
> if (IS_ERR(map)) {
> err = PTR_ERR(map);
>
next prev parent reply other threads:[~2021-11-15 12:37 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-14 11:12 [PATCH v3 0/6] drm/i915/ttm: Async migration Thomas Hellström
2021-11-14 11:12 ` [PATCH v3 1/6] drm/i915: Add functions to set/get moving fence Thomas Hellström
2021-11-15 12:39 ` Matthew Auld
2021-11-15 12:44 ` Thomas Hellström
2021-11-14 11:12 ` [PATCH v3 2/6] drm/i915: Add support for asynchronous moving fence waiting Thomas Hellström
2021-11-15 12:36 ` Matthew Auld [this message]
2021-11-15 12:42 ` Thomas Hellström
2021-11-15 13:13 ` Matthew Auld
2021-11-15 13:29 ` Thomas Hellström
2021-11-14 11:12 ` [PATCH v3 3/6] drm/i915/ttm: Move the i915_gem_obj_copy_ttm() function Thomas Hellström
2021-11-15 10:42 ` Matthew Auld
2021-11-14 11:12 ` [PATCH v3 4/6] drm/i915/ttm: Break refcounting loops at device region unref time Thomas Hellström
2021-11-15 10:49 ` Matthew Auld
2021-11-14 11:12 ` [PATCH v3 5/6] drm/i915/ttm: Implement asynchronous TTM moves Thomas Hellström
2021-11-15 17:16 ` Matthew Auld
2021-11-16 7:20 ` Thomas Hellström
2021-11-18 7:13 ` Thomas Hellström
2021-11-14 11:12 ` [PATCH v3 6/6] drm/i915/ttm: Update i915_gem_obj_copy_ttm() to be asynchronous Thomas Hellström
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=7a20547f-0189-1072-ec90-9fdb9e3f5e04@intel.com \
--to=matthew.auld@intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--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