drm/i915/gem: Ensure aperture exists before setting domain to GTT
diff mbox series

Message ID 20191119213032.133992-1-stuart.summers@intel.com
State New
Headers show
Series
  • drm/i915/gem: Ensure aperture exists before setting domain to GTT
Related show

Commit Message

Summers, Stuart Nov. 19, 2019, 9:30 p.m. UTC
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(+)

Comments

Chris Wilson Nov. 19, 2019, 10:08 p.m. UTC | #1
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
Summers, Stuart Nov. 19, 2019, 10:58 p.m. UTC | #2
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
Chris Wilson Nov. 19, 2019, 11:03 p.m. UTC | #3
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
Summers, Stuart Nov. 19, 2019, 11:06 p.m. UTC | #4
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
>

Patch
diff mbox series

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