[PATCH 2/5] drm/amdgpu: Add vm context module param

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




-----Original Message-----
From: Christian König [mailto:deathsimple@xxxxxxxxxxx] 
Sent: Wednesday, May 24, 2017 5:41 AM
To: Kasiviswanathan, Harish <Harish.Kasiviswanathan at amd.com>; amd-gfx at lists.freedesktop.org
Subject: Re: [PATCH 2/5] drm/amdgpu: Add vm context module param

Am 15.05.2017 um 23:32 schrieb Harish Kasiviswanathan:
> Add VM update mode module param (amdgpu.vm_update_mode) that can used 
> to control how VM pde/pte are updated for Graphics and Compute.
>
> BIT0 controls Graphics and BIT1 Compute.
>   BIT0 [= 0] Graphics updated by SDMA [= 1] by CPU
>   BIT1 [= 0] Compute updated by SDMA [= 1] by CPU
>
> By default, only for large BAR system vm_update_mode = 2, indicating 
> that Graphics VMs will be updated via SDMA and Compute VMs will be 
> updated via CPU. And for all all other systems (by default) 
> vm_update_mode = 0
>
> Signed-off-by: Harish Kasiviswanathan <Harish.Kasiviswanathan at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  1 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  4 ++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  3 ++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 35 ++++++++++++++++++++++++++++++++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  | 20 ++++++++++++++++++-
>   5 files changed, 60 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index fadeb55..fd84410 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -94,6 +94,7 @@
>   extern int amdgpu_vm_block_size;
>   extern int amdgpu_vm_fault_stop;
>   extern int amdgpu_vm_debug;
> +extern int amdgpu_vm_update_mode;
>   extern int amdgpu_dc;
>   extern int amdgpu_sched_jobs;
>   extern int amdgpu_sched_hw_submission; diff --git 
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 130c45d..8d28a35 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -94,6 +94,7 @@
>   int amdgpu_vm_fault_stop = 0;
>   int amdgpu_vm_debug = 0;
>   int amdgpu_vram_page_split = 512;
> +int amdgpu_vm_update_mode = -1;
>   int amdgpu_exp_hw_support = 0;
>   int amdgpu_dc = -1;
>   int amdgpu_sched_jobs = 32;
> @@ -180,6 +181,9 @@
>   MODULE_PARM_DESC(vm_debug, "Debug VM handling (0 = disabled (default), 1 = enabled)");
>   module_param_named(vm_debug, amdgpu_vm_debug, int, 0644);
>   
> +MODULE_PARM_DESC(vm_update_mode, "VM update using CPU (0 = never 
> +(default except for large BAR(LB)), 1 = Graphics only, 2 = Compute 
> +only (default for LB), 3 = Both"); module_param_named(vm_update_mode, 
> +amdgpu_vm_update_mode, int, 0444);
> +
>   MODULE_PARM_DESC(vram_page_split, "Number of pages after we split VRAM allocations (default 1024, -1 = disable)");
>   module_param_named(vram_page_split, amdgpu_vram_page_split, int, 
> 0444);
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index d167949..8f6c20f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -774,7 +774,8 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
>   		goto out_suspend;
>   	}
>   
> -	r = amdgpu_vm_init(adev, &fpriv->vm);
> +	r = amdgpu_vm_init(adev, &fpriv->vm,
> +			   AMDGPU_VM_CONTEXT_GFX);
>   	if (r) {
>   		kfree(fpriv);
>   		goto out_suspend;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index c644e54..9c89cb2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -721,6 +721,11 @@ bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
>   	return true;
>   }
>   
> +static bool amdgpu_vm_is_large_bar(struct amdgpu_device *adev) {
> +	return (adev->mc.real_vram_size == adev->mc.visible_vram_size); }
> +
>   /**
>    * amdgpu_vm_flush - hardware flush the vm
>    *
> @@ -2291,10 +2296,12 @@ void amdgpu_vm_adjust_size(struct amdgpu_device *adev, uint64_t vm_size)
>    *
>    * @adev: amdgpu_device pointer
>    * @vm: requested vm
> + * @vm_context: Indicates if it GFX or Compute context
>    *
>    * Init @vm fields.
>    */
> -int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm)
> +int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> +		   int vm_context)
>   {
>   	const unsigned align = min(AMDGPU_VM_PTB_ALIGN_SIZE,
>   		AMDGPU_VM_PTE_COUNT(adev) * 8);
> @@ -2323,6 +2330,16 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   	if (r)
>   		return r;
>   
> +	if (vm_context == AMDGPU_VM_CONTEXT_COMPUTE)
> +		vm->use_cpu_for_update = !!(adev->vm_manager.vm_update_mode &
> +						AMDGPU_VM_USE_CPU_FOR_COMPUTE);
> +	else
> +		vm->use_cpu_for_update = !!(adev->vm_manager.vm_update_mode &
> +						AMDGPU_VM_USE_CPU_FOR_GFX);
> +	DRM_DEBUG_DRIVER("VM update mode is %s\n",
> +			 vm->use_cpu_for_update ? "CPU" : "SDMA");
> +	WARN_ONCE((vm->use_cpu_for_update & !amdgpu_vm_is_large_bar(adev)),
> +		  "CPU update of VM recommended only for large BAR system\n");
>   	vm->last_dir_update = NULL;
>   
>   	r = amdgpu_bo_create(adev, amdgpu_vm_bo_size(adev, 0), align, true, 
> @@ -2454,6 +2471,22 @@ void amdgpu_vm_manager_init(struct amdgpu_device *adev)
>   	atomic64_set(&adev->vm_manager.client_counter, 0);
>   	spin_lock_init(&adev->vm_manager.prt_lock);
>   	atomic_set(&adev->vm_manager.num_prt_users, 0);
> +
> +	/* If not overridden by the user, by default, only in large BAR systems
> +	 * Compute VM tables will be updated by CPU
> +	 */
> +#ifdef CONFIG_X86_64
> +	if (amdgpu_vm_update_mode == -1) {
> +		if (amdgpu_vm_is_large_bar(adev))
> +			adev->vm_manager.vm_update_mode =
> +				AMDGPU_VM_USE_CPU_FOR_COMPUTE;
> +		else
> +			adev->vm_manager.vm_update_mode = 0;
> +	}

Aren't you missing the else case here?

In other words when amdgpu_vm_update_mode is not -1 we should take it's value for vm_update_mode.

[HK]: Good catch. Thanks for reviews. I have fixed it and pushed the code.


With that fixed the patch is Reviewed-by: Christian König <christian.koenig at amd.com>.

Regards,
Christian.

> +#else
> +	adev->vm_manager.vm_update_mode = 0; #endif
> +
>   }
>   
>   /**
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index afe9073..9aa00d9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -87,6 +87,14 @@
>   /* max vmids dedicated for process */
>   #define AMDGPU_VM_MAX_RESERVED_VMID	1
>   
> +#define AMDGPU_VM_CONTEXT_GFX 0
> +#define AMDGPU_VM_CONTEXT_COMPUTE 1
> +
> +/* See vm_update_mode */
> +#define AMDGPU_VM_USE_CPU_FOR_GFX (1 << 0) #define 
> +AMDGPU_VM_USE_CPU_FOR_COMPUTE (1 << 1)
> +
> +
>   struct amdgpu_vm_pt {
>   	struct amdgpu_bo	*bo;
>   	uint64_t		addr;
> @@ -129,6 +137,9 @@ struct amdgpu_vm {
>   	struct amdgpu_vm_id	*reserved_vmid[AMDGPU_MAX_VMHUBS];
>   	/* each VM will map on CSA */
>   	struct amdgpu_bo_va *csa_bo_va;
> +
> +	/* Flag to indicate if VM tables are updated by CPU or GPU (SDMA) */
> +	bool                    use_cpu_for_update;
>   };
>   
>   struct amdgpu_vm_id {
> @@ -184,11 +195,18 @@ struct amdgpu_vm_manager {
>   	/* partial resident texture handling */
>   	spinlock_t				prt_lock;
>   	atomic_t				num_prt_users;
> +
> +	/* controls how VM page tables are updated for Graphics and Compute.
> +	 * BIT0[= 0] Graphics updated by SDMA [= 1] by CPU
> +	 * BIT1[= 0] Compute updated by SDMA [= 1] by CPU
> +	 */
> +	int					vm_update_mode;
>   };
>   
>   void amdgpu_vm_manager_init(struct amdgpu_device *adev);
>   void amdgpu_vm_manager_fini(struct amdgpu_device *adev); -int 
> amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm);
> +int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> +		   int vm_context);
>   void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm);
>   void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
>   			 struct list_head *validated,




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux