On Wed, Nov 09, 2016 at 11:11:07AM -0800, Anusha Srivatsa wrote:
> Replace i915.enable_guc_loading and i915.enable_guc_submission
> with a single parameter - i915.enable_guc. Where:
>
> -1 : Platform default (Only load GuC)
> 0 : Do not use GuC
> 1 : Load GuC, do not use Command Submission through GuC
> 2 : Load and use GuC for Command Submission
>
I think this approach could get ugly as we add more GuC functionality and
the list of combinations under this one parameter grows.
What is the issue we are trying to solve? I thought it was that folks
didn't like that we had an option to just load GuC and do nothing with it.
Can those folks please comment?
Jeff
> Cc: Tvrtko Ursulin <tcrtko.erdulin@xxxxxxxxx>
> Cc: Jeff Mcgee <jeff.mcgee@xxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_guc_submission.c | 16 +++++++++---
> drivers/gpu/drm/i915/i915_params.c | 17 +++++--------
> drivers/gpu/drm/i915/i915_params.h | 3 +--
> drivers/gpu/drm/i915/intel_guc.h | 2 ++
> drivers/gpu/drm/i915/intel_guc_loader.c | 39 +++++++++++++++++++-----------
> drivers/gpu/drm/i915/intel_lrc.c | 14 ++++++++---
> 6 files changed, 56 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 7809acf..a5ef268 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -1471,13 +1471,15 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
> const size_t poolsize = GUC_MAX_GPU_CONTEXTS * ctxsize;
> const size_t gemsize = round_up(poolsize, PAGE_SIZE);
> struct intel_guc *guc = &dev_priv->guc;
> + struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> +
> struct i915_vma *vma;
>
> /* Wipe bitmap & delete client in case of reinitialisation */
> bitmap_clear(guc->doorbell_bitmap, 0, GUC_MAX_DOORBELLS);
> i915_guc_submission_disable(dev_priv);
>
> - if (!i915.enable_guc_submission)
> + if (!guc_fw->enable_guc_submission)
> return 0; /* not enabled */
>
> if (guc->ctx_pool_vma)
> @@ -1629,7 +1631,9 @@ void i915_guc_capture_logs(struct drm_i915_private *dev_priv)
>
> void i915_guc_flush_logs(struct drm_i915_private *dev_priv)
> {
> - if (!i915.enable_guc_submission || (i915.guc_log_level < 0))
> + struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> +
> + if (!guc_fw->enable_guc_submission || (i915.guc_log_level < 0))
> return;
>
> /* First disable the interrupts, will be renabled afterwards */
> @@ -1649,7 +1653,9 @@ void i915_guc_flush_logs(struct drm_i915_private *dev_priv)
>
> void i915_guc_unregister(struct drm_i915_private *dev_priv)
> {
> - if (!i915.enable_guc_submission)
> + struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> +
> + if (!guc_fw->enable_guc_submission)
> return;
>
> mutex_lock(&dev_priv->drm.struct_mutex);
> @@ -1659,7 +1665,9 @@ void i915_guc_unregister(struct drm_i915_private *dev_priv)
>
> void i915_guc_register(struct drm_i915_private *dev_priv)
> {
> - if (!i915.enable_guc_submission)
> + struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> +
> + if (!guc_fw->enable_guc_submission)
> return;
>
> mutex_lock(&dev_priv->drm.struct_mutex);
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 629e433..4600f83 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -56,8 +56,7 @@ struct i915_params i915 __read_mostly = {
> .verbose_state_checks = 1,
> .nuclear_pageflip = 0,
> .edp_vswing = 0,
> - .enable_guc_loading = 0,
> - .enable_guc_submission = 0,
> + .enable_guc = 0,
> .guc_log_level = -1,
> .enable_dp_mst = true,
> .inject_load_failure = 0,
> @@ -215,15 +214,11 @@ MODULE_PARM_DESC(edp_vswing,
> "(0=use value from vbt [default], 1=low power swing(200mV),"
> "2=default swing(400mV))");
>
> -module_param_named_unsafe(enable_guc_loading, i915.enable_guc_loading, int, 0400);
> -MODULE_PARM_DESC(enable_guc_loading,
> - "Enable GuC firmware loading "
> - "(-1=auto, 0=never [default], 1=if available, 2=required)");
> -
> -module_param_named_unsafe(enable_guc_submission, i915.enable_guc_submission, int, 0400);
> -MODULE_PARM_DESC(enable_guc_submission,
> - "Enable GuC submission "
> - "(-1=auto, 0=never [default], 1=if available, 2=required)");
> +module_param_named_unsafe(enable_guc, i915.enable_guc, int, 0400);
> +MODULE_PARM_DESC(enable_guc,
> + "Enable GuC firmware loading and loading and submission"
> + "(-1=auto [default], 0=do not use GuC, 1=load only,"
> + "2=loading and submission)");
>
> module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
> MODULE_PARM_DESC(guc_log_level,
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index 94efc89..d235595 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -45,8 +45,7 @@ struct i915_params {
> int enable_ips;
> int invert_brightness;
> int enable_cmd_parser;
> - int enable_guc_loading;
> - int enable_guc_submission;
> + int enable_guc;
> int guc_log_level;
> int use_mmio_flip;
> int mmio_debug;
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 67a500c..1131a73 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -114,6 +114,8 @@ struct intel_uc_fw {
> struct drm_i915_gem_object *uc_fw_obj;
> enum intel_uc_fw_status fetch_status;
> enum intel_uc_fw_status load_status;
> + bool enable_guc_loading;
> + bool enable_guc_submission;
>
> uint16_t major_ver_wanted;
> uint16_t minor_ver_wanted;
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 11e3bbb..621f588 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -189,6 +189,7 @@ static u32 get_core_family(struct drm_i915_private *dev_priv)
> static void guc_params_init(struct drm_i915_private *dev_priv)
> {
> struct intel_guc *guc = &dev_priv->guc;
> + struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> u32 params[GUC_CTL_MAX_DWORDS];
> int i;
>
> @@ -226,7 +227,7 @@ static void guc_params_init(struct drm_i915_private *dev_priv)
> }
>
> /* If GuC submission is enabled, set up additional parameters here */
> - if (i915.enable_guc_submission) {
> + if (guc_fw->enable_guc_submission) {
> u32 pgs = i915_ggtt_offset(dev_priv->guc.ctx_pool_vma);
> u32 ctx_in_16 = GUC_MAX_GPU_CONTEXTS / 16;
>
> @@ -462,7 +463,7 @@ int intel_guc_setup(struct drm_device *dev)
> intel_uc_fw_status_repr(guc_fw->load_status));
>
> /* Loading forbidden, or no firmware to load? */
> - if (!i915.enable_guc_loading) {
> + if (!guc_fw->enable_guc_loading) {
> err = 0;
> goto fail;
> } else if (fw_path == NULL) {
> @@ -533,7 +534,7 @@ int intel_guc_setup(struct drm_device *dev)
>
> intel_guc_auth_huc(dev);
>
> - if (i915.enable_guc_submission) {
> + if (guc_fw->enable_guc_submission) {
> if (i915.guc_log_level >= 0)
> gen9_enable_guc_interrupts(dev_priv);
>
> @@ -562,9 +563,9 @@ int intel_guc_setup(struct drm_device *dev)
> * nonfatal error (i.e. it doesn't prevent driver load, but
> * marks the GPU as wedged until reset).
> */
> - if (i915.enable_guc_loading > 1) {
> + if (guc_fw->enable_guc_loading) {
> ret = -EIO;
> - } else if (i915.enable_guc_submission > 1) {
> + } else if (guc_fw->enable_guc_submission) {
> ret = -EIO;
> } else {
> ret = 0;
> @@ -579,7 +580,7 @@ int intel_guc_setup(struct drm_device *dev)
> else
> DRM_WARN("GuC firmware load failed: %d\n", err);
>
> - if (i915.enable_guc_submission) {
> + if (guc_fw->enable_guc_submission) {
> if (fw_path == NULL)
> DRM_INFO("GuC submission without firmware not supported\n");
> if (ret == 0)
> @@ -587,7 +588,7 @@ int intel_guc_setup(struct drm_device *dev)
> else
> DRM_ERROR("GuC init failed: %d\n", ret);
> }
> - i915.enable_guc_submission = 0;
> + guc_fw->enable_guc_submission = false;
>
> return ret;
> }
> @@ -745,14 +746,24 @@ void intel_guc_init(struct drm_device *dev)
> const char *fw_path;
>
> if (!HAS_GUC(dev)) {
> - i915.enable_guc_loading = 0;
> - i915.enable_guc_submission = 0;
> + guc_fw->enable_guc_loading = false;
> + guc_fw->enable_guc_submission = false;
> } else {
> /* A negative value means "use platform default" */
> - if (i915.enable_guc_loading < 0)
> - i915.enable_guc_loading = HAS_GUC_UCODE(dev);
> - if (i915.enable_guc_submission < 0)
> - i915.enable_guc_submission = HAS_GUC_SCHED(dev);
> + if (i915.enable_guc < 0) {
> + guc_fw->enable_guc_loading = HAS_GUC_UCODE(dev);
> + } else if (i915.enable_guc == 0) {
> + guc_fw->enable_guc_loading = false;
> + guc_fw->enable_guc_submission = false;
> + } else if (i915.enable_guc == 1) {
> + guc_fw->enable_guc_loading = HAS_GUC_UCODE(dev);
> + guc_fw->enable_guc_submission = false;
> + } else if (i915.enable_guc == 2) {
> + guc_fw->enable_guc_loading = HAS_GUC_UCODE(dev);
> + guc_fw->enable_guc_submission = HAS_GUC_SCHED(dev);
> + } else {
> + DRM_ERROR("Invalid parameter to enable_guc\n");
> + }
> }
>
> if (!HAS_GUC_UCODE(dev)) {
> @@ -779,7 +790,7 @@ void intel_guc_init(struct drm_device *dev)
> guc_fw->load_status = UC_FIRMWARE_NONE;
>
> /* Early (and silent) return if GuC loading is disabled */
> - if (!i915.enable_guc_loading)
> + if (!guc_fw->enable_guc_loading)
> return;
> if (fw_path == NULL)
> return;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index dde04b764..69ddb93 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -641,6 +641,8 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
> {
> struct intel_engine_cs *engine = request->engine;
> struct intel_context *ce = &request->ctx->engine[engine->id];
> + struct drm_i915_private *dev_priv = engine->i915;
> + struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> int ret;
>
> /* Flush enough space to reduce the likelihood of waiting after
> @@ -661,7 +663,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
> if (ret)
> return ret;
>
> - if (i915.enable_guc_submission) {
> + if (guc_fw->enable_guc_submission) {
> /*
> * Check that the GuC has space for the request before
> * going any further, as the i915_add_request() call
> @@ -695,7 +697,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
> return 0;
>
> err_unreserve:
> - if (i915.enable_guc_submission)
> + if (guc_fw->enable_guc_submission)
> i915_guc_wq_unreserve(request);
> err_unpin:
> intel_lr_context_unpin(request->ctx, engine);
> @@ -706,6 +708,9 @@ static int intel_lr_context_pin(struct i915_gem_context *ctx,
> struct intel_engine_cs *engine)
> {
> struct intel_context *ce = &ctx->engine[engine->id];
> + struct drm_i915_private *dev_priv = engine->i915;
> + struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
> +
> void *vaddr;
> int ret;
>
> @@ -738,7 +743,7 @@ static int intel_lr_context_pin(struct i915_gem_context *ctx,
> ce->state->obj->mm.dirty = true;
>
> /* Invalidate GuC TLB. */
> - if (i915.enable_guc_submission) {
> + if (guc_fw->enable_guc_submission) {
> struct drm_i915_private *dev_priv = ctx->i915;
> I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
> }
> @@ -1285,6 +1290,7 @@ static void reset_common_ring(struct intel_engine_cs *engine,
> struct drm_i915_private *dev_priv = engine->i915;
> struct execlist_port *port = engine->execlist_port;
> struct intel_context *ce = &request->ctx->engine[engine->id];
> + struct intel_uc_fw *guc_fw = &dev_priv->guc.guc_fw;
>
> /* We want a simple context + ring to execute the breadcrumb update.
> * We cannot rely on the context being intact across the GPU hang,
> @@ -1305,7 +1311,7 @@ static void reset_common_ring(struct intel_engine_cs *engine,
> request->ring->last_retired_head = -1;
> intel_ring_update_space(request->ring);
>
> - if (i915.enable_guc_submission)
> + if (guc_fw->enable_guc_submission)
> return;
>
> /* Catch up with any missed context-switch interrupts */
> --
> 2.7.4
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
