[Intel-gfx] [PATCH 1/2] drm/i915: Fix GEN8_MISCCPCTL
Lucas De Marchi
lucas.demarchi at intel.com
Fri Feb 3 18:03:49 UTC 2023
On Thu, Feb 02, 2023 at 05:12:10PM -0800, Matt Roper wrote:
>On Thu, Feb 02, 2023 at 04:57:08PM -0800, Lucas De Marchi wrote:
>> Register 0x9424 is not replicated on any platform, so it shouldn't be
>> declared with REG_MCR(). Declaring it with _MMIO() is basically
>> duplicate of the GEN7 version, so just remove the GEN8 and change all
>> the callers to use the right functions.
>
>According to an old copy of bspec page 13991, 0x9400-0x97FF was an MCR
>range on gen8 platforms. Newer copies of that bspec page forgot to even
>include the register range table, so it's not obvious unless you dig
>through the history and look at a version from before Aug 2020.
>
>However bspec page 66673 indicates that this range went back to being a
>singleton range in gen9 (and the other forcewake pages for newer
>platforms indicate it stayed that way), so that means BDW and CHV are
>the _only_ platforms that should treat it as MCR. Usage for other
>platforms should either add a new "GEN9" definition, or just go back to
>using the GEN7 definition.
sounds like more a spec mistake. This range was listed as
"slice common". I'm not sure we'd really have to set any steering for
specific slice. Another thing is that we didn't set any steering for a
long time in this register and it was working. Even now there is no
table for gen8/gen9 in drivers/gpu/drm/i915/gt/intel_gt_mcr.c, so any
call to intel_gt_mcr_* will simply fallback to "no steering required".
For me, any MCR_REG() should correspond to registers in these
tables. I don't think there's much point in annotating the register as
MCR in its definition and then do nothing with it. Btw, this is how I
started getting warning wrt this register: as you knowm, in xe driver
you added a warning for registers missing from the mcr tables,
which I think is indeed the right thing to do for the recent platforms.
For gen8, this change here should not change any behavior. It
changes for gen11+ to the correct behavior. So I don't think we need to
care much about double checking if gen8 had a unique behavior no other
platforms have. I think just amending the commit message with more
information like this would be ok.
Lucas De Marchi
>
>
>Matt
>
>>
>> Also use intel_uncore_rmw() rather than a read + write where possible.
>>
>> Fixes: a9e69428b1b4 ("drm/i915: Define MCR registers explicitly")
>> Cc: Matt Roper <matthew.d.roper at intel.com>
>> Cc: Balasubramani Vivekanandan <balasubramani.vivekanandan at intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> Cc: Gustavo Sousa <gustavo.sousa at intel.com>
>> Cc: Matt Atwood <matthew.s.atwood at intel.com>
>> Cc: Ashutosh Dixit <ashutosh.dixit at intel.com>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>> drivers/gpu/drm/i915/gt/intel_gt_regs.h | 5 +----
>> drivers/gpu/drm/i915/gt/intel_workarounds.c | 4 ++--
>> drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c | 5 ++---
>> drivers/gpu/drm/i915/intel_pm.c | 10 +++++-----
>> 4 files changed, 10 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
>> index 7fa18a3b3957..cc1539c7a6b6 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
>> @@ -686,10 +686,7 @@
>> #define GEN6_RSTCTL _MMIO(0x9420)
>>
>> #define GEN7_MISCCPCTL _MMIO(0x9424)
>> -#define GEN7_DOP_CLOCK_GATE_ENABLE (1 << 0)
>> -
>> -#define GEN8_MISCCPCTL MCR_REG(0x9424)
>> -#define GEN8_DOP_CLOCK_GATE_ENABLE REG_BIT(0)
>> +#define GEN7_DOP_CLOCK_GATE_ENABLE REG_BIT(0)
>> #define GEN12_DOP_CLOCK_GATE_RENDER_ENABLE REG_BIT(1)
>> #define GEN8_DOP_CLOCK_GATE_CFCLK_ENABLE (1 << 2)
>> #define GEN8_DOP_CLOCK_GATE_GUC_ENABLE (1 << 4)
>> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> index 29718d0595f4..cfc122c17e28 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> @@ -1645,7 +1645,7 @@ dg2_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
>> wa_mcr_write_or(wal, XEHP_SQCM, EN_32B_ACCESS);
>>
>> /* Wa_14015795083 */
>> - wa_mcr_write_clr(wal, GEN8_MISCCPCTL, GEN12_DOP_CLOCK_GATE_RENDER_ENABLE);
>> + wa_write_clr(wal, GEN7_MISCCPCTL, GEN12_DOP_CLOCK_GATE_RENDER_ENABLE);
>>
>> /* Wa_18018781329 */
>> wa_mcr_write_or(wal, RENDER_MOD_CTRL, FORCE_MISS_FTLB);
>> @@ -1664,7 +1664,7 @@ pvc_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
>> pvc_init_mcr(gt, wal);
>>
>> /* Wa_14015795083 */
>> - wa_mcr_write_clr(wal, GEN8_MISCCPCTL, GEN12_DOP_CLOCK_GATE_RENDER_ENABLE);
>> + wa_write_clr(wal, GEN7_MISCCPCTL, GEN12_DOP_CLOCK_GATE_RENDER_ENABLE);
>>
>> /* Wa_18018781329 */
>> wa_mcr_write_or(wal, RENDER_MOD_CTRL, FORCE_MISS_FTLB);
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
>> index 3d2249bda368..69133420c78b 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
>> @@ -39,9 +39,8 @@ static void guc_prepare_xfer(struct intel_gt *gt)
>>
>> if (GRAPHICS_VER(uncore->i915) == 9) {
>> /* DOP Clock Gating Enable for GuC clocks */
>> - intel_gt_mcr_multicast_write(gt, GEN8_MISCCPCTL,
>> - GEN8_DOP_CLOCK_GATE_GUC_ENABLE |
>> - intel_gt_mcr_read_any(gt, GEN8_MISCCPCTL));
>> + intel_uncore_rmw(uncore, GEN7_MISCCPCTL, 0,
>> + GEN8_DOP_CLOCK_GATE_GUC_ENABLE);
>>
>> /* allows for 5us (in 10ns units) before GT can go to RC6 */
>> intel_uncore_write(uncore, GUC_ARAT_C6DIS, 0x1FF);
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index e0364c4141b8..798607959458 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -4300,8 +4300,8 @@ static void gen8_set_l3sqc_credits(struct drm_i915_private *dev_priv,
>> u32 val;
>>
>> /* WaTempDisableDOPClkGating:bdw */
>> - misccpctl = intel_gt_mcr_multicast_rmw(to_gt(dev_priv), GEN8_MISCCPCTL,
>> - GEN8_DOP_CLOCK_GATE_ENABLE, 0);
>> + misccpctl = intel_uncore_rmw(&dev_priv->uncore, GEN7_MISCCPCTL,
>> + GEN7_DOP_CLOCK_GATE_ENABLE, 0);
>>
>> val = intel_gt_mcr_read_any(to_gt(dev_priv), GEN8_L3SQCREG1);
>> val &= ~L3_PRIO_CREDITS_MASK;
>> @@ -4315,7 +4315,7 @@ static void gen8_set_l3sqc_credits(struct drm_i915_private *dev_priv,
>> */
>> intel_gt_mcr_read_any(to_gt(dev_priv), GEN8_L3SQCREG1);
>> udelay(1);
>> - intel_gt_mcr_multicast_write(to_gt(dev_priv), GEN8_MISCCPCTL, misccpctl);
>> + intel_uncore_write(&dev_priv->uncore, GEN7_MISCCPCTL, misccpctl);
>> }
>>
>> static void icl_init_clock_gating(struct drm_i915_private *dev_priv)
>> @@ -4453,8 +4453,8 @@ static void skl_init_clock_gating(struct drm_i915_private *dev_priv)
>> gen9_init_clock_gating(dev_priv);
>>
>> /* WaDisableDopClockGating:skl */
>> - intel_gt_mcr_multicast_rmw(to_gt(dev_priv), GEN8_MISCCPCTL,
>> - GEN8_DOP_CLOCK_GATE_ENABLE, 0);
>> + intel_uncore_rmw(&dev_priv->uncore, GEN7_MISCCPCTL,
>> + GEN7_DOP_CLOCK_GATE_ENABLE, 0);
>>
>> /* WAC6entrylatency:skl */
>> intel_uncore_rmw(&dev_priv->uncore, FBC_LLC_READ_CTRL, 0, FBC_LLC_FULLY_OPEN);
>> --
>> 2.39.0
>>
>
>--
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation
More information about the Intel-gfx
mailing list