Re: [PATCH] drm/i915/guc: Keep the ctx_pool_vaddr mapped, for easy access

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On to, 2017-02-09 at 02:23 -0800, Oscar Mateo wrote:
> From: Michal Wajdeczko <michal.wajdeczko@intel.com>
> 
> The GuC descriptor is big in size. If we use a local definition of
> guc_desc we have a chance to overflow stack, so avoid it.
> 
> Also, Chris abhors scatterlists :)
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>

<SNIP>

> @@ -116,15 +114,8 @@ static int guc_update_doorbell_id(struct intel_guc *guc,
>  	}
>  
>  	/* Update the GuC's idea of the doorbell ID */
> -	len = sg_pcopy_to_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
> -			     sizeof(desc) * client->ctx_index);
> -	if (len != sizeof(desc))
> -		return -EFAULT;
> -	desc.db_id = new_id;
> -	len = sg_pcopy_from_buffer(sg->sgl, sg->nents, &desc, sizeof(desc),
> -			     sizeof(desc) * client->ctx_index);
> -	if (len != sizeof(desc))
> -		return -EFAULT;
> +	desc = guc->ctx_pool_vaddr + sizeof(*desc) * client->ctx_index;

As Chris mentioned, could use a __get_doorbell_desc helper for this.

> @@ -895,6 +876,13 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
>  		return PTR_ERR(vma);
>  
>  	guc->ctx_pool_vma = vma;
> +
> +	vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
> +	if (IS_ERR(vaddr))

After onion teardown added to this function, propagate errors;

		err = ERR_PTR(vaddr);
> +		goto err;
> +
> +	guc->ctx_pool_vaddr = vaddr;
> +

> @@ -983,8 +971,11 @@ void i915_guc_submission_fini(struct  drm_i915_private *dev_priv)
>  	i915_vma_unpin_and_release(&guc->ads_vma);
>  	i915_vma_unpin_and_release(&guc->log.vma);
>  
> -	if (guc->ctx_pool_vma)
> +	if (guc->ctx_pool_vaddr) {
>  		ida_destroy(&guc->ctx_ids);
> +		i915_gem_object_unpin_map(guc->ctx_pool_vma->obj);
> +	}

This converted to unconditional chain of teardown.

> +
>  	i915_vma_unpin_and_release(&guc->ctx_pool_vma);
>  }
>  
> @@ -1040,5 +1031,3 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
>  
> >  	return intel_guc_send(guc, data, ARRAY_SIZE(data));
>  }
> -
> -
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 8a33a46..29ee373 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -155,6 +155,7 @@ struct intel_guc {
>  
>  	struct i915_vma *ads_vma;
>  	struct i915_vma *ctx_pool_vma;

s/ctx_pool_vma/ctx_pool/ would bring out the dependency explicitly.

> +	void *ctx_pool_vaddr;
>  	struct ida ctx_ids;
>  
>  	struct i915_guc_client *execbuf_client;

This needs a rebase on top of my cleanup, but is very welcome
improvement.

The types of variables are still wild, but not introduced by this
change.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx