[Intel-gfx] [PATCH v9 2/7] drm/i915/guc: Rename guc_ggtt_offset to intel_guc_ggtt_offset

[Joonas Lahtinen]

Quoting Jackie Li (2018-02-09 01:03:50)
> GuC related exported functions should start with "intel_guc_" prefix and
> pass intel_guc as the first parameter since its guc related. Current
> guc_ggtt_offset() failed to follow this code convention and this is a
> problem for future patches that needs to access intel_guc data to verify
> the GGTT offset against the GuC WOPCM top.
> 
> This patch renames the guc_ggtt_offset to intel_guc_ggtt_offset and updates
> the related code to pass intel_guc pointer to this function call, so that
> we can have a unified coding style for GuC code and also enable the future
> patches to get GuC related data from intel_guc to do the offset
> verification. Meanwhile, this patch also moves the GUC_GGTT_TOP from
> intel_guc_regs.h to intel_guc.h since it is not GuC register related
> definition.
> 
> v8:
>  - Fixed coding style issues and moved GUC_GGTT_TOP to intel_guc.h (Sagar)
>  - Updated commit message to explain to reason and motivation to add
>    intel_guc as the first parameter of intel_guc_ggtt_offset (Chris)
> 
> v9:
>  - Fixed code alignment issue due to line break (Chris)
> 
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble at intel.com> (v8)
> Signed-off-by: Jackie Li <yaodong.li at intel.com>

<SNIP>

> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -269,8 +269,10 @@ void intel_guc_init_params(struct intel_guc *guc)
>  
>         /* If GuC submission is enabled, set up additional parameters here */
>         if (USES_GUC_SUBMISSION(dev_priv)) {
> -               u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
> -               u32 pgs = guc_ggtt_offset(dev_priv->guc.stage_desc_pool);
> +               u32 ads = intel_guc_ggtt_offset(guc,
> +                                               guc->ads_vma) >> PAGE_SHIFT;
> +               u32 pgs = intel_guc_ggtt_offset(guc,
> +                                               dev_priv->guc.stage_desc_pool);

I must wonder why the other is addressed through dev_priv->guc and other
directly. guc->stage_desc_pool would work equally, right?

> @@ -135,11 +124,23 @@ int intel_guc_ads_create(struct intel_guc *guc)
>                 blob->ads.eng_state_size[engine->guc_id] =
>                         engine->context_size - skipped_size;
>  
> -       base = guc_ggtt_offset(vma);
> +       base = intel_guc_ggtt_offset(guc, vma);
>         blob->ads.scheduler_policies = base + ptr_offset(blob, policies);
>         blob->ads.reg_state_buffer = base + ptr_offset(blob, reg_state_buffer);
>         blob->ads.reg_state_addr = base + ptr_offset(blob, reg_state);
>  
> +       /*
> +        * The GuC requires a "Golden Context" when it reinitialises
> +        * engines after a reset. Here we use the Render ring default
> +        * context, which must already exist and be pinned in the GGTT,
> +        * so its address won't change after we've told the GuC where
> +        * to find it. Note that we have to skip our header (1 page),
> +        * because our GuC shared data is there.
> +        */
> +       vma = dev_priv->kernel_context->engine[RCS].state;

vma variable is being reused here, you want to have another variable for
each different use, this avoids nasty merge conflicts and at worst, bugs.

> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index 7b5074e..eca8725 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -638,7 +638,7 @@ int intel_guc_log_create(struct intel_guc *guc)
>                 (GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) |
>                 (GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT);
>  
> -       offset = guc_ggtt_offset(vma) >> PAGE_SHIFT; /* in pages */
> +       offset = intel_guc_ggtt_offset(guc, vma) >> PAGE_SHIFT; /* in pages */

It's obvious it's in pages, you can drop the comment.

With those addressed, this is:

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

Regards, Joonas