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@xxxxxxxxxxxxxxx>
>> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
>> ---
>> 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) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
