Re: [PATCH v4 2/2] drm/i915: Make GuC GGTT reservation work on ggtt

["Michal Wajdeczko" <michal.wajdeczko@intel.com>]

On Tue, 11 Jun 2019 15:13:21 +0200, Tvrtko Ursulin  
<tvrtko.ursulin@linux.intel.com> wrote:

>
> Michal,
>
> On 11/06/2019 13:23, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>  These functions operate on ggtt so make them take that directly as
>> parameter.
>>  At the same time move the USES_GUC conditional down to
>> intel_guc_reserve_ggtt_top for symmetry with
>> intel_guc_reserved_gtt_size.
>>  v2:
>>   * Rename and move functions to be static in i915_gem_gtt.c (Michal)
>>  v3:
>>   * Add comment explaining reason for reservation, add assert and fix
>>     error message. (Michal)
>>  v4:
>>   * Fix checkpatch error.
>>  Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>
> I've assumed I would be able to implement your small suggestions  
> correctly so I've kept the r-b. Could you just give it a quick glance  
> over to see if that is indeed true.

It's ok, r-b is valid.

Michal

>
> Regards,
>
> Tvrtko
>
>> ---
>>   drivers/gpu/drm/i915/i915_gem_gtt.c | 43 ++++++++++++++++++++++++-----
>>   drivers/gpu/drm/i915/intel_guc.c    | 27 ------------------
>>   drivers/gpu/drm/i915/intel_guc.h    |  2 --
>>   3 files changed, 36 insertions(+), 36 deletions(-)
>>  diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c  
>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index 5b5125ee49f3..c13b52b66ef1 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -2807,6 +2807,32 @@ static void fini_aliasing_ppgtt(struct  
>> drm_i915_private *i915)
>>   	ggtt->vm.vma_ops.unbind_vma = ggtt_unbind_vma;
>>   }
>>   +static int ggtt_reserve_guc_top(struct i915_ggtt *ggtt)
>> +{
>> +	u64 size;
>> +	int ret;
>> +
>> +	if (!USES_GUC(ggtt->vm.i915))
>> +		return 0;
>> +
>> +	GEM_BUG_ON(ggtt->vm.total <= GUC_GGTT_TOP);
>> +	size = ggtt->vm.total - GUC_GGTT_TOP;
>> +
>> +	ret = i915_gem_gtt_reserve(&ggtt->vm, &ggtt->uc_fw, size,
>> +				   GUC_GGTT_TOP, I915_COLOR_UNEVICTABLE,
>> +				   PIN_NOEVICT);
>> +	if (ret)
>> +		DRM_DEBUG_DRIVER("Failed to reserve top of GGTT for GuC\n");
>> +
>> +	return ret;
>> +}
>> +
>> +static void ggtt_release_guc_top(struct i915_ggtt *ggtt)
>> +{
>> +	if (drm_mm_node_allocated(&ggtt->uc_fw))
>> +		drm_mm_remove_node(&ggtt->uc_fw);
>> +}
>> +
>>   int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
>>   {
>>   	/* Let GEM Manage all of the aperture.
>> @@ -2844,11 +2870,14 @@ int i915_gem_init_ggtt(struct drm_i915_private  
>> *dev_priv)
>>   	if (ret)
>>   		return ret;
>>   -	if (USES_GUC(dev_priv)) {
>> -		ret = intel_guc_reserve_ggtt_top(&dev_priv->guc);
>> -		if (ret)
>> -			goto err_reserve;
>> -	}
>> +	/*
>> +	 * The upper portion of the GuC address space has a sizeable hole
>> +	 * (several MB) that is inaccessible by GuC. Reserve this range within
>> +	 * GGTT as it can comfortably hold GuC/HuC firmware images.
>> +	 */
>> +	ret = ggtt_reserve_guc_top(ggtt);
>> +	if (ret)
>> +		goto err_reserve;
>>     	/* Clear any non-preallocated blocks */
>>   	drm_mm_for_each_hole(entry, &ggtt->vm.mm, hole_start, hole_end) {
>> @@ -2870,7 +2899,7 @@ int i915_gem_init_ggtt(struct drm_i915_private  
>> *dev_priv)
>>   	return 0;
>>     err_appgtt:
>> -	intel_guc_release_ggtt_top(&dev_priv->guc);
>> +	ggtt_release_guc_top(ggtt);
>>   err_reserve:
>>   	drm_mm_remove_node(&ggtt->error_capture);
>>   	return ret;
>> @@ -2897,7 +2926,7 @@ void i915_ggtt_cleanup_hw(struct drm_i915_private  
>> *dev_priv)
>>   	if (drm_mm_node_allocated(&ggtt->error_capture))
>>   		drm_mm_remove_node(&ggtt->error_capture);
>>   -	intel_guc_release_ggtt_top(&dev_priv->guc);
>> +	ggtt_release_guc_top(ggtt);
>>     	if (drm_mm_initialized(&ggtt->vm.mm)) {
>>   		intel_vgt_deballoon(dev_priv);
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index d45d97624402..c40a6efdd33a 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -685,30 +685,3 @@ struct i915_vma *intel_guc_allocate_vma(struct  
>> intel_guc *guc, u32 size)
>>   	i915_gem_object_put(obj);
>>   	return vma;
>>   }
>> -
>> -int intel_guc_reserve_ggtt_top(struct intel_guc *guc)
>> -{
>> -	struct drm_i915_private *i915 = guc_to_i915(guc);
>> -	struct i915_ggtt *ggtt = &i915->ggtt;
>> -	u64 size;
>> -	int ret;
>> -
>> -	size = ggtt->vm.total - GUC_GGTT_TOP;
>> -
>> -	ret = i915_gem_gtt_reserve(&ggtt->vm, &ggtt->uc_fw, size,
>> -				   GUC_GGTT_TOP, I915_COLOR_UNEVICTABLE,
>> -				   PIN_NOEVICT);
>> -	if (ret)
>> -		DRM_DEBUG_DRIVER("GuC: failed to reserve top of ggtt\n");
>> -
>> -	return ret;
>> -}
>> -
>> -void intel_guc_release_ggtt_top(struct intel_guc *guc)
>> -{
>> -	struct drm_i915_private *i915 = guc_to_i915(guc);
>> -	struct i915_ggtt *ggtt = &i915->ggtt;
>> -
>> -	if (drm_mm_node_allocated(&ggtt->uc_fw))
>> -		drm_mm_remove_node(&ggtt->uc_fw);
>> -}
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h  
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index 85c3b02a0c08..08c906abdfa2 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -172,8 +172,6 @@ int intel_guc_auth_huc(struct intel_guc *guc, u32  
>> rsa_offset);
>>   int intel_guc_suspend(struct intel_guc *guc);
>>   int intel_guc_resume(struct intel_guc *guc);
>>   struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32  
>> size);
>> -int intel_guc_reserve_ggtt_top(struct intel_guc *guc);
>> -void intel_guc_release_ggtt_top(struct intel_guc *guc);
>>     static inline bool intel_guc_is_loaded(struct intel_guc *guc)
>>   {
>>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx