On Wed, Sep 15, 2021 at 12:00:28AM +0000, Souza, Jose wrote: > On Tue, 2021-09-14 at 16:30 -0700, José Roberto de Souza wrote: > > On Tue, 2021-09-14 at 11:20 +0300, Ville Syrjälä wrote: > > > On Mon, Sep 13, 2021 at 04:28:35PM +0000, Souza, Jose wrote: > > > > On Mon, 2021-09-13 at 17:44 +0300, Ville Syrjala wrote: > > > > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > > > > > > > Disabling planes in the middle of the modeset seuqnece does not make > > > > > sense since userspace can anyway disable planes before the modeset > > > > > even starts. So when the modeset seuqence starts the set of enabled > > > > > planes is entirely arbitrary. Trying to sprinkle the plane disabling > > > > > into the modeset sequence just means more randomness and potential > > > > > for hard to reproduce bugs. > > > > > > > > The patch being reverted did not changed anything about plane, it only disables audio and PSR before pipe is disabled in this case. > > > > > > The commit message only talks about planes. Also we already disable > > > the pipe in the post_disable hook, so PSR/audio was always disabled > > > before the pipe IIRC. > > > > That is true, my bad. > > > > Reviewed-by: José Roberto de Souza <jose.souza@xxxxxxxxx> > > Sorry I missed the intel_crtc_disable_planes() call, so here is the problem: > > > intel_commit_modeset_disables() > intel_old_crtc_state_disables() > intel_crtc_disable_planes() > intel_disable_plane() > dev_priv->display.crtc_disable(state, crtc)/hsw_crtc_disable() > intel_encoders_disable() > encoder->disable()/intel_disable_ddi() > intel_psr_disable() > intel_encoders_post_disable() > post_disable/intel_ddi_post_disable() > intel_disable_pipe() > > So all the planes are disabled while PSR is still on, that is why this patch fixed the underrun. > > We need to call the pre_disable() before intel_crtc_disable_planes() and for the case where pipe is not disabled but all of its planes are requires > the pending patch that I have. > > Or do you have other suggestion? I would like to follow the same sequence always, ie. disable planes first (be it from userspace or from the kernel just before the modeset), and then we take the exact same measures in both cases to deal with whatever is the problem with PSR vs. disabled planes. That makes the sequence as deterministic as possible, and thus we avoid potential weird bugs stemming from userspace behaviour wrt. disabling planes. Hmm. Our modeset plane disable code is certainly a bit lackluster. It misses a bunch of stuff that we do for normal plane updates. So we might want to put a few extra things in there. Maybe PSR needs the vblank_get+psr_idle trick? And we might want a vrr_push/etc. in there as well, not sure. What exactly is your solution to the case where the planes are already disabled by userspace? -- Ville Syrjälä Intel
