Re: [PATCH v2 08/13] drm/i915: Make shared dpll functions take crtc_state

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Oct 04, 2018 at 03:25:42PM +0200, Maarten Lankhorst wrote:
> Op 04-10-18 om 14:57 schreef Ville Syrjälä:
> > On Thu, Oct 04, 2018 at 11:45:59AM +0200, Maarten Lankhorst wrote:
> >> Do not rely on crtc->config any more. Remove the assertion from
> >> ibx_pch_dpll_disable, because we the dpll state tracking should
> >> already handle this case correctly.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c  | 14 ++++++------
> >>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 31 +++++++++++----------------
> >>  drivers/gpu/drm/i915/intel_dpll_mgr.h |  6 +++---
> >>  3 files changed, 22 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index fe8ccbdd4ea1..4df4293a7917 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -4749,7 +4749,7 @@ static void ironlake_pch_enable(const struct intel_atomic_state *state,
> >>  	 * Note that enable_shared_dpll tries to do the right thing, but
> >>  	 * get_shared_dpll unconditionally resets the pll - we need that to have
> >>  	 * the right LVDS enable sequence. */
> >> -	intel_enable_shared_dpll(crtc);
> >> +	intel_enable_shared_dpll(crtc_state);
> >>  
> >>  	/* set transcoder timing, panel must allow it */
> >>  	assert_panel_unlocked(dev_priv, pipe);
> >> @@ -5591,8 +5591,8 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config,
> >>  	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
> >>  	intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false);
> >>  
> >> -	if (intel_crtc->config->has_pch_encoder)
> >> -		intel_prepare_shared_dpll(intel_crtc);
> >> +	if (pipe_config->has_pch_encoder)
> >> +		intel_prepare_shared_dpll(pipe_config);
> >>  
> >>  	if (intel_crtc_has_dp_encoder(intel_crtc->config))
> >>  		intel_dp_set_m_n(intel_crtc, M1_N1);
> >> @@ -5710,8 +5710,8 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
> >>  
> >>  	intel_encoders_pre_pll_enable(crtc, pipe_config, old_state);
> >>  
> >> -	if (intel_crtc->config->shared_dpll)
> >> -		intel_enable_shared_dpll(intel_crtc);
> >> +	if (pipe_config->shared_dpll)
> >> +		intel_enable_shared_dpll(pipe_config);
> >>  
> >>  	if (INTEL_GEN(dev_priv) >= 11)
> >>  		icl_map_plls_to_ports(crtc, pipe_config, old_state);
> >> @@ -6286,7 +6286,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
> >>  
> >>  	intel_fbc_disable(intel_crtc);
> >>  	intel_update_watermarks(intel_crtc);
> >> -	intel_disable_shared_dpll(intel_crtc);
> >> +	intel_disable_shared_dpll(crtc_state);
> > I guess this is the explosion on i915g/i965g. We've already freed this
> > crtc state at this point. So I guess s/crtc_state/crtc->state/ and it
> > should be fine?
> >
> No, the explosion is at
> 
> -	if (WARN_ON(pll == NULL))
> +	if (!WARN_ON(!pll))
>  		return;
> 
> Oops! Should be fixed in v2 I'm sending now.

<4>[   11.181254] R10: 0000000000000000 R11: 0000000000000000 R12: 6b6b6b6b6b6b6b6b
<4>[   11.181141] RIP: 0010:intel_disable_shared_dpll+0x12/0x130 [i915]
<4>[   11.181157] Code: c7 e9 67 25 a0 e8 9e da ec e0 0f 0b e9 18 ff ff ff 0f 1f 80 00 00 00 00 41 56 41 55 41 54 55 53 4c 8b 27 48 8b 9f 98 02 00 00 <4d> 8b 34 24 41 80 be 5c 0d 00 00 04 76 05 48 85 db 75 09 5b 5d 41

72a:   4d 8b 34 24             mov    (%r12),%r14

Looks like it's still trying to drink poison.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx