[20/21] drm/i915: Make GuC GGTT reservation work on ggtt

Message ID	20190606093639.9372-21-tvrtko.ursulin@linux.intel.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <intel-gfx-bounces@lists.freedesktop.org> From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> To: Intel-gfx@lists.freedesktop.org Date: Thu, 6 Jun 2019 10:36:38 +0100 Message-Id: <20190606093639.9372-21-tvrtko.ursulin@linux.intel.com> In-Reply-To: <20190606093639.9372-1-tvrtko.ursulin@linux.intel.com> References: <20190606093639.9372-1-tvrtko.ursulin@linux.intel.com> MIME-Version: 1.0 Subject: [Intel-gfx] [PATCH 20/21] drm/i915: Make GuC GGTT reservation work on ggtt Precedence: list Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" <intel-gfx-bounces@lists.freedesktop.org>
Series	Implicit dev_priv removal \| expand [00/21] Implicit dev_priv removal [01/21] drm/i915: Reset only affected engines when handling error capture [02/21] drm/i915: Tidy engine mask types in hangcheck [03/21] drm/i915: Make Gen6/7 RING_FAULT_REG access engine centric [04/21] drm/i915: Extract engine fault reset to a helper [05/21] drm/i915: Make i915_clear_error_registers take uncore [06/21] drm/i915: Convert some more bits to use engine mmio accessors [07/21] drm/i915: Make read_subslice_reg take uncore [08/21] drm/i915: Tidy intel_execlists_submission_init [09/21] drm/i915: Make i915_check_and_clear_faults take uncore [10/21] drm/i915: Move scheduler caps init to i915_gem_init [11/21] drm/i915: Remove impossible path from i915_gem_init_swizzling [12/21] drm/i915: Convert i915_gem_init_swizzling to uncore [13/21] drm/i915: Convert init_unused_rings to uncore [14/21] drm/i915: Convert gt workarounds to uncore [15/21] drm/i915: Convert intel_mocs_init_l3cc_table to uncore [16/21] drm/i915: Convert i915_ppgtt_init_hw to uncore [17/21] drm/i915: Consolidate some open coded mmio rmw [18/21] drm/i915: Convert i915_gem_init_hw to uncore [19/21] drm/i915: Convert intel_vgt_(de)balloon to uncore [20/21] drm/i915: Make GuC GGTT reservation work on ggtt [21/21] drm/i915: Unexport i915_gem_init/fini_aliasing_ppgtt

Message ID

20190606093639.9372-21-tvrtko.ursulin@linux.intel.com (mailing list archive)

State

New, archived

Headers

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Intel-gfx@lists.freedesktop.org
Date: Thu,  6 Jun 2019 10:36:38 +0100
Message-Id: <20190606093639.9372-21-tvrtko.ursulin@linux.intel.com>
In-Reply-To: <20190606093639.9372-1-tvrtko.ursulin@linux.intel.com>
References: <20190606093639.9372-1-tvrtko.ursulin@linux.intel.com>
MIME-Version: 1.0
Subject: [Intel-gfx] [PATCH 20/21] drm/i915: Make GuC GGTT reservation work
 on ggtt
Precedence: list
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: base64
Errors-To: intel-gfx-bounces@lists.freedesktop.org
Sender: "Intel-gfx" <intel-gfx-bounces@lists.freedesktop.org>

Series

Implicit dev_priv removal | expand

Commit Message

Tvrtko Ursulin June 6, 2019, 9:36 a.m. UTC

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.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 14 ++++++--------
 drivers/gpu/drm/i915/intel_guc.c    | 18 ++++++++----------
 drivers/gpu/drm/i915/intel_guc.h    |  6 +++---
 3 files changed, 17 insertions(+), 21 deletions(-)

Comments

Michal Wajdeczko June 6, 2019, 11:58 a.m. UTC | #1

On Thu, 06 Jun 2019 11:36:38 +0200, Tvrtko Ursulin  
<tvrtko.ursulin@linux.intel.com> wrote:

> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> These functions operate on ggtt so make them take that directly as
> parameter.

Not quite.

Function intel_guc_reserved_gtt_size() operates on struct intel_guc
and is defined in intel_guc.c for proper layering, so NAK here.

For other two intel_guc_reserve|release_ggtt_top() I would rather:
- rename them to i915_ggtt_reserve|release_guc_top(),
- move to i915_gem_ggtt.c
- make them static

Michal

