[PATCH] drm/i915: Wait and retry if there is no space in the aperture mappable area

[PATCH] drm/i915: Wait and retry if there is no space in the aperture mappable area

[Siluvery, Arun]

From: "Siluvery, Arun" <arun.siluvery@intel.com>

When a mapping is requested and if there is no space the mapping fails
and the region is not physically backed. This results in
signal 7(SIGBUS), code 2 (BUS_ADRERR) when it is actually accessed.
This patch handles this error, continues to wait and retries to find space.

Signed-off-by: Siluvery, Arun <arun.siluvery@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e7b39d7..927a27b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3275,6 +3275,10 @@ search_free:
 					       obj->cache_level,
 					       map_and_fenceable,
 					       nonblocking);
+
+		if (ret == -ENOSPC)
+			ret = i915_gem_evict_everything(dev);
+
 		if (ret == 0)
 			goto search_free;
 
-- 
1.8.4

Re: [PATCH] drm/i915: Wait and retry if there is no space in the aperture mappable area

[Chris Wilson]

On Tue, Oct 22, 2013 at 12:04:17PM +0000, Siluvery, Arun wrote:
> From: "Siluvery, Arun" <arun.siluvery@intel.com>
> 
> When a mapping is requested and if there is no space the mapping fails
> and the region is not physically backed. This results in
> signal 7(SIGBUS), code 2 (BUS_ADRERR) when it is actually accessed.
> This patch handles this error, continues to wait and retries to find space.

Eh, no. The line before will remove everything from the aperture that is
unpinned. Throwing an evict_everything in here breaks reservations, so I
think you are just papering over a bug.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Wait and retry if there is no space in the aperture mappable area

[Daniel Vetter]

On Tue, Oct 22, 2013 at 01:15:32PM +0100, Chris Wilson wrote:
> On Tue, Oct 22, 2013 at 12:04:17PM +0000, Siluvery, Arun wrote:
> > From: "Siluvery, Arun" <arun.siluvery@intel.com>
> > 
> > When a mapping is requested and if there is no space the mapping fails
> > and the region is not physically backed. This results in
> > signal 7(SIGBUS), code 2 (BUS_ADRERR) when it is actually accessed.
> > This patch handles this error, continues to wait and retries to find space.
> 
> Eh, no. The line before will remove everything from the aperture that is
> unpinned. Throwing an evict_everything in here breaks reservations, so I
> think you are just papering over a bug.

If we want to fix this for real (i.e. allow userspace to reliably map
stuff, mabye even bigger than the aperture) we need to fall back to
suballocating tile-row aligned strides of the buffer (maybe pick the tile
row multiply in between 1M-2M). Until that's done userspace can't rely on
gtt mmaps relibly working for large buffers. The current heuristics we're
using is half of the mappable space, but that's probably a bit too
optimistic.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Wait and retry if there is no space in the aperture mappable area

[Bloomfield, Jon]

> On Tue, Oct 22, 2013 at 01:15:32PM +0100, Chris Wilson wrote:
> > On Tue, Oct 22, 2013 at 12:04:17PM +0000, Siluvery, Arun wrote:
> > > From: "Siluvery, Arun" <arun.siluvery@intel.com>
> > >
> > > When a mapping is requested and if there is no space the mapping
> > > fails and the region is not physically backed. This results in
> > > signal 7(SIGBUS), code 2 (BUS_ADRERR) when it is actually accessed.
> > > This patch handles this error, continues to wait and retries to find space.
> >
> > Eh, no. The line before will remove everything from the aperture that
> > is unpinned. Throwing an evict_everything in here breaks reservations,
> > so I think you are just papering over a bug.
> 
> If we want to fix this for real (i.e. allow userspace to reliably map stuff,
> mabye even bigger than the aperture) we need to fall back to suballocating
> tile-row aligned strides of the buffer (maybe pick the tile row multiply in
> between 1M-2M). Until that's done userspace can't rely on gtt mmaps relibly
> working for large buffers. The current heuristics we're using is half of the
> mappable space, but that's probably a bit too optimistic.
> -Daniel

Is calling i915_gem_evict_everything() actually dangerous ? Despite its name, it appears to only evict unpinned buffers. Or am I missing something ?

What it does do is update the status of buffers which are no longer in use on the ring by calling i915_gem_retire_requests(). So from what I can tell (from a 10 minute trawl of the code admittedly) all this patch is doing is getting a more up to date view of the GTT demands so that we only fail with ENOSPC if there are no pinned buffers which can now be unpinned.

It doesn't address our underlying issue - userspace should still handle ENOSPC gracefully. However it certainly seems to be improving things considerably, so is beneficial if it really is a safe thing to do.

Jon

---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

Re: [PATCH] drm/i915: Wait and retry if there is no space in the aperture mappable area

[Daniel Vetter]

On Wed, Oct 23, 2013 at 11:44 AM, Bloomfield, Jon
<jon.bloomfield@intel.com> wrote:
> Is calling i915_gem_evict_everything() actually dangerous ? Despite its name, it appears to only evict unpinned buffers. Or am I missing something ?
>
> What it does do is update the status of buffers which are no longer in use on the ring by calling i915_gem_retire_requests(). So from what I can tell (from a 10 minute trawl of the code admittedly) all this patch is doing is getting a more up to date view of the GTT demands so that we only fail with ENOSPC if there are no pinned buffers which can now be unpinned.
>
> It doesn't address our underlying issue - userspace should still handle ENOSPC gracefully. However it certainly seems to be improving things considerably, so is beneficial if it really is a safe thing to do.

We use evict_everything in the execbuf code to defragment the gtt. But
that's only beneficial because execbuffer needs to find space for
multiple buffers at once. For just binding one single buffer the
eviction code should scan all lists. Throwing in an additional
evict_everything on top won't help at all.

Of course if you throw in copious amounts of evict_everything once
you're in a bad spot the defragmentation this causes might help to get
out of that corner. But it really shouldn't help with preventing
SIGBUS in the first place. So I really wonder what exactly's going on
here. Can you perhaps distill a testcase and share it (preferrably as
an i-g-t patch ofc)?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Wait and retry if there is no space in the aperture mappable area

[Chris Wilson]

On Wed, Oct 23, 2013 at 09:44:09AM +0000, Bloomfield, Jon wrote:
> > On Tue, Oct 22, 2013 at 01:15:32PM +0100, Chris Wilson wrote:
> > > On Tue, Oct 22, 2013 at 12:04:17PM +0000, Siluvery, Arun wrote:
> > > > From: "Siluvery, Arun" <arun.siluvery@intel.com>
> > > >
> > > > When a mapping is requested and if there is no space the mapping
> > > > fails and the region is not physically backed. This results in
> > > > signal 7(SIGBUS), code 2 (BUS_ADRERR) when it is actually accessed.
> > > > This patch handles this error, continues to wait and retries to find space.
> > >
> > > Eh, no. The line before will remove everything from the aperture that
> > > is unpinned. Throwing an evict_everything in here breaks reservations,
> > > so I think you are just papering over a bug.
> > 
> > If we want to fix this for real (i.e. allow userspace to reliably map stuff,
> > mabye even bigger than the aperture) we need to fall back to suballocating
> > tile-row aligned strides of the buffer (maybe pick the tile row multiply in
> > between 1M-2M). Until that's done userspace can't rely on gtt mmaps relibly
> > working for large buffers. The current heuristics we're using is half of the
> > mappable space, but that's probably a bit too optimistic.
> > -Daniel
> 
> Is calling i915_gem_evict_everything() actually dangerous ? Despite its name, it appears to only evict unpinned buffers. Or am I missing something ?

It breaks the expectations of the callers, which when they have multiple
buffers to reserve at once have a better strategy than can be performed
individually on a buffer. (That is when evict-everything becomes
useful.)

> What it does do is update the status of buffers which are no longer in use on the ring by calling i915_gem_retire_requests(). So from what I can tell (from a 10 minute trawl of the code admittedly) all this patch is doing is getting a more up to date view of the GTT demands so that we only fail with ENOSPC if there are no pinned buffers which can now be unpinned.

No, evict-something also scans the requests to find the first available
slot large enough to fit the buffer. (Except when evict-something is
instructed not to block for pending retirement.)

There's a bug here if evict-everything finds something that
evict-something doesn't.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Wait and retry if there is no space in the aperture mappable area

[Daniel Vetter]

Picking up this old thread, hopefully I haven't forgotten anyone from
the cc list interested in the previous discussions ...


On Tue, Oct 22, 2013 at 2:04 PM, Siluvery, Arun <arun.siluvery@intel.com> wrote:
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e7b39d7..927a27b 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3275,6 +3275,10 @@ search_free:
>                                                obj->cache_level,
>                                                map_and_fenceable,
>                                                nonblocking);
> +
> +               if (ret == -ENOSPC)
> +                       ret = i915_gem_evict_everything(dev);
> +

Despite my earlier claim this is indeed a real bugfix - compared to
evict_something this will move away any hw context objects. So this
isn't just purely duct-tape.

I think the right approach for this patch would be to create a
i915_gem_evict_unpin_special_objects function or similar which atm
just does the do_switch to the default context. We can then call this
here and in evict_everything.

Can you please look into that and test whether that helps as well as
your current hack?

I've noticed this while trying to create a testcase for the recently
fought do_switch eviction related oops. I just couldn't and that got
me thinking a bit ;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Wait and retry if there is no space in the aperture mappable area

[Daniel Vetter]

On Thu, Dec 5, 2013 at 3:28 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Tue, Oct 22, 2013 at 2:04 PM, Siluvery, Arun <arun.siluvery@intel.com> wrote:
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index e7b39d7..927a27b 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -3275,6 +3275,10 @@ search_free:
>>                                                obj->cache_level,
>>                                                map_and_fenceable,
>>                                                nonblocking);
>> +
>> +               if (ret == -ENOSPC)
>> +                       ret = i915_gem_evict_everything(dev);
>> +
>
> Despite my earlier claim this is indeed a real bugfix - compared to
> evict_something this will move away any hw context objects. So this
> isn't just purely duct-tape.
>
> I think the right approach for this patch would be to create a
> i915_gem_evict_unpin_special_objects function or similar which atm
> just does the do_switch to the default context. We can then call this
> here and in evict_everything.
>
> Can you please look into that and test whether that helps as well as
> your current hack?
>
> I've noticed this while trying to create a testcase for the recently
> fought do_switch eviction related oops. I just couldn't and that got
> me thinking a bit ;-)

Fyi I've just pushed out igt/gem_ctx_exec/eviction which exercises our
current failure to evict the current contxt. It's going through
execbuf though and doesn't test the gtt mmap path. Fixing up that is
imo only worth it as part of the large plan I've outlined somewhere
else in this thread. Atm the test fails, but it should work with
proper context eviction. So this should be useful for a more minimal
testcase.

Since the testcase starts with a too-big execbuf that should also
exercise the endless loop you've outlined in the internal mail, but I
think for safety we want to add one more execbuf call in the eviction
test with a 3rd context and just 1 buffer too much in the reloc list.
That should perfectly reproduce your failure case.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915: When evicting something fails, try unpinning old contexts and framebuffers

[Chris Wilson]

Before declaring that we cannot fit an object into the aperture, we
should first check if we can clear the aperture of pinned objects that
are now idle. These pinned objects include old hardware contexts and old
scanout buffers.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c       |  1 +
 drivers/gpu/drm/i915/i915_gem_evict.c | 11 ++++++++---
 drivers/gpu/drm/i915/intel_display.c  |  8 ++++++++
 drivers/gpu/drm/i915/intel_drv.h      |  1 +
 4 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a99aaff1656a..8756a44774a2 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2921,6 +2921,7 @@ int i915_gpu_idle(struct drm_device *dev)
 			return ret;
 	}
 
+	intel_wait_for_pending_flips(dev);
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index b7376533633d..a66b82dac2f8 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -80,6 +80,7 @@ i915_gem_evict_something(struct drm_device *dev, struct i915_address_space *vm,
 	 */
 
 	INIT_LIST_HEAD(&unwind_list);
+search_again:
 	if (mappable) {
 		BUG_ON(!i915_is_ggtt(vm));
 		drm_mm_init_scan_with_range(&vm->mm, min_size,
@@ -115,10 +116,14 @@ none:
 		list_del_init(&vma->exec_list);
 	}
 
-	/* We expect the caller to unpin, evict all and try again, or give up.
-	 * So calling i915_gem_evict_vm() is unnecessary.
+	/* Can we unpin some objects such as idle hw contents,
+	 * or pending flips?
 	 */
-	return -ENOSPC;
+	ret = nonblocking ? -ENOSPC : i915_gpu_idle(dev);
+	if (ret)
+		return ret;
+
+	goto search_again;
 
 found:
 	/* drm_mm doesn't allow any other other operations while
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fe4c5f055973..10d677a1ae2f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2983,6 +2983,14 @@ static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
 	mutex_unlock(&dev->struct_mutex);
 }
 
+void intel_wait_for_pending_flips(struct drm_device *dev)
+{
+	struct drm_crtc *crtc;
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
+		intel_crtc_wait_for_pending_flips(crtc);
+}
+
 /* Program iCLKIP clock to the desired frequency */
 static void lpt_program_iclkip(struct drm_crtc *crtc)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2b5bcb617908..e8fd10aa07ea 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -648,6 +648,7 @@ enum transcoder intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv,
 					     enum pipe pipe);
 void intel_wait_for_vblank(struct drm_device *dev, int pipe);
 void intel_wait_for_pipe_off(struct drm_device *dev, int pipe);
+void intel_wait_for_pending_flips(struct drm_device *dev);
 int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp);
 void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
 			 struct intel_digital_port *dport);
-- 
1.8.5.1

Re: [PATCH] drm/i915: When evicting something fails, try unpinning old contexts and framebuffers

[Daniel Vetter]

On Thu, Dec 5, 2013 at 4:14 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> @@ -115,10 +116,14 @@ none:
>                 list_del_init(&vma->exec_list);
>         }
>
> -       /* We expect the caller to unpin, evict all and try again, or give up.
> -        * So calling i915_gem_evict_vm() is unnecessary.
> +       /* Can we unpin some objects such as idle hw contents,
> +        * or pending flips?
>          */
> -       return -ENOSPC;
> +       ret = nonblocking ? -ENOSPC : i915_gpu_idle(dev);
> +       if (ret)
> +               return ret;
> +
> +       goto search_again;

We need to make sure that we only try to do this once, for otherwise
we spin forever. Jon Bloomfield just pointed out that issue in an
internal mail with their evict_everything sledgehammer. Also can you
please check whether this fixes the test I've just pushed out? Also
please check that the test exercise the endless looping.

Finally we should also have a testcase for outstanding flips, and I
think it's worth it to split the flip-related changes in gpu_idle out
into a separate patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: When evicting something fails, try unpinning old contexts and framebuffers

[Chris Wilson]

On Thu, Dec 05, 2013 at 04:19:21PM +0100, Daniel Vetter wrote:
> On Thu, Dec 5, 2013 at 4:14 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > @@ -115,10 +116,14 @@ none:
> >                 list_del_init(&vma->exec_list);
> >         }
> >
> > -       /* We expect the caller to unpin, evict all and try again, or give up.
> > -        * So calling i915_gem_evict_vm() is unnecessary.
> > +       /* Can we unpin some objects such as idle hw contents,
> > +        * or pending flips?
> >          */
> > -       return -ENOSPC;
> > +       ret = nonblocking ? -ENOSPC : i915_gpu_idle(dev);
> > +       if (ret)
> > +               return ret;
> > +
> > +       goto search_again;
> 
> We need to make sure that we only try to do this once, for otherwise
> we spin forever. 

For sure. I had intended to do a if (pinned) do_idle(), but lost that
track of thought when adding wait_for_flips. Too overexcited. Back to
tracking damage.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Wait and retry if there is no space in the aperture mappable area

[Daniel Vetter]

Dragging this discussion back onto the mailing list.

The Intel GenX kernel team is massively distributed over mutliple
continents and lots fo different business groups within Intel, and a
few interested parties outside of Intel. We need to work hard and
actively to make sure everyone is in the loop, that interested parties
can jump into a discussion or at least learn a bit by following it.

Which means _please_ _always_ cc mailing list. Either the public
intel-gfx list or when discussing confidential topics (and they can't
be redacted out without causing confusion) the internal
gfx-internal-devel mailing list.

/end of sermon, thanks for your attention

Short recap for newcomers: Jon's group is hitting reliability issues
in stress testing where gtt mmappings fail with SIGBUS. The frequency
sharply increased when they started to support more outputs/sprites
and so squarely points at mappable gtt fragmentation due to pinned
buffers.

On Fri, Nov 1, 2013 at 1:47 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, Nov 01, 2013 at 12:02:07PM +0000, Bloomfield, Jon wrote:
>> > > > That statement is false.
>> > > Fair enough, but why is it false ?
>> >
>> > You often have buffers larger than the mappable aperture. At present, such
>> > mappings fail (they should pruned out earlier, but in the fault they would
>> > give a SIGBUS). That limit is oft requested to be raised by magic.
>> But a mapping like this should fail when the user asks to map the buffer, not when they subsequently access it. It can never succeed, so tell the user immediately.
>>
>> > > Userspace fails when accessing a 90MB mapping.
>> > > Don't understand how a trampoline would simplify this - trampoline as in
>> > vectored jump ?
>> > > Don't understand the 'educate' comment - they are simply mapping a
>> > 90MB buffer and trying to use it. Why should that be slow in the general case
>> > ?
>> >
>> > Failing to map a 90MiB buffer is a kernel bug, pure and simple. Failing to map
>> > a 900MiB buffer is a user education issue. (It could be done, but there are at
>> > least a dozen ways the user could do the operation differently that will be
>> > faster.)
>> Why a kernel bug ? If there are too many surfaces pinned for display (or sufficiently fragmented), is it guaranteed that a 90MB map should still succeed ?
>>
>> > The borderline is whether the user expects a 128MiB mapping to
>> > always work. Worst case scenario (without a
>> > trampoline) is that requires the outputs to be disabled first i.e.
>> > process deadlock which leads to a system deadlock with anything like X.
>> Again, how would a trampoline help ?
>
> All I am stating here is that your idea can^Wwill lead to a system
> deadlock. NAK.

I agree with Chris, implicit waiting for a random pin count to drop is
a deadlock nightmare. We'd need at least an explicti unpin_lru where
we track all soon-to-be-unpinned objects. And then we need to walk
that list in evict_something as a last resort.

Still resolving the locking inversion this causes between gem and kms
code won't be fun at all, and we still haven't gained any solid
guarantees for userspace.The mappable gtt can still be easily
fragmented, it's just a notch less likely to cause problems.

We need a better solution, and we need one with actual guarantees for
userspace to depend upon.

So here's my idea to solve this for real:
- split long-term pinned objects into two classes: Those that might
need to be access through the mappable gtt mmio window (like scanout
buffers) and those that don't (like hw contexts).
- Pin objects that are never accessed through the mappable gtt by the
cpu to the unmappable part of the gtt. This will get rid of all the hw
contexts causing havoc by sitting in the middle of the mappable gtt
due to bad luck.
- Restrict all other pinned objects to [0, mappable_size / 2]. This
will ensure that the range [mappable_size / 2, mappable] can always be
evicted. Add some vicious checks to the code to make sure we never
stumble over a pinned object in there. Also, expose this value to
userspace as the guaranteed upper limit for gtt mmaps.

Unfortunately we can't enforce this when creating the mapping since
old userspace didn't bother with this, and most often (if you never do
a modeset at least) it works _really_ well.

Of course we need solid testcases to make sure we don't break this
again. Luckily the gtt allocater is bottom-up and fully predictable if
nothing else is running (which is the default assumption for i-g-t
tests). So we need the following:
- two objects A1/A2 of size gtt_mappable / 3
- some means to trick the kernel into pinning an object of the class
we want to test. We need 2 such objects B1, B2.

1. fault in object A1 through cpu gtt access.
2. trick kernel into pinning B1.
3. fault in object A2 through cpu gtt access.
4. trick kernel into pinning B2.
5. Try to fault in a new object with gtt cpu access of size mappable/2.

Now we have perfectly fragemented the gtt and there's no room for an
object of size mappable/2. But if we restrict pinned objects it will
work.

Repeat the above test for all classes of pinned objects (hw context,
scanout buffers, cursors, ...).

I think this is conceptually the simplest change that actually gives
us real guarantees. And it doesn't change anything fundamentally with
how gem works (as opposed to the pinned buffer eviction logic) and so
should be much less tricky to implement.

Please poke holes into the plan.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Wait and retry if there is no space in the aperture mappable area

[Bloomfield, Jon]

Thanks Daniel, sermon noted.

I hadn't twigged that we were pinning buffers to the mapable GTT region which didn't really need to be there. Do we definitely never need to modify or interrogate the hw contexts from the CPU ? What about for debug ?

Jon

> -----Original Message-----
> From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> Daniel Vetter
> Sent: Friday, November 01, 2013 4:57 PM
> To: Chris Wilson
> Cc: Bloomfield, Jon; intel-gfx
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Wait and retry if there is no space
> in the aperture mappable area
> 
> Dragging this discussion back onto the mailing list.
> 
> The Intel GenX kernel team is massively distributed over mutliple continents
> and lots fo different business groups within Intel, and a few interested
> parties outside of Intel. We need to work hard and actively to make sure
> everyone is in the loop, that interested parties can jump into a discussion or
> at least learn a bit by following it.
> 
> Which means _please_ _always_ cc mailing list. Either the public intel-gfx list
> or when discussing confidential topics (and they can't be redacted out
> without causing confusion) the internal gfx-internal-devel mailing list.
> 
> /end of sermon, thanks for your attention
> 
> Short recap for newcomers: Jon's group is hitting reliability issues in stress
> testing where gtt mmappings fail with SIGBUS. The frequency sharply
> increased when they started to support more outputs/sprites and so
> squarely points at mappable gtt fragmentation due to pinned buffers.
> 
> On Fri, Nov 1, 2013 at 1:47 PM, Chris Wilson <chris@chris-wilson.co.uk>
> wrote:
> > On Fri, Nov 01, 2013 at 12:02:07PM +0000, Bloomfield, Jon wrote:
> >> > > > That statement is false.
> >> > > Fair enough, but why is it false ?
> >> >
> >> > You often have buffers larger than the mappable aperture. At
> >> > present, such mappings fail (they should pruned out earlier, but in
> >> > the fault they would give a SIGBUS). That limit is oft requested to be
> raised by magic.
> >> But a mapping like this should fail when the user asks to map the buffer,
> not when they subsequently access it. It can never succeed, so tell the user
> immediately.
> >>
> >> > > Userspace fails when accessing a 90MB mapping.
> >> > > Don't understand how a trampoline would simplify this -
> >> > > trampoline as in
> >> > vectored jump ?
> >> > > Don't understand the 'educate' comment - they are simply mapping
> >> > > a
> >> > 90MB buffer and trying to use it. Why should that be slow in the
> >> > general case ?
> >> >
> >> > Failing to map a 90MiB buffer is a kernel bug, pure and simple.
> >> > Failing to map a 900MiB buffer is a user education issue. (It could
> >> > be done, but there are at least a dozen ways the user could do the
> >> > operation differently that will be
> >> > faster.)
> >> Why a kernel bug ? If there are too many surfaces pinned for display (or
> sufficiently fragmented), is it guaranteed that a 90MB map should still
> succeed ?
> >>
> >> > The borderline is whether the user expects a 128MiB mapping to
> >> > always work. Worst case scenario (without a
> >> > trampoline) is that requires the outputs to be disabled first i.e.
> >> > process deadlock which leads to a system deadlock with anything like X.
> >> Again, how would a trampoline help ?
> >
> > All I am stating here is that your idea can^Wwill lead to a system
> > deadlock. NAK.
> 
> I agree with Chris, implicit waiting for a random pin count to drop is a
> deadlock nightmare. We'd need at least an explicti unpin_lru where we track
> all soon-to-be-unpinned objects. And then we need to walk that list in
> evict_something as a last resort.
> 
> Still resolving the locking inversion this causes between gem and kms code
> won't be fun at all, and we still haven't gained any solid guarantees for
> userspace.The mappable gtt can still be easily fragmented, it's just a notch
> less likely to cause problems.
> 
> We need a better solution, and we need one with actual guarantees for
> userspace to depend upon.
> 
> So here's my idea to solve this for real:
> - split long-term pinned objects into two classes: Those that might need to be
> access through the mappable gtt mmio window (like scanout
> buffers) and those that don't (like hw contexts).
> - Pin objects that are never accessed through the mappable gtt by the cpu to
> the unmappable part of the gtt. This will get rid of all the hw contexts causing
> havoc by sitting in the middle of the mappable gtt due to bad luck.
> - Restrict all other pinned objects to [0, mappable_size / 2]. This will ensure
> that the range [mappable_size / 2, mappable] can always be evicted. Add
> some vicious checks to the code to make sure we never stumble over a
> pinned object in there. Also, expose this value to userspace as the
> guaranteed upper limit for gtt mmaps.
> 
> Unfortunately we can't enforce this when creating the mapping since old
> userspace didn't bother with this, and most often (if you never do a modeset
> at least) it works _really_ well.
> 
> Of course we need solid testcases to make sure we don't break this again.
> Luckily the gtt allocater is bottom-up and fully predictable if nothing else is
> running (which is the default assumption for i-g-t tests). So we need the
> following:
> - two objects A1/A2 of size gtt_mappable / 3
> - some means to trick the kernel into pinning an object of the class we want
> to test. We need 2 such objects B1, B2.
> 
> 1. fault in object A1 through cpu gtt access.
> 2. trick kernel into pinning B1.
> 3. fault in object A2 through cpu gtt access.
> 4. trick kernel into pinning B2.
> 5. Try to fault in a new object with gtt cpu access of size mappable/2.
> 
> Now we have perfectly fragemented the gtt and there's no room for an
> object of size mappable/2. But if we restrict pinned objects it will work.
> 
> Repeat the above test for all classes of pinned objects (hw context, scanout
> buffers, cursors, ...).
> 
> I think this is conceptually the simplest change that actually gives us real
> guarantees. And it doesn't change anything fundamentally with how gem
> works (as opposed to the pinned buffer eviction logic) and so should be
> much less tricky to implement.
> 
> Please poke holes into the plan.
> 
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Wait and retry if there is no space in the aperture mappable area

