Re: [PATCH 3/9] drm/xe: Add functions to convert regular address to canonical address and back

["Summers, Stuart" <stuart.summers@intel.com>]

On Wed, 2024-01-24 at 15:20 +0000, Souza, Jose wrote:
> On Wed, 2024-01-24 at 15:16 +0000, Summers, Stuart wrote:
> > On Mon, 2024-01-22 at 18:43 +0000, Souza, Jose wrote:
> > > On Mon, 2024-01-22 at 18:38 +0000, Summers, Stuart wrote:
> > > > On Mon, 2024-01-22 at 09:04 -0800, José Roberto de Souza wrote:
> > > > > Some instructions requires canonical address like
> > > > > MI_BATCH_BUFFER_START(UMDs must call xe_exec with a canonical
> > > > > address
> > > > > for Xe2+).
> > > > > 
> > > > > So here adding functions to convert regular address to
> > > > > canonical
> > > > > address and back, the first user of this functions will be
> > > > > added
> > > > > in the next patches.
> > > > > 
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Cc: Maarten Lankhorst <dev@lankhorst.se>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/xe/xe_device.c | 15 +++++++++++++++
> > > > >  drivers/gpu/drm/xe/xe_device.h |  3 +++
> > > > >  2 files changed, 18 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/xe/xe_device.c
> > > > > b/drivers/gpu/drm/xe/xe_device.c
> > > > > index ab417f4f7d2a7..b4cdcf1b2081a 100644
> > > > > --- a/drivers/gpu/drm/xe/xe_device.c
> > > > > +++ b/drivers/gpu/drm/xe/xe_device.c
> > > > > @@ -727,3 +727,18 @@ void xe_device_mem_access_put(struct
> > > > > xe_device
> > > > > *xe)
> > > > >  
> > > > >         xe_assert(xe, ref >= 0);
> > > > >  }
> > > > > +
> > > > > +static inline int highest_address_bit_get(struct xe_device
> > > > > *xe)
> > > > > +{
> > > > > +       return xe->info.dma_mask_size > 48 ? 57 : 47;
> > > > 
> > > > Why not just xe->info.dma_mask_size - 1?
> > > 
> > > We have platforms that set dma_mask_size to 39, 46 and 52 but the
> > > canonical address boundaries is 48bit and 58bit bits.
> > 
> > hm.. I feel like it would be more clear to have a canonical size
> > then
> > and then do (canonical_size - 1).
> 
> Like this?
> 
> static inline int highest_address_bit_get(struct xe_device *xe)

Well, this naming doesn't make sense if we do it this way IMO.

Maybe just va_top() or va_max()?

Or, for instance, in i915 we are using ppgtt_msb:
i915_pci.c:	.ppgtt_msb = 47, \
i915_pci.c:	.ppgtt_msb = 47, \
i915_pci.c:	.ppgtt_msb = 56, \

as a per-platform feature flag.

I think also there was a comment here about avoiding the "inline", were
you planning to address that?

Thanks,
Stuart

> {
>         return xe->info.dma_mask_size > 48 ? 58 : 48;
> }
> 
> u64 xe_device_canonicalize_addr(struct xe_device *xe, u64 address)
> {
>         return sign_extend64(address, highest_address_bit_get(xe) -
> 1);
> }
> 
> u64 xe_device_uncanonicalize_addr(struct xe_device *xe, u64 address)
> {
>         return address & GENMASK_ULL(highest_address_bit_get(xe) - 1,
> 0);
> }
> 
> > 
> > Thanks,
> > Stuart
> > 
> > > 
> > > > 
> > > > Thanks,
> > > > Stuart
> > > > 
> > > > > +}
> > > > > +
> > > > > +u64 xe_device_canonicalize_addr(struct xe_device *xe, u64
> > > > > address)
> > > > > +{
> > > > > +       return sign_extend64(address,
> > > > > highest_address_bit_get(xe));
> > > > > +}
> > > > > +
> > > > > +u64 xe_device_uncanonicalize_addr(struct xe_device *xe, u64
> > > > > address)
> > > > > +{
> > > > > +       return address &
> > > > > GENMASK_ULL(highest_address_bit_get(xe),
> > > > > 0);
> > > > > +}
> > > > > diff --git a/drivers/gpu/drm/xe/xe_device.h
> > > > > b/drivers/gpu/drm/xe/xe_device.h
> > > > > index af8ac2e9e2709..ce20f6fe6219a 100644
> > > > > --- a/drivers/gpu/drm/xe/xe_device.h
> > > > > +++ b/drivers/gpu/drm/xe/xe_device.h
> > > > > @@ -175,4 +175,7 @@ static inline bool
> > > > > xe_device_has_memirq(struct
> > > > > xe_device *xe)
> > > > >  
> > > > >  u32 xe_device_ccs_bytes(struct xe_device *xe, u64 size);
> > > > >  
> > > > > +u64 xe_device_canonicalize_addr(struct xe_device *xe, u64
> > > > > address);
> > > > > +u64 xe_device_uncanonicalize_addr(struct xe_device *xe, u64
> > > > > address);
> > > > > +
> > > > >  #endif
> > > > 
> > > 
> > 
>