[Intel-gfx] [PATCH] drm/i915: Defer adding preallocated stolen objects to the VM list

[Jani Nikula]

On Thu, 24 Sep 2015, "Winiarski, Michal" <michal.winiarski at intel.com> wrote:
> On Thu, 2015-09-24 at 11:57 +0100, Chris Wilson wrote:
>> When preallocating a stolen object during early initialisation, we
>> may
>> be running before we have setup the the global GTT VM state, in
>> particular before we have initialised the range manager and
>> associated
>> lists. As this is the case, we defer binding the stolen object until
>> we
>> call i915_gem_setup_global_gtt(). Not only should we defer the
>> binding,
>> but we should also defer the VM list manipulation.
>> 
>> Fixes regression uncovered by commit
>> a2cad9dff4dd44d0244b966d980de9d602d87593
>> Author: Michał Winiarski <michal.winiarski at intel.com>
>> Date:   Wed Sep 16 11:49:00 2015 +0200
>> 
>>     drm/i915/gtt: Do not initialize drm_mm twice.

I confirm reverting the above works.

This patch is

Tested-by: Jani Nikula <jani.nikula at intel.com>

on a BWD GT2 ULT.

>> 
>> Whilst I am here remove the duplicate work leaving dangling pointers
>> from the error path...
>> 
>> v2: Typos galore before coffee.
>> 
>> Reported-by: Jesse Barnes <jbarnes at virtuousgeek.org>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92099
>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Jesse Barnes <jbarnes at virtuousgeek.org>
>> Cc: Michel Thierry <michel.thierry at intel.com>
>> Cc: Mika Kuoppala <mika.kuoppala at intel.com>
>> Cc: Michał Winiarski <michal.winiarski at intel.com>
>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>> Cc: Jani Nikula <jani.nikula at linux.intel.com>
>
> Reviewed-by: Michał Winiarski <michal.winiarski at intel.com>
>
> -Michał
>
>> ---
>>  drivers/gpu/drm/i915/i915_gem_gtt.c    |  2 +-
>>  drivers/gpu/drm/i915/i915_gem_stolen.c | 16 ++++++----------
>>  2 files changed, 7 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index 01f3521e77d3..291305fb2f3c 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -2533,7 +2533,6 @@ static int ggtt_bind_vma(struct i915_vma *vma,
>>  		 * the bound flag ourselves.
>>  		 */
>>  		vma->bound |= GLOBAL_BIND;
>> -
>>  	}
>>  
>>  	if (dev_priv->mm.aliasing_ppgtt && flags & LOCAL_BIND) {
>> @@ -2657,6 +2656,7 @@ static int i915_gem_setup_global_gtt(struct
>> drm_device *dev,
>>  			return ret;
>>  		}
>>  		vma->bound |= GLOBAL_BIND;
>> +		list_add_tail(&vma->mm_list, &ggtt_vm
>> ->inactive_list);
>>  	}
>>  
>>  	/* Clear any non-preallocated blocks */
>> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
>> b/drivers/gpu/drm/i915/i915_gem_stolen.c
>> index 55df6ce34751..15207796e1b3 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
>> @@ -584,7 +584,7 @@
>> i915_gem_object_create_stolen_for_preallocated(struct drm_device
>> *dev,
>>  	vma = i915_gem_obj_lookup_or_create_vma(obj, ggtt);
>>  	if (IS_ERR(vma)) {
>>  		ret = PTR_ERR(vma);
>> -		goto err_out;
>> +		goto err;
>>  	}
>>  
>>  	/* To simplify the initialisation sequence between KMS and
>> GTT,
>> @@ -598,23 +598,19 @@
>> i915_gem_object_create_stolen_for_preallocated(struct drm_device
>> *dev,
>>  		ret = drm_mm_reserve_node(&ggtt->mm, &vma->node);
>>  		if (ret) {
>>  			DRM_DEBUG_KMS("failed to allocate stolen GTT
>> space\n");
>> -			goto err_vma;
>> +			goto err;
>>  		}
>> -	}
>>  
>> -	vma->bound |= GLOBAL_BIND;
>> +		vma->bound |= GLOBAL_BIND;
>> +		list_add_tail(&vma->mm_list, &ggtt->inactive_list);
>> +	}
>>  
>>  	list_add_tail(&obj->global_list, &dev_priv->mm.bound_list);
>> -	list_add_tail(&vma->mm_list, &ggtt->inactive_list);
>>  	i915_gem_object_pin_pages(obj);
>>  
>>  	return obj;
>>  
>> -err_vma:
>> -	i915_gem_vma_destroy(vma);
>> -err_out:
>> -	i915_gem_stolen_remove_node(dev_priv, stolen);
>> -	kfree(stolen);
>> +err:
>>  	drm_gem_object_unreference(&obj->base);
>>  	return NULL;
>>  }

-- 
Jani Nikula, Intel Open Source Technology Center