Re: [PATCH] drm/amdkfd: Simplify the mmap offset related bit operations

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> +     /* 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 <Felix.Kuehling@xxxxxxx>
Sent: Friday, November 1, 2019 6:48 PM
To: Zhao, Yong <Yong.Zhao@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx <amd-gfx@xxxxxxxxxxxxxxxxxxxxx>
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 <Yong.Zhao@xxxxxxx>
> ---
>   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   (0x3FFFFFFFFFFFULL >> 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 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);
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux