Re: [Intel-gfx] [PATCH 4/5] drm/i915: Create a new category of display WAs

2018-02-13 Thread Oscar Mateo



On 2/13/2018 12:50 PM, Chris Wilson wrote:

Quoting Jani Nikula (2018-02-13 20:02:55)

On Tue, 13 Feb 2018, Oscar Mateo  wrote:

@@ -8979,6 +8701,7 @@ static void i830_init_clock_gating(struct 
drm_i915_private *dev_priv)
  void intel_init_clock_gating(struct drm_i915_private *dev_priv)
  {
   dev_priv->display.init_clock_gating(dev_priv);
+ intel_disp_workarounds_apply(dev_priv);

Please don't abbreviate display to disp, use the full word instead,
throughout the series. Rationale:

And in general, use the full name for global functions and data. They
are written infrequently so not overburdensome, but the clarity is a
good reward. Locals by contrast quite often are more readable when using
(consistent) shorthand, or at least I think so.
-Chris


Sure, makes sense. I'll rename a few things and resend.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH 4/5] drm/i915: Create a new category of display WAs

2018-02-13 Thread Chris Wilson
Quoting Jani Nikula (2018-02-13 20:02:55)
> On Tue, 13 Feb 2018, Oscar Mateo  wrote:
> > @@ -8979,6 +8701,7 @@ static void i830_init_clock_gating(struct 
> > drm_i915_private *dev_priv)
> >  void intel_init_clock_gating(struct drm_i915_private *dev_priv)
> >  {
> >   dev_priv->display.init_clock_gating(dev_priv);
> > + intel_disp_workarounds_apply(dev_priv);
> 
> Please don't abbreviate display to disp, use the full word instead,
> throughout the series. Rationale:

And in general, use the full name for global functions and data. They
are written infrequently so not overburdensome, but the clarity is a
good reward. Locals by contrast quite often are more readable when using
(consistent) shorthand, or at least I think so.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


Re: [Intel-gfx] [PATCH 4/5] drm/i915: Create a new category of display WAs

2018-02-13 Thread Jani Nikula
On Tue, 13 Feb 2018, Oscar Mateo  wrote:
> @@ -8979,6 +8701,7 @@ static void i830_init_clock_gating(struct 
> drm_i915_private *dev_priv)
>  void intel_init_clock_gating(struct drm_i915_private *dev_priv)
>  {
>   dev_priv->display.init_clock_gating(dev_priv);
> + intel_disp_workarounds_apply(dev_priv);

Please don't abbreviate display to disp, use the full word instead,
throughout the series. Rationale:

$ git grep disp_ -- drivers/gpu/drm/i915 | wc -l
8

$ git grep display_ -- drivers/gpu/drm/i915 | wc -l
752

> diff --git a/drivers/gpu/drm/i915/intel_workarounds.c 
> b/drivers/gpu/drm/i915/intel_workarounds.c

> +static void bdw_disp_workarounds_apply(struct drm_i915_private *dev_priv)

If you want to shorten the names within intel_workarounds.c please
abbreviate workarounds to wa instead. I think it should be obvious
within that file. But I don't mind longer names here. It's the long
names that are used throughout the driver that I'd avoid.


BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx


[Intel-gfx] [PATCH 4/5] drm/i915: Create a new category of display WAs

2018-02-13 Thread Oscar Mateo
Display workarounds do not need to be re-applied on a GPU reset
(this is, in Ville's words: "at the very least wasted effort [...]
and could even be actively harmful in case we end up clobbering
something the current display configuration depends on"). Therefore,
they have to be applied in a different place that GT ones so they
deserve their own category.

Initially, populate this with the WAs that were being applied in
init_clock_gating. Actually discriminating which of these WAS is
a real display WA, a GT WA or a context WA is left for the next
patch (as it requires a good deal of careful review).

v2: Rebased to carry the init_early nomenclature over (Chris)
v3:
  - Rebased to before the WAs are stored
  - Initially populate with all WAs in init_clock_gating
v4: Rebased

Suggested-by: Ville Syrjälä 
Reviewed-by: Chris Wilson  (v2)
Signed-off-by: Oscar Mateo 
Cc: Mika Kuoppala 
Cc: Rodrigo Vivi 
---
 drivers/gpu/drm/i915/intel_pm.c  | 304 +-
 drivers/gpu/drm/i915/intel_workarounds.c | 309 +++
 drivers/gpu/drm/i915/intel_workarounds.h |   2 +
 3 files changed, 319 insertions(+), 296 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b026b02..6917a9d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -29,6 +29,7 @@
 #include 
 #include "i915_drv.h"
 #include "intel_drv.h"
+#include "intel_workarounds.h"
 #include "../../../platform/x86/intel_ips.h"
 #include 
 #include 
@@ -53,92 +54,6 @@
  * require higher latency to switch to and wake up.
  */
 
-static void gen9_init_clock_gating(struct drm_i915_private *dev_priv)
-{
-   if (HAS_LLC(dev_priv)) {
-   /*
-* WaCompressedResourceDisplayNewHashMode:skl,kbl
-* Display WA #0390: skl,kbl
-*
-* Must match Sampler, Pixel Back End, and Media. See
-* WaCompressedResourceSamplerPbeMediaNewHashMode.
-*/
-   I915_WRITE(CHICKEN_PAR1_1,
-  I915_READ(CHICKEN_PAR1_1) |
-  SKL_DE_COMPRESSED_HASH_MODE);
-   }
-
-   /* See Bspec note for PSR2_CTL bit 31, Wa#828:skl,bxt,kbl,cfl */
-   I915_WRITE(CHICKEN_PAR1_1,
-  I915_READ(CHICKEN_PAR1_1) | SKL_EDP_PSR_FIX_RDWRAP);
-
-   /* WaEnableChickenDCPR:skl,bxt,kbl,glk,cfl */
-   I915_WRITE(GEN8_CHICKEN_DCPR_1,
-  I915_READ(GEN8_CHICKEN_DCPR_1) | MASK_WAKEMEM);
-
-   /* WaFbcTurnOffFbcWatermark:skl,bxt,kbl,cfl */
-   /* WaFbcWakeMemOn:skl,bxt,kbl,glk,cfl */
-   I915_WRITE(DISP_ARB_CTL, I915_READ(DISP_ARB_CTL) |
-  DISP_FBC_WM_DIS |
-  DISP_FBC_MEMORY_WAKE);
-
-   /* WaFbcHighMemBwCorruptionAvoidance:skl,bxt,kbl,cfl */
-   I915_WRITE(ILK_DPFC_CHICKEN, I915_READ(ILK_DPFC_CHICKEN) |
-  ILK_DPFC_DISABLE_DUMMY0);
-
-   if (IS_SKYLAKE(dev_priv)) {
-   /* WaDisableDopClockGating */
-   I915_WRITE(GEN7_MISCCPCTL, I915_READ(GEN7_MISCCPCTL)
-  & ~GEN7_DOP_CLOCK_GATE_ENABLE);
-   }
-}
-
-static void bxt_init_clock_gating(struct drm_i915_private *dev_priv)
-{
-   gen9_init_clock_gating(dev_priv);
-
-   /* WaDisableSDEUnitClockGating:bxt */
-   I915_WRITE(GEN8_UCGCTL6, I915_READ(GEN8_UCGCTL6) |
-  GEN8_SDEUNIT_CLOCK_GATE_DISABLE);
-
-   /*
-* FIXME:
-* GEN8_HDCUNIT_CLOCK_GATE_DISABLE_HDCREQ applies on 3x6 GT SKUs only.
-*/
-   I915_WRITE(GEN8_UCGCTL6, I915_READ(GEN8_UCGCTL6) |
-  GEN8_HDCUNIT_CLOCK_GATE_DISABLE_HDCREQ);
-
-   /*
-* Wa: Backlight PWM may stop in the asserted state, causing backlight
-* to stay fully on.
-*/
-   I915_WRITE(GEN9_CLKGATE_DIS_0, I915_READ(GEN9_CLKGATE_DIS_0) |
-  PWM1_GATING_DIS | PWM2_GATING_DIS);
-}
-
-static void glk_init_clock_gating(struct drm_i915_private *dev_priv)
-{
-   gen9_init_clock_gating(dev_priv);
-
-   /*
-* WaDisablePWMClockGating:glk
-* Backlight PWM may stop in the asserted state, causing backlight
-* to stay fully on.
-*/
-   I915_WRITE(GEN9_CLKGATE_DIS_0, I915_READ(GEN9_CLKGATE_DIS_0) |
-  PWM1_GATING_DIS | PWM2_GATING_DIS);
-
-   /* WaDDIIOTimeout:glk */
-   if (IS_GLK_REVID(dev_priv, 0, GLK_REVID_A1)) {
-   u32 val = I915_READ(CHICKEN_MISC_2);
-   val &= ~(GLK_CL0_PWR_DOWN |
-GLK_CL1_PWR_DOWN |
-GLK_CL2_PWR_DOWN);
-   I915_WRITE(CHICKEN_MISC_2, val);
-   }
-
-}
-
 static void i915_pineview_get_mem_freq(struct drm_i915_private *dev_priv)
 {
u32 tmp;
@@ -8457,168 +8372,9 @@ static void lpt_suspend_hw(struct drm_i915_private 
*dev_priv)
}
 }
 
-static void gen8_set_l3sqc_credits(struct drm_i915_private *dev_priv,
-