Re: [PATCH] drm/i915: Remove unsafe i915.enable_rc6

From: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Cc: Jani Nikula <jani.nikula@intel.com>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Rodrigo Vivi <rodrigo.vivi@intel.com>
Subject: Re: [PATCH] drm/i915: Remove unsafe i915.enable_rc6
Date: Thu, 12 Oct 2017 12:37:39 +0300	[thread overview]
Message-ID: <1507801059.5138.40.camel@linux.intel.com> (raw)
In-Reply-To: <20171011091205.11302-1-chris@chris-wilson.co.uk>

On Wed, 2017-10-11 at 10:12 +0100, Chris Wilson wrote:
> It has been many years since the last confirmed sighting (and fix) of an
> RC6 related bug (usually a system hang). Remove the parameter to stop
> users from setting dangerous values, as they often set it during triage
> and end up disabling the entire runtime pm instead (the option is not a
> fine scalpel!).
> 
> Furthermore, it allows users to set known dangerous values which were
> intended for testing and not for production use. For testing, we can
> always patch in the required setting without having to expose ourselves
> to random abuse.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

<SNIP>

> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -215,6 +215,8 @@ static const struct intel_device_info intel_gm45_info __initconst = {
>  	GEN_DEFAULT_PAGE_SIZES, \
>  	CURSOR_OFFSETS
>  
> +/* Ironlake does support rc6, but we do not implement [power] contexts */

This would feel more home above an explicit .has_rc6 = 0

Maybe;

/* Ironlake ... */
#define ILK_D_PLATFORM \
	GEN5_FEATURES, \
	.platform = INTEL_IRONLAKE, \
	.has_rc6 = 0

Or just generalize to GEN5_FEATURES and lift the comment on top of it.

> +
>  static const struct intel_device_info intel_ironlake_d_info __initconst = {
>  	GEN5_FEATURES,
>  	.platform = INTEL_IRONLAKE,

<SNIP>

> @@ -6763,22 +6709,12 @@ static void gen6_enable_rc6(struct drm_i915_private *dev_priv)
>  	I915_WRITE(GEN6_RC6p_THRESHOLD, 150000);
>  	I915_WRITE(GEN6_RC6pp_THRESHOLD, 64000); /* unused */
>  
> -	/* Check if we are enabling RC6 */
> -	rc6_mode = intel_rc6_enabled();
> -	if (rc6_mode & INTEL_RC6_ENABLE)
> -		rc6_mask |= GEN6_RC_CTL_RC6_ENABLE;
> -
>  	/* We don't use those on Haswell */

Comment feels bit alone now, just drop it.

> -	if (!IS_HASWELL(dev_priv)) {
> -		if (rc6_mode & INTEL_RC6p_ENABLE)
> -			rc6_mask |= GEN6_RC_CTL_RC6p_ENABLE;
> -
> -		if (rc6_mode & INTEL_RC6pp_ENABLE)
> -			rc6_mask |= GEN6_RC_CTL_RC6pp_ENABLE;
> -	}
> -
> -	intel_print_rc6_info(dev_priv, rc6_mask);
> -
> +	rc6_mask = GEN6_RC_CTL_RC6_ENABLE;
> +	if (HAS_RC6p(dev_priv))
> +		rc6_mask |= GEN6_RC_CTL_RC6p_ENABLE;
> +	if (HAS_RC6pp(dev_priv))
> +		rc6_mask |= GEN6_RC_CTL_RC6pp_ENABLE;
>  	I915_WRITE(GEN6_RC_CONTROL,
>  		   rc6_mask |
>  		   GEN6_RC_CTL_EI_MODE(1) |

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx