| Message ID | 1478718667-5112-1-git-send-email-anusha.srivatsa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@intel.com> > Cc: Jeff Mcgee <jeff.mcgee@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> > --- > 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 >
[corrected my email in cc] On 12/11/2016 02:21, Jeff McGee wrote: > 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? I am not one of those folks but I also am sure about the proposed change. Same concern about extensibility and also usability. What is the difference between -1 and 1 for example? Is 1 equivalent to the current "must use" (2) option? And -1 is equivalent to the current "try to use" (1)? Then you got proposed 2 (load and use guc) but that does not capture the option for built-in GuC firmware Dave has planned for in his version. I don't know if that is real or not, just saying I am also not sure it is so imperative to change this at all. But if people do want to change it we should make it really good. :) One idea could be to hide the guc loading form the user altogether and hence improve usability (decrease exposed complexity) by having only two parameters; i915.enable_guc_scheduling and i915.enable_huc. Whether or not firmware would be loaded would depend on if either of the two is turned on. That would also preserve the option of current fallback or wedge behaviour for guc. Regards, Tvrtko
>-----Original Message----- >From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com] >Sent: Monday, November 14, 2016 1:35 AM >To: Mcgee, Jeff <jeff.mcgee@intel.com>; Srivatsa, Anusha ><anusha.srivatsa@intel.com> >Cc: Ursulin, Tvrtko <tvrtko.ursulin@intel.com>; intel-gfx@lists.freedesktop.org; >Vivi, Rodrigo <rodrigo.vivi@intel.com> >Subject: Re: [Intel-gfx] [PATCH] drm/i915/GuC: Combine the two kernel >parameter into one > > >[corrected my email in cc] Thankyou! >On 12/11/2016 02:21, Jeff McGee wrote: >> 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. Yes I see your point. >> 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? Two parameters was not desired. One parameter that could control the GuC functionality was something that led to this patch. >I am not one of those folks but I also am sure about the proposed change. Same >concern about extensibility and also usability. > >What is the difference between -1 and 1 for example? Is 1 equivalent to the >current "must use" (2) option? And -1 is equivalent to the current "try to use" (1)? Right now -1 is for platform default and 1 is for loading and no submission. -1: platform default for now is set to do only loading of GuC firmware, unless we change that. For now both 1 and -1 are the same. In future say if for SKL we make loading and submission as default, the code has to be changes slightly. >Then you got proposed 2 (load and use guc) but that does not capture the option >for built-in GuC firmware Dave has planned for in his version. I don't know if that >is real or not, just saying > >I am also not sure it is so imperative to change this at all. But if people do want to >change it we should make it really good. :) I agree. I sent this patch to see what people feel about it and if we have to go ahead with these changes. >One idea could be to hide the guc loading form the user altogether and hence >improve usability (decrease exposed complexity) by having only two parameters; >i915.enable_guc_scheduling and i915.enable_huc. That's a good point. But with this we will have two parameters (which kills the point of why the patch was written in the first place), then we can rather leave it the way it is. Right? -Anusha >Whether or not firmware would be loaded would depend on if either of the two is >turned on. That would also preserve the option of current fallback or wedge >behaviour for guc. > >Regards, > >Tvrtko
On 14/11/2016 17:34, Srivatsa, Anusha wrote: [snip] >> One idea could be to hide the guc loading form the user altogether and hence >> improve usability (decrease exposed complexity) by having only two parameters; >> i915.enable_guc_scheduling and i915.enable_huc. > That's a good point. But with this we will have two parameters (which kills the point of why the patch was written in the first place), then we can rather leave it the way it is. Right? For some reason I thought the HuC patch series add a another module parameter. What is the failure mode for HuC is GuC firmware loading is disabled btw? Regards, Tvrtko
>-----Original Message----- >From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com] >Sent: Tuesday, November 15, 2016 2:31 AM >To: Srivatsa, Anusha <anusha.srivatsa@intel.com>; Mcgee, Jeff ><jeff.mcgee@intel.com> >Cc: Ursulin, Tvrtko <tvrtko.ursulin@intel.com>; intel-gfx@lists.freedesktop.org; >Vivi, Rodrigo <rodrigo.vivi@intel.com> >Subject: Re: [Intel-gfx] [PATCH] drm/i915/GuC: Combine the two kernel >parameter into one > > >On 14/11/2016 17:34, Srivatsa, Anusha wrote: > >[snip] > >>> One idea could be to hide the guc loading form the user altogether >>> and hence improve usability (decrease exposed complexity) by having >>> only two parameters; i915.enable_guc_scheduling and i915.enable_huc. >> That's a good point. But with this we will have two parameters (which kills the >point of why the patch was written in the first place), then we can rather leave it >the way it is. Right? > >For some reason I thought the HuC patch series add a another module >parameter. > >What is the failure mode for HuC is GuC firmware loading is disabled btw? Hi Tvrtko, in the function intel_guc_auth_huc() there is a check to see if GuC is loaded or not. If GuC loading has failed or loading is disabled, then HuC authentication does not happen. >Regards, > >Tvrtko
>-----Original Message----- >From: Mcgee, Jeff >Sent: Tuesday, November 15, 2016 2:46 PM >To: Srivatsa, Anusha <anusha.srivatsa@intel.com> >Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>; Ursulin, Tvrtko ><tvrtko.ursulin@intel.com>; intel-gfx@lists.freedesktop.org; Vivi, Rodrigo ><rodrigo.vivi@intel.com> >Subject: Re: [Intel-gfx] [PATCH] drm/i915/GuC: Combine the two kernel >parameter into one > >On Tue, Nov 15, 2016 at 10:06:47AM -0800, Srivatsa, Anusha wrote: >> >> >> >-----Original Message----- >> >From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com] >> >Sent: Tuesday, November 15, 2016 2:31 AM >> >To: Srivatsa, Anusha <anusha.srivatsa@intel.com>; Mcgee, Jeff >> ><jeff.mcgee@intel.com> >> >Cc: Ursulin, Tvrtko <tvrtko.ursulin@intel.com>; >> >intel-gfx@lists.freedesktop.org; Vivi, Rodrigo >> ><rodrigo.vivi@intel.com> >> >Subject: Re: [Intel-gfx] [PATCH] drm/i915/GuC: Combine the two kernel >> >parameter into one >> > >> > >> >On 14/11/2016 17:34, Srivatsa, Anusha wrote: >> > >> >[snip] >> > >> >>> One idea could be to hide the guc loading form the user altogether >> >>> and hence improve usability (decrease exposed complexity) by >> >>> having only two parameters; i915.enable_guc_scheduling and >i915.enable_huc. >> >> That's a good point. But with this we will have two parameters >> >> (which kills the >> >point of why the patch was written in the first place), then we can >> >rather leave it the way it is. Right? >> > >> >For some reason I thought the HuC patch series add a another module >> >parameter. >> > >> >What is the failure mode for HuC is GuC firmware loading is disabled btw? >> >> >> Hi Tvrtko, in the function intel_guc_auth_huc() there is a check to see if GuC is >loaded or not. If GuC loading has failed or loading is disabled, then HuC >authentication does not happen. >> >Yes, GuC must authenticate HuC firmware. > >I am in favor of Tvrtko's suggestion for dropping i915.enable_guc_loading, >keeping i915.enable_guc_submission, and adding i915.enable_huc. If either of >the last two are enabled, GuC loading is implied. So kernel parameters are tied to >enabling specific functionality. I think the specific parameter for loading is legacy >from the first hurdle for GuC long ago. I assume we are not bound by ABI to keep >that around if it is no longer needed, yes? > >The other thing I would want to reconsider is the "casual" enable vs. "force" >enable options. Does anyone remember why these 2 levels of enable have been >used. Maybe this is also a legacy. Can we just do a auto (-1), disable (0), and >enable (1)? Well , -1 was for platform default, 1: if guc is available use it, 0 do not use it and 2 "force" use the guC. I agree with the above suggestion.
On Tue, Nov 15, 2016 at 10:06:47AM -0800, Srivatsa, Anusha wrote: > > > >-----Original Message----- > >From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com] > >Sent: Tuesday, November 15, 2016 2:31 AM > >To: Srivatsa, Anusha <anusha.srivatsa@intel.com>; Mcgee, Jeff > ><jeff.mcgee@intel.com> > >Cc: Ursulin, Tvrtko <tvrtko.ursulin@intel.com>; intel-gfx@lists.freedesktop.org; > >Vivi, Rodrigo <rodrigo.vivi@intel.com> > >Subject: Re: [Intel-gfx] [PATCH] drm/i915/GuC: Combine the two kernel > >parameter into one > > > > > >On 14/11/2016 17:34, Srivatsa, Anusha wrote: > > > >[snip] > > > >>> One idea could be to hide the guc loading form the user altogether > >>> and hence improve usability (decrease exposed complexity) by having > >>> only two parameters; i915.enable_guc_scheduling and i915.enable_huc. > >> That's a good point. But with this we will have two parameters (which kills the > >point of why the patch was written in the first place), then we can rather leave it > >the way it is. Right? > > > >For some reason I thought the HuC patch series add a another module > >parameter. > > > >What is the failure mode for HuC is GuC firmware loading is disabled btw? > > > Hi Tvrtko, in the function intel_guc_auth_huc() there is a check to see if GuC is loaded or not. If GuC loading has failed or loading is disabled, then HuC authentication does not happen. > Yes, GuC must authenticate HuC firmware. I am in favor of Tvrtko's suggestion for dropping i915.enable_guc_loading, keeping i915.enable_guc_submission, and adding i915.enable_huc. If either of the last two are enabled, GuC loading is implied. So kernel parameters are tied to enabling specific functionality. I think the specific parameter for loading is legacy from the first hurdle for GuC long ago. I assume we are not bound by ABI to keep that around if it is no longer needed, yes? The other thing I would want to reconsider is the "casual" enable vs. "force" enable options. Does anyone remember why these 2 levels of enable have been used. Maybe this is also a legacy. Can we just do a auto (-1), disable (0), and enable (1)?
On 15/11/2016 22:46, Jeff McGee wrote: > On Tue, Nov 15, 2016 at 10:06:47AM -0800, Srivatsa, Anusha wrote: >>> -----Original Message----- >>> From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com] >>> Sent: Tuesday, November 15, 2016 2:31 AM >>> To: Srivatsa, Anusha <anusha.srivatsa@intel.com>; Mcgee, Jeff >>> <jeff.mcgee@intel.com> >>> Cc: Ursulin, Tvrtko <tvrtko.ursulin@intel.com>; intel-gfx@lists.freedesktop.org; >>> Vivi, Rodrigo <rodrigo.vivi@intel.com> >>> Subject: Re: [Intel-gfx] [PATCH] drm/i915/GuC: Combine the two kernel >>> parameter into one >>> >>> >>> On 14/11/2016 17:34, Srivatsa, Anusha wrote: >>> >>> [snip] >>> >>>>> One idea could be to hide the guc loading form the user altogether >>>>> and hence improve usability (decrease exposed complexity) by having >>>>> only two parameters; i915.enable_guc_scheduling and i915.enable_huc. >>>> That's a good point. But with this we will have two parameters (which kills the >>> point of why the patch was written in the first place), then we can rather leave it >>> the way it is. Right? >>> >>> For some reason I thought the HuC patch series add a another module >>> parameter. >>> >>> What is the failure mode for HuC is GuC firmware loading is disabled btw? >> >> >> Hi Tvrtko, in the function intel_guc_auth_huc() there is a check to see if GuC is loaded or not. If GuC loading has failed or loading is disabled, then HuC authentication does not happen. >> > Yes, GuC must authenticate HuC firmware. I was wondering about the failure mode - or in other words does it suggest in the error message what might be the problem so it is helpful for the user. Let me check.. I did not find anything - what it will say? Just generic firmware load fetch messages with a timeout / error? > I am in favor of Tvrtko's suggestion for dropping i915.enable_guc_loading, > keeping i915.enable_guc_submission, and adding i915.enable_huc. If either of > the last two are enabled, GuC loading is implied. So kernel parameters are tied > to enabling specific functionality. I think the specific parameter for loading > is legacy from the first hurdle for GuC long ago. I assume we are not bound by > ABI to keep that around if it is no longer needed, yes? I think it is OK to change them. But there would have to be wider agreement on the end result which I think means soliciting opinions from maintainers and domain owners. > The other thing I would want to reconsider is the "casual" enable vs. "force" > enable options. Does anyone remember why these 2 levels of enable have been > used. Maybe this is also a legacy. Can we just do a auto (-1), disable (0), > and enable (1)? Yes I was wondering myself. Don't know really. Dave is not around any more so all we could do is trawl the mailing list archives in case there is something there. Regards, Tvrtko
>-----Original Message----- >From: Mcgee, Jeff >Sent: Tuesday, November 15, 2016 2:46 PM >To: Srivatsa, Anusha <anusha.srivatsa@intel.com> >Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>; Ursulin, Tvrtko ><tvrtko.ursulin@intel.com>; intel-gfx@lists.freedesktop.org; Vivi, Rodrigo ><rodrigo.vivi@intel.com> >Subject: Re: [Intel-gfx] [PATCH] drm/i915/GuC: Combine the two kernel >parameter into one > >On Tue, Nov 15, 2016 at 10:06:47AM -0800, Srivatsa, Anusha wrote: >> >> >> >-----Original Message----- >> >From: Tvrtko Ursulin [mailto:tvrtko.ursulin@linux.intel.com] >> >Sent: Tuesday, November 15, 2016 2:31 AM >> >To: Srivatsa, Anusha <anusha.srivatsa@intel.com>; Mcgee, Jeff >> ><jeff.mcgee@intel.com> >> >Cc: Ursulin, Tvrtko <tvrtko.ursulin@intel.com>; >> >intel-gfx@lists.freedesktop.org; Vivi, Rodrigo >> ><rodrigo.vivi@intel.com> >> >Subject: Re: [Intel-gfx] [PATCH] drm/i915/GuC: Combine the two kernel >> >parameter into one >> > >> > >> >On 14/11/2016 17:34, Srivatsa, Anusha wrote: >> > >> >[snip] >> > >> >>> One idea could be to hide the guc loading form the user altogether >> >>> and hence improve usability (decrease exposed complexity) by >> >>> having only two parameters; i915.enable_guc_scheduling and >i915.enable_huc. >> >> That's a good point. But with this we will have two parameters >> >> (which kills the >> >point of why the patch was written in the first place), then we can >> >rather leave it the way it is. Right? >> > >> >For some reason I thought the HuC patch series add a another module >> >parameter. >> > >> >What is the failure mode for HuC is GuC firmware loading is disabled btw? >> >> >> Hi Tvrtko, in the function intel_guc_auth_huc() there is a check to see if GuC is >loaded or not. If GuC loading has failed or loading is disabled, then HuC >authentication does not happen. >> >Yes, GuC must authenticate HuC firmware. > >I am in favor of Tvrtko's suggestion for dropping i915.enable_guc_loading, >keeping i915.enable_guc_submission, and adding i915.enable_huc. If either of >the last two are enabled, GuC loading is implied. So kernel parameters are tied to >enabling specific functionality. I think the specific parameter for loading is legacy >from the first hurdle for GuC long ago. I assume we are not bound by ABI to keep >that around if it is no longer needed, yes? Right now we don't have any parameter for HuC, so why introduce one? We can keep guc_submission, if it is enabled then loading is implied which means HuC should have gotten authenticated. Also, why not just make guc loads default? Guc_submission should be the only parameter with which we can enable or disable submission. In other words, if a platform has a guc, then load it. >The other thing I would want to reconsider is the "casual" enable vs. "force" >enable options. Does anyone remember why these 2 levels of enable have been >used. Maybe this is also a legacy. Can we just do a auto (-1), disable (0), and >enable (1)?
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 */
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 Cc: Tvrtko Ursulin <tcrtko.erdulin@intel.com> Cc: Jeff Mcgee <jeff.mcgee@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com> --- 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(-)