[PATCH] drm/amdgpu: improve HMM error -ENOMEM and -EBUSY handling

[Yang, Philip]

Under memory pressure, hmm_range_fault may return error code -ENOMEM
or -EBUSY, change pr_info to pr_debug to remove unnecessary kernel log
message because we will retry restore again.

Call get_user_pages_done if TTM get user pages failed will have
WARN_ONCE kernel calling stack dump log.

Change-Id: I086f92944630f9d1a70365c00417cb9440662464
Signed-off-by: Philip Yang 
---
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 38 +++
 1 file changed, 6 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 74e86952553f..10abae398e51 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -1731,35 +1731,17 @@ static int update_invalid_user_pages(struct 
amdkfd_process_info *process_info,
ret = amdgpu_ttm_tt_get_user_pages(bo->tbo.ttm,
   bo->tbo.ttm->pages);
if (ret) {
-   bo->tbo.ttm->pages[0] = NULL;
-   pr_info("%s: Failed to get user pages: %d\n",
+   pr_debug("%s: Failed to get user pages: %d\n",
__func__, ret);
-   /* Pretend it succeeded. It will fail later
-* with a VM fault if the GPU tries to access
-* it. Better than hanging indefinitely with
-* stalled user mode queues.
-*/
-   }
-   }
-
-   return 0;
-}
 
-/* Remove invalid userptr BOs from hmm track list
- *
- * Stop HMM track the userptr update
- */
-static void untrack_invalid_user_pages(struct amdkfd_process_info 
*process_info)
-{
-   struct kgd_mem *mem, *tmp_mem;
-   struct amdgpu_bo *bo;
+   /* Return error -EBUSY or -ENOMEM, retry restore */
+   return ret;
+   }
 
-   list_for_each_entry_safe(mem, tmp_mem,
-_info->userptr_inval_list,
-validate_list.head) {
-   bo = mem->bo;
amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm);
}
+
+   return 0;
 }
 
 /* Validate invalid userptr BOs
@@ -1841,13 +1823,6 @@ static int validate_invalid_user_pages(struct 
amdkfd_process_info *process_info)
list_move_tail(>validate_list.head,
   _info->userptr_valid_list);
 
-   /* Stop HMM track the userptr update. We dont check the return
-* value for concurrent CPU page table update because we will
-* reschedule the restore worker if process_info->evicted_bos
-* is updated.
-*/
-   amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm);
-
/* Update mapping. If the BO was not validated
 * (because we couldn't get user pages), this will
 * clear the page table entries, which will result in
@@ -1946,7 +1921,6 @@ static void amdgpu_amdkfd_restore_userptr_worker(struct 
work_struct *work)
}
 
 unlock_out:
-   untrack_invalid_user_pages(process_info);
mutex_unlock(_info->lock);
mmput(mm);
put_task_struct(usertask);
-- 
2.17.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/4] Revert "drm/amdkfd: Fix sdma queue allocate race condition"

[Yang, Philip]

I just figured out previous patch have issue. New patch is simple and 
looks good to me.

This series is Reviewed-by: Philip.Yang 

On 2019-06-14 9:27 p.m., Zeng, Oak wrote:
> This reverts commit 0a7c7281bdaae8cf63d77be26a4b46128114bdec.
> This fix is not proper. allocate_mqd can't be moved before
> allocate_sdma_queue as it depends on q->properties->sdma_id
> set in later.
> 
> Change-Id: If4934afebda8cf37dfcde9b50ce53643d526584d
> Signed-off-by: Oak Zeng 
> ---
>   .../gpu/drm/amd/amdkfd/kfd_device_queue_manager.c  | 26 
> --
>   1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> index 250798b..d566c26 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> @@ -1133,27 +1133,23 @@ static int create_queue_cpsch(struct 
> device_queue_manager *dqm, struct queue *q,
>   if (dqm->total_queue_count >= max_num_of_queues_per_device) {
>   pr_warn("Can't create new usermode queue because %d queues were 
> already created\n",
>   dqm->total_queue_count);
> - return -EPERM;
> + retval = -EPERM;
> + goto out;
>   }
>   
> - mqd_mgr = dqm->mqd_mgrs[get_mqd_type_from_queue_type(
> - q->properties.type)];
> - q->mqd_mem_obj = mqd_mgr->allocate_mqd(mqd_mgr->dev, >properties);
> - if (!q->mqd_mem_obj)
> - return -ENOMEM;
> -
> - dqm_lock(dqm);
>   if (q->properties.type == KFD_QUEUE_TYPE_SDMA ||
>   q->properties.type == KFD_QUEUE_TYPE_SDMA_XGMI) {
>   retval = allocate_sdma_queue(dqm, q);
>   if (retval)
> - goto out_unlock;
> + goto out;
>   }
>   
>   retval = allocate_doorbell(qpd, q);
>   if (retval)
>   goto out_deallocate_sdma_queue;
>   
> + mqd_mgr = dqm->mqd_mgrs[get_mqd_type_from_queue_type(
> + q->properties.type)];
>   /*
>* Eviction state logic: mark all queues as evicted, even ones
>* not currently active. Restoring inactive queues later only
> @@ -1165,8 +1161,14 @@ static int create_queue_cpsch(struct 
> device_queue_manager *dqm, struct queue *q,
>   dqm->asic_ops.init_sdma_vm(dqm, q, qpd);
>   q->properties.tba_addr = qpd->tba_addr;
>   q->properties.tma_addr = qpd->tma_addr;
> + q->mqd_mem_obj = mqd_mgr->allocate_mqd(mqd_mgr->dev, >properties);
> + if (!q->mqd_mem_obj) {
> + retval = -ENOMEM;
> + goto out_deallocate_doorbell;
> + }
>   mqd_mgr->init_mqd(mqd_mgr, >mqd, q->mqd_mem_obj,
>   >gart_mqd_addr, >properties);
> + dqm_lock(dqm);
>   
>   list_add(>list, >queues_list);
>   qpd->queue_count++;
> @@ -1192,13 +1194,13 @@ static int create_queue_cpsch(struct 
> device_queue_manager *dqm, struct queue *q,
>   dqm_unlock(dqm);
>   return retval;
>   
> +out_deallocate_doorbell:
> + deallocate_doorbell(qpd, q);
>   out_deallocate_sdma_queue:
>   if (q->properties.type == KFD_QUEUE_TYPE_SDMA ||
>   q->properties.type == KFD_QUEUE_TYPE_SDMA_XGMI)
>   deallocate_sdma_queue(dqm, q);
> -out_unlock:
> - dqm_unlock(dqm);
> - mqd_mgr->free_mqd(mqd_mgr, q->mqd, q->mqd_mem_obj);
> +out:
>   return retval;
>   }
>   
> 
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/4] Revert "drm/amdkfd: Fix a circular lock dependency"

[Zeng, Oak]

This reverts commit 49b7f386343b4da9d9b14d97061c34fdd3dd2628.
This fix is not proper. allocate_mqd can't be moved before
allocate_sdma_queue as it depends on q->properties->sdma_id
set in later.

Change-Id: Ia99ec628e9df5abdf4c4c730e57d40cab0b6a4ad
Signed-off-by: Oak Zeng 
---
 .../gpu/drm/amd/amdkfd/kfd_device_queue_manager.c   | 21 +++--
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
index d566c26..c7ab203 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
@@ -274,12 +274,6 @@ static int create_queue_nocpsch(struct 
device_queue_manager *dqm,
 
print_queue(q);
 
-   mqd_mgr = dqm->mqd_mgrs[get_mqd_type_from_queue_type(
-   q->properties.type)];
-   q->mqd_mem_obj = mqd_mgr->allocate_mqd(mqd_mgr->dev, >properties);
-   if (!q->mqd_mem_obj)
-   return -ENOMEM;
-
dqm_lock(dqm);
 
if (dqm->total_queue_count >= max_num_of_queues_per_device) {
@@ -305,6 +299,8 @@ static int create_queue_nocpsch(struct device_queue_manager 
*dqm,
q->properties.tba_addr = qpd->tba_addr;
q->properties.tma_addr = qpd->tma_addr;
 
+   mqd_mgr = dqm->mqd_mgrs[get_mqd_type_from_queue_type(
+   q->properties.type)];
if (q->properties.type == KFD_QUEUE_TYPE_COMPUTE) {
retval = allocate_hqd(dqm, q);
if (retval)
@@ -323,6 +319,11 @@ static int create_queue_nocpsch(struct 
device_queue_manager *dqm,
if (retval)
goto out_deallocate_hqd;
 
+   q->mqd_mem_obj = mqd_mgr->allocate_mqd(mqd_mgr->dev, >properties);
+   if (!q->mqd_mem_obj) {
+   retval = -ENOMEM;
+   goto out_deallocate_doorbell;
+   }
mqd_mgr->init_mqd(mqd_mgr, >mqd, q->mqd_mem_obj,
>gart_mqd_addr, >properties);
if (q->properties.is_active) {
@@ -334,7 +335,7 @@ static int create_queue_nocpsch(struct device_queue_manager 
*dqm,
retval = mqd_mgr->load_mqd(mqd_mgr, q->mqd, q->pipe,
q->queue, >properties, current->mm);
if (retval)
-   goto out_deallocate_doorbell;
+   goto out_free_mqd;
}
 
list_add(>list, >queues_list);
@@ -354,9 +355,10 @@ static int create_queue_nocpsch(struct 
device_queue_manager *dqm,
dqm->total_queue_count++;
pr_debug("Total of %d queues are accountable so far\n",
dqm->total_queue_count);
-   dqm_unlock(dqm);
-   return retval;
+   goto out_unlock;
 
+out_free_mqd:
+   mqd_mgr->free_mqd(mqd_mgr, q->mqd, q->mqd_mem_obj);
 out_deallocate_doorbell:
deallocate_doorbell(qpd, q);
 out_deallocate_hqd:
@@ -370,7 +372,6 @@ static int create_queue_nocpsch(struct device_queue_manager 
*dqm,
deallocate_vmid(dqm, qpd, q);
 out_unlock:
dqm_unlock(dqm);
-   mqd_mgr->free_mqd(mqd_mgr, q->mqd, q->mqd_mem_obj);
return retval;
 }
 
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/4] Revert "drm/amdkfd: Fix sdma queue allocate race condition"

[Zeng, Oak]

This reverts commit 0a7c7281bdaae8cf63d77be26a4b46128114bdec.
This fix is not proper. allocate_mqd can't be moved before
allocate_sdma_queue as it depends on q->properties->sdma_id
set in later.

Change-Id: If4934afebda8cf37dfcde9b50ce53643d526584d
Signed-off-by: Oak Zeng 
---
 .../gpu/drm/amd/amdkfd/kfd_device_queue_manager.c  | 26 --
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
index 250798b..d566c26 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
@@ -1133,27 +1133,23 @@ static int create_queue_cpsch(struct 
device_queue_manager *dqm, struct queue *q,
if (dqm->total_queue_count >= max_num_of_queues_per_device) {
pr_warn("Can't create new usermode queue because %d queues were 
already created\n",
dqm->total_queue_count);
-   return -EPERM;
+   retval = -EPERM;
+   goto out;
}
 
-   mqd_mgr = dqm->mqd_mgrs[get_mqd_type_from_queue_type(
-   q->properties.type)];
-   q->mqd_mem_obj = mqd_mgr->allocate_mqd(mqd_mgr->dev, >properties);
-   if (!q->mqd_mem_obj)
-   return -ENOMEM;
-
-   dqm_lock(dqm);
if (q->properties.type == KFD_QUEUE_TYPE_SDMA ||
q->properties.type == KFD_QUEUE_TYPE_SDMA_XGMI) {
retval = allocate_sdma_queue(dqm, q);
if (retval)
-   goto out_unlock;
+   goto out;
}
 
retval = allocate_doorbell(qpd, q);
if (retval)
goto out_deallocate_sdma_queue;
 
+   mqd_mgr = dqm->mqd_mgrs[get_mqd_type_from_queue_type(
+   q->properties.type)];
/*
 * Eviction state logic: mark all queues as evicted, even ones
 * not currently active. Restoring inactive queues later only
@@ -1165,8 +1161,14 @@ static int create_queue_cpsch(struct 
device_queue_manager *dqm, struct queue *q,
dqm->asic_ops.init_sdma_vm(dqm, q, qpd);
q->properties.tba_addr = qpd->tba_addr;
q->properties.tma_addr = qpd->tma_addr;
+   q->mqd_mem_obj = mqd_mgr->allocate_mqd(mqd_mgr->dev, >properties);
+   if (!q->mqd_mem_obj) {
+   retval = -ENOMEM;
+   goto out_deallocate_doorbell;
+   }
mqd_mgr->init_mqd(mqd_mgr, >mqd, q->mqd_mem_obj,
>gart_mqd_addr, >properties);
+   dqm_lock(dqm);
 
list_add(>list, >queues_list);
qpd->queue_count++;
@@ -1192,13 +1194,13 @@ static int create_queue_cpsch(struct 
device_queue_manager *dqm, struct queue *q,
dqm_unlock(dqm);
return retval;
 
+out_deallocate_doorbell:
+   deallocate_doorbell(qpd, q);
 out_deallocate_sdma_queue:
if (q->properties.type == KFD_QUEUE_TYPE_SDMA ||
q->properties.type == KFD_QUEUE_TYPE_SDMA_XGMI)
deallocate_sdma_queue(dqm, q);
-out_unlock:
-   dqm_unlock(dqm);
-   mqd_mgr->free_mqd(mqd_mgr, q->mqd, q->mqd_mem_obj);
+out:
return retval;
 }
 
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/4] drm/amdkfd: Fix a circular lock dependency

[Zeng, Oak]

The idea to break the circular lock dependency is to temporarily drop
dqm lock before calling allocate_mqd. See callstack #1 below.

[   59.510149] [drm] Initialized amdgpu 3.30.0 20150101 for :04:00.0 on 
minor 0

[  513.604034] ==
[  513.604205] WARNING: possible circular locking dependency detected
[  513.604375] 4.18.0-kfd-root #2 Tainted: GW
[  513.604530] --
[  513.604699] kswapd0/611 is trying to acquire lock:
[  513.604840] d254022e (>lock_hidden){+.+.}, at: 
evict_process_queues_nocpsch+0x26/0x140 [amdgpu]
[  513.605150]
   but task is already holding lock:
[  513.605307] 961547fc (_vma->rwsem){}, at: 
page_lock_anon_vma_read+0xe4/0x250
[  513.605540]
   which lock already depends on the new lock.

[  513.605747]
   the existing dependency chain (in reverse order) is:
[  513.605944]
   -> #4 (_vma->rwsem){}:
[  513.606106]__vma_adjust+0x147/0x7f0
[  513.606231]__split_vma+0x179/0x190
[  513.606353]mprotect_fixup+0x217/0x260
[  513.606553]do_mprotect_pkey+0x211/0x380
[  513.606752]__x64_sys_mprotect+0x1b/0x20
[  513.606954]do_syscall_64+0x50/0x1a0
[  513.607149]entry_SYSCALL_64_after_hwframe+0x49/0xbe
[  513.607380]
   -> #3 (>i_mmap_rwsem){}:
[  513.607678]rmap_walk_file+0x1f0/0x280
[  513.607887]page_referenced+0xdd/0x180
[  513.608081]shrink_page_list+0x853/0xcb0
[  513.608279]shrink_inactive_list+0x33b/0x700
[  513.608483]shrink_node_memcg+0x37a/0x7f0
[  513.608682]shrink_node+0xd8/0x490
[  513.608869]balance_pgdat+0x18b/0x3b0
[  513.609062]kswapd+0x203/0x5c0
[  513.609241]kthread+0x100/0x140
[  513.609420]ret_from_fork+0x24/0x30
[  513.609607]
   -> #2 (fs_reclaim){+.+.}:
[  513.609883]kmem_cache_alloc_trace+0x34/0x2e0
[  513.610093]reservation_object_reserve_shared+0x139/0x300
[  513.610326]ttm_bo_init_reserved+0x291/0x480 [ttm]
[  513.610567]amdgpu_bo_do_create+0x1d2/0x650 [amdgpu]
[  513.610811]amdgpu_bo_create+0x40/0x1f0 [amdgpu]
[  513.611041]amdgpu_bo_create_reserved+0x249/0x2d0 [amdgpu]
[  513.611290]amdgpu_bo_create_kernel+0x12/0x70 [amdgpu]
[  513.611584]amdgpu_ttm_init+0x2cb/0x560 [amdgpu]
[  513.611823]gmc_v9_0_sw_init+0x400/0x750 [amdgpu]
[  513.612491]amdgpu_device_init+0x14eb/0x1990 [amdgpu]
[  513.612730]amdgpu_driver_load_kms+0x78/0x290 [amdgpu]
[  513.612958]drm_dev_register+0x111/0x1a0
[  513.613171]amdgpu_pci_probe+0x11c/0x1e0 [amdgpu]
[  513.613389]local_pci_probe+0x3f/0x90
[  513.613581]pci_device_probe+0x102/0x1c0
[  513.613779]driver_probe_device+0x2a7/0x480
[  513.613984]__driver_attach+0x10a/0x110
[  513.614179]bus_for_each_dev+0x67/0xc0
[  513.614372]bus_add_driver+0x1eb/0x260
[  513.614565]driver_register+0x5b/0xe0
[  513.614756]do_one_initcall+0xac/0x357
[  513.614952]do_init_module+0x5b/0x213
[  513.615145]load_module+0x2542/0x2d30
[  513.615337]__do_sys_finit_module+0xd2/0x100
[  513.615541]do_syscall_64+0x50/0x1a0
[  513.615731]entry_SYSCALL_64_after_hwframe+0x49/0xbe
[  513.615963]
   -> #1 (reservation_ww_class_mutex){+.+.}:
[  513.616293]amdgpu_amdkfd_alloc_gtt_mem+0xcf/0x2c0 [amdgpu]
[  513.616554]init_mqd+0x223/0x260 [amdgpu]
[  513.616779]create_queue_nocpsch+0x4d9/0x600 [amdgpu]
[  513.617031]pqm_create_queue+0x37c/0x520 [amdgpu]
[  513.617270]kfd_ioctl_create_queue+0x2f9/0x650 [amdgpu]
[  513.617522]kfd_ioctl+0x202/0x350 [amdgpu]
[  513.617724]do_vfs_ioctl+0x9f/0x6c0
[  513.617914]ksys_ioctl+0x66/0x70
[  513.618095]__x64_sys_ioctl+0x16/0x20
[  513.618286]do_syscall_64+0x50/0x1a0
[  513.618476]entry_SYSCALL_64_after_hwframe+0x49/0xbe
[  513.618695]
   -> #0 (>lock_hidden){+.+.}:
[  513.618984]__mutex_lock+0x98/0x970
[  513.619197]evict_process_queues_nocpsch+0x26/0x140 [amdgpu]
[  513.619459]kfd_process_evict_queues+0x3b/0xb0 [amdgpu]
[  513.619710]kgd2kfd_quiesce_mm+0x1c/0x40 [amdgpu]
[  513.620103]amdgpu_amdkfd_evict_userptr+0x38/0x70 [amdgpu]
[  513.620363]amdgpu_mn_invalidate_range_start_hsa+0xa6/0xc0 [amdgpu]
[  513.620614]__mmu_notifier_invalidate_range_start+0x70/0xb0
[  513.620851]try_to_unmap_one+0x7fc/0x8f0
[  513.621049]rmap_walk_anon+0x121/0x290
[  513.621242]try_to_unmap+0x93/0xf0
[  513.621428]shrink_page_list+0x606/0xcb0
[  513.621625]shrink_inactive_list+0x33b/0x700
[  513.621835]shrink_node_memcg+0x37a/0x7f0
[  513.622034]shrink_node+0xd8/0x490
[  513.622219]

[PATCH 4/4] drm/amdkfd: Fix sdma queue allocate race condition

[Zeng, Oak]

SDMA queue allocation requires the dqm lock as it modify
the global dqm members. Enclose it in the dqm_lock.

Change-Id: I2fd37a60613c06333e08fcfe90b6ddb367ea43ee
Signed-off-by: Oak Zeng 
---
 drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
index e077db8..b4a5b48 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
@@ -1144,7 +1144,9 @@ static int create_queue_cpsch(struct device_queue_manager 
*dqm, struct queue *q,
 
if (q->properties.type == KFD_QUEUE_TYPE_SDMA ||
q->properties.type == KFD_QUEUE_TYPE_SDMA_XGMI) {
+   dqm_lock(dqm);
retval = allocate_sdma_queue(dqm, q);
+   dqm_unlock(dqm);
if (retval)
goto out;
}
@@ -1203,8 +1205,11 @@ static int create_queue_cpsch(struct 
device_queue_manager *dqm, struct queue *q,
deallocate_doorbell(qpd, q);
 out_deallocate_sdma_queue:
if (q->properties.type == KFD_QUEUE_TYPE_SDMA ||
-   q->properties.type == KFD_QUEUE_TYPE_SDMA_XGMI)
+   q->properties.type == KFD_QUEUE_TYPE_SDMA_XGMI) {
+   dqm_lock(dqm);
deallocate_sdma_queue(dqm, q);
+   dqm_unlock(dqm);
+   }
 out:
return retval;
 }
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 06/59] drm/prime: Actually remove DRIVER_PRIME everywhere

[Sam Ravnborg]

Hi Daniel.

Minor nitpick..

> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 65d599065709..4fd09a9ad67a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -3193,7 +3193,7 @@ static struct drm_driver driver = {
>* deal with them for Intel hardware.
>*/
>   .driver_features =
> - DRIVER_GEM | DRIVER_PRIME |
> + DRIVER_GEM | 
>   DRIVER_RENDER | DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_SYNCOBJ,
Adds a whitespace.

