[Intel-gfx] [PATCH] drm/i915: Tweaked Wa_14010685332 for PCHs used on gen11 platforms
Lucas De Marchi
lucas.demarchi at intel.com
Thu Nov 5 23:10:40 UTC 2020
On Thu, Nov 05, 2020 at 10:47:15AM +0530, Anshuman Gupta wrote:
>On 2020-11-03 at 17:06:42 -0500, Rodrigo Vivi wrote:
>> On Fri, Oct 30, 2020 at 11:46:58AM +0530, Anshuman Gupta wrote:
>> > From: Bob Paauwe <bob.j.paauwe at intel.com>
>> >
>> > The WA specifies that we need to toggle a SDE chicken bit on and then
>> > off as the final step in preparation for s0ix entry.
>> >
>> > Bspec: 33450
>> > Bspec: 8402
>> >
>> > However, something is happening after we toggle the bit that causes
>> > the WA to be invalidated. This makes dispcnlunit1_cp_xosc_clkreq
>> > active being already in s0ix state i.e SLP_S0 counter incremented.
>> > Tweaking the Wa_14010685332 by setting the bit on suspend and clearing
>> > it on resume turns down the dispcnlunit1_cp_xosc_clkreq.
>> > B.Spec has Documented this tweaked sequence of WA as an alternative.
>> > Let keep this tweaked WA for Gen11 platforms and keep untweaked WA for
>> > other platforms which never observed this issue.
>> >
>> > v2 (MattR):
>> > - Change the comment on the workaround to give PCH names rather than
>> > platform names. Although the bspec is setup to list workarounds by
>> > platform, the hardware team has confirmed that the actual issue being
>> > worked around here is something that was introduced back in the
>> > Cannon Lake PCH and carried forward to subsequent PCH's.
>> > - Extend the untweaked version of the workaround to include PCH_CNP as
>> > well. Note that since PCH_CNP is used to represent CMP, this will
>> > apply on CML and some variants of RKL too.
>> > - Cap the untweaked version of the workaround so that it won't apply to
>> > "fake" PCH's (i.e., DG1). The issue we're working around really is
>> > an issue in the PCH itself, not the South Display, so it shouldn't
>> > apply when there isn't a real PCH.
>> >
>> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> > Signed-off-by: Bob Paauwe <bob.j.paauwe at intel.com>
>> > Signed-off-by: Anshuman Gupta <anshuman.gupta at intel.com>
>> > Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>> > ---
>> > .../drm/i915/display/intel_display_power.c | 21 +++++++++++++++++--
>> > drivers/gpu/drm/i915/i915_irq.c | 6 ++++--
>> > 2 files changed, 23 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
>> > index 689922480661..d2a6518329d7 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
>> > @@ -5858,17 +5858,34 @@ static void intel_power_domains_verify_state(struct drm_i915_private *i915)
>> >
>> > void intel_display_power_suspend_late(struct drm_i915_private *i915)
>> > {
>> > - if (INTEL_GEN(i915) >= 11 || IS_GEN9_LP(i915))
>> > + u32 val;
>> > +
>> > + if (INTEL_GEN(i915) >= 11 || IS_GEN9_LP(i915)) {
>> > bxt_enable_dc9(i915);
>> > - else if (IS_HASWELL(i915) || IS_BROADWELL(i915))
>> > + /* Tweaked Wa_14010685332:icp,jsp,mcc */
>> > + if (INTEL_PCH_TYPE(i915) >= PCH_ICP && INTEL_PCH_TYPE(i915) <= PCH_MCC) {
>> > + val = intel_de_read(i915, SOUTH_CHICKEN1);
>> > + val |= SBCLK_RUN_REFCLK_DIS;
>> > + intel_de_write(i915, SOUTH_CHICKEN1, val);
>>
>> could we use intel_de_rmw here?
>May be i had misunderstod it earlier, i thought it was your recommendation
>to use manual read, modify write without using intel_uncore_rmw(),
>Was the actual idea to use intel_de_rmw flavour of API instead of intel_uncore_rmw?
intel_de_rmw() is the exact equivalent of what's done above. As this is
a PCH register I think it would be appropriate to use intel_de_*
Jani, is intel_de_* meant to be only a shortcut or are we going to
enforce accessing only DE registers with it?
>Also would it require to use at original Wa in gen11_display_irq_reset as well?
since that file is outside display/ we'd need to be very careful in using
intel_de_* there.
thanks
Lucas De Marchi
>Thanks,
>Anshuman Gupta.
>>
>> > + }
>> > + } else if (IS_HASWELL(i915) || IS_BROADWELL(i915)) {
>> > hsw_enable_pc8(i915);
>> > + }
>> > }
>> >
>> > void intel_display_power_resume_early(struct drm_i915_private *i915)
>> > {
>> > + u32 val;
>> > +
>> > if (INTEL_GEN(i915) >= 11 || IS_GEN9_LP(i915)) {
>> > gen9_sanitize_dc_state(i915);
>> > bxt_disable_dc9(i915);
>> > + /* Tweaked Wa_14010685332:icp,jsp,mcc */
>> > + if (INTEL_PCH_TYPE(i915) >= PCH_ICP && INTEL_PCH_TYPE(i915) <= PCH_MCC) {
>> > + val = intel_de_read(i915, SOUTH_CHICKEN1);
>> > + val &= ~SBCLK_RUN_REFCLK_DIS;
>> > + intel_de_write(i915, SOUTH_CHICKEN1, val);
>>
>> and here?
>>
>> sorry for not having spotted that sooner.
>>
>> > + }
>> > } else if (IS_HASWELL(i915) || IS_BROADWELL(i915)) {
>> > hsw_disable_pc8(i915);
>> > }
>> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> > index dc33c96d741d..410c03624c6a 100644
>> > --- a/drivers/gpu/drm/i915/i915_irq.c
>> > +++ b/drivers/gpu/drm/i915/i915_irq.c
>> > @@ -3055,8 +3055,10 @@ static void gen11_display_irq_reset(struct drm_i915_private *dev_priv)
>> > if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP)
>> > GEN3_IRQ_RESET(uncore, SDE);
>> >
>> > - /* Wa_14010685332:icl,jsl,ehl,tgl,rkl */
>> > - if (INTEL_PCH_TYPE(dev_priv) >= PCH_ICP) {
>> > + /* Wa_14010685332:cnp/cmp,tgp,adp */
>> > + if (INTEL_PCH_TYPE(dev_priv) == PCH_CNP ||
>> > + (INTEL_PCH_TYPE(dev_priv) >= PCH_TGP &&
>> > + INTEL_PCH_TYPE(dev_priv) < PCH_DG1)) {
>> > intel_uncore_rmw(uncore, SOUTH_CHICKEN1,
>> > SBCLK_RUN_REFCLK_DIS, SBCLK_RUN_REFCLK_DIS);
>> > intel_uncore_rmw(uncore, SOUTH_CHICKEN1,
>> > --
>> > 2.26.2
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx at lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list