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 1B828C77B7F for ; Wed, 17 May 2023 16:32:11 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id ACFC210E443; Wed, 17 May 2023 16:32:10 +0000 (UTC) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTPS id 1FE4710E443 for ; Wed, 17 May 2023 16:32:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1684341129; x=1715877129; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version:content-transfer-encoding; bh=tqB3h+DZAN4K2K8kWzYjYCpIWD/4AVW7GbagMboMQoo=; b=WWgS7Y/b5vhpDOwvhDA20gUbUBf87QxVP/VpcsIVofko8/M25FKz6s+j I8xgSz1HxCZCRM3tNeMAMH/roxJZzA7WWe4tdZ01siyt/oZuKLWmdBFh0 0EqCaCPU01jHZkS8RjvvazUtKtikm4PAmWuW2EjoBWQ6gGs2rO7K61EJ2 8I3DGwjRRzZsxRI7Hqvn24GCj2LMQVIURsB0ahXfAMi3zu2+vU2Zh8Wsm rA/cs3pKRYM0x0U77F07oldDNmkjUepTNo6sXnrIs/ExqOP1Pe7bgZbgi F0Y98sxcF2I+kNyG4I66BtW3LVeJpQ4oKwc2btyXBOX9N+Eh7pWjOh8I3 Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10713"; a="351829807" X-IronPort-AV: E=Sophos;i="5.99,282,1677571200"; d="scan'208";a="351829807" Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 May 2023 09:32:08 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10713"; a="948330680" X-IronPort-AV: E=Sophos;i="5.99,282,1677571200"; d="scan'208";a="948330680" Received: from adixit-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.251.19.98]) by fmsmga006-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 May 2023 09:32:07 -0700 Date: Wed, 17 May 2023 09:25:03 -0700 Message-ID: <87ednf3oyo.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Tvrtko Ursulin In-Reply-To: <0a1babb7-80cf-cfe7-4746-37b76934175a@linux.intel.com> References: <20230516233534.3610598-1-umesh.nerlige.ramappa@intel.com> <20230516233534.3610598-2-umesh.nerlige.ramappa@intel.com> <87cz2zpzw1.wl-ashutosh.dixit@intel.com> <0a1babb7-80cf-cfe7-4746-37b76934175a@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 v5 1/7] drm/i915/pmu: Change bitmask of enabled events to u32 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 Wed, 17 May 2023 01:26:15 -0700, Tvrtko Ursulin wrote: > > > On 17/05/2023 07:55, Umesh Nerlige Ramappa wrote: > > On Tue, May 16, 2023 at 05:25:50PM -0700, Dixit, Ashutosh wrote: > >> On Tue, 16 May 2023 16:35:28 -0700, Umesh Nerlige Ramappa wrote: > >>> > >> > >> Hi Umesh/Tvrtko, > >> > >> Mostly repeating comments/questions made on the previous patch below. > > First of all thanks for improving this, my v1 obviously wasn't good enoug= h. > > >> > >>> From: Tvrtko Ursulin > >>> > >>> Having it as u64 was a confusing (but harmless) mistake. > >>> > >>> Also add some asserts to make sure the internal field does not overfl= ow > >>> in the future. > >>> > >>> v2: Fix WARN_ON firing for INTERRUPT event (Umesh) > >>> > >>> Signed-off-by: Tvrtko Ursulin > >>> Signed-off-by: Umesh Nerlige Ramappa > >>> Cc: Ashutosh Dixit > >>> --- > >>> =A0drivers/gpu/drm/i915/i915_pmu.c | 26 ++++++++++++++++++-------- > >>> =A01 file changed, 18 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c > >>> b/drivers/gpu/drm/i915/i915_pmu.c > >>> index 7ece883a7d95..96543dce2db1 100644 > >>> --- a/drivers/gpu/drm/i915/i915_pmu.c > >>> +++ b/drivers/gpu/drm/i915/i915_pmu.c > >>> @@ -50,7 +50,7 @@ static u8 engine_event_instance(struct perf_event > >>> *event) > >>> =A0=A0=A0=A0return (event->attr.config >> I915_PMU_SAMPLE_BITS) & 0xf= f; > >>> =A0} > >>> > >>> -static bool is_engine_config(u64 config) > >>> +static bool is_engine_config(const u64 config) > >>> =A0{ > >>> =A0=A0=A0=A0return config < __I915_PMU_OTHER(0); > >>> =A0} > >>> @@ -88,9 +88,20 @@ static unsigned int config_bit(const u64 config) > >>> =A0=A0=A0=A0=A0=A0=A0 return other_bit(config); > >>> =A0} > >>> > >>> -static u64 config_mask(u64 config) > >>> +static u32 config_mask(const u64 config) > >>> =A0{ > >>> -=A0=A0=A0 return BIT_ULL(config_bit(config)); > >>> +=A0=A0=A0 unsigned int bit =3D config_bit(config); > >> > >> Give that config_bit() can return -1 (I understand it is avoided in > >> moving > >> the code to config_mask from config_bit), maybe the code below should > >> also > >> have that check? > > > > config_mask is only called to check frequency related events in the cod= e, > > so I don't see it returing -1 here. > > Yeah that should be fine since -1 would make the below asserts fire > anyway. (If it would get called from a different path in the future.) > > >> > >> =A0=A0=A0=A0int bit =3D config_bit(config); > >> > >> =A0=A0=A0=A0if (bit !=3D -1) > >> =A0=A0=A0=A0{ > >> =A0=A0=A0=A0=A0=A0=A0 ... > >> =A0=A0=A0=A0} > >> > >> Though as mentioned below the 'if (__builtin_constant_p())' would have= to > >> go. Maybe the code could even have stayed in config_bit with the check. > >> > >>> + > >>> +=A0=A0=A0 if (__builtin_constant_p(config)) > >>> +=A0=A0=A0=A0=A0=A0=A0 BUILD_BUG_ON(bit > > >>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 BITS_PER_TYPE(typeo= f_member(struct i915_pmu, > >>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0 enable)) - 1); > >> > >> Given that config comes from the event (it is event->attr.config), can > >> this > >> ever be a builtin constant? > > > > Not sure about earlier code where these checks were inside config_bit(), > > but with changes I made, I don't see this being a builtin > > constant. However, nothing prevents a caller from just passing a > > builtin_constant to this in future. > > Are you sure? I would have thought it would always be a compile time > constant now that the check is in config_mask. Aahhh.. with the multi-tile > changes maybe it can't unroll the loops and calculate the masks at compile > time. Maybe it is a bit too much and we should drop the > __builtin_constant_p branch? Probably.. Ah yes, with the code move to config_mask, they really all are compile time constants (provided compiler can unroll the loops) so at least that is the justfication for leaving the __builtin_constant_p in. So I'd probably just leave it as is (though it is a bit too much). > But I guess it is safe to use GEM_WARN_ON_ONCE instead of WARN_ON_ONCE > since there are no external callers (nothing coming from event) now. That > way at least production builds don't have to have the check. Hmm, there's a GEM_WARN_ON but no GEM_WARN_ON_ONCE. So leave that as is too I guess. So I'm ok with the code staying as is. Enough bike-shed on this already. Thanks. -- Ashutosh > > Regards, > > Tvrtko > > > > > Thanks, > > Umesh > > > >> > >>> +=A0=A0=A0 else > >>> +=A0=A0=A0=A0=A0=A0=A0 WARN_ON_ONCE(bit > > >>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 BITS_PER_TYPE(typeo= f_member(struct i915_pmu, > >>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0 enable)) - 1); > >> > >> There is really an even stricter limit on what the bit can be, which is > >> the > >> total number of possible events but anyway this is good enough. > >> > >> After addressing the above, this patch is: > >> > >> Reviewed-by: Ashutosh Dixit > >> > >>> + > >>> +=A0=A0=A0 return BIT(config_bit(config)); > >>> =A0} > >>> > >>> =A0static bool is_engine_event(struct perf_event *event) > >>> @@ -633,11 +644,10 @@ static void i915_pmu_enable(struct perf_event > >>> *event) > >>> =A0{ > >>> =A0=A0=A0=A0struct drm_i915_private *i915 =3D > >>> =A0=A0=A0=A0=A0=A0=A0 container_of(event->pmu, typeof(*i915), pmu.bas= e); > >>> +=A0=A0=A0 const unsigned int bit =3D event_bit(event); > >>> =A0=A0=A0=A0struct i915_pmu *pmu =3D &i915->pmu; > >>> =A0=A0=A0=A0unsigned long flags; > >>> -=A0=A0=A0 unsigned int bit; > >>> > >>> -=A0=A0=A0 bit =3D event_bit(event); > >>> =A0=A0=A0=A0if (bit =3D=3D -1) > >>> =A0=A0=A0=A0=A0=A0=A0 goto update; > >>> > >>> @@ -651,7 +661,7 @@ static void i915_pmu_enable(struct perf_event > >>> *event) > >>> =A0=A0=A0=A0GEM_BUG_ON(bit >=3D ARRAY_SIZE(pmu->enable_count)); > >>> =A0=A0=A0=A0GEM_BUG_ON(pmu->enable_count[bit] =3D=3D ~0); > >>> > >>> -=A0=A0=A0 pmu->enable |=3D BIT_ULL(bit); > >>> +=A0=A0=A0 pmu->enable |=3D BIT(bit); > >>> =A0=A0=A0=A0pmu->enable_count[bit]++; > >>> > >>> =A0=A0=A0=A0/* > >>> @@ -698,7 +708,7 @@ static void i915_pmu_disable(struct perf_event > >>> *event) > >>> =A0{ > >>> =A0=A0=A0=A0struct drm_i915_private *i915 =3D > >>> =A0=A0=A0=A0=A0=A0=A0 container_of(event->pmu, typeof(*i915), pmu.bas= e); > >>> -=A0=A0=A0 unsigned int bit =3D event_bit(event); > >>> +=A0=A0=A0 const unsigned int bit =3D event_bit(event); > >>> =A0=A0=A0=A0struct i915_pmu *pmu =3D &i915->pmu; > >>> =A0=A0=A0=A0unsigned long flags; > >>> > >>> @@ -734,7 +744,7 @@ static void i915_pmu_disable(struct perf_event > >>> *event) > >>> =A0=A0=A0=A0 * bitmask when the last listener on an event goes away. > >>> =A0=A0=A0=A0 */ > >>> =A0=A0=A0=A0if (--pmu->enable_count[bit] =3D=3D 0) { > >>> -=A0=A0=A0=A0=A0=A0=A0 pmu->enable &=3D ~BIT_ULL(bit); > >>> +=A0=A0=A0=A0=A0=A0=A0 pmu->enable &=3D ~BIT(bit); > >>> =A0=A0=A0=A0=A0=A0=A0 pmu->timer_enabled &=3D pmu_needs_timer(pmu, tr= ue); > >>> =A0=A0=A0=A0} > >>> > >>> -- > >>> 2.36.1 > >>>