Hi Dave,
there still seems to be the need for some further discussion about the SA code, so I again split that out of the patchset and tested the result a bit.
Most of the stuff still works fine without those offending changes, so to avoid mailing around unrelated and already reviewed patches, I request the include the following 17 patches into drm-next.
If you prefer to merge they are also available from git://people.freedesktop.org/~deathsimple/linux branch reset-rework.
Cheers, Christian.
Different rings have different criteria to test if they are stuck.
v2: rebased on current drm-next
Signed-off-by: Christian König deathsimple@vodafone.de Reviewed-by: Alex Deucher alexander.deucher@amd.com Reviewed-by: Jerome Glisse jglisse@redhat.com --- drivers/gpu/drm/radeon/radeon.h | 4 +- drivers/gpu/drm/radeon/radeon_asic.c | 44 ++++++++++++++++++-------------- drivers/gpu/drm/radeon/radeon_fence.c | 2 +- 3 files changed, 28 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index 610acee..4026a4c0 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -1162,7 +1162,6 @@ struct radeon_asic { int (*resume)(struct radeon_device *rdev); int (*suspend)(struct radeon_device *rdev); void (*vga_set_state)(struct radeon_device *rdev, bool state); - bool (*gpu_is_lockup)(struct radeon_device *rdev, struct radeon_ring *cp); int (*asic_reset)(struct radeon_device *rdev); /* ioctl hw specific callback. Some hw might want to perform special * operation on specific ioctl. For instance on wait idle some hw @@ -1191,6 +1190,7 @@ struct radeon_asic { void (*ring_start)(struct radeon_device *rdev, struct radeon_ring *cp); int (*ring_test)(struct radeon_device *rdev, struct radeon_ring *cp); int (*ib_test)(struct radeon_device *rdev, struct radeon_ring *cp); + bool (*is_lockup)(struct radeon_device *rdev, struct radeon_ring *cp); } ring[RADEON_NUM_RINGS]; /* irqs */ struct { @@ -1740,7 +1740,6 @@ void radeon_ring_write(struct radeon_ring *ring, uint32_t v); #define radeon_suspend(rdev) (rdev)->asic->suspend((rdev)) #define radeon_cs_parse(rdev, r, p) (rdev)->asic->ring[(r)].cs_parse((p)) #define radeon_vga_set_state(rdev, state) (rdev)->asic->vga_set_state((rdev), (state)) -#define radeon_gpu_is_lockup(rdev, cp) (rdev)->asic->gpu_is_lockup((rdev), (cp)) #define radeon_asic_reset(rdev) (rdev)->asic->asic_reset((rdev)) #define radeon_gart_tlb_flush(rdev) (rdev)->asic->gart.tlb_flush((rdev)) #define radeon_gart_set_page(rdev, i, p) (rdev)->asic->gart.set_page((rdev), (i), (p)) @@ -1749,6 +1748,7 @@ void radeon_ring_write(struct radeon_ring *ring, uint32_t v); #define radeon_ib_test(rdev, r, cp) (rdev)->asic->ring[(r)].ib_test((rdev), (cp)) #define radeon_ring_ib_execute(rdev, r, ib) (rdev)->asic->ring[(r)].ib_execute((rdev), (ib)) #define radeon_ring_ib_parse(rdev, r, ib) (rdev)->asic->ring[(r)].ib_parse((rdev), (ib)) +#define radeon_ring_is_lockup(rdev, r, cp) (rdev)->asic->ring[(r)].is_lockup((rdev), (cp)) #define radeon_irq_set(rdev) (rdev)->asic->irq.set((rdev)) #define radeon_irq_process(rdev) (rdev)->asic->irq.process((rdev)) #define radeon_get_vblank_counter(rdev, crtc) (rdev)->asic->display.get_vblank_counter((rdev), (crtc)) diff --git a/drivers/gpu/drm/radeon/radeon_asic.c b/drivers/gpu/drm/radeon/radeon_asic.c index be4dc2f..958b9ea 100644 --- a/drivers/gpu/drm/radeon/radeon_asic.c +++ b/drivers/gpu/drm/radeon/radeon_asic.c @@ -134,7 +134,6 @@ static struct radeon_asic r100_asic = { .suspend = &r100_suspend, .resume = &r100_resume, .vga_set_state = &r100_vga_set_state, - .gpu_is_lockup = &r100_gpu_is_lockup, .asic_reset = &r100_asic_reset, .ioctl_wait_idle = NULL, .gui_idle = &r100_gui_idle, @@ -152,6 +151,7 @@ static struct radeon_asic r100_asic = { .ring_start = &r100_ring_start, .ring_test = &r100_ring_test, .ib_test = &r100_ib_test, + .is_lockup = &r100_gpu_is_lockup, } }, .irq = { @@ -208,7 +208,6 @@ static struct radeon_asic r200_asic = { .suspend = &r100_suspend, .resume = &r100_resume, .vga_set_state = &r100_vga_set_state, - .gpu_is_lockup = &r100_gpu_is_lockup, .asic_reset = &r100_asic_reset, .ioctl_wait_idle = NULL, .gui_idle = &r100_gui_idle, @@ -226,6 +225,7 @@ static struct radeon_asic r200_asic = { .ring_start = &r100_ring_start, .ring_test = &r100_ring_test, .ib_test = &r100_ib_test, + .is_lockup = &r100_gpu_is_lockup, } }, .irq = { @@ -282,7 +282,6 @@ static struct radeon_asic r300_asic = { .suspend = &r300_suspend, .resume = &r300_resume, .vga_set_state = &r100_vga_set_state, - .gpu_is_lockup = &r300_gpu_is_lockup, .asic_reset = &r300_asic_reset, .ioctl_wait_idle = NULL, .gui_idle = &r100_gui_idle, @@ -300,6 +299,7 @@ static struct radeon_asic r300_asic = { .ring_start = &r300_ring_start, .ring_test = &r100_ring_test, .ib_test = &r100_ib_test, + .is_lockup = &r300_gpu_is_lockup, } }, .irq = { @@ -356,7 +356,6 @@ static struct radeon_asic r300_asic_pcie = { .suspend = &r300_suspend, .resume = &r300_resume, .vga_set_state = &r100_vga_set_state, - .gpu_is_lockup = &r300_gpu_is_lockup, .asic_reset = &r300_asic_reset, .ioctl_wait_idle = NULL, .gui_idle = &r100_gui_idle, @@ -374,6 +373,7 @@ static struct radeon_asic r300_asic_pcie = { .ring_start = &r300_ring_start, .ring_test = &r100_ring_test, .ib_test = &r100_ib_test, + .is_lockup = &r300_gpu_is_lockup, } }, .irq = { @@ -430,7 +430,6 @@ static struct radeon_asic r420_asic = { .suspend = &r420_suspend, .resume = &r420_resume, .vga_set_state = &r100_vga_set_state, - .gpu_is_lockup = &r300_gpu_is_lockup, .asic_reset = &r300_asic_reset, .ioctl_wait_idle = NULL, .gui_idle = &r100_gui_idle, @@ -448,6 +447,7 @@ static struct radeon_asic r420_asic = { .ring_start = &r300_ring_start, .ring_test = &r100_ring_test, .ib_test = &r100_ib_test, + .is_lockup = &r300_gpu_is_lockup, } }, .irq = { @@ -504,7 +504,6 @@ static struct radeon_asic rs400_asic = { .suspend = &rs400_suspend, .resume = &rs400_resume, .vga_set_state = &r100_vga_set_state, - .gpu_is_lockup = &r300_gpu_is_lockup, .asic_reset = &r300_asic_reset, .ioctl_wait_idle = NULL, .gui_idle = &r100_gui_idle, @@ -522,6 +521,7 @@ static struct radeon_asic rs400_asic = { .ring_start = &r300_ring_start, .ring_test = &r100_ring_test, .ib_test = &r100_ib_test, + .is_lockup = &r300_gpu_is_lockup, } }, .irq = { @@ -578,7 +578,6 @@ static struct radeon_asic rs600_asic = { .suspend = &rs600_suspend, .resume = &rs600_resume, .vga_set_state = &r100_vga_set_state, - .gpu_is_lockup = &r300_gpu_is_lockup, .asic_reset = &rs600_asic_reset, .ioctl_wait_idle = NULL, .gui_idle = &r100_gui_idle, @@ -596,6 +595,7 @@ static struct radeon_asic rs600_asic = { .ring_start = &r300_ring_start, .ring_test = &r100_ring_test, .ib_test = &r100_ib_test, + .is_lockup = &r300_gpu_is_lockup, } }, .irq = { @@ -652,7 +652,6 @@ static struct radeon_asic rs690_asic = { .suspend = &rs690_suspend, .resume = &rs690_resume, .vga_set_state = &r100_vga_set_state, - .gpu_is_lockup = &r300_gpu_is_lockup, .asic_reset = &rs600_asic_reset, .ioctl_wait_idle = NULL, .gui_idle = &r100_gui_idle, @@ -670,6 +669,7 @@ static struct radeon_asic rs690_asic = { .ring_start = &r300_ring_start, .ring_test = &r100_ring_test, .ib_test = &r100_ib_test, + .is_lockup = &r300_gpu_is_lockup, } }, .irq = { @@ -726,7 +726,6 @@ static struct radeon_asic rv515_asic = { .suspend = &rv515_suspend, .resume = &rv515_resume, .vga_set_state = &r100_vga_set_state, - .gpu_is_lockup = &r300_gpu_is_lockup, .asic_reset = &rs600_asic_reset, .ioctl_wait_idle = NULL, .gui_idle = &r100_gui_idle, @@ -744,6 +743,7 @@ static struct radeon_asic rv515_asic = { .ring_start = &rv515_ring_start, .ring_test = &r100_ring_test, .ib_test = &r100_ib_test, + .is_lockup = &r300_gpu_is_lockup, } }, .irq = { @@ -800,7 +800,6 @@ static struct radeon_asic r520_asic = { .suspend = &rv515_suspend, .resume = &r520_resume, .vga_set_state = &r100_vga_set_state, - .gpu_is_lockup = &r300_gpu_is_lockup, .asic_reset = &rs600_asic_reset, .ioctl_wait_idle = NULL, .gui_idle = &r100_gui_idle, @@ -818,6 +817,7 @@ static struct radeon_asic r520_asic = { .ring_start = &rv515_ring_start, .ring_test = &r100_ring_test, .ib_test = &r100_ib_test, + .is_lockup = &r300_gpu_is_lockup, } }, .irq = { @@ -874,7 +874,6 @@ static struct radeon_asic r600_asic = { .suspend = &r600_suspend, .resume = &r600_resume, .vga_set_state = &r600_vga_set_state, - .gpu_is_lockup = &r600_gpu_is_lockup, .asic_reset = &r600_asic_reset, .ioctl_wait_idle = r600_ioctl_wait_idle, .gui_idle = &r600_gui_idle, @@ -891,6 +890,7 @@ static struct radeon_asic r600_asic = { .cs_parse = &r600_cs_parse, .ring_test = &r600_ring_test, .ib_test = &r600_ib_test, + .is_lockup = &r600_gpu_is_lockup, } }, .irq = { @@ -946,7 +946,6 @@ static struct radeon_asic rs780_asic = { .fini = &r600_fini, .suspend = &r600_suspend, .resume = &r600_resume, - .gpu_is_lockup = &r600_gpu_is_lockup, .vga_set_state = &r600_vga_set_state, .asic_reset = &r600_asic_reset, .ioctl_wait_idle = r600_ioctl_wait_idle, @@ -964,6 +963,7 @@ static struct radeon_asic rs780_asic = { .cs_parse = &r600_cs_parse, .ring_test = &r600_ring_test, .ib_test = &r600_ib_test, + .is_lockup = &r600_gpu_is_lockup, } }, .irq = { @@ -1020,7 +1020,6 @@ static struct radeon_asic rv770_asic = { .suspend = &rv770_suspend, .resume = &rv770_resume, .asic_reset = &r600_asic_reset, - .gpu_is_lockup = &r600_gpu_is_lockup, .vga_set_state = &r600_vga_set_state, .ioctl_wait_idle = r600_ioctl_wait_idle, .gui_idle = &r600_gui_idle, @@ -1037,6 +1036,7 @@ static struct radeon_asic rv770_asic = { .cs_parse = &r600_cs_parse, .ring_test = &r600_ring_test, .ib_test = &r600_ib_test, + .is_lockup = &r600_gpu_is_lockup, } }, .irq = { @@ -1092,7 +1092,6 @@ static struct radeon_asic evergreen_asic = { .fini = &evergreen_fini, .suspend = &evergreen_suspend, .resume = &evergreen_resume, - .gpu_is_lockup = &evergreen_gpu_is_lockup, .asic_reset = &evergreen_asic_reset, .vga_set_state = &r600_vga_set_state, .ioctl_wait_idle = r600_ioctl_wait_idle, @@ -1110,6 +1109,7 @@ static struct radeon_asic evergreen_asic = { .cs_parse = &evergreen_cs_parse, .ring_test = &r600_ring_test, .ib_test = &r600_ib_test, + .is_lockup = &evergreen_gpu_is_lockup, } }, .irq = { @@ -1165,7 +1165,6 @@ static struct radeon_asic sumo_asic = { .fini = &evergreen_fini, .suspend = &evergreen_suspend, .resume = &evergreen_resume, - .gpu_is_lockup = &evergreen_gpu_is_lockup, .asic_reset = &evergreen_asic_reset, .vga_set_state = &r600_vga_set_state, .ioctl_wait_idle = r600_ioctl_wait_idle, @@ -1183,6 +1182,7 @@ static struct radeon_asic sumo_asic = { .cs_parse = &evergreen_cs_parse, .ring_test = &r600_ring_test, .ib_test = &r600_ib_test, + .is_lockup = &evergreen_gpu_is_lockup, }, }, .irq = { @@ -1238,7 +1238,6 @@ static struct radeon_asic btc_asic = { .fini = &evergreen_fini, .suspend = &evergreen_suspend, .resume = &evergreen_resume, - .gpu_is_lockup = &evergreen_gpu_is_lockup, .asic_reset = &evergreen_asic_reset, .vga_set_state = &r600_vga_set_state, .ioctl_wait_idle = r600_ioctl_wait_idle, @@ -1256,6 +1255,7 @@ static struct radeon_asic btc_asic = { .cs_parse = &evergreen_cs_parse, .ring_test = &r600_ring_test, .ib_test = &r600_ib_test, + .is_lockup = &evergreen_gpu_is_lockup, } }, .irq = { @@ -1321,7 +1321,6 @@ static struct radeon_asic cayman_asic = { .fini = &cayman_fini, .suspend = &cayman_suspend, .resume = &cayman_resume, - .gpu_is_lockup = &cayman_gpu_is_lockup, .asic_reset = &cayman_asic_reset, .vga_set_state = &r600_vga_set_state, .ioctl_wait_idle = r600_ioctl_wait_idle, @@ -1340,6 +1339,7 @@ static struct radeon_asic cayman_asic = { .cs_parse = &evergreen_cs_parse, .ring_test = &r600_ring_test, .ib_test = &r600_ib_test, + .is_lockup = &cayman_gpu_is_lockup, }, [CAYMAN_RING_TYPE_CP1_INDEX] = { .ib_execute = &cayman_ring_ib_execute, @@ -1349,6 +1349,7 @@ static struct radeon_asic cayman_asic = { .cs_parse = &evergreen_cs_parse, .ring_test = &r600_ring_test, .ib_test = &r600_ib_test, + .is_lockup = &cayman_gpu_is_lockup, }, [CAYMAN_RING_TYPE_CP2_INDEX] = { .ib_execute = &cayman_ring_ib_execute, @@ -1358,6 +1359,7 @@ static struct radeon_asic cayman_asic = { .cs_parse = &evergreen_cs_parse, .ring_test = &r600_ring_test, .ib_test = &r600_ib_test, + .is_lockup = &cayman_gpu_is_lockup, } }, .irq = { @@ -1413,7 +1415,6 @@ static struct radeon_asic trinity_asic = { .fini = &cayman_fini, .suspend = &cayman_suspend, .resume = &cayman_resume, - .gpu_is_lockup = &cayman_gpu_is_lockup, .asic_reset = &cayman_asic_reset, .vga_set_state = &r600_vga_set_state, .ioctl_wait_idle = r600_ioctl_wait_idle, @@ -1432,6 +1433,7 @@ static struct radeon_asic trinity_asic = { .cs_parse = &evergreen_cs_parse, .ring_test = &r600_ring_test, .ib_test = &r600_ib_test, + .is_lockup = &cayman_gpu_is_lockup, }, [CAYMAN_RING_TYPE_CP1_INDEX] = { .ib_execute = &cayman_ring_ib_execute, @@ -1441,6 +1443,7 @@ static struct radeon_asic trinity_asic = { .cs_parse = &evergreen_cs_parse, .ring_test = &r600_ring_test, .ib_test = &r600_ib_test, + .is_lockup = &cayman_gpu_is_lockup, }, [CAYMAN_RING_TYPE_CP2_INDEX] = { .ib_execute = &cayman_ring_ib_execute, @@ -1450,6 +1453,7 @@ static struct radeon_asic trinity_asic = { .cs_parse = &evergreen_cs_parse, .ring_test = &r600_ring_test, .ib_test = &r600_ib_test, + .is_lockup = &cayman_gpu_is_lockup, } }, .irq = { @@ -1515,7 +1519,6 @@ static struct radeon_asic si_asic = { .fini = &si_fini, .suspend = &si_suspend, .resume = &si_resume, - .gpu_is_lockup = &si_gpu_is_lockup, .asic_reset = &si_asic_reset, .vga_set_state = &r600_vga_set_state, .ioctl_wait_idle = r600_ioctl_wait_idle, @@ -1534,6 +1537,7 @@ static struct radeon_asic si_asic = { .cs_parse = NULL, .ring_test = &r600_ring_test, .ib_test = &r600_ib_test, + .is_lockup = &si_gpu_is_lockup, }, [CAYMAN_RING_TYPE_CP1_INDEX] = { .ib_execute = &si_ring_ib_execute, @@ -1543,6 +1547,7 @@ static struct radeon_asic si_asic = { .cs_parse = NULL, .ring_test = &r600_ring_test, .ib_test = &r600_ib_test, + .is_lockup = &si_gpu_is_lockup, }, [CAYMAN_RING_TYPE_CP2_INDEX] = { .ib_execute = &si_ring_ib_execute, @@ -1552,6 +1557,7 @@ static struct radeon_asic si_asic = { .cs_parse = NULL, .ring_test = &r600_ring_test, .ib_test = &r600_ib_test, + .is_lockup = &si_gpu_is_lockup, } }, .irq = { diff --git a/drivers/gpu/drm/radeon/radeon_fence.c b/drivers/gpu/drm/radeon/radeon_fence.c index 4bd36a3..66b2229 100644 --- a/drivers/gpu/drm/radeon/radeon_fence.c +++ b/drivers/gpu/drm/radeon/radeon_fence.c @@ -259,7 +259,7 @@ retry: * if we experiencing a lockup the value doesn't change */ if (seq == rdev->fence_drv[fence->ring].last_seq && - radeon_gpu_is_lockup(rdev, &rdev->ring[fence->ring])) { + radeon_ring_is_lockup(rdev, fence->ring, &rdev->ring[fence->ring])) { /* good news we believe it's a lockup */ printk(KERN_WARNING "GPU lockup (waiting for 0x%08X last fence id 0x%08X)\n", fence->seq, seq);
It makes no sense at all to have more than one flag.
Signed-off-by: Christian König deathsimple@vodafone.de Reviewed-by: Alex Deucher alexander.deucher@amd.com Reviewed-by: Jerome Glisse jglisse@redhat.com --- drivers/gpu/drm/radeon/r100.c | 1 - drivers/gpu/drm/radeon/r300.c | 1 - drivers/gpu/drm/radeon/radeon.h | 1 - drivers/gpu/drm/radeon/radeon_device.c | 1 - drivers/gpu/drm/radeon/radeon_fence.c | 36 +++++++++++-------------------- drivers/gpu/drm/radeon/rs600.c | 1 - 6 files changed, 13 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c index cb11418..a0b44a5 100644 --- a/drivers/gpu/drm/radeon/r100.c +++ b/drivers/gpu/drm/radeon/r100.c @@ -2300,7 +2300,6 @@ int r100_asic_reset(struct radeon_device *rdev) if (G_000E40_SE_BUSY(status) || G_000E40_RE_BUSY(status) || G_000E40_TAM_BUSY(status) || G_000E40_PB_BUSY(status)) { dev_err(rdev->dev, "failed to reset GPU\n"); - rdev->gpu_lockup = true; ret = -1; } else dev_info(rdev->dev, "GPU reset succeed\n"); diff --git a/drivers/gpu/drm/radeon/r300.c b/drivers/gpu/drm/radeon/r300.c index fa14383..a63f432 100644 --- a/drivers/gpu/drm/radeon/r300.c +++ b/drivers/gpu/drm/radeon/r300.c @@ -449,7 +449,6 @@ int r300_asic_reset(struct radeon_device *rdev) /* Check if GPU is idle */ if (G_000E40_GA_BUSY(status) || G_000E40_VAP_BUSY(status)) { dev_err(rdev->dev, "failed to reset GPU\n"); - rdev->gpu_lockup = true; ret = -1; } else dev_info(rdev->dev, "GPU reset succeed\n"); diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index 4026a4c0..c76724b 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -1547,7 +1547,6 @@ struct radeon_device { struct radeon_mutex cs_mutex; struct radeon_wb wb; struct radeon_dummy_page dummy_page; - bool gpu_lockup; bool shutdown; bool suspend; bool need_dma32; diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c index 0fb4f89..dedb398 100644 --- a/drivers/gpu/drm/radeon/radeon_device.c +++ b/drivers/gpu/drm/radeon/radeon_device.c @@ -714,7 +714,6 @@ int radeon_device_init(struct radeon_device *rdev, rdev->is_atom_bios = false; rdev->usec_timeout = RADEON_MAX_USEC_TIMEOUT; rdev->mc.gtt_size = radeon_gart_size * 1024 * 1024; - rdev->gpu_lockup = false; rdev->accel_working = false;
DRM_INFO("initializing kernel modesetting (%s 0x%04X:0x%04X 0x%04X:0x%04X).\n", diff --git a/drivers/gpu/drm/radeon/radeon_fence.c b/drivers/gpu/drm/radeon/radeon_fence.c index 66b2229..36c411f 100644 --- a/drivers/gpu/drm/radeon/radeon_fence.c +++ b/drivers/gpu/drm/radeon/radeon_fence.c @@ -71,14 +71,7 @@ int radeon_fence_emit(struct radeon_device *rdev, struct radeon_fence *fence) return 0; } fence->seq = atomic_add_return(1, &rdev->fence_drv[fence->ring].seq); - if (!rdev->ring[fence->ring].ready) - /* FIXME: cp is not running assume everythings is done right - * away - */ - radeon_fence_write(rdev, fence->seq, fence->ring); - else - radeon_fence_ring_emit(rdev, fence->ring, fence); - + radeon_fence_ring_emit(rdev, fence->ring, fence); trace_radeon_fence_emit(rdev->ddev, fence->seq); fence->emitted = true; list_move_tail(&fence->list, &rdev->fence_drv[fence->ring].emitted); @@ -191,9 +184,6 @@ bool radeon_fence_signaled(struct radeon_fence *fence) if (!fence) return true;
- if (fence->rdev->gpu_lockup) - return true; - write_lock_irqsave(&fence->rdev->fence_lock, irq_flags); signaled = fence->signaled; /* if we are shuting down report all fence as signaled */ @@ -260,18 +250,16 @@ retry: */ if (seq == rdev->fence_drv[fence->ring].last_seq && radeon_ring_is_lockup(rdev, fence->ring, &rdev->ring[fence->ring])) { + /* good news we believe it's a lockup */ printk(KERN_WARNING "GPU lockup (waiting for 0x%08X last fence id 0x%08X)\n", fence->seq, seq); - /* FIXME: what should we do ? marking everyone - * as signaled for now - */ - rdev->gpu_lockup = true; + + /* mark the ring as not ready any more */ + rdev->ring[fence->ring].ready = false; r = radeon_gpu_reset(rdev); if (r) return r; - radeon_fence_write(rdev, fence->seq, fence->ring); - rdev->gpu_lockup = false; } timeout = RADEON_FENCE_JIFFIES_TIMEOUT; write_lock_irqsave(&rdev->fence_lock, irq_flags); @@ -289,10 +277,11 @@ int radeon_fence_wait_next(struct radeon_device *rdev, int ring) struct radeon_fence *fence; int r;
- if (rdev->gpu_lockup) { - return 0; - } write_lock_irqsave(&rdev->fence_lock, irq_flags); + if (!rdev->ring[ring].ready) { + write_unlock_irqrestore(&rdev->fence_lock, irq_flags); + return -EBUSY; + } if (list_empty(&rdev->fence_drv[ring].emitted)) { write_unlock_irqrestore(&rdev->fence_lock, irq_flags); return 0; @@ -312,10 +301,11 @@ int radeon_fence_wait_last(struct radeon_device *rdev, int ring) struct radeon_fence *fence; int r;
- if (rdev->gpu_lockup) { - return 0; - } write_lock_irqsave(&rdev->fence_lock, irq_flags); + if (!rdev->ring[ring].ready) { + write_unlock_irqrestore(&rdev->fence_lock, irq_flags); + return -EBUSY; + } if (list_empty(&rdev->fence_drv[ring].emitted)) { write_unlock_irqrestore(&rdev->fence_lock, irq_flags); return 0; diff --git a/drivers/gpu/drm/radeon/rs600.c b/drivers/gpu/drm/radeon/rs600.c index 10706c6..7fb3b1f 100644 --- a/drivers/gpu/drm/radeon/rs600.c +++ b/drivers/gpu/drm/radeon/rs600.c @@ -396,7 +396,6 @@ int rs600_asic_reset(struct radeon_device *rdev) /* Check if GPU is idle */ if (G_000E40_GA_BUSY(status) || G_000E40_VAP_BUSY(status)) { dev_err(rdev->dev, "failed to reset GPU\n"); - rdev->gpu_lockup = true; ret = -1; } else dev_info(rdev->dev, "GPU reset succeed\n");
Just register the debugfs files on init instead of checking the chipset type multiple times.
Signed-off-by: Christian König deathsimple@vodafone.de Reviewed-by: Alex Deucher alexander.deucher@amd.com Reviewed-by: Jerome Glisse jglisse@redhat.com --- drivers/gpu/drm/radeon/radeon_ring.c | 31 +++++++++++++++++++------------ 1 files changed, 19 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c index cc33b3d..b6eb1d2 100644 --- a/drivers/gpu/drm/radeon/radeon_ring.c +++ b/drivers/gpu/drm/radeon/radeon_ring.c @@ -34,7 +34,7 @@ #include "atom.h"
int radeon_debugfs_ib_init(struct radeon_device *rdev); -int radeon_debugfs_ring_init(struct radeon_device *rdev); +int radeon_debugfs_ring_init(struct radeon_device *rdev, struct radeon_ring *ring);
u32 radeon_get_ib_value(struct radeon_cs_parser *p, int idx) { @@ -237,9 +237,6 @@ int radeon_ib_pool_init(struct radeon_device *rdev) if (radeon_debugfs_ib_init(rdev)) { DRM_ERROR("Failed to register debugfs file for IB !\n"); } - if (radeon_debugfs_ring_init(rdev)) { - DRM_ERROR("Failed to register debugfs file for rings !\n"); - } radeon_mutex_unlock(&rdev->ib_pool.mutex); return 0; } @@ -411,6 +408,9 @@ int radeon_ring_init(struct radeon_device *rdev, struct radeon_ring *ring, unsig } ring->ptr_mask = (ring->ring_size / 4) - 1; ring->ring_free_dw = ring->ring_size / 4; + if (radeon_debugfs_ring_init(rdev, ring)) { + DRM_ERROR("Failed to register debugfs file for rings !\n"); + } return 0; }
@@ -501,17 +501,24 @@ static char radeon_debugfs_ib_names[RADEON_IB_POOL_SIZE][32]; static unsigned radeon_debugfs_ib_idx[RADEON_IB_POOL_SIZE]; #endif
-int radeon_debugfs_ring_init(struct radeon_device *rdev) +int radeon_debugfs_ring_init(struct radeon_device *rdev, struct radeon_ring *ring) { #if defined(CONFIG_DEBUG_FS) - if (rdev->family >= CHIP_CAYMAN) - return radeon_debugfs_add_files(rdev, radeon_debugfs_ring_info_list, - ARRAY_SIZE(radeon_debugfs_ring_info_list)); - else - return radeon_debugfs_add_files(rdev, radeon_debugfs_ring_info_list, 1); -#else - return 0; + unsigned i; + for (i = 0; i < ARRAY_SIZE(radeon_debugfs_ring_info_list); ++i) { + struct drm_info_list *info = &radeon_debugfs_ring_info_list[i]; + int ridx = *(int*)radeon_debugfs_ring_info_list[i].data; + unsigned r; + + if (&rdev->ring[ridx] != ring) + continue; + + r = radeon_debugfs_add_files(rdev, info, 1); + if (r) + return r; + } #endif + return 0; }
int radeon_debugfs_ib_init(struct radeon_device *rdev)
Removing all the different error messages and having just one standard behaviour over all chipset generations.
Signed-off-by: Christian König deathsimple@vodafone.de Reviewed-by: Alex Deucher alexander.deucher@amd.com Reviewed-by: Jerome Glisse jglisse@redhat.com --- drivers/gpu/drm/radeon/evergreen.c | 7 ++----- drivers/gpu/drm/radeon/ni.c | 7 ++----- drivers/gpu/drm/radeon/r100.c | 7 ++----- drivers/gpu/drm/radeon/r300.c | 7 ++----- drivers/gpu/drm/radeon/r420.c | 7 ++----- drivers/gpu/drm/radeon/r520.c | 8 +++----- drivers/gpu/drm/radeon/r600.c | 7 ++----- drivers/gpu/drm/radeon/radeon.h | 1 + drivers/gpu/drm/radeon/radeon_ring.c | 30 ++++++++++++++++++++++++++++++ drivers/gpu/drm/radeon/rs400.c | 7 ++----- drivers/gpu/drm/radeon/rs600.c | 7 ++----- drivers/gpu/drm/radeon/rs690.c | 7 ++----- drivers/gpu/drm/radeon/rv515.c | 8 +++----- drivers/gpu/drm/radeon/rv770.c | 7 ++----- 14 files changed, 57 insertions(+), 60 deletions(-)
diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c index eed7ace..8b7a01b 100644 --- a/drivers/gpu/drm/radeon/evergreen.c +++ b/drivers/gpu/drm/radeon/evergreen.c @@ -3376,12 +3376,9 @@ static int evergreen_startup(struct radeon_device *rdev) if (r) return r;
- r = radeon_ib_test(rdev, RADEON_RING_TYPE_GFX_INDEX, &rdev->ring[RADEON_RING_TYPE_GFX_INDEX]); - if (r) { - DRM_ERROR("radeon: failed testing IB (%d).\n", r); - rdev->accel_working = false; + r = radeon_ib_ring_tests(rdev); + if (r) return r; - }
r = r600_audio_init(rdev); if (r) { diff --git a/drivers/gpu/drm/radeon/ni.c b/drivers/gpu/drm/radeon/ni.c index a48ca53..0146428 100644 --- a/drivers/gpu/drm/radeon/ni.c +++ b/drivers/gpu/drm/radeon/ni.c @@ -1601,12 +1601,9 @@ static int cayman_startup(struct radeon_device *rdev) if (r) return r;
- r = radeon_ib_test(rdev, RADEON_RING_TYPE_GFX_INDEX, &rdev->ring[RADEON_RING_TYPE_GFX_INDEX]); - if (r) { - DRM_ERROR("radeon: failed testing IB (%d).\n", r); - rdev->accel_working = false; + r = radeon_ib_ring_tests(rdev); + if (r) return r; - }
r = radeon_vm_manager_start(rdev); if (r) diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c index a0b44a5..825f117 100644 --- a/drivers/gpu/drm/radeon/r100.c +++ b/drivers/gpu/drm/radeon/r100.c @@ -3968,12 +3968,9 @@ static int r100_startup(struct radeon_device *rdev) if (r) return r;
- r = radeon_ib_test(rdev, RADEON_RING_TYPE_GFX_INDEX, &rdev->ring[RADEON_RING_TYPE_GFX_INDEX]); - if (r) { - dev_err(rdev->dev, "failed testing IB (%d).\n", r); - rdev->accel_working = false; + r = radeon_ib_ring_tests(rdev); + if (r) return r; - }
return 0; } diff --git a/drivers/gpu/drm/radeon/r300.c b/drivers/gpu/drm/radeon/r300.c index a63f432..26e0db8 100644 --- a/drivers/gpu/drm/radeon/r300.c +++ b/drivers/gpu/drm/radeon/r300.c @@ -1417,12 +1417,9 @@ static int r300_startup(struct radeon_device *rdev) if (r) return r;
- r = radeon_ib_test(rdev, RADEON_RING_TYPE_GFX_INDEX, &rdev->ring[RADEON_RING_TYPE_GFX_INDEX]); - if (r) { - dev_err(rdev->dev, "failed testing IB (%d).\n", r); - rdev->accel_working = false; + r = radeon_ib_ring_tests(rdev); + if (r) return r; - }
return 0; } diff --git a/drivers/gpu/drm/radeon/r420.c b/drivers/gpu/drm/radeon/r420.c index f3fcaac..99137be 100644 --- a/drivers/gpu/drm/radeon/r420.c +++ b/drivers/gpu/drm/radeon/r420.c @@ -279,12 +279,9 @@ static int r420_startup(struct radeon_device *rdev) if (r) return r;
- r = radeon_ib_test(rdev, RADEON_RING_TYPE_GFX_INDEX, &rdev->ring[RADEON_RING_TYPE_GFX_INDEX]); - if (r) { - dev_err(rdev->dev, "failed testing IB (%d).\n", r); - rdev->accel_working = false; + r = radeon_ib_ring_tests(rdev); + if (r) return r; - }
return 0; } diff --git a/drivers/gpu/drm/radeon/r520.c b/drivers/gpu/drm/radeon/r520.c index ebcc15b..b5cf837 100644 --- a/drivers/gpu/drm/radeon/r520.c +++ b/drivers/gpu/drm/radeon/r520.c @@ -207,12 +207,10 @@ static int r520_startup(struct radeon_device *rdev) if (r) return r;
- r = radeon_ib_test(rdev, RADEON_RING_TYPE_GFX_INDEX, &rdev->ring[RADEON_RING_TYPE_GFX_INDEX]); - if (r) { - dev_err(rdev->dev, "failed testing IB (%d).\n", r); - rdev->accel_working = false; + r = radeon_ib_ring_tests(rdev); + if (r) return r; - } + return 0; }
diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c index 222245d..6070f90 100644 --- a/drivers/gpu/drm/radeon/r600.c +++ b/drivers/gpu/drm/radeon/r600.c @@ -2494,12 +2494,9 @@ int r600_startup(struct radeon_device *rdev) if (r) return r;
- r = radeon_ib_test(rdev, RADEON_RING_TYPE_GFX_INDEX, &rdev->ring[RADEON_RING_TYPE_GFX_INDEX]); - if (r) { - DRM_ERROR("radeon: failed testing IB (%d).\n", r); - rdev->accel_working = false; + r = radeon_ib_ring_tests(rdev); + if (r) return r; - }
return 0; } diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index c76724b..65855af 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -802,6 +802,7 @@ int radeon_ib_pool_init(struct radeon_device *rdev); void radeon_ib_pool_fini(struct radeon_device *rdev); int radeon_ib_pool_start(struct radeon_device *rdev); int radeon_ib_pool_suspend(struct radeon_device *rdev); +int radeon_ib_ring_tests(struct radeon_device *rdev); /* Ring access between begin & end cannot sleep */ int radeon_ring_index(struct radeon_device *rdev, struct radeon_ring *cp); void radeon_ring_free_size(struct radeon_device *rdev, struct radeon_ring *cp); diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c index b6eb1d2..1b020ef 100644 --- a/drivers/gpu/drm/radeon/radeon_ring.c +++ b/drivers/gpu/drm/radeon/radeon_ring.c @@ -267,6 +267,36 @@ int radeon_ib_pool_suspend(struct radeon_device *rdev) return radeon_sa_bo_manager_suspend(rdev, &rdev->ib_pool.sa_manager); }
+int radeon_ib_ring_tests(struct radeon_device *rdev) +{ + unsigned i; + int r; + + for (i = 0; i < RADEON_NUM_RINGS; ++i) { + struct radeon_ring *ring = &rdev->ring[i]; + + if (!ring->ready) + continue; + + r = radeon_ib_test(rdev, i, ring); + if (r) { + ring->ready = false; + + if (i == RADEON_RING_TYPE_GFX_INDEX) { + /* oh, oh, that's really bad */ + DRM_ERROR("radeon: failed testing IB on GFX ring (%d).\n", r); + rdev->accel_working = false; + return r; + + } else { + /* still not good, but we can live with it */ + DRM_ERROR("radeon: failed testing IB on ring %d (%d).\n", i, r); + } + } + } + return 0; +} + /* * Ring. */ diff --git a/drivers/gpu/drm/radeon/rs400.c b/drivers/gpu/drm/radeon/rs400.c index 4cf381b..a464eb5 100644 --- a/drivers/gpu/drm/radeon/rs400.c +++ b/drivers/gpu/drm/radeon/rs400.c @@ -430,12 +430,9 @@ static int rs400_startup(struct radeon_device *rdev) if (r) return r;
- r = radeon_ib_test(rdev, RADEON_RING_TYPE_GFX_INDEX, &rdev->ring[RADEON_RING_TYPE_GFX_INDEX]); - if (r) { - dev_err(rdev->dev, "failed testing IB (%d).\n", r); - rdev->accel_working = false; + r = radeon_ib_ring_tests(rdev); + if (r) return r; - }
return 0; } diff --git a/drivers/gpu/drm/radeon/rs600.c b/drivers/gpu/drm/radeon/rs600.c index 7fb3b1f..25f9eef 100644 --- a/drivers/gpu/drm/radeon/rs600.c +++ b/drivers/gpu/drm/radeon/rs600.c @@ -918,12 +918,9 @@ static int rs600_startup(struct radeon_device *rdev) if (r) return r;
- r = radeon_ib_test(rdev, RADEON_RING_TYPE_GFX_INDEX, &rdev->ring[RADEON_RING_TYPE_GFX_INDEX]); - if (r) { - dev_err(rdev->dev, "failed testing IB (%d).\n", r); - rdev->accel_working = false; + r = radeon_ib_ring_tests(rdev); + if (r) return r; - }
return 0; } diff --git a/drivers/gpu/drm/radeon/rs690.c b/drivers/gpu/drm/radeon/rs690.c index f2c3b9d..3277dde 100644 --- a/drivers/gpu/drm/radeon/rs690.c +++ b/drivers/gpu/drm/radeon/rs690.c @@ -647,12 +647,9 @@ static int rs690_startup(struct radeon_device *rdev) if (r) return r;
- r = radeon_ib_test(rdev, RADEON_RING_TYPE_GFX_INDEX, &rdev->ring[RADEON_RING_TYPE_GFX_INDEX]); - if (r) { - dev_err(rdev->dev, "failed testing IB (%d).\n", r); - rdev->accel_working = false; + r = radeon_ib_ring_tests(rdev); + if (r) return r; - }
return 0; } diff --git a/drivers/gpu/drm/radeon/rv515.c b/drivers/gpu/drm/radeon/rv515.c index d8d78fe..7f08ced 100644 --- a/drivers/gpu/drm/radeon/rv515.c +++ b/drivers/gpu/drm/radeon/rv515.c @@ -412,12 +412,10 @@ static int rv515_startup(struct radeon_device *rdev) if (r) return r;
- r = radeon_ib_test(rdev, RADEON_RING_TYPE_GFX_INDEX, &rdev->ring[RADEON_RING_TYPE_GFX_INDEX]); - if (r) { - dev_err(rdev->dev, "failed testing IB (%d).\n", r); - rdev->accel_working = false; + r = radeon_ib_ring_tests(rdev); + if (r) return r; - } + return 0; }
diff --git a/drivers/gpu/drm/radeon/rv770.c b/drivers/gpu/drm/radeon/rv770.c index c62ae4b..cacec0e 100644 --- a/drivers/gpu/drm/radeon/rv770.c +++ b/drivers/gpu/drm/radeon/rv770.c @@ -1114,12 +1114,9 @@ static int rv770_startup(struct radeon_device *rdev) if (r) return r;
- r = radeon_ib_test(rdev, RADEON_RING_TYPE_GFX_INDEX, &rdev->ring[RADEON_RING_TYPE_GFX_INDEX]); - if (r) { - dev_err(rdev->dev, "IB test failed (%d).\n", r); - rdev->accel_working = false; + r = radeon_ib_ring_tests(rdev); + if (r) return r; - }
return 0; }
Previusly multiple rings could trigger multiple GPU resets at the same time.
Signed-off-by: Christian König deathsimple@vodafone.de Reviewed-by: Jerome Glisse jglisse@redhat.com --- drivers/gpu/drm/radeon/radeon.h | 3 +- drivers/gpu/drm/radeon/radeon_fence.c | 146 +++++++++++++++++---------------- 2 files changed, 75 insertions(+), 74 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index 65855af..35db5bd 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -255,8 +255,7 @@ struct radeon_fence_driver { volatile uint32_t *cpu_addr; atomic_t seq; uint32_t last_seq; - unsigned long last_jiffies; - unsigned long last_timeout; + unsigned long last_activity; wait_queue_head_t queue; struct list_head created; struct list_head emitted; diff --git a/drivers/gpu/drm/radeon/radeon_fence.c b/drivers/gpu/drm/radeon/radeon_fence.c index 36c411f..1a9765a 100644 --- a/drivers/gpu/drm/radeon/radeon_fence.c +++ b/drivers/gpu/drm/radeon/radeon_fence.c @@ -74,6 +74,10 @@ int radeon_fence_emit(struct radeon_device *rdev, struct radeon_fence *fence) radeon_fence_ring_emit(rdev, fence->ring, fence); trace_radeon_fence_emit(rdev->ddev, fence->seq); fence->emitted = true; + /* are we the first fence on a previusly idle ring? */ + if (list_empty(&rdev->fence_drv[fence->ring].emitted)) { + rdev->fence_drv[fence->ring].last_activity = jiffies; + } list_move_tail(&fence->list, &rdev->fence_drv[fence->ring].emitted); write_unlock_irqrestore(&rdev->fence_lock, irq_flags); return 0; @@ -85,34 +89,14 @@ static bool radeon_fence_poll_locked(struct radeon_device *rdev, int ring) struct list_head *i, *n; uint32_t seq; bool wake = false; - unsigned long cjiffies;
seq = radeon_fence_read(rdev, ring); - if (seq != rdev->fence_drv[ring].last_seq) { - rdev->fence_drv[ring].last_seq = seq; - rdev->fence_drv[ring].last_jiffies = jiffies; - rdev->fence_drv[ring].last_timeout = RADEON_FENCE_JIFFIES_TIMEOUT; - } else { - cjiffies = jiffies; - if (time_after(cjiffies, rdev->fence_drv[ring].last_jiffies)) { - cjiffies -= rdev->fence_drv[ring].last_jiffies; - if (time_after(rdev->fence_drv[ring].last_timeout, cjiffies)) { - /* update the timeout */ - rdev->fence_drv[ring].last_timeout -= cjiffies; - } else { - /* the 500ms timeout is elapsed we should test - * for GPU lockup - */ - rdev->fence_drv[ring].last_timeout = 1; - } - } else { - /* wrap around update last jiffies, we will just wait - * a little longer - */ - rdev->fence_drv[ring].last_jiffies = cjiffies; - } + if (seq == rdev->fence_drv[ring].last_seq) return false; - } + + rdev->fence_drv[ring].last_seq = seq; + rdev->fence_drv[ring].last_activity = jiffies; + n = NULL; list_for_each(i, &rdev->fence_drv[ring].emitted) { fence = list_entry(i, struct radeon_fence, list); @@ -207,66 +191,84 @@ int radeon_fence_wait(struct radeon_fence *fence, bool intr) struct radeon_device *rdev; unsigned long irq_flags, timeout; u32 seq; - int r; + int i, r; + bool signaled;
if (fence == NULL) { WARN(1, "Querying an invalid fence : %p !\n", fence); - return 0; + return -EINVAL; } + rdev = fence->rdev; - if (radeon_fence_signaled(fence)) { - return 0; - } - timeout = rdev->fence_drv[fence->ring].last_timeout; -retry: - /* save current sequence used to check for GPU lockup */ - seq = rdev->fence_drv[fence->ring].last_seq; - trace_radeon_fence_wait_begin(rdev->ddev, seq); - if (intr) { + signaled = radeon_fence_signaled(fence); + while (!signaled) { + read_lock_irqsave(&rdev->fence_lock, irq_flags); + timeout = jiffies - RADEON_FENCE_JIFFIES_TIMEOUT; + if (time_after(rdev->fence_drv[fence->ring].last_activity, timeout)) { + /* the normal case, timeout is somewhere before last_activity */ + timeout = rdev->fence_drv[fence->ring].last_activity - timeout; + } else { + /* either jiffies wrapped around, or no fence was signaled in the last 500ms + * anyway we will just wait for the minimum amount and then check for a lockup */ + timeout = 1; + } + /* save current sequence value used to check for GPU lockups */ + seq = rdev->fence_drv[fence->ring].last_seq; + read_unlock_irqrestore(&rdev->fence_lock, irq_flags); + + trace_radeon_fence_wait_begin(rdev->ddev, seq); radeon_irq_kms_sw_irq_get(rdev, fence->ring); - r = wait_event_interruptible_timeout(rdev->fence_drv[fence->ring].queue, - radeon_fence_signaled(fence), timeout); + if (intr) { + r = wait_event_interruptible_timeout( + rdev->fence_drv[fence->ring].queue, + (signaled = radeon_fence_signaled(fence)), timeout); + } else { + r = wait_event_timeout( + rdev->fence_drv[fence->ring].queue, + (signaled = radeon_fence_signaled(fence)), timeout); + } radeon_irq_kms_sw_irq_put(rdev, fence->ring); if (unlikely(r < 0)) { return r; } - } else { - radeon_irq_kms_sw_irq_get(rdev, fence->ring); - r = wait_event_timeout(rdev->fence_drv[fence->ring].queue, - radeon_fence_signaled(fence), timeout); - radeon_irq_kms_sw_irq_put(rdev, fence->ring); - } - trace_radeon_fence_wait_end(rdev->ddev, seq); - if (unlikely(!radeon_fence_signaled(fence))) { - /* we were interrupted for some reason and fence isn't - * isn't signaled yet, resume wait - */ - if (r) { - timeout = r; - goto retry; - } - /* don't protect read access to rdev->fence_drv[t].last_seq - * if we experiencing a lockup the value doesn't change - */ - if (seq == rdev->fence_drv[fence->ring].last_seq && - radeon_ring_is_lockup(rdev, fence->ring, &rdev->ring[fence->ring])) { - - /* good news we believe it's a lockup */ - printk(KERN_WARNING "GPU lockup (waiting for 0x%08X last fence id 0x%08X)\n", - fence->seq, seq); - - /* mark the ring as not ready any more */ - rdev->ring[fence->ring].ready = false; - r = radeon_gpu_reset(rdev); - if (r) - return r; + trace_radeon_fence_wait_end(rdev->ddev, seq); + + if (unlikely(!signaled)) { + /* we were interrupted for some reason and fence + * isn't signaled yet, resume waiting */ + if (r) { + continue; + } + + write_lock_irqsave(&rdev->fence_lock, irq_flags); + /* check if sequence value has changed since last_activity */ + if (seq != rdev->fence_drv[fence->ring].last_seq) { + write_unlock_irqrestore(&rdev->fence_lock, irq_flags); + continue; + } + + /* change sequence value on all rings, so nobody else things there is a lockup */ + for (i = 0; i < RADEON_NUM_RINGS; ++i) + rdev->fence_drv[i].last_seq -= 0x10000; + write_unlock_irqrestore(&rdev->fence_lock, irq_flags); + + if (radeon_ring_is_lockup(rdev, fence->ring, &rdev->ring[fence->ring])) { + + /* good news we believe it's a lockup */ + printk(KERN_WARNING "GPU lockup (waiting for 0x%08X last fence id 0x%08X)\n", + fence->seq, seq); + + /* mark the ring as not ready any more */ + rdev->ring[fence->ring].ready = false; + r = radeon_gpu_reset(rdev); + if (r) + return r; + + write_lock_irqsave(&rdev->fence_lock, irq_flags); + rdev->fence_drv[fence->ring].last_activity = jiffies; + write_unlock_irqrestore(&rdev->fence_lock, irq_flags); + } } - timeout = RADEON_FENCE_JIFFIES_TIMEOUT; - write_lock_irqsave(&rdev->fence_lock, irq_flags); - rdev->fence_drv[fence->ring].last_timeout = RADEON_FENCE_JIFFIES_TIMEOUT; - rdev->fence_drv[fence->ring].last_jiffies = jiffies; - write_unlock_irqrestore(&rdev->fence_lock, irq_flags); - goto retry; } return 0; }
Aligning offset can make it bigger than tmp->offset leading to an overrun bug in the following subtraction.
v2: Against initial suspicions this can't happen in mainline, so no need to push it into stable.
Signed-off-by: Christian König deathsimple@vodafone.de Reviewed-by: Michel Dänzer michel.daenzer@amd.com Reviewed-by: Jerome Glisse jglisse@redhat.com --- drivers/gpu/drm/radeon/radeon_sa.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_sa.c b/drivers/gpu/drm/radeon/radeon_sa.c index 4cce47e..8fbfe69 100644 --- a/drivers/gpu/drm/radeon/radeon_sa.c +++ b/drivers/gpu/drm/radeon/radeon_sa.c @@ -150,7 +150,7 @@ int radeon_sa_bo_new(struct radeon_device *rdev, offset = 0; list_for_each_entry(tmp, &sa_manager->sa_bo, list) { /* room before this object ? */ - if ((tmp->offset - offset) >= size) { + if (offset < tmp->offset && (tmp->offset - offset) >= size) { head = tmp->list.prev; goto out; }
We should signal the caller that we haven't waited at all.
v2: only change fence_wait_next not fence_wait_last.
Signed-off-by: Christian König deathsimple@vodafone.de Reviewed-by: Michel Dänzer michel.daenzer@amd.com Reviewed-by: Jerome Glisse jglisse@redhat.com --- drivers/gpu/drm/radeon/radeon_fence.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_fence.c b/drivers/gpu/drm/radeon/radeon_fence.c index 1a9765a..2fbbc34 100644 --- a/drivers/gpu/drm/radeon/radeon_fence.c +++ b/drivers/gpu/drm/radeon/radeon_fence.c @@ -286,7 +286,7 @@ int radeon_fence_wait_next(struct radeon_device *rdev, int ring) } if (list_empty(&rdev->fence_drv[ring].emitted)) { write_unlock_irqrestore(&rdev->fence_lock, irq_flags); - return 0; + return -ENOENT; } fence = list_entry(rdev->fence_drv[ring].emitted.next, struct radeon_fence, list);
As discussed with Michel that name better describes the behavior of this function.
Signed-off-by: Christian König deathsimple@vodafone.de Reviewed-by: Michel Dänzer michel.daenzer@amd.com Reviewed-by: Jerome Glisse jglisse@redhat.com --- drivers/gpu/drm/radeon/radeon.h | 2 +- drivers/gpu/drm/radeon/radeon_device.c | 2 +- drivers/gpu/drm/radeon/radeon_fence.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index 35db5bd..d73bba8 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -285,7 +285,7 @@ void radeon_fence_process(struct radeon_device *rdev, int ring); bool radeon_fence_signaled(struct radeon_fence *fence); int radeon_fence_wait(struct radeon_fence *fence, bool interruptible); int radeon_fence_wait_next(struct radeon_device *rdev, int ring); -int radeon_fence_wait_last(struct radeon_device *rdev, int ring); +int radeon_fence_wait_empty(struct radeon_device *rdev, int ring); struct radeon_fence *radeon_fence_ref(struct radeon_fence *fence); void radeon_fence_unref(struct radeon_fence **fence); int radeon_fence_count_emitted(struct radeon_device *rdev, int ring); diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c index dedb398..89be94b 100644 --- a/drivers/gpu/drm/radeon/radeon_device.c +++ b/drivers/gpu/drm/radeon/radeon_device.c @@ -915,7 +915,7 @@ int radeon_suspend_kms(struct drm_device *dev, pm_message_t state) radeon_bo_evict_vram(rdev); /* wait for gpu to finish processing current batch */ for (i = 0; i < RADEON_NUM_RINGS; i++) - radeon_fence_wait_last(rdev, i); + radeon_fence_wait_empty(rdev, i);
radeon_save_bios_scratch_regs(rdev);
diff --git a/drivers/gpu/drm/radeon/radeon_fence.c b/drivers/gpu/drm/radeon/radeon_fence.c index 2fbbc34..2d13843 100644 --- a/drivers/gpu/drm/radeon/radeon_fence.c +++ b/drivers/gpu/drm/radeon/radeon_fence.c @@ -297,7 +297,7 @@ int radeon_fence_wait_next(struct radeon_device *rdev, int ring) return r; }
-int radeon_fence_wait_last(struct radeon_device *rdev, int ring) +int radeon_fence_wait_empty(struct radeon_device *rdev, int ring) { unsigned long irq_flags; struct radeon_fence *fence; @@ -442,7 +442,7 @@ void radeon_fence_driver_fini(struct radeon_device *rdev) for (ring = 0; ring < RADEON_NUM_RINGS; ring++) { if (!rdev->fence_drv[ring].initialized) continue; - radeon_fence_wait_last(rdev, ring); + radeon_fence_wait_empty(rdev, ring); wake_up_all(&rdev->fence_drv[ring].queue); write_lock_irqsave(&rdev->fence_lock, irq_flags); radeon_scratch_free(rdev, rdev->fence_drv[ring].scratch_reg);
It's never used and so practically superfluous.
Signed-off-by: Christian König deathsimple@vodafone.de Reviewed-by: Jerome Glisse jglisse@redhat.com --- drivers/gpu/drm/radeon/radeon.h | 1 - drivers/gpu/drm/radeon/radeon_fence.c | 7 ------- 2 files changed, 0 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index d73bba8..794182a 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -257,7 +257,6 @@ struct radeon_fence_driver { uint32_t last_seq; unsigned long last_activity; wait_queue_head_t queue; - struct list_head created; struct list_head emitted; struct list_head signaled; bool initialized; diff --git a/drivers/gpu/drm/radeon/radeon_fence.c b/drivers/gpu/drm/radeon/radeon_fence.c index 2d13843..aadd73a 100644 --- a/drivers/gpu/drm/radeon/radeon_fence.c +++ b/drivers/gpu/drm/radeon/radeon_fence.c @@ -139,8 +139,6 @@ int radeon_fence_create(struct radeon_device *rdev, struct radeon_fence **fence, int ring) { - unsigned long irq_flags; - *fence = kmalloc(sizeof(struct radeon_fence), GFP_KERNEL); if ((*fence) == NULL) { return -ENOMEM; @@ -153,10 +151,6 @@ int radeon_fence_create(struct radeon_device *rdev, (*fence)->ring = ring; (*fence)->semaphore = NULL; INIT_LIST_HEAD(&(*fence)->list); - - write_lock_irqsave(&rdev->fence_lock, irq_flags); - list_add_tail(&(*fence)->list, &rdev->fence_drv[ring].created); - write_unlock_irqrestore(&rdev->fence_lock, irq_flags); return 0; }
@@ -411,7 +405,6 @@ static void radeon_fence_driver_init_ring(struct radeon_device *rdev, int ring) rdev->fence_drv[ring].cpu_addr = NULL; rdev->fence_drv[ring].gpu_addr = 0; atomic_set(&rdev->fence_drv[ring].seq, 0); - INIT_LIST_HEAD(&rdev->fence_drv[ring].created); INIT_LIST_HEAD(&rdev->fence_drv[ring].emitted); INIT_LIST_HEAD(&rdev->fence_drv[ring].signaled); init_waitqueue_head(&rdev->fence_drv[ring].queue);
Rings need to lock in order, otherwise the ring subsystem can deadlock.
v2: fix error handling and number of locked doublewords. v3: stop creating unneeded semaphores.
Signed-off-by: Christian König deathsimple@vodafone.de Reviewed-by: Jerome Glisse jglisse@redhat.com --- drivers/gpu/drm/radeon/radeon.h | 4 ++ drivers/gpu/drm/radeon/radeon_cs.c | 35 ++++++------------ drivers/gpu/drm/radeon/radeon_semaphore.c | 56 +++++++++++++++++++++++++++++ drivers/gpu/drm/radeon/radeon_ttm.c | 46 ++++++++++------------- 4 files changed, 92 insertions(+), 49 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index 794182a..f7372c4 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -460,6 +460,10 @@ void radeon_semaphore_emit_signal(struct radeon_device *rdev, int ring, struct radeon_semaphore *semaphore); void radeon_semaphore_emit_wait(struct radeon_device *rdev, int ring, struct radeon_semaphore *semaphore); +int radeon_semaphore_sync_rings(struct radeon_device *rdev, + struct radeon_semaphore *semaphore, + bool sync_to[RADEON_NUM_RINGS], + int dst_ring); void radeon_semaphore_free(struct radeon_device *rdev, struct radeon_semaphore *semaphore);
diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index e7b0b5d..24fb001 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -118,6 +118,7 @@ static int radeon_cs_get_ring(struct radeon_cs_parser *p, u32 ring, s32 priority static int radeon_cs_sync_rings(struct radeon_cs_parser *p) { bool sync_to_ring[RADEON_NUM_RINGS] = { }; + bool need_sync = false; int i, r;
for (i = 0; i < p->nrelocs; i++) { @@ -126,36 +127,24 @@ static int radeon_cs_sync_rings(struct radeon_cs_parser *p)
if (!(p->relocs[i].flags & RADEON_RELOC_DONT_SYNC)) { struct radeon_fence *fence = p->relocs[i].robj->tbo.sync_obj; - if (!radeon_fence_signaled(fence)) { + if (fence->ring != p->ring && !radeon_fence_signaled(fence)) { sync_to_ring[fence->ring] = true; + need_sync = true; } } }
- for (i = 0; i < RADEON_NUM_RINGS; ++i) { - /* no need to sync to our own or unused rings */ - if (i == p->ring || !sync_to_ring[i] || !p->rdev->ring[i].ready) - continue; - - if (!p->ib->fence->semaphore) { - r = radeon_semaphore_create(p->rdev, &p->ib->fence->semaphore); - if (r) - return r; - } - - r = radeon_ring_lock(p->rdev, &p->rdev->ring[i], 3); - if (r) - return r; - radeon_semaphore_emit_signal(p->rdev, i, p->ib->fence->semaphore); - radeon_ring_unlock_commit(p->rdev, &p->rdev->ring[i]); + if (!need_sync) { + return 0; + }
- r = radeon_ring_lock(p->rdev, &p->rdev->ring[p->ring], 3); - if (r) - return r; - radeon_semaphore_emit_wait(p->rdev, p->ring, p->ib->fence->semaphore); - radeon_ring_unlock_commit(p->rdev, &p->rdev->ring[p->ring]); + r = radeon_semaphore_create(p->rdev, &p->ib->fence->semaphore); + if (r) { + return r; } - return 0; + + return radeon_semaphore_sync_rings(p->rdev, p->ib->fence->semaphore, + sync_to_ring, p->ring); }
int radeon_cs_parser_init(struct radeon_cs_parser *p, void *data) diff --git a/drivers/gpu/drm/radeon/radeon_semaphore.c b/drivers/gpu/drm/radeon/radeon_semaphore.c index 61dd4e3..930a08a 100644 --- a/drivers/gpu/drm/radeon/radeon_semaphore.c +++ b/drivers/gpu/drm/radeon/radeon_semaphore.c @@ -149,6 +149,62 @@ void radeon_semaphore_emit_wait(struct radeon_device *rdev, int ring, radeon_semaphore_ring_emit(rdev, ring, &rdev->ring[ring], semaphore, true); }
+int radeon_semaphore_sync_rings(struct radeon_device *rdev, + struct radeon_semaphore *semaphore, + bool sync_to[RADEON_NUM_RINGS], + int dst_ring) +{ + int i, r; + + for (i = 0; i < RADEON_NUM_RINGS; ++i) { + unsigned num_ops = i == dst_ring ? RADEON_NUM_RINGS : 1; + + /* don't lock unused rings */ + if (!sync_to[i] && i != dst_ring) + continue; + + /* prevent GPU deadlocks */ + if (!rdev->ring[i].ready) { + dev_err(rdev->dev, "Trying to sync to a disabled ring!"); + r = -EINVAL; + goto error; + } + + r = radeon_ring_lock(rdev, &rdev->ring[i], num_ops * 8); + if (r) + goto error; + } + + for (i = 0; i < RADEON_NUM_RINGS; ++i) { + /* no need to sync to our own or unused rings */ + if (!sync_to[i] || i == dst_ring) + continue; + + radeon_semaphore_emit_signal(rdev, i, semaphore); + radeon_semaphore_emit_wait(rdev, dst_ring, semaphore); + } + + for (i = 0; i < RADEON_NUM_RINGS; ++i) { + + /* don't unlock unused rings */ + if (!sync_to[i] && i != dst_ring) + continue; + + radeon_ring_unlock_commit(rdev, &rdev->ring[i]); + } + + return 0; + +error: + /* unlock all locks taken so far */ + for (--i; i >= 0; --i) { + if (sync_to[i] || i == dst_ring) { + radeon_ring_unlock_undo(rdev, &rdev->ring[i]); + } + } + return r; +} + void radeon_semaphore_free(struct radeon_device *rdev, struct radeon_semaphore *semaphore) { diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c index f493c64..5e3d54d 100644 --- a/drivers/gpu/drm/radeon/radeon_ttm.c +++ b/drivers/gpu/drm/radeon/radeon_ttm.c @@ -222,8 +222,8 @@ static int radeon_move_blit(struct ttm_buffer_object *bo, { struct radeon_device *rdev; uint64_t old_start, new_start; - struct radeon_fence *fence; - int r, i; + struct radeon_fence *fence, *old_fence; + int r;
rdev = radeon_get_rdev(bo->bdev); r = radeon_fence_create(rdev, &fence, radeon_copy_ring_index(rdev)); @@ -242,6 +242,7 @@ static int radeon_move_blit(struct ttm_buffer_object *bo, break; default: DRM_ERROR("Unknown placement %d\n", old_mem->mem_type); + radeon_fence_unref(&fence); return -EINVAL; } switch (new_mem->mem_type) { @@ -253,42 +254,35 @@ static int radeon_move_blit(struct ttm_buffer_object *bo, break; default: DRM_ERROR("Unknown placement %d\n", old_mem->mem_type); + radeon_fence_unref(&fence); return -EINVAL; } if (!rdev->ring[radeon_copy_ring_index(rdev)].ready) { DRM_ERROR("Trying to move memory with ring turned off.\n"); + radeon_fence_unref(&fence); return -EINVAL; }
BUILD_BUG_ON((PAGE_SIZE % RADEON_GPU_PAGE_SIZE) != 0);
/* sync other rings */ - if (rdev->family >= CHIP_R600) { - for (i = 0; i < RADEON_NUM_RINGS; ++i) { - /* no need to sync to our own or unused rings */ - if (i == radeon_copy_ring_index(rdev) || !rdev->ring[i].ready) - continue; - - if (!fence->semaphore) { - r = radeon_semaphore_create(rdev, &fence->semaphore); - /* FIXME: handle semaphore error */ - if (r) - continue; - } + old_fence = bo->sync_obj; + if (old_fence && old_fence->ring != fence->ring + && !radeon_fence_signaled(old_fence)) { + bool sync_to_ring[RADEON_NUM_RINGS] = { }; + sync_to_ring[old_fence->ring] = true; + + r = radeon_semaphore_create(rdev, &fence->semaphore); + if (r) { + radeon_fence_unref(&fence); + return r; + }
- r = radeon_ring_lock(rdev, &rdev->ring[i], 3); - /* FIXME: handle ring lock error */ - if (r) - continue; - radeon_semaphore_emit_signal(rdev, i, fence->semaphore); - radeon_ring_unlock_commit(rdev, &rdev->ring[i]); - - r = radeon_ring_lock(rdev, &rdev->ring[radeon_copy_ring_index(rdev)], 3); - /* FIXME: handle ring lock error */ - if (r) - continue; - radeon_semaphore_emit_wait(rdev, radeon_copy_ring_index(rdev), fence->semaphore); - radeon_ring_unlock_commit(rdev, &rdev->ring[radeon_copy_ring_index(rdev)]); + r = radeon_semaphore_sync_rings(rdev, fence->semaphore, + sync_to_ring, fence->ring); + if (r) { + radeon_fence_unref(&fence); + return r; } }
Instead of all this humpy pumpy with recursive mutex (which also fixes only halve of the problem) move the actual gpu reset out of the fence code, return -EDEADLK and then reset the gpu in the calling ioctl function.
v2: Split removal of radeon_mutex into separate patch. Return -EAGAIN if reset is successful.
Signed-off-by: Christian König deathsimple@vodafone.de Reviewed-by: Jerome Glisse jglisse@redhat.com --- drivers/gpu/drm/radeon/radeon_cs.c | 13 +++++++++++++ drivers/gpu/drm/radeon/radeon_device.c | 5 ----- drivers/gpu/drm/radeon/radeon_fence.c | 10 +++------- drivers/gpu/drm/radeon/radeon_gem.c | 16 ++++++++++++++++ 4 files changed, 32 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index 24fb001..02eee4b 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -496,6 +496,16 @@ out: return r; }
+static int radeon_cs_handle_lockup(struct radeon_device *rdev, int r) +{ + if (r == -EDEADLK) { + r = radeon_gpu_reset(rdev); + if (!r) + r = -EAGAIN; + } + return r; +} + int radeon_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp) { struct radeon_device *rdev = dev->dev_private; @@ -517,6 +527,7 @@ int radeon_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp) if (r) { DRM_ERROR("Failed to initialize parser !\n"); radeon_cs_parser_fini(&parser, r); + r = radeon_cs_handle_lockup(rdev, r); radeon_mutex_unlock(&rdev->cs_mutex); return r; } @@ -525,6 +536,7 @@ int radeon_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp) if (r != -ERESTARTSYS) DRM_ERROR("Failed to parse relocation %d!\n", r); radeon_cs_parser_fini(&parser, r); + r = radeon_cs_handle_lockup(rdev, r); radeon_mutex_unlock(&rdev->cs_mutex); return r; } @@ -538,6 +550,7 @@ int radeon_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp) } out: radeon_cs_parser_fini(&parser, r); + r = radeon_cs_handle_lockup(rdev, r); radeon_mutex_unlock(&rdev->cs_mutex); return r; } diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c index 89be94b..d18f0c4 100644 --- a/drivers/gpu/drm/radeon/radeon_device.c +++ b/drivers/gpu/drm/radeon/radeon_device.c @@ -986,9 +986,6 @@ int radeon_gpu_reset(struct radeon_device *rdev) int r; int resched;
- /* Prevent CS ioctl from interfering */ - radeon_mutex_lock(&rdev->cs_mutex); - radeon_save_bios_scratch_regs(rdev); /* block TTM */ resched = ttm_bo_lock_delayed_workqueue(&rdev->mman.bdev); @@ -1003,8 +1000,6 @@ int radeon_gpu_reset(struct radeon_device *rdev) ttm_bo_unlock_delayed_workqueue(&rdev->mman.bdev, resched); }
- radeon_mutex_unlock(&rdev->cs_mutex); - if (r) { /* bad news, how to tell it to userspace ? */ dev_info(rdev->dev, "GPU reset failed\n"); diff --git a/drivers/gpu/drm/radeon/radeon_fence.c b/drivers/gpu/drm/radeon/radeon_fence.c index aadd73a..5bb78bf 100644 --- a/drivers/gpu/drm/radeon/radeon_fence.c +++ b/drivers/gpu/drm/radeon/radeon_fence.c @@ -244,6 +244,8 @@ int radeon_fence_wait(struct radeon_fence *fence, bool intr) /* change sequence value on all rings, so nobody else things there is a lockup */ for (i = 0; i < RADEON_NUM_RINGS; ++i) rdev->fence_drv[i].last_seq -= 0x10000; + + rdev->fence_drv[fence->ring].last_activity = jiffies; write_unlock_irqrestore(&rdev->fence_lock, irq_flags);
if (radeon_ring_is_lockup(rdev, fence->ring, &rdev->ring[fence->ring])) { @@ -254,13 +256,7 @@ int radeon_fence_wait(struct radeon_fence *fence, bool intr)
/* mark the ring as not ready any more */ rdev->ring[fence->ring].ready = false; - r = radeon_gpu_reset(rdev); - if (r) - return r; - - write_lock_irqsave(&rdev->fence_lock, irq_flags); - rdev->fence_drv[fence->ring].last_activity = jiffies; - write_unlock_irqrestore(&rdev->fence_lock, irq_flags); + return -EDEADLK; } } } diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c index c7008b5..e15cb1f 100644 --- a/drivers/gpu/drm/radeon/radeon_gem.c +++ b/drivers/gpu/drm/radeon/radeon_gem.c @@ -154,6 +154,17 @@ void radeon_gem_object_close(struct drm_gem_object *obj, radeon_bo_unreserve(rbo); }
+static int radeon_gem_handle_lockup(struct radeon_device *rdev, int r) +{ + if (r == -EDEADLK) { + radeon_mutex_lock(&rdev->cs_mutex); + r = radeon_gpu_reset(rdev); + if (!r) + r = -EAGAIN; + radeon_mutex_unlock(&rdev->cs_mutex); + } + return r; +}
/* * GEM ioctls. @@ -210,12 +221,14 @@ int radeon_gem_create_ioctl(struct drm_device *dev, void *data, args->initial_domain, false, false, &gobj); if (r) { + r = radeon_gem_handle_lockup(rdev, r); return r; } r = drm_gem_handle_create(filp, gobj, &handle); /* drop reference from allocate - handle holds it now */ drm_gem_object_unreference_unlocked(gobj); if (r) { + r = radeon_gem_handle_lockup(rdev, r); return r; } args->handle = handle; @@ -245,6 +258,7 @@ int radeon_gem_set_domain_ioctl(struct drm_device *dev, void *data, r = radeon_gem_set_domain(gobj, args->read_domains, args->write_domain);
drm_gem_object_unreference_unlocked(gobj); + r = radeon_gem_handle_lockup(robj->rdev, r); return r; }
@@ -301,6 +315,7 @@ int radeon_gem_busy_ioctl(struct drm_device *dev, void *data, break; } drm_gem_object_unreference_unlocked(gobj); + r = radeon_gem_handle_lockup(robj->rdev, r); return r; }
@@ -322,6 +337,7 @@ int radeon_gem_wait_idle_ioctl(struct drm_device *dev, void *data, if (robj->rdev->asic->ioctl_wait_idle) robj->rdev->asic->ioctl_wait_idle(robj->rdev, robj); drm_gem_object_unreference_unlocked(gobj); + r = radeon_gem_handle_lockup(robj->rdev, r); return r; }
It isn't chipset specific, so it makes no sense to have that inside r100.c.
Signed-off-by: Christian König deathsimple@vodafone.de Reviewed-by: Jerome Glisse jglisse@redhat.com --- drivers/gpu/drm/radeon/evergreen.c | 5 +-- drivers/gpu/drm/radeon/ni.c | 5 +-- drivers/gpu/drm/radeon/r100.c | 57 +-------------------------------- drivers/gpu/drm/radeon/r300.c | 4 +- drivers/gpu/drm/radeon/r600.c | 10 +----- drivers/gpu/drm/radeon/radeon.h | 16 ++------- drivers/gpu/drm/radeon/radeon_asic.h | 5 --- drivers/gpu/drm/radeon/radeon_ring.c | 53 +++++++++++++++++++++++++++++++ drivers/gpu/drm/radeon/si.c | 5 +-- 9 files changed, 69 insertions(+), 91 deletions(-)
diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c index 8b7a01b..66f05bb 100644 --- a/drivers/gpu/drm/radeon/evergreen.c +++ b/drivers/gpu/drm/radeon/evergreen.c @@ -2424,7 +2424,6 @@ bool evergreen_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *rin u32 srbm_status; u32 grbm_status; u32 grbm_status_se0, grbm_status_se1; - struct r100_gpu_lockup *lockup = &rdev->config.evergreen.lockup; int r;
srbm_status = RREG32(SRBM_STATUS); @@ -2432,7 +2431,7 @@ bool evergreen_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *rin grbm_status_se0 = RREG32(GRBM_STATUS_SE0); grbm_status_se1 = RREG32(GRBM_STATUS_SE1); if (!(grbm_status & GUI_ACTIVE)) { - r100_gpu_lockup_update(lockup, ring); + radeon_ring_lockup_update(ring); return false; } /* force CP activities */ @@ -2444,7 +2443,7 @@ bool evergreen_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *rin radeon_ring_unlock_commit(rdev, ring); } ring->rptr = RREG32(CP_RB_RPTR); - return r100_gpu_cp_is_lockup(rdev, lockup, ring); + return radeon_ring_test_lockup(rdev, ring); }
static int evergreen_gpu_soft_reset(struct radeon_device *rdev) diff --git a/drivers/gpu/drm/radeon/ni.c b/drivers/gpu/drm/radeon/ni.c index 0146428..8404b2a 100644 --- a/drivers/gpu/drm/radeon/ni.c +++ b/drivers/gpu/drm/radeon/ni.c @@ -1397,7 +1397,6 @@ bool cayman_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring) u32 srbm_status; u32 grbm_status; u32 grbm_status_se0, grbm_status_se1; - struct r100_gpu_lockup *lockup = &rdev->config.cayman.lockup; int r;
srbm_status = RREG32(SRBM_STATUS); @@ -1405,7 +1404,7 @@ bool cayman_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring) grbm_status_se0 = RREG32(GRBM_STATUS_SE0); grbm_status_se1 = RREG32(GRBM_STATUS_SE1); if (!(grbm_status & GUI_ACTIVE)) { - r100_gpu_lockup_update(lockup, ring); + radeon_ring_lockup_update(ring); return false; } /* force CP activities */ @@ -1418,7 +1417,7 @@ bool cayman_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring) } /* XXX deal with CP0,1,2 */ ring->rptr = RREG32(ring->rptr_reg); - return r100_gpu_cp_is_lockup(rdev, lockup, ring); + return radeon_ring_test_lockup(rdev, ring); }
static int cayman_gpu_soft_reset(struct radeon_device *rdev) diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c index 825f117..ccf5e3b 100644 --- a/drivers/gpu/drm/radeon/r100.c +++ b/drivers/gpu/drm/radeon/r100.c @@ -2159,59 +2159,6 @@ int r100_mc_wait_for_idle(struct radeon_device *rdev) return -1; }
-void r100_gpu_lockup_update(struct r100_gpu_lockup *lockup, struct radeon_ring *ring) -{ - lockup->last_cp_rptr = ring->rptr; - lockup->last_jiffies = jiffies; -} - -/** - * r100_gpu_cp_is_lockup() - check if CP is lockup by recording information - * @rdev: radeon device structure - * @lockup: r100_gpu_lockup structure holding CP lockup tracking informations - * @cp: radeon_cp structure holding CP information - * - * We don't need to initialize the lockup tracking information as we will either - * have CP rptr to a different value of jiffies wrap around which will force - * initialization of the lockup tracking informations. - * - * A possible false positivie is if we get call after while and last_cp_rptr == - * the current CP rptr, even if it's unlikely it might happen. To avoid this - * if the elapsed time since last call is bigger than 2 second than we return - * false and update the tracking information. Due to this the caller must call - * r100_gpu_cp_is_lockup several time in less than 2sec for lockup to be reported - * the fencing code should be cautious about that. - * - * Caller should write to the ring to force CP to do something so we don't get - * false positive when CP is just gived nothing to do. - * - **/ -bool r100_gpu_cp_is_lockup(struct radeon_device *rdev, struct r100_gpu_lockup *lockup, struct radeon_ring *ring) -{ - unsigned long cjiffies, elapsed; - - cjiffies = jiffies; - if (!time_after(cjiffies, lockup->last_jiffies)) { - /* likely a wrap around */ - lockup->last_cp_rptr = ring->rptr; - lockup->last_jiffies = jiffies; - return false; - } - if (ring->rptr != lockup->last_cp_rptr) { - /* CP is still working no lockup */ - lockup->last_cp_rptr = ring->rptr; - lockup->last_jiffies = jiffies; - return false; - } - elapsed = jiffies_to_msecs(cjiffies - lockup->last_jiffies); - if (elapsed >= 10000) { - dev_err(rdev->dev, "GPU lockup CP stall for more than %lumsec\n", elapsed); - return true; - } - /* give a chance to the GPU ... */ - return false; -} - bool r100_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring) { u32 rbbm_status; @@ -2219,7 +2166,7 @@ bool r100_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring)
rbbm_status = RREG32(R_000E40_RBBM_STATUS); if (!G_000E40_GUI_ACTIVE(rbbm_status)) { - r100_gpu_lockup_update(&rdev->config.r100.lockup, ring); + radeon_ring_lockup_update(ring); return false; } /* force CP activities */ @@ -2231,7 +2178,7 @@ bool r100_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring) radeon_ring_unlock_commit(rdev, ring); } ring->rptr = RREG32(ring->rptr_reg); - return r100_gpu_cp_is_lockup(rdev, &rdev->config.r100.lockup, ring); + return radeon_ring_test_lockup(rdev, ring); }
void r100_bm_disable(struct radeon_device *rdev) diff --git a/drivers/gpu/drm/radeon/r300.c b/drivers/gpu/drm/radeon/r300.c index 26e0db8..e207664 100644 --- a/drivers/gpu/drm/radeon/r300.c +++ b/drivers/gpu/drm/radeon/r300.c @@ -384,7 +384,7 @@ bool r300_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring)
rbbm_status = RREG32(R_000E40_RBBM_STATUS); if (!G_000E40_GUI_ACTIVE(rbbm_status)) { - r100_gpu_lockup_update(&rdev->config.r300.lockup, ring); + radeon_ring_lockup_update(ring); return false; } /* force CP activities */ @@ -396,7 +396,7 @@ bool r300_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring) radeon_ring_unlock_commit(rdev, ring); } ring->rptr = RREG32(RADEON_CP_RB_RPTR); - return r100_gpu_cp_is_lockup(rdev, &rdev->config.r300.lockup, ring); + return radeon_ring_test_lockup(rdev, ring); }
int r300_asic_reset(struct radeon_device *rdev) diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c index 6070f90..45d52cc 100644 --- a/drivers/gpu/drm/radeon/r600.c +++ b/drivers/gpu/drm/radeon/r600.c @@ -1350,19 +1350,13 @@ bool r600_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring) u32 srbm_status; u32 grbm_status; u32 grbm_status2; - struct r100_gpu_lockup *lockup; int r;
- if (rdev->family >= CHIP_RV770) - lockup = &rdev->config.rv770.lockup; - else - lockup = &rdev->config.r600.lockup; - srbm_status = RREG32(R_000E50_SRBM_STATUS); grbm_status = RREG32(R_008010_GRBM_STATUS); grbm_status2 = RREG32(R_008014_GRBM_STATUS2); if (!G_008010_GUI_ACTIVE(grbm_status)) { - r100_gpu_lockup_update(lockup, ring); + radeon_ring_lockup_update(ring); return false; } /* force CP activities */ @@ -1374,7 +1368,7 @@ bool r600_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring) radeon_ring_unlock_commit(rdev, ring); } ring->rptr = RREG32(ring->rptr_reg); - return r100_gpu_cp_is_lockup(rdev, lockup, ring); + return radeon_ring_test_lockup(rdev, ring); }
int r600_asic_reset(struct radeon_device *rdev) diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index f7372c4..b8d4d94 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -670,6 +670,8 @@ struct radeon_ring { unsigned ring_size; unsigned ring_free_dw; int count_dw; + unsigned long last_activity; + unsigned last_rptr; uint64_t gpu_addr; uint32_t align_mask; uint32_t ptr_mask; @@ -814,6 +816,8 @@ void radeon_ring_commit(struct radeon_device *rdev, struct radeon_ring *cp); void radeon_ring_unlock_commit(struct radeon_device *rdev, struct radeon_ring *cp); void radeon_ring_unlock_undo(struct radeon_device *rdev, struct radeon_ring *cp); int radeon_ring_test(struct radeon_device *rdev, struct radeon_ring *cp); +void radeon_ring_lockup_update(struct radeon_ring *ring); +bool radeon_ring_test_lockup(struct radeon_device *rdev, struct radeon_ring *ring); int radeon_ring_init(struct radeon_device *rdev, struct radeon_ring *cp, unsigned ring_size, unsigned rptr_offs, unsigned rptr_reg, unsigned wptr_reg, u32 ptr_reg_shift, u32 ptr_reg_mask, u32 nop); @@ -1272,16 +1276,10 @@ struct radeon_asic { /* * Asic structures */ -struct r100_gpu_lockup { - unsigned long last_jiffies; - u32 last_cp_rptr; -}; - struct r100_asic { const unsigned *reg_safe_bm; unsigned reg_safe_bm_size; u32 hdp_cntl; - struct r100_gpu_lockup lockup; };
struct r300_asic { @@ -1289,7 +1287,6 @@ struct r300_asic { unsigned reg_safe_bm_size; u32 resync_scratch; u32 hdp_cntl; - struct r100_gpu_lockup lockup; };
struct r600_asic { @@ -1311,7 +1308,6 @@ struct r600_asic { unsigned tiling_group_size; unsigned tile_config; unsigned backend_map; - struct r100_gpu_lockup lockup; };
struct rv770_asic { @@ -1337,7 +1333,6 @@ struct rv770_asic { unsigned tiling_group_size; unsigned tile_config; unsigned backend_map; - struct r100_gpu_lockup lockup; };
struct evergreen_asic { @@ -1364,7 +1359,6 @@ struct evergreen_asic { unsigned tiling_group_size; unsigned tile_config; unsigned backend_map; - struct r100_gpu_lockup lockup; };
struct cayman_asic { @@ -1403,7 +1397,6 @@ struct cayman_asic { unsigned multi_gpu_tile_size;
unsigned tile_config; - struct r100_gpu_lockup lockup; };
struct si_asic { @@ -1434,7 +1427,6 @@ struct si_asic { unsigned multi_gpu_tile_size;
unsigned tile_config; - struct r100_gpu_lockup lockup; };
union radeon_asic_config { diff --git a/drivers/gpu/drm/radeon/radeon_asic.h b/drivers/gpu/drm/radeon/radeon_asic.h index b135bec..b0941f9 100644 --- a/drivers/gpu/drm/radeon/radeon_asic.h +++ b/drivers/gpu/drm/radeon/radeon_asic.h @@ -103,11 +103,6 @@ int r100_pci_gart_enable(struct radeon_device *rdev); void r100_pci_gart_disable(struct radeon_device *rdev); int r100_debugfs_mc_info_init(struct radeon_device *rdev); int r100_gui_wait_for_idle(struct radeon_device *rdev); -void r100_gpu_lockup_update(struct r100_gpu_lockup *lockup, - struct radeon_ring *cp); -bool r100_gpu_cp_is_lockup(struct radeon_device *rdev, - struct r100_gpu_lockup *lockup, - struct radeon_ring *cp); void r100_ib_fini(struct radeon_device *rdev); int r100_ib_test(struct radeon_device *rdev, struct radeon_ring *ring); void r100_irq_disable(struct radeon_device *rdev); diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c index 1b020ef..203ec1f 100644 --- a/drivers/gpu/drm/radeon/radeon_ring.c +++ b/drivers/gpu/drm/radeon/radeon_ring.c @@ -396,6 +396,59 @@ void radeon_ring_unlock_undo(struct radeon_device *rdev, struct radeon_ring *rin mutex_unlock(&ring->mutex); }
+void radeon_ring_lockup_update(struct radeon_ring *ring) +{ + ring->last_rptr = ring->rptr; + ring->last_activity = jiffies; +} + +/** + * radeon_ring_test_lockup() - check if ring is lockedup by recording information + * @rdev: radeon device structure + * @ring: radeon_ring structure holding ring information + * + * We don't need to initialize the lockup tracking information as we will either + * have CP rptr to a different value of jiffies wrap around which will force + * initialization of the lockup tracking informations. + * + * A possible false positivie is if we get call after while and last_cp_rptr == + * the current CP rptr, even if it's unlikely it might happen. To avoid this + * if the elapsed time since last call is bigger than 2 second than we return + * false and update the tracking information. Due to this the caller must call + * radeon_ring_test_lockup several time in less than 2sec for lockup to be reported + * the fencing code should be cautious about that. + * + * Caller should write to the ring to force CP to do something so we don't get + * false positive when CP is just gived nothing to do. + * + **/ +bool radeon_ring_test_lockup(struct radeon_device *rdev, struct radeon_ring *ring) +{ + unsigned long cjiffies, elapsed; + uint32_t rptr; + + cjiffies = jiffies; + if (!time_after(cjiffies, ring->last_activity)) { + /* likely a wrap around */ + radeon_ring_lockup_update(ring); + return false; + } + rptr = RREG32(ring->rptr_reg); + ring->rptr = (rptr & ring->ptr_reg_mask) >> ring->ptr_reg_shift; + if (ring->rptr != ring->last_rptr) { + /* CP is still working no lockup */ + radeon_ring_lockup_update(ring); + return false; + } + elapsed = jiffies_to_msecs(cjiffies - ring->last_activity); + if (elapsed >= 10000) { + dev_err(rdev->dev, "GPU lockup CP stall for more than %lumsec\n", elapsed); + return true; + } + /* give a chance to the GPU ... */ + return false; +} + int radeon_ring_init(struct radeon_device *rdev, struct radeon_ring *ring, unsigned ring_size, unsigned rptr_offs, unsigned rptr_reg, unsigned wptr_reg, u32 ptr_reg_shift, u32 ptr_reg_mask, u32 nop) diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c index 14919e1..c2422f5 100644 --- a/drivers/gpu/drm/radeon/si.c +++ b/drivers/gpu/drm/radeon/si.c @@ -2217,7 +2217,6 @@ bool si_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring) u32 srbm_status; u32 grbm_status, grbm_status2; u32 grbm_status_se0, grbm_status_se1; - struct r100_gpu_lockup *lockup = &rdev->config.si.lockup; int r;
srbm_status = RREG32(SRBM_STATUS); @@ -2226,7 +2225,7 @@ bool si_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring) grbm_status_se0 = RREG32(GRBM_STATUS_SE0); grbm_status_se1 = RREG32(GRBM_STATUS_SE1); if (!(grbm_status & GUI_ACTIVE)) { - r100_gpu_lockup_update(lockup, ring); + radeon_ring_lockup_update(ring); return false; } /* force CP activities */ @@ -2239,7 +2238,7 @@ bool si_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring) } /* XXX deal with CP0,1,2 */ ring->rptr = RREG32(ring->rptr_reg); - return r100_gpu_cp_is_lockup(rdev, lockup, ring); + return radeon_ring_test_lockup(rdev, ring); }
static int si_gpu_soft_reset(struct radeon_device *rdev)
Don't hard code the 10 seconds timeout. Compute jobs can run much longer.
Signed-off-by: Christian König deathsimple@vodafone.de Reviewed-by: Jerome Glisse jglisse@redhat.com --- drivers/gpu/drm/radeon/radeon.h | 1 + drivers/gpu/drm/radeon/radeon_drv.c | 4 ++++ drivers/gpu/drm/radeon/radeon_ring.c | 2 +- 3 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index b8d4d94..0784e4d 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -94,6 +94,7 @@ extern int radeon_disp_priority; extern int radeon_hw_i2c; extern int radeon_pcie_gen2; extern int radeon_msi; +extern int radeon_lockup_timeout;
/* * Copy from radeon_drv.h so we don't have to include both and have conflicting diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c index ef7bb3f..e62e56a 100644 --- a/drivers/gpu/drm/radeon/radeon_drv.c +++ b/drivers/gpu/drm/radeon/radeon_drv.c @@ -128,6 +128,7 @@ int radeon_disp_priority = 0; int radeon_hw_i2c = 0; int radeon_pcie_gen2 = 0; int radeon_msi = -1; +int radeon_lockup_timeout = 10000;
MODULE_PARM_DESC(no_wb, "Disable AGP writeback for scratch registers"); module_param_named(no_wb, radeon_no_wb, int, 0444); @@ -177,6 +178,9 @@ module_param_named(pcie_gen2, radeon_pcie_gen2, int, 0444); MODULE_PARM_DESC(msi, "MSI support (1 = enable, 0 = disable, -1 = auto)"); module_param_named(msi, radeon_msi, int, 0444);
+MODULE_PARM_DESC(lockup_timeout, "GPU lockup timeout in ms (defaul 10000 = 10 seconds, 0 = disable)"); +module_param_named(lockup_timeout, radeon_lockup_timeout, int, 0444); + static int radeon_suspend(struct drm_device *dev, pm_message_t state) { drm_radeon_private_t *dev_priv = dev->dev_private; diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c index 203ec1f..08e1578 100644 --- a/drivers/gpu/drm/radeon/radeon_ring.c +++ b/drivers/gpu/drm/radeon/radeon_ring.c @@ -441,7 +441,7 @@ bool radeon_ring_test_lockup(struct radeon_device *rdev, struct radeon_ring *rin return false; } elapsed = jiffies_to_msecs(cjiffies - ring->last_activity); - if (elapsed >= 10000) { + if (radeon_lockup_timeout && elapsed >= radeon_lockup_timeout) { dev_err(rdev->dev, "GPU lockup CP stall for more than %lumsec\n", elapsed); return true; }
Fixing just another deadlock problem with gpu reset tests.
Signed-off-by: Christian König deathsimple@vodafone.de Reviewed-by: Jerome Glisse jglisse@redhat.com --- drivers/gpu/drm/radeon/radeon_ring.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c index 08e1578..407d90a 100644 --- a/drivers/gpu/drm/radeon/radeon_ring.c +++ b/drivers/gpu/drm/radeon/radeon_ring.c @@ -346,7 +346,9 @@ int radeon_ring_alloc(struct radeon_device *rdev, struct radeon_ring *ring, unsi if (ndw < ring->ring_free_dw) { break; } + mutex_unlock(&ring->mutex); r = radeon_fence_wait_next(rdev, radeon_ring_index(rdev, ring)); + mutex_lock(&ring->mutex); if (r) return r; }
Nothing chipset or ring specific with it, so also move it to radon_ring.
Signed-off-by: Christian König deathsimple@vodafone.de Reviewed-by: Jerome Glisse jglisse@redhat.com --- drivers/gpu/drm/radeon/evergreen.c | 10 +--------- drivers/gpu/drm/radeon/ni.c | 11 +---------- drivers/gpu/drm/radeon/r100.c | 10 +--------- drivers/gpu/drm/radeon/r300.c | 10 +--------- drivers/gpu/drm/radeon/r600.c | 10 +--------- drivers/gpu/drm/radeon/radeon.h | 1 + drivers/gpu/drm/radeon/radeon_ring.c | 16 ++++++++++++++++ drivers/gpu/drm/radeon/si.c | 11 +---------- 8 files changed, 23 insertions(+), 56 deletions(-)
diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c index 66f05bb..ecc29bc 100644 --- a/drivers/gpu/drm/radeon/evergreen.c +++ b/drivers/gpu/drm/radeon/evergreen.c @@ -2424,7 +2424,6 @@ bool evergreen_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *rin u32 srbm_status; u32 grbm_status; u32 grbm_status_se0, grbm_status_se1; - int r;
srbm_status = RREG32(SRBM_STATUS); grbm_status = RREG32(GRBM_STATUS); @@ -2435,14 +2434,7 @@ bool evergreen_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *rin return false; } /* force CP activities */ - r = radeon_ring_lock(rdev, ring, 2); - if (!r) { - /* PACKET2 NOP */ - radeon_ring_write(ring, 0x80000000); - radeon_ring_write(ring, 0x80000000); - radeon_ring_unlock_commit(rdev, ring); - } - ring->rptr = RREG32(CP_RB_RPTR); + radeon_ring_force_activity(rdev, ring); return radeon_ring_test_lockup(rdev, ring); }
diff --git a/drivers/gpu/drm/radeon/ni.c b/drivers/gpu/drm/radeon/ni.c index 8404b2a..1184635 100644 --- a/drivers/gpu/drm/radeon/ni.c +++ b/drivers/gpu/drm/radeon/ni.c @@ -1397,7 +1397,6 @@ bool cayman_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring) u32 srbm_status; u32 grbm_status; u32 grbm_status_se0, grbm_status_se1; - int r;
srbm_status = RREG32(SRBM_STATUS); grbm_status = RREG32(GRBM_STATUS); @@ -1408,15 +1407,7 @@ bool cayman_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring) return false; } /* force CP activities */ - r = radeon_ring_lock(rdev, ring, 2); - if (!r) { - /* PACKET2 NOP */ - radeon_ring_write(ring, 0x80000000); - radeon_ring_write(ring, 0x80000000); - radeon_ring_unlock_commit(rdev, ring); - } - /* XXX deal with CP0,1,2 */ - ring->rptr = RREG32(ring->rptr_reg); + radeon_ring_force_activity(rdev, ring); return radeon_ring_test_lockup(rdev, ring); }
diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c index ccf5e3b..42d60ab 100644 --- a/drivers/gpu/drm/radeon/r100.c +++ b/drivers/gpu/drm/radeon/r100.c @@ -2162,7 +2162,6 @@ int r100_mc_wait_for_idle(struct radeon_device *rdev) bool r100_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring) { u32 rbbm_status; - int r;
rbbm_status = RREG32(R_000E40_RBBM_STATUS); if (!G_000E40_GUI_ACTIVE(rbbm_status)) { @@ -2170,14 +2169,7 @@ bool r100_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring) return false; } /* force CP activities */ - r = radeon_ring_lock(rdev, ring, 2); - if (!r) { - /* PACKET2 NOP */ - radeon_ring_write(ring, 0x80000000); - radeon_ring_write(ring, 0x80000000); - radeon_ring_unlock_commit(rdev, ring); - } - ring->rptr = RREG32(ring->rptr_reg); + radeon_ring_force_activity(rdev, ring); return radeon_ring_test_lockup(rdev, ring); }
diff --git a/drivers/gpu/drm/radeon/r300.c b/drivers/gpu/drm/radeon/r300.c index e207664..04ec269 100644 --- a/drivers/gpu/drm/radeon/r300.c +++ b/drivers/gpu/drm/radeon/r300.c @@ -380,7 +380,6 @@ void r300_gpu_init(struct radeon_device *rdev) bool r300_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring) { u32 rbbm_status; - int r;
rbbm_status = RREG32(R_000E40_RBBM_STATUS); if (!G_000E40_GUI_ACTIVE(rbbm_status)) { @@ -388,14 +387,7 @@ bool r300_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring) return false; } /* force CP activities */ - r = radeon_ring_lock(rdev, ring, 2); - if (!r) { - /* PACKET2 NOP */ - radeon_ring_write(ring, 0x80000000); - radeon_ring_write(ring, 0x80000000); - radeon_ring_unlock_commit(rdev, ring); - } - ring->rptr = RREG32(RADEON_CP_RB_RPTR); + radeon_ring_force_activity(rdev, ring); return radeon_ring_test_lockup(rdev, ring); }
diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c index 45d52cc..87a2333 100644 --- a/drivers/gpu/drm/radeon/r600.c +++ b/drivers/gpu/drm/radeon/r600.c @@ -1350,7 +1350,6 @@ bool r600_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring) u32 srbm_status; u32 grbm_status; u32 grbm_status2; - int r;
srbm_status = RREG32(R_000E50_SRBM_STATUS); grbm_status = RREG32(R_008010_GRBM_STATUS); @@ -1360,14 +1359,7 @@ bool r600_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring) return false; } /* force CP activities */ - r = radeon_ring_lock(rdev, ring, 2); - if (!r) { - /* PACKET2 NOP */ - radeon_ring_write(ring, 0x80000000); - radeon_ring_write(ring, 0x80000000); - radeon_ring_unlock_commit(rdev, ring); - } - ring->rptr = RREG32(ring->rptr_reg); + radeon_ring_force_activity(rdev, ring); return radeon_ring_test_lockup(rdev, ring); }
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index 0784e4d..82ffa6a 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -817,6 +817,7 @@ void radeon_ring_commit(struct radeon_device *rdev, struct radeon_ring *cp); void radeon_ring_unlock_commit(struct radeon_device *rdev, struct radeon_ring *cp); void radeon_ring_unlock_undo(struct radeon_device *rdev, struct radeon_ring *cp); int radeon_ring_test(struct radeon_device *rdev, struct radeon_ring *cp); +void radeon_ring_force_activity(struct radeon_device *rdev, struct radeon_ring *ring); void radeon_ring_lockup_update(struct radeon_ring *ring); bool radeon_ring_test_lockup(struct radeon_device *rdev, struct radeon_ring *ring); int radeon_ring_init(struct radeon_device *rdev, struct radeon_ring *cp, unsigned ring_size, diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c index 407d90a..2eb4c6e 100644 --- a/drivers/gpu/drm/radeon/radeon_ring.c +++ b/drivers/gpu/drm/radeon/radeon_ring.c @@ -398,6 +398,22 @@ void radeon_ring_unlock_undo(struct radeon_device *rdev, struct radeon_ring *rin mutex_unlock(&ring->mutex); }
+void radeon_ring_force_activity(struct radeon_device *rdev, struct radeon_ring *ring) +{ + int r; + + mutex_lock(&ring->mutex); + radeon_ring_free_size(rdev, ring); + if (ring->rptr == ring->wptr) { + r = radeon_ring_alloc(rdev, ring, 1); + if (!r) { + radeon_ring_write(ring, ring->nop); + radeon_ring_commit(rdev, ring); + } + } + mutex_unlock(&ring->mutex); +} + void radeon_ring_lockup_update(struct radeon_ring *ring) { ring->last_rptr = ring->rptr; diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c index c2422f5..0bad5ff 100644 --- a/drivers/gpu/drm/radeon/si.c +++ b/drivers/gpu/drm/radeon/si.c @@ -2217,7 +2217,6 @@ bool si_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring) u32 srbm_status; u32 grbm_status, grbm_status2; u32 grbm_status_se0, grbm_status_se1; - int r;
srbm_status = RREG32(SRBM_STATUS); grbm_status = RREG32(GRBM_STATUS); @@ -2229,15 +2228,7 @@ bool si_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring) return false; } /* force CP activities */ - r = radeon_ring_lock(rdev, ring, 2); - if (!r) { - /* PACKET2 NOP */ - radeon_ring_write(ring, 0x80000000); - radeon_ring_write(ring, 0x80000000); - radeon_ring_unlock_commit(rdev, ring); - } - /* XXX deal with CP0,1,2 */ - ring->rptr = RREG32(ring->rptr_reg); + radeon_ring_force_activity(rdev, ring); return radeon_ring_test_lockup(rdev, ring); }
Since it is now identical to r100_gpu_is_lockup.
Signed-off-by: Christian König deathsimple@vodafone.de Reviewed-by: Jerome Glisse jglisse@redhat.com --- drivers/gpu/drm/radeon/r300.c | 14 -------------- drivers/gpu/drm/radeon/radeon_asic.c | 16 ++++++++-------- drivers/gpu/drm/radeon/radeon_asic.h | 1 - 3 files changed, 8 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/radeon/r300.c b/drivers/gpu/drm/radeon/r300.c index 04ec269..6419a59 100644 --- a/drivers/gpu/drm/radeon/r300.c +++ b/drivers/gpu/drm/radeon/r300.c @@ -377,20 +377,6 @@ void r300_gpu_init(struct radeon_device *rdev) rdev->num_gb_pipes, rdev->num_z_pipes); }
-bool r300_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring) -{ - u32 rbbm_status; - - rbbm_status = RREG32(R_000E40_RBBM_STATUS); - if (!G_000E40_GUI_ACTIVE(rbbm_status)) { - radeon_ring_lockup_update(ring); - return false; - } - /* force CP activities */ - radeon_ring_force_activity(rdev, ring); - return radeon_ring_test_lockup(rdev, ring); -} - int r300_asic_reset(struct radeon_device *rdev) { struct r100_mc_save save; diff --git a/drivers/gpu/drm/radeon/radeon_asic.c b/drivers/gpu/drm/radeon/radeon_asic.c index 958b9ea..5e5694e 100644 --- a/drivers/gpu/drm/radeon/radeon_asic.c +++ b/drivers/gpu/drm/radeon/radeon_asic.c @@ -299,7 +299,7 @@ static struct radeon_asic r300_asic = { .ring_start = &r300_ring_start, .ring_test = &r100_ring_test, .ib_test = &r100_ib_test, - .is_lockup = &r300_gpu_is_lockup, + .is_lockup = &r100_gpu_is_lockup, } }, .irq = { @@ -373,7 +373,7 @@ static struct radeon_asic r300_asic_pcie = { .ring_start = &r300_ring_start, .ring_test = &r100_ring_test, .ib_test = &r100_ib_test, - .is_lockup = &r300_gpu_is_lockup, + .is_lockup = &r100_gpu_is_lockup, } }, .irq = { @@ -447,7 +447,7 @@ static struct radeon_asic r420_asic = { .ring_start = &r300_ring_start, .ring_test = &r100_ring_test, .ib_test = &r100_ib_test, - .is_lockup = &r300_gpu_is_lockup, + .is_lockup = &r100_gpu_is_lockup, } }, .irq = { @@ -521,7 +521,7 @@ static struct radeon_asic rs400_asic = { .ring_start = &r300_ring_start, .ring_test = &r100_ring_test, .ib_test = &r100_ib_test, - .is_lockup = &r300_gpu_is_lockup, + .is_lockup = &r100_gpu_is_lockup, } }, .irq = { @@ -595,7 +595,7 @@ static struct radeon_asic rs600_asic = { .ring_start = &r300_ring_start, .ring_test = &r100_ring_test, .ib_test = &r100_ib_test, - .is_lockup = &r300_gpu_is_lockup, + .is_lockup = &r100_gpu_is_lockup, } }, .irq = { @@ -669,7 +669,7 @@ static struct radeon_asic rs690_asic = { .ring_start = &r300_ring_start, .ring_test = &r100_ring_test, .ib_test = &r100_ib_test, - .is_lockup = &r300_gpu_is_lockup, + .is_lockup = &r100_gpu_is_lockup, } }, .irq = { @@ -743,7 +743,7 @@ static struct radeon_asic rv515_asic = { .ring_start = &rv515_ring_start, .ring_test = &r100_ring_test, .ib_test = &r100_ib_test, - .is_lockup = &r300_gpu_is_lockup, + .is_lockup = &r100_gpu_is_lockup, } }, .irq = { @@ -817,7 +817,7 @@ static struct radeon_asic r520_asic = { .ring_start = &rv515_ring_start, .ring_test = &r100_ring_test, .ib_test = &r100_ib_test, - .is_lockup = &r300_gpu_is_lockup, + .is_lockup = &r100_gpu_is_lockup, } }, .irq = { diff --git a/drivers/gpu/drm/radeon/radeon_asic.h b/drivers/gpu/drm/radeon/radeon_asic.h index b0941f9..1e128e0 100644 --- a/drivers/gpu/drm/radeon/radeon_asic.h +++ b/drivers/gpu/drm/radeon/radeon_asic.h @@ -154,7 +154,6 @@ extern int r300_init(struct radeon_device *rdev); extern void r300_fini(struct radeon_device *rdev); extern int r300_suspend(struct radeon_device *rdev); extern int r300_resume(struct radeon_device *rdev); -extern bool r300_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *cp); extern int r300_asic_reset(struct radeon_device *rdev); extern void r300_ring_start(struct radeon_device *rdev, struct radeon_ring *ring); extern void r300_fence_ring_emit(struct radeon_device *rdev,
Since it is now identical to evergreen_gpu_is_lockup.
Signed-off-by: Christian König deathsimple@vodafone.de Reviewed-by: Jerome Glisse jglisse@redhat.com --- drivers/gpu/drm/radeon/ni.c | 19 ------------------- drivers/gpu/drm/radeon/radeon_asic.c | 12 ++++++------ drivers/gpu/drm/radeon/radeon_asic.h | 1 - 3 files changed, 6 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/radeon/ni.c b/drivers/gpu/drm/radeon/ni.c index 1184635..9cd2657 100644 --- a/drivers/gpu/drm/radeon/ni.c +++ b/drivers/gpu/drm/radeon/ni.c @@ -1392,25 +1392,6 @@ int cayman_cp_resume(struct radeon_device *rdev) return 0; }
-bool cayman_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *ring) -{ - u32 srbm_status; - u32 grbm_status; - u32 grbm_status_se0, grbm_status_se1; - - srbm_status = RREG32(SRBM_STATUS); - grbm_status = RREG32(GRBM_STATUS); - grbm_status_se0 = RREG32(GRBM_STATUS_SE0); - grbm_status_se1 = RREG32(GRBM_STATUS_SE1); - if (!(grbm_status & GUI_ACTIVE)) { - radeon_ring_lockup_update(ring); - return false; - } - /* force CP activities */ - radeon_ring_force_activity(rdev, ring); - return radeon_ring_test_lockup(rdev, ring); -} - static int cayman_gpu_soft_reset(struct radeon_device *rdev) { struct evergreen_mc_save save; diff --git a/drivers/gpu/drm/radeon/radeon_asic.c b/drivers/gpu/drm/radeon/radeon_asic.c index 5e5694e..f533df5 100644 --- a/drivers/gpu/drm/radeon/radeon_asic.c +++ b/drivers/gpu/drm/radeon/radeon_asic.c @@ -1339,7 +1339,7 @@ static struct radeon_asic cayman_asic = { .cs_parse = &evergreen_cs_parse, .ring_test = &r600_ring_test, .ib_test = &r600_ib_test, - .is_lockup = &cayman_gpu_is_lockup, + .is_lockup = &evergreen_gpu_is_lockup, }, [CAYMAN_RING_TYPE_CP1_INDEX] = { .ib_execute = &cayman_ring_ib_execute, @@ -1349,7 +1349,7 @@ static struct radeon_asic cayman_asic = { .cs_parse = &evergreen_cs_parse, .ring_test = &r600_ring_test, .ib_test = &r600_ib_test, - .is_lockup = &cayman_gpu_is_lockup, + .is_lockup = &evergreen_gpu_is_lockup, }, [CAYMAN_RING_TYPE_CP2_INDEX] = { .ib_execute = &cayman_ring_ib_execute, @@ -1359,7 +1359,7 @@ static struct radeon_asic cayman_asic = { .cs_parse = &evergreen_cs_parse, .ring_test = &r600_ring_test, .ib_test = &r600_ib_test, - .is_lockup = &cayman_gpu_is_lockup, + .is_lockup = &evergreen_gpu_is_lockup, } }, .irq = { @@ -1433,7 +1433,7 @@ static struct radeon_asic trinity_asic = { .cs_parse = &evergreen_cs_parse, .ring_test = &r600_ring_test, .ib_test = &r600_ib_test, - .is_lockup = &cayman_gpu_is_lockup, + .is_lockup = &evergreen_gpu_is_lockup, }, [CAYMAN_RING_TYPE_CP1_INDEX] = { .ib_execute = &cayman_ring_ib_execute, @@ -1443,7 +1443,7 @@ static struct radeon_asic trinity_asic = { .cs_parse = &evergreen_cs_parse, .ring_test = &r600_ring_test, .ib_test = &r600_ib_test, - .is_lockup = &cayman_gpu_is_lockup, + .is_lockup = &evergreen_gpu_is_lockup, }, [CAYMAN_RING_TYPE_CP2_INDEX] = { .ib_execute = &cayman_ring_ib_execute, @@ -1453,7 +1453,7 @@ static struct radeon_asic trinity_asic = { .cs_parse = &evergreen_cs_parse, .ring_test = &r600_ring_test, .ib_test = &r600_ib_test, - .is_lockup = &cayman_gpu_is_lockup, + .is_lockup = &evergreen_gpu_is_lockup, } }, .irq = { diff --git a/drivers/gpu/drm/radeon/radeon_asic.h b/drivers/gpu/drm/radeon/radeon_asic.h index 1e128e0..7830931 100644 --- a/drivers/gpu/drm/radeon/radeon_asic.h +++ b/drivers/gpu/drm/radeon/radeon_asic.h @@ -437,7 +437,6 @@ int cayman_init(struct radeon_device *rdev); void cayman_fini(struct radeon_device *rdev); int cayman_suspend(struct radeon_device *rdev); int cayman_resume(struct radeon_device *rdev); -bool cayman_gpu_is_lockup(struct radeon_device *rdev, struct radeon_ring *cp); int cayman_asic_reset(struct radeon_device *rdev); void cayman_ring_ib_execute(struct radeon_device *rdev, struct radeon_ib *ib); int cayman_vm_init(struct radeon_device *rdev);
On Wed, May 2, 2012 at 9:11 AM, Christian König deathsimple@vodafone.de wrote:
Hi Dave,
there still seems to be the need for some further discussion about the SA code, so I again split that out of the patchset and tested the result a bit.
Most of the stuff still works fine without those offending changes, so to avoid mailing around unrelated and already reviewed patches, I request the include the following 17 patches into drm-next.
If you prefer to merge they are also available from git://people.freedesktop.org/~deathsimple/linux branch reset-rework.
Cheers, Christian.
I am ok with this 17 patchset, i just sent 3 patch on top of those 17 that bring back some other of the previous cleanup.
So for the 17 Reviewed-by: Jerome Glisse jglisse@redhat.com
Cheers, Jerome
On 02.05.2012 18:01, Jerome Glisse wrote:
On Wed, May 2, 2012 at 9:11 AM, Christian Königdeathsimple@vodafone.de wrote:
Hi Dave,
there still seems to be the need for some further discussion about the SA code, so I again split that out of the patchset and tested the result a bit.
Most of the stuff still works fine without those offending changes, so to avoid mailing around unrelated and already reviewed patches, I request the include the following 17 patches into drm-next.
If you prefer to merge they are also available from git://people.freedesktop.org/~deathsimple/linux branch reset-rework.
Cheers, Christian.
I am ok with this 17 patchset, i just sent 3 patch on top of those 17 that bring back some other of the previous cleanup.
At least for now those three are NAK, cause I just realized we need to put those on top of a more sophisticated fence implementation.
Your idea of not using a list, but 64 bit sequences instead actually sounds quite nifty to me. Going to hack something together in the next couple of hours.
Christian.
On Thu, May 3, 2012 at 4:19 AM, Christian König deathsimple@vodafone.de wrote:
On 02.05.2012 18:01, Jerome Glisse wrote:
On Wed, May 2, 2012 at 9:11 AM, Christian Königdeathsimple@vodafone.de wrote:
Hi Dave,
there still seems to be the need for some further discussion about the SA code, so I again split that out of the patchset and tested the result a bit.
Most of the stuff still works fine without those offending changes, so to avoid mailing around unrelated and already reviewed patches, I request the include the following 17 patches into drm-next.
If you prefer to merge they are also available from git://people.freedesktop.org/~deathsimple/linux branch reset-rework.
Cheers, Christian.
I am ok with this 17 patchset, i just sent 3 patch on top of those 17 that bring back some other of the previous cleanup.
At least for now those three are NAK, cause I just realized we need to put those on top of a more sophisticated fence implementation.
Your idea of not using a list, but 64 bit sequences instead actually sounds quite nifty to me. Going to hack something together in the next couple of hours.
Christian.
Btw you said that you are having issue when using multiple ring, it comes to my attention that you never sync with the GFX ring (unless asked by userspace) that's wrong, before scheduling on another ring than GFX index you need to emit semaphore to make the ring wait for the last emited fence on the GFX ring because of ttm. What might happen is that ttm scheduled bo move on the GFX ring and that the ring you work on start using those bo at there soon to be GPU offset while the bo data is not there yet.
Cheers, Jerome
On 03.05.2012 18:32, Jerome Glisse wrote:
On Thu, May 3, 2012 at 4:19 AM, Christian Königdeathsimple@vodafone.de wrote:
On 02.05.2012 18:01, Jerome Glisse wrote:
On Wed, May 2, 2012 at 9:11 AM, Christian Königdeathsimple@vodafone.de wrote:
Hi Dave,
there still seems to be the need for some further discussion about the SA code, so I again split that out of the patchset and tested the result a bit.
Most of the stuff still works fine without those offending changes, so to avoid mailing around unrelated and already reviewed patches, I request the include the following 17 patches into drm-next.
If you prefer to merge they are also available from git://people.freedesktop.org/~deathsimple/linux branch reset-rework.
Cheers, Christian.
I am ok with this 17 patchset, i just sent 3 patch on top of those 17 that bring back some other of the previous cleanup.
At least for now those three are NAK, cause I just realized we need to put those on top of a more sophisticated fence implementation.
Your idea of not using a list, but 64 bit sequences instead actually sounds quite nifty to me. Going to hack something together in the next couple of hours.
Christian.
Btw you said that you are having issue when using multiple ring, it comes to my attention that you never sync with the GFX ring (unless asked by userspace) that's wrong, before scheduling on another ring than GFX index you need to emit semaphore to make the ring wait for the last emited fence on the GFX ring because of ttm. What might happen is that ttm scheduled bo move on the GFX ring and that the ring you work on start using those bo at there soon to be GPU offset while the bo data is not there yet.
Yeah, already stumbled over that but IIRC Alex already solved that issue.
We set the sync_obj to the fence of the move operation, so when there is a move operation in progress we sync to it correctly (at least I hope so).
Christian.
On Thu, May 3, 2012 at 12:39 PM, Christian König deathsimple@vodafone.de wrote:
On 03.05.2012 18:32, Jerome Glisse wrote:
On Thu, May 3, 2012 at 4:19 AM, Christian Königdeathsimple@vodafone.de wrote:
On 02.05.2012 18:01, Jerome Glisse wrote:
On Wed, May 2, 2012 at 9:11 AM, Christian Königdeathsimple@vodafone.de wrote:
Hi Dave,
there still seems to be the need for some further discussion about the SA code, so I again split that out of the patchset and tested the result a bit.
Most of the stuff still works fine without those offending changes, so to avoid mailing around unrelated and already reviewed patches, I request the include the following 17 patches into drm-next.
If you prefer to merge they are also available from git://people.freedesktop.org/~deathsimple/linux branch reset-rework.
Cheers, Christian.
I am ok with this 17 patchset, i just sent 3 patch on top of those 17 that bring back some other of the previous cleanup.
At least for now those three are NAK, cause I just realized we need to put those on top of a more sophisticated fence implementation.
Your idea of not using a list, but 64 bit sequences instead actually sounds quite nifty to me. Going to hack something together in the next couple of hours.
Christian.
Btw you said that you are having issue when using multiple ring, it comes to my attention that you never sync with the GFX ring (unless asked by userspace) that's wrong, before scheduling on another ring than GFX index you need to emit semaphore to make the ring wait for the last emited fence on the GFX ring because of ttm. What might happen is that ttm scheduled bo move on the GFX ring and that the ring you work on start using those bo at there soon to be GPU offset while the bo data is not there yet.
Yeah, already stumbled over that but IIRC Alex already solved that issue.
We set the sync_obj to the fence of the move operation, so when there is a move operation in progress we sync to it correctly (at least I hope so).
Christian.
Looking at code doesn't seems ok, yes you use the fence from the move operation but you only emit wait if userspace ask for it, that's wrong.
if (!(p->relocs[i].flags & RADEON_RELOC_DONT_SYNC)) {
in radeon_cs.c
Cheers, Jerome
2012/5/3 Jerome Glisse j.glisse@gmail.com:
On Thu, May 3, 2012 at 12:39 PM, Christian König deathsimple@vodafone.de wrote:
On 03.05.2012 18:32, Jerome Glisse wrote:
On Thu, May 3, 2012 at 4:19 AM, Christian Königdeathsimple@vodafone.de wrote:
On 02.05.2012 18:01, Jerome Glisse wrote:
On Wed, May 2, 2012 at 9:11 AM, Christian Königdeathsimple@vodafone.de wrote:
Hi Dave,
there still seems to be the need for some further discussion about the SA code, so I again split that out of the patchset and tested the result a bit.
Most of the stuff still works fine without those offending changes, so to avoid mailing around unrelated and already reviewed patches, I request the include the following 17 patches into drm-next.
If you prefer to merge they are also available from git://people.freedesktop.org/~deathsimple/linux branch reset-rework.
Cheers, Christian.
I am ok with this 17 patchset, i just sent 3 patch on top of those 17 that bring back some other of the previous cleanup.
At least for now those three are NAK, cause I just realized we need to put those on top of a more sophisticated fence implementation.
Your idea of not using a list, but 64 bit sequences instead actually sounds quite nifty to me. Going to hack something together in the next couple of hours.
Christian.
Btw you said that you are having issue when using multiple ring, it comes to my attention that you never sync with the GFX ring (unless asked by userspace) that's wrong, before scheduling on another ring than GFX index you need to emit semaphore to make the ring wait for the last emited fence on the GFX ring because of ttm. What might happen is that ttm scheduled bo move on the GFX ring and that the ring you work on start using those bo at there soon to be GPU offset while the bo data is not there yet.
Yeah, already stumbled over that but IIRC Alex already solved that issue.
We set the sync_obj to the fence of the move operation, so when there is a move operation in progress we sync to it correctly (at least I hope so).
Christian.
Looking at code doesn't seems ok, yes you use the fence from the move operation but you only emit wait if userspace ask for it, that's wrong.
if (!(p->relocs[i].flags & RADEON_RELOC_DONT_SYNC)) {
The default is to sync all the rings. We only skip the sync if the _DONT_ sync flag is set.
Alex
in radeon_cs.c
Cheers, Jerome
On Thu, May 3, 2012 at 1:20 PM, Alex Deucher alexdeucher@gmail.com wrote:
2012/5/3 Jerome Glisse j.glisse@gmail.com:
On Thu, May 3, 2012 at 12:39 PM, Christian König deathsimple@vodafone.de wrote:
On 03.05.2012 18:32, Jerome Glisse wrote:
On Thu, May 3, 2012 at 4:19 AM, Christian Königdeathsimple@vodafone.de wrote:
On 02.05.2012 18:01, Jerome Glisse wrote:
On Wed, May 2, 2012 at 9:11 AM, Christian Königdeathsimple@vodafone.de wrote: > > Hi Dave, > > there still seems to be the need for some further discussion about the > SA > code, > so I again split that out of the patchset and tested the result a bit. > > Most of the stuff still works fine without those offending changes, so > to > avoid > mailing around unrelated and already reviewed patches, I request the > include > the following 17 patches into drm-next. > > If you prefer to merge they are also available from > git://people.freedesktop.org/~deathsimple/linux branch reset-rework. > > Cheers, > Christian. > I am ok with this 17 patchset, i just sent 3 patch on top of those 17 that bring back some other of the previous cleanup.
At least for now those three are NAK, cause I just realized we need to put those on top of a more sophisticated fence implementation.
Your idea of not using a list, but 64 bit sequences instead actually sounds quite nifty to me. Going to hack something together in the next couple of hours.
Christian.
Btw you said that you are having issue when using multiple ring, it comes to my attention that you never sync with the GFX ring (unless asked by userspace) that's wrong, before scheduling on another ring than GFX index you need to emit semaphore to make the ring wait for the last emited fence on the GFX ring because of ttm. What might happen is that ttm scheduled bo move on the GFX ring and that the ring you work on start using those bo at there soon to be GPU offset while the bo data is not there yet.
Yeah, already stumbled over that but IIRC Alex already solved that issue.
We set the sync_obj to the fence of the move operation, so when there is a move operation in progress we sync to it correctly (at least I hope so).
Christian.
Looking at code doesn't seems ok, yes you use the fence from the move operation but you only emit wait if userspace ask for it, that's wrong.
if (!(p->relocs[i].flags & RADEON_RELOC_DONT_SYNC)) {
The default is to sync all the rings. We only skip the sync if the _DONT_ sync flag is set.
Alex
That's my point exactly, so if a bo move happen and userspace set dont sync flag then the other ring might start doing work before bo data is in place. This test should be removed or at least make the sync to the bo move fence unconditional and use this flag only for sync with other ring relative to current job you are scheduling.
Cheers, Jerome
On 03.05.2012 19:20, Alex Deucher wrote:
2012/5/3 Jerome Glissej.glisse@gmail.com:
On Thu, May 3, 2012 at 12:39 PM, Christian König deathsimple@vodafone.de wrote:
On 03.05.2012 18:32, Jerome Glisse wrote:
On Thu, May 3, 2012 at 4:19 AM, Christian Königdeathsimple@vodafone.de wrote:
On 02.05.2012 18:01, Jerome Glisse wrote:
On Wed, May 2, 2012 at 9:11 AM, Christian Königdeathsimple@vodafone.de wrote: > Hi Dave, > > there still seems to be the need for some further discussion about the > SA > code, > so I again split that out of the patchset and tested the result a bit. > > Most of the stuff still works fine without those offending changes, so > to > avoid > mailing around unrelated and already reviewed patches, I request the > include > the following 17 patches into drm-next. > > If you prefer to merge they are also available from > git://people.freedesktop.org/~deathsimple/linux branch reset-rework. > > Cheers, > Christian. > I am ok with this 17 patchset, i just sent 3 patch on top of those 17 that bring back some other of the previous cleanup.
At least for now those three are NAK, cause I just realized we need to put those on top of a more sophisticated fence implementation.
Your idea of not using a list, but 64 bit sequences instead actually sounds quite nifty to me. Going to hack something together in the next couple of hours.
Christian.
Btw you said that you are having issue when using multiple ring, it comes to my attention that you never sync with the GFX ring (unless asked by userspace) that's wrong, before scheduling on another ring than GFX index you need to emit semaphore to make the ring wait for the last emited fence on the GFX ring because of ttm. What might happen is that ttm scheduled bo move on the GFX ring and that the ring you work on start using those bo at there soon to be GPU offset while the bo data is not there yet.
Yeah, already stumbled over that but IIRC Alex already solved that issue..
We set the sync_obj to the fence of the move operation, so when there is a move operation in progress we sync to it correctly (at least I hope so).
Christian.
Looking at code doesn't seems ok, yes you use the fence from the move operation but you only emit wait if userspace ask for it, that's wrong.
if (!(p->relocs[i].flags& RADEON_RELOC_DONT_SYNC)) {
The default is to sync all the rings. We only skip the sync if the _DONT_ sync flag is set.
Yeah, but the _DONT_ sync flag is just an optimization, and we only want to not sync to things userspace has submitted.
E.g. userspace tells us that two jobs can happily run on the same bo even if they are both writing to it....
So Jerome is completely right, userspace doesn't expect that TTM is jumping in between and moving the bo around, that is indeed a bug and another thing for the todo list.
Christian.
On Thu, May 3, 2012 at 1:28 PM, Christian König deathsimple@vodafone.de wrote:
On 03.05.2012 19:20, Alex Deucher wrote:
2012/5/3 Jerome Glissej.glisse@gmail.com:
On Thu, May 3, 2012 at 12:39 PM, Christian König deathsimple@vodafone.de wrote:
On 03.05.2012 18:32, Jerome Glisse wrote:
On Thu, May 3, 2012 at 4:19 AM, Christian Königdeathsimple@vodafone.de wrote:
On 02.05.2012 18:01, Jerome Glisse wrote: > > On Wed, May 2, 2012 at 9:11 AM, Christian > Königdeathsimple@vodafone.de > wrote: >> >> Hi Dave, >> >> there still seems to be the need for some further discussion about >> the >> SA >> code, >> so I again split that out of the patchset and tested the result a >> bit. >> >> Most of the stuff still works fine without those offending changes, >> so >> to >> avoid >> mailing around unrelated and already reviewed patches, I request the >> include >> the following 17 patches into drm-next. >> >> If you prefer to merge they are also available from >> git://people.freedesktop.org/~deathsimple/linux branch reset-rework. >> >> Cheers, >> Christian. >> > I am ok with this 17 patchset, i just sent 3 patch on top of those 17 > that > bring back some other of the previous cleanup.
At least for now those three are NAK, cause I just realized we need to put those on top of a more sophisticated fence implementation.
Your idea of not using a list, but 64 bit sequences instead actually sounds quite nifty to me. Going to hack something together in the next couple of hours.
Christian.
Btw you said that you are having issue when using multiple ring, it comes to my attention that you never sync with the GFX ring (unless asked by userspace) that's wrong, before scheduling on another ring than GFX index you need to emit semaphore to make the ring wait for the last emited fence on the GFX ring because of ttm. What might happen is that ttm scheduled bo move on the GFX ring and that the ring you work on start using those bo at there soon to be GPU offset while the bo data is not there yet.
Yeah, already stumbled over that but IIRC Alex already solved that issue..
We set the sync_obj to the fence of the move operation, so when there is a move operation in progress we sync to it correctly (at least I hope so).
Christian.
Looking at code doesn't seems ok, yes you use the fence from the move operation but you only emit wait if userspace ask for it, that's wrong.
if (!(p->relocs[i].flags& RADEON_RELOC_DONT_SYNC)) {
The default is to sync all the rings. We only skip the sync if the _DONT_ sync flag is set.
Yeah, but the _DONT_ sync flag is just an optimization, and we only want to not sync to things userspace has submitted.
E.g. userspace tells us that two jobs can happily run on the same bo even if they are both writing to it....
So Jerome is completely right, userspace doesn't expect that TTM is jumping in between and moving the bo around, that is indeed a bug and another thing for the todo list.
Christian.
Well until userspace can tell kernel on which fence it wants to wait i believe this flags become obsolete and can be remove, i am pretty no release userspace code ever used that flags.
Cheers, Jerome
On 03.05.2012 19:34, Jerome Glisse wrote:
On Thu, May 3, 2012 at 1:28 PM, Christian Königdeathsimple@vodafone.de wrote:
On 03.05.2012 19:20, Alex Deucher wrote:
2012/5/3 Jerome Glissej.glisse@gmail.com:
On Thu, May 3, 2012 at 12:39 PM, Christian König deathsimple@vodafone.de wrote:
On 03.05.2012 18:32, Jerome Glisse wrote:
On Thu, May 3, 2012 at 4:19 AM, Christian Königdeathsimple@vodafone.de wrote: > On 02.05.2012 18:01, Jerome Glisse wrote: >> On Wed, May 2, 2012 at 9:11 AM, Christian >> Königdeathsimple@vodafone.de >> wrote: >>> Hi Dave, >>> >>> there still seems to be the need for some further discussion about >>> the >>> SA >>> code, >>> so I again split that out of the patchset and tested the result a >>> bit. >>> >>> Most of the stuff still works fine without those offending changes, >>> so >>> to >>> avoid >>> mailing around unrelated and already reviewed patches, I request the >>> include >>> the following 17 patches into drm-next. >>> >>> If you prefer to merge they are also available from >>> git://people.freedesktop.org/~deathsimple/linux branch reset-rework. >>> >>> Cheers, >>> Christian. >>> >> I am ok with this 17 patchset, i just sent 3 patch on top of those 17 >> that >> bring back some other of the previous cleanup. > At least for now those three are NAK, cause I just realized we need to > put > those on top of a more sophisticated fence implementation. > > Your idea of not using a list, but 64 bit sequences instead actually > sounds > quite nifty to me. Going to hack something together in the next couple > of > hours. > > Christian. Btw you said that you are having issue when using multiple ring, it comes to my attention that you never sync with the GFX ring (unless asked by userspace) that's wrong, before scheduling on another ring than GFX index you need to emit semaphore to make the ring wait for the last emited fence on the GFX ring because of ttm. What might happen is that ttm scheduled bo move on the GFX ring and that the ring you work on start using those bo at there soon to be GPU offset while the bo data is not there yet.
Yeah, already stumbled over that but IIRC Alex already solved that issue..
We set the sync_obj to the fence of the move operation, so when there is a move operation in progress we sync to it correctly (at least I hope so).
Christian.
Looking at code doesn't seems ok, yes you use the fence from the move operation but you only emit wait if userspace ask for it, that's wrong.
if (!(p->relocs[i].flags& RADEON_RELOC_DONT_SYNC)) {
The default is to sync all the rings. We only skip the sync if the _DONT_ sync flag is set.
Yeah, but the _DONT_ sync flag is just an optimization, and we only want to not sync to things userspace has submitted.
E.g. userspace tells us that two jobs can happily run on the same bo even if they are both writing to it....
So Jerome is completely right, userspace doesn't expect that TTM is jumping in between and moving the bo around, that is indeed a bug and another thing for the todo list.
Christian.
Well until userspace can tell kernel on which fence it wants to wait i believe this flags become obsolete and can be remove, i am pretty no release userspace code ever used that flags.
Agree, AFAIK it currently isn't used anyway.
Christian.
2012/5/3 Christian König deathsimple@vodafone.de:
On 03.05.2012 19:20, Alex Deucher wrote:
2012/5/3 Jerome Glissej.glisse@gmail.com:
On Thu, May 3, 2012 at 12:39 PM, Christian König deathsimple@vodafone.de wrote:
On 03.05.2012 18:32, Jerome Glisse wrote:
On Thu, May 3, 2012 at 4:19 AM, Christian Königdeathsimple@vodafone.de wrote:
On 02.05.2012 18:01, Jerome Glisse wrote: > > On Wed, May 2, 2012 at 9:11 AM, Christian > Königdeathsimple@vodafone.de > wrote: >> >> Hi Dave, >> >> there still seems to be the need for some further discussion about >> the >> SA >> code, >> so I again split that out of the patchset and tested the result a >> bit. >> >> Most of the stuff still works fine without those offending changes, >> so >> to >> avoid >> mailing around unrelated and already reviewed patches, I request the >> include >> the following 17 patches into drm-next. >> >> If you prefer to merge they are also available from >> git://people.freedesktop.org/~deathsimple/linux branch reset-rework. >> >> Cheers, >> Christian. >> > I am ok with this 17 patchset, i just sent 3 patch on top of those 17 > that > bring back some other of the previous cleanup.
At least for now those three are NAK, cause I just realized we need to put those on top of a more sophisticated fence implementation.
Your idea of not using a list, but 64 bit sequences instead actually sounds quite nifty to me. Going to hack something together in the next couple of hours.
Christian.
Btw you said that you are having issue when using multiple ring, it comes to my attention that you never sync with the GFX ring (unless asked by userspace) that's wrong, before scheduling on another ring than GFX index you need to emit semaphore to make the ring wait for the last emited fence on the GFX ring because of ttm. What might happen is that ttm scheduled bo move on the GFX ring and that the ring you work on start using those bo at there soon to be GPU offset while the bo data is not there yet.
Yeah, already stumbled over that but IIRC Alex already solved that issue..
We set the sync_obj to the fence of the move operation, so when there is a move operation in progress we sync to it correctly (at least I hope so).
Christian.
Looking at code doesn't seems ok, yes you use the fence from the move operation but you only emit wait if userspace ask for it, that's wrong.
if (!(p->relocs[i].flags& RADEON_RELOC_DONT_SYNC)) {
The default is to sync all the rings. We only skip the sync if the _DONT_ sync flag is set.
Yeah, but the _DONT_ sync flag is just an optimization, and we only want to not sync to things userspace has submitted.
E.g. userspace tells us that two jobs can happily run on the same bo even if they are both writing to it....
So Jerome is completely right, userspace doesn't expect that TTM is jumping in between and moving the bo around, that is indeed a bug and another thing for the todo list.
Ah, yes, that makes sense. I misinterpreted the original comment.
Alex
there still seems to be the need for some further discussion about the SA code, so I again split that out of the patchset and tested the result a bit.
Most of the stuff still works fine without those offending changes, so to avoid mailing around unrelated and already reviewed patches, I request the include the following 17 patches into drm-next.
Okay I've merged these 17 patches into drm-next.
Thanks, Dave,
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Christian König -
Dave Airlie -
Jerome Glisse