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 Tue, Nov 17, 2015 at 07:03:13PM +0000, Zanoni, Paulo R wrote:
> Em Sáb, 2015-11-14 às 01:17 +0200, Ville Syrjälä escreveu:
> > 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,
> 
> All the points mentioned above are easily fixable.
> 
> > but the race I outlined in the earlier mail still exists.
> 
> I've been trying to understand this, and it seems
> i915_get_vblank_counter() tries to work around that problem with the
> logic behind the "pixel" and "vbl_start" variables. Isn't that enough?
> If no, why?

No i915_get_vblank_counter() is there for another reason; On gen3/4
the hw counter increments at the wrong time, so we have to convert
it to look like it incremented at the right time. Fortunately we
can read out the hw counter and the pixel counter atomically, so
there's no race or anything due to the conversion.

> >  And yes,
> > drm_wait_one_vblank() is no good due to that same race.
> 
> So shouldn't we stop using intel_wait_for_vblank() on the affected
> platforms?

All platforms are affected, just some are more susceptible than others.
We should fix intel_wait_for_vblank() to do the right thing.

> Or at least add a big comment explaining the problem, or add
> a WARN(gen == 3 || gen == 4 && !is_g4x), or something else. I got a
> little surprised when you said the function is unreliable (was the
> previous implementation based on I915_READ also equally unreliable?).

On g4x+ the polling implementation didn't suffer from this issue since
it was polling the hw frame counter. But the fact that it was polling
was sub-optimal.

On older platforms a similar race was there since it was polling the
"frame start" bit and not the "start of vblank" bit. In addition
it could also effectively eat the interrupt that someone was waiting
for (on gen2/3 since they use the "frame start" as the interrupt as
well).

> Also, since we are already running this risk in other parts of the
> code, I'm not sure what are the conditions for my patch to be
> acceptable.

The main problem in your patch is using drm_vblank_count() w/o a
drm_vblank_get/put(). So that should be fixed somehow.

I guess we can ignore the race with the vblank interrupt handler
for now since the same problem exists everywhere else already.

> Just fixing the original implementation, while keeping it
> based on drm_wait_one_vblank() and maybe adding some TODO comment would
> do? Or do you see an actual simple solution for the race you mentioned?
> 
> Thanks,
> Paulo
> 
> > 

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