[Intel-gfx] [PATCH 1/4] drm/i915/guc: drop guc parameter from guc_ggtt_offset
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Thu Apr 9 16:13:40 UTC 2020
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 at intel.com>
>> Cc: Matthew Brost <matthew.brost at intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: John Harrison <john.c.harrison at intel.com>
>> ---
>> 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
>
> Michal
>
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
>> index 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.c
>> index 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;
>>
More information about the Intel-gfx
mailing list