[Intel-xe] [PATCH v2 05/10] drm/xe: Decouple vram check from xe_bo_addr()
Lucas De Marchi
lucas.demarchi at intel.com
Wed Jul 26 17:23:29 UTC 2023
On Wed, Jul 26, 2023 at 09:54:34AM -0700, Matt Roper wrote:
>On Wed, Jul 26, 2023 at 09:07:03AM -0700, Lucas De Marchi wrote:
>> The output arg is_vram in xe_bo_addr() is unused by several callers.
>> It's also not what the function is mainly doing. Remove the argument and
>> let the interested callers to call xe_bo_is_vram().
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>> drivers/gpu/drm/i915/display/intel_fbdev.c | 4 +---
>> drivers/gpu/drm/xe/xe_bo.c | 15 +++++----------
>> drivers/gpu/drm/xe/xe_bo.h | 10 +++-------
>> drivers/gpu/drm/xe/xe_ggtt.c | 5 ++---
>> drivers/gpu/drm/xe/xe_migrate.c | 16 ++++------------
>> drivers/gpu/drm/xe/xe_pt.c | 10 +++-------
>> drivers/gpu/drm/xe/xe_vm.c | 10 ++++++----
>> 7 files changed, 24 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
>> index a4f30773e314..f38d5c114a9d 100644
>> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
>> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
>> @@ -369,14 +369,12 @@ static int intelfb_create(struct drm_fb_helper *helper,
>>
>> #else
>> if (!(obj->flags & XE_BO_CREATE_SYSTEM_BIT)) {
>> - bool lmem;
>> -
>> if (obj->flags & XE_BO_CREATE_STOLEN_BIT)
>> info->fix.smem_start = xe_ttm_stolen_io_offset(obj, 0);
>> else
>> info->fix.smem_start =
>> pci_resource_start(pdev, 2) +
>> - xe_bo_addr(obj, 0, XE_PAGE_SIZE, &lmem);
>> + xe_bo_addr(obj, 0, XE_PAGE_SIZE);
>>
>> info->fix.smem_len = obj->ttm.base.size;
>> } else {
>> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
>> index 6a96cb0689c4..29813271cc4c 100644
>> --- a/drivers/gpu/drm/xe/xe_bo.c
>> +++ b/drivers/gpu/drm/xe/xe_bo.c
>> @@ -1516,12 +1516,11 @@ int xe_bo_pin(struct xe_bo *bo)
>> if (IS_DGFX(xe) && !(IS_ENABLED(CONFIG_DRM_XE_DEBUG) &&
>> bo->flags & XE_BO_INTERNAL_TEST)) {
>> struct ttm_place *place = &(bo->placements[0]);
>> - bool vram;
>>
>> if (mem_type_is_vram(place->mem_type)) {
>> XE_BUG_ON(!(place->flags & TTM_PL_FLAG_CONTIGUOUS));
>>
>> - place->fpfn = (xe_bo_addr(bo, 0, PAGE_SIZE, &vram) -
>> + place->fpfn = (xe_bo_addr(bo, 0, PAGE_SIZE) -
>> vram_region_gpu_offset(bo->ttm.resource)) >> PAGE_SHIFT;
>> place->lpfn = place->fpfn + (bo->size >> PAGE_SHIFT);
>>
>> @@ -1646,8 +1645,7 @@ bool xe_bo_is_xe_bo(struct ttm_buffer_object *bo)
>> * address, such as printing debug information, but not in cases where memory is
>> * written based on this result.
>> */
>> -dma_addr_t __xe_bo_addr(struct xe_bo *bo, u64 offset,
>> - size_t page_size, bool *is_vram)
>> +dma_addr_t __xe_bo_addr(struct xe_bo *bo, u64 offset, size_t page_size)
>> {
>> struct xe_res_cursor cur;
>> u64 page;
>> @@ -1656,9 +1654,7 @@ dma_addr_t __xe_bo_addr(struct xe_bo *bo, u64 offset,
>> page = offset >> PAGE_SHIFT;
>> offset &= (PAGE_SIZE - 1);
>>
>> - *is_vram = xe_bo_is_vram(bo);
>> -
>> - if (!*is_vram && !xe_bo_is_stolen(bo)) {
>> + if (!xe_bo_is_vram(bo) && !xe_bo_is_stolen(bo)) {
>> XE_BUG_ON(!bo->ttm.ttm);
>>
>> xe_res_first_sg(xe_bo_get_sg(bo), page << PAGE_SHIFT,
>> @@ -1673,12 +1669,11 @@ dma_addr_t __xe_bo_addr(struct xe_bo *bo, u64 offset,
>> }
>> }
>>
>> -dma_addr_t xe_bo_addr(struct xe_bo *bo, u64 offset,
>> - size_t page_size, bool *is_vram)
>> +dma_addr_t xe_bo_addr(struct xe_bo *bo, u64 offset, size_t page_size)
>> {
>> if (!READ_ONCE(bo->ttm.pin_count))
>> xe_bo_assert_held(bo);
>> - return __xe_bo_addr(bo, offset, page_size, is_vram);
>> + return __xe_bo_addr(bo, offset, page_size);
>> }
>>
>> int xe_bo_vmap(struct xe_bo *bo)
>> diff --git a/drivers/gpu/drm/xe/xe_bo.h b/drivers/gpu/drm/xe/xe_bo.h
>> index 3fed3e18c223..894ea0deb34b 100644
>> --- a/drivers/gpu/drm/xe/xe_bo.h
>> +++ b/drivers/gpu/drm/xe/xe_bo.h
>> @@ -207,17 +207,13 @@ static inline void xe_bo_unpin_map_no_vm(struct xe_bo *bo)
>> }
>>
>> bool xe_bo_is_xe_bo(struct ttm_buffer_object *bo);
>> -dma_addr_t __xe_bo_addr(struct xe_bo *bo, u64 offset,
>> - size_t page_size, bool *is_vram);
>> -dma_addr_t xe_bo_addr(struct xe_bo *bo, u64 offset,
>> - size_t page_size, bool *is_vram);
>> +dma_addr_t __xe_bo_addr(struct xe_bo *bo, u64 offset, size_t page_size);
>> +dma_addr_t xe_bo_addr(struct xe_bo *bo, u64 offset, size_t page_size);
>>
>> static inline dma_addr_t
>> xe_bo_main_addr(struct xe_bo *bo, size_t page_size)
>> {
>> - bool is_vram;
>> -
>> - return xe_bo_addr(bo, 0, page_size, &is_vram);
>> + return xe_bo_addr(bo, 0, page_size);
>> }
>>
>> static inline u32
>> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
>> index e1b84bc25375..f57dd8703d93 100644
>> --- a/drivers/gpu/drm/xe/xe_ggtt.c
>> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
>> @@ -31,12 +31,11 @@ u64 xe_ggtt_pte_encode(struct xe_bo *bo, u64 bo_offset)
>> {
>> struct xe_device *xe = xe_bo_device(bo);
>> u64 pte;
>> - bool is_vram;
>>
>> - pte = xe_bo_addr(bo, bo_offset, XE_PAGE_SIZE, &is_vram);
>> + pte = xe_bo_addr(bo, bo_offset, XE_PAGE_SIZE);
>> pte |= XE_PAGE_PRESENT;
>>
>> - if (is_vram)
>> + if (xe_bo_is_vram(bo))
>> pte |= XE_GGTT_PTE_LM;
>>
>> /* FIXME: vfunc + pass in caching rules */
>> diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
>> index 3c7d5cfd30bc..2a4b22c3a024 100644
>> --- a/drivers/gpu/drm/xe/xe_migrate.c
>> +++ b/drivers/gpu/drm/xe/xe_migrate.c
>> @@ -132,7 +132,6 @@ static int xe_migrate_create_cleared_bo(struct xe_migrate *m, struct xe_vm *vm)
>> struct xe_device *xe = vm->xe;
>> size_t cleared_size;
>> u64 vram_addr;
>> - bool is_vram;
>>
>> if (!xe_device_has_flat_ccs(xe))
>> return 0;
>> @@ -147,8 +146,7 @@ static int xe_migrate_create_cleared_bo(struct xe_migrate *m, struct xe_vm *vm)
>> return PTR_ERR(m->cleared_bo);
>>
>> xe_map_memset(xe, &m->cleared_bo->vmap, 0, 0x00, cleared_size);
>> - vram_addr = xe_bo_addr(m->cleared_bo, 0, XE_PAGE_SIZE, &is_vram);
>> - XE_BUG_ON(!is_vram);
>
>Should we keep the assertion here (but as a warning rather than a bug)?
>
> drm_WARN_ON(&xe->drm, !xe_bo_is_vram(m->cleared_bo));
>
>> + vram_addr = xe_bo_addr(m->cleared_bo, 0, XE_PAGE_SIZE);
>> m->cleared_vram_ofs = xe_migrate_vram_ofs(vram_addr);
>>
>> return 0;
>> @@ -221,15 +219,13 @@ static int xe_migrate_prepare_vm(struct xe_tile *tile, struct xe_migrate *m,
>> level++;
>> }
>> } else {
>> - bool is_vram;
>> - u64 batch_addr = xe_bo_addr(batch, 0, XE_PAGE_SIZE, &is_vram);
>> + u64 batch_addr = xe_bo_addr(batch, 0, XE_PAGE_SIZE);
>>
>> m->batch_base_ofs = xe_migrate_vram_ofs(batch_addr);
>>
>> if (xe->info.supports_usm) {
>> batch = tile->primary_gt->usm.bb_pool->bo;
>> - batch_addr = xe_bo_addr(batch, 0, XE_PAGE_SIZE,
>> - &is_vram);
>> + batch_addr = xe_bo_addr(batch, 0, XE_PAGE_SIZE);
>> m->usm_batch_base_ofs = xe_migrate_vram_ofs(batch_addr);
>> }
>> }
>> @@ -1000,12 +996,8 @@ static void write_pgtable(struct xe_tile *tile, struct xe_bb *bb, u64 ppgtt_ofs,
>> */
>> XE_BUG_ON(update->qwords > 0x1ff);
>> if (!ppgtt_ofs) {
>> - bool is_vram;
>> -
>> ppgtt_ofs = xe_migrate_vram_ofs(xe_bo_addr(update->pt_bo, 0,
>> - XE_PAGE_SIZE,
>> - &is_vram));
>> - XE_BUG_ON(!is_vram);
>
>Same here.
>
>> + XE_PAGE_SIZE));
>> }
>>
>> do {
>> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
>> index ac01bc42e54f..38ccc96c4584 100644
>> --- a/drivers/gpu/drm/xe/xe_pt.c
>> +++ b/drivers/gpu/drm/xe/xe_pt.c
>> @@ -61,13 +61,10 @@ u64 xe_pde_encode(struct xe_bo *bo, u64 bo_offset,
>> const enum xe_cache_level level)
>> {
>> u64 pde;
>> - bool is_vram;
>>
>> - pde = xe_bo_addr(bo, bo_offset, XE_PAGE_SIZE, &is_vram);
>> + pde = xe_bo_addr(bo, bo_offset, XE_PAGE_SIZE);
>> pde |= XE_PAGE_PRESENT | XE_PAGE_RW;
>>
>> - XE_WARN_ON(IS_DGFX(xe_bo_device(bo)) && !is_vram);
>
>And here.
I looked at those and thinking about calling xe_bo_is_vram(), but it
seems the places they were put are pretty much arbitrary. Why do we have
it here and not in the pte encode where it is actually used?
Also, I didn't see much value in keeping the BUG_ON() as there is
already an effort to remove/convert them. Since these BUG_ON and
warnings never triggered, I don't see a point in keeping them.
>
>> -
>> /* FIXME: I don't think the PPAT handling is correct for MTL */
>>
>> if (level != XE_CACHE_NONE)
>> @@ -128,10 +125,9 @@ u64 xe_pte_encode(struct xe_bo *bo, u64 offset, enum xe_cache_level cache,
>> u32 pt_level)
>> {
>> u64 pte;
>> - bool is_vram;
>>
>> - pte = xe_bo_addr(bo, offset, XE_PAGE_SIZE, &is_vram);
>> - if (is_vram)
>> + pte = xe_bo_addr(bo, offset, XE_PAGE_SIZE);
>> + if (xe_bo_is_vram(bo))
>> pte |= XE_PPGTT_PTE_LM;
>>
>> return __pte_encode(pte, cache, NULL, pt_level);
>> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>> index 6429d6e5113d..87f32cfc475a 100644
>> --- a/drivers/gpu/drm/xe/xe_vm.c
>> +++ b/drivers/gpu/drm/xe/xe_vm.c
>> @@ -3497,9 +3497,10 @@ int xe_analyze_vm(struct drm_printer *p, struct xe_vm *vm, int gt_id)
>> return 0;
>> }
>> if (vm->pt_root[gt_id]) {
>> - addr = xe_bo_addr(vm->pt_root[gt_id]->bo, 0, XE_PAGE_SIZE,
>> - &is_vram);
>> - drm_printf(p, " VM root: A:0x%llx %s\n", addr, is_vram ? "VRAM" : "SYS");
>> + addr = xe_bo_addr(vm->pt_root[gt_id]->bo, 0, XE_PAGE_SIZE);
>> + is_vram = xe_bo_is_vram(vm->pt_root[gt_id]->bo);
>> + drm_printf(p, " VM root: A:0x%llx %s\n", addr,
>> + is_vram ? "VRAM" : "SYS");
>> }
>>
>> drm_gpuva_for_each_va(gpuva, &vm->mgr) {
>> @@ -3520,7 +3521,8 @@ int xe_analyze_vm(struct drm_printer *p, struct xe_vm *vm, int gt_id)
>> addr = 0;
>> }
>> } else {
>> - addr = __xe_bo_addr(xe_vma_bo(vma), 0, XE_PAGE_SIZE, &is_vram);
>> + addr = __xe_bo_addr(xe_vma_bo(vma), 0, XE_PAGE_SIZE);
>> + is_vram = xe_bo_is_vram(xe_vma_bo(vma));
>
>This is an accurate transformation of the existing code, but is that
>code setting is_vram everywhere it should be? Shouldn't the null vma
>and userptr branches be setting is_vram to false rather than just
>leaving it at whatever value it already had?
I had initially added a "is_vram = false" before the if/else ladder due
to the same observation. However I then noticed is_vram is only used for
the drm_printf() and it will only be used if it executed this specific
branch:
is_null ? "NULL" : // 1st branch
is_userptr ? "USR" : // 2nd branch
is_vram ? "VRAM" : "SYS"); // 3rd branch
>
>Anyway, aside from possible restoring the various assertions that were
>removed, the changes look correct to me.
I will wait a little bit to see if anyone is strictly on the "keep the
warnings" side.
>
>Reviewed-by: Matt Roper <matthew.d.roper at intel.com>
thanks
Lucas De Marchi
>
>> }
>> drm_printf(p, " [%016llx-%016llx] S:0x%016llx A:%016llx %s\n",
>> xe_vma_start(vma), xe_vma_end(vma) - 1,
>> --
>> 2.40.1
>>
>
>--
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation
More information about the Intel-xe
mailing list