On Mon, Nov 21, 2011 at 11:16 PM, Keith Packard keithp@keithp.com wrote:
On Mon, 21 Nov 2011 17:23:06 +0100, Daniel Vetter daniel@ffwll.ch wrote:
Indeed, nice catch (albeit totally unlikely to be hit, because the error only happens when the gpu ceases to progress in the ring, so imo not stable material). Keith, please pick this up for fixes, thanks.
It's already there and queued for airlied :-)
Thank you guys for reviewing and taking the patch.
Now, while I was looking at the uses of i915_add_request(), I found the following code :
ret = i915_gem_flush_ring(ring, 0, I915_GEM_GPU_DOMAINS); request = kzalloc(sizeof(*request), GFP_KERNEL); if (ret || request == NULL || i915_add_request(ring, NULL, request)) kfree(request);
From above code, we might ended up by calling kfree(request) without
allocating request. Though, it's unlikely cause if request is NULL then BUG_ON will be hit in i915_add_request(). So, to unify the callee uses of i915_add_request(), I'm proposing the following patch. Please let me know what do you guys think. If you guys agree, I can sent a formal patch.
Index: work/modi.linux-3.1.1/drivers/gpu/drm/i915/i915_gem.c =================================================================== --- work.orig/modi.linux-3.1.1/drivers/gpu/drm/i915/i915_gem.c +++ work/modi.linux-3.1.1/drivers/gpu/drm/i915/i915_gem.c @@ -1736,8 +1736,6 @@ i915_add_request(struct intel_ring_buffe int was_empty; int ret;
- BUG_ON(request == NULL); - ret = ring->add_request(ring, &seqno); if (ret) return ret; @@ -2002,9 +2000,11 @@ i915_gem_retire_work_handler(struct work ret = i915_gem_flush_ring(ring, 0, I915_GEM_GPU_DOMAINS); request = kzalloc(sizeof(*request), GFP_KERNEL); - if (ret || request == NULL || - i915_add_request(ring, NULL, request)) - kfree(request); + if (request) { + ret = i915_add_request(ring, NULL, request); + if (ret) + kfree(request); + } }
idle &= list_empty(&ring->request_list);
Thanks, Rakib