| Message ID | 1508865685-7722-7-git-send-email-sujaritha.sundaresan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 24 Oct 2017 19:21:25 +0200, Sujaritha Sundaresan <sujaritha.sundaresan@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@intel.com> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Oscar Mateo <oscar.mateo@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@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 (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); > }
On 10/25/2017 9:49 PM, Michal Wajdeczko wrote: > On Tue, 24 Oct 2017 19:21:25 +0200, Sujaritha Sundaresan > <sujaritha.sundaresan@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@intel.com> >> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Oscar Mateo <oscar.mateo@intel.com> >> Cc: Sagar Arun Kamble <sagar.a.kamble@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); >> }
On 11/02/2017 02:23 AM, Sagar Arun Kamble wrote: > > > On 10/25/2017 9:49 PM, Michal Wajdeczko wrote: >> On Tue, 24 Oct 2017 19:21:25 +0200, Sujaritha Sundaresan >> <sujaritha.sundaresan@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@intel.com> >>> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> >>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >>> Cc: Oscar Mateo <oscar.mateo@intel.com> >>> Cc: Sagar Arun Kamble <sagar.a.kamble@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. Yes sure, I will include these changes in the next revision. Thanks for the review. >> >>> + >>> 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); >>> } > Regards, Sujaritha
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; + + ret = intel_guc_log_create(guc); + if (ret < 0) + goto err_logs; + + 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 (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); }
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@intel.com> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Oscar Mateo <oscar.mateo@intel.com> Cc: Sagar Arun Kamble <sagar.a.kamble@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