On Mon, Jan 03, 2022 at 01:17:10PM -0800, Matt Roper wrote:
On Tue, Dec 21, 2021 at 09:46:29PM +0200, Andi Shyti wrote:
Hi Matt,
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > index 170bba913c30..128315aec517 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -1630,7 +1630,7 @@ static int alloc_noa_wait(struct i915_perf_stream *stream)
> > 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,
Umesh
Matt
Thanks,
Andi
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
