Re: [Intel-gfx] [PATCH] drm/i915: Clear pipe's pll hw state in hsw_dp_set_ddi_pll_sel()

[Daniel Vetter]

On Wed, Jul 01, 2015 at 05:54:06PM +0300, Ander Conselvan De Oliveira wrote:
> On Tue, 2015-06-30 at 18:41 +0300, Jani Nikula wrote:
> > On Tue, 30 Jun 2015, Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
> > >> > 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@xxxxxxxxx>
> > >> 
> > >> Reported-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> > >> Tested-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> > >
> > > 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@xxxxxxxxx>
> > > 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@xxxxxxxx>
> > 
> > Pushed to drm-intel-next-fixes, thanks for the patch and review. One
> > down, another one left to fix.
> 
> I made some progress on the second issue, but I'm afraid Jani might have
> a found a third bug. The warning he gets happens because we try to wait
> for vblanks while updating the primary plane during the modeset. At that
> point, the crtc is off. The problem is in intel_check_primary_plane(),
> which is called from drm_atomic_helper_check_planes(). That function
> makes decisions about waiting for a vblank based on intel_crtc->active.
> Since the check is called before we disable the crtcs, active might be
> true, even though the plane update is done with crtcs disable.
> 
> The patch below makes the warning go away, but I still need to figure
> out how to set crtc_state->planes_changed properly if we are going down
> that route.
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index dcb1d25..f14727c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12480,10 +12480,6 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
>  
>         intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,"[modeset]");
>  
> -       ret = drm_atomic_helper_check_planes(state->dev, state);
> -       if (ret)
> -               return ERR_PTR(ret);
> -
>         return pipe_config;
>  }
>  
> 
> The backtrace on Linus' machine is different, though. It comes from the
> call to intel_crtc_disable_planes() in __intel_set_mode(). That would
> indicate we have a crtc with crtc->state->enable == true but that is
> actually inactive. I'm still not sure how we can get in that state.

Using intel_crtc->active to precompute any kind of decisions won't work. I
guess we just need to delay the decision whether to make a vblank wait or
not to where we do the vblank wait, and use the (then current
intel_crtc->active) there. This will be fixed properly in 4.3.

I suspect Linus' backtrace is something similar - we try to precompute
what needs to be updated, get it wrong and the go boom. Sprinkling an

	if (!intel_crtc->active)
		return;

early return into the set_mode should help. But I haven't looked at what
4.2 looks precisely yet in this area - too much flux because of the atomic
conversion.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/