Re: [PATCH] drm/i915: Allow control of PSR at runtime through debugfs, v3.

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx