[Intel-gfx] [PATCH] drm/i915: Avoid keeping waitboost active for signaling threads

[Chris Wilson]

Quoting Michał Winiarski (2017-06-23 11:35:06)
> On Thu, Jun 22, 2017 at 11:55:51AM +0100, Chris Wilson wrote:
> > Once a client has requested a waitboost, we keep that waitboost active
> > until all clients are no longer waiting. This is because we don't
> > distinguish which waiter deserves the boost. However, with the advent of
> > fence signaling, the signaler threads appear as waiters to the RPS
> > interrupt handler. So instead of using a single boolean to track when to
> > keep the waitboost active, use a counter of all outstanding waitboosted
> > requests.
> > 
> > At this point, I have removed all vestiges of the rate limiting on
> > clients. Whilst this means that compositors should remain more fluid,
> > it also means that boosts are more prevalent.
> > 
> > A drawback of this implementation is that it requires constant request
> > submission to keep the waitboost trimmed (as it is now cancelled when the
> > request is completed). This will be fine for a busy system, but near
> > idle the boosts may be kept for longer than desired (effectively tens of
> > vblanks worstcase) and there is a reliance on rc6 instead.
> 
> In other words, now we're disabling boost when all requests that required boost
> are retired.
> We may need to tweak igt pm_rps boost scenarios to account for that extra time.

It should be less time boosted than before, if the requests are retired
promptly. But that's a big if.

> > Reported-by: Michał Winiarski <michal.winiarski at intel.com>
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Michał Winiarski <michal.winiarski at intel.com>
> 
> [SNIP]
> 
> > @@ -2347,11 +2349,10 @@ static int i915_rps_boost_info(struct seq_file *m, void *data)
> >  
> >               rcu_read_lock();
> >               task = pid_task(file->pid, PIDTYPE_PID);
> > -             seq_printf(m, "%s [%d]: %d boosts%s\n",
> > +             seq_printf(m, "%s [%d]: %d boosts\n",
> >                          task ? task->comm : "<unknown>",
> >                          task ? task->pid : -1,
> > -                        file_priv->rps.boosts,
> > -                        list_empty(&file_priv->rps.link) ? "" : ", active");
> > +                        atomic_read(&file_priv->rps.boosts));
> >               rcu_read_unlock();
> >       }
> >       seq_printf(m, "Kernel (anonymous) boosts: %d\n", dev_priv->rps.boosts);
> 
> dev_priv->rps.boosts seems to be unused at this point, could you remove it while
> you're here?

Oh, I should just hook up the other side of

> > +     if (rps != NULL)
> > +             atomic_inc(&rps->boosts);

else
	atomic_inc(&dev_priv->rps.boosts);

The only user is dead-code atm, but it may come in useful again for
atomic modesetting.
-Chris