>
> At the same time move the USES_GUC conditional down to
> intel_guc_reserve_ggtt_top for symmetry with
> intel_guc_reserved_gtt_size.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 14 ++++++--------
>  drivers/gpu/drm/i915/intel_guc.c    | 18 ++++++++----------
>  drivers/gpu/drm/i915/intel_guc.h    |  6 +++---
>  3 files changed, 17 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c  
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index d3b3676d10f3..d967a4e9ceb0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2912,7 +2912,7 @@ int i915_gem_init_ggtt(struct drm_i915_private  
> *dev_priv)
>  	 * why.
>  	 */
>  	ggtt->pin_bias = max_t(u32, I915_GTT_PAGE_SIZE,
> -			       intel_guc_reserved_gtt_size(&dev_priv->guc));
> +			       intel_guc_reserved_gtt_size(ggtt));
> 	ret = intel_vgt_balloon(ggtt);
>  	if (ret)
> @@ -2926,11 +2926,9 @@ 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;
> -	}
> +	ret = intel_guc_reserve_ggtt_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) {
> @@ -2952,7 +2950,7 @@ int i915_gem_init_ggtt(struct drm_i915_private  
> *dev_priv)
>  	return 0;
> err_appgtt:
> -	intel_guc_release_ggtt_top(&dev_priv->guc);
> +	intel_guc_release_ggtt_top(ggtt);
>  err_reserve:
>  	drm_mm_remove_node(&ggtt->error_capture);
>  	return ret;
> @@ -2979,7 +2977,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);
> +	intel_guc_release_ggtt_top(ggtt);
> 	if (drm_mm_initialized(&ggtt->vm.mm)) {
>  		intel_vgt_deballoon(ggtt);
> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
> b/drivers/gpu/drm/i915/intel_guc.c
> index b88c349c4fa6..633248b7da25 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -719,7 +719,7 @@ struct i915_vma *intel_guc_allocate_vma(struct  
> intel_guc *guc, u32 size)
> /**
>   * intel_guc_reserved_gtt_size()
> - * @guc:	intel_guc structure
> + * @ggtt:	Pointer to struct i915_ggtt
>   *
>   * The GuC WOPCM mapping shadows the lower part of the GGTT, so if we  
> are using
>   * GuC we can't have any objects pinned in that region. This function  
> returns
> @@ -729,18 +729,19 @@ struct i915_vma *intel_guc_allocate_vma(struct  
> intel_guc *guc, u32 size)
>   * 0 if GuC is not present or not in use.
>   * Otherwise, the GuC WOPCM size.
>   */
> -u32 intel_guc_reserved_gtt_size(struct intel_guc *guc)
> +u32 intel_guc_reserved_gtt_size(struct i915_ggtt *ggtt)
>  {
> -	return guc_to_i915(guc)->wopcm.guc.size;
> +	return ggtt->vm.i915->wopcm.guc.size;
>  }
> -int intel_guc_reserve_ggtt_top(struct intel_guc *guc)
> +int intel_guc_reserve_ggtt_top(struct i915_ggtt *ggtt)
>  {
> -	struct drm_i915_private *i915 = guc_to_i915(guc);
> -	struct i915_ggtt *ggtt = &i915->ggtt;
>  	u64 size;
>  	int ret;
> +	if (!USES_GUC(ggtt->vm.i915))
> +		return 0;
> +
>  	size = ggtt->vm.total - GUC_GGTT_TOP;
> 	ret = i915_gem_gtt_reserve(&ggtt->vm, &ggtt->uc_fw, size,
> @@ -752,11 +753,8 @@ int intel_guc_reserve_ggtt_top(struct intel_guc  
> *guc)
>  	return ret;
>  }
> -void intel_guc_release_ggtt_top(struct intel_guc *guc)
> +void intel_guc_release_ggtt_top(struct i915_ggtt *ggtt)
>  {
> -	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 cbfed7a77c8b..55ea14176c5e 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -173,9 +173,9 @@ 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);
> -u32 intel_guc_reserved_gtt_size(struct intel_guc *guc);
> -int intel_guc_reserve_ggtt_top(struct intel_guc *guc);
> -void intel_guc_release_ggtt_top(struct intel_guc *guc);
> +u32 intel_guc_reserved_gtt_size(struct i915_ggtt *ggtt);
> +int intel_guc_reserve_ggtt_top(struct i915_ggtt *ggtt);
> +void intel_guc_release_ggtt_top(struct i915_ggtt *ggtt);
> static inline bool intel_guc_is_loaded(struct intel_guc *guc)
>  {

Tvrtko Ursulin June 6, 2019, 12:23 p.m. UTC | #2

On 06/06/2019 12:58, Michal Wajdeczko wrote:
> On Thu, 06 Jun 2019 11:36:38 +0200, Tvrtko Ursulin 
> <tvrtko.ursulin@linux.intel.com> wrote:
> 
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> These functions operate on ggtt so make them take that directly as
>> parameter.
> 
> Not quite.
> 
> Function intel_guc_reserved_gtt_size() operates on struct intel_guc
> and is defined in intel_guc.c for proper layering, so NAK here.

But it doesn't really. It operates on intel_wopcm if we want to be true.

u32 intel_guc_reserved_gtt_size(struct i915_ggtt *ggtt)
{
	return ggtt->vm.i915->wopcm.guc.size;
}

And GuC portion it needs is just one part of struct intel_wopcm - so 
whether or not this function must live in intel_guc.c and so how hard we 
should see this as layering violation is not so clear in my opinion.

Unless it will need to actually use intel_guc in the future.

> For other two intel_guc_reserve|release_ggtt_top() I would rather:
> - rename them to i915_ggtt_reserve|release_guc_top(),
> - move to i915_gem_ggtt.c
> - make them static

Fair, will do.

Regards,

Tvrtko

> Michal
> 
>>
>> At the same time move the USES_GUC conditional down to
>> intel_guc_reserve_ggtt_top for symmetry with
>> intel_guc_reserved_gtt_size.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_gem_gtt.c | 14 ++++++--------
>>  drivers/gpu/drm/i915/intel_guc.c    | 18 ++++++++----------
>>  drivers/gpu/drm/i915/intel_guc.h    |  6 +++---
>>  3 files changed, 17 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index d3b3676d10f3..d967a4e9ceb0 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -2912,7 +2912,7 @@ int i915_gem_init_ggtt(struct drm_i915_private 
>> *dev_priv)
>>       * why.
>>       */
>>      ggtt->pin_bias = max_t(u32, I915_GTT_PAGE_SIZE,
>> -                   intel_guc_reserved_gtt_size(&dev_priv->guc));
>> +                   intel_guc_reserved_gtt_size(ggtt));
>>     ret = intel_vgt_balloon(ggtt);
>>      if (ret)
>> @@ -2926,11 +2926,9 @@ 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;
>> -    }
>> +    ret = intel_guc_reserve_ggtt_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) {
>> @@ -2952,7 +2950,7 @@ int i915_gem_init_ggtt(struct drm_i915_private 
>> *dev_priv)
>>      return 0;
>> err_appgtt:
>> -    intel_guc_release_ggtt_top(&dev_priv->guc);
>> +    intel_guc_release_ggtt_top(ggtt);
>>  err_reserve:
>>      drm_mm_remove_node(&ggtt->error_capture);
>>      return ret;
>> @@ -2979,7 +2977,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);
>> +    intel_guc_release_ggtt_top(ggtt);
>>     if (drm_mm_initialized(&ggtt->vm.mm)) {
>>          intel_vgt_deballoon(ggtt);
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c 
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index b88c349c4fa6..633248b7da25 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -719,7 +719,7 @@ struct i915_vma *intel_guc_allocate_vma(struct 
>> intel_guc *guc, u32 size)
>> /**
>>   * intel_guc_reserved_gtt_size()
>> - * @guc:    intel_guc structure
>> + * @ggtt:    Pointer to struct i915_ggtt
>>   *
>>   * The GuC WOPCM mapping shadows the lower part of the GGTT, so if we 
>> are using
>>   * GuC we can't have any objects pinned in that region. This function 
>> returns
>> @@ -729,18 +729,19 @@ struct i915_vma *intel_guc_allocate_vma(struct 
>> intel_guc *guc, u32 size)
>>   * 0 if GuC is not present or not in use.
>>   * Otherwise, the GuC WOPCM size.
>>   */
>> -u32 intel_guc_reserved_gtt_size(struct intel_guc *guc)
>> +u32 intel_guc_reserved_gtt_size(struct i915_ggtt *ggtt)
>>  {
>> -    return guc_to_i915(guc)->wopcm.guc.size;
>> +    return ggtt->vm.i915->wopcm.guc.size;
>>  }
>> -int intel_guc_reserve_ggtt_top(struct intel_guc *guc)
>> +int intel_guc_reserve_ggtt_top(struct i915_ggtt *ggtt)
>>  {
>> -    struct drm_i915_private *i915 = guc_to_i915(guc);
>> -    struct i915_ggtt *ggtt = &i915->ggtt;
>>      u64 size;
>>      int ret;
>> +    if (!USES_GUC(ggtt->vm.i915))
>> +        return 0;
>> +
>>      size = ggtt->vm.total - GUC_GGTT_TOP;
>>     ret = i915_gem_gtt_reserve(&ggtt->vm, &ggtt->uc_fw, size,
>> @@ -752,11 +753,8 @@ int intel_guc_reserve_ggtt_top(struct intel_guc 
>> *guc)
>>      return ret;
>>  }
>> -void intel_guc_release_ggtt_top(struct intel_guc *guc)
>> +void intel_guc_release_ggtt_top(struct i915_ggtt *ggtt)
>>  {
>> -    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 cbfed7a77c8b..55ea14176c5e 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -173,9 +173,9 @@ 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);
>> -u32 intel_guc_reserved_gtt_size(struct intel_guc *guc);
>> -int intel_guc_reserve_ggtt_top(struct intel_guc *guc);
>> -void intel_guc_release_ggtt_top(struct intel_guc *guc);
>> +u32 intel_guc_reserved_gtt_size(struct i915_ggtt *ggtt);
>> +int intel_guc_reserve_ggtt_top(struct i915_ggtt *ggtt);
>> +void intel_guc_release_ggtt_top(struct i915_ggtt *ggtt);
>> static inline bool intel_guc_is_loaded(struct intel_guc *guc)
>>  {
>

Michal Wajdeczko June 6, 2019, 1:21 p.m. UTC | #3

On Thu, 06 Jun 2019 14:23:09 +0200, Tvrtko Ursulin  
<tvrtko.ursulin@linux.intel.com> wrote:

>
> On 06/06/2019 12:58, Michal Wajdeczko wrote:
>> On Thu, 06 Jun 2019 11:36:38 +0200, Tvrtko Ursulin  
>> <tvrtko.ursulin@linux.intel.com> wrote:
>>
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> These functions operate on ggtt so make them take that directly as
>>> parameter.
>>  Not quite.
>>  Function intel_guc_reserved_gtt_size() operates on struct intel_guc
>> and is defined in intel_guc.c for proper layering, so NAK here.
>
> But it doesn't really. It operates on intel_wopcm if we want to be true.

oops, I forgot that finally we placed wopcm directly under i915 (initially
it was declared inside intel_guc)

>
> u32 intel_guc_reserved_gtt_size(struct i915_ggtt *ggtt)
> {
> 	return ggtt->vm.i915->wopcm.guc.size;
> }
>
> And GuC portion it needs is just one part of struct intel_wopcm - so  
> whether or not this function must live in intel_guc.c and so how hard we  
> should see this as layering violation is not so clear in my opinion.

agreed. so we have two options:

move this function to intel_wopcm.h as:

static inline u32 intel_wopcm_guc_size(struct intel_wopcm *wopcm)
{
	GEM_BUG_ON(!wopcm->guc.size);
	return wopcm->guc.size;
}

and update existing callers or update just old one to:

u32 intel_guc_reserved_gtt_size(struct intel_guc *guc)
{
	return intel_wopcm_guc_size(&guc_to_i915(guc)->wopcm);
}

>
> Unless it will need to actually use intel_guc in the future.

There was a plan to create struct intel_uc with wopcm, guc and huc.
Not sure if it is still applicable.

>
>> For other two intel_guc_reserve|release_ggtt_top() I would rather:
>> - rename them to i915_ggtt_reserve|release_guc_top(),
>> - move to i915_gem_ggtt.c
>> - make them static
>
> Fair, will do.
>
> Regards,
>
> Tvrtko
>
>> Michal
>>
>>>
>>> At the same time move the USES_GUC conditional down to
>>> intel_guc_reserve_ggtt_top for symmetry with
>>> intel_guc_reserved_gtt_size.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/i915_gem_gtt.c | 14 ++++++--------
>>>  drivers/gpu/drm/i915/intel_guc.c    | 18 ++++++++----------
>>>  drivers/gpu/drm/i915/intel_guc.h    |  6 +++---
>>>  3 files changed, 17 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c  
>>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> index d3b3676d10f3..d967a4e9ceb0 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> @@ -2912,7 +2912,7 @@ int i915_gem_init_ggtt(struct drm_i915_private  
>>> *dev_priv)
>>>       * why.
>>>       */
>>>      ggtt->pin_bias = max_t(u32, I915_GTT_PAGE_SIZE,
>>> -                   intel_guc_reserved_gtt_size(&dev_priv->guc));
>>> +                   intel_guc_reserved_gtt_size(ggtt));
>>>     ret = intel_vgt_balloon(ggtt);
>>>      if (ret)
>>> @@ -2926,11 +2926,9 @@ 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;
>>> -    }
>>> +    ret = intel_guc_reserve_ggtt_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) {
>>> @@ -2952,7 +2950,7 @@ int i915_gem_init_ggtt(struct drm_i915_private  
>>> *dev_priv)
>>>      return 0;
>>> err_appgtt:
>>> -    intel_guc_release_ggtt_top(&dev_priv->guc);
>>> +    intel_guc_release_ggtt_top(ggtt);
>>>  err_reserve:
>>>      drm_mm_remove_node(&ggtt->error_capture);
>>>      return ret;
>>> @@ -2979,7 +2977,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);
>>> +    intel_guc_release_ggtt_top(ggtt);
>>>     if (drm_mm_initialized(&ggtt->vm.mm)) {
>>>          intel_vgt_deballoon(ggtt);
>>> diff --git a/drivers/gpu/drm/i915/intel_guc.c  
>>> b/drivers/gpu/drm/i915/intel_guc.c
>>> index b88c349c4fa6..633248b7da25 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc.c
>>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>>> @@ -719,7 +719,7 @@ struct i915_vma *intel_guc_allocate_vma(struct  
>>> intel_guc *guc, u32 size)
>>> /**
>>>   * intel_guc_reserved_gtt_size()
>>> - * @guc:    intel_guc structure
>>> + * @ggtt:    Pointer to struct i915_ggtt
>>>   *
>>>   * The GuC WOPCM mapping shadows the lower part of the GGTT, so if we  
>>> are using
>>>   * GuC we can't have any objects pinned in that region. This function  
>>> returns
>>> @@ -729,18 +729,19 @@ struct i915_vma *intel_guc_allocate_vma(struct  
>>> intel_guc *guc, u32 size)
>>>   * 0 if GuC is not present or not in use.
>>>   * Otherwise, the GuC WOPCM size.
>>>   */
>>> -u32 intel_guc_reserved_gtt_size(struct intel_guc *guc)
>>> +u32 intel_guc_reserved_gtt_size(struct i915_ggtt *ggtt)
>>>  {
>>> -    return guc_to_i915(guc)->wopcm.guc.size;
>>> +    return ggtt->vm.i915->wopcm.guc.size;
>>>  }
>>> -int intel_guc_reserve_ggtt_top(struct intel_guc *guc)
>>> +int intel_guc_reserve_ggtt_top(struct i915_ggtt *ggtt)
>>>  {
>>> -    struct drm_i915_private *i915 = guc_to_i915(guc);
>>> -    struct i915_ggtt *ggtt = &i915->ggtt;
>>>      u64 size;
>>>      int ret;
>>> +    if (!USES_GUC(ggtt->vm.i915))
>>> +        return 0;
>>> +
>>>      size = ggtt->vm.total - GUC_GGTT_TOP;
>>>     ret = i915_gem_gtt_reserve(&ggtt->vm, &ggtt->uc_fw, size,
>>> @@ -752,11 +753,8 @@ int intel_guc_reserve_ggtt_top(struct intel_guc  
>>> *guc)
>>>      return ret;
>>>  }
>>> -void intel_guc_release_ggtt_top(struct intel_guc *guc)
>>> +void intel_guc_release_ggtt_top(struct i915_ggtt *ggtt)
>>>  {
>>> -    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 cbfed7a77c8b..55ea14176c5e 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc.h
>>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>>> @@ -173,9 +173,9 @@ 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);
>>> -u32 intel_guc_reserved_gtt_size(struct intel_guc *guc);
>>> -int intel_guc_reserve_ggtt_top(struct intel_guc *guc);
>>> -void intel_guc_release_ggtt_top(struct intel_guc *guc);
>>> +u32 intel_guc_reserved_gtt_size(struct i915_ggtt *ggtt);
>>> +int intel_guc_reserve_ggtt_top(struct i915_ggtt *ggtt);
>>> +void intel_guc_release_ggtt_top(struct i915_ggtt *ggtt);
>>> static inline bool intel_guc_is_loaded(struct intel_guc *guc)
>>>  {

Rodrigo Vivi June 6, 2019, 1:44 p.m. UTC | #4

On Thu, Jun 06, 2019 at 10:36:38AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> These functions operate on ggtt so make them take that directly as
> parameter.

This patch makes me wonder where we really want and need to go.

We need to move out of dev_priv and global i915...
but do we need to go and reduce to all minimal stuff used like
uncore and ggtt or could we find a middle solution where
each group has its own "class"?

like this guc stuff would keep the intel_guc, but the i915_gem
stuff or intel_gt stuff would have their own structs where
we have everything needed for that group?

> 
> At the same time move the USES_GUC conditional down to
> intel_guc_reserve_ggtt_top for symmetry with
> intel_guc_reserved_gtt_size.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 14 ++++++--------
>  drivers/gpu/drm/i915/intel_guc.c    | 18 ++++++++----------
>  drivers/gpu/drm/i915/intel_guc.h    |  6 +++---
>  3 files changed, 17 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index d3b3676d10f3..d967a4e9ceb0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2912,7 +2912,7 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
>  	 * why.
>  	 */
>  	ggtt->pin_bias = max_t(u32, I915_GTT_PAGE_SIZE,
> -			       intel_guc_reserved_gtt_size(&dev_priv->guc));
> +			       intel_guc_reserved_gtt_size(ggtt));
>  
>  	ret = intel_vgt_balloon(ggtt);
>  	if (ret)
> @@ -2926,11 +2926,9 @@ 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;
> -	}
> +	ret = intel_guc_reserve_ggtt_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) {
> @@ -2952,7 +2950,7 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
>  	return 0;
>  
>  err_appgtt:
> -	intel_guc_release_ggtt_top(&dev_priv->guc);
> +	intel_guc_release_ggtt_top(ggtt);
>  err_reserve:
>  	drm_mm_remove_node(&ggtt->error_capture);
>  	return ret;
> @@ -2979,7 +2977,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);
> +	intel_guc_release_ggtt_top(ggtt);
>  
>  	if (drm_mm_initialized(&ggtt->vm.mm)) {
>  		intel_vgt_deballoon(ggtt);
> diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
> index b88c349c4fa6..633248b7da25 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -719,7 +719,7 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
>  
>  /**
>   * intel_guc_reserved_gtt_size()
> - * @guc:	intel_guc structure
> + * @ggtt:	Pointer to struct i915_ggtt
>   *
>   * The GuC WOPCM mapping shadows the lower part of the GGTT, so if we are using
>   * GuC we can't have any objects pinned in that region. This function returns
> @@ -729,18 +729,19 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
>   * 0 if GuC is not present or not in use.
>   * Otherwise, the GuC WOPCM size.
>   */
> -u32 intel_guc_reserved_gtt_size(struct intel_guc *guc)
> +u32 intel_guc_reserved_gtt_size(struct i915_ggtt *ggtt)
>  {
> -	return guc_to_i915(guc)->wopcm.guc.size;
> +	return ggtt->vm.i915->wopcm.guc.size;
>  }
>  
> -int intel_guc_reserve_ggtt_top(struct intel_guc *guc)
> +int intel_guc_reserve_ggtt_top(struct i915_ggtt *ggtt)
>  {
> -	struct drm_i915_private *i915 = guc_to_i915(guc);
> -	struct i915_ggtt *ggtt = &i915->ggtt;
>  	u64 size;
>  	int ret;
>  
> +	if (!USES_GUC(ggtt->vm.i915))
> +		return 0;
> +
>  	size = ggtt->vm.total - GUC_GGTT_TOP;
>  
>  	ret = i915_gem_gtt_reserve(&ggtt->vm, &ggtt->uc_fw, size,
> @@ -752,11 +753,8 @@ int intel_guc_reserve_ggtt_top(struct intel_guc *guc)
>  	return ret;
>  }
>  
> -void intel_guc_release_ggtt_top(struct intel_guc *guc)
> +void intel_guc_release_ggtt_top(struct i915_ggtt *ggtt)
>  {
> -	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 cbfed7a77c8b..55ea14176c5e 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -173,9 +173,9 @@ 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);
> -u32 intel_guc_reserved_gtt_size(struct intel_guc *guc);
> -int intel_guc_reserve_ggtt_top(struct intel_guc *guc);
> -void intel_guc_release_ggtt_top(struct intel_guc *guc);
> +u32 intel_guc_reserved_gtt_size(struct i915_ggtt *ggtt);
> +int intel_guc_reserve_ggtt_top(struct i915_ggtt *ggtt);
> +void intel_guc_release_ggtt_top(struct i915_ggtt *ggtt);
>  
>  static inline bool intel_guc_is_loaded(struct intel_guc *guc)
>  {
> -- 
> 2.20.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index d3b3676d10f3..d967a4e9ceb0 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2912,7 +2912,7 @@  int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
 	 * why.
 	 */
 	ggtt->pin_bias = max_t(u32, I915_GTT_PAGE_SIZE,
-			       intel_guc_reserved_gtt_size(&dev_priv->guc));
+			       intel_guc_reserved_gtt_size(ggtt));
 
 	ret = intel_vgt_balloon(ggtt);
 	if (ret)
@@ -2926,11 +2926,9 @@  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;
-	}
+	ret = intel_guc_reserve_ggtt_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) {
@@ -2952,7 +2950,7 @@  int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
 	return 0;
 
 err_appgtt:
-	intel_guc_release_ggtt_top(&dev_priv->guc);
+	intel_guc_release_ggtt_top(ggtt);
 err_reserve:
 	drm_mm_remove_node(&ggtt->error_capture);
 	return ret;
@@ -2979,7 +2977,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);
+	intel_guc_release_ggtt_top(ggtt);
 
 	if (drm_mm_initialized(&ggtt->vm.mm)) {
 		intel_vgt_deballoon(ggtt);
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index b88c349c4fa6..633248b7da25 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -719,7 +719,7 @@  struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
 
 /**
  * intel_guc_reserved_gtt_size()
- * @guc:	intel_guc structure
+ * @ggtt:	Pointer to struct i915_ggtt
  *
  * The GuC WOPCM mapping shadows the lower part of the GGTT, so if we are using
  * GuC we can't have any objects pinned in that region. This function returns
@@ -729,18 +729,19 @@  struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
  * 0 if GuC is not present or not in use.
  * Otherwise, the GuC WOPCM size.
  */
-u32 intel_guc_reserved_gtt_size(struct intel_guc *guc)
+u32 intel_guc_reserved_gtt_size(struct i915_ggtt *ggtt)
 {
-	return guc_to_i915(guc)->wopcm.guc.size;
+	return ggtt->vm.i915->wopcm.guc.size;
 }
 
-int intel_guc_reserve_ggtt_top(struct intel_guc *guc)
+int intel_guc_reserve_ggtt_top(struct i915_ggtt *ggtt)
 {
-	struct drm_i915_private *i915 = guc_to_i915(guc);
-	struct i915_ggtt *ggtt = &i915->ggtt;
 	u64 size;
 	int ret;
 
+	if (!USES_GUC(ggtt->vm.i915))
+		return 0;
+
 	size = ggtt->vm.total - GUC_GGTT_TOP;
 
 	ret = i915_gem_gtt_reserve(&ggtt->vm, &ggtt->uc_fw, size,
@@ -752,11 +753,8 @@  int intel_guc_reserve_ggtt_top(struct intel_guc *guc)
 	return ret;
 }
 
-void intel_guc_release_ggtt_top(struct intel_guc *guc)
+void intel_guc_release_ggtt_top(struct i915_ggtt *ggtt)
 {
-	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 cbfed7a77c8b..55ea14176c5e 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -173,9 +173,9 @@  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);
-u32 intel_guc_reserved_gtt_size(struct intel_guc *guc);
-int intel_guc_reserve_ggtt_top(struct intel_guc *guc);
-void intel_guc_release_ggtt_top(struct intel_guc *guc);
+u32 intel_guc_reserved_gtt_size(struct i915_ggtt *ggtt);
+int intel_guc_reserve_ggtt_top(struct i915_ggtt *ggtt);
+void intel_guc_release_ggtt_top(struct i915_ggtt *ggtt);
 
 static inline bool intel_guc_is_loaded(struct intel_guc *guc)
 {

[20/21] drm/i915: Make GuC GGTT reservation work on ggtt

Commit Message

Comments

Patch