Sam
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 51/59] drm/radeon: Fill out gem_object->resv

[Daniel Vetter]

That way we can ditch our gem_prime_res_obj implementation. Since ttm
absolutely needs the right reservation object all the boilerplate is
already there and we just have to wire it up correctly.

Note that gem/prime doesn't care when we do this, as long as we do it
before the bo is registered and someone can call the handle2fd ioctl
on it.

Aside: ttm_buffer_object.ttm_resv could probably be ditched in favour
of always passing a non-NULL resv to ttm_bo_init(). At least for gem
drivers that would avoid having two of these, on in ttm_buffer_object
and the other in drm_gem_object, one just there for confusion.

Signed-off-by: Daniel Vetter 
Cc: Alex Deucher 
Cc: "Christian König" 
Cc: "David (ChunMing) Zhou" 
Cc: amd-gfx@lists.freedesktop.org
---
 drivers/gpu/drm/radeon/radeon_drv.c| 2 --
 drivers/gpu/drm/radeon/radeon_object.c | 1 +
 drivers/gpu/drm/radeon/radeon_prime.c  | 7 ---
 3 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_drv.c 
b/drivers/gpu/drm/radeon/radeon_drv.c
index 4403e76e1ae0..a4a78dfdef37 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -152,7 +152,6 @@ struct drm_gem_object 
*radeon_gem_prime_import_sg_table(struct drm_device *dev,
struct sg_table *sg);
 int radeon_gem_prime_pin(struct drm_gem_object *obj);
 void radeon_gem_prime_unpin(struct drm_gem_object *obj);
-struct reservation_object *radeon_gem_prime_res_obj(struct drm_gem_object *);
 void *radeon_gem_prime_vmap(struct drm_gem_object *obj);
 void radeon_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr);
 
