On Wed, Mar 14, 2018 at 12:46:58PM +0100, Maarten Lankhorst wrote: > Allow controlling link status through i915_edp_psr_status, in the same way kernel does. > This replaces changing the module parameter at runtime, then forcing a modeset. > > Writing -1 restores the original PSR mode set through the module parameter. > Writing 0 disables PSR. > Writing 1 enables PSR with default link standby mode. > Writing 2 enables PSR with link standby. > Writing 3 enables PSR with link standby disabled. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > --- > This breaks kms_frontbuffer_tracking, and needs the following patch to work: > https://patchwork.freedesktop.org/patch/209956/ > > XXX: Make global_enable a tristate, to keep older versions of kms_frontbuffer_tracking working? > > drivers/gpu/drm/i915/i915_debugfs.c | 182 +++++++++++++++++++++++++++++++++++- > drivers/gpu/drm/i915/i915_drv.h | 2 + > drivers/gpu/drm/i915/i915_params.c | 2 +- > drivers/gpu/drm/i915/intel_drv.h | 7 ++ > drivers/gpu/drm/i915/intel_psr.c | 139 ++++++++++++++++----------- > 5 files changed, 271 insertions(+), 61 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 574fcf234007..534a3b04e6a3 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2546,16 +2546,13 @@ static const char *psr2_live_status(u32 val) > > static int i915_edp_psr_status(struct seq_file *m, void *data) > { > - struct drm_i915_private *dev_priv = node_to_i915(m->private); > + struct drm_i915_private *dev_priv = m->private; > u32 psrperf = 0; > u32 stat[3]; > enum pipe pipe; > bool enabled = false; > bool sink_support; > > - if (!HAS_PSR(dev_priv)) > - return -ENODEV; > - > sink_support = dev_priv->psr.sink_support; > seq_printf(m, "Sink_Support: %s\n", yesno(sink_support)); > if (!sink_support) > @@ -2631,6 +2628,181 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) > return 0; > } > > +static bool psr_needs_disable(struct drm_i915_private *dev_priv, > + bool enable, bool link_standby) > +{ > + if (!dev_priv->psr.global_enable) > + return false; > + > + if (!enable) > + return true; > + > + if (dev_priv->psr.link_standby != link_standby) > + return true; > + > + return false; > +} > + > +static bool psr_needs_enable(struct drm_i915_private *dev_priv, > + bool enable) > +{ > + if (!enable) > + return false; > + > + return !dev_priv->psr.global_enable; > +} > + > +static int __i915_edp_psr_write(struct drm_i915_private *dev_priv, > + struct drm_modeset_acquire_ctx *ctx, > + bool enable, bool link_standby) > +{ > + struct drm_device *dev = &dev_priv->drm; > + struct drm_connector_list_iter conn_iter; > + struct drm_connector *connector; > + struct drm_encoder *encoder; > + struct drm_crtc *crtc; > + int ret; > + bool needs_enable, found; > + > + ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx); > + if (ret) > + return ret; > + > + mutex_lock(&dev_priv->psr.lock); > +retry: > + if (!dev_priv->psr.enabled) { > + dev_priv->psr.global_enable = enable; > + dev_priv->psr.link_standby = link_standby; > + goto end; > + } > + encoder = &dp_to_dig_port(dev_priv->psr.enabled)->base.base; > + mutex_unlock(&dev_priv->psr.lock); > + > + found = false; > + drm_connector_list_iter_begin(dev, &conn_iter); > + drm_for_each_connector_iter(connector, &conn_iter) > + if (connector->state->best_encoder == encoder) { > + found = true; > + break; > + } > + drm_connector_list_iter_end(&conn_iter); > + > + if (WARN_ON(!found)) > + return -EINVAL; > + > + crtc = connector->state->crtc; > + ret = drm_modeset_lock(&crtc->mutex, ctx); > + if (ret) > + return ret; > + > + mutex_lock(&dev_priv->psr.lock); > + if (dev_priv->psr.enabled != enc_to_intel_dp(encoder)) > + goto retry; > + > + if ((connector->state->commit && !try_wait_for_completion(&connector->state->commit->hw_done)) || > + (crtc->state->commit && !try_wait_for_completion(&crtc->state->commit->hw_done))) { > + ret = -EBUSY; > + goto end; > + } > + > + if (psr_needs_disable(dev_priv, enable, link_standby)) { > + __intel_psr_disable(dev_priv, dev_priv->psr.enabled, to_intel_crtc_state(crtc->state)); > + dev_priv->psr.global_enable = false; > + } > + > + needs_enable = psr_needs_enable(dev_priv, enable); > + dev_priv->psr.global_enable = enable; > + dev_priv->psr.link_standby = link_standby; > + > + if (needs_enable) > + __intel_psr_enable(dev_priv, dev_priv->psr.enabled, to_intel_crtc_state(crtc->state)); > + > +end: > + mutex_unlock(&dev_priv->psr.lock); > + return ret; > +} > + > +static ssize_t i915_edp_psr_write(struct file *file, const char __user *ubuf, > + size_t len, loff_t *offp) > +{ > + struct seq_file *m = file->private_data; > + struct drm_i915_private *dev_priv = m->private; > + struct drm_modeset_acquire_ctx ctx; > + int ret, val; > + bool link_standby; > + > + if (!dev_priv->psr.sink_support) > + return -ENODEV; > + > + ret = kstrtoint_from_user(ubuf, len, 10, &val); > + if (ret < 0) { > + bool enable; > + ret = kstrtobool_from_user(ubuf, len, &enable); > + > + if (ret < 0) > + return ret; > + > + val = enable; > + } > + > + /* -1 -> reset to default */ > + if (val == -1) > + val = i915_modparams.enable_psr; > + > + switch (val) { > + case 0: > + case 1: > + link_standby = intel_psr_default_link_standby(dev_priv); > + break; > + case 2: > + link_standby = true; > + break; > + case 3: > + link_standby = false; > + break; > + default: > + return -EINVAL; > + } > + > + intel_runtime_pm_get(dev_priv); > + > + drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE); > + > +retry: > + ret = __i915_edp_psr_write(dev_priv, &ctx, val, link_standby); > + if (ret == -EBUSY) { > + ret = drm_modeset_backoff(&ctx); > + if (!ret) > + goto retry; > + } > + > + drm_modeset_drop_locks(&ctx); > + drm_modeset_acquire_fini(&ctx); > + > + intel_runtime_pm_put(dev_priv); > + > + return ret ?: len; > +} > + > +static int i915_edp_psr_open(struct inode *inode, struct file *file) > +{ > + struct drm_i915_private *dev_priv = inode->i_private; > + > + if (!HAS_PSR(dev_priv)) > + return -ENODEV; > + > + return single_open(file, i915_edp_psr_status, dev_priv); > +} > + > +static const struct file_operations i915_edp_psr_ops = { > + .owner = THIS_MODULE, > + .open = i915_edp_psr_open, > + .read = seq_read, > + .llseek = seq_lseek, > + .release = single_release, > + .write = i915_edp_psr_write > +}; > + > static int i915_sink_crc(struct seq_file *m, void *data) > { > struct drm_i915_private *dev_priv = node_to_i915(m->private); > @@ -4734,7 +4906,6 @@ static const struct drm_info_list i915_debugfs_list[] = { > {"i915_swizzle_info", i915_swizzle_info, 0}, > {"i915_ppgtt_info", i915_ppgtt_info, 0}, > {"i915_llc", i915_llc, 0}, > - {"i915_edp_psr_status", i915_edp_psr_status, 0}, > {"i915_sink_crc_eDP1", i915_sink_crc, 0}, > {"i915_energy_uJ", i915_energy_uJ, 0}, > {"i915_runtime_pm_status", i915_runtime_pm_status, 0}, > @@ -4761,6 +4932,7 @@ static const struct i915_debugfs_files { > {"i915_wedged", &i915_wedged_fops}, > {"i915_max_freq", &i915_max_freq_fops}, > {"i915_min_freq", &i915_min_freq_fops}, > + {"i915_edp_psr_status", &i915_edp_psr_ops}, > {"i915_cache_sharing", &i915_cache_sharing_fops}, > {"i915_ring_missed_irq", &i915_ring_missed_irq_fops}, > {"i915_ring_test_irq", &i915_ring_test_irq_fops}, > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index b39c5f68efb2..dbc9c062c658 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -608,6 +608,8 @@ struct i915_psr { > bool alpm; > bool has_hw_tracking; > > + bool global_enable; > + > void (*enable_source)(struct intel_dp *, > const struct intel_crtc_state *); > void (*disable_source)(struct intel_dp *, > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > index 08108ce5be21..0d02980b3a2c 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -90,7 +90,7 @@ i915_param_named_unsafe(enable_ppgtt, int, 0400, > "Override PPGTT usage. " > "(-1=auto [default], 0=disabled, 1=aliasing, 2=full, 3=full with extended address space)"); > > -i915_param_named_unsafe(enable_psr, int, 0600, > +i915_param_named_unsafe(enable_psr, int, 0400, > "Enable PSR " > "(0=disabled, 1=enabled - link mode chosen per-platform, 2=force link-standby mode, 3=force link-off mode) " > "Default: -1 (use per-chip default)"); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 1f0e8f1e4594..c486090f4f52 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1871,11 +1871,18 @@ bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port); > > /* intel_psr.c */ > #define CAN_PSR(dev_priv) (HAS_PSR(dev_priv) && dev_priv->psr.sink_support) > +bool intel_psr_default_link_standby(struct drm_i915_private *dev_priv); > void intel_psr_init_dpcd(struct intel_dp *intel_dp); > void intel_psr_enable(struct intel_dp *intel_dp, > const struct intel_crtc_state *crtc_state); > +void __intel_psr_enable(struct drm_i915_private *dev_priv, > + struct intel_dp *intel_dp, > + const struct intel_crtc_state *crtc_state); > void intel_psr_disable(struct intel_dp *intel_dp, > const struct intel_crtc_state *old_crtc_state); > +void __intel_psr_disable(struct drm_i915_private *dev_priv, > + struct intel_dp *intel_dp, > + const struct intel_crtc_state *old_crtc_state); I don't like exporting around functions with "_" also I don't believe you need dev_priv and intel_dp. But why don't simply add another function parameter for disable and enable? something like: void intel_psr_disable(struct intel_dp *intel_dp, const struct intel_crtc_state *old_crtc_state, bool block); void intel_psr_enable(struct intel_dp *intel_dp, const struct intel_crtc_state *crtc_state, bool unblock); And maybe we will need a sort for referrence count on this block because the same mechanism could be used when we need to disable aux for mutex transactions for instance. > void intel_psr_invalidate(struct drm_i915_private *dev_priv, > unsigned frontbuffer_bits, > enum fb_op_origin origin); > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index 317cb4a12693..7f59faef7ea7 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -56,6 +56,36 @@ > #include "intel_drv.h" > #include "i915_drv.h" > > +bool intel_psr_default_link_standby(struct drm_i915_private *dev_priv) > +{ > + /* Override link_standby x link_off defaults */ > + if (i915_modparams.enable_psr == 2) { > + DRM_DEBUG_KMS("PSR: Forcing link standby\n"); > + return true; > + } > + > + if (i915_modparams.enable_psr == 3) { > + DRM_DEBUG_KMS("PSR: Forcing main link off\n"); > + return false; > + } > + > + /* Set link_standby x link_off defaults */ > + if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) > + /* HSW and BDW require workarounds that we don't implement. */ > + return false; > + else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > + /* On VLV and CHV only standby mode is supported. */ > + return true; > + else > + /* For new platforms let's respect VBT back again */ > + return dev_priv->vbt.psr.full_link; > +} > + > +static bool intel_psr_enabled(struct drm_i915_private *dev_priv) > +{ > + return dev_priv->psr.enabled && dev_priv->psr.global_enable; > +} > + > static inline enum intel_display_power_domain > psr_aux_domain(struct intel_dp *intel_dp) > { > @@ -502,11 +532,6 @@ void intel_psr_compute_config(struct intel_dp *intel_dp, > if (!CAN_PSR(dev_priv)) > return; > > - if (!i915_modparams.enable_psr) { > - DRM_DEBUG_KMS("PSR disable by flag\n"); > - return; > - } > - > /* > * HSW spec explicitly says PSR is tied to port A. > * BDW+ platforms with DDI implementation of PSR have different > @@ -559,7 +584,11 @@ void intel_psr_compute_config(struct intel_dp *intel_dp, > > crtc_state->has_psr = true; > crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp, crtc_state); > - DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : ""); > + > + if (dev_priv->psr.global_enable) > + DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : ""); > + else > + DRM_DEBUG_KMS("PSR disable by flag\n"); > } > > static void intel_psr_activate(struct intel_dp *intel_dp) > @@ -617,6 +646,32 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp, > } > } > > +void __intel_psr_enable(struct drm_i915_private *dev_priv, > + struct intel_dp *intel_dp, > + const struct intel_crtc_state *crtc_state) > +{ > + dev_priv->psr.setup_vsc(intel_dp, crtc_state); > + dev_priv->psr.enable_sink(intel_dp); > + dev_priv->psr.enable_source(intel_dp, crtc_state); > + > + if (INTEL_GEN(dev_priv) >= 9) { > + intel_psr_activate(intel_dp); > + } else { > + /* > + * FIXME: Activation should happen immediately since this > + * function is just called after pipe is fully trained and > + * enabled. > + * However on some platforms we face issues when first > + * activation follows a modeset so quickly. > + * - On VLV/CHV we get bank screen on first activation > + * - On HSW/BDW we get a recoverable frozen screen until > + * next exit-activate sequence. > + */ > + schedule_delayed_work(&dev_priv->psr.work, > + msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5)); > + } > +} > + > /** > * intel_psr_enable - Enable PSR > * @intel_dp: Intel DP > @@ -647,27 +702,10 @@ void intel_psr_enable(struct intel_dp *intel_dp, > dev_priv->psr.psr2_support = crtc_state->has_psr2; > dev_priv->psr.busy_frontbuffer_bits = 0; > > - dev_priv->psr.setup_vsc(intel_dp, crtc_state); > - dev_priv->psr.enable_sink(intel_dp); > - dev_priv->psr.enable_source(intel_dp, crtc_state); > dev_priv->psr.enabled = intel_dp; > > - if (INTEL_GEN(dev_priv) >= 9) { > - intel_psr_activate(intel_dp); > - } else { > - /* > - * FIXME: Activation should happen immediately since this > - * function is just called after pipe is fully trained and > - * enabled. > - * However on some platforms we face issues when first > - * activation follows a modeset so quickly. > - * - On VLV/CHV we get bank screen on first activation > - * - On HSW/BDW we get a recoverable frozen screen until > - * next exit-activate sequence. > - */ > - schedule_delayed_work(&dev_priv->psr.work, > - msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5)); > - } > + if (dev_priv->psr.global_enable) > + __intel_psr_enable(dev_priv, intel_dp, crtc_state); > > unlock: > mutex_unlock(&dev_priv->psr.lock); > @@ -752,6 +790,16 @@ static void hsw_psr_disable(struct intel_dp *intel_dp, > psr_aux_io_power_put(intel_dp); > } > > +void __intel_psr_disable(struct drm_i915_private *dev_priv, > + struct intel_dp *intel_dp, > + const struct intel_crtc_state *old_crtc_state) > +{ > + dev_priv->psr.disable_source(intel_dp, old_crtc_state); > + > + /* Disable PSR on Sink */ > + drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0); > +} > + > /** > * intel_psr_disable - Disable PSR > * @intel_dp: Intel DP > @@ -773,15 +821,13 @@ void intel_psr_disable(struct intel_dp *intel_dp, > return; > > mutex_lock(&dev_priv->psr.lock); > - if (!dev_priv->psr.enabled) { > + if (intel_dp != dev_priv->psr.enabled) { > mutex_unlock(&dev_priv->psr.lock); > return; > } > > - dev_priv->psr.disable_source(intel_dp, old_crtc_state); > - > - /* Disable PSR on Sink */ > - drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0); > + if (intel_psr_enabled(dev_priv)) > + __intel_psr_disable(dev_priv, intel_dp, old_crtc_state); > > dev_priv->psr.enabled = NULL; > mutex_unlock(&dev_priv->psr.lock); > @@ -833,11 +879,11 @@ static void intel_psr_work(struct work_struct *work) > } > } > mutex_lock(&dev_priv->psr.lock); > - intel_dp = dev_priv->psr.enabled; > - > - if (!intel_dp) > + if (!intel_psr_enabled(dev_priv)) > goto unlock; > > + intel_dp = dev_priv->psr.enabled; > + > /* > * The delayed work can race with an invalidate hence we need to > * recheck. Since psr_flush first clears this and then reschedules we > @@ -933,7 +979,7 @@ void intel_psr_single_frame_update(struct drm_i915_private *dev_priv, > return; > > mutex_lock(&dev_priv->psr.lock); > - if (!dev_priv->psr.enabled) { > + if (!intel_psr_enabled(dev_priv)) { > mutex_unlock(&dev_priv->psr.lock); > return; > } > @@ -979,7 +1025,7 @@ void intel_psr_invalidate(struct drm_i915_private *dev_priv, > return; > > mutex_lock(&dev_priv->psr.lock); > - if (!dev_priv->psr.enabled) { > + if (!intel_psr_enabled(dev_priv)) { > mutex_unlock(&dev_priv->psr.lock); > return; > } > @@ -1022,7 +1068,7 @@ void intel_psr_flush(struct drm_i915_private *dev_priv, > return; > > mutex_lock(&dev_priv->psr.lock); > - if (!dev_priv->psr.enabled) { > + if (!intel_psr_enabled(dev_priv)) { > mutex_unlock(&dev_priv->psr.lock); > return; > } > @@ -1081,26 +1127,9 @@ void intel_psr_init(struct drm_i915_private *dev_priv) > if (i915_modparams.enable_psr == -1) > i915_modparams.enable_psr = 0; > > - /* Set link_standby x link_off defaults */ > - if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) > - /* HSW and BDW require workarounds that we don't implement. */ > - dev_priv->psr.link_standby = false; > - else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > - /* On VLV and CHV only standby mode is supported. */ > - dev_priv->psr.link_standby = true; > - else > - /* For new platforms let's respect VBT back again */ > - dev_priv->psr.link_standby = dev_priv->vbt.psr.full_link; > + dev_priv->psr.link_standby = intel_psr_default_link_standby(dev_priv); > > - /* Override link_standby x link_off defaults */ > - if (i915_modparams.enable_psr == 2 && !dev_priv->psr.link_standby) { > - DRM_DEBUG_KMS("PSR: Forcing link standby\n"); > - dev_priv->psr.link_standby = true; > - } > - if (i915_modparams.enable_psr == 3 && dev_priv->psr.link_standby) { > - DRM_DEBUG_KMS("PSR: Forcing main link off\n"); > - dev_priv->psr.link_standby = false; > - } > + dev_priv->psr.global_enable = i915_modparams.enable_psr; > > INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work); > mutex_init(&dev_priv->psr.lock); > -- > 2.16.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
