Re: [PATCH v1] drm/i915: Skip modeset for cdclk changes if possible

["Kumar, Abhay" <abhay.kumar@xxxxxxxxx>]

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@xxxxxxxxxxxxxxx>

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@xxxxxxxxxxxxxxx>
Signed-off-by: Abhay Kumar <abhay.kumar@xxxxxxxxx>
---
 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] calling  1-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] calling  phy0+ @ 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] calling  1-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",

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