[Intel-gfx] [PATCH v8 6/6] drm/i915/guc : Decouple logs and ADS from submission
Sujaritha
sujaritha.sundaresan at intel.com
Thu Nov 2 15:59:45 UTC 2017
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 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.
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
More information about the Intel-gfx
mailing list