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 DB18FC4332F for ; Wed, 19 Oct 2022 05:23:37 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 529EB10F140; Wed, 19 Oct 2022 05:23:34 +0000 (UTC) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by gabe.freedesktop.org (Postfix) with ESMTPS id E076C10F12C; Wed, 19 Oct 2022 05:23: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=1666157002; x=1697693002; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=O3VwPsk3iMrGn7WIMcPsBTELGmgq6647ss+sbK7+etg=; b=iiKx8SMtLlXZ329UVCLDt5nyv413VjxbwUBdHAAMXg8PMXUVEppu7c0E nOFWXBgrwOtIIpsidd267LWRyCisDK2JAxvdFl0jIl2L8MXBpEPoerf3M VMDk/WPel5909CNWH1bCK8guD9BwGxCcQiY4AAzzofMTjZzicyst31oEj mM1lrJaR/7sVGmHgyZu/yKQQOOuVdY1yF9SEaie/upb/23AQ4Gc/rGzkw VBQvIVcf4hb4GcScUmXlD1OjvhSDbA1Fb0TgtSUOH6+Y/efJSuaXmHsR6 O2gTF5Sh0ftRTmHl3HvNPDXym4wlACf1scyvrHNKr22+QuHD/gzv9RTsb A==; X-IronPort-AV: E=McAfee;i="6500,9779,10504"; a="370528214" X-IronPort-AV: E=Sophos;i="5.95,195,1661842800"; d="scan'208";a="370528214" Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Oct 2022 22:23:22 -0700 X-IronPort-AV: E=McAfee;i="6500,9779,10504"; a="734025262" X-IronPort-AV: E=Sophos;i="5.95,195,1661842800"; d="scan'208";a="734025262" Received: from adixit-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.209.13.99]) by fmsmga002-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Oct 2022 22:23:21 -0700 Date: Tue, 18 Oct 2022 22:22:59 -0700 Message-ID: <8735bkjsjw.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Jani Nikula In-Reply-To: <87czaqsvm0.fsf@intel.com> References: <20221015032618.2458429-1-ashutosh.dixit@intel.com> <20221015032618.2458429-2-ashutosh.dixit@intel.com> <87czaqsvm0.fsf@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 1/3] drm/i915/gt: Change RC6 residency functions to accept register ID's 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, Rodrigo Vivi Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Mon, 17 Oct 2022 01:27:35 -0700, Jani Nikula wrote: Hi Jani, Thanks for reviewing, great suggestions overall. I have taken care of most of them in series version v6. Please see below. > On Fri, 14 Oct 2022, Ashutosh Dixit wrote: > > @@ -811,9 +809,23 @@ u64 intel_rc6_residency_ns(struct intel_rc6 *rc6, const i915_reg_t reg) > > return mul_u64_u32_div(time_hw, mul, div); > > } > > > > -u64 intel_rc6_residency_us(struct intel_rc6 *rc6, i915_reg_t reg) > > +u64 intel_rc6_residency_us(struct intel_rc6 *rc6, const enum rc6_res_reg id) > > +{ > > + return DIV_ROUND_UP_ULL(intel_rc6_residency_ns(rc6, id), 1000); > > +} > > + > > +void intel_rc6_print_rc6_res(struct seq_file *m, > > + const char *title, > > + const enum rc6_res_reg id) > > intel_rc6_print_rc5_res is unnecessary duplication. > > intel_rc6_print_residency() maybe? Done. > > > { > > - return DIV_ROUND_UP_ULL(intel_rc6_residency_ns(rc6, reg), 1000); > > + struct intel_gt *gt = m->private; > > + i915_reg_t reg = gt->rc6.res_reg[id]; > > + intel_wakeref_t wakeref; > > + > > + with_intel_runtime_pm(gt->uncore->rpm, wakeref) > > + seq_printf(m, "%s %u (%llu us)\n", title, > > + intel_uncore_read(gt->uncore, reg), > > + intel_rc6_residency_us(>->rc6, id)); > > } > > > > #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) > > diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.h b/drivers/gpu/drm/i915/gt/intel_rc6.h > > index b6fea71afc223..584d2d3b2ec3f 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_rc6.h > > +++ b/drivers/gpu/drm/i915/gt/intel_rc6.h > > @@ -6,7 +6,7 @@ > > #ifndef INTEL_RC6_H > > #define INTEL_RC6_H > > > > -#include "i915_reg_defs.h" > > +#include "intel_rc6_types.h" > > You can forward declare enums as a gcc extension. > > enum rc6_res_reg; Tried but was seeing compile errors so left as is. > > struct intel_engine_cs; > > struct intel_rc6; > > @@ -21,7 +21,10 @@ void intel_rc6_sanitize(struct intel_rc6 *rc6); > > void intel_rc6_enable(struct intel_rc6 *rc6); > > void intel_rc6_disable(struct intel_rc6 *rc6); > > > > -u64 intel_rc6_residency_ns(struct intel_rc6 *rc6, i915_reg_t reg); > > -u64 intel_rc6_residency_us(struct intel_rc6 *rc6, i915_reg_t reg); > > +u64 intel_rc6_residency_ns(struct intel_rc6 *rc6, const enum rc6_res_reg id); > > +u64 intel_rc6_residency_us(struct intel_rc6 *rc6, const enum rc6_res_reg id); > > +void intel_rc6_print_rc6_res(struct seq_file *m, > > + const char *title, > > + const enum rc6_res_reg id); > > "const enum" makes no sense. Removed. Probably const for pass-by-value function arguments never makes sense, I had left the const thinking it would indicate that the function won't modify that argument, but is probably not worth it so removed all "const enum"s. > > > > > #endif /* INTEL_RC6_H */ > > diff --git a/drivers/gpu/drm/i915/gt/intel_rc6_types.h b/drivers/gpu/drm/i915/gt/intel_rc6_types.h > > index e747492b2f46e..0386a3f6e4dc6 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_rc6_types.h > > +++ b/drivers/gpu/drm/i915/gt/intel_rc6_types.h > > @@ -13,7 +13,17 @@ > > > > struct drm_i915_gem_object; > > > > +enum rc6_res_reg { > > + RC6_RES_REG_RC6_LOCKED, > > + RC6_RES_REG_RC6, > > + RC6_RES_REG_RC6p, > > + RC6_RES_REG_RC6pp > > +}; > > Naming: intel_rc6_* for all. Done. > I think you need to take the abstraction further away from > registers. You don't need the *register* part here for anything. Stop > thinking in terms of registers in the interface. > > The callers care about things like "RC6+ residency since boot", and the > callers don't care about where or how this information originates > from. They just want the info, and the register is an implementation > detail hidden behind the interface. > > I.e. use the enum to identify the data you want, not which register it > comes from. Done, please take a look at the new patch. > > > + > > +#define VLV_RC6_RES_REG_MEDIA_RC6 RC6_RES_REG_RC6p > > Please handle this in the enum. Done. > > > + > > struct intel_rc6 { > > + i915_reg_t res_reg[4]; > > Maybe the id enum should have _MAX as last value, used for size here. Done. Thanks. -- Ashutosh > > > u64 prev_hw_residency[4]; > > u64 cur_residency[4]; > > > > diff --git a/drivers/gpu/drm/i915/gt/selftest_rc6.c b/drivers/gpu/drm/i915/gt/selftest_rc6.c > > index 8c70b7e120749..a236e3f8f3183 100644 > > --- a/drivers/gpu/drm/i915/gt/selftest_rc6.c > > +++ b/drivers/gpu/drm/i915/gt/selftest_rc6.c > > @@ -19,11 +19,11 @@ static u64 rc6_residency(struct intel_rc6 *rc6) > > > > /* XXX VLV_GT_MEDIA_RC6? */ > > > > - result = intel_rc6_residency_ns(rc6, GEN6_GT_GFX_RC6); > > + result = intel_rc6_residency_ns(rc6, RC6_RES_REG_RC6); > > if (HAS_RC6p(rc6_to_i915(rc6))) > > - result += intel_rc6_residency_ns(rc6, GEN6_GT_GFX_RC6p); > > + result += intel_rc6_residency_ns(rc6, RC6_RES_REG_RC6p); > > if (HAS_RC6pp(rc6_to_i915(rc6))) > > - result += intel_rc6_residency_ns(rc6, GEN6_GT_GFX_RC6pp); > > + result += intel_rc6_residency_ns(rc6, RC6_RES_REG_RC6pp); > > > > return result; > > } > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c > > index 958b37123bf12..15d0f88136394 100644 > > --- a/drivers/gpu/drm/i915/i915_pmu.c > > +++ b/drivers/gpu/drm/i915/i915_pmu.c > > @@ -148,13 +148,13 @@ static u64 __get_rc6(struct intel_gt *gt) > > struct drm_i915_private *i915 = gt->i915; > > u64 val; > > > > - val = intel_rc6_residency_ns(>->rc6, GEN6_GT_GFX_RC6); > > + val = intel_rc6_residency_ns(>->rc6, RC6_RES_REG_RC6); > > > > if (HAS_RC6p(i915)) > > - val += intel_rc6_residency_ns(>->rc6, GEN6_GT_GFX_RC6p); > > + val += intel_rc6_residency_ns(>->rc6, RC6_RES_REG_RC6p); > > > > if (HAS_RC6pp(i915)) > > - val += intel_rc6_residency_ns(>->rc6, GEN6_GT_GFX_RC6pp); > > + val += intel_rc6_residency_ns(>->rc6, RC6_RES_REG_RC6pp); > > > > return val; > > } > > -- > Jani Nikula, Intel Open Source Graphics Center 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 7DB94C433FE for ; Wed, 19 Oct 2022 05:23:34 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id DE02210F12D; Wed, 19 Oct 2022 05:23:31 +0000 (UTC) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by gabe.freedesktop.org (Postfix) with ESMTPS id E076C10F12C; Wed, 19 Oct 2022 05:23: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=1666157002; x=1697693002; h=date:message-id:from:to:cc:subject:in-reply-to: references:mime-version; bh=O3VwPsk3iMrGn7WIMcPsBTELGmgq6647ss+sbK7+etg=; b=iiKx8SMtLlXZ329UVCLDt5nyv413VjxbwUBdHAAMXg8PMXUVEppu7c0E nOFWXBgrwOtIIpsidd267LWRyCisDK2JAxvdFl0jIl2L8MXBpEPoerf3M VMDk/WPel5909CNWH1bCK8guD9BwGxCcQiY4AAzzofMTjZzicyst31oEj mM1lrJaR/7sVGmHgyZu/yKQQOOuVdY1yF9SEaie/upb/23AQ4Gc/rGzkw VBQvIVcf4hb4GcScUmXlD1OjvhSDbA1Fb0TgtSUOH6+Y/efJSuaXmHsR6 O2gTF5Sh0ftRTmHl3HvNPDXym4wlACf1scyvrHNKr22+QuHD/gzv9RTsb A==; X-IronPort-AV: E=McAfee;i="6500,9779,10504"; a="370528214" X-IronPort-AV: E=Sophos;i="5.95,195,1661842800"; d="scan'208";a="370528214" Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Oct 2022 22:23:22 -0700 X-IronPort-AV: E=McAfee;i="6500,9779,10504"; a="734025262" X-IronPort-AV: E=Sophos;i="5.95,195,1661842800"; d="scan'208";a="734025262" Received: from adixit-mobl.amr.corp.intel.com (HELO adixit-arch.intel.com) ([10.209.13.99]) by fmsmga002-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Oct 2022 22:23:21 -0700 Date: Tue, 18 Oct 2022 22:22:59 -0700 Message-ID: <8735bkjsjw.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Jani Nikula Subject: Re: [PATCH 1/3] drm/i915/gt: Change RC6 residency functions to accept register ID's In-Reply-To: <87czaqsvm0.fsf@intel.com> References: <20221015032618.2458429-1-ashutosh.dixit@intel.com> <20221015032618.2458429-2-ashutosh.dixit@intel.com> <87czaqsvm0.fsf@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 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: intel-gfx@lists.freedesktop.org, Badal Nilawar , dri-devel@lists.freedesktop.org, Rodrigo Vivi Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Mon, 17 Oct 2022 01:27:35 -0700, Jani Nikula wrote: Hi Jani, Thanks for reviewing, great suggestions overall. I have taken care of most of them in series version v6. Please see below. > On Fri, 14 Oct 2022, Ashutosh Dixit wrote: > > @@ -811,9 +809,23 @@ u64 intel_rc6_residency_ns(struct intel_rc6 *rc6, const i915_reg_t reg) > > return mul_u64_u32_div(time_hw, mul, div); > > } > > > > -u64 intel_rc6_residency_us(struct intel_rc6 *rc6, i915_reg_t reg) > > +u64 intel_rc6_residency_us(struct intel_rc6 *rc6, const enum rc6_res_reg id) > > +{ > > + return DIV_ROUND_UP_ULL(intel_rc6_residency_ns(rc6, id), 1000); > > +} > > + > > +void intel_rc6_print_rc6_res(struct seq_file *m, > > + const char *title, > > + const enum rc6_res_reg id) > > intel_rc6_print_rc5_res is unnecessary duplication. > > intel_rc6_print_residency() maybe? Done. > > > { > > - return DIV_ROUND_UP_ULL(intel_rc6_residency_ns(rc6, reg), 1000); > > + struct intel_gt *gt = m->private; > > + i915_reg_t reg = gt->rc6.res_reg[id]; > > + intel_wakeref_t wakeref; > > + > > + with_intel_runtime_pm(gt->uncore->rpm, wakeref) > > + seq_printf(m, "%s %u (%llu us)\n", title, > > + intel_uncore_read(gt->uncore, reg), > > + intel_rc6_residency_us(>->rc6, id)); > > } > > > > #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) > > diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.h b/drivers/gpu/drm/i915/gt/intel_rc6.h > > index b6fea71afc223..584d2d3b2ec3f 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_rc6.h > > +++ b/drivers/gpu/drm/i915/gt/intel_rc6.h > > @@ -6,7 +6,7 @@ > > #ifndef INTEL_RC6_H > > #define INTEL_RC6_H > > > > -#include "i915_reg_defs.h" > > +#include "intel_rc6_types.h" > > You can forward declare enums as a gcc extension. > > enum rc6_res_reg; Tried but was seeing compile errors so left as is. > > struct intel_engine_cs; > > struct intel_rc6; > > @@ -21,7 +21,10 @@ void intel_rc6_sanitize(struct intel_rc6 *rc6); > > void intel_rc6_enable(struct intel_rc6 *rc6); > > void intel_rc6_disable(struct intel_rc6 *rc6); > > > > -u64 intel_rc6_residency_ns(struct intel_rc6 *rc6, i915_reg_t reg); > > -u64 intel_rc6_residency_us(struct intel_rc6 *rc6, i915_reg_t reg); > > +u64 intel_rc6_residency_ns(struct intel_rc6 *rc6, const enum rc6_res_reg id); > > +u64 intel_rc6_residency_us(struct intel_rc6 *rc6, const enum rc6_res_reg id); > > +void intel_rc6_print_rc6_res(struct seq_file *m, > > + const char *title, > > + const enum rc6_res_reg id); > > "const enum" makes no sense. Removed. Probably const for pass-by-value function arguments never makes sense, I had left the const thinking it would indicate that the function won't modify that argument, but is probably not worth it so removed all "const enum"s. > > > > > #endif /* INTEL_RC6_H */ > > diff --git a/drivers/gpu/drm/i915/gt/intel_rc6_types.h b/drivers/gpu/drm/i915/gt/intel_rc6_types.h > > index e747492b2f46e..0386a3f6e4dc6 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_rc6_types.h > > +++ b/drivers/gpu/drm/i915/gt/intel_rc6_types.h > > @@ -13,7 +13,17 @@ > > > > struct drm_i915_gem_object; > > > > +enum rc6_res_reg { > > + RC6_RES_REG_RC6_LOCKED, > > + RC6_RES_REG_RC6, > > + RC6_RES_REG_RC6p, > > + RC6_RES_REG_RC6pp > > +}; > > Naming: intel_rc6_* for all. Done. > I think you need to take the abstraction further away from > registers. You don't need the *register* part here for anything. Stop > thinking in terms of registers in the interface. > > The callers care about things like "RC6+ residency since boot", and the > callers don't care about where or how this information originates > from. They just want the info, and the register is an implementation > detail hidden behind the interface. > > I.e. use the enum to identify the data you want, not which register it > comes from. Done, please take a look at the new patch. > > > + > > +#define VLV_RC6_RES_REG_MEDIA_RC6 RC6_RES_REG_RC6p > > Please handle this in the enum. Done. > > > + > > struct intel_rc6 { > > + i915_reg_t res_reg[4]; > > Maybe the id enum should have _MAX as last value, used for size here. Done. Thanks. -- Ashutosh > > > u64 prev_hw_residency[4]; > > u64 cur_residency[4]; > > > > diff --git a/drivers/gpu/drm/i915/gt/selftest_rc6.c b/drivers/gpu/drm/i915/gt/selftest_rc6.c > > index 8c70b7e120749..a236e3f8f3183 100644 > > --- a/drivers/gpu/drm/i915/gt/selftest_rc6.c > > +++ b/drivers/gpu/drm/i915/gt/selftest_rc6.c > > @@ -19,11 +19,11 @@ static u64 rc6_residency(struct intel_rc6 *rc6) > > > > /* XXX VLV_GT_MEDIA_RC6? */ > > > > - result = intel_rc6_residency_ns(rc6, GEN6_GT_GFX_RC6); > > + result = intel_rc6_residency_ns(rc6, RC6_RES_REG_RC6); > > if (HAS_RC6p(rc6_to_i915(rc6))) > > - result += intel_rc6_residency_ns(rc6, GEN6_GT_GFX_RC6p); > > + result += intel_rc6_residency_ns(rc6, RC6_RES_REG_RC6p); > > if (HAS_RC6pp(rc6_to_i915(rc6))) > > - result += intel_rc6_residency_ns(rc6, GEN6_GT_GFX_RC6pp); > > + result += intel_rc6_residency_ns(rc6, RC6_RES_REG_RC6pp); > > > > return result; > > } > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c > > index 958b37123bf12..15d0f88136394 100644 > > --- a/drivers/gpu/drm/i915/i915_pmu.c > > +++ b/drivers/gpu/drm/i915/i915_pmu.c > > @@ -148,13 +148,13 @@ static u64 __get_rc6(struct intel_gt *gt) > > struct drm_i915_private *i915 = gt->i915; > > u64 val; > > > > - val = intel_rc6_residency_ns(>->rc6, GEN6_GT_GFX_RC6); > > + val = intel_rc6_residency_ns(>->rc6, RC6_RES_REG_RC6); > > > > if (HAS_RC6p(i915)) > > - val += intel_rc6_residency_ns(>->rc6, GEN6_GT_GFX_RC6p); > > + val += intel_rc6_residency_ns(>->rc6, RC6_RES_REG_RC6p); > > > > if (HAS_RC6pp(i915)) > > - val += intel_rc6_residency_ns(>->rc6, GEN6_GT_GFX_RC6pp); > > + val += intel_rc6_residency_ns(>->rc6, RC6_RES_REG_RC6pp); > > > > return val; > > } > > -- > Jani Nikula, Intel Open Source Graphics Center