On 23/06/2015 14:25, Daniel Vetter wrote:
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
I thought the edict from yourself was that BUG_ONs should never be used,
it should always be a WARN_ON and if the driver oopses afterwards then
so be it. The WARN_ON does add value in that it gives you a line number
(and file) which in turn gives you exactly what variable was null.
Whereas the oops just gives you an offset into a function. Unless you
happen to have the exact same binary that generated the bug report, the
chances of identifying what was null can be slim.
John.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
