drm/i915/skl: Handle eDP from generic crtc_compute_clock vfunc
diff mbox

Message ID 1431535244-8311-1-git-send-email-tvrtko.ursulin@linux.intel.com
State New
Headers show

Commit Message

Tvrtko Ursulin May 13, 2015, 4:40 p.m. UTC
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>
---

Big CC list so no one misses the chance of saying how this is not the
right fix. :) But that is OK, I was just annoyed by the constant stream
of warnings obscuring real problems.
---
 drivers/gpu/drm/i915/intel_ddi.c | 4 +++-
 drivers/gpu/drm/i915/intel_dp.c  | 2 +-
 drivers/gpu/drm/i915/intel_drv.h | 3 +++
 3 files changed, 7 insertions(+), 2 deletions(-)

Comments

Lespiau, Damien May 13, 2015, 4:56 p.m. UTC | #1
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.
Lespiau, Damien May 13, 2015, 5:25 p.m. UTC | #2
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().
Ander Conselvan de Oliveira May 15, 2015, 8:46 a.m. UTC | #3
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
Ander Conselvan de Oliveira May 15, 2015, 10:31 a.m. UTC | #4
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
Shuang He May 15, 2015, 2:44 p.m. UTC | #5
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 '*'

Patch
diff mbox

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);