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

["Hellstrom, Thomas" <thomas.hellstrom@intel.com>]

On Thu, 2024-03-14 at 17:39 +0000, Matthew Brost wrote:
> 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.

Please use imperative language according to the patch guidelines:
Something like "Add a" or "Introduce A"

> > 
> > Signed-off-by: Oak Zeng <oak.zeng@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)

static inline header functions also need kerneldoc unless strong
reasons not to.

/Thomas



> > +{
> > +	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.
> 
> > +	dpa = mr->dpa_base + offset;
> > +	return dpa;
> > +}
> 
> Same as previous patch, should be *.h not a *_types.h file.
> 
> Also as this is xe_mem_region not explictly vram. Maybe:
> 
> s/vram_pfn_to_dpa/xe_mem_region_pfn_to_dpa/
> 
> Matt
> 
> > +
> >  #endif
> > -- 
> > 2.26.3
> >