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 7E361C433EF for ; Wed, 8 Jun 2022 06:35:08 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 9FAAA10E0A2; Wed, 8 Jun 2022 06:35:07 +0000 (UTC) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by gabe.freedesktop.org (Postfix) with ESMTPS id 13D3010E0A2; Wed, 8 Jun 2022 06:35:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1654670106; x=1686206106; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=kg6x+lZ5Hy6d/AgdPvu2rwKDPUvIhscPIg6TyP1pHSg=; b=EZguWupcgAK/c07BwcEVQ+qN6r1eNQ/eAbgaisgd1tJobp6JXczwFukD mrghm313N36zOQyLgUF3HCMRWrSyNCHUwdbAbo9lo5cBW7r7DZ+mSTpjA SE5b3DQhQ/ohV2G9Ev0/vI3KOtw417PODUZmdld6qmoI9lieO1ag2MCWB hTz6EeDAN7c+SZ4H8yiAuOnjB2HwoCzEwSgPpbLzC+LXGMH58PuBKd5pd nI/y3x8cOCyBWmtE5cMhojh3FZ/DwIgPx1MUZtMunCX7f0ufRttW1rycE FyEwDsXLS4d9UYXf0QPbvS6c4YWAte/z/Kyi5fsz2f3UhEBpAdmHt1HTO A==; X-IronPort-AV: E=McAfee;i="6400,9594,10371"; a="259929598" X-IronPort-AV: E=Sophos;i="5.91,285,1647327600"; d="scan'208";a="259929598" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Jun 2022 23:35:05 -0700 X-IronPort-AV: E=Sophos;i="5.91,285,1647327600"; d="scan'208";a="636612447" Received: from hchegond-ivm1.jf.intel.com (HELO intel.com) ([10.165.21.208]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Jun 2022 23:35:04 -0700 Date: Tue, 7 Jun 2022 23:35:00 -0700 From: Harish Chegondi To: Matt Roper Message-ID: References: <20220603005335.1733129-1-matthew.d.roper@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20220603005335.1733129-1-matthew.d.roper@intel.com> Subject: Re: [Intel-gfx] [PATCH] drm/i915/pvc: Add register steering 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 Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Thu, Jun 02, 2022 at 05:53:35PM -0700, Matt Roper wrote: > Ponte Vecchio no longer has MSLICE or LNCF steering, but the bspec does > document several new types of multicast register ranges. Fortunately, > most of the different MCR types all provide valid values at instance > (0,0) so there's no need to read fuse registers and calculate a valid > instance. We'll lump all of those range types (BSLICE, HALFBSLICE, > TILEPSMI, CC, and L3BANK) into a single category called "INSTANCE0" to > keep things simple. We'll also perform explicit steering for each of > these multicast register types, even if the implicit steering setup for > COMPUTE/DSS ranges would have worked too; this is based on guidance from > our hardware architects who suggested that we move away from implicit > steering and start explicitly steer all MCR register accesses on modern > platforms (we'll work on transitioning COMPUTE/DSS to explicit steering > in the future). > > Note that there's one additional MCR range type defined in the bspec > (SQIDI) that we don't handle here. Those ranges use a different > steering control register that we never touch; since instance 0 is also > always a valid setting there, we can just ignore those ranges. > > Finally, we'll rename the HAS_MSLICES() macro to HAS_MSLICE_STEERING(). > PVC hardware still has units referred to as mslices, but there's no > register steering based on mslice for this platform. > > Bspec: 67609 > Signed-off-by: Matt Roper > --- > drivers/gpu/drm/i915/gt/intel_gt.c | 50 ++++++++++++++++++--- > drivers/gpu/drm/i915/gt/intel_gt_types.h | 7 +++ > drivers/gpu/drm/i915/gt/intel_workarounds.c | 22 ++++++++- > drivers/gpu/drm/i915/i915_drv.h | 3 +- > drivers/gpu/drm/i915/i915_pci.c | 3 +- > drivers/gpu/drm/i915/intel_device_info.h | 2 +- > 6 files changed, 76 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c > index ddfb98f70489..b335eacd7a6c 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c > @@ -106,6 +106,7 @@ static const char * const intel_steering_types[] = { > "L3BANK", > "MSLICE", > "LNCF", > + "INSTANCE 0", > }; > > static const struct intel_mmio_range icl_l3bank_steering_table[] = { > @@ -133,6 +134,27 @@ static const struct intel_mmio_range dg2_lncf_steering_table[] = { > {}, > }; > > +/* > + * We have several types of MCR registers on PVC where steering to (0,0) > + * will always provide us with a non-terminated value. We'll stick them > + * all in the same table for simplicity. > + */ > +static const struct intel_mmio_range pvc_instance0_steering_table[] = { > + { 0x004000, 0x004AFF }, /* HALF-BSLICE */ > + { 0x008A80, 0x008AFF }, /* TILEPSMI */ > + { 0x008800, 0x00887F }, /* CC */ minor nit: The above two lines need to be swapped to keep this table sorted in the increasing order of addresses. > + { 0x00B000, 0x00B0FF }, /* HALF-BSLICE */ > + { 0x00B100, 0x00B3FF }, /* L3BANK */ > + { 0x00C800, 0x00CFFF }, /* HALF-BSLICE */ > + { 0x00D800, 0x00D8FF }, /* HALF-BSLICE */ > + { 0x00DD00, 0x00DDFF }, /* BSLICE */ > + { 0x00E900, 0x00E9FF }, /* HALF-BSLICE */ > + { 0x00EC00, 0x00EEFF }, /* HALF-BSLICE */ > + { 0x00F000, 0x00FFFF }, /* HALF-BSLICE */ > + { 0x024180, 0x0241FF }, /* HALF-BSLICE */ > + {}, > +}; > + > int intel_gt_init_mmio(struct intel_gt *gt) > { > struct drm_i915_private *i915 = gt->i915; > @@ -146,7 +168,7 @@ int intel_gt_init_mmio(struct intel_gt *gt) > * An mslice is unavailable only if both the meml3 for the slice is > * disabled *and* all of the DSS in the slice (quadrant) are disabled. > */ > - if (HAS_MSLICES(i915)) { > + if (HAS_MSLICE_STEERING(i915)) { > gt->info.mslice_mask = > intel_slicemask_from_xehp_dssmask(gt->info.sseu.subslice_mask, > GEN_DSS_PER_MSLICE); > @@ -158,7 +180,9 @@ int intel_gt_init_mmio(struct intel_gt *gt) > drm_warn(&i915->drm, "mslice mask all zero!\n"); > } > > - if (IS_DG2(i915)) { > + if (IS_PONTEVECCHIO(i915)) { > + gt->steering_table[INSTANCE0] = pvc_instance0_steering_table; > + } else if (IS_DG2(i915)) { > gt->steering_table[MSLICE] = xehpsdv_mslice_steering_table; > gt->steering_table[LNCF] = dg2_lncf_steering_table; > } else if (IS_XEHPSDV(i915)) { > @@ -172,7 +196,11 @@ int intel_gt_init_mmio(struct intel_gt *gt) > GEN10_L3BANK_MASK; > if (!gt->info.l3bank_mask) /* should be impossible! */ > drm_warn(&i915->drm, "L3 bank mask is all zero!\n"); > - } else if (HAS_MSLICES(i915)) { > + } else if (GRAPHICS_VER(i915) >= 11) { > + /* > + * We expect all modern platforms to have at least some > + * type of steering that needs to be initialized. > + */ > MISSING_CASE(INTEL_INFO(i915)->platform); > } > > @@ -888,7 +916,7 @@ static void intel_gt_get_valid_steering(struct intel_gt *gt, > *subsliceid = __ffs(gt->info.l3bank_mask); > break; > case MSLICE: > - GEM_WARN_ON(!HAS_MSLICES(gt->i915)); > + GEM_WARN_ON(!HAS_MSLICE_STEERING(gt->i915)); > *sliceid = __ffs(gt->info.mslice_mask); > *subsliceid = 0; /* unused */ > break; > @@ -897,10 +925,18 @@ static void intel_gt_get_valid_steering(struct intel_gt *gt, > * An LNCF is always present if its mslice is present, so we > * can safely just steer to LNCF 0 in all cases. > */ > - GEM_WARN_ON(!HAS_MSLICES(gt->i915)); > + GEM_WARN_ON(!HAS_MSLICE_STEERING(gt->i915)); > *sliceid = __ffs(gt->info.mslice_mask) << 1; > *subsliceid = 0; /* unused */ > break; > + case INSTANCE0: > + /* > + * There are a lot of MCR types for which instance (0, 0) > + * will always provide a non-terminated value. > + */ > + *sliceid = 0; > + *subsliceid = 0; > + break; > default: > MISSING_CASE(type); > *sliceid = 0; > @@ -1020,7 +1056,9 @@ void intel_gt_report_steering(struct drm_printer *p, struct intel_gt *gt, > gt->default_steering.groupid, > gt->default_steering.instanceid); > > - if (HAS_MSLICES(gt->i915)) { > + if (IS_PONTEVECCHIO(gt->i915)) { > + report_steering_type(p, gt, INSTANCE0, dump_table); > + } else if (HAS_MSLICE_STEERING(gt->i915)) { > report_steering_type(p, gt, MSLICE, dump_table); > report_steering_type(p, gt, LNCF, dump_table); > } > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h > index 993f003dad1d..df708802889d 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h > +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h > @@ -59,6 +59,13 @@ enum intel_steering_type { > MSLICE, > LNCF, > > + /* > + * On some platforms there are multiple types of MCR registers that > + * will always return a non-terminated value at instance (0, 0). We'll > + * lump those all into a single category to keep things simple. > + */ > + INSTANCE0, > + > NUM_STEERING_TYPES > }; > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c > index eb0598593724..1b191b234160 100644 > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c > @@ -1195,6 +1195,20 @@ xehp_init_mcr(struct intel_gt *gt, struct i915_wa_list *wal) > __set_mcr_steering(wal, SF_MCR_SELECTOR, 0, 2); > } > > +static void > +pvc_init_mcr(struct intel_gt *gt, struct i915_wa_list *wal) > +{ > + unsigned int dss; > + > + /* > + * Setup implicit steering for COMPUTE and DSS ranges to the first > + * non-fused-off DSS. All other types of MCR registers will be > + * explicitly steered. > + */ > + dss = intel_sseu_find_first_xehp_dss(>->info.sseu, 0, 0); > + __add_mcr_wa(gt, wal, dss / GEN_DSS_PER_CSLICE, dss % GEN_DSS_PER_CSLICE); > +} > + > static void > icl_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal) > { > @@ -1488,13 +1502,19 @@ dg2_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal) > wa_write_or(wal, GEN12_SQCM, EN_32B_ACCESS); > } > > +static void > +pvc_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal) > +{ > + pvc_init_mcr(gt, wal); > +} > + > static void > gt_init_workarounds(struct intel_gt *gt, struct i915_wa_list *wal) > { > struct drm_i915_private *i915 = gt->i915; > > if (IS_PONTEVECCHIO(i915)) > - ; /* none yet */ > + pvc_gt_workarounds_init(gt, wal); > else if (IS_DG2(i915)) > dg2_gt_workarounds_init(gt, wal); > else if (IS_XEHPSDV(i915)) > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index c3854b8a014f..5870cf9eb0b4 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1283,8 +1283,7 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, > #define HAS_RUNTIME_PM(dev_priv) (INTEL_INFO(dev_priv)->has_runtime_pm) > #define HAS_64BIT_RELOC(dev_priv) (INTEL_INFO(dev_priv)->has_64bit_reloc) > > -#define HAS_MSLICES(dev_priv) \ > - (INTEL_INFO(dev_priv)->has_mslices) > +#define HAS_MSLICE_STEERING(dev_priv) (INTEL_INFO(dev_priv)->has_mslice_steering) > > /* > * Set this flag, when platform requires 64K GTT page sizes or larger for > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c > index a5a1a7647320..5e51fc29bb8b 100644 > --- a/drivers/gpu/drm/i915/i915_pci.c > +++ b/drivers/gpu/drm/i915/i915_pci.c > @@ -1021,7 +1021,7 @@ static const struct intel_device_info adl_p_info = { > .has_llc = 1, \ > .has_logical_ring_contexts = 1, \ > .has_logical_ring_elsq = 1, \ > - .has_mslices = 1, \ > + .has_mslice_steering = 1, \ > .has_rc6 = 1, \ > .has_reset_engine = 1, \ > .has_rps = 1, \ > @@ -1091,6 +1091,7 @@ static const struct intel_device_info ats_m_info = { > .has_3d_pipeline = 0, \ > .has_guc_deprivilege = 1, \ > .has_l3_ccs_read = 1, \ > + .has_mslice_steering = 0, \ > .has_one_eu_per_fuse_bit = 1 > > __maybe_unused > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h > index 346f17f2dce8..08341174ee0a 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.h > +++ b/drivers/gpu/drm/i915/intel_device_info.h > @@ -157,7 +157,7 @@ enum intel_ppgtt_type { > func(has_logical_ring_contexts); \ > func(has_logical_ring_elsq); \ > func(has_media_ratio_mode); \ > - func(has_mslices); \ > + func(has_mslice_steering); \ > func(has_one_eu_per_fuse_bit); \ > func(has_pooled_eu); \ > func(has_pxp); \ Looks good to me. Reviewed-by: Harish Chegondi > -- > 2.35.3 >