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 0D4BDC77B7A for ; Fri, 2 Jun 2023 03:28:08 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 40C3110E60F; Fri, 2 Jun 2023 03:28:08 +0000 (UTC) Received: from mga06.intel.com (mga06b.intel.com [134.134.136.31]) by gabe.freedesktop.org (Postfix) with ESMTPS id C6E9310E60F for ; Fri, 2 Jun 2023 03:28:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1685676485; x=1717212485; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=uK0fFkApQxrI3c+0Z0mc1LQoaXqo2MC1I4vPDT3Ub2I=; b=kjIrRRo1Yn+QUdNu0q/YlyovrUiBzHmwzNNKo/CIW0qmDqutoYinE/Jy 964otzeJBM/+fyunNQI2uOfNVhu+MmQAvJ8vS7KOo/5ZllHImI//vtTgS Bm0XlK4uuz3oXgiaSADH7JQKnECsqkHC9haLhSbwD7FHQioK0ABOfaWXP IokslFg1G+dSbnmA/mUyxHAaLqaHIHkMgG9KwInw3K1Zro93qgXgTaCDP 8Mo8gQJ1jFR+vu5IMLJjYdvLn+a9N12w082tQuNeIbjUBKHqHGN23/wD2 oYySZqBbaS1Hed0ayvL6lbUF83+82dwTqBaf+SeB+46BGMwnFpUDRuTXT Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10728"; a="419283845" X-IronPort-AV: E=Sophos;i="6.00,211,1681196400"; d="scan'208";a="419283845" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Jun 2023 20:28:04 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10728"; a="737365494" X-IronPort-AV: E=Sophos;i="6.00,211,1681196400"; d="scan'208";a="737365494" Received: from adixit-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.209.85.141]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Jun 2023 20:28:04 -0700 Date: Thu, 01 Jun 2023 20:21:53 -0700 Message-ID: <87pm6elfam.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Andi Shyti In-Reply-To: References: <20230531213547.1525692-1-matthew.s.atwood@intel.com> <87wn0nujol.wl-ashutosh.dixit@intel.com> <87v8g7ujaj.wl-ashutosh.dixit@intel.com> <87ttvqvhj7.wl-ashutosh.dixit@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: sync I915_PMU_MAX_GTS to I915_MAX_GT 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, 01 Jun 2023 17:40:18 -0700, Andi Shyti wrote: > Hi Andi, > On Thu, Jun 01, 2023 at 05:23:24PM -0700, Dixit, Ashutosh wrote: > > On Thu, 01 Jun 2023 11:30:44 -0700, Dixit, Ashutosh wrote: > > > > > > On Thu, 01 Jun 2023 11:22:18 -0700, Dixit, Ashutosh wrote: > > > > > > > > On Wed, 31 May 2023 14:35:47 -0700, Matt Atwood wrote: > > > > > > > > > > > > > Hi Matt, > > > > > > > > > Set I915_PMU_MAX_GTS to value in I915_MAX_GT, theres no reason for these > > > > > values to be different. > > > > > > Also, we can't be so sure so as to be able to say "theres no reason for > > > these values to be different" till we have actually verified it. E.g. there > > > are various bitfields in the code which might not fit in a u32 if we > > > increase MAX_GT from 2 to 4. Has this been verified? > > > > > > If anything, to keep the code from doing unnecessary stuff, IMO I915_MAX_GT > > > should be reduced to 2 and should be increased to 4 only once/if we have > > > i915 supported platforms with 4 GT's. > > > > Matt explained the issue offline to me (it would have helped to explain the > > reason for the patch in the commit message). The issue is that in uses of > > for_each_gt such as below (there are others too in the PMU code): > > > > for_each_gt(gt, i915, i) { > > intel_wakeref_t wakeref; > > > > with_intel_runtime_pm(gt->uncore->rpm, wakeref) { > > u64 val = __get_rc6(gt); > > > > store_sample(pmu, i, __I915_SAMPLE_RC6, val); > > store_sample(pmu, i, __I915_SAMPLE_RC6_LAST_REPORTED, > > val); > > pmu->sleep_last[i] = ktime_get_raw(); > > } > > } > > > > static checkers are complaining that for_each_gt can read/write outside the > > bounds of PMU arrays. Because absent gt's will be NULL in for_each_gt this > > cannot really happen but we still need to keep static checkers happy. > > > > So to resolve this issue we need I915_PMU_MAX_GTS and I915_MAX_GT to have > > the same value. So either we need to increase I915_PMU_MAX_GTS to 4 or > > reduce I915_MAX_GT to 2. > > the number of GT's is a GPU concept and should remain as such all > over the GPU. If max GT is 4 then it should be 4 everywhere. > > The I915_PMU_MAX_GTS define should not exist at all as it is > creating this sort of inconsistencies and everything should refer > to a single I915_MAX_GT. The reason for having I915_PMU_MAX_GTS, > in a first place, is purely practical to avoid over inclusions. > Still I consider it hacky. > > On the other had, already I915_MAX_GT is a hardcoded value and > many times there have been discussions about removing it and > fetch it dynamically during the i915 boot. But this requires > quite a good amount of refactoring that no one is willing to do. > > If we can't get rid of I915_PMU_MAX_GTS then I strongly believe > it should be aligned with I915_MAX_GT and for this reason I gave > my r-b. The use of for_each_gt() is a clear consequence of this > difference. Yes, not disagreeing. At this point I think my preferred solution is something like: #define I915_MAX_GT 2 #define I915_PMU_MAX_GTS I915_MAX_GT Unless someone can explain why I915_MAX_GT cannot be 2. As I see it, there's no need for I915_MAX_GT to be 4 after xehpsdv disappeared and support for future platforms is moving to xe. Thanks. -- Ashutosh