Re: [PATCH 09/12] drm/i915: wait for a vblank instead of 50ms when enabling FBC

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

On Fri, Nov 13, 2015 at 09:38:50PM +0000, Zanoni, Paulo R wrote:
> Em Sex, 2015-11-13 às 23:26 +0200, Ville Syrjälä escreveu:
> > On Fri, Nov 13, 2015 at 11:20:19PM +0200, Ville Syrjälä wrote:
> > > On Fri, Nov 13, 2015 at 09:03:43PM +0000, Chris Wilson wrote:
> > > > On Fri, Nov 13, 2015 at 05:53:41PM -0200, Paulo Zanoni wrote:
> > > > > Instead of waiting for 50ms, just wait until the next vblank,
> > > > > since
> > > > > it's the minimum requirement.
> > > > > 
> > > > > This moves PC7 residency on my specific BDW machine running
> > > > > Cinnamon
> > > > > from 60-70% to 84-89%. Without FBC, I get 20-25%. I'm using a
> > > > > 3200x1800 eDP panel. Notice: this was the case when the patch
> > > > > was
> > > > > originally proposed, the order of the FBC patches changed since
> > > > > then,
> > > > > so the actual numbers might be slightly different now.
> > > > > 
> > > > > v2:
> > > > >   - Rebase after changing the patch order.
> > > > >   - Update the commit message.
> > > > > 
> > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_drv.h  |  2 +-
> > > > >  drivers/gpu/drm/i915/intel_fbc.c | 12 +++---------
> > > > >  2 files changed, 4 insertions(+), 10 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > > index 9418bd5..ea08714 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > @@ -919,9 +919,9 @@ struct i915_fbc {
> > > > >  
> > > > >  	struct intel_fbc_work {
> > > > >  		bool scheduled;
> > > > > +		u32 scheduled_vblank;
> > > > >  		struct work_struct work;
> > > > >  		struct drm_framebuffer *fb;
> > > > > -		unsigned long enable_jiffies;
> > > > >  	} work;
> > > > >  
> > > > >  	const char *no_fbc_reason;
> > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> > > > > b/drivers/gpu/drm/i915/intel_fbc.c
> > > > > index aa82075..72de8a1 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_fbc.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c
> > > > > @@ -391,7 +391,6 @@ static void intel_fbc_work_fn(struct
> > > > > work_struct *__work)
> > > > >  		container_of(__work, struct drm_i915_private,
> > > > > fbc.work.work);
> > > > >  	struct intel_fbc_work *work = &dev_priv->fbc.work;
> > > > >  	struct intel_crtc *crtc = dev_priv->fbc.crtc;
> > > > > -	unsigned long delay_jiffies = msecs_to_jiffies(50);
> > > > >  
> > > > >  retry:
> > > > >  	/* Delay the actual enabling to let pageflipping cease
> > > > > and the
> > > > > @@ -400,14 +399,9 @@ retry:
> > > > >  	 * vblank to pass after disabling the FBC before we
> > > > > attempt
> > > > >  	 * to modify the control registers.
> > > > >  	 *
> > > > > -	 * A more complicated solution would involve tracking
> > > > > vblanks
> > > > > -	 * following the termination of the page-flipping
> > > > > sequence
> > > > > -	 * and indeed performing the enable as a co-routine
> > > > > and not
> > > > > -	 * waiting synchronously upon the vblank.
> > > > > -	 *
> > > > >  	 * WaFbcWaitForVBlankBeforeEnable:ilk,snb
> > > > >  	 */
> > > > > -	wait_remaining_ms_from_jiffies(work->enable_jiffies,
> > > > > delay_jiffies);
> > > > > +	intel_wait_for_vblank(dev_priv->dev, crtc->pipe);
> > > > >  
> > > > >  	mutex_lock(&dev_priv->fbc.lock);
> > > > >  
> > > > > @@ -416,7 +410,7 @@ retry:
> > > > >  		goto out;
> > > > >  
> > > > >  	/* Were we delayed again while this function was
> > > > > sleeping? */
> > > > > -	if (time_after(work->enable_jiffies + delay_jiffies,
> > > > > jiffies)) {
> > > > > +	if (drm_crtc_vblank_get(&crtc->base) == work-
> > > > > >scheduled_vblank) {
> > > > >  		mutex_unlock(&dev_priv->fbc.lock);
> > > > >  		goto retry;
> > > > >  	}
> > > > > @@ -449,7 +443,7 @@ static void
> > > > > intel_fbc_schedule_activation(struct intel_crtc *crtc)
> > > > >  	 * jiffy count. */
> > > > >  	work->fb = crtc->base.primary->fb;
> > > > >  	work->scheduled = true;
> > > > > -	work->enable_jiffies = jiffies;
> > > > > +	work->scheduled_vblank = drm_crtc_vblank_count(&crtc-
> > > > > >base);
> > > > 
> > > > Isn't the frame counter only incrementing whilst the vblank IRQ
> > > > is
> > > > enabled? Ville?
> > > 
> > > I see a "+ if (drm_crtc_vblank_get(" earlier.
> > 
> > Hmm. Actually it's doing
> > "drm_crtc_vblank_get(&crtc->base) == work->scheduled_vblank)"
> > which looks rather like nonsense.
> > 
> > Not sure what the intention here was...
> 
> Ouch. The intent was for that to be another call for
> drm_crtc_vblank_count().
> 
> The code in discussion is completely based on the drm_wait_one_vblank()
> code: call drm_vblank_count(), then call it again until it returns
> something different. The difference is that we actually call
> drm_wait_one_vblank() in the middle of the process, and that
> scheduled_vblank may also be updated in the meantime, so so may have to
> call drm_wait_one_vblank() again.

You have no guarantees that drm_crtc_vblank_count() won't give you
something totally stale unless you have a vblank reference when calling
it. If you have a reference it's guaranteed to give you something fairly
recent, but the race I outlined in the earlier mail still exists. And yes,
drm_wait_one_vblank() is no good due to that same race.

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