Quoting Tvrtko Ursulin (2018-05-08 12:38:06)
>
> On 08/05/2018 12:05, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-05-08 11:56:37)
> >>
> >> On 08/05/2018 11:24, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2018-05-08 11:10:41)
> >>>> On 07/05/2018 14:57, Chris Wilson wrote:
> >>>>> @@ -744,13 +748,25 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
> >>>>> /* We must always keep the beast fed if we have work piled up */
> >>>>> GEM_BUG_ON(execlists->first && !port_isset(execlists->port));
> >>>>>
> >>>>> -unlock:
> >>>>> - spin_unlock_irq(&engine->timeline.lock);
> >>>>> -
> >>>>> - if (submit) {
> >>>>> + /* Re-evaluate the executing context setup after each preemptive kick */
> >>>>> + if (last)
> >>>>> execlists_user_begin(execlists, execlists->port);
> >>>>
> >>>> Last can be non-null and submit false, so this is not equivalent.
> >>>>
> >>>> By the looks of it makes no difference since it is OK to set the
> >>>> execlists user active bit multiple times. Even though the helper is
> >>>> called execlists_set_active_once. But the return value is not looked at.
> >>>>
> >>>> Still, why not keep doing this when submit is true?
> >>>
> >>> It's a subtle difference, in that we want the context reevaluated every
> >>> time we kick the queue as we may have changed state that we want to
> >>> reload, and not just ELSP. Sometimes we need inheritance of more than
> >>> just priority...
> >>
> >> What do you mean by context re-evaluated?
> >>
> >> ACTIVE_USER is set from first to last request, no? I don't understand
> >> what would change if you would set it multiple times while it is already
> >> set.
> >
> > It's not about ACTIVE_USER. This is a hook to indicate a change in
> > context state being executed on the GPU. It's to be hooked into by the
> > cpufreq/pstate code, gpufreq code, etc. Later realisation is that they
> > need to be notified for all context changes here.
>
> Ok so nothing as such relating to making tasklets hardirq safe.
Ssh.
> Is the execlists_user_begin still the right name then?
It's never been quite the right name. Just the best I could come up
with. execlists_user_busy()/execlists_user_idle() might be more
sensible.
> Any future use for execlists_set_active_once or we could drop it
> (replacing with execlists_set_active)?
set_active_once is intended to be used by the cpufreq/pstate code, or
anything else that only cares about the off->on/on->off transition and
not every reload. I'm waiting for that code to be merged...
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
