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

[Souza, Jose]

On Thu, 2021-09-16 at 16:21 +0300, Ville Syrjälä wrote:
> 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 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
> 
> 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.

Thanks for catching it, fixed here: https://github.com/zehortigoza/linux/commit/77f3dd1e1dccdf25d04cb42a45b95570b2d3d3e8

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

I can help with reviews if you want to do that.

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