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

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

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@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@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

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