[PATCH 1/2] drm/amdgpu: return EINVAL instead of ENOENT in the VM code

[PATCH 1/2] drm/amdgpu: return EINVAL instead of ENOENT in the VM code

[Christian König]

That we can't find a PD above the root is expected can only happen if
we try to update a larger range than actually managed by the VM.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 5cb182231f5d..8119f32ca94d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1475,7 +1475,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
 			 * shift we should go up one level and check it again.
 			 */
 			if (!amdgpu_vm_pt_ancestor(&cursor))
-				return -ENOENT;
+				return -EINVAL;
 			continue;
 		}
 
-- 
2.14.1

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

[PATCH 2/2] drm/amdgpu: allow higher level PD invalidations

[Christian König]

Allow partial invalidation on unallocated PDs. This is useful when we
need to silence faults to stop interrupt floods on Vega.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 8119f32ca94d..0f79c17118bf 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1467,9 +1467,8 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
 			 * smaller than the address shift. Go to the next
 			 * child entry and try again.
 			 */
-			if (!amdgpu_vm_pt_descendant(adev, &cursor))
-				return -ENOENT;
-			continue;
+			if (amdgpu_vm_pt_descendant(adev, &cursor))
+				continue;
 		} else if (frag >= parent_shift) {
 			/* If the fragment size is even larger than the parent
 			 * shift we should go up one level and check it again.
@@ -1480,8 +1479,19 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
 		}
 
 		pt = cursor.entry->base.bo;
-		if (!pt)
-			return -ENOENT;
+		if (!pt) {
+			/* We need all PDs and PTs for mapping something, */
+			if (flags & AMDGPU_PTE_VALID)
+				return -ENOENT;
+
+			/* but unmapping something can happen at a higher
+			 * level. */
+			if (!amdgpu_vm_pt_ancestor(&cursor))
+				return -EINVAL;
+
+			pt = cursor.entry->base.bo;
+			shift = parent_shift;
+		}
 
 		/* Looks good so far, calculate parameters for the update */
 		incr = (uint64_t)AMDGPU_GPU_PAGE_SIZE << shift;
@@ -1495,6 +1505,9 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
 			uint64_t upd_end = min(entry_end, frag_end);
 			unsigned nptes = (upd_end - frag_start) >> shift;
 
+			/* This can happen when we set higher level PDs to
+			 * silent to stop fault floods. */
+			nptes = max(nptes, 1u);
 			amdgpu_vm_update_flags(params, pt, cursor.level,
 					       pe_start, dst, nptes, incr,
 					       flags | AMDGPU_PTE_FRAG(frag));
-- 
2.14.1

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

Re: [PATCH 1/2] drm/amdgpu: return EINVAL instead of ENOENT in the VM code

[Luben Tuikov]

Do we have user-space/libs rely on this errno in particular?

On 2020-01-22 9:03 a.m., Christian König wrote:
> That we can't find a PD above the root is expected can only happen if
> we try to update a larger range than actually managed by the VM.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 5cb182231f5d..8119f32ca94d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1475,7 +1475,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
>  			 * shift we should go up one level and check it again.
>  			 */
>  			if (!amdgpu_vm_pt_ancestor(&cursor))
> -				return -ENOENT;
> +				return -EINVAL;
>  			continue;
>  		}
>  
> 

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

Re: [PATCH 1/2] drm/amdgpu: return EINVAL instead of ENOENT in the VM code

[Christian König]

Am 22.01.20 um 19:11 schrieb Luben Tuikov:
> Do we have user-space/libs rely on this errno in particular?

No, userspace can't even directly call this function.

The problem was rather that during debugging I called the function with 
fixed parameters and wondered for quite a while why it can't find the 
table in the tree structure.

Turned out that the table was perfectly fine, but my parameters had a 0 
to much and so was outside of the valid range.

Regards,
Christian.

>
> On 2020-01-22 9:03 a.m., Christian König wrote:
>> That we can't find a PD above the root is expected can only happen if
>> we try to update a larger range than actually managed by the VM.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 5cb182231f5d..8119f32ca94d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1475,7 +1475,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
>>   			 * shift we should go up one level and check it again.
>>   			 */
>>   			if (!amdgpu_vm_pt_ancestor(&cursor))
>> -				return -ENOENT;
>> +				return -EINVAL;
>>   			continue;
>>   		}
>>   
>>

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

Re: [PATCH 2/2] drm/amdgpu: allow higher level PD invalidations

[Felix Kuehling]

Two style nit-picks inline. Otherwise the series is

Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>

On 2020-01-22 9:03 a.m., Christian König wrote:
> Allow partial invalidation on unallocated PDs. This is useful when we
> need to silence faults to stop interrupt floods on Vega.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 23 ++++++++++++++++++-----
>   1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 8119f32ca94d..0f79c17118bf 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1467,9 +1467,8 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
>   			 * smaller than the address shift. Go to the next
>   			 * child entry and try again.
>   			 */
> -			if (!amdgpu_vm_pt_descendant(adev, &cursor))
> -				return -ENOENT;
> -			continue;
> +			if (amdgpu_vm_pt_descendant(adev, &cursor))
> +				continue;
>   		} else if (frag >= parent_shift) {
>   			/* If the fragment size is even larger than the parent
>   			 * shift we should go up one level and check it again.
> @@ -1480,8 +1479,19 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
>   		}
>   
>   		pt = cursor.entry->base.bo;
> -		if (!pt)
> -			return -ENOENT;
> +		if (!pt) {
> +			/* We need all PDs and PTs for mapping something, */
> +			if (flags & AMDGPU_PTE_VALID)
> +				return -ENOENT;
> +
> +			/* but unmapping something can happen at a higher
> +			 * level. */

Nit-pick: This comment would upset checkpatch.pl.


> +			if (!amdgpu_vm_pt_ancestor(&cursor))
> +				return -EINVAL;
> +
> +			pt = cursor.entry->base.bo;
> +			shift = parent_shift;
> +		}
>   
>   		/* Looks good so far, calculate parameters for the update */
>   		incr = (uint64_t)AMDGPU_GPU_PAGE_SIZE << shift;
> @@ -1495,6 +1505,9 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
>   			uint64_t upd_end = min(entry_end, frag_end);
>   			unsigned nptes = (upd_end - frag_start) >> shift;
>   
> +			/* This can happen when we set higher level PDs to
> +			 * silent to stop fault floods. */

Same as above.


> +			nptes = max(nptes, 1u);
>   			amdgpu_vm_update_flags(params, pt, cursor.level,
>   					       pe_start, dst, nptes, incr,
>   					       flags | AMDGPU_PTE_FRAG(frag));
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx