Re: [PATCH 4/4] drm/i915/gt: Remove timeslice suppression

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Quoting Chris Wilson (2021-01-06 16:08:40)
> Quoting Tvrtko Ursulin (2021-01-06 15:57:49)
> > 
> > 
> > On 06/01/2021 12:39, Chris Wilson wrote:
> > > In the next^W future patch, we remove the strict priority system and
> > > continuously re-evaluate the relative priority of tasks. As such we need
> > > to enable the timeslice whenever there is more than one context in the
> > > pipeline. This simplifies the decision and removes some of the tweaks to
> > > suppress timeslicing, allowing us to lift the timeslice enabling to a
> > > common spot at the end of running the submission tasklet.
> > > 
> > > One consequence of the suppression is that it was reducing fairness
> > > between virtual engines on an over saturated system; undermining the
> > > principle for timeslicing.
> > > 
> > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/2802
> > > Testcase: igt/gem_exec_balancer/fairslice
> > > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > > ---
> > >   drivers/gpu/drm/i915/gt/intel_engine_types.h  |  10 -
> > >   .../drm/i915/gt/intel_execlists_submission.c  | 173 +++++++-----------
> > >   2 files changed, 68 insertions(+), 115 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > index 430066e5884c..df62e793e747 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> > > @@ -238,16 +238,6 @@ struct intel_engine_execlists {
> > >        */
> > >       unsigned int port_mask;
> > >   
> > > -     /**
> > > -      * @switch_priority_hint: Second context priority.
> > > -      *
> > > -      * We submit multiple contexts to the HW simultaneously and would
> > > -      * like to occasionally switch between them to emulate timeslicing.
> > > -      * To know when timeslicing is suitable, we track the priority of
> > > -      * the context submitted second.
> > > -      */
> > > -     int switch_priority_hint;
> > > -
> > >       /**
> > >        * @queue_priority_hint: Highest pending priority.
> > >        *
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > > index ba3114fd4389..50d4308023f3 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> > > @@ -1143,25 +1143,6 @@ static void defer_active(struct intel_engine_cs *engine)
> > >       defer_request(rq, i915_sched_lookup_priolist(engine, rq_prio(rq)));
> > >   }
> > >   
> > > -static bool
> > > -need_timeslice(const struct intel_engine_cs *engine,
> > > -            const struct i915_request *rq)
> > > -{
> > > -     int hint;
> > > -
> > > -     if (!intel_engine_has_timeslices(engine))
> > > -             return false;
> > > -
> > > -     hint = max(engine->execlists.queue_priority_hint,
> > > -                virtual_prio(&engine->execlists));
> > > -
> > > -     if (!list_is_last(&rq->sched.link, &engine->active.requests))
> > > -             hint = max(hint, rq_prio(list_next_entry(rq, sched.link)));
> > > -
> > > -     GEM_BUG_ON(hint >= I915_PRIORITY_UNPREEMPTABLE);
> > > -     return hint >= effective_prio(rq);
> > > -}
> > > -
> > >   static bool
> > >   timeslice_yield(const struct intel_engine_execlists *el,
> > >               const struct i915_request *rq)
> > > @@ -1181,76 +1162,68 @@ timeslice_yield(const struct intel_engine_execlists *el,
> > >       return rq->context->lrc.ccid == READ_ONCE(el->yield);
> > >   }
> > >   
> > > -static bool
> > > -timeslice_expired(const struct intel_engine_execlists *el,
> > > -               const struct i915_request *rq)
> > > +static bool needs_timeslice(const struct intel_engine_cs *engine,
> > > +                         const struct i915_request *rq)
> > >   {
> > > +     if (!intel_engine_has_timeslices(engine))
> > > +             return false;
> > > +
> > > +     /* If not currently active, or about to switch, wait for next event */
> > > +     if (!rq || __i915_request_is_complete(rq))
> > > +             return false;
> > > +
> > > +     /* We do not need to start the timeslice until after the ACK */
> > > +     if (READ_ONCE(engine->execlists.pending[0]))
> > > +             return false;
> > > +
> > > +     /* If ELSP[1] is occupied, always check to see if worth slicing */
> > > +     if (!list_is_last_rcu(&rq->sched.link, &engine->active.requests))
> > > +             return true;
> > > +
> > > +     /* Otherwise, ELSP[0] is by itself, but may be waiting in the queue */
> > > +     if (!RB_EMPTY_ROOT(&engine->execlists.queue.rb_root))
> > > +             return true;
> > > +
> > > +     return !RB_EMPTY_ROOT(&engine->execlists.virtual.rb_root);
> > > +}
> > > +
> > > +static bool
> > > +timeslice_expired(struct intel_engine_cs *engine, const struct i915_request *rq)
> > > +{
> > > +     const struct intel_engine_execlists *el = &engine->execlists;
> > > +
> > > +     if (i915_request_has_nopreempt(rq) && __i915_request_has_started(rq))
> > > +             return false;
> > > +
> > > +     if (!needs_timeslice(engine, rq))
> > > +             return false;
> > > +
> > >       return timer_expired(&el->timer) || timeslice_yield(el, rq);
> > >   }
> > >   
> > > -static int
> > > -switch_prio(struct intel_engine_cs *engine, const struct i915_request *rq)
> > > -{
> > > -     if (list_is_last(&rq->sched.link, &engine->active.requests))
> > > -             return engine->execlists.queue_priority_hint;
> > > -
> > > -     return rq_prio(list_next_entry(rq, sched.link));
> > > -}
> > > -
> > > -static inline unsigned long
> > > -timeslice(const struct intel_engine_cs *engine)
> > > +static unsigned long timeslice(const struct intel_engine_cs *engine)
> > >   {
> > >       return READ_ONCE(engine->props.timeslice_duration_ms);
> > >   }
> > >   
> > > -static unsigned long active_timeslice(const struct intel_engine_cs *engine)
> > > -{
> > > -     const struct intel_engine_execlists *execlists = &engine->execlists;
> > > -     const struct i915_request *rq = *execlists->active;
> > > -
> > > -     if (!rq || __i915_request_is_complete(rq))
> > > -             return 0;
> > > -
> > > -     if (READ_ONCE(execlists->switch_priority_hint) < effective_prio(rq))
> > > -             return 0;
> > > -
> > > -     return timeslice(engine);
> > > -}
> > > -
> > > -static void set_timeslice(struct intel_engine_cs *engine)
> > > +static void start_timeslice(struct intel_engine_cs *engine)
> > >   {
> > > +     struct intel_engine_execlists *el = &engine->execlists;
> > >       unsigned long duration;
> > >   
> > > -     if (!intel_engine_has_timeslices(engine))
> > > -             return;
> > > +     /* Disable the timer if there is nothing to switch to */
> > > +     duration = 0;
> > > +     if (needs_timeslice(engine, *el->active)) {
> > > +             if (el->timer.expires) {
> > 
> > Why not just timer_pending check? Are you sure timer->expires cannot 
> > legitimately be at jiffie 0 in wrap conditions?
> 
> This is actually to test if we have set the timer or not, and avoid
> extending an already active timeslice. We are abusing the jiffie wrap
> being unlikely and of no great consequence (one missed timeslice/preempt
> timer should be picked up by the next poke of the driver) as part of
> set_timer_ms/cancel_timer.

I see you've asked this before:

void set_timer_ms(struct timer_list *t, unsigned long timeout)
{
        if (!timeout) {
                cancel_timer(t);
                return;
        }

        timeout = msecs_to_jiffies(timeout);

        /*
         * Paranoia to make sure the compiler computes the timeout before
         * loading 'jiffies' as jiffies is volatile and may be updated in
         * the background by a timer tick. All to reduce the complexity
         * of the addition and reduce the risk of losing a jiffie.
         */
        barrier();

        /* Keep t->expires = 0 reserved to indicate a canceled timer. */
        mod_timer(t, jiffies + timeout ?: 1);
}

:)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx



[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux