Re: [Intel-xe] [PATCH 16/21] drm/xe/oa: Make xe_oa_timestamp_frequency per gt

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Sep 21, 2023 at 02:58:14PM -0700, Dixit, Ashutosh wrote:
> On Thu, 21 Sep 2023 13:45:54 -0700, Rodrigo Vivi wrote:
> >
> 
> Hi Rodrigo,
> 
> > On Tue, Sep 19, 2023 at 09:10:44AM -0700, Ashutosh Dixit wrote:
> > > Clock freq's can be different for different gt's.
> > >
> > > Suggested-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_oa.c    | 44 +++++++++++++++++------------------
> > >  drivers/gpu/drm/xe/xe_oa.h    |  2 +-
> > >  drivers/gpu/drm/xe/xe_query.c |  2 +-
> > >  3 files changed, 24 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> > > index d49debe732bbd..8648652e05aa5 100644
> > > --- a/drivers/gpu/drm/xe/xe_oa.c
> > > +++ b/drivers/gpu/drm/xe/xe_oa.c
> > > @@ -1496,7 +1496,7 @@ xe_oa_stream_open_ioctl_locked(struct xe_oa *oa,
> > >   * platforms OA unit ignores the CTC_SHIFT and the 2 timestamps differ. In such
> > >   * cases, return the adjusted CS timestamp frequency to the user.
> > >   */
> > > -u32 xe_oa_timestamp_frequency(struct xe_device *xe)
> > > +u32 xe_oa_timestamp_frequency(struct xe_gt *gt)
> > >  {
> > >	u32 reg, shift;
> > >
> > > @@ -1505,26 +1505,26 @@ u32 xe_oa_timestamp_frequency(struct xe_device *xe)
> > >	 * Wa_14015568240:pvc
> > >	 * Wa_14015846243:mtl
> > >	 */
> > > -	switch (xe->info.platform) {
> > > +	switch (gt->tile->xe->info.platform) {
> > >	case XE_DG2:
> > >	case XE_PVC:
> > >	case XE_METEORLAKE:
> > > -		xe_device_mem_access_get(xe);
> > > -		reg = xe_mmio_read32(xe_root_mmio_gt(xe), RPM_CONFIG0);
> > > -		xe_device_mem_access_put(xe);
> > > +		xe_device_mem_access_get(gt->tile->xe);
> > > +		reg = xe_mmio_read32(gt, RPM_CONFIG0);
> > > +		xe_device_mem_access_put(gt->tile->xe);
> > >
> > >		shift = REG_FIELD_GET(RPM_CONFIG0_CTC_SHIFT_PARAMETER_MASK, reg);
> > > -		return xe_root_mmio_gt(xe)->info.clock_freq << (3 - shift);
> > > +		return gt->info.clock_freq << (3 - shift);
> > >
> > >	default:
> > > -		return xe_root_mmio_gt(xe)->info.clock_freq;
> > > +		return gt->info.clock_freq;
> > >	}
> > >  }
> 
> /snip/
> 
> > > diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
> > > index 4a3a9c11e8cc4..ad280bac9eed4 100644
> > > --- a/drivers/gpu/drm/xe/xe_query.c
> > > +++ b/drivers/gpu/drm/xe/xe_query.c
> > > @@ -244,7 +244,7 @@ static int query_gts(struct xe_device *xe, struct drm_xe_device_query *query)
> > >			gts->gts[id].type = XE_QUERY_GT_TYPE_MAIN;
> > >		gts->gts[id].instance = id;
> > >		gts->gts[id].clock_freq = gt->info.clock_freq;
> >
> > first look it here                      ^
> >
> > > -		gts->gts[id].oa_timestamp_freq = xe_oa_timestamp_frequency(xe);
> > > +		gts->gts[id].oa_timestamp_freq = xe_oa_timestamp_frequency(gt);
> >
> > Then please notice that we are trying to kill the duplications on the uapi [1]
> > and now notice that this series is adding yet another duplication:
> >
> > +u32 xe_oa_timestamp_frequency(struct xe_gt *gt)
> > +               return gt->info.clock_freq;
> > (copied from above)
> >
> > [1] - https://lore.kernel.org/all/20230920192940.135004-20-rodrigo.vivi@intel.com/
> 
> Well it's duplicated on some platforms but not on others.

Oh, I see now. Thanks for the explanation.

> If it helps,
> there is another proposal which will move all OA related information
> (including oa_timestamp_freq) into a separate drm_xe_query_oa_info struct
> (so out of the current uapi struct's into drm_xe_query_oa_info). See:
> 
> https://patchwork.freedesktop.org/patch/558367/?series=121084&rev=6

yeap, the individual query looks good.

> 
> Thanks.
> --
> Ashutosh