[Intel-gfx] [PATCH v2 1/3] drm/i915/guc: keep GuC objects mapped in kernel

[Chris Wilson]

On Mon, Apr 18, 2016 at 12:28:43PM +0100, Dave Gordon wrote:
> On 18/04/16 11:25, Chris Wilson wrote:
> >On Mon, Apr 18, 2016 at 11:15:07AM +0100, Dave Gordon wrote:
> >>With the new i915_gem_obj_pin_map() interface, it makes sense to keep
> >>GuC objects (which are always pinned in memory and in the GGTT anyway)
> >>mapped into kernel address space, rather than mapping and unmapping them
> >>on each access.
> >>
> >>This preliminary patch sets up the pin-and-map for all GuC-specific
> >>objects, and updates the various setup/shutdown functions to use these
> >>long-term mappings rather than doing their own kmap_atomic() calls.
> >>
> >>v2:
> >>     Invent & use accessor function i915_object_mapped_address() rather
> >>     than accessing obj->mapping directly (Tvrtko Ursulin)
> >>     Use IS_ERR() rather than checking for NULL (Tvrtko Ursulin)
> >>     Added big comment about struct i915_guc_client & usage of the GEM
> >>     object that it describes
> >>
> >>Cc: <tvrtko.ursulin at intel.com>
> >>Signed-off-by: Alex Dai <yu.dai at intel.com>
> >>Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
> >>---
> >>  drivers/gpu/drm/i915/i915_drv.h            | 12 ++++++++++
> >>  drivers/gpu/drm/i915/i915_guc_submission.c | 38 +++++++++++-------------------
> >>  drivers/gpu/drm/i915/intel_guc.h           | 24 +++++++++++++++++++
> >>  3 files changed, 50 insertions(+), 24 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>index 85102ad..439f149 100644
> >>--- a/drivers/gpu/drm/i915/i915_drv.h
> >>+++ b/drivers/gpu/drm/i915/i915_drv.h
> >>@@ -3025,6 +3025,18 @@ static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
> >>  void *__must_check i915_gem_object_pin_map(struct drm_i915_gem_object *obj);
> >>
> >>  /**
> >>+ * i915_object_mapped_address - address at which a GEM object is mapped
> >>+ * @obj - the object (presumably already mapped into kernel address space)
> >>+ *
> >>+ * Returns the address at which an object has been mapped by a call to
> >>+ * i915_gem_object_pin_map() above, or NULL if not currently mapped and pinned
> >>+ */
> >>+static inline void *i915_object_mapped_address(struct drm_i915_gem_object *obj)
> >>+{
> >>+       return obj->pages_pin_count == 0 ? NULL : obj->mapping;
> >>+}
> >
> >No.
> 
> The kernel address at which an object is pinned is just as much a
> property of the object as its GGTT address, so should have a similar
> function for retrieving it.

An object doesn't have a GGTT, a vma does. The object has vma. To use
the vma, you must pin it.
 
> The pages_pin_count check is there because once that goes to zero,
> the object doesn't logically /have/ a memory address, even if
> 'mapping' is still set.
> 
> Or was it just the name you objected to?

Name and function. obj->mapping is tied into the shinker which can and
will run at any time, stealing all locks. Using obj->mapping which
holding an actual page reference for yourself is broken.
> 

> >>@@ -649,6 +639,9 @@ static void gem_release_guc_obj(struct drm_i915_gem_object *obj)
> >>  	if (i915_gem_obj_is_pinned(obj))
> >>  		i915_gem_object_ggtt_unpin(obj);
> >>
> >>+	if (i915_object_mapped_address(obj))
> >>+		i915_gem_object_unpin_map(obj);
> >
> >Eh? If you aren't tracking your pin, you can't just randomly free
> >someone elses.
> 
> The GuC code exclusively owns this object, so it can't be "someone
> else's". The pages_pin count here will always be 1 (at present).
> This was to allow for the possibility that we already chose to
> release the pin-and-map while leaving the object in the GGTT, in
> which case the pages_pin_count will be 0. (We don't do this, at
> present, but see below).

That is bad justification for adding unsafe code.

> >>  	drm_gem_object_unreference(&obj->base);
> >>  }
> >>
> >>@@ -729,6 +722,8 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
> >>  		goto err;
> >>
> >>  	client->client_obj = obj;
> >>+	client->client_base = i915_object_mapped_address(obj);
> >>+	WARN_ON(!client->client_base);
> >>  	client->wq_offset = GUC_DB_SIZE;
> >>  	client->wq_size = GUC_WQ_SIZE;
> >>
> >>@@ -841,7 +836,6 @@ static void guc_create_ads(struct intel_guc *guc)
> >>  	struct guc_policies *policies;
> >>  	struct guc_mmio_reg_state *reg_state;
> >>  	struct intel_engine_cs *engine;
> >>-	struct page *page;
> >>  	u32 size;
> >>
> >>  	/* The ads obj includes the struct itself and buffers passed to GuC */
> >>@@ -857,9 +851,7 @@ static void guc_create_ads(struct intel_guc *guc)
> >>
> >>  		guc->ads_obj = obj;
> >>  	}
> >>-
> >>-	page = i915_gem_object_get_page(obj, 0);
> >>-	ads = kmap(page);
> >>+	ads = i915_object_mapped_address(obj);
> >
> >You need to explicitly aknowlege that you are using the memory tracked
> >by the object otherwise it *will* be removed by the shrinker.
> >So what is wrong with using i915_gem_object_pin_map()?
> >-Chris
> 
> The object is pinned-and-mapped just once, when it's created, and
> kept that way until it's destroyed. Calling pin-and-map here would
> give it a pin count of two, which would be redundant, and require us
> to unpin-and-map it again at the end of the function - which just
> seems a waste of time.

Considering the alternative is an unsound API. Just do it, or pass
around the mapping in a local you have already pinned.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre