[Intel-gfx] [PATCH 01/16] Revert "drm/i915/display: Disable audio, DRRS and PSR before planes"

[Souza, Jose]

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 at linux.intel.com>
> > > > > > 
> > > > > > 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 at intel.com>
> > 
> > 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

Whole branch: https://github.com/zehortigoza/linux/commits/upstream-psr2-sel-fetch-new

>