From: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
To: Matt Roper <matthew.d.roper@intel.com>
Cc: "Sujaritha Sundaresan" <sujaritha.sundaresan@intel.com>,
"Andi Shyti" <andi@etezian.org>,
"Intel GFX" <intel-gfx@lists.freedesktop.org>,
"Lucas De Marchi" <lucas.demarchi@intel.com>,
"DRI Devel" <dri-devel@lists.freedesktop.org>,
"Chris Wilson" <chris@chris-wilson.co.uk>,
"Andi Shyti" <andi.shyti@linux.intel.com>,
"Michał Winiarski" <michal.winiarski@intel.com>
Subject: Re: [PATCH v9 2/6] drm/i915: Use to_gt() helper for GGTT accesses
Date: Tue, 4 Jan 2022 09:52:31 -0800 [thread overview]
Message-ID: <20220104175231.GA12985@orsosgc001.ra.intel.com> (raw)
In-Reply-To: <YdNn1uOzEwwnm91N@mdroper-desk1.amr.corp.intel.com>
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
next prev parent reply other threads:[~2022-01-04 17:52 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-19 21:24 [PATCH v9 0/6] More preparation for multi gt patches Andi Shyti
2021-12-19 21:24 ` [PATCH v9 1/6] drm/i915/gt: Use to_gt() helper for GGTT accesses Andi Shyti
2021-12-21 16:47 ` Matt Roper
2021-12-21 19:59 ` [PATCH v10 " Andi Shyti
2021-12-19 21:24 ` [PATCH v9 2/6] drm/i915: " Andi Shyti
2021-12-21 17:01 ` Matt Roper
2021-12-21 19:46 ` Andi Shyti
2022-01-03 21:17 ` Matt Roper
2022-01-04 17:52 ` Umesh Nerlige Ramappa [this message]
2022-01-04 22:27 ` Andi Shyti
2022-01-04 22:35 ` [PATCH v10 " Andi Shyti
2022-01-05 5:31 ` Matt Roper
2021-12-19 21:24 ` [PATCH v9 3/6] drm/i915/gem: " Andi Shyti
2021-12-19 21:24 ` [PATCH v9 4/6] drm/i915/display: " Andi Shyti
2021-12-19 21:24 ` [PATCH v9 5/6] drm/i915/selftests: " Andi Shyti
2021-12-19 21:25 ` [PATCH v9 6/6] drm/i915: Remove unused i915->ggtt Andi Shyti
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220104175231.GA12985@orsosgc001.ra.intel.com \
--to=umesh.nerlige.ramappa@intel.com \
--cc=andi.shyti@linux.intel.com \
--cc=andi@etezian.org \
--cc=chris@chris-wilson.co.uk \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=lucas.demarchi@intel.com \
--cc=matthew.d.roper@intel.com \
--cc=michal.winiarski@intel.com \
--cc=sujaritha.sundaresan@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).