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 7D981C77B75 for ; Tue, 16 May 2023 16:54:20 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id E985B10E27D; Tue, 16 May 2023 16:54:14 +0000 (UTC) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by gabe.freedesktop.org (Postfix) with ESMTPS id 3039910E27B; Tue, 16 May 2023 16:54:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1684256052; x=1715792052; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=ozjfJBHYBbAm5X+YYnlOyx+UUC5Y3vCqJBhWUKpfg+Q=; b=ak1FcqV+0uEJ/MV+2cfGgNHErlNs9TwEs19NMzz2bRddLWq/7/wZQEvI balWqY82EHPijiz9/EZYut8UrysC0quR56HpAVfteYGiYBQzozq2BBe5m 4Mnz43jq+T58HAmpN//Kk3mLme0/X8x4CxueieCbqt1vOu6MT7DRU/SDP MWMm3/FjuYOTBRzdDooih8+n7RhH9914VNWgV5ZQOJundt9YzGicm/c5i 2WNtpd3E3AmBHInYL0dgfsjF/Z29BC6Qm/vFC6yrwYWktfGq65I4v+WBh Ea8Z1nWoadFxb/sX8q8/oODu4srAe7HnEFjSARVwitDow4/PBoraqYaDZ A==; X-IronPort-AV: E=McAfee;i="6600,9927,10712"; a="331150377" X-IronPort-AV: E=Sophos;i="5.99,278,1677571200"; d="scan'208";a="331150377" Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 May 2023 09:54:11 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10712"; a="771129552" X-IronPort-AV: E=Sophos;i="5.99,278,1677571200"; d="scan'208";a="771129552" Received: from samie-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.212.213.20]) by fmsmga004-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 May 2023 09:54:10 -0700 Date: Tue, 16 May 2023 09:54:01 -0700 Message-ID: <87edngp68m.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Tvrtko Ursulin In-Reply-To: <20230516092445.184823-1-tvrtko.ursulin@linux.intel.com> References: <20230516092445.184823-1-tvrtko.ursulin@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=US-ASCII Subject: Re: [Intel-gfx] [PATCH] 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, dri-devel@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Tue, 16 May 2023 02:24:45 -0700, Tvrtko Ursulin wrote: > > 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 overflow > in the future. > > Signed-off-by: Tvrtko Ursulin > Cc: Ashutosh Dixit > Cc: Umesh Nerlige Ramappa > --- > I am not entirely sure the __builtin_constant_p->BUILD_BUG_ON branch will > work with all compilers. Lets see... > > Compile tested only. > --- > drivers/gpu/drm/i915/i915_pmu.c | 32 ++++++++++++++++++++++---------- > 1 file changed, 22 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c > index 7ece883a7d95..8736b3418f88 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) > return (event->attr.config >> I915_PMU_SAMPLE_BITS) & 0xff; > } > > -static bool is_engine_config(u64 config) > +static bool is_engine_config(const u64 config) > { > return config < __I915_PMU_OTHER(0); > } > @@ -82,15 +82,28 @@ static unsigned int other_bit(const u64 config) > > static unsigned int config_bit(const u64 config) > { > + unsigned int bit; > + > if (is_engine_config(config)) > - return engine_config_sample(config); > + bit = engine_config_sample(config); > else > - return other_bit(config); > + bit = other_bit(config); > + > + if (__builtin_constant_p(config)) > + BUILD_BUG_ON(bit > > + BITS_PER_TYPE(typeof_member(struct i915_pmu, > + enable)) - 1); Given that config comes from the event (it is event->attr.config), can this ever be a builtin constant? > + else > + WARN_ON_ONCE(bit > > + BITS_PER_TYPE(typeof_member(struct i915_pmu, > + 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. So this patch is: Reviewed-by: Ashutosh Dixit > + > + return bit; > } > > -static u64 config_mask(u64 config) > +static u32 config_mask(const u64 config) > { > - return BIT_ULL(config_bit(config)); > + return BIT(config_bit(config)); > } > > static bool is_engine_event(struct perf_event *event) > @@ -633,11 +646,10 @@ static void i915_pmu_enable(struct perf_event *event) > { > struct drm_i915_private *i915 = > container_of(event->pmu, typeof(*i915), pmu.base); > + const unsigned int bit = event_bit(event); > struct i915_pmu *pmu = &i915->pmu; > unsigned long flags; > - unsigned int bit; > > - bit = event_bit(event); > if (bit == -1) > goto update; > > @@ -651,7 +663,7 @@ static void i915_pmu_enable(struct perf_event *event) > GEM_BUG_ON(bit >= ARRAY_SIZE(pmu->enable_count)); > GEM_BUG_ON(pmu->enable_count[bit] == ~0); > > - pmu->enable |= BIT_ULL(bit); > + pmu->enable |= BIT(bit); > pmu->enable_count[bit]++; > > /* > @@ -698,7 +710,7 @@ static void i915_pmu_disable(struct perf_event *event) > { > struct drm_i915_private *i915 = > container_of(event->pmu, typeof(*i915), pmu.base); > - unsigned int bit = event_bit(event); > + const unsigned int bit = event_bit(event); > struct i915_pmu *pmu = &i915->pmu; > unsigned long flags; > > @@ -734,7 +746,7 @@ static void i915_pmu_disable(struct perf_event *event) > * bitmask when the last listener on an event goes away. > */ > if (--pmu->enable_count[bit] == 0) { > - pmu->enable &= ~BIT_ULL(bit); > + pmu->enable &= ~BIT(bit); > pmu->timer_enabled &= pmu_needs_timer(pmu, true); > } > > -- > 2.39.2 >