On 15/09/2022 11:33, Anshuman Gupta wrote:
If i915 gem obj lies in lmem, then i915_gem_object_pin_map
need to grab a rpm wakeref to make sure gfx PCIe endpoint
function stays in D0 state during any access to mapping
returned by i915_gem_object_pin_map().
Subsequently i915_gem_object_upin_map will put the wakref as well.
Cc: Matthew Auld <matthew.auld@xxxxxxxxx>
Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
Cc: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
Signed-off-by: Anshuman Gupta <anshuman.gupta@xxxxxxxxx>
---
drivers/gpu/drm/i915/gem/i915_gem_object.c | 2 ++
drivers/gpu/drm/i915/gem/i915_gem_object.h | 5 +++++
drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 14 ++++++++++++++
drivers/gpu/drm/i915/gem/i915_gem_pages.c | 8 ++++++++
4 files changed, 29 insertions(+)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 85482a04d158..f291f990838d 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -95,6 +95,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
mutex_init(&obj->mm.get_page.lock);
INIT_RADIX_TREE(&obj->mm.get_dma_page.radix, GFP_KERNEL | __GFP_NOWARN);
mutex_init(&obj->mm.get_dma_page.lock);
+ mutex_init(&obj->wakeref_lock);
}
/**
@@ -110,6 +111,7 @@ void __i915_gem_object_fini(struct drm_i915_gem_object *obj)
{
mutex_destroy(&obj->mm.get_page.lock);
mutex_destroy(&obj->mm.get_dma_page.lock);
+ mutex_destroy(&obj->wakeref_lock);
dma_resv_fini(&obj->base._resv);
}
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index 7317d4102955..b31ac6e4c272 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -501,6 +501,11 @@ static inline void i915_gem_object_flush_map(struct drm_i915_gem_object *obj)
*/
static inline void i915_gem_object_unpin_map(struct drm_i915_gem_object *obj)
{
+ mutext_lock(obj->wakeref_lock);
+ if (!--obj->wakeref_count)
+ intel_runtime_pm_put(&to_i915(obj->base.dev)->runtime_pm, obj->wakeref);
+ mutext_unlock(obj->wakeref_lock);
+
i915_gem_object_unpin_pages(obj);
}
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
index 9f6b14ec189a..34aff95a1984 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
@@ -657,6 +657,20 @@ struct drm_i915_gem_object {
void *gvt_info;
};
+
+ /**
+ * wakeref to protect the i915 lmem iomem mappings.
+ * We don't pin_map an object partially that makes easy
+ * to track the wakeref cookie, if wakeref is already held
+ * then we don't need to grab it again for other pin_map.
+ * first pin_map will grab the wakeref and last unpin_map
+ * will put the wakeref.
+ */
+ intel_wakeref_t wakeref;
+ unsigned int wakeref_count;
+
+ /** protects the wakeref_count wakeref cookie against multiple pin_map and unpin_map */
+ struct mutex wakeref_lock;
On one side it feels wasteful to have counters per object. But then I
also notice pin_map is only allowed under the obj dma_resv locked -
meaning that lock is already held. So you possibly don't need a new
mutex, someone more up to date please confirm.
Option B - trading space efficieny for one more atomic - would be to
track it on the level of i915 and maybe use atomic_t? Would we have to
worry about overflow more in this case? Hm some protection regardless of
the option will be needed just in case.
Regards,
Tvrtko
};
static inline struct drm_i915_gem_object *
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
index 4df50b049cea..b638b5413280 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
@@ -370,6 +370,14 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj,
assert_object_held(obj);
+ if (i915_gem_object_is_lmem(obj)) {
+ mutex_lock(&obj->wakeref_lock);
+ if (!obj->wakeref_count++)
+ obj->wakeref =
+ intel_runtime_pm_get(&to_i915(obj->base.dev)->runtime_pm);
+ mutex_unlock(&obj->wakeref_lock);
+ }
+
pinned = !(type & I915_MAP_OVERRIDE);
type &= ~I915_MAP_OVERRIDE;
