[1/3] drm/amdgpu: track if a PD needs an update

Submitted by Christian König on July 13, 2017, 1:25 p.m.

Details

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

Not browsing as part of any series.

Commit Message

Christian König July 13, 2017, 1:25 p.m.
From: Christian König <christian.koenig@amd.com>

That should reduce our command submission overhead quite a bit.

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

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 55d1c7f..3f5863e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -173,6 +173,7 @@  static int amdgpu_vm_validate_level(struct amdgpu_vm_pt *parent,
 	if (!parent->entries)
 		return 0;
 
+	parent->needs_update = true;
 	for (i = 0; i <= parent->last_entry_used; ++i) {
 		struct amdgpu_vm_pt *entry = &parent->entries[i];
 
@@ -351,6 +352,7 @@  static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
 
 			entry->bo = pt;
 			entry->addr = 0;
+			parent->needs_update = true;
 		}
 
 		if (level < adev->vm_manager.num_level) {
@@ -361,6 +363,8 @@  static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
 						   sub_eaddr, level);
 			if (r)
 				return r;
+
+			parent->needs_update |= entry->needs_update;
 		}
 	}
 
@@ -1050,7 +1054,7 @@  static int amdgpu_vm_update_level(struct amdgpu_device *adev,
 
 	int r;
 
-	if (!parent->entries)
+	if (!parent->entries || !parent->needs_update)
 		return 0;
 
 	memset(&params, 0, sizeof(params));
@@ -1196,6 +1200,7 @@  static int amdgpu_vm_update_level(struct amdgpu_device *adev,
 			return r;
 	}
 
+	parent->needs_update = false;
 	return 0;
 
 error_free:
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 3441ec5..c3dfd3e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -102,6 +102,7 @@  struct amdgpu_vm_pt {
 	/* array of page tables, one for each directory entry */
 	struct amdgpu_vm_pt	*entries;
 	unsigned		last_entry_used;
+	bool			needs_update;
 };
 
 struct amdgpu_vm {

Comments

On 17-07-13 09:25 AM, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
>
> That should reduce our command submission overhead quite a bit.
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 7 ++++++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 1 +
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 55d1c7f..3f5863e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -173,6 +173,7 @@ static int amdgpu_vm_validate_level(struct amdgpu_vm_pt *parent,
>  	if (!parent->entries)
>  		return 0;
>  
> +	parent->needs_update = true;

This means that after amdgpu_vm_validate_pt_bos, all vm_pts will have
needs_update set, even if the last eviction didn't evict any page
tables. That will create unnecessary overhead in amdgpu_vm_update_level.
You could probably add another check in the loop below for whether
validate actually moved any child page tables.

Regards,
  Felix

>  	for (i = 0; i <= parent->last_entry_used; ++i) {
>  		struct amdgpu_vm_pt *entry = &parent->entries[i];
>  
> @@ -351,6 +352,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>  
>  			entry->bo = pt;
>  			entry->addr = 0;
> +			parent->needs_update = true;
>  		}
>  
>  		if (level < adev->vm_manager.num_level) {
> @@ -361,6 +363,8 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>  						   sub_eaddr, level);
>  			if (r)
>  				return r;
> +
> +			parent->needs_update |= entry->needs_update;
>  		}
>  	}
>  
> @@ -1050,7 +1054,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>  
>  	int r;
>  
> -	if (!parent->entries)
> +	if (!parent->entries || !parent->needs_update)
>  		return 0;
>  
>  	memset(&params, 0, sizeof(params));
> @@ -1196,6 +1200,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>  			return r;
>  	}
>  
> +	parent->needs_update = false;
>  	return 0;
>  
>  error_free:
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 3441ec5..c3dfd3e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -102,6 +102,7 @@ struct amdgpu_vm_pt {
>  	/* array of page tables, one for each directory entry */
>  	struct amdgpu_vm_pt	*entries;
>  	unsigned		last_entry_used;
> +	bool			needs_update;
>  };
>  
>  struct amdgpu_vm {
'if (parent->entries[pt_idx].addr == pt)' is good enough for condition 
of need_update, adding previous Felix's optimization patch, then the 
whole picture looks better than this one, IIRC.

Regards,
David Zhou
On 2017年07月13日 21:25, Christian König wrote:
> From: Christian König <christian.koenig@amd.com>
>
> That should reduce our command submission overhead quite a bit.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 7 ++++++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 1 +
>   2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 55d1c7f..3f5863e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -173,6 +173,7 @@ static int amdgpu_vm_validate_level(struct amdgpu_vm_pt *parent,
>   	if (!parent->entries)
>   		return 0;
>   
> +	parent->needs_update = true;
>   	for (i = 0; i <= parent->last_entry_used; ++i) {
>   		struct amdgpu_vm_pt *entry = &parent->entries[i];
>   
> @@ -351,6 +352,7 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>   
>   			entry->bo = pt;
>   			entry->addr = 0;
> +			parent->needs_update = true;
>   		}
>   
>   		if (level < adev->vm_manager.num_level) {
> @@ -361,6 +363,8 @@ static int amdgpu_vm_alloc_levels(struct amdgpu_device *adev,
>   						   sub_eaddr, level);
>   			if (r)
>   				return r;
> +
> +			parent->needs_update |= entry->needs_update;
>   		}
>   	}
>   
> @@ -1050,7 +1054,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>   
>   	int r;
>   
> -	if (!parent->entries)
> +	if (!parent->entries || !parent->needs_update)
>   		return 0;
>   
>   	memset(&params, 0, sizeof(params));
> @@ -1196,6 +1200,7 @@ static int amdgpu_vm_update_level(struct amdgpu_device *adev,
>   			return r;
>   	}
>   
> +	parent->needs_update = false;
>   	return 0;
>   
>   error_free:
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 3441ec5..c3dfd3e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -102,6 +102,7 @@ struct amdgpu_vm_pt {
>   	/* array of page tables, one for each directory entry */
>   	struct amdgpu_vm_pt	*entries;
>   	unsigned		last_entry_used;
> +	bool			needs_update;
>   };
>   
>   struct amdgpu_vm {