| Message ID | 20191119213032.133992-1-stuart.summers@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gem: Ensure aperture exists before setting domain to GTT | expand |
Quoting Stuart Summers (2019-11-19 21:30:32) > mmap_gtt is already covered by a check for aperture presence. > Also add the case to the gem_set_domain IOCTL to avoid this > path for unsupported platforms. It doesn't harm either, it will just mean in a place where it is neither in the GPU nor in the CPU cache, same as it does today. The additional flushes coming out of a GTT write domain should already be elided. It is used internally to mean precisely that... Userspace should already be weaning itself off set-domain altogether. -Chris
On Tue, 2019-11-19 at 22:08 +0000, Chris Wilson wrote: > Quoting Stuart Summers (2019-11-19 21:30:32) > > mmap_gtt is already covered by a check for aperture presence. > > Also add the case to the gem_set_domain IOCTL to avoid this > > path for unsupported platforms. > > It doesn't harm either, it will just mean in a place where it is > neither > in the GPU nor in the CPU cache, same as it does today. The > additional > flushes coming out of a GTT write domain should already be elided. > > It is used internally to mean precisely that... > > Userspace should already be weaning itself off set-domain altogether. But even with the weaning, shouldn't we have these checks in place for safety? Let's say there's some platform issue with the GTT flushes and moving ahead causes the hardware to get in a bad state. Isn't it better to bail early? Let me know if I'm missing something obvious here though. Thanks, Stuart > -Chris
Quoting Summers, Stuart (2019-11-19 22:58:38) > On Tue, 2019-11-19 at 22:08 +0000, Chris Wilson wrote: > > Quoting Stuart Summers (2019-11-19 21:30:32) > > > mmap_gtt is already covered by a check for aperture presence. > > > Also add the case to the gem_set_domain IOCTL to avoid this > > > path for unsupported platforms. > > > > It doesn't harm either, it will just mean in a place where it is > > neither > > in the GPU nor in the CPU cache, same as it does today. The > > additional > > flushes coming out of a GTT write domain should already be elided. > > > > It is used internally to mean precisely that... > > > > Userspace should already be weaning itself off set-domain altogether. > > But even with the weaning, shouldn't we have these checks in place for > safety? Let's say there's some platform issue with the GTT flushes and > moving ahead causes the hardware to get in a bad state. Isn't it better > to bail early? Let me know if I'm missing something obvious here > though. That we use it internally. It will never be unsafe simply because if the extra flushes don't exist, they don't exist. Just like today! -Chris
On Tue, 2019-11-19 at 22:57 +0000, Patchwork wrote: > == Series Details == > > Series: drm/i915/gem: Ensure aperture exists before setting domain to > GTT > URL : https://patchwork.freedesktop.org/series/69698/ > State : failure > > == Summary == > > CALL scripts/checksyscalls.sh > CALL scripts/atomic/check-atomics.sh > DESCEND objtool > CHK include/generated/compile.h > AR drivers/gpu/drm/i915/built-in.a > CC [M] drivers/gpu/drm/i915/gem/i915_gem_domain.o > drivers/gpu/drm/i915/gem/i915_gem_domain.c: In function > ‘i915_gem_object_set_to_gtt_domain’: > drivers/gpu/drm/i915/gem/i915_gem_domain.c:115:31: error: ‘i915’ > undeclared (first use in this function); did you mean ‘to_i915’? > if (!i915_ggtt_has_aperture(&i915->ggtt)) I swear I had this defined above... obviously not. I'll repost if Chris agrees with the approach here. Thanks, Stuart > ^~~~ > to_i915 > drivers/gpu/drm/i915/gem/i915_gem_domain.c:115:31: note: each > undeclared identifier is reported only once for each function it > appears in > scripts/Makefile.build:265: recipe for target > 'drivers/gpu/drm/i915/gem/i915_gem_domain.o' failed > make[4]: *** [drivers/gpu/drm/i915/gem/i915_gem_domain.o] Error 1 > scripts/Makefile.build:509: recipe for target 'drivers/gpu/drm/i915' > failed > make[3]: *** [drivers/gpu/drm/i915] Error 2 > scripts/Makefile.build:509: recipe for target 'drivers/gpu/drm' > failed > make[2]: *** [drivers/gpu/drm] Error 2 > scripts/Makefile.build:509: recipe for target 'drivers/gpu' failed > make[1]: *** [drivers/gpu] Error 2 > Makefile:1652: recipe for target 'drivers' failed > make: *** [drivers] Error 2 >
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c index 9aebcf263191..cf1e221fe27a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c @@ -112,6 +112,9 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write) assert_object_held(obj); + if (!i915_ggtt_has_aperture(&i915->ggtt)) + return -ENODEV; + ret = i915_gem_object_wait(obj, I915_WAIT_INTERRUPTIBLE | (write ? I915_WAIT_ALL : 0),
mmap_gtt is already covered by a check for aperture presence. Also add the case to the gem_set_domain IOCTL to avoid this path for unsupported platforms. Signed-off-by: Stuart Summers <stuart.summers@intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_domain.c | 3 +++ 1 file changed, 3 insertions(+)