| Message ID | 1399440098-17378-2-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 06, 2014 at 10:21:31PM -0700, Ben Widawsky wrote: > The DRM node allocation code was already a bit of an ugly bit of code > within a complex function. Removing it serves the purpose of cleaning > the function up. More importantly, it provides a way to have a > preallocated (address space) VMA to easily skip this code. Something > we're very likely to need. > > This is essentially a wrapper for DRM node allocation with an eviction + > retry. It is something which could be moved to core DRM eventually, if > other drivers had similar eviction semantics. > > This removes a goto used for something other than error unwinding, a > generally reviled (I am neutral) practice, and replaces it with a > function call to itself that should have the same effect. Note that it's > not a recursive function as all the problem set reduction is done in the > eviction code. > > Some might say this change is worse than before because we are using > stack for each subsequent call. Frankly, I'd rather overflow the stack > and blow it up than loop forever. In either case, this is addressed in > the next patch. > > I believe, and intend, that other than the stack usage, there is no > functional change here. Nope, but decoupling evict_something() further does introduce lots more implied magic. -Chris
On Wed, May 07, 2014 at 08:02:38AM +0100, Chris Wilson wrote: > On Tue, May 06, 2014 at 10:21:31PM -0700, Ben Widawsky wrote: > > The DRM node allocation code was already a bit of an ugly bit of code > > within a complex function. Removing it serves the purpose of cleaning > > the function up. More importantly, it provides a way to have a > > preallocated (address space) VMA to easily skip this code. Something > > we're very likely to need. > > > > This is essentially a wrapper for DRM node allocation with an eviction + > > retry. It is something which could be moved to core DRM eventually, if > > other drivers had similar eviction semantics. > > > > This removes a goto used for something other than error unwinding, a > > generally reviled (I am neutral) practice, and replaces it with a > > function call to itself that should have the same effect. Note that it's > > not a recursive function as all the problem set reduction is done in the > > eviction code. > > > > Some might say this change is worse than before because we are using > > stack for each subsequent call. Frankly, I'd rather overflow the stack > > and blow it up than loop forever. In either case, this is addressed in > > the next patch. > > > > I believe, and intend, that other than the stack usage, there is no > > functional change here. > > Nope, but decoupling evict_something() further does introduce lots more > implied magic. > -Chris > Hmm. I really don't see what's actually upsetting. Can you be a bit more explicit about what's so bothersome to you for my edification?
On Wed, May 07, 2014 at 08:45:38AM -0700, Ben Widawsky wrote: > On Wed, May 07, 2014 at 08:02:38AM +0100, Chris Wilson wrote: > > On Tue, May 06, 2014 at 10:21:31PM -0700, Ben Widawsky wrote: > > > The DRM node allocation code was already a bit of an ugly bit of code > > > within a complex function. Removing it serves the purpose of cleaning > > > the function up. More importantly, it provides a way to have a > > > preallocated (address space) VMA to easily skip this code. Something > > > we're very likely to need. > > > > > > This is essentially a wrapper for DRM node allocation with an eviction + > > > retry. It is something which could be moved to core DRM eventually, if > > > other drivers had similar eviction semantics. > > > > > > This removes a goto used for something other than error unwinding, a > > > generally reviled (I am neutral) practice, and replaces it with a > > > function call to itself that should have the same effect. Note that it's > > > not a recursive function as all the problem set reduction is done in the > > > eviction code. > > > > > > Some might say this change is worse than before because we are using > > > stack for each subsequent call. Frankly, I'd rather overflow the stack > > > and blow it up than loop forever. In either case, this is addressed in > > > the next patch. > > > > > > I believe, and intend, that other than the stack usage, there is no > > > functional change here. > > > > Nope, but decoupling evict_something() further does introduce lots more > > implied magic. > > -Chris > > > > Hmm. I really don't see what's actually upsetting. Can you be a bit more > explicit about what's so bothersome to you for my edification? evict_something() make assumptions about the ranges of the vm which it searches. At the moment, they mirror its parent's function. -Chris
On Wed, May 07, 2014 at 04:53:08PM +0100, Chris Wilson wrote: > On Wed, May 07, 2014 at 08:45:38AM -0700, Ben Widawsky wrote: > > On Wed, May 07, 2014 at 08:02:38AM +0100, Chris Wilson wrote: > > > On Tue, May 06, 2014 at 10:21:31PM -0700, Ben Widawsky wrote: > > > > The DRM node allocation code was already a bit of an ugly bit of code > > > > within a complex function. Removing it serves the purpose of cleaning > > > > the function up. More importantly, it provides a way to have a > > > > preallocated (address space) VMA to easily skip this code. Something > > > > we're very likely to need. > > > > > > > > This is essentially a wrapper for DRM node allocation with an eviction + > > > > retry. It is something which could be moved to core DRM eventually, if > > > > other drivers had similar eviction semantics. > > > > > > > > This removes a goto used for something other than error unwinding, a > > > > generally reviled (I am neutral) practice, and replaces it with a > > > > function call to itself that should have the same effect. Note that it's > > > > not a recursive function as all the problem set reduction is done in the > > > > eviction code. > > > > > > > > Some might say this change is worse than before because we are using > > > > stack for each subsequent call. Frankly, I'd rather overflow the stack > > > > and blow it up than loop forever. In either case, this is addressed in > > > > the next patch. > > > > > > > > I believe, and intend, that other than the stack usage, there is no > > > > functional change here. > > > > > > Nope, but decoupling evict_something() further does introduce lots more > > > implied magic. > > > -Chris > > > > > > > Hmm. I really don't see what's actually upsetting. Can you be a bit more > > explicit about what's so bothersome to you for my edification? > > evict_something() make assumptions about the ranges of the vm which it > searches. At the moment, they mirror its parent's function. > -Chris > Ah, thanks. So is plumbing starting eviction offset into evict something an appealing solution here? > -- > Chris Wilson, Intel Open Source Technology Centre
On Wed, May 07, 2014 at 09:00:16AM -0700, Ben Widawsky wrote: > On Wed, May 07, 2014 at 04:53:08PM +0100, Chris Wilson wrote: > > On Wed, May 07, 2014 at 08:45:38AM -0700, Ben Widawsky wrote: > > > Hmm. I really don't see what's actually upsetting. Can you be a bit more > > > explicit about what's so bothersome to you for my edification? > > > > evict_something() make assumptions about the ranges of the vm which it > > searches. At the moment, they mirror its parent's function. > > Ah, thanks. So is plumbing starting eviction offset into evict something an > appealing solution here? Yes. I have to admit to not being overly sold on the code migration yet. I guess you have an ulterior motive... Evictable vm? -Chris
On Wed, May 07, 2014 at 05:55:00PM +0100, Chris Wilson wrote: > On Wed, May 07, 2014 at 09:00:16AM -0700, Ben Widawsky wrote: > > On Wed, May 07, 2014 at 04:53:08PM +0100, Chris Wilson wrote: > > > On Wed, May 07, 2014 at 08:45:38AM -0700, Ben Widawsky wrote: > > > > Hmm. I really don't see what's actually upsetting. Can you be a bit more > > > > explicit about what's so bothersome to you for my edification? > > > > > > evict_something() make assumptions about the ranges of the vm which it > > > searches. At the moment, they mirror its parent's function. > > > > Ah, thanks. So is plumbing starting eviction offset into evict something an > > appealing solution here? > > Yes. I have to admit to not being overly sold on the code migration yet. > I guess you have an ulterior motive... Evictable vm? > -Chris > The only immediate goal is to be able to get this bit logic out of bind_to_vm, so I can use "preallocated" nodes a bit more cleanly. At least for the present, I have no plans with it other than that. I did like the resuse of the PDE allocation for gen7. If I think of a better reason, I'll let you know.
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index dae51c3..2a07fa1 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3216,6 +3216,34 @@ static void i915_gem_verify_gtt(struct drm_device *dev) #endif } +static int +i915_gem_find_vm_space(struct i915_address_space *vm, + struct drm_mm_node *node, + unsigned long size, + unsigned long align, + unsigned long color, + unsigned long start, + unsigned long end, + uint32_t flags) +{ + int ret; + ret = drm_mm_insert_node_in_range_generic(&vm->mm, node, + size, align, color, + start, end, + DRM_MM_SEARCH_DEFAULT, + DRM_MM_CREATE_DEFAULT); + if (ret) { + ret = i915_gem_evict_something(vm->dev, vm, size, align, color, + flags); + if (ret == 0) + return i915_gem_find_vm_space(vm, node, + size, align, color, + start, end, flags); + } + + return ret; +} + /** * Finds free space in the GTT aperture and binds the object there. */ @@ -3275,20 +3303,11 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj, if (IS_ERR(vma)) goto err_unpin; -search_free: - ret = drm_mm_insert_node_in_range_generic(&vm->mm, &vma->node, - size, alignment, - obj->cache_level, 0, gtt_max, - DRM_MM_SEARCH_DEFAULT, - DRM_MM_CREATE_DEFAULT); - if (ret) { - ret = i915_gem_evict_something(dev, vm, size, alignment, - obj->cache_level, flags); - if (ret == 0) - goto search_free; - + ret = i915_gem_find_vm_space(vm, &vma->node, size, alignment, + obj->cache_level, 0, gtt_max, flags); + if (ret) goto err_free_vma; - } + if (WARN_ON(!i915_gem_valid_gtt_space(dev, &vma->node, obj->cache_level))) { ret = -EINVAL;
The DRM node allocation code was already a bit of an ugly bit of code within a complex function. Removing it serves the purpose of cleaning the function up. More importantly, it provides a way to have a preallocated (address space) VMA to easily skip this code. Something we're very likely to need. This is essentially a wrapper for DRM node allocation with an eviction + retry. It is something which could be moved to core DRM eventually, if other drivers had similar eviction semantics. This removes a goto used for something other than error unwinding, a generally reviled (I am neutral) practice, and replaces it with a function call to itself that should have the same effect. Note that it's not a recursive function as all the problem set reduction is done in the eviction code. Some might say this change is worse than before because we are using stack for each subsequent call. Frankly, I'd rather overflow the stack and blow it up than loop forever. In either case, this is addressed in the next patch. I believe, and intend, that other than the stack usage, there is no functional change here. Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_gem.c | 45 +++++++++++++++++++++++++++++------------ 1 file changed, 32 insertions(+), 13 deletions(-)