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

Michal Wajdeczko
2019-06-06T13:21:52Z

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)
>>> {