Re: [PATCH 45/70] drm/i915: Remove request->uniq

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Apr 07, 2015 at 04:21:09PM +0100, Chris Wilson wrote:
> We already assign a unique identifier to every request: seqno. That
> someone felt like adding a second one without even mentioning why and
> tweaking ABI smells very fishy.
> 
> Fixes regression from
> commit b3a38998f042b862f5ba4d7f2268f3a8dfb4883a
> Author: Nick Hoath <nicholas.hoath@intel.com>
> Date:   Thu Feb 19 16:30:47 2015 +0000
> 
>     drm/i915: Fix a use after free, and unbalanced refcounting
> 
> v2: Rebase
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Nick Hoath <nicholas.hoath@intel.com>
> Cc: Thomas Daniel <thomas.daniel@intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@intel.com>

Queued for -next, thanks for the patch.
-Daniel
> ---
>  drivers/gpu/drm/i915/i915_drv.h   |  4 ----
>  drivers/gpu/drm/i915/i915_gem.c   |  1 -
>  drivers/gpu/drm/i915/i915_trace.h | 13 ++++---------
>  3 files changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 262ebb620112..89839751237c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1843,8 +1843,6 @@ struct drm_i915_private {
>  		void (*stop_ring)(struct intel_engine_cs *ring);
>  	} gt;
>  
> -	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.
> @@ -2120,8 +2118,6 @@ 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 c394c0d13eb7..e90894545fa4 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2660,7 +2660,6 @@ int i915_gem_request_alloc(struct intel_engine_cs *ring,
>  	}
>  
>  	rq->ring = ring;
> -	rq->uniq = dev_priv->request_uniq++;
>  
>  	if (i915.enable_execlists)
>  		ret = intel_logical_ring_alloc_request_extras(rq, ctx);
> diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
> index ce8ee9e8bced..6e2eee52aaa2 100644
> --- a/drivers/gpu/drm/i915/i915_trace.h
> +++ b/drivers/gpu/drm/i915/i915_trace.h
> @@ -499,7 +499,6 @@ DECLARE_EVENT_CLASS(i915_gem_request,
>  	    TP_STRUCT__entry(
>  			     __field(u32, dev)
>  			     __field(u32, ring)
> -			     __field(u32, uniq)
>  			     __field(u32, seqno)
>  			     ),
>  
> @@ -508,13 +507,11 @@ 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, uniq=%u, seqno=%u",
> -		      __entry->dev, __entry->ring, __entry->uniq,
> -		      __entry->seqno)
> +	    TP_printk("dev=%u, ring=%u, seqno=%u",
> +		      __entry->dev, __entry->ring, __entry->seqno)
>  );
>  
>  DEFINE_EVENT(i915_gem_request, i915_gem_request_add,
> @@ -559,7 +556,6 @@ 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)
>  			     ),
> @@ -575,14 +571,13 @@ 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, uniq=%u, seqno=%u, blocking=%s",
> -		      __entry->dev, __entry->ring, __entry->uniq,
> +	    TP_printk("dev=%u, ring=%u, seqno=%u, blocking=%s",
> +		      __entry->dev, __entry->ring,
>  		      __entry->seqno, __entry->blocking ?  "yes (NB)" : "no")
>  );
>  
> -- 
> 2.1.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx