Revert "drm/radeon: Try evicting from CPU accessible to inaccessible VRAM first"

Submitted by Zachary Michaels on March 22, 2017, 6:19 p.m.

Details

Message ID 1490206797-15653-1-git-send-email-zmichaels@oblong.com
State New
Headers show
Series "Revert "drm/radeon: Try evicting from CPU accessible to inaccessible VRAM first"" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Zachary Michaels March 22, 2017, 6:19 p.m.
We were experiencing an infinite loop due to VRAM bos getting added back
to the VRAM lru on eviction via ttm_bo_mem_force_space, and reverting
this commit solves the problem.

Signed-off-by: Zachary Michaels <zmichaels@oblong.com>
Signed-off-by: Julien Isorce <jisorce@oblong.com>
---
 drivers/gpu/drm/radeon/radeon_ttm.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index 0cf03ccbf0a7..d50777f1b48e 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -198,30 +198,7 @@  static void radeon_evict_flags(struct ttm_buffer_object *bo,
 	case TTM_PL_VRAM:
 		if (rbo->rdev->ring[radeon_copy_ring_index(rbo->rdev)].ready == false)
 			radeon_ttm_placement_from_domain(rbo, RADEON_GEM_DOMAIN_CPU);
-		else if (rbo->rdev->mc.visible_vram_size < rbo->rdev->mc.real_vram_size &&
-			 bo->mem.start < (rbo->rdev->mc.visible_vram_size >> PAGE_SHIFT)) {
-			unsigned fpfn = rbo->rdev->mc.visible_vram_size >> PAGE_SHIFT;
-			int i;
-
-			/* Try evicting to the CPU inaccessible part of VRAM
-			 * first, but only set GTT as busy placement, so this
-			 * BO will be evicted to GTT rather than causing other
-			 * BOs to be evicted from VRAM
-			 */
-			radeon_ttm_placement_from_domain(rbo, RADEON_GEM_DOMAIN_VRAM |
-							 RADEON_GEM_DOMAIN_GTT);
-			rbo->placement.num_busy_placement = 0;
-			for (i = 0; i < rbo->placement.num_placement; i++) {
-				if (rbo->placements[i].flags & TTM_PL_FLAG_VRAM) {
-					if (rbo->placements[0].fpfn < fpfn)
-						rbo->placements[0].fpfn = fpfn;
-				} else {
-					rbo->placement.busy_placement =
-						&rbo->placements[i];
-					rbo->placement.num_busy_placement = 1;
-				}
-			}
-		} else
+		else
 			radeon_ttm_placement_from_domain(rbo, RADEON_GEM_DOMAIN_GTT);
 		break;
 	case TTM_PL_TT:

Comments

On 23/03/17 03:19 AM, Zachary Michaels wrote:
> We were experiencing an infinite loop due to VRAM bos getting added back
> to the VRAM lru on eviction via ttm_bo_mem_force_space,

Can you share more details about what happened? I can imagine that
moving a BO from CPU visible to CPU invisible VRAM would put it back on
the LRU, but next time around it shouldn't hit this code anymore but get
evicted to GTT directly.

Was userspace maybe performing concurrent CPU access to the BOs in question?


> and reverting this commit solves the problem.

I hope we can find a better solution.
Hi Michel,

When it happens, the main thread of our gl based app is stuck on a
ioctl(RADEON_CS). I set RADEON_THREAD=false to ease the debugging but same
thing happens if true. Other threads are only si_shader:0,1,2,3 and are
doing nothing, just waiting for jobs. I can also do sudo gdb -p $(pidof
Xorg) to block the X11 server, to make sure there is no ping pong between 2
processes. All other processes are not loading dri/radeonsi_dri.so . And
adding a few traces shows that the above ioctl call is looping for ever on
https://github.com/torvalds/linux/blob/master/drivers/gpu/
drm/ttm/ttm_bo.c#L819 and comes from mesa
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/winsys/radeon/drm/radeon_drm_cs.c#n454
.

After adding even more traces I can see that the bo, which is being
indefinitely evicted, has the flag RADEON_GEM_NO_CPU_ACCESS.
And it gets 3 potential placements after calling "radeon_evict_flags".
 1: VRAM cpu inaccessible, fpfn is 65536
 2: VRAM cpu accessible, fpfn is 0
 3: GTT, fpfn is 0

And it looks like it continuously succeeds to move on the second placement.
So I might be wrong but it looks it is not even a ping pong between VRAM
accessible / not accessible, it just keeps being blited in the CPU
accessible part of the VRAM.

Maybe radeon_evict_flags should just not add the second placement if its
current placement is already VRAM cpu accessible.
Or could be a bug in the get_node that should not succeed in that case.

Note that this happens when the VRAM is nearly full.

FWIW I noticed that amdgpu is doing something different:
https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c#L205
vs
https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/radeon/radeon_ttm.c#L198


Finally the NMI watchdog and the kernel soft lockup and hard lockup
detectors do not detect this looping in that ioctl(RADEON_CS). Maybe
because it estimates it is doing real work. Same for radeon_lockup_timeout,
it does not detect it.

The gpu is a FirePro W600 Cape Verde 2048M.

Thx
Julien

On Thu, Mar 23, 2017 at 8:10 AM, Michel Dänzer <michel@daenzer.net> wrote:

> On 23/03/17 03:19 AM, Zachary Michaels wrote:
> > We were experiencing an infinite loop due to VRAM bos getting added back
> > to the VRAM lru on eviction via ttm_bo_mem_force_space,
>
> Can you share more details about what happened? I can imagine that
> moving a BO from CPU visible to CPU invisible VRAM would put it back on
> the LRU, but next time around it shouldn't hit this code anymore but get
> evicted to GTT directly.
>
> Was userspace maybe performing concurrent CPU access to the BOs in
> question?
>
>
> > and reverting this commit solves the problem.
>
> I hope we can find a better solution.
>
>
> --
> Earthling Michel Dänzer               |               http://www.amd.com
> Libre software enthusiast             |             Mesa and X developer
>
>
>
> Was userspace maybe performing concurrent CPU access to the BOs in
> question?


As far as I know Julien has demonstrated that this is not the case.


> I hope we can find a better solution.


Understood -- I thought you might not want to take this patch, but I went
ahead and sent it out because Christian requested it, and it seems like he
doesn't think VRAM bos should ever evict back to VRAM at all?

Is my understanding of the original commit correct in that it tries to
rewrite the eviction placements of CPU accessible bos so that they are
either size zero (fpfn and lpfn = start of inaccessible VRAM) or they are
in inaccessible VRAM (fpfn = start of inaccessible VRAM and lpfn = 0)?

In this case, to me it seems that the simplest fix would be to iterate
using i to rewrite all the VRAM placements instead of just the first one
(rbo->placements[i] instead of rbo->placements[0]). In the case where
RADEON_GEM_NO_CPU_ACCESS
is set, the second placement will be in CPU accessible VRAM, and that
doesn't seem correct to me as there is no longer any sort of ordering for
evictions. (Unfortunately I'm not currently in a position to test whether
this fixes our issue.) Sorry, I meant to make a note of this originally.

Also, I don't claim to understand this code well enough, but I wonder: if
these sorts of evictions are desirable, would it make more sense to treat
CPU inaccessible/accessible VRAM as distinct entities with their own lrus?

I should also note that we are experiencing another issue where the kernel
locks up in similar circumstances. As Julien noted, we get no output, and
the watchdogs don't seem to work. It may be the case that Xorg and our
process are calling ttm_bo_mem_force_space concurrently, but I don't think
we have enough information yet to say for sure. Reverting this commit does
not fix that issue. I have some small amount of evidence indicating that
bos flagged for CPU access are getting placed in CPU inaccessible memory.
Could that cause this sort of kernel lockup?

Thanks for your help.
> Understood -- I thought you might not want to take this patch, but I 
> went ahead and sent it out because Christian requested it, and it 
> seems like he doesn't think VRAM bos should ever evict back to VRAM at 
> all?
No, I've requested reverting the patch for now because it causes an 
obviously and rather severe problem. If you guys can quickly find how to 
fix it feel free to use that instead.

> Is my understanding of the original commit correct in that it tries to 
> rewrite the eviction placements of CPU accessible bos so that they are 
> either size zero (fpfn and lpfn = start of inaccessible VRAM) or they 
> are in inaccessible VRAM (fpfn = start of inaccessible VRAM and lpfn = 0)?
That for example could work as well, but see below.

> if these sorts of evictions are desirable, would it make more sense to 
> treat CPU inaccessible/accessible VRAM as distinct entities with their 
> own lrus?
Actually I'm pretty sure that it isn't desirable. See the evict function 
doesn't know if we try to evict BOs because we need CPU accessible VRAM 
or if we just run out of VRAM.

This code only makes sense when we need to move different BOs into the 
CPU accessible part round robin because they are accessed by the CPU, 
but then it is actually better to move them to GTT sooner or later.

Regards,
Christian.

Am 23.03.2017 um 16:31 schrieb Zachary Michaels:
>
>     Was userspace maybe performing concurrent CPU access to the BOs in
>     question?
>
>
> As far as I know Julien has demonstrated that this is not the case.
>
>     I hope we can find a better solution.
>
>
> Understood -- I thought you might not want to take this patch, but I 
> went ahead and sent it out because Christian requested it, and it 
> seems like he doesn't think VRAM bos should ever evict back to VRAM at 
> all?
>
> Is my understanding of the original commit correct in that it tries to 
> rewrite the eviction placements of CPU accessible bos so that they are 
> either size zero (fpfn and lpfn = start of inaccessible VRAM) or they 
> are in inaccessible VRAM (fpfn = start of inaccessible VRAM and lpfn = 0)?
>
> In this case, to me it seems that the simplest fix would be to iterate 
> using i to rewrite all the VRAM placements instead of just the first 
> one (rbo->placements[i] instead of rbo->placements[0]). In the case 
> where RADEON_GEM_NO_CPU_ACCESS is set, the second placement will be in 
> CPU accessible VRAM, and that doesn't seem correct to me as there is 
> no longer any sort of ordering for evictions. (Unfortunately I'm not 
> currently in a position to test whether this fixes our issue.) Sorry, 
> I meant to make a note of this originally.
>
> Also, I don't claim to understand this code well enough, but I wonder: 
> if these sorts of evictions are desirable, would it make more sense to 
> treat CPU inaccessible/accessible VRAM as distinct entities with their 
> own lrus?
>
> I should also note that we are experiencing another issue where the 
> kernel locks up in similar circumstances. As Julien noted, we get no 
> output, and the watchdogs don't seem to work. It may be the case that 
> Xorg and our process are calling ttm_bo_mem_force_space concurrently, 
> but I don't think we have enough information yet to say for 
> sure. Reverting this commit does not fix that issue. I have some small 
> amount of evidence indicating that bos flagged for CPU access are 
> getting placed in CPU inaccessible memory. Could that cause this sort 
> of kernel lockup?
>
> Thanks for your help.
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
> No, I've requested reverting the patch for now because it causes an
> obviously and rather severe problem. If you guys can quickly find how to
> fix it feel free to use that instead.
>
> My mistake! That makes sense. Thanks again.