drm/i915: Tweaked Wa_14010685332 for PCHs used on gen11 platforms

Commit Message

Gupta, Anshuman Oct. 30, 2020, 6:16 a.m. UTC

From: Bob Paauwe <[e-mail]>

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 <[e-mail]>
Signed-off-by: Bob Paauwe <[e-mail]>
Signed-off-by: Anshuman Gupta <[e-mail]>
Signed-off-by: Matt Roper <[e-mail]>
---
 .../drm/i915/display/intel_display_power.c    | 21 +++++++++++++++++--
 drivers/gpu/drm/i915/i915_irq.c               |  6 ++++--
 2 files changed, 23 insertions(+), 4 deletions(-)

Comments

Rodrigo Vivi Nov. 3, 2020, 10:06 p.m. UTC | #1

On Fri, Oct 30, 2020 at 11:46:58AM +0530, Anshuman Gupta wrote:
> From: Bob Paauwe <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Bob Paauwe <[e-mail]>
> Signed-off-by: Anshuman Gupta <[e-mail]>
> Signed-off-by: Matt Roper <[e-mail]>
> ---
>  .../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?

> +		}
> +	} 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
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Gupta, Anshuman Nov. 5, 2020, 5:17 a.m. UTC | #2

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 <[e-mail]>
> > 
> > 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 <[e-mail]>
> > Signed-off-by: Bob Paauwe <[e-mail]>
> > Signed-off-by: Anshuman Gupta <[e-mail]>
> > Signed-off-by: Matt Roper <[e-mail]>
> > ---
> >  .../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?
Also would it require to use at original Wa in gen11_display_irq_reset as well?  
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
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Lucas De Marchi Nov. 5, 2020, 11:10 p.m. UTC | #3

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 <[e-mail]>
>> >
>> > 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 <[e-mail]>
>> > Signed-off-by: Bob Paauwe <[e-mail]>
>> > Signed-off-by: Anshuman Gupta <[e-mail]>
>> > Signed-off-by: Matt Roper <[e-mail]>
>> > ---
>> >  .../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
>> > [e-mail]
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>_______________________________________________
>Intel-gfx mailing list
>[e-mail]
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Jani Nikula Nov. 6, 2020, 6:03 a.m. UTC | #4

On Thu, 05 Nov 2020, Lucas De Marchi <[e-mail]> wrote:
> 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 <[e-mail]>
>>> >
>>> > 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 <[e-mail]>
>>> > Signed-off-by: Bob Paauwe <[e-mail]>
>>> > Signed-off-by: Anshuman Gupta <[e-mail]>
>>> > Signed-off-by: Matt Roper <[e-mail]>
>>> > ---
>>> >  .../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?

The intel_de_* family of functions should only be used for DE registers,
and you should assume it'll be enforced in the future.

BR,
Jani.


>
>>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
>>> > [e-mail]
>>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>_______________________________________________
>>Intel-gfx mailing list
>>[e-mail]
>>https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Rodrigo Vivi Nov. 6, 2020, 7:01 p.m. UTC | #5

> On Nov 4, 2020, at 9:17 PM, Anshuman Gupta <[e-mail]> 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 <[e-mail]>
>>> 
>>> 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 <[e-mail]>
>>> Signed-off-by: Bob Paauwe <[e-mail]>
>>> Signed-off-by: Anshuman Gupta <[e-mail]>
>>> Signed-off-by: Matt Roper <[e-mail]>
>>> ---
>>> .../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(),

ouch, I'm really sorry for causing this confusion here.
My recommendation was for debug purposes only on the original w/a
I haven't noticed it had expanded to the alternate/tweaked one.
This shouldn't be needed here.

> Was the actual idea to use intel_de_rmw flavour of API instead of intel_uncore_rmw?

I'm assuming that both south or north registers would still be considered
display engine. Jani can correct me if I'm wrong here.
But I believe _de_ is more appropriated here.

> Also would it require to use at original Wa in gen11_display_irq_reset as well?

any modification there could be done in separated patches as needed.

>  
> 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
>>> [e-mail]
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch

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);
+		}
+	} 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);
+		}
 	} 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,