drm/i915: Don't overwrite (e)DP PLL selection on SKL
diff mbox

Message ID 1431686069-6717-1-git-send-email-ander.conselvan.de.oliveira@intel.com
State New
Headers show

Commit Message

Ander Conselvan de Oliveira May 15, 2015, 10:34 a.m. UTC
In the following commit, the place where the contents of dpll_hw_state
in crtc_state where zeroed was changed. Prior to that commit, it
happened when the new state was allocated, but now that happens just
before the call the .crtc_compute_clock() hook. The DP code for SKL,
however, sets up the (private) PLL in the encoder compute config
function that has already run by the time that memset() is reached,
causing the previous value to be lost.

This patch fixes the issue by moving the memset() down the call chain,
so that it is only called if the values in dpll_hw_state are going to be
updated.

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

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90462
---

On Fri, 2015-05-15 at 13:31 +0300, Ander Conselvan De Oliveira wrote:
> 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.

Like this completely untested patch.

---
 drivers/gpu/drm/i915/intel_ddi.c     | 9 +++++++++
 drivers/gpu/drm/i915/intel_display.c | 8 ++++++--
 drivers/gpu/drm/i915/intel_dp.c      | 3 +++
 3 files changed, 18 insertions(+), 2 deletions(-)

Comments

Jani Nikula May 15, 2015, 10:56 a.m. UTC | #1
On Fri, 15 May 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote:
> In the following commit, the place where the contents of dpll_hw_state
> in crtc_state where zeroed was changed. Prior to that commit, it
> happened when the new state was allocated, but now that happens just
> before the call the .crtc_compute_clock() hook. The DP code for SKL,
> however, sets up the (private) PLL in the encoder compute config
> function that has already run by the time that memset() is reached,
> causing the previous value to be lost.
>
> This patch fixes the issue by moving the memset() down the call chain,
> so that it is only called if the values in dpll_hw_state are going to be
> updated.
>
> 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
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90462
> ---
>
> On Fri, 2015-05-15 at 13:31 +0300, Ander Conselvan De Oliveira wrote:
>> 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.
>
> Like this completely untested patch.

Missing sob.

Jani.

>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c     | 9 +++++++++
>  drivers/gpu/drm/i915/intel_display.c | 8 ++++++--
>  drivers/gpu/drm/i915/intel_dp.c      | 3 +++
>  3 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index ec5d2ea..48d45b2 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1087,6 +1087,9 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
>  		      WRPLL_DIVIDER_REFERENCE(r2) | WRPLL_DIVIDER_FEEDBACK(n2) |
>  		      WRPLL_DIVIDER_POST(p);
>  
> +		memset(&crtc_state->dpll_hw_state, 0,
> +		       sizeof(crtc_state->dpll_hw_state));
> +
>  		crtc_state->dpll_hw_state.wrpll = val;
>  
>  		pll = intel_get_shared_dpll(intel_crtc, crtc_state);
> @@ -1309,6 +1312,9 @@ skl_ddi_pll_select(struct intel_crtc *intel_crtc,
>  	} else /* eDP */
>  		return true;
>  
> +	memset(&crtc_state->dpll_hw_state, 0,
> +	       sizeof(crtc_state->dpll_hw_state));
> +
>  	crtc_state->dpll_hw_state.ctrl1 = ctrl1;
>  	crtc_state->dpll_hw_state.cfgcr1 = cfgcr1;
>  	crtc_state->dpll_hw_state.cfgcr2 = cfgcr2;
> @@ -1419,6 +1425,9 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
>  		}
>  	}
>  
> +	memset(&crtc_state->dpll_hw_state, 0,
> +	       sizeof(crtc_state->dpll_hw_state));
> +
>  	crtc_state->dpll_hw_state.ebb0 =
>  		PORT_PLL_P1(clk_div.p1) | PORT_PLL_P2(clk_div.p2);
>  	crtc_state->dpll_hw_state.pll0 = clk_div.m2_int;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8d40d7d..a7732b4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7475,6 +7475,9 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
>  	struct drm_connector_state *connector_state;
>  	int i;
>  
> +	memset(&crtc_state->dpll_hw_state, 0,
> +	       sizeof(crtc_state->dpll_hw_state));
> +
>  	for_each_connector_in_state(state, connector, connector_state, i) {
>  		if (connector_state->crtc != &crtc->base)
>  			continue;
> @@ -8516,6 +8519,9 @@ static int ironlake_crtc_compute_clock(struct intel_crtc *crtc,
>  	bool is_lvds = false;
>  	struct intel_shared_dpll *pll;
>  
> +	memset(&crtc_state->dpll_hw_state, 0,
> +	       sizeof(crtc_state->dpll_hw_state));
> +
>  	is_lvds = intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS);
>  
>  	WARN(!(HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)),
> @@ -12265,8 +12271,6 @@ static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
>  		if (needs_modeset(crtc_state)) {
>  			clear_pipes |= 1 << intel_crtc->pipe;
>  			intel_crtc_state->shared_dpll = DPLL_ID_PRIVATE;
> -			memset(&intel_crtc_state->dpll_hw_state, 0,
> -			       sizeof(intel_crtc_state->dpll_hw_state));
>  		}
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 5bd73ad..6fd9c60 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1097,6 +1097,9 @@ skl_edp_set_pll_config(struct intel_crtc_state *pipe_config, int link_clock)
>  {
>  	u32 ctrl1;
>  
> +	memset(&pipe_config->dpll_hw_state, 0,
> +	       sizeof(pipe_config->dpll_hw_state));
> +
>  	pipe_config->ddi_pll_sel = SKL_DPLL0;
>  	pipe_config->dpll_hw_state.cfgcr1 = 0;
>  	pipe_config->dpll_hw_state.cfgcr2 = 0;
> -- 
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Tvrtko Ursulin May 15, 2015, 10:59 a.m. UTC | #2
Hi,

On 05/15/2015 11:34 AM, Ander Conselvan de Oliveira wrote:
> In the following commit, the place where the contents of dpll_hw_state
> in crtc_state where zeroed was changed. Prior to that commit, it
> happened when the new state was allocated, but now that happens just
> before the call the .crtc_compute_clock() hook. The DP code for SKL,
> however, sets up the (private) PLL in the encoder compute config
> function that has already run by the time that memset() is reached,
> causing the previous value to be lost.
>
> This patch fixes the issue by moving the memset() down the call chain,
> so that it is only called if the values in dpll_hw_state are going to be
> updated.
>
> 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
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90462
> ---

I gave it a quick spin - it does fix the warnings on my system.

Regards,

Tvrtko
Ander Conselvan de Oliveira May 15, 2015, 11:02 a.m. UTC | #3
On Fri, 2015-05-15 at 13:56 +0300, Jani Nikula wrote:
> On Fri, 15 May 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote:
> > In the following commit, the place where the contents of dpll_hw_state
> > in crtc_state where zeroed was changed. Prior to that commit, it
> > happened when the new state was allocated, but now that happens just
> > before the call the .crtc_compute_clock() hook. The DP code for SKL,
> > however, sets up the (private) PLL in the encoder compute config
> > function that has already run by the time that memset() is reached,
> > causing the previous value to be lost.
> >
> > This patch fixes the issue by moving the memset() down the call chain,
> > so that it is only called if the values in dpll_hw_state are going to be
> > updated.
> >
> > 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
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90462
> > ---
> >
> > On Fri, 2015-05-15 at 13:31 +0300, Ander Conselvan De Oliveira wrote:
> >> 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.
> >
> > Like this completely untested patch.
> 
> Missing sob.

Oops.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

> 
> Jani.
> 
> >
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c     | 9 +++++++++
> >  drivers/gpu/drm/i915/intel_display.c | 8 ++++++--
> >  drivers/gpu/drm/i915/intel_dp.c      | 3 +++
> >  3 files changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index ec5d2ea..48d45b2 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1087,6 +1087,9 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
> >  		      WRPLL_DIVIDER_REFERENCE(r2) | WRPLL_DIVIDER_FEEDBACK(n2) |
> >  		      WRPLL_DIVIDER_POST(p);
> >  
> > +		memset(&crtc_state->dpll_hw_state, 0,
> > +		       sizeof(crtc_state->dpll_hw_state));
> > +
> >  		crtc_state->dpll_hw_state.wrpll = val;
> >  
> >  		pll = intel_get_shared_dpll(intel_crtc, crtc_state);
> > @@ -1309,6 +1312,9 @@ skl_ddi_pll_select(struct intel_crtc *intel_crtc,
> >  	} else /* eDP */
> >  		return true;
> >  
> > +	memset(&crtc_state->dpll_hw_state, 0,
> > +	       sizeof(crtc_state->dpll_hw_state));
> > +
> >  	crtc_state->dpll_hw_state.ctrl1 = ctrl1;
> >  	crtc_state->dpll_hw_state.cfgcr1 = cfgcr1;
> >  	crtc_state->dpll_hw_state.cfgcr2 = cfgcr2;
> > @@ -1419,6 +1425,9 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
> >  		}
> >  	}
> >  
> > +	memset(&crtc_state->dpll_hw_state, 0,
> > +	       sizeof(crtc_state->dpll_hw_state));
> > +
> >  	crtc_state->dpll_hw_state.ebb0 =
> >  		PORT_PLL_P1(clk_div.p1) | PORT_PLL_P2(clk_div.p2);
> >  	crtc_state->dpll_hw_state.pll0 = clk_div.m2_int;
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 8d40d7d..a7732b4 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -7475,6 +7475,9 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
> >  	struct drm_connector_state *connector_state;
> >  	int i;
> >  
> > +	memset(&crtc_state->dpll_hw_state, 0,
> > +	       sizeof(crtc_state->dpll_hw_state));
> > +
> >  	for_each_connector_in_state(state, connector, connector_state, i) {
> >  		if (connector_state->crtc != &crtc->base)
> >  			continue;
> > @@ -8516,6 +8519,9 @@ static int ironlake_crtc_compute_clock(struct intel_crtc *crtc,
> >  	bool is_lvds = false;
> >  	struct intel_shared_dpll *pll;
> >  
> > +	memset(&crtc_state->dpll_hw_state, 0,
> > +	       sizeof(crtc_state->dpll_hw_state));
> > +
> >  	is_lvds = intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS);
> >  
> >  	WARN(!(HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)),
> > @@ -12265,8 +12271,6 @@ static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
> >  		if (needs_modeset(crtc_state)) {
> >  			clear_pipes |= 1 << intel_crtc->pipe;
> >  			intel_crtc_state->shared_dpll = DPLL_ID_PRIVATE;
> > -			memset(&intel_crtc_state->dpll_hw_state, 0,
> > -			       sizeof(intel_crtc_state->dpll_hw_state));
> >  		}
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 5bd73ad..6fd9c60 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1097,6 +1097,9 @@ skl_edp_set_pll_config(struct intel_crtc_state *pipe_config, int link_clock)
> >  {
> >  	u32 ctrl1;
> >  
> > +	memset(&pipe_config->dpll_hw_state, 0,
> > +	       sizeof(pipe_config->dpll_hw_state));
> > +
> >  	pipe_config->ddi_pll_sel = SKL_DPLL0;
> >  	pipe_config->dpll_hw_state.cfgcr1 = 0;
> >  	pipe_config->dpll_hw_state.cfgcr2 = 0;
> > -- 
> > 2.1.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
Lespiau, Damien May 15, 2015, 11:19 a.m. UTC | #4
On Fri, May 15, 2015 at 01:34:29PM +0300, Ander Conselvan de Oliveira wrote:
> In the following commit, the place where the contents of dpll_hw_state
> in crtc_state where zeroed was changed. Prior to that commit, it
> happened when the new state was allocated, but now that happens just
> before the call the .crtc_compute_clock() hook. The DP code for SKL,
> however, sets up the (private) PLL in the encoder compute config
> function that has already run by the time that memset() is reached,
> causing the previous value to be lost.
> 
> This patch fixes the issue by moving the memset() down the call chain,
> so that it is only called if the values in dpll_hw_state are going to be
> updated.
> 
> 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
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90462

Looks good to me:

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
Shuang He May 18, 2015, 8 a.m. UTC | #5
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6417
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  302/302              302/302
SNB                 -1              314/314              313/314
IVB                                  338/338              338/338
BYT                                  286/286              286/286
BDW                                  320/320              320/320
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 SNB  igt@pm_rpm@dpms-mode-unset-non-lpsp      DMESG_WARN(16)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 '*'
Daniel Vetter May 18, 2015, 8:04 a.m. UTC | #6
On Fri, May 15, 2015 at 12:19:12PM +0100, Damien Lespiau wrote:
> On Fri, May 15, 2015 at 01:34:29PM +0300, Ander Conselvan de Oliveira wrote:
> > In the following commit, the place where the contents of dpll_hw_state
> > in crtc_state where zeroed was changed. Prior to that commit, it
> > happened when the new state was allocated, but now that happens just
> > before the call the .crtc_compute_clock() hook. The DP code for SKL,
> > however, sets up the (private) PLL in the encoder compute config
> > function that has already run by the time that memset() is reached,
> > causing the previous value to be lost.
> > 
> > This patch fixes the issue by moving the memset() down the call chain,
> > so that it is only called if the values in dpll_hw_state are going to be
> > updated.
> > 
> > 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
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90462
> 
> Looks good to me:
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

Queued for -next, thanks for the patch.
-Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index ec5d2ea..48d45b2 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1087,6 +1087,9 @@  hsw_ddi_pll_select(struct intel_crtc *intel_crtc,
 		      WRPLL_DIVIDER_REFERENCE(r2) | WRPLL_DIVIDER_FEEDBACK(n2) |
 		      WRPLL_DIVIDER_POST(p);
 
+		memset(&crtc_state->dpll_hw_state, 0,
+		       sizeof(crtc_state->dpll_hw_state));
+
 		crtc_state->dpll_hw_state.wrpll = val;
 
 		pll = intel_get_shared_dpll(intel_crtc, crtc_state);
@@ -1309,6 +1312,9 @@  skl_ddi_pll_select(struct intel_crtc *intel_crtc,
 	} else /* eDP */
 		return true;
 
+	memset(&crtc_state->dpll_hw_state, 0,
+	       sizeof(crtc_state->dpll_hw_state));
+
 	crtc_state->dpll_hw_state.ctrl1 = ctrl1;
 	crtc_state->dpll_hw_state.cfgcr1 = cfgcr1;
 	crtc_state->dpll_hw_state.cfgcr2 = cfgcr2;
@@ -1419,6 +1425,9 @@  bxt_ddi_pll_select(struct intel_crtc *intel_crtc,
 		}
 	}
 
+	memset(&crtc_state->dpll_hw_state, 0,
+	       sizeof(crtc_state->dpll_hw_state));
+
 	crtc_state->dpll_hw_state.ebb0 =
 		PORT_PLL_P1(clk_div.p1) | PORT_PLL_P2(clk_div.p2);
 	crtc_state->dpll_hw_state.pll0 = clk_div.m2_int;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8d40d7d..a7732b4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7475,6 +7475,9 @@  static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
 	struct drm_connector_state *connector_state;
 	int i;
 
+	memset(&crtc_state->dpll_hw_state, 0,
+	       sizeof(crtc_state->dpll_hw_state));
+
 	for_each_connector_in_state(state, connector, connector_state, i) {
 		if (connector_state->crtc != &crtc->base)
 			continue;
@@ -8516,6 +8519,9 @@  static int ironlake_crtc_compute_clock(struct intel_crtc *crtc,
 	bool is_lvds = false;
 	struct intel_shared_dpll *pll;
 
+	memset(&crtc_state->dpll_hw_state, 0,
+	       sizeof(crtc_state->dpll_hw_state));
+
 	is_lvds = intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS);
 
 	WARN(!(HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)),
@@ -12265,8 +12271,6 @@  static int __intel_set_mode_setup_plls(struct drm_atomic_state *state)
 		if (needs_modeset(crtc_state)) {
 			clear_pipes |= 1 << intel_crtc->pipe;
 			intel_crtc_state->shared_dpll = DPLL_ID_PRIVATE;
-			memset(&intel_crtc_state->dpll_hw_state, 0,
-			       sizeof(intel_crtc_state->dpll_hw_state));
 		}
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5bd73ad..6fd9c60 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1097,6 +1097,9 @@  skl_edp_set_pll_config(struct intel_crtc_state *pipe_config, int link_clock)
 {
 	u32 ctrl1;
 
+	memset(&pipe_config->dpll_hw_state, 0,
+	       sizeof(pipe_config->dpll_hw_state));
+
 	pipe_config->ddi_pll_sel = SKL_DPLL0;
 	pipe_config->dpll_hw_state.cfgcr1 = 0;
 	pipe_config->dpll_hw_state.cfgcr2 = 0;