| Message ID | 20210526163730.3423181-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Disable gpu relocations | expand |
On Thu, 27 May 2021 at 02:37, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > Media userspace was the last userspace to still use them, and they > converted now too: > > https://github.com/intel/media-driver/commit/144020c37770083974bedf59902b70b8f444c799 > > This means no reason anymore to make relocations faster than they've > been for the first 9 years of gem. This code was added in > > commit 7dd4f6729f9243bd7046c6f04c107a456bda38eb > Author: Chris Wilson <chris@chris-wilson.co.uk> > Date: Fri Jun 16 15:05:24 2017 +0100 > > drm/i915: Async GPU relocation processing > > Furthermore there's pretty strong indications it's buggy, since the > code to use it by default as the only option had to be reverted: > > commit ad5d95e4d538737ed3fa25493777decf264a3011 > Author: Dave Airlie <airlied@redhat.com> > Date: Tue Sep 8 15:41:17 2020 +1000 > > Revert "drm/i915/gem: Async GPU relocations only" > > This code just disables gpu relocations, leaving the garbage > collection for later patches and more importantly, much less confusing > diff. Also given how much headaches this code has caused in the past, > letting this soak for a bit seems justified. > > Cc: Jon Bloomfield <jon.bloomfield@intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@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> Acked-by: Dave Airlie <airlied@redhat.com> Thanks for making this happen, hope the softpin world is a happier future. Dave.
Op 2021-05-26 om 18:37 schreef Daniel Vetter: > Media userspace was the last userspace to still use them, and they > converted now too: > > https://github.com/intel/media-driver/commit/144020c37770083974bedf59902b70b8f444c799 > > This means no reason anymore to make relocations faster than they've > been for the first 9 years of gem. This code was added in > > commit 7dd4f6729f9243bd7046c6f04c107a456bda38eb > Author: Chris Wilson <chris@chris-wilson.co.uk> > Date: Fri Jun 16 15:05:24 2017 +0100 > > drm/i915: Async GPU relocation processing > > Furthermore there's pretty strong indications it's buggy, since the > code to use it by default as the only option had to be reverted: > > commit ad5d95e4d538737ed3fa25493777decf264a3011 > Author: Dave Airlie <airlied@redhat.com> > Date: Tue Sep 8 15:41:17 2020 +1000 > > Revert "drm/i915/gem: Async GPU relocations only" > > This code just disables gpu relocations, leaving the garbage > collection for later patches and more importantly, much less confusing > diff. Also given how much headaches this code has caused in the past, > letting this soak for a bit seems justified. > > Cc: Jon Bloomfield <jon.bloomfield@intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@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> > --- > .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 43 ++++++++----------- > 1 file changed, 18 insertions(+), 25 deletions(-) Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Note that a lot of complexity may be removed with gpu relocations gone. Some igt tests might also start to fail, as they expect relocations to complete asynchronously. Is it kept in case we need to revive it? ~Maarten
On Thu, May 27, 2021 at 01:16:13PM +0200, Maarten Lankhorst wrote: > Op 2021-05-26 om 18:37 schreef Daniel Vetter: > > Media userspace was the last userspace to still use them, and they > > converted now too: > > > > https://github.com/intel/media-driver/commit/144020c37770083974bedf59902b70b8f444c799 > > > > This means no reason anymore to make relocations faster than they've > > been for the first 9 years of gem. This code was added in > > > > commit 7dd4f6729f9243bd7046c6f04c107a456bda38eb > > Author: Chris Wilson <chris@chris-wilson.co.uk> > > Date: Fri Jun 16 15:05:24 2017 +0100 > > > > drm/i915: Async GPU relocation processing > > > > Furthermore there's pretty strong indications it's buggy, since the > > code to use it by default as the only option had to be reverted: > > > > commit ad5d95e4d538737ed3fa25493777decf264a3011 > > Author: Dave Airlie <airlied@redhat.com> > > Date: Tue Sep 8 15:41:17 2020 +1000 > > > > Revert "drm/i915/gem: Async GPU relocations only" > > > > This code just disables gpu relocations, leaving the garbage > > collection for later patches and more importantly, much less confusing > > diff. Also given how much headaches this code has caused in the past, > > letting this soak for a bit seems justified. > > > > Cc: Jon Bloomfield <jon.bloomfield@intel.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@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> > > --- > > .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 43 ++++++++----------- > > 1 file changed, 18 insertions(+), 25 deletions(-) > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Note that a lot of complexity may be removed with gpu relocations gone. > Some igt tests might also start to fail, as they expect relocations to > complete asynchronously. Yeah I have the kernel side patch for that, at least in the execbuf code + selftests. For igt I'm wawiting on CI to tell me what I all need to look at and decide what to do with it. > Is it kept in case we need to revive it? I don't want to revive it, but I want to split the huge code changes from the functional changes at least. -Daniel
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 297143511f99..31e904f79d0a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -1571,7 +1571,7 @@ static int __reloc_entry_gpu(struct i915_execbuffer *eb, return true; } -static int reloc_entry_gpu(struct i915_execbuffer *eb, +static int __maybe_unused reloc_entry_gpu(struct i915_execbuffer *eb, struct i915_vma *vma, u64 offset, u64 target_addr) @@ -1593,32 +1593,25 @@ relocate_entry(struct i915_vma *vma, { u64 target_addr = relocation_target(reloc, target); u64 offset = reloc->offset; - int reloc_gpu = reloc_entry_gpu(eb, vma, offset, target_addr); - - if (reloc_gpu < 0) - return reloc_gpu; - - if (!reloc_gpu) { - bool wide = eb->reloc_cache.use_64bit_reloc; - void *vaddr; + bool wide = eb->reloc_cache.use_64bit_reloc; + void *vaddr; repeat: - vaddr = reloc_vaddr(vma->obj, eb, - offset >> PAGE_SHIFT); - if (IS_ERR(vaddr)) - return PTR_ERR(vaddr); - - GEM_BUG_ON(!IS_ALIGNED(offset, sizeof(u32))); - clflush_write32(vaddr + offset_in_page(offset), - lower_32_bits(target_addr), - eb->reloc_cache.vaddr); - - if (wide) { - offset += sizeof(u32); - target_addr >>= 32; - wide = false; - goto repeat; - } + vaddr = reloc_vaddr(vma->obj, eb, + offset >> PAGE_SHIFT); + if (IS_ERR(vaddr)) + return PTR_ERR(vaddr); + + GEM_BUG_ON(!IS_ALIGNED(offset, sizeof(u32))); + clflush_write32(vaddr + offset_in_page(offset), + lower_32_bits(target_addr), + eb->reloc_cache.vaddr); + + if (wide) { + offset += sizeof(u32); + target_addr >>= 32; + wide = false; + goto repeat; } return target->node.start | UPDATE;
Media userspace was the last userspace to still use them, and they converted now too: https://github.com/intel/media-driver/commit/144020c37770083974bedf59902b70b8f444c799 This means no reason anymore to make relocations faster than they've been for the first 9 years of gem. This code was added in commit 7dd4f6729f9243bd7046c6f04c107a456bda38eb Author: Chris Wilson <chris@chris-wilson.co.uk> Date: Fri Jun 16 15:05:24 2017 +0100 drm/i915: Async GPU relocation processing Furthermore there's pretty strong indications it's buggy, since the code to use it by default as the only option had to be reverted: commit ad5d95e4d538737ed3fa25493777decf264a3011 Author: Dave Airlie <airlied@redhat.com> Date: Tue Sep 8 15:41:17 2020 +1000 Revert "drm/i915/gem: Async GPU relocations only" This code just disables gpu relocations, leaving the garbage collection for later patches and more importantly, much less confusing diff. Also given how much headaches this code has caused in the past, letting this soak for a bit seems justified. Cc: Jon Bloomfield <jon.bloomfield@intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@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> --- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 43 ++++++++----------- 1 file changed, 18 insertions(+), 25 deletions(-)