<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style> </head> <body dir="ltr"> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> Hi Felix,</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> </div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> The <span style="color: rgb(32, 31, 30); font-family: "Segoe UI", "Segoe UI Web (West European)", "Segoe UI", -apple-system, BlinkMacSystemFont, Roboto, "Helvetica Neue", sans-serif; font-size: 14.6667px; background-color: rgb(255, 255, 255); display: inline !important">PAGE_SHIFT was not deleted but merged into the <span style="font-family: "Segoe UI", "Segoe UI Web (West European)", "Segoe UI", -apple-system, BlinkMacSystemFont, Roboto, "Helvetica Neue", sans-serif; background-color: rgb(255, 255, 255); display: inline !important">KFD_*_SHIFT in kfd_priv.h. Because of that, this change is actually transparent to the thunk, and it only straightens up the bit shift operations in most cases.</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> <span style="color: rgb(32, 31, 30); font-family: "Segoe UI", "Segoe UI Web (West European)", "Segoe UI", -apple-system, BlinkMacSystemFont, Roboto, "Helvetica Neue", sans-serif; font-size: 14.6667px; background-color: rgb(255, 255, 255); display: inline !important"><span style="font-family: "Segoe UI", "Segoe UI Web (West European)", "Segoe UI", -apple-system, BlinkMacSystemFont, Roboto, "Helvetica Neue", sans-serif; background-color: rgb(255, 255, 255); display: inline !important"> </div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> <span style="color: rgb(32, 31, 30); font-family: "Segoe UI", "Segoe UI Web (West European)", "Segoe UI", -apple-system, BlinkMacSystemFont, Roboto, "Helvetica Neue", sans-serif; font-size: 14.6667px; background-color: rgb(255, 255, 255); display: inline !important"><span style="font-family: "Segoe UI", "Segoe UI Web (West European)", "Segoe UI", -apple-system, BlinkMacSystemFont, Roboto, "Helvetica Neue", sans-serif; background-color: rgb(255, 255, 255); display: inline !important">Regards,</div> <div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);"> <span style="color: rgb(32, 31, 30); font-family: "Segoe UI", "Segoe UI Web (West European)", "Segoe UI", -apple-system, BlinkMacSystemFont, Roboto, "Helvetica Neue", sans-serif; font-size: 14.6667px; background-color: rgb(255, 255, 255); display: inline !important"><span style="font-family: "Segoe UI", "Segoe UI Web (West European)", "Segoe UI", -apple-system, BlinkMacSystemFont, Roboto, "Helvetica Neue", sans-serif; background-color: rgb(255, 255, 255); display: inline !important">Yong</div> <div id="appendonsend"></div> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr">From: Kuehling, Felix <Felix.Kuehling@amd.com> Sent: Friday, November 1, 2019 5:13 PM To: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; Zhao, Yong <Yong.Zhao@amd.com> Subject: Re: [PATCH] drm/amdkfd: Simplify the mmap offset related bit operations <div> </div> </div> <div class="BodyFragment"> <div class="PlainText">NAK. This won't work for several reasons. The mmap_offset is used as offset parameter in the mmap system call. If you check the man page of mmap, you'll see that "offset must be a multiple of the page size". Therefore the PAGE_SHIFT is necessary. In the case of doorbell offsets, the offset is parsed and processed by the Thunk in user mode. On GFX9 GPUs the lower bits are used for the offset of the doorbell within the doorbell page. On GFX8 the queue ID was used, but on GFX9 we had to decoupled the doorbell ID from the queue ID. If you remove the PAGE_SHIFT, you'll need to put those bits somewhere else. But that change in the encoding would break the ABI with the Thunk. Regards, Felix On 2019-11-01 4:48 p.m., Zhao, Yong wrote: > The new code is much cleaner and results in better readability. > > Change-Id: I0c1f7cca7e24ddb7b4ffe1cb0fa71943828ae373 > Signed-off-by: Yong Zhao <Yong.Zhao@amd.com> > --- > drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 13 +++++++------ > drivers/gpu/drm/amd/amdkfd/kfd_events.c | 1 - > drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 9 +++------ > drivers/gpu/drm/amd/amdkfd/kfd_process.c | 3 +-- > 4 files changed, 11 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > index b91993753b82..590138727ca9 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > @@ -298,7 +298,6 @@ static int kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p, > /* Return gpu_id as doorbell offset for mmap usage */ > args->doorbell_offset = KFD_MMAP_TYPE_DOORBELL; > args->doorbell_offset |= KFD_MMAP_GPU_ID(args->gpu_id); > - args->doorbell_offset <<= PAGE_SHIFT; > if (KFD_IS_SOC15(dev->device_info->asic_family)) > /* On SOC15 ASICs, include the doorbell offset within the > * process doorbell frame, which could be 1 page or 2 pages. > @@ -1938,20 +1937,22 @@ static int kfd_mmap(struct file *filp, struct vm_area_struct *vma) > { > struct kfd_process *process; > struct kfd_dev *dev = NULL; > - unsigned long vm_pgoff; > + unsigned long mmap_offset; > unsigned int gpu_id; > > process = kfd_get_process(current); > if (IS_ERR(process)) > return PTR_ERR(process); > > - vm_pgoff = vma->vm_pgoff; > - vma->vm_pgoff = KFD_MMAP_OFFSET_VALUE_GET(vm_pgoff); > - gpu_id = KFD_MMAP_GPU_ID_GET(vm_pgoff); > + mmap_offset = vma->vm_pgoff << PAGE_SHIFT; > + gpu_id = KFD_MMAP_GET_GPU_ID(mmap_offset); > if (gpu_id) > dev = kfd_device_by_id(gpu_id); > > - switch (vm_pgoff & KFD_MMAP_TYPE_MASK) { > + /* only leave the offset segment */ > + vma->vm_pgoff &= (1ULL << (KFD_MMAP_GPU_ID_SHIFT - PAGE_SHIFT)) - 1; > + > + switch (mmap_offset & KFD_MMAP_TYPE_MASK) { > case KFD_MMAP_TYPE_DOORBELL: > if (!dev) > return -ENODEV; > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c > index 908081c85de1..1f8365575b12 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c > @@ -346,7 +346,6 @@ int kfd_event_create(struct file *devkfd, struct kfd_process *p, > ret = create_signal_event(devkfd, p, ev); > if (!ret) { > *event_page_offset = KFD_MMAP_TYPE_EVENTS; > - *event_page_offset <<= PAGE_SHIFT; > *event_slot_index = ev->event_id; > } > break; > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h > index 66bae8f2dad1..8eecd2cd1fd2 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h > @@ -59,24 +59,21 @@ > * NOTE: struct vm_area_struct.vm_pgoff uses offset in pages. Hence, these > * defines are w.r.t to PAGE_SIZE > */ > -#define KFD_MMAP_TYPE_SHIFT (62 - PAGE_SHIFT) > +#define KFD_MMAP_TYPE_SHIFT (62) > #define KFD_MMAP_TYPE_MASK (0x3ULL << KFD_MMAP_TYPE_SHIFT) > #define KFD_MMAP_TYPE_DOORBELL (0x3ULL << KFD_MMAP_TYPE_SHIFT) > #define KFD_MMAP_TYPE_EVENTS (0x2ULL << KFD_MMAP_TYPE_SHIFT) > #define KFD_MMAP_TYPE_RESERVED_MEM (0x1ULL << KFD_MMAP_TYPE_SHIFT) > #define KFD_MMAP_TYPE_MMIO (0x0ULL << KFD_MMAP_TYPE_SHIFT) > > -#define KFD_MMAP_GPU_ID_SHIFT (46 - PAGE_SHIFT) > +#define KFD_MMAP_GPU_ID_SHIFT (46) > #define KFD_MMAP_GPU_ID_MASK (((1ULL << KFD_GPU_ID_HASH_WIDTH) - 1) \ > << KFD_MMAP_GPU_ID_SHIFT) > #define KFD_MMAP_GPU_ID(gpu_id) ((((uint64_t)gpu_id) << KFD_MMAP_GPU_ID_SHIFT)\ > & KFD_MMAP_GPU_ID_MASK) > -#define KFD_MMAP_GPU_ID_GET(offset) ((offset & KFD_MMAP_GPU_ID_MASK) \ > +#define KFD_MMAP_GET_GPU_ID(offset) ((offset & KFD_MMAP_GPU_ID_MASK) \ > >> KFD_MMAP_GPU_ID_SHIFT) > > -#define KFD_MMAP_OFFSET_VALUE_MASK (0x3FFFFFFFFFFFULL >> PAGE_SHIFT) > -#define KFD_MMAP_OFFSET_VALUE_GET(offset) (offset & KFD_MMAP_OFFSET_VALUE_MASK) > - > /* > * When working with cp scheduler we should assign the HIQ manually or via > * the amdgpu driver to a fixed hqd slot, here are the fixed HIQ hqd slot > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c > index 6abfb77ae540..39dc49b8fd85 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c > @@ -554,8 +554,7 @@ static int kfd_process_init_cwsr_apu(struct kfd_process *p, struct file *filep) > if (!dev->cwsr_enabled || qpd->cwsr_kaddr || qpd->cwsr_base) > continue; > > - offset = (KFD_MMAP_TYPE_RESERVED_MEM | KFD_MMAP_GPU_ID(dev->id)) > - << PAGE_SHIFT; > + offset = KFD_MMAP_TYPE_RESERVED_MEM | KFD_MMAP_GPU_ID(dev->id); > qpd->tba_addr = (int64_t)vm_mmap(filep, 0, > KFD_CWSR_TBA_TMA_SIZE, PROT_READ | PROT_EXEC, > MAP_SHARED, offset); </div> </div> </body> </html>