From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 5AE46C77B7C for ; Fri, 5 May 2023 19:41:36 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 3181710E665; Fri, 5 May 2023 19:41:36 +0000 (UTC) Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by gabe.freedesktop.org (Postfix) with ESMTPS id 17CF610E665 for ; Fri, 5 May 2023 19:41:34 +0000 (UTC) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 75C9462EB9; Fri, 5 May 2023 19:41:34 +0000 (UTC) Received: from rdvivi-mobl4 (unknown [192.55.54.48]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.kernel.org (Postfix) with ESMTPSA id 29F63C433EF; Fri, 5 May 2023 19:41:32 +0000 (UTC) Date: Fri, 5 May 2023 15:41:27 -0400 From: Rodrigo Vivi To: Matthew Brost Message-ID: References: <20230502001727.3211096-1-matthew.brost@intel.com> <20230502001727.3211096-25-matthew.brost@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230502001727.3211096-25-matthew.brost@intel.com> Subject: Re: [Intel-xe] [PATCH v2 24/31] drm/xe: Userptr refactor X-BeenThere: intel-xe@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel Xe graphics driver List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: intel-xe@lists.freedesktop.org Errors-To: intel-xe-bounces@lists.freedesktop.org Sender: "Intel-xe" On Mon, May 01, 2023 at 05:17:20PM -0700, Matthew Brost wrote: > Add GPUVA userptr flag, add GPUVA userptr sub-struct, and drop sg > pointer. A larger follow on cleanup may push more of userptr > implementation to GPUVA. here as well, please have xe and drm changes in separated patches. But the final result looks good.... > > Signed-off-by: Matthew Brost > --- > drivers/gpu/drm/xe/xe_pt.c | 6 +-- > drivers/gpu/drm/xe/xe_vm.c | 41 +++++++++++---------- > drivers/gpu/drm/xe/xe_vm.h | 23 +++++++----- > drivers/gpu/drm/xe/xe_vm_types.h | 20 +++++----- > include/drm/drm_gpuva_mgr.h | 63 +++++++++++++++++++++----------- > 5 files changed, 89 insertions(+), 64 deletions(-) > > diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c > index 0f40f1950686..964baa24eba3 100644 > --- a/drivers/gpu/drm/xe/xe_pt.c > +++ b/drivers/gpu/drm/xe/xe_pt.c > @@ -92,8 +92,8 @@ static dma_addr_t vma_addr(struct xe_vma *vma, u64 offset, > page = offset >> PAGE_SHIFT; > offset &= (PAGE_SIZE - 1); > > - xe_res_first_sg(vma->userptr.sg, page << PAGE_SHIFT, page_size, > - &cur); > + xe_res_first_sg(&vma->userptr.sgt, page << PAGE_SHIFT, > + page_size, &cur); > return xe_res_dma(&cur) + offset; > } else { > return xe_bo_addr(xe_vma_bo(vma), offset, page_size, is_vram); > @@ -813,7 +813,7 @@ xe_pt_stage_bind(struct xe_gt *gt, struct xe_vma *vma, > xe_bo_assert_held(bo); > if (!xe_vma_is_null(vma)) { > if (xe_vma_is_userptr(vma)) > - xe_res_first_sg(vma->userptr.sg, 0, xe_vma_size(vma), > + xe_res_first_sg(&vma->userptr.sgt, 0, xe_vma_size(vma), > &curs); > else if (xe_bo_is_vram(bo) || xe_bo_is_stolen(bo)) > xe_res_first(bo->ttm.resource, xe_vma_bo_offset(vma), > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c > index 336e21c710a5..4d734ec4d6ab 100644 > --- a/drivers/gpu/drm/xe/xe_vm.c > +++ b/drivers/gpu/drm/xe/xe_vm.c > @@ -73,13 +73,13 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma) > if (!pages) > return -ENOMEM; > > - if (vma->userptr.sg) { > + if (xe_vma_userptr_sg_mapped(vma)) { > dma_unmap_sgtable(xe->drm.dev, > - vma->userptr.sg, > + &vma->userptr.sgt, > read_only ? DMA_TO_DEVICE : > DMA_BIDIRECTIONAL, 0); > - sg_free_table(vma->userptr.sg); > - vma->userptr.sg = NULL; > + sg_free_table(&vma->userptr.sgt); > + vma->gpuva.flags &= ~XE_VMA_USERPTR_SG_MAPPED; > } > > pinned = ret = 0; > @@ -119,19 +119,19 @@ int xe_vma_userptr_pin_pages(struct xe_vma *vma) > 0, (u64)pinned << PAGE_SHIFT, > GFP_KERNEL); > if (ret) { > - vma->userptr.sg = NULL; > + vma->gpuva.flags &= ~XE_VMA_USERPTR_SG_MAPPED; > goto out; > } > - vma->userptr.sg = &vma->userptr.sgt; > + vma->gpuva.flags |= XE_VMA_USERPTR_SG_MAPPED; > > - ret = dma_map_sgtable(xe->drm.dev, vma->userptr.sg, > + ret = dma_map_sgtable(xe->drm.dev, &vma->userptr.sgt, > read_only ? DMA_TO_DEVICE : > DMA_BIDIRECTIONAL, > DMA_ATTR_SKIP_CPU_SYNC | > DMA_ATTR_NO_KERNEL_MAPPING); > if (ret) { > - sg_free_table(vma->userptr.sg); > - vma->userptr.sg = NULL; > + sg_free_table(&vma->userptr.sgt); > + vma->gpuva.flags &= ~XE_VMA_USERPTR_SG_MAPPED; > goto out; > } > > @@ -820,15 +820,13 @@ static struct xe_vma *xe_vma_create(struct xe_vm *vm, > */ > INIT_LIST_HEAD(&vma->rebind_link); > > - INIT_LIST_HEAD(&vma->gpuva.gem.entry); > - INIT_LIST_HEAD(&vma->gpuva.gem.extobj_link); > vma->gpuva.mgr = &vm->mgr; > vma->gpuva.va.addr = start; > vma->gpuva.va.range = end - start + 1; > if (read_only) > vma->gpuva.flags |= XE_VMA_READ_ONLY; > if (null) > - vma->gpuva.flags |= XE_VMA_NULL; > + vma->gpuva.flags |= DRM_GPUVA_SPARSE; > > if (gt_mask) { > vma->gt_mask = gt_mask; > @@ -845,6 +843,8 @@ static struct xe_vma *xe_vma_create(struct xe_vm *vm, > xe_bo_assert_held(bo); > > drm_gem_object_get(&bo->ttm.base); > + INIT_LIST_HEAD(&vma->gpuva.gem.entry); > + INIT_LIST_HEAD(&vma->gpuva.gem.extobj_link); > vma->gpuva.gem.obj = &bo->ttm.base; > vma->gpuva.gem.offset = bo_offset_or_userptr; > if (!bo->vm) > @@ -855,7 +855,8 @@ static struct xe_vma *xe_vma_create(struct xe_vm *vm, > u64 size = end - start + 1; > int err; > > - vma->gpuva.gem.offset = bo_offset_or_userptr; > + vma->gpuva.flags |= DRM_GPUVA_USERPTR; > + vma->gpuva.userptr.address= bo_offset_or_userptr; > err = mmu_interval_notifier_insert(&vma->userptr.notifier, > current->mm, > xe_vma_userptr(vma), > @@ -883,13 +884,13 @@ static void xe_vma_destroy_late(struct xe_vma *vma) > bool read_only = xe_vma_read_only(vma); > > if (xe_vma_is_userptr(vma)) { > - if (vma->userptr.sg) { > + if (xe_vma_userptr_sg_mapped(vma)) { > dma_unmap_sgtable(xe->drm.dev, > - vma->userptr.sg, > + &vma->userptr.sgt, > read_only ? DMA_TO_DEVICE : > DMA_BIDIRECTIONAL, 0); > - sg_free_table(vma->userptr.sg); > - vma->userptr.sg = NULL; > + sg_free_table(&vma->userptr.sgt); > + vma->gpuva.flags &= ~XE_VMA_USERPTR_SG_MAPPED; > } > > /* > @@ -2309,7 +2310,7 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct xe_engine *e, > XE_VMA_READ_ONLY; > bool null = > op->base.remap.unmap->va->flags & > - XE_VMA_NULL; > + DRM_GPUVA_SPARSE; > > vma = new_vma(vm, op->base.remap.prev, > op->gt_mask, read_only, > @@ -2344,7 +2345,7 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct xe_engine *e, > > bool null = > op->base.remap.unmap->va->flags & > - XE_VMA_NULL; > + DRM_GPUVA_SPARSE; > > vma = new_vma(vm, op->base.remap.next, > op->gt_mask, read_only, > @@ -3320,7 +3321,7 @@ int xe_analyze_vm(struct drm_printer *p, struct xe_vm *vm, int gt_id) > } else if (is_userptr) { > struct xe_res_cursor cur; > > - xe_res_first_sg(vma->userptr.sg, 0, XE_PAGE_SIZE, &cur); > + xe_res_first_sg(&vma->userptr.sgt, 0, XE_PAGE_SIZE, &cur); > addr = xe_res_dma(&cur); > } else { > addr = xe_bo_addr(xe_vma_bo(vma), 0, XE_PAGE_SIZE, &is_vram); > diff --git a/drivers/gpu/drm/xe/xe_vm.h b/drivers/gpu/drm/xe/xe_vm.h > index 12de652d8d1c..f279fa622260 100644 > --- a/drivers/gpu/drm/xe/xe_vm.h > +++ b/drivers/gpu/drm/xe/xe_vm.h > @@ -101,12 +101,6 @@ static inline u64 xe_vma_bo_offset(struct xe_vma *vma) > return vma->gpuva.gem.offset; > } > > -static inline struct xe_bo *xe_vma_bo(struct xe_vma *vma) > -{ > - return !vma->gpuva.gem.obj ? NULL : > - container_of(vma->gpuva.gem.obj, struct xe_bo, ttm.base); > -} > - > static inline struct xe_vm *xe_vma_vm(struct xe_vma *vma) > { > return container_of(vma->gpuva.mgr, struct xe_vm, mgr); > @@ -129,7 +123,7 @@ static inline bool xe_vma_read_only(struct xe_vma *vma) > > static inline u64 xe_vma_userptr(struct xe_vma *vma) > { > - return vma->gpuva.gem.offset; > + return vma->gpuva.userptr.address; > } > > #define xe_vm_assert_held(vm) dma_resv_assert_held(&(vm)->mgr.resv) > @@ -197,12 +191,18 @@ static inline void xe_vm_reactivate_rebind(struct xe_vm *vm) > > static inline bool xe_vma_is_null(struct xe_vma *vma) > { > - return vma->gpuva.flags & XE_VMA_NULL; > + return vma->gpuva.flags & DRM_GPUVA_SPARSE; > } > > static inline bool xe_vma_is_userptr(struct xe_vma *vma) > { > - return !xe_vma_bo(vma) && !xe_vma_is_null(vma); > + return vma->gpuva.flags & DRM_GPUVA_USERPTR; > +} > + > +static inline struct xe_bo *xe_vma_bo(struct xe_vma *vma) > +{ > + return xe_vma_is_null(vma) || xe_vma_is_userptr(vma) ? NULL : > + container_of(vma->gpuva.gem.obj, struct xe_bo, ttm.base); > } > > static inline bool xe_vma_has_no_bo(struct xe_vma *vma) > @@ -210,6 +210,11 @@ static inline bool xe_vma_has_no_bo(struct xe_vma *vma) > return !xe_vma_bo(vma); > } > > +static inline bool xe_vma_userptr_sg_mapped(struct xe_vma *vma) > +{ > + return vma->gpuva.flags & XE_VMA_USERPTR_SG_MAPPED; > +} > + > int xe_vma_userptr_pin_pages(struct xe_vma *vma); > > int xe_vma_userptr_check_repin(struct xe_vma *vma); > diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h > index 0b59bde3bc4e..ce1260b8d3ef 100644 > --- a/drivers/gpu/drm/xe/xe_vm_types.h > +++ b/drivers/gpu/drm/xe/xe_vm_types.h > @@ -23,15 +23,15 @@ struct xe_vm; > #define TEST_VM_ASYNC_OPS_ERROR > #define FORCE_ASYNC_OP_ERROR BIT(31) > > -#define XE_VMA_READ_ONLY DRM_GPUVA_USERBITS > -#define XE_VMA_DESTROYED (DRM_GPUVA_USERBITS << 1) > -#define XE_VMA_ATOMIC_PTE_BIT (DRM_GPUVA_USERBITS << 2) > -#define XE_VMA_FIRST_REBIND (DRM_GPUVA_USERBITS << 3) > -#define XE_VMA_LAST_REBIND (DRM_GPUVA_USERBITS << 4) > -#define XE_VMA_NULL (DRM_GPUVA_USERBITS << 5) > -#define XE_VMA_PTE_4K (DRM_GPUVA_USERBITS << 6) > -#define XE_VMA_PTE_2M (DRM_GPUVA_USERBITS << 7) > -#define XE_VMA_PTE_1G (DRM_GPUVA_USERBITS << 8) > +#define XE_VMA_READ_ONLY DRM_GPUVA_USERBITS > +#define XE_VMA_DESTROYED (DRM_GPUVA_USERBITS << 1) > +#define XE_VMA_ATOMIC_PTE_BIT (DRM_GPUVA_USERBITS << 2) > +#define XE_VMA_FIRST_REBIND (DRM_GPUVA_USERBITS << 3) > +#define XE_VMA_LAST_REBIND (DRM_GPUVA_USERBITS << 4) > +#define XE_VMA_USERPTR_SG_MAPPED (DRM_GPUVA_USERBITS << 5) > +#define XE_VMA_PTE_4K (DRM_GPUVA_USERBITS << 6) > +#define XE_VMA_PTE_2M (DRM_GPUVA_USERBITS << 7) > +#define XE_VMA_PTE_1G (DRM_GPUVA_USERBITS << 8) > > /** struct xe_userptr - User pointer */ > struct xe_userptr { > @@ -41,8 +41,6 @@ struct xe_userptr { > struct mmu_interval_notifier notifier; > /** @sgt: storage for a scatter gather table */ > struct sg_table sgt; > - /** @sg: allocated scatter gather table */ > - struct sg_table *sg; > /** @notifier_seq: notifier sequence number */ > unsigned long notifier_seq; > /** > diff --git a/include/drm/drm_gpuva_mgr.h b/include/drm/drm_gpuva_mgr.h > index 57861a7ed504..943c8fcda533 100644 > --- a/include/drm/drm_gpuva_mgr.h > +++ b/include/drm/drm_gpuva_mgr.h > @@ -62,10 +62,17 @@ enum drm_gpuva_flags { > */ > DRM_GPUVA_EXTOBJ = (1 << 2), > > + /** > + * @DRM_GPUVA_USERPTR: > + * > + * Flag indicating that the &drm_gpuva is a user pointer mapping. > + */ > + DRM_GPUVA_USERPTR = (1 << 3), > + > /** > * @DRM_GPUVA_USERBITS: user defined bits > */ > - DRM_GPUVA_USERBITS = (1 << 3), > + DRM_GPUVA_USERBITS = (1 << 4), > }; > > /** > @@ -102,31 +109,45 @@ struct drm_gpuva { > u64 range; > } va; > > - /** > - * @gem: structure containing the &drm_gem_object and it's offset > - */ > - struct { > - /** > - * @offset: the offset within the &drm_gem_object > - */ > - u64 offset; > - > - /** > - * @obj: the mapped &drm_gem_object > - */ > - struct drm_gem_object *obj; > - > + union { > /** > - * @entry: the &list_head to attach this object to a &drm_gem_object > + * @gem: structure containing the &drm_gem_object and it's > + * offset > */ > - struct list_head entry; > + struct { > + /** > + * @offset: the offset within the &drm_gem_object > + */ > + u64 offset; > + > + /** > + * @obj: the mapped &drm_gem_object > + */ > + struct drm_gem_object *obj; > + > + /** > + * @entry: the &list_head to attach this object to a > + * &drm_gem_object > + */ > + struct list_head entry; > + > + /** > + * @extobj_link: the &list_head to attach this object to > + * a @drm_gpuva_manager.extobj.list > + */ > + struct list_head extobj_link; > + } gem; > > /** > - * @extobj_link: the &list_head to attach this object to a > - * @drm_gpuva_manager.extobj.list > + * @userptr: structure containing user pointer state > */ > - struct list_head extobj_link; > - } gem; > + struct { > + /** > + * @address: user pointer address > + */ > + u64 address; > + } userptr; > + }; > }; > > void drm_gpuva_link(struct drm_gpuva *va); > -- > 2.34.1 >