On Wed, Sep 15, 2021 at 08:19:41PM +0000, Souza, Jose wrote: > On Wed, 2021-09-15 at 15:30 +0300, Ville Syrjälä wrote: > > 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? > > https://github.com/zehortigoza/linux/commit/013478a67e0b96abbaf6ab2d1b4be324b0fe737b That's not going to work correctly. You can't depend on connectors being part of the state since that's not the case for pure plane updates/etc. In general I really dislike the PSR code's reliance on the encoder/connector. Tht makes it really hard to do these sorts of things. So I think we'd have to redesign it to try to operate purely on the crtc and not need the encoder/connector. > > Whole branch: https://github.com/zehortigoza/linux/commits/upstream-psr2-sel-fetch-new > > > > -- Ville Syrjälä Intel
