-----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,
