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 C3E33CA0FE2 for ; Fri, 1 Sep 2023 00:18:56 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 783CF10E100; Fri, 1 Sep 2023 00:18:56 +0000 (UTC) Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTPS id 0DF3710E100 for ; Fri, 1 Sep 2023 00:18:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1693527535; x=1725063535; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version:content-transfer-encoding; bh=nd13E1IkrdO/4VuthhZkJU2845wzcrQjALHNglXLJK4=; b=RSZKzBTiC2a0A9rx03eIosVQP2mPHvUKj3znnOCjJujqLKjh5+zOiX2V Ig1ju9frT3HB+h4cQpkKGvPQS0zjtA5XUC98cZ9jEqK471IZJyl329IAi SQfhR/dV1nIooM0KNP4TLECijTq9DNl12uXeChOxW3Nz5jG9KnxTjIzKq A3SxUIKlZea+X9zdBi9gWkbezusCLrEP8rXsNmaoruZjwaHPZ3FMUAUSF 6x+xwa0exgm923a/82soFlvRiit5o6JwVWhCRA4FnrfUQmQWP8giaMbLy XSY1js8s9z+BLNMrGWmrdNOvplpx0u1HXXRjIaVkotOe+Z/JM2vvvc8Uy g==; X-IronPort-AV: E=McAfee;i="6600,9927,10819"; a="376036015" X-IronPort-AV: E=Sophos;i="6.02,218,1688454000"; d="scan'208";a="376036015" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 31 Aug 2023 17:18:54 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10819"; a="768996273" X-IronPort-AV: E=Sophos;i="6.02,218,1688454000"; d="scan'208";a="768996273" Received: from adixit-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.212.199.173]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 31 Aug 2023 17:18:53 -0700 Date: Thu, 31 Aug 2023 16:58:37 -0700 Message-ID: <871qfi228i.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: "Belgaumkar, Vinay" In-Reply-To: <2872a593-8c93-a7c4-331a-51dc946db581@intel.com> References: <20230830051544.369643-1-aravind.iddamsetty@linux.intel.com> <20230830051544.369643-4-aravind.iddamsetty@linux.intel.com> <87a5u724xe.wl-ashutosh.dixit@intel.com> <3b85e034-b1a4-29d1-2e4f-08ea6562b6ba@linux.intel.com> <877cpb173l.wl-ashutosh.dixit@intel.com> <496fa477-0361-5e65-e7a8-d3c0d02e114a@linux.intel.com> <77134894-47b2-98ce-d3e5-f30e611e03df@intel.com> <3c4ab23e-3b63-2f6f-a397-217f1818ead2@intel.com> <8734zy2471.wl-ashutosh.dixit@intel.com> <2872a593-8c93-a7c4-331a-51dc946db581@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.1 (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-xe] [PATCH v5 3/3] drm/xe/pmu: Enable PMU interface 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: , Cc: Bommu Krishnaiah , intel-xe@lists.freedesktop.org, Tvrtko Ursulin Errors-To: intel-xe-bounces@lists.freedesktop.org Sender: "Intel-xe" On Thu, 31 Aug 2023 16:57:25 -0700, Belgaumkar, Vinay wrote: > > On 8/31/2023 4:16 PM, Dixit, Ashutosh wrote: > > On Thu, 31 Aug 2023 16:22:10 -0700, Belgaumkar, Vinay wrote: > >>>>>>> +static void xe_pmu_event_read(struct perf_event *event) > >>>>>>> +{ > >>>>>>> +=A0=A0=A0 struct xe_device *xe =3D > >>>>>>> +=A0=A0=A0=A0=A0=A0=A0 container_of(event->pmu, typeof(*xe), pmu.= base); > >>>>>>> +=A0=A0=A0 struct hw_perf_event *hwc =3D &event->hw; > >>>>>>> +=A0=A0=A0 struct xe_pmu *pmu =3D &xe->pmu; > >>>>>>> +=A0=A0=A0 u64 prev, new; > >>>>>>> + > >>>>>>> +=A0=A0=A0 if (pmu->closed) { > >>>>>>> +=A0=A0=A0=A0=A0=A0=A0 event->hw.state =3D PERF_HES_STOPPED; > >>>>>>> +=A0=A0=A0=A0=A0=A0=A0 return; > >>>>>>> +=A0=A0=A0 } > >>>>>>> +again: > >>>>>>> +=A0=A0=A0 prev =3D local64_read(&hwc->prev_count); > >>>>>>> +=A0=A0=A0 new =3D __xe_pmu_event_read(event); > >>>>>>> + > >>>>>>> +=A0=A0=A0 if (local64_cmpxchg(&hwc->prev_count, prev, new) !=3D = prev) > >>>>>>> +=A0=A0=A0=A0=A0=A0=A0 goto again; > >>>>>>> + > >>>>>>> +=A0=A0=A0 local64_add(new - prev, &event->count); > >>>>>>> +} > >>>>>>> + > >>>>>>> +static void xe_pmu_enable(struct perf_event *event) > >>>>>>> +{ > >>>> The i915_pmu code checks which event is requested here and according= ly sets pmu->enable (which doesn't seem to be defined here yet). Any reason= we are not doing this yet? > >>> in i915 pmu->enable is only used by events for which there is an inte= rnal timer sampler > >>> which periodically samples those events, this series is not adding su= ch events. > >> Ok, the tracked events are bound by I915_NUM_PMU_SAMPLERS in i915, whe= reas > >> you have already defined the max non/tracking ones as > >> __XE_NUM_PMU_SAMPLERS, hence my confusion. Should we use a different n= ame > >> in lieu of the tracked events which will be defined in subsequent patc= hes? > >> > >> enum { > >> > >> =A0 =A0 =A0 =A0 __I915_SAMPLE_FREQ_ACT =3D 0, > >> > >> =A0=A0=A0=A0=A0=A0=A0 __I915_SAMPLE_FREQ_REQ, > >> =A0=A0=A0=A0=A0=A0=A0 __I915_SAMPLE_RC6, > >> =A0=A0=A0=A0=A0=A0=A0 __I915_SAMPLE_RC6_LAST_REPORTED, > >> =A0=A0=A0=A0=A0=A0=A0 __I915_NUM_PMU_SAMPLERS > >> > >> }; > > +enum { > > + __XE_SAMPLE_RENDER_GROUP_BUSY, > > + __XE_SAMPLE_COPY_GROUP_BUSY, > > + __XE_SAMPLE_MEDIA_GROUP_BUSY, > > + __XE_SAMPLE_ANY_ENGINE_GROUP_BUSY, > > + __XE_NUM_PMU_SAMPLERS > > +}; > > + > > > > I'd think any future events will be added after > > __XE_SAMPLE_ANY_ENGINE_GROUP_BUSY, changing the value of > > __XE_NUM_PMU_SAMPLERS, no? Why do we need a different name? > > I guess that'll work, but the events in i915 are divided into ones that > need the timer (non-engine) (max of I915_NUM_PMU_SAMPLERS) and the > per-engine events (that don't need the timer). i915 per engine events need timer. They are just stored in engine->pmu->sample[] array, not in i915->pmu->sample[][] array. __I915_NUM_PMU_SAMPLERS is the size of the 2nd array. > We now should have 3 types? > Non-engine-non-timer, non-engine-timer and per-engine? RC6 events in i915 are also non-engine-non-timer (same as this patch). So it's the same. __XE_NUM_PMU_SAMPLERS is just the size of the xe->pmu->sample[][] array. > Or just club together the non-engine ones? I'd say let's restrict here to reviewing this patch. Everything in this patch can be changed except the uapi, the implementation can be completely changed/enhanced by future patches. So we should just make sure the uapi should not change by future changes. Incidentally I am against exposing freq through the PMU since it is available through sysfs. But we can have that discussion when we get to it. Thanks. -- Ashutosh