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 <Felix.Kuehling@xxxxxxx>
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 <yong.zhao@xxxxxxx>
Signed-off-by: Felix Kuehling <Felix.Kuehling@xxxxxxx>
Acked-by: Oded Gabbay <oded.gabbay@xxxxxxxxx>
Signed-off-by: Oded Gabbay <oded.gabbay@xxxxxxxxx>
Regards,
Felix
>
> 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