@@ -566,7 +565,6 @@ static struct drm_driver kms_driver = {
.gem_prime_export = radeon_gem_prime_export,
.gem_prime_pin = radeon_gem_prime_pin,
.gem_prime_unpin = radeon_gem_prime_unpin,
-   .gem_prime_res_obj = radeon_gem_prime_res_obj,
.gem_prime_get_sg_table = radeon_gem_prime_get_sg_table,
.gem_prime_import_sg_table = radeon_gem_prime_import_sg_table,
.gem_prime_vmap = radeon_gem_prime_vmap,
diff --git a/drivers/gpu/drm/radeon/radeon_object.c 
b/drivers/gpu/drm/radeon/radeon_object.c
index 21f73fc86f38..7a2bad843f8a 100644
--- a/drivers/gpu/drm/radeon/radeon_object.c
+++ b/drivers/gpu/drm/radeon/radeon_object.c
@@ -262,6 +262,7 @@ int radeon_bo_create(struct radeon_device *rdev,
r = ttm_bo_init(>mman.bdev, >tbo, size, type,
>placement, page_align, !kernel, acc_size,
sg, resv, _ttm_bo_destroy);
+   bo->gem_base.resv = bo->tbo.resv;
up_read(>pm.mclk_lock);
if (unlikely(r != 0)) {
return r;
diff --git a/drivers/gpu/drm/radeon/radeon_prime.c 
b/drivers/gpu/drm/radeon/radeon_prime.c
index deaffce50a2e..8ce3e8045d42 100644
--- a/drivers/gpu/drm/radeon/radeon_prime.c
+++ b/drivers/gpu/drm/radeon/radeon_prime.c
@@ -117,13 +117,6 @@ void radeon_gem_prime_unpin(struct drm_gem_object *obj)
 }
 
 
-struct reservation_object *radeon_gem_prime_res_obj(struct drm_gem_object *obj)
-{
-   struct radeon_bo *bo = gem_to_radeon_bo(obj);
-
-   return bo->tbo.resv;
-}
-
 struct dma_buf *radeon_gem_prime_export(struct drm_gem_object *gobj,
int flags)
 {
-- 
2.20.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 09/59] drm/prime: Align gem_prime_export with obj_funcs.export

[Daniel Vetter]

The idea is that gem_prime_export is deprecated in favor of
obj_funcs.export. That's much easier to do if both have matching
function signatures.

Signed-off-by: Daniel Vetter 
Cc: Russell King 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Sean Paul 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: Zhenyu Wang 
Cc: Zhi Wang 
Cc: Jani Nikula 
Cc: Joonas Lahtinen 
Cc: Rodrigo Vivi 
Cc: Tomi Valkeinen 
Cc: Alex Deucher 
Cc: "Christian König" 
Cc: "David (ChunMing) Zhou" 
Cc: Thierry Reding 
Cc: Jonathan Hunter 
Cc: Dave Airlie 
Cc: Eric Anholt 
Cc: "Michel Dänzer" 
Cc: Chris Wilson 
Cc: Huang Rui 
Cc: Felix Kuehling 
Cc: Hawking Zhang 
Cc: Feifei Xu 
Cc: Jim Qu 
Cc: Evan Quan 
Cc: Matthew Auld 
Cc: Mika Kuoppala 
Cc: Thomas Zimmermann 
Cc: Kate Stewart 
Cc: Sumit Semwal 
Cc: Jilayne Lovejoy 
Cc: Thomas Gleixner 
Cc: Mikulas Patocka 
Cc: Greg Kroah-Hartman 
Cc: Junwei Zhang 
Cc: intel-gvt-...@lists.freedesktop.org
Cc: intel-...@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org
Cc: linux-te...@vger.kernel.org
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c  | 7 +++
 drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h  | 3 +--
 drivers/gpu/drm/armada/armada_gem.c  | 5 ++---
 drivers/gpu/drm/armada/armada_gem.h  | 3 +--
 drivers/gpu/drm/drm_prime.c  | 9 -
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c   | 5 ++---
 drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c | 8 
 drivers/gpu/drm/i915/gvt/dmabuf.c| 2 +-
 drivers/gpu/drm/i915/i915_drv.h  | 3 +--
 drivers/gpu/drm/omapdrm/omap_gem.h   | 3 +--
 drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c| 5 ++---
 drivers/gpu/drm/radeon/radeon_drv.c  | 3 +--
 drivers/gpu/drm/radeon/radeon_prime.c| 5 ++---
 drivers/gpu/drm/tegra/gem.c  | 7 +++
 drivers/gpu/drm/tegra/gem.h  | 3 +--
 drivers/gpu/drm/udl/udl_dmabuf.c | 5 ++---
 drivers/gpu/drm/udl/udl_drv.h| 3 +--
 drivers/gpu/drm/vc4/vc4_bo.c | 5 ++---
 drivers/gpu/drm/vc4/vc4_drv.h| 3 +--
 drivers/gpu/drm/vgem/vgem_fence.c| 2 +-
 include/drm/drm_drv.h| 4 ++--
 include/drm/drm_prime.h  | 3 +--
 22 files changed, 39 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
index 489041df1f45..4809d4a5d72a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
@@ -345,8 +345,7 @@ const struct dma_buf_ops amdgpu_dmabuf_ops = {
  * Returns:
  * Shared DMA buffer representing the GEM BO from the given device.
  */
-struct dma_buf *amdgpu_gem_prime_export(struct drm_device *dev,
-   struct drm_gem_object *gobj,
+struct dma_buf *amdgpu_gem_prime_export(struct drm_gem_object *gobj,
int flags)
 {
struct amdgpu_bo *bo = gem_to_amdgpu_bo(gobj);
@@ -356,9 +355,9 @@ struct dma_buf *amdgpu_gem_prime_export(struct drm_device 
*dev,
bo->flags & AMDGPU_GEM_CREATE_VM_ALWAYS_VALID)
return ERR_PTR(-EPERM);
 
-   buf = drm_gem_prime_export(dev, gobj, flags);
+   buf = drm_gem_prime_export(gobj, flags);
if (!IS_ERR(buf)) {
-   buf->file->f_mapping = dev->anon_inode->i_mapping;
+   buf->file->f_mapping = gobj->dev->anon_inode->i_mapping;
buf->ops = _dmabuf_ops;
}
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h
index c7056cbe8685..7f73a4f94204 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.h
@@ -30,8 +30,7 @@ struct drm_gem_object *
 amdgpu_gem_prime_import_sg_table(struct drm_device *dev,
 struct dma_buf_attachment *attach,
 struct sg_table *sg);
-struct dma_buf *amdgpu_gem_prime_export(struct drm_device *dev,
-   struct drm_gem_object *gobj,
+struct dma_buf *amdgpu_gem_prime_export(struct drm_gem_object *gobj,
int flags);
 struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev,
struct dma_buf *dma_buf);
diff --git a/drivers/gpu/drm/armada/armada_gem.c 
b/drivers/gpu/drm/armada/armada_gem.c
index 642d0e70d0f8..7e7fcc3f1f7f 100644
--- a/drivers/gpu/drm/armada/armada_gem.c
+++ b/drivers/gpu/drm/armada/armada_gem.c
@@ -485,8 +485,7 @@ static const struct dma_buf_ops armada_gem_prime_dmabuf_ops 
= {
 };
 
 struct dma_buf *
-armada_gem_prime_export(struct drm_device *dev, struct drm_gem_object *obj,
-   int flags)

[PATCH 06/59] drm/prime: Actually remove DRIVER_PRIME everywhere

[Daniel Vetter]

Split out to make the functional changes stick out more.

v2: amdgpu gained DRIVER_SYNCOBJ_TIMELINE.

v3: amdgpu lost DRIVER_SYNCOBJ_TIMELINE.

Signed-off-by: Daniel Vetter 
Cc: amd-gfx@lists.freedesktop.org
Cc: etna...@lists.freedesktop.org
Cc: freedr...@lists.freedesktop.org
Cc: intel-...@lists.freedesktop.org
Cc: l...@lists.freedesktop.org
Cc: linux-amlo...@lists.infradead.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-arm-...@vger.kernel.org
Cc: linux-asp...@lists.ozlabs.org
Cc: linux-renesas-...@vger.kernel.org
Cc: linux-rockc...@lists.infradead.org
Cc: linux-samsung-...@vger.kernel.org
Cc: linux-st...@st-md-mailman.stormreply.com
Cc: linux-te...@vger.kernel.org
Cc: nouv...@lists.freedesktop.org
Cc: NXP Linux Team 
Cc: spice-de...@lists.freedesktop.org
Cc: virtualizat...@lists.linux-foundation.org
Cc: VMware Graphics 
Cc: xen-de...@lists.xenproject.org
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +-
 drivers/gpu/drm/arc/arcpgu_drv.c| 3 +--
 drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 2 +-
 drivers/gpu/drm/arm/hdlcd_drv.c | 4 +---
 drivers/gpu/drm/arm/malidp_drv.c| 3 +--
 drivers/gpu/drm/armada/armada_drv.c | 3 +--
 drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 3 +--
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c| 4 +---
 drivers/gpu/drm/bochs/bochs_drv.c   | 3 +--
 drivers/gpu/drm/cirrus/cirrus.c | 2 +-
 drivers/gpu/drm/etnaviv/etnaviv_drv.c   | 4 +---
 drivers/gpu/drm/exynos/exynos_drm_drv.c | 2 +-
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c   | 3 +--
 drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 3 +--
 drivers/gpu/drm/i915/i915_drv.c | 2 +-
 drivers/gpu/drm/imx/imx-drm-core.c  | 3 +--
 drivers/gpu/drm/lima/lima_drv.c | 2 +-
 drivers/gpu/drm/mcde/mcde_drv.c | 2 +-
 drivers/gpu/drm/mediatek/mtk_drm_drv.c  | 3 +--
 drivers/gpu/drm/meson/meson_drv.c   | 4 +---
 drivers/gpu/drm/msm/msm_drv.c   | 1 -
 drivers/gpu/drm/mxsfb/mxsfb_drv.c   | 3 +--
 drivers/gpu/drm/nouveau/nouveau_drm.c   | 2 +-
 drivers/gpu/drm/omapdrm/omap_drv.c  | 2 +-
 drivers/gpu/drm/panfrost/panfrost_drv.c | 3 +--
 drivers/gpu/drm/pl111/pl111_drv.c   | 2 +-
 drivers/gpu/drm/qxl/qxl_drv.c   | 3 +--
 drivers/gpu/drm/radeon/radeon_drv.c | 2 +-
 drivers/gpu/drm/rcar-du/rcar_du_drv.c   | 3 +--
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 3 +--
 drivers/gpu/drm/shmobile/shmob_drm_drv.c| 3 +--
 drivers/gpu/drm/sti/sti_drv.c   | 3 +--
 drivers/gpu/drm/stm/drv.c   | 3 +--
 drivers/gpu/drm/sun4i/sun4i_drv.c   | 2 +-
 drivers/gpu/drm/tegra/drm.c | 2 +-
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 3 +--
 drivers/gpu/drm/tinydrm/hx8357d.c   | 2 +-
 drivers/gpu/drm/tinydrm/ili9225.c   | 3 +--
 drivers/gpu/drm/tinydrm/ili9341.c   | 2 +-
 drivers/gpu/drm/tinydrm/mi0283qt.c  | 3 +--
 drivers/gpu/drm/tinydrm/repaper.c   | 3 +--
 drivers/gpu/drm/tinydrm/st7586.c| 3 +--
 drivers/gpu/drm/tinydrm/st7735r.c   | 3 +--
 drivers/gpu/drm/tve200/tve200_drv.c | 3 +--
 drivers/gpu/drm/udl/udl_drv.c   | 2 +-
 drivers/gpu/drm/v3d/v3d_drv.c   | 1 -
 drivers/gpu/drm/vboxvideo/vbox_drv.c| 2 +-
 drivers/gpu/drm/vc4/vc4_drv.c   | 1 -
 drivers/gpu/drm/vgem/vgem_drv.c | 3 +--
 drivers/gpu/drm/virtio/virtgpu_drv.c| 2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 2 +-
 drivers/gpu/drm/xen/xen_drm_front.c | 3 +--
 drivers/gpu/drm/zte/zx_drm_drv.c| 3 +--
 include/drm/drm_drv.h   | 6 --
 54 files changed, 50 insertions(+), 94 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 0a577a389024..8e1b269351e8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -1309,7 +1309,7 @@ static struct drm_driver kms_driver = {
.driver_features =
DRIVER_USE_AGP | DRIVER_ATOMIC |
DRIVER_GEM |
-   DRIVER_PRIME | DRIVER_RENDER | DRIVER_MODESET | DRIVER_SYNCOBJ,
+   DRIVER_RENDER | DRIVER_MODESET | DRIVER_SYNCOBJ,
.load = amdgpu_driver_load_kms,
.open = amdgpu_driver_open_kms,
.postclose = amdgpu_driver_postclose_kms,
diff --git a/drivers/gpu/drm/arc/arcpgu_drv.c b/drivers/gpu/drm/arc/arcpgu_drv.c
index af60c6d7a5f4..74240cc1c300 100644
--- a/drivers/gpu/drm/arc/arcpgu_drv.c
+++ b/drivers/gpu/drm/arc/arcpgu_drv.c
@@ -135,8 +135,7 @@ static int arcpgu_debugfs_init(struct drm_minor *minor)
 #endif
 
 static struct drm_driver arcpgu_drm_driver = {
-   

Re: [PATCH] drm/amdgpu: extend AMDGPU_CTX_PRIORITY_NORMAL comment

[Koenig, Christian]

Am 14.06.19 um 19:33 schrieb Emil Velikov:
> From: Emil Velikov 
>
> Currently the AMDGPU_CTX_PRIORITY_* defines are used in both
> drm_amdgpu_ctx_in::priority and drm_amdgpu_sched_in::priority.
>
> Extend the comment to mention the CAP_SYS_NICE or DRM_MASTER requirement
> is only applicable with the former.
>
> Cc: Bas Nieuwenhuizen 
> Cc: Christian König 
> Cc: Alex Deucher 
> Signed-off-by: Emil Velikov 
> ---
> Mildly curious: why didn't one extend ctx_amdgpu_ctx instead of adding
> drm_amdgpu_sched? New flag + _u32 fd at the end (for the former) would
> have been enough (and tweaking the ioctl permission thingy).

The drm_amdgpu_sched is only allowed for DRM_MASTER.

Christian.

>
> Speaking of flags, drm_amdgpu_sched_in lost its so extending it will
> be "fun"
> ---
>   include/uapi/drm/amdgpu_drm.h | 7 ++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
> index 4788730dbe78..dfb10fba2fe8 100644
> --- a/include/uapi/drm/amdgpu_drm.h
> +++ b/include/uapi/drm/amdgpu_drm.h
> @@ -219,7 +219,10 @@ union drm_amdgpu_bo_list {
>   #define AMDGPU_CTX_PRIORITY_VERY_LOW-1023
>   #define AMDGPU_CTX_PRIORITY_LOW -512
>   #define AMDGPU_CTX_PRIORITY_NORMAL  0
> -/* Selecting a priority above NORMAL requires CAP_SYS_NICE or DRM_MASTER */
> +/*
> + * When used in struct drm_amdgpu_ctx_in, a priority above NORMAL requires
> + * CAP_SYS_NICE or DRM_MASTER
> +*/
>   #define AMDGPU_CTX_PRIORITY_HIGH512
>   #define AMDGPU_CTX_PRIORITY_VERY_HIGH   1023
>   
> @@ -229,6 +232,7 @@ struct drm_amdgpu_ctx_in {
>   /** For future use, no flags defined so far */
>   __u32   flags;
>   __u32   ctx_id;
> + /** AMDGPU_CTX_PRIORITY_* */
>   __s32   priority;
>   };
>   
> @@ -281,6 +285,7 @@ struct drm_amdgpu_sched_in {
>   /* AMDGPU_SCHED_OP_* */
>   __u32   op;
>   __u32   fd;
> + /** AMDGPU_CTX_PRIORITY_* */
>   __s32   priority;
>   __u32   ctx_id;
>   };

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu: extend AMDGPU_CTX_PRIORITY_NORMAL comment

[Emil Velikov]

From: Emil Velikov 

Currently the AMDGPU_CTX_PRIORITY_* defines are used in both
drm_amdgpu_ctx_in::priority and drm_amdgpu_sched_in::priority.

Extend the comment to mention the CAP_SYS_NICE or DRM_MASTER requirement
is only applicable with the former.

Cc: Bas Nieuwenhuizen 
Cc: Christian König 
Cc: Alex Deucher 
Signed-off-by: Emil Velikov 
---
Mildly curious: why didn't one extend ctx_amdgpu_ctx instead of adding
drm_amdgpu_sched? New flag + _u32 fd at the end (for the former) would
have been enough (and tweaking the ioctl permission thingy).

Speaking of flags, drm_amdgpu_sched_in lost its so extending it will
be "fun"
---
 include/uapi/drm/amdgpu_drm.h | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
index 4788730dbe78..dfb10fba2fe8 100644
--- a/include/uapi/drm/amdgpu_drm.h
+++ b/include/uapi/drm/amdgpu_drm.h
@@ -219,7 +219,10 @@ union drm_amdgpu_bo_list {
 #define AMDGPU_CTX_PRIORITY_VERY_LOW-1023
 #define AMDGPU_CTX_PRIORITY_LOW -512
 #define AMDGPU_CTX_PRIORITY_NORMAL  0
-/* Selecting a priority above NORMAL requires CAP_SYS_NICE or DRM_MASTER */
+/*
+ * When used in struct drm_amdgpu_ctx_in, a priority above NORMAL requires
+ * CAP_SYS_NICE or DRM_MASTER
+*/
 #define AMDGPU_CTX_PRIORITY_HIGH512
 #define AMDGPU_CTX_PRIORITY_VERY_HIGH   1023
 
@@ -229,6 +232,7 @@ struct drm_amdgpu_ctx_in {
/** For future use, no flags defined so far */
__u32   flags;
__u32   ctx_id;
+   /** AMDGPU_CTX_PRIORITY_* */
__s32   priority;
 };
 
@@ -281,6 +285,7 @@ struct drm_amdgpu_sched_in {
/* AMDGPU_SCHED_OP_* */
__u32   op;
__u32   fd;
+   /** AMDGPU_CTX_PRIORITY_* */
__s32   priority;
__u32   ctx_id;
 };
-- 
2.21.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amd/amdgpu: cast mem->num_pages to 64-bits when shifting

[StDenis, Tom]

On 32-bit hosts mem->num_pages is 32-bits and can overflow
when shifted.  Add a cast to avoid this.

Signed-off-by: Tom St Denis 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
index c963ad86072e..31895d3c33de 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
@@ -279,14 +279,16 @@ static int amdgpu_vram_mgr_new(struct 
ttm_mem_type_manager *man,
uint64_t vis_usage = 0;
unsigned i;
int r;
+   uint64_t mem_bytes;
 
lpfn = place->lpfn;
if (!lpfn)
lpfn = man->size;
 
/* bail out quickly if there's likely not enough VRAM for this BO */
-   if (atomic64_add_return(mem->num_pages << PAGE_SHIFT, >usage) > 
adev->gmc.mc_vram_size) {
-   atomic64_sub(mem->num_pages << PAGE_SHIFT, >usage);
+   mem_bytes = (u64)mem->num_pages << PAGE_SHIFT;
+   if (atomic64_add_return(mem_bytes, >usage) > 
adev->gmc.mc_vram_size) {
+   atomic64_sub(mem_bytes, >usage);
mem->mm_node = NULL;
return 0;
}
@@ -308,7 +310,7 @@ static int amdgpu_vram_mgr_new(struct ttm_mem_type_manager 
*man,
nodes = kvmalloc_array((uint32_t)num_nodes, sizeof(*nodes),
   GFP_KERNEL | __GFP_ZERO);
if (!nodes) {
-   atomic64_sub(mem->num_pages << PAGE_SHIFT, >usage);
+   atomic64_sub(mem_bytes, >usage);
return -ENOMEM;
}
 
-- 
2.21.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/06/14, Koenig, Christian wrote:
> Am 14.06.19 um 17:53 schrieb Emil Velikov:
> > On 2019/06/14, Koenig, Christian wrote:
> >> Am 14.06.19 um 14:09 schrieb Emil Velikov:
> >>> On 2019/05/27, Emil Velikov wrote:
> >>> [SNIP]
> >>> Hi Christian,
> >>>
> >>>
> >>> In the following, I would like to summarise and emphasize the need for
> >>> DRM_AUTH removal. I would kindly ask you to spend a couple of minutes
> >>> extra reading it.
> >>>
> >>>
> >>> Today DRM drivers* do not make any distinction between primary and
> >>> render node clients.
> >> That is actually not 100% correct. We have a special case where a DRM
> >> master is allowed to change the priority of render node clients.
> >>
> > Can you provide a link? I cannot find that code.
> 
> See amdgpu_sched_ioctl().
> 
> >>> Thus for a render capable driver, any premise of
> >>> separation, security or otherwise imposed via DRM_AUTH is a fallacy.
> >> Yeah, that's what I agree on. I just don't think that removing DRM_AUTH
> >> now is the right direction to take.
> >>
> > Could have been clearer - I'm talking about DRM_AUTH | DRM_RENDER_ALLOW
> > ioctls.
> >
> > That aside, can you propose an alternative solution that addresses this
> > and the second point just below?
> 
> Give me a few days to work on this, it's already Friday 6pm here.
> 
Great thanks. Fwiw I was asking for a ideas/proposal, was not expecting you
to write the patches ;-)

Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Koenig, Christian]

Am 14.06.19 um 17:53 schrieb Emil Velikov:
> On 2019/06/14, Koenig, Christian wrote:
>> Am 14.06.19 um 14:09 schrieb Emil Velikov:
>>> On 2019/05/27, Emil Velikov wrote:
>>> [SNIP]
>>> Hi Christian,
>>>
>>>
>>> In the following, I would like to summarise and emphasize the need for
>>> DRM_AUTH removal. I would kindly ask you to spend a couple of minutes
>>> extra reading it.
>>>
>>>
>>> Today DRM drivers* do not make any distinction between primary and
>>> render node clients.
>> That is actually not 100% correct. We have a special case where a DRM
>> master is allowed to change the priority of render node clients.
>>
> Can you provide a link? I cannot find that code.

See amdgpu_sched_ioctl().

>>> Thus for a render capable driver, any premise of
>>> separation, security or otherwise imposed via DRM_AUTH is a fallacy.
>> Yeah, that's what I agree on. I just don't think that removing DRM_AUTH
>> now is the right direction to take.
>>
> Could have been clearer - I'm talking about DRM_AUTH | DRM_RENDER_ALLOW
> ioctls.
>
> That aside, can you propose an alternative solution that addresses this
> and the second point just below?

Give me a few days to work on this, it's already Friday 6pm here.

Christian.

>
>>> Considering the examples of flaky or outright missing drmAuth in prime
>>> open-source projects (mesa, kmscube, libva) we can reasonably assume
>>> other projects exbibit the same problem.
>>>
>>> For these and/or other reasons, two DRM drivers have dropped DRM_AUTH
>>> since day one.
>>>
>>> Since we are interested in providing consistent and predictable
>>> behaviour to user-space, dropping DRM_AUTH seems to be the most
>>> effective way forward.
>> Well and what do you do with drivers which doesn't implement render nodes?
>>
> AFAICT there is a single non-DC driver which does not expose - QXL.
> I would expect it runs on a rather customised stack.
>
> Would be great to fix that, but ETIME and it's the only exception to the
> rule.
>
>
>>> Of course, I'd be more than happy to hear for any other way to achieve
>>> the goal outlined.
>>>
>>> Based on the series, other maintainers agree with the idea/goal here.
>>> Amdgpu not following the same pattern would compromise predictability
>>> across drivers and complicate userspace, so I would kindly ask you to
>>> reconsider.
>>>
>>> Alternatively, I see two ways to special case:
>>>- New flag annotating amdgpu/radeon - akin to the one proposed earlier
>>>- Check for amdgpu/radeon in core DRM
>> I perfectly agree that I don't want any special handling for amdgpu/radeon.
>>
>> My key point is that with DRM_AUTH we created an authorization and
>> authentication mess in DRM which is functionality which doesn't belong
>> into the DRM subsystem in the first place.
>>
> Precisely and I've outlined below how to resolve this in the long run.
>
>>> Now on your pain point - not allowing render iocts via the primary node,
>>> and how this patch could make it harder to achieve that goal.
>>>
>>> While I love the idea, there are number of obstacles that prevent us
>>> from doing so at this time:
>>>- Ensuring both old and new userspace does not regress
>> Yeah, agree totally. That's why I said we should probably start doing
>> this for only for upcoming hardware generations.
>>
> That will side-step the regression issue, but will enforce driver
> specific behaviour outlined before.
>
>>>- Drivers (QXL, others?) do not expose a render node
>> Well isn't that is a rather big problem for the removal of DRM_AUTH in
>> general?
>>
>> E.g. at least QXL would then expose functionality on the primary node
>> without authentication.
>>
> With this series QXL remains functionally unchanged. I would love to fix
> that as well yet ETIME as mentioned above.
>
>>>- We want to avoid driver specific behaviour
>>>
>>> The only way forward that I can see is:
>>>- Address QXL/others to expose render nodes
>>>- Add a Kconfig toggle to disable !KMS ioctls via the primary node
>>>- Jump-start ^^ with distribution X
>>>- Fix user-space fallouts
>>>- X months down the line, flip the Kconfig
>>>- In case of regressions - revert the KConfig, goto Fix user-space...
>> Well that at least sounds like a plan :) Let's to this!
>>
> We're talking about months if not years until it comes to fruition. We
> need something quicker.
>
> That said, I'm quite happy to take the first stab, yet this is not a
> replacement for this series.
>
>>> That said, the proposal will not conflict with the DRM_AUTH removal. If
>>> anything it is step 0.5 of the grand master plan.
>> That's the point I strongly disagree on.
>>
>> By lowering the DRM_AUTH restriction you are encouraging userspace to
>> use the shortcut and use the primary node for rendering instead of
>> fixing the code and using the render node.
>>
> Have to disagree here. After working on the user-space side and fixing
> issues in various projects I can honestly say that most user-space is
> sane and 

Re: [pull] amdgpu drm-fixes-5.2

[Daniel Vetter]

On Fri, Jun 14, 2019 at 5:49 PM Daniel Vetter  wrote:
>
> On Wed, Jun 12, 2019 at 09:18:56PM -0500, Alex Deucher wrote:
> > Hi Dave, Daniel,
> >
> > Fixes for 5.2:
> > - Extend previous vce fix for resume to uvd and vcn
> > - Fix bounds checking in ras debugfs interface
> > - Fix a regression on SI using amdgpu
> >
> > The following changes since commit 671e2ee5ee2127179ca884b439ab6001a623edd6:
> >
> >   Merge branch 'linux-5.2' of git://github.com/skeggsb/linux into drm-fixes 
> > (2019-06-07 17:16:00 +1000)
>
> Somehow missed this one, but just found it before I wanted to push out the
> -fixes pull to Linus ...
>
> > are available in the Git repository at:
> >
> >   git://people.freedesktop.org/~agd5f/linux drm-fixes-5.2
>
> Pulled, thanks.

Was a bit a lie, the script was still running, and complained that I
didn't add a proper merge commit message. Can you pls use annotated
tags, then the tooling we use makes this happen automatically? dim
pull-request for cheatsheet, if you need one.

Cheers, Daniel

> -Daniel
>
> >
> > for you to fetch changes up to f3a5231c8f14acd42845e9e60f506b4e948f0e68:
> >
> >   drm/amdgpu: return 0 by default in amdgpu_pm_load_smu_firmware 
> > (2019-06-12 20:39:49 -0500)
> >
> > 
> > Alex Deucher (1):
> >   drm/amdgpu: return 0 by default in amdgpu_pm_load_smu_firmware
> >
> > Dan Carpenter (1):
> >   drm/amdgpu: Fix bounds checking in amdgpu_ras_is_supported()
> >
> > Shirish S (1):
> >   drm/amdgpu/{uvd,vcn}: fetch ring's read_ptr after alloc
> >
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c  | 4 ++--
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h | 2 ++
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 4 +++-
> >  drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c   | 5 -
> >  drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c   | 5 -
> >  5 files changed, 15 insertions(+), 5 deletions(-)
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/06/14, Koenig, Christian wrote:
> Am 14.06.19 um 14:09 schrieb Emil Velikov:
> > On 2019/05/27, Emil Velikov wrote:
> > [SNIP]
> > Hi Christian,
> >
> >
> > In the following, I would like to summarise and emphasize the need for
> > DRM_AUTH removal. I would kindly ask you to spend a couple of minutes
> > extra reading it.
> >
> >
> > Today DRM drivers* do not make any distinction between primary and
> > render node clients.
> 
> That is actually not 100% correct. We have a special case where a DRM 
> master is allowed to change the priority of render node clients.
> 
Can you provide a link? I cannot find that code.

> > Thus for a render capable driver, any premise of
> > separation, security or otherwise imposed via DRM_AUTH is a fallacy.
> 
> Yeah, that's what I agree on. I just don't think that removing DRM_AUTH 
> now is the right direction to take.
> 
Could have been clearer - I'm talking about DRM_AUTH | DRM_RENDER_ALLOW
ioctls.

That aside, can you propose an alternative solution that addresses this
and the second point just below?

> > Considering the examples of flaky or outright missing drmAuth in prime
> > open-source projects (mesa, kmscube, libva) we can reasonably assume
> > other projects exbibit the same problem.
> >
> > For these and/or other reasons, two DRM drivers have dropped DRM_AUTH
> > since day one.
> >
> > Since we are interested in providing consistent and predictable
> > behaviour to user-space, dropping DRM_AUTH seems to be the most
> > effective way forward.
> 
> Well and what do you do with drivers which doesn't implement render nodes?
> 
AFAICT there is a single non-DC driver which does not expose - QXL.
I would expect it runs on a rather customised stack.

Would be great to fix that, but ETIME and it's the only exception to the
rule.


> > Of course, I'd be more than happy to hear for any other way to achieve
> > the goal outlined.
> >
> > Based on the series, other maintainers agree with the idea/goal here.
> > Amdgpu not following the same pattern would compromise predictability
> > across drivers and complicate userspace, so I would kindly ask you to
> > reconsider.
> >
> > Alternatively, I see two ways to special case:
> >   - New flag annotating amdgpu/radeon - akin to the one proposed earlier
> >   - Check for amdgpu/radeon in core DRM
> 
> I perfectly agree that I don't want any special handling for amdgpu/radeon.
> 
> My key point is that with DRM_AUTH we created an authorization and 
> authentication mess in DRM which is functionality which doesn't belong 
> into the DRM subsystem in the first place.
> 
Precisely and I've outlined below how to resolve this in the long run.

> > Now on your pain point - not allowing render iocts via the primary node,
> > and how this patch could make it harder to achieve that goal.
> >
> > While I love the idea, there are number of obstacles that prevent us
> > from doing so at this time:
> >   - Ensuring both old and new userspace does not regress
> 
> Yeah, agree totally. That's why I said we should probably start doing 
> this for only for upcoming hardware generations.
> 
That will side-step the regression issue, but will enforce driver
specific behaviour outlined before.

> >   - Drivers (QXL, others?) do not expose a render node
> 
> Well isn't that is a rather big problem for the removal of DRM_AUTH in 
> general?
> 
> E.g. at least QXL would then expose functionality on the primary node 
> without authentication.
> 
With this series QXL remains functionally unchanged. I would love to fix
that as well yet ETIME as mentioned above.

> >   - We want to avoid driver specific behaviour
> >
> > The only way forward that I can see is:
> >   - Address QXL/others to expose render nodes
> >   - Add a Kconfig toggle to disable !KMS ioctls via the primary node
> >   - Jump-start ^^ with distribution X
> >   - Fix user-space fallouts
> >   - X months down the line, flip the Kconfig
> >   - In case of regressions - revert the KConfig, goto Fix user-space...
> 
> Well that at least sounds like a plan :) Let's to this!
> 
We're talking about months if not years until it comes to fruition. We
need something quicker.

That said, I'm quite happy to take the first stab, yet this is not a
replacement for this series.

> > That said, the proposal will not conflict with the DRM_AUTH removal. If
> > anything it is step 0.5 of the grand master plan.
> 
> That's the point I strongly disagree on.
> 
> By lowering the DRM_AUTH restriction you are encouraging userspace to 
> use the shortcut and use the primary node for rendering instead of 
> fixing the code and using the render node.
> 
Have to disagree here. After working on the user-space side and fixing
issues in various projects I can honestly say that most user-space is
sane and developers _care_ about doing things correctly.

> So at the end of the day userspace will most likely completely drop 
> support for the render node, simply for the reason that it became 
> superfluous. You can 

Re: [pull] amdgpu drm-fixes-5.2

[Daniel Vetter]

On Wed, Jun 12, 2019 at 09:18:56PM -0500, Alex Deucher wrote:
> Hi Dave, Daniel,
> 
> Fixes for 5.2:
> - Extend previous vce fix for resume to uvd and vcn
> - Fix bounds checking in ras debugfs interface
> - Fix a regression on SI using amdgpu
> 
> The following changes since commit 671e2ee5ee2127179ca884b439ab6001a623edd6:
> 
>   Merge branch 'linux-5.2' of git://github.com/skeggsb/linux into drm-fixes 
> (2019-06-07 17:16:00 +1000)

Somehow missed this one, but just found it before I wanted to push out the
-fixes pull to Linus ...

> are available in the Git repository at:
> 
>   git://people.freedesktop.org/~agd5f/linux drm-fixes-5.2

Pulled, thanks.
-Daniel

> 
> for you to fetch changes up to f3a5231c8f14acd42845e9e60f506b4e948f0e68:
> 
>   drm/amdgpu: return 0 by default in amdgpu_pm_load_smu_firmware (2019-06-12 
> 20:39:49 -0500)
> 
> 
> Alex Deucher (1):
>   drm/amdgpu: return 0 by default in amdgpu_pm_load_smu_firmware
> 
> Dan Carpenter (1):
>   drm/amdgpu: Fix bounds checking in amdgpu_ras_is_supported()
> 
> Shirish S (1):
>   drm/amdgpu/{uvd,vcn}: fetch ring's read_ptr after alloc
> 
>  drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c  | 4 ++--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h | 2 ++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 4 +++-
>  drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c   | 5 -
>  drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c   | 5 -
>  5 files changed, 15 insertions(+), 5 deletions(-)

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd: fix hotplug race at startup

[Daniel Vetter]

On Fri, Jun 14, 2019 at 07:29:23PM +0800, Young Xiao wrote:
> We should check mode_config_initialized flag in amdgpu_hotplug_work_func.
> 
> See commit 7f98ca454ad3 ("drm/radeon: fix hotplug race at startup") for 
> details.
> 
> Signed-off-by: Young Xiao <92siuy...@gmail.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> index af4c3b1..13186d6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
> @@ -85,6 +85,9 @@ static void amdgpu_hotplug_work_func(struct work_struct 
> *work)
>   struct drm_mode_config *mode_config = >mode_config;
>   struct drm_connector *connector;
>  
> + if (!adev->mode_info.mode_config_initialized)
> + return;

I think you want to delay your hotplug initialization until you're ready
to serve hotplug events, this here is fairly racy ...
-Daniel

> +
>   mutex_lock(_config->mutex);
>   list_for_each_entry(connector, _config->connector_list, head)
>   amdgpu_connector_hotplug(connector);
> -- 
> 2.7.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amd: fix hotplug race at startup

[Young Xiao]

We should check mode_config_initialized flag in amdgpu_hotplug_work_func.

See commit 7f98ca454ad3 ("drm/radeon: fix hotplug race at startup") for details.

Signed-off-by: Young Xiao <92siuy...@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
index af4c3b1..13186d6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c
@@ -85,6 +85,9 @@ static void amdgpu_hotplug_work_func(struct work_struct *work)
struct drm_mode_config *mode_config = >mode_config;
struct drm_connector *connector;
 
+   if (!adev->mode_info.mode_config_initialized)
+   return;
+
mutex_lock(_config->mutex);
list_for_each_entry(connector, _config->connector_list, head)
amdgpu_connector_hotplug(connector);
-- 
2.7.4

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Michel Dänzer]

On 2019-06-14 2:55 p.m., Koenig, Christian wrote:
> Am 14.06.19 um 14:09 schrieb Emil Velikov:
> 
>> That said, the proposal will not conflict with the DRM_AUTH removal. If
>> anything it is step 0.5 of the grand master plan.
> 
> That's the point I strongly disagree on.
> 
> By lowering the DRM_AUTH restriction you are encouraging userspace to 
> use the shortcut and use the primary node for rendering instead of 
> fixing the code and using the render node.
> 
> So at the end of the day userspace will most likely completely drop 
> support for the render node, simply for the reason that it became 
> superfluous. You can just open up the primary node and get the same 
> functionality.
> 
> I absolutely can't understand how you can argue that this won't make it 
> harder to cleanly separate rendering and primary node functionality.

FWIW, I agree with Christian on this.


Also FWIW, the solution I'm working on for
https://bugs.freedesktop.org/110903 should allow making libdrm_amdgpu
always use a render node for rendering. For backwards compatibility
it'll probably require adding a new variant of amdgpu_device_initialize
though, since existing users may rely on the first amdgpu_device for a
GPU using the DRM file description passed to amdgpu_device_initialize
for rendering.


-- 
Earthling Michel Dänzer   |  https://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Need to set the baco cap before baco reset

[Yang, Philip]

Hi Emily,

I am not familiar with vbios and driver init part, just based on my 
experience, the patch don't modify amdgpu_get_bios but move 
amdgpu_get_bios to amdgpu_device_ip_early_init from amdgpu_device_init, 
so amdgpu_get_bios is executed earlier. The kernel error message "BUG: 
kernel NULL pointer dereference" means something is not initialized. 
Please review the change. This issue blocks rocm release now.

Regards,
Philip

On 2019-06-13 11:19 p.m., Deng, Emily wrote:
> Hi Russell,
>   This patch will read vbios, and parse vbios to get the baco reset 
> feature bit.  From the call trace, it shows error in " amdgpu_get_bios ", but 
> this patch don't modify amdgpu_get_bios, and code before amdgpu_get_bios. 
> Please first check why it will has error when read vbios.
> 
> Best wishes
> Emily Deng
> 
> 
> 
>> -Original Message-
>> From: Russell, Kent 
>> Sent: Thursday, June 13, 2019 7:11 PM
>> To: Quan, Evan ; Deng, Emily
>> ; amd-gfx@lists.freedesktop.org
>> Cc: Deng, Emily 
>> Subject: RE: [PATCH] drm/amdgpu: Need to set the baco cap before baco reset
>>
>> Hi Emily,
>>
>> This patch caused a regression on MI25 (Chip 6860, VBIOS 113-D0513700-001)
>> machines where the driver would not boot. Note that this was not seen on
>> Vega10 Frontier (Chip 6863, VBIOS 113-D0501100-X09) or Radeon64 (Chip
>> 697f). Reverting this patch resolved the issue with no other work required 
>> and
>> was confirmed on all 3 machines.
>>
>> Here is the dmesg:
>>
>> [3.908653] amdgpu :23:00.0: BAR 6: can't assign [??? 0x flags
>> 0x2000] (bogus alignment)
>> [3.908692] BUG: kernel NULL pointer dereference, address:
>> 0008
>> [3.908716] #PF: supervisor read access in kernel mode
>> [3.908734] #PF: error_code(0x) - not-present page
>> [3.908753] PGD 0 P4D 0
>> [3.908767] Oops:  [#1] SMP NOPTI
>> [3.909293] CPU: 8 PID: 409 Comm: kworker/8:1 Not tainted 5.2.0-rc1-kfd-
>> compute-roc-master-10734 #1
>> [3.909753] Hardware name: Inventec P47  
>> WC2071019001
>> /P47 , BIOS 0.64 04/09/2018
>> [3.910534] Workqueue: events work_for_cpu_fn
>> [3.910953] RIP: 0010:amdgpu_get_bios+0x3aa/0x580 [amdgpu]
>> [3.911314] Code: c0 48 c7 44 24 5c 00 00 00 00 48 c7 84 24 90 00 00 00 
>> 00 00
>> 00 00 48 89 d9 48 29 f9 83 c1 3c c1 e9 03 f3 48 ab 49 8b 44 24 38 <48> 8b 40 
>> 08
>> 48 85 c0 74 24 ba 3c 00 00 00 48 89 de 4c 89 e7 ff d0
>> [3.912069] RSP: 0018:a27dce28fc50 EFLAGS: 00010212
>> [3.912502] RAX:  RBX: a27dce28fcac RCX:
>> 
>> [3.912980] RDX:  RSI: 0082 RDI:
>> a27dce28fce8
>> [3.913467] RBP:  R08: 0001 R09:
>> 079a
>> [3.913940] R10:  R11: 0001 R12:
>> 88d657af
>> [3.914349] R13: c0c38120 R14: a27dce28fc68 R15:
>> 88d657af
>> [3.914767] FS:  () GS:88d65f40()
>> knlGS:
>> [3.915203] CS:  0010 DS:  ES:  CR0: 80050033
>> [3.915637] CR2: 0008 CR3: 003e7540a000 CR4:
>> 003406e0
>> [3.916075] Call Trace:
>> [3.916522]  ? pcie_capability_clear_and_set_word+0x53/0x80
>> [3.917014]  amdgpu_device_init+0x923/0x1820 [amdgpu]
>> [3.917515]  amdgpu_driver_load_kms+0x71/0x310 [amdgpu]
>> [3.917997]  drm_dev_register+0x113/0x1a0 [drm]
>> [3.918514]  amdgpu_pci_probe+0xb8/0x150 [amdgpu]
>> [3.919003]  ? __pm_runtime_resume+0x54/0x70
>> [3.919270] usb 1-2: New USB device found, idVendor=14dd, idProduct=1005,
>> bcdDevice= 0.00
>> [3.919498]  local_pci_probe+0x3d/0x90
>> [3.919503]  ? __schedule+0x3de/0x690
>> [3.920374] usb 1-2: New USB device strings: Mfr=1, Product=2,
>> SerialNumber=3
>> [3.921137]  work_for_cpu_fn+0x10/0x20
>> [3.922028] usb 1-2: Product: D2CIM-VUSB
>> [3.922815]  process_one_work+0x159/0x3e0
>> [3.923633] usb 1-2: Manufacturer: Raritan
>> [3.923635] usb 1-2: SerialNumber: EFFB212D0A6E32F
>> [3.924416]  worker_thread+0x22b/0x440
>> [3.924419]  ? rescuer_thread+0x350/0x350
>> [3.927812]  kthread+0xf6/0x130
>> [3.928157] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
>> [3.928365]  ? kthread_destroy_worker+0x40/0x40
>> [3.929401] ata1.00: ATA-10: INTEL SSDSC2KG960G7, SCV10100, max
>> UDMA/133
>> [3.930101]  ret_from_fork+0x1f/0x30
>> [3.930103] Modules linked in: amdgpu(+) crct10dif_pclmul crc32_pclmul
>> ghash_clmulni_intel ast amd_iommu_v2 aesni_intel gpu_sched i2c_algo_bit
>> aes_x86_64 ttm crypto_simd drm_kms_helper cryptd glue_helper
>> syscopyarea sysfillrect ahci sysimgblt libahci fb_sys_fops ixgbe(+) drm dca
>> mdio
>> [3.930965] ata1.00: 1875385008 sectors, multi 1: LBA48 NCQ (depth 32)
>> [3.931085] ata1.00: configured for UDMA/133
>> [3.931809] CR2: 0008
>> [3.934723] 

Re: [PATCH xf86-video-ati 3/3] Remove dri2_drawable_crtc parameter consider_disabled

[Deucher, Alexander]

Series is:
Reviewed-by: Alex Deucher 

From: amd-gfx  on behalf of Michel 
Dänzer 
Sent: Friday, June 14, 2019 5:27 AM
To: amd-gfx@lists.freedesktop.org
Subject: [PATCH xf86-video-ati 3/3] Remove dri2_drawable_crtc parameter 
consider_disabled

From: Michel Dänzer 

All callers were passing TRUE.

(Ported from amdgpu commit ea19a5207054bb159fc7fb6d88e0ceb10c3da010)

Signed-off-by: Michel Dänzer 
---
 src/radeon_dri2.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
index c59d799ff..66a223d87 100644
--- a/src/radeon_dri2.c
+++ b/src/radeon_dri2.c
@@ -509,11 +509,11 @@ static Bool radeon_dri2_get_crtc_msc(xf86CrtcPtr crtc, 
CARD64 *ust, CARD64 *msc)
 }

 static
-xf86CrtcPtr radeon_dri2_drawable_crtc(DrawablePtr pDraw, Bool 
consider_disabled)
+xf86CrtcPtr radeon_dri2_drawable_crtc(DrawablePtr pDraw)
 {
 ScreenPtr pScreen = pDraw->pScreen;
 ScrnInfoPtr pScrn = xf86ScreenToScrn(pScreen);
-xf86CrtcPtr crtc = radeon_pick_best_crtc(pScrn, consider_disabled,
+xf86CrtcPtr crtc = radeon_pick_best_crtc(pScrn, TRUE,
   pDraw->x, pDraw->x + 
pDraw->width,
   pDraw->y, pDraw->y + 
pDraw->height);

@@ -928,7 +928,7 @@ CARD32 radeon_dri2_extrapolate_msc_delay(xf86CrtcPtr crtc, 
CARD64 *target_msc,
  */
 static int radeon_dri2_get_msc(DrawablePtr draw, CARD64 *ust, CARD64 *msc)
 {
-xf86CrtcPtr crtc = radeon_dri2_drawable_crtc(draw, TRUE);
+xf86CrtcPtr crtc = radeon_dri2_drawable_crtc(draw);

 /* Drawable not displayed, make up a value */
 if (!crtc) {
@@ -1043,7 +1043,7 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr 
client, DrawablePtr draw,
 ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
 DRI2FrameEventPtr wait_info = NULL;
 uintptr_t drm_queue_seq = 0;
-xf86CrtcPtr crtc = radeon_dri2_drawable_crtc(draw, TRUE);
+xf86CrtcPtr crtc = radeon_dri2_drawable_crtc(draw);
 uint32_t msc_delta;
 uint32_t seq;
 CARD64 current_msc;
@@ -1192,7 +1192,7 @@ static int radeon_dri2_schedule_swap(ClientPtr client, 
DrawablePtr draw,
 {
 ScreenPtr screen = draw->pScreen;
 ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
-xf86CrtcPtr crtc = radeon_dri2_drawable_crtc(draw, TRUE);
+xf86CrtcPtr crtc = radeon_dri2_drawable_crtc(draw);
 uint32_t msc_delta;
 drmVBlankSeqType type;
 uint32_t seq;
--
2.20.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/powerplay: detect version of smu backend

[William Lewis]

On 6/14/19 2:01 AM, Prike Liang wrote:
> Change-Id: Ib050c8cf0c2c5af4c1f747cf596860f9be01a2d3
> Signed-off-by: Prike Liang 
> ---
>   drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c | 1 +
>   drivers/gpu/drm/amd/powerplay/inc/hwmgr.h   | 1 +
>   drivers/gpu/drm/amd/powerplay/smumgr/ci_smumgr.c| 1 +
>   drivers/gpu/drm/amd/powerplay/smumgr/fiji_smumgr.c  | 1 +
>   drivers/gpu/drm/amd/powerplay/smumgr/iceland_smumgr.c   | 1 +
>   drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c | 1 +
>   drivers/gpu/drm/amd/powerplay/smumgr/smu10_smumgr.c | 1 +
>   drivers/gpu/drm/amd/powerplay/smumgr/smu8_smumgr.c  | 1 +
>   drivers/gpu/drm/amd/powerplay/smumgr/tonga_smumgr.c | 1 +
>   drivers/gpu/drm/amd/powerplay/smumgr/vega10_smumgr.c| 1 +
>   drivers/gpu/drm/amd/powerplay/smumgr/vega12_smumgr.c| 1 +
>   drivers/gpu/drm/amd/powerplay/smumgr/vega20_smumgr.c| 1 +
>   drivers/gpu/drm/amd/powerplay/smumgr/vegam_smumgr.c | 1 +
>   13 files changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c 
> b/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
> index f1d326c..b996819 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
> @@ -194,6 +194,7 @@ int hwmgr_sw_init(struct pp_hwmgr *hwmgr)
>   return -EINVAL;
>   
>   phm_register_irq_handlers(hwmgr);
> + pr_info("hwmgr_sw_init smu backed is %s\n",hwmgr->smumgr_funcs->name);
s/backed/backend/
>   
>   return hwmgr->smumgr_funcs->smu_init(hwmgr);
>   }
> diff --git a/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h 
> b/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
> index c92999a..47dbecc 100644
> --- a/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
> +++ b/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
> @@ -190,6 +190,7 @@ struct phm_vce_clock_voltage_dependency_table {
>   };
>   
>   struct pp_smumgr_func {
> + char *name;
>   int (*smu_init)(struct pp_hwmgr  *hwmgr);
>   int (*smu_fini)(struct pp_hwmgr  *hwmgr);
>   int (*start_smu)(struct pp_hwmgr  *hwmgr);
> diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/ci_smumgr.c 
> b/drivers/gpu/drm/amd/powerplay/smumgr/ci_smumgr.c
> index 9ef57fc..022f3c8 100644
> --- a/drivers/gpu/drm/amd/powerplay/smumgr/ci_smumgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/smumgr/ci_smumgr.c
> @@ -2935,6 +2935,7 @@ static int ci_update_smc_table(struct pp_hwmgr *hwmgr, 
> uint32_t type)
>   }
>   
>   const struct pp_smumgr_func ci_smu_funcs = {
> + .name = "ci_smu",
>   .smu_init = ci_smu_init,
>   .smu_fini = ci_smu_fini,
>   .start_smu = ci_start_smu,
> diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/fiji_smumgr.c 
> b/drivers/gpu/drm/amd/powerplay/smumgr/fiji_smumgr.c
> index 0ce85b7..da025b1 100644
> --- a/drivers/gpu/drm/amd/powerplay/smumgr/fiji_smumgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/smumgr/fiji_smumgr.c
> @@ -2643,6 +2643,7 @@ static int fiji_update_dpm_settings(struct pp_hwmgr 
> *hwmgr,
>   }
>   
>   const struct pp_smumgr_func fiji_smu_funcs = {
> + .name = "fiji_smu",
>   .smu_init = _smu_init,
>   .smu_fini = _smu_fini,
>   .start_smu = _start_smu,
> diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/iceland_smumgr.c 
> b/drivers/gpu/drm/amd/powerplay/smumgr/iceland_smumgr.c
> index f24f13d..f414f22 100644
> --- a/drivers/gpu/drm/amd/powerplay/smumgr/iceland_smumgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/smumgr/iceland_smumgr.c
> @@ -2661,6 +2661,7 @@ static bool iceland_is_dpm_running(struct pp_hwmgr 
> *hwmgr)
>   }
>   
>   const struct pp_smumgr_func iceland_smu_funcs = {
> + .name = "iceland_smu",
>   .smu_init = _smu_init,
>   .smu_fini = _smu_fini,
>   .start_smu = _start_smu,
> diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c 
> b/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c
> index 0d8958e..fbac2d3 100644
> --- a/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c
> @@ -2550,6 +2550,7 @@ static int polaris10_update_dpm_settings(struct 
> pp_hwmgr *hwmgr,
>   }
>   
>   const struct pp_smumgr_func polaris10_smu_funcs = {
> + .name = "polaris10_smu",
>   .smu_init = polaris10_smu_init,
>   .smu_fini = smu7_smu_fini,
>   .start_smu = polaris10_start_smu,
> diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/smu10_smumgr.c 
> b/drivers/gpu/drm/amd/powerplay/smumgr/smu10_smumgr.c
> index 6d11076a..ca66035 100644
> --- a/drivers/gpu/drm/amd/powerplay/smumgr/smu10_smumgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/smumgr/smu10_smumgr.c
> @@ -291,6 +291,7 @@ static int smu10_smc_table_manager(struct pp_hwmgr 
> *hwmgr, uint8_t *table, uint1
>   
>   
>   const struct pp_smumgr_func smu10_smu_funcs = {
> + .name = "smu10_smu",
>   .smu_init = _smu_init,
>   .smu_fini = _smu_fini,
>   .start_smu = _start_smu,
> diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/smu8_smumgr.c 
> 

Re: [PATCH] drm/amd/powerplay: detect version of smu backend

[Deucher, Alexander]

Reviewed-by: Alex Deucher 

From: amd-gfx  on behalf of Prike Liang 

Sent: Friday, June 14, 2019 3:01 AM
To: amd-gfx@lists.freedesktop.org
Cc: Liang, Prike; Huang, Ray; Feng, Kenneth; Quan, Evan
Subject: [PATCH] drm/amd/powerplay: detect version of smu backend

Change-Id: Ib050c8cf0c2c5af4c1f747cf596860f9be01a2d3
Signed-off-by: Prike Liang 
---
 drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c | 1 +
 drivers/gpu/drm/amd/powerplay/inc/hwmgr.h   | 1 +
 drivers/gpu/drm/amd/powerplay/smumgr/ci_smumgr.c| 1 +
 drivers/gpu/drm/amd/powerplay/smumgr/fiji_smumgr.c  | 1 +
 drivers/gpu/drm/amd/powerplay/smumgr/iceland_smumgr.c   | 1 +
 drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c | 1 +
 drivers/gpu/drm/amd/powerplay/smumgr/smu10_smumgr.c | 1 +
 drivers/gpu/drm/amd/powerplay/smumgr/smu8_smumgr.c  | 1 +
 drivers/gpu/drm/amd/powerplay/smumgr/tonga_smumgr.c | 1 +
 drivers/gpu/drm/amd/powerplay/smumgr/vega10_smumgr.c| 1 +
 drivers/gpu/drm/amd/powerplay/smumgr/vega12_smumgr.c| 1 +
 drivers/gpu/drm/amd/powerplay/smumgr/vega20_smumgr.c| 1 +
 drivers/gpu/drm/amd/powerplay/smumgr/vegam_smumgr.c | 1 +
 13 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c 
b/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
index f1d326c..b996819 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
@@ -194,6 +194,7 @@ int hwmgr_sw_init(struct pp_hwmgr *hwmgr)
 return -EINVAL;

 phm_register_irq_handlers(hwmgr);
+   pr_info("hwmgr_sw_init smu backed is %s\n",hwmgr->smumgr_funcs->name);

 return hwmgr->smumgr_funcs->smu_init(hwmgr);
 }
diff --git a/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h 
b/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
index c92999a..47dbecc 100644
--- a/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
+++ b/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
@@ -190,6 +190,7 @@ struct phm_vce_clock_voltage_dependency_table {
 };

 struct pp_smumgr_func {
+   char *name;
 int (*smu_init)(struct pp_hwmgr  *hwmgr);
 int (*smu_fini)(struct pp_hwmgr  *hwmgr);
 int (*start_smu)(struct pp_hwmgr  *hwmgr);
diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/ci_smumgr.c 
b/drivers/gpu/drm/amd/powerplay/smumgr/ci_smumgr.c
index 9ef57fc..022f3c8 100644
--- a/drivers/gpu/drm/amd/powerplay/smumgr/ci_smumgr.c
+++ b/drivers/gpu/drm/amd/powerplay/smumgr/ci_smumgr.c
@@ -2935,6 +2935,7 @@ static int ci_update_smc_table(struct pp_hwmgr *hwmgr, 
uint32_t type)
 }

 const struct pp_smumgr_func ci_smu_funcs = {
+   .name = "ci_smu",
 .smu_init = ci_smu_init,
 .smu_fini = ci_smu_fini,
 .start_smu = ci_start_smu,
diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/fiji_smumgr.c 
b/drivers/gpu/drm/amd/powerplay/smumgr/fiji_smumgr.c
index 0ce85b7..da025b1 100644
--- a/drivers/gpu/drm/amd/powerplay/smumgr/fiji_smumgr.c
+++ b/drivers/gpu/drm/amd/powerplay/smumgr/fiji_smumgr.c
@@ -2643,6 +2643,7 @@ static int fiji_update_dpm_settings(struct pp_hwmgr 
*hwmgr,
 }

 const struct pp_smumgr_func fiji_smu_funcs = {
+   .name = "fiji_smu",
 .smu_init = _smu_init,
 .smu_fini = _smu_fini,
 .start_smu = _start_smu,
diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/iceland_smumgr.c 
b/drivers/gpu/drm/amd/powerplay/smumgr/iceland_smumgr.c
index f24f13d..f414f22 100644
--- a/drivers/gpu/drm/amd/powerplay/smumgr/iceland_smumgr.c
+++ b/drivers/gpu/drm/amd/powerplay/smumgr/iceland_smumgr.c
@@ -2661,6 +2661,7 @@ static bool iceland_is_dpm_running(struct pp_hwmgr *hwmgr)
 }

 const struct pp_smumgr_func iceland_smu_funcs = {
+   .name = "iceland_smu",
 .smu_init = _smu_init,
 .smu_fini = _smu_fini,
 .start_smu = _start_smu,
diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c 
b/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c
index 0d8958e..fbac2d3 100644
--- a/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c
+++ b/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c
@@ -2550,6 +2550,7 @@ static int polaris10_update_dpm_settings(struct pp_hwmgr 
*hwmgr,
 }

 const struct pp_smumgr_func polaris10_smu_funcs = {
+   .name = "polaris10_smu",
 .smu_init = polaris10_smu_init,
 .smu_fini = smu7_smu_fini,
 .start_smu = polaris10_start_smu,
diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/smu10_smumgr.c 
b/drivers/gpu/drm/amd/powerplay/smumgr/smu10_smumgr.c
index 6d11076a..ca66035 100644
--- a/drivers/gpu/drm/amd/powerplay/smumgr/smu10_smumgr.c
+++ b/drivers/gpu/drm/amd/powerplay/smumgr/smu10_smumgr.c
@@ -291,6 +291,7 @@ static int smu10_smc_table_manager(struct pp_hwmgr *hwmgr, 
uint8_t *table, uint1


 const struct pp_smumgr_func smu10_smu_funcs = {
+   .name = "smu10_smu",
 .smu_init = _smu_init,
 .smu_fini = _smu_fini,
 .start_smu = _start_smu,
diff --git 

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Koenig, Christian]

Am 14.06.19 um 14:09 schrieb Emil Velikov:
> On 2019/05/27, Emil Velikov wrote:
> [SNIP]
> Hi Christian,
>
>
> In the following, I would like to summarise and emphasize the need for
> DRM_AUTH removal. I would kindly ask you to spend a couple of minutes
> extra reading it.
>
>
> Today DRM drivers* do not make any distinction between primary and
> render node clients.

That is actually not 100% correct. We have a special case where a DRM 
master is allowed to change the priority of render node clients.

> Thus for a render capable driver, any premise of
> separation, security or otherwise imposed via DRM_AUTH is a fallacy.

Yeah, that's what I agree on. I just don't think that removing DRM_AUTH 
now is the right direction to take.

> Considering the examples of flaky or outright missing drmAuth in prime
> open-source projects (mesa, kmscube, libva) we can reasonably assume
> other projects exbibit the same problem.
>
> For these and/or other reasons, two DRM drivers have dropped DRM_AUTH
> since day one.
>
> Since we are interested in providing consistent and predictable
> behaviour to user-space, dropping DRM_AUTH seems to be the most
> effective way forward.

Well and what do you do with drivers which doesn't implement render nodes?

> Of course, I'd be more than happy to hear for any other way to achieve
> the goal outlined.
>
> Based on the series, other maintainers agree with the idea/goal here.
> Amdgpu not following the same pattern would compromise predictability
> across drivers and complicate userspace, so I would kindly ask you to
> reconsider.
>
> Alternatively, I see two ways to special case:
>   - New flag annotating amdgpu/radeon - akin to the one proposed earlier
>   - Check for amdgpu/radeon in core DRM

I perfectly agree that I don't want any special handling for amdgpu/radeon.

My key point is that with DRM_AUTH we created an authorization and 
authentication mess in DRM which is functionality which doesn't belong 
into the DRM subsystem in the first place.

> Now on your pain point - not allowing render iocts via the primary node,
> and how this patch could make it harder to achieve that goal.
>
> While I love the idea, there are number of obstacles that prevent us
> from doing so at this time:
>   - Ensuring both old and new userspace does not regress

Yeah, agree totally. That's why I said we should probably start doing 
this for only for upcoming hardware generations.

>   - Drivers (QXL, others?) do not expose a render node

Well isn't that is a rather big problem for the removal of DRM_AUTH in 
general?

E.g. at least QXL would then expose functionality on the primary node 
without authentication.

>   - We want to avoid driver specific behaviour
>
> The only way forward that I can see is:
>   - Address QXL/others to expose render nodes
>   - Add a Kconfig toggle to disable !KMS ioctls via the primary node
>   - Jump-start ^^ with distribution X
>   - Fix user-space fallouts
>   - X months down the line, flip the Kconfig
>   - In case of regressions - revert the KConfig, goto Fix user-space...

Well that at least sounds like a plan :) Let's to this!

> That said, the proposal will not conflict with the DRM_AUTH removal. If
> anything it is step 0.5 of the grand master plan.

That's the point I strongly disagree on.

By lowering the DRM_AUTH restriction you are encouraging userspace to 
use the shortcut and use the primary node for rendering instead of 
fixing the code and using the render node.

So at the end of the day userspace will most likely completely drop 
support for the render node, simply for the reason that it became 
superfluous. You can just open up the primary node and get the same 
functionality.

I absolutely can't understand how you can argue that this won't make it 
harder to cleanly separate rendering and primary node functionality.

Regards,
Christian.

>
>
> Tl;Dr: Removing DRM_AUTH is orthogonal to not allowing render iocts via
> the primary node. Here's an idea how to achieve the latter.
>
>
> Thanks
> Emil

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm: radeon: add a missing break in evergreen_cs_handle_reg

[Mathieu Malaterre]

On Thu, Jan 17, 2019 at 9:40 PM Mathieu Malaterre  wrote:
>
> In commit dd220a00e8bd ("drm/radeon/kms: add support for streamout v7")
> case statements were added without a terminating break statement. This
> commit adds the missing break. This was discovered during a compilation
> with W=1.
>
> This commit removes the following warning:
>
>   drivers/gpu/drm/radeon/evergreen_cs.c:1301:11: warning: this statement may 
> fall through [-Wimplicit-fallthrough=]
>
> Suggested-by: Alex Deucher 
> Fixes: dd220a00e8bd ("drm/radeon/kms: add support for streamout v7")
> Signed-off-by: Mathieu Malaterre 
> ---
> v2: Add missing break statement, instead of marking it as fall through

Replaced by:

cc5034a5d293 drm/radeon/evergreen_cs: fix missing break in switch statement

>  drivers/gpu/drm/radeon/evergreen_cs.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/radeon/evergreen_cs.c 
> b/drivers/gpu/drm/radeon/evergreen_cs.c
> index f471537c852f..1e14c6921454 100644
> --- a/drivers/gpu/drm/radeon/evergreen_cs.c
> +++ b/drivers/gpu/drm/radeon/evergreen_cs.c
> @@ -1299,6 +1299,7 @@ static int evergreen_cs_handle_reg(struct 
> radeon_cs_parser *p, u32 reg, u32 idx)
> return -EINVAL;
> }
> ib[idx] += (u32)((reloc->gpu_offset >> 8) & 0x);
> +   break;
> case CB_TARGET_MASK:
> track->cb_target_mask = radeon_get_ib_value(p, idx);
> track->cb_dirty = true;
> --
> 2.19.2
>

Re: [PATCH 01/13] drm/amdgpu: introduce and honour DRM_FORCE_AUTH workaround

[Emil Velikov]

On 2019/05/27, Emil Velikov wrote:
> From: Emil Velikov 
> 
> Currently one can circumvent DRM_AUTH, when the ioctl is exposed via the
> render node. A seemingly deliberate design decision.
> 
> Hence we can drop the DRM_AUTH all together (details in follow-up patch)
> yet not all userspace checks if it's authenticated, but instead uses
> uncommon assumptions.
> 
> After days of digging through git log and testing, only a single (ab)use
> was spotted - the Mesa RADV driver, using the AMDGPU_INFO ioctl and
> assuming that failure implies lack of authentication.
> 
> Affected versions are:
>  - the whole 18.2.x series, which is EOL
>  - the whole 18.3.x series, which is EOL
>  - the 19.0.x series, prior to 19.0.4
> 
> Add a special quirk for that case, thus we can drop DRM_AUTH bits as
> mentioned earlier.
> 
> Since all the affected userspace is EOL, we also add a kconfig option
> to disable this quirk.
> 
> The whole approach is inspired by DRIVER_KMS_LEGACY_CONTEXT
> 
> Cc: Alex Deucher 
> Cc: Christian König 
> Cc: amd-gfx@lists.freedesktop.org
> Cc: David Airlie 
> Cc: Daniel Vetter 
> Signed-off-by: Emil Velikov 
> ---
>  drivers/gpu/drm/amd/amdgpu/Kconfig  | 16 
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 12 +++-
>  drivers/gpu/drm/drm_ioctl.c |  5 +
>  include/drm/drm_ioctl.h | 17 +
>  4 files changed, 49 insertions(+), 1 deletion(-)
> 

Hi Christian,


In the following, I would like to summarise and emphasize the need for
DRM_AUTH removal. I would kindly ask you to spend a couple of minutes
extra reading it.


Today DRM drivers* do not make any distinction between primary and
render node clients. Thus for a render capable driver, any premise of
separation, security or otherwise imposed via DRM_AUTH is a fallacy.

Considering the examples of flaky or outright missing drmAuth in prime
open-source projects (mesa, kmscube, libva) we can reasonably assume
other projects exbibit the same problem.

For these and/or other reasons, two DRM drivers have dropped DRM_AUTH
since day one.

Since we are interested in providing consistent and predictable
behaviour to user-space, dropping DRM_AUTH seems to be the most
effective way forward.

Of course, I'd be more than happy to hear for any other way to achieve
the goal outlined.

Based on the series, other maintainers agree with the idea/goal here.
Amdgpu not following the same pattern would compromise predictability
across drivers and complicate userspace, so I would kindly ask you to
reconsider.

Alternatively, I see two ways to special case:
 - New flag annotating amdgpu/radeon - akin to the one proposed earlier
 - Check for amdgpu/radeon in core DRM



Now on your pain point - not allowing render iocts via the primary node,
and how this patch could make it harder to achieve that goal.

While I love the idea, there are number of obstacles that prevent us
from doing so at this time:
 - Ensuring both old and new userspace does not regress
 - Drivers (QXL, others?) do not expose a render node
 - We want to avoid driver specific behaviour

The only way forward that I can see is:
 - Address QXL/others to expose render nodes
 - Add a Kconfig toggle to disable !KMS ioctls via the primary node
 - Jump-start ^^ with distribution X
 - Fix user-space fallouts
 - X months down the line, flip the Kconfig
 - In case of regressions - revert the KConfig, goto Fix user-space...


That said, the proposal will not conflict with the DRM_AUTH removal. If
anything it is step 0.5 of the grand master plan.


Tl;Dr: Removing DRM_AUTH is orthogonal to not allowing render iocts via
the primary node. Here's an idea how to achieve the latter.


Thanks
Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-ati 3/3] Remove dri2_drawable_crtc parameter consider_disabled

[Michel Dänzer]

From: Michel Dänzer 

All callers were passing TRUE.

(Ported from amdgpu commit ea19a5207054bb159fc7fb6d88e0ceb10c3da010)

Signed-off-by: Michel Dänzer 
---
 src/radeon_dri2.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
index c59d799ff..66a223d87 100644
--- a/src/radeon_dri2.c
+++ b/src/radeon_dri2.c
@@ -509,11 +509,11 @@ static Bool radeon_dri2_get_crtc_msc(xf86CrtcPtr crtc, 
CARD64 *ust, CARD64 *msc)
 }
 
 static
-xf86CrtcPtr radeon_dri2_drawable_crtc(DrawablePtr pDraw, Bool 
consider_disabled)
+xf86CrtcPtr radeon_dri2_drawable_crtc(DrawablePtr pDraw)
 {
 ScreenPtr pScreen = pDraw->pScreen;
 ScrnInfoPtr pScrn = xf86ScreenToScrn(pScreen);
-xf86CrtcPtr crtc = radeon_pick_best_crtc(pScrn, consider_disabled,
+xf86CrtcPtr crtc = radeon_pick_best_crtc(pScrn, TRUE,
  pDraw->x, pDraw->x + pDraw->width,
  pDraw->y, pDraw->y + 
pDraw->height);
 
@@ -928,7 +928,7 @@ CARD32 radeon_dri2_extrapolate_msc_delay(xf86CrtcPtr crtc, 
CARD64 *target_msc,
  */
 static int radeon_dri2_get_msc(DrawablePtr draw, CARD64 *ust, CARD64 *msc)
 {
-xf86CrtcPtr crtc = radeon_dri2_drawable_crtc(draw, TRUE);
+xf86CrtcPtr crtc = radeon_dri2_drawable_crtc(draw);
 
 /* Drawable not displayed, make up a value */
 if (!crtc) {
@@ -1043,7 +1043,7 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr 
client, DrawablePtr draw,
 ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
 DRI2FrameEventPtr wait_info = NULL;
 uintptr_t drm_queue_seq = 0;
-xf86CrtcPtr crtc = radeon_dri2_drawable_crtc(draw, TRUE);
+xf86CrtcPtr crtc = radeon_dri2_drawable_crtc(draw);
 uint32_t msc_delta;
 uint32_t seq;
 CARD64 current_msc;
@@ -1192,7 +1192,7 @@ static int radeon_dri2_schedule_swap(ClientPtr client, 
DrawablePtr draw,
 {
 ScreenPtr screen = draw->pScreen;
 ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
-xf86CrtcPtr crtc = radeon_dri2_drawable_crtc(draw, TRUE);
+xf86CrtcPtr crtc = radeon_dri2_drawable_crtc(draw);
 uint32_t msc_delta;
 drmVBlankSeqType type;
 uint32_t seq;
-- 
2.20.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-ati 1/3] dri2: reply to client for WaitMSC request in any case

[Michel Dänzer]

From: Flora Cui 

otherwise client would wait for reply forever and desktop appears hang.

Signed-off-by: Flora Cui 
(Ported from amdgpu commit fb06fb814700a47464abd756edcc76d0d776)
Signed-off-by: Michel Dänzer 
---
 src/radeon_dri2.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
index a9f14e8d8..3c04e6fe9 100644
--- a/src/radeon_dri2.c
+++ b/src/radeon_dri2.c
@@ -1156,6 +1156,9 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr 
client, DrawablePtr draw,
 out_complete:
 if (wait_info)
radeon_dri2_deferred_event(NULL, 0, wait_info);
+else
+   DRI2WaitMSCComplete(client, draw, 0, 0, 0);
+
 return TRUE;
 }
 
-- 
2.20.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-ati 2/3] dri2: Re-use previous CRTC when possible if pick_best_crtc returns NULL

[Michel Dänzer]

From: Michel Dänzer 

This way, the MSC will continue ticking at the rate of (the last mode
which was enabled for) that CRTC, instead of the client running
unthrottled.

(Ported from amdgpu commit 3109f088fdbd89c2ee8078625d4f073852492656)

Signed-off-by: Michel Dänzer 
---
 src/radeon_dri2.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
index 3c04e6fe9..c59d799ff 100644
--- a/src/radeon_dri2.c
+++ b/src/radeon_dri2.c
@@ -517,10 +517,12 @@ xf86CrtcPtr radeon_dri2_drawable_crtc(DrawablePtr pDraw, 
Bool consider_disabled)
  pDraw->x, pDraw->x + pDraw->width,
  pDraw->y, pDraw->y + 
pDraw->height);
 
-if (crtc && pDraw->type == DRAWABLE_WINDOW) {
+if (pDraw->type == DRAWABLE_WINDOW) {
struct dri2_window_priv *priv = get_dri2_window_priv((WindowPtr)pDraw);
 
-   if (priv->crtc && priv->crtc != crtc) {
+   if (!crtc) {
+   crtc = priv->crtc;
+   } else if (priv->crtc && priv->crtc != crtc) {
CARD64 ust, mscold, mscnew;
 
if (radeon_dri2_get_crtc_msc(priv->crtc, , ) &&
-- 
2.20.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Fix connector atomic_check compilation fail

[Maarten Lankhorst]

Op 14-06-2019 om 02:27 schreef Sean Paul:
> From: Sean Paul 
>
> I missed amdgpu in my connnector_helper_funcs->atomic_check conversion,
> which is understandably causing compilation failures.
>
> Fixes: 6f3b62781bbd ("drm: Convert connector_helper_funcs->atomic_check to 
> accept drm_atomic_state")
> Cc: Daniel Vetter 
> Cc: Ville Syrjälä 
> Cc: Jani Nikula 
> Cc: Joonas Lahtinen 
> Cc: Rodrigo Vivi 
> Cc: Ben Skeggs 
> Cc: Laurent Pinchart 
> Cc: Kieran Bingham 
> Cc: Eric Anholt 
> Cc: Laurent Pinchart  [for rcar lvds]
> Cc: Sean Paul 
> Cc: Maarten Lankhorst 
> Cc: Maxime Ripard 
> Cc: Sean Paul 
> Cc: David Airlie 
> Cc: Lyude Paul 
> Cc: Karol Herbst 
> Cc: Ilia Mirkin 
> Cc: dri-de...@lists.freedesktop.org
> Cc: intel-...@lists.freedesktop.org
> Cc: linux-renesas-...@vger.kernel.org
> Reported-by: Chris Wilson 
> Signed-off-by: Sean Paul 
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 3a723e553a193..3d5e828c3d284 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3953,9 +3953,10 @@ is_hdr_metadata_different(const struct 
> drm_connector_state *old_state,
>  
>  static int
>  amdgpu_dm_connector_atomic_check(struct drm_connector *conn,
> -  struct drm_connector_state *new_con_state)
> +  struct drm_atomic_state *state)
>  {
> - struct drm_atomic_state *state = new_con_state->state;
> + struct drm_connector_state *new_con_state =
> + drm_atomic_get_new_connector_state(state, conn);
>   struct drm_connector_state *old_con_state =
>   drm_atomic_get_old_connector_state(state, conn);
>   struct drm_crtc *crtc = new_con_state->crtc;

Thanks, applied. :)

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v3 hmm 05/12] mm/hmm: Remove duplicate condition test before wait_event_timeout

[Jason Gunthorpe]

From: Jason Gunthorpe 

The wait_event_timeout macro already tests the condition as its first
action, so there is no reason to open code another version of this, all
that does is skip the might_sleep() debugging in common cases, which is
not helpful.

Further, based on prior patches, we can now simplify the required condition
test:
 - If range is valid memory then so is range->hmm
 - If hmm_release() has run then range->valid is set to false
   at the same time as dead, so no reason to check both.
 - A valid hmm has a valid hmm->mm.

Allowing the return value of wait_event_timeout() (along with its internal
barriers) to compute the result of the function.

Signed-off-by: Jason Gunthorpe 
Reviewed-by: Ralph Campbell 
Reviewed-by: John Hubbard 
Reviewed-by: Ira Weiny 
Tested-by: Philip Yang 
---
v3
- Simplify the wait_event_timeout to not check valid
---
 include/linux/hmm.h | 13 ++---
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 1d97b6d62c5bcf..26e7c477490c4e 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -209,17 +209,8 @@ static inline unsigned long hmm_range_page_size(const 
struct hmm_range *range)
 static inline bool hmm_range_wait_until_valid(struct hmm_range *range,
  unsigned long timeout)
 {
-   /* Check if mm is dead ? */
-   if (range->hmm == NULL || range->hmm->dead || range->hmm->mm == NULL) {
-   range->valid = false;
-   return false;
-   }
-   if (range->valid)
-   return true;
-   wait_event_timeout(range->hmm->wq, range->valid || range->hmm->dead,
-  msecs_to_jiffies(timeout));
-   /* Return current valid status just in case we get lucky */
-   return range->valid;
+   return wait_event_timeout(range->hmm->wq, range->valid,
+ msecs_to_jiffies(timeout)) != 0;
 }
 
 /*
-- 
2.21.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v3 hmm 09/12] mm/hmm: Poison hmm_range during unregister

[Jason Gunthorpe]

From: Jason Gunthorpe 

Trying to misuse a range outside its lifetime is a kernel bug. Use poison
bytes to help detect this condition. Double unregister will reliably crash.

Signed-off-by: Jason Gunthorpe 
Reviewed-by: Jérôme Glisse 
Reviewed-by: John Hubbard 
Acked-by: Souptick Joarder 
Reviewed-by: Ralph Campbell 
Reviewed-by: Ira Weiny 
Tested-by: Philip Yang 
---
v2
- Keep range start/end valid after unregistration (Jerome)
v3
- Revise some comments (John)
- Remove start/end WARN_ON (Souptick)
---
 mm/hmm.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/mm/hmm.c b/mm/hmm.c
index e3e0a811a3a774..e214668cba3474 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -933,19 +933,21 @@ void hmm_range_unregister(struct hmm_range *range)
 {
struct hmm *hmm = range->hmm;
 
-   /* Sanity check this really should not happen. */
-   if (hmm == NULL || range->end <= range->start)
-   return;
-
mutex_lock(>lock);
list_del_rcu(>list);
mutex_unlock(>lock);
 
/* Drop reference taken by hmm_range_register() */
-   range->valid = false;
mmput(hmm->mm);
hmm_put(hmm);
-   range->hmm = NULL;
+
+   /*
+* The range is now invalid and the ref on the hmm is dropped, so
+ * poison the pointer.  Leave other fields in place, for the caller's
+ * use.
+ */
+   range->valid = false;
+   memset(>hmm, POISON_INUSE, sizeof(range->hmm));
 }
 EXPORT_SYMBOL(hmm_range_unregister);
 
-- 
2.21.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v3 hmm 03/12] mm/hmm: Hold a mmgrab from hmm to mm

[Jason Gunthorpe]

From: Jason Gunthorpe 

So long as a struct hmm pointer exists, so should the struct mm it is
linked too. Hold the mmgrab() as soon as a hmm is created, and mmdrop() it
once the hmm refcount goes to zero.

Since mmdrop() (ie a 0 kref on struct mm) is now impossible with a !NULL
mm->hmm delete the hmm_hmm_destroy().

Signed-off-by: Jason Gunthorpe 
Reviewed-by: Jérôme Glisse 
Reviewed-by: John Hubbard 
Reviewed-by: Ralph Campbell 
Reviewed-by: Ira Weiny 
Tested-by: Philip Yang 
---
v2:
 - Fix error unwind paths in hmm_get_or_create (Jerome/Jason)
---
 include/linux/hmm.h |  3 ---
 kernel/fork.c   |  1 -
 mm/hmm.c| 22 --
 3 files changed, 4 insertions(+), 22 deletions(-)

diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 1fba6979adf460..1d97b6d62c5bcf 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -577,14 +577,11 @@ static inline int hmm_vma_fault(struct hmm_mirror *mirror,
 }
 
 /* Below are for HMM internal use only! Not to be used by device driver! */
-void hmm_mm_destroy(struct mm_struct *mm);
-
 static inline void hmm_mm_init(struct mm_struct *mm)
 {
mm->hmm = NULL;
 }
 #else /* IS_ENABLED(CONFIG_HMM_MIRROR) */
-static inline void hmm_mm_destroy(struct mm_struct *mm) {}
 static inline void hmm_mm_init(struct mm_struct *mm) {}
 #endif /* IS_ENABLED(CONFIG_HMM_MIRROR) */
 
diff --git a/kernel/fork.c b/kernel/fork.c
index 75675b9bf6dfd3..c704c3cedee78d 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -673,7 +673,6 @@ void __mmdrop(struct mm_struct *mm)
WARN_ON_ONCE(mm == current->active_mm);
mm_free_pgd(mm);
destroy_context(mm);
-   hmm_mm_destroy(mm);
mmu_notifier_mm_destroy(mm);
check_mm(mm);
put_user_ns(mm->user_ns);
diff --git a/mm/hmm.c b/mm/hmm.c
index 22a97ada108b4e..080b17a2e87e2d 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -20,6 +20,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -73,6 +74,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
hmm->notifiers = 0;
hmm->dead = false;
hmm->mm = mm;
+   mmgrab(hmm->mm);
 
spin_lock(>page_table_lock);
if (!mm->hmm)
@@ -100,6 +102,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
mm->hmm = NULL;
spin_unlock(>page_table_lock);
 error:
+   mmdrop(hmm->mm);
kfree(hmm);
return NULL;
 }
@@ -121,6 +124,7 @@ static void hmm_free(struct kref *kref)
mm->hmm = NULL;
spin_unlock(>page_table_lock);
 
+   mmdrop(hmm->mm);
mmu_notifier_call_srcu(>rcu, hmm_free_rcu);
 }
 
@@ -129,24 +133,6 @@ static inline void hmm_put(struct hmm *hmm)
kref_put(>kref, hmm_free);
 }
 
-void hmm_mm_destroy(struct mm_struct *mm)
-{
-   struct hmm *hmm;
-
-   spin_lock(>page_table_lock);
-   hmm = mm_get_hmm(mm);
-   mm->hmm = NULL;
-   if (hmm) {
-   hmm->mm = NULL;
-   hmm->dead = true;
-   spin_unlock(>page_table_lock);
-   hmm_put(hmm);
-   return;
-   }
-
-   spin_unlock(>page_table_lock);
-}
-
 static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
 {
struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
-- 
2.21.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v3 hmm 07/12] mm/hmm: Use lockdep instead of comments

[Jason Gunthorpe]

From: Jason Gunthorpe 

So we can check locking at runtime.

Signed-off-by: Jason Gunthorpe 
Reviewed-by: Jérôme Glisse 
Reviewed-by: John Hubbard 
Reviewed-by: Ralph Campbell 
Acked-by: Souptick Joarder 
Tested-by: Philip Yang 
---
v2
- Fix missing & in lockdeps (Jason)
---
 mm/hmm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/hmm.c b/mm/hmm.c
index 58712d74edd585..c0f622f86223c2 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -253,11 +253,11 @@ static const struct mmu_notifier_ops hmm_mmu_notifier_ops 
= {
  *
  * To start mirroring a process address space, the device driver must register
  * an HMM mirror struct.
- *
- * THE mm->mmap_sem MUST BE HELD IN WRITE MODE !
  */
 int hmm_mirror_register(struct hmm_mirror *mirror, struct mm_struct *mm)
 {
+   lockdep_assert_held_exclusive(>mmap_sem);
+
/* Sanity check */
if (!mm || !mirror || !mirror->ops)
return -EINVAL;
-- 
2.21.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v3 hmm 11/12] mm/hmm: Remove confusing comment and logic from hmm_release

[Jason Gunthorpe]

From: Jason Gunthorpe 

hmm_release() is called exactly once per hmm. ops->release() cannot
accidentally trigger any action that would recurse back onto
hmm->mirrors_sem.

This fixes a use after-free race of the form:

   CPU0   CPU1
   hmm_release()
 up_write(>mirrors_sem);
 hmm_mirror_unregister(mirror)
  down_write(>mirrors_sem);
  up_write(>mirrors_sem);
  kfree(mirror)
 mirror->ops->release(mirror)

The only user we have today for ops->release is an empty function, so this
is unambiguously safe.

As a consequence of plugging this race drivers are not allowed to
register/unregister mirrors from within a release op.

Signed-off-by: Jason Gunthorpe 
Tested-by: Philip Yang 
---
 mm/hmm.c | 28 +---
 1 file changed, 9 insertions(+), 19 deletions(-)

diff --git a/mm/hmm.c b/mm/hmm.c
index 26af511cbdd075..c0d43302fd6b2f 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -137,26 +137,16 @@ static void hmm_release(struct mmu_notifier *mn, struct 
mm_struct *mm)
WARN_ON(!list_empty(>ranges));
mutex_unlock(>lock);
 
-   down_write(>mirrors_sem);
-   mirror = list_first_entry_or_null(>mirrors, struct hmm_mirror,
- list);
-   while (mirror) {
-   list_del_init(>list);
-   if (mirror->ops->release) {
-   /*
-* Drop mirrors_sem so the release callback can wait
-* on any pending work that might itself trigger a
-* mmu_notifier callback and thus would deadlock with
-* us.
-*/
-   up_write(>mirrors_sem);
+   down_read(>mirrors_sem);
+   list_for_each_entry(mirror, >mirrors, list) {
+   /*
+* Note: The driver is not allowed to trigger
+* hmm_mirror_unregister() from this thread.
+*/
+   if (mirror->ops->release)
mirror->ops->release(mirror);
-   down_write(>mirrors_sem);
-   }
-   mirror = list_first_entry_or_null(>mirrors,
- struct hmm_mirror, list);
}
-   up_write(>mirrors_sem);
+   up_read(>mirrors_sem);
 
hmm_put(hmm);
 }
@@ -286,7 +276,7 @@ void hmm_mirror_unregister(struct hmm_mirror *mirror)
struct hmm *hmm = mirror->hmm;
 
down_write(>mirrors_sem);
-   list_del_init(>list);
+   list_del(>list);
up_write(>mirrors_sem);
hmm_put(hmm);
memset(>hmm, POISON_INUSE, sizeof(mirror->hmm));
-- 
2.21.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v3 hmm 02/12] mm/hmm: Use hmm_mirror not mm as an argument for hmm_range_register

[Jason Gunthorpe]

From: Jason Gunthorpe 

Ralph observes that hmm_range_register() can only be called by a driver
while a mirror is registered. Make this clear in the API by passing in the
mirror structure as a parameter.

This also simplifies understanding the lifetime model for struct hmm, as
the hmm pointer must be valid as part of a registered mirror so all we
need in hmm_register_range() is a simple kref_get.

Suggested-by: Ralph Campbell 
Signed-off-by: Jason Gunthorpe 
Reviewed-by: John Hubbard 
Reviewed-by: Ralph Campbell 
Reviewed-by: Ira Weiny 
Tested-by: Philip Yang 
---
v2
- Include the oneline patch to nouveau_svm.c
---
 drivers/gpu/drm/nouveau/nouveau_svm.c |  2 +-
 include/linux/hmm.h   |  7 ---
 mm/hmm.c  | 13 -
 3 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c 
b/drivers/gpu/drm/nouveau/nouveau_svm.c
index 93ed43c413f0bb..8c92374afcf227 100644
--- a/drivers/gpu/drm/nouveau/nouveau_svm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
@@ -649,7 +649,7 @@ nouveau_svm_fault(struct nvif_notify *notify)
range.values = nouveau_svm_pfn_values;
range.pfn_shift = NVIF_VMM_PFNMAP_V0_ADDR_SHIFT;
 again:
-   ret = hmm_vma_fault(, true);
+   ret = hmm_vma_fault(>mirror, , true);
if (ret == 0) {
mutex_lock(>mutex);
if (!hmm_vma_range_done()) {
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index cb01cf1fa3c08b..1fba6979adf460 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -496,7 +496,7 @@ static inline bool hmm_mirror_mm_is_alive(struct hmm_mirror 
*mirror)
  * Please see Documentation/vm/hmm.rst for how to use the range API.
  */
 int hmm_range_register(struct hmm_range *range,
-  struct mm_struct *mm,
+  struct hmm_mirror *mirror,
   unsigned long start,
   unsigned long end,
   unsigned page_shift);
@@ -532,7 +532,8 @@ static inline bool hmm_vma_range_done(struct hmm_range 
*range)
 }
 
 /* This is a temporary helper to avoid merge conflict between trees. */
-static inline int hmm_vma_fault(struct hmm_range *range, bool block)
+static inline int hmm_vma_fault(struct hmm_mirror *mirror,
+   struct hmm_range *range, bool block)
 {
long ret;
 
@@ -545,7 +546,7 @@ static inline int hmm_vma_fault(struct hmm_range *range, 
bool block)
range->default_flags = 0;
range->pfn_flags_mask = -1UL;
 
-   ret = hmm_range_register(range, range->vma->vm_mm,
+   ret = hmm_range_register(range, mirror,
 range->start, range->end,
 PAGE_SHIFT);
if (ret)
diff --git a/mm/hmm.c b/mm/hmm.c
index f6956d78e3cb25..22a97ada108b4e 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -914,13 +914,13 @@ static void hmm_pfns_clear(struct hmm_range *range,
  * Track updates to the CPU page table see include/linux/hmm.h
  */
 int hmm_range_register(struct hmm_range *range,
-  struct mm_struct *mm,
+  struct hmm_mirror *mirror,
   unsigned long start,
   unsigned long end,
   unsigned page_shift)
 {
unsigned long mask = ((1UL << page_shift) - 1UL);
-   struct hmm *hmm;
+   struct hmm *hmm = mirror->hmm;
 
range->valid = false;
range->hmm = NULL;
@@ -934,20 +934,15 @@ int hmm_range_register(struct hmm_range *range,
range->start = start;
range->end = end;
 
-   hmm = hmm_get_or_create(mm);
-   if (!hmm)
-   return -EFAULT;
-
/* Check if hmm_mm_destroy() was call. */
-   if (hmm->mm == NULL || hmm->dead) {
-   hmm_put(hmm);
+   if (hmm->mm == NULL || hmm->dead)
return -EFAULT;
-   }
 
/* Initialize range to track CPU page table updates. */
mutex_lock(>lock);
 
range->hmm = hmm;
+   kref_get(>kref);
list_add_rcu(>list, >ranges);
 
/*
-- 
2.21.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v3 hmm 01/12] mm/hmm: fix use after free with struct hmm in the mmu notifiers

[Jason Gunthorpe]

From: Jason Gunthorpe 

mmu_notifier_unregister_no_release() is not a fence and the mmu_notifier
system will continue to reference hmm->mn until the srcu grace period
expires.

Resulting in use after free races like this:

 CPU0 CPU1
   
__mmu_notifier_invalidate_range_start()
 srcu_read_lock
 hlist_for_each ()
   // mn == hmm->mn
hmm_mirror_unregister()
  hmm_put()
hmm_free()
  mmu_notifier_unregister_no_release()
 hlist_del_init_rcu(hmm-mn->list)
   
mn->ops->invalidate_range_start(mn, range);
 mm_get_hmm()
  mm->hmm = NULL;
  kfree(hmm)
 mutex_lock(>lock);

Use SRCU to kfree the hmm memory so that the notifiers can rely on hmm
existing. Get the now-safe hmm struct through container_of and directly
check kref_get_unless_zero to lock it against free.

Signed-off-by: Jason Gunthorpe 
Reviewed-by: Ira Weiny 
Reviewed-by: John Hubbard 
Reviewed-by: Ralph Campbell 
Tested-by: Philip Yang 
---
v2:
- Spell 'free' properly (Jerome/Ralph)
v3:
- Have only one clearer comment about kref_get_unless_zero (John)
---
 include/linux/hmm.h |  1 +
 mm/hmm.c| 23 +--
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 7007123842ba76..cb01cf1fa3c08b 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -93,6 +93,7 @@ struct hmm {
struct mmu_notifier mmu_notifier;
struct rw_semaphore mirrors_sem;
wait_queue_head_t   wq;
+   struct rcu_head rcu;
longnotifiers;
booldead;
 };
diff --git a/mm/hmm.c b/mm/hmm.c
index 826816ab237799..f6956d78e3cb25 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -104,6 +104,11 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
return NULL;
 }
 
+static void hmm_free_rcu(struct rcu_head *rcu)
+{
+   kfree(container_of(rcu, struct hmm, rcu));
+}
+
 static void hmm_free(struct kref *kref)
 {
struct hmm *hmm = container_of(kref, struct hmm, kref);
@@ -116,7 +121,7 @@ static void hmm_free(struct kref *kref)
mm->hmm = NULL;
spin_unlock(>page_table_lock);
 
-   kfree(hmm);
+   mmu_notifier_call_srcu(>rcu, hmm_free_rcu);
 }
 
 static inline void hmm_put(struct hmm *hmm)
@@ -144,10 +149,14 @@ void hmm_mm_destroy(struct mm_struct *mm)
 
 static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
 {
-   struct hmm *hmm = mm_get_hmm(mm);
+   struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
struct hmm_mirror *mirror;
struct hmm_range *range;
 
+   /* Bail out if hmm is in the process of being freed */
+   if (!kref_get_unless_zero(>kref))
+   return;
+
/* Report this HMM as dying. */
hmm->dead = true;
 
@@ -185,13 +194,14 @@ static void hmm_release(struct mmu_notifier *mn, struct 
mm_struct *mm)
 static int hmm_invalidate_range_start(struct mmu_notifier *mn,
const struct mmu_notifier_range *nrange)
 {
-   struct hmm *hmm = mm_get_hmm(nrange->mm);
+   struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
struct hmm_mirror *mirror;
struct hmm_update update;
struct hmm_range *range;
int ret = 0;
 
-   VM_BUG_ON(!hmm);
+   if (!kref_get_unless_zero(>kref))
+   return 0;
 
update.start = nrange->start;
update.end = nrange->end;
@@ -236,9 +246,10 @@ static int hmm_invalidate_range_start(struct mmu_notifier 
*mn,
 static void hmm_invalidate_range_end(struct mmu_notifier *mn,
const struct mmu_notifier_range *nrange)
 {
-   struct hmm *hmm = mm_get_hmm(nrange->mm);
+   struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
 
-   VM_BUG_ON(!hmm);
+   if (!kref_get_unless_zero(>kref))
+   return;
 
mutex_lock(>lock);
hmm->notifiers--;
-- 
2.21.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v3 hmm 12/12] mm/hmm: Fix error flows in hmm_invalidate_range_start

[Jason Gunthorpe]

From: Jason Gunthorpe 

If the trylock on the hmm->mirrors_sem fails the function will return
without decrementing the notifiers that were previously incremented. Since
the caller will not call invalidate_range_end() on EAGAIN this will result
in notifiers becoming permanently incremented and deadlock.

If the sync_cpu_device_pagetables() required blocking the function will
not return EAGAIN even though the device continues to touch the
pages. This is a violation of the mmu notifier contract.

Switch, and rename, the ranges_lock to a spin lock so we can reliably
obtain it without blocking during error unwind.

The error unwind is necessary since the notifiers count must be held
incremented across the call to sync_cpu_device_pagetables() as we cannot
allow the range to become marked valid by a parallel
invalidate_start/end() pair while doing sync_cpu_device_pagetables().

Signed-off-by: Jason Gunthorpe 
Reviewed-by: Ralph Campbell 
Tested-by: Philip Yang 
---
 include/linux/hmm.h |  2 +-
 mm/hmm.c| 77 +++--
 2 files changed, 48 insertions(+), 31 deletions(-)

diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index bf013e96525771..0fa8ea34ccef6d 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -86,7 +86,7 @@
 struct hmm {
struct mm_struct*mm;
struct kref kref;
-   struct mutexlock;
+   spinlock_t  ranges_lock;
struct list_headranges;
struct list_headmirrors;
struct mmu_notifier mmu_notifier;
diff --git a/mm/hmm.c b/mm/hmm.c
index c0d43302fd6b2f..1172a4f0206963 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -67,7 +67,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
init_rwsem(>mirrors_sem);
hmm->mmu_notifier.ops = NULL;
INIT_LIST_HEAD(>ranges);
-   mutex_init(>lock);
+   spin_lock_init(>ranges_lock);
kref_init(>kref);
hmm->notifiers = 0;
hmm->mm = mm;
@@ -124,18 +124,19 @@ static void hmm_release(struct mmu_notifier *mn, struct 
mm_struct *mm)
 {
struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
struct hmm_mirror *mirror;
+   unsigned long flags;
 
/* Bail out if hmm is in the process of being freed */
if (!kref_get_unless_zero(>kref))
return;
 
-   mutex_lock(>lock);
+   spin_lock_irqsave(>ranges_lock, flags);
/*
 * Since hmm_range_register() holds the mmget() lock hmm_release() is
 * prevented as long as a range exists.
 */
WARN_ON(!list_empty(>ranges));
-   mutex_unlock(>lock);
+   spin_unlock_irqrestore(>ranges_lock, flags);
 
down_read(>mirrors_sem);
list_for_each_entry(mirror, >mirrors, list) {
@@ -151,6 +152,23 @@ static void hmm_release(struct mmu_notifier *mn, struct 
mm_struct *mm)
hmm_put(hmm);
 }
 
+static void notifiers_decrement(struct hmm *hmm)
+{
+   lockdep_assert_held(>ranges_lock);
+
+   hmm->notifiers--;
+   if (!hmm->notifiers) {
+   struct hmm_range *range;
+
+   list_for_each_entry(range, >ranges, list) {
+   if (range->valid)
+   continue;
+   range->valid = true;
+   }
+   wake_up_all(>wq);
+   }
+}
+
 static int hmm_invalidate_range_start(struct mmu_notifier *mn,
const struct mmu_notifier_range *nrange)
 {
@@ -158,6 +176,7 @@ static int hmm_invalidate_range_start(struct mmu_notifier 
*mn,
struct hmm_mirror *mirror;
struct hmm_update update;
struct hmm_range *range;
+   unsigned long flags;
int ret = 0;
 
if (!kref_get_unless_zero(>kref))
@@ -168,12 +187,7 @@ static int hmm_invalidate_range_start(struct mmu_notifier 
*mn,
update.event = HMM_UPDATE_INVALIDATE;
update.blockable = mmu_notifier_range_blockable(nrange);
 
-   if (mmu_notifier_range_blockable(nrange))
-   mutex_lock(>lock);
-   else if (!mutex_trylock(>lock)) {
-   ret = -EAGAIN;
-   goto out;
-   }
+   spin_lock_irqsave(>ranges_lock, flags);
hmm->notifiers++;
list_for_each_entry(range, >ranges, list) {
if (update.end < range->start || update.start >= range->end)
@@ -181,7 +195,7 @@ static int hmm_invalidate_range_start(struct mmu_notifier 
*mn,
 
range->valid = false;
}
-   mutex_unlock(>lock);
+   spin_unlock_irqrestore(>ranges_lock, flags);
 
if (mmu_notifier_range_blockable(nrange))
down_read(>mirrors_sem);
@@ -189,16 +203,26 @@ static int hmm_invalidate_range_start(struct mmu_notifier 
*mn,
ret = -EAGAIN;
goto out;
}
+
list_for_each_entry(mirror, >mirrors, list) {
-   int ret;
+   int rc;
 
-   ret = 

Re: [PATCH v2 hmm 00/11] Various revisions from a locking/code review

[Jason Gunthorpe]

On Wed, Jun 12, 2019 at 09:49:12PM +, Yang, Philip wrote:
> Rebase to https://github.com/jgunthorpe/linux.git hmm branch, need some 
> changes because of interface hmm_range_register change. Then run a quick 
> amdgpu_test. Test is finished, result is ok.

Great! Thanks

I'll add your Tested-by to the series

>  But there is below kernel BUG message, seems hmm_free_rcu calls
> down_write.
> 
> [ 1171.919921] BUG: sleeping function called from invalid context at 
> /home/yangp/git/compute_staging/kernel/kernel/locking/rwsem.c:65
> [ 1171.919933] in_atomic(): 1, irqs_disabled(): 0, pid: 53, name: 
> kworker/1:1
> [ 1171.919938] 2 locks held by kworker/1:1/53:
> [ 1171.919940]  #0: 1c7c19d4 ((wq_completion)rcu_gp){+.+.}, at: 
> process_one_work+0x20e/0x630
> [ 1171.919951]  #1: 923f2cfa 
> ((work_completion)(>work)){+.+.}, at: process_one_work+0x20e/0x630
> [ 1171.919959] CPU: 1 PID: 53 Comm: kworker/1:1 Tainted: GW 
> 5.2.0-rc1-kfd-yangp #196
> [ 1171.919961] Hardware name: ASUS All Series/Z97-PRO(Wi-Fi ac)/USB 3.1, 
> BIOS 9001 03/07/2016
> [ 1171.919965] Workqueue: rcu_gp srcu_invoke_callbacks
> [ 1171.919968] Call Trace:
> [ 1171.919974]  dump_stack+0x67/0x9b
> [ 1171.919980]  ___might_sleep+0x149/0x230
> [ 1171.919985]  down_write+0x1c/0x70
> [ 1171.919989]  hmm_free_rcu+0x24/0x80
> [ 1171.919993]  srcu_invoke_callbacks+0xc9/0x150
> [ 1171.92]  process_one_work+0x28e/0x630
> [ 1171.920008]  worker_thread+0x39/0x3f0
> [ 1171.920014]  ? process_one_work+0x630/0x630
> [ 1171.920017]  kthread+0x11c/0x140
> [ 1171.920021]  ? kthread_park+0x90/0x90
> [ 1171.920026]  ret_from_fork+0x24/0x30

Thank you Phillip, it seems the prior tests were not done with
lockdep..

Sigh, I will keep this with the gross pagetable_lock then. I updated
the patches on the git with this modification. I think we have covered
all the bases so I will send another V of the series to the list and
if no more comments then it will move ahead to hmm.git. Thanks to all.

diff --git a/mm/hmm.c b/mm/hmm.c
index 136c812faa2790..4c64d4c32f4825 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -49,16 +49,15 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
 
lockdep_assert_held_exclusive(>mmap_sem);
 
+   /* Abuse the page_table_lock to also protect mm->hmm. */
+   spin_lock(>page_table_lock);
if (mm->hmm) {
-   if (kref_get_unless_zero(>hmm->kref))
+   if (kref_get_unless_zero(>hmm->kref)) {
+   spin_unlock(>page_table_lock);
return mm->hmm;
-   /*
-* The hmm is being freed by some other CPU and is pending a
-* RCU grace period, but this CPU can NULL now it since we
-* have the mmap_sem.
-*/
-   mm->hmm = NULL;
+   }
}
+   spin_unlock(>page_table_lock);
 
hmm = kmalloc(sizeof(*hmm), GFP_KERNEL);
if (!hmm)
@@ -81,7 +80,14 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
}
 
mmgrab(hmm->mm);
+
+   /*
+* We hold the exclusive mmap_sem here so we know that mm->hmm is
+* still NULL or 0 kref, and is safe to update.
+*/
+   spin_lock(>page_table_lock);
mm->hmm = hmm;
+   spin_unlock(>page_table_lock);
return hmm;
 }
 
@@ -89,10 +95,14 @@ static void hmm_free_rcu(struct rcu_head *rcu)
 {
struct hmm *hmm = container_of(rcu, struct hmm, rcu);
 
-   down_write(>mm->mmap_sem);
+   /*
+* The mm->hmm pointer is kept valid while notifier ops can be running
+* so they don't have to deal with a NULL mm->hmm value
+*/
+   spin_lock(>mm->page_table_lock);
if (hmm->mm->hmm == hmm)
hmm->mm->hmm = NULL;
-   up_write(>mm->mmap_sem);
+   spin_unlock(>mm->page_table_lock);
mmdrop(hmm->mm);
 
kfree(hmm);
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v3 hmm 06/12] mm/hmm: Hold on to the mmget for the lifetime of the range

[Jason Gunthorpe]

From: Jason Gunthorpe 

Range functions like hmm_range_snapshot() and hmm_range_fault() call
find_vma, which requires hodling the mmget() and the mmap_sem for the mm.

Make this simpler for the callers by holding the mmget() inside the range
for the lifetime of the range. Other functions that accept a range should
only be called if the range is registered.

This has the side effect of directly preventing hmm_release() from
happening while a range is registered. That means range->dead cannot be
false during the lifetime of the range, so remove dead and
hmm_mirror_mm_is_alive() entirely.

Signed-off-by: Jason Gunthorpe 
Reviewed-by: John Hubbard 
Reviewed-by: Ralph Campbell 
Tested-by: Philip Yang 
---
v2:
 - Use Jerome's idea of just holding the mmget() for the range lifetime,
   rework the patch to use that as as simplification to remove dead in
   one step
---
 include/linux/hmm.h | 26 --
 mm/hmm.c| 28 ++--
 2 files changed, 10 insertions(+), 44 deletions(-)

diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 26e7c477490c4e..bf013e96525771 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -82,7 +82,6 @@
  * @mirrors_sem: read/write semaphore protecting the mirrors list
  * @wq: wait queue for user waiting on a range invalidation
  * @notifiers: count of active mmu notifiers
- * @dead: is the mm dead ?
  */
 struct hmm {
struct mm_struct*mm;
@@ -95,7 +94,6 @@ struct hmm {
wait_queue_head_t   wq;
struct rcu_head rcu;
longnotifiers;
-   booldead;
 };
 
 /*
@@ -459,30 +457,6 @@ struct hmm_mirror {
 int hmm_mirror_register(struct hmm_mirror *mirror, struct mm_struct *mm);
 void hmm_mirror_unregister(struct hmm_mirror *mirror);
 
-/*
- * hmm_mirror_mm_is_alive() - test if mm is still alive
- * @mirror: the HMM mm mirror for which we want to lock the mmap_sem
- * Return: false if the mm is dead, true otherwise
- *
- * This is an optimization, it will not always accurately return false if the
- * mm is dead; i.e., there can be false negatives (process is being killed but
- * HMM is not yet informed of that). It is only intended to be used to optimize
- * out cases where the driver is about to do something time consuming and it
- * would be better to skip it if the mm is dead.
- */
-static inline bool hmm_mirror_mm_is_alive(struct hmm_mirror *mirror)
-{
-   struct mm_struct *mm;
-
-   if (!mirror || !mirror->hmm)
-   return false;
-   mm = READ_ONCE(mirror->hmm->mm);
-   if (mirror->hmm->dead || !mm)
-   return false;
-
-   return true;
-}
-
 /*
  * Please see Documentation/vm/hmm.rst for how to use the range API.
  */
diff --git a/mm/hmm.c b/mm/hmm.c
index 4c64d4c32f4825..58712d74edd585 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -70,7 +70,6 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
mutex_init(>lock);
kref_init(>kref);
hmm->notifiers = 0;
-   hmm->dead = false;
hmm->mm = mm;
 
hmm->mmu_notifier.ops = _mmu_notifier_ops;
@@ -125,20 +124,17 @@ static void hmm_release(struct mmu_notifier *mn, struct 
mm_struct *mm)
 {
struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
struct hmm_mirror *mirror;
-   struct hmm_range *range;
 
/* Bail out if hmm is in the process of being freed */
if (!kref_get_unless_zero(>kref))
return;
 
-   /* Report this HMM as dying. */
-   hmm->dead = true;
-
-   /* Wake-up everyone waiting on any range. */
mutex_lock(>lock);
-   list_for_each_entry(range, >ranges, list)
-   range->valid = false;
-   wake_up_all(>wq);
+   /*
+* Since hmm_range_register() holds the mmget() lock hmm_release() is
+* prevented as long as a range exists.
+*/
+   WARN_ON(!list_empty(>ranges));
mutex_unlock(>lock);
 
down_write(>mirrors_sem);
@@ -908,8 +904,8 @@ int hmm_range_register(struct hmm_range *range,
range->start = start;
range->end = end;
 
-   /* Check if hmm_mm_destroy() was call. */
-   if (hmm->mm == NULL || hmm->dead)
+   /* Prevent hmm_release() from running while the range is valid */
+   if (!mmget_not_zero(hmm->mm))
return -EFAULT;
 
/* Initialize range to track CPU page table updates. */
@@ -952,6 +948,7 @@ void hmm_range_unregister(struct hmm_range *range)
 
/* Drop reference taken by hmm_range_register() */
range->valid = false;
+   mmput(hmm->mm);
hmm_put(hmm);
range->hmm = NULL;
 }
@@ -979,10 +976,7 @@ long hmm_range_snapshot(struct hmm_range *range)
struct vm_area_struct *vma;
struct mm_walk mm_walk;
 
-   /* Check if hmm_mm_destroy() was call. */
-   if (hmm->mm == NULL || hmm->dead)
-   return -EFAULT;
-
+   

[PATCH v3 hmm 00/12] mm/hmm: Various revisions from a locking/code review

[Jason Gunthorpe]

From: Jason Gunthorpe 

This patch series arised out of discussions with Jerome when looking at the
ODP changes, particularly informed by use after free races we have already
found and fixed in the ODP code (thanks to syzkaller) working with mmu
notifiers, and the discussion with Ralph on how to resolve the lifetime model.

Overall this brings in a simplified locking scheme and easy to explain
lifetime model:

 If a hmm_range is valid, then the hmm is valid, if a hmm is valid then the mm
 is allocated memory.

 If the mm needs to still be alive (ie to lock the mmap_sem, find a vma, etc)
 then the mmget must be obtained via mmget_not_zero().

The use unlocked reads on 'hmm->dead' are also eliminated in favour of using
standard mmget() locking to prevent the mm from being released. Many of the
debugging checks of !range->hmm and !hmm->mm are dropped in favour of poison -
which is much clearer as to the lifetime intent.

The trailing patches are just some random cleanups I noticed when reviewing
this code.

I would like to run some testing with the ODP patch, but haven't
yet. Otherwise I think this is reviewed enough, and if there is nothing more
say I hope to apply it next week.

I plan to continue to work on the idea with CH to move more of this mirror
code into mmu notifiers and other places, but this will take some time and
research.

Thanks to everyone who took time to look at this!

Jason Gunthorpe (12):
  mm/hmm: fix use after free with struct hmm in the mmu notifiers
  mm/hmm: Use hmm_mirror not mm as an argument for hmm_range_register
  mm/hmm: Hold a mmgrab from hmm to mm
  mm/hmm: Simplify hmm_get_or_create and make it reliable
  mm/hmm: Remove duplicate condition test before wait_event_timeout
  mm/hmm: Hold on to the mmget for the lifetime of the range
  mm/hmm: Use lockdep instead of comments
  mm/hmm: Remove racy protection against double-unregistration
  mm/hmm: Poison hmm_range during unregister
  mm/hmm: Do not use list*_rcu() for hmm->ranges
  mm/hmm: Remove confusing comment and logic from hmm_release
  mm/hmm: Fix error flows in hmm_invalidate_range_start

 drivers/gpu/drm/nouveau/nouveau_svm.c |   2 +-
 include/linux/hmm.h   |  52 +
 kernel/fork.c |   1 -
 mm/hmm.c  | 286 --
 4 files changed, 140 insertions(+), 201 deletions(-)

-- 
2.21.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v3 hmm 04/12] mm/hmm: Simplify hmm_get_or_create and make it reliable

[Jason Gunthorpe]

From: Jason Gunthorpe 

As coded this function can false-fail in various racy situations. Make it
reliable and simpler by running under the write side of the mmap_sem and
avoiding the false-failing compare/exchange pattern. Due to the mmap_sem
this no longer has to avoid racing with a 2nd parallel
hmm_get_or_create().

Unfortunately this still has to use the page_table_lock as the
non-sleeping lock protecting mm->hmm, since the contexts where we free the
hmm are incompatible with mmap_sem.

Signed-off-by: Jason Gunthorpe 
Reviewed-by: John Hubbard 
Reviewed-by: Ralph Campbell 
Reviewed-by: Ira Weiny 
Tested-by: Philip Yang 
---
v2:
- Fix error unwind of mmgrab (Jerome)
- Use hmm local instead of 2nd container_of (Jerome)
v3:
- Can't use mmap_sem in the SRCU callback, keep using the
  page_table_lock (Philip)
---
 mm/hmm.c | 84 
 1 file changed, 36 insertions(+), 48 deletions(-)

diff --git a/mm/hmm.c b/mm/hmm.c
index 080b17a2e87e2d..4c64d4c32f4825 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -31,16 +31,6 @@
 #if IS_ENABLED(CONFIG_HMM_MIRROR)
 static const struct mmu_notifier_ops hmm_mmu_notifier_ops;
 
-static inline struct hmm *mm_get_hmm(struct mm_struct *mm)
-{
-   struct hmm *hmm = READ_ONCE(mm->hmm);
-
-   if (hmm && kref_get_unless_zero(>kref))
-   return hmm;
-
-   return NULL;
-}
-
 /**
  * hmm_get_or_create - register HMM against an mm (HMM internal)
  *
@@ -55,11 +45,19 @@ static inline struct hmm *mm_get_hmm(struct mm_struct *mm)
  */
 static struct hmm *hmm_get_or_create(struct mm_struct *mm)
 {
-   struct hmm *hmm = mm_get_hmm(mm);
-   bool cleanup = false;
+   struct hmm *hmm;
 
-   if (hmm)
-   return hmm;
+   lockdep_assert_held_exclusive(>mmap_sem);
+
+   /* Abuse the page_table_lock to also protect mm->hmm. */
+   spin_lock(>page_table_lock);
+   if (mm->hmm) {
+   if (kref_get_unless_zero(>hmm->kref)) {
+   spin_unlock(>page_table_lock);
+   return mm->hmm;
+   }
+   }
+   spin_unlock(>page_table_lock);
 
hmm = kmalloc(sizeof(*hmm), GFP_KERNEL);
if (!hmm)
@@ -74,57 +72,47 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
hmm->notifiers = 0;
hmm->dead = false;
hmm->mm = mm;
-   mmgrab(hmm->mm);
 
-   spin_lock(>page_table_lock);
-   if (!mm->hmm)
-   mm->hmm = hmm;
-   else
-   cleanup = true;
-   spin_unlock(>page_table_lock);
+   hmm->mmu_notifier.ops = _mmu_notifier_ops;
+   if (__mmu_notifier_register(>mmu_notifier, mm)) {
+   kfree(hmm);
+   return NULL;
+   }
 
-   if (cleanup)
-   goto error;
+   mmgrab(hmm->mm);
 
/*
-* We should only get here if hold the mmap_sem in write mode ie on
-* registration of first mirror through hmm_mirror_register()
+* We hold the exclusive mmap_sem here so we know that mm->hmm is
+* still NULL or 0 kref, and is safe to update.
 */
-   hmm->mmu_notifier.ops = _mmu_notifier_ops;
-   if (__mmu_notifier_register(>mmu_notifier, mm))
-   goto error_mm;
-
-   return hmm;
-
-error_mm:
spin_lock(>page_table_lock);
-   if (mm->hmm == hmm)
-   mm->hmm = NULL;
+   mm->hmm = hmm;
spin_unlock(>page_table_lock);
-error:
-   mmdrop(hmm->mm);
-   kfree(hmm);
-   return NULL;
+   return hmm;
 }
 
 static void hmm_free_rcu(struct rcu_head *rcu)
 {
-   kfree(container_of(rcu, struct hmm, rcu));
+   struct hmm *hmm = container_of(rcu, struct hmm, rcu);
+
+   /*
+* The mm->hmm pointer is kept valid while notifier ops can be running
+* so they don't have to deal with a NULL mm->hmm value
+*/
+   spin_lock(>mm->page_table_lock);
+   if (hmm->mm->hmm == hmm)
+   hmm->mm->hmm = NULL;
+   spin_unlock(>mm->page_table_lock);
+   mmdrop(hmm->mm);
+
+   kfree(hmm);
 }
 
 static void hmm_free(struct kref *kref)
 {
struct hmm *hmm = container_of(kref, struct hmm, kref);
-   struct mm_struct *mm = hmm->mm;
-
-   mmu_notifier_unregister_no_release(>mmu_notifier, mm);
 
-   spin_lock(>page_table_lock);
-   if (mm->hmm == hmm)
-   mm->hmm = NULL;
-   spin_unlock(>page_table_lock);
-
-   mmdrop(hmm->mm);
+   mmu_notifier_unregister_no_release(>mmu_notifier, hmm->mm);
mmu_notifier_call_srcu(>rcu, hmm_free_rcu);
 }
 
-- 
2.21.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v3 hmm 10/12] mm/hmm: Do not use list*_rcu() for hmm->ranges

[Jason Gunthorpe]

From: Jason Gunthorpe 

This list is always read and written while holding hmm->lock so there is
no need for the confusing _rcu annotations.

Signed-off-by: Jason Gunthorpe 
Reviewed-by: Jérôme Glisse 
Reviewed-by: John Hubbard 
Acked-by: Souptick Joarder 
Reviewed-by: Ralph Campbell 
Acked-by: Souptick Joarder 
Reviewed-by: Ira Weiny 
Tested-by: Philip Yang 
---
 mm/hmm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/hmm.c b/mm/hmm.c
index e214668cba3474..26af511cbdd075 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -908,7 +908,7 @@ int hmm_range_register(struct hmm_range *range,
 
range->hmm = hmm;
kref_get(>kref);
-   list_add_rcu(>list, >ranges);
+   list_add(>list, >ranges);
 
/*
 * If there are any concurrent notifiers we have to wait for them for
@@ -934,7 +934,7 @@ void hmm_range_unregister(struct hmm_range *range)
struct hmm *hmm = range->hmm;
 
mutex_lock(>lock);
-   list_del_rcu(>list);
+   list_del(>list);
mutex_unlock(>lock);
 
/* Drop reference taken by hmm_range_register() */
-- 
2.21.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v3 hmm 08/12] mm/hmm: Remove racy protection against double-unregistration

[Jason Gunthorpe]

From: Jason Gunthorpe 

No other register/unregister kernel API attempts to provide this kind of
protection as it is inherently racy, so just drop it.

Callers should provide their own protection, it appears nouveau already
does, but just in case drop a debugging POISON.

Signed-off-by: Jason Gunthorpe 
Reviewed-by: Jérôme Glisse 
Reviewed-by: John Hubbard 
Reviewed-by: Ralph Campbell 
Tested-by: Philip Yang 
---
 mm/hmm.c | 9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/mm/hmm.c b/mm/hmm.c
index c0f622f86223c2..e3e0a811a3a774 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -283,18 +283,13 @@ EXPORT_SYMBOL(hmm_mirror_register);
  */
 void hmm_mirror_unregister(struct hmm_mirror *mirror)
 {
-   struct hmm *hmm = READ_ONCE(mirror->hmm);
-
-   if (hmm == NULL)
-   return;
+   struct hmm *hmm = mirror->hmm;
 
down_write(>mirrors_sem);
list_del_init(>list);
-   /* To protect us against double unregister ... */
-   mirror->hmm = NULL;
up_write(>mirrors_sem);
-
hmm_put(hmm);
+   memset(>hmm, POISON_INUSE, sizeof(mirror->hmm));
 }
 EXPORT_SYMBOL(hmm_mirror_unregister);
 
-- 
2.21.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amd/powerplay: detect version of smu backend

[Prike Liang]

Change-Id: Ib050c8cf0c2c5af4c1f747cf596860f9be01a2d3
Signed-off-by: Prike Liang 
---
 drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c | 1 +
 drivers/gpu/drm/amd/powerplay/inc/hwmgr.h   | 1 +
 drivers/gpu/drm/amd/powerplay/smumgr/ci_smumgr.c| 1 +
 drivers/gpu/drm/amd/powerplay/smumgr/fiji_smumgr.c  | 1 +
 drivers/gpu/drm/amd/powerplay/smumgr/iceland_smumgr.c   | 1 +
 drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c | 1 +
 drivers/gpu/drm/amd/powerplay/smumgr/smu10_smumgr.c | 1 +
 drivers/gpu/drm/amd/powerplay/smumgr/smu8_smumgr.c  | 1 +
 drivers/gpu/drm/amd/powerplay/smumgr/tonga_smumgr.c | 1 +
 drivers/gpu/drm/amd/powerplay/smumgr/vega10_smumgr.c| 1 +
 drivers/gpu/drm/amd/powerplay/smumgr/vega12_smumgr.c| 1 +
 drivers/gpu/drm/amd/powerplay/smumgr/vega20_smumgr.c| 1 +
 drivers/gpu/drm/amd/powerplay/smumgr/vegam_smumgr.c | 1 +
 13 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c 
b/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
index f1d326c..b996819 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
@@ -194,6 +194,7 @@ int hwmgr_sw_init(struct pp_hwmgr *hwmgr)
return -EINVAL;
 
phm_register_irq_handlers(hwmgr);
+   pr_info("hwmgr_sw_init smu backed is %s\n",hwmgr->smumgr_funcs->name);
 
return hwmgr->smumgr_funcs->smu_init(hwmgr);
 }
diff --git a/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h 
b/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
index c92999a..47dbecc 100644
--- a/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
+++ b/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
@@ -190,6 +190,7 @@ struct phm_vce_clock_voltage_dependency_table {
 };
 
 struct pp_smumgr_func {
+   char *name;
int (*smu_init)(struct pp_hwmgr  *hwmgr);
int (*smu_fini)(struct pp_hwmgr  *hwmgr);
int (*start_smu)(struct pp_hwmgr  *hwmgr);
diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/ci_smumgr.c 
b/drivers/gpu/drm/amd/powerplay/smumgr/ci_smumgr.c
index 9ef57fc..022f3c8 100644
--- a/drivers/gpu/drm/amd/powerplay/smumgr/ci_smumgr.c
+++ b/drivers/gpu/drm/amd/powerplay/smumgr/ci_smumgr.c
@@ -2935,6 +2935,7 @@ static int ci_update_smc_table(struct pp_hwmgr *hwmgr, 
uint32_t type)
 }
 
 const struct pp_smumgr_func ci_smu_funcs = {
+   .name = "ci_smu",
.smu_init = ci_smu_init,
.smu_fini = ci_smu_fini,
.start_smu = ci_start_smu,
diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/fiji_smumgr.c 
b/drivers/gpu/drm/amd/powerplay/smumgr/fiji_smumgr.c
index 0ce85b7..da025b1 100644
--- a/drivers/gpu/drm/amd/powerplay/smumgr/fiji_smumgr.c
+++ b/drivers/gpu/drm/amd/powerplay/smumgr/fiji_smumgr.c
@@ -2643,6 +2643,7 @@ static int fiji_update_dpm_settings(struct pp_hwmgr 
*hwmgr,
 }
 
 const struct pp_smumgr_func fiji_smu_funcs = {
+   .name = "fiji_smu",
.smu_init = _smu_init,
.smu_fini = _smu_fini,
.start_smu = _start_smu,
diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/iceland_smumgr.c 
b/drivers/gpu/drm/amd/powerplay/smumgr/iceland_smumgr.c
index f24f13d..f414f22 100644
--- a/drivers/gpu/drm/amd/powerplay/smumgr/iceland_smumgr.c
+++ b/drivers/gpu/drm/amd/powerplay/smumgr/iceland_smumgr.c
@@ -2661,6 +2661,7 @@ static bool iceland_is_dpm_running(struct pp_hwmgr *hwmgr)
 }
 
 const struct pp_smumgr_func iceland_smu_funcs = {
+   .name = "iceland_smu",
.smu_init = _smu_init,
.smu_fini = _smu_fini,
.start_smu = _start_smu,
diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c 
b/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c
index 0d8958e..fbac2d3 100644
--- a/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c
+++ b/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c
@@ -2550,6 +2550,7 @@ static int polaris10_update_dpm_settings(struct pp_hwmgr 
*hwmgr,
 }
 
 const struct pp_smumgr_func polaris10_smu_funcs = {
+   .name = "polaris10_smu",
.smu_init = polaris10_smu_init,
.smu_fini = smu7_smu_fini,
.start_smu = polaris10_start_smu,
diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/smu10_smumgr.c 
b/drivers/gpu/drm/amd/powerplay/smumgr/smu10_smumgr.c
index 6d11076a..ca66035 100644
--- a/drivers/gpu/drm/amd/powerplay/smumgr/smu10_smumgr.c
+++ b/drivers/gpu/drm/amd/powerplay/smumgr/smu10_smumgr.c
@@ -291,6 +291,7 @@ static int smu10_smc_table_manager(struct pp_hwmgr *hwmgr, 
uint8_t *table, uint1
 
 
 const struct pp_smumgr_func smu10_smu_funcs = {
+   .name = "smu10_smu",
.smu_init = _smu_init,
.smu_fini = _smu_fini,
.start_smu = _start_smu,
diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/smu8_smumgr.c 
b/drivers/gpu/drm/amd/powerplay/smumgr/smu8_smumgr.c
index e2787e1..8189fe4 100644
--- a/drivers/gpu/drm/amd/powerplay/smumgr/smu8_smumgr.c
+++ b/drivers/gpu/drm/amd/powerplay/smumgr/smu8_smumgr.c
@@ -881,6 +881,7 @@ static bool smu8_is_dpm_running(struct pp_hwmgr *hwmgr)
 }