[4/9] drm/amdgpu: add bo_va cleared flag again

Submitted by Christian König on Aug. 25, 2017, 9:38 a.m.

Details

Message ID 1503653899-1781-4-git-send-email-deathsimple@vodafone.de
State New
Headers show
Series "Series without cover letter" ( rev: 2 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Christian König Aug. 25, 2017, 9:38 a.m.
From: Christian König <christian.koenig@amd.com>

We changed this to use an extra list a while back, but for the next
series I need a separate flag again.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  3 +++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 37 ++++++++++++++----------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h     |  3 ---
 3 files changed, 20 insertions(+), 23 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index a288fa6..e613ba4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -55,6 +55,9 @@  struct amdgpu_bo_va {
 	/* mappings for this bo_va */
 	struct list_head		invalids;
 	struct list_head		valids;
+
+	/* If the mappings are cleared or filled */
+	bool				cleared;
 };
 
 struct amdgpu_bo {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index f621dba..16148ef 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1787,10 +1787,13 @@  int amdgpu_vm_bo_update(struct amdgpu_device *adev,
 	else
 		flags = 0x0;
 
-	spin_lock(&vm->status_lock);
-	if (!list_empty(&bo_va->base.vm_status))
+	/* We access vm_status without the status lock here, but that is ok
+	 * because when we don't clear the BO is locked and so the status can't
+	 * change
+	 */
+	if ((!clear && !list_empty(&bo_va->base.vm_status)) ||
+	    bo_va->cleared != clear)
 		list_splice_init(&bo_va->valids, &bo_va->invalids);
-	spin_unlock(&vm->status_lock);
 
 	list_for_each_entry(mapping, &bo_va->invalids, list) {
 		r = amdgpu_vm_bo_split_mapping(adev, exclusive, pages_addr, vm,
@@ -1800,25 +1803,22 @@  int amdgpu_vm_bo_update(struct amdgpu_device *adev,
 			return r;
 	}
 
-	if (trace_amdgpu_vm_bo_mapping_enabled()) {
-		list_for_each_entry(mapping, &bo_va->valids, list)
-			trace_amdgpu_vm_bo_mapping(mapping);
-
-		list_for_each_entry(mapping, &bo_va->invalids, list)
-			trace_amdgpu_vm_bo_mapping(mapping);
+	if (vm->use_cpu_for_update) {
+		/* Flush HDP */
+		mb();
+		amdgpu_gart_flush_gpu_tlb(adev, 0);
 	}
 
 	spin_lock(&vm->status_lock);
-	list_splice_init(&bo_va->invalids, &bo_va->valids);
 	list_del_init(&bo_va->base.vm_status);
-	if (clear)
-		list_add(&bo_va->base.vm_status, &vm->cleared);
 	spin_unlock(&vm->status_lock);
 
-	if (vm->use_cpu_for_update) {
-		/* Flush HDP */
-		mb();
-		amdgpu_gart_flush_gpu_tlb(adev, 0);
+	list_splice_init(&bo_va->invalids, &bo_va->valids);
+	bo_va->cleared = clear;
+
+	if (trace_amdgpu_vm_bo_mapping_enabled()) {
+		list_for_each_entry(mapping, &bo_va->valids, list)
+			trace_amdgpu_vm_bo_mapping(mapping);
 	}
 
 	return 0;
@@ -2419,9 +2419,7 @@  void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
 
 	list_for_each_entry(bo_base, &bo->va, bo_list) {
 		spin_lock(&bo_base->vm->status_lock);
-		if (list_empty(&bo_base->vm_status))
-			list_add(&bo_base->vm_status,
-				 &bo_base->vm->moved);
+		list_move(&bo_base->vm_status, &bo_base->vm->moved);
 		spin_unlock(&bo_base->vm->status_lock);
 	}
 }
@@ -2508,7 +2506,6 @@  int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		vm->reserved_vmid[i] = NULL;
 	spin_lock_init(&vm->status_lock);
 	INIT_LIST_HEAD(&vm->moved);
-	INIT_LIST_HEAD(&vm->cleared);
 	INIT_LIST_HEAD(&vm->freed);
 
 	/* create scheduler entity for page table updates */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 9347d28..e705f0f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -126,9 +126,6 @@  struct amdgpu_vm {
 	/* BOs moved, but not yet updated in the PT */
 	struct list_head	moved;
 
-	/* BOs cleared in the PT because of a move */
-	struct list_head	cleared;
-
 	/* BO mappings freed, but not yet updated in the PT */
 	struct list_head	freed;
 

Comments

I'm not sure this one, since you remove status lock for vm_status list, 
I think we need to test this carefully.

Regards,

David Zhou


On 2017年08月25日 17:38, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
>
> We changed this to use an extra list a while back, but for the next
> series I need a separate flag again.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  3 +++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 37 ++++++++++++++----------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h     |  3 ---
>   3 files changed, 20 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> index a288fa6..e613ba4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> @@ -55,6 +55,9 @@ struct amdgpu_bo_va {
>   	/* mappings for this bo_va */
>   	struct list_head		invalids;
>   	struct list_head		valids;
> +
> +	/* If the mappings are cleared or filled */
> +	bool				cleared;
>   };
>   
>   struct amdgpu_bo {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index f621dba..16148ef 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1787,10 +1787,13 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
>   	else
>   		flags = 0x0;
>   
> -	spin_lock(&vm->status_lock);
> -	if (!list_empty(&bo_va->base.vm_status))
> +	/* We access vm_status without the status lock here, but that is ok
> +	 * because when we don't clear the BO is locked and so the status can't
> +	 * change
> +	 */
> +	if ((!clear && !list_empty(&bo_va->base.vm_status)) ||
> +	    bo_va->cleared != clear)
>   		list_splice_init(&bo_va->valids, &bo_va->invalids);
> -	spin_unlock(&vm->status_lock);
>   
>   	list_for_each_entry(mapping, &bo_va->invalids, list) {
>   		r = amdgpu_vm_bo_split_mapping(adev, exclusive, pages_addr, vm,
> @@ -1800,25 +1803,22 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
>   			return r;
>   	}
>   
> -	if (trace_amdgpu_vm_bo_mapping_enabled()) {
> -		list_for_each_entry(mapping, &bo_va->valids, list)
> -			trace_amdgpu_vm_bo_mapping(mapping);
> -
> -		list_for_each_entry(mapping, &bo_va->invalids, list)
> -			trace_amdgpu_vm_bo_mapping(mapping);
> +	if (vm->use_cpu_for_update) {
> +		/* Flush HDP */
> +		mb();
> +		amdgpu_gart_flush_gpu_tlb(adev, 0);
>   	}
>   
>   	spin_lock(&vm->status_lock);
> -	list_splice_init(&bo_va->invalids, &bo_va->valids);
>   	list_del_init(&bo_va->base.vm_status);
> -	if (clear)
> -		list_add(&bo_va->base.vm_status, &vm->cleared);
>   	spin_unlock(&vm->status_lock);
>   
> -	if (vm->use_cpu_for_update) {
> -		/* Flush HDP */
> -		mb();
> -		amdgpu_gart_flush_gpu_tlb(adev, 0);
> +	list_splice_init(&bo_va->invalids, &bo_va->valids);
> +	bo_va->cleared = clear;
> +
> +	if (trace_amdgpu_vm_bo_mapping_enabled()) {
> +		list_for_each_entry(mapping, &bo_va->valids, list)
> +			trace_amdgpu_vm_bo_mapping(mapping);
>   	}
>   
>   	return 0;
> @@ -2419,9 +2419,7 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
>   
>   	list_for_each_entry(bo_base, &bo->va, bo_list) {
>   		spin_lock(&bo_base->vm->status_lock);
> -		if (list_empty(&bo_base->vm_status))
> -			list_add(&bo_base->vm_status,
> -				 &bo_base->vm->moved);
> +		list_move(&bo_base->vm_status, &bo_base->vm->moved);
>   		spin_unlock(&bo_base->vm->status_lock);
>   	}
>   }
> @@ -2508,7 +2506,6 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		vm->reserved_vmid[i] = NULL;
>   	spin_lock_init(&vm->status_lock);
>   	INIT_LIST_HEAD(&vm->moved);
> -	INIT_LIST_HEAD(&vm->cleared);
>   	INIT_LIST_HEAD(&vm->freed);
>   
>   	/* create scheduler entity for page table updates */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 9347d28..e705f0f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -126,9 +126,6 @@ struct amdgpu_vm {
>   	/* BOs moved, but not yet updated in the PT */
>   	struct list_head	moved;
>   
> -	/* BOs cleared in the PT because of a move */
> -	struct list_head	cleared;
> -
>   	/* BO mappings freed, but not yet updated in the PT */
>   	struct list_head	freed;
>
Actually that lockless access is removed again in the next patch.

I will reorder the patches, so that it never even occur.

Christian.

Am 28.08.2017 um 06:12 schrieb zhoucm1:
> I'm not sure this one, since you remove status lock for vm_status 
> list, I think we need to test this carefully.
>
> Regards,
>
> David Zhou
>
>
> On 2017年08月25日 17:38, Christian König wrote:
>> From: Christian König <christian.koenig@amd.com>
>>
>> We changed this to use an extra list a while back, but for the next
>> series I need a separate flag again.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  3 +++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 37 
>> ++++++++++++++----------------
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h     |  3 ---
>>   3 files changed, 20 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> index a288fa6..e613ba4 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> @@ -55,6 +55,9 @@ struct amdgpu_bo_va {
>>       /* mappings for this bo_va */
>>       struct list_head        invalids;
>>       struct list_head        valids;
>> +
>> +    /* If the mappings are cleared or filled */
>> +    bool                cleared;
>>   };
>>     struct amdgpu_bo {
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index f621dba..16148ef 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1787,10 +1787,13 @@ int amdgpu_vm_bo_update(struct amdgpu_device 
>> *adev,
>>       else
>>           flags = 0x0;
>>   -    spin_lock(&vm->status_lock);
>> -    if (!list_empty(&bo_va->base.vm_status))
>> +    /* We access vm_status without the status lock here, but that is ok
>> +     * because when we don't clear the BO is locked and so the 
>> status can't
>> +     * change
>> +     */
>> +    if ((!clear && !list_empty(&bo_va->base.vm_status)) ||
>> +        bo_va->cleared != clear)
>>           list_splice_init(&bo_va->valids, &bo_va->invalids);
>> -    spin_unlock(&vm->status_lock);
>>         list_for_each_entry(mapping, &bo_va->invalids, list) {
>>           r = amdgpu_vm_bo_split_mapping(adev, exclusive, pages_addr, 
>> vm,
>> @@ -1800,25 +1803,22 @@ int amdgpu_vm_bo_update(struct amdgpu_device 
>> *adev,
>>               return r;
>>       }
>>   -    if (trace_amdgpu_vm_bo_mapping_enabled()) {
>> -        list_for_each_entry(mapping, &bo_va->valids, list)
>> -            trace_amdgpu_vm_bo_mapping(mapping);
>> -
>> -        list_for_each_entry(mapping, &bo_va->invalids, list)
>> -            trace_amdgpu_vm_bo_mapping(mapping);
>> +    if (vm->use_cpu_for_update) {
>> +        /* Flush HDP */
>> +        mb();
>> +        amdgpu_gart_flush_gpu_tlb(adev, 0);
>>       }
>>         spin_lock(&vm->status_lock);
>> -    list_splice_init(&bo_va->invalids, &bo_va->valids);
>>       list_del_init(&bo_va->base.vm_status);
>> -    if (clear)
>> -        list_add(&bo_va->base.vm_status, &vm->cleared);
>>       spin_unlock(&vm->status_lock);
>>   -    if (vm->use_cpu_for_update) {
>> -        /* Flush HDP */
>> -        mb();
>> -        amdgpu_gart_flush_gpu_tlb(adev, 0);
>> +    list_splice_init(&bo_va->invalids, &bo_va->valids);
>> +    bo_va->cleared = clear;
>> +
>> +    if (trace_amdgpu_vm_bo_mapping_enabled()) {
>> +        list_for_each_entry(mapping, &bo_va->valids, list)
>> +            trace_amdgpu_vm_bo_mapping(mapping);
>>       }
>>         return 0;
>> @@ -2419,9 +2419,7 @@ void amdgpu_vm_bo_invalidate(struct 
>> amdgpu_device *adev,
>>         list_for_each_entry(bo_base, &bo->va, bo_list) {
>>           spin_lock(&bo_base->vm->status_lock);
>> -        if (list_empty(&bo_base->vm_status))
>> -            list_add(&bo_base->vm_status,
>> -                 &bo_base->vm->moved);
>> +        list_move(&bo_base->vm_status, &bo_base->vm->moved);
>>           spin_unlock(&bo_base->vm->status_lock);
>>       }
>>   }
>> @@ -2508,7 +2506,6 @@ int amdgpu_vm_init(struct amdgpu_device *adev, 
>> struct amdgpu_vm *vm,
>>           vm->reserved_vmid[i] = NULL;
>>       spin_lock_init(&vm->status_lock);
>>       INIT_LIST_HEAD(&vm->moved);
>> -    INIT_LIST_HEAD(&vm->cleared);
>>       INIT_LIST_HEAD(&vm->freed);
>>         /* create scheduler entity for page table updates */
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> index 9347d28..e705f0f 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> @@ -126,9 +126,6 @@ struct amdgpu_vm {
>>       /* BOs moved, but not yet updated in the PT */
>>       struct list_head    moved;
>>   -    /* BOs cleared in the PT because of a move */
>> -    struct list_head    cleared;
>> -
>>       /* BO mappings freed, but not yet updated in the PT */
>>       struct list_head    freed;
>