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@xxxxxxxxx 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
