Re: [PATCH] drm/amdkfd: Simplify the mmap offset related bit operations
On 2019-11-07 12:33, Zhao, Yong wrote:
> The new code uses straightforward bit shifts and thus has better readability.
>
> Change-Id: I0c1f7cca7e24ddb7b4ffe1cb0fa71943828ae373
> Signed-off-by: Yong Zhao
Reviewed-by: Felix Kuehling
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 17 +++--
> 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(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index b91993753b82..e59c229861e6 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.
> @@ -1312,10 +1311,9 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct file
> *filep,
> /* MMIO is mapped through kfd device
>* Generate a kfd mmap offset
>*/
> - if (flags & KFD_IOC_ALLOC_MEM_FLAGS_MMIO_REMAP) {
> - args->mmap_offset = KFD_MMAP_TYPE_MMIO |
> KFD_MMAP_GPU_ID(args->gpu_id);
> - args->mmap_offset <<= PAGE_SHIFT;
> - }
> + if (flags & KFD_IOC_ALLOC_MEM_FLAGS_MMIO_REMAP)
> + args->mmap_offset = KFD_MMAP_TYPE_MMIO
> + | KFD_MMAP_GPU_ID(args->gpu_id);
>
> return 0;
>
> @@ -1938,20 +1936,19 @@ 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) {
> + 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 (0x3FFFULL >> PAGE_SHIFT)
> -#define KFD_MMAP_OFFSET_VALUE_GET(offset) (offset &
[PATCH] drm/amdkfd: Simplify the mmap offset related bit operations
The new code uses straightforward bit shifts and thus has better readability.
Change-Id: I0c1f7cca7e24ddb7b4ffe1cb0fa71943828ae373
Signed-off-by: Yong Zhao
---
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 17 +++--
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(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index b91993753b82..e59c229861e6 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.
@@ -1312,10 +1311,9 @@ static int kfd_ioctl_alloc_memory_of_gpu(struct file
*filep,
/* MMIO is mapped through kfd device
* Generate a kfd mmap offset
*/
- if (flags & KFD_IOC_ALLOC_MEM_FLAGS_MMIO_REMAP) {
- args->mmap_offset = KFD_MMAP_TYPE_MMIO |
KFD_MMAP_GPU_ID(args->gpu_id);
- args->mmap_offset <<= PAGE_SHIFT;
- }
+ if (flags & KFD_IOC_ALLOC_MEM_FLAGS_MMIO_REMAP)
+ args->mmap_offset = KFD_MMAP_TYPE_MMIO
+ | KFD_MMAP_GPU_ID(args->gpu_id);
return 0;
@@ -1938,20 +1936,19 @@ 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) {
+ 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 (0x3FFFULL >> 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
Re: [PATCH] drm/amdkfd: Simplify the mmap offset related bit operations
True. Thank you for spotting that. MMAP-related code was added after I
inititally drafted this change earlier this year.
Regards,
Yong
From: Kuehling, Felix
Sent: Thursday, November 7, 2019 12:05 AM
To: Zhao, Yong ; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdkfd: Simplify the mmap offset related bit operations
On 2019-11-05 18:18, Zhao, Yong wrote:
> The new code uses straightforward bit shifts and thus has better
> readability.
You're missing the MMAP-related code for mmio remapping. In
kfd_ioctl_alloc_memory_of_gpu:
/* MMIO is mapped through kfd device
* Generate a kfd mmap offset
*/
if (flags & KFD_IOC_ALLOC_MEM_FLAGS_MMIO_REMAP) {
args->mmap_offset = KFD_MMAP_TYPE_MMIO |
KFD_MMAP_GPU_ID(args->gpu_id);
args->mmap_offset <<= PAGE_SHIFT;
}
Regards,
Felix
>
> Change-Id: I0c1f7cca7e24ddb7b4ffe1cb0fa71943828ae373
> Signed-off-by: Yong Zhao
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 10 --
> 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, 8 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..34078df36621 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,19 @@ 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) {
> + 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 (0x3FFFULL >> PAGE_SHIFT
Re: [PATCH] drm/amdkfd: Simplify the mmap offset related bit operations
On 2019-11-05 18:18, Zhao, Yong wrote:
> The new code uses straightforward bit shifts and thus has better
> readability.
You're missing the MMAP-related code for mmio remapping. In
kfd_ioctl_alloc_memory_of_gpu:
/* MMIO is mapped through kfd device
* Generate a kfd mmap offset
*/
if (flags & KFD_IOC_ALLOC_MEM_FLAGS_MMIO_REMAP) {
args->mmap_offset = KFD_MMAP_TYPE_MMIO |
KFD_MMAP_GPU_ID(args->gpu_id);
args->mmap_offset <<= PAGE_SHIFT;
}
Regards,
Felix
>
> Change-Id: I0c1f7cca7e24ddb7b4ffe1cb0fa71943828ae373
> Signed-off-by: Yong Zhao
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 10 --
> 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, 8 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..34078df36621 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,19 @@ 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) {
> + 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 (0x3FFFULL >> 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 =
[PATCH] drm/amdkfd: Simplify the mmap offset related bit operations
The new code uses straightforward bit shifts and thus has better readability.
Change-Id: I0c1f7cca7e24ddb7b4ffe1cb0fa71943828ae373
Signed-off-by: Yong Zhao
---
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 10 --
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, 8 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..34078df36621 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,19 @@ 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) {
+ 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 (0x3FFFULL >> 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 |
Re: [PATCH] drm/amdkfd: Simplify the mmap offset related bit operations
Okay. I will delete that line.
Regards,
Yong
From: Kuehling, Felix
Sent: Tuesday, November 5, 2019 5:34 PM
To: Zhao, Yong ; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdkfd: Simplify the mmap offset related bit operations
On 2019-11-01 7:03 p.m., Zhao, Yong wrote:
>
> > + /* only leave the offset segment */
> > + vma->vm_pgoff &= (1ULL << (KFD_MMAP_GPU_ID_SHIFT - PAGE_SHIFT)) -
> 1;
>
> You're now open-coding what used to be done by the
> KFD_MMAP_OFFSET_VALUE_GET macro. I don't see how this is an
> improvement.
> Maybe better to update the macro to do this.
>
>
> I can definitely do that, but I think we'd better delete this line
> completely as it seems odd to change vm_pgoff. Moreover this vm_pgoff
> is not used at all in the following function calls. What do you think?
I think you're right. Looks like a historical accident. I see that older
versions of kfd_event_mmap used to access vm_pgoff and probably depended
on this. We removed that in this commit:
commit 50cb7dd94cb43a6204813376e1be1d21780b71fb
Author: Felix Kuehling
Date: Fri Oct 27 19:35:26 2017 -0400
drm/amdkfd: Simplify events page allocator
The first event page is always big enough to handle all events.
Handling of multiple events pages is not supported by user mode, and
not necessary.
Signed-off-by: Yong Zhao
Signed-off-by: Felix Kuehling
Acked-by: Oded Gabbay
Signed-off-by: Oded Gabbay
Regards,
Felix
>
> Regards,
> Yong
>
> *From:* Kuehling, Felix
> *Sent:* Friday, November 1, 2019 6:48 PM
> *To:* Zhao, Yong ; amd-gfx@lists.freedesktop.org
>
> *Subject:* Re: [PATCH] drm/amdkfd: Simplify the mmap offset related
> bit operations
> 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
> > ---
> > 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;
>
> You're now open-coding what used to be done by the
> KFD_MMAP_OFFSET_VALUE_GET macro. I don't see how this is an improvement.
> Maybe better to update the macro to do this.
>
>
> > +
> > + switch (mmap_offset & KFD_MMAP_TYPE_MASK) {
> >case KFD_MMAP_TYPE_DOORBELL:
> >if (!dev)
> >return -ENODEV;
> > diff --git a/drivers/gpu/drm/amd/amdkfd
Re: [PATCH] drm/amdkfd: Simplify the mmap offset related bit operations
On 2019-11-01 7:03 p.m., Zhao, Yong wrote:
>
> > + /* only leave the offset segment */
> > + vma->vm_pgoff &= (1ULL << (KFD_MMAP_GPU_ID_SHIFT - PAGE_SHIFT)) -
> 1;
>
> You're now open-coding what used to be done by the
> KFD_MMAP_OFFSET_VALUE_GET macro. I don't see how this is an
> improvement.
> Maybe better to update the macro to do this.
>
>
> I can definitely do that, but I think we'd better delete this line
> completely as it seems odd to change vm_pgoff. Moreover this vm_pgoff
> is not used at all in the following function calls. What do you think?
I think you're right. Looks like a historical accident. I see that older
versions of kfd_event_mmap used to access vm_pgoff and probably depended
on this. We removed that in this commit:
commit 50cb7dd94cb43a6204813376e1be1d21780b71fb
Author: Felix Kuehling
Date: Fri Oct 27 19:35:26 2017 -0400
drm/amdkfd: Simplify events page allocator
The first event page is always big enough to handle all events.
Handling of multiple events pages is not supported by user mode, and
not necessary.
Signed-off-by: Yong Zhao
Signed-off-by: Felix Kuehling
Acked-by: Oded Gabbay
Signed-off-by: Oded Gabbay
Regards,
Felix
>
> Regards,
> Yong
>
> *From:* Kuehling, Felix
> *Sent:* Friday, November 1, 2019 6:48 PM
> *To:* Zhao, Yong ; amd-gfx@lists.freedesktop.org
>
> *Subject:* Re: [PATCH] drm/amdkfd: Simplify the mmap offset related
> bit operations
> 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
> > ---
> > 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;
>
> You're now open-coding what used to be done by the
> KFD_MMAP_OFFSET_VALUE_GET macro. I don't see how this is an improvement.
> Maybe better to update the macro to do this.
>
>
> > +
> > + 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_
Re: [PATCH] drm/amdkfd: Simplify the mmap offset related bit operations
> + /* only leave the offset segment */
> + vma->vm_pgoff &= (1ULL << (KFD_MMAP_GPU_ID_SHIFT - PAGE_SHIFT)) - 1;
You're now open-coding what used to be done by the
KFD_MMAP_OFFSET_VALUE_GET macro. I don't see how this is an improvement.
Maybe better to update the macro to do this.
I can definitely do that, but I think we'd better delete this line completely
as it seems odd to change vm_pgoff. Moreover this vm_pgoff is not used at all
in the following function calls. What do you think?
Regards,
Yong
From: Kuehling, Felix
Sent: Friday, November 1, 2019 6:48 PM
To: Zhao, Yong ; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amdkfd: Simplify the mmap offset related bit operations
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
> ---
> 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;
You're now open-coding what used to be done by the
KFD_MMAP_OFFSET_VALUE_GET macro. I don't see how this is an improvement.
Maybe better to update the macro to do this.
> +
> + 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)
> #def
Re: [PATCH] drm/amdkfd: Simplify the mmap offset related bit operations
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
> ---
> 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;
You're now open-coding what used to be done by the
KFD_MMAP_OFFSET_VALUE_GET macro. I don't see how this is an improvement.
Maybe better to update the macro to do this.
> +
> + 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 (0x3FFFULL >> PAGE_SHIFT)
> -#define KFD_MMAP_OFFSET_VALUE_GET(offset) (offset &
> KFD_MMAP_OFFSET_VALUE_MASK)
This macro is still useful. See above. I think you should just update
the mask and continue using it for clarity.
Regards,
Felix
> -
> /*
>* 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
Re: [PATCH] drm/amdkfd: Simplify the mmap offset related bit operations
Hi Felix,
The PAGE_SHIFT was not deleted but merged into the 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.
Regards,
Yong
From: Kuehling, Felix
Sent: Friday, November 1, 2019 5:13 PM
To: amd-gfx@lists.freedesktop.org ; Zhao, Yong
Subject: Re: [PATCH] drm/amdkfd: Simplify the mmap offset related bit operations
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
> ---
> 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)
>
Re: [PATCH] drm/amdkfd: Simplify the mmap offset related bit operations
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
> ---
> 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) \
>
Re: [PATCH] drm/amdkfd: Simplify the mmap offset related bit operations
Please discard this one and look for an update version.
Regards,
Yong
From: Zhao, Yong
Sent: Friday, November 1, 2019 4:11 PM
To: amd-gfx@lists.freedesktop.org
Cc: Zhao, Yong
Subject: [PATCH] drm/amdkfd: Simplify the mmap offset related bit operations
The new code is much cleaner and results in better readability.
Change-Id: I0c1f7cca7e24ddb7b4ffe1cb0fa71943828ae373
Signed-off-by: Yong Zhao
---
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 10 --
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, 8 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..34078df36621 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,19 @@ 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) {
+ 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 (0x3FFFULL >> 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/d
[PATCH] drm/amdkfd: Simplify the mmap offset related bit operations
The new code is much cleaner and results in better readability.
Change-Id: I0c1f7cca7e24ddb7b4ffe1cb0fa71943828ae373
Signed-off-by: Yong Zhao
---
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 (0x3FFFULL >> 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 |
[PATCH] drm/amdkfd: Simplify the mmap offset related bit operations
The new code is much cleaner and results in better readability.
Change-Id: I0c1f7cca7e24ddb7b4ffe1cb0fa71943828ae373
Signed-off-by: Yong Zhao
---
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 10 --
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, 8 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..34078df36621 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,19 @@ 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) {
+ 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 (0x3FFFULL >> 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 |
