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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


On 06/06/2019 12:58, Michal Wajdeczko wrote:
On Thu, 06 Jun 2019 11:36:38 +0200, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:

From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>

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@xxxxxxxxx>
---
 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)
 {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux