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@xxxxxxxxxxxxxxxxxx>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Signed-off-by: Oscar Mateo <oscar.mateo@xxxxxxxxx>
<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@xxxxxxxxxxxxxxx>
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
