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