[PATCH 1/2] drm/amdgpu: add amdgpu_vm_entries_mask

[PATCH 1/2] drm/amdgpu: add amdgpu_vm_entries_mask

[Christian König]

We can't get the mask for the root directory from the number of entries.

So add a new function to avoid that problem.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index a7f9aaa47c49..aaf54fc8cafe 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -190,6 +190,26 @@ static unsigned amdgpu_vm_num_entries(struct amdgpu_device *adev,
 		return AMDGPU_VM_PTE_COUNT(adev);
 }
 
+/**
+ * amdgpu_vm_entries_mask - the mask to get the entry number of a PD/PT
+ *
+ * @adev: amdgpu_device pointer
+ * @level: VMPT level
+ *
+ * Returns:
+ * The mask to extract the entry number of a PD/PT from an address.
+ */
+static uint32_t amdgpu_vm_entries_mask(struct amdgpu_device *adev,
+				       unsigned int level)
+{
+	if (level <= adev->vm_manager.root_level)
+		return 0xffffffff;
+	else if (level != AMDGPU_VM_PTB)
+		return 0x1f;
+	else
+		return AMDGPU_VM_PTE_COUNT(adev) - 1;
+}
+
 /**
  * amdgpu_vm_bo_size - returns the size of the BOs in bytes
  *
@@ -399,17 +419,17 @@ static void amdgpu_vm_pt_start(struct amdgpu_device *adev,
 static bool amdgpu_vm_pt_descendant(struct amdgpu_device *adev,
 				    struct amdgpu_vm_pt_cursor *cursor)
 {
-	unsigned num_entries, shift, idx;
+	unsigned mask, shift, idx;
 
 	if (!cursor->entry->entries)
 		return false;
 
 	BUG_ON(!cursor->entry->base.bo);
-	num_entries = amdgpu_vm_num_entries(adev, cursor->level);
+	mask = amdgpu_vm_entries_mask(adev, cursor->level);
 	shift = amdgpu_vm_level_shift(adev, cursor->level);
 
 	++cursor->level;
-	idx = (cursor->pfn >> shift) % num_entries;
+	idx = (cursor->pfn >> shift) & mask;
 	cursor->parent = cursor->entry;
 	cursor->entry = &cursor->entry->entries[idx];
 	return true;
@@ -1599,7 +1619,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
 	amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
 	while (cursor.pfn < end) {
 		struct amdgpu_bo *pt = cursor.entry->base.bo;
-		unsigned shift, parent_shift, num_entries;
+		unsigned shift, parent_shift, mask;
 		uint64_t incr, entry_end, pe_start;
 
 		if (!pt)
@@ -1654,9 +1674,9 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
 
 		/* Looks good so far, calculate parameters for the update */
 		incr = AMDGPU_GPU_PAGE_SIZE << shift;
-		num_entries = amdgpu_vm_num_entries(adev, cursor.level);
-		pe_start = ((cursor.pfn >> shift) & (num_entries - 1)) * 8;
-		entry_end = num_entries << shift;
+		mask = amdgpu_vm_entries_mask(adev, cursor.level);
+		pe_start = ((cursor.pfn >> shift) & mask) * 8;
+		entry_end = (mask + 1) << shift;
 		entry_end += cursor.pfn & ~(entry_end - 1);
 		entry_end = min(entry_end, end);
 
-- 
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: fix parameter documentation for amdgpu_vm_free_pts

[Christian König]

The function was modified without updating the documentation.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index aaf54fc8cafe..cad275eaba74 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -961,8 +961,7 @@ int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
  * amdgpu_vm_free_pts - free PD/PT levels
  *
  * @adev: amdgpu device structure
- * @parent: PD/PT starting level to free
- * @level: level of parent structure
+ * @vm: amdgpu vm structure
  *
  * Free the page directory or page table level and all sub levels.
  */
-- 
2.14.1

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

Re: [PATCH 2/2] drm/amdgpu: fix parameter documentation for amdgpu_vm_free_pts

[Huang Rui]

On Sat, Sep 15, 2018 at 10:05:41AM +0200, Christian König wrote:
> The function was modified without updating the documentation.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---

Reviewed-by: Huang Rui <ray.huang@amd.com>

