[PATCH 3/5] drm/xe: Helper to get dpa from pfn

[Zeng, Oak]

> -----Original Message-----
> From: Brost, Matthew <matthew.brost at intel.com>
> Sent: Friday, March 15, 2024 9:34 PM
> To: Zeng, Oak <oak.zeng at intel.com>
> Cc: intel-xe at lists.freedesktop.org; Hellstrom, Thomas
> <thomas.hellstrom at intel.com>; airlied at gmail.com; Welty, Brian
> <brian.welty at intel.com>; Ghimiray, Himal Prasad
> <himal.prasad.ghimiray at intel.com>
> Subject: Re: [PATCH 3/5] drm/xe: Helper to get dpa from pfn
> 
> On Fri, Mar 15, 2024 at 11:29:33AM -0600, Zeng, Oak wrote:
> >
> >
> > > -----Original Message-----
> > > From: Brost, Matthew <matthew.brost at intel.com>
> > > Sent: Thursday, March 14, 2024 1:39 PM
> > > To: Zeng, Oak <oak.zeng at intel.com>
> > > Cc: intel-xe at lists.freedesktop.org; Hellstrom, Thomas
> > > <thomas.hellstrom at intel.com>; airlied at gmail.com; Welty, Brian
> > > <brian.welty at intel.com>; Ghimiray, Himal Prasad
> > > <himal.prasad.ghimiray at intel.com>
> > > Subject: Re: [PATCH 3/5] drm/xe: Helper to get dpa from pfn
> > >
> > > On Wed, Mar 13, 2024 at 11:35:51PM -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.
> > > >
> > > > Signed-off-by: Oak Zeng <oak.zeng at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/xe/xe_device_types.h | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/xe_device_types.h
> > > b/drivers/gpu/drm/xe/xe_device_types.h
> > > > index bbea40b57e84..bf349321f037 100644
> > > > --- a/drivers/gpu/drm/xe/xe_device_types.h
> > > > +++ b/drivers/gpu/drm/xe/xe_device_types.h
> > > > @@ -576,4 +576,12 @@ static inline struct xe_tile
> *mem_region_to_tile(struct
> > > xe_mem_region *mr)
> > > >  	return container_of(mr, struct xe_tile, mem.vram);
> > > >  }
> > > >
> > > > +static inline u64 vram_pfn_to_dpa(struct xe_mem_region *mr, u64 pfn)
> > > > +{
> > > > +	u64 dpa;
> > > > +	u64 offset = (pfn << PAGE_SHIFT) - mr->hpa_base;
> > >
> > > Can't this be negative?
> > >
> > > e.g. if pfn == 0, offset == -mr->hpa_base.
> > >
> > > Or is the assumption (pfn << PAGE_SHIFT) is always > mr->hpa_base?
> > >
> > > If so can we an assert or something to ensure we using this function correctly.
> >
> > Yes we can assert it. The hpa_base is the host physical base address for this
> memory region, while pfn should point to a page inside this memory region.
> >
> > I will add an assertion.
> >
> >
> > >
> > > > +	dpa = mr->dpa_base + offset;
> > > > +	return dpa;
> > > > +}
> > >
> > > Same as previous patch, should be *.h not a *_types.h file.
> >
> > Yes will fix.
> > >
> > > Also as this is xe_mem_region not explictly vram. Maybe:
> > >
> > > s/vram_pfn_to_dpa/xe_mem_region_pfn_to_dpa/
> >
> > Xe_mem_region can only represent vram, right? I mean it can't represent
> system memory. Copied the definition below:
> >
> > /**
> >  * struct xe_mem_region - memory region structure
> >  * This is used to describe a memory region in xe
> >  * device, such as HBM memory or CXL extension memory.
> >  */
> >
> 
> Ah yes but still as the first argument is xe_mem_region I think the
> function name should reflect that.

Sure, will rename it then.

Oak
> 
> Matt
> 
> > Oak
> >
> > >
> > > Matt
> > >
> > > > +
> > > >  #endif
> > > > --
> > > > 2.26.3
> > > >