[Daniel Vetter]

On Fri, Nov 1, 2013 at 7:06 PM, Bloomfield, Jon
<jon.bloomfield@intel.com> wrote:
> Thanks Daniel, sermon noted.
>
> I hadn't twigged that we were pinning buffers to the mapable GTT region which didn't really need to be there. Do we definitely never need to modify or interrogate the hw contexts from the CPU ? What about for debug ?

We can always use direct cpu mappings to the backing storage as long
as we remember to do the coherency dances. Originally we've started to
pin all scanout buffers into the mappable part because that's what our
old userspace expects to be possible. So by default we kinda need to
keep that working.

But even for scanout buffers we could add a new flag somewhere so that
userspace can promise to the kernel to never ever to a gtt mmap (and
then the kernel could also reject any such attempts outright). But
that'd be an extension on top if we notice that the 128M of mappable
for pinned buffers isn't enough. I expect this to happen once 4k
screens get too common ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Wait and retry if there is no space in the aperture mappable area

[Daniel Vetter]

On Fri, Nov 1, 2013 at 5:56 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> So here's my idea to solve this for real:
> - split long-term pinned objects into two classes: Those that might
> need to be access through the mappable gtt mmio window (like scanout
> buffers) and those that don't (like hw contexts).
> - Pin objects that are never accessed through the mappable gtt by the
> cpu to the unmappable part of the gtt. This will get rid of all the hw
> contexts causing havoc by sitting in the middle of the mappable gtt
> due to bad luck.
> - Restrict all other pinned objects to [0, mappable_size / 2]. This
> will ensure that the range [mappable_size / 2, mappable] can always be
> evicted. Add some vicious checks to the code to make sure we never
> stumble over a pinned object in there. Also, expose this value to
> userspace as the guaranteed upper limit for gtt mmaps.
>
> Unfortunately we can't enforce this when creating the mapping since
> old userspace didn't bother with this, and most often (if you never do
> a modeset at least) it works _really_ well.
>
> Of course we need solid testcases to make sure we don't break this
> again. Luckily the gtt allocater is bottom-up and fully predictable if
> nothing else is running (which is the default assumption for i-g-t
> tests). So we need the following:
> - two objects A1/A2 of size gtt_mappable / 3
> - some means to trick the kernel into pinning an object of the class
> we want to test. We need 2 such objects B1, B2.
>
> 1. fault in object A1 through cpu gtt access.
> 2. trick kernel into pinning B1.
> 3. fault in object A2 through cpu gtt access.
> 4. trick kernel into pinning B2.
> 5. Try to fault in a new object with gtt cpu access of size mappable/2.
>
> Now we have perfectly fragemented the gtt and there's no room for an
> object of size mappable/2. But if we restrict pinned objects it will
> work.
>
> Repeat the above test for all classes of pinned objects (hw context,
> scanout buffers, cursors, ...).
>
> I think this is conceptually the simplest change that actually gives
> us real guarantees. And it doesn't change anything fundamentally with
> how gem works (as opposed to the pinned buffer eviction logic) and so
> should be much less tricky to implement.
>
> Please poke holes into the plan.

One problem of the above approach is that we the unmappable pinned bo
will sit uncomfortably near to the middle of the gtt. Which could be a
problem on platforms without real ppgtt (currently all, soon only
older stuff) and userspace assuming it can map 1G textures for gpu
access. We probably need to allocate pinned unmappable bos top-down.
Ben's ppgtt patches already has teh infrastructure for that, but that
means we'll have to add a bit more testing for interactions between
pinning ppgtt pdes and hw contexts. Might be that we need a bit more
alignment constraints ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch