Re: [PATCH 10/12] drm/i915/guc: Refactor the concept "GuC context descriptor" into "GuC stage descriptor"

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

On ti, 2017-03-21 at 02:02 -0700, Oscar Mateo wrote:
> A GuC context and a HW context are in no way related, so the name "GuC context descriptor"
> is very unfortunate, because a new reader of the code gets overwhelmed very quickly with
> a lot of things called "context" that refer to different things. We can improve legibility
> a lot by simply renaming a few objects in the GuC code.
> 
> v2:
>   - Rebased
>   - s/ctx_desc_pool/stage_desc_pool
>   - Move some explanations to the definition of the guc_stage_desc struct (Chris)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>

<SNIP>
 
> -static struct guc_context_desc *__get_context_desc(struct i915_guc_client *client)
> +static struct guc_stage_desc *__get_stage_desc(struct i915_guc_client *client)
>  {
> -	return (struct guc_context_desc *)((char *)client->guc->ctx_pool_vaddr +
> -		sizeof(struct guc_context_desc) * client->ctx_index);
> +	return (struct guc_stage_desc *)((char *)client->guc->stage_desc_pool_vaddr +
> +		sizeof(struct guc_stage_desc) * client->stage_id);

Am I missing something or isn't this just a hard way of doing;

	struct guc_stage_desc *stage_descs = client->guc->stage_desc_pool_vaddr;

	return &stage_descs[client->stage_id];

(+/- fixing sparse warnings, if any)

>  }
>  

<SNIP>

> @@ -1089,30 +1092,30 @@ static void guc_ads_destroy(struct intel_guc *guc)
>   */
>  int i915_guc_submission_init(struct drm_i915_private *dev_priv)
>  {
> -	const size_t ctxsize = sizeof(struct guc_context_desc);
> -	const size_t poolsize = GUC_MAX_GPU_CONTEXTS * ctxsize;
> +	const size_t ctxsize = sizeof(struct guc_stage_desc);

I was about to comment to rename ctxsize -> stagesize, but;

> +	const size_t poolsize = GUC_MAX_STAGE_DESCRIPTORS * ctxsize;
>  	const size_t gemsize = round_up(poolsize, PAGE_SIZE);

The above are only used once, so instead:

>  	struct intel_guc *guc = &dev_priv->guc;
>  	struct i915_vma *vma;
>  	void *vaddr;
>  	int ret;
>  
> -	if (guc->ctx_pool)
> +	if (guc->stage_desc_pool)
>  		return 0;
>  
>  	vma = intel_guc_allocate_vma(guc, gemsize);

Do

	vma = intel_guc_allocate_vma(guc,
				     PAGE_ALIGN(sizeof(struct guc_stage_desc) *
				     		GUC_MAX_GPU_CONTEXTS));
>  	if (IS_ERR(vma))
>  		return PTR_ERR(vma);

<SNIP>

 
> -#define GUC_CTX_DESC_ATTR_ACTIVE	(1 << 0)
> -#define GUC_CTX_DESC_ATTR_PENDING_DB	(1 << 1)
> -#define GUC_CTX_DESC_ATTR_KERNEL	(1 << 2)
> -#define GUC_CTX_DESC_ATTR_PREEMPT	(1 << 3)
> -#define GUC_CTX_DESC_ATTR_RESET		(1 << 4)
> -#define GUC_CTX_DESC_ATTR_WQLOCKED	(1 << 5)
> -#define GUC_CTX_DESC_ATTR_PCH		(1 << 6)
> -#define GUC_CTX_DESC_ATTR_TERMINATED	(1 << 7)
> +#define GUC_STAGE_DESC_ATTR_ACTIVE	(1 << 0)
> +#define GUC_STAGE_DESC_ATTR_PENDING_DB	(1 << 1)
> +#define GUC_STAGE_DESC_ATTR_KERNEL	(1 << 2)
> +#define GUC_STAGE_DESC_ATTR_PREEMPT	(1 << 3)
> +#define GUC_STAGE_DESC_ATTR_RESET	(1 << 4)
> +#define GUC_STAGE_DESC_ATTR_WQLOCKED	(1 << 5)
> +#define GUC_STAGE_DESC_ATTR_PCH		(1 << 6)
> +#define GUC_STAGE_DESC_ATTR_TERMINATED	(1 << 7)

While here, make 'em BIT()

With above, this is;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

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