Re: [Intel-gfx] [PATCH v12 1/6] drm/i915/guc: Rename guc_ggtt_offset to intel_guc_ggtt_offset
Quoting Jackie Li (2018-03-02 02:16:41) > 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) > > v10: > - Removed unnecessary comments, redundant code and avoided reuse variable >to avoid potential issues (Joonas) > > Cc: Michal Wajdeczko> Cc: Sagar Arun Kamble > Cc: Chris Wilson > Cc: Joonas Lahtinen > Reviewed-by: Sagar Arun Kamble (v8) > Reviewed-by: Joonas Lahtinen (v9) > Signed-off-by: Jackie Li > Reviewed-by: Michal Wajdeczko (v11) Reviewed-by: Joonas Lahtinen Regards, Joonas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v12 1/6] drm/i915/guc: Rename guc_ggtt_offset to intel_guc_ggtt_offset
On 03/02/2018 12:04 AM, Sagar Arun Kamble wrote: Cc: Michal WajdeczkoCc: Sagar Arun Kamble Cc: Chris Wilson Cc: Joonas Lahtinen Reviewed-by: Sagar Arun Kamble (v8) Reviewed-by: Joonas Lahtinen (v9) Signed-off-by: Jackie Li I think maintainers will prefer as: either Cc: Cc: S-o-b: R-b: or S-o-b: Cc: Cc: R-b: Thanks Sagar! I will include these changes along with other comments/suggestion from Joonas (if any:-)) Regards, -Jackie ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v12 1/6] drm/i915/guc: Rename guc_ggtt_offset to intel_guc_ggtt_offset
On 3/2/2018 5:46 AM, Jackie Li wrote: 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) v10: - Removed unnecessary comments, redundant code and avoided reuse variable to avoid potential issues (Joonas) Cc: Michal WajdeczkoCc: Sagar Arun Kamble Cc: Chris Wilson Cc: Joonas Lahtinen Reviewed-by: Sagar Arun Kamble (v8) Reviewed-by: Joonas Lahtinen (v9) Signed-off-by: Jackie Li I think maintainers will prefer as: either Cc: Cc: S-o-b: R-b: or S-o-b: Cc: Cc: R-b: Similar for other patches. Reviewed-by: Michal Wajdeczko (v11) --- drivers/gpu/drm/i915/intel_guc.c| 11 ++- drivers/gpu/drm/i915/intel_guc.h| 14 -- drivers/gpu/drm/i915/intel_guc_ads.c| 8 drivers/gpu/drm/i915/intel_guc_ct.c | 5 +++-- drivers/gpu/drm/i915/intel_guc_fw.c | 2 +- drivers/gpu/drm/i915/intel_guc_log.c| 2 +- drivers/gpu/drm/i915/intel_guc_reg.h| 3 --- drivers/gpu/drm/i915/intel_guc_submission.c | 10 +- drivers/gpu/drm/i915/intel_huc.c| 6 -- 9 files changed, 36 insertions(+), 25 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c index e6512cc..a788e15 100644 --- a/drivers/gpu/drm/i915/intel_guc.c +++ b/drivers/gpu/drm/i915/intel_guc.c @@ -269,8 +269,9 @@ 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, guc->stage_desc_pool); u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16; params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT; @@ -418,7 +419,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv) data[0] = INTEL_GUC_ACTION_ENTER_S_STATE; /* any value greater than GUC_POWER_D0 */ data[1] = GUC_POWER_D1; - data[2] = guc_ggtt_offset(guc->shared_data); + data[2] = intel_guc_ggtt_offset(guc, guc->shared_data); return intel_guc_send(guc, data, ARRAY_SIZE(data)); } @@ -441,7 +442,7 @@ int intel_guc_reset_engine(struct intel_guc *guc, data[3] = 0; data[4] = 0; data[5] = guc->execbuf_client->stage_id; - data[6] = guc_ggtt_offset(guc->shared_data); + data[6] = intel_guc_ggtt_offset(guc, guc->shared_data); return intel_guc_send(guc, data, ARRAY_SIZE(data)); } @@ -463,7 +464,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv) data[0] = INTEL_GUC_ACTION_EXIT_S_STATE; data[1] = GUC_POWER_D0; - data[2] = guc_ggtt_offset(guc->shared_data); + data[2] = intel_guc_ggtt_offset(guc, guc->shared_data); return intel_guc_send(guc, data, ARRAY_SIZE(data)); } diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h index 52856a9..0c8b10a 100644 --- a/drivers/gpu/drm/i915/intel_guc.h +++ b/drivers/gpu/drm/i915/intel_guc.h @@ -100,13 +100,23 @@ static inline void intel_guc_notify(struct intel_guc *guc) guc->notify(guc); } -/* +/* GuC addresses above GUC_GGTT_TOP also don't map through the GTT */ +#define GUC_GGTT_TOP 0xFEE0 + +/** + * intel_guc_ggtt_offset() - Get and validate the GGTT offset of @vma + * @guc: intel_guc structure. + * @vma: i915 graphics virtual memory area. + * * GuC does not allow any gfx GGTT address that falls into range [0, WOPCM_TOP), * which is reserved for Boot ROM, SRAM and WOPCM.
[Intel-gfx] [PATCH v12 1/6] drm/i915/guc: Rename guc_ggtt_offset to intel_guc_ggtt_offset
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) v10: - Removed unnecessary comments, redundant code and avoided reuse variable to avoid potential issues (Joonas) Cc: Michal WajdeczkoCc: Sagar Arun Kamble Cc: Chris Wilson Cc: Joonas Lahtinen Reviewed-by: Sagar Arun Kamble (v8) Reviewed-by: Joonas Lahtinen (v9) Signed-off-by: Jackie Li Reviewed-by: Michal Wajdeczko (v11) --- drivers/gpu/drm/i915/intel_guc.c| 11 ++- drivers/gpu/drm/i915/intel_guc.h| 14 -- drivers/gpu/drm/i915/intel_guc_ads.c| 8 drivers/gpu/drm/i915/intel_guc_ct.c | 5 +++-- drivers/gpu/drm/i915/intel_guc_fw.c | 2 +- drivers/gpu/drm/i915/intel_guc_log.c| 2 +- drivers/gpu/drm/i915/intel_guc_reg.h| 3 --- drivers/gpu/drm/i915/intel_guc_submission.c | 10 +- drivers/gpu/drm/i915/intel_huc.c| 6 -- 9 files changed, 36 insertions(+), 25 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c index e6512cc..a788e15 100644 --- a/drivers/gpu/drm/i915/intel_guc.c +++ b/drivers/gpu/drm/i915/intel_guc.c @@ -269,8 +269,9 @@ 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, guc->stage_desc_pool); u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16; params[GUC_CTL_DEBUG] |= ads << GUC_ADS_ADDR_SHIFT; @@ -418,7 +419,7 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv) data[0] = INTEL_GUC_ACTION_ENTER_S_STATE; /* any value greater than GUC_POWER_D0 */ data[1] = GUC_POWER_D1; - data[2] = guc_ggtt_offset(guc->shared_data); + data[2] = intel_guc_ggtt_offset(guc, guc->shared_data); return intel_guc_send(guc, data, ARRAY_SIZE(data)); } @@ -441,7 +442,7 @@ int intel_guc_reset_engine(struct intel_guc *guc, data[3] = 0; data[4] = 0; data[5] = guc->execbuf_client->stage_id; - data[6] = guc_ggtt_offset(guc->shared_data); + data[6] = intel_guc_ggtt_offset(guc, guc->shared_data); return intel_guc_send(guc, data, ARRAY_SIZE(data)); } @@ -463,7 +464,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv) data[0] = INTEL_GUC_ACTION_EXIT_S_STATE; data[1] = GUC_POWER_D0; - data[2] = guc_ggtt_offset(guc->shared_data); + data[2] = intel_guc_ggtt_offset(guc, guc->shared_data); return intel_guc_send(guc, data, ARRAY_SIZE(data)); } diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h index 52856a9..0c8b10a 100644 --- a/drivers/gpu/drm/i915/intel_guc.h +++ b/drivers/gpu/drm/i915/intel_guc.h @@ -100,13 +100,23 @@ static inline void intel_guc_notify(struct intel_guc *guc) guc->notify(guc); } -/* +/* GuC addresses above GUC_GGTT_TOP also don't map through the GTT */ +#define GUC_GGTT_TOP 0xFEE0 + +/** + * intel_guc_ggtt_offset() - Get and validate the GGTT offset of @vma + * @guc: intel_guc structure. + * @vma: i915 graphics virtual memory area. + * * GuC does not allow any gfx GGTT address that falls into range [0, WOPCM_TOP), * which is reserved for Boot ROM, SRAM and WOPCM. Currently this top address is * 512K. In order to exclude 0-512K address space from GGTT, all gfx objects * used by GuC is pinned with PIN_OFFSET_BIAS along