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 72B07C35FFC for ; Sat, 22 Mar 2025 21:06:14 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 15D2810E0FF; Sat, 22 Mar 2025 21:06:14 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="TNLEwWbN"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.20]) by gabe.freedesktop.org (Postfix) with ESMTPS id 7EF1610E0AC for ; Sat, 22 Mar 2025 21:06: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=1742677573; x=1774213573; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version:content-transfer-encoding; bh=ufVuj6tgFt0YytdVDi0sNJJu3s9iVTGITuTld1l501Q=; b=TNLEwWbNv9TAhrMTId9e9doWpshXHLtKXuA0i0zUa/aChdWeEqGaj/2V Yxjh1tpEmvTjaSooUZCkNpfJXXmH6YXj9/jqqMzIID0NcsbGBVEgS44lr p1rEMcoBdL9p3gAQt0Sx8g+cNySgUi7aMH3yGZfHrmB2zi6+M0R7jk0i+ tpnfGDAC+XPxGMPPDZ76kIFcz3JtV4iE2ml4Fq2l5hCOuyIcYbl6P+uWa H4i55tPu2QkEjWyAT6NsVC0tyJOZBQuC1eONXtUUh+0TvAkxfOC+EKDJh vNhJ70bqF44Sa54vCLRDhTkX3djAFR7Po6NQZfnoUOD8bSrulwx031zAO Q==; X-CSE-ConnectionGUID: MvLBhkZ5QAmcQx1rIWLBvw== X-CSE-MsgGUID: guI3LRGeSKikaEwAZfpUQw== X-IronPort-AV: E=McAfee;i="6700,10204,11381"; a="43644238" X-IronPort-AV: E=Sophos;i="6.14,268,1736841600"; d="scan'208";a="43644238" Received: from orviesa002.jf.intel.com ([10.64.159.142]) by orvoesa112.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Mar 2025 14:06:11 -0700 X-CSE-ConnectionGUID: vmm3snBzQeybYIkSh5MYlg== X-CSE-MsgGUID: Pf43fMsfTHy2gb5Pj/qXGA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.14,268,1736841600"; d="scan'208";a="154584036" Received: from bbeebe-mobl.amr.corp.intel.com (HELO adixit-MOBL3.intel.com) ([10.125.186.95]) by orviesa002-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Mar 2025 14:06:11 -0700 Date: Sat, 22 Mar 2025 14:06:09 -0700 Message-ID: <87o6xsai7y.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Lucas De Marchi Cc: "Belgaumkar, Vinay" , , Riana Tauro , Rodrigo Vivi Subject: Re: [PATCH v3] drm/xe/pmu: Add GT frequency events In-Reply-To: References: <20250312001408.804125-1-vinay.belgaumkar@intel.com> <4283fcec-5ddd-4d06-9eae-8e622e9695ee@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/29.4 (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 X-BeenThere: intel-xe@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel Xe graphics driver List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-xe-bounces@lists.freedesktop.org Sender: "Intel-xe" On Sat, 22 Mar 2025 05:57:12 -0700, Lucas De Marchi wrote: > > On Fri, Mar 21, 2025 at 03:45:21PM -0700, Belgaumkar, Vinay wrote: > > > > On 3/13/2025 2:45 PM, Lucas De Marchi wrote: > >> On Tue, Mar 11, 2025 at 05:14:08PM -0700, Vinay Belgaumkar wrote: > >>> Define PMU events for GT frequency (actual and requested). This is > >>> a port from the i915 driver implementation, where an internal timer > >>> is used to aggregate GT frequencies over certain fixed interval. > >>> Following PMU events are being added- > >> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 ^ > >> why do you use "-"=A0 instead of ":"? > >> > >>> > >>> =A0xe_0000_00_02.0/gt-actual-frequency/=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0 [Kernel PMU event] > >>> =A0xe_0000_00_02.0/gt-requested-frequency/=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0 [Kernel PMU event] > >>> > >>> Standard perf commands can be used to monitor GT frequency- > >>> =A0$ perf stat -e xe_0000_00_02.0/gt-requested-frequency,gt=3D0/ -I10= 00 > >>> > >>> =A0=A0=A0 1.001175175=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 70= 0 M xe/gt-requested-frequency,gt=3D0/ > >>> =A0=A0=A0 2.005891881=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 70= 3 M xe/gt-requested-frequency,gt=3D0/ > >>> =A0=A0=A0 3.007318169=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 70= 0 M xe/gt-requested-frequency,gt=3D0/ > >>> > >>> Actual frequencies will be reported as 0 when GT is in C6. > >> > >> > >> I think we need to document somewhere, but at the very least in the > >> commit message what the event count actually is. Let me see if I get > >> this right:=A0 if userspace is sampling every 1sec, and assuming the g= pu > >> is at 700MHz for the first 0.5sec and at 1.4 GHz, userspace should > >> expect to see ~1050 as the value. Correct?=A0 I find this frequency > >> handling very different from anything else reported via perf. Other > >> than i915, are there any other cases you know of? > > > > Yes,=A0 user will see a gradual ramp. One thing that could work is > > something along these lines: > > > > @@ -324,7 +327,10 @@ static void xe_pmu_event_update(struct perf_event > > *event) > > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 new =3D __xe_pmu_event_re= ad(event); > > =A0=A0=A0=A0=A0=A0=A0 } while (!local64_try_cmpxchg(&hwc->prev_count, &= prev, new)); > > > > -=A0=A0=A0=A0=A0=A0 local64_add(new - prev, &event->count); > > +=A0=A0=A0=A0=A0=A0 if (is_gt_frequency_event(id)) > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 local64_add(new, &event->co= unt); > > +=A0=A0=A0=A0=A0=A0 else > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 local64_add(new - prev, &ev= ent->count); > > > > This will give us instantaneous values and will not need the use of an > > internal timer.=A0 Should be ok to do it this way? > > yes, I think it'd be preferred. It's much simpler and don't prevent us > from eventually adding an avg_* event if the needs arise. Which would > also be clearer on what that is. Instantaneous values would show 0 if the sampling instant landed when gt is in C6. Also instantaneous values are already available via sysfs. Also, the way i915 calculates the average freq is itself controversial. i915 computes average freq when gt is awake and disregards those intervals when gt is in C6. There was this patch to suggest that average freq also take into account those intervals when gt is in C6: https://patchwork.freedesktop.org/series/109372/ But it was not accepted since it would change uapi.