Re: [PATCH 2/2] drm/xe: Remove function xe_device_canonicalize_addr

[Francois Dugast <francois.dugast@intel.com>]

On Sun, Jul 21, 2024 at 12:45:47AM +0530, Ghimiray, Himal Prasad wrote:
> 
> 
> On 15-07-2024 23:44, Souza, Jose wrote:
> > On Mon, 2024-07-15 at 23:26 +0530, Ghimiray, Himal Prasad wrote:
> > > 
> > > On 15-07-2024 19:35, Souza, Jose wrote:
> > > > On Mon, 2024-07-15 at 16:25 +0530, Himal Prasad Ghimiray wrote:
> > > > > There is no need for converting regular address to canonical address in
> > > > > driver, therefore remove unused xe_device_canonicalize_addr.
> > > > 
> > > > I can see several places that should use it but are not.
> > > > Like every place that does xe_exec, the address needs to canonical in Xe2+ platforms.
> > > 
> > > Hi Jose, Thanks for feedback. Isn't it the responsibility of UMDs to
> > > call xe_exec with a canonical address ?
> > 
> > Yep but xe it self also does xe_execs.
> > 
> > > 
> > > > We are probably not getting any issues so far by luck, so I would not remove it to add it later.
> > > 
> > > Can you please elaborate on what do you mean by luck here ?
> > 
> > We are luck that so far none of the xe kmd internal usage has a batch buffer with that address that sets the va_bits - 1, if that happened the address
> > would not be in canonical format and HW would not execute the batch buffer.
> 
> Ok got it. Thanks for the reply.

Just a thought: could be good to add a test for this in IGT.

Francois

> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > > Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > > > > ---
> > > > >    drivers/gpu/drm/xe/xe_device.c | 5 -----
> > > > >    drivers/gpu/drm/xe/xe_device.h | 1 -
> > > > >    2 files changed, 6 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> > > > > index 06cebaffb451..eb3bb434b180 100644
> > > > > --- a/drivers/gpu/drm/xe/xe_device.c
> > > > > +++ b/drivers/gpu/drm/xe/xe_device.c
> > > > > @@ -885,11 +885,6 @@ void xe_device_snapshot_print(struct xe_device *xe, struct drm_printer *p)
> > > > >    	}
> > > > >    }
> > > > > -u64 xe_device_canonicalize_addr(struct xe_device *xe, u64 address)
> > > > > -{
> > > > > -	return sign_extend64(address, xe->info.va_bits - 1);
> > > > > -}
> > > > > -
> > > > >    u64 xe_device_uncanonicalize_addr(struct xe_device *xe, u64 address)
> > > > >    {
> > > > >    	return address & GENMASK_ULL(xe->info.va_bits - 1, 0);
> > > > > diff --git a/drivers/gpu/drm/xe/xe_device.h b/drivers/gpu/drm/xe/xe_device.h
> > > > > index 0a2a3e7fd402..14d4171c467e 100644
> > > > > --- a/drivers/gpu/drm/xe/xe_device.h
> > > > > +++ b/drivers/gpu/drm/xe/xe_device.h
> > > > > @@ -158,7 +158,6 @@ u32 xe_device_ccs_bytes(struct xe_device *xe, u64 size);
> > > > >    void xe_device_snapshot_print(struct xe_device *xe, struct drm_printer *p);
> > > > > -u64 xe_device_canonicalize_addr(struct xe_device *xe, u64 address);
> > > > >    u64 xe_device_uncanonicalize_addr(struct xe_device *xe, u64 address);
> > > > >    void xe_device_td_flush(struct xe_device *xe);
> > > > 
> >