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

[Chris Wilson]

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.

> @@ -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