| Message ID | 20180315102827.30946-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2018-03-15 at 11:28 +0100, Maarten Lankhorst wrote: > Currently tests modify i915.enable_psr and then do a modeset cycle > to change PSR. We can write a value to i915_edp_psr_status to force > a certain value without a modeset. > > To retain compatibility with older userspace, we also still allow > the override through the module parameter, and add some tracking > to check whether a debugfs mode is specified. > While this is something we want to be able to do, I am concerned about adding more complexity to a feature that has barely been tested. How about doing a modeset before frontbuffer_tracking PSR subtests and one at the end? I'm assuming all of them are grouped together. Comments on this patch itself. 1) please split intel_psr_default_link_standby() into a separate patch. 2) how does the user know what values to write without looking at the code? 3) Can the connector and crtc be stored somewhere to avoid loops? 4) Has this been tested on any platforms with PSR? 5) Do subtests need a finer control of PSR i.e., psr_exit() and psr_activate() instead of enable and disable > Changes since v1: > - Rename dev_priv->psr.enabled to .dp, and .hw_configured to .enabled. > - Fix i915_psr_debugfs_mode to match the writes to debugfs. > - Rename __i915_edp_psr_write to intel_psr_set_debugfs_mode, simplify > it and move it to intel_psr.c. This keeps all internals in intel_psr.c > - Perform an interruptible wait for hw completion outside of the psr > lock, instead of being forced to trywait and return -EBUSY. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 73 +++++++++- > drivers/gpu/drm/i915/i915_drv.h | 11 +- > drivers/gpu/drm/i915/intel_drv.h | 3 + > drivers/gpu/drm/i915/intel_psr.c | 262 +++++++++++++++++++++++++++--------- > 4 files changed, 281 insertions(+), 68 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 574fcf234007..98e169636f86 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) > @@ -2564,7 +2561,7 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) > intel_runtime_pm_get(dev_priv); > > mutex_lock(&dev_priv->psr.lock); > - seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv->psr.enabled)); > + seq_printf(m, "Enabled: %s\n", yesno(dev_priv->psr.enabled)); > seq_printf(m, "Busy frontbuffer bits: 0x%03x\n", > dev_priv->psr.busy_frontbuffer_bits); > seq_printf(m, "Re-enable work scheduled: %s\n", > @@ -2631,6 +2628,70 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) > return 0; > } > > +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; > + > + 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; > + } > + > + if (val < -1 || val > 3) > + return -EINVAL; > + > + intel_runtime_pm_get(dev_priv); > + > + drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE); > + > +retry: > + ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, val); > + 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 +4795,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 +4821,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..9262cfb8aac2 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -596,7 +596,8 @@ struct i915_drrs { > struct i915_psr { > struct mutex lock; > bool sink_support; > - struct intel_dp *enabled; > + bool enabled; > + struct intel_dp *dp; > bool active; > struct delayed_work work; > unsigned busy_frontbuffer_bits; > @@ -608,6 +609,14 @@ struct i915_psr { > bool alpm; > bool has_hw_tracking; > > + enum i915_psr_debugfs_mode { > + PSR_DEBUGFS_MODE_DEFAULT = -1, > + PSR_DEBUGFS_MODE_DISABLED, > + PSR_DEBUGFS_MODE_ENABLED, > + PSR_DEBUGFS_MODE_ENABLED_FORCE_LINK_STANDBY, > + PSR_DEBUGFS_MODE_ENABLED_NO_LINK_STANDBY > + } debugfs_mode; > + > void (*enable_source)(struct intel_dp *, > const struct intel_crtc_state *); > void (*disable_source)(struct intel_dp *, > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 1f0e8f1e4594..af3c5578d2ea 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1876,6 +1876,9 @@ void intel_psr_enable(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); > +int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv, > + struct drm_modeset_acquire_ctx *ctx, > + enum i915_psr_debugfs_mode mode); > 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..3dc0eda8efe0 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -56,6 +56,52 @@ > #include "intel_drv.h" > #include "i915_drv.h" > > +static 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 get_link_standby_for_mode(struct drm_i915_private *dev_priv, > + enum i915_psr_debugfs_mode mode) > +{ > + if (mode == PSR_DEBUGFS_MODE_ENABLED_FORCE_LINK_STANDBY) > + return true; > + else if (mode == PSR_DEBUGFS_MODE_ENABLED_NO_LINK_STANDBY) > + return false; > + else > + return intel_psr_default_link_standby(dev_priv); > +} > + > +static bool psr_global_enabled(enum i915_psr_debugfs_mode mode) > +{ > + if (mode == PSR_DEBUGFS_MODE_DEFAULT) > + return i915_modparams.enable_psr; > + else if (mode == PSR_DEBUGFS_MODE_DISABLED) > + return false; > + else > + return true; > +} > + > static inline enum intel_display_power_domain > psr_aux_domain(struct intel_dp *intel_dp) > { > @@ -502,11 +548,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 +600,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 (psr_global_enabled(dev_priv->psr.debugfs_mode)) > + 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 +662,38 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp, > } > } > > +static void __intel_psr_enable(struct drm_i915_private *dev_priv, > + const struct intel_crtc_state *crtc_state) > +{ > + struct intel_dp *intel_dp = dev_priv->psr.dp; > + > + if (dev_priv->psr.enabled) > + return; > + > + dev_priv->psr.enabled = true; > + > + 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 > @@ -639,7 +716,7 @@ void intel_psr_enable(struct intel_dp *intel_dp, > > WARN_ON(dev_priv->drrs.dp); > mutex_lock(&dev_priv->psr.lock); > - if (dev_priv->psr.enabled) { > + if (dev_priv->psr.dp) { > DRM_DEBUG_KMS("PSR already in use\n"); > goto unlock; > } > @@ -647,27 +724,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; > + dev_priv->psr.dp = 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 (psr_global_enabled(dev_priv->psr.debugfs_mode)) > + __intel_psr_enable(dev_priv, crtc_state); > > unlock: > mutex_unlock(&dev_priv->psr.lock); > @@ -752,6 +812,21 @@ static void hsw_psr_disable(struct intel_dp *intel_dp, > psr_aux_io_power_put(intel_dp); > } > > +static void __intel_psr_disable(struct drm_i915_private *dev_priv, > + const struct intel_crtc_state *old_crtc_state) > +{ > + struct intel_dp *intel_dp = dev_priv->psr.dp; > + > + if (!dev_priv->psr.enabled) > + 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); > + dev_priv->psr.enabled = false; > +} > + > /** > * intel_psr_disable - Disable PSR > * @intel_dp: Intel DP > @@ -773,27 +848,110 @@ 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.dp) { > 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); > + __intel_psr_disable(dev_priv, old_crtc_state); > > - dev_priv->psr.enabled = NULL; > + dev_priv->psr.dp = NULL; > mutex_unlock(&dev_priv->psr.lock); > > cancel_delayed_work_sync(&dev_priv->psr.work); > } > > +static struct drm_crtc * > +find_idle_crtc_for_encoder(struct drm_encoder *encoder, > + struct drm_modeset_acquire_ctx *ctx) > +{ > + struct drm_connector_list_iter conn_iter; > + struct drm_device *dev = encoder->dev; > + struct drm_connector *connector; > + struct drm_crtc *crtc; > + bool found = false; > + int ret; > + > + 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 ERR_PTR(-EINVAL); > + > + crtc = connector->state->crtc; > + ret = drm_modeset_lock(&crtc->mutex, ctx); > + if (ret) > + return ERR_PTR(ret); > + > + if (connector->state->commit) > + ret = wait_for_completion_interruptible(&connector->state->commit->hw_done); > + if (!ret && crtc->state->commit) > + ret = wait_for_completion_interruptible(&crtc->state->commit->hw_done); > + if (ret) > + return ERR_PTR(ret); > + > + return crtc; > +} > + > +int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv, > + struct drm_modeset_acquire_ctx *ctx, > + enum i915_psr_debugfs_mode mode) > +{ > + struct drm_device *dev = &dev_priv->drm; > + struct drm_encoder *encoder; > + struct drm_crtc *crtc; > + int ret; > + bool enable, link_standby; > + > + ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx); > + if (ret) > + return ret; > + > + link_standby = get_link_standby_for_mode(dev_priv, mode); > + enable = psr_global_enabled(mode); > + > + mutex_lock(&dev_priv->psr.lock); > + > + do { > + if (!dev_priv->psr.dp) { > + dev_priv->psr.debugfs_mode = mode; > + dev_priv->psr.link_standby = link_standby; > + goto end; > + } > + encoder = &dp_to_dig_port(dev_priv->psr.dp)->base.base; > + mutex_unlock(&dev_priv->psr.lock); > + > + crtc = find_idle_crtc_for_encoder(encoder, ctx); > + if (IS_ERR(crtc)) > + return PTR_ERR(crtc); > + > + mutex_lock(&dev_priv->psr.lock); > + } while (dev_priv->psr.dp != enc_to_intel_dp(encoder)); > + > + if (!enable || dev_priv->psr.link_standby != link_standby) > + __intel_psr_disable(dev_priv, to_intel_crtc_state(crtc->state)); > + > + dev_priv->psr.debugfs_mode = mode; > + dev_priv->psr.link_standby = link_standby; > + > + if (enable) > + __intel_psr_enable(dev_priv, to_intel_crtc_state(crtc->state)); > + > +end: > + mutex_unlock(&dev_priv->psr.lock); > + return ret; > +} > + > static void intel_psr_work(struct work_struct *work) > { > struct drm_i915_private *dev_priv = > container_of(work, typeof(*dev_priv), psr.work.work); > - struct intel_dp *intel_dp = dev_priv->psr.enabled; > + struct intel_dp *intel_dp = dev_priv->psr.dp; > struct drm_crtc *crtc = dp_to_dig_port(intel_dp)->base.base.crtc; > enum pipe pipe = to_intel_crtc(crtc)->pipe; > > @@ -833,11 +991,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 (!dev_priv->psr.enabled) > goto unlock; > > + intel_dp = dev_priv->psr.dp; > + > /* > * The delayed work can race with an invalidate hence we need to > * recheck. Since psr_flush first clears this and then reschedules we > @@ -853,7 +1011,7 @@ static void intel_psr_work(struct work_struct *work) > > static void intel_psr_exit(struct drm_i915_private *dev_priv) > { > - struct intel_dp *intel_dp = dev_priv->psr.enabled; > + struct intel_dp *intel_dp = dev_priv->psr.dp; > struct drm_crtc *crtc = dp_to_dig_port(intel_dp)->base.base.crtc; > enum pipe pipe = to_intel_crtc(crtc)->pipe; > u32 val; > @@ -938,7 +1096,7 @@ void intel_psr_single_frame_update(struct drm_i915_private *dev_priv, > return; > } > > - crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc; > + crtc = dp_to_dig_port(dev_priv->psr.dp)->base.base.crtc; > pipe = to_intel_crtc(crtc)->pipe; > > if (frontbuffer_bits & INTEL_FRONTBUFFER_ALL_MASK(pipe)) { > @@ -984,7 +1142,7 @@ void intel_psr_invalidate(struct drm_i915_private *dev_priv, > return; > } > > - crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc; > + crtc = dp_to_dig_port(dev_priv->psr.dp)->base.base.crtc; > pipe = to_intel_crtc(crtc)->pipe; > > frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe); > @@ -1027,7 +1185,7 @@ void intel_psr_flush(struct drm_i915_private *dev_priv, > return; > } > > - crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc; > + crtc = dp_to_dig_port(dev_priv->psr.dp)->base.base.crtc; > pipe = to_intel_crtc(crtc)->pipe; > > frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe); > @@ -1081,26 +1239,8 @@ 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; > - > - /* 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.link_standby = intel_psr_default_link_standby(dev_priv); > + dev_priv->psr.debugfs_mode = PSR_DEBUGFS_MODE_DEFAULT; > > INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work); > mutex_init(&dev_priv->psr.lock);
Op 22-03-18 om 02:45 schreef Pandiyan, Dhinakaran: > On Thu, 2018-03-15 at 11:28 +0100, Maarten Lankhorst wrote: >> Currently tests modify i915.enable_psr and then do a modeset cycle >> to change PSR. We can write a value to i915_edp_psr_status to force >> a certain value without a modeset. >> >> To retain compatibility with older userspace, we also still allow >> the override through the module parameter, and add some tracking >> to check whether a debugfs mode is specified. >> > While this is something we want to be able to do, I am concerned about > adding more complexity to a feature that has barely been tested. > > How about doing a modeset before frontbuffer_tracking PSR subtests and > one at the end? I'm assuming all of them are grouped together. Currently we run all subtests individually, this means that we also need to do some extra modesets per test. One to disable PSR and collect the reference CRC, the other to enable PSR for the actual test. With these changes, we don't need to do so. > Comments on this patch itself. > 1) please split intel_psr_default_link_standby() into a separate patch. Will do. > 2) how does the user know what values to write without looking at the > code? We match the modparam options for i915.enable_psr, but in general user shouldn't touch it unless asked to. :) This is mostly meant for IGT tests, could also be useful for debugging i915 in general though. But if you really want, perhaps if we someone writes an invalid value, we could also output the possible values to debugfs? Though I don't think we ought to. debugfs doesn't always have documentation. > 3) Can the connector and crtc be stored somewhere to avoid loops? intel_psr_set_debugfs mode checks for idleness and waits for all atomic commits to complete. We need the HW state to toggle PSR, and this is the only way to guarantee that crtc->state matches the actual hw state. If we don't drop the psr lock, we would get a deadlock when intel_psr_enable/disable is called from .crtc_disable, because we wait for hw_done with psr lock already taken. > 4) Has this been tested on any platforms with PSR? I've had someone test v1 on a PSR capable system. It hung in the same way as enabling PSR during boot did, so that part is the same. For the later patches I used f2-cnl-alpha, but that system hangs a few seconds / half a minute after loading i915. Still gave me enough time to check we can write any value to debugfs. > 5) Do subtests need a finer control of PSR i.e., psr_exit() and > psr_activate() instead of enable and disable Not afaict. We sometimes invalidate the FB with dirtyfb, but that's all the control we need I think. ~Maarten
On Thu, 2018-03-22 at 10:41 +0100, Maarten Lankhorst wrote: > Op 22-03-18 om 02:45 schreef Pandiyan, Dhinakaran: > > > > On Thu, 2018-03-15 at 11:28 +0100, Maarten Lankhorst wrote: > > > > > > Currently tests modify i915.enable_psr and then do a modeset > > > cycle > > > to change PSR. We can write a value to i915_edp_psr_status to > > > force > > > a certain value without a modeset. > > > > > > To retain compatibility with older userspace, we also still allow > > > the override through the module parameter, and add some tracking > > > to check whether a debugfs mode is specified. > > > > > While this is something we want to be able to do, I am concerned > > about > > adding more complexity to a feature that has barely been tested. > > > > How about doing a modeset before frontbuffer_tracking PSR subtests > > and > > one at the end? I'm assuming all of them are grouped together. > Currently we run all subtests individually, this means that we also > need to do > some extra modesets per test. One to disable PSR and collect the > reference CRC, the > other to enable PSR for the actual test. > > With these changes, we don't need to do so. > > > > > > Comments on this patch itself. > > 1) please split intel_psr_default_link_standby() into a separate > > patch. > Will do. Maarten, do you plan to rebase this patch? I would like to use this in the IGTs to enable PSR1 on PSR2 panels. -DK > > > > 2) how does the user know what values to write without looking at > > the > > code? > We match the modparam options for i915.enable_psr, but in general > user shouldn't touch it unless asked to. :) This is mostly meant for > IGT tests, > could also be useful for debugging i915 in general though. > > But if you really want, perhaps if we someone writes an invalid > value, we could also > output the possible values to debugfs? Though I don't think we ought > to. debugfs > doesn't always have documentation. > > > > 3) Can the connector and crtc be stored somewhere to avoid loops? > intel_psr_set_debugfs mode checks for idleness and waits for all > atomic commits to complete. > We need the HW state to toggle PSR, and this is the only way to > guarantee that crtc->state matches > the actual hw state. > > If we don't drop the psr lock, we would get a deadlock when > intel_psr_enable/disable is called from .crtc_disable, > because we wait for hw_done with psr lock already taken. > > > > 4) Has this been tested on any platforms with PSR? > I've had someone test v1 on a PSR capable system. It hung in the same > way as enabling PSR during boot did, > so that part is the same. > > For the later patches I used f2-cnl-alpha, but that system hangs a > few seconds / half a minute after loading i915. > Still gave me enough time to check we can write any value to debugfs. > > > > > 5) Do subtests need a finer control of PSR i.e., psr_exit() and > > psr_activate() instead of enable and disable > Not afaict. We sometimes invalidate the FB with dirtyfb, but that's > all the control we need I think. > > ~Maarten > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 26-07-18 om 08:32 schreef Dhinakaran Pandiyan: > On Thu, 2018-03-22 at 10:41 +0100, Maarten Lankhorst wrote: >> Op 22-03-18 om 02:45 schreef Pandiyan, Dhinakaran: >>> On Thu, 2018-03-15 at 11:28 +0100, Maarten Lankhorst wrote: >>>> Currently tests modify i915.enable_psr and then do a modeset >>>> cycle >>>> to change PSR. We can write a value to i915_edp_psr_status to >>>> force >>>> a certain value without a modeset. >>>> >>>> To retain compatibility with older userspace, we also still allow >>>> the override through the module parameter, and add some tracking >>>> to check whether a debugfs mode is specified. >>>> >>> While this is something we want to be able to do, I am concerned >>> about >>> adding more complexity to a feature that has barely been tested. >>> >>> How about doing a modeset before frontbuffer_tracking PSR subtests >>> and >>> one at the end? I'm assuming all of them are grouped together. >> Currently we run all subtests individually, this means that we also >> need to do >> some extra modesets per test. One to disable PSR and collect the >> reference CRC, the >> other to enable PSR for the actual test. >> >> With these changes, we don't need to do so. >> >> >>> Comments on this patch itself. >>> 1) please split intel_psr_default_link_standby() into a separate >>> patch. >> Will do. > Maarten, do you plan to rebase this patch? I would like to use this in > the IGTs to enable PSR1 on PSR2 panels. I've rebased, can you check it works as intended? ~Maarten IGT patch below. ---- commit 2ba09dc60dd00c474566fc3d0bb4e550a2c7fcca Author: Maarten Lankhorst Date: Mon Mar 12 17:49:40 2018 +0100 tests/kms_frontbuffer_tracking: Add support for toggling edp psr through debugfs, v2. It's harmful to write to enable_psr at runtime, and the patch that allows us to change i915_edp_psr_status with the panel running will require us to abandon the module parameter. Hence the userspace change needs to be put in IGT first before we can change it at kernel time. Toggling it to debugfs will mean we can skip a modeset when changing our feature set. Changes since v1: - Rebase with the previous patches dropped. Signed-off-by: Maarten Lankhorst diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c index 1dfd7c1cee8d..a043230d150a 100644 --- a/tests/kms_frontbuffer_tracking.c +++ b/tests/kms_frontbuffer_tracking.c @@ -775,6 +775,38 @@ static void drrs_set(unsigned int val) igt_assert_f(ret == (sizeof(buf) - 1), "debugfs_write failed"); } +static void restore_psr_debugfs(int sig) +{ + debugfs_write("i915_edp_psr_status", "-1"); +} + +static bool psr_set(unsigned int val) +{ + static int oldval = -1; + char buf[4]; + int ret; + + snprintf(buf, sizeof(buf), "%i\n", val); + + ret = debugfs_write("i915_edp_psr_status", buf); + if (ret != -EINVAL) { + igt_assert(ret > 0 || val <= 0); + + if (ret > 0) + igt_install_exit_handler(restore_psr_debugfs); + + return false; + } + + igt_set_module_param_int("enable_psr", val); + + if (val == oldval) + return false; + + oldval = val; + return true; +} + static bool is_drrs_high(void) { char buf[MAX_DRRS_STATUS_BUF_LEN]; @@ -941,8 +973,8 @@ static bool drrs_wait_until_rr_switch_to_low(void) #define fbc_enable() igt_set_module_param_int("enable_fbc", 1) #define fbc_disable() igt_set_module_param_int("enable_fbc", 0) -#define psr_enable() igt_set_module_param_int("enable_psr", 1) -#define psr_disable() igt_set_module_param_int("enable_psr", 0) +#define psr_enable() psr_set(1) +#define psr_disable() psr_set(0) #define drrs_enable() drrs_set(1) #define drrs_disable() drrs_set(0)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 574fcf234007..98e169636f86 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) @@ -2564,7 +2561,7 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) intel_runtime_pm_get(dev_priv); mutex_lock(&dev_priv->psr.lock); - seq_printf(m, "Enabled: %s\n", yesno((bool)dev_priv->psr.enabled)); + seq_printf(m, "Enabled: %s\n", yesno(dev_priv->psr.enabled)); seq_printf(m, "Busy frontbuffer bits: 0x%03x\n", dev_priv->psr.busy_frontbuffer_bits); seq_printf(m, "Re-enable work scheduled: %s\n", @@ -2631,6 +2628,70 @@ static int i915_edp_psr_status(struct seq_file *m, void *data) return 0; } +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; + + 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; + } + + if (val < -1 || val > 3) + return -EINVAL; + + intel_runtime_pm_get(dev_priv); + + drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE); + +retry: + ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, val); + 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 +4795,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 +4821,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..9262cfb8aac2 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -596,7 +596,8 @@ struct i915_drrs { struct i915_psr { struct mutex lock; bool sink_support; - struct intel_dp *enabled; + bool enabled; + struct intel_dp *dp; bool active; struct delayed_work work; unsigned busy_frontbuffer_bits; @@ -608,6 +609,14 @@ struct i915_psr { bool alpm; bool has_hw_tracking; + enum i915_psr_debugfs_mode { + PSR_DEBUGFS_MODE_DEFAULT = -1, + PSR_DEBUGFS_MODE_DISABLED, + PSR_DEBUGFS_MODE_ENABLED, + PSR_DEBUGFS_MODE_ENABLED_FORCE_LINK_STANDBY, + PSR_DEBUGFS_MODE_ENABLED_NO_LINK_STANDBY + } debugfs_mode; + void (*enable_source)(struct intel_dp *, const struct intel_crtc_state *); void (*disable_source)(struct intel_dp *, diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 1f0e8f1e4594..af3c5578d2ea 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1876,6 +1876,9 @@ void intel_psr_enable(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); +int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv, + struct drm_modeset_acquire_ctx *ctx, + enum i915_psr_debugfs_mode mode); 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..3dc0eda8efe0 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -56,6 +56,52 @@ #include "intel_drv.h" #include "i915_drv.h" +static 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 get_link_standby_for_mode(struct drm_i915_private *dev_priv, + enum i915_psr_debugfs_mode mode) +{ + if (mode == PSR_DEBUGFS_MODE_ENABLED_FORCE_LINK_STANDBY) + return true; + else if (mode == PSR_DEBUGFS_MODE_ENABLED_NO_LINK_STANDBY) + return false; + else + return intel_psr_default_link_standby(dev_priv); +} + +static bool psr_global_enabled(enum i915_psr_debugfs_mode mode) +{ + if (mode == PSR_DEBUGFS_MODE_DEFAULT) + return i915_modparams.enable_psr; + else if (mode == PSR_DEBUGFS_MODE_DISABLED) + return false; + else + return true; +} + static inline enum intel_display_power_domain psr_aux_domain(struct intel_dp *intel_dp) { @@ -502,11 +548,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 +600,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 (psr_global_enabled(dev_priv->psr.debugfs_mode)) + 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 +662,38 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp, } } +static void __intel_psr_enable(struct drm_i915_private *dev_priv, + const struct intel_crtc_state *crtc_state) +{ + struct intel_dp *intel_dp = dev_priv->psr.dp; + + if (dev_priv->psr.enabled) + return; + + dev_priv->psr.enabled = true; + + 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 @@ -639,7 +716,7 @@ void intel_psr_enable(struct intel_dp *intel_dp, WARN_ON(dev_priv->drrs.dp); mutex_lock(&dev_priv->psr.lock); - if (dev_priv->psr.enabled) { + if (dev_priv->psr.dp) { DRM_DEBUG_KMS("PSR already in use\n"); goto unlock; } @@ -647,27 +724,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; + dev_priv->psr.dp = 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 (psr_global_enabled(dev_priv->psr.debugfs_mode)) + __intel_psr_enable(dev_priv, crtc_state); unlock: mutex_unlock(&dev_priv->psr.lock); @@ -752,6 +812,21 @@ static void hsw_psr_disable(struct intel_dp *intel_dp, psr_aux_io_power_put(intel_dp); } +static void __intel_psr_disable(struct drm_i915_private *dev_priv, + const struct intel_crtc_state *old_crtc_state) +{ + struct intel_dp *intel_dp = dev_priv->psr.dp; + + if (!dev_priv->psr.enabled) + 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); + dev_priv->psr.enabled = false; +} + /** * intel_psr_disable - Disable PSR * @intel_dp: Intel DP @@ -773,27 +848,110 @@ 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.dp) { 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); + __intel_psr_disable(dev_priv, old_crtc_state); - dev_priv->psr.enabled = NULL; + dev_priv->psr.dp = NULL; mutex_unlock(&dev_priv->psr.lock); cancel_delayed_work_sync(&dev_priv->psr.work); } +static struct drm_crtc * +find_idle_crtc_for_encoder(struct drm_encoder *encoder, + struct drm_modeset_acquire_ctx *ctx) +{ + struct drm_connector_list_iter conn_iter; + struct drm_device *dev = encoder->dev; + struct drm_connector *connector; + struct drm_crtc *crtc; + bool found = false; + int ret; + + 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 ERR_PTR(-EINVAL); + + crtc = connector->state->crtc; + ret = drm_modeset_lock(&crtc->mutex, ctx); + if (ret) + return ERR_PTR(ret); + + if (connector->state->commit) + ret = wait_for_completion_interruptible(&connector->state->commit->hw_done); + if (!ret && crtc->state->commit) + ret = wait_for_completion_interruptible(&crtc->state->commit->hw_done); + if (ret) + return ERR_PTR(ret); + + return crtc; +} + +int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv, + struct drm_modeset_acquire_ctx *ctx, + enum i915_psr_debugfs_mode mode) +{ + struct drm_device *dev = &dev_priv->drm; + struct drm_encoder *encoder; + struct drm_crtc *crtc; + int ret; + bool enable, link_standby; + + ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx); + if (ret) + return ret; + + link_standby = get_link_standby_for_mode(dev_priv, mode); + enable = psr_global_enabled(mode); + + mutex_lock(&dev_priv->psr.lock); + + do { + if (!dev_priv->psr.dp) { + dev_priv->psr.debugfs_mode = mode; + dev_priv->psr.link_standby = link_standby; + goto end; + } + encoder = &dp_to_dig_port(dev_priv->psr.dp)->base.base; + mutex_unlock(&dev_priv->psr.lock); + + crtc = find_idle_crtc_for_encoder(encoder, ctx); + if (IS_ERR(crtc)) + return PTR_ERR(crtc); + + mutex_lock(&dev_priv->psr.lock); + } while (dev_priv->psr.dp != enc_to_intel_dp(encoder)); + + if (!enable || dev_priv->psr.link_standby != link_standby) + __intel_psr_disable(dev_priv, to_intel_crtc_state(crtc->state)); + + dev_priv->psr.debugfs_mode = mode; + dev_priv->psr.link_standby = link_standby; + + if (enable) + __intel_psr_enable(dev_priv, to_intel_crtc_state(crtc->state)); + +end: + mutex_unlock(&dev_priv->psr.lock); + return ret; +} + static void intel_psr_work(struct work_struct *work) { struct drm_i915_private *dev_priv = container_of(work, typeof(*dev_priv), psr.work.work); - struct intel_dp *intel_dp = dev_priv->psr.enabled; + struct intel_dp *intel_dp = dev_priv->psr.dp; struct drm_crtc *crtc = dp_to_dig_port(intel_dp)->base.base.crtc; enum pipe pipe = to_intel_crtc(crtc)->pipe; @@ -833,11 +991,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 (!dev_priv->psr.enabled) goto unlock; + intel_dp = dev_priv->psr.dp; + /* * The delayed work can race with an invalidate hence we need to * recheck. Since psr_flush first clears this and then reschedules we @@ -853,7 +1011,7 @@ static void intel_psr_work(struct work_struct *work) static void intel_psr_exit(struct drm_i915_private *dev_priv) { - struct intel_dp *intel_dp = dev_priv->psr.enabled; + struct intel_dp *intel_dp = dev_priv->psr.dp; struct drm_crtc *crtc = dp_to_dig_port(intel_dp)->base.base.crtc; enum pipe pipe = to_intel_crtc(crtc)->pipe; u32 val; @@ -938,7 +1096,7 @@ void intel_psr_single_frame_update(struct drm_i915_private *dev_priv, return; } - crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc; + crtc = dp_to_dig_port(dev_priv->psr.dp)->base.base.crtc; pipe = to_intel_crtc(crtc)->pipe; if (frontbuffer_bits & INTEL_FRONTBUFFER_ALL_MASK(pipe)) { @@ -984,7 +1142,7 @@ void intel_psr_invalidate(struct drm_i915_private *dev_priv, return; } - crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc; + crtc = dp_to_dig_port(dev_priv->psr.dp)->base.base.crtc; pipe = to_intel_crtc(crtc)->pipe; frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe); @@ -1027,7 +1185,7 @@ void intel_psr_flush(struct drm_i915_private *dev_priv, return; } - crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc; + crtc = dp_to_dig_port(dev_priv->psr.dp)->base.base.crtc; pipe = to_intel_crtc(crtc)->pipe; frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe); @@ -1081,26 +1239,8 @@ 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; - - /* 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.link_standby = intel_psr_default_link_standby(dev_priv); + dev_priv->psr.debugfs_mode = PSR_DEBUGFS_MODE_DEFAULT; INIT_DELAYED_WORK(&dev_priv->psr.work, intel_psr_work); mutex_init(&dev_priv->psr.lock);
Currently tests modify i915.enable_psr and then do a modeset cycle to change PSR. We can write a value to i915_edp_psr_status to force a certain value without a modeset. To retain compatibility with older userspace, we also still allow the override through the module parameter, and add some tracking to check whether a debugfs mode is specified. Changes since v1: - Rename dev_priv->psr.enabled to .dp, and .hw_configured to .enabled. - Fix i915_psr_debugfs_mode to match the writes to debugfs. - Rename __i915_edp_psr_write to intel_psr_set_debugfs_mode, simplify it and move it to intel_psr.c. This keeps all internals in intel_psr.c - Perform an interruptible wait for hw completion outside of the psr lock, instead of being forced to trywait and return -EBUSY. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 73 +++++++++- drivers/gpu/drm/i915/i915_drv.h | 11 +- drivers/gpu/drm/i915/intel_drv.h | 3 + drivers/gpu/drm/i915/intel_psr.c | 262 +++++++++++++++++++++++++++--------- 4 files changed, 281 insertions(+), 68 deletions(-)