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 4BC80C77B75 for ; Fri, 19 May 2023 05:02:25 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 6222F10E060; Fri, 19 May 2023 05:02:24 +0000 (UTC) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTPS id 73A2D10E060 for ; Fri, 19 May 2023 05:02:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1684472542; x=1716008542; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version:content-transfer-encoding; bh=dseCMfZ2zix4pmKZeav4v04nccYCar8Vy2/ld0Vza+A=; b=NCFacxeELke+Q9Jdd72dzdZJxjBaNwfmrUM6Ve5/fWCo6dZekr+kfERF hKkDTstBMWRZLNMfs1JsT8RmBXy+aY1iq5KcOTCCiz0tcqtkROeEgzo+d WG+2cF3YQn6FYPFlm8GIHw/ZmNr5wukvN3lBY8+lkj+UOV0zCjxjmNtlA iAObJ59eyLGhQbOHmZfBFZRGOqswI6pz3X+EWp9J+R9XrN1rcKrLeM72E Tm2wyZuOkIjGa3+JFbPSB3y9kB44sewnFHgDtMe8McztAqRq1VhJJ005q wF0YJEcpe1lyOXXY5BtfZDyhou1lKF1/hFtfuHab677U+7QA1BHqmnCvP w==; X-IronPort-AV: E=McAfee;i="6600,9927,10714"; a="341718497" X-IronPort-AV: E=Sophos;i="6.00,175,1681196400"; d="scan'208";a="341718497" Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 May 2023 22:02:06 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10714"; a="652912051" X-IronPort-AV: E=Sophos;i="6.00,175,1681196400"; d="scan'208";a="652912051" Received: from adixit-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.212.159.158]) by orsmga003-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 May 2023 22:02:06 -0700 Date: Thu, 18 May 2023 22:02:05 -0700 Message-ID: <874jo8x6b6.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Tvrtko Ursulin In-Reply-To: <1f6f00c0-a362-454c-53a3-740b559cdb88@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> <87ednf3oyo.wl-ashutosh.dixit@intel.com> <1f6f00c0-a362-454c-53a3-740b559cdb88@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 Thu, 18 May 2023 02:07:55 -0700, Tvrtko Ursulin wrote: > > On 17/05/2023 17:25, Dixit, Ashutosh wrote: > > 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 en= ough. > >> > >>>> > >>>>> 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 over= flow > >>>>> 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) & = 0xff; > >>>>> =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 c= ode, > >>> 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 ha= ve to > >>>> go. Maybe the code could even have stayed in config_bit with the che= ck. > >>>> > >>>>> + > >>>>> +=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(typ= eof_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), c= an > >>>> 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 com= pile > >> 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 j= ust > > 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. T= hat > >> 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. > > Latest series looks fine to me and thanks for your patience. Hope you wou= ld > agree changing that one thing to u32 made more sense than changing the > other to u64 so bike shed wasn't for nothing. Hi Tvrtko, yes definitely, no issues :) Thanks for your patience too. -- Ashutosh