Quoting Tvrtko Ursulin (2020-09-24 14:43:08)
>
> On 16/09/2020 10:42, Chris Wilson wrote:
> > Currently, we check we can send a pulse prior to disabling the
> > heartbeat to verify that we can change the heartbeat, but since we may
> > re-evaluate execution upon changing the heartbeat interval we need another
> > pulse afterwards to refresh execution.
> >
> > Fixes: 9a40bddd47ca ("drm/i915/gt: Expose heartbeat interval via sysfs")
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> > Cc: <stable@xxxxxxxxxxxxxxx> # v5.7+
> > ---
> > drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> > index 8ffdf676c0a0..d09df370f7cd 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> > @@ -192,10 +192,12 @@ int intel_engine_set_heartbeat(struct intel_engine_cs *engine,
> > WRITE_ONCE(engine->props.heartbeat_interval_ms, delay);
> >
> > if (intel_engine_pm_get_if_awake(engine)) {
> > - if (delay)
> > + if (delay) {
> > intel_engine_unpark_heartbeat(engine);
> > - else
> > + } else {
> > intel_engine_park_heartbeat(engine);
> > + intel_engine_pulse(engine); /* recheck execution */
> > + }
> > intel_engine_pm_put(engine);
> > }
> >
> >
>
> I did not immediately get this one. Do we really need two pulses or
> maybe we could re-order the code a bit and just undo the heartbeat park
> if pulse after parking did not work?
We use the first pulse to determine if it's legal for the parameter to
be changed (checking we support the preemptive pulse to remove
non-persistent contexts). Then the second pulse after changing the
parameter to flush the changes through.
I like checking for support before making the change, although we could
try and fixup after failure, there would still be a window where the
change would be visible to the system. We don't need to use the pulse per
se for that check, that's pure convenience as it performs the checking
already.
-Chris
