Re: [PATCH 10/10] drm/i915: Flush idle barriers when waiting

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 10/10/2019 08:14, Chris Wilson wrote:
> If we do find ourselves with an idle barrier inside our active while
> waiting, attempt to flush it by emitting a pulse using the kernel
> context.

The point of this one completely escapes me at the moment. Idle barriers 
are kept in there to be consumed by the engine_pm parking, so if any 
random waiter finds some (there will always be some, as long as the 
engine executed some user context, right?), why would it want to handle 
them? Again just to use the opportunity for some house keeping? But what 
if the system is otherwise quite busy and a low-priority client just 
happens to want to wait on something silly?

Regards,

Tvrtko

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   .../gpu/drm/i915/gt/intel_engine_heartbeat.c  | 14 +++++++++++++
>   .../gpu/drm/i915/gt/intel_engine_heartbeat.h  |  1 +
>   drivers/gpu/drm/i915/i915_active.c            | 21 +++++++++++++++++--
>   3 files changed, 34 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 f68acf9118f3..e27bb7f028bd 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
> @@ -169,3 +169,17 @@ int intel_engine_pulse(struct intel_engine_cs *engine)
>   	intel_engine_pm_put(engine);
>   	return err;
>   }
> +
> +int intel_engine_flush_barriers(struct intel_engine_cs *engine)
> +{
> +	struct i915_request *rq;
> +
> +	rq = i915_request_create(engine->kernel_context);
> +	if (IS_ERR(rq))
> +		return PTR_ERR(rq);
> +
> +	idle_pulse(engine, rq);
> +	i915_request_add(rq);
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.h b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.h
> index 39391004554d..0c1ad0fc091d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.h
> @@ -15,5 +15,6 @@ void intel_engine_park_heartbeat(struct intel_engine_cs *engine);
>   void intel_engine_unpark_heartbeat(struct intel_engine_cs *engine);
>   
>   int intel_engine_pulse(struct intel_engine_cs *engine);
> +int intel_engine_flush_barriers(struct intel_engine_cs *engine);
>   
>   #endif /* INTEL_ENGINE_HEARTBEAT_H */
> diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
> index aa37c07004b9..98d5fe1c7e19 100644
> --- a/drivers/gpu/drm/i915/i915_active.c
> +++ b/drivers/gpu/drm/i915/i915_active.c
> @@ -6,6 +6,7 @@
>   
>   #include <linux/debugobjects.h>
>   
> +#include "gt/intel_engine_heartbeat.h"
>   #include "gt/intel_engine_pm.h"
>   
>   #include "i915_drv.h"
> @@ -435,6 +436,21 @@ static void enable_signaling(struct i915_active_fence *active)
>   	dma_fence_put(fence);
>   }
>   
> +static int flush_barrier(struct active_node *it)
> +{
> +	struct intel_engine_cs *engine;
> +
> +	if (!is_barrier(&it->base))
> +		return 0;
> +
> +	engine = __barrier_to_engine(it);
> +	smp_rmb(); /* serialise with add_active_barriers */
> +	if (!is_barrier(&it->base))
> +		return 0;
> +
> +	return intel_engine_flush_barriers(engine);
> +}
> +
>   int i915_active_wait(struct i915_active *ref)
>   {
>   	struct active_node *it, *n;
> @@ -448,8 +464,9 @@ int i915_active_wait(struct i915_active *ref)
>   	/* Flush lazy signals */
>   	enable_signaling(&ref->excl);
>   	rbtree_postorder_for_each_entry_safe(it, n, &ref->tree, node) {
> -		if (is_barrier(&it->base)) /* unconnected idle barrier */
> -			continue;
> +		err = flush_barrier(it); /* unconnected idle barrier? */
> +		if (err)
> +			break;
>   
>   		enable_signaling(&it->base);
>   	}
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx