| Message ID | 1435669838-24747-1-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 30 Jun 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote: > Similarly to what is done for SKL, clear the dpll_hw_state of the pipe > config in hsw_dp_set_ddi_pll_sel(), since it main contain stale values. > That can happen if a crtc that was previously driving an HDMI connector > switches to a DP connector. In that case, the wrpll field was left with > its old value, leading to warnings like the one below: > > [drm:check_crtc_state [i915]] *ERROR* mismatch in dpll_hw_state.wrpll (expected 0xb035061f, found 0x00000000) > ------------[ cut here ]------------ > WARNING: CPU: 1 PID: 767 at drivers/gpu/drm/i915/intel_display.c:12324 check_crtc_state+0x975/0x10b0 [i915]() > pipe state doesn't match! > > This regression was indroduced in > > commit dd3cd74acf12723045a64f1f2c6298ac7b34a5d5 > Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > Date: Fri May 15 13:34:29 2015 +0300 > > drm/i915: Don't overwrite (e)DP PLL selection on SKL > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> Reported-by: Linus Torvalds <torvalds@linux-foundation.org> Tested-by: Jani Nikula <jani.nikula@intel.com> > --- > > Only compile tested, for the wrpll warning. > > Thanks, > Ander > > drivers/gpu/drm/i915/intel_dp.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 4ebfc3a..fbd9ac3 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1147,6 +1147,9 @@ skl_edp_set_pll_config(struct intel_crtc_state *pipe_config, int link_clock) > static void > hsw_dp_set_ddi_pll_sel(struct intel_crtc_state *pipe_config, int link_bw) > { > + memset(&pipe_config->dpll_hw_state, 0, > + sizeof(pipe_config->dpll_hw_state)); > + > switch (link_bw) { > case DP_LINK_BW_1_62: > pipe_config->ddi_pll_sel = PORT_CLK_SEL_LCPLL_810; > -- > 2.1.0 >
On Tue, Jun 30, 2015 at 04:47:06PM +0300, Jani Nikula wrote: > On Tue, 30 Jun 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote: > > Similarly to what is done for SKL, clear the dpll_hw_state of the pipe > > config in hsw_dp_set_ddi_pll_sel(), since it main contain stale values. > > That can happen if a crtc that was previously driving an HDMI connector > > switches to a DP connector. In that case, the wrpll field was left with > > its old value, leading to warnings like the one below: > > > > [drm:check_crtc_state [i915]] *ERROR* mismatch in dpll_hw_state.wrpll (expected 0xb035061f, found 0x00000000) > > ------------[ cut here ]------------ > > WARNING: CPU: 1 PID: 767 at drivers/gpu/drm/i915/intel_display.c:12324 check_crtc_state+0x975/0x10b0 [i915]() > > pipe state doesn't match! > > > > This regression was indroduced in > > > > commit dd3cd74acf12723045a64f1f2c6298ac7b34a5d5 > > Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > > Date: Fri May 15 13:34:29 2015 +0300 > > > > drm/i915: Don't overwrite (e)DP PLL selection on SKL > > > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > > Reported-by: Linus Torvalds <torvalds@linux-foundation.org> > Tested-by: Jani Nikula <jani.nikula@intel.com> Yeah makes sense as a fix for 4.2. But for 4.3 I wonder whether the original commit that started this chain needs to be changed a bit: 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 All the trouble this caused is because it not only preserves the sharing config (in crtc_state->shared_dpll) but also the ->dpll_hw_state. And I think with Maarten's latest code (for 4.3) we'd just do an unconditional compute_config (need it for fast pfit updates and fastboot), which means the bogus values in ->dpll_hw_state aren't a problem any more since we'll overwrite them again. And then we could remove that sprinkle of memsets we have all over, which would be good (since the current approach is obviously a bit fragile). Anyway: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Thanks, Daniel > > > --- > > > > Only compile tested, for the wrpll warning. > > > > Thanks, > > Ander > > > > drivers/gpu/drm/i915/intel_dp.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 4ebfc3a..fbd9ac3 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1147,6 +1147,9 @@ skl_edp_set_pll_config(struct intel_crtc_state *pipe_config, int link_clock) > > static void > > hsw_dp_set_ddi_pll_sel(struct intel_crtc_state *pipe_config, int link_bw) > > { > > + memset(&pipe_config->dpll_hw_state, 0, > > + sizeof(pipe_config->dpll_hw_state)); > > + > > switch (link_bw) { > > case DP_LINK_BW_1_62: > > pipe_config->ddi_pll_sel = PORT_CLK_SEL_LCPLL_810; > > -- > > 2.1.0 > > > > -- > Jani Nikula, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 30 Jun 2015, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Jun 30, 2015 at 04:47:06PM +0300, Jani Nikula wrote: >> On Tue, 30 Jun 2015, Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> wrote: >> > Similarly to what is done for SKL, clear the dpll_hw_state of the pipe >> > config in hsw_dp_set_ddi_pll_sel(), since it main contain stale values. >> > That can happen if a crtc that was previously driving an HDMI connector >> > switches to a DP connector. In that case, the wrpll field was left with >> > its old value, leading to warnings like the one below: >> > >> > [drm:check_crtc_state [i915]] *ERROR* mismatch in dpll_hw_state.wrpll (expected 0xb035061f, found 0x00000000) >> > ------------[ cut here ]------------ >> > WARNING: CPU: 1 PID: 767 at drivers/gpu/drm/i915/intel_display.c:12324 check_crtc_state+0x975/0x10b0 [i915]() >> > pipe state doesn't match! >> > >> > This regression was indroduced in >> > >> > commit dd3cd74acf12723045a64f1f2c6298ac7b34a5d5 >> > Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> >> > Date: Fri May 15 13:34:29 2015 +0300 >> > >> > drm/i915: Don't overwrite (e)DP PLL selection on SKL >> > >> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> >> >> Reported-by: Linus Torvalds <torvalds@linux-foundation.org> >> Tested-by: Jani Nikula <jani.nikula@intel.com> > > Yeah makes sense as a fix for 4.2. But for 4.3 I wonder whether the > original commit that started this chain needs to be changed a bit: > > 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 > > All the trouble this caused is because it not only preserves the sharing > config (in crtc_state->shared_dpll) but also the ->dpll_hw_state. And I > think with Maarten's latest code (for 4.3) we'd just do an unconditional > compute_config (need it for fast pfit updates and fastboot), which means > the bogus values in ->dpll_hw_state aren't a problem any more since we'll > overwrite them again. And then we could remove that sprinkle of memsets we > have all over, which would be good (since the current approach is > obviously a bit fragile). Anyway: > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Pushed to drm-intel-next-fixes, thanks for the patch and review. One down, another one left to fix. BR, Jani. > > Thanks, Daniel > >> >> > --- >> > >> > Only compile tested, for the wrpll warning. >> > >> > Thanks, >> > Ander >> > >> > drivers/gpu/drm/i915/intel_dp.c | 3 +++ >> > 1 file changed, 3 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> > index 4ebfc3a..fbd9ac3 100644 >> > --- a/drivers/gpu/drm/i915/intel_dp.c >> > +++ b/drivers/gpu/drm/i915/intel_dp.c >> > @@ -1147,6 +1147,9 @@ skl_edp_set_pll_config(struct intel_crtc_state *pipe_config, int link_clock) >> > static void >> > hsw_dp_set_ddi_pll_sel(struct intel_crtc_state *pipe_config, int link_bw) >> > { >> > + memset(&pipe_config->dpll_hw_state, 0, >> > + sizeof(pipe_config->dpll_hw_state)); >> > + >> > switch (link_bw) { >> > case DP_LINK_BW_1_62: >> > pipe_config->ddi_pll_sel = PORT_CLK_SEL_LCPLL_810; >> > -- >> > 2.1.0 >> > >> >> -- >> Jani Nikula, Intel Open Source Technology Center >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6681
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
ILK 302/302 302/302
SNB 312/316 312/316
IVB 343/343 343/343
BYT -2 287/287 285/287
HSW 380/380 380/380
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*BYT igt@gem_partial_pwrite_pread@reads PASS(1) FAIL(1)
*BYT igt@gem_tiled_partial_pwrite_pread@reads PASS(1) FAIL(1)
Note: You need to pay more attention to line start with '*'
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 4ebfc3a..fbd9ac3 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1147,6 +1147,9 @@ skl_edp_set_pll_config(struct intel_crtc_state *pipe_config, int link_clock) static void hsw_dp_set_ddi_pll_sel(struct intel_crtc_state *pipe_config, int link_bw) { + memset(&pipe_config->dpll_hw_state, 0, + sizeof(pipe_config->dpll_hw_state)); + switch (link_bw) { case DP_LINK_BW_1_62: pipe_config->ddi_pll_sel = PORT_CLK_SEL_LCPLL_810;
Similarly to what is done for SKL, clear the dpll_hw_state of the pipe config in hsw_dp_set_ddi_pll_sel(), since it main contain stale values. That can happen if a crtc that was previously driving an HDMI connector switches to a DP connector. In that case, the wrpll field was left with its old value, leading to warnings like the one below: [drm:check_crtc_state [i915]] *ERROR* mismatch in dpll_hw_state.wrpll (expected 0xb035061f, found 0x00000000) ------------[ cut here ]------------ WARNING: CPU: 1 PID: 767 at drivers/gpu/drm/i915/intel_display.c:12324 check_crtc_state+0x975/0x10b0 [i915]() pipe state doesn't match! This regression was indroduced in commit dd3cd74acf12723045a64f1f2c6298ac7b34a5d5 Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> Date: Fri May 15 13:34:29 2015 +0300 drm/i915: Don't overwrite (e)DP PLL selection on SKL Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> --- Only compile tested, for the wrpll warning. Thanks, Ander drivers/gpu/drm/i915/intel_dp.c | 3 +++ 1 file changed, 3 insertions(+)