[Intel-gfx] [PATCH 46/55] drm/i915: Update intel_ring_begin() to take a request structure

[Daniel Vetter]

On Tue, Jun 23, 2015 at 11:37:53AM +0100, John Harrison wrote:
> On 23/06/2015 11:24, Chris Wilson wrote:
> >On Fri, May 29, 2015 at 05:44:07PM +0100, John.C.Harrison at Intel.com wrote:
> >>-int intel_ring_begin(struct intel_engine_cs *ring,
> >>+int intel_ring_begin(struct drm_i915_gem_request *req,
> >>  		     int num_dwords)
> >>  {
> >>-	struct drm_i915_gem_request *req;
> >>-	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> >>+	struct intel_engine_cs *ring;
> >>+	struct drm_i915_private *dev_priv;
> >>  	int ret;
> >>+	WARN_ON(req == NULL);
> >>+	ring = req->ring;
> >What was the point?
> >-Chris
> >
> 
> The point is to remove the OLR. The significant change within
> intel_ring_begin is the next few lines:
> 
> -	/* Preallocate the olr before touching the ring */
> -	ret = i915_gem_request_alloc(ring, ring->default_context, &req);
> 
> 
> That is the part that causes problems by randomly creating a brand new
> request that no-one knows about and squirreling it away in the OLR to scoop
> up random bits of work. This is the whole point of the entire patch series -
> to ensure that all ring work is assigned to a known request by whoever
> instigated that work.

I think the point was that a WARN_ON(pointer) followed by an immediate
deref of said pointer doesn't add value. This is the one case where a
BUG_ON is the right joice. Or just let the kernel oops on the NULL deref
without warning first that it'll happen.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch