* [PATCH 1/4] drm/panthor: Force an immediate reset on unrecoverable faults
2024-05-02 18:38 [PATCH 0/4] drm/panthor: More reset fixes Boris Brezillon
@ 2024-05-02 18:38 ` Boris Brezillon
2024-05-03 9:21 ` Steven Price
2024-05-03 11:23 ` Liviu Dudau
2024-05-02 18:38 ` [PATCH 2/4] drm/panthor: Keep a ref to the VM at the panthor_kernel_bo level Boris Brezillon
` (3 subsequent siblings)
4 siblings, 2 replies; 14+ messages in thread
From: Boris Brezillon @ 2024-05-02 18:38 UTC (permalink / raw)
To: Boris Brezillon, Steven Price, Liviu Dudau, Adrián Larumbe
Cc: Christopher Healy, dri-devel, kernel
If the FW reports an unrecoverable fault, we need to reset the GPU
before we can start re-using it again.
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
drivers/gpu/drm/panthor/panthor_device.c | 1 +
drivers/gpu/drm/panthor/panthor_device.h | 1 +
drivers/gpu/drm/panthor/panthor_sched.c | 11 ++++++++++-
3 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
index 75276cbeba20..4c5b54e7abb7 100644
--- a/drivers/gpu/drm/panthor/panthor_device.c
+++ b/drivers/gpu/drm/panthor/panthor_device.c
@@ -293,6 +293,7 @@ static const struct panthor_exception_info panthor_exception_infos[] = {
PANTHOR_EXCEPTION(ACTIVE),
PANTHOR_EXCEPTION(CS_RES_TERM),
PANTHOR_EXCEPTION(CS_CONFIG_FAULT),
+ PANTHOR_EXCEPTION(CS_UNRECOVERABLE),
PANTHOR_EXCEPTION(CS_ENDPOINT_FAULT),
PANTHOR_EXCEPTION(CS_BUS_FAULT),
PANTHOR_EXCEPTION(CS_INSTR_INVALID),
diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
index 2fdd671b38fd..e388c0472ba7 100644
--- a/drivers/gpu/drm/panthor/panthor_device.h
+++ b/drivers/gpu/drm/panthor/panthor_device.h
@@ -216,6 +216,7 @@ enum drm_panthor_exception_type {
DRM_PANTHOR_EXCEPTION_CS_RES_TERM = 0x0f,
DRM_PANTHOR_EXCEPTION_MAX_NON_FAULT = 0x3f,
DRM_PANTHOR_EXCEPTION_CS_CONFIG_FAULT = 0x40,
+ DRM_PANTHOR_EXCEPTION_CS_UNRECOVERABLE = 0x41,
DRM_PANTHOR_EXCEPTION_CS_ENDPOINT_FAULT = 0x44,
DRM_PANTHOR_EXCEPTION_CS_BUS_FAULT = 0x48,
DRM_PANTHOR_EXCEPTION_CS_INSTR_INVALID = 0x49,
diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index 7f16a4a14e9a..1d2708c3ab0a 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -1281,7 +1281,16 @@ cs_slot_process_fatal_event_locked(struct panthor_device *ptdev,
if (group)
group->fatal_queues |= BIT(cs_id);
- sched_queue_delayed_work(sched, tick, 0);
+ if (CS_EXCEPTION_TYPE(fatal) == DRM_PANTHOR_EXCEPTION_CS_UNRECOVERABLE) {
+ /* If this exception is unrecoverable, queue a reset, and make
+ * sure we stop scheduling groups until the reset has happened.
+ */
+ panthor_device_schedule_reset(ptdev);
+ cancel_delayed_work(&sched->tick_work);
+ } else {
+ sched_queue_delayed_work(sched, tick, 0);
+ }
+
drm_warn(&ptdev->base,
"CSG slot %d CS slot: %d\n"
"CS_FATAL.EXCEPTION_TYPE: 0x%x (%s)\n"
--
2.44.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 1/4] drm/panthor: Force an immediate reset on unrecoverable faults
2024-05-02 18:38 ` [PATCH 1/4] drm/panthor: Force an immediate reset on unrecoverable faults Boris Brezillon
@ 2024-05-03 9:21 ` Steven Price
2024-05-03 11:23 ` Liviu Dudau
1 sibling, 0 replies; 14+ messages in thread
From: Steven Price @ 2024-05-03 9:21 UTC (permalink / raw)
To: Boris Brezillon, Liviu Dudau, Adrián Larumbe
Cc: Christopher Healy, dri-devel, kernel
On 02/05/2024 19:38, Boris Brezillon wrote:
> If the FW reports an unrecoverable fault, we need to reset the GPU
> before we can start re-using it again.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Steven Price <steven.price@arm.com>
> ---
> drivers/gpu/drm/panthor/panthor_device.c | 1 +
> drivers/gpu/drm/panthor/panthor_device.h | 1 +
> drivers/gpu/drm/panthor/panthor_sched.c | 11 ++++++++++-
> 3 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 75276cbeba20..4c5b54e7abb7 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -293,6 +293,7 @@ static const struct panthor_exception_info panthor_exception_infos[] = {
> PANTHOR_EXCEPTION(ACTIVE),
> PANTHOR_EXCEPTION(CS_RES_TERM),
> PANTHOR_EXCEPTION(CS_CONFIG_FAULT),
> + PANTHOR_EXCEPTION(CS_UNRECOVERABLE),
> PANTHOR_EXCEPTION(CS_ENDPOINT_FAULT),
> PANTHOR_EXCEPTION(CS_BUS_FAULT),
> PANTHOR_EXCEPTION(CS_INSTR_INVALID),
> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> index 2fdd671b38fd..e388c0472ba7 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.h
> +++ b/drivers/gpu/drm/panthor/panthor_device.h
> @@ -216,6 +216,7 @@ enum drm_panthor_exception_type {
> DRM_PANTHOR_EXCEPTION_CS_RES_TERM = 0x0f,
> DRM_PANTHOR_EXCEPTION_MAX_NON_FAULT = 0x3f,
> DRM_PANTHOR_EXCEPTION_CS_CONFIG_FAULT = 0x40,
> + DRM_PANTHOR_EXCEPTION_CS_UNRECOVERABLE = 0x41,
> DRM_PANTHOR_EXCEPTION_CS_ENDPOINT_FAULT = 0x44,
> DRM_PANTHOR_EXCEPTION_CS_BUS_FAULT = 0x48,
> DRM_PANTHOR_EXCEPTION_CS_INSTR_INVALID = 0x49,
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 7f16a4a14e9a..1d2708c3ab0a 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -1281,7 +1281,16 @@ cs_slot_process_fatal_event_locked(struct panthor_device *ptdev,
> if (group)
> group->fatal_queues |= BIT(cs_id);
>
> - sched_queue_delayed_work(sched, tick, 0);
> + if (CS_EXCEPTION_TYPE(fatal) == DRM_PANTHOR_EXCEPTION_CS_UNRECOVERABLE) {
> + /* If this exception is unrecoverable, queue a reset, and make
> + * sure we stop scheduling groups until the reset has happened.
> + */
> + panthor_device_schedule_reset(ptdev);
> + cancel_delayed_work(&sched->tick_work);
> + } else {
> + sched_queue_delayed_work(sched, tick, 0);
> + }
> +
> drm_warn(&ptdev->base,
> "CSG slot %d CS slot: %d\n"
> "CS_FATAL.EXCEPTION_TYPE: 0x%x (%s)\n"
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/4] drm/panthor: Force an immediate reset on unrecoverable faults
2024-05-02 18:38 ` [PATCH 1/4] drm/panthor: Force an immediate reset on unrecoverable faults Boris Brezillon
2024-05-03 9:21 ` Steven Price
@ 2024-05-03 11:23 ` Liviu Dudau
1 sibling, 0 replies; 14+ messages in thread
From: Liviu Dudau @ 2024-05-03 11:23 UTC (permalink / raw)
To: Boris Brezillon
Cc: Steven Price, Adrián Larumbe, Christopher Healy, dri-devel,
kernel
On Thu, May 02, 2024 at 08:38:09PM +0200, Boris Brezillon wrote:
> If the FW reports an unrecoverable fault, we need to reset the GPU
> before we can start re-using it again.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> ---
> drivers/gpu/drm/panthor/panthor_device.c | 1 +
> drivers/gpu/drm/panthor/panthor_device.h | 1 +
> drivers/gpu/drm/panthor/panthor_sched.c | 11 ++++++++++-
> 3 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 75276cbeba20..4c5b54e7abb7 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -293,6 +293,7 @@ static const struct panthor_exception_info panthor_exception_infos[] = {
> PANTHOR_EXCEPTION(ACTIVE),
> PANTHOR_EXCEPTION(CS_RES_TERM),
> PANTHOR_EXCEPTION(CS_CONFIG_FAULT),
> + PANTHOR_EXCEPTION(CS_UNRECOVERABLE),
> PANTHOR_EXCEPTION(CS_ENDPOINT_FAULT),
> PANTHOR_EXCEPTION(CS_BUS_FAULT),
> PANTHOR_EXCEPTION(CS_INSTR_INVALID),
> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> index 2fdd671b38fd..e388c0472ba7 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.h
> +++ b/drivers/gpu/drm/panthor/panthor_device.h
> @@ -216,6 +216,7 @@ enum drm_panthor_exception_type {
> DRM_PANTHOR_EXCEPTION_CS_RES_TERM = 0x0f,
> DRM_PANTHOR_EXCEPTION_MAX_NON_FAULT = 0x3f,
> DRM_PANTHOR_EXCEPTION_CS_CONFIG_FAULT = 0x40,
> + DRM_PANTHOR_EXCEPTION_CS_UNRECOVERABLE = 0x41,
> DRM_PANTHOR_EXCEPTION_CS_ENDPOINT_FAULT = 0x44,
> DRM_PANTHOR_EXCEPTION_CS_BUS_FAULT = 0x48,
> DRM_PANTHOR_EXCEPTION_CS_INSTR_INVALID = 0x49,
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 7f16a4a14e9a..1d2708c3ab0a 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -1281,7 +1281,16 @@ cs_slot_process_fatal_event_locked(struct panthor_device *ptdev,
> if (group)
> group->fatal_queues |= BIT(cs_id);
>
> - sched_queue_delayed_work(sched, tick, 0);
> + if (CS_EXCEPTION_TYPE(fatal) == DRM_PANTHOR_EXCEPTION_CS_UNRECOVERABLE) {
> + /* If this exception is unrecoverable, queue a reset, and make
> + * sure we stop scheduling groups until the reset has happened.
> + */
> + panthor_device_schedule_reset(ptdev);
> + cancel_delayed_work(&sched->tick_work);
> + } else {
> + sched_queue_delayed_work(sched, tick, 0);
> + }
> +
> drm_warn(&ptdev->base,
> "CSG slot %d CS slot: %d\n"
> "CS_FATAL.EXCEPTION_TYPE: 0x%x (%s)\n"
> --
> 2.44.0
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/4] drm/panthor: Keep a ref to the VM at the panthor_kernel_bo level
2024-05-02 18:38 [PATCH 0/4] drm/panthor: More reset fixes Boris Brezillon
2024-05-02 18:38 ` [PATCH 1/4] drm/panthor: Force an immediate reset on unrecoverable faults Boris Brezillon
@ 2024-05-02 18:38 ` Boris Brezillon
2024-05-03 9:22 ` Steven Price
2024-05-03 11:46 ` Liviu Dudau
2024-05-02 18:38 ` [PATCH 3/4] drm/panthor: Reset the FW VM to NULL on unplug Boris Brezillon
` (2 subsequent siblings)
4 siblings, 2 replies; 14+ messages in thread
From: Boris Brezillon @ 2024-05-02 18:38 UTC (permalink / raw)
To: Boris Brezillon, Steven Price, Liviu Dudau, Adrián Larumbe
Cc: Christopher Healy, dri-devel, kernel
Avoids use-after-free situations when panthor_fw_unplug() is called
and the kernel BO was mapped to the FW VM.
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
drivers/gpu/drm/panthor/panthor_fw.c | 4 ++--
drivers/gpu/drm/panthor/panthor_gem.c | 8 +++++---
drivers/gpu/drm/panthor/panthor_gem.h | 8 ++++++--
drivers/gpu/drm/panthor/panthor_heap.c | 8 ++++----
drivers/gpu/drm/panthor/panthor_sched.c | 11 +++++------
5 files changed, 22 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
index 181395e2859a..b41685304a83 100644
--- a/drivers/gpu/drm/panthor/panthor_fw.c
+++ b/drivers/gpu/drm/panthor/panthor_fw.c
@@ -453,7 +453,7 @@ panthor_fw_alloc_queue_iface_mem(struct panthor_device *ptdev,
ret = panthor_kernel_bo_vmap(mem);
if (ret) {
- panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), mem);
+ panthor_kernel_bo_destroy(mem);
return ERR_PTR(ret);
}
@@ -1133,7 +1133,7 @@ void panthor_fw_unplug(struct panthor_device *ptdev)
panthor_fw_stop(ptdev);
list_for_each_entry(section, &ptdev->fw->sections, node)
- panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), section->mem);
+ panthor_kernel_bo_destroy(section->mem);
/* We intentionally don't call panthor_vm_idle() and let
* panthor_mmu_unplug() release the AS we acquired with
diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
index d6483266d0c2..38f560864879 100644
--- a/drivers/gpu/drm/panthor/panthor_gem.c
+++ b/drivers/gpu/drm/panthor/panthor_gem.c
@@ -26,18 +26,18 @@ static void panthor_gem_free_object(struct drm_gem_object *obj)
/**
* panthor_kernel_bo_destroy() - Destroy a kernel buffer object
- * @vm: The VM this BO was mapped to.
* @bo: Kernel buffer object to destroy. If NULL or an ERR_PTR(), the destruction
* is skipped.
*/
-void panthor_kernel_bo_destroy(struct panthor_vm *vm,
- struct panthor_kernel_bo *bo)
+void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo)
{
+ struct panthor_vm *vm;
int ret;
if (IS_ERR_OR_NULL(bo))
return;
+ vm = bo->vm;
panthor_kernel_bo_vunmap(bo);
if (drm_WARN_ON(bo->obj->dev,
@@ -53,6 +53,7 @@ void panthor_kernel_bo_destroy(struct panthor_vm *vm,
drm_gem_object_put(bo->obj);
out_free_bo:
+ panthor_vm_put(vm);
kfree(bo);
}
@@ -106,6 +107,7 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm,
if (ret)
goto err_free_va;
+ kbo->vm = panthor_vm_get(vm);
bo->exclusive_vm_root_gem = panthor_vm_root_gem(vm);
drm_gem_object_get(bo->exclusive_vm_root_gem);
bo->base.base.resv = bo->exclusive_vm_root_gem->resv;
diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h
index 3bccba394d00..e43021cf6d45 100644
--- a/drivers/gpu/drm/panthor/panthor_gem.h
+++ b/drivers/gpu/drm/panthor/panthor_gem.h
@@ -61,6 +61,11 @@ struct panthor_kernel_bo {
*/
struct drm_gem_object *obj;
+ /**
+ * @vm: VM this private buffer is attached to.
+ */
+ struct panthor_vm *vm;
+
/**
* @va_node: VA space allocated to this GEM.
*/
@@ -136,7 +141,6 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm,
size_t size, u32 bo_flags, u32 vm_map_flags,
u64 gpu_va);
-void panthor_kernel_bo_destroy(struct panthor_vm *vm,
- struct panthor_kernel_bo *bo);
+void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo);
#endif /* __PANTHOR_GEM_H__ */
diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c
index 143fa35f2e74..65921296a18c 100644
--- a/drivers/gpu/drm/panthor/panthor_heap.c
+++ b/drivers/gpu/drm/panthor/panthor_heap.c
@@ -127,7 +127,7 @@ static void panthor_free_heap_chunk(struct panthor_vm *vm,
heap->chunk_count--;
mutex_unlock(&heap->lock);
- panthor_kernel_bo_destroy(vm, chunk->bo);
+ panthor_kernel_bo_destroy(chunk->bo);
kfree(chunk);
}
@@ -183,7 +183,7 @@ static int panthor_alloc_heap_chunk(struct panthor_device *ptdev,
return 0;
err_destroy_bo:
- panthor_kernel_bo_destroy(vm, chunk->bo);
+ panthor_kernel_bo_destroy(chunk->bo);
err_free_chunk:
kfree(chunk);
@@ -391,7 +391,7 @@ int panthor_heap_return_chunk(struct panthor_heap_pool *pool,
mutex_unlock(&heap->lock);
if (removed) {
- panthor_kernel_bo_destroy(pool->vm, chunk->bo);
+ panthor_kernel_bo_destroy(chunk->bo);
kfree(chunk);
ret = 0;
} else {
@@ -587,7 +587,7 @@ void panthor_heap_pool_destroy(struct panthor_heap_pool *pool)
drm_WARN_ON(&pool->ptdev->base, panthor_heap_destroy_locked(pool, i));
if (!IS_ERR_OR_NULL(pool->gpu_contexts))
- panthor_kernel_bo_destroy(pool->vm, pool->gpu_contexts);
+ panthor_kernel_bo_destroy(pool->gpu_contexts);
/* Reflects the fact the pool has been destroyed. */
pool->vm = NULL;
diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index 1d2708c3ab0a..6ea094b00cf9 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -826,8 +826,8 @@ static void group_free_queue(struct panthor_group *group, struct panthor_queue *
panthor_queue_put_syncwait_obj(queue);
- panthor_kernel_bo_destroy(group->vm, queue->ringbuf);
- panthor_kernel_bo_destroy(panthor_fw_vm(group->ptdev), queue->iface.mem);
+ panthor_kernel_bo_destroy(queue->ringbuf);
+ panthor_kernel_bo_destroy(queue->iface.mem);
kfree(queue);
}
@@ -837,15 +837,14 @@ static void group_release_work(struct work_struct *work)
struct panthor_group *group = container_of(work,
struct panthor_group,
release_work);
- struct panthor_device *ptdev = group->ptdev;
u32 i;
for (i = 0; i < group->queue_count; i++)
group_free_queue(group, group->queues[i]);
- panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), group->suspend_buf);
- panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), group->protm_suspend_buf);
- panthor_kernel_bo_destroy(group->vm, group->syncobjs);
+ panthor_kernel_bo_destroy(group->suspend_buf);
+ panthor_kernel_bo_destroy(group->protm_suspend_buf);
+ panthor_kernel_bo_destroy(group->syncobjs);
panthor_vm_put(group->vm);
kfree(group);
--
2.44.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 2/4] drm/panthor: Keep a ref to the VM at the panthor_kernel_bo level
2024-05-02 18:38 ` [PATCH 2/4] drm/panthor: Keep a ref to the VM at the panthor_kernel_bo level Boris Brezillon
@ 2024-05-03 9:22 ` Steven Price
2024-05-03 11:46 ` Liviu Dudau
1 sibling, 0 replies; 14+ messages in thread
From: Steven Price @ 2024-05-03 9:22 UTC (permalink / raw)
To: Boris Brezillon, Liviu Dudau, Adrián Larumbe
Cc: Christopher Healy, dri-devel, kernel
On 02/05/2024 19:38, Boris Brezillon wrote:
> Avoids use-after-free situations when panthor_fw_unplug() is called
> and the kernel BO was mapped to the FW VM.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
It makes the code more readable too - I like it.
Reviewed-by: Steven Price <steven.price@arm.com>
> ---
> drivers/gpu/drm/panthor/panthor_fw.c | 4 ++--
> drivers/gpu/drm/panthor/panthor_gem.c | 8 +++++---
> drivers/gpu/drm/panthor/panthor_gem.h | 8 ++++++--
> drivers/gpu/drm/panthor/panthor_heap.c | 8 ++++----
> drivers/gpu/drm/panthor/panthor_sched.c | 11 +++++------
> 5 files changed, 22 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> index 181395e2859a..b41685304a83 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> @@ -453,7 +453,7 @@ panthor_fw_alloc_queue_iface_mem(struct panthor_device *ptdev,
>
> ret = panthor_kernel_bo_vmap(mem);
> if (ret) {
> - panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), mem);
> + panthor_kernel_bo_destroy(mem);
> return ERR_PTR(ret);
> }
>
> @@ -1133,7 +1133,7 @@ void panthor_fw_unplug(struct panthor_device *ptdev)
> panthor_fw_stop(ptdev);
>
> list_for_each_entry(section, &ptdev->fw->sections, node)
> - panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), section->mem);
> + panthor_kernel_bo_destroy(section->mem);
>
> /* We intentionally don't call panthor_vm_idle() and let
> * panthor_mmu_unplug() release the AS we acquired with
> diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
> index d6483266d0c2..38f560864879 100644
> --- a/drivers/gpu/drm/panthor/panthor_gem.c
> +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> @@ -26,18 +26,18 @@ static void panthor_gem_free_object(struct drm_gem_object *obj)
>
> /**
> * panthor_kernel_bo_destroy() - Destroy a kernel buffer object
> - * @vm: The VM this BO was mapped to.
> * @bo: Kernel buffer object to destroy. If NULL or an ERR_PTR(), the destruction
> * is skipped.
> */
> -void panthor_kernel_bo_destroy(struct panthor_vm *vm,
> - struct panthor_kernel_bo *bo)
> +void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo)
> {
> + struct panthor_vm *vm;
> int ret;
>
> if (IS_ERR_OR_NULL(bo))
> return;
>
> + vm = bo->vm;
> panthor_kernel_bo_vunmap(bo);
>
> if (drm_WARN_ON(bo->obj->dev,
> @@ -53,6 +53,7 @@ void panthor_kernel_bo_destroy(struct panthor_vm *vm,
> drm_gem_object_put(bo->obj);
>
> out_free_bo:
> + panthor_vm_put(vm);
> kfree(bo);
> }
>
> @@ -106,6 +107,7 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm,
> if (ret)
> goto err_free_va;
>
> + kbo->vm = panthor_vm_get(vm);
> bo->exclusive_vm_root_gem = panthor_vm_root_gem(vm);
> drm_gem_object_get(bo->exclusive_vm_root_gem);
> bo->base.base.resv = bo->exclusive_vm_root_gem->resv;
> diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h
> index 3bccba394d00..e43021cf6d45 100644
> --- a/drivers/gpu/drm/panthor/panthor_gem.h
> +++ b/drivers/gpu/drm/panthor/panthor_gem.h
> @@ -61,6 +61,11 @@ struct panthor_kernel_bo {
> */
> struct drm_gem_object *obj;
>
> + /**
> + * @vm: VM this private buffer is attached to.
> + */
> + struct panthor_vm *vm;
> +
> /**
> * @va_node: VA space allocated to this GEM.
> */
> @@ -136,7 +141,6 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm,
> size_t size, u32 bo_flags, u32 vm_map_flags,
> u64 gpu_va);
>
> -void panthor_kernel_bo_destroy(struct panthor_vm *vm,
> - struct panthor_kernel_bo *bo);
> +void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo);
>
> #endif /* __PANTHOR_GEM_H__ */
> diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c
> index 143fa35f2e74..65921296a18c 100644
> --- a/drivers/gpu/drm/panthor/panthor_heap.c
> +++ b/drivers/gpu/drm/panthor/panthor_heap.c
> @@ -127,7 +127,7 @@ static void panthor_free_heap_chunk(struct panthor_vm *vm,
> heap->chunk_count--;
> mutex_unlock(&heap->lock);
>
> - panthor_kernel_bo_destroy(vm, chunk->bo);
> + panthor_kernel_bo_destroy(chunk->bo);
> kfree(chunk);
> }
>
> @@ -183,7 +183,7 @@ static int panthor_alloc_heap_chunk(struct panthor_device *ptdev,
> return 0;
>
> err_destroy_bo:
> - panthor_kernel_bo_destroy(vm, chunk->bo);
> + panthor_kernel_bo_destroy(chunk->bo);
>
> err_free_chunk:
> kfree(chunk);
> @@ -391,7 +391,7 @@ int panthor_heap_return_chunk(struct panthor_heap_pool *pool,
> mutex_unlock(&heap->lock);
>
> if (removed) {
> - panthor_kernel_bo_destroy(pool->vm, chunk->bo);
> + panthor_kernel_bo_destroy(chunk->bo);
> kfree(chunk);
> ret = 0;
> } else {
> @@ -587,7 +587,7 @@ void panthor_heap_pool_destroy(struct panthor_heap_pool *pool)
> drm_WARN_ON(&pool->ptdev->base, panthor_heap_destroy_locked(pool, i));
>
> if (!IS_ERR_OR_NULL(pool->gpu_contexts))
> - panthor_kernel_bo_destroy(pool->vm, pool->gpu_contexts);
> + panthor_kernel_bo_destroy(pool->gpu_contexts);
>
> /* Reflects the fact the pool has been destroyed. */
> pool->vm = NULL;
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 1d2708c3ab0a..6ea094b00cf9 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -826,8 +826,8 @@ static void group_free_queue(struct panthor_group *group, struct panthor_queue *
>
> panthor_queue_put_syncwait_obj(queue);
>
> - panthor_kernel_bo_destroy(group->vm, queue->ringbuf);
> - panthor_kernel_bo_destroy(panthor_fw_vm(group->ptdev), queue->iface.mem);
> + panthor_kernel_bo_destroy(queue->ringbuf);
> + panthor_kernel_bo_destroy(queue->iface.mem);
>
> kfree(queue);
> }
> @@ -837,15 +837,14 @@ static void group_release_work(struct work_struct *work)
> struct panthor_group *group = container_of(work,
> struct panthor_group,
> release_work);
> - struct panthor_device *ptdev = group->ptdev;
> u32 i;
>
> for (i = 0; i < group->queue_count; i++)
> group_free_queue(group, group->queues[i]);
>
> - panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), group->suspend_buf);
> - panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), group->protm_suspend_buf);
> - panthor_kernel_bo_destroy(group->vm, group->syncobjs);
> + panthor_kernel_bo_destroy(group->suspend_buf);
> + panthor_kernel_bo_destroy(group->protm_suspend_buf);
> + panthor_kernel_bo_destroy(group->syncobjs);
>
> panthor_vm_put(group->vm);
> kfree(group);
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 2/4] drm/panthor: Keep a ref to the VM at the panthor_kernel_bo level
2024-05-02 18:38 ` [PATCH 2/4] drm/panthor: Keep a ref to the VM at the panthor_kernel_bo level Boris Brezillon
2024-05-03 9:22 ` Steven Price
@ 2024-05-03 11:46 ` Liviu Dudau
1 sibling, 0 replies; 14+ messages in thread
From: Liviu Dudau @ 2024-05-03 11:46 UTC (permalink / raw)
To: Boris Brezillon
Cc: Steven Price, Adrián Larumbe, Christopher Healy, dri-devel,
kernel
On Thu, May 02, 2024 at 08:38:10PM +0200, Boris Brezillon wrote:
> Avoids use-after-free situations when panthor_fw_unplug() is called
> and the kernel BO was mapped to the FW VM.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> ---
> drivers/gpu/drm/panthor/panthor_fw.c | 4 ++--
> drivers/gpu/drm/panthor/panthor_gem.c | 8 +++++---
> drivers/gpu/drm/panthor/panthor_gem.h | 8 ++++++--
> drivers/gpu/drm/panthor/panthor_heap.c | 8 ++++----
> drivers/gpu/drm/panthor/panthor_sched.c | 11 +++++------
> 5 files changed, 22 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> index 181395e2859a..b41685304a83 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> @@ -453,7 +453,7 @@ panthor_fw_alloc_queue_iface_mem(struct panthor_device *ptdev,
>
> ret = panthor_kernel_bo_vmap(mem);
> if (ret) {
> - panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), mem);
> + panthor_kernel_bo_destroy(mem);
> return ERR_PTR(ret);
> }
>
> @@ -1133,7 +1133,7 @@ void panthor_fw_unplug(struct panthor_device *ptdev)
> panthor_fw_stop(ptdev);
>
> list_for_each_entry(section, &ptdev->fw->sections, node)
> - panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), section->mem);
> + panthor_kernel_bo_destroy(section->mem);
>
> /* We intentionally don't call panthor_vm_idle() and let
> * panthor_mmu_unplug() release the AS we acquired with
> diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
> index d6483266d0c2..38f560864879 100644
> --- a/drivers/gpu/drm/panthor/panthor_gem.c
> +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> @@ -26,18 +26,18 @@ static void panthor_gem_free_object(struct drm_gem_object *obj)
>
> /**
> * panthor_kernel_bo_destroy() - Destroy a kernel buffer object
> - * @vm: The VM this BO was mapped to.
> * @bo: Kernel buffer object to destroy. If NULL or an ERR_PTR(), the destruction
> * is skipped.
> */
> -void panthor_kernel_bo_destroy(struct panthor_vm *vm,
> - struct panthor_kernel_bo *bo)
> +void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo)
> {
> + struct panthor_vm *vm;
> int ret;
>
> if (IS_ERR_OR_NULL(bo))
> return;
>
> + vm = bo->vm;
> panthor_kernel_bo_vunmap(bo);
>
> if (drm_WARN_ON(bo->obj->dev,
> @@ -53,6 +53,7 @@ void panthor_kernel_bo_destroy(struct panthor_vm *vm,
> drm_gem_object_put(bo->obj);
>
> out_free_bo:
> + panthor_vm_put(vm);
> kfree(bo);
> }
>
> @@ -106,6 +107,7 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm,
> if (ret)
> goto err_free_va;
>
> + kbo->vm = panthor_vm_get(vm);
> bo->exclusive_vm_root_gem = panthor_vm_root_gem(vm);
> drm_gem_object_get(bo->exclusive_vm_root_gem);
> bo->base.base.resv = bo->exclusive_vm_root_gem->resv;
> diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h
> index 3bccba394d00..e43021cf6d45 100644
> --- a/drivers/gpu/drm/panthor/panthor_gem.h
> +++ b/drivers/gpu/drm/panthor/panthor_gem.h
> @@ -61,6 +61,11 @@ struct panthor_kernel_bo {
> */
> struct drm_gem_object *obj;
>
> + /**
> + * @vm: VM this private buffer is attached to.
> + */
> + struct panthor_vm *vm;
> +
> /**
> * @va_node: VA space allocated to this GEM.
> */
> @@ -136,7 +141,6 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm,
> size_t size, u32 bo_flags, u32 vm_map_flags,
> u64 gpu_va);
>
> -void panthor_kernel_bo_destroy(struct panthor_vm *vm,
> - struct panthor_kernel_bo *bo);
> +void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo);
>
> #endif /* __PANTHOR_GEM_H__ */
> diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c
> index 143fa35f2e74..65921296a18c 100644
> --- a/drivers/gpu/drm/panthor/panthor_heap.c
> +++ b/drivers/gpu/drm/panthor/panthor_heap.c
> @@ -127,7 +127,7 @@ static void panthor_free_heap_chunk(struct panthor_vm *vm,
> heap->chunk_count--;
> mutex_unlock(&heap->lock);
>
> - panthor_kernel_bo_destroy(vm, chunk->bo);
> + panthor_kernel_bo_destroy(chunk->bo);
> kfree(chunk);
> }
>
> @@ -183,7 +183,7 @@ static int panthor_alloc_heap_chunk(struct panthor_device *ptdev,
> return 0;
>
> err_destroy_bo:
> - panthor_kernel_bo_destroy(vm, chunk->bo);
> + panthor_kernel_bo_destroy(chunk->bo);
>
> err_free_chunk:
> kfree(chunk);
> @@ -391,7 +391,7 @@ int panthor_heap_return_chunk(struct panthor_heap_pool *pool,
> mutex_unlock(&heap->lock);
>
> if (removed) {
> - panthor_kernel_bo_destroy(pool->vm, chunk->bo);
> + panthor_kernel_bo_destroy(chunk->bo);
> kfree(chunk);
> ret = 0;
> } else {
> @@ -587,7 +587,7 @@ void panthor_heap_pool_destroy(struct panthor_heap_pool *pool)
> drm_WARN_ON(&pool->ptdev->base, panthor_heap_destroy_locked(pool, i));
>
> if (!IS_ERR_OR_NULL(pool->gpu_contexts))
> - panthor_kernel_bo_destroy(pool->vm, pool->gpu_contexts);
> + panthor_kernel_bo_destroy(pool->gpu_contexts);
>
> /* Reflects the fact the pool has been destroyed. */
> pool->vm = NULL;
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 1d2708c3ab0a..6ea094b00cf9 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -826,8 +826,8 @@ static void group_free_queue(struct panthor_group *group, struct panthor_queue *
>
> panthor_queue_put_syncwait_obj(queue);
>
> - panthor_kernel_bo_destroy(group->vm, queue->ringbuf);
> - panthor_kernel_bo_destroy(panthor_fw_vm(group->ptdev), queue->iface.mem);
> + panthor_kernel_bo_destroy(queue->ringbuf);
> + panthor_kernel_bo_destroy(queue->iface.mem);
>
> kfree(queue);
> }
> @@ -837,15 +837,14 @@ static void group_release_work(struct work_struct *work)
> struct panthor_group *group = container_of(work,
> struct panthor_group,
> release_work);
> - struct panthor_device *ptdev = group->ptdev;
> u32 i;
>
> for (i = 0; i < group->queue_count; i++)
> group_free_queue(group, group->queues[i]);
>
> - panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), group->suspend_buf);
> - panthor_kernel_bo_destroy(panthor_fw_vm(ptdev), group->protm_suspend_buf);
> - panthor_kernel_bo_destroy(group->vm, group->syncobjs);
> + panthor_kernel_bo_destroy(group->suspend_buf);
> + panthor_kernel_bo_destroy(group->protm_suspend_buf);
> + panthor_kernel_bo_destroy(group->syncobjs);
>
> panthor_vm_put(group->vm);
> kfree(group);
> --
> 2.44.0
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/4] drm/panthor: Reset the FW VM to NULL on unplug
2024-05-02 18:38 [PATCH 0/4] drm/panthor: More reset fixes Boris Brezillon
2024-05-02 18:38 ` [PATCH 1/4] drm/panthor: Force an immediate reset on unrecoverable faults Boris Brezillon
2024-05-02 18:38 ` [PATCH 2/4] drm/panthor: Keep a ref to the VM at the panthor_kernel_bo level Boris Brezillon
@ 2024-05-02 18:38 ` Boris Brezillon
2024-05-03 9:22 ` Steven Price
2024-05-02 18:38 ` [PATCH 4/4] drm/panthor: Call panthor_sched_post_reset() even if the reset failed Boris Brezillon
2024-05-13 11:40 ` [PATCH 0/4] drm/panthor: More reset fixes Boris Brezillon
4 siblings, 1 reply; 14+ messages in thread
From: Boris Brezillon @ 2024-05-02 18:38 UTC (permalink / raw)
To: Boris Brezillon, Steven Price, Liviu Dudau, Adrián Larumbe
Cc: Christopher Healy, dri-devel, kernel
This way get NULL derefs instead of use-after-free if the FW VM is
referenced after the device has been unplugged.
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
drivers/gpu/drm/panthor/panthor_fw.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
index b41685304a83..93165961a6b5 100644
--- a/drivers/gpu/drm/panthor/panthor_fw.c
+++ b/drivers/gpu/drm/panthor/panthor_fw.c
@@ -1141,6 +1141,7 @@ void panthor_fw_unplug(struct panthor_device *ptdev)
* state to keep the active_refcnt balanced.
*/
panthor_vm_put(ptdev->fw->vm);
+ ptdev->fw->vm = NULL;
panthor_gpu_power_off(ptdev, L2, ptdev->gpu_info.l2_present, 20000);
}
--
2.44.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] drm/panthor: Reset the FW VM to NULL on unplug
2024-05-02 18:38 ` [PATCH 3/4] drm/panthor: Reset the FW VM to NULL on unplug Boris Brezillon
@ 2024-05-03 9:22 ` Steven Price
2024-05-03 11:47 ` Liviu Dudau
0 siblings, 1 reply; 14+ messages in thread
From: Steven Price @ 2024-05-03 9:22 UTC (permalink / raw)
To: Boris Brezillon, Liviu Dudau, Adrián Larumbe
Cc: Christopher Healy, dri-devel, kernel
On 02/05/2024 19:38, Boris Brezillon wrote:
> This way get NULL derefs instead of use-after-free if the FW VM is
> referenced after the device has been unplugged.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Steven Price <steven.price@arm.com>
> ---
> drivers/gpu/drm/panthor/panthor_fw.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> index b41685304a83..93165961a6b5 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> @@ -1141,6 +1141,7 @@ void panthor_fw_unplug(struct panthor_device *ptdev)
> * state to keep the active_refcnt balanced.
> */
> panthor_vm_put(ptdev->fw->vm);
> + ptdev->fw->vm = NULL;
>
> panthor_gpu_power_off(ptdev, L2, ptdev->gpu_info.l2_present, 20000);
> }
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] drm/panthor: Reset the FW VM to NULL on unplug
2024-05-03 9:22 ` Steven Price
@ 2024-05-03 11:47 ` Liviu Dudau
0 siblings, 0 replies; 14+ messages in thread
From: Liviu Dudau @ 2024-05-03 11:47 UTC (permalink / raw)
To: Steven Price
Cc: Boris Brezillon, Adrián Larumbe, Christopher Healy,
dri-devel, kernel
On Fri, May 03, 2024 at 10:22:13AM +0100, Steven Price wrote:
> On 02/05/2024 19:38, Boris Brezillon wrote:
> > This way get NULL derefs instead of use-after-free if the FW VM is
> > referenced after the device has been unplugged.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
>
> Reviewed-by: Steven Price <steven.price@arm.com>
Acked-by: Liviu Dudau <liviu.dudau@arm.com>
>
> > ---
> > drivers/gpu/drm/panthor/panthor_fw.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> > index b41685304a83..93165961a6b5 100644
> > --- a/drivers/gpu/drm/panthor/panthor_fw.c
> > +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> > @@ -1141,6 +1141,7 @@ void panthor_fw_unplug(struct panthor_device *ptdev)
> > * state to keep the active_refcnt balanced.
> > */
> > panthor_vm_put(ptdev->fw->vm);
> > + ptdev->fw->vm = NULL;
> >
> > panthor_gpu_power_off(ptdev, L2, ptdev->gpu_info.l2_present, 20000);
> > }
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/4] drm/panthor: Call panthor_sched_post_reset() even if the reset failed
2024-05-02 18:38 [PATCH 0/4] drm/panthor: More reset fixes Boris Brezillon
` (2 preceding siblings ...)
2024-05-02 18:38 ` [PATCH 3/4] drm/panthor: Reset the FW VM to NULL on unplug Boris Brezillon
@ 2024-05-02 18:38 ` Boris Brezillon
2024-05-03 9:22 ` Steven Price
2024-05-03 11:49 ` Liviu Dudau
2024-05-13 11:40 ` [PATCH 0/4] drm/panthor: More reset fixes Boris Brezillon
4 siblings, 2 replies; 14+ messages in thread
From: Boris Brezillon @ 2024-05-02 18:38 UTC (permalink / raw)
To: Boris Brezillon, Steven Price, Liviu Dudau, Adrián Larumbe
Cc: Christopher Healy, dri-devel, kernel
We need to undo what was done in panthor_sched_pre_reset() even if the
reset failed. We just flag all previously running groups as terminated
when that happens to unblock things.
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
drivers/gpu/drm/panthor/panthor_device.c | 7 +------
drivers/gpu/drm/panthor/panthor_sched.c | 19 ++++++++++++++-----
drivers/gpu/drm/panthor/panthor_sched.h | 2 +-
3 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
index 4c5b54e7abb7..4082c8f2951d 100644
--- a/drivers/gpu/drm/panthor/panthor_device.c
+++ b/drivers/gpu/drm/panthor/panthor_device.c
@@ -129,13 +129,8 @@ static void panthor_device_reset_work(struct work_struct *work)
panthor_gpu_l2_power_on(ptdev);
panthor_mmu_post_reset(ptdev);
ret = panthor_fw_post_reset(ptdev);
- if (ret)
- goto out_dev_exit;
-
atomic_set(&ptdev->reset.pending, 0);
- panthor_sched_post_reset(ptdev);
-
-out_dev_exit:
+ panthor_sched_post_reset(ptdev, ret != 0);
drm_dev_exit(cookie);
if (ret) {
diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index 6ea094b00cf9..fc43ff62c77d 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -2728,15 +2728,22 @@ void panthor_sched_pre_reset(struct panthor_device *ptdev)
mutex_unlock(&sched->reset.lock);
}
-void panthor_sched_post_reset(struct panthor_device *ptdev)
+void panthor_sched_post_reset(struct panthor_device *ptdev, bool reset_failed)
{
struct panthor_scheduler *sched = ptdev->scheduler;
struct panthor_group *group, *group_tmp;
mutex_lock(&sched->reset.lock);
- list_for_each_entry_safe(group, group_tmp, &sched->reset.stopped_groups, run_node)
+ list_for_each_entry_safe(group, group_tmp, &sched->reset.stopped_groups, run_node) {
+ /* Consider all previously running group as terminated if the
+ * reset failed.
+ */
+ if (reset_failed)
+ group->state = PANTHOR_CS_GROUP_TERMINATED;
+
panthor_group_start(group);
+ }
/* We're done resetting the GPU, clear the reset.in_progress bit so we can
* kick the scheduler.
@@ -2744,9 +2751,11 @@ void panthor_sched_post_reset(struct panthor_device *ptdev)
atomic_set(&sched->reset.in_progress, false);
mutex_unlock(&sched->reset.lock);
- sched_queue_delayed_work(sched, tick, 0);
-
- sched_queue_work(sched, sync_upd);
+ /* No need to queue a tick and update syncs if the reset failed. */
+ if (!reset_failed) {
+ sched_queue_delayed_work(sched, tick, 0);
+ sched_queue_work(sched, sync_upd);
+ }
}
static void group_sync_upd_work(struct work_struct *work)
diff --git a/drivers/gpu/drm/panthor/panthor_sched.h b/drivers/gpu/drm/panthor/panthor_sched.h
index 66438b1f331f..3a30d2328b30 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.h
+++ b/drivers/gpu/drm/panthor/panthor_sched.h
@@ -40,7 +40,7 @@ void panthor_group_pool_destroy(struct panthor_file *pfile);
int panthor_sched_init(struct panthor_device *ptdev);
void panthor_sched_unplug(struct panthor_device *ptdev);
void panthor_sched_pre_reset(struct panthor_device *ptdev);
-void panthor_sched_post_reset(struct panthor_device *ptdev);
+void panthor_sched_post_reset(struct panthor_device *ptdev, bool reset_failed);
void panthor_sched_suspend(struct panthor_device *ptdev);
void panthor_sched_resume(struct panthor_device *ptdev);
--
2.44.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 4/4] drm/panthor: Call panthor_sched_post_reset() even if the reset failed
2024-05-02 18:38 ` [PATCH 4/4] drm/panthor: Call panthor_sched_post_reset() even if the reset failed Boris Brezillon
@ 2024-05-03 9:22 ` Steven Price
2024-05-03 11:49 ` Liviu Dudau
1 sibling, 0 replies; 14+ messages in thread
From: Steven Price @ 2024-05-03 9:22 UTC (permalink / raw)
To: Boris Brezillon, Liviu Dudau, Adrián Larumbe
Cc: Christopher Healy, dri-devel, kernel
On 02/05/2024 19:38, Boris Brezillon wrote:
> We need to undo what was done in panthor_sched_pre_reset() even if the
> reset failed. We just flag all previously running groups as terminated
> when that happens to unblock things.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Seems reasonable, although I hope this case doesn't happen in practice ;)
Reviewed-by: Steven Price <steven.price@arm.com>
> ---
> drivers/gpu/drm/panthor/panthor_device.c | 7 +------
> drivers/gpu/drm/panthor/panthor_sched.c | 19 ++++++++++++++-----
> drivers/gpu/drm/panthor/panthor_sched.h | 2 +-
> 3 files changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 4c5b54e7abb7..4082c8f2951d 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -129,13 +129,8 @@ static void panthor_device_reset_work(struct work_struct *work)
> panthor_gpu_l2_power_on(ptdev);
> panthor_mmu_post_reset(ptdev);
> ret = panthor_fw_post_reset(ptdev);
> - if (ret)
> - goto out_dev_exit;
> -
> atomic_set(&ptdev->reset.pending, 0);
> - panthor_sched_post_reset(ptdev);
> -
> -out_dev_exit:
> + panthor_sched_post_reset(ptdev, ret != 0);
> drm_dev_exit(cookie);
>
> if (ret) {
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 6ea094b00cf9..fc43ff62c77d 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -2728,15 +2728,22 @@ void panthor_sched_pre_reset(struct panthor_device *ptdev)
> mutex_unlock(&sched->reset.lock);
> }
>
> -void panthor_sched_post_reset(struct panthor_device *ptdev)
> +void panthor_sched_post_reset(struct panthor_device *ptdev, bool reset_failed)
> {
> struct panthor_scheduler *sched = ptdev->scheduler;
> struct panthor_group *group, *group_tmp;
>
> mutex_lock(&sched->reset.lock);
>
> - list_for_each_entry_safe(group, group_tmp, &sched->reset.stopped_groups, run_node)
> + list_for_each_entry_safe(group, group_tmp, &sched->reset.stopped_groups, run_node) {
> + /* Consider all previously running group as terminated if the
> + * reset failed.
> + */
> + if (reset_failed)
> + group->state = PANTHOR_CS_GROUP_TERMINATED;
> +
> panthor_group_start(group);
> + }
>
> /* We're done resetting the GPU, clear the reset.in_progress bit so we can
> * kick the scheduler.
> @@ -2744,9 +2751,11 @@ void panthor_sched_post_reset(struct panthor_device *ptdev)
> atomic_set(&sched->reset.in_progress, false);
> mutex_unlock(&sched->reset.lock);
>
> - sched_queue_delayed_work(sched, tick, 0);
> -
> - sched_queue_work(sched, sync_upd);
> + /* No need to queue a tick and update syncs if the reset failed. */
> + if (!reset_failed) {
> + sched_queue_delayed_work(sched, tick, 0);
> + sched_queue_work(sched, sync_upd);
> + }
> }
>
> static void group_sync_upd_work(struct work_struct *work)
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.h b/drivers/gpu/drm/panthor/panthor_sched.h
> index 66438b1f331f..3a30d2328b30 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.h
> +++ b/drivers/gpu/drm/panthor/panthor_sched.h
> @@ -40,7 +40,7 @@ void panthor_group_pool_destroy(struct panthor_file *pfile);
> int panthor_sched_init(struct panthor_device *ptdev);
> void panthor_sched_unplug(struct panthor_device *ptdev);
> void panthor_sched_pre_reset(struct panthor_device *ptdev);
> -void panthor_sched_post_reset(struct panthor_device *ptdev);
> +void panthor_sched_post_reset(struct panthor_device *ptdev, bool reset_failed);
> void panthor_sched_suspend(struct panthor_device *ptdev);
> void panthor_sched_resume(struct panthor_device *ptdev);
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 4/4] drm/panthor: Call panthor_sched_post_reset() even if the reset failed
2024-05-02 18:38 ` [PATCH 4/4] drm/panthor: Call panthor_sched_post_reset() even if the reset failed Boris Brezillon
2024-05-03 9:22 ` Steven Price
@ 2024-05-03 11:49 ` Liviu Dudau
1 sibling, 0 replies; 14+ messages in thread
From: Liviu Dudau @ 2024-05-03 11:49 UTC (permalink / raw)
To: Boris Brezillon
Cc: Steven Price, Adrián Larumbe, Christopher Healy, dri-devel,
kernel
On Thu, May 02, 2024 at 08:38:12PM +0200, Boris Brezillon wrote:
> We need to undo what was done in panthor_sched_pre_reset() even if the
> reset failed. We just flag all previously running groups as terminated
> when that happens to unblock things.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> ---
> drivers/gpu/drm/panthor/panthor_device.c | 7 +------
> drivers/gpu/drm/panthor/panthor_sched.c | 19 ++++++++++++++-----
> drivers/gpu/drm/panthor/panthor_sched.h | 2 +-
> 3 files changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 4c5b54e7abb7..4082c8f2951d 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -129,13 +129,8 @@ static void panthor_device_reset_work(struct work_struct *work)
> panthor_gpu_l2_power_on(ptdev);
> panthor_mmu_post_reset(ptdev);
> ret = panthor_fw_post_reset(ptdev);
> - if (ret)
> - goto out_dev_exit;
> -
> atomic_set(&ptdev->reset.pending, 0);
> - panthor_sched_post_reset(ptdev);
> -
> -out_dev_exit:
> + panthor_sched_post_reset(ptdev, ret != 0);
> drm_dev_exit(cookie);
>
> if (ret) {
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 6ea094b00cf9..fc43ff62c77d 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -2728,15 +2728,22 @@ void panthor_sched_pre_reset(struct panthor_device *ptdev)
> mutex_unlock(&sched->reset.lock);
> }
>
> -void panthor_sched_post_reset(struct panthor_device *ptdev)
> +void panthor_sched_post_reset(struct panthor_device *ptdev, bool reset_failed)
> {
> struct panthor_scheduler *sched = ptdev->scheduler;
> struct panthor_group *group, *group_tmp;
>
> mutex_lock(&sched->reset.lock);
>
> - list_for_each_entry_safe(group, group_tmp, &sched->reset.stopped_groups, run_node)
> + list_for_each_entry_safe(group, group_tmp, &sched->reset.stopped_groups, run_node) {
> + /* Consider all previously running group as terminated if the
> + * reset failed.
> + */
> + if (reset_failed)
> + group->state = PANTHOR_CS_GROUP_TERMINATED;
> +
> panthor_group_start(group);
> + }
>
> /* We're done resetting the GPU, clear the reset.in_progress bit so we can
> * kick the scheduler.
> @@ -2744,9 +2751,11 @@ void panthor_sched_post_reset(struct panthor_device *ptdev)
> atomic_set(&sched->reset.in_progress, false);
> mutex_unlock(&sched->reset.lock);
>
> - sched_queue_delayed_work(sched, tick, 0);
> -
> - sched_queue_work(sched, sync_upd);
> + /* No need to queue a tick and update syncs if the reset failed. */
> + if (!reset_failed) {
> + sched_queue_delayed_work(sched, tick, 0);
> + sched_queue_work(sched, sync_upd);
> + }
> }
>
> static void group_sync_upd_work(struct work_struct *work)
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.h b/drivers/gpu/drm/panthor/panthor_sched.h
> index 66438b1f331f..3a30d2328b30 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.h
> +++ b/drivers/gpu/drm/panthor/panthor_sched.h
> @@ -40,7 +40,7 @@ void panthor_group_pool_destroy(struct panthor_file *pfile);
> int panthor_sched_init(struct panthor_device *ptdev);
> void panthor_sched_unplug(struct panthor_device *ptdev);
> void panthor_sched_pre_reset(struct panthor_device *ptdev);
> -void panthor_sched_post_reset(struct panthor_device *ptdev);
> +void panthor_sched_post_reset(struct panthor_device *ptdev, bool reset_failed);
> void panthor_sched_suspend(struct panthor_device *ptdev);
> void panthor_sched_resume(struct panthor_device *ptdev);
>
> --
> 2.44.0
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/4] drm/panthor: More reset fixes
2024-05-02 18:38 [PATCH 0/4] drm/panthor: More reset fixes Boris Brezillon
` (3 preceding siblings ...)
2024-05-02 18:38 ` [PATCH 4/4] drm/panthor: Call panthor_sched_post_reset() even if the reset failed Boris Brezillon
@ 2024-05-13 11:40 ` Boris Brezillon
4 siblings, 0 replies; 14+ messages in thread
From: Boris Brezillon @ 2024-05-13 11:40 UTC (permalink / raw)
To: Boris Brezillon, Steven Price, Liviu Dudau, Adrián Larumbe
Cc: Christopher Healy, dri-devel, kernel
On Thu, 2 May 2024 20:38:08 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:
> Hello,
>
> This is a collection of fixes for bugs found while chasing an
> unrecoverable fault leading to a device unplug (because of some
> other bugs that was introduced in my local dev branch).
>
> The first patch makes sure we immediately reset the GPU on an
> unrecoverable fault, and following patches are fixing various
> NULL/invalid pointer derefs caused by use-after-free situations
> following a device unplug.
>
> Regards,
>
> Boris
>
> Boris Brezillon (4):
> drm/panthor: Force an immediate reset on unrecoverable faults
> drm/panthor: Keep a ref to the VM at the panthor_kernel_bo level
> drm/panthor: Reset the FW VM to NULL on unplug
> drm/panthor: Call panthor_sched_post_reset() even if the reset failed
Queued to drm-misc-next-fixes.
>
> drivers/gpu/drm/panthor/panthor_device.c | 8 ++---
> drivers/gpu/drm/panthor/panthor_device.h | 1 +
> drivers/gpu/drm/panthor/panthor_fw.c | 5 +--
> drivers/gpu/drm/panthor/panthor_gem.c | 8 +++--
> drivers/gpu/drm/panthor/panthor_gem.h | 8 +++--
> drivers/gpu/drm/panthor/panthor_heap.c | 8 ++---
> drivers/gpu/drm/panthor/panthor_sched.c | 40 +++++++++++++++++-------
> drivers/gpu/drm/panthor/panthor_sched.h | 2 +-
> 8 files changed, 51 insertions(+), 29 deletions(-)
>
^ permalink raw reply [flat|nested] 14+ messages in thread