[PATCH 4/8] drm/xe: Introduce a helper to get dpa from pfn

[Zeng, Oak]

Hi Brian,

> -----Original Message-----
> From: Welty, Brian <brian.welty at intel.com>
> Sent: Wednesday, March 20, 2024 5:42 PM
> To: Zeng, Oak <oak.zeng at intel.com>; intel-xe at lists.freedesktop.org
> Cc: Hellstrom, Thomas <thomas.hellstrom at intel.com>; Brost, Matthew
> <matthew.brost at intel.com>; Ghimiray, Himal Prasad
> <himal.prasad.ghimiray at intel.com>
> Subject: Re: [PATCH 4/8] drm/xe: Introduce a helper to get dpa from pfn
> 
> 
> 
> On 3/19/2024 8:44 PM, 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 at 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"
> > +
> > +/**
> > + * 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)
> > +{
> > +	u64 dpa;
> > +	struct xe_tile *tile = xe_mem_region_to_tile(mr);
> 
> I believe you are dropping this patch for now, but wanted to comment.
> 
> Is above the only usage of xe_mem_region_to_tile() ?
> 
> As mentioned in earlier review, it's better to avoid the tight
> association with xe_mem_region and tile.  Such as for the UMA we talked
> before, it's nice to have flexibility for xe_mem_region to be associated
> with multiple tiles.  But I guess this is all future/pending, so can
> leave to solve in future if you prefer.
> 
> Not sure you have other usage of xe_mem_region_to_tile().  But here,
> looks only used for getting tile->xe pointer.
> Can caller of xe_mem_region_pfn_to_dpa() just pass in the xe device pointer?

Yes, in this case we can pass in the xe pointer. It is a good idea.

I agree what you said above. Couple region with tile is a bad idea. I will consider this perspective in the coming series.

Oak
> 
> -Brian
> 
> 
> > +	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);