drm/i915/gt: Only drop heartbeat.systole if the sole owner
diff mbox series

Message ID 20191106133129.17732-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • drm/i915/gt: Only drop heartbeat.systole if the sole owner
Related show

Commit Message

Chris Wilson Nov. 6, 2019, 1:31 p.m. UTC
Mika spotted that only using cancel_delayed_work() could mean that we
attempted to clear the heartbeat.systole while the worker was still
running. Rectify the situation by only touching the systole from outside
the worker if we suceeded in cancelling the worker before it could run.
The worker is expected to clean up by itself upon idling.

Reported-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Fixes: 058179e72e09 ("drm/i915/gt: Replace hangcheck by heartbeats")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Mika Kuoppala Nov. 6, 2019, 2:40 p.m. UTC | #1
Chris Wilson <chris@chris-wilson.co.uk> writes:

> Mika spotted that only using cancel_delayed_work() could mean that we
> attempted to clear the heartbeat.systole while the worker was still
> running. Rectify the situation by only touching the systole from outside
> the worker if we suceeded in cancelling the worker before it could run.

succeeded

> The worker is expected to clean up by itself upon idling.
>
> Reported-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Fixes: 058179e72e09 ("drm/i915/gt: Replace hangcheck by heartbeats")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c | 4 ++--
>  1 file changed, 2 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 5051f304705b..06aa14c7aa8c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> @@ -141,8 +141,8 @@ void intel_engine_unpark_heartbeat(struct intel_engine_cs *engine)
>  
>  void intel_engine_park_heartbeat(struct intel_engine_cs *engine)
>  {
> -	cancel_delayed_work(&engine->heartbeat.work);
> -	i915_request_put(fetch_and_zero(&engine->heartbeat.systole));
> +	if (cancel_delayed_work(&engine->heartbeat.work))
> +		i915_request_put(fetch_and_zero(&engine->heartbeat.systole));

We don't arm in park path so this should do the trick.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

>  }
>  
>  void intel_engine_init_heartbeat(struct intel_engine_cs *engine)
> -- 
> 2.24.0

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
index 5051f304705b..06aa14c7aa8c 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
@@ -141,8 +141,8 @@  void intel_engine_unpark_heartbeat(struct intel_engine_cs *engine)
 
 void intel_engine_park_heartbeat(struct intel_engine_cs *engine)
 {
-	cancel_delayed_work(&engine->heartbeat.work);
-	i915_request_put(fetch_and_zero(&engine->heartbeat.systole));
+	if (cancel_delayed_work(&engine->heartbeat.work))
+		i915_request_put(fetch_and_zero(&engine->heartbeat.systole));
 }
 
 void intel_engine_init_heartbeat(struct intel_engine_cs *engine)