<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">
<blockquote style="margin: 0 0 0 40px; border: none; padding: 0px;">
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<span style="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">> + /* only
leave the offset segment */</span></div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<span style="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">> + vma->vm_pgoff
&= (1ULL << (KFD_MMAP_GPU_ID_SHIFT - PAGE_SHIFT)) - 1;</span></div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<br style="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)">
</div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<span style="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">You're now open-coding
what used to be done by the</span></div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<span style="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">KFD_MMAP_OFFSET_VALUE_GET
macro. I don't see how this is an improvement.</span></div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<span style="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">Maybe better to
update the macro to do this.</span></div>
</blockquote>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0);">
<span style="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"><br>
</span></div>
<div style="color: rgb(0, 0, 0);"><span style="font-size: 14.6667px;">I can <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">
definitely </span>do that, but I think we'd better delete this line completely as it seems odd to change vm_pgoff. Moreover this <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">vm_pgoff
is not used at all in the following function calls. What do you think?</span></span></div>
<div style="color: rgb(0, 0, 0);"><span style="font-size: 14.6667px;"><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"><br>
</span></span></div>
<div style="color: rgb(0, 0, 0);"><span style="font-size: 14.6667px;"><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,</span></span></div>
<div style="color: rgb(0, 0, 0);"><span style="font-size: 14.6667px;"><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</span></span></div>
<div id="appendonsend"></div>
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> Kuehling, Felix <Felix.Kuehling@amd.com><br>
<b>Sent:</b> Friday, November 1, 2019 6:48 PM<br>
<b>To:</b> Zhao, Yong <Yong.Zhao@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org><br>
<b>Subject:</b> Re: [PATCH] drm/amdkfd: Simplify the mmap offset related bit operations</font>
<div> </div>
</div>
<div class="BodyFragment"><font size="2"><span style="font-size:11pt;">
<div class="PlainText">On 2019-11-01 4:48 p.m., Zhao, Yong wrote:<br>
> The new code is much cleaner and results in better readability.<br>
><br>
> Change-Id: I0c1f7cca7e24ddb7b4ffe1cb0fa71943828ae373<br>
> Signed-off-by: Yong Zhao <Yong.Zhao@amd.com><br>
> ---<br>
> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 13 +++++++------<br>
> drivers/gpu/drm/amd/amdkfd/kfd_events.c | 1 -<br>
> drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 9 +++------<br>
> drivers/gpu/drm/amd/amdkfd/kfd_process.c | 3 +--<br>
> 4 files changed, 11 insertions(+), 15 deletions(-)<br>
><br>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c<br>
> index b91993753b82..590138727ca9 100644<br>
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c<br>
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c<br>
> @@ -298,7 +298,6 @@ static int kfd_ioctl_create_queue(struct file *filep, struct kfd_process *p,<br>
> /* Return gpu_id as doorbell offset for mmap usage */<br>
> args->doorbell_offset = KFD_MMAP_TYPE_DOORBELL;<br>
> args->doorbell_offset |= KFD_MMAP_GPU_ID(args->gpu_id);<br>
> - args->doorbell_offset <<= PAGE_SHIFT;<br>
> if (KFD_IS_SOC15(dev->device_info->asic_family))<br>
> /* On SOC15 ASICs, include the doorbell offset within the<br>
> * process doorbell frame, which could be 1 page or 2 pages.<br>
> @@ -1938,20 +1937,22 @@ static int kfd_mmap(struct file *filp, struct vm_area_struct *vma)<br>
> {<br>
> struct kfd_process *process;<br>
> struct kfd_dev *dev = NULL;<br>
> - unsigned long vm_pgoff;<br>
> + unsigned long mmap_offset;<br>
> unsigned int gpu_id;<br>
> <br>
> process = kfd_get_process(current);<br>
> if (IS_ERR(process))<br>
> return PTR_ERR(process);<br>
> <br>
> - vm_pgoff = vma->vm_pgoff;<br>
> - vma->vm_pgoff = KFD_MMAP_OFFSET_VALUE_GET(vm_pgoff);<br>
> - gpu_id = KFD_MMAP_GPU_ID_GET(vm_pgoff);<br>
> + mmap_offset = vma->vm_pgoff << PAGE_SHIFT;<br>
> + gpu_id = KFD_MMAP_GET_GPU_ID(mmap_offset);<br>
> if (gpu_id)<br>
> dev = kfd_device_by_id(gpu_id);<br>
> <br>
> - switch (vm_pgoff & KFD_MMAP_TYPE_MASK) {<br>
> + /* only leave the offset segment */<br>
> + vma->vm_pgoff &= (1ULL << (KFD_MMAP_GPU_ID_SHIFT - PAGE_SHIFT)) - 1;<br>
<br>
You're now open-coding what used to be done by the <br>
KFD_MMAP_OFFSET_VALUE_GET macro. I don't see how this is an improvement. <br>
Maybe better to update the macro to do this.<br>
<br>
<br>
> +<br>
> + switch (mmap_offset & KFD_MMAP_TYPE_MASK) {<br>
> case KFD_MMAP_TYPE_DOORBELL:<br>
> if (!dev)<br>
> return -ENODEV;<br>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c<br>
> index 908081c85de1..1f8365575b12 100644<br>
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c<br>
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c<br>
> @@ -346,7 +346,6 @@ int kfd_event_create(struct file *devkfd, struct kfd_process *p,<br>
> ret = create_signal_event(devkfd, p, ev);<br>
> if (!ret) {<br>
> *event_page_offset = KFD_MMAP_TYPE_EVENTS;<br>
> - *event_page_offset <<= PAGE_SHIFT;<br>
> *event_slot_index = ev->event_id;<br>
> }<br>
> break;<br>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h<br>
> index 66bae8f2dad1..8eecd2cd1fd2 100644<br>
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h<br>
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h<br>
> @@ -59,24 +59,21 @@<br>
> * NOTE: struct vm_area_struct.vm_pgoff uses offset in pages. Hence, these<br>
> * defines are w.r.t to PAGE_SIZE<br>
> */<br>
> -#define KFD_MMAP_TYPE_SHIFT (62 - PAGE_SHIFT)<br>
> +#define KFD_MMAP_TYPE_SHIFT (62)<br>
> #define KFD_MMAP_TYPE_MASK (0x3ULL << KFD_MMAP_TYPE_SHIFT)<br>
> #define KFD_MMAP_TYPE_DOORBELL (0x3ULL << KFD_MMAP_TYPE_SHIFT)<br>
> #define KFD_MMAP_TYPE_EVENTS (0x2ULL << KFD_MMAP_TYPE_SHIFT)<br>
> #define KFD_MMAP_TYPE_RESERVED_MEM (0x1ULL << KFD_MMAP_TYPE_SHIFT)<br>
> #define KFD_MMAP_TYPE_MMIO (0x0ULL << KFD_MMAP_TYPE_SHIFT)<br>
> <br>
> -#define KFD_MMAP_GPU_ID_SHIFT (46 - PAGE_SHIFT)<br>
> +#define KFD_MMAP_GPU_ID_SHIFT (46)<br>
> #define KFD_MMAP_GPU_ID_MASK (((1ULL << KFD_GPU_ID_HASH_WIDTH) - 1) \<br>
> << KFD_MMAP_GPU_ID_SHIFT)<br>
> #define KFD_MMAP_GPU_ID(gpu_id) ((((uint64_t)gpu_id) << KFD_MMAP_GPU_ID_SHIFT)\<br>
> & KFD_MMAP_GPU_ID_MASK)<br>
> -#define KFD_MMAP_GPU_ID_GET(offset) ((offset & KFD_MMAP_GPU_ID_MASK) \<br>
> +#define KFD_MMAP_GET_GPU_ID(offset) ((offset & KFD_MMAP_GPU_ID_MASK) \<br>
> >> KFD_MMAP_GPU_ID_SHIFT)<br>
> <br>
> -#define KFD_MMAP_OFFSET_VALUE_MASK (0x3FFFFFFFFFFFULL >> PAGE_SHIFT)<br>
> -#define KFD_MMAP_OFFSET_VALUE_GET(offset) (offset & KFD_MMAP_OFFSET_VALUE_MASK)<br>
<br>
This macro is still useful. See above. I think you should just update <br>
the mask and continue using it for clarity.<br>
<br>
Regards,<br>
Felix<br>
<br>
<br>
> -<br>
> /*<br>
> * When working with cp scheduler we should assign the HIQ manually or via<br>
> * the amdgpu driver to a fixed hqd slot, here are the fixed HIQ hqd slot<br>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c<br>
> index 6abfb77ae540..39dc49b8fd85 100644<br>
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c<br>
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c<br>
> @@ -554,8 +554,7 @@ static int kfd_process_init_cwsr_apu(struct kfd_process *p, struct file *filep)<br>
> if (!dev->cwsr_enabled || qpd->cwsr_kaddr || qpd->cwsr_base)<br>
> continue;<br>
> <br>
> - offset = (KFD_MMAP_TYPE_RESERVED_MEM | KFD_MMAP_GPU_ID(dev->id))<br>
> - << PAGE_SHIFT;<br>
> + offset = KFD_MMAP_TYPE_RESERVED_MEM | KFD_MMAP_GPU_ID(dev->id);<br>
> qpd->tba_addr = (int64_t)vm_mmap(filep, 0,<br>
> KFD_CWSR_TBA_TMA_SIZE, PROT_READ | PROT_EXEC,<br>
> MAP_SHARED, offset);<br>
</div>
</span></font></div>
</body>
</html>