[Intel-gfx] [PATCH v1] drm/i915: Skip modeset for cdclk changes if possible

Wed Aug 29 00:55:06 UTC 2018

On 8/28/2018 5:39 AM, Ville Syrjälä wrote:
> On Mon, Aug 27, 2018 at 11:50:32AM -0700, Abhay Kumar wrote:
>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>
>> If we have only a single active pipe and the cdclk change only requires
>> the cd2x divider to be updated bxt+ can do the update with forcing a full
>> modeset on the pipe. Try to hook that up.
>>
>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> Signed-off-by: Abhay Kumar <abhay.kumar at intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h      |   3 +-
>>   drivers/gpu/drm/i915/i915_reg.h      |   3 +-
>>   drivers/gpu/drm/i915/intel_cdclk.c   | 105 +++++++++++++++++++++++++----------
>>   drivers/gpu/drm/i915/intel_display.c |  20 ++++++-
>>   drivers/gpu/drm/i915/intel_drv.h     |   9 ++-
>>   5 files changed, 105 insertions(+), 35 deletions(-)
>>
> <snip>
>> @@ -12252,12 +12253,24 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>>   				return ret;
>>   		}
>>   
>> +
>>   		/* All pipes must be switched off while we change the cdclk. */
>> -		if (intel_cdclk_needs_modeset(&dev_priv->cdclk.actual,
>> -					      &intel_state->cdclk.actual)) {
>> +		if (is_power_of_2(intel_state->active_crtcs) &&
>> +		    intel_cdclk_needs_cd2x_update(dev_priv,
>> +						  &dev_priv->cdclk.actual,
>> +						  &intel_state->cdclk.actual)) {
>> +			ret = intel_lock_all_pipes(state);
>> +			if (ret < 0)
>> +				return ret;
>> +
>> +			intel_state->cdclk.pipe = ilog2(intel_state->active_crtcs);
> BTW on further reflection this probably isn't quite sufficient. Let's
> say we have a commit with allow_modeset=true, but we aren't actually
> required to do a modeset based on any of the state changes. If we still
> have to change cdclk we should actually be doing the cd2x update
> atomically with the plane updates, or we should do it before or after
> the plane updates depending on whether the cdclk freq is going up or
> down.
>
> Doing the update atomically with the plane updates might be nicer in the
> end, but for that we would likely need to split the .set_cdclk() hooks
> into three parts (pre+commit+post). Whereas just doing the update before
> or after the plane updates as needed would probably be a little simpler.
Yeah. That might also get rid of cdclk mismatch warning during multiple 
suspend resume cycle.

[280.600259] cdclk state doesn't match!

[280.600270] calling1-8+ @ 3110, parent: usb1, cb: usb_dev_resume

[280.600276] WARNING: CPU: 3 PID: 5224 at 
/mnt/host/source/src/third_party/ker

nel/v4.14/drivers/gpu/drm/i915/intel_cdclk.c:1867 intel_set_cdclk+0xaa/0xdb

[280.600277] Modules linked in: cmac rfcomm uinput 
snd_soc_sst_bxt_da7219_max9

8357a snd_soc_hdac_hdmi snd_soc_dmic lzo lzo_compress snd_soc_skl 
snd_soc_skl_ip

c snd_soc_sst_ipc snd_soc_sst_dsp snd_soc_acpi snd_hda_ext_core snd_hda_core

[280.600307] callingphy0+ @ 3102, parent: 0000:00:0c.0, cb: wiphy_resume [cf

g80211]

[280.600308]zram snd_soc_max98357a acpi_als snd_soc_da7219 bridge stp llc ip

t_MASQUERADE nf_nat_masquerade_ipv4 xt_mark fuse snd_seq_dummy snd_seq 
snd_seq_d

evice btusb btrtl btbcm iio_trig_sysfs btintel uvcvideo bluetooth 
videobuf2_vmal

loc videobuf2_memops videobuf2_v4l2 ecdh_generic videobuf2_core 
cros_ec_sensors

cros_ec_sensors_ring cros_ec_sensors_core industrialio_triggered_buffer 
kfifo_bu

[280.600346] RDX: ffffffffb8258dd0 RSI: 0000000000000002 RDI: 
ffffffffb8258db0

[280.600347] RBP: ffffbb9546b73aa0 R08: 0000000000000000 R09: 
0000000000000000

[280.600348] R10: 0000000000000000 R11: ffffffffb86d8518 R12: 
ffffa22075790000

[280.600349] R13: ffffa22075793d24 R14: 00000000ffffffff R15: 
ffffa2202eec9800

00000000000

[280.600352] CS:0010 DS: 0000 ES: 0000 CR0: 0000000080050033

[280.600353] CR2: 00007f43c04d0e50 CR3: 0000000224112000 CR4: 
00000000003406e0

[280.600354] Call Trace:

[280.600361]intel_atomic_commit_tail+0x20a/0xacb

[280.600363]? intel_atomic_commit_ready+0x44/0x4c

[280.600365]intel_atomic_commit+0x227/0x238

[280.600368]glk_force_audio_cdclk+0x9f/0x119

[280.600370]i915_audio_component_get_power+0x3e/0x4d

[280.600376]snd_hdac_display_power+0x53/0x97 [snd_hda_core]

[280.600379] calling1-9+ @ 3086, parent: usb1, cb: usb_dev_resume

[280.600384]skl_resume+0x3a/0x17a [snd_soc_skl]

[280.600387]? pci_pm_suspend_noirq+0x1e9/0x1e9

[280.600391]dpm_run_callback+0x59/0xbf

[280.600394]device_resume+0x192/0x1d4

[280.600396]dpm_resume+0x145/0x1da

[280.600398]dpm_resume_end+0x11/0x1a

[280.600403]suspend_devices_and_enter+0x354/0x5c2

[280.600407]? remove_wait_queue+0x51/0x51

[280.600409]pm_suspend+0x29c/0x2e2

[280.600411]state_store+0xa2/0xcb

[280.600415]kernfs_fop_write+0x103/0x14a

[280.600420]__vfs_write+0x37/0xd0

[280.600424]? inode_security+0x19/0x20

[280.600426]? selinux_file_permission+0x78/0xad

[280.600428]vfs_write+0xb9/0xfd

[280.600430]SyS_write+0x5f/0xa3

[280.600434]do_syscall_64+0x64/0x72

[280.600438]entry_SYSCALL_64_after_hwframe+0x3d/0xa2

[280.600441] RIP: 0033:0x7f969d8af3b0

00001

[280.600443] RAX: ffffffffffffffda RBX: 00007f969ddf0000 RCX: 
00007f969d8af3b0

[280.600444] RDX: 0000000000000006 RSI: 00007f969ddf0000 RDI: 
0000000000000001

[280.600445] RBP: 00007ffd9e3f4880 R08: ffffffffffffffff R09: 
0000000000000000

[280.600446] R10: 0000000000001000 R11: 0000000000000246 R12: 
0000000000000006

[280.600447] R13: 0000000000020000 R14: 00007f969ddf0000 R15: 
0000000000000001

>
>
>> +		} else if (intel_cdclk_needs_modeset(&dev_priv->cdclk.actual,
>> +						     &intel_state->cdclk.actual)) {
>>   			ret = intel_modeset_all_pipes(state);
>>   			if (ret < 0)
>>   				return ret;
>> +
>> +			intel_state->cdclk.pipe = INVALID_PIPE;
>>   		}
>>   
>>   		DRM_DEBUG_KMS("New cdclk calculated to be logical %u kHz, actual %u kHz\n",

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20180828/c692ab9c/attachment-0001.html>