The saved GPU hang state is published by vc4_save_hang_state() and claimed by VC4_GET_HANG_STATE, and access to the pointer was serialized by job_lock. However, job_lock already has other attributions related with the jobs currently executed on hardware.
The hang state has no invariant in common with the jobs programmed into the hardware. vc4_save_hang_state() already drops job_lock between reading those jobs and publishing the dump, so the two critical sections are independent and nothing is made less atomic by separating them. Give the hang state its own spinlock. The two locks are never nested, so this introduces no ordering requirement, and neither hang state path has to disable interrupts any more. Signed-off-by: Maíra Canal <[email protected]> --- drivers/gpu/drm/vc4/vc4_drv.h | 2 ++ drivers/gpu/drm/vc4/vc4_gem.c | 16 ++++++++-------- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index 2fad8a489558..c6ae54d2e8b8 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -99,6 +99,8 @@ struct vc4_dev { struct vc4_hvs *hvs; struct vc4_v3d *v3d; + /* Protects @hang_state. */ + spinlock_t hang_state_lock; struct vc4_hang_state *hang_state; /* The kernel-space BO cache. Tracks buffers that have been diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c index 1f3cb3c2c7fc..230558b3f684 100644 --- a/drivers/gpu/drm/vc4/vc4_gem.c +++ b/drivers/gpu/drm/vc4/vc4_gem.c @@ -74,7 +74,6 @@ vc4_get_hang_state_ioctl(struct drm_device *dev, void *data, struct vc4_hang_state *kernel_state; struct drm_vc4_get_hang_state *state; struct vc4_dev *vc4 = to_vc4_dev(dev); - unsigned long irqflags; u32 i; int ret = 0; @@ -86,10 +85,10 @@ vc4_get_hang_state_ioctl(struct drm_device *dev, void *data, return -ENODEV; } - spin_lock_irqsave(&vc4->job_lock, irqflags); + spin_lock(&vc4->hang_state_lock); kernel_state = vc4->hang_state; if (!kernel_state) { - spin_unlock_irqrestore(&vc4->job_lock, irqflags); + spin_unlock(&vc4->hang_state_lock); return -ENOENT; } state = &kernel_state->user_state; @@ -99,12 +98,12 @@ vc4_get_hang_state_ioctl(struct drm_device *dev, void *data, */ if (get_state->bo_count < state->bo_count) { get_state->bo_count = state->bo_count; - spin_unlock_irqrestore(&vc4->job_lock, irqflags); + spin_unlock(&vc4->hang_state_lock); return 0; } vc4->hang_state = NULL; - spin_unlock_irqrestore(&vc4->job_lock, irqflags); + spin_unlock(&vc4->hang_state_lock); /* Save the user's BO pointer, so we don't stomp it with the memcpy. */ state->bo = get_state->bo; @@ -273,13 +272,13 @@ vc4_save_hang_state(struct drm_device *dev) mutex_unlock(&bo->madv_lock); } - spin_lock_irqsave(&vc4->job_lock, irqflags); + spin_lock(&vc4->hang_state_lock); if (vc4->hang_state) { - spin_unlock_irqrestore(&vc4->job_lock, irqflags); + spin_unlock(&vc4->hang_state_lock); vc4_free_hang_state(dev, kernel_state); } else { vc4->hang_state = kernel_state; - spin_unlock_irqrestore(&vc4->job_lock, irqflags); + spin_unlock(&vc4->hang_state_lock); } return; @@ -1177,6 +1176,7 @@ int vc4_gem_init(struct drm_device *dev) INIT_LIST_HEAD(&vc4->render_job_list); INIT_LIST_HEAD(&vc4->job_done_list); spin_lock_init(&vc4->job_lock); + spin_lock_init(&vc4->hang_state_lock); spin_lock_init(&vc4->perfmon_state.lock); INIT_WORK(&vc4->hangcheck.reset_work, vc4_reset_work); -- 2.55.0
