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.
> @@ -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.
> 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
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
