From: Lucas De Marchi <lucas.demarchi@intel.com>
To: "Souza, Jose" <jose.souza@intel.com>
Cc: "Nikula, Jani" <jani.nikula@intel.com>,
"intel-gfx@lists.freedesktop.org"
<intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH 02/21] drm/i915/tgl: Fix macros for TGL SOC based WA
Date: Tue, 17 Nov 2020 11:28:54 -0800 [thread overview]
Message-ID: <20201117192854.57tki3vges6zdenp@ldmartin-desk1> (raw)
In-Reply-To: <02ea7d2b7eb151787abe8cf42771db40a7f0ef7d.camel@intel.com>
On Tue, Nov 17, 2020 at 07:03:05PM +0000, Jose Souza wrote:
>On Tue, 2020-11-17 at 10:50 -0800, Aditya Swarup wrote:
>> Fix macros for applying TGL SOC WAs by using INTEL_REVID()
>> as index to fetch correct revision offset in TGL GT/DISP stepping
>> table.
>
>Please explain what exactly is the issue you are fixing, the change you did in tgl_revids_get() + IS_TGL_GT_REVID looks a improvement but not a fix.
otherwise it always gets the first entry from the table, regardless
what' s the revid we are running on... so it does look like a very
important fix.
>
>>
>> Also, remove redundant macros and simplify it to use GT and DISP
>> macros for getting applicable stepping for TGL.
As a fix, this should not be mixed with the noisy s/TGL_REVID/REVID/, as
it makes it much more difficult for backports and to review. Please
split it in another patch (I actually don' t see a reason to do it
actually... I'd rather try to move away from these tables if possible).
Lucas De Marchi
>>
>> Fixes: ("drm/i915/tgl: Fix stepping WA matching")
>> Cc: José Roberto de Souza <jose.souza@intel.com>
>> Cc: Matt Roper <matthew.d.roper@intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Aditya Swarup <aditya.swarup@intel.com>
>> ---
>> .../drm/i915/display/intel_display_power.c | 2 +-
>> drivers/gpu/drm/i915/display/intel_psr.c | 4 ++--
>> drivers/gpu/drm/i915/display/intel_sprite.c | 2 +-
>> drivers/gpu/drm/i915/gt/intel_workarounds.c | 20 ++++++++--------
>> drivers/gpu/drm/i915/i915_drv.h | 24 +++++++------------
>> drivers/gpu/drm/i915/intel_pm.c | 2 +-
>> 6 files changed, 24 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
>> index fe2d90bba536..06c036e2092c 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
>> @@ -5283,7 +5283,7 @@ static void tgl_bw_buddy_init(struct drm_i915_private *dev_priv)
>> int config, i;
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> if (IS_DG1_REVID(dev_priv, DG1_REVID_A0, DG1_REVID_A0) ||
>> - IS_TGL_DISP_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_B0))
>> + IS_TGL_DISP_REVID(dev_priv, REVID_A0, REVID_B0))
>> /* Wa_1409767108:tgl,dg1 */
>> table = wa_1409767108_buddy_page_masks;
>> else
>> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
>> index b3631b722de3..c057a03b2ed4 100644
>> --- a/drivers/gpu/drm/i915/display/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>> @@ -550,7 +550,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> if (dev_priv->psr.psr2_sel_fetch_enabled) {
>> /* WA 1408330847 */
>> - if (IS_TGL_DISP_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_A0) ||
>> + if (IS_TGL_DISP_REVID(dev_priv, REVID_A0, REVID_A0) ||
>> IS_RKL_REVID(dev_priv, RKL_REVID_A0, RKL_REVID_A0))
>> intel_de_rmw(dev_priv, CHICKEN_PAR1_1,
>> DIS_RAM_BYPASS_PSR2_MAN_TRACK,
>> @@ -1102,7 +1102,7 @@ static void intel_psr_disable_locked(struct intel_dp *intel_dp)
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> /* WA 1408330847 */
>> if (dev_priv->psr.psr2_sel_fetch_enabled &&
>> - (IS_TGL_DISP_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_A0) ||
>> + (IS_TGL_DISP_REVID(dev_priv, REVID_A0, REVID_A0) ||
>> IS_RKL_REVID(dev_priv, RKL_REVID_A0, RKL_REVID_A0)))
>> intel_de_rmw(dev_priv, CHICKEN_PAR1_1,
>> DIS_RAM_BYPASS_PSR2_MAN_TRACK, 0);
>> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
>> index a3ab44694118..f7da4a56054e 100644
>> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
>> @@ -3022,7 +3022,7 @@ static bool gen12_plane_supports_mc_ccs(struct drm_i915_private *dev_priv,
>> {
>> /* Wa_14010477008:tgl[a0..c0],rkl[all],dg1[all] */
>> if (IS_DG1(dev_priv) || IS_ROCKETLAKE(dev_priv) ||
>> - IS_TGL_DISP_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_C0))
>> + IS_TGL_DISP_REVID(dev_priv, REVID_A0, REVID_C0))
>> return false;
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> return plane_id < PLANE_SPRITE4;
>> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> index a82554baa6ac..d756155d82ea 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>> @@ -71,16 +71,16 @@ const struct i915_rev_steppings kbl_revids[] = {
>> };
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> const struct i915_rev_steppings tgl_uy_revids[] = {
>> - [0] = { .gt_stepping = TGL_REVID_A0, .disp_stepping = TGL_REVID_A0 },
>> - [1] = { .gt_stepping = TGL_REVID_B0, .disp_stepping = TGL_REVID_C0 },
>> - [2] = { .gt_stepping = TGL_REVID_B1, .disp_stepping = TGL_REVID_C0 },
>> - [3] = { .gt_stepping = TGL_REVID_C0, .disp_stepping = TGL_REVID_D0 },
>> + [0] = { .gt_stepping = REVID_A0, .disp_stepping = REVID_A0 },
>> + [1] = { .gt_stepping = REVID_B0, .disp_stepping = REVID_C0 },
>> + [2] = { .gt_stepping = REVID_B1, .disp_stepping = REVID_C0 },
>> + [3] = { .gt_stepping = REVID_C0, .disp_stepping = REVID_D0 },
>> };
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> /* Same GT stepping between tgl_uy_revids and tgl_revids don't mean the same HW */
>> const struct i915_rev_steppings tgl_revids[] = {
>> - [0] = { .gt_stepping = TGL_REVID_A0, .disp_stepping = TGL_REVID_B0 },
>> - [1] = { .gt_stepping = TGL_REVID_B0, .disp_stepping = TGL_REVID_D0 },
>> + [0] = { .gt_stepping = REVID_A0, .disp_stepping = REVID_B0 },
>> + [1] = { .gt_stepping = REVID_B0, .disp_stepping = REVID_D0 },
>> };
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> static void wa_init_start(struct i915_wa_list *wal, const char *name, const char *engine_name)
>> @@ -1250,13 +1250,13 @@ tgl_gt_workarounds_init(struct drm_i915_private *i915, struct i915_wa_list *wal)
>> gen12_gt_workarounds_init(i915, wal);
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> /* Wa_1409420604:tgl */
>> - if (IS_TGL_UY_GT_REVID(i915, TGL_REVID_A0, TGL_REVID_A0))
>> + if (IS_TGL_GT_REVID(i915, REVID_A0, REVID_A0))
>> wa_write_or(wal,
>> SUBSLICE_UNIT_LEVEL_CLKGATE2,
>> CPSSUNIT_CLKGATE_DIS);
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> /* Wa_1607087056:tgl also know as BUG:1409180338 */
>> - if (IS_TGL_UY_GT_REVID(i915, TGL_REVID_A0, TGL_REVID_A0))
>> + if (IS_TGL_GT_REVID(i915, REVID_A0, REVID_A0))
>> wa_write_or(wal,
>> SLICE_UNIT_LEVEL_CLKGATE,
>> L3_CLKGATE_DIS | L3_CR2X_CLKGATE_DIS);
>> @@ -1734,7 +1734,7 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
>> struct drm_i915_private *i915 = engine->i915;
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> if (IS_DG1_REVID(i915, DG1_REVID_A0, DG1_REVID_A0) ||
>> - IS_TGL_UY_GT_REVID(i915, TGL_REVID_A0, TGL_REVID_A0)) {
>> + IS_TGL_GT_REVID(i915, REVID_A0, REVID_A0)) {
>> /*
>> * Wa_1607138336:tgl[a0],dg1[a0]
>> * Wa_1607063988:tgl[a0],dg1[a0]
>> @@ -1744,7 +1744,7 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
>> GEN12_DISABLE_POSH_BUSY_FF_DOP_CG);
>> }
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> - if (IS_TGL_UY_GT_REVID(i915, TGL_REVID_A0, TGL_REVID_A0)) {
>> + if (IS_TGL_GT_REVID(i915, REVID_A0, REVID_A0)) {
>> /*
>> * Wa_1606679103:tgl
>> * (see also Wa_1606682166:icl)
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 15be8debae54..437916aacaa6 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1565,11 +1565,11 @@ extern const struct i915_rev_steppings kbl_revids[];
>> (IS_JSL_EHL(p) && IS_REVID(p, since, until))
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> enum {
>> - TGL_REVID_A0,
>> - TGL_REVID_B0,
>> - TGL_REVID_B1,
>> - TGL_REVID_C0,
>> - TGL_REVID_D0,
>> + REVID_A0,
>> + REVID_B0,
>> + REVID_B1,
>> + REVID_C0,
>> + REVID_D0,
>
>Better keep "TGL_" otherwise this could be used in other platforms that have different values for each revision.
>
>> };
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>> extern const struct i915_rev_steppings tgl_uy_revids[];
>> @@ -1579,9 +1579,9 @@ static inline const struct i915_rev_steppings *
>> tgl_revids_get(struct drm_i915_private *dev_priv)
>> {
>> if (IS_TGL_U(dev_priv) || IS_TGL_Y(dev_priv))
>> - return tgl_uy_revids;
>> + return tgl_uy_revids + INTEL_REVID(dev_priv);
>> else
>> - return tgl_revids;
>> + return tgl_revids + INTEL_REVID(dev_priv);
>
>better do tgl_revids[INTEL_REVID(dev_priv)] with a array size check first.
>
>> }
>>
>>
>>
>>
>>
>>
>>
>>
>> #define IS_TGL_DISP_REVID(p, since, until) \
>> @@ -1589,16 +1589,10 @@ tgl_revids_get(struct drm_i915_private *dev_priv)
>> tgl_revids_get(p)->disp_stepping >= (since) && \
>> tgl_revids_get(p)->disp_stepping <= (until))
>>
>>
>>
>>
>>
>>
>>
>>
>> -#define IS_TGL_UY_GT_REVID(p, since, until) \
>> - ((IS_TGL_U(p) || IS_TGL_Y(p)) && \
>> - tgl_uy_revids->gt_stepping >= (since) && \
>> - tgl_uy_revids->gt_stepping <= (until))
>> -
>> #define IS_TGL_GT_REVID(p, since, until) \
>> (IS_TIGERLAKE(p) && \
>> - !(IS_TGL_U(p) || IS_TGL_Y(p)) && \
>> - tgl_revids->gt_stepping >= (since) && \
>> - tgl_revids->gt_stepping <= (until))
>> + tgl_revids_get(p)->gt_stepping >= (since) && \
>> + tgl_revids_get(p)->gt_stepping <= (until))
>>
>>
>>
>>
>>
>>
>>
>>
>> #define RKL_REVID_A0 0x0
>> #define RKL_REVID_B0 0x1
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index a20b5051f18c..69840aa0d4db 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -7110,7 +7110,7 @@ static void tgl_init_clock_gating(struct drm_i915_private *dev_priv)
>> ILK_DPFC_CHICKEN_COMP_DUMMY_PIXEL);
>>
>>
>>
>>
>>
>>
>>
>>
>> /* Wa_1409825376:tgl (pre-prod)*/
>> - if (IS_TGL_DISP_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_B1))
>> + if (IS_TGL_DISP_REVID(dev_priv, REVID_A0, REVID_B1))
>> I915_WRITE(GEN9_CLKGATE_DIS_3, I915_READ(GEN9_CLKGATE_DIS_3) |
>> TGL_VRH_GATING_DIS);
>>
>>
>>
>>
>>
>>
>>
>>
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2020-11-17 19:29 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-17 18:50 [Intel-gfx] [PATCH 00/21] Introduce Alderlake-S Aditya Swarup
2020-11-17 18:50 ` [Intel-gfx] [PATCH 01/21] drm/i915/dg1: Enable ports Aditya Swarup
2020-11-17 18:50 ` [Intel-gfx] [PATCH 02/21] drm/i915/tgl: Fix macros for TGL SOC based WA Aditya Swarup
2020-11-17 19:03 ` Souza, Jose
2020-11-17 19:28 ` Lucas De Marchi [this message]
2020-11-17 19:33 ` Souza, Jose
2020-11-18 7:56 ` Lucas De Marchi
2020-11-17 19:31 ` Lucas De Marchi
2020-11-18 9:18 ` Jani Nikula
2020-11-24 1:32 ` Aditya Swarup
2020-11-24 13:14 ` Lucas De Marchi
2020-11-24 14:20 ` Jani Nikula
2020-11-24 20:11 ` Lucas De Marchi
2020-11-25 0:48 ` Aditya Swarup
2020-11-25 8:36 ` Jani Nikula
2020-11-17 18:50 ` [Intel-gfx] [PATCH 03/21] drm/i915/adl_s: Add ADL-S platform info and PCI ids Aditya Swarup
2020-11-17 19:17 ` Jani Nikula
2020-11-24 1:50 ` Aditya Swarup
2020-11-24 9:28 ` Jani Nikula
2020-11-17 18:50 ` [Intel-gfx] [PATCH 04/21] x86/gpu: add ADL_S stolen memory support Aditya Swarup
2020-11-17 18:50 ` [Intel-gfx] [PATCH 05/21] drm/i915/adl_s: Add PCH support Aditya Swarup
2020-11-20 0:09 ` Matt Roper
2020-11-17 18:50 ` [Intel-gfx] [PATCH 06/21] drm/i915/adl_s: Add Interrupt Support Aditya Swarup
2020-11-20 0:12 ` Matt Roper
2020-11-17 18:50 ` [Intel-gfx] [PATCH 07/21] drm/i915/adl_s: Add PHYs for Alderlake S Aditya Swarup
2020-11-20 0:20 ` Matt Roper
2020-11-17 18:50 ` [Intel-gfx] [PATCH 08/21] drm/i915/adl_s: Configure DPLL for ADL-S Aditya Swarup
2020-11-17 18:50 ` [Intel-gfx] [PATCH 09/21] drm/i915/adl_s: Configure Port clock registers " Aditya Swarup
2020-11-17 18:50 ` [Intel-gfx] [PATCH 10/21] drm/i915/adl_s: Add HTI support and initialize display " Aditya Swarup
2020-11-20 0:27 ` Matt Roper
2020-11-17 18:50 ` [Intel-gfx] [PATCH 11/21] drm/i915/adl_s: Add adl-s ddc pin mapping Aditya Swarup
2020-11-20 0:33 ` Matt Roper
2020-11-17 18:50 ` [Intel-gfx] [PATCH 12/21] drm/i915/adl_s: Add vbt port and aux channel settings for adls Aditya Swarup
2020-11-17 18:50 ` [Intel-gfx] [PATCH 13/21] drm/i915/adl_s: Update combo PHY master/slave relationships Aditya Swarup
2020-11-25 23:38 ` Srivatsa, Anusha
2020-11-17 18:50 ` [Intel-gfx] [PATCH 14/21] drm/i915/adl_s: Update PHY_MISC programming Aditya Swarup
2020-11-17 18:50 ` [Intel-gfx] [PATCH 15/21] drm/i915/adl_s: Add display, gt, ctx and ADL-S Aditya Swarup
2020-12-01 18:46 ` Srivatsa, Anusha
2020-12-01 20:51 ` Lucas De Marchi
2020-11-17 18:50 ` [Intel-gfx] [PATCH 16/21] drm/i915/adl_s: MCHBAR memory info registers are moved Aditya Swarup
2020-11-20 20:18 ` Lucas De Marchi
2020-11-20 20:39 ` Caz Yokoyama
2020-11-25 0:11 ` Lucas De Marchi
2020-11-17 18:50 ` [Intel-gfx] [PATCH 17/21] drm/i915/adl_s: Add power wells Aditya Swarup
2020-11-17 18:50 ` [Intel-gfx] [PATCH 18/21] drm/i915/adl_s: Re-use TGL GuC/HuC firmware Aditya Swarup
2020-11-25 22:52 ` Srivatsa, Anusha
2020-11-17 18:50 ` [Intel-gfx] [PATCH 19/21] drm/i915/display: Add HAS_D12_PLANE_MINIMIZATION Aditya Swarup
2020-12-01 18:35 ` Srivatsa, Anusha
2020-11-17 18:50 ` [Intel-gfx] [PATCH 20/21] drm/i915/adl_s: Load DMC Aditya Swarup
2020-11-17 18:50 ` [Intel-gfx] [PATCH 21/21] drm/i915/adl_s: Update memory bandwidth parameters Aditya Swarup
2020-11-25 22:46 ` Srivatsa, Anusha
2020-11-18 1:28 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Introduce Alderlake-S (rev2) Patchwork
2020-11-18 1:29 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2020-11-18 1:57 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-11-18 7:53 ` [Intel-gfx] [PATCH 00/21] Introduce Alderlake-S Lucas De Marchi
2020-11-18 15:14 ` [Intel-gfx] ✗ Fi.CI.IGT: failure for Introduce Alderlake-S (rev2) Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201117192854.57tki3vges6zdenp@ldmartin-desk1 \
--to=lucas.demarchi@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jani.nikula@intel.com \
--cc=jose.souza@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.