From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 30EA3C77B7D for ; Mon, 15 May 2023 21:24:58 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id C65B610E008; Mon, 15 May 2023 21:24:57 +0000 (UTC) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTPS id 0CBF010E008 for ; Mon, 15 May 2023 21:24:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1684185897; x=1715721897; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version:content-transfer-encoding; bh=hFT8XaPvip+dwYJgdJHhIvRlvF598bk//xu2aqbhLJ8=; b=RFcpNSvelD1FNWZyQ9MtU1cgInqttYptzLokZ30pUzJwZjkIClG79rFU UiNy2AtdNDTtCxxdlkyq+lPK2TPodUVw6uh8Rw+KZAVh2Y3eS8d0hAY5u JbHCQyviNfJ2dEhaEcLlK50euBvIUJS3Qmii+sUv2VZINdj8vJHAqFvrg QfZStJbuITkNglEywoBhGZNnj9E1DuDSgqjHVKy6ZDaVsqUUpar6j8xJG 9lfrzDHq/XPB51T4hOVXbRwKqYx69aOdaBy26nv7Dlx4n1TNYsiWndg/s tihruhG2qbrBsA2vmw1Ol4sUDztsyJ+vYIoeTm0wHwV8eBDhFODRaisN5 A==; X-IronPort-AV: E=McAfee;i="6600,9927,10711"; a="351348091" X-IronPort-AV: E=Sophos;i="5.99,277,1677571200"; d="scan'208";a="351348091" Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 May 2023 14:24:56 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10711"; a="770746298" X-IronPort-AV: E=Sophos;i="5.99,277,1677571200"; d="scan'208";a="770746298" Received: from adixit-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.212.225.207]) by fmsmga004-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 May 2023 14:24:55 -0700 Date: Mon, 15 May 2023 14:24:11 -0700 Message-ID: <87lehpp9tw.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Tvrtko Ursulin In-Reply-To: <1aabf497-81e8-d3b6-1547-251324562494@linux.intel.com> References: <20230506005816.1891043-1-umesh.nerlige.ramappa@intel.com> <20230506005816.1891043-5-umesh.nerlige.ramappa@intel.com> <87ilcxdw0g.wl-ashutosh.dixit@intel.com> <87h6shdtn0.wl-ashutosh.dixit@intel.com> <1aabf497-81e8-d3b6-1547-251324562494@linux.intel.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Intel-gfx] [PATCH 4/6] drm/i915/pmu: Add reference counting to the sampling timer X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: intel-gfx@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Mon, 15 May 2023 02:52:35 -0700, Tvrtko Ursulin wrote: > > On 13/05/2023 00:44, Umesh Nerlige Ramappa wrote: > > On Fri, May 12, 2023 at 04:20:19PM -0700, Dixit, Ashutosh wrote: > >> On Fri, 12 May 2023 15:44:00 -0700, Umesh Nerlige Ramappa wrote: > >>> > >>> On Fri, May 12, 2023 at 03:29:03PM -0700, Dixit, Ashutosh wrote: > >>> > On Fri, 05 May 2023 17:58:14 -0700, Umesh Nerlige Ramappa wrote: > >>> >> > >>> > > >>> > Hi Umesh/Tvrtko, > >>> > > >>> >> From: Tvrtko Ursulin > >>> >> > >>> >> We do not want to have timers per tile and waste CPU cycles and > >>> energy via > >>> >> multiple wake-up sources, for a relatively un-important task of PMU > >>> >> sampling, so keeping a single timer works well. But we also do not > >>> want > >>> >> the first GT which goes idle to turn off the timer. > >>> >> > >>> >> Add some reference counting, via a mask of unparked GTs, to solve > >>> this. > >>> >> > >>> >> Signed-off-by: Tvrtko Ursulin > >>> >> Signed-off-by: Umesh Nerlige Ramappa > >>> > >>> >> --- > >>> >>=A0 drivers/gpu/drm/i915/i915_pmu.c | 12 ++++++++++-- > >>> >>=A0 drivers/gpu/drm/i915/i915_pmu.h |=A0 4 ++++ > >>> >>=A0 2 files changed, 14 insertions(+), 2 deletions(-) > >>> >> > >>> >> diff --git a/drivers/gpu/drm/i915/i915_pmu.c > >>> b/drivers/gpu/drm/i915/i915_pmu.c > >>> >> index 2b63ee31e1b3..669a42e44082 100644 > >>> >> --- a/drivers/gpu/drm/i915/i915_pmu.c > >>> >> +++ b/drivers/gpu/drm/i915/i915_pmu.c > >>> >> @@ -251,7 +251,9 @@ void i915_pmu_gt_parked(struct intel_gt *gt) > >>> >>=A0=A0=A0=A0 * Signal sampling timer to stop if only engine events = are > >>> enabled and > >>> >>=A0=A0=A0=A0 * GPU went idle. > >>> >>=A0=A0=A0=A0 */ > >>> >> -=A0=A0=A0 pmu->timer_enabled =3D pmu_needs_timer(pmu, false); > >>> >> +=A0=A0=A0 pmu->unparked &=3D ~BIT(gt->info.id); > >>> >> +=A0=A0=A0 if (pmu->unparked =3D=3D 0) > >>> >> +=A0=A0=A0=A0=A0=A0=A0 pmu->timer_enabled =3D pmu_needs_timer(pmu,= false); > >>> >> > >>> >>=A0=A0=A0 spin_unlock_irq(&pmu->lock); > >>> >>=A0 } > >>> >> @@ -268,7 +270,10 @@ void i915_pmu_gt_unparked(struct intel_gt *gt) > >>> >>=A0=A0=A0 /* > >>> >>=A0=A0=A0=A0 * Re-enable sampling timer when GPU goes active. > >>> >>=A0=A0=A0=A0 */ > >>> >> -=A0=A0=A0 __i915_pmu_maybe_start_timer(pmu); > >>> >> +=A0=A0=A0 if (pmu->unparked =3D=3D 0) > >>> >> +=A0=A0=A0=A0=A0=A0=A0 __i915_pmu_maybe_start_timer(pmu); > >>> >> + > >>> >> +=A0=A0=A0 pmu->unparked |=3D BIT(gt->info.id); > >>> >> > >>> >>=A0=A0=A0 spin_unlock_irq(&pmu->lock); > >>> >>=A0 } > >>> >> @@ -438,6 +443,9 @@ static enum hrtimer_restart i915_sample(struct > >>> hrtimer *hrtimer) > >>> >>=A0=A0=A0=A0 */ > >>> >> > >>> >>=A0=A0=A0 for_each_gt(gt, i915, i) { > >>> >> +=A0=A0=A0=A0=A0=A0=A0 if (!(pmu->unparked & BIT(i))) > >>> >> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 continue; > >>> >> + > >>> > > >>> > This is not correct. In this series we are at least sampling > >>> frequencies > >>> > (calling frequency_sample) even when GT is parked. So these 3 lines > >>> should be > >>> > deleted. engines_sample will get called and will return without doi= ng > >>> > anything if engine events are disabled. > >>> > >>> Not sure I understand. This is checking pmu->'un'parked bits. > >> > >> Sorry, my bad. Not "engines_sample will get called and will return > >> without > >> doing anything if engine events are disabled" but "engines_sample will > >> get > >> called and will return without doing anything if GT is not awake". This > >> is > >> the same as the previous behavior before this series. > >> > >> Umesh and I discussed this but writing this out in case Tvrtko takes a > >> look. > > > > Sounds good, Dropping the check here in the new revision. Hi Tvrtko, > I think it is safe to not have the check, but I didn't quite understand t= he > "this is not correct" part. I can only see the argument that it could be > redundant, not that it is incorrect. I said that it looks incorrect to me because in this series we are still sampling freq when gt is parked and we would be skipping that if we included: if (!(pmu->unparked & BIT(i))) continue; > In which case I think it should better stay since it is way more efficien= t, > given this gets called at 200Hz, than the *atomic* atomic_inc_not_zero > (from intel_wakeref_get_if_active). Where efficiency goes, when we merge the patch below (I have a v2 based on your suggestion but I am waiting till Umesh's series gets merged): https://patchwork.freedesktop.org/series/117658/ this will turn off the timer itself which will be even more efficient. Rather than use the above code where the timer is running and then we skip. So after the link above is merged the above code will be truly redundant. That was a second reason why I said delete it. Thanks. -- Ashutosh