Message-ID: <1475695951.2381.48.camel@intel.com>
Subject: Re: [Intel-gfx] [PATCH 3/6] drm/i915: Add enable_sagv option
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
To: Lyude <cpaul@redhat.com>, intel-gfx@lists.freedesktop.org
Cc: David Airlie <airlied@linux.ie>, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org,
        Daniel Vetter <daniel.vetter@intel.com>
Date: Wed, 05 Oct 2016 16:32:31 -0300
In-Reply-To: <1475681598-12081-4-git-send-email-cpaul@redhat.com>
References: <1475681598-12081-1-git-send-email-cpaul@redhat.com>
         <1475681598-12081-4-git-send-email-cpaul@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3935
Lines: 108

Em Qua, 2016-10-05 às 11:33 -0400, Lyude escreveu:
> This option allows us to manually control the SAGV at module load
> time.
> This can be useful in situations such as trying to debug watermark
> changes, since enabled SAGV + incorrect watermarks = total GPU
> annihilation.

I'm not a huge fan of adding options that are only for very limited
debugging situations, especially simple ones that can always just be
re-implemented during debugging sessions such as this one. Anyway, I'm
not opposed to adding the option since it's marked as unsafe anyway,
I'm just stating my general opinion. See more below.


> 
> Signed-off-by: Lyude <cpaul@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_params.c   |  5 +++++
>  drivers/gpu/drm/i915/i915_params.h   |  1 +
>  drivers/gpu/drm/i915/intel_display.c | 16 +++++++++++++---
>  3 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_params.c
> b/drivers/gpu/drm/i915/i915_params.c
> index 768ad89..f462cd4 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -62,6 +62,7 @@ struct i915_params i915 __read_mostly = {
>  	.inject_load_failure = 0,
>  	.enable_dpcd_backlight = false,
>  	.enable_gvt = false,
> +	.enable_sagv = -1,
>  };
>  
>  module_param_named(modeset, i915.modeset, int, 0400);
> @@ -233,3 +234,7 @@ MODULE_PARM_DESC(enable_dpcd_backlight,
>  module_param_named(enable_gvt, i915.enable_gvt, bool, 0400);
>  MODULE_PARM_DESC(enable_gvt,
>  	"Enable support for Intel GVT-g graphics virtualization host
> support(default:false)");
> +
> +module_param_named_unsafe(enable_sagv, i915.enable_sagv, int, 0400);
> +MODULE_PARM_DESC(enable_sagv,
> +	"Enable the SAGV (gen9+ only)(1=enabled, 0=disabled,
> -1=driver discretion [default])");
> diff --git a/drivers/gpu/drm/i915/i915_params.h
> b/drivers/gpu/drm/i915/i915_params.h
> index 3a0dd78..a7db125 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -65,6 +65,7 @@ struct i915_params {
>  	bool enable_dp_mst;
>  	bool enable_dpcd_backlight;
>  	bool enable_gvt;
> +	int enable_sagv;
>  };
>  
>  extern struct i915_params i915 __read_mostly;
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index a71d05a..dd15ae2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -16904,12 +16904,22 @@ intel_modeset_setup_hw_state(struct
> drm_device *dev)
>  		pll->on = false;
>  	}
>  
> -	if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
> +	if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) {
>  		vlv_wm_get_hw_state(dev);
> -	else if (IS_GEN9(dev))
> +	} else if (IS_GEN9(dev)) {
>  		skl_wm_get_hw_state(dev);
> -	else if (HAS_PCH_SPLIT(dev))
> +
> +		if (i915.enable_sagv != -1) {
> +			if (i915.enable_sagv)
> +				intel_enable_sagv(dev_priv);
> +			else
> +				intel_disable_sagv(dev_priv);
> +
> +			dev_priv->sagv_status =
> I915_SAGV_NOT_CONTROLLED;

Adding this code to the middle of a get_hw_state() if-ladder doesn't
seem to be the best approach. My suggestion would be to create
intel_setup_sagv() (or intel_init_sagv) and then call it from somewhere
(maybe the end of this function?).

Also, I915_SAGV_NOT_CONTROLLED is only used on Skylake. By setting
sagv_status to to you're making i915.enable_sagv behave differently on
SKL compared to "all the other" (aka only KBL now) platforms. It would
probably be better to have unified behavior, maybe by reworking the
I915_SAGV_NOT_CONTROLLED handling or just adding a new flag or
something else.


> +		}
> +	} else if (HAS_PCH_SPLIT(dev)) {
>  		ilk_wm_get_hw_state(dev);
> +	}
>  
>  	for_each_intel_crtc(dev, crtc) {
>  		unsigned long put_domains;