On Fri, 2017-12-08 at 13:41 -0800, Jackie Li wrote:
> intel_guc_reg.h should only include definition for GuC registers
> and related register bits. GuC WOPCM related values should not
> be defined in intel_guc_reg.h
>
> This patch creates a better file structure by moving GuC WOPCM
> related definitions int to a new header intel_guc_wopcm.h
> and moving GuC WOPCM related functions to a new source file
> intel_guc_wopcm.c
>
> Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> Cc: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
> Signed-off-by: Jackie Li <yaodong.li@xxxxxxxxx>
Please add Cc:s to patches for people who comment on the previous
iterations of the patches.
> +++ b/drivers/gpu/drm/i915/intel_guc_wopcm.c
> +u32 intel_guc_wopcm_size(struct intel_guc *guc)
> +{
> + struct drm_i915_private *i915 = guc_to_i915(guc);
> +
> + u32 wopcm_size = GUC_WOPCM_TOP;
> +
> + /* On BXT, the top of WOPCM is reserved for RC6 context */
> + if (IS_GEN9_LP(i915))
> + wopcm_size -= BXT_GUC_WOPCM_RC6_RESERVED;
This is still bit confusing. How exactly is WOPCM size different from
the WOPCM top? If the WOPCM is used also by the hardware, when GuC is
disabled, then it should be intel_wopcm_*. If we only need this
information for the single GuC register programming, then I think this
should instead be local to programming that GuC register.
There should be a very clear split in the registers/functions to show
what is specific to the some hardware function and what is more
generic.
If this is all GuC related and only ever needs to be programmed for GuC
as the current naming suggest, then it's a great question why we are
not programming the register according to some firmware reported size
instead of replicating here.
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
