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

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

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. 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

Thanks.
--
Ashutosh