[Intel-gfx] [PATCH 2/2] drm/i915: Don't dereference request if it may have been retired

[Tvrtko Ursulin]

On 18/06/2019 17:19, Chris Wilson wrote:
> This has count me out on countless occasions, when we retrieve a pointer
> from the submission/execlists backend, it does not carry a reference to
> the context or ring. Those are only pinned while the rquest is active,
> so if we see the request is completed, it may be in the process of being
> retired and those pointers defunct.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110938
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/gt/intel_engine_cs.c | 16 +++++++++-------
>   1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 898692989313..7fd33e81c2d9 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -1311,12 +1311,13 @@ static void hexdump(struct drm_printer *m, const void *buf, size_t len)
>   	}
>   }
>   
> -static void intel_engine_print_registers(const struct intel_engine_cs *engine,
> +static void intel_engine_print_registers(struct intel_engine_cs *engine,
>   					 struct drm_printer *m)
>   {
>   	struct drm_i915_private *dev_priv = engine->i915;
>   	const struct intel_engine_execlists * const execlists =
>   		&engine->execlists;
> +	unsigned long flags;
>   	u64 addr;
>   
>   	if (engine->id == RCS0 && IS_GEN_RANGE(dev_priv, 4, 7))
> @@ -1397,15 +1398,16 @@ static void intel_engine_print_registers(const struct intel_engine_cs *engine,
>   				   idx, hws[idx * 2], hws[idx * 2 + 1]);
>   		}
>   
> -		rcu_read_lock();
> +		spin_lock_irqsave(&engine->active.lock, flags);
>   		for (idx = 0; idx < execlists_num_ports(execlists); idx++) {
>   			struct i915_request *rq;
>   			unsigned int count;
> +			char hdr[80];
>   
>   			rq = port_unpack(&execlists->port[idx], &count);
> -			if (rq) {
> -				char hdr[80];
> -
> +			if (!rq) {
> +				drm_printf(m, "\t\tELSP[%d] idle\n", idx);
> +			} else if (!i915_request_signaled(rq)) {
>   				snprintf(hdr, sizeof(hdr),
>   					 "\t\tELSP[%d] count=%d, ring:{start:%08x, hwsp:%08x, seqno:%08x}, rq: ",
>   					 idx, count,
> @@ -1414,11 +1416,11 @@ static void intel_engine_print_registers(const struct intel_engine_cs *engine,
>   					 hwsp_seqno(rq));
>   				print_request(m, rq, hdr);
>   			} else {
> -				drm_printf(m, "\t\tELSP[%d] idle\n", idx);
> +				print_request(m, rq, "\t\tELSP[%d] rq: ");
>   			}
>   		}
>   		drm_printf(m, "\t\tHW active? 0x%x\n", execlists->active);
> -		rcu_read_unlock();
> +		spin_unlock_irqrestore(&engine->active.lock, flags);
>   	} else if (INTEL_GEN(dev_priv) > 6) {
>   		drm_printf(m, "\tPP_DIR_BASE: 0x%08x\n",
>   			   ENGINE_READ(engine, RING_PP_DIR_BASE));
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko