[Public]
> >
> > +static uint32_t fold_memtype(uint32_t memtype) {
>
> In general please add prefixes to even static functions, e.g. amdgpu_vm_ or
> amdgpu_bo_.
>
> > + /* Squash private placements into 'cpu' to keep the legacy userspace view.
> */
> > + switch (mem_type) {
> > + case TTM_PL_VRAM:
> > + case TTM_PL_TT:
> > + return memtype
> > + default:
> > + return TTM_PL_SYSTEM;
> > + }
> > +}
> > +
> > +static uint32_t bo_get_memtype(struct amdgpu_bo *bo) {
>
> That whole function belongs into amdgpu_bo.c
Do you mean bo_get_memtype or fold_memtype? I debated whether bo_get_memtype should go into amdgpu_vm.c or amdgpu_bo.c, and since it's using fold_memtype and only useful for memory stats because of folding the private placements I just left them here together with the other mem stats code.
I can move it to amdgpu_bo.c make it return the memtype verbatim and just fold it when I do the accounting.
>
> > + struct ttm_resource *res = bo->tbo.resource;
> > + const uint32_t domain_to_pl[] = {
> > + [ilog2(AMDGPU_GEM_DOMAIN_CPU)] = TTM_PL_SYSTEM,
> > + [ilog2(AMDGPU_GEM_DOMAIN_GTT)] = TTM_PL_TT,
> > + [ilog2(AMDGPU_GEM_DOMAIN_VRAM)] = TTM_PL_VRAM,
> > + [ilog2(AMDGPU_GEM_DOMAIN_GDS)] = AMDGPU_PL_GDS,
> > + [ilog2(AMDGPU_GEM_DOMAIN_GWS)] = AMDGPU_PL_GWS,
> > + [ilog2(AMDGPU_GEM_DOMAIN_OA)] = AMDGPU_PL_OA,
> > + [ilog2(AMDGPU_GEM_DOMAIN_DOORBELL)] =
> AMDGPU_PL_DOORBELL,
> > + };
> > + uint32_t domain;
> > +
> > + if (res)
> > + return fold_memtype(res->mem_type);
> > +
> > + /*
> > + * If no backing store use one of the preferred domain for basic
> > + * stats. We take the MSB since that should give a reasonable
> > + * view.
> > + */
> > + BUILD_BUG_ON(TTM_PL_VRAM < TTM_PL_TT || TTM_PL_VRAM <
> TTM_PL_SYSTEM);
> > + domain = fls(bo->preferred_domains & AMDGPU_GEM_DOMAIN_MASK);
> > + if (drm_WARN_ON_ONCE(&adev->ddev,
> > + domain == 0 || --domain >= ARRAY_SIZE(domain_to_pl)))
>
> It's perfectly legal to create a BO without a placement. That one just won't have a
> backing store.
>
This is lifted from the previous change I'm rebasing onto. I think what it’s trying to do is if the BO doesn't have a placement, use the "biggest" (VRAM > TT > SYSTEM) preferred placement for the purpose of accounting. Previously we just ignore BOs that doesn't have a placement. I guess there's argument for going with either approaches.
> > + return 0;
> > + return fold_memtype(domain_to_pl[domain])
>
> That would need specular execution mitigation if I'm not completely mistaken.
>
> Better use a switch/case statement.
>
Do you mean change the array indexing to a switch statement?
Regards,
Teddy
