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

["Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>]

On Thu, 2018-03-01 at 11:00 -0800, Rodrigo Vivi wrote:
> On Thu, Mar 01, 2018 at 08:43:05PM +0200, Ville Syrjälä wrote:
> > 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@intel.com>
> > > > > > > > > > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > > > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > > > > > >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > > > > > >> Also
> > > > > > > > > > >> Cc: Maarten Lankhorst <maarten.lankhorst@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.
> 
> My memory is terrible. But this makes so much sense and in sync
> with some vague memory flashes here :)
> 
> And probably what blocked me for looking there besides the
> other bugs on core platforms was my disbelieve that would
> be possible without seeing any blink of a blank screen when
> exiting PSR. :/
> 

We've got a couple of options here -
1) Drop this patch because it breaks VLV/CHV link standby. 

2) Keep this patch because VLV/CHV link standby is already broken (no
plane programming on exit). Follow this up with reworking PSR on
VLV/CHV.


3) Or should this be -
if (VLV() || CHV)
	intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);




> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx