Re: [v2 15/31] drm/xe: Introduce a helper to get dpa from pfn

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Apr 09, 2024 at 04:17:26PM -0400, Oak Zeng wrote:
> Since we now create struct page backing for each vram page,
> each vram page now also has a pfn, just like system memory.
> This allow us to calcuate device physical address from pfn.
> 
> v1: move the function to xe_svm.h (Matt)
>     s/vram_pfn_to_dpa/xe_mem_region_pfn_to_dpa (Matt)
>     add kernel document for the helper (Thomas)
> 
> Signed-off-by: Oak Zeng <oak.zeng@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_svm.h | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_svm.h b/drivers/gpu/drm/xe/xe_svm.h
> index e944971cfc6d..8a34429eb674 100644
> --- a/drivers/gpu/drm/xe/xe_svm.h
> +++ b/drivers/gpu/drm/xe/xe_svm.h
> @@ -6,8 +6,31 @@
>  #ifndef __XE_SVM_H
>  #define __XE_SVM_H
>  
> -struct xe_tile;
> -struct xe_mem_region;
> +#include "xe_device_types.h"
> +#include "xe_device.h"
> +#include "xe_assert.h"

Hmm, including all these headers is a frowned upon and indicates to me
this likely the wrong location. The new header hopefully is clean with
only forward delc and function defs. I know Xe headers are not great at
this but lets not make this worse than it is.

Maybe should be xe_device.h? Also if we move the entire implementation 1
*.c file it is possible this function be private to that C file too.

> +
> +/**
> + * xe_mem_region_pfn_to_dpa() - Calculate page's dpa from pfn
> + *
> + * @mr: The memory region that page resides in
> + * @pfn: page frame number of the page
> + *
> + * Returns: the device physical address of the page
> + */
> +static inline u64 xe_mem_region_pfn_to_dpa(struct xe_mem_region *mr, u64 pfn)

I'd change to xe_mem_region_page_to_dpa with a struct page argument
rather than pfn. The pfn then be derived from the page.

I think this better as we will 3 types pfn all with different values /
shifts.

- hmm pfn
- migrate pfn
- linux core pfn

If a migrate pfn or hmm pfn were passed in as argument we'd get the
wrong dpa. I think passing in a page is safer and less bug prone. In my
example if we had a migrate pfn or hmm pfn, we'd use the appropriate
helper to get the struct page.

This also aligns with how a similar AMD helper (svm_migrate_addr, [1]) is
implemented.

Matt

[1] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c#L234

> +{
> +	u64 dpa;
> +	struct xe_tile *tile = xe_mem_region_to_tile(mr);
> +	struct xe_device *xe = tile_to_xe(tile);
> +	u64 offset;
> +
> +	xe_assert(xe, (pfn << PAGE_SHIFT) >= mr->hpa_base);
> +	offset = (pfn << PAGE_SHIFT) - mr->hpa_base;
> +	dpa = mr->dpa_base + offset;
> +
> +	return dpa;
> +}
>  
>  int xe_devm_add(struct xe_tile *tile, struct xe_mem_region *mr);
>  void xe_devm_remove(struct xe_tile *tile, struct xe_mem_region *mr);
> -- 
> 2.26.3
>