[Intel-gfx] [PATCH v9 7/8] drm/i915/guc : Decouple logs and ADS from submission
Michal Wajdeczko
michal.wajdeczko at intel.com
Sun Nov 12 17:12:42 UTC 2017
On Sat, 11 Nov 2017 01:06:37 +0100, 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)
>
> v9: Defining intel_guc_init function (Sagar)
> Applying review comments (Michal, Sagar)
>
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.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/intel_guc.c | 63 ++++++++++
> drivers/gpu/drm/i915/intel_guc.h | 2 +
> drivers/gpu/drm/i915/intel_guc_ads.c | 147 ++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_guc_ads.h | 33 +++++
> drivers/gpu/drm/i915/intel_guc_submission.c | 189
> +---------------------------
> drivers/gpu/drm/i915/intel_guc_submission.h | 9 +-
> drivers/gpu/drm/i915/intel_uc.c | 16 +--
> 8 files changed, 261 insertions(+), 199 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 9469c37..694e7ca 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -82,6 +82,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_fw.o \
> intel_guc_log.o \
> diff --git a/drivers/gpu/drm/i915/intel_guc.c
> b/drivers/gpu/drm/i915/intel_guc.c
> index 629ef5d..c688586 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -23,6 +23,7 @@
> */
> #include "intel_guc.h"
> +#include "intel_guc_ads.h"
> #include "intel_guc_submission.h"
> #include "i915_drv.h"
> @@ -224,6 +225,68 @@ int intel_guc_send_mmio(struct intel_guc *guc,
> const u32 *action, u32 len)
> return ret;
> }
> +/*
> + * Set up the memory resources to be shared with the GuC (via the GGTT)
> + * at firmware loading time.
> + */
> +
> +int intel_guc_init(struct intel_guc *guc)
> +{
> + int ret;
> +
> + if (guc->stage_desc_pool)
> + return 0;
> +
> + ret = guc_stage_desc_pool_create(guc);
> + if (ret)
> + return ret;
> + /*
> + * Keep static analysers happy, let them know that we allocated the
> + * vma after testing that it didn't exist earlier.
> + */
> + GEM_BUG_ON(!guc->stage_desc_pool);
> +
> + ret = guc_shared_data_create(guc);
> + if (ret)
> + goto err_stage_desc_pool;
> + GEM_BUG_ON(!guc->shared_data);
> +
> + ret = intel_guc_log_create(guc);
> + if (ret < 0)
> + goto err_shared_data;
> +
> + ret = guc_preempt_work_create(guc);
> + if (ret)
> + goto err_log;
> + GEM_BUG_ON(!guc->preempt_wq);
> +
> + ret = intel_guc_ads_create(guc);
> + if (ret < 0)
> + goto err_wq;
> + GEM_BUG_ON(!guc->ads_vma);
> +
> + return 0;
> +
> +err_wq:
> + guc_preempt_work_destroy(guc);
> +err_log:
> + intel_guc_log_destroy(guc);
> +err_shared_data:
> + guc_shared_data_destroy(guc);
> +err_stage_desc_pool:
> + guc_stage_desc_pool_destroy(guc);
> + return ret;
> +}
> +
> +void intel_guc_fini(struct intel_guc *guc)
> +{
> + intel_guc_ads_destroy(guc);
> + guc_preempt_work_destroy(guc);
> + intel_guc_log_destroy(guc);
> + guc_shared_data_destroy(guc);
> + guc_stage_desc_pool_destroy(guc);
> +}
> +
> int intel_guc_sample_forcewake(struct intel_guc *guc)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> diff --git a/drivers/gpu/drm/i915/intel_guc.h
> b/drivers/gpu/drm/i915/intel_guc.h
> index 75c4cfe..a805c79 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -121,6 +121,8 @@ static inline u32 guc_ggtt_offset(struct i915_vma
> *vma)
> void intel_guc_init_params(struct intel_guc *guc);
> int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32
> len);
> int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32
> len);
> +int intel_guc_init(struct intel_guc *guc);
> +void intel_guc_fini(struct intel_guc *guc);
> int intel_guc_sample_forcewake(struct intel_guc *guc);
> int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
> int intel_guc_suspend(struct drm_i915_private *dev_priv);
> 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..c97704d
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_guc_ads.c
> @@ -0,0 +1,147 @@
> +/*
> + * 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 "intel_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;
> +}
> +
> +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))
> +
> +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..cf5821a
> --- /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_guc_submission.c
> b/drivers/gpu/drm/i915/intel_guc_submission.c
> index cca56cc..ce7d96f1 100644
> --- a/drivers/gpu/drm/i915/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/intel_guc_submission.c
> @@ -73,13 +73,6 @@
> * ELSP context descriptor dword into Work Item.
> * See guc_add_request()
> *
> - * 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 intel_guc_client* client)
> @@ -316,7 +309,7 @@ static void guc_proc_desc_init(struct intel_guc *guc,
> desc->priority = client->priority;
> }
> -static int guc_stage_desc_pool_create(struct intel_guc *guc)
> +int guc_stage_desc_pool_create(struct intel_guc *guc)
> {
> struct i915_vma *vma;
> void *vaddr;
> @@ -340,7 +333,7 @@ static int guc_stage_desc_pool_create(struct
> intel_guc *guc)
> return 0;
> }
> -static void guc_stage_desc_pool_destroy(struct intel_guc *guc)
> +void guc_stage_desc_pool_destroy(struct intel_guc *guc)
> {
> ida_destroy(&guc->stage_ids);
> i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
> @@ -444,7 +437,7 @@ static void guc_stage_desc_fini(struct intel_guc
> *guc,
> memset(desc, 0, sizeof(*desc));
> }
> -static int guc_shared_data_create(struct intel_guc *guc)
> +int guc_shared_data_create(struct intel_guc *guc)
> {
> struct i915_vma *vma;
> void *vaddr;
> @@ -465,7 +458,7 @@ static int guc_shared_data_create(struct intel_guc
> *guc)
> return 0;
> }
> -static void guc_shared_data_destroy(struct intel_guc *guc)
> +void guc_shared_data_destroy(struct intel_guc *guc)
> {
> i915_gem_object_unpin_map(guc->shared_data->obj);
> i915_vma_unpin_and_release(&guc->shared_data);
> @@ -1095,116 +1088,7 @@ static void guc_clients_destroy(struct intel_guc
> *guc)
> guc_client_free(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);
> -}
> -
> -static int guc_preempt_work_create(struct intel_guc *guc)
> +int guc_preempt_work_create(struct intel_guc *guc)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> struct intel_engine_cs *engine;
> @@ -1236,7 +1120,7 @@ static int guc_preempt_work_create(struct
> intel_guc *guc)
> return 0;
> }
> -static void guc_preempt_work_destroy(struct intel_guc *guc)
> +void guc_preempt_work_destroy(struct intel_guc *guc)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> struct intel_engine_cs *engine;
> @@ -1249,67 +1133,6 @@ static void guc_preempt_work_destroy(struct
> intel_guc *guc)
> guc->preempt_wq = NULL;
> }
> -/*
> - * Set up the memory resources to be shared with the GuC (via the GGTT)
> - * at firmware loading time.
> - */
> -int intel_guc_submission_init(struct intel_guc *guc)
> -{
> - int ret;
> -
> - if (guc->stage_desc_pool)
> - return 0;
> -
> - ret = guc_stage_desc_pool_create(guc);
> - if (ret)
> - return ret;
> - /*
> - * Keep static analysers happy, let them know that we allocated the
> - * vma after testing that it didn't exist earlier.
> - */
> - GEM_BUG_ON(!guc->stage_desc_pool);
> -
> - ret = guc_shared_data_create(guc);
> - if (ret)
> - goto err_stage_desc_pool;
> - GEM_BUG_ON(!guc->shared_data);
> -
> - ret = intel_guc_log_create(guc);
> - if (ret < 0)
> - goto err_shared_data;
> -
> - ret = guc_preempt_work_create(guc);
> - if (ret)
> - goto err_log;
> - GEM_BUG_ON(!guc->preempt_wq);
> -
> - ret = guc_ads_create(guc);
> - if (ret < 0)
> - goto err_wq;
> - GEM_BUG_ON(!guc->ads_vma);
> -
> - return 0;
> -
> -err_wq:
> - guc_preempt_work_destroy(guc);
> -err_log:
> - intel_guc_log_destroy(guc);
> -err_shared_data:
> - guc_shared_data_destroy(guc);
> -err_stage_desc_pool:
> - guc_stage_desc_pool_destroy(guc);
> - return ret;
> -}
> -
> -void intel_guc_submission_fini(struct intel_guc *guc)
> -{
> - guc_ads_destroy(guc);
> - guc_preempt_work_destroy(guc);
> - intel_guc_log_destroy(guc);
> - guc_shared_data_destroy(guc);
> - guc_stage_desc_pool_destroy(guc);
> -}
> -
> static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
> {
> struct intel_rps *rps = &dev_priv->gt_pm.rps;
> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.h
> b/drivers/gpu/drm/i915/intel_guc_submission.h
> index 3e46916..15af6ef 100644
> --- a/drivers/gpu/drm/i915/intel_guc_submission.h
> +++ b/drivers/gpu/drm/i915/intel_guc_submission.h
> @@ -72,9 +72,14 @@ struct intel_guc_client {
> u64 submissions[I915_NUM_ENGINES];
> };
> -int intel_guc_submission_init(struct intel_guc *guc);
> int intel_guc_submission_enable(struct intel_guc *guc);
> void intel_guc_submission_disable(struct intel_guc *guc);
> -void intel_guc_submission_fini(struct intel_guc *guc);
> +
> +int guc_stage_desc_pool_create(struct intel_guc *guc);
> +int guc_shared_data_create(struct intel_guc *guc);
> +int guc_preempt_work_create(struct intel_guc *guc);
> +void guc_stage_desc_pool_destroy(struct intel_guc *guc);
> +void guc_shared_data_destroy(struct intel_guc *guc);
> +void guc_preempt_work_destroy(struct intel_guc *guc);
Do we really need to export all these functions ?
Maybe all you need is to split guc_submission_init/fini
and move only unrelated code to guc_init/fini
Also note that files exported by this file shall start
with "intel_guc_submission_" prefix
> #endif
> diff --git a/drivers/gpu/drm/i915/intel_uc.c
> b/drivers/gpu/drm/i915/intel_uc.c
> index 980de7a..0e2c4a6 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -170,16 +170,6 @@ 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);
> - if (i915_modparams.enable_guc) {
> - /*
> - * This is stuff we need to have available at fw load time
> - * if we are planning to enable submission later
> - */
> - ret = intel_guc_submission_init(guc);
> - if (ret)
> - goto err_guc;
> - }
> -
Is it ok to just remove this code now?
This may render driver unusable until next patch, where this code
will be reintroduced again...
> /* init WOPCM */
> I915_WRITE(GUC_WOPCM_SIZE, intel_guc_wopcm_size(dev_priv));
> I915_WRITE(DMA_GUC_WOPCM_OFFSET,
> @@ -253,9 +243,7 @@ int intel_uc_init_hw(struct drm_i915_private
> *dev_priv)
> guc_capture_load_err_log(guc);
> err_submission:
> if (i915_modparams.enable_guc)
> - intel_guc_submission_fini(guc);
> -err_guc:
> - i915_ggtt_disable_guc(dev_priv);
> + intel_guc_fini(guc);
> if (i915_modparams.enable_guc > 1) {
> DRM_ERROR("GuC init failed. Firmware loading disabled.\n");
> @@ -284,7 +272,7 @@ void intel_uc_fini_hw(struct drm_i915_private
> *dev_priv)
> if (i915_modparams.enable_guc) {
> gen9_disable_guc_interrupts(dev_priv);
> - intel_guc_submission_fini(&dev_priv->guc);
> + intel_guc_fini(&dev_priv->guc);
> }
> i915_ggtt_disable_guc(dev_priv);
More information about the Intel-gfx
mailing list