[Intel-gfx] [PATCH v8 6/6] drm/i915/guc : Decouple logs and ADS from submission
Sagar Arun Kamble
sagar.a.kamble at intel.com
Thu Nov 2 09:23:13 UTC 2017
On 10/25/2017 9:49 PM, Michal Wajdeczko wrote:
> On Tue, 24 Oct 2017 19:21:25 +0200, Sujaritha Sundaresan
> <sujaritha.sundaresan at intel.com> wrote:
>
>> The Additional Data Struct (ADS) contains objects that are required by
>> guc post FW load and are not necessarily submission-only (although
>> that's
>> our current only use-case). If in the future we load GuC with submission
>> disabled to use some other GuC feature we might still end up requiring
>> something inside the ADS, so it makes more sense for them to be always
>> created if GuC is loaded.
>>
>> Similarly, we still want to access GuC logs even if GuC submission is
>> disable to debug issues with GuC loading or with wathever we're using
>> GuC for.
>>
>> To make a concrete example, the pages used by GuC to save state during
>> suspend are allocated as part of the ADS.
>>
>> v3: Group initialization of GuC objects
>>
>> v2: Decoupling ADS together with logs
>>
>> v3: Re-factoring code as per review (Michal)
>>
>> v4: Rebase
>>
>> v5: Separating group object initialization into next patch
>> Clarifying commit message
>>
>> v6: Reverting to goto err format (Michal)
>> Moved guc_ads functions to dedicated file
>> Rebase
>>
>> v7: Rebase
>>
>> v8: Applying review comments (Michal)
>>
>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan at intel.com>
>> Cc: Anusha Srivatsa <anusha.srivatsa at intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Oscar Mateo <oscar.mateo at intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>> ---
>> drivers/gpu/drm/i915/Makefile | 1 +
>> drivers/gpu/drm/i915/i915_guc_submission.c | 139
>> +--------------------------
>> drivers/gpu/drm/i915/intel_guc_ads.c | 149
>> +++++++++++++++++++++++++++++
>> drivers/gpu/drm/i915/intel_guc_ads.h | 33 +++++++
>> drivers/gpu/drm/i915/intel_uc.c | 38 +++++++-
>> 5 files changed, 220 insertions(+), 140 deletions(-)
>> create mode 100644 drivers/gpu/drm/i915/intel_guc_ads.c
>> create mode 100644 drivers/gpu/drm/i915/intel_guc_ads.h
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile
>> b/drivers/gpu/drm/i915/Makefile
>> index 6c3b048..d7ce07e 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -62,6 +62,7 @@ i915-y += i915_cmd_parser.o \
>> i915-y += intel_uc.o \
>> intel_uc_fw.o \
>> intel_guc.o \
>> + intel_guc_ads.o \
>> intel_guc_ct.o \
>> intel_guc_log.o \
>> intel_guc_fw.o \
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
>> b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index a2e8114..3a56429 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -72,13 +72,6 @@
>> * ELSP context descriptor dword into Work Item.
>> * See guc_wq_item_append()
>> *
>> - * ADS:
>> - * The Additional Data Struct (ADS) has pointers for different
>> buffers used by
>> - * the GuC. One single gem object contains the ADS struct itself
>> (guc_ads), the
>> - * scheduling policies (guc_policies), a structure describing a
>> collection of
>> - * register sets (guc_mmio_reg_state) and some extra pages for the
>> GuC to save
>> - * its internal state for sleep.
>> - *
>> */
>> static inline bool is_high_priority(struct i915_guc_client* client)
>> @@ -855,115 +848,6 @@ static void guc_client_free(struct
>> i915_guc_client *client)
>> kfree(client);
>> }
>> -static void guc_policy_init(struct guc_policy *policy)
>> -{
>> - policy->execution_quantum = POLICY_DEFAULT_EXECUTION_QUANTUM_US;
>> - policy->preemption_time = POLICY_DEFAULT_PREEMPTION_TIME_US;
>> - policy->fault_time = POLICY_DEFAULT_FAULT_TIME_US;
>> - policy->policy_flags = 0;
>> -}
>> -
>> -static void guc_policies_init(struct guc_policies *policies)
>> -{
>> - struct guc_policy *policy;
>> - u32 p, i;
>> -
>> - policies->dpc_promote_time = POLICY_DEFAULT_DPC_PROMOTE_TIME_US;
>> - policies->max_num_work_items = POLICY_MAX_NUM_WI;
>> -
>> - for (p = 0; p < GUC_CLIENT_PRIORITY_NUM; p++) {
>> - for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
>> - policy = &policies->policy[p][i];
>> -
>> - guc_policy_init(policy);
>> - }
>> - }
>> -
>> - policies->is_valid = 1;
>> -}
>> -
>> -/*
>> - * The first 80 dwords of the register state context, containing the
>> - * execlists and ppgtt registers.
>> - */
>> -#define LR_HW_CONTEXT_SIZE (80 * sizeof(u32))
>> -
>> -static int guc_ads_create(struct intel_guc *guc)
>> -{
>> - struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> - struct i915_vma *vma;
>> - struct page *page;
>> - /* The ads obj includes the struct itself and buffers passed to
>> GuC */
>> - struct {
>> - struct guc_ads ads;
>> - struct guc_policies policies;
>> - struct guc_mmio_reg_state reg_state;
>> - u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
>> - } __packed *blob;
>> - struct intel_engine_cs *engine;
>> - enum intel_engine_id id;
>> - const u32 skipped_offset = LRC_HEADER_PAGES * PAGE_SIZE;
>> - const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE +
>> LR_HW_CONTEXT_SIZE;
>> - u32 base;
>> -
>> - GEM_BUG_ON(guc->ads_vma);
>> -
>> - vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(sizeof(*blob)));
>> - if (IS_ERR(vma))
>> - return PTR_ERR(vma);
>> -
>> - guc->ads_vma = vma;
>> -
>> - page = i915_vma_first_page(vma);
>> - blob = kmap(page);
>> -
>> - /* GuC scheduling policies */
>> - guc_policies_init(&blob->policies);
>> -
>> - /* MMIO reg state */
>> - for_each_engine(engine, dev_priv, id) {
>> - blob->reg_state.white_list[engine->guc_id].mmio_start =
>> - engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
>> -
>> - /* Nothing to be saved or restored for now. */
>> - blob->reg_state.white_list[engine->guc_id].count = 0;
>> - }
>> -
>> - /*
>> - * 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.
>> - */
>> - blob->ads.golden_context_lrca =
>> - guc_ggtt_offset(dev_priv->kernel_context->engine[RCS].state) +
>> skipped_offset;
>> -
>> - /*
>> - * The GuC expects us to exclude the portion of the context
>> image that
>> - * it skips from the size it is to read. It starts reading from
>> after
>> - * the execlist context (so skipping the first page [PPHWSP] and 80
>> - * dwords). Weird guc is weird.
>> - */
>> - for_each_engine(engine, dev_priv, id)
>> - blob->ads.eng_state_size[engine->guc_id] =
>> engine->context_size - skipped_size;
>> -
>> - base = guc_ggtt_offset(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);
>> -
>> - kunmap(page);
>> -
>> - return 0;
>> -}
>> -
>> -static void guc_ads_destroy(struct intel_guc *guc)
>> -{
>> - i915_vma_unpin_and_release(&guc->ads_vma);
>> -}
>> -
>> /*
>> * Set up the memory resources to be shared with the GuC (via the GGTT)
>> * at firmware loading time.
>> @@ -973,7 +857,6 @@ int i915_guc_submission_init(struct
>> drm_i915_private *dev_priv)
>> struct intel_guc *guc = &dev_priv->guc;
>> struct i915_vma *vma;
>> void *vaddr;
>> - int ret;
>> if (guc->stage_desc_pool)
>> return 0;
>> @@ -988,31 +871,15 @@ int i915_guc_submission_init(struct
>> drm_i915_private *dev_priv)
>> vaddr = i915_gem_object_pin_map(guc->stage_desc_pool->obj,
>> I915_MAP_WB);
>> if (IS_ERR(vaddr)) {
>> - ret = PTR_ERR(vaddr);
>> - goto err_vma;
>> + i915_vma_unpin_and_release(&guc->stage_desc_pool);
>> + return PTR_ERR(vaddr);
>> }
>> guc->stage_desc_pool_vaddr = vaddr;
>> - ret = intel_guc_log_create(guc);
>> - if (ret < 0)
>> - goto err_vaddr;
>> -
>> - ret = guc_ads_create(guc);
>> - if (ret < 0)
>> - goto err_log;
>> -
>> ida_init(&guc->stage_ids);
>> return 0;
>> -
>> -err_log:
>> - intel_guc_log_destroy(guc);
>> -err_vaddr:
>> - i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
>> -err_vma:
>> - i915_vma_unpin_and_release(&guc->stage_desc_pool);
>> - return ret;
>> }
>> void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
>> @@ -1020,8 +887,6 @@ void i915_guc_submission_fini(struct
>> drm_i915_private *dev_priv)
>> struct intel_guc *guc = &dev_priv->guc;
>> ida_destroy(&guc->stage_ids);
>> - guc_ads_destroy(guc);
>> - intel_guc_log_destroy(guc);
>> i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
>> i915_vma_unpin_and_release(&guc->stage_desc_pool);
>> }
>> diff --git a/drivers/gpu/drm/i915/intel_guc_ads.c
>> b/drivers/gpu/drm/i915/intel_guc_ads.c
>> new file mode 100644
>> index 0000000..bff93fd
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_guc_ads.c
>> @@ -0,0 +1,149 @@
>> +/*
>> + * Copyright © 2014-2017 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person
>> obtaining a
>> + * copy of this software and associated documentation files (the
>> "Software"),
>> + * to deal in the Software without restriction, including without
>> limitation
>> + * the rights to use, copy, modify, merge, publish, distribute,
>> sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom
>> the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including
>> the next
>> + * paragraph) shall be included in all copies or substantial
>> portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
>> EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
>> OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + *
>> + */
>> +
>> +#include "intel_guc_ads.h"
>> +#include "intel_uc.h"
>> +#include "i915_drv.h"
>> +#include "i915_guc_submission.h"
>> +
>> +/**
>> + * DOC: GuC Additional Data Struct
>> + *
>> + * ADS:
>> + * The Additional Data Struct (ADS) has pointers for different
>> buffers used by
>> + * the GuC. One single gem object contains the ADS struct itself
>> (guc_ads), the
>> + * scheduling policies (guc_policies), a structure describing a
>> collection of
>> + * register sets (guc_mmio_reg_state) and some extra pages for the
>> GuC to save
>> + * its internal state for sleep.
>> + *
>> + */
>> +
>> +static void guc_policy_init(struct guc_policy *policy)
>> +{
>> + policy->execution_quantum = POLICY_DEFAULT_EXECUTION_QUANTUM_US;
>> + policy->preemption_time = POLICY_DEFAULT_PREEMPTION_TIME_US;
>> + policy->fault_time = POLICY_DEFAULT_FAULT_TIME_US;
>> + policy->policy_flags = 0;
>> +}
>> +
>> +void guc_policies_init(struct guc_policies *policies)
>> +{
>> + struct guc_policy *policy;
>> + u32 p, i;
>> +
>> + policies->dpc_promote_time = POLICY_DEFAULT_DPC_PROMOTE_TIME_US;
>> + policies->max_num_work_items = POLICY_MAX_NUM_WI;
>> +
>> + for (p = 0; p < GUC_CLIENT_PRIORITY_NUM; p++) {
>> + for (i = GUC_RENDER_ENGINE; i < GUC_MAX_ENGINES_NUM; i++) {
>> + policy = &policies->policy[p][i];
>> +
>> + guc_policy_init(policy);
>> + }
>> + }
>> +
>> + policies->is_valid = 1;
>> +}
>> +
>> +/*
>> + * The first 80 dwords of the register state context, containing the
>> + * execlists and ppgtt registers.
>> + */
>> +#define LR_HW_CONTEXT_SIZE (80 * sizeof(u32))
>> +
>> +int intel_guc_ads_create(struct intel_guc *guc)
>> +{
>> + struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> + struct i915_vma *vma;
>> + struct page *page;
>> + /* The ads obj includes the struct itself and buffers passed to
>> GuC */
>> + struct {
>> + struct guc_ads ads;
>> + struct guc_policies policies;
>> + struct guc_mmio_reg_state reg_state;
>> + u8 reg_state_buffer[GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE];
>> + } __packed *blob;
>> + struct intel_engine_cs *engine;
>> + enum intel_engine_id id;
>> + const u32 skipped_offset = LRC_HEADER_PAGES * PAGE_SIZE;
>> + const u32 skipped_size = LRC_PPHWSP_SZ * PAGE_SIZE +
>> LR_HW_CONTEXT_SIZE;
>> + u32 base;
>> +
>> + GEM_BUG_ON(guc->ads_vma);
>> +
>> + vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(sizeof(*blob)));
>> + if (IS_ERR(vma))
>> + return PTR_ERR(vma);
>> +
>> + guc->ads_vma = vma;
>> +
>> + page = i915_vma_first_page(vma);
>> + blob = kmap(page);
>> +
>> + /* GuC scheduling policies */
>> + guc_policies_init(&blob->policies);
>> +
>> + /* MMIO reg state */
>> + for_each_engine(engine, dev_priv, id) {
>> + blob->reg_state.white_list[engine->guc_id].mmio_start =
>> + engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
>> +
>> + /* Nothing to be saved or restored for now. */
>> + blob->reg_state.white_list[engine->guc_id].count = 0;
>> + }
>> +
>> + /*
>> + * 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.
>> + */
>> + blob->ads.golden_context_lrca =
>> + guc_ggtt_offset(dev_priv->kernel_context->engine[RCS].state) +
>> skipped_offset;
>> +
>> + /*
>> + * The GuC expects us to exclude the portion of the context
>> image that
>> + * it skips from the size it is to read. It starts reading from
>> after
>> + * the execlist context (so skipping the first page [PPHWSP] and 80
>> + * dwords). Weird guc is weird.
>> + */
>> + for_each_engine(engine, dev_priv, id)
>> + blob->ads.eng_state_size[engine->guc_id] =
>> engine->context_size - skipped_size;
>> +
>> + base = guc_ggtt_offset(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);
>> +
>> + kunmap(page);
>> +
>> + return 0;
>> +}
>> +
>> +void intel_guc_ads_destroy(struct intel_guc *guc)
>> +{
>> + i915_vma_unpin_and_release(&guc->ads_vma);
>> +}
>> \ No newline at end of file
>> diff --git a/drivers/gpu/drm/i915/intel_guc_ads.h
>> b/drivers/gpu/drm/i915/intel_guc_ads.h
>> new file mode 100644
>> index 0000000..3ef9a5e
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_guc_ads.h
>> @@ -0,0 +1,33 @@
>> +/*
>> + * Copyright © 2017 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person
>> obtaining a
>> + * copy of this software and associated documentation files (the
>> "Software"),
>> + * to deal in the Software without restriction, including without
>> limitation
>> + * the rights to use, copy, modify, merge, publish, distribute,
>> sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom
>> the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including
>> the next
>> + * paragraph) shall be included in all copies or substantial
>> portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
>> EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
>> OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + *
>> + */
>> +
>> +#ifndef _INTEL_GUC_ADS_H_
>> +#define _INTEL_GUC_ADS_H_
>> +
>> +struct intel_guc;
>> +
>> +int intel_guc_ads_create(struct intel_guc *guc);
>> +void intel_guc_ads_destroy(struct intel_guc *guc);
>> +
>> +#endif
>> \ No newline at end of file
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index dc978a0..12db5bd 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -24,6 +24,7 @@
>> #include "intel_uc.h"
>> #include "i915_drv.h"
>> +#include "intel_guc_ads.h"
>> #include "i915_guc_submission.h"
>> /* Reset GuC providing us with fresh state for both GuC and HuC.
>> @@ -154,6 +155,34 @@ static void guc_disable_communication(struct
>> intel_guc *guc)
>> guc->send = intel_guc_send_nop;
>> }
>> +static int guc_shared_objects_init(struct intel_guc *guc)
>> +{
>> + int ret;
>> +
>> + if (guc->ads_vma)
>> + return 0;
>
> Hmm, this 'ads' internal member, so maybe this check should
> be moved to intel_guc_ads_create() ?
>
>> +
>> + ret = intel_guc_log_create(guc);
>> + if (ret < 0)
>> + goto err_logs;
>
> Hmm, if intel_guc_log_create() failed then there should be
> no reason to call intel_guc_log_destroy()
>
>> +
>> + ret = intel_guc_ads_create(guc);
>> + if (ret < 0)
>> + goto err_ads;
>> +
>> +err_ads:
>> + intel_guc_ads_destroy(guc);
>> +err_logs:
>> + intel_guc_log_destroy(guc);
>> + return ret;
>> +}
>> +
>> +static void guc_shared_objects_fini(struct intel_guc *guc)
>> +{
>> + intel_guc_ads_destroy(guc);
>> + intel_guc_log_destroy(guc);
>> +}
>> +
>> int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>> {
>> struct intel_guc *guc = &dev_priv->guc;
>> @@ -168,6 +197,8 @@ int intel_uc_init_hw(struct drm_i915_private
>> *dev_priv)
>> /* We need to notify the guc whenever we change the GGTT */
>> i915_ggtt_enable_guc(dev_priv);
>> + ret = guc_shared_objects_init(guc);
>
> If this fails, does it make sense to continue ?
If log creation fails I don't see a reason to not init GuC and also we
can plan to split the log creation
into log vma and log runtime handling so I think it would be good if we
handle log and ads separately and not
together as shared_objects.
Another improvement can be to move this init out of intel_uc_init_hw
into i915_gem_init (and remove the stage_desc_pool check
GEM_BUG_ON in ads/log create?)
That way it is also clear that these structures are allocated only once.
>
>> +
>> if (i915_modparams.enable_guc_submission) {
>> /*
>> * This is stuff we need to have available at fw load time
>> @@ -175,7 +206,7 @@ int intel_uc_init_hw(struct drm_i915_private
>> *dev_priv)
>> */
>> ret = i915_guc_submission_init(dev_priv);
>> if (ret)
>> - goto err_guc;
>> + goto err_shared;
>> }
>> /* init WOPCM */
>> @@ -252,8 +283,8 @@ int intel_uc_init_hw(struct drm_i915_private
>> *dev_priv)
>> err_submission:
>> if (i915_modparams.enable_guc_submission)
>> i915_guc_submission_fini(dev_priv);
>> -err_guc:
>> - i915_ggtt_disable_guc(dev_priv);
>> +err_shared:
>> + guc_shared_objects_fini(guc);
>> if (i915_modparams.enable_guc_submission > 1) {
>> DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
>> @@ -285,5 +316,6 @@ void intel_uc_fini_hw(struct drm_i915_private
>> *dev_priv)
>> i915_guc_submission_fini(dev_priv);
>> }
>> + guc_shared_objects_fini(&dev_priv->guc);
>> i915_ggtt_disable_guc(dev_priv);
>> }
More information about the Intel-gfx
mailing list