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@xxxxxxxxx>
> Cc: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx> (v8)
> Signed-off-by: Jackie Li <yaodong.li@xxxxxxxxx>
<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@xxxxxxxxxxxxxxx>
Regards, Joonas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
