[PATCH] drm, i915: Fix memory leak in i915_gem_busy_ioctl().
On Sat, Nov 26, 2011 at 4:57 PM, Chris Wilson wrote: > On Sat, 26 Nov 2011 10:44:17 +0600, Rakib Mullick gmail.com> wrote: >> On Mon, Nov 21, 2011 at 11:16 PM, Keith Packard wrote: >> > On Mon, 21 Nov 2011 17:23:06 +0100, Daniel Vetter >> > 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. > > kfree(NULL) is permitted and taken advantage of here along with the > short-circuiting behaviour of '||'. Yes, no real problem with current code. I was just thinking from code cleanup's pov. Is BUG_ON really needed in i915_add_request() ? thanks, Rakib
[PATCH] DRM: omapdrm DRM/KMS driver for TI OMAP platforms
On Fri, Nov 25, 2011 at 09:13:56PM +0100, Daniel Vetter wrote: > On Fri, Nov 25, 2011 at 01:14:00PM -0600, Rob Clark wrote: > > btw, Inki, Daniel, Konrad, and everyone who's reviewed this driver > > over the (what seems like) years, I'd appreciate r-b's or comments if > > you think there is anything remaining that should be done before first > > version is merged. I think it's been reviewed to death by now, and > > I'd *really* like to make sure it gets merged for 3.3 > > Honestly I've already looked at this too often and want to spare my review > bandwidth for when this moves out of staging. TODO.txt contains all the > things I think need to be fixed before that. So > > Highly-Recommended-for-Staging-by: Daniel Vetter I'll take that as an ack :) Now queued up. greg k-h
[PATCH] drm, i915: Fix memory leak in i915_gem_busy_ioctl().
On Sat, 26 Nov 2011 22:29:12 +0600, Rakib Mullick wrote: > Yes, no real problem with current code. I was just thinking from code > cleanup's pov. Is BUG_ON really needed in i915_add_request() ? No, just documentation as a reminder that the request should be preallocated, ideally so that we can fail gracefully without touching hardware and leaving it in an inconsistent state wrt to our bookkeeping. (This is more apparent in the overlay code which could hang the chip/driver if we hit a malloc error too late.) The BUG_ON has certainly outlived its usefulness. -Chris -- Chris Wilson, Intel Open Source Technology Centre
[Linaro-mm-sig] [RFC 1/2] dma-buf: Introduce dma buffer sharing mechanism
On Fri, Nov 25, 2011 at 17:28, Dave Airlie wrote: > I've rebuilt my PRIME interface on top of dmabuf to see how it would work, > > I've got primed gears running again on top, but I expect all my object > lifetime and memory ownership rules need fixing up (i.e. leaks like a > sieve). > > http://cgit.freedesktop.org/~airlied/linux/log/?h=drm-prime-dmabuf > > has the i915/nouveau patches for the kernel to produce the prime interface. I've noticed that your implementations for get_scatterlist (at least for the i915 driver) doesn't return the sg table mapped into the device address space. I've checked and the documentation makes it clear that this should be the case (and we really need this to support certain insane hw), but the get/put_scatterlist names are a bit misleading. Proposal: - use struct sg_table instead of scatterlist like you've already done in you branch. Simply more consistent with the dma api. - rename get/put_scatterlist into map/unmap for consistency with all the map/unmap dma api functions. The attachement would then serve as the abstract cookie to the backing storage, similar to how struct page * works as an abstract cookie for dma_map/unmap_page. The only special thing is that struct device * parameter because that's already part of the attachment. - add new wrapper functions dma_buf_map_attachment and dma_buf_unmap_attachement to hide all the pointer/vtable-chasing that we currently expose to users of this interface. Comments? Cheers, Daniel -- Daniel Vetter daniel.vetter at ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch
[PATCH] drm, i915: Fix memory leak in i915_gem_busy_ioctl().
On Sat, 26 Nov 2011 10:44:17 +0600, Rakib Mullick wrote: > On Mon, Nov 21, 2011 at 11:16 PM, Keith Packard wrote: > > On Mon, 21 Nov 2011 17:23:06 +0100, Daniel Vetter > > 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. kfree(NULL) is permitted and taken advantage of here along with the short-circuiting behaviour of '||'. -Chris -- Chris Wilson, Intel Open Source Technology Centre
[PATCH] drm, i915: Fix memory leak in i915_gem_busy_ioctl().
On Mon, Nov 21, 2011 at 11:16 PM, Keith Packard wrote: > On Mon, 21 Nov 2011 17:23:06 +0100, Daniel Vetter 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);
No subject
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, );
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(>request_list);
Thanks,
Rakib
[Bug 43248] Starting teeworlds results in black screen
https://bugs.freedesktop.org/show_bug.cgi?id=43248 --- Comment #2 from Sandeep 2011-11-25 16:02:45 PST --- This happens for OpenArena as well. Trine on the other hand works properly (on low detail) -- Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug.
Re: [PATCH] drm, i915: Fix memory leak in i915_gem_busy_ioctl().
On Sat, 26 Nov 2011 10:44:17 +0600, Rakib Mullick rakib.mull...@gmail.com wrote: On Mon, Nov 21, 2011 at 11:16 PM, Keith Packard kei...@keithp.com wrote: On Mon, 21 Nov 2011 17:23:06 +0100, Daniel Vetter dan...@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. kfree(NULL) is permitted and taken advantage of here along with the short-circuiting behaviour of '||'. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Linaro-mm-sig] [RFC 1/2] dma-buf: Introduce dma buffer sharing mechanism
On Fri, Nov 25, 2011 at 17:28, Dave Airlie airl...@gmail.com wrote: I've rebuilt my PRIME interface on top of dmabuf to see how it would work, I've got primed gears running again on top, but I expect all my object lifetime and memory ownership rules need fixing up (i.e. leaks like a sieve). http://cgit.freedesktop.org/~airlied/linux/log/?h=drm-prime-dmabuf has the i915/nouveau patches for the kernel to produce the prime interface. I've noticed that your implementations for get_scatterlist (at least for the i915 driver) doesn't return the sg table mapped into the device address space. I've checked and the documentation makes it clear that this should be the case (and we really need this to support certain insane hw), but the get/put_scatterlist names are a bit misleading. Proposal: - use struct sg_table instead of scatterlist like you've already done in you branch. Simply more consistent with the dma api. - rename get/put_scatterlist into map/unmap for consistency with all the map/unmap dma api functions. The attachement would then serve as the abstract cookie to the backing storage, similar to how struct page * works as an abstract cookie for dma_map/unmap_page. The only special thing is that struct device * parameter because that's already part of the attachment. - add new wrapper functions dma_buf_map_attachment and dma_buf_unmap_attachement to hide all the pointer/vtable-chasing that we currently expose to users of this interface. Comments? Cheers, Daniel -- Daniel Vetter daniel.vet...@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm, i915: Fix memory leak in i915_gem_busy_ioctl().
On Sat, Nov 26, 2011 at 4:57 PM, Chris Wilson ch...@chris-wilson.co.uk wrote: On Sat, 26 Nov 2011 10:44:17 +0600, Rakib Mullick rakib.mull...@gmail.com wrote: On Mon, Nov 21, 2011 at 11:16 PM, Keith Packard kei...@keithp.com wrote: On Mon, 21 Nov 2011 17:23:06 +0100, Daniel Vetter dan...@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. kfree(NULL) is permitted and taken advantage of here along with the short-circuiting behaviour of '||'. Yes, no real problem with current code. I was just thinking from code cleanup's pov. Is BUG_ON really needed in i915_add_request() ? thanks, Rakib ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm, i915: Fix memory leak in i915_gem_busy_ioctl().
On Sat, 26 Nov 2011 22:29:12 +0600, Rakib Mullick rakib.mull...@gmail.com wrote: Yes, no real problem with current code. I was just thinking from code cleanup's pov. Is BUG_ON really needed in i915_add_request() ? No, just documentation as a reminder that the request should be preallocated, ideally so that we can fail gracefully without touching hardware and leaving it in an inconsistent state wrt to our bookkeeping. (This is more apparent in the overlay code which could hang the chip/driver if we hit a malloc error too late.) The BUG_ON has certainly outlived its usefulness. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Linaro-mm-sig] [RFC 1/2] dma-buf: Introduce dma buffer sharing mechanism
On Sat, Nov 26, 2011 at 8:00 AM, Daniel Vetter dan...@ffwll.ch wrote: On Fri, Nov 25, 2011 at 17:28, Dave Airlie airl...@gmail.com wrote: I've rebuilt my PRIME interface on top of dmabuf to see how it would work, I've got primed gears running again on top, but I expect all my object lifetime and memory ownership rules need fixing up (i.e. leaks like a sieve). http://cgit.freedesktop.org/~airlied/linux/log/?h=drm-prime-dmabuf has the i915/nouveau patches for the kernel to produce the prime interface. I've noticed that your implementations for get_scatterlist (at least for the i915 driver) doesn't return the sg table mapped into the device address space. I've checked and the documentation makes it clear that this should be the case (and we really need this to support certain insane hw), but the get/put_scatterlist names are a bit misleading. Proposal: - use struct sg_table instead of scatterlist like you've already done in you branch. Simply more consistent with the dma api. yup - rename get/put_scatterlist into map/unmap for consistency with all the map/unmap dma api functions. The attachement would then serve as the abstract cookie to the backing storage, similar to how struct page * works as an abstract cookie for dma_map/unmap_page. The only special thing is that struct device * parameter because that's already part of the attachment. yup - add new wrapper functions dma_buf_map_attachment and dma_buf_unmap_attachement to hide all the pointer/vtable-chasing that we currently expose to users of this interface. I thought that was one of the earlier comments on the initial dmabuf patch, but either way: yup BR, -R Comments? Cheers, Daniel -- Daniel Vetter daniel.vet...@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
