Re: [Intel-gfx] [PATCH v1] drm/i915: Skip modeset for cdclk changes if possible
+Susanta From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of Kumar, Abhay Sent: Tuesday, August 28, 2018 5:55 PM To: Ville Syrjälä Cc: intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH v1] drm/i915: Skip modeset for cdclk changes if possible 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ä <mailto:ville.syrj...@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ä <mailto:ville.syrj...@linux.intel.com> Signed-off-by: Abhay Kumar <mailto:abhay.ku...@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(-) @@ -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(_priv->cdclk.actual, - _state->cdclk.actual)) { + if (is_power_of_2(intel_state->active_crtcs) && + intel_cdclk_needs_cd2x_update(dev_priv, + _priv->cdclk.actual, + _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: :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: b8258dd0 RSI: 0002 RDI: b8258db0 [ 280.600347] RBP: bb9546b73aa0 R08: R09: [ 280.600348] R10: R11: b86d8518 R12: a2207579 [ 280.600349] R13: a22075793d24 R14: R15: a2202eec9800 000 [ 280.600352] CS: 0010 DS: ES: CR0: 80050033 [ 280.600353] CR2: 7f43c04d0e50 CR3: 000224112000 CR4: 003406e0 [ 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 [ 28
Re: [Intel-gfx] [PATCH v1] drm/i915: Skip modeset for cdclk changes if possible
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ä 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ä Signed-off-by: Abhay Kumar --- 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(-) @@ -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(_priv->cdclk.actual, - _state->cdclk.actual)) { + if (is_power_of_2(intel_state->active_crtcs) && + intel_cdclk_needs_cd2x_update(dev_priv, + _priv->cdclk.actual, + _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: :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: b8258dd0 RSI: 0002 RDI: b8258db0 [280.600347] RBP: bb9546b73aa0 R08: R09: [280.600348] R10: R11: b86d8518 R12: a2207579 [280.600349] R13: a22075793d24 R14: R15: a2202eec9800 000 [280.600352] CS:0010 DS: ES: CR0: 80050033 [280.600353] CR2: 7f43c04d0e50 CR3: 000224112000 CR4: 003406e0 [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
Re: [Intel-gfx] [PATCH v1] drm/i915: Skip modeset for cdclk changes if possible
On Mon, Aug 27, 2018 at 11:50:32AM -0700, Abhay Kumar wrote: > From: Ville Syrjälä > > 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ä > Signed-off-by: Abhay Kumar > --- > 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(-) > > @@ -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(_priv->cdclk.actual, > - _state->cdclk.actual)) { > + if (is_power_of_2(intel_state->active_crtcs) && > + intel_cdclk_needs_cd2x_update(dev_priv, > + _priv->cdclk.actual, > + _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. > + } else if (intel_cdclk_needs_modeset(_priv->cdclk.actual, > + > _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", -- Ville Syrjälä Intel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v1] drm/i915: Skip modeset for cdclk changes if possible
From: Ville Syrjälä 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ä Signed-off-by: Abhay Kumar --- 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(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index e5b9d3c77139..1f0a6427e76c 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -408,7 +408,8 @@ struct drm_i915_display_funcs { void (*get_cdclk)(struct drm_i915_private *dev_priv, struct intel_cdclk_state *cdclk_state); void (*set_cdclk)(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state); + const struct intel_cdclk_state *cdclk_state, + enum pipe pipe); int (*get_fifo_size)(struct drm_i915_private *dev_priv, enum i9xx_plane_id i9xx_plane); int (*compute_pipe_wm)(struct intel_crtc_state *cstate); diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 8534f88a60f6..7702cec70b0d 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -9262,7 +9262,8 @@ enum skl_power_gate { #define BXT_CDCLK_CD2X_PIPE(pipe) ((pipe) << 20) #define CDCLK_DIVMUX_CD_OVERRIDE (1 << 19) #define BXT_CDCLK_CD2X_PIPE_NONE BXT_CDCLK_CD2X_PIPE(3) -#define ICL_CDCLK_CD2X_PIPE_NONE (7 << 19) +#define ICL_CDCLK_CD2X_PIPE(pipe) ((pipe) << 19) +#define ICL_CDCLK_CD2X_PIPE_NONE ICL_CDCLK_CD2X_PIPE(7) #define BXT_CDCLK_SSA_PRECHARGE_ENABLE(1 << 16) #define CDCLK_FREQ_DECIMAL_MASK (0x7ff) diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c index 29075c763428..1955f6aa54e1 100644 --- a/drivers/gpu/drm/i915/intel_cdclk.c +++ b/drivers/gpu/drm/i915/intel_cdclk.c @@ -516,7 +516,8 @@ static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv) } static void vlv_set_cdclk(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state) + const struct intel_cdclk_state *cdclk_state, + enum pipe pipe) { int cdclk = cdclk_state->cdclk; u32 val, cmd = cdclk_state->voltage_level; @@ -597,7 +598,8 @@ static void vlv_set_cdclk(struct drm_i915_private *dev_priv, } static void chv_set_cdclk(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state) + const struct intel_cdclk_state *cdclk_state, + enum pipe pipe) { int cdclk = cdclk_state->cdclk; u32 val, cmd = cdclk_state->voltage_level; @@ -695,7 +697,8 @@ static void bdw_get_cdclk(struct drm_i915_private *dev_priv, } static void bdw_set_cdclk(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state) + const struct intel_cdclk_state *cdclk_state, + enum pipe pipe) { int cdclk = cdclk_state->cdclk; uint32_t val; @@ -985,7 +988,8 @@ static void skl_dpll0_disable(struct drm_i915_private *dev_priv) } static void skl_set_cdclk(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state) + const struct intel_cdclk_state *cdclk_state, + enum pipe pipe) { int cdclk = cdclk_state->cdclk; int vco = cdclk_state->vco; @@ -1156,7 +1160,7 @@ void skl_init_cdclk(struct drm_i915_private *dev_priv) cdclk_state.cdclk = skl_calc_cdclk(0, cdclk_state.vco); cdclk_state.voltage_level = skl_calc_voltage_level(cdclk_state.cdclk); - skl_set_cdclk(dev_priv, _state); + skl_set_cdclk(dev_priv, _state, INVALID_PIPE); } /** @@ -1174,7 +1178,7 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv) cdclk_state.vco = 0; cdclk_state.voltage_level = skl_calc_voltage_level(cdclk_state.cdclk); - skl_set_cdclk(dev_priv, _state); + skl_set_cdclk(dev_priv, _state, INVALID_PIPE); } static int bxt_calc_cdclk(int min_cdclk) @@ -1353,7 +1357,8 @@ static void bxt_de_pll_enable(struct drm_i915_private *dev_priv, int vco) } static void bxt_set_cdclk(struct drm_i915_private *dev_priv, - const struct intel_cdclk_state *cdclk_state) + const struct intel_cdclk_state *cdclk_state, + enum pipe pipe) {