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);elseintel_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;