>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index aaf54fc8cafe..cad275eaba74 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -961,8 +961,7 @@ int amdgpu_vm_alloc_pts(struct amdgpu_device *adev,
>   * amdgpu_vm_free_pts - free PD/PT levels
>   *
>   * @adev: amdgpu device structure
> - * @parent: PD/PT starting level to free
> - * @level: level of parent structure
> + * @vm: amdgpu vm structure
>   *
>   * Free the page directory or page table level and all sub levels.
>   */
> -- 
> 2.14.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 1/2] drm/amdgpu: add amdgpu_vm_entries_mask

[Huang Rui]

On Sat, Sep 15, 2018 at 10:05:40AM +0200, Christian König wrote:
> We can't get the mask for the root directory from the number of entries.
> 
> So add a new function to avoid that problem.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 34 +++++++++++++++++++++++++++-------
>  1 file changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index a7f9aaa47c49..aaf54fc8cafe 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -190,6 +190,26 @@ static unsigned amdgpu_vm_num_entries(struct amdgpu_device *adev,
>  		return AMDGPU_VM_PTE_COUNT(adev);
>  }
>  
> +/**
> + * amdgpu_vm_entries_mask - the mask to get the entry number of a PD/PT
> + *
> + * @adev: amdgpu_device pointer
> + * @level: VMPT level
> + *
> + * Returns:
> + * The mask to extract the entry number of a PD/PT from an address.
> + */
> +static uint32_t amdgpu_vm_entries_mask(struct amdgpu_device *adev,
> +				       unsigned int level)
> +{
> +	if (level <= adev->vm_manager.root_level)
> +		return 0xffffffff;
> +	else if (level != AMDGPU_VM_PTB)
> +		return 0x1f;

I think the mask should be 0x1ff (9 bits) while the level is on PDB. The
purpose here is to figure out the PTE entry id with the mask, am I right.
So the mask should bit width of one entry. 

Thanks,
Ray

> +	else
> +		return AMDGPU_VM_PTE_COUNT(adev) - 1;
> +}
> +
>  /**
>   * amdgpu_vm_bo_size - returns the size of the BOs in bytes
>   *
> @@ -399,17 +419,17 @@ static void amdgpu_vm_pt_start(struct amdgpu_device *adev,
>  static bool amdgpu_vm_pt_descendant(struct amdgpu_device *adev,
>  				    struct amdgpu_vm_pt_cursor *cursor)
>  {
> -	unsigned num_entries, shift, idx;
> +	unsigned mask, shift, idx;
>  
>  	if (!cursor->entry->entries)
>  		return false;
>  
>  	BUG_ON(!cursor->entry->base.bo);
> -	num_entries = amdgpu_vm_num_entries(adev, cursor->level);
> +	mask = amdgpu_vm_entries_mask(adev, cursor->level);
>  	shift = amdgpu_vm_level_shift(adev, cursor->level);
>  
>  	++cursor->level;
> -	idx = (cursor->pfn >> shift) % num_entries;
> +	idx = (cursor->pfn >> shift) & mask;
>  	cursor->parent = cursor->entry;
>  	cursor->entry = &cursor->entry->entries[idx];
>  	return true;
> @@ -1599,7 +1619,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>  	amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
>  	while (cursor.pfn < end) {
>  		struct amdgpu_bo *pt = cursor.entry->base.bo;
> -		unsigned shift, parent_shift, num_entries;
> +		unsigned shift, parent_shift, mask;
>  		uint64_t incr, entry_end, pe_start;
>  
>  		if (!pt)
> @@ -1654,9 +1674,9 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>  
>  		/* Looks good so far, calculate parameters for the update */
>  		incr = AMDGPU_GPU_PAGE_SIZE << shift;
> -		num_entries = amdgpu_vm_num_entries(adev, cursor.level);
> -		pe_start = ((cursor.pfn >> shift) & (num_entries - 1)) * 8;
> -		entry_end = num_entries << shift;
> +		mask = amdgpu_vm_entries_mask(adev, cursor.level);
> +		pe_start = ((cursor.pfn >> shift) & mask) * 8;
> +		entry_end = (mask + 1) << shift;
>  		entry_end += cursor.pfn & ~(entry_end - 1);
>  		entry_end = min(entry_end, end);
>  
> -- 
> 2.14.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 1/2] drm/amdgpu: add amdgpu_vm_entries_mask

[Christian König]

Am 17.09.2018 um 10:52 schrieb Huang Rui:
> On Sat, Sep 15, 2018 at 10:05:40AM +0200, Christian König wrote:
>> We can't get the mask for the root directory from the number of entries.
>>
>> So add a new function to avoid that problem.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 34 +++++++++++++++++++++++++++-------
>>   1 file changed, 27 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index a7f9aaa47c49..aaf54fc8cafe 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -190,6 +190,26 @@ static unsigned amdgpu_vm_num_entries(struct amdgpu_device *adev,
>>   		return AMDGPU_VM_PTE_COUNT(adev);
>>   }
>>   
>> +/**
>> + * amdgpu_vm_entries_mask - the mask to get the entry number of a PD/PT
>> + *
>> + * @adev: amdgpu_device pointer
>> + * @level: VMPT level
>> + *
>> + * Returns:
>> + * The mask to extract the entry number of a PD/PT from an address.
>> + */
>> +static uint32_t amdgpu_vm_entries_mask(struct amdgpu_device *adev,
>> +				       unsigned int level)
>> +{
>> +	if (level <= adev->vm_manager.root_level)
>> +		return 0xffffffff;
>> +	else if (level != AMDGPU_VM_PTB)
>> +		return 0x1f;
> I think the mask should be 0x1ff (9 bits) while the level is on PDB. The
> purpose here is to figure out the PTE entry id with the mask, am I right.
> So the mask should bit width of one entry.

Oh, yes of course that is a typo.

Thanks for catching,
Christian.

>
> Thanks,
> Ray
>
>> +	else
>> +		return AMDGPU_VM_PTE_COUNT(adev) - 1;
>> +}
>> +
>>   /**
>>    * amdgpu_vm_bo_size - returns the size of the BOs in bytes
>>    *
>> @@ -399,17 +419,17 @@ static void amdgpu_vm_pt_start(struct amdgpu_device *adev,
>>   static bool amdgpu_vm_pt_descendant(struct amdgpu_device *adev,
>>   				    struct amdgpu_vm_pt_cursor *cursor)
>>   {
>> -	unsigned num_entries, shift, idx;
>> +	unsigned mask, shift, idx;
>>   
>>   	if (!cursor->entry->entries)
>>   		return false;
>>   
>>   	BUG_ON(!cursor->entry->base.bo);
>> -	num_entries = amdgpu_vm_num_entries(adev, cursor->level);
>> +	mask = amdgpu_vm_entries_mask(adev, cursor->level);
>>   	shift = amdgpu_vm_level_shift(adev, cursor->level);
>>   
>>   	++cursor->level;
>> -	idx = (cursor->pfn >> shift) % num_entries;
>> +	idx = (cursor->pfn >> shift) & mask;
>>   	cursor->parent = cursor->entry;
>>   	cursor->entry = &cursor->entry->entries[idx];
>>   	return true;
>> @@ -1599,7 +1619,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>   	amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
>>   	while (cursor.pfn < end) {
>>   		struct amdgpu_bo *pt = cursor.entry->base.bo;
>> -		unsigned shift, parent_shift, num_entries;
>> +		unsigned shift, parent_shift, mask;
>>   		uint64_t incr, entry_end, pe_start;
>>   
>>   		if (!pt)
>> @@ -1654,9 +1674,9 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
>>   
>>   		/* Looks good so far, calculate parameters for the update */
>>   		incr = AMDGPU_GPU_PAGE_SIZE << shift;
>> -		num_entries = amdgpu_vm_num_entries(adev, cursor.level);
>> -		pe_start = ((cursor.pfn >> shift) & (num_entries - 1)) * 8;
>> -		entry_end = num_entries << shift;
>> +		mask = amdgpu_vm_entries_mask(adev, cursor.level);
>> +		pe_start = ((cursor.pfn >> shift) & mask) * 8;
>> +		entry_end = (mask + 1) << shift;
>>   		entry_end += cursor.pfn & ~(entry_end - 1);
>>   		entry_end = min(entry_end, end);
>>   
>> -- 
>> 2.14.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 1/2] drm/amdgpu: add amdgpu_vm_entries_mask

[Huang Rui]

On Mon, Sep 17, 2018 at 05:00:42PM +0800, Christian König wrote:
> Am 17.09.2018 um 10:52 schrieb Huang Rui:
> > On Sat, Sep 15, 2018 at 10:05:40AM +0200, Christian König wrote:
> >> We can't get the mask for the root directory from the number of entries.
> >>
> >> So add a new function to avoid that problem.
> >>
> >> Signed-off-by: Christian König <christian.koenig@amd.com>
> >> ---
> >>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 34 +++++++++++++++++++++++++++-------
> >>   1 file changed, 27 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> >> index a7f9aaa47c49..aaf54fc8cafe 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> >> @@ -190,6 +190,26 @@ static unsigned amdgpu_vm_num_entries(struct amdgpu_device *adev,
> >>   		return AMDGPU_VM_PTE_COUNT(adev);
> >>   }
> >>   
> >> +/**
> >> + * amdgpu_vm_entries_mask - the mask to get the entry number of a PD/PT
> >> + *
> >> + * @adev: amdgpu_device pointer
> >> + * @level: VMPT level
> >> + *
> >> + * Returns:
> >> + * The mask to extract the entry number of a PD/PT from an address.
> >> + */
> >> +static uint32_t amdgpu_vm_entries_mask(struct amdgpu_device *adev,
> >> +				       unsigned int level)
> >> +{
> >> +	if (level <= adev->vm_manager.root_level)
> >> +		return 0xffffffff;
> >> +	else if (level != AMDGPU_VM_PTB)
> >> +		return 0x1f;
> > I think the mask should be 0x1ff (9 bits) while the level is on PDB. The
> > purpose here is to figure out the PTE entry id with the mask, am I right.
> > So the mask should bit width of one entry.
> 
> Oh, yes of course that is a typo.
> 
> Thanks for catching,
> Christian.

No problem~
With that fixed, feel free to add my RB:
Reviewed-by: Huang Rui <ray.huang@amd.com>

> 
> >
> > Thanks,
> > Ray
> >
> >> +	else
> >> +		return AMDGPU_VM_PTE_COUNT(adev) - 1;
> >> +}
> >> +
> >>   /**
> >>    * amdgpu_vm_bo_size - returns the size of the BOs in bytes
> >>    *
> >> @@ -399,17 +419,17 @@ static void amdgpu_vm_pt_start(struct amdgpu_device *adev,
> >>   static bool amdgpu_vm_pt_descendant(struct amdgpu_device *adev,
> >>   				    struct amdgpu_vm_pt_cursor *cursor)
> >>   {
> >> -	unsigned num_entries, shift, idx;
> >> +	unsigned mask, shift, idx;
> >>   
> >>   	if (!cursor->entry->entries)
> >>   		return false;
> >>   
> >>   	BUG_ON(!cursor->entry->base.bo);
> >> -	num_entries = amdgpu_vm_num_entries(adev, cursor->level);
> >> +	mask = amdgpu_vm_entries_mask(adev, cursor->level);
> >>   	shift = amdgpu_vm_level_shift(adev, cursor->level);
> >>   
> >>   	++cursor->level;
> >> -	idx = (cursor->pfn >> shift) % num_entries;
> >> +	idx = (cursor->pfn >> shift) & mask;
> >>   	cursor->parent = cursor->entry;
> >>   	cursor->entry = &cursor->entry->entries[idx];
> >>   	return true;
> >> @@ -1599,7 +1619,7 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
> >>   	amdgpu_vm_pt_start(adev, params->vm, start, &cursor);
> >>   	while (cursor.pfn < end) {
> >>   		struct amdgpu_bo *pt = cursor.entry->base.bo;
> >> -		unsigned shift, parent_shift, num_entries;
> >> +		unsigned shift, parent_shift, mask;
> >>   		uint64_t incr, entry_end, pe_start;
> >>   
> >>   		if (!pt)
> >> @@ -1654,9 +1674,9 @@ static int amdgpu_vm_update_ptes(struct amdgpu_pte_update_params *params,
> >>   
> >>   		/* Looks good so far, calculate parameters for the update */
> >>   		incr = AMDGPU_GPU_PAGE_SIZE << shift;
> >> -		num_entries = amdgpu_vm_num_entries(adev, cursor.level);
> >> -		pe_start = ((cursor.pfn >> shift) & (num_entries - 1)) * 8;
> >> -		entry_end = num_entries << shift;
> >> +		mask = amdgpu_vm_entries_mask(adev, cursor.level);
> >> +		pe_start = ((cursor.pfn >> shift) & mask) * 8;
> >> +		entry_end = (mask + 1) << shift;
> >>   		entry_end += cursor.pfn & ~(entry_end - 1);
> >>   		entry_end = min(entry_end, end);
> >>   
> >> -- 
> >> 2.14.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