Re: [PATCH v9 2/6] drm/i915: Use to_gt() helper for GGTT accesses

[Andi Shyti <andi.shyti@linux.intel.com>]

Hi guys,

> > > > >  	struct drm_i915_gem_object *bo;
> > > > >  	struct i915_vma *vma;
> > > > >  	const u64 delay_ticks = 0xffffffffffffffff -
> > > > > -		intel_gt_ns_to_clock_interval(stream->perf->i915->ggtt.vm.gt,
> > > > > +		intel_gt_ns_to_clock_interval(to_gt(stream->perf->i915)->ggtt->vm.gt,
> > > >
> > > > I'm not too familiar with the perf code, but this looks a bit roundabout
> > > > since we're ultimately trying to get to a GT...do we even need to go
> > > > through the ggtt structure here or can we just pass
> > > > "to_gt(stream->perf->i915)" as the first parameter?
> > > >
> > > > >  					      atomic64_read(&stream->perf->noa_programming_delay));
> > > > >  	const u32 base = stream->engine->mmio_base;
> > > > >  #define CS_GPR(x) GEN8_RING_CS_GPR(base, x)
> > > > > @@ -3542,7 +3542,7 @@ i915_perf_open_ioctl_locked(struct i915_perf *perf,
> > > > >
> > > > >  static u64 oa_exponent_to_ns(struct i915_perf *perf, int exponent)
> > > > >  {
> > > > > -	return intel_gt_clock_interval_to_ns(perf->i915->ggtt.vm.gt,
> > > > > +	return intel_gt_clock_interval_to_ns(to_gt(perf->i915)->ggtt->vm.gt,
> > > >
> > > > Ditto; this looks like "to_gt(perf->i915)" might be all we need?
> > > 
> > > I think this function is looking for the GT coming from the VM,
> > > otherwise originally it could have taken it from &i915->gt. In my
> > > first version I proposed a wrapper around this but it was
> > > rejected by Lucas.
> > > 
> > > Besides, as we discussed earlier when I was proposed the static
> > > allocation, the ggtt might not always be linked to the same gt,
> > > so that I assumed that sometimes:
> > > 
> > >    to_gt(perf->i915)->ggtt->vm.gt != to_gt(perf->i915)
> > > 
> > > if two GTs are sharing the same ggtt, what would the ggtt->vm.gt
> > > link be?
> > 
> > From the git history, it doesn't look like this really needs to care
> > about the GGTT at all; I think it was just unintentionally written in a
> > roundabout manner when intel_gt was first being introduced in the code.
> > The reference here first showed up in commit f170523a7b8e ("drm/i915/gt:
> > Consolidate the CS timestamp clocks").
> > 
> > Actually the most correct thing to do is probably to use
> > 'stream->engine->gt' to ensure we grab the GT actually associated with
> > the stream's engine.
> > 
> 
> stream is not yet created at this point, so I would do this:
> 
> pass intel_gt to the helper instead of perf:
> static u64 oa_exponent_to_ns(struct intel_gt *gt, int exponent)
> {
> 	return intel_gt_clock_interval_to_ns(gt, 2ULL << exponent);
> }
> 
> caller would then be:
> oa_period = oa_exponent_to_ns(props->engine->gt, value);

thanks for the suggestions, but this is out of the scope of this
patch... I did propose a wrapper but it was rejected because it
was, indeed, out of scope.

I'm going to use to_gt(perf->i915) as Matt suggested originally,
patch is ready.

Thanks,
Andi