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@xxxxxxxxx>
> >>Signed-off-by: Alex Dai <yu.dai@xxxxxxxxx>
> >>Signed-off-by: Dave Gordon <david.s.gordon@xxxxxxxxx>
> >>---
> >> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
