Re: [PATCH 1/3] drm/i915/guc: keep GuC objects mapped in kernel

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 14/04/16 18:19, 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.
>
> Cc: <tvrtko.ursulin@intel.com>
> Signed-off-by: Alex Dai <yu.dai@intel.com>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 37 +++++++++++-------------------
>   drivers/gpu/drm/i915/intel_guc.h           |  1 +
>   2 files changed, 14 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index da86bdb..f80f577 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -179,15 +179,11 @@ static void guc_init_doorbell(struct intel_guc *guc,
>   			      struct i915_guc_client *client)
>   {
>   	struct guc_doorbell_info *doorbell;
> -	void *base;
>
> -	base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0));
> -	doorbell = base + client->doorbell_offset;
> +	doorbell = client->client_base + client->doorbell_offset;
>
> -	doorbell->db_status = 1;
> +	doorbell->db_status = GUC_DOORBELL_ENABLED;
>   	doorbell->cookie = 0;
> -
> -	kunmap_atomic(base);
>   }
>
>   static int guc_ring_doorbell(struct i915_guc_client *gc)
> @@ -256,16 +252,12 @@ static void guc_disable_doorbell(struct intel_guc *guc,
>   {
>   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
>   	struct guc_doorbell_info *doorbell;
> -	void *base;
>   	i915_reg_t drbreg = GEN8_DRBREGL(client->doorbell_id);
>   	int value;
>
> -	base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0));
> -	doorbell = base + client->doorbell_offset;
> -
> -	doorbell->db_status = 0;
> +	doorbell = client->client_base + client->doorbell_offset;

Not 100% sure of the object lifetimes in GuC, but would it be even 
simpler to store a pointer to struct struct guc_doorbell_info as 
guc->doorbell ? There aren't that many call sites true, but kind of 
looks logical at least from the outside.

>
> -	kunmap_atomic(base);
> +	doorbell->db_status = GUC_DOORBELL_DISABLED;
>
>   	I915_WRITE(drbreg, I915_READ(drbreg) & ~GEN8_DRB_VALID);
>
> @@ -341,10 +333,8 @@ static void guc_init_proc_desc(struct intel_guc *guc,
>   			       struct i915_guc_client *client)
>   {
>   	struct guc_process_desc *desc;
> -	void *base;
>
> -	base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0));
> -	desc = base + client->proc_desc_offset;
> +	desc = client->client_base + client->proc_desc_offset;

And the same maybe for this?

>   	memset(desc, 0, sizeof(*desc));
>
> @@ -361,8 +351,6 @@ static void guc_init_proc_desc(struct intel_guc *guc,
>   	desc->wq_size_bytes = client->wq_size;
>   	desc->wq_status = WQ_STATUS_ACTIVE;
>   	desc->priority = client->priority;
> -
> -	kunmap_atomic(base);
>   }
>
>   /*
> @@ -607,6 +595,7 @@ int i915_guc_submit(struct i915_guc_client *client,
>    * This is a wrapper to create a gem obj. In order to use it inside GuC, the
>    * object needs to be pinned lifetime. Also we must pin it to gtt space other
>    * than [0, GUC_WOPCM_TOP) because this range is reserved inside GuC.
> + * The object is also pinned & mapped into kernel address space.
>    *
>    * Return:	A drm_i915_gem_object if successful, otherwise NULL.
>    */
> @@ -620,13 +609,14 @@ static struct drm_i915_gem_object *gem_allocate_guc_obj(struct drm_device *dev,
>   	if (!obj)
>   		return NULL;
>
> -	if (i915_gem_object_get_pages(obj)) {
> +	if (i915_gem_object_pin_map(obj) == NULL) {

This should be IS_ERR check.

>   		drm_gem_object_unreference(&obj->base);
>   		return NULL;
>   	}
>
>   	if (i915_gem_obj_ggtt_pin(obj, PAGE_SIZE,
>   			PIN_OFFSET_BIAS | GUC_WOPCM_TOP)) {
> +		i915_gem_object_unpin_map(obj);
>   		drm_gem_object_unreference(&obj->base);
>   		return NULL;
>   	}
> @@ -649,6 +639,8 @@ 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);
>
> +	i915_gem_object_unpin_map(obj);
> +
>   	drm_gem_object_unreference(&obj->base);
>   }
>
> @@ -729,6 +721,8 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
>   		goto err;
>
>   	client->client_obj = obj;
> +	client->client_base = obj->mapping;

It think outside code should not access obj->mapping directly but use 
what i915_gem_object_pin_map has returned.

> +	WARN_ON(!client->client_base);

And this has already been handled at the i915_gem_object_pin_map call 
site so I don't think it serves any purpose.

>   	client->wq_offset = GUC_DB_SIZE;
>   	client->wq_size = GUC_WQ_SIZE;
>
> @@ -841,7 +835,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 +850,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 = obj->mapping;

Same as above. I suggest storing the base address in the guc client or 
somewhere appropriate.

Or if objects have separate explicit lifetimes, even if they don't 
overlap, you could even nest i915_gem_object_pin_map and unpin. Depends 
what makes the code simpler.

>
>   	/*
>   	 * The GuC requires a "Golden Context" when it reinitialises
> @@ -897,8 +888,6 @@ static void guc_create_ads(struct intel_guc *guc)
>
>   	ads->reg_state_buffer = ads->reg_state_addr +
>   			sizeof(struct guc_mmio_reg_state);
> -
> -	kunmap(page);
>   }
>
>   /*
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 3bb85b1..9ab3564 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -31,6 +31,7 @@ struct drm_i915_gem_request;
>
>   struct i915_guc_client {
>   	struct drm_i915_gem_object *client_obj;
> +	void *client_base;		/* Mapped address of above	*/
>   	struct intel_context *owner;
>   	struct intel_guc *guc;
>   	uint32_t priority;
>

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx