Hi Sagar,

sorry for the delay, see below for my comments.

On Wed, 2015-11-04 at 15:34 +0530, Sagar Arun Kamble wrote:
> RC6 setup is shared between BIOS and Driver. BIOS sets up subset of RC6
> setup registers. If those are not setup Driver should not enable RC6.
> For implementing this, driver can check RC_CTRL0 and RC_CTRL1 values
> to know if BIOS has enabled HW/SW RC6.
> This will also enable user to control RC6 using BIOS settings alone.
> RC6 related instability can be avoided by disabling via BIOS settings
> till driver fixes it.
> 
> RC6 setup is shared between BIOS and Driver. BIOS sets up subset of
> RC6 configuration registers. If those are not setup Driver should not
> enable RC6.
> For implementing this, driver can check RC_CTRL0 and RC_CTRL1 values
> to know if BIOS has enabled HW/SW RC6.
> This will also enable user to control RC6 using BIOS settings alone.
> 
> v2: Had placed logic in gen8 function by mistake. Fixed it.
> Ensuring RPM is not enabled in case BIOS disabled RC6.
> 
> v3: Need to disable RPM if RC6 is disabled due to BIOS settings. (Daniel)
> Runtime PM enabling happens before gen9_enable_rc6.
> Moved the updation of enable_rc6 parameter in intel_uncore_sanitize.
> 
> Change-Id: If89518708e133be6b3c7c6f90869fb66224b7b87
> Signed-off-by: Sagar Arun Kamble <sagar.a.kam...@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h     |  1 +
>  drivers/gpu/drm/i915/intel_uncore.c | 27 +++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8942532..6ed3542 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6823,6 +6823,7 @@ enum skl_disp_power_wells {
>  #define GEN6_RPDEUC                          0xA084
>  #define GEN6_RPDEUCSW                                0xA088
>  #define GEN6_RC_STATE                                0xA094
> +#define RC6_STATE                            (1<<18)
>  #define GEN6_RC1_WAKE_RATE_LIMIT             0xA098
>  #define GEN6_RC6_WAKE_RATE_LIMIT             0xA09C
>  #define GEN6_RC6pp_WAKE_RATE_LIMIT           0xA0A0
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c 
> b/drivers/gpu/drm/i915/intel_uncore.c
> index f0f97b2..bedb089 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -364,8 +364,35 @@ void intel_uncore_early_sanitize(struct drm_device *dev, 
> bool restore_forcewake)
>       i915_check_and_clear_faults(dev);
>  }
>  
> +static void sanitize_bios_rc6_setup(const struct drm_device *dev)
> +{
> +     struct drm_i915_private *dev_priv = dev->dev_private;
> +     bool hw_rc6_enabled = false, sw_rc6_enabled = false;
> +
> +     if (IS_BROXTON(dev)) {
> +             /* Get forcewake during program sequence. Although the driver
> +              * hasn't enabled a state yet where we need forcewake, BIOS may 
> have.*/
> +             intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);

Do we really need FW for reading? We don't program things here and
I915_READ() does take FW for the access itself.

> +
> +             /* Check if BIOS has enabled HW/SW RC6. Only then enable RC6 */
> +             hw_rc6_enabled = I915_READ(GEN6_RC_CONTROL) &
> +                                     (GEN6_RC_CTL_RC6_ENABLE | 
> GEN6_RC_CTL_HW_ENABLE);
> +             sw_rc6_enabled = !(I915_READ(GEN6_RC_CONTROL) & 
> GEN6_RC_CTL_HW_ENABLE)
> +                                     && (I915_READ(GEN6_RC_STATE) & 
> RC6_STATE);

Could you also add the following checks and bail out if anyone is
invalid?:

RC6LOCATION (0xd40) [0] should be 1.
RC6CTXBASE (0xd48) [31:4] should be non-zero. We could also check if it
falls within the WOPCM as it should.
PWRCTX_MAXCNT_RCSUNIT (0x2054),
 PWRCTX_MAXCNT_VCSUNIT0 (0x12054),
 PWRCTX_MAXCNT_BCSUNIT (0x22054),
 PWRCTX_MAXCNT_VECSUNIT (0x1A054),
 PWRCTX_MAXCNT_VCSUNIT1 (0x1C054) [19:0] should be greater than 1.

> +
> +             intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> +
> +             if (!hw_rc6_enabled && !sw_rc6_enabled) {
> +                     i915.enable_rc6 = 0;
> +                     DRM_INFO("RC6 disabled by BIOS\n");
> +             }
> +     }
> +}
> +
>  void intel_uncore_sanitize(struct drm_device *dev)
>  {
> +     sanitize_bios_rc6_setup(dev);
> +

I'd prefer keeping this together with the RC6-sanitize code done for
other platforms in intel_init_gt_powersave(). You could also add a
broxton_check_pctx() similar to VLV/CHV that would perform the above
sanity checks any time intel_enable_gt_powersave() is called (in
addition to the check during intel_init_gt_powersave()).

>       /* BIOS often leaves RC6 enabled, but disable it for hw init */
>       intel_disable_gt_powersave(dev);
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to