| Message ID | 1431535244-8311-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 13, 2015 at 05:40:44PM +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Since commit 4978cc93d9ac240b435ce60431aef24239b4c270 started clearing > dpll state and recomputing it via crtc_compute_clock (and probably some > other commit which triggered pipe config checking), modesetting is now > constantly triggering warnings about dpll_hw_state.ctrl1 mismatch. > > Reason is crtc_compute_clock calls skl_ddi_pll_select which does not do > anything for eDP, leaving the ctrl1 state at the default of zero. > > This potentially hacky fix makes skl_ddi_pll_select call > skl_edp_set_pll_config which fixes the problem for me. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Damien Lespiau <damien.lespiau@intel.com> > Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Nop! (at least I really don't think so). As discussed on IRC, on DDI platforms, the (e)DP compute_config() does the private DPLL selection, and the ddi_pll_select() does the the same for shared DPLLs (I'm not saying that the end result we want, just how it works today). That split comes from the introduction of shared_dpll in the DDI PLL selection last summer. Anyway, this means that, for SKL, dpll_hw_state is touched by the encoder's compute_config() for (e)DP. I think we could just remove the memset() in 4978cc93 (maybe?), or try to unify a bit better and only have one place where we do PLL selection (which I assume is part of the bigger atomic plan). Not skl_edp_set_pll_config() in both compute_config() and ddi_pll_select() though.
On Wed, May 13, 2015 at 05:56:17PM +0100, Damien Lespiau wrote: > On Wed, May 13, 2015 at 05:40:44PM +0100, Tvrtko Ursulin wrote: > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > Since commit 4978cc93d9ac240b435ce60431aef24239b4c270 started clearing > > dpll state and recomputing it via crtc_compute_clock (and probably some > > other commit which triggered pipe config checking), modesetting is now > > constantly triggering warnings about dpll_hw_state.ctrl1 mismatch. > > > > Reason is crtc_compute_clock calls skl_ddi_pll_select which does not do > > anything for eDP, leaving the ctrl1 state at the default of zero. > > > > This potentially hacky fix makes skl_ddi_pll_select call > > skl_edp_set_pll_config which fixes the problem for me. > > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Cc: Damien Lespiau <damien.lespiau@intel.com> > > Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Nop! (at least I really don't think so). > > As discussed on IRC, on DDI platforms, the (e)DP compute_config() does > the private DPLL selection, and the ddi_pll_select() does the the same > for shared DPLLs (I'm not saying that the end result we want, just how > it works today). That split comes from the introduction of shared_dpll > in the DDI PLL selection last summer. > > Anyway, this means that, for SKL, dpll_hw_state is touched by the > encoder's compute_config() for (e)DP. > > I think we could just remove the memset() in 4978cc93 (maybe?), or try > to unify a bit better and only have one place where we do PLL selection > (which I assume is part of the bigger atomic plan). Not > skl_edp_set_pll_config() in both compute_config() and ddi_pll_select() > though. To be more precise: commit 4978cc93d9ac240b435ce60431aef24239b4c270 Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> Date: Tue Apr 21 17:13:21 2015 +0300 drm/i915: Preserve shared DPLL information in new pipe_config When a new pipe_config is calculated, the fields related to shared dplls are reset, under the assumption that they will be recalculated as part of the modeset, which is true with the current state of the code. As we convert to atomic, however, it will be possible to calculate a new pipe_config and skip the modeset. In that case, after the state swap we still want the shared dplls to be preserved. Except that dpll_hw_state is not just for shared DPLLs. So maybe dpll_hw_state shouldn't be preserved (in clear_intel_crtc_state() in the case where shared_dpll is DPLL_ID_PRIVATE? Note that ddi_pll_sel is also a field set by the DDI PLL selection code you may want to preserve in clear_intel_crtc_state().
On Wed, 2015-05-13 at 18:25 +0100, Damien Lespiau wrote: > On Wed, May 13, 2015 at 05:56:17PM +0100, Damien Lespiau wrote: > > On Wed, May 13, 2015 at 05:40:44PM +0100, Tvrtko Ursulin wrote: > > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > > > Since commit 4978cc93d9ac240b435ce60431aef24239b4c270 started clearing > > > dpll state and recomputing it via crtc_compute_clock (and probably some > > > other commit which triggered pipe config checking), modesetting is now > > > constantly triggering warnings about dpll_hw_state.ctrl1 mismatch. > > > > > > Reason is crtc_compute_clock calls skl_ddi_pll_select which does not do > > > anything for eDP, leaving the ctrl1 state at the default of zero. > > > > > > This potentially hacky fix makes skl_ddi_pll_select call > > > skl_edp_set_pll_config which fixes the problem for me. > > > > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > Cc: Damien Lespiau <damien.lespiau@intel.com> > > > Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Nop! (at least I really don't think so). > > > > As discussed on IRC, on DDI platforms, the (e)DP compute_config() does > > the private DPLL selection, and the ddi_pll_select() does the the same > > for shared DPLLs (I'm not saying that the end result we want, just how > > it works today). That split comes from the introduction of shared_dpll > > in the DDI PLL selection last summer. > > > > Anyway, this means that, for SKL, dpll_hw_state is touched by the > > encoder's compute_config() for (e)DP. > > > > I think we could just remove the memset() in 4978cc93 (maybe?), or try > > to unify a bit better and only have one place where we do PLL selection > > (which I assume is part of the bigger atomic plan). Not > > skl_edp_set_pll_config() in both compute_config() and ddi_pll_select() > > though. > > To be more precise: > > commit 4978cc93d9ac240b435ce60431aef24239b4c270 > Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > Date: Tue Apr 21 17:13:21 2015 +0300 > > drm/i915: Preserve shared DPLL information in new pipe_config > > When a new pipe_config is calculated, the fields related to shared dplls > are reset, under the assumption that they will be recalculated as part > of the modeset, which is true with the current state of the code. > > As we convert to atomic, however, it will be possible to calculate a new > pipe_config and skip the modeset. In that case, after the state swap we > still want the shared dplls to be preserved. > > Except that dpll_hw_state is not just for shared DPLLs. So maybe dpll_hw_state > shouldn't be preserved (in clear_intel_crtc_state() in the case where > shared_dpll is DPLL_ID_PRIVATE? The idea here is that when the modeset code does a flip only, we are still doing a state swap. Before the swap was in place, we simply wouldn't change the pipe_config, so the value for dpll_hw_state wouldn't change either. If there is an actual modeset these values are recalculated, so preserving the state should be harmless. > Note that ddi_pll_sel is also a field set by the DDI PLL selection code you may > want to preserve in clear_intel_crtc_state(). Yeah, I missed that one. We *definitely* want to preserve it. I'll send a patch for doing that shortly. Thanks, Ander
On Wed, 2015-05-13 at 17:56 +0100, Damien Lespiau wrote: > On Wed, May 13, 2015 at 05:40:44PM +0100, Tvrtko Ursulin wrote: > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > Since commit 4978cc93d9ac240b435ce60431aef24239b4c270 started clearing > > dpll state and recomputing it via crtc_compute_clock (and probably some > > other commit which triggered pipe config checking), modesetting is now > > constantly triggering warnings about dpll_hw_state.ctrl1 mismatch. > > > > Reason is crtc_compute_clock calls skl_ddi_pll_select which does not do > > anything for eDP, leaving the ctrl1 state at the default of zero. > > > > This potentially hacky fix makes skl_ddi_pll_select call > > skl_edp_set_pll_config which fixes the problem for me. > > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Cc: Damien Lespiau <damien.lespiau@intel.com> > > Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Nop! (at least I really don't think so). > > As discussed on IRC, on DDI platforms, the (e)DP compute_config() does > the private DPLL selection, and the ddi_pll_select() does the the same > for shared DPLLs (I'm not saying that the end result we want, just how > it works today). That split comes from the introduction of shared_dpll > in the DDI PLL selection last summer. > > Anyway, this means that, for SKL, dpll_hw_state is touched by the > encoder's compute_config() for (e)DP. > > I think we could just remove the memset() in 4978cc93 (maybe?), or try > to unify a bit better and only have one place where we do PLL selection > (which I assume is part of the bigger atomic plan). Not > skl_edp_set_pll_config() in both compute_config() and ddi_pll_select() > though. The memset() was added to fix a similar warning on a different platform, but I can't remember which one now. Perhaps a more immediate fix would be to move the memset() down the call chain. Long term I think we should remove all the assumptions of initial values from the compute config path. Having only one place that selects PLL would also be nice IMO. Ander
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6410
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV 234/234 234/234
ILK 262/262 262/262
SNB -1 282/282 281/282
IVB 300/300 300/300
BYT 254/254 254/254
BDW 275/275 275/275
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
SNB igt@pm_rpm@dpms-mode-unset-non-lpsp DMESG_WARN(4)PASS(1) DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_uncore.c:#assert_device_not_suspended[i915]()@WARNING:.* at .* assert_device_not_suspended+0x
Note: You need to pay more attention to line start with '*'
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 807e15d..e5b7723 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1306,8 +1306,10 @@ skl_ddi_pll_select(struct intel_crtc *intel_crtc, } cfgcr1 = cfgcr2 = 0; - } else /* eDP */ + } else /* eDP */ { + skl_edp_set_pll_config(crtc_state, clock); return true; + } crtc_state->dpll_hw_state.ctrl1 = ctrl1; crtc_state->dpll_hw_state.cfgcr1 = cfgcr1; diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 75bccd6..13b5b0e 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1092,7 +1092,7 @@ intel_dp_connector_unregister(struct intel_connector *intel_connector) intel_connector_unregister(intel_connector); } -static void +void skl_edp_set_pll_config(struct intel_crtc_state *pipe_config, int link_clock) { u32 ctrl1; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index aa77af7..6e26644 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1193,6 +1193,9 @@ void intel_edp_drrs_invalidate(struct drm_device *dev, unsigned frontbuffer_bits); void intel_edp_drrs_flush(struct drm_device *dev, unsigned frontbuffer_bits); +void +skl_edp_set_pll_config(struct intel_crtc_state *pipe_config, int link_clock); + /* intel_dp_mst.c */ int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id); void intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port);