[Intel-gfx] [PATCH] drm/i915/gem: Ignore readonly failures when updating relocs

[Intel-gfx] [PATCH] drm/i915/gem: Ignore readonly failures when updating relocs

[Chris Wilson]

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(-)

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);
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/gem: Ignore readonly failures when updating relocs

[Andi Shyti]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/gem: Ignore readonly failures when updating relocs

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/gem: Ignore readonly failures when updating relocs

[Patchwork]

== Series Details ==

Series: drm/i915/gem: Ignore readonly failures when updating relocs
URL   : https://patchwork.freedesktop.org/series/75331/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
6b39b83d1e26 drm/i915/gem: Ignore readonly failures when updating relocs
-:7: WARNING:TYPO_SPELLING: 'commited' may be misspelled - perhaps 'committed'?
#7: 
already commited to modifying the execobjects, or have indeed done so

-:13: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#13: 
References: fddcd00a49e9 ("drm/i915: Force the slow path after a user-write error")

-:13: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit fddcd00a49e9 ("drm/i915: Force the slow path after a user-write error")'
#13: 
References: fddcd00a49e9 ("drm/i915: Force the slow path after a user-write error")

-:32: CHECK:SPACING: spaces preferred around that '-' (ctx:VxV)
#32: FILE: drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1544:
+					   &urelocs[r-stack].presumed_offset);
 					             ^

total: 1 errors, 2 warnings, 1 checks, 12 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/gem: Ignore readonly failures when updating relocs

[Patchwork]

== Series Details ==

Series: drm/i915/gem: Ignore readonly failures when updating relocs
URL   : https://patchwork.freedesktop.org/series/75331/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_8229 -> Patchwork_17161
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_17161 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_17161, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17161/index.html

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_17161:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_wait@basic-busy-all:
    - fi-gdg-551:         [PASS][1] -> [FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8229/fi-gdg-551/igt@gem_wait@basic-busy-all.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17161/fi-gdg-551/igt@gem_wait@basic-busy-all.html

  


Participating hosts (47 -> 35)
------------------------------

  Additional (1): fi-skl-lmem 
  Missing    (13): fi-bdw-5557u fi-bsw-n3050 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-bwr-2160 fi-snb-2520m fi-ctg-p8600 fi-ivb-3770 fi-bdw-samus fi-byt-clapper fi-skl-6600u fi-snb-2600 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_8229 -> Patchwork_17161

  CI-20190529: 20190529
  CI_DRM_8229: 9ee7ea928ee3d5f76d5ca40d025d9c9b39a7f0fb @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5550: 98927dfde17aecaecfe67bb9853ceca326ca2b23 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_17161: 6b39b83d1e269d56fda0e07adacc0508e64c8c9e @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

6b39b83d1e26 drm/i915/gem: Ignore readonly failures when updating relocs

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17161/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx