drm/i915/gem: Ignore readonly failures when updating relocs
diff mbox series

Message ID 20200331162150.3635-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • drm/i915/gem: Ignore readonly failures when updating relocs
Related show

Commit Message

Chris Wilson March 31, 2020, 4:21 p.m. UTC
If the user passes in a readonly reloc[], by the time we notice we have
already commited to modifying the execobjects, or have indeed done so
already. Reporting the failure just compounds the issue as we have no
second pass to fall back to anymore.

Testcase: igt/gem_exec_reloc/readonly
Fixes: 7dc8f1143778 ("drm/i915/gem: Drop relocation slowpath")
References: fddcd00a49e9 ("drm/i915: Force the slow path after a user-write error")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Matthew Auld <matthew.auld@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Andi Shyti March 31, 2020, 9:37 p.m. UTC | #1
Hi Chris,

> If the user passes in a readonly reloc[], by the time we notice we have
> already commited to modifying the execobjects, or have indeed done so
> already. Reporting the failure just compounds the issue as we have no
> second pass to fall back to anymore.

It's also written in the comment, btw.

> Testcase: igt/gem_exec_reloc/readonly

if one day we will change igt, we can't fix this commit.

> Fixes: 7dc8f1143778 ("drm/i915/gem: Drop relocation slowpath")
> References: fddcd00a49e9 ("drm/i915: Force the slow path after a user-write error")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Reviewed-by: Andi Shyti <andi.shyti@intel.com>

Andi
Chris Wilson March 31, 2020, 9:46 p.m. UTC | #2
Quoting Andi Shyti (2020-03-31 22:37:14)
> Hi Chris,
> 
> > If the user passes in a readonly reloc[], by the time we notice we have
> > already commited to modifying the execobjects, or have indeed done so
> > already. Reporting the failure just compounds the issue as we have no
> > second pass to fall back to anymore.
> 
> It's also written in the comment, btw.
> 
> > Testcase: igt/gem_exec_reloc/readonly
> 
> if one day we will change igt, we can't fix this commit.

Heh, I'm old, I've been using this from the inception of igt even though
it's not formalized anywhere. All it is is a clue that there is some
code somewhere that exercises this patch in some way.

And short of including the test code in the patch... Hey wait that's what
selftests are so -- only execbuf2 is not amenable to unit testing atm, or
at least no effect has been spent doing so. A new interface would be
built around testing, but we might still struggle to create valid user
addresses. I think there's a way to do so with set_fs(USER_DS) but I am
not sure. Or we use usermodehelper to wrap the testcode and run it from
userspace but included as part of the driver sources.
-Chris

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index cda35e6dfc44..bc8aa9604787 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -1540,10 +1540,8 @@  static int eb_relocate_vma(struct i915_execbuffer *eb, struct eb_vma *ev)
 				 * can read from this userspace address.
 				 */
 				offset = gen8_canonical_addr(offset & ~UPDATE);
-				if (unlikely(__put_user(offset, &urelocs[r-stack].presumed_offset))) {
-					remain = -EFAULT;
-					goto out;
-				}
+				__put_user(offset,
+					   &urelocs[r-stack].presumed_offset);
 			}
 		} while (r++, --count);
 		urelocs += ARRAY_SIZE(stack);