Re: [PATCH 01/55] drm/i915: Re-instate request->uniq becuase it is extremely useful

[Tomas Elf <tomas.elf@intel.com>]

On 29/05/2015 17:43, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> The seqno value cannot always be used when debugging issues via trace
> points. This is because it can be reset back to start, especially
> during TDR type tests. Also, when the scheduler arrives the seqno is
> only valid while a given request is executing on the hardware. While
> the request is simply queued waiting for submission, it's seqno value
> will be zero (meaning invalid).
>
> For: VIZ-5115
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h   |    4 ++++
>   drivers/gpu/drm/i915/i915_gem.c   |    1 +
>   drivers/gpu/drm/i915/i915_trace.h |   13 +++++++++----
>   drivers/gpu/drm/i915/intel_lrc.c  |    2 ++
>   4 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1038f5c..0347eb9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1882,6 +1882,8 @@ struct drm_i915_private {
>
>   	bool edp_low_vswing;
>
> +	uint32_t request_uniq;
> +
>   	/*
>   	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
>   	 * will be rejected. Instead look for a better place.
> @@ -2160,6 +2162,8 @@ struct drm_i915_gem_request {
>   	/** process identifier submitting this request */
>   	struct pid *pid;
>
> +	uint32_t uniq;
> +
>   	/**
>   	 * The ELSP only accepts two elements at a time, so we queue
>   	 * context/tail pairs on a given queue (ring->execlist_queue) until the
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index cc206f1..68f1d1e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2657,6 +2657,7 @@ int i915_gem_request_alloc(struct intel_engine_cs *ring,
>   		goto err;
>
>   	req->ring = ring;
> +	req->uniq = dev_priv->request_uniq++;
>
>   	if (i915.enable_execlists)
>   		ret = intel_logical_ring_alloc_request_extras(req, ctx);
> diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
> index 497cba5..6cbc280 100644
> --- a/drivers/gpu/drm/i915/i915_trace.h
> +++ b/drivers/gpu/drm/i915/i915_trace.h
> @@ -504,6 +504,7 @@ DECLARE_EVENT_CLASS(i915_gem_request,
>   	    TP_STRUCT__entry(
>   			     __field(u32, dev)
>   			     __field(u32, ring)
> +			     __field(u32, uniq)
>   			     __field(u32, seqno)
>   			     ),
>
> @@ -512,11 +513,13 @@ DECLARE_EVENT_CLASS(i915_gem_request,
>   						i915_gem_request_get_ring(req);
>   			   __entry->dev = ring->dev->primary->index;
>   			   __entry->ring = ring->id;
> +			   __entry->uniq = req ? req->uniq : 0;
>   			   __entry->seqno = i915_gem_request_get_seqno(req);
>   			   ),
>
> -	    TP_printk("dev=%u, ring=%u, seqno=%u",
> -		      __entry->dev, __entry->ring, __entry->seqno)
> +	    TP_printk("dev=%u, ring=%u, uniq=%u, seqno=%u",
> +		      __entry->dev, __entry->ring, __entry->uniq,
> +		      __entry->seqno)
>   );
>
>   DEFINE_EVENT(i915_gem_request, i915_gem_request_add,
> @@ -561,6 +564,7 @@ TRACE_EVENT(i915_gem_request_wait_begin,
>   	    TP_STRUCT__entry(
>   			     __field(u32, dev)
>   			     __field(u32, ring)
> +			     __field(u32, uniq)
>   			     __field(u32, seqno)
>   			     __field(bool, blocking)
>   			     ),
> @@ -576,13 +580,14 @@ TRACE_EVENT(i915_gem_request_wait_begin,
>   						i915_gem_request_get_ring(req);
>   			   __entry->dev = ring->dev->primary->index;
>   			   __entry->ring = ring->id;
> +			   __entry->uniq = req ? req->uniq : 0;
>   			   __entry->seqno = i915_gem_request_get_seqno(req);
>   			   __entry->blocking =
>   				     mutex_is_locked(&ring->dev->struct_mutex);
>   			   ),
>
> -	    TP_printk("dev=%u, ring=%u, seqno=%u, blocking=%s",
> -		      __entry->dev, __entry->ring,
> +	    TP_printk("dev=%u, ring=%u, uniq=%u, seqno=%u, blocking=%s",
> +		      __entry->dev, __entry->ring, __entry->uniq,
>   		      __entry->seqno, __entry->blocking ?  "yes (NB)" : "no")
>   );
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 96ae90a..6a5ed07 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -549,6 +549,7 @@ static int execlists_context_queue(struct intel_engine_cs *ring,
>   				   struct drm_i915_gem_request *request)
>   {
>   	struct drm_i915_gem_request *cursor;
> +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
>   	int num_elements = 0;
>
>   	if (to != ring->default_context)
> @@ -565,6 +566,7 @@ static int execlists_context_queue(struct intel_engine_cs *ring,
>   		request->ring = ring;
>   		request->ctx = to;
>   		kref_init(&request->ref);
> +		request->uniq = dev_priv->request_uniq++;
>   		i915_gem_context_reference(request->ctx);
>   	} else {
>   		i915_gem_request_reference(request);
>


Reviewed-by: Tomas Elf <tomas.elf@intel.com>

Thanks,
Tomas

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx