On 4/9/20 7:03 AM, Michal Wajdeczko wrote:
On 09.04.2020 02:56, Daniele Ceraolo Spurio wrote:We stopped using the parameter in commit dd18cedfa36f ("drm/i915/guc: Move the pin bias value from GuC to GGTT"), so we can safely remove it. Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx> Cc: Matthew Brost <matthew.brost@xxxxxxxxx> Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx> Cc: John Harrison <john.c.harrison@xxxxxxxxx> --- drivers/gpu/drm/i915/gt/uc/intel_guc.c | 6 +++--- drivers/gpu/drm/i915/gt/uc/intel_guc.h | 4 +--- drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c | 2 +- drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 5 ++--- drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 4 ++-- drivers/gpu/drm/i915/gt/uc/intel_huc.c | 3 +-- 6 files changed, 10 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c index 861657897c0f..5134d544bf4c 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c @@ -220,7 +220,7 @@ static u32 guc_ctl_ctxinfo_flags(struct intel_guc *guc) if (intel_guc_submission_is_used(guc)) { u32 ctxnum, base;- base = intel_guc_ggtt_offset(guc, guc->stage_desc_pool);+ base = intel_guc_ggtt_offset(guc->stage_desc_pool); ctxnum = GUC_MAX_STAGE_DESCRIPTORS / 16;base >>= PAGE_SHIFT;@@ -232,7 +232,7 @@ static u32 guc_ctl_ctxinfo_flags(struct intel_guc *guc)static u32 guc_ctl_log_params_flags(struct intel_guc *guc){ - u32 offset = intel_guc_ggtt_offset(guc, guc->log.vma) >> PAGE_SHIFT; + u32 offset = intel_guc_ggtt_offset(guc->log.vma) >> PAGE_SHIFT; u32 flags;#if (((CRASH_BUFFER_SIZE) % SZ_1M) == 0)@@ -273,7 +273,7 @@ static u32 guc_ctl_log_params_flags(struct intel_guc *guc)static u32 guc_ctl_ads_flags(struct intel_guc *guc){ - u32 ads = intel_guc_ggtt_offset(guc, guc->ads_vma) >> PAGE_SHIFT; + u32 ads = intel_guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT; u32 flags = ads << GUC_ADS_ADDR_SHIFT;return flags;diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index e84ab67b317d..366191204a7d 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -103,7 +103,6 @@ static inline void intel_guc_to_host_event_handler(struct intel_guc *guc)/*** 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 @@ -114,8 +113,7 @@ static inline void intel_guc_to_host_event_handler(struct intel_guc *guc) * * Return: GGTT offset of the @vma. */ -static inline u32 intel_guc_ggtt_offset(struct intel_guc *guc, - struct i915_vma *vma) +static inline u32 intel_guc_ggtt_offset(struct i915_vma *vma)leaving this function with 'intel_guc' prefix without param guc would break our naming schema, maybe we should rename it to: static inline u32 i915_ggtt_offset_guc(struct i915_vma *vma)
I'm not convinced this is a good idea, the guc code still owns this function, so IMO it should still be called intel_guc_* to make that clear. We do have plenty of examples where the prefix and the param don't match, e.g. i915_ggtt_pin_bias just below accepts a vma and not the GGTT structure. With this I don't want to say that we should not try to match the prefix and param as much as possible, just that we should avoid enforcing that rule too strictly.
as code inside init_ggtt() already understands guc specifics ...{ u32 offset = i915_ggtt_offset(vma);btw, we have here (not shown) in diff: GEM_BUG_ON(offset < i915_ggtt_pin_bias(vma)); that I would move to i915_ggtt_offset() as it quite generic
Not all objects are pinned using the bias so can't do that.
and replace it with something more GuC specific, like: GEM_BUG_ON(offset < intel_wopcm_guc_size(wopcm))
I have a patch later in the series to move the wopcm under uc, so getting it from the vma would be troublesome (vma->vm->gt.uc). Since we use this for HuC as well maybe we can change to:
intel_uc_ggtt_offset(struct intel_uc *uc, struct i915_vma *vma)
{
GEM_BUG_ON(offset < intel_uc_wopcm_guc_size(uc));
...
}
but not sure if passing in the uc just to get to the wopcm is overkill
when we have the bias value easily accessible via i915_ggtt_pin_bias.
Thoughts?
Daniele
Michaldiff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.cindex 101728006ae9..9237d798f7f4 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c @@ -107,7 +107,7 @@ static void __guc_ads_init(struct intel_guc *guc) blob->system_info.vebox_enable_mask = VEBOX_MASK(dev_priv); blob->system_info.vdbox_sfc_support_mask = RUNTIME_INFO(dev_priv)->vdbox_sfc_access;- base = intel_guc_ggtt_offset(guc, guc->ads_vma);+ base = intel_guc_ggtt_offset(guc->ads_vma);/* Clients info */guc_ct_pool_entries_init(blob->ct_pool, ARRAY_SIZE(blob->ct_pool)); diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index 11742fca0e9e..aad5ac54c1ba 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -187,7 +187,7 @@ int intel_guc_ct_init(struct intel_guc_ct *ct) return err; }- CT_DEBUG(ct, "vma base=%#x\n", intel_guc_ggtt_offset(guc, ct->vma));+ CT_DEBUG(ct, "vma base=%#x\n", intel_guc_ggtt_offset(ct->vma));/* store pointers to desc and cmds */for (i = 0; i < ARRAY_SIZE(ct->ctbs); i++) { @@ -220,7 +220,6 @@ void intel_guc_ct_fini(struct intel_guc_ct *ct) */ int intel_guc_ct_enable(struct intel_guc_ct *ct) { - struct intel_guc *guc = ct_to_guc(ct); u32 base, cmds, size; int err; int i; @@ -229,7 +228,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)/* vma should be already allocated and map'ed */GEM_BUG_ON(!ct->vma); - base = intel_guc_ggtt_offset(guc, ct->vma); + base = intel_guc_ggtt_offset(ct->vma);/* (re)initialize descriptors* cmds buffers are in the second half of the blob page diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index fe7778c28d2d..7eaf173dd588 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -153,8 +153,8 @@ static void guc_stage_desc_init(struct intel_guc *guc) desc->stage_id = 0; desc->priority = GUC_CLIENT_PRIORITY_KMD_NORMAL;- desc->process_desc = intel_guc_ggtt_offset(guc, guc->proc_desc);- desc->wq_addr = intel_guc_ggtt_offset(guc, guc->workqueue); + desc->process_desc = intel_guc_ggtt_offset(guc->proc_desc); + desc->wq_addr = intel_guc_ggtt_offset(guc->workqueue); desc->wq_size = GUC_WQ_SIZE; }diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.cindex 65eeb44b397d..534f4d9f6591 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c @@ -166,8 +166,7 @@ int intel_huc_auth(struct intel_huc *huc) if (ret) goto fail;- ret = intel_guc_auth_huc(guc,- intel_guc_ggtt_offset(guc, huc->rsa_data)); + ret = intel_guc_auth_huc(guc, intel_guc_ggtt_offset(huc->rsa_data)); if (ret) { DRM_ERROR("HuC: GuC did not ack Auth request %d\n", ret); goto fail;
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
