[PATCH] radeon: avoid double free in ci_dpm_init()
There are several calls to ci_dpm_fini() in ci_dpm_init() when there
occur errors in functions like r600_parse_extended_power_table().
This is harmful as it can lead to double free situations: for
instance, r600_parse_extended_power_table() will call for
r600_free_extended_power_table() as will ci_dpm_fini(), both
of which will try to free resources.
Other drivers do not call *_dpm_fini functions from their
respective *_dpm_init calls - neither should cpm_dpm_init().
Fix this by removing extra calls to ci_dpm_fini().
Found by Linux Verification Center (linuxtesting.org) with static
analysis tool SVACE.
Fixes: cc8dbbb4f62a ("drm/radeon: add dpm support for CI dGPUs (v2)")
Cc: sta...@vger.kernel.org
Co-developed-by: Natalia Petrova
Signed-off-by: Nikita Zhandarovich
---
drivers/gpu/drm/radeon/ci_dpm.c | 20 +---
1 file changed, 5 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/radeon/ci_dpm.c b/drivers/gpu/drm/radeon/ci_dpm.c
index 8ef25ab305ae..7b77d4c93f1d 100644
--- a/drivers/gpu/drm/radeon/ci_dpm.c
+++ b/drivers/gpu/drm/radeon/ci_dpm.c
@@ -5677,28 +5677,20 @@ int ci_dpm_init(struct radeon_device *rdev)
pi->pcie_lane_powersaving.min = 16;
ret = ci_get_vbios_boot_values(rdev, &pi->vbios_boot_state);
- if (ret) {
- ci_dpm_fini(rdev);
+ if (ret)
return ret;
- }
ret = r600_get_platform_caps(rdev);
- if (ret) {
- ci_dpm_fini(rdev);
+ if (ret)
return ret;
- }
ret = r600_parse_extended_power_table(rdev);
- if (ret) {
- ci_dpm_fini(rdev);
+ if (ret)
return ret;
- }
ret = ci_parse_power_table(rdev);
- if (ret) {
- ci_dpm_fini(rdev);
+ if (ret)
return ret;
- }
pi->dll_default_on = false;
pi->sram_end = SMC_RAM_END;
@@ -5749,10 +5741,8 @@ int ci_dpm_init(struct radeon_device *rdev)
kcalloc(4,
sizeof(struct radeon_clock_voltage_dependency_entry),
GFP_KERNEL);
- if (!rdev->pm.dpm.dyn_state.vddc_dependency_on_dispclk.entries) {
- ci_dpm_fini(rdev);
+ if (!rdev->pm.dpm.dyn_state.vddc_dependency_on_dispclk.entries)
return -ENOMEM;
- }
rdev->pm.dpm.dyn_state.vddc_dependency_on_dispclk.count = 4;
rdev->pm.dpm.dyn_state.vddc_dependency_on_dispclk.entries[0].clk = 0;
rdev->pm.dpm.dyn_state.vddc_dependency_on_dispclk.entries[0].v = 0;
[PATCH] drm/amd/display: Fix potential null dereference
The adev->dm.dc pointer can be NULL and dereferenced in amdgpu_dm_fini()
without checking.
Add a NULL pointer check before calling dc_dmub_srv_destroy().
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: 9a71c7d31734 ("drm/amd/display: Register DMUB service with DC")
Signed-off-by: Igor Artemiev
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index a01fd41643fc..27f7a554874e 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1854,7 +1854,8 @@ static void amdgpu_dm_fini(struct amdgpu_device *adev)
dc_deinit_callbacks(adev->dm.dc);
#endif
- dc_dmub_srv_destroy(&adev->dm.dc->ctx->dmub_srv);
+ if (adev->dm.dc)
+ dc_dmub_srv_destroy(&adev->dm.dc->ctx->dmub_srv);
if (dc_enable_dmub_notifications(adev->dm.dc)) {
kfree(adev->dm.dmub_notify);
--
2.30.2
[bug report] drm/amd/display: Use per pipe P-State force for FPO
Hello Alvin Lee, This is a semi-automatic email about new static checker warnings. The patch 4ed793083afc: "drm/amd/display: Use per pipe P-State force for FPO" from Mar 15, 2023, leads to the following Smatch complaint: drivers/gpu/drm/amd/amdgpu/../display/dc/dcn20/dcn20_hwseq.c:2009 dcn20_post_unlock_program_front_end() error: we previously assumed 'hwseq' could be null (see line 2003) drivers/gpu/drm/amd/amdgpu/../display/dc/dcn20/dcn20_hwseq.c 2002 */ 2003 if (hwseq && hwseq->funcs.update_force_pstate) ^ The patch adds this NULL check but hopefully it can be deleted. Otherwise we are screwed. 2004 dc->hwseq->funcs.update_force_pstate(dc, context); 2005 2006 /* Only program the MALL registers after all the main and phantom pipes 2007 * are done programming. 2008 */ 2009 if (hwseq->funcs.program_mall_pipe_config) 2010 hwseq->funcs.program_mall_pipe_config(dc, context); 2011 regards, dan carpenter
Re: [RFC PATCH 0/4] uapi, drm: Add and implement RLIMIT_GPUPRIO
Adding a bunch of people who have been involved in this before. Am 03.04.23 um 22:15 schrieb Joshua Ashton: On 4/3/23 20:54, Christian König wrote: Am 03.04.23 um 21:40 schrieb Joshua Ashton: [SNIP] Anyway, please let me know what you think! Definitely open to any feedback and advice you may have. :D Well the basic problem is that higher priority queues can be used to starve low priority queues. This starvation in turn is very very bad for memory management since the dma_fence's the GPU scheduler deals with have very strong restrictions. Even exposing this under CAP_SYS_NICE is questionable, so we will most likely have to NAK this. This is already exposed with CAP_SYS_NICE and is relied on by SteamVR for async reprojection and Gamescope's composite path on Steam Deck. Yeah, I know I was the one who designed that :) Having a high priority async compute queue is really really important and advantageous for these tasks. The majority of usecases for something like this is going to be a compositor which does some really tiny amount of work per-frame but is incredibly latency dependent (as it depends on latching onto buffers just before vblank to do it's work) Starving and surpassing work on other queues is kind of the entire point. Gamescope and SteamVR do it on ACE as well so GFX work can run alongside it. Yes, unfortunately exactly that. The problem is that our memory management is designed around the idea that submissions to the hardware are guaranteed to finish at some point in the future. When we now have a functionality which allows to extend the amount of time some work needs to finish on the hardware infinitely, then we have a major problem at hand. What we could do is to make the GPU scheduler more clever and make sure that while higher priority submissions get precedence and can even preempt low priority submissions we still guarantee some forward progress for everybody. Luben has been looking into a similar problem AMD internally as well, maybe he has some idea here but I doubt that the solution will be simple. Regards, Christian. - Joshie 🐸✨
[PATCH 1/3] drm/amdgpu: Add userptr bo support for mGPUs when iommu is on
For userptr bo with iommu on, multiple GPUs use same system
memory dma mapping address when both bo_adev and adev in identity
mode or in the same iommu group.
Signed-off-by: Shane Xiao
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index e7403f8e4eba..33cda358cb9e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -804,11 +804,11 @@ static int kfd_mem_attach(struct amdgpu_device *adev,
struct kgd_mem *mem,
va + bo_size, vm);
if ((adev == bo_adev && !(mem->alloc_flags &
KFD_IOC_ALLOC_MEM_FLAGS_MMIO_REMAP)) ||
- (amdgpu_ttm_tt_get_usermm(mem->bo->tbo.ttm) &&
adev->ram_is_direct_mapped) ||
- same_hive) {
+ (amdgpu_ttm_tt_get_usermm(mem->bo->tbo.ttm) &&
((adev->ram_is_direct_mapped && bo_adev->ram_is_direct_mapped) ||
+ adev->dev->iommu_group == bo_adev->dev->iommu_group))
|| same_hive){
/* Mappings on the local GPU, or VRAM mappings in the
-* local hive, or userptr mapping IOMMU direct map mode
-* share the original BO
+* local hive, or userptr mapping in the same dma
+* address space share the original BO
*/
attachment[i]->type = KFD_MEM_ATT_SHARED;
bo[i] = mem->bo;
--
2.25.1
[PATCH 2/3] amd/amdgpu: Inherit coherence flags base on original BO flags
For SG BO to DMA-map userptrs on other GPUs, the SG BO need inherit
MTYPEs in PTEs from original BO.
If we set the flags, the device can be coherent with the CPUs and other GPUs.
Signed-off-by: Shane Xiao
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 33cda358cb9e..bcb0a7b32703 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -253,14 +253,22 @@ create_dmamap_sg_bo(struct amdgpu_device *adev,
{
struct drm_gem_object *gem_obj;
int ret, align;
+ uint64_t flags = 0;
ret = amdgpu_bo_reserve(mem->bo, false);
if (ret)
return ret;
align = 1;
+ if(mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_USERPTR)
+ {
+ flags |= mem->bo->flags &(AMDGPU_GEM_CREATE_CPU_GTT_USWC |
+ AMDGPU_GEM_CREATE_COHERENT |
AMDGPU_GEM_CREATE_UNCACHED);
+ align = PAGE_SIZE;
+ }
+
ret = amdgpu_gem_object_create(adev, mem->bo->tbo.base.size, align,
- AMDGPU_GEM_DOMAIN_CPU, AMDGPU_GEM_CREATE_PREEMPTIBLE,
+ AMDGPU_GEM_DOMAIN_CPU, AMDGPU_GEM_CREATE_PREEMPTIBLE |
flags,
ttm_bo_type_sg, mem->bo->tbo.base.resv, &gem_obj);
amdgpu_bo_unreserve(mem->bo);
--
2.25.1
[PATCH 3/3] drm/amdgpu: DROP redundant drm_prime_sg_to_dma_addr_array
For DMA-MAP userptr on other GPUs, the dma address array has been populated in amdgpu_ttm_backend_bind. Remove the redundant call from the driver. Signed-off-by: Shane Xiao --- drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index bcb0a7b32703..94ee8f638c12 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -488,9 +488,6 @@ kfd_mem_dmamap_userptr(struct kgd_mem *mem, if (unlikely(ret)) goto release_sg; - drm_prime_sg_to_dma_addr_array(ttm->sg, ttm->dma_address, - ttm->num_pages); - amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT); ret = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); if (ret) -- 2.25.1
Re: [PATCH 1/3] drm/amdgpu: Add userptr bo support for mGPUs when iommu is on
Am 04.04.23 um 11:56 schrieb Shane Xiao:
For userptr bo with iommu on, multiple GPUs use same system
memory dma mapping address when both bo_adev and adev in identity
mode or in the same iommu group.
WTF? Userptr BOs are not allowed to be exported/imported between
different GPUs.
So how can the same userptr BO be used on different GPUs?
Regards,
Christian.
Signed-off-by: Shane Xiao
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index e7403f8e4eba..33cda358cb9e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -804,11 +804,11 @@ static int kfd_mem_attach(struct amdgpu_device *adev,
struct kgd_mem *mem,
va + bo_size, vm);
if ((adev == bo_adev && !(mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_MMIO_REMAP)) ||
- (amdgpu_ttm_tt_get_usermm(mem->bo->tbo.ttm) &&
adev->ram_is_direct_mapped) ||
- same_hive) {
+ (amdgpu_ttm_tt_get_usermm(mem->bo->tbo.ttm) &&
((adev->ram_is_direct_mapped && bo_adev->ram_is_direct_mapped) ||
+ adev->dev->iommu_group == bo_adev->dev->iommu_group))
|| same_hive){
/* Mappings on the local GPU, or VRAM mappings in the
-* local hive, or userptr mapping IOMMU direct map mode
-* share the original BO
+* local hive, or userptr mapping in the same dma
+* address space share the original BO
*/
attachment[i]->type = KFD_MEM_ATT_SHARED;
bo[i] = mem->bo;
Re: BUG: KASAN: slab-use-after-free in drm_sched_get_cleanup_job+0x47b/0x5c0 [gpu_sched]
On Fri, Mar 24, 2023 at 7:37 PM Christian König wrote: > > Yeah, that one > > Thanks for the info, looks like this isn't fixed. > > Christian. > Hi, glad to see that "BUG: KASAN: slab-use-after-free in drm_sched_get_cleanup_job+0x47b/0x5c0" was fixed in 6.3-rc5. For history it would be good to know the commit which fixes this issue. I waited for this moment because I know other one issue which was also found by KASAN santiniser. BUG: KASAN: null-ptr-deref in drm_sched_job_cleanup+0x96/0x290 [gpu_sched] Read of size 4 at addr 0078 by task GameThread/23915 CPU: 10 PID: 23915 Comm: GameThread Tainted: GWL --- --- 6.3.0-0.rc5.42.fc39.x86_64+debug #1 Hardware name: System manufacturer System Product Name/ROG STRIX X570-I GAMING, BIOS 4601 02/02/2023 Call Trace: dump_stack_lvl+0x72/0xc0 kasan_report+0xa4/0xe0 ? drm_sched_job_cleanup+0x96/0x290 [gpu_sched] kasan_check_range+0x104/0x1b0 drm_sched_job_cleanup+0x96/0x290 [gpu_sched] ? __pfx_drm_sched_job_cleanup+0x10/0x10 [gpu_sched] ? slab_free_freelist_hook+0x11e/0x1d0 ? amdgpu_cs_parser_fini+0x363/0x5a0 [amdgpu] amdgpu_job_free+0x40/0x1b0 [amdgpu] amdgpu_cs_parser_fini+0x3c9/0x5a0 [amdgpu] ? __pfx_amdgpu_cs_parser_fini+0x10/0x10 [amdgpu] amdgpu_cs_ioctl+0x3d9/0x5630 [amdgpu] ? __pfx_amdgpu_cs_ioctl+0x10/0x10 [amdgpu] ? mark_lock+0x101/0x16e0 ? __lock_acquire+0xe54/0x59f0 ? __pfx_lock_release+0x10/0x10 ? __pfx_amdgpu_cs_ioctl+0x10/0x10 [amdgpu] drm_ioctl_kernel+0x1f8/0x3d0 ? __pfx_drm_ioctl_kernel+0x10/0x10 drm_ioctl+0x4c1/0xaa0 ? __pfx_amdgpu_cs_ioctl+0x10/0x10 [amdgpu] ? __pfx_drm_ioctl+0x10/0x10 ? _raw_spin_unlock_irqrestore+0x62/0x80 ? lockdep_hardirqs_on+0x7d/0x100 ? _raw_spin_unlock_irqrestore+0x4b/0x80 amdgpu_drm_ioctl+0xce/0x1b0 [amdgpu] __x64_sys_ioctl+0x12d/0x1a0 do_syscall_64+0x5c/0x90 ? do_syscall_64+0x68/0x90 ? lockdep_hardirqs_on+0x7d/0x100 ? do_syscall_64+0x68/0x90 ? do_syscall_64+0x68/0x90 ? lockdep_hardirqs_on+0x7d/0x100 ? do_syscall_64+0x68/0x90 ? do_syscall_64+0x68/0x90 ? lockdep_hardirqs_on+0x7d/0x100 entry_SYSCALL_64_after_hwframe+0x72/0xdc RIP: 0033:0x7fe97a50881d Code: 04 25 28 00 00 00 48 89 45 c8 31 c0 48 8d 45 10 c7 45 b0 10 00 00 00 48 89 45 b8 48 8d 45 d0 48 89 45 c0 b8 10 00 00 00 0f 05 <89> c2 3d 00 f0 ff ff 77 1a 48 8b 45 c8 64 48 2b 04 25 28 00 00 00 RSP: 002b:7c35d3f0 EFLAGS: 0246 ORIG_RAX: 0010 RAX: ffda RBX: 7c35d6e8 RCX: 7fe97a50881d RDX: 7c35d4d0 RSI: c0186444 RDI: 00ae RBP: 7c35d440 R08: 7fe8fc0f0970 R09: 7c35d490 R10: 7fb79000 R11: 0246 R12: 7c35d4d0 R13: c0186444 R14: 00ae R15: 7fe8fc0f0900 I know at least 3 games which 100% triggering this bug: - Cyberpunk 2077 - Forza Horizon 4 - Forza Horizon 5 We would continue to discuss it here or better create a new thread (for someone who is also faced with this issue could easily find a solution on the internet)? A full kernel log as usual attached here. -- Best Regards, Mike Gavrilov. dmesg.tar.xz Description: application/xz
Re: [RFC PATCH 0/4] uapi, drm: Add and implement RLIMIT_GPUPRIO
Hi, On 03/04/2023 20:40, Joshua Ashton wrote: Hello all! I would like to propose a new API for allowing processes to control the priority of GPU queues similar to RLIMIT_NICE/RLIMIT_RTPRIO. The main reason for this is for compositors such as Gamescope and SteamVR vrcompositor to be able to create realtime async compute queues on AMD without the need of CAP_SYS_NICE. The current situation is bad for a few reasons, one being that in order to setcap the executable, typically one must run as root which involves a pretty high privelage escalation in order to achieve one small feat, a realtime async compute queue queue for VR or a compositor. The executable cannot be setcap'ed inside a container nor can the setcap'ed executable be run in a container with NO_NEW_PRIVS. I go into more detail in the description in `uapi: Add RLIMIT_GPUPRIO`. My initial proposal here is to add a new RLIMIT, `RLIMIT_GPUPRIO`, which seems to make most initial sense to me to solve the problem. I am definitely not set that this is the best formulation however or if this should be linked to DRM (in terms of it's scheduler priority enum/definitions) in any way and and would really like other people's opinions across the stack on this. Once initial concern is that potentially this RLIMIT could out-live the lifespan of DRM. It sounds crazy saying it right now, something that definitely popped into my mind when touching `resource.h`. :-) Anyway, please let me know what you think! Definitely open to any feedback and advice you may have. :D Interesting! I tried to solved the similar problem two times in the past already. First time I was proposing to tie nice to DRM scheduling priority [1] - if the latter has been left at default - drawing the analogy with the nice+ionice handling. That was rejected and I was nudged towards the cgroups route. So with that second attempt I implemented a hierarchical opaque drm.priority cgroup controller [2]. I think it would allow you to solve your use case too by placing your compositor in a cgroup with an elevated priority level. Implementation wise in my proposal it was left to individual drivers to "meld" the opaque cgroup drm.priority with the driver specific priority concept. That too wasn't too popular with the feedback (AFAIR) that the priority is a too subsystem specific concept. Finally I was left with a weight based drm cgroup controller, exactly following the controls of the CPU and IO ones, but with much looser runtime guarantees. [3] I don't think this last one works for your use case, at least not at the current state for drm scheduling capability, where the implementation is a "bit" too reactive for realtime. Depending on how the discussion around your rlimit proposal goes, perhaps one alternative could be to go the cgroup route and add an attribute like drm.realtime. That perhaps sounds abstract and generic enough to be passable. Built as a simplification of [2] it wouldn't be too complicated. On the actual proposal of RLIMIT_GPUPRIO... The name would be problematic since we have generic hw accelerators (not just GPUs) under the DRM subsystem. Perhaps RLIMIT_DRMPRIO would be better but I think you will need to copy some more mailing lists and people on that one. Because I can imagine one or two more fundamental questions this opens up, as you have eluded in your cover letter as well. Regards, Tvrtko [1] https://lore.kernel.org/dri-devel/20220407152806.3387898-1-tvrtko.ursu...@linux.intel.com/T/ [2] https://lore.kernel.org/lkml/20221019173254.3361334-4-tvrtko.ursu...@linux.intel.com/T/#u [3] https://lore.kernel.org/lkml/20230314141904.1210824-1-tvrtko.ursu...@linux.intel.com/
RE: [PATCH 1/3] drm/amdgpu: Add userptr bo support for mGPUs when iommu is on
[AMD Official Use Only - General]
> -Original Message-
> From: Koenig, Christian
> Sent: Tuesday, April 4, 2023 6:27 PM
> To: Xiao, Shane ; amd-gfx@lists.freedesktop.org;
> Kuehling, Felix
> Cc: Liu, Aaron ; Guo, Shikai
> Subject: Re: [PATCH 1/3] drm/amdgpu: Add userptr bo support for mGPUs
> when iommu is on
>
> Am 04.04.23 um 11:56 schrieb Shane Xiao:
> > For userptr bo with iommu on, multiple GPUs use same system memory dma
> > mapping address when both bo_adev and adev in identity mode or in the
> > same iommu group.
Hi Christian,
>
> WTF? Userptr BOs are not allowed to be exported/imported between different
> GPUs.
>
> So how can the same userptr BO be used on different GPUs?
If GPUs are all in iommu identity mode which means dma address are the same as
physical address, all of the GPUs can see the system memory directly.
In such case, should we export/import the BO, then create a new SG BO for
another GPU?
Best Regards,
Shane
>
> Regards,
> Christian.
>
> >
> > Signed-off-by: Shane Xiao
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 8
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > index e7403f8e4eba..33cda358cb9e 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > @@ -804,11 +804,11 @@ static int kfd_mem_attach(struct amdgpu_device
> *adev, struct kgd_mem *mem,
> > va + bo_size, vm);
> >
> > if ((adev == bo_adev && !(mem->alloc_flags &
> KFD_IOC_ALLOC_MEM_FLAGS_MMIO_REMAP)) ||
> > - (amdgpu_ttm_tt_get_usermm(mem->bo->tbo.ttm) &&
> adev->ram_is_direct_mapped) ||
> > - same_hive) {
> > + (amdgpu_ttm_tt_get_usermm(mem->bo->tbo.ttm) &&
> ((adev->ram_is_direct_mapped && bo_adev->ram_is_direct_mapped) ||
> > + adev->dev->iommu_group == bo_adev->dev-
> >iommu_group)) ||
> > +same_hive){
> > /* Mappings on the local GPU, or VRAM mappings in
> the
> > -* local hive, or userptr mapping IOMMU direct map
> mode
> > -* share the original BO
> > +* local hive, or userptr mapping in the same dma
> > +* address space share the original BO
> > */
> > attachment[i]->type = KFD_MEM_ATT_SHARED;
> > bo[i] = mem->bo;
Re: [PATCH 1/3] drm/amdgpu: Add userptr bo support for mGPUs when iommu is on
Am 04.04.23 um 12:56 schrieb Xiao, Shane:
[AMD Official Use Only - General]
-Original Message-
From: Koenig, Christian
Sent: Tuesday, April 4, 2023 6:27 PM
To: Xiao, Shane ; amd-gfx@lists.freedesktop.org;
Kuehling, Felix
Cc: Liu, Aaron ; Guo, Shikai
Subject: Re: [PATCH 1/3] drm/amdgpu: Add userptr bo support for mGPUs
when iommu is on
Am 04.04.23 um 11:56 schrieb Shane Xiao:
For userptr bo with iommu on, multiple GPUs use same system memory dma
mapping address when both bo_adev and adev in identity mode or in the
same iommu group.
Hi Christian,
WTF? Userptr BOs are not allowed to be exported/imported between different
GPUs.
So how can the same userptr BO be used on different GPUs?
If GPUs are all in iommu identity mode which means dma address are the same as
physical address, all of the GPUs can see the system memory directly.
In such case, should we export/import the BO, then create a new SG BO for
another GPU?
Yes, absolutely. Each userptr BO is only meant to be used on one GPU.
Even if you could use the same BO for multiple GPUs it's not necessary a
good idea since you then have live time problems for example.
E.g. what happens when one GPU is hot removed while the other one who
imported the BO is still in use?
Felix can you comment on that? My recollection was that we rather
improve the storage of DMA addresses instead of duplicating the BOs over
different GPUs.
Regards,
Christian.
Best Regards,
Shane
Regards,
Christian.
Signed-off-by: Shane Xiao
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index e7403f8e4eba..33cda358cb9e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -804,11 +804,11 @@ static int kfd_mem_attach(struct amdgpu_device
*adev, struct kgd_mem *mem,
va + bo_size, vm);
if ((adev == bo_adev && !(mem->alloc_flags &
KFD_IOC_ALLOC_MEM_FLAGS_MMIO_REMAP)) ||
- (amdgpu_ttm_tt_get_usermm(mem->bo->tbo.ttm) &&
adev->ram_is_direct_mapped) ||
- same_hive) {
+ (amdgpu_ttm_tt_get_usermm(mem->bo->tbo.ttm) &&
((adev->ram_is_direct_mapped && bo_adev->ram_is_direct_mapped) ||
+ adev->dev->iommu_group == bo_adev->dev-
iommu_group)) ||
+same_hive){
/* Mappings on the local GPU, or VRAM mappings in
the
-* local hive, or userptr mapping IOMMU direct map
mode
-* share the original BO
+* local hive, or userptr mapping in the same dma
+* address space share the original BO
*/
attachment[i]->type = KFD_MEM_ATT_SHARED;
bo[i] = mem->bo;
RE: [PATCH] drm/amdkfd: On GFX11 check PCIe atomics support and set CP_HQD_HQ_STATUS0[29]
[AMD Official Use Only - General]
Comments inline
> -Original Message-
> From: amd-gfx On Behalf Of
> Sreekant Somasekharan
> Sent: Monday, April 3, 2023 3:59 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Somasekharan, Sreekant
> Subject: [PATCH] drm/amdkfd: On GFX11 check PCIe atomics support and set
> CP_HQD_HQ_STATUS0[29]
>
> On GFX11, CP_HQD_HQ_STATUS0[29] bit will be used by CPFW to acknowledge
> whether PCIe atomics are supported. The default value of this bit is set
> to 0. Driver will check whether PCIe atomics are supported and set the
> bit to 1 if supported. This will force CPFW to use real atomic ops.
> If the bit is not set, CPFW will default to read/modify/write using the
> firmware itself.
>
> This is applicable only to RS64 based GFX11 with MEC FW greater than or
> equal to 509. If MEC FW is less than 509, PCIe atomics needs to be
> supported, else it will skip the device.
>
> This commit also involves moving amdgpu_amdkfd_device_probe() function
> call after per-IP early_init loop in amdgpu_device_ip_early_init()
> function so as to check for RS64 enabled device.
>
> Signed-off-by: Sreekant Somasekharan
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
> drivers/gpu/drm/amd/amdkfd/kfd_device.c | 11 +++
> drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v11.c | 9 +
> 3 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 7116119ed038..b3a754ca0923 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2150,7 +2150,6 @@ static int amdgpu_device_ip_early_init(struct
> amdgpu_device *adev)
> adev->has_pr3 = parent ? pci_pr3_present(parent) : false;
> }
>
> - amdgpu_amdkfd_device_probe(adev);
>
> adev->pm.pp_feature = amdgpu_pp_feature_mask;
> if (amdgpu_sriov_vf(adev) || sched_policy ==
> KFD_SCHED_POLICY_NO_HWS)
> @@ -2206,6 +2205,7 @@ static int amdgpu_device_ip_early_init(struct
> amdgpu_device *adev)
> if (!total)
> return -ENODEV;
>
> + amdgpu_amdkfd_device_probe(adev);
> adev->cg_flags &= amdgpu_cg_mask;
> adev->pg_flags &= amdgpu_pg_mask;
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> index 521dfa88aad8..64a295a35d37 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> @@ -204,6 +204,17 @@ static void kfd_device_info_init(struct kfd_dev *kfd,
> /* Navi1x+ */
> if (gc_version >= IP_VERSION(10, 1, 1))
> kfd->device_info.needs_pci_atomics = true;
> + } else if (gc_version < IP_VERSION(12, 0, 0)) {
What if we get a GFX9 with MEC v509? Wouldn't that trigger this too? Wondering
if this should be
if (gc_version>=IP_VERSION(11,0,0) && gc_version < IP_VERSION(12,0,0))
thus ensuring it's only GFX11. Or maybe there is some better check than that.
Either way, checking that it's < GFX11 might false-positive on GFX10- too, so
we should probably be explicit in our GFX check that it's only GFX11.
Kent
> + /* On GFX11 running on RS64, MEC FW version must be
> greater than
> + * or equal to version 509 to support acknowledging
> whether
> + * PCIe atomics are supported. Before MEC version 509,
> PCIe
> + * atomics are required. After that, the FW's use of
> atomics
> + * is controlled by CP_HQD_HQ_STATUS0[29].
> + * This will fail on GFX11 when PCIe atomics are not
> supported
> + * and MEC FW version < 509 for RS64 based CPFW.
> + */
> + kfd->device_info.needs_pci_atomics = true;
> + kfd->device_info.no_atomic_fw_version = kfd->adev-
> >gfx.rs64_enable ? 509 : 0;
> }
> } else {
> kfd->device_info.doorbell_size = 4;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v11.c
> b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v11.c
> index 4a9af800b1f1..c5ea594abbf6 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v11.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v11.c
> @@ -143,6 +143,15 @@ static void init_mqd(struct mqd_manager *mm, void
> **mqd,
> 1 << CP_HQD_QUANTUM__QUANTUM_SCALE__SHIFT
> |
> 1 <<
> CP_HQD_QUANTUM__QUANTUM_DURATION__SHIFT;
>
> + /*
> + * If PCIe atomics are supported, set CP_HQD_HQ_STATUS0[29] == 1
> + * to force CPFW to use atomics. This is supported only on MEC FW
> + * version >= 509 and on RS64 based CPFW only. On previous versions,
> + * platforms running on GFX11 must support atomics else will skip the
> device.
> + */
> + if (amdgpu_amdkfd_have_atomics_s
Re: [PATCH 1/3] drm/amdgpu: Add userptr bo support for mGPUs when iommu is on
[+Philip]
Am 2023-04-04 um 08:47 schrieb Christian König:
Am 04.04.23 um 12:56 schrieb Xiao, Shane:
[AMD Official Use Only - General]
-Original Message-
From: Koenig, Christian
Sent: Tuesday, April 4, 2023 6:27 PM
To: Xiao, Shane ; amd-gfx@lists.freedesktop.org;
Kuehling, Felix
Cc: Liu, Aaron ; Guo, Shikai
Subject: Re: [PATCH 1/3] drm/amdgpu: Add userptr bo support for mGPUs
when iommu is on
Am 04.04.23 um 11:56 schrieb Shane Xiao:
For userptr bo with iommu on, multiple GPUs use same system memory dma
mapping address when both bo_adev and adev in identity mode or in the
same iommu group.
Hi Christian,
WTF? Userptr BOs are not allowed to be exported/imported between
different
GPUs.
So how can the same userptr BO be used on different GPUs?
If GPUs are all in iommu identity mode which means dma address are
the same as physical address, all of the GPUs can see the system
memory directly.
In such case, should we export/import the BO, then create a new SG
BO for another GPU?
Yes, absolutely. Each userptr BO is only meant to be used on one GPU.
Even if you could use the same BO for multiple GPUs it's not necessary
a good idea since you then have live time problems for example.
E.g. what happens when one GPU is hot removed while the other one who
imported the BO is still in use?
Felix can you comment on that? My recollection was that we rather
improve the storage of DMA addresses instead of duplicating the BOs
over different GPUs.
For KFD we currently enable sharing of userptr BOs between multiple GPUs
by creating a userptr BO for the first GPU, and creating additional SG
BOs using the same page list for additional GPUs. That way we don't have
to call hmm_range_fault N times and setup N MMU notifiers for the same
address range on an N GPU system. But we do have to create N DMA
mappings, which is facilitated by the SG BOs.
We have a further optimization to not even store separate DMA addresses
per-GPU if they are a direct mapping. In that case we just increase the
reference count on the original userptr BO. (I agree that we should also
look into more efficient storage of DMA addresses. However, last time we
talked about this, you basically told us that scatter gather tables are
being deprecated, but I haven't seen the alternative yet.)
I think this patch fixes a potential issue with that optimization. There
is an implicit assumption, that the DMA addresses stored in the original
userptr BO are a direct mapping, so we can reuse them on other GPUs that
also use a direct mapping. But, we didn't actually check that
assumption. I think this patch fixes that for systems where system
memory is direct mapped on some but not all GPUs.
This scenario should probably be called out explicitly in the patch
description. The condition is also getting pretty hard to read and
understand. Maybe move the both-direct-map-or-same-iommu-group
conditions into a helper function, say
"amdgpu_ttm_tt_get_usermm(mem->bo->tbo.ttm) && reuse_dmamap(adev, bo_adev)".
Regards,
Felix
Regards,
Christian.
Best Regards,
Shane
Regards,
Christian.
Signed-off-by: Shane Xiao
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index e7403f8e4eba..33cda358cb9e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -804,11 +804,11 @@ static int kfd_mem_attach(struct amdgpu_device
*adev, struct kgd_mem *mem,
va + bo_size, vm);
if ((adev == bo_adev && !(mem->alloc_flags &
KFD_IOC_ALLOC_MEM_FLAGS_MMIO_REMAP)) ||
- (amdgpu_ttm_tt_get_usermm(mem->bo->tbo.ttm) &&
adev->ram_is_direct_mapped) ||
- same_hive) {
+ (amdgpu_ttm_tt_get_usermm(mem->bo->tbo.ttm) &&
((adev->ram_is_direct_mapped && bo_adev->ram_is_direct_mapped) ||
+ adev->dev->iommu_group == bo_adev->dev-
iommu_group)) ||
+same_hive){
/* Mappings on the local GPU, or VRAM mappings in
the
- * local hive, or userptr mapping IOMMU direct map
mode
- * share the original BO
+ * local hive, or userptr mapping in the same dma
+ * address space share the original BO
*/
attachment[i]->type = KFD_MEM_ATT_SHARED;
bo[i] = mem->bo;
Re: [PATCH 2/3] amd/amdgpu: Inherit coherence flags base on original BO flags
Am 2023-04-04 um 05:56 schrieb Shane Xiao:
For SG BO to DMA-map userptrs on other GPUs, the SG BO need inherit
MTYPEs in PTEs from original BO.
Good catch. See two comments inline.
If we set the flags, the device can be coherent with the CPUs and other GPUs.
Signed-off-by: Shane Xiao
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 33cda358cb9e..bcb0a7b32703 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -253,14 +253,22 @@ create_dmamap_sg_bo(struct amdgpu_device *adev,
{
struct drm_gem_object *gem_obj;
int ret, align;
+ uint64_t flags = 0;
ret = amdgpu_bo_reserve(mem->bo, false);
if (ret)
return ret;
align = 1;
+ if(mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_USERPTR)
+ {
+ flags |= mem->bo->flags &(AMDGPU_GEM_CREATE_CPU_GTT_USWC |
I think userptrs never use USWC because the pages are not allocated by
the driver. You can drop this flag.
+ AMDGPU_GEM_CREATE_COHERENT |
AMDGPU_GEM_CREATE_UNCACHED);
+ align = PAGE_SIZE;
Isn't a page alignment implicit anyway? I don't see why we need to use a
different alignment for userptrs. If PAGE_SIZE is needed for this case,
we can use the same for all cases We don't even need a local variable
for this.
Regards,
Felix
+ }
+
ret = amdgpu_gem_object_create(adev, mem->bo->tbo.base.size, align,
- AMDGPU_GEM_DOMAIN_CPU, AMDGPU_GEM_CREATE_PREEMPTIBLE,
+ AMDGPU_GEM_DOMAIN_CPU, AMDGPU_GEM_CREATE_PREEMPTIBLE |
flags,
ttm_bo_type_sg, mem->bo->tbo.base.resv, &gem_obj);
amdgpu_bo_unreserve(mem->bo);
Re: [PATCH 3/3] drm/amdgpu: DROP redundant drm_prime_sg_to_dma_addr_array
Am 2023-04-04 um 05:56 schrieb Shane Xiao: For DMA-MAP userptr on other GPUs, the dma address array has been populated in amdgpu_ttm_backend_bind. OK. I think "has been populated" should be "will be populated", because amdgpu_ttm_backend_bind happens as a callback from the ttm_bo_validate call below. With that fixed in the patch description, the patch is Reviewed-by: Felix Kuehling Remove the redundant call from the driver. Signed-off-by: Shane Xiao --- drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index bcb0a7b32703..94ee8f638c12 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -488,9 +488,6 @@ kfd_mem_dmamap_userptr(struct kgd_mem *mem, if (unlikely(ret)) goto release_sg; - drm_prime_sg_to_dma_addr_array(ttm->sg, ttm->dma_address, - ttm->num_pages); - amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT); ret = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); if (ret)
Re: [PATCH] drm/amdkfd: Fix dmabuf's redundant eviction when unmapping
If we keep the BO in the GTT domain, it means it will not be updated if
we validate it again later in kfd_mem_dmamap_dmabuf. This means we'll
use stale DMA addresses when we update the page tables after evictions.
I think we'll need to find a different way to avoid triggering the
eviction fence on the original BO when changing the placement of the
DMABuf import here. If you need help brainstorming here, please share a
backtrace from the eviction generated with the debug_evictions module param.
Regards,
Felix
Am 2023-04-03 um 13:59 schrieb Eric Huang:
dmabuf is allocated/mapped as GTT domain, when dma-unmapping dmabuf
changing placement to CPU will trigger memory eviction after calling
ttm_bo_validate, and the eviction will cause performance drop.
Keeping the correct domain will solve the issue.
Signed-off-by: Eric Huang
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index a3b09edfd1bf..17b708acb447 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -642,7 +642,7 @@ kfd_mem_dmaunmap_dmabuf(struct kfd_mem_attachment
*attachment)
struct ttm_operation_ctx ctx = {.interruptible = true};
struct amdgpu_bo *bo = attachment->bo_va->base.bo;
- amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_CPU);
+ amdgpu_bo_placement_from_domain(bo, AMDGPU_GEM_DOMAIN_GTT);
ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
}
Re: [PATCH] drm/amdkfd: Fix dmabuf's redundant eviction when unmapping
Here is the backtrace from Jira: Thu Nov 10 13:10:23 2022] Scheduling eviction of pid 97784 in 0 jiffies [Thu Nov 10 13:10:23 2022] WARNING: CPU: 173 PID: 97784 at /var/lib/dkms/amdgpu/5.16.9.22.20-1438746~20.04/build/amd/amdgpu/../amdkfd/kfd_device.c:878 kgd2kfd_schedule_evict_and_restore_process+0x104/0x120 [amdgpu] [Thu Nov 10 13:10:23 2022] Modules linked in: veth amdgpu(OE) amddrm_ttm_helper(OE) amdttm(OE) iommu_v2 amd_sched(OE) amdkcl(OE) xt_conntrack xt_MASQUERADE nf_conntrack_netlink nfnetlink xfrm_user xfrm_algo xt_addrtype iptable_filter iptable_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 bpfilter br_netfilter bridge stp llc aufs overlay binfmt_misc nls_iso8859_1 dm_multipath scsi_dh_rdac scsi_dh_emc scsi_dh_alua intel_rapl_msr intel_rapl_common amd64_edac edac_mce_amd kvm_amd kvm efi_pstore rapl ipmi_ssif ccp acpi_ipmi k10temp ipmi_si ipmi_devintf ipmi_msghandler mac_hid sch_fq_codel msr ip_tables x_tables autofs4 btrfs blake2b_generic zstd_compress raid10 raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx xor raid6_pq libcrc32c raid1 raid0 multipath linear mlx5_ib ib_uverbs ib_core crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel crypto_simd cryptd ast drm_vram_helper drm_ttm_helper ttm mlx5_core drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops [Thu Nov 10 13:10:23 2022] pci_hyperv_intf cec psample igb mlxfw rc_core dca ahci xhci_pci tls drm i2c_algo_bit libahci xhci_pci_renesas i2c_piix4 [Thu Nov 10 13:10:23 2022] CPU: 173 PID: 97784 Comm: onnxruntime_tes Tainted: G W OE 5.13.0-30-generic #33~20.04.1-Ubuntu [Thu Nov 10 13:10:23 2022] Hardware name: GIGABYTE G482-Z53-YF/MZ52-G40-00, BIOS R12 05/13/2020 [Thu Nov 10 13:10:23 2022] RIP: 0010:kgd2kfd_schedule_evict_and_restore_process+0x104/0x120 [amdgpu] [Thu Nov 10 13:10:23 2022] Code: 5e 5d c3 4c 89 e7 e8 cb c6 44 df eb e7 49 8b 45 60 48 89 ca 48 c7 c7 38 8b d7 c1 48 89 4d e0 8b b0 20 09 00 00 e8 87 ee 7e df <0f> 0b 48 8b 4d e0 eb 9f 41 be ea ff ff ff eb ba 41 be ed ff ff ff [Thu Nov 10 13:10:23 2022] RSP: 0018:b25f2a173978 EFLAGS: 00010086 [Thu Nov 10 13:10:23 2022] RAX: RBX: 0001 RCX: 0027 [Thu Nov 10 13:10:23 2022] RDX: 0027 RSI: fffe RDI: 95d06e4a09c8 [Thu Nov 10 13:10:23 2022] RBP: b25f2a173998 R08: 95d06e4a09c0 R09: b25f2a173750 [Thu Nov 10 13:10:23 2022] R10: 0001 R11: 0001 R12: 95c371d74580 [Thu Nov 10 13:10:23 2022] R13: 95b1cd3f2000 R14: R15: 95c371d74580 [Thu Nov 10 13:10:23 2022] FS: 7fcaff268b00() GS:95d06e48() knlGS: [Thu Nov 10 13:10:23 2022] CS: 0010 DS: ES: CR0: 80050033 [Thu Nov 10 13:10:23 2022] CR2: 7fc64398 CR3: 0003e9492000 CR4: 00350ee0 [Thu Nov 10 13:10:23 2022] Call Trace: [Thu Nov 10 13:10:23 2022] [Thu Nov 10 13:10:23 2022] amdkfd_fence_enable_signaling+0x46/0x50 [amdgpu] [Thu Nov 10 13:10:23 2022] __dma_fence_enable_signaling+0x52/0xb0 [Thu Nov 10 13:10:23 2022] dma_fence_default_wait+0xa9/0x200 [Thu Nov 10 13:10:23 2022] dma_fence_wait_timeout+0xbd/0xe0 [Thu Nov 10 13:10:23 2022] amddma_resv_wait_timeout+0x6f/0xd0 [amdkcl] [Thu Nov 10 13:10:23 2022] amdttm_bo_wait+0x39/0x50 [amdttm] [Thu Nov 10 13:10:23 2022] amdgpu_bo_move+0x41e/0x7b0 [amdgpu] [Thu Nov 10 13:10:23 2022] ? down_write+0x13/0x50 [Thu Nov 10 13:10:23 2022] ? unmap_mapping_pages+0x68/0x130 [Thu Nov 10 13:10:23 2022] ttm_bo_handle_move_mem+0x7f/0x120 [amdttm] [Thu Nov 10 13:10:23 2022] amdttm_bo_validate+0xbf/0x100 [amdttm] [Thu Nov 10 13:10:23 2022] kfd_mem_dmaunmap_attachment+0x131/0x140 [amdgpu] [Thu Nov 10 13:10:23 2022] unmap_bo_from_gpuvm+0x67/0x80 [amdgpu] [Thu Nov 10 13:10:23 2022] amdgpu_amdkfd_gpuvm_unmap_memory_from_gpu+0x114/0x220 [amdgpu] [Thu Nov 10 13:10:23 2022] ? __mod_memcg_lruvec_state+0x22/0xe0 [Thu Nov 10 13:10:23 2022] kfd_ioctl_unmap_memory_from_gpu+0xe8/0x270 [amdgpu] [Thu Nov 10 13:10:23 2022] kfd_ioctl+0x23c/0x590 [amdgpu] [Thu Nov 10 13:10:23 2022] ? kfd_ioctl_get_process_apertures_new+0x330/0x330 [amdgpu] [Thu Nov 10 13:10:23 2022] ? exit_to_user_mode_prepare+0x3d/0x1c0 [Thu Nov 10 13:10:23 2022] ? __fget_files+0xa7/0xd0 [Thu Nov 10 13:10:23 2022] __x64_sys_ioctl+0x91/0xc0 [Thu Nov 10 13:10:23 2022] do_syscall_64+0x61/0xb0 [Thu Nov 10 13:10:23 2022] ? do_syscall_64+0x6e/0xb0 [Thu Nov 10 13:10:23 2022] ? do_syscall_64+0x6e/0xb0 [Thu Nov 10 13:10:23 2022] ? do_syscall_64+0x6e/0xb0 [Thu Nov 10 13:10:23 2022] ? do_syscall_64+0x6e/0xb0 [Thu Nov 10 13:10:23 2022] ? asm_sysvec_apic_timer_interrupt+0xa/0x20 [Thu Nov 10 13:10:23 2022] entry_SYSCALL_64_after_hwframe+0x44/0xae [Thu Nov 10 13:10:23 2022] RIP: 0033:0x7fcaff57b3ab [Thu Nov 10 13:10:23 2022] Code: 0f 1e fa 48 8b 05 e5 7a 0d 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa b8 10 00 00 00 0f 05 <48> 3d 0
RE: [PATCH 1/3] drm/amdgpu: Add userptr bo support for mGPUs when iommu is on
[AMD Official Use Only - General]
> -Original Message-
> From: Kuehling, Felix
> Sent: Tuesday, April 4, 2023 9:45 PM
> To: Christian König ; Xiao, Shane
> ; Koenig, Christian ;
> amd-gfx@lists.freedesktop.org; Yang, Philip
> Cc: Liu, Aaron ; Guo, Shikai
> Subject: Re: [PATCH 1/3] drm/amdgpu: Add userptr bo support for mGPUs
> when iommu is on
>
> [+Philip]
>
> Am 2023-04-04 um 08:47 schrieb Christian König:
>
> > Am 04.04.23 um 12:56 schrieb Xiao, Shane:
> >> [AMD Official Use Only - General]
> >>
> >>> -Original Message-
> >>> From: Koenig, Christian
> >>> Sent: Tuesday, April 4, 2023 6:27 PM
> >>> To: Xiao, Shane ; amd-gfx@lists.freedesktop.org;
> >>> Kuehling, Felix
> >>> Cc: Liu, Aaron ; Guo, Shikai
> >>> Subject: Re: [PATCH 1/3] drm/amdgpu: Add userptr bo support for
> >>> mGPUs when iommu is on
> >>>
> >>> Am 04.04.23 um 11:56 schrieb Shane Xiao:
> For userptr bo with iommu on, multiple GPUs use same system memory
> dma mapping address when both bo_adev and adev in identity mode or
> in the same iommu group.
> >> Hi Christian,
> >>
> >>> WTF? Userptr BOs are not allowed to be exported/imported between
> >>> different GPUs.
> >>>
> >>> So how can the same userptr BO be used on different GPUs?
> >> If GPUs are all in iommu identity mode which means dma address are
> >> the same as physical address, all of the GPUs can see the system
> >> memory directly.
> >>
> >> In such case, should we export/import the BO, then create a new SG
> >> BO for another GPU?
> >
> > Yes, absolutely. Each userptr BO is only meant to be used on one GPU.
> >
> > Even if you could use the same BO for multiple GPUs it's not necessary
> > a good idea since you then have live time problems for example.
> >
> > E.g. what happens when one GPU is hot removed while the other one who
> > imported the BO is still in use?
> >
> > Felix can you comment on that? My recollection was that we rather
> > improve the storage of DMA addresses instead of duplicating the BOs
> > over different GPUs.
>
> For KFD we currently enable sharing of userptr BOs between multiple GPUs by
> creating a userptr BO for the first GPU, and creating additional SG BOs using
> the same page list for additional GPUs. That way we don't have to call
> hmm_range_fault N times and setup N MMU notifiers for the same address
> range on an N GPU system. But we do have to create N DMA mappings, which
> is facilitated by the SG BOs.
>
> We have a further optimization to not even store separate DMA addresses per-
> GPU if they are a direct mapping. In that case we just increase the reference
> count on the original userptr BO. (I agree that we should also look into more
> efficient storage of DMA addresses. However, last time we talked about this,
> you basically told us that scatter gather tables are being deprecated, but I
> haven't seen the alternative yet.)
>
> I think this patch fixes a potential issue with that optimization. There is an
> implicit assumption, that the DMA addresses stored in the original userptr BO
> are a direct mapping, so we can reuse them on other GPUs that also use a
> direct mapping. But, we didn't actually check that assumption. I think this
> patch
> fixes that for systems where system memory is direct mapped on some but not
> all GPUs.
>
> This scenario should probably be called out explicitly in the patch
> description.
Yes, I will add this scenario on the comment.
> The condition is also getting pretty hard to read and understand. Maybe move
> the both-direct-map-or-same-iommu-group
> conditions into a helper function, say
> "amdgpu_ttm_tt_get_usermm(mem->bo->tbo.ttm) && reuse_dmamap(adev,
> bo_adev)".
It's a good suggestion. I will make the change.
Best Regards,
Shane
>
> Regards,
> Felix
>
>
> >
> > Regards,
> > Christian.
> >
> >>
> >>
> >> Best Regards,
> >> Shane
> >>
> >>> Regards,
> >>> Christian.
> >>>
> Signed-off-by: Shane Xiao
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index e7403f8e4eba..33cda358cb9e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -804,11 +804,11 @@ static int kfd_mem_attach(struct
> amdgpu_device
> >>> *adev, struct kgd_mem *mem,
> va + bo_size, vm);
>
> if ((adev == bo_adev && !(mem->alloc_flags &
> >>> KFD_IOC_ALLOC_MEM_FLAGS_MMIO_REMAP)) ||
> - (amdgpu_ttm_tt_get_usermm(mem->bo->tbo.ttm) &&
> >>> adev->ram_is_direct_mapped) ||
> - same_hive) {
> + (amdgpu_ttm_tt_get_usermm(mem->bo->tbo.ttm) &&
> >>> ((adev->ram_is_direct_mapped && bo_adev->ram_is_direct_mapped) ||
> + adev->dev->iommu_group == bo_adev->dev-
> iommu_group)) ||
> +s
RE: [PATCH 2/3] amd/amdgpu: Inherit coherence flags base on original BO flags
[AMD Official Use Only - General]
> -Original Message-
> From: Kuehling, Felix
> Sent: Tuesday, April 4, 2023 9:52 PM
> To: Xiao, Shane ; amd-gfx@lists.freedesktop.org;
> Koenig, Christian
> Cc: Liu, Aaron ; Guo, Shikai
> Subject: Re: [PATCH 2/3] amd/amdgpu: Inherit coherence flags base on original
> BO flags
>
> Am 2023-04-04 um 05:56 schrieb Shane Xiao:
> > For SG BO to DMA-map userptrs on other GPUs, the SG BO need inherit
> > MTYPEs in PTEs from original BO.
>
> Good catch. See two comments inline.
>
>
> >
> > If we set the flags, the device can be coherent with the CPUs and other
> > GPUs.
> >
> > Signed-off-by: Shane Xiao
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 10 +-
> > 1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > index 33cda358cb9e..bcb0a7b32703 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> > @@ -253,14 +253,22 @@ create_dmamap_sg_bo(struct amdgpu_device
> *adev,
> > {
> > struct drm_gem_object *gem_obj;
> > int ret, align;
> > + uint64_t flags = 0;
> >
> > ret = amdgpu_bo_reserve(mem->bo, false);
> > if (ret)
> > return ret;
> >
> > align = 1;
> > + if(mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_USERPTR)
> > + {
> > + flags |= mem->bo->flags
> &(AMDGPU_GEM_CREATE_CPU_GTT_USWC |
>
> I think userptrs never use USWC because the pages are not allocated by the
> driver. You can drop this flag.
OK. I will do it.
>
>
> > + AMDGPU_GEM_CREATE_COHERENT |
> AMDGPU_GEM_CREATE_UNCACHED);
> > + align = PAGE_SIZE;
>
> Isn't a page alignment implicit anyway? I don't see why we need to use a
> different alignment for userptrs. If PAGE_SIZE is needed for this case,
> we can use the same for all cases We don't even need a local variable
> for this.
Yes, a page alignment is implicit, and the local variable will be removed.
Best Regards,
Shane
>
> Regards,
> Felix
>
>
> > + }
> > +
> > ret = amdgpu_gem_object_create(adev, mem->bo->tbo.base.size, align,
> > - AMDGPU_GEM_DOMAIN_CPU,
> AMDGPU_GEM_CREATE_PREEMPTIBLE,
> > + AMDGPU_GEM_DOMAIN_CPU,
> AMDGPU_GEM_CREATE_PREEMPTIBLE | flags,
> > ttm_bo_type_sg, mem->bo->tbo.base.resv, &gem_obj);
> >
> > amdgpu_bo_unreserve(mem->bo);
RE: [PATCH 3/3] drm/amdgpu: DROP redundant drm_prime_sg_to_dma_addr_array
[AMD Official Use Only - General] > -Original Message- > From: Kuehling, Felix > Sent: Tuesday, April 4, 2023 9:59 PM > To: Xiao, Shane ; amd-gfx@lists.freedesktop.org; > Koenig, Christian > Cc: Liu, Aaron ; Guo, Shikai > Subject: Re: [PATCH 3/3] drm/amdgpu: DROP redundant > drm_prime_sg_to_dma_addr_array > > Am 2023-04-04 um 05:56 schrieb Shane Xiao: > > For DMA-MAP userptr on other GPUs, the dma address array has been > > populated in amdgpu_ttm_backend_bind. > > OK. I think "has been populated" should be "will be populated", because > amdgpu_ttm_backend_bind happens as a callback from the ttm_bo_validate > call below. With that fixed in the patch description, the patch is Yes, I will fix the comment. Best Regards, Shane > > Reviewed-by: Felix Kuehling > > > > > > Remove the redundant call from the driver. > > > > Signed-off-by: Shane Xiao > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c > > index bcb0a7b32703..94ee8f638c12 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c > > @@ -488,9 +488,6 @@ kfd_mem_dmamap_userptr(struct kgd_mem *mem, > > if (unlikely(ret)) > > goto release_sg; > > > > - drm_prime_sg_to_dma_addr_array(ttm->sg, ttm->dma_address, > > - ttm->num_pages); > > - > > amdgpu_bo_placement_from_domain(bo, > AMDGPU_GEM_DOMAIN_GTT); > > ret = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx); > > if (ret)
RE: [PATCH] drm/amdkfd: On GFX11 check PCIe atomics support and set CP_HQD_HQ_STATUS0[29]
[Public]
> -Original Message-
> From: amd-gfx On Behalf Of
> Russell, Kent
> Sent: Tuesday, April 4, 2023 9:43 AM
> To: Somasekharan, Sreekant ; amd-
> g...@lists.freedesktop.org
> Cc: Somasekharan, Sreekant
> Subject: RE: [PATCH] drm/amdkfd: On GFX11 check PCIe atomics support and
> set CP_HQD_HQ_STATUS0[29]
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> [AMD Official Use Only - General]
>
> Comments inline
>
> > -Original Message-
> > From: amd-gfx On Behalf Of
> > Sreekant Somasekharan
> > Sent: Monday, April 3, 2023 3:59 PM
> > To: amd-gfx@lists.freedesktop.org
> > Cc: Somasekharan, Sreekant
> > Subject: [PATCH] drm/amdkfd: On GFX11 check PCIe atomics support and
> > set CP_HQD_HQ_STATUS0[29]
> >
> > On GFX11, CP_HQD_HQ_STATUS0[29] bit will be used by CPFW to
> > acknowledge whether PCIe atomics are supported. The default value of
> > this bit is set to 0. Driver will check whether PCIe atomics are
> > supported and set the bit to 1 if supported. This will force CPFW to use
> > real
> atomic ops.
> > If the bit is not set, CPFW will default to read/modify/write using
> > the firmware itself.
> >
> > This is applicable only to RS64 based GFX11 with MEC FW greater than
> > or equal to 509. If MEC FW is less than 509, PCIe atomics needs to be
> > supported, else it will skip the device.
> >
> > This commit also involves moving amdgpu_amdkfd_device_probe()
> function
> > call after per-IP early_init loop in amdgpu_device_ip_early_init()
> > function so as to check for RS64 enabled device.
> >
> > Signed-off-by: Sreekant Somasekharan
>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
> > drivers/gpu/drm/amd/amdkfd/kfd_device.c | 11 +++
> > drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v11.c | 9 +
> > 3 files changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index 7116119ed038..b3a754ca0923 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -2150,7 +2150,6 @@ static int amdgpu_device_ip_early_init(struct
> > amdgpu_device *adev)
> > adev->has_pr3 = parent ? pci_pr3_present(parent) : false;
> > }
> >
> > - amdgpu_amdkfd_device_probe(adev);
> >
> > adev->pm.pp_feature = amdgpu_pp_feature_mask;
> > if (amdgpu_sriov_vf(adev) || sched_policy ==
> > KFD_SCHED_POLICY_NO_HWS)
> > @@ -2206,6 +2205,7 @@ static int amdgpu_device_ip_early_init(struct
> > amdgpu_device *adev)
> > if (!total)
> > return -ENODEV;
> >
> > + amdgpu_amdkfd_device_probe(adev);
> > adev->cg_flags &= amdgpu_cg_mask;
> > adev->pg_flags &= amdgpu_pg_mask;
> >
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> > b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> > index 521dfa88aad8..64a295a35d37 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> > @@ -204,6 +204,17 @@ static void kfd_device_info_init(struct kfd_dev
> *kfd,
> > /* Navi1x+ */
> > if (gc_version >= IP_VERSION(10, 1, 1))
> > kfd->device_info.needs_pci_atomics =
> > true;
> > + } else if (gc_version < IP_VERSION(12, 0, 0)) {
>
>
> What if we get a GFX9 with MEC v509? Wouldn't that trigger this too?
> Wondering if this should be if (gc_version>=IP_VERSION(11,0,0) &&
> gc_version < IP_VERSION(12,0,0)) thus ensuring it's only GFX11. Or maybe
> there is some better check than that. Either way, checking that it's < GFX11
> might false-positive on GFX10- too, so we should probably be explicit in our
> GFX check that it's only GFX11.
The previous condition is for gc_version < IP_VERSION(11, 0, 0), so that
condition will (and currently is) taken for gfx9/gfx10/etc.
That's to say the logic after this change will look like:
If (KFD_IS_SOC15(kfd)) {
<...>
If (gc_version < IP_VERSION(11, 0, 0)) {
<...>
} else if (gc_version < IP_VERSION(12, 0, 0)) {
<...>
}
}
So this new path will only be taken for gfx11.
Best,
Graham
>
> Kent
>
> > + /* On GFX11 running on RS64, MEC FW version must
> > + be
> > greater than
> > + * or equal to version 509 to support
> > + acknowledging
> > whether
> > + * PCIe atomics are supported. Before MEC
> > + version 509,
> > PCIe
> > + * atomics are required. After that, the FW's
> > + use of
> > atomics
> > + * is controlled by CP_HQD_HQ_STATUS0[29].
> > + * This will fail on GFX11 when PCIe atomics are
> > + not
> > supported
> > + * and MEC FW version < 509 for RS64 based CPFW.
> > +
Re: [PATCH v3 2/9] drm/amdgpu: add usermode queue base code
On 2023-03-29 12:04, Shashank Sharma wrote:
> From: Shashank Sharma
>
> This patch adds skeleton code for amdgpu usermode queue. It contains:
> - A new files with init functions of usermode queues.
> - A queue context manager in driver private data.
>
> V1: Worked on design review comments from RFC patch series:
> (https://patchwork.freedesktop.org/series/112214/)
> - Alex: Keep a list of queues, instead of single queue per process.
> - Christian: Use the queue manager instead of global ptrs,
>Don't keep the queue structure in amdgpu_ctx
>
> V2:
> - Reformatted code, split the big patch into two
>
> V3:
> - Integration with doorbell manager
>
> Cc: Alex Deucher
> Cc: Christian Koenig
> Signed-off-by: Shashank Sharma
> ---
> drivers/gpu/drm/amd/amdgpu/Makefile | 2 +
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 10 +++-
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 1 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 6 +++
> drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 39 +++
> .../gpu/drm/amd/include/amdgpu_userqueue.h| 49 +++
> 6 files changed, 106 insertions(+), 1 deletion(-)
> create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> create mode 100644 drivers/gpu/drm/amd/include/amdgpu_userqueue.h
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile
> b/drivers/gpu/drm/amd/amdgpu/Makefile
> index 204665f20319..2d90ba618e5d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
> @@ -210,6 +210,8 @@ amdgpu-y += \
> # add amdkfd interfaces
> amdgpu-y += amdgpu_amdkfd.o
>
> +# add usermode queue
> +amdgpu-y += amdgpu_userqueue.o
>
> ifneq ($(CONFIG_HSA_AMD),)
> AMDKFD_PATH := ../amdkfd
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 6b74df446694..c5f9af0e74ee 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -438,6 +438,14 @@ struct amdgpu_sa_manager {
> uint32_talign;
> };
>
> +/* Gfx usermode queues */
> +struct amdgpu_userq_mgr {
> + struct idr userq_idr;
> + struct mutex userq_mutex;
> + struct amdgpu_device *adev;
> + const struct amdgpu_userq_funcs *userq_funcs[AMDGPU_HW_IP_NUM];
> +};
> +
Could you align the member names to the largest member's column,
as opposed to having only a single space in between?
It makes it so much more readable.
> /* sub-allocation buffer */
> struct amdgpu_sa_bo {
> struct list_headolist;
> @@ -470,7 +478,6 @@ struct amdgpu_flip_work {
> boolasync;
> };
>
> -
> /*
> * file private structure
> */
> @@ -482,6 +489,7 @@ struct amdgpu_fpriv {
> struct mutexbo_list_lock;
> struct idr bo_list_handles;
> struct amdgpu_ctx_mgr ctx_mgr;
> + struct amdgpu_userq_mgr userq_mgr;
> };
Like here, and pretty much the rest of the kernel code.
>
> int amdgpu_file_to_fpriv(struct file *filp, struct amdgpu_fpriv **fpriv);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index b4f2d61ea0d5..2d6bcfd727c8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -52,6 +52,7 @@
> #include "amdgpu_ras.h"
> #include "amdgpu_xgmi.h"
> #include "amdgpu_reset.h"
> +#include "amdgpu_userqueue.h"
>
> /*
> * KMS wrapper.
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 7aa7e52ca784..b16b8155a157 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -43,6 +43,7 @@
> #include "amdgpu_gem.h"
> #include "amdgpu_display.h"
> #include "amdgpu_ras.h"
> +#include "amdgpu_userqueue.h"
>
> void amdgpu_unregister_gpu_instance(struct amdgpu_device *adev)
> {
> @@ -1187,6 +1188,10 @@ int amdgpu_driver_open_kms(struct drm_device *dev,
> struct drm_file *file_priv)
>
> amdgpu_ctx_mgr_init(&fpriv->ctx_mgr, adev);
>
> + r = amdgpu_userq_mgr_init(&fpriv->userq_mgr, adev);
> + if (r)
> + DRM_WARN("Can't setup usermode queues, only legacy workload
> submission will work\n");
> +
> file_priv->driver_priv = fpriv;
> goto out_suspend;
>
> @@ -1254,6 +1259,7 @@ void amdgpu_driver_postclose_kms(struct drm_device *dev,
>
> amdgpu_ctx_mgr_fini(&fpriv->ctx_mgr);
> amdgpu_vm_fini(adev, &fpriv->vm);
> + amdgpu_userq_mgr_fini(&fpriv->userq_mgr);
>
> if (pasid)
> amdgpu_pasid_free_delayed(pd->tbo.base.resv, pasid);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> new file mode 100644
> index ..13e1eebc1cb6
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> @@ -0,0 +1,39 @@
> +/*
> + * Copyright 2022 Advanced Micro Devices
RE: [PATCH v2] drm/amdgpu: add print for iommu translation mode
[AMD Official Use Only - General]
Ping :)
Best,
Graham
> -Original Message-
> From: Kuehling, Felix
> Sent: Monday, March 20, 2023 12:16 PM
> To: Sider, Graham ; amd-
> g...@lists.freedesktop.org
> Subject: Re: [PATCH v2] drm/amdgpu: add print for iommu translation mode
>
> On 2023-03-20 10:08, Graham Sider wrote:
> > Add log to display whether RAM is direct vs DMA mapped.
> >
> > Signed-off-by: Graham Sider
>
> Acked-by: Felix Kuehling
>
>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 6 +-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index 3bd6c5aef796..83774824694b 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -3528,8 +3528,12 @@ static void
> amdgpu_device_check_iommu_direct_map(struct amdgpu_device *adev)
> > struct iommu_domain *domain;
> >
> > domain = iommu_get_domain_for_dev(adev->dev);
> > - if (!domain || domain->type == IOMMU_DOMAIN_IDENTITY)
> > + if (!domain || domain->type == IOMMU_DOMAIN_IDENTITY) {
> > + dev_info(adev->dev, "RAM is direct mapped to GPU (not
> translated by IOMMU)\n");
> > adev->ram_is_direct_mapped = true;
> > + } else {
> > + dev_info(adev->dev, "RAM is DMA mapped to GPU
> (translated by IOMMU)\n");
> > + }
> > }
> >
> > static const struct attribute *amdgpu_dev_attributes[] = {
Re: [PATCH 01/16] drm/amdgpu: rename num_doorbells
On 2023-03-29 11:47, Shashank Sharma wrote:
> From: Shashank Sharma
>
> Rename doorbell.num_doorbells to doorbell.num_kernel_doorbells to
> make it more readable.
>
> Cc: Alex Deucher
> Cc: Christian Koenig
> Signed-off-by: Shashank Sharma
> ---
Is there any reason you break up the Cc list between the Cc tags in the commit
message,
and the SMTP CC list?
Just do either/or, but it is preferable to add all Cc into the Cc tags of the
commit
message, and then let git-send-email fill in the SMTP CC list, and just using
MLs
in the --to= argument. (Although, one can include those too in the Cc list.)
Regards,
Luben
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 6 +++---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 22 ++--
> drivers/gpu/drm/amd/amdgpu/amdgpu_doorbell.h | 4 +++-
> 3 files changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> index f99d4873bf22..0385f7f69278 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> @@ -96,7 +96,7 @@ static void amdgpu_doorbell_get_kfd_info(struct
> amdgpu_device *adev,
>size_t *start_offset)
> {
> /*
> - * The first num_doorbells are used by amdgpu.
> + * The first num_kernel_doorbells are used by amdgpu.
>* amdkfd takes whatever's left in the aperture.
>*/
> if (adev->enable_mes) {
> @@ -109,11 +109,11 @@ static void amdgpu_doorbell_get_kfd_info(struct
> amdgpu_device *adev,
> *aperture_base = adev->doorbell.base;
> *aperture_size = 0;
> *start_offset = 0;
> - } else if (adev->doorbell.size > adev->doorbell.num_doorbells *
> + } else if (adev->doorbell.size > adev->doorbell.num_kernel_doorbells *
> sizeof(u32)) {
> *aperture_base = adev->doorbell.base;
> *aperture_size = adev->doorbell.size;
> - *start_offset = adev->doorbell.num_doorbells * sizeof(u32);
> + *start_offset = adev->doorbell.num_kernel_doorbells *
> sizeof(u32);
> } else {
> *aperture_base = 0;
> *aperture_size = 0;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index afe6af9c0138..57ee1c4a81e9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -593,7 +593,7 @@ u32 amdgpu_mm_rdoorbell(struct amdgpu_device *adev, u32
> index)
> if (amdgpu_device_skip_hw_access(adev))
> return 0;
>
> - if (index < adev->doorbell.num_doorbells) {
> + if (index < adev->doorbell.num_kernel_doorbells) {
> return readl(adev->doorbell.ptr + index);
> } else {
> DRM_ERROR("reading beyond doorbell aperture: 0x%08x!\n", index);
> @@ -616,7 +616,7 @@ void amdgpu_mm_wdoorbell(struct amdgpu_device *adev, u32
> index, u32 v)
> if (amdgpu_device_skip_hw_access(adev))
> return;
>
> - if (index < adev->doorbell.num_doorbells) {
> + if (index < adev->doorbell.num_kernel_doorbells) {
> writel(v, adev->doorbell.ptr + index);
> } else {
> DRM_ERROR("writing beyond doorbell aperture: 0x%08x!\n", index);
> @@ -637,7 +637,7 @@ u64 amdgpu_mm_rdoorbell64(struct amdgpu_device *adev, u32
> index)
> if (amdgpu_device_skip_hw_access(adev))
> return 0;
>
> - if (index < adev->doorbell.num_doorbells) {
> + if (index < adev->doorbell.num_kernel_doorbells) {
> return atomic64_read((atomic64_t *)(adev->doorbell.ptr +
> index));
> } else {
> DRM_ERROR("reading beyond doorbell aperture: 0x%08x!\n", index);
> @@ -660,7 +660,7 @@ void amdgpu_mm_wdoorbell64(struct amdgpu_device *adev,
> u32 index, u64 v)
> if (amdgpu_device_skip_hw_access(adev))
> return;
>
> - if (index < adev->doorbell.num_doorbells) {
> + if (index < adev->doorbell.num_kernel_doorbells) {
> atomic64_set((atomic64_t *)(adev->doorbell.ptr + index), v);
> } else {
> DRM_ERROR("writing beyond doorbell aperture: 0x%08x!\n", index);
> @@ -1034,7 +1034,7 @@ static int amdgpu_device_doorbell_init(struct
> amdgpu_device *adev)
> if (adev->asic_type < CHIP_BONAIRE) {
> adev->doorbell.base = 0;
> adev->doorbell.size = 0;
> - adev->doorbell.num_doorbells = 0;
> + adev->doorbell.num_kernel_doorbells = 0;
> adev->doorbell.ptr = NULL;
> return 0;
> }
> @@ -1049,27 +1049,27 @@ static int amdgpu_device_doorbell_init(struct
> amdgpu_device *adev)
> adev->doorbell.size = pci_resource_len(adev->pdev, 2);
>
> if (adev->enable_mes) {
> - adev->doorbell.num_doorbells
Re: [PATCH v3 4/9] drm/amdgpu: create GFX-gen11 MQD for userqueue
On 2023-03-29 12:04, Shashank Sharma wrote:
> From: Shashank Sharma
>
> A Memory queue descriptor (MQD) of a userqueue defines it in the harware's
> context. As MQD format can vary between different graphics IPs, we need gfx
> GEN specific handlers to create MQDs.
>
> This patch:
> - Introduces MQD hander functions for the usermode queues.
> - Adds new functions to create and destroy MQD for GFX-GEN-11-IP
>
> V1: Worked on review comments from Alex:
> - Make MQD functions GEN and IP specific
>
> V2: Worked on review comments from Alex:
> - Reuse the existing adev->mqd[ip] for MQD creation
> - Formatting and arrangement of code
>
> V3:
> - Integration with doorbell manager
>
> Cc: Alex Deucher
> Cc: Christian Koenig
>
> Signed-off-by: Shashank Sharma
> Signed-off-by: Arvind Yadav
> ---
Don't break up the Cc tag list and the Sob tag list.
Check out the following resources:
https://www.kernel.org/doc/html/v4.12/process/5.Posting.html#patch-formatting-and-changelogs
https://www.kernel.org/doc/html/v4.12/process/submitting-patches.html#the-canonical-patch-format
> drivers/gpu/drm/amd/amdgpu/Makefile | 1 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 21 +
> .../drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c | 84 +++
> .../gpu/drm/amd/include/amdgpu_userqueue.h| 7 ++
> 4 files changed, 113 insertions(+)
> create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile
> b/drivers/gpu/drm/amd/amdgpu/Makefile
> index 2d90ba618e5d..2cc7897de7e6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/Makefile
> +++ b/drivers/gpu/drm/amd/amdgpu/Makefile
> @@ -212,6 +212,7 @@ amdgpu-y += amdgpu_amdkfd.o
>
> # add usermode queue
> amdgpu-y += amdgpu_userqueue.o
> +amdgpu-y += amdgpu_userqueue_gfx_v11.o
>
> ifneq ($(CONFIG_HSA_AMD),)
> AMDKFD_PATH := ../amdkfd
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> index 353f57c5a772..052c2c1e8aed 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> @@ -81,6 +81,12 @@ static int amdgpu_userqueue_create(struct drm_file *filp,
> union drm_amdgpu_userq
> goto free_queue;
> }
>
> +r = uq_mgr->userq_funcs[queue->queue_type]->mqd_create(uq_mgr, queue);
> +if (r) {
> +DRM_ERROR("Failed to create/map userqueue MQD\n");
> +goto free_queue;
> +}
> +
> args->out.queue_id = queue->queue_id;
> args->out.flags = 0;
> mutex_unlock(&uq_mgr->userq_mutex);
> @@ -105,6 +111,7 @@ static void amdgpu_userqueue_destroy(struct drm_file
> *filp, int queue_id)
> }
>
> mutex_lock(&uq_mgr->userq_mutex);
> +uq_mgr->userq_funcs[queue->queue_type]->mqd_destroy(uq_mgr, queue);
> amdgpu_userqueue_free_index(uq_mgr, queue->queue_id);
> mutex_unlock(&uq_mgr->userq_mutex);
> kfree(queue);
> @@ -135,6 +142,19 @@ int amdgpu_userq_ioctl(struct drm_device *dev, void
> *data,
> return r;
> }
>
> +extern const struct amdgpu_userq_funcs userq_gfx_v11_funcs;
> +
> +static void
> +amdgpu_userqueue_setup_ip_funcs(struct amdgpu_userq_mgr *uq_mgr)
> +{
> +int maj;
> +struct amdgpu_device *adev = uq_mgr->adev;
> +uint32_t version = adev->ip_versions[GC_HWIP][0];
> +
> +maj = IP_VERSION_MAJ(version);
> +if (maj == 11)
> +uq_mgr->userq_funcs[AMDGPU_HW_IP_GFX] = &userq_gfx_v11_funcs;
> +}
>
> int amdgpu_userq_mgr_init(struct amdgpu_userq_mgr *userq_mgr, struct
> amdgpu_device *adev)
> {
> @@ -142,6 +162,7 @@ int amdgpu_userq_mgr_init(struct amdgpu_userq_mgr
> *userq_mgr, struct amdgpu_devi
> idr_init_base(&userq_mgr->userq_idr, 1);
> userq_mgr->adev = adev;
>
> +amdgpu_userqueue_setup_ip_funcs(userq_mgr);
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
> new file mode 100644
> index ..12e1a785b65a
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
> @@ -0,0 +1,84 @@
> +/*
> + * Copyright 2022 Advanced Micro Devices, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHAN
Re: [PATCH v3 5/9] drm/amdgpu: create context space for usermode queue
On 2023-03-29 12:04, Shashank Sharma wrote:
> From: Shashank Sharma
>
> The FW expects us to allocate atleast one page as context space to
> process gang, process, shadow, GDS and FW related work. This patch
> creates a joint object for the same, and calculates GPU space offsets
> for each of these spaces.
>
> V1: Addressed review comments on RFC patch:
> Alex: Make this function IP specific
>
> V2: Addressed review comments from Christian
> - Allocate only one object for total FW space, and calculate
> offsets for each of these objects.
>
> V3: Integration with doorbell manager
>
> Cc: Alex Deucher
> Cc: Christian Koenig
> Signed-off-by: Shashank Sharma
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 1 +
> .../drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c | 60 ++-
> .../gpu/drm/amd/include/amdgpu_userqueue.h| 7 +++
> 3 files changed, 66 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> index 052c2c1e8aed..5672efcbcffc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> @@ -71,6 +71,7 @@ static int amdgpu_userqueue_create(struct drm_file *filp,
> union drm_amdgpu_userq
> queue->userq_prop.queue_size = mqd_in->queue_size;
>
> queue->doorbell_handle = mqd_in->doorbell_handle;
> +queue->shadow_ctx_gpu_addr = mqd_in->shadow_va;
> queue->queue_type = mqd_in->ip_type;
> queue->flags = mqd_in->flags;
> queue->vm = &fpriv->vm;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
> index 12e1a785b65a..52de96727f98 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
> @@ -23,6 +23,51 @@
> #include "amdgpu.h"
> #include "amdgpu_userqueue.h"
>
> +#define AMDGPU_USERQ_PROC_CTX_SZ PAGE_SIZE
> +#define AMDGPU_USERQ_GANG_CTX_SZ PAGE_SIZE
> +#define AMDGPU_USERQ_FW_CTX_SZ PAGE_SIZE
> +#define AMDGPU_USERQ_GDS_CTX_SZ PAGE_SIZE
> +
Leaving a single space after the macro name and its value
makes it hard to read. Please align the value columns
and leave at least 3 spaces--this would make it readable.
For instance,
#define AMDGPU_USERQ_PROC_CTX_SZ PAGE_SIZE
#define AMDGPU_USERQ_GANG_CTX_SZ PAGE_SIZE
#define AMDGPU_USERQ_FW_CTX_SZ PAGE_SIZE
#define AMDGPU_USERQ_GDS_CTX_SZPAGE_SIZE
Regards,
Luben
> +static int amdgpu_userq_gfx_v11_create_ctx_space(struct amdgpu_userq_mgr
> *uq_mgr,
> + struct
> amdgpu_usermode_queue *queue)
> +{
> +struct amdgpu_device *adev = uq_mgr->adev;
> +struct amdgpu_userq_ctx_space *ctx = &queue->fw_space;
> +int r, size;
> +
> +/*
> + * The FW expects atleast one page space allocated for
> + * process ctx, gang ctx, gds ctx, fw ctx and shadow ctx each.
> + */
> +size = AMDGPU_USERQ_PROC_CTX_SZ + AMDGPU_USERQ_GANG_CTX_SZ +
> + AMDGPU_USERQ_FW_CTX_SZ + AMDGPU_USERQ_GDS_CTX_SZ;
> +r = amdgpu_bo_create_kernel(adev, size, PAGE_SIZE,
> +AMDGPU_GEM_DOMAIN_GTT,
> +&ctx->obj,
> +&ctx->gpu_addr,
> +&ctx->cpu_ptr);
> +if (r) {
> +DRM_ERROR("Failed to allocate ctx space bo for userqueue, err:%d\n",
> r);
> +return r;
> +}
> +
> +queue->proc_ctx_gpu_addr = ctx->gpu_addr;
> +queue->gang_ctx_gpu_addr = queue->proc_ctx_gpu_addr +
> AMDGPU_USERQ_PROC_CTX_SZ;
> +queue->fw_ctx_gpu_addr = queue->gang_ctx_gpu_addr +
> AMDGPU_USERQ_GANG_CTX_SZ;
> +queue->gds_ctx_gpu_addr = queue->fw_ctx_gpu_addr +
> AMDGPU_USERQ_FW_CTX_SZ;
> +return 0;
> +}
> +
> +static void amdgpu_userq_gfx_v11_destroy_ctx_space(struct amdgpu_userq_mgr
> *uq_mgr,
> + struct
> amdgpu_usermode_queue *queue)
> +{
> +struct amdgpu_userq_ctx_space *ctx = &queue->fw_space;
> +
> +amdgpu_bo_free_kernel(&ctx->obj,
> + &ctx->gpu_addr,
> + &ctx->cpu_ptr);
> +}
> +
> static int
> amdgpu_userq_gfx_v11_mqd_create(struct amdgpu_userq_mgr *uq_mgr, struct
> amdgpu_usermode_queue *queue)
> {
> @@ -43,10 +88,17 @@ amdgpu_userq_gfx_v11_mqd_create(struct amdgpu_userq_mgr
> *uq_mgr, struct amdgpu_u
> }
>
> memset(mqd->cpu_ptr, 0, size);
> +
> +r = amdgpu_userq_gfx_v11_create_ctx_space(uq_mgr, queue);
> +if (r) {
> +DRM_ERROR("Failed to create CTX space for userqueue (%d)\n", r);
> +goto free_mqd;
> +}
> +
> r = amdgpu_bo_reserve(mqd->obj, false);
> if (unlikely(r != 0)) {
> DRM_ERROR("Failed to reserve mqd for userqueue (%d)", r);
> -goto free_mqd;
> +goto free_ctx;
> }
>
> queue->userq_prop.use_doo
RE: [PATCH] drm/amdkfd: On GFX11 check PCIe atomics support and set CP_HQD_HQ_STATUS0[29]
[Public]
> -Original Message-
> From: Sider, Graham
> Sent: Tuesday, April 4, 2023 12:00 PM
> To: Russell, Kent ; Somasekharan, Sreekant
> ; amd-gfx@lists.freedesktop.org
> Cc: Somasekharan, Sreekant
> Subject: RE: [PATCH] drm/amdkfd: On GFX11 check PCIe atomics support and set
> CP_HQD_HQ_STATUS0[29]
>
> [Public]
>
> > -Original Message-
> > From: amd-gfx On Behalf Of
> > Russell, Kent
> > Sent: Tuesday, April 4, 2023 9:43 AM
> > To: Somasekharan, Sreekant ; amd-
> > g...@lists.freedesktop.org
> > Cc: Somasekharan, Sreekant
> > Subject: RE: [PATCH] drm/amdkfd: On GFX11 check PCIe atomics support and
> > set CP_HQD_HQ_STATUS0[29]
> >
> > Caution: This message originated from an External Source. Use proper
> > caution when opening attachments, clicking links, or responding.
> >
> >
> > [AMD Official Use Only - General]
> >
> > Comments inline
> >
> > > -Original Message-
> > > From: amd-gfx On Behalf Of
> > > Sreekant Somasekharan
> > > Sent: Monday, April 3, 2023 3:59 PM
> > > To: amd-gfx@lists.freedesktop.org
> > > Cc: Somasekharan, Sreekant
> > > Subject: [PATCH] drm/amdkfd: On GFX11 check PCIe atomics support and
> > > set CP_HQD_HQ_STATUS0[29]
> > >
> > > On GFX11, CP_HQD_HQ_STATUS0[29] bit will be used by CPFW to
> > > acknowledge whether PCIe atomics are supported. The default value of
> > > this bit is set to 0. Driver will check whether PCIe atomics are
> > > supported and set the bit to 1 if supported. This will force CPFW to use
> > > real
> > atomic ops.
> > > If the bit is not set, CPFW will default to read/modify/write using
> > > the firmware itself.
> > >
> > > This is applicable only to RS64 based GFX11 with MEC FW greater than
> > > or equal to 509. If MEC FW is less than 509, PCIe atomics needs to be
> > > supported, else it will skip the device.
> > >
> > > This commit also involves moving amdgpu_amdkfd_device_probe()
> > function
> > > call after per-IP early_init loop in amdgpu_device_ip_early_init()
> > > function so as to check for RS64 enabled device.
> > >
> > > Signed-off-by: Sreekant Somasekharan
> >
> > > ---
> > > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
> > > drivers/gpu/drm/amd/amdkfd/kfd_device.c | 11 +++
> > > drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v11.c | 9 +
> > > 3 files changed, 21 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > index 7116119ed038..b3a754ca0923 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > > @@ -2150,7 +2150,6 @@ static int amdgpu_device_ip_early_init(struct
> > > amdgpu_device *adev)
> > > adev->has_pr3 = parent ? pci_pr3_present(parent) : false;
> > > }
> > >
> > > - amdgpu_amdkfd_device_probe(adev);
> > >
> > > adev->pm.pp_feature = amdgpu_pp_feature_mask;
> > > if (amdgpu_sriov_vf(adev) || sched_policy ==
> > > KFD_SCHED_POLICY_NO_HWS)
> > > @@ -2206,6 +2205,7 @@ static int amdgpu_device_ip_early_init(struct
> > > amdgpu_device *adev)
> > > if (!total)
> > > return -ENODEV;
> > >
> > > + amdgpu_amdkfd_device_probe(adev);
> > > adev->cg_flags &= amdgpu_cg_mask;
> > > adev->pg_flags &= amdgpu_pg_mask;
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> > > b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> > > index 521dfa88aad8..64a295a35d37 100644
> > > --- a/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> > > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device.c
> > > @@ -204,6 +204,17 @@ static void kfd_device_info_init(struct kfd_dev
> > *kfd,
> > > /* Navi1x+ */
> > > if (gc_version >= IP_VERSION(10, 1, 1))
> > > kfd->device_info.needs_pci_atomics =
> > > true;
> > > + } else if (gc_version < IP_VERSION(12, 0, 0)) {
> >
> >
> > What if we get a GFX9 with MEC v509? Wouldn't that trigger this too?
> > Wondering if this should be if (gc_version>=IP_VERSION(11,0,0) &&
> > gc_version < IP_VERSION(12,0,0)) thus ensuring it's only GFX11. Or maybe
> > there is some better check than that. Either way, checking that it's < GFX11
> > might false-positive on GFX10- too, so we should probably be explicit in our
> > GFX check that it's only GFX11.
>
> The previous condition is for gc_version < IP_VERSION(11, 0, 0), so that
> condition will (and currently is) taken for gfx9/gfx10/etc.
>
> That's to say the logic after this change will look like:
>
> If (KFD_IS_SOC15(kfd)) {
> <...>
> If (gc_version < IP_VERSION(11, 0, 0)) {
> <...>
> } else if (gc_version < IP_VERSION(12, 0, 0)) {
> <...>
> }
> }
>
> So this new path will only be taken for gfx11.
Thanks Graham. I should've pulled up the file and looked at it in context.
Ignore my comment then.
Kent
>
> Best,
> Graham
>
> >
> > Kent
> >
Re: [PATCH v3 7/9] drm/amdgpu: map usermode queue into MES
On 2023-03-29 12:04, Shashank Sharma wrote:
> From: Shashank Sharma
>
> This patch adds new functions to map/unmap a usermode queue into
> the FW, using the MES ring. As soon as this mapping is done, the
> queue would be considered ready to accept the workload.
>
> V1: Addressed review comments from Alex on the RFC patch series
> - Map/Unmap should be IP specific.
> V2:
> Addressed review comments from Christian:
> - Fix the wptr_mc_addr calculation (moved into another patch)
> Addressed review comments from Alex:
> - Do not add fptrs for map/unmap
>
> V3: Integration with doorbell manager
>
> Cc: Alex Deucher
> Cc: Christian Koenig
> Signed-off-by: Shashank Sharma
> ---
Just add all your Cc right here, and let git-send-email figure it out.
Between the Cc tags and the SMTP CC list, Felix is the only one missing.
Regards,
Luben
> .../drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c | 70 +++
> 1 file changed, 70 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
> index 39e90ea32fcb..1627641a4a4e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
> @@ -23,12 +23,73 @@
> #include "amdgpu.h"
> #include "amdgpu_userqueue.h"
> #include "v11_structs.h"
> +#include "amdgpu_mes.h"
>
> #define AMDGPU_USERQ_PROC_CTX_SZ PAGE_SIZE
> #define AMDGPU_USERQ_GANG_CTX_SZ PAGE_SIZE
> #define AMDGPU_USERQ_FW_CTX_SZ PAGE_SIZE
> #define AMDGPU_USERQ_GDS_CTX_SZ PAGE_SIZE
>
> +static int
> +amdgpu_userq_gfx_v11_map(struct amdgpu_userq_mgr *uq_mgr,
> + struct amdgpu_usermode_queue *queue)
> +{
> +struct amdgpu_device *adev = uq_mgr->adev;
> +struct mes_add_queue_input queue_input;
> +int r;
> +
> +memset(&queue_input, 0x0, sizeof(struct mes_add_queue_input));
> +
> +queue_input.process_va_start = 0;
> +queue_input.process_va_end = (adev->vm_manager.max_pfn - 1) <<
> AMDGPU_GPU_PAGE_SHIFT;
> +queue_input.process_quantum = 10; /* 10ms */
> +queue_input.gang_quantum = 1; /* 1ms */
> +queue_input.paging = false;
> +
> +queue_input.gang_context_addr = queue->gang_ctx_gpu_addr;
> +queue_input.process_context_addr = queue->proc_ctx_gpu_addr;
> +queue_input.inprocess_gang_priority = AMDGPU_MES_PRIORITY_LEVEL_NORMAL;
> +queue_input.gang_global_priority_level =
> AMDGPU_MES_PRIORITY_LEVEL_NORMAL;
> +
> +queue_input.process_id = queue->vm->pasid;
> +queue_input.queue_type = queue->queue_type;
> +queue_input.mqd_addr = queue->mqd.gpu_addr;
> +queue_input.wptr_addr = queue->userq_prop.wptr_gpu_addr;
> +queue_input.queue_size = queue->userq_prop.queue_size >> 2;
> +queue_input.doorbell_offset = queue->userq_prop.doorbell_index;
> +queue_input.page_table_base_addr =
> amdgpu_gmc_pd_addr(queue->vm->root.bo);
> +
> +amdgpu_mes_lock(&adev->mes);
> +r = adev->mes.funcs->add_hw_queue(&adev->mes, &queue_input);
> +amdgpu_mes_unlock(&adev->mes);
> +if (r) {
> +DRM_ERROR("Failed to map queue in HW, err (%d)\n", r);
> +return r;
> +}
> +
> +DRM_DEBUG_DRIVER("Queue %d mapped successfully\n", queue->queue_id);
> +return 0;
> +}
> +
> +static void
> +amdgpu_userq_gfx_v11_unmap(struct amdgpu_userq_mgr *uq_mgr,
> + struct amdgpu_usermode_queue *queue)
> +{
> +struct amdgpu_device *adev = uq_mgr->adev;
> +struct mes_remove_queue_input queue_input;
> +int r;
> +
> +memset(&queue_input, 0x0, sizeof(struct mes_remove_queue_input));
> +queue_input.doorbell_offset = queue->userq_prop.doorbell_index;
> +queue_input.gang_context_addr = queue->gang_ctx_gpu_addr;
> +
> +amdgpu_mes_lock(&adev->mes);
> +r = adev->mes.funcs->remove_hw_queue(&adev->mes, &queue_input);
> +amdgpu_mes_unlock(&adev->mes);
> +if (r)
> +DRM_ERROR("Failed to unmap queue in HW, err (%d)\n", r);
> +}
> +
> static int amdgpu_userq_gfx_v11_create_ctx_space(struct amdgpu_userq_mgr
> *uq_mgr,
> struct
> amdgpu_usermode_queue *queue)
> {
> @@ -129,6 +190,14 @@ amdgpu_userq_gfx_v11_mqd_create(struct amdgpu_userq_mgr
> *uq_mgr, struct amdgpu_u
>
> amdgpu_userq_set_ctx_space(uq_mgr, queue);
> amdgpu_bo_unreserve(mqd->obj);
> +
> +/* Map the queue in HW using MES ring */
> +r = amdgpu_userq_gfx_v11_map(uq_mgr, queue);
> +if (r) {
> +DRM_ERROR("Failed to map userqueue (%d)\n", r);
> +goto free_ctx;
> +}
> +
> DRM_DEBUG_DRIVER("MQD for queue %d created\n", queue->queue_id);
> return 0;
>
> @@ -147,6 +216,7 @@ amdgpu_userq_gfx_v11_mqd_destroy(struct amdgpu_userq_mgr
> *uq_mgr, struct amdgpu_
> {
> struct amdgpu_userq_ctx_space *mqd = &queue->mqd;
>
> +amdgpu_userq_gfx_v11_unmap(uq_mgr, queue);
> amdgpu_userq_gfx_v11_destroy_ctx_spac
Re: [PATCH v3 7/9] drm/amdgpu: map usermode queue into MES
On 04/04/2023 18:30, Luben Tuikov wrote:
On 2023-03-29 12:04, Shashank Sharma wrote:
From: Shashank Sharma
This patch adds new functions to map/unmap a usermode queue into
the FW, using the MES ring. As soon as this mapping is done, the
queue would be considered ready to accept the workload.
V1: Addressed review comments from Alex on the RFC patch series
- Map/Unmap should be IP specific.
V2:
Addressed review comments from Christian:
- Fix the wptr_mc_addr calculation (moved into another patch)
Addressed review comments from Alex:
- Do not add fptrs for map/unmap
V3: Integration with doorbell manager
Cc: Alex Deucher
Cc: Christian Koenig
Signed-off-by: Shashank Sharma
---
Just add all your Cc right here, and let git-send-email figure it out.
Between the Cc tags and the SMTP CC list, Felix is the only one missing.
No, that's not how it is.
You keep people cc'ed in the cover letter so that they get informed
every time this patch is pushed/used on any opensource branch.
People who are added manually in cc are required for this code review
session.
- Shashank
Regards,
Luben
.../drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c | 70 +++
1 file changed, 70 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
index 39e90ea32fcb..1627641a4a4e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
@@ -23,12 +23,73 @@
#include "amdgpu.h"
#include "amdgpu_userqueue.h"
#include "v11_structs.h"
+#include "amdgpu_mes.h"
#define AMDGPU_USERQ_PROC_CTX_SZ PAGE_SIZE
#define AMDGPU_USERQ_GANG_CTX_SZ PAGE_SIZE
#define AMDGPU_USERQ_FW_CTX_SZ PAGE_SIZE
#define AMDGPU_USERQ_GDS_CTX_SZ PAGE_SIZE
+static int
+amdgpu_userq_gfx_v11_map(struct amdgpu_userq_mgr *uq_mgr,
+ struct amdgpu_usermode_queue *queue)
+{
+struct amdgpu_device *adev = uq_mgr->adev;
+struct mes_add_queue_input queue_input;
+int r;
+
+memset(&queue_input, 0x0, sizeof(struct mes_add_queue_input));
+
+queue_input.process_va_start = 0;
+queue_input.process_va_end = (adev->vm_manager.max_pfn - 1) <<
AMDGPU_GPU_PAGE_SHIFT;
+queue_input.process_quantum = 10; /* 10ms */
+queue_input.gang_quantum = 1; /* 1ms */
+queue_input.paging = false;
+
+queue_input.gang_context_addr = queue->gang_ctx_gpu_addr;
+queue_input.process_context_addr = queue->proc_ctx_gpu_addr;
+queue_input.inprocess_gang_priority = AMDGPU_MES_PRIORITY_LEVEL_NORMAL;
+queue_input.gang_global_priority_level = AMDGPU_MES_PRIORITY_LEVEL_NORMAL;
+
+queue_input.process_id = queue->vm->pasid;
+queue_input.queue_type = queue->queue_type;
+queue_input.mqd_addr = queue->mqd.gpu_addr;
+queue_input.wptr_addr = queue->userq_prop.wptr_gpu_addr;
+queue_input.queue_size = queue->userq_prop.queue_size >> 2;
+queue_input.doorbell_offset = queue->userq_prop.doorbell_index;
+queue_input.page_table_base_addr = amdgpu_gmc_pd_addr(queue->vm->root.bo);
+
+amdgpu_mes_lock(&adev->mes);
+r = adev->mes.funcs->add_hw_queue(&adev->mes, &queue_input);
+amdgpu_mes_unlock(&adev->mes);
+if (r) {
+DRM_ERROR("Failed to map queue in HW, err (%d)\n", r);
+return r;
+}
+
+DRM_DEBUG_DRIVER("Queue %d mapped successfully\n", queue->queue_id);
+return 0;
+}
+
+static void
+amdgpu_userq_gfx_v11_unmap(struct amdgpu_userq_mgr *uq_mgr,
+ struct amdgpu_usermode_queue *queue)
+{
+struct amdgpu_device *adev = uq_mgr->adev;
+struct mes_remove_queue_input queue_input;
+int r;
+
+memset(&queue_input, 0x0, sizeof(struct mes_remove_queue_input));
+queue_input.doorbell_offset = queue->userq_prop.doorbell_index;
+queue_input.gang_context_addr = queue->gang_ctx_gpu_addr;
+
+amdgpu_mes_lock(&adev->mes);
+r = adev->mes.funcs->remove_hw_queue(&adev->mes, &queue_input);
+amdgpu_mes_unlock(&adev->mes);
+if (r)
+DRM_ERROR("Failed to unmap queue in HW, err (%d)\n", r);
+}
+
static int amdgpu_userq_gfx_v11_create_ctx_space(struct amdgpu_userq_mgr
*uq_mgr,
struct amdgpu_usermode_queue
*queue)
{
@@ -129,6 +190,14 @@ amdgpu_userq_gfx_v11_mqd_create(struct amdgpu_userq_mgr
*uq_mgr, struct amdgpu_u
amdgpu_userq_set_ctx_space(uq_mgr, queue);
amdgpu_bo_unreserve(mqd->obj);
+
+/* Map the queue in HW using MES ring */
+r = amdgpu_userq_gfx_v11_map(uq_mgr, queue);
+if (r) {
+DRM_ERROR("Failed to map userqueue (%d)\n", r);
+goto free_ctx;
+}
+
DRM_DEBUG_DRIVER("MQD for queue %d created\n", queue->queue_id);
return 0;
@@ -147,6 +216,7 @@ amdgpu_userq_gfx_v11_mqd_destroy(struct amdgpu_userq_mgr *uq_mgr, struct amdgpu_
{
struct amdgpu_userq_ctx_space *mqd = &queue->mqd;
+amdgpu_userq
Re: [PATCH v3 5/9] drm/amdgpu: create context space for usermode queue
On 04/04/2023 18:24, Luben Tuikov wrote:
On 2023-03-29 12:04, Shashank Sharma wrote:
From: Shashank Sharma
The FW expects us to allocate atleast one page as context space to
process gang, process, shadow, GDS and FW related work. This patch
creates a joint object for the same, and calculates GPU space offsets
for each of these spaces.
V1: Addressed review comments on RFC patch:
Alex: Make this function IP specific
V2: Addressed review comments from Christian
- Allocate only one object for total FW space, and calculate
offsets for each of these objects.
V3: Integration with doorbell manager
Cc: Alex Deucher
Cc: Christian Koenig
Signed-off-by: Shashank Sharma
---
drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 1 +
.../drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c | 60 ++-
.../gpu/drm/amd/include/amdgpu_userqueue.h| 7 +++
3 files changed, 66 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
index 052c2c1e8aed..5672efcbcffc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
@@ -71,6 +71,7 @@ static int amdgpu_userqueue_create(struct drm_file *filp,
union drm_amdgpu_userq
queue->userq_prop.queue_size = mqd_in->queue_size;
queue->doorbell_handle = mqd_in->doorbell_handle;
+queue->shadow_ctx_gpu_addr = mqd_in->shadow_va;
queue->queue_type = mqd_in->ip_type;
queue->flags = mqd_in->flags;
queue->vm = &fpriv->vm;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
index 12e1a785b65a..52de96727f98 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
@@ -23,6 +23,51 @@
#include "amdgpu.h"
#include "amdgpu_userqueue.h"
+#define AMDGPU_USERQ_PROC_CTX_SZ PAGE_SIZE
+#define AMDGPU_USERQ_GANG_CTX_SZ PAGE_SIZE
+#define AMDGPU_USERQ_FW_CTX_SZ PAGE_SIZE
+#define AMDGPU_USERQ_GDS_CTX_SZ PAGE_SIZE
+
Leaving a single space after the macro name and its value
makes it hard to read. Please align the value columns
and leave at least 3 spaces--this would make it readable.
For instance,
#define AMDGPU_USERQ_PROC_CTX_SZ PAGE_SIZE
#define AMDGPU_USERQ_GANG_CTX_SZ PAGE_SIZE
#define AMDGPU_USERQ_FW_CTX_SZ PAGE_SIZE
#define AMDGPU_USERQ_GDS_CTX_SZPAGE_SIZE
Noted,
Shashank
Regards,
Luben
+static int amdgpu_userq_gfx_v11_create_ctx_space(struct amdgpu_userq_mgr
*uq_mgr,
+ struct amdgpu_usermode_queue
*queue)
+{
+struct amdgpu_device *adev = uq_mgr->adev;
+struct amdgpu_userq_ctx_space *ctx = &queue->fw_space;
+int r, size;
+
+/*
+ * The FW expects atleast one page space allocated for
+ * process ctx, gang ctx, gds ctx, fw ctx and shadow ctx each.
+ */
+size = AMDGPU_USERQ_PROC_CTX_SZ + AMDGPU_USERQ_GANG_CTX_SZ +
+ AMDGPU_USERQ_FW_CTX_SZ + AMDGPU_USERQ_GDS_CTX_SZ;
+r = amdgpu_bo_create_kernel(adev, size, PAGE_SIZE,
+AMDGPU_GEM_DOMAIN_GTT,
+&ctx->obj,
+&ctx->gpu_addr,
+&ctx->cpu_ptr);
+if (r) {
+DRM_ERROR("Failed to allocate ctx space bo for userqueue, err:%d\n",
r);
+return r;
+}
+
+queue->proc_ctx_gpu_addr = ctx->gpu_addr;
+queue->gang_ctx_gpu_addr = queue->proc_ctx_gpu_addr +
AMDGPU_USERQ_PROC_CTX_SZ;
+queue->fw_ctx_gpu_addr = queue->gang_ctx_gpu_addr +
AMDGPU_USERQ_GANG_CTX_SZ;
+queue->gds_ctx_gpu_addr = queue->fw_ctx_gpu_addr + AMDGPU_USERQ_FW_CTX_SZ;
+return 0;
+}
+
+static void amdgpu_userq_gfx_v11_destroy_ctx_space(struct amdgpu_userq_mgr
*uq_mgr,
+ struct
amdgpu_usermode_queue *queue)
+{
+struct amdgpu_userq_ctx_space *ctx = &queue->fw_space;
+
+amdgpu_bo_free_kernel(&ctx->obj,
+ &ctx->gpu_addr,
+ &ctx->cpu_ptr);
+}
+
static int
amdgpu_userq_gfx_v11_mqd_create(struct amdgpu_userq_mgr *uq_mgr, struct
amdgpu_usermode_queue *queue)
{
@@ -43,10 +88,17 @@ amdgpu_userq_gfx_v11_mqd_create(struct amdgpu_userq_mgr
*uq_mgr, struct amdgpu_u
}
memset(mqd->cpu_ptr, 0, size);
+
+r = amdgpu_userq_gfx_v11_create_ctx_space(uq_mgr, queue);
+if (r) {
+DRM_ERROR("Failed to create CTX space for userqueue (%d)\n", r);
+goto free_mqd;
+}
+
r = amdgpu_bo_reserve(mqd->obj, false);
if (unlikely(r != 0)) {
DRM_ERROR("Failed to reserve mqd for userqueue (%d)", r);
-goto free_mqd;
+goto free_ctx;
}
queue->userq_prop.use_doorbell = true;
@@ -55,12 +107,15 @@ amdgpu_userq_gfx_v11_mqd_create(struct amdgpu_userq_mgr
*uq_mgr, struct amdgpu_u
amdgpu_bo_unreserve(mqd->ob
Re: [PATCH] drm/amdkfd: Fix dmabuf's redundant eviction when unmapping
[+Christian]
OK, this comes from the ttm_bo_wait_ctx call in this section of
amdgpu_bo_move:
if ((old_mem->mem_type == TTM_PL_TT ||
old_mem->mem_type == AMDGPU_PL_PREEMPT) &&
new_mem->mem_type == TTM_PL_SYSTEM) {
r = ttm_bo_wait_ctx(bo, ctx);
if (r)
return r;
amdgpu_ttm_backend_unbind(bo->bdev, bo->ttm);
ttm_resource_free(bo, &bo->resource);
ttm_bo_assign_mem(bo, new_mem);
goto out;
}
We can't just remove this wait. It's not even specific to KFD or DMABuf
imports. We also can't just change it to avoid waiting for eviction
fences because it's also used for GTT BOs (e.g. before a BO gets swapped
under extreme memory pressure). So we also need to trigger the eviction
fence in general case.
In the specific case of DMABuf imports, they share the reservation
object with the original BO. So waiting on the reservation triggers the
eviction fence on the original BO. I think we want to avoid the waiting
on eviction fences for all BOs where the underlying memory is managed by
some other BO, and at the same time also avoid ever evicting the DMABuf
import BO. That's what AMDGPU_PL_PREEMPT is for. So I think a
combination of two changes should to the trick:
1. Change kfd_mem_dmamap_dmabuf to use AMDGPU_GEM_DOMAIN_PREEMPTIBLE
2. Add a special case in the above if-block for old_mem->mem_type ==
AMDGPU_PL_PREEMPT: use amdgpu_bo_sync_wait with
owner=AMDGPU_FENCE_OWNER_KFD so that it doesn't wait for eviction fences
Regards,
Felix
Am 2023-04-04 um 10:36 schrieb Eric Huang:
Here is the backtrace from Jira:
Thu Nov 10 13:10:23 2022] Scheduling eviction of pid 97784 in 0 jiffies
[Thu Nov 10 13:10:23 2022] WARNING: CPU: 173 PID: 97784 at
/var/lib/dkms/amdgpu/5.16.9.22.20-1438746~20.04/build/amd/amdgpu/../amdkfd/kfd_device.c:878
kgd2kfd_schedule_evict_and_restore_process+0x104/0x120 [amdgpu]
[Thu Nov 10 13:10:23 2022] Modules linked in: veth amdgpu(OE)
amddrm_ttm_helper(OE) amdttm(OE) iommu_v2 amd_sched(OE) amdkcl(OE)
xt_conntrack xt_MASQUERADE nf_conntrack_netlink nfnetlink xfrm_user
xfrm_algo xt_addrtype iptable_filter iptable_nat nf_nat nf_conntrack
nf_defrag_ipv6 nf_defrag_ipv4 bpfilter br_netfilter bridge stp llc
aufs overlay binfmt_misc nls_iso8859_1 dm_multipath scsi_dh_rdac
scsi_dh_emc scsi_dh_alua intel_rapl_msr intel_rapl_common amd64_edac
edac_mce_amd kvm_amd kvm efi_pstore rapl ipmi_ssif ccp acpi_ipmi
k10temp ipmi_si ipmi_devintf ipmi_msghandler mac_hid sch_fq_codel msr
ip_tables x_tables autofs4 btrfs blake2b_generic zstd_compress raid10
raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx xor
raid6_pq libcrc32c raid1 raid0 multipath linear mlx5_ib ib_uverbs
ib_core crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel
crypto_simd cryptd ast drm_vram_helper drm_ttm_helper ttm mlx5_core
drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops
[Thu Nov 10 13:10:23 2022] pci_hyperv_intf cec psample igb mlxfw
rc_core dca ahci xhci_pci tls drm i2c_algo_bit libahci
xhci_pci_renesas i2c_piix4
[Thu Nov 10 13:10:23 2022] CPU: 173 PID: 97784 Comm: onnxruntime_tes
Tainted: G W OE 5.13.0-30-generic #33~20.04.1-Ubuntu
[Thu Nov 10 13:10:23 2022] Hardware name: GIGABYTE
G482-Z53-YF/MZ52-G40-00, BIOS R12 05/13/2020
[Thu Nov 10 13:10:23 2022] RIP:
0010:kgd2kfd_schedule_evict_and_restore_process+0x104/0x120 [amdgpu]
[Thu Nov 10 13:10:23 2022] Code: 5e 5d c3 4c 89 e7 e8 cb c6 44 df eb
e7 49 8b 45 60 48 89 ca 48 c7 c7 38 8b d7 c1 48 89 4d e0 8b b0 20 09
00 00 e8 87 ee 7e df <0f> 0b 48 8b 4d e0 eb 9f 41 be ea ff ff ff eb ba
41 be ed ff ff ff
[Thu Nov 10 13:10:23 2022] RSP: 0018:b25f2a173978 EFLAGS: 00010086
[Thu Nov 10 13:10:23 2022] RAX: RBX: 0001
RCX: 0027
[Thu Nov 10 13:10:23 2022] RDX: 0027 RSI: fffe
RDI: 95d06e4a09c8
[Thu Nov 10 13:10:23 2022] RBP: b25f2a173998 R08: 95d06e4a09c0
R09: b25f2a173750
[Thu Nov 10 13:10:23 2022] R10: 0001 R11: 0001
R12: 95c371d74580
[Thu Nov 10 13:10:23 2022] R13: 95b1cd3f2000 R14:
R15: 95c371d74580
[Thu Nov 10 13:10:23 2022] FS: 7fcaff268b00()
GS:95d06e48() knlGS:
[Thu Nov 10 13:10:23 2022] CS: 0010 DS: ES: CR0:
80050033
[Thu Nov 10 13:10:23 2022] CR2: 7fc64398 CR3: 0003e9492000
CR4: 00350ee0
[Thu Nov 10 13:10:23 2022] Call Trace:
[Thu Nov 10 13:10:23 2022]
[Thu Nov 10 13:10:23 2022] amdkfd_fence_enable_signaling+0x46/0x50
[amdgpu]
[Thu Nov 10 13:10:23 2022] __dma_fence_enable_signaling+0x52/0xb0
[Thu Nov 10 13:10:23 2022] dma_fence_default_wait+0xa9/0x200
[Thu Nov 10 13:10:23 2022] dma_fence_wait_timeout+0xbd/0xe0
[Thu Nov 10 13:10:23 2022] amddma_resv_wait_timeout+0x6f/0xd0 [amdkcl]
[Thu Nov 10 13:10:23 2022] amdttm_bo
[linux-next:master] BUILD REGRESSION 6a53bda3aaf3de5edeea27d0b1d8781d067640b6
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master branch HEAD: 6a53bda3aaf3de5edeea27d0b1d8781d067640b6 Add linux-next specific files for 20230404 Error/Warning reports: https://lore.kernel.org/oe-kbuild-all/202303082135.njdx1bij-...@intel.com https://lore.kernel.org/oe-kbuild-all/202303161521.jbgbafjj-...@intel.com https://lore.kernel.org/oe-kbuild-all/202304041708.siwlxmyd-...@intel.com https://lore.kernel.org/oe-kbuild-all/202304041748.0sqc4k4l-...@intel.com https://lore.kernel.org/oe-kbuild-all/202304042104.ufiuevbp-...@intel.com https://lore.kernel.org/oe-kbuild-all/202304050029.38ndbqpf-...@intel.com Error/Warning: (recently discovered and may have been fixed) Documentation/virt/kvm/api.rst:8303: WARNING: Field list ends without a blank line; unexpected unindent. ERROR: modpost: "bpf_fentry_test1" [tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.ko] undefined! Error: failed to load BTF from vmlinux: No data available Makefile:77: *** Cannot find a vmlinux for VMLINUX_BTF at any of "vmlinux vmlinux ../../../../vmlinux /sys/kernel/btf/vmlinux /boot/vmlinux-5.9.0-0.bpo.2-amd64". Stop. arch/m68k/include/asm/irq.h:78:11: error: expected ';' before 'void' arch/m68k/include/asm/irq.h:78:40: warning: 'struct pt_regs' declared inside parameter list will not be visible outside of this definition or declaration diff: tools/arch/s390/include/uapi/asm/ptrace.h: No such file or directory drivers/gpu/drm/amd/amdgpu/../display/dc/link/link_validation.c:351:13: warning: variable 'bw_needed' set but not used [-Wunused-but-set-variable] drivers/gpu/drm/amd/amdgpu/../display/dc/link/link_validation.c:352:25: warning: variable 'link' set but not used [-Wunused-but-set-variable] drivers/gpu/drm/amd/amdgpu/../pm/swsmu/smu13/smu_v13_0_6_ppt.c:309:17: sparse: int drivers/gpu/drm/amd/amdgpu/../pm/swsmu/smu13/smu_v13_0_6_ppt.c:309:17: sparse: void drivers/net/ethernet/mellanox/mlx5/core/pci_irq.c:148:31: error: implicit declaration of function 'pci_msix_can_alloc_dyn' [-Werror=implicit-function-declaration] drivers/net/wireless/legacy/ray_cs.c:628:17: warning: 'strncpy' specified bound 32 equals destination size [-Wstringop-truncation] kernel/bpf/verifier.c:18503: undefined reference to `find_kallsyms_symbol_value' ld.lld: error: .btf.vmlinux.bin.o: unknown file type ld.lld: error: undefined symbol: find_kallsyms_symbol_value tcp_mmap.c:211:61: warning: 'lu' may be used uninitialized in this function [-Wmaybe-uninitialized] thermal_nl.h:6:10: fatal error: netlink/netlink.h: No such file or directory thermometer.c:21:10: fatal error: libconfig.h: No such file or directory Unverified Error/Warning (likely false positive, please contact us if interested): drivers/acpi/property.c:985 acpi_data_prop_read_single() error: potentially dereferencing uninitialized 'obj'. drivers/pinctrl/pinctrl-mlxbf3.c:162:20: sparse: sparse: symbol 'mlxbf3_pmx_funcs' was not declared. Should it be static? drivers/soc/fsl/qe/tsa.c:140:26: sparse: sparse: incorrect type in argument 2 (different address spaces) drivers/soc/fsl/qe/tsa.c:150:27: sparse: sparse: incorrect type in argument 1 (different address spaces) drivers/soc/fsl/qe/tsa.c:189:26: sparse: sparse: dereference of noderef expression drivers/soc/fsl/qe/tsa.c:663:22: sparse: sparse: incorrect type in assignment (different address spaces) drivers/soc/fsl/qe/tsa.c:673:21: sparse: sparse: incorrect type in assignment (different address spaces) include/linux/gpio/consumer.h: linux/err.h is included more than once. include/linux/gpio/driver.h: asm/bug.h is included more than once. io_uring/io_uring.c:432 io_prep_async_work() error: we previously assumed 'req->file' could be null (see line 425) io_uring/kbuf.c:221 __io_remove_buffers() warn: variable dereferenced before check 'bl->buf_ring' (see line 219) Error/Warning ids grouped by kconfigs: gcc_recent_errors |-- alpha-allyesconfig | |-- drivers-gpu-drm-amd-amdgpu-..-display-dc-link-link_validation.c:warning:variable-bw_needed-set-but-not-used | |-- drivers-gpu-drm-amd-amdgpu-..-display-dc-link-link_validation.c:warning:variable-link-set-but-not-used | `-- drivers-net-wireless-legacy-ray_cs.c:warning:strncpy-specified-bound-equals-destination-size |-- alpha-buildonly-randconfig-r005-20230403 | |-- drivers-gpu-drm-amd-amdgpu-..-display-dc-link-link_validation.c:warning:variable-bw_needed-set-but-not-used | `-- drivers-gpu-drm-amd-amdgpu-..-display-dc-link-link_validation.c:warning:variable-link-set-but-not-used |-- alpha-randconfig-s051-20230403 | |-- drivers-gpu-drm-amd-amdgpu-..-pm-swsmu-smu13-smu_v13_0_6_ppt.c:sparse:int | |-- drivers-gpu-drm-amd-amdgpu-..-pm-swsmu-smu13-smu_v13_0_6_ppt.c:sparse:sparse:incompatible-types-in-conditional-expression-(different-base-types): | |-- drivers-gpu
Re: [PATCH v3 7/9] drm/amdgpu: map usermode queue into MES
On 2023-04-04 12:36, Shashank Sharma wrote:
>
> On 04/04/2023 18:30, Luben Tuikov wrote:
>> On 2023-03-29 12:04, Shashank Sharma wrote:
>>> From: Shashank Sharma
>>>
>>> This patch adds new functions to map/unmap a usermode queue into
>>> the FW, using the MES ring. As soon as this mapping is done, the
>>> queue would be considered ready to accept the workload.
>>>
>>> V1: Addressed review comments from Alex on the RFC patch series
>>> - Map/Unmap should be IP specific.
>>> V2:
>>> Addressed review comments from Christian:
>>> - Fix the wptr_mc_addr calculation (moved into another patch)
>>> Addressed review comments from Alex:
>>> - Do not add fptrs for map/unmap
>>>
>>> V3: Integration with doorbell manager
>>>
>>> Cc: Alex Deucher
>>> Cc: Christian Koenig
>>> Signed-off-by: Shashank Sharma
>>> ---
>> Just add all your Cc right here, and let git-send-email figure it out.
>> Between the Cc tags and the SMTP CC list, Felix is the only one missing.
>
> No, that's not how it is.
>
> You keep people cc'ed in the cover letter so that they get informed
> every time this patch is pushed/used on any opensource branch.
The cover letter is optional, and you can add Cc tags
into the cover letter and then git-send-email would extract those
(any and all) tags from the cover letter too.
When you pick and choose whom to add in the Cc tags, and whom to
add to the SMTP CC list, it creates division.
> People who are added manually in cc are required for this code review
> session.
No such rule exists. It is best to add all the Cc into the commit message,
so that it is preserved in Git history.
For instance, I just randomly did "git log drivers/gpu/drm/*.[hc]" in
amd-staging-drm-next, and this is the first commit which came up,
commit 097ee58f3ddf7d
Author: Harry Wentland
Date: Fri Jan 13 11:24:09 2023 -0500
drm/connector: print max_requested_bpc in state debugfs
This is useful to understand the bpc defaults and
support of a driver.
Signed-off-by: Harry Wentland
Cc: Pekka Paalanen
Cc: Sebastian Wick
Cc: vitaly.pros...@amd.com
Cc: Uma Shankar
Cc: Ville Syrjälä
Cc: Joshua Ashton
Cc: Jani Nikula
Cc: dri-de...@lists.freedesktop.org
Cc: amd-gfx@lists.freedesktop.org
Reviewed-By: Joshua Ashton
Link:
https://patchwork.freedesktop.org/patch/msgid/20230113162428.33874-3-harry.wentl...@amd.com
Signed-off-by: Alex Deucher
As you can see the whole Cc list and the MLs are part of the Cc tags.
And the rest of the commits are also good examples of how to do it.
(Don't worry about the Link tag--it is automatically added by tools
maintainers use, although some use Lore.)
This preserves things in Git history, and it's a good thing if we need
to data mine and brainstorm later on on patches, design, and so on.
A good tool to use is "scripts/get_maintainer.pl" which works
on a file, directory and even patch files.
I usually include everyone get_maintainer.pl prints, and on subsequent patch
revisions, also people who have previously commented on the patchset, as they
might be interested to follow up. It's a good thing to do.
Here are a couple of resources, in addition to DRM commits in the tree,
https://www.kernel.org/doc/html/v4.12/process/5.Posting.html#patch-formatting-and-changelogs
https://www.kernel.org/doc/html/v4.12/process/submitting-patches.html#the-canonical-patch-format
(The second link includes links to more resources on good patch writing.)
Hope this helps.
Regards,
Luben
>
> - Shashank
>
>> Regards,
>> Luben
>>
>>> .../drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c | 70 +++
>>> 1 file changed, 70 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
>>> index 39e90ea32fcb..1627641a4a4e 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
>>> @@ -23,12 +23,73 @@
>>> #include "amdgpu.h"
>>> #include "amdgpu_userqueue.h"
>>> #include "v11_structs.h"
>>> +#include "amdgpu_mes.h"
>>>
>>> #define AMDGPU_USERQ_PROC_CTX_SZ PAGE_SIZE
>>> #define AMDGPU_USERQ_GANG_CTX_SZ PAGE_SIZE
>>> #define AMDGPU_USERQ_FW_CTX_SZ PAGE_SIZE
>>> #define AMDGPU_USERQ_GDS_CTX_SZ PAGE_SIZE
>>>
>>> +static int
>>> +amdgpu_userq_gfx_v11_map(struct amdgpu_userq_mgr *uq_mgr,
>>> + struct amdgpu_usermode_queue *queue)
>>> +{
>>> +struct amdgpu_device *adev = uq_mgr->adev;
>>> +struct mes_add_queue_input queue_input;
>>> +int r;
>>> +
>>> +memset(&queue_input, 0x0, sizeof(struct mes_add_queue_input));
>>> +
>>> +queue_input.process_va_start = 0;
>>> +queue_input.process_va_end = (adev->vm_manager.max_pfn - 1) <<
>>> AMDGPU_GPU_PAGE_SHIFT;
>>> +queue_input.process_quantum = 10; /* 10ms */
>>> +queue_input.gang_quantum = 1; /* 1ms */
>>> +queue_input.pagi
[PATCH] drm/amdgpu: Update invalid PTE flag setting
Update the invalid PTE flag setting to ensure, in addition
to transitioning the retry fault to a no-retry fault, it
also causes the wavefront to enter the trap handler. With the
current setting, it only transitions to a no-retry fault.
Signed-off-by: Mukul Joshi
---
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 af6f26a97fc5..5df4f7bb241f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2488,7 +2488,7 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev,
u32 pasid,
/* Intentionally setting invalid PTE flag
* combination to force a no-retry-fault
*/
- flags = AMDGPU_PTE_SNOOPED | AMDGPU_PTE_PRT;
+ flags = AMDGPU_PTE_VALID | AMDGPU_PTE_SYSTEM | AMDGPU_PTE_PRT;
value = 0;
} else if (amdgpu_vm_fault_stop == AMDGPU_VM_FAULT_STOP_NEVER) {
/* Redirect the access to the dummy page */
--
2.35.1
Re: [RFC PATCH 0/4] uapi, drm: Add and implement RLIMIT_GPUPRIO
Hi! On 2023-04-04 04:50, Christian König wrote: > Adding a bunch of people who have been involved in this before. > > Am 03.04.23 um 22:15 schrieb Joshua Ashton: >> On 4/3/23 20:54, Christian König wrote: >>> Am 03.04.23 um 21:40 schrieb Joshua Ashton: [SNIP] Anyway, please let me know what you think! Definitely open to any feedback and advice you may have. :D >>> >>> Well the basic problem is that higher priority queues can be used to >>> starve low priority queues. >>> >>> This starvation in turn is very very bad for memory management since >>> the dma_fence's the GPU scheduler deals with have very strong >>> restrictions. >>> >>> Even exposing this under CAP_SYS_NICE is questionable, so we will >>> most likely have to NAK this. >> >> This is already exposed with CAP_SYS_NICE and is relied on by SteamVR >> for async reprojection and Gamescope's composite path on Steam Deck. > > Yeah, I know I was the one who designed that :) > >> >> Having a high priority async compute queue is really really important >> and advantageous for these tasks. >> >> The majority of usecases for something like this is going to be a >> compositor which does some really tiny amount of work per-frame but is >> incredibly latency dependent (as it depends on latching onto buffers >> just before vblank to do it's work) There seems to be a dependency here. Is it possible to express this dependency so that this work is done on vblank, then whoever needs this, can latch onto vblank and get scheduled and completed before the vblank? The problem generally is "We need to do some work B in order to satisfy some condition in work A. Let's raise the ``priority'' of work B so that if A needs it, when it needs it, it is ready." Or something to that effect. The system would be much more responsive and run optimally, if such dependencies are expressed directly, as opposed to trying to game the scheduler and add more and more priorities, one on top of the other, every so often. It's okay to have priorities when tasks are independent and unrelated. But when they do depend on each other directly, or indirectly (as in when memory allocation or freeing is concerned), thus creating priority inversion, then the best scheduler is the fair, oldest-ready-first scheduling, which is the default GPU scheduler in DRM at the moment (for the last few months). >> Starving and surpassing work on other queues is kind of the entire >> point. Gamescope and SteamVR do it on ACE as well so GFX work can run >> alongside it. Are there no dependencies between them? I mean if they're independent, we already have run queues with different priorities. But if they're dependent, perhaps we can express this explicitly so that we don't starve other tasks/queues... Regards, Luben > > Yes, unfortunately exactly that. > > The problem is that our memory management is designed around the idea > that submissions to the hardware are guaranteed to finish at some point > in the future. > > When we now have a functionality which allows to extend the amount of > time some work needs to finish on the hardware infinitely, then we have > a major problem at hand. > > What we could do is to make the GPU scheduler more clever and make sure > that while higher priority submissions get precedence and can even > preempt low priority submissions we still guarantee some forward > progress for everybody. > > Luben has been looking into a similar problem AMD internally as well, > maybe he has some idea here but I doubt that the solution will be simple. > > Regards, > Christian. > >> >> - Joshie 🐸✨ >> >
[PATCH] drm/amd/amdgpu: Fix trivial style errors
Fix coding style errors reported by checkpatch, specifically: ERROR: space prohibited before that ',' (ctx:WxV) +module_param_named(job_hang_limit, amdgpu_job_hang_limit, int ,0444); ^ ERROR: space required after that ',' (ctx:WxV) +module_param_named(job_hang_limit, amdgpu_job_hang_limit, int ,0444); This patch gets rid of all above type of "ERROR" messages in drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c: Cc: Christian König Cc: Alex Deucher Cc: Mario Limonciello Signed-off-by: Srinivasan Shanmugam --- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index e652ffb2c68e..d2af86c2af2b 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -526,7 +526,7 @@ module_param_named(virtual_display, amdgpu_virtual_display, charp, 0444); * Set how much time allow a job hang and not drop it. The default is 0. */ MODULE_PARM_DESC(job_hang_limit, "how much time allow a job hang and not drop it (default 0)"); -module_param_named(job_hang_limit, amdgpu_job_hang_limit, int ,0444); +module_param_named(job_hang_limit, amdgpu_job_hang_limit, int, 0444); /** * DOC: lbpw (int) -- 2.25.1
