[Intel-gfx] [PATCH 4/5] drm/i915/frontbuffer: Remove early frontbuffer flush in prepare_plane_fb()

[Ville Syrjälä]

On Thu, Mar 01, 2018 at 10:35:48AM -0800, Rodrigo Vivi wrote:
> On Thu, Mar 01, 2018 at 01:22:50PM +0200, Ville Syrjälä wrote:
> > On Thu, Mar 01, 2018 at 12:51:45PM +0200, Ville Syrjälä wrote:
> > > On Wed, Feb 28, 2018 at 11:38:56PM +0000, Pandiyan, Dhinakaran wrote:
> > > > 
> > > > 
> > > > 
> > > > On Wed, 2018-02-28 at 22:38 +0200, Ville Syrjälä wrote:
> > > > > On Wed, Feb 28, 2018 at 10:28:13PM +0200, Ville Syrjälä wrote:
> > > > > > On Sat, Feb 24, 2018 at 03:24:55AM +0000, Pandiyan, Dhinakaran wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On Mon, 2018-02-19 at 10:07 +0100, Maarten Lankhorst wrote:
> > > > > > > > Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran:
> > > > > > > > > On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote:
> > > > > > > > >> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21)
> > > > > > > > >>> Preparing a framebuffer should not require a flush. _post_plane_update()
> > > > > > > > >>> takes care of flushing when a flip is scheduled, this should be
> > > > > > > > >>> sufficient for PSR and FBC.
> > > > > > > > >> Makes sense.
> > > > > > > > >>  
> > > > > > > > > I also think this might speed up the flips a bit by avoiding flushes. 
> > > > > > > > >
> > > > > > > > >>> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > > > > > > > >>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > > > > > >>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > > > > > > >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > > > > > > >> Also
> > > > > > > > >> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > > > > > > > >> to validate the flow through atomic.
> > > > > > > > >> -Chris
> > > > > > > > >>
> > > > > > > > Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that?
> > > > > > > > 
> > > > > > > 
> > > > > > > I have no context why it was removed, I'll have to understand that
> > > > > > > change and get back to you.
> > > > > > 
> > > > > > Since we supposedly have hw nuke for both fbc and psr there doesn't seem
> > > > > > to be much need to do anything for flips. I guess DRRS is the only
> > > > > > thing that kinda needs it (not really, just avoids flipping with the
> > > > > > slow timings). But I think DRRS should really be tied into the vblank
> > > > > > stuff somehow so that we switch to the fast timings whenever a vblank
> > > > > > interrupts are enabled.
> > > > > 
> > > > > Oh, I guess VLV/CHV PSR is what would need this. To do that properly
> > > > > (ie. main link off) I think we'd basically need to do a full modeset
> > > > > when exiting PSR, so it should probably handled somewhere higher up
> > > > > during modeset, and for other uses the frontbuffer tracking
> > > > > should perhaps just schedule a work to do the full modeset.
> > > > > 
> > > > The mention of "full modeset" got me thinking. I believe you said full
> > > > modeset because the link needs to be trained on PSR exit if it was off.
> > > > But, link off isn't supported on VLV/CHV
> > > > 
> > > > else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > 	/* On VLV and CHV only standby mode is supported. */
> > > > 	dev_priv->psr.link_standby = true;
> > > 
> > > I think that's just because we've been lazy and done it. I think even
> > > with the link off we'd need to reprogram all planes at least.
> > 
> > Not had coffee yet today, and it shows. Let me rewrite that entire thing:
> > 
> > I think that's just because we've been lazy and not done it (link off mode).
> 
> I agree with you here. This was probably exactly what was missing.
> But, how to do this without flickering (blinking?!) the screen?
> 
> > I think even without the link off we'd need to reprogram all planes at least.
> 
> on VLV/CHV or everywhere? and why do you think that?

VLV/CHV. I believe the hw implements PSR by simplty turning off the
planes (and pipes+dplls for link off), but it doesn't have any automagic
stuff for recovering the original state. It's all up to the driver.

IIRC Rodrigo even confirmed this at some point, or at least he had to
trigger a plane update to get something visible on the screen.

-- 
Ville Syrjälä
Intel OTC