| Message ID | 20180726090604.12142-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Allow control of PSR at runtime through debugfs, v3. | expand |
On Thu, 2018-07-26 at 11:06 +0200, 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. > > 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. > Changes since v2: > - Rebase on top of intel_psr changes. Thanks for resending this, I have some questions to understand the patch better. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 75 ++++++++++++-- > drivers/gpu/drm/i915/i915_drv.h | 9 +- > drivers/gpu/drm/i915/intel_drv.h | 3 + > drivers/gpu/drm/i915/intel_psr.c | 154 ++++++++++++++++++++++++ > ---- > 4 files changed, 214 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index 59dc0610ea44..b2904bb2be49 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2689,14 +2689,11 @@ psr_source_status(struct drm_i915_private > *dev_priv, struct seq_file *m) > > 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; > 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) > @@ -2705,7 +2702,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); > > @@ -2776,6 +2773,72 @@ > DEFINE_SIMPLE_ATTRIBUTE(i915_edp_psr_debug_fops, > i915_edp_psr_debug_get, > i915_edp_psr_debug_set, > "%llu\n"); > > +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 != PSR_DEBUGFS_MODE_DEFAULT && > + val != PSR_DEBUGFS_MODE_DISABLED && > + val != PSR_DEBUGFS_MODE_ENABLED) > + 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); Deadlocked here during testing $ echo 0 > /sys/kernel/debug/dri/0/i915_edp_psr_status bash: echo: write error: Resource deadlock avoided [ 1248.856671] WARNING: CPU: 2 PID: 1788 at drivers/gpu/drm/drm_modeset_lock.c:223 drm_modeset_drop_locks+0x56/0x60 [drm] [ 1248.856757] Modules linked in: rfcomm cmac bnep arc4 iwlmvm nls_iso8859_1 mac80211 snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hwdep snd_hda_core iwlwifi snd_pcm snd_seq_midi snd_seq_midi_event snd_rawmidi intel_rapl btusb btrtl snd_seq btbcm x86_pkg_temp_thermal btintel intel_powerclamp bluetooth coretemp crct10dif_pclmul crc32_pclmul snd_timer snd_seq_device ghash_clmulni_intel cfg80211 aesni_intel snd aes_x86_64 crypto_simd cryptd soundcore ecdh_generic glue_helper input_leds mei_me mei intel_pch_thermal mac_hid parport_pc ppdev lp parport autofs4 i915 i2c_algo_bit drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops drm e1000e psmouse ahci libahci video [ 1248.857288] CPU: 2 PID: 1788 Comm: bash Not tainted 4.18.0-rc6drm- tip #138 [ 1248.857297] Hardware name: LENOVO 20F6CTO1WW/20F6CTO1WW, BIOS R02ET48W (1.21 ) 06/01/2016 [ 1248.857354] RIP: 0010:drm_modeset_drop_locks+0x56/0x60 [drm] [ 1248.857363] Code: 50 08 48 8d b8 50 ff ff ff 48 89 51 08 48 89 0a 48 89 00 48 89 40 08 e8 a8 90 7d c9 48 8b 43 70 49 39 c4 75 d2 5b 41 5c 5d c3 <0f> 0b eb bc 66 0f 1f 44 00 00 0f 1f 44 00 00 48 8b 97 d0 0a 00 00 [ 1248.857860] RSP: 0018:ffffa4dd01fabcf8 EFLAGS: 00010286 [ 1248.857878] RAX: 00000000ffffffdd RBX: ffffa4dd01fabd28 RCX: 0000000000000000 [ 1248.857889] RDX: 0000000000000000 RSI: ffff97bb88476898 RDI: ffffa4dd01fabd28 [ 1248.857898] RBP: ffffa4dd01fabd08 R08: 0000000000000000 R09: 0000000000000001 [ 1248.857908] R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000002 [ 1248.857918] R13: 00005603cdb60880 R14: 0000000000000002 R15: ffffa4dd01fabee8 [ 1248.857930] FS: 00007f83b1dea740(0000) GS:ffff97bb99a00000(0000) knlGS:0000000000000000 [ 1248.857940] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 1248.857950] CR2: 00005603cdb60880 CR3: 0000000214338003 CR4: 00000000003606e0 [ 1248.857959] Call Trace: [ 1248.858094] i915_edp_psr_write+0xd5/0x180 [i915] [ 1248.858172] full_proxy_write+0x5f/0x90 [ 1248.858207] __vfs_write+0x3a/0x1a0 [ 1248.858227] ? rcu_read_lock_sched_held+0x79/0x80 [ 1248.858243] ? rcu_sync_lockdep_assert+0x32/0x60 [ 1248.858260] ? __sb_start_write+0x135/0x190 [ 1248.858275] ? vfs_write+0x193/0x1c0 [ 1248.858306] vfs_write+0xc6/0x1c0 [ 1248.858335] ksys_write+0x58/0xc0 [ 1248.858370] __x64_sys_write+0x1a/0x20 [ 1248.858387] do_syscall_64+0x65/0x1b0 [ 1248.858409] entry_SYSCALL_64_after_hwframe+0x49/0xbe [ 1248.858423] RIP: 0033:0x7f83b14f2154 [ 1248.858430] Code: 89 02 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 8d 05 b1 07 2e 00 8b 00 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 41 54 55 49 89 d4 53 48 89 f5 [ 1248.858928] RSP: 002b:00007ffee7320168 EFLAGS: 00000246 ORIG_RAX: 0000000000000001 [ 1248.858946] RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 00007f83b14f2154 [ 1248.858956] RDX: 0000000000000002 RSI: 00005603cdb60880 RDI: 0000000000000001 [ 1248.858965] RBP: 00005603cdb60880 R08: 000000000000000a R09: 0000000000000001 [ 1248.858975] R10: 000000000000000a R11: 0000000000000246 R12: 00007f83b17ce760 [ 1248.858984] R13: 0000000000000002 R14: 00007f83b17ca2a0 R15: 00007f83b17c9760 [ 1248.859043] irq event stamp: 59768 [ 1248.859058] hardirqs last enabled at (59767): [<ffffffff89a31dc6>] _raw_spin_unlock_irqrestore+0x36/0x60 [ 1248.859072] hardirqs last disabled at (59768): [<ffffffff89c01309>] error_entry+0x89/0x110 [ 1248.859087] softirqs last enabled at (59724): [<ffffffff89e00378>] __do_softirq+0x378/0x4e3 [ 1248.859100] softirqs last disabled at (59711): [<ffffffff89098e0d>] irq_exit+0xcd/0xe0 [ 1248.859156] WARNING: CPU: 2 PID: 1788 at drivers/gpu/drm/drm_modeset_lock.c:223 drm_modeset_drop_locks+0x56/0x60 [drm] [ 1248.859166] ---[ end trace b7222f9239d3065b ]-- > + > + 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); What do you think of using "i915_edp_psr_debug" instead? > +} > + > +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_energy_uJ(struct seq_file *m, void *data) > { > struct drm_i915_private *dev_priv = node_to_i915(m- > >private); > @@ -4720,7 +4783,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_energy_uJ", i915_energy_uJ, 0}, > {"i915_runtime_pm_status", i915_runtime_pm_status, 0}, > {"i915_power_domain_info", i915_power_domain_info, 0}, > @@ -4744,6 +4806,7 @@ static const struct i915_debugfs_files { > const struct file_operations *fops; > } i915_debugfs_files[] = { > {"i915_wedged", &i915_wedged_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 0f49f9988dfa..d8583770e8a6 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -612,7 +612,8 @@ struct i915_drrs { > struct i915_psr { > struct mutex lock; > bool sink_support; > - struct intel_dp *enabled; > + bool enabled; > + struct intel_dp *dp; Separate patch for this change? How about keeping i915_psr.dp always valid? > bool active; > struct work_struct work; > unsigned busy_frontbuffer_bits; > @@ -625,6 +626,12 @@ struct i915_psr { > bool debug; > ktime_t last_entry_attempt; > ktime_t last_exit; > + > + enum i915_psr_debugfs_mode { > + PSR_DEBUGFS_MODE_DEFAULT = -1, > + PSR_DEBUGFS_MODE_DISABLED, > + PSR_DEBUGFS_MODE_ENABLED If we add another enum, we can write tests to enable PSR1 on PSR2 panels. PSR_DEBUGFS_MODE_PSR1 PSR_DEBUGFS_MODE_PSR2 > + } debugfs_mode; > }; > > enum intel_pch { > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index c275f91244a6..751ed257fbba 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1926,6 +1926,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 4bd5768731ee..97424ae769f3 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -56,6 +56,16 @@ > #include "intel_drv.h" > #include "i915_drv.h" > > +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; > +} > + > void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool > debug) > { > u32 debug_mask, mask; > @@ -471,11 +481,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 > @@ -517,7 +522,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" : ""); This gets printed during a modeset that is shutting down the crtc. > + else > + DRM_DEBUG_KMS("PSR disable by flag\n"); > } So, neither debugfs nor modparam has any effect on crtc_state->has_psr or crtc_state->has_psr2. Why was this check moved to the end? We could also rewrite the debug message to look similar to the other compute_config functions > > static void intel_psr_activate(struct intel_dp *intel_dp) > @@ -589,6 +598,22 @@ static void intel_psr_enable_source(struct > intel_dp *intel_dp, > } > } > > +static void intel_psr_enable_locked(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; > + This function gets called from intel_psr_set_debugfs_mode() Doesn't this allow debugfs to enable PSR even if mode related checks in psr_compute_config() had failed? For e.g., crtc_state->has_psr was false. > + intel_psr_setup_vsc(intel_dp, crtc_state); > + intel_psr_enable_sink(intel_dp); > + intel_psr_enable_source(intel_dp, crtc_state); > + dev_priv->psr.enabled = true; > + > + intel_psr_activate(intel_dp); > +} > + > /** > * intel_psr_enable - Enable PSR > * @intel_dp: Intel DP > @@ -611,7 +636,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) { Check for dev_priv->psr.enabled instead? > DRM_DEBUG_KMS("PSR already in use\n"); > goto unlock; > } > @@ -619,12 +644,10 @@ void intel_psr_enable(struct intel_dp > *intel_dp, > dev_priv->psr.psr2_enabled = crtc_state->has_psr2; > dev_priv->psr.busy_frontbuffer_bits = 0; > > - intel_psr_setup_vsc(intel_dp, crtc_state); > - intel_psr_enable_sink(intel_dp); > - intel_psr_enable_source(intel_dp, crtc_state); > - dev_priv->psr.enabled = intel_dp; > + dev_priv->psr.dp = intel_dp; Now that there is psr.enabled, should we always keep this pointer valid? > > - intel_psr_activate(intel_dp); > + if (psr_global_enabled(dev_priv->psr.debugfs_mode)) Okay, now I see why you have psr_global_enabled() as the last check in psr_compute_config(). > + intel_psr_enable_locked(dev_priv, crtc_state); > > unlock: > mutex_unlock(&dev_priv->psr.lock); > @@ -688,7 +711,7 @@ static void intel_psr_disable_locked(struct > intel_dp *intel_dp) > /* Disable PSR on Sink */ > drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0); > > - dev_priv->psr.enabled = NULL; > + dev_priv->psr.enabled = false; > } > > /** > @@ -712,7 +735,14 @@ void intel_psr_disable(struct intel_dp > *intel_dp, > return; > > mutex_lock(&dev_priv->psr.lock); > + if (intel_dp != dev_priv->psr.dp) { > + mutex_unlock(&dev_priv->psr.lock); > + return; > + } > + > intel_psr_disable_locked(intel_dp); > + > + dev_priv->psr.dp = NULL; Is there still a need to use this field as flag? > mutex_lunlock(&dev_priv->psr.lock); > cancel_work_sync(&dev_priv->psr.work); > } > @@ -756,13 +786,11 @@ int intel_psr_wait_for_idle(const struct > intel_crtc_state *new_crtc_state) > > static bool __psr_wait_for_idle_locked(struct drm_i915_private > *dev_priv) > { > - struct intel_dp *intel_dp; > i915_reg_t reg; > u32 mask; > int err; > > - intel_dp = dev_priv->psr.enabled; > - if (!intel_dp) > + if (!dev_priv->psr.enabled) > return false; > > if (dev_priv->psr.psr2_enabled) { > @@ -784,6 +812,89 @@ static bool __psr_wait_for_idle_locked(struct > drm_i915_private *dev_priv) > return err == 0 && dev_priv->psr.enabled; > } > > +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; > + > + ret = drm_modeset_lock(&dev->mode_config.connection_mutex, > ctx); > + if (ret) > + return ret; > + > + enable = psr_global_enabled(mode); > + > + mutex_lock(&dev_priv->psr.lock); > + > + do { > + if (!dev_priv->psr.dp) { > + dev_priv->psr.debugfs_mode = mode; > + 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)); When will this not be true? > + > + if (!enable) > + intel_psr_disable_locked(enc_to_intel_dp(encoder)); > + > + dev_priv->psr.debugfs_mode = mode; > + > + if (enable) bspec says PSR enable sequence requires ": The associated transcoder and port are running and Aux channel associated with this port has IO power enabled" Shouldn't crtc_state->active be checked. > + intel_psr_enable_locked(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 = > @@ -811,7 +922,8 @@ static void intel_psr_work(struct work_struct > *work) > if (dev_priv->psr.busy_frontbuffer_bits || dev_priv- > >psr.active) > goto unlock; > > - intel_psr_activate(dev_priv->psr.enabled); > + intel_psr_activate(dev_priv->psr.dp > +); Spurious new line. > unlock: > mutex_unlock(&dev_priv->psr.lock); > } > @@ -866,7 +978,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); > @@ -909,7 +1021,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); > @@ -971,6 +1083,8 @@ void intel_psr_init(struct drm_i915_private > *dev_priv) > /* For new platforms let's respect VBT back again */ > dev_priv->psr.link_standby = dev_priv- > >vbt.psr.full_link; > > + dev_priv->psr.debugfs_mode = PSR_DEBUGFS_MODE_DEFAULT; > + > INIT_WORK(&dev_priv->psr.work, intel_psr_work); > mutex_init(&dev_priv->psr.lock); > } > @@ -991,7 +1105,7 @@ void intel_psr_short_pulse(struct intel_dp > *intel_dp) > > mutex_lock(&psr->lock); > > - if (psr->enabled != intel_dp) > + if (!psr->enabled || psr->dp != intel_dp) > goto exit; > > if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val) > != 1) {
Op 27-07-18 om 05:27 schreef Dhinakaran Pandiyan: > On Thu, 2018-07-26 at 11:06 +0200, 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. >> >> 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. >> Changes since v2: >> - Rebase on top of intel_psr changes. > Thanks for resending this, I have some questions to understand the > patch better. > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >> --- >> drivers/gpu/drm/i915/i915_debugfs.c | 75 ++++++++++++-- >> drivers/gpu/drm/i915/i915_drv.h | 9 +- >> drivers/gpu/drm/i915/intel_drv.h | 3 + >> drivers/gpu/drm/i915/intel_psr.c | 154 ++++++++++++++++++++++++ >> ---- >> 4 files changed, 214 insertions(+), 27 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c >> b/drivers/gpu/drm/i915/i915_debugfs.c >> index 59dc0610ea44..b2904bb2be49 100644 >> --- a/drivers/gpu/drm/i915/i915_debugfs.c >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >> @@ -2689,14 +2689,11 @@ psr_source_status(struct drm_i915_private >> *dev_priv, struct seq_file *m) >> >> 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; >> 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) >> @@ -2705,7 +2702,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); >> >> @@ -2776,6 +2773,72 @@ >> DEFINE_SIMPLE_ATTRIBUTE(i915_edp_psr_debug_fops, >> i915_edp_psr_debug_get, >> i915_edp_psr_debug_set, >> "%llu\n"); >> >> +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 != PSR_DEBUGFS_MODE_DEFAULT && >> + val != PSR_DEBUGFS_MODE_DISABLED && >> + val != PSR_DEBUGFS_MODE_ENABLED) >> + 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); > Deadlocked here during testing > > $ echo 0 > /sys/kernel/debug/dri/0/i915_edp_psr_status > bash: echo: write error: Resource deadlock avoided Oops, that check should be: "if (ret == -EDEADLK)" That should fix your error. :) > [ 1248.856671] WARNING: CPU: 2 PID: 1788 at > drivers/gpu/drm/drm_modeset_lock.c:223 drm_modeset_drop_locks+0x56/0x60 > [drm] > [ 1248.856757] Modules linked in: rfcomm cmac bnep arc4 iwlmvm > nls_iso8859_1 mac80211 snd_hda_codec_hdmi snd_hda_codec_realtek > snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hwdep > snd_hda_core iwlwifi snd_pcm snd_seq_midi snd_seq_midi_event > snd_rawmidi intel_rapl btusb btrtl snd_seq btbcm x86_pkg_temp_thermal > btintel intel_powerclamp bluetooth coretemp crct10dif_pclmul > crc32_pclmul snd_timer snd_seq_device ghash_clmulni_intel cfg80211 > aesni_intel snd aes_x86_64 crypto_simd cryptd soundcore ecdh_generic > glue_helper input_leds mei_me mei intel_pch_thermal mac_hid parport_pc > ppdev lp parport autofs4 i915 i2c_algo_bit drm_kms_helper syscopyarea > sysfillrect sysimgblt fb_sys_fops drm e1000e psmouse ahci libahci video > [ 1248.857288] CPU: 2 PID: 1788 Comm: bash Not tainted 4.18.0-rc6drm- > tip #138 > [ 1248.857297] Hardware name: LENOVO 20F6CTO1WW/20F6CTO1WW, BIOS > R02ET48W (1.21 ) 06/01/2016 > [ 1248.857354] RIP: 0010:drm_modeset_drop_locks+0x56/0x60 [drm] > [ 1248.857363] Code: 50 08 48 8d b8 50 ff ff ff 48 89 51 08 48 89 0a 48 > 89 00 48 89 40 08 e8 a8 90 7d c9 48 8b 43 70 49 39 c4 75 d2 5b 41 5c 5d > c3 <0f> 0b eb bc 66 0f 1f 44 00 00 0f 1f 44 00 00 48 8b 97 d0 0a 00 00 > [ 1248.857860] RSP: 0018:ffffa4dd01fabcf8 EFLAGS: 00010286 > [ 1248.857878] RAX: 00000000ffffffdd RBX: ffffa4dd01fabd28 RCX: > 0000000000000000 > [ 1248.857889] RDX: 0000000000000000 RSI: ffff97bb88476898 RDI: > ffffa4dd01fabd28 > [ 1248.857898] RBP: ffffa4dd01fabd08 R08: 0000000000000000 R09: > 0000000000000001 > [ 1248.857908] R10: 0000000000000001 R11: 0000000000000000 R12: > 0000000000000002 > [ 1248.857918] R13: 00005603cdb60880 R14: 0000000000000002 R15: > ffffa4dd01fabee8 > [ 1248.857930] FS: 00007f83b1dea740(0000) GS:ffff97bb99a00000(0000) > knlGS:0000000000000000 > [ 1248.857940] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 1248.857950] CR2: 00005603cdb60880 CR3: 0000000214338003 CR4: > 00000000003606e0 > [ 1248.857959] Call Trace: > [ 1248.858094] i915_edp_psr_write+0xd5/0x180 [i915] > [ 1248.858172] full_proxy_write+0x5f/0x90 > [ 1248.858207] __vfs_write+0x3a/0x1a0 > [ 1248.858227] ? rcu_read_lock_sched_held+0x79/0x80 > [ 1248.858243] ? rcu_sync_lockdep_assert+0x32/0x60 > [ 1248.858260] ? __sb_start_write+0x135/0x190 > [ 1248.858275] ? vfs_write+0x193/0x1c0 > [ 1248.858306] vfs_write+0xc6/0x1c0 > [ 1248.858335] ksys_write+0x58/0xc0 > [ 1248.858370] __x64_sys_write+0x1a/0x20 > [ 1248.858387] do_syscall_64+0x65/0x1b0 > [ 1248.858409] entry_SYSCALL_64_after_hwframe+0x49/0xbe > [ 1248.858423] RIP: 0033:0x7f83b14f2154 > [ 1248.858430] Code: 89 02 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 > 00 00 00 66 90 48 8d 05 b1 07 2e 00 8b 00 85 c0 75 13 b8 01 00 00 00 0f > 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 41 54 55 49 89 d4 53 48 89 f5 > [ 1248.858928] RSP: 002b:00007ffee7320168 EFLAGS: 00000246 ORIG_RAX: > 0000000000000001 > [ 1248.858946] RAX: ffffffffffffffda RBX: 0000000000000002 RCX: > 00007f83b14f2154 > [ 1248.858956] RDX: 0000000000000002 RSI: 00005603cdb60880 RDI: > 0000000000000001 > [ 1248.858965] RBP: 00005603cdb60880 R08: 000000000000000a R09: > 0000000000000001 > [ 1248.858975] R10: 000000000000000a R11: 0000000000000246 R12: > 00007f83b17ce760 > [ 1248.858984] R13: 0000000000000002 R14: 00007f83b17ca2a0 R15: > 00007f83b17c9760 > [ 1248.859043] irq event stamp: 59768 > [ 1248.859058] hardirqs last enabled at (59767): [<ffffffff89a31dc6>] > _raw_spin_unlock_irqrestore+0x36/0x60 > [ 1248.859072] hardirqs last disabled at (59768): [<ffffffff89c01309>] > error_entry+0x89/0x110 > [ 1248.859087] softirqs last enabled at (59724): [<ffffffff89e00378>] > __do_softirq+0x378/0x4e3 > [ 1248.859100] softirqs last disabled at (59711): [<ffffffff89098e0d>] > irq_exit+0xcd/0xe0 > [ 1248.859156] WARNING: CPU: 2 PID: 1788 at > drivers/gpu/drm/drm_modeset_lock.c:223 drm_modeset_drop_locks+0x56/0x60 > [drm] > [ 1248.859166] ---[ end trace b7222f9239d3065b ]-- And this warning. > >> + >> + 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); > What do you think of using "i915_edp_psr_debug" instead? All for it. >> +} >> + >> +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_energy_uJ(struct seq_file *m, void *data) >> { >> struct drm_i915_private *dev_priv = node_to_i915(m- >>> private); >> @@ -4720,7 +4783,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_energy_uJ", i915_energy_uJ, 0}, >> {"i915_runtime_pm_status", i915_runtime_pm_status, 0}, >> {"i915_power_domain_info", i915_power_domain_info, 0}, >> @@ -4744,6 +4806,7 @@ static const struct i915_debugfs_files { >> const struct file_operations *fops; >> } i915_debugfs_files[] = { >> {"i915_wedged", &i915_wedged_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 0f49f9988dfa..d8583770e8a6 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -612,7 +612,8 @@ struct i915_drrs { >> struct i915_psr { >> struct mutex lock; >> bool sink_support; >> - struct intel_dp *enabled; >> + bool enabled; >> + struct intel_dp *dp; > Separate patch for this change? How about keeping i915_psr.dp always > valid? I'm keeping i915_psr.dp only valid when the modeset calls intel_psr_enable, until the modeset disable calls intel_psr_disable() i915_psr.dp is used to also indicate that we can currently enable PSR. .enabled is used to determine it's currently enabled. >> bool active; >> struct work_struct work; >> unsigned busy_frontbuffer_bits; >> @@ -625,6 +626,12 @@ struct i915_psr { >> bool debug; >> ktime_t last_entry_attempt; >> ktime_t last_exit; >> + >> + enum i915_psr_debugfs_mode { >> + PSR_DEBUGFS_MODE_DEFAULT = -1, >> + PSR_DEBUGFS_MODE_DISABLED, >> + PSR_DEBUGFS_MODE_ENABLED > If we add another enum, we can write tests to enable PSR1 on PSR2 > panels. > PSR_DEBUGFS_MODE_PSR1 > PSR_DEBUGFS_MODE_PSR2 Should be easy to add, but annoying to toggle. Maybe some followup? >> + } debugfs_mode; >> }; >> >> enum intel_pch { >> diff --git a/drivers/gpu/drm/i915/intel_drv.h >> b/drivers/gpu/drm/i915/intel_drv.h >> index c275f91244a6..751ed257fbba 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -1926,6 +1926,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 4bd5768731ee..97424ae769f3 100644 >> --- a/drivers/gpu/drm/i915/intel_psr.c >> +++ b/drivers/gpu/drm/i915/intel_psr.c >> @@ -56,6 +56,16 @@ >> #include "intel_drv.h" >> #include "i915_drv.h" >> >> +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; >> +} >> + >> void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool >> debug) >> { >> u32 debug_mask, mask; >> @@ -471,11 +481,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 >> @@ -517,7 +522,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" : ""); > This gets printed during a modeset that is shutting down the crtc. > >> + else >> + DRM_DEBUG_KMS("PSR disable by flag\n"); > >> } > So, neither debugfs nor modparam has any effect on crtc_state->has_psr > or crtc_state->has_psr2. Why was this check moved to the end? We calculate crtc_state->has_psr(2) to see if PSR can be enabled hardware wise. This will make sure that the state is correctly written in intel_psr_enable, even if we may not enable PSR by default. > We could also rewrite the debug message to look similar to the other > compute_config functions The debug message could be removed, or moved to intel_psr_enable. >> >> static void intel_psr_activate(struct intel_dp *intel_dp) >> @@ -589,6 +598,22 @@ static void intel_psr_enable_source(struct >> intel_dp *intel_dp, >> } >> } >> >> +static void intel_psr_enable_locked(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; >> + > This function gets called from intel_psr_set_debugfs_mode() Doesn't > this allow debugfs to enable PSR even if mode related checks in > psr_compute_config() had failed? For e.g., crtc_state->has_psr was > false. No, see intel_psr_enable. It only sets up state when crtc_state->has_psr is true. This is also why the check in intel_psr_compute_config is moved. >> + intel_psr_setup_vsc(intel_dp, crtc_state); >> + intel_psr_enable_sink(intel_dp); >> + intel_psr_enable_source(intel_dp, crtc_state); >> + dev_priv->psr.enabled = true; >> + >> + intel_psr_activate(intel_dp); >> +} >> + >> /** >> * intel_psr_enable - Enable PSR >> * @intel_dp: Intel DP >> @@ -611,7 +636,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) { > Check for dev_priv->psr.enabled instead? This is handled in intel_psr_enable_locked(). intel_psr_enable configures the state, but may not enable PSR right away if disabled by modparam or debugfs. > >> DRM_DEBUG_KMS("PSR already in use\n"); >> goto unlock; >> } >> @@ -619,12 +644,10 @@ void intel_psr_enable(struct intel_dp >> *intel_dp, >> dev_priv->psr.psr2_enabled = crtc_state->has_psr2; >> dev_priv->psr.busy_frontbuffer_bits = 0; >> >> - intel_psr_setup_vsc(intel_dp, crtc_state); >> - intel_psr_enable_sink(intel_dp); >> - intel_psr_enable_source(intel_dp, crtc_state); >> - dev_priv->psr.enabled = intel_dp; >> + dev_priv->psr.dp = intel_dp; > Now that there is psr.enabled, should we always keep this pointer > valid? No. >> >> - intel_psr_activate(intel_dp); >> + if (psr_global_enabled(dev_priv->psr.debugfs_mode)) > Okay, now I see why you have psr_global_enabled() as the last check in > psr_compute_config(). :) >> + intel_psr_enable_locked(dev_priv, crtc_state); >> >> unlock: >> mutex_unlock(&dev_priv->psr.lock); >> @@ -688,7 +711,7 @@ static void intel_psr_disable_locked(struct >> intel_dp *intel_dp) >> /* Disable PSR on Sink */ >> drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0); >> >> - dev_priv->psr.enabled = NULL; >> + dev_priv->psr.enabled = false; >> } >> >> /** >> @@ -712,7 +735,14 @@ void intel_psr_disable(struct intel_dp >> *intel_dp, >> return; >> >> mutex_lock(&dev_priv->psr.lock); >> + if (intel_dp != dev_priv->psr.dp) { >> + mutex_unlock(&dev_priv->psr.lock); >> + return; >> + } >> + >> intel_psr_disable_locked(intel_dp); >> + >> + dev_priv->psr.dp = NULL; > Is there still a need to use this field as flag? Yes. >> mutex_lunlock(&dev_priv->psr.lock); >> cancel_work_sync(&dev_priv->psr.work); >> } >> @@ -756,13 +786,11 @@ int intel_psr_wait_for_idle(const struct >> intel_crtc_state *new_crtc_state) >> >> static bool __psr_wait_for_idle_locked(struct drm_i915_private >> *dev_priv) >> { >> - struct intel_dp *intel_dp; >> i915_reg_t reg; >> u32 mask; >> int err; >> >> - intel_dp = dev_priv->psr.enabled; >> - if (!intel_dp) >> + if (!dev_priv->psr.enabled) >> return false; >> >> if (dev_priv->psr.psr2_enabled) { >> @@ -784,6 +812,89 @@ static bool __psr_wait_for_idle_locked(struct >> drm_i915_private *dev_priv) >> return err == 0 && dev_priv->psr.enabled; >> } >> >> +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; >> + >> + ret = drm_modeset_lock(&dev->mode_config.connection_mutex, >> ctx); >> + if (ret) >> + return ret; >> + >> + enable = psr_global_enabled(mode); >> + >> + mutex_lock(&dev_priv->psr.lock); >> + >> + do { >> + if (!dev_priv->psr.dp) { >> + dev_priv->psr.debugfs_mode = mode; >> + 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)); > When will this not be true? nonblocking modeset, for example. >> + >> + if (!enable) >> + intel_psr_disable_locked(enc_to_intel_dp(encoder)); >> + >> + dev_priv->psr.debugfs_mode = mode; >> + >> + if (enable) > bspec says PSR enable sequence requires ": The associated transcoder > and port are running and Aux channel associated with this port has IO > power enabled" Shouldn't crtc_state->active be checked. It's implied by having psr.dp != NULL >> + intel_psr_enable_locked(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 = >> @@ -811,7 +922,8 @@ static void intel_psr_work(struct work_struct >> *work) >> if (dev_priv->psr.busy_frontbuffer_bits || dev_priv- >>> psr.active) >> goto unlock; >> >> - intel_psr_activate(dev_priv->psr.enabled); >> + intel_psr_activate(dev_priv->psr.dp >> +); > Spurious new line. Indeed! >> unlock: >> mutex_unlock(&dev_priv->psr.lock); >> } >> @@ -866,7 +978,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); >> @@ -909,7 +1021,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); >> @@ -971,6 +1083,8 @@ void intel_psr_init(struct drm_i915_private >> *dev_priv) >> /* For new platforms let's respect VBT back again */ >> dev_priv->psr.link_standby = dev_priv- >>> vbt.psr.full_link; >> >> + dev_priv->psr.debugfs_mode = PSR_DEBUGFS_MODE_DEFAULT; >> + >> INIT_WORK(&dev_priv->psr.work, intel_psr_work); >> mutex_init(&dev_priv->psr.lock); >> } >> @@ -991,7 +1105,7 @@ void intel_psr_short_pulse(struct intel_dp >> *intel_dp) >> >> mutex_lock(&psr->lock); >> >> - if (psr->enabled != intel_dp) >> + if (!psr->enabled || psr->dp != intel_dp) >> goto exit; >> >> if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val) >> != 1) {
On Fri, 2018-07-27 at 10:41 +0200, Maarten Lankhorst wrote: > Op 27-07-18 om 05:27 schreef Dhinakaran Pandiyan: > > > > On Thu, 2018-07-26 at 11:06 +0200, 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. > > > > > > 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. > > > Changes since v2: > > > - Rebase on top of intel_psr changes. > > Thanks for resending this, I have some questions to understand the > > patch better. > > > > > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.c > > > om> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_debugfs.c | 75 ++++++++++++-- > > > drivers/gpu/drm/i915/i915_drv.h | 9 +- > > > drivers/gpu/drm/i915/intel_drv.h | 3 + > > > drivers/gpu/drm/i915/intel_psr.c | 154 > > > ++++++++++++++++++++++++ > > > ---- > > > 4 files changed, 214 insertions(+), 27 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > > b/drivers/gpu/drm/i915/i915_debugfs.c > > > index 59dc0610ea44..b2904bb2be49 100644 > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > > @@ -2689,14 +2689,11 @@ psr_source_status(struct drm_i915_private > > > *dev_priv, struct seq_file *m) > > > > > > 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; > > > 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) > > > @@ -2705,7 +2702,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); > > > > > > @@ -2776,6 +2773,72 @@ > > > DEFINE_SIMPLE_ATTRIBUTE(i915_edp_psr_debug_fops, > > > i915_edp_psr_debug_get, > > > i915_edp_psr_debug_set, > > > "%llu\n"); > > > > > > +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 != PSR_DEBUGFS_MODE_DEFAULT && > > > + val != PSR_DEBUGFS_MODE_DISABLED && > > > + val != PSR_DEBUGFS_MODE_ENABLED) > > > + 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); > > Deadlocked here during testing > > > > $ echo 0 > /sys/kernel/debug/dri/0/i915_edp_psr_status > > bash: echo: write error: Resource deadlock avoided > Oops, that check should be: "if (ret == -EDEADLK)" > > That should fix your error. :) > > > > [ 1248.856671] WARNING: CPU: 2 PID: 1788 at > > drivers/gpu/drm/drm_modeset_lock.c:223 > > drm_modeset_drop_locks+0x56/0x60 > > [drm] > > [ 1248.856757] Modules linked in: rfcomm cmac bnep arc4 iwlmvm > > nls_iso8859_1 mac80211 snd_hda_codec_hdmi snd_hda_codec_realtek > > snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hwdep > > snd_hda_core iwlwifi snd_pcm snd_seq_midi snd_seq_midi_event > > snd_rawmidi intel_rapl btusb btrtl snd_seq btbcm > > x86_pkg_temp_thermal > > btintel intel_powerclamp bluetooth coretemp crct10dif_pclmul > > crc32_pclmul snd_timer snd_seq_device ghash_clmulni_intel cfg80211 > > aesni_intel snd aes_x86_64 crypto_simd cryptd soundcore > > ecdh_generic > > glue_helper input_leds mei_me mei intel_pch_thermal mac_hid > > parport_pc > > ppdev lp parport autofs4 i915 i2c_algo_bit drm_kms_helper > > syscopyarea > > sysfillrect sysimgblt fb_sys_fops drm e1000e psmouse ahci libahci > > video > > [ 1248.857288] CPU: 2 PID: 1788 Comm: bash Not tainted 4.18.0- > > rc6drm- > > tip #138 > > [ 1248.857297] Hardware name: LENOVO 20F6CTO1WW/20F6CTO1WW, BIOS > > R02ET48W (1.21 ) 06/01/2016 > > [ 1248.857354] RIP: 0010:drm_modeset_drop_locks+0x56/0x60 [drm] > > [ 1248.857363] Code: 50 08 48 8d b8 50 ff ff ff 48 89 51 08 48 89 > > 0a 48 > > 89 00 48 89 40 08 e8 a8 90 7d c9 48 8b 43 70 49 39 c4 75 d2 5b 41 > > 5c 5d > > c3 <0f> 0b eb bc 66 0f 1f 44 00 00 0f 1f 44 00 00 48 8b 97 d0 0a 00 > > 00 > > [ 1248.857860] RSP: 0018:ffffa4dd01fabcf8 EFLAGS: 00010286 > > [ 1248.857878] RAX: 00000000ffffffdd RBX: ffffa4dd01fabd28 RCX: > > 0000000000000000 > > [ 1248.857889] RDX: 0000000000000000 RSI: ffff97bb88476898 RDI: > > ffffa4dd01fabd28 > > [ 1248.857898] RBP: ffffa4dd01fabd08 R08: 0000000000000000 R09: > > 0000000000000001 > > [ 1248.857908] R10: 0000000000000001 R11: 0000000000000000 R12: > > 0000000000000002 > > [ 1248.857918] R13: 00005603cdb60880 R14: 0000000000000002 R15: > > ffffa4dd01fabee8 > > [ 1248.857930] FS: 00007f83b1dea740(0000) > > GS:ffff97bb99a00000(0000) > > knlGS:0000000000000000 > > [ 1248.857940] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > [ 1248.857950] CR2: 00005603cdb60880 CR3: 0000000214338003 CR4: > > 00000000003606e0 > > [ 1248.857959] Call Trace: > > [ 1248.858094] i915_edp_psr_write+0xd5/0x180 [i915] > > [ 1248.858172] full_proxy_write+0x5f/0x90 > > [ 1248.858207] __vfs_write+0x3a/0x1a0 > > [ 1248.858227] ? rcu_read_lock_sched_held+0x79/0x80 > > [ 1248.858243] ? rcu_sync_lockdep_assert+0x32/0x60 > > [ 1248.858260] ? __sb_start_write+0x135/0x190 > > [ 1248.858275] ? vfs_write+0x193/0x1c0 > > [ 1248.858306] vfs_write+0xc6/0x1c0 > > [ 1248.858335] ksys_write+0x58/0xc0 > > [ 1248.858370] __x64_sys_write+0x1a/0x20 > > [ 1248.858387] do_syscall_64+0x65/0x1b0 > > [ 1248.858409] entry_SYSCALL_64_after_hwframe+0x49/0xbe > > [ 1248.858423] RIP: 0033:0x7f83b14f2154 > > [ 1248.858430] Code: 89 02 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 > > 00 00 > > 00 00 00 66 90 48 8d 05 b1 07 2e 00 8b 00 85 c0 75 13 b8 01 00 00 > > 00 0f > > 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 41 54 55 49 89 d4 53 48 89 > > f5 > > [ 1248.858928] RSP: 002b:00007ffee7320168 EFLAGS: 00000246 > > ORIG_RAX: > > 0000000000000001 > > [ 1248.858946] RAX: ffffffffffffffda RBX: 0000000000000002 RCX: > > 00007f83b14f2154 > > [ 1248.858956] RDX: 0000000000000002 RSI: 00005603cdb60880 RDI: > > 0000000000000001 > > [ 1248.858965] RBP: 00005603cdb60880 R08: 000000000000000a R09: > > 0000000000000001 > > [ 1248.858975] R10: 000000000000000a R11: 0000000000000246 R12: > > 00007f83b17ce760 > > [ 1248.858984] R13: 0000000000000002 R14: 00007f83b17ca2a0 R15: > > 00007f83b17c9760 > > [ 1248.859043] irq event stamp: 59768 > > [ 1248.859058] hardirqs last enabled at (59767): > > [<ffffffff89a31dc6>] > > _raw_spin_unlock_irqrestore+0x36/0x60 > > [ 1248.859072] hardirqs last disabled at (59768): > > [<ffffffff89c01309>] > > error_entry+0x89/0x110 > > [ 1248.859087] softirqs last enabled at (59724): > > [<ffffffff89e00378>] > > __do_softirq+0x378/0x4e3 > > [ 1248.859100] softirqs last disabled at (59711): > > [<ffffffff89098e0d>] > > irq_exit+0xcd/0xe0 > > [ 1248.859156] WARNING: CPU: 2 PID: 1788 at > > drivers/gpu/drm/drm_modeset_lock.c:223 > > drm_modeset_drop_locks+0x56/0x60 > > [drm] > > [ 1248.859166] ---[ end trace b7222f9239d3065b ]-- > And this warning. > > > > > > > > > > + > > > + 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); > > What do you think of using "i915_edp_psr_debug" instead? > All for it. > > > > > > > > +} > > > + > > > +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_energy_uJ(struct seq_file *m, void *data) > > > { > > > struct drm_i915_private *dev_priv = node_to_i915(m- > > > > > > > > private); > > > @@ -4720,7 +4783,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_energy_uJ", i915_energy_uJ, 0}, > > > {"i915_runtime_pm_status", i915_runtime_pm_status, 0}, > > > {"i915_power_domain_info", i915_power_domain_info, 0}, > > > @@ -4744,6 +4806,7 @@ static const struct i915_debugfs_files { > > > const struct file_operations *fops; > > > } i915_debugfs_files[] = { > > > {"i915_wedged", &i915_wedged_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 0f49f9988dfa..d8583770e8a6 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -612,7 +612,8 @@ struct i915_drrs { > > > struct i915_psr { > > > struct mutex lock; > > > bool sink_support; > > > - struct intel_dp *enabled; > > > + bool enabled; > > > + struct intel_dp *dp; > > Separate patch for this change? How about keeping i915_psr.dp > > always > > valid? > I'm keeping i915_psr.dp only valid when the modeset calls > intel_psr_enable, until the modeset disable calls intel_psr_disable() > i915_psr.dp is used to also indicate that we can currently enable > PSR. .enabled is used to determine it's currently enabled. > Realized .dp might mean something else after I hit send. Thanks for explaining it. I think the field warrants renaming, not the least because now .psr2_enabled means can enable PSR2 .enabled means psr1 or psr2 is currently enabled .dp means can enable psr1 or psr2 how about having { bool enable_ready; bool enable; struct intel_dp *dp; } Where .dp is just a pointer to intel_dp, no hidden meaning attached. Given that the code currently supports PSR on only one encoder, we can assign dp = intel_dp during init. > > > > > > > > bool active; > > > struct work_struct work; > > > unsigned busy_frontbuffer_bits; > > > @@ -625,6 +626,12 @@ struct i915_psr { > > > bool debug; > > > ktime_t last_entry_attempt; > > > ktime_t last_exit; > > > + > > > + enum i915_psr_debugfs_mode { > > > + PSR_DEBUGFS_MODE_DEFAULT = -1, > > > + PSR_DEBUGFS_MODE_DISABLED, > > > + PSR_DEBUGFS_MODE_ENABLED > > If we add another enum, we can write tests to enable PSR1 on PSR2 > > panels. > > PSR_DEBUGFS_MODE_PSR1 > > PSR_DEBUGFS_MODE_PSR2 > Should be easy to add, but annoying to toggle. > > Maybe some followup? > > > > > > > > + } debugfs_mode; > > > }; > > > > > > enum intel_pch { > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > b/drivers/gpu/drm/i915/intel_drv.h > > > index c275f91244a6..751ed257fbba 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -1926,6 +1926,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 4bd5768731ee..97424ae769f3 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -56,6 +56,16 @@ > > > #include "intel_drv.h" > > > #include "i915_drv.h" > > > > > > +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; > > > +} > > > + > > > void intel_psr_irq_control(struct drm_i915_private *dev_priv, > > > bool > > > debug) > > > { > > > u32 debug_mask, mask; > > > @@ -471,11 +481,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 > > > @@ -517,7 +522,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" : ""); > > This gets printed during a modeset that is shutting down the crtc. > > > > > > > > + else > > > + DRM_DEBUG_KMS("PSR disable by flag\n"); > > > > > > } > > So, neither debugfs nor modparam has any effect on crtc_state- > > >has_psr > > or crtc_state->has_psr2. Why was this check moved to the end? > We calculate crtc_state->has_psr(2) to see if PSR can be enabled > hardware wise. > > This will make sure that the state is correctly written in > intel_psr_enable, even if we may not enable PSR by default. > > > > We could also rewrite the debug message to look similar to the > > other > > compute_config functions > The debug message could be removed, or moved to intel_psr_enable. let's move enable debug messages to psr_enable_locked() and psr_disable_locked() > > > > > > > > > > > static void intel_psr_activate(struct intel_dp *intel_dp) > > > @@ -589,6 +598,22 @@ static void intel_psr_enable_source(struct > > > intel_dp *intel_dp, > > > } > > > } > > > > > > +static void intel_psr_enable_locked(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; > > > + > > This function gets called from intel_psr_set_debugfs_mode() Doesn't > > this allow debugfs to enable PSR even if mode related checks in > > psr_compute_config() had failed? For e.g., crtc_state->has_psr was > > false. > No, see intel_psr_enable. It only sets up state when crtc_state- > >has_psr is true. This is also why the check in > intel_psr_compute_config is moved. > > > > > > > > + intel_psr_setup_vsc(intel_dp, crtc_state); > > > + intel_psr_enable_sink(intel_dp); > > > + intel_psr_enable_source(intel_dp, crtc_state); > > > + dev_priv->psr.enabled = true; > > > + > > > + intel_psr_activate(intel_dp); > > > +} > > > + > > > /** > > > * intel_psr_enable - Enable PSR > > > * @intel_dp: Intel DP > > > @@ -611,7 +636,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) { > > Check for dev_priv->psr.enabled instead? > This is handled in intel_psr_enable_locked(). > > intel_psr_enable configures the state, but may not enable PSR right > away if disabled by modparam or debugfs. > > > > > > > > > > DRM_DEBUG_KMS("PSR already in use\n"); > > > goto unlock; > > > } > > > @@ -619,12 +644,10 @@ void intel_psr_enable(struct intel_dp > > > *intel_dp, > > > dev_priv->psr.psr2_enabled = crtc_state->has_psr2; > > > dev_priv->psr.busy_frontbuffer_bits = 0; > > > > > > - intel_psr_setup_vsc(intel_dp, crtc_state); > > > - intel_psr_enable_sink(intel_dp); > > > - intel_psr_enable_source(intel_dp, crtc_state); > > > - dev_priv->psr.enabled = intel_dp; > > > + dev_priv->psr.dp = intel_dp; > > Now that there is psr.enabled, should we always keep this pointer > > valid? > No. > > > > > > > > > > > - intel_psr_activate(intel_dp); > > > + if (psr_global_enabled(dev_priv->psr.debugfs_mode)) > > Okay, now I see why you have psr_global_enabled() as the last check > > in > > psr_compute_config(). > :) > > > > > > > > + intel_psr_enable_locked(dev_priv, crtc_state); > > > > > > unlock: > > > mutex_unlock(&dev_priv->psr.lock); > > > @@ -688,7 +711,7 @@ static void intel_psr_disable_locked(struct > > > intel_dp *intel_dp) > > > /* Disable PSR on Sink */ > > > drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0); > > > > > > - dev_priv->psr.enabled = NULL; > > > + dev_priv->psr.enabled = false; > > > } > > > > > > /** > > > @@ -712,7 +735,14 @@ void intel_psr_disable(struct intel_dp > > > *intel_dp, > > > return; > > > > > > mutex_lock(&dev_priv->psr.lock); > > > + if (intel_dp != dev_priv->psr.dp) { > > > + mutex_unlock(&dev_priv->psr.lock); > > > + return; > > > + } > > > + > > > intel_psr_disable_locked(intel_dp); > > > + > > > + dev_priv->psr.dp = NULL; > > Is there still a need to use this field as flag? > Yes. > > > > > > > > mutex_lunlock(&dev_priv->psr.lock); > > > cancel_work_sync(&dev_priv->psr.work); > > > } > > > @@ -756,13 +786,11 @@ int intel_psr_wait_for_idle(const struct > > > intel_crtc_state *new_crtc_state) > > > > > > static bool __psr_wait_for_idle_locked(struct drm_i915_private > > > *dev_priv) > > > { > > > - struct intel_dp *intel_dp; > > > i915_reg_t reg; > > > u32 mask; > > > int err; > > > > > > - intel_dp = dev_priv->psr.enabled; > > > - if (!intel_dp) > > > + if (!dev_priv->psr.enabled) > > > return false; > > > > > > if (dev_priv->psr.psr2_enabled) { > > > @@ -784,6 +812,89 @@ static bool > > > __psr_wait_for_idle_locked(struct > > > drm_i915_private *dev_priv) > > > return err == 0 && dev_priv->psr.enabled; > > > } > > > > > > +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; > > > + > > > + ret = drm_modeset_lock(&dev- > > > >mode_config.connection_mutex, > > > ctx); > > > + if (ret) > > > + return ret; > > > + > > > + enable = psr_global_enabled(mode); > > > + > > > + mutex_lock(&dev_priv->psr.lock); > > > + > > > + do { > > > + if (!dev_priv->psr.dp) { > > > + dev_priv->psr.debugfs_mode = mode; > > > + 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)); > > When will this not be true? > nonblocking modeset, for example. With psr.dp = intel_dp statically assigned in psr_init_dpcd(), can we get rid of this check? And the connector loop in find_idle_crtc_for_encoder() can just be intel_dp->attached_connector. > > > > > > > > + > > > + if (!enable) > > > + intel_psr_disable_locked(enc_to_intel_dp(encoder > > > )); > > > + > > > + dev_priv->psr.debugfs_mode = mode; > > > + > > > + if (enable) > > bspec says PSR enable sequence requires ": The associated > > transcoder > > and port are running and Aux channel associated with this port has > > IO > > power enabled" Shouldn't crtc_state->active be checked. > It's implied by having psr.dp != NULL > > > > > > > > + intel_psr_enable_locked(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 = > > > @@ -811,7 +922,8 @@ static void intel_psr_work(struct work_struct > > > *work) > > > if (dev_priv->psr.busy_frontbuffer_bits || dev_priv- > > > > > > > > psr.active) > > > goto unlock; > > > > > > - intel_psr_activate(dev_priv->psr.enabled); > > > + intel_psr_activate(dev_priv->psr.dp > > > +); > > Spurious new line. > Indeed! > > > > > > > > unlock: > > > mutex_unlock(&dev_priv->psr.lock); > > > } > > > @@ -866,7 +978,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); > > > @@ -909,7 +1021,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); > > > @@ -971,6 +1083,8 @@ void intel_psr_init(struct drm_i915_private > > > *dev_priv) > > > /* For new platforms let's respect VBT back > > > again */ > > > dev_priv->psr.link_standby = dev_priv- > > > > > > > > vbt.psr.full_link; > > > > > > + dev_priv->psr.debugfs_mode = PSR_DEBUGFS_MODE_DEFAULT; > > > + > > > INIT_WORK(&dev_priv->psr.work, intel_psr_work); > > > mutex_init(&dev_priv->psr.lock); > > > } > > > @@ -991,7 +1105,7 @@ void intel_psr_short_pulse(struct intel_dp > > > *intel_dp) > > > > > > mutex_lock(&psr->lock); > > > > > > - if (psr->enabled != intel_dp) > > > + if (!psr->enabled || psr->dp != intel_dp) > > > goto exit; > > > > > > if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, > > > &val) > > > != 1) { >
Op 28-07-18 om 07:23 schreef Dhinakaran Pandiyan: > On Fri, 2018-07-27 at 10:41 +0200, Maarten Lankhorst wrote: >> Op 27-07-18 om 05:27 schreef Dhinakaran Pandiyan: >>> On Thu, 2018-07-26 at 11:06 +0200, 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. >>>> >>>> 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. >>>> Changes since v2: >>>> - Rebase on top of intel_psr changes. >>> Thanks for resending this, I have some questions to understand the >>> patch better. >>> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.c >>>> om> >>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/i915_debugfs.c | 75 ++++++++++++-- >>>> drivers/gpu/drm/i915/i915_drv.h | 9 +- >>>> drivers/gpu/drm/i915/intel_drv.h | 3 + >>>> drivers/gpu/drm/i915/intel_psr.c | 154 >>>> ++++++++++++++++++++++++ >>>> ---- >>>> 4 files changed, 214 insertions(+), 27 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c >>>> b/drivers/gpu/drm/i915/i915_debugfs.c >>>> index 59dc0610ea44..b2904bb2be49 100644 >>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c >>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >>>> @@ -2689,14 +2689,11 @@ psr_source_status(struct drm_i915_private >>>> *dev_priv, struct seq_file *m) >>>> >>>> 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; >>>> 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) >>>> @@ -2705,7 +2702,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); >>>> >>>> @@ -2776,6 +2773,72 @@ >>>> DEFINE_SIMPLE_ATTRIBUTE(i915_edp_psr_debug_fops, >>>> i915_edp_psr_debug_get, >>>> i915_edp_psr_debug_set, >>>> "%llu\n"); >>>> >>>> +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 != PSR_DEBUGFS_MODE_DEFAULT && >>>> + val != PSR_DEBUGFS_MODE_DISABLED && >>>> + val != PSR_DEBUGFS_MODE_ENABLED) >>>> + 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); >>> Deadlocked here during testing >>> >>> $ echo 0 > /sys/kernel/debug/dri/0/i915_edp_psr_status >>> bash: echo: write error: Resource deadlock avoided >> Oops, that check should be: "if (ret == -EDEADLK)" >> >> That should fix your error. :) >>> [ 1248.856671] WARNING: CPU: 2 PID: 1788 at >>> drivers/gpu/drm/drm_modeset_lock.c:223 >>> drm_modeset_drop_locks+0x56/0x60 >>> [drm] >>> [ 1248.856757] Modules linked in: rfcomm cmac bnep arc4 iwlmvm >>> nls_iso8859_1 mac80211 snd_hda_codec_hdmi snd_hda_codec_realtek >>> snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hwdep >>> snd_hda_core iwlwifi snd_pcm snd_seq_midi snd_seq_midi_event >>> snd_rawmidi intel_rapl btusb btrtl snd_seq btbcm >>> x86_pkg_temp_thermal >>> btintel intel_powerclamp bluetooth coretemp crct10dif_pclmul >>> crc32_pclmul snd_timer snd_seq_device ghash_clmulni_intel cfg80211 >>> aesni_intel snd aes_x86_64 crypto_simd cryptd soundcore >>> ecdh_generic >>> glue_helper input_leds mei_me mei intel_pch_thermal mac_hid >>> parport_pc >>> ppdev lp parport autofs4 i915 i2c_algo_bit drm_kms_helper >>> syscopyarea >>> sysfillrect sysimgblt fb_sys_fops drm e1000e psmouse ahci libahci >>> video >>> [ 1248.857288] CPU: 2 PID: 1788 Comm: bash Not tainted 4.18.0- >>> rc6drm- >>> tip #138 >>> [ 1248.857297] Hardware name: LENOVO 20F6CTO1WW/20F6CTO1WW, BIOS >>> R02ET48W (1.21 ) 06/01/2016 >>> [ 1248.857354] RIP: 0010:drm_modeset_drop_locks+0x56/0x60 [drm] >>> [ 1248.857363] Code: 50 08 48 8d b8 50 ff ff ff 48 89 51 08 48 89 >>> 0a 48 >>> 89 00 48 89 40 08 e8 a8 90 7d c9 48 8b 43 70 49 39 c4 75 d2 5b 41 >>> 5c 5d >>> c3 <0f> 0b eb bc 66 0f 1f 44 00 00 0f 1f 44 00 00 48 8b 97 d0 0a 00 >>> 00 >>> [ 1248.857860] RSP: 0018:ffffa4dd01fabcf8 EFLAGS: 00010286 >>> [ 1248.857878] RAX: 00000000ffffffdd RBX: ffffa4dd01fabd28 RCX: >>> 0000000000000000 >>> [ 1248.857889] RDX: 0000000000000000 RSI: ffff97bb88476898 RDI: >>> ffffa4dd01fabd28 >>> [ 1248.857898] RBP: ffffa4dd01fabd08 R08: 0000000000000000 R09: >>> 0000000000000001 >>> [ 1248.857908] R10: 0000000000000001 R11: 0000000000000000 R12: >>> 0000000000000002 >>> [ 1248.857918] R13: 00005603cdb60880 R14: 0000000000000002 R15: >>> ffffa4dd01fabee8 >>> [ 1248.857930] FS: 00007f83b1dea740(0000) >>> GS:ffff97bb99a00000(0000) >>> knlGS:0000000000000000 >>> [ 1248.857940] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >>> [ 1248.857950] CR2: 00005603cdb60880 CR3: 0000000214338003 CR4: >>> 00000000003606e0 >>> [ 1248.857959] Call Trace: >>> [ 1248.858094] i915_edp_psr_write+0xd5/0x180 [i915] >>> [ 1248.858172] full_proxy_write+0x5f/0x90 >>> [ 1248.858207] __vfs_write+0x3a/0x1a0 >>> [ 1248.858227] ? rcu_read_lock_sched_held+0x79/0x80 >>> [ 1248.858243] ? rcu_sync_lockdep_assert+0x32/0x60 >>> [ 1248.858260] ? __sb_start_write+0x135/0x190 >>> [ 1248.858275] ? vfs_write+0x193/0x1c0 >>> [ 1248.858306] vfs_write+0xc6/0x1c0 >>> [ 1248.858335] ksys_write+0x58/0xc0 >>> [ 1248.858370] __x64_sys_write+0x1a/0x20 >>> [ 1248.858387] do_syscall_64+0x65/0x1b0 >>> [ 1248.858409] entry_SYSCALL_64_after_hwframe+0x49/0xbe >>> [ 1248.858423] RIP: 0033:0x7f83b14f2154 >>> [ 1248.858430] Code: 89 02 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 >>> 00 00 >>> 00 00 00 66 90 48 8d 05 b1 07 2e 00 8b 00 85 c0 75 13 b8 01 00 00 >>> 00 0f >>> 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 41 54 55 49 89 d4 53 48 89 >>> f5 >>> [ 1248.858928] RSP: 002b:00007ffee7320168 EFLAGS: 00000246 >>> ORIG_RAX: >>> 0000000000000001 >>> [ 1248.858946] RAX: ffffffffffffffda RBX: 0000000000000002 RCX: >>> 00007f83b14f2154 >>> [ 1248.858956] RDX: 0000000000000002 RSI: 00005603cdb60880 RDI: >>> 0000000000000001 >>> [ 1248.858965] RBP: 00005603cdb60880 R08: 000000000000000a R09: >>> 0000000000000001 >>> [ 1248.858975] R10: 000000000000000a R11: 0000000000000246 R12: >>> 00007f83b17ce760 >>> [ 1248.858984] R13: 0000000000000002 R14: 00007f83b17ca2a0 R15: >>> 00007f83b17c9760 >>> [ 1248.859043] irq event stamp: 59768 >>> [ 1248.859058] hardirqs last enabled at (59767): >>> [<ffffffff89a31dc6>] >>> _raw_spin_unlock_irqrestore+0x36/0x60 >>> [ 1248.859072] hardirqs last disabled at (59768): >>> [<ffffffff89c01309>] >>> error_entry+0x89/0x110 >>> [ 1248.859087] softirqs last enabled at (59724): >>> [<ffffffff89e00378>] >>> __do_softirq+0x378/0x4e3 >>> [ 1248.859100] softirqs last disabled at (59711): >>> [<ffffffff89098e0d>] >>> irq_exit+0xcd/0xe0 >>> [ 1248.859156] WARNING: CPU: 2 PID: 1788 at >>> drivers/gpu/drm/drm_modeset_lock.c:223 >>> drm_modeset_drop_locks+0x56/0x60 >>> [drm] >>> [ 1248.859166] ---[ end trace b7222f9239d3065b ]-- >> And this warning. >>> >>>> + >>>> + 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); >>> What do you think of using "i915_edp_psr_debug" instead? >> All for it. >>>> +} >>>> + >>>> +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_energy_uJ(struct seq_file *m, void *data) >>>> { >>>> struct drm_i915_private *dev_priv = node_to_i915(m- >>>>> private); >>>> @@ -4720,7 +4783,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_energy_uJ", i915_energy_uJ, 0}, >>>> {"i915_runtime_pm_status", i915_runtime_pm_status, 0}, >>>> {"i915_power_domain_info", i915_power_domain_info, 0}, >>>> @@ -4744,6 +4806,7 @@ static const struct i915_debugfs_files { >>>> const struct file_operations *fops; >>>> } i915_debugfs_files[] = { >>>> {"i915_wedged", &i915_wedged_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 0f49f9988dfa..d8583770e8a6 100644 >>>> --- a/drivers/gpu/drm/i915/i915_drv.h >>>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>>> @@ -612,7 +612,8 @@ struct i915_drrs { >>>> struct i915_psr { >>>> struct mutex lock; >>>> bool sink_support; >>>> - struct intel_dp *enabled; >>>> + bool enabled; >>>> + struct intel_dp *dp; >>> Separate patch for this change? How about keeping i915_psr.dp >>> always >>> valid? >> I'm keeping i915_psr.dp only valid when the modeset calls >> intel_psr_enable, until the modeset disable calls intel_psr_disable() >> i915_psr.dp is used to also indicate that we can currently enable >> PSR. .enabled is used to determine it's currently enabled. >> > Realized .dp might mean something else after I hit send. Thanks for > explaining it. > > I think the field warrants renaming, not the least because now > .psr2_enabled means can enable PSR2 > .enabled means psr1 or psr2 is currently enabled > .dp means can enable psr1 or psr2 > > how about having > > { > bool enable_ready; > bool enable; > struct intel_dp *dp; > } > > Where .dp is just a pointer to intel_dp, no hidden meaning attached. > Given that the code currently supports PSR on only one encoder, we can > assign dp = intel_dp during init. Yeah would make sense. I think renaming it from enable_ready to prepared would be more clear.. > >>>> bool active; >>>> struct work_struct work; >>>> unsigned busy_frontbuffer_bits; >>>> @@ -625,6 +626,12 @@ struct i915_psr { >>>> bool debug; >>>> ktime_t last_entry_attempt; >>>> ktime_t last_exit; >>>> + >>>> + enum i915_psr_debugfs_mode { >>>> + PSR_DEBUGFS_MODE_DEFAULT = -1, >>>> + PSR_DEBUGFS_MODE_DISABLED, >>>> + PSR_DEBUGFS_MODE_ENABLED >>> If we add another enum, we can write tests to enable PSR1 on PSR2 >>> panels. >>> PSR_DEBUGFS_MODE_PSR1 >>> PSR_DEBUGFS_MODE_PSR2 >> Should be easy to add, but annoying to toggle. >> >> Maybe some followup? >>>> + } debugfs_mode; >>>> }; >>>> >>>> enum intel_pch { >>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h >>>> b/drivers/gpu/drm/i915/intel_drv.h >>>> index c275f91244a6..751ed257fbba 100644 >>>> --- a/drivers/gpu/drm/i915/intel_drv.h >>>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>>> @@ -1926,6 +1926,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 4bd5768731ee..97424ae769f3 100644 >>>> --- a/drivers/gpu/drm/i915/intel_psr.c >>>> +++ b/drivers/gpu/drm/i915/intel_psr.c >>>> @@ -56,6 +56,16 @@ >>>> #include "intel_drv.h" >>>> #include "i915_drv.h" >>>> >>>> +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; >>>> +} >>>> + >>>> void intel_psr_irq_control(struct drm_i915_private *dev_priv, >>>> bool >>>> debug) >>>> { >>>> u32 debug_mask, mask; >>>> @@ -471,11 +481,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 >>>> @@ -517,7 +522,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" : ""); >>> This gets printed during a modeset that is shutting down the crtc. >>> >>>> + else >>>> + DRM_DEBUG_KMS("PSR disable by flag\n"); >>>> >>>> } >>> So, neither debugfs nor modparam has any effect on crtc_state- >>>> has_psr >>> or crtc_state->has_psr2. Why was this check moved to the end? >> We calculate crtc_state->has_psr(2) to see if PSR can be enabled >> hardware wise. >> >> This will make sure that the state is correctly written in >> intel_psr_enable, even if we may not enable PSR by default. >>> We could also rewrite the debug message to look similar to the >>> other >>> compute_config functions >> The debug message could be removed, or moved to intel_psr_enable. > let's move enable debug messages to psr_enable_locked() and > psr_disable_locked() Yeah. >>>> >>>> static void intel_psr_activate(struct intel_dp *intel_dp) >>>> @@ -589,6 +598,22 @@ static void intel_psr_enable_source(struct >>>> intel_dp *intel_dp, >>>> } >>>> } >>>> >>>> +static void intel_psr_enable_locked(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; >>>> + >>> This function gets called from intel_psr_set_debugfs_mode() Doesn't >>> this allow debugfs to enable PSR even if mode related checks in >>> psr_compute_config() had failed? For e.g., crtc_state->has_psr was >>> false. >> No, see intel_psr_enable. It only sets up state when crtc_state- >>> has_psr is true. This is also why the check in >> intel_psr_compute_config is moved. >>>> + intel_psr_setup_vsc(intel_dp, crtc_state); >>>> + intel_psr_enable_sink(intel_dp); >>>> + intel_psr_enable_source(intel_dp, crtc_state); >>>> + dev_priv->psr.enabled = true; >>>> + >>>> + intel_psr_activate(intel_dp); >>>> +} >>>> + >>>> /** >>>> * intel_psr_enable - Enable PSR >>>> * @intel_dp: Intel DP >>>> @@ -611,7 +636,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) { >>> Check for dev_priv->psr.enabled instead? >> This is handled in intel_psr_enable_locked(). >> >> intel_psr_enable configures the state, but may not enable PSR right >> away if disabled by modparam or debugfs. >>> >>>> DRM_DEBUG_KMS("PSR already in use\n"); >>>> goto unlock; >>>> } >>>> @@ -619,12 +644,10 @@ void intel_psr_enable(struct intel_dp >>>> *intel_dp, >>>> dev_priv->psr.psr2_enabled = crtc_state->has_psr2; >>>> dev_priv->psr.busy_frontbuffer_bits = 0; >>>> >>>> - intel_psr_setup_vsc(intel_dp, crtc_state); >>>> - intel_psr_enable_sink(intel_dp); >>>> - intel_psr_enable_source(intel_dp, crtc_state); >>>> - dev_priv->psr.enabled = intel_dp; >>>> + dev_priv->psr.dp = intel_dp; >>> Now that there is psr.enabled, should we always keep this pointer >>> valid? >> No. >>>> >>>> - intel_psr_activate(intel_dp); >>>> + if (psr_global_enabled(dev_priv->psr.debugfs_mode)) >>> Okay, now I see why you have psr_global_enabled() as the last check >>> in >>> psr_compute_config(). >> :) >>>> + intel_psr_enable_locked(dev_priv, crtc_state); >>>> >>>> unlock: >>>> mutex_unlock(&dev_priv->psr.lock); >>>> @@ -688,7 +711,7 @@ static void intel_psr_disable_locked(struct >>>> intel_dp *intel_dp) >>>> /* Disable PSR on Sink */ >>>> drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0); >>>> >>>> - dev_priv->psr.enabled = NULL; >>>> + dev_priv->psr.enabled = false; >>>> } >>>> >>>> /** >>>> @@ -712,7 +735,14 @@ void intel_psr_disable(struct intel_dp >>>> *intel_dp, >>>> return; >>>> >>>> mutex_lock(&dev_priv->psr.lock); >>>> + if (intel_dp != dev_priv->psr.dp) { >>>> + mutex_unlock(&dev_priv->psr.lock); >>>> + return; >>>> + } >>>> + >>>> intel_psr_disable_locked(intel_dp); >>>> + >>>> + dev_priv->psr.dp = NULL; >>> Is there still a need to use this field as flag? >> Yes. >>>> mutex_lunlock(&dev_priv->psr.lock); >>>> cancel_work_sync(&dev_priv->psr.work); >>>> } >>>> @@ -756,13 +786,11 @@ int intel_psr_wait_for_idle(const struct >>>> intel_crtc_state *new_crtc_state) >>>> >>>> static bool __psr_wait_for_idle_locked(struct drm_i915_private >>>> *dev_priv) >>>> { >>>> - struct intel_dp *intel_dp; >>>> i915_reg_t reg; >>>> u32 mask; >>>> int err; >>>> >>>> - intel_dp = dev_priv->psr.enabled; >>>> - if (!intel_dp) >>>> + if (!dev_priv->psr.enabled) >>>> return false; >>>> >>>> if (dev_priv->psr.psr2_enabled) { >>>> @@ -784,6 +812,89 @@ static bool >>>> __psr_wait_for_idle_locked(struct >>>> drm_i915_private *dev_priv) >>>> return err == 0 && dev_priv->psr.enabled; >>>> } >>>> >>>> +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; >>>> + >>>> + ret = drm_modeset_lock(&dev- >>>>> mode_config.connection_mutex, >>>> ctx); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + enable = psr_global_enabled(mode); >>>> + >>>> + mutex_lock(&dev_priv->psr.lock); >>>> + >>>> + do { >>>> + if (!dev_priv->psr.dp) { >>>> + dev_priv->psr.debugfs_mode = mode; >>>> + 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)); >>> When will this not be true? >> nonblocking modeset, for example. > With psr.dp = intel_dp statically assigned in psr_init_dpcd(), can we > get rid of this check? And the connector loop in > find_idle_crtc_for_encoder() can just be intel_dp->attached_connector. Should simplify things indeed. ~Maarten
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 59dc0610ea44..b2904bb2be49 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2689,14 +2689,11 @@ psr_source_status(struct drm_i915_private *dev_priv, struct seq_file *m) 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; 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) @@ -2705,7 +2702,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); @@ -2776,6 +2773,72 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_edp_psr_debug_fops, i915_edp_psr_debug_get, i915_edp_psr_debug_set, "%llu\n"); +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 != PSR_DEBUGFS_MODE_DEFAULT && + val != PSR_DEBUGFS_MODE_DISABLED && + val != PSR_DEBUGFS_MODE_ENABLED) + 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_energy_uJ(struct seq_file *m, void *data) { struct drm_i915_private *dev_priv = node_to_i915(m->private); @@ -4720,7 +4783,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_energy_uJ", i915_energy_uJ, 0}, {"i915_runtime_pm_status", i915_runtime_pm_status, 0}, {"i915_power_domain_info", i915_power_domain_info, 0}, @@ -4744,6 +4806,7 @@ static const struct i915_debugfs_files { const struct file_operations *fops; } i915_debugfs_files[] = { {"i915_wedged", &i915_wedged_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 0f49f9988dfa..d8583770e8a6 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -612,7 +612,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 work_struct work; unsigned busy_frontbuffer_bits; @@ -625,6 +626,12 @@ struct i915_psr { bool debug; ktime_t last_entry_attempt; ktime_t last_exit; + + enum i915_psr_debugfs_mode { + PSR_DEBUGFS_MODE_DEFAULT = -1, + PSR_DEBUGFS_MODE_DISABLED, + PSR_DEBUGFS_MODE_ENABLED + } debugfs_mode; }; enum intel_pch { diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index c275f91244a6..751ed257fbba 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1926,6 +1926,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 4bd5768731ee..97424ae769f3 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -56,6 +56,16 @@ #include "intel_drv.h" #include "i915_drv.h" +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; +} + void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool debug) { u32 debug_mask, mask; @@ -471,11 +481,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 @@ -517,7 +522,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) @@ -589,6 +598,22 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp, } } +static void intel_psr_enable_locked(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; + + intel_psr_setup_vsc(intel_dp, crtc_state); + intel_psr_enable_sink(intel_dp); + intel_psr_enable_source(intel_dp, crtc_state); + dev_priv->psr.enabled = true; + + intel_psr_activate(intel_dp); +} + /** * intel_psr_enable - Enable PSR * @intel_dp: Intel DP @@ -611,7 +636,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; } @@ -619,12 +644,10 @@ void intel_psr_enable(struct intel_dp *intel_dp, dev_priv->psr.psr2_enabled = crtc_state->has_psr2; dev_priv->psr.busy_frontbuffer_bits = 0; - intel_psr_setup_vsc(intel_dp, crtc_state); - intel_psr_enable_sink(intel_dp); - intel_psr_enable_source(intel_dp, crtc_state); - dev_priv->psr.enabled = intel_dp; + dev_priv->psr.dp = intel_dp; - intel_psr_activate(intel_dp); + if (psr_global_enabled(dev_priv->psr.debugfs_mode)) + intel_psr_enable_locked(dev_priv, crtc_state); unlock: mutex_unlock(&dev_priv->psr.lock); @@ -688,7 +711,7 @@ static void intel_psr_disable_locked(struct intel_dp *intel_dp) /* Disable PSR on Sink */ drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0); - dev_priv->psr.enabled = NULL; + dev_priv->psr.enabled = false; } /** @@ -712,7 +735,14 @@ void intel_psr_disable(struct intel_dp *intel_dp, return; mutex_lock(&dev_priv->psr.lock); + if (intel_dp != dev_priv->psr.dp) { + mutex_unlock(&dev_priv->psr.lock); + return; + } + intel_psr_disable_locked(intel_dp); + + dev_priv->psr.dp = NULL; mutex_unlock(&dev_priv->psr.lock); cancel_work_sync(&dev_priv->psr.work); } @@ -756,13 +786,11 @@ int intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state) static bool __psr_wait_for_idle_locked(struct drm_i915_private *dev_priv) { - struct intel_dp *intel_dp; i915_reg_t reg; u32 mask; int err; - intel_dp = dev_priv->psr.enabled; - if (!intel_dp) + if (!dev_priv->psr.enabled) return false; if (dev_priv->psr.psr2_enabled) { @@ -784,6 +812,89 @@ static bool __psr_wait_for_idle_locked(struct drm_i915_private *dev_priv) return err == 0 && dev_priv->psr.enabled; } +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; + + ret = drm_modeset_lock(&dev->mode_config.connection_mutex, ctx); + if (ret) + return ret; + + enable = psr_global_enabled(mode); + + mutex_lock(&dev_priv->psr.lock); + + do { + if (!dev_priv->psr.dp) { + dev_priv->psr.debugfs_mode = mode; + 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) + intel_psr_disable_locked(enc_to_intel_dp(encoder)); + + dev_priv->psr.debugfs_mode = mode; + + if (enable) + intel_psr_enable_locked(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 = @@ -811,7 +922,8 @@ static void intel_psr_work(struct work_struct *work) if (dev_priv->psr.busy_frontbuffer_bits || dev_priv->psr.active) goto unlock; - intel_psr_activate(dev_priv->psr.enabled); + intel_psr_activate(dev_priv->psr.dp +); unlock: mutex_unlock(&dev_priv->psr.lock); } @@ -866,7 +978,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); @@ -909,7 +1021,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); @@ -971,6 +1083,8 @@ void intel_psr_init(struct drm_i915_private *dev_priv) /* For new platforms let's respect VBT back again */ dev_priv->psr.link_standby = dev_priv->vbt.psr.full_link; + dev_priv->psr.debugfs_mode = PSR_DEBUGFS_MODE_DEFAULT; + INIT_WORK(&dev_priv->psr.work, intel_psr_work); mutex_init(&dev_priv->psr.lock); } @@ -991,7 +1105,7 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp) mutex_lock(&psr->lock); - if (psr->enabled != intel_dp) + if (!psr->enabled || psr->dp != intel_dp) goto exit; if (drm_dp_dpcd_readb(&intel_dp->aux, DP_PSR_STATUS, &val) != 1) {
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. Changes since v2: - Rebase on top of intel_psr changes. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 75 ++++++++++++-- drivers/gpu/drm/i915/i915_drv.h | 9 +- drivers/gpu/drm/i915/intel_drv.h | 3 + drivers/gpu/drm/i915/intel_psr.c | 154 ++++++++++++++++++++++++---- 4 files changed, 214 insertions(+), 27 deletions(-)