| Message ID | 1508280649-26705-3-git-send-email-sujaritha.sundaresan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Sujaritha Sundaresan (2017-10-17 23:50:47) > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index dd141b2..5b9bdd0 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -3201,9 +3201,12 @@ static inline unsigned int i915_sg_segment_size(void) > */ > #define HAS_GUC(dev_priv) ((dev_priv)->info.has_guc) > #define HAS_GUC_CT(dev_priv) ((dev_priv)->info.has_guc_ct) > -#define HAS_GUC_UCODE(dev_priv) (HAS_GUC(dev_priv)) > -#define HAS_GUC_SCHED(dev_priv) (HAS_GUC(dev_priv)) > -#define HAS_HUC_UCODE(dev_priv) (HAS_GUC(dev_priv)) > +#define HAS_GUC_UCODE(dev_priv) ((dev_priv)->guc.fw.path != NULL) > +#define HAS_HUC_UCODE(dev_priv) ((dev_priv)->huc.fw.path != NULL) > + > +#define NEEDS_GUC_LOADING(dev_priv) \ > + (HAS_GUC(dev_priv) && \ > + (i915_modparams.enable_guc_submission || HAS_HUC_UCODE(dev_priv))) NEEDS_GUC_FW ? -Chris
On Tue, 2017-10-17 at 15:50 -0700, Sujaritha Sundaresan wrote: > We currently have two module parameters that control GuC: > "enable_guc_loading" and "enable_guc_submission". Whenever > we need i915_modparams.enable_guc_submission=1, we also need > enable_guc_loading=1. We also need enable_guc_loading=1 when > we want to verify the HuC, which is every time we have a HuC > (but all platforms with HuC have a GuC and viceversa). I already gave comments about clarifying the commit message, that does not seem to have been addressed. Regards, Joonas
On Wed, 18 Oct 2017 00:50:47 +0200, Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> wrote: > We currently have two module parameters that control GuC: > "enable_guc_loading" and "enable_guc_submission". Whenever > we need i915_modparams.enable_guc_submission=1, we also need > enable_guc_loading=1. We also need enable_guc_loading=1 when > we want to verify the HuC, which is every time we have a HuC > (but all platforms with HuC have a GuC and viceversa). > > v2: Clarifying the commit message (Anusha) > > v3: Unify seq_puts messages, Re-factoring code as per review (Michal) > > v4: Rebase > > v5: Separating message unification into a separate patch > > v6: Re-factoring code (Sagar, Michal) > Rebase > > v7: Applying review comments (Sagar) > Rebase > > Suggested by: Oscar Mateo <oscar.mateo@intel.com> > Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > Cc: Oscar Mateo <oscar.mateo@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 6 +-- > drivers/gpu/drm/i915/i915_drv.h | 9 +++-- > drivers/gpu/drm/i915/i915_gem_context.c | 2 +- > drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +- > drivers/gpu/drm/i915/i915_irq.c | 2 +- > drivers/gpu/drm/i915/i915_params.c | 4 -- > drivers/gpu/drm/i915/i915_params.h | 1 - > drivers/gpu/drm/i915/intel_uc.c | 69 > ++++++++++++++++++--------------- > drivers/gpu/drm/i915/intel_uncore.c | 3 +- > 9 files changed, 50 insertions(+), 48 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index ac25d63..bc31769 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -2361,7 +2361,7 @@ static int i915_huc_load_status_info(struct > seq_file *m, void *data) > struct drm_i915_private *dev_priv = node_to_i915(m->private); > struct intel_uc_fw *huc_fw = &dev_priv->huc.fw; > - if (!HAS_HUC_UCODE(dev_priv)) { > + if (!HAS_GUC(dev_priv)) { > seq_puts(m, "not supported\n"); > return 0; > } > @@ -2397,7 +2397,7 @@ static int i915_guc_load_status_info(struct > seq_file *m, void *data) > struct intel_uc_fw *guc_fw = &dev_priv->guc.fw; > u32 tmp, i; > - if (!HAS_GUC_UCODE(dev_priv)) { > + if (!HAS_GUC(dev_priv)) { > seq_puts(m, "not supported\n"); > return 0; > } > @@ -2496,7 +2496,7 @@ static bool check_guc_submission(struct seq_file > *m) > if (!guc->execbuf_client) { > seq_printf(m, "GuC submission %s\n", > - HAS_GUC_SCHED(dev_priv) ? > + HAS_GUC(dev_priv) ? > "disabled" : > "not supported"); Btw, there is also 3rd case: "failed" when we have Guc, but something went wrong with fw loading or enabling... > return false; > diff --git a/drivers/gpu/drm/i915/i915_drv.h > b/drivers/gpu/drm/i915/i915_drv.h > index dd141b2..5b9bdd0 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -3201,9 +3201,12 @@ static inline unsigned int > i915_sg_segment_size(void) > */ > #define HAS_GUC(dev_priv) ((dev_priv)->info.has_guc) > #define HAS_GUC_CT(dev_priv) ((dev_priv)->info.has_guc_ct) > -#define HAS_GUC_UCODE(dev_priv) (HAS_GUC(dev_priv)) > -#define HAS_GUC_SCHED(dev_priv) (HAS_GUC(dev_priv)) > -#define HAS_HUC_UCODE(dev_priv) (HAS_GUC(dev_priv)) > +#define HAS_GUC_UCODE(dev_priv) ((dev_priv)->guc.fw.path != NULL) > +#define HAS_HUC_UCODE(dev_priv) ((dev_priv)->huc.fw.path != NULL) > + > +#define NEEDS_GUC_LOADING(dev_priv) \ > + (HAS_GUC(dev_priv) && \ > + (i915_modparams.enable_guc_submission || HAS_HUC_UCODE(dev_priv))) Hmm, so by the moment we add huc fw definition to the driver we will enable guc loading, and as huc is always present with guc, this will silently enable guc loading for all platforms with guc(huc) and there will be no option to turn this off ... What if we don't care about Huc functionality ? If there will be Huc fw, both guc and huc will be loaded. If there will be no Huc fw on the system (but it will be defined in driver) then we will generate errors from Huc firware loading, and likely try to load Guc firmware for no purpose ... > #define HAS_RESOURCE_STREAMER(dev_priv) > ((dev_priv)->info.has_resource_streamer) > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c > b/drivers/gpu/drm/i915/i915_gem_context.c > index 5bf96a2..692d609 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -314,7 +314,7 @@ static u32 default_desc_template(const struct > drm_i915_private *i915, > * present or not in use we still need a small bias as ring wraparound > * at offset 0 sometimes hangs. No idea why. > */ > - if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading) > + if (NEEDS_GUC_LOADING(dev_priv)) > ctx->ggtt_offset_bias = GUC_WOPCM_TOP; > else > ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE; > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 527a2d2..0bbc8f0 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -3481,7 +3481,7 @@ int i915_ggtt_probe_hw(struct drm_i915_private > *dev_priv) > * currently don't have any bits spare to pass in this upper > * restriction! > */ > - if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading) { > + if (NEEDS_GUC_LOADING(dev_priv)) { > ggtt->base.total = min_t(u64, ggtt->base.total, GUC_GGTT_TOP); > ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total); > } > diff --git a/drivers/gpu/drm/i915/i915_irq.c > b/drivers/gpu/drm/i915/i915_irq.c > index b1296a5..ec76aac 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -4026,7 +4026,7 @@ void intel_irq_init(struct drm_i915_private > *dev_priv) > for (i = 0; i < MAX_L3_SLICES; ++i) > dev_priv->l3_parity.remap_info[i] = NULL; > - if (HAS_GUC_SCHED(dev_priv)) > + if (HAS_GUC(dev_priv)) > dev_priv->pm_guc_events = GEN9_GUC_TO_HOST_INT_EVENT; > /* Let's track the enabled rps events */ > diff --git a/drivers/gpu/drm/i915/i915_params.c > b/drivers/gpu/drm/i915/i915_params.c > index b4faeb6..1c25f45 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -162,10 +162,6 @@ struct i915_params i915_modparams __read_mostly = { > "(0=use value from vbt [default], 1=low power swing(200mV)," > "2=default swing(400mV))"); > -i915_param_named_unsafe(enable_guc_loading, int, 0400, > - "Enable GuC firmware loading " > - "(-1=auto, 0=never [default], 1=if available, 2=required)"); > - > i915_param_named_unsafe(enable_guc_submission, int, 0400, > "Enable GuC submission " > "(-1=auto, 0=never [default], 1=if available, 2=required)"); > diff --git a/drivers/gpu/drm/i915/i915_params.h > b/drivers/gpu/drm/i915/i915_params.h > index c729226..9e1e231 100644 > --- a/drivers/gpu/drm/i915/i915_params.h > +++ b/drivers/gpu/drm/i915/i915_params.h > @@ -44,7 +44,6 @@ > param(int, disable_power_well, -1) \ > param(int, enable_ips, 1) \ > param(int, invert_brightness, 0) \ > - param(int, enable_guc_loading, 0) \ > param(int, enable_guc_submission, 0) \ > param(int, guc_log_level, -1) \ > param(char *, guc_firmware_path, NULL) \ > diff --git a/drivers/gpu/drm/i915/intel_uc.c > b/drivers/gpu/drm/i915/intel_uc.c > index 25bd162..df281525 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -49,36 +49,44 @@ static int __intel_uc_reset_hw(struct > drm_i915_private *dev_priv) > void intel_uc_sanitize_options(struct drm_i915_private *dev_priv) > { > + /* Verify Hardware support */ > if (!HAS_GUC(dev_priv)) { > - if (i915_modparams.enable_guc_loading > 0 || > - i915_modparams.enable_guc_submission > 0) > - DRM_INFO("Ignoring GuC options, no hardware\n"); > - > - i915_modparams.enable_guc_loading = 0; > - i915_modparams.enable_guc_submission = 0; > + if (i915_modparams.enable_guc_submission > 0) > + DRM_INFO("Ignoring GuC submission enable", > + "no hardware\n"); Hmm, maybe to give clearer message to the user: DRM_INFO("Ignoring option %s - no hardware", "enable_guc_submission"); > + i915_modparams.enable_guc_submission = 0; > return; > } > - /* A negative value means "use platform default" */ > - if (i915_modparams.enable_guc_loading < 0) > - i915_modparams.enable_guc_loading = HAS_GUC_UCODE(dev_priv); > + /* Verify Firmware support */ > + if (!HAS_GUC_UCODE(dev_priv)) { > + if (i915_modparams.enable_guc_submission == 1) { Hmm, I really don't like that we leak here enable_guc_submission = 2 knowing that it will not work ... > + DRM_INFO("Ignoring GuC submission enable", > + "no firmware\n"); DRM_INFO("Ignoring option %s - no firmware", "enable_guc_submission"); > + i915_modparams.enable_guc_submission = 0; > + return; > + } > - /* Verify firmware version */ > - if (i915_modparams.enable_guc_loading) { > - if (HAS_HUC_UCODE(dev_priv)) > - intel_huc_select_fw(&dev_priv->huc); > + if (i915_modparams.enable_guc_submission < 0) { > + i915_modparams.enable_guc_submission = 0; > + return; > + } > - if (intel_guc_fw_select(&dev_priv->guc)) > - i915_modparams.enable_guc_loading = 0; > + /* > + * If "required" (> 1), let it continue and we will fail later > + * due to the lack of firmware Hmm, but doing so will break the goal of the 'sanitize' options function > + */ > + > } > - /* Can't enable guc submission without guc loaded */ > - if (!i915_modparams.enable_guc_loading) > - i915_modparams.enable_guc_submission = 0; > + /* > + * A negative value means "use paltform default" (enabled if we have Typo > + * survived to get here) > + */ > - /* A negative value means "use platform default" */ > if (i915_modparams.enable_guc_submission < 0) > - i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv); > + i915_modparams.enable_guc_submission = HAS_GUC(dev_priv); > + Drop above extra line > } > void intel_uc_init_early(struct drm_i915_private *dev_priv) > @@ -88,6 +96,8 @@ void intel_uc_init_early(struct drm_i915_private > *dev_priv) > void intel_uc_init_fw(struct drm_i915_private *dev_priv) > { > + if (!HAS_GUC(dev_priv)) > + return; Do we need this now ? > intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw); > intel_uc_fw_fetch(dev_priv, &dev_priv->guc.fw); > } > @@ -154,7 +164,7 @@ int intel_uc_init_hw(struct drm_i915_private > *dev_priv) > struct intel_guc *guc = &dev_priv->guc; > int ret, attempts; > - if (!i915_modparams.enable_guc_loading) > + if (!NEEDS_GUC_LOADING(dev_priv)) > return 0; > guc_disable_communication(guc); > @@ -250,22 +260,17 @@ int intel_uc_init_hw(struct drm_i915_private > *dev_priv) > err_guc: > i915_ggtt_disable_guc(dev_priv); > - if (i915_modparams.enable_guc_loading > 1 || > - i915_modparams.enable_guc_submission > 1) { > + if (i915_modparams.enable_guc_submission > 1) { > DRM_ERROR("GuC init failed. Firmware loading disabled.\n"); > ret = -EIO; > + } else if (i915_modparams.enable_guc_submission == 1) { > + DRM_NOTE("Falling back from GuC submission to execlist mode\n"); > + i915_modparams.enable_guc_submission = 0; > + ret = 0; > } else { > - DRM_NOTE("GuC init failed. Firmware loading disabled.\n"); > ret = 0; > } > - if (i915_modparams.enable_guc_submission) { > - i915_modparams.enable_guc_submission = 0; > - DRM_NOTE("Falling back from GuC submission to execlist mode\n"); > - } > - > - i915_modparams.enable_guc_loading = 0; > - > return ret; > } > @@ -273,7 +278,7 @@ void intel_uc_fini_hw(struct drm_i915_private > *dev_priv) > { > guc_free_load_err_log(&dev_priv->guc); > - if (!i915_modparams.enable_guc_loading) > + if (!NEEDS_GUC_LOADING(dev_priv)) > return; > if (i915_modparams.enable_guc_submission) > diff --git a/drivers/gpu/drm/i915/intel_uncore.c > b/drivers/gpu/drm/i915/intel_uncore.c > index 20e3c65c..c631b0e 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -1803,8 +1803,7 @@ int intel_guc_reset(struct drm_i915_private > *dev_priv) > { > int ret; > - if (!HAS_GUC(dev_priv)) > - return -EINVAL; > + GEM_BUG_ON(!HAS_GUC(dev_priv)); Hmm, this looks like unrelated change - please move to separate patch > intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); > ret = gen6_hw_domain_reset(dev_priv, GEN9_GRDOM_GUC);
On 10/18/2017 03:58 AM, Joonas Lahtinen wrote: > On Tue, 2017-10-17 at 15:50 -0700, Sujaritha Sundaresan wrote: >> We currently have two module parameters that control GuC: >> "enable_guc_loading" and "enable_guc_submission". Whenever >> we need i915_modparams.enable_guc_submission=1, we also need >> enable_guc_loading=1. We also need enable_guc_loading=1 when >> we want to verify the HuC, which is every time we have a HuC >> (but all platforms with HuC have a GuC and viceversa). > I already gave comments about clarifying the commit message, that does > not seem to have been addressed. > > Regards, Joonas Sorry about that, I was hoping to fix the commit message after this revision. I will fix the commit message in the next revised series. Thanks for the review. Regards, Sujaritha
On Wed, 2017-10-18 at 09:25 -0700, Sujaritha wrote: > > On 10/18/2017 03:58 AM, Joonas Lahtinen wrote: > > On Tue, 2017-10-17 at 15:50 -0700, Sujaritha Sundaresan wrote: > > > We currently have two module parameters that control GuC: > > > "enable_guc_loading" and "enable_guc_submission". Whenever > > > we need i915_modparams.enable_guc_submission=1, we also need > > > enable_guc_loading=1. We also need enable_guc_loading=1 when > > > we want to verify the HuC, which is every time we have a HuC > > > (but all platforms with HuC have a GuC and viceversa). > > > > I already gave comments about clarifying the commit message, that does > > not seem to have been addressed. > > > > Regards, Joonas > > > Sorry about that, I was hoping to fix the commit message after this > revision. > I will fix the commit message in the next revised series. In general, it's good idea to take review comments for all of the patches and apply all the changes at once to avoid double digit version numbers :) Every revision you send to the list, should be one you feel is complete and ready to be merged. A new revision should only be needed when something new is brought up in review (usually due to the changes made from the last revision). The CI system automatically picks up all patch series from the mailing list, so sending intermediate versions will only cause unnecessary load. If you feel the need for iterating some feature more, it's adviseable to join the IRC channel and ping the reviewer there. That way you don't have to wait for a day or day and a half to get comments for the small changes. Regards, Joonas
On 10/18/2017 09:50 AM, Joonas Lahtinen wrote: > On Wed, 2017-10-18 at 09:25 -0700, Sujaritha wrote: >> On 10/18/2017 03:58 AM, Joonas Lahtinen wrote: >>> On Tue, 2017-10-17 at 15:50 -0700, Sujaritha Sundaresan wrote: >>>> We currently have two module parameters that control GuC: >>>> "enable_guc_loading" and "enable_guc_submission". Whenever >>>> we need i915_modparams.enable_guc_submission=1, we also need >>>> enable_guc_loading=1. We also need enable_guc_loading=1 when >>>> we want to verify the HuC, which is every time we have a HuC >>>> (but all platforms with HuC have a GuC and viceversa). >>> I already gave comments about clarifying the commit message, that does >>> not seem to have been addressed. >>> >>> Regards, Joonas >> >> Sorry about that, I was hoping to fix the commit message after this >> revision. >> I will fix the commit message in the next revised series. > In general, it's good idea to take review comments for all of the > patches and apply all the changes at once to avoid double digit version > numbers :) > > Every revision you send to the list, should be one you feel is complete > and ready to be merged. A new revision should only be needed when > something new is brought up in review (usually due to the changes made > from the last revision). > > The CI system automatically picks up all patch series from the mailing > list, so sending intermediate versions will only cause unnecessary > load. If you feel the need for iterating some feature more, it's > adviseable to join the IRC channel and ping the reviewer there. That > way you don't have to wait for a day or day and a half to get comments > for the small changes. > > Regards, Joonas Will make sure to keep this in mind before sending the next patches. Thanks, Sujaritha
On 10/17/2017 03:57 PM, Chris Wilson wrote: > Quoting Sujaritha Sundaresan (2017-10-17 23:50:47) >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index dd141b2..5b9bdd0 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -3201,9 +3201,12 @@ static inline unsigned int i915_sg_segment_size(void) >> */ >> #define HAS_GUC(dev_priv) ((dev_priv)->info.has_guc) >> #define HAS_GUC_CT(dev_priv) ((dev_priv)->info.has_guc_ct) >> -#define HAS_GUC_UCODE(dev_priv) (HAS_GUC(dev_priv)) >> -#define HAS_GUC_SCHED(dev_priv) (HAS_GUC(dev_priv)) >> -#define HAS_HUC_UCODE(dev_priv) (HAS_GUC(dev_priv)) >> +#define HAS_GUC_UCODE(dev_priv) ((dev_priv)->guc.fw.path != NULL) >> +#define HAS_HUC_UCODE(dev_priv) ((dev_priv)->huc.fw.path != NULL) >> + >> +#define NEEDS_GUC_LOADING(dev_priv) \ >> + (HAS_GUC(dev_priv) && \ >> + (i915_modparams.enable_guc_submission || HAS_HUC_UCODE(dev_priv))) > NEEDS_GUC_FW ? > -Chris Sure. Will do. Thanks for the review. Sujaritha
On 10/18/2017 04:53 AM, Michal Wajdeczko wrote: > On Wed, 18 Oct 2017 00:50:47 +0200, Sujaritha Sundaresan > <sujaritha.sundaresan@intel.com> wrote: > >> We currently have two module parameters that control GuC: >> "enable_guc_loading" and "enable_guc_submission". Whenever >> we need i915_modparams.enable_guc_submission=1, we also need >> enable_guc_loading=1. We also need enable_guc_loading=1 when >> we want to verify the HuC, which is every time we have a HuC >> (but all platforms with HuC have a GuC and viceversa). >> >> v2: Clarifying the commit message (Anusha) >> >> v3: Unify seq_puts messages, Re-factoring code as per review (Michal) >> >> v4: Rebase >> >> v5: Separating message unification into a separate patch >> >> v6: Re-factoring code (Sagar, Michal) >> Rebase >> >> v7: Applying review comments (Sagar) >> Rebase >> >> Suggested by: Oscar Mateo <oscar.mateo@intel.com> >> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> >> Cc: Oscar Mateo <oscar.mateo@intel.com> >> --- >> drivers/gpu/drm/i915/i915_debugfs.c | 6 +-- >> drivers/gpu/drm/i915/i915_drv.h | 9 +++-- >> drivers/gpu/drm/i915/i915_gem_context.c | 2 +- >> drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +- >> drivers/gpu/drm/i915/i915_irq.c | 2 +- >> drivers/gpu/drm/i915/i915_params.c | 4 -- >> drivers/gpu/drm/i915/i915_params.h | 1 - >> drivers/gpu/drm/i915/intel_uc.c | 69 >> ++++++++++++++++++--------------- >> drivers/gpu/drm/i915/intel_uncore.c | 3 +- >> 9 files changed, 50 insertions(+), 48 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c >> b/drivers/gpu/drm/i915/i915_debugfs.c >> index ac25d63..bc31769 100644 >> --- a/drivers/gpu/drm/i915/i915_debugfs.c >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >> @@ -2361,7 +2361,7 @@ static int i915_huc_load_status_info(struct >> seq_file *m, void *data) >> struct drm_i915_private *dev_priv = node_to_i915(m->private); >> struct intel_uc_fw *huc_fw = &dev_priv->huc.fw; >> - if (!HAS_HUC_UCODE(dev_priv)) { >> + if (!HAS_GUC(dev_priv)) { >> seq_puts(m, "not supported\n"); >> return 0; >> } >> @@ -2397,7 +2397,7 @@ static int i915_guc_load_status_info(struct >> seq_file *m, void *data) >> struct intel_uc_fw *guc_fw = &dev_priv->guc.fw; >> u32 tmp, i; >> - if (!HAS_GUC_UCODE(dev_priv)) { >> + if (!HAS_GUC(dev_priv)) { >> seq_puts(m, "not supported\n"); >> return 0; >> } >> @@ -2496,7 +2496,7 @@ static bool check_guc_submission(struct >> seq_file *m) >> if (!guc->execbuf_client) { >> seq_printf(m, "GuC submission %s\n", >> - HAS_GUC_SCHED(dev_priv) ? >> + HAS_GUC(dev_priv) ? >> "disabled" : >> "not supported"); > > Btw, there is also 3rd case: "failed" when we have Guc, but something > went > wrong with fw loading or enabling... > >> return false; >> diff --git a/drivers/gpu/drm/i915/i915_drv.h >> b/drivers/gpu/drm/i915/i915_drv.h >> index dd141b2..5b9bdd0 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -3201,9 +3201,12 @@ static inline unsigned int >> i915_sg_segment_size(void) >> */ >> #define HAS_GUC(dev_priv) ((dev_priv)->info.has_guc) >> #define HAS_GUC_CT(dev_priv) ((dev_priv)->info.has_guc_ct) >> -#define HAS_GUC_UCODE(dev_priv) (HAS_GUC(dev_priv)) >> -#define HAS_GUC_SCHED(dev_priv) (HAS_GUC(dev_priv)) >> -#define HAS_HUC_UCODE(dev_priv) (HAS_GUC(dev_priv)) >> +#define HAS_GUC_UCODE(dev_priv) ((dev_priv)->guc.fw.path != NULL) >> +#define HAS_HUC_UCODE(dev_priv) ((dev_priv)->huc.fw.path != NULL) >> + >> +#define NEEDS_GUC_LOADING(dev_priv) \ >> + (HAS_GUC(dev_priv) && \ >> + (i915_modparams.enable_guc_submission || HAS_HUC_UCODE(dev_priv))) > > Hmm, so by the moment we add huc fw definition to the driver we will > enable guc loading, and as huc is always present with guc, this will > silently enable guc loading for all platforms with guc(huc) and there > will be no option to turn this off ... > > What if we don't care about Huc functionality ? > > If there will be Huc fw, both guc and huc will be loaded. > If there will be no Huc fw on the system (but it will be defined in > driver) > then we will generate errors from Huc firware loading, and likely try > to load > Guc firmware for no purpose ... Hmm, ok. So will the change will be to make the macro NEEDS_GUC_LOADING(dev_priv) \ (HAS_GUC(dev_priv) && i915_mopdarams.enable_guc_submission) ? > >> #define HAS_RESOURCE_STREAMER(dev_priv) >> ((dev_priv)->info.has_resource_streamer) >> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c >> b/drivers/gpu/drm/i915/i915_gem_context.c >> index 5bf96a2..692d609 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_context.c >> +++ b/drivers/gpu/drm/i915/i915_gem_context.c >> @@ -314,7 +314,7 @@ static u32 default_desc_template(const struct >> drm_i915_private *i915, >> * present or not in use we still need a small bias as ring >> wraparound >> * at offset 0 sometimes hangs. No idea why. >> */ >> - if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading) >> + if (NEEDS_GUC_LOADING(dev_priv)) >> ctx->ggtt_offset_bias = GUC_WOPCM_TOP; >> else >> ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE; >> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c >> b/drivers/gpu/drm/i915/i915_gem_gtt.c >> index 527a2d2..0bbc8f0 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c >> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c >> @@ -3481,7 +3481,7 @@ int i915_ggtt_probe_hw(struct drm_i915_private >> *dev_priv) >> * currently don't have any bits spare to pass in this upper >> * restriction! >> */ >> - if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading) { >> + if (NEEDS_GUC_LOADING(dev_priv)) { >> ggtt->base.total = min_t(u64, ggtt->base.total, GUC_GGTT_TOP); >> ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total); >> } >> diff --git a/drivers/gpu/drm/i915/i915_irq.c >> b/drivers/gpu/drm/i915/i915_irq.c >> index b1296a5..ec76aac 100644 >> --- a/drivers/gpu/drm/i915/i915_irq.c >> +++ b/drivers/gpu/drm/i915/i915_irq.c >> @@ -4026,7 +4026,7 @@ void intel_irq_init(struct drm_i915_private >> *dev_priv) >> for (i = 0; i < MAX_L3_SLICES; ++i) >> dev_priv->l3_parity.remap_info[i] = NULL; >> - if (HAS_GUC_SCHED(dev_priv)) >> + if (HAS_GUC(dev_priv)) >> dev_priv->pm_guc_events = GEN9_GUC_TO_HOST_INT_EVENT; >> /* Let's track the enabled rps events */ >> diff --git a/drivers/gpu/drm/i915/i915_params.c >> b/drivers/gpu/drm/i915/i915_params.c >> index b4faeb6..1c25f45 100644 >> --- a/drivers/gpu/drm/i915/i915_params.c >> +++ b/drivers/gpu/drm/i915/i915_params.c >> @@ -162,10 +162,6 @@ struct i915_params i915_modparams __read_mostly = { >> "(0=use value from vbt [default], 1=low power swing(200mV)," >> "2=default swing(400mV))"); >> -i915_param_named_unsafe(enable_guc_loading, int, 0400, >> - "Enable GuC firmware loading " >> - "(-1=auto, 0=never [default], 1=if available, 2=required)"); >> - >> i915_param_named_unsafe(enable_guc_submission, int, 0400, >> "Enable GuC submission " >> "(-1=auto, 0=never [default], 1=if available, 2=required)"); >> diff --git a/drivers/gpu/drm/i915/i915_params.h >> b/drivers/gpu/drm/i915/i915_params.h >> index c729226..9e1e231 100644 >> --- a/drivers/gpu/drm/i915/i915_params.h >> +++ b/drivers/gpu/drm/i915/i915_params.h >> @@ -44,7 +44,6 @@ >> param(int, disable_power_well, -1) \ >> param(int, enable_ips, 1) \ >> param(int, invert_brightness, 0) \ >> - param(int, enable_guc_loading, 0) \ >> param(int, enable_guc_submission, 0) \ >> param(int, guc_log_level, -1) \ >> param(char *, guc_firmware_path, NULL) \ >> diff --git a/drivers/gpu/drm/i915/intel_uc.c >> b/drivers/gpu/drm/i915/intel_uc.c >> index 25bd162..df281525 100644 >> --- a/drivers/gpu/drm/i915/intel_uc.c >> +++ b/drivers/gpu/drm/i915/intel_uc.c >> @@ -49,36 +49,44 @@ static int __intel_uc_reset_hw(struct >> drm_i915_private *dev_priv) >> void intel_uc_sanitize_options(struct drm_i915_private *dev_priv) >> { >> + /* Verify Hardware support */ >> if (!HAS_GUC(dev_priv)) { >> - if (i915_modparams.enable_guc_loading > 0 || >> - i915_modparams.enable_guc_submission > 0) >> - DRM_INFO("Ignoring GuC options, no hardware\n"); >> - >> - i915_modparams.enable_guc_loading = 0; >> - i915_modparams.enable_guc_submission = 0; >> + if (i915_modparams.enable_guc_submission > 0) >> + DRM_INFO("Ignoring GuC submission enable", >> + "no hardware\n"); > > Hmm, maybe to give clearer message to the user: > > DRM_INFO("Ignoring option %s - no hardware", "enable_guc_submission"); Will do. > >> + i915_modparams.enable_guc_submission = 0; >> return; >> } >> - /* A negative value means "use platform default" */ >> - if (i915_modparams.enable_guc_loading < 0) >> - i915_modparams.enable_guc_loading = HAS_GUC_UCODE(dev_priv); >> + /* Verify Firmware support */ >> + if (!HAS_GUC_UCODE(dev_priv)) { >> + if (i915_modparams.enable_guc_submission == 1) { > > Hmm, I really don't like that we leak here enable_guc_submission = 2 > knowing that it will not work ... I agree. A 2 is "required", so I will change it to (i915_modparams.enable_guc_submission > 0). > >> + DRM_INFO("Ignoring GuC submission enable", >> + "no firmware\n"); > > DRM_INFO("Ignoring option %s - no firmware", "enable_guc_submission"); > >> + i915_modparams.enable_guc_submission = 0; >> + return; >> + } >> - /* Verify firmware version */ >> - if (i915_modparams.enable_guc_loading) { >> - if (HAS_HUC_UCODE(dev_priv)) >> - intel_huc_select_fw(&dev_priv->huc); >> + if (i915_modparams.enable_guc_submission < 0) { >> + i915_modparams.enable_guc_submission = 0; >> + return; >> + } >> - if (intel_guc_fw_select(&dev_priv->guc)) >> - i915_modparams.enable_guc_loading = 0; >> + /* >> + * If "required" (> 1), let it continue and we will fail later >> + * due to the lack of firmware > > Hmm, but doing so will break the goal of the 'sanitize' options function > >> + */ >> + >> } >> - /* Can't enable guc submission without guc loaded */ >> - if (!i915_modparams.enable_guc_loading) >> - i915_modparams.enable_guc_submission = 0; >> + /* >> + * A negative value means "use paltform default" (enabled if we >> have > > Typo > >> + * survived to get here) >> + */ >> - /* A negative value means "use platform default" */ >> if (i915_modparams.enable_guc_submission < 0) >> - i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv); >> + i915_modparams.enable_guc_submission = HAS_GUC(dev_priv); >> + > > Drop above extra line > >> } >> void intel_uc_init_early(struct drm_i915_private *dev_priv) >> @@ -88,6 +96,8 @@ void intel_uc_init_early(struct drm_i915_private >> *dev_priv) >> void intel_uc_init_fw(struct drm_i915_private *dev_priv) >> { >> + if (!HAS_GUC(dev_priv)) >> + return; > > Do we need this now ? > >> intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw); >> intel_uc_fw_fetch(dev_priv, &dev_priv->guc.fw); >> } >> @@ -154,7 +164,7 @@ int intel_uc_init_hw(struct drm_i915_private >> *dev_priv) >> struct intel_guc *guc = &dev_priv->guc; >> int ret, attempts; >> - if (!i915_modparams.enable_guc_loading) >> + if (!NEEDS_GUC_LOADING(dev_priv)) >> return 0; >> guc_disable_communication(guc); >> @@ -250,22 +260,17 @@ int intel_uc_init_hw(struct drm_i915_private >> *dev_priv) >> err_guc: >> i915_ggtt_disable_guc(dev_priv); >> - if (i915_modparams.enable_guc_loading > 1 || >> - i915_modparams.enable_guc_submission > 1) { >> + if (i915_modparams.enable_guc_submission > 1) { >> DRM_ERROR("GuC init failed. Firmware loading disabled.\n"); >> ret = -EIO; >> + } else if (i915_modparams.enable_guc_submission == 1) { >> + DRM_NOTE("Falling back from GuC submission to execlist >> mode\n"); >> + i915_modparams.enable_guc_submission = 0; >> + ret = 0; >> } else { >> - DRM_NOTE("GuC init failed. Firmware loading disabled.\n"); >> ret = 0; >> } >> - if (i915_modparams.enable_guc_submission) { >> - i915_modparams.enable_guc_submission = 0; >> - DRM_NOTE("Falling back from GuC submission to execlist >> mode\n"); >> - } >> - >> - i915_modparams.enable_guc_loading = 0; >> - >> return ret; >> } >> @@ -273,7 +278,7 @@ void intel_uc_fini_hw(struct drm_i915_private >> *dev_priv) >> { >> guc_free_load_err_log(&dev_priv->guc); >> - if (!i915_modparams.enable_guc_loading) >> + if (!NEEDS_GUC_LOADING(dev_priv)) >> return; >> if (i915_modparams.enable_guc_submission) >> diff --git a/drivers/gpu/drm/i915/intel_uncore.c >> b/drivers/gpu/drm/i915/intel_uncore.c >> index 20e3c65c..c631b0e 100644 >> --- a/drivers/gpu/drm/i915/intel_uncore.c >> +++ b/drivers/gpu/drm/i915/intel_uncore.c >> @@ -1803,8 +1803,7 @@ int intel_guc_reset(struct drm_i915_private >> *dev_priv) >> { >> int ret; >> - if (!HAS_GUC(dev_priv)) >> - return -EINVAL; >> + GEM_BUG_ON(!HAS_GUC(dev_priv)); > > Hmm, this looks like unrelated change - please move to separate patch Will do > >> intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); >> ret = gen6_hw_domain_reset(dev_priv, GEN9_GRDOM_GUC);
On 10/18/2017 04:53 AM, Michal Wajdeczko wrote: > On Wed, 18 Oct 2017 00:50:47 +0200, Sujaritha Sundaresan > <sujaritha.sundaresan@intel.com> wrote: > >> We currently have two module parameters that control GuC: >> "enable_guc_loading" and "enable_guc_submission". Whenever >> we need i915_modparams.enable_guc_submission=1, we also need >> enable_guc_loading=1. We also need enable_guc_loading=1 when >> we want to verify the HuC, which is every time we have a HuC >> (but all platforms with HuC have a GuC and viceversa). >> >> v2: Clarifying the commit message (Anusha) >> >> v3: Unify seq_puts messages, Re-factoring code as per review (Michal) >> >> v4: Rebase >> >> v5: Separating message unification into a separate patch >> >> v6: Re-factoring code (Sagar, Michal) >> Rebase >> >> v7: Applying review comments (Sagar) >> Rebase >> >> Suggested by: Oscar Mateo <oscar.mateo@intel.com> >> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> >> Cc: Oscar Mateo <oscar.mateo@intel.com> >> --- >> drivers/gpu/drm/i915/i915_debugfs.c | 6 +-- >> drivers/gpu/drm/i915/i915_drv.h | 9 +++-- >> drivers/gpu/drm/i915/i915_gem_context.c | 2 +- >> drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +- >> drivers/gpu/drm/i915/i915_irq.c | 2 +- >> drivers/gpu/drm/i915/i915_params.c | 4 -- >> drivers/gpu/drm/i915/i915_params.h | 1 - >> drivers/gpu/drm/i915/intel_uc.c | 69 >> ++++++++++++++++++--------------- >> drivers/gpu/drm/i915/intel_uncore.c | 3 +- >> 9 files changed, 50 insertions(+), 48 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c >> b/drivers/gpu/drm/i915/i915_debugfs.c >> index ac25d63..bc31769 100644 >> --- a/drivers/gpu/drm/i915/i915_debugfs.c >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >> @@ -2361,7 +2361,7 @@ static int i915_huc_load_status_info(struct >> seq_file *m, void *data) >> struct drm_i915_private *dev_priv = node_to_i915(m->private); >> struct intel_uc_fw *huc_fw = &dev_priv->huc.fw; >> - if (!HAS_HUC_UCODE(dev_priv)) { >> + if (!HAS_GUC(dev_priv)) { >> seq_puts(m, "not supported\n"); >> return 0; >> } >> @@ -2397,7 +2397,7 @@ static int i915_guc_load_status_info(struct >> seq_file *m, void *data) >> struct intel_uc_fw *guc_fw = &dev_priv->guc.fw; >> u32 tmp, i; >> - if (!HAS_GUC_UCODE(dev_priv)) { >> + if (!HAS_GUC(dev_priv)) { >> seq_puts(m, "not supported\n"); >> return 0; >> } >> @@ -2496,7 +2496,7 @@ static bool check_guc_submission(struct >> seq_file *m) >> if (!guc->execbuf_client) { >> seq_printf(m, "GuC submission %s\n", >> - HAS_GUC_SCHED(dev_priv) ? >> + HAS_GUC(dev_priv) ? >> "disabled" : >> "not supported"); > > Btw, there is also 3rd case: "failed" when we have Guc, but something > went > wrong with fw loading or enabling... > >> return false; >> diff --git a/drivers/gpu/drm/i915/i915_drv.h >> b/drivers/gpu/drm/i915/i915_drv.h >> index dd141b2..5b9bdd0 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -3201,9 +3201,12 @@ static inline unsigned int >> i915_sg_segment_size(void) >> */ >> #define HAS_GUC(dev_priv) ((dev_priv)->info.has_guc) >> #define HAS_GUC_CT(dev_priv) ((dev_priv)->info.has_guc_ct) >> -#define HAS_GUC_UCODE(dev_priv) (HAS_GUC(dev_priv)) >> -#define HAS_GUC_SCHED(dev_priv) (HAS_GUC(dev_priv)) >> -#define HAS_HUC_UCODE(dev_priv) (HAS_GUC(dev_priv)) >> +#define HAS_GUC_UCODE(dev_priv) ((dev_priv)->guc.fw.path != NULL) >> +#define HAS_HUC_UCODE(dev_priv) ((dev_priv)->huc.fw.path != NULL) >> + >> +#define NEEDS_GUC_LOADING(dev_priv) \ >> + (HAS_GUC(dev_priv) && \ >> + (i915_modparams.enable_guc_submission || HAS_HUC_UCODE(dev_priv))) > > Hmm, so by the moment we add huc fw definition to the driver we will > enable guc loading, and as huc is always present with guc, this will > silently enable guc loading for all platforms with guc(huc) and there > will be no option to turn this off ... > > What if we don't care about Huc functionality ? > > If there will be Huc fw, both guc and huc will be loaded. > If there will be no Huc fw on the system (but it will be defined in > driver) > then we will generate errors from Huc firware loading, and likely try > to load > Guc firmware for no purpose ... > Yes that is true. So if the user wants to avoid the GuC firmware from getting loaded, they must not have a HuC firmware to be loaded; in addition to not using submission. I think if this is included in the commit message, it will make things clearer. >> #define HAS_RESOURCE_STREAMER(dev_priv) >> ((dev_priv)->info.has_resource_streamer) >> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c >> b/drivers/gpu/drm/i915/i915_gem_context.c >> index 5bf96a2..692d609 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_context.c >> +++ b/drivers/gpu/drm/i915/i915_gem_context.c >> @@ -314,7 +314,7 @@ static u32 default_desc_template(const struct >> drm_i915_private *i915, >> * present or not in use we still need a small bias as ring >> wraparound >> * at offset 0 sometimes hangs. No idea why. >> */ >> - if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading) >> + if (NEEDS_GUC_LOADING(dev_priv)) >> ctx->ggtt_offset_bias = GUC_WOPCM_TOP; >> else >> ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE; >> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c >> b/drivers/gpu/drm/i915/i915_gem_gtt.c >> index 527a2d2..0bbc8f0 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c >> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c >> @@ -3481,7 +3481,7 @@ int i915_ggtt_probe_hw(struct drm_i915_private >> *dev_priv) >> * currently don't have any bits spare to pass in this upper >> * restriction! >> */ >> - if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading) { >> + if (NEEDS_GUC_LOADING(dev_priv)) { >> ggtt->base.total = min_t(u64, ggtt->base.total, GUC_GGTT_TOP); >> ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total); >> } >> diff --git a/drivers/gpu/drm/i915/i915_irq.c >> b/drivers/gpu/drm/i915/i915_irq.c >> index b1296a5..ec76aac 100644 >> --- a/drivers/gpu/drm/i915/i915_irq.c >> +++ b/drivers/gpu/drm/i915/i915_irq.c >> @@ -4026,7 +4026,7 @@ void intel_irq_init(struct drm_i915_private >> *dev_priv) >> for (i = 0; i < MAX_L3_SLICES; ++i) >> dev_priv->l3_parity.remap_info[i] = NULL; >> - if (HAS_GUC_SCHED(dev_priv)) >> + if (HAS_GUC(dev_priv)) >> dev_priv->pm_guc_events = GEN9_GUC_TO_HOST_INT_EVENT; >> /* Let's track the enabled rps events */ >> diff --git a/drivers/gpu/drm/i915/i915_params.c >> b/drivers/gpu/drm/i915/i915_params.c >> index b4faeb6..1c25f45 100644 >> --- a/drivers/gpu/drm/i915/i915_params.c >> +++ b/drivers/gpu/drm/i915/i915_params.c >> @@ -162,10 +162,6 @@ struct i915_params i915_modparams __read_mostly = { >> "(0=use value from vbt [default], 1=low power swing(200mV)," >> "2=default swing(400mV))"); >> -i915_param_named_unsafe(enable_guc_loading, int, 0400, >> - "Enable GuC firmware loading " >> - "(-1=auto, 0=never [default], 1=if available, 2=required)"); >> - >> i915_param_named_unsafe(enable_guc_submission, int, 0400, >> "Enable GuC submission " >> "(-1=auto, 0=never [default], 1=if available, 2=required)"); >> diff --git a/drivers/gpu/drm/i915/i915_params.h >> b/drivers/gpu/drm/i915/i915_params.h >> index c729226..9e1e231 100644 >> --- a/drivers/gpu/drm/i915/i915_params.h >> +++ b/drivers/gpu/drm/i915/i915_params.h >> @@ -44,7 +44,6 @@ >> param(int, disable_power_well, -1) \ >> param(int, enable_ips, 1) \ >> param(int, invert_brightness, 0) \ >> - param(int, enable_guc_loading, 0) \ >> param(int, enable_guc_submission, 0) \ >> param(int, guc_log_level, -1) \ >> param(char *, guc_firmware_path, NULL) \ >> diff --git a/drivers/gpu/drm/i915/intel_uc.c >> b/drivers/gpu/drm/i915/intel_uc.c >> index 25bd162..df281525 100644 >> --- a/drivers/gpu/drm/i915/intel_uc.c >> +++ b/drivers/gpu/drm/i915/intel_uc.c >> @@ -49,36 +49,44 @@ static int __intel_uc_reset_hw(struct >> drm_i915_private *dev_priv) >> void intel_uc_sanitize_options(struct drm_i915_private *dev_priv) >> { >> + /* Verify Hardware support */ >> if (!HAS_GUC(dev_priv)) { >> - if (i915_modparams.enable_guc_loading > 0 || >> - i915_modparams.enable_guc_submission > 0) >> - DRM_INFO("Ignoring GuC options, no hardware\n"); >> - >> - i915_modparams.enable_guc_loading = 0; >> - i915_modparams.enable_guc_submission = 0; >> + if (i915_modparams.enable_guc_submission > 0) >> + DRM_INFO("Ignoring GuC submission enable", >> + "no hardware\n"); > > Hmm, maybe to give clearer message to the user: > > DRM_INFO("Ignoring option %s - no hardware", "enable_guc_submission"); > >> + i915_modparams.enable_guc_submission = 0; >> return; >> } >> - /* A negative value means "use platform default" */ >> - if (i915_modparams.enable_guc_loading < 0) >> - i915_modparams.enable_guc_loading = HAS_GUC_UCODE(dev_priv); >> + /* Verify Firmware support */ >> + if (!HAS_GUC_UCODE(dev_priv)) { >> + if (i915_modparams.enable_guc_submission == 1) { > > Hmm, I really don't like that we leak here enable_guc_submission = 2 > knowing that it will not work ... > >> + DRM_INFO("Ignoring GuC submission enable", >> + "no firmware\n"); > > DRM_INFO("Ignoring option %s - no firmware", "enable_guc_submission"); > >> + i915_modparams.enable_guc_submission = 0; >> + return; >> + } >> - /* Verify firmware version */ >> - if (i915_modparams.enable_guc_loading) { >> - if (HAS_HUC_UCODE(dev_priv)) >> - intel_huc_select_fw(&dev_priv->huc); >> + if (i915_modparams.enable_guc_submission < 0) { >> + i915_modparams.enable_guc_submission = 0; >> + return; >> + } >> - if (intel_guc_fw_select(&dev_priv->guc)) >> - i915_modparams.enable_guc_loading = 0; >> + /* >> + * If "required" (> 1), let it continue and we will fail later >> + * due to the lack of firmware > > Hmm, but doing so will break the goal of the 'sanitize' options function > >> + */ >> + >> } >> - /* Can't enable guc submission without guc loaded */ >> - if (!i915_modparams.enable_guc_loading) >> - i915_modparams.enable_guc_submission = 0; >> + /* >> + * A negative value means "use paltform default" (enabled if we >> have > > Typo > >> + * survived to get here) >> + */ >> - /* A negative value means "use platform default" */ >> if (i915_modparams.enable_guc_submission < 0) >> - i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv); >> + i915_modparams.enable_guc_submission = HAS_GUC(dev_priv); >> + > > Drop above extra line > >> } >> void intel_uc_init_early(struct drm_i915_private *dev_priv) >> @@ -88,6 +96,8 @@ void intel_uc_init_early(struct drm_i915_private >> *dev_priv) >> void intel_uc_init_fw(struct drm_i915_private *dev_priv) >> { >> + if (!HAS_GUC(dev_priv)) >> + return; > > Do we need this now ? > >> intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw); >> intel_uc_fw_fetch(dev_priv, &dev_priv->guc.fw); >> } >> @@ -154,7 +164,7 @@ int intel_uc_init_hw(struct drm_i915_private >> *dev_priv) >> struct intel_guc *guc = &dev_priv->guc; >> int ret, attempts; >> - if (!i915_modparams.enable_guc_loading) >> + if (!NEEDS_GUC_LOADING(dev_priv)) >> return 0; >> guc_disable_communication(guc); >> @@ -250,22 +260,17 @@ int intel_uc_init_hw(struct drm_i915_private >> *dev_priv) >> err_guc: >> i915_ggtt_disable_guc(dev_priv); >> - if (i915_modparams.enable_guc_loading > 1 || >> - i915_modparams.enable_guc_submission > 1) { >> + if (i915_modparams.enable_guc_submission > 1) { >> DRM_ERROR("GuC init failed. Firmware loading disabled.\n"); >> ret = -EIO; >> + } else if (i915_modparams.enable_guc_submission == 1) { >> + DRM_NOTE("Falling back from GuC submission to execlist >> mode\n"); >> + i915_modparams.enable_guc_submission = 0; >> + ret = 0; >> } else { >> - DRM_NOTE("GuC init failed. Firmware loading disabled.\n"); >> ret = 0; >> } >> - if (i915_modparams.enable_guc_submission) { >> - i915_modparams.enable_guc_submission = 0; >> - DRM_NOTE("Falling back from GuC submission to execlist >> mode\n"); >> - } >> - >> - i915_modparams.enable_guc_loading = 0; >> - >> return ret; >> } >> @@ -273,7 +278,7 @@ void intel_uc_fini_hw(struct drm_i915_private >> *dev_priv) >> { >> guc_free_load_err_log(&dev_priv->guc); >> - if (!i915_modparams.enable_guc_loading) >> + if (!NEEDS_GUC_LOADING(dev_priv)) >> return; >> if (i915_modparams.enable_guc_submission) >> diff --git a/drivers/gpu/drm/i915/intel_uncore.c >> b/drivers/gpu/drm/i915/intel_uncore.c >> index 20e3c65c..c631b0e 100644 >> --- a/drivers/gpu/drm/i915/intel_uncore.c >> +++ b/drivers/gpu/drm/i915/intel_uncore.c >> @@ -1803,8 +1803,7 @@ int intel_guc_reset(struct drm_i915_private >> *dev_priv) >> { >> int ret; >> - if (!HAS_GUC(dev_priv)) >> - return -EINVAL; >> + GEM_BUG_ON(!HAS_GUC(dev_priv)); > > Hmm, this looks like unrelated change - please move to separate patch > >> intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); >> ret = gen6_hw_domain_reset(dev_priv, GEN9_GRDOM_GUC); Thanks for the review, Regards, Sujaritha
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index ac25d63..bc31769 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2361,7 +2361,7 @@ static int i915_huc_load_status_info(struct seq_file *m, void *data) struct drm_i915_private *dev_priv = node_to_i915(m->private); struct intel_uc_fw *huc_fw = &dev_priv->huc.fw; - if (!HAS_HUC_UCODE(dev_priv)) { + if (!HAS_GUC(dev_priv)) { seq_puts(m, "not supported\n"); return 0; } @@ -2397,7 +2397,7 @@ static int i915_guc_load_status_info(struct seq_file *m, void *data) struct intel_uc_fw *guc_fw = &dev_priv->guc.fw; u32 tmp, i; - if (!HAS_GUC_UCODE(dev_priv)) { + if (!HAS_GUC(dev_priv)) { seq_puts(m, "not supported\n"); return 0; } @@ -2496,7 +2496,7 @@ static bool check_guc_submission(struct seq_file *m) if (!guc->execbuf_client) { seq_printf(m, "GuC submission %s\n", - HAS_GUC_SCHED(dev_priv) ? + HAS_GUC(dev_priv) ? "disabled" : "not supported"); return false; diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index dd141b2..5b9bdd0 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3201,9 +3201,12 @@ static inline unsigned int i915_sg_segment_size(void) */ #define HAS_GUC(dev_priv) ((dev_priv)->info.has_guc) #define HAS_GUC_CT(dev_priv) ((dev_priv)->info.has_guc_ct) -#define HAS_GUC_UCODE(dev_priv) (HAS_GUC(dev_priv)) -#define HAS_GUC_SCHED(dev_priv) (HAS_GUC(dev_priv)) -#define HAS_HUC_UCODE(dev_priv) (HAS_GUC(dev_priv)) +#define HAS_GUC_UCODE(dev_priv) ((dev_priv)->guc.fw.path != NULL) +#define HAS_HUC_UCODE(dev_priv) ((dev_priv)->huc.fw.path != NULL) + +#define NEEDS_GUC_LOADING(dev_priv) \ + (HAS_GUC(dev_priv) && \ + (i915_modparams.enable_guc_submission || HAS_HUC_UCODE(dev_priv))) #define HAS_RESOURCE_STREAMER(dev_priv) ((dev_priv)->info.has_resource_streamer) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 5bf96a2..692d609 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -314,7 +314,7 @@ static u32 default_desc_template(const struct drm_i915_private *i915, * present or not in use we still need a small bias as ring wraparound * at offset 0 sometimes hangs. No idea why. */ - if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading) + if (NEEDS_GUC_LOADING(dev_priv)) ctx->ggtt_offset_bias = GUC_WOPCM_TOP; else ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE; diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 527a2d2..0bbc8f0 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3481,7 +3481,7 @@ int i915_ggtt_probe_hw(struct drm_i915_private *dev_priv) * currently don't have any bits spare to pass in this upper * restriction! */ - if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading) { + if (NEEDS_GUC_LOADING(dev_priv)) { ggtt->base.total = min_t(u64, ggtt->base.total, GUC_GGTT_TOP); ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total); } diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index b1296a5..ec76aac 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -4026,7 +4026,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv) for (i = 0; i < MAX_L3_SLICES; ++i) dev_priv->l3_parity.remap_info[i] = NULL; - if (HAS_GUC_SCHED(dev_priv)) + if (HAS_GUC(dev_priv)) dev_priv->pm_guc_events = GEN9_GUC_TO_HOST_INT_EVENT; /* Let's track the enabled rps events */ diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index b4faeb6..1c25f45 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -162,10 +162,6 @@ struct i915_params i915_modparams __read_mostly = { "(0=use value from vbt [default], 1=low power swing(200mV)," "2=default swing(400mV))"); -i915_param_named_unsafe(enable_guc_loading, int, 0400, - "Enable GuC firmware loading " - "(-1=auto, 0=never [default], 1=if available, 2=required)"); - i915_param_named_unsafe(enable_guc_submission, int, 0400, "Enable GuC submission " "(-1=auto, 0=never [default], 1=if available, 2=required)"); diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index c729226..9e1e231 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -44,7 +44,6 @@ param(int, disable_power_well, -1) \ param(int, enable_ips, 1) \ param(int, invert_brightness, 0) \ - param(int, enable_guc_loading, 0) \ param(int, enable_guc_submission, 0) \ param(int, guc_log_level, -1) \ param(char *, guc_firmware_path, NULL) \ diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 25bd162..df281525 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -49,36 +49,44 @@ static int __intel_uc_reset_hw(struct drm_i915_private *dev_priv) void intel_uc_sanitize_options(struct drm_i915_private *dev_priv) { + /* Verify Hardware support */ if (!HAS_GUC(dev_priv)) { - if (i915_modparams.enable_guc_loading > 0 || - i915_modparams.enable_guc_submission > 0) - DRM_INFO("Ignoring GuC options, no hardware\n"); - - i915_modparams.enable_guc_loading = 0; - i915_modparams.enable_guc_submission = 0; + if (i915_modparams.enable_guc_submission > 0) + DRM_INFO("Ignoring GuC submission enable", + "no hardware\n"); + i915_modparams.enable_guc_submission = 0; return; } - /* A negative value means "use platform default" */ - if (i915_modparams.enable_guc_loading < 0) - i915_modparams.enable_guc_loading = HAS_GUC_UCODE(dev_priv); + /* Verify Firmware support */ + if (!HAS_GUC_UCODE(dev_priv)) { + if (i915_modparams.enable_guc_submission == 1) { + DRM_INFO("Ignoring GuC submission enable", + "no firmware\n"); + i915_modparams.enable_guc_submission = 0; + return; + } - /* Verify firmware version */ - if (i915_modparams.enable_guc_loading) { - if (HAS_HUC_UCODE(dev_priv)) - intel_huc_select_fw(&dev_priv->huc); + if (i915_modparams.enable_guc_submission < 0) { + i915_modparams.enable_guc_submission = 0; + return; + } - if (intel_guc_fw_select(&dev_priv->guc)) - i915_modparams.enable_guc_loading = 0; + /* + * If "required" (> 1), let it continue and we will fail later + * due to the lack of firmware + */ + } - /* Can't enable guc submission without guc loaded */ - if (!i915_modparams.enable_guc_loading) - i915_modparams.enable_guc_submission = 0; + /* + * A negative value means "use paltform default" (enabled if we have + * survived to get here) + */ - /* A negative value means "use platform default" */ if (i915_modparams.enable_guc_submission < 0) - i915_modparams.enable_guc_submission = HAS_GUC_SCHED(dev_priv); + i915_modparams.enable_guc_submission = HAS_GUC(dev_priv); + } void intel_uc_init_early(struct drm_i915_private *dev_priv) @@ -88,6 +96,8 @@ void intel_uc_init_early(struct drm_i915_private *dev_priv) void intel_uc_init_fw(struct drm_i915_private *dev_priv) { + if (!HAS_GUC(dev_priv)) + return; intel_uc_fw_fetch(dev_priv, &dev_priv->huc.fw); intel_uc_fw_fetch(dev_priv, &dev_priv->guc.fw); } @@ -154,7 +164,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) struct intel_guc *guc = &dev_priv->guc; int ret, attempts; - if (!i915_modparams.enable_guc_loading) + if (!NEEDS_GUC_LOADING(dev_priv)) return 0; guc_disable_communication(guc); @@ -250,22 +260,17 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) err_guc: i915_ggtt_disable_guc(dev_priv); - if (i915_modparams.enable_guc_loading > 1 || - i915_modparams.enable_guc_submission > 1) { + if (i915_modparams.enable_guc_submission > 1) { DRM_ERROR("GuC init failed. Firmware loading disabled.\n"); ret = -EIO; + } else if (i915_modparams.enable_guc_submission == 1) { + DRM_NOTE("Falling back from GuC submission to execlist mode\n"); + i915_modparams.enable_guc_submission = 0; + ret = 0; } else { - DRM_NOTE("GuC init failed. Firmware loading disabled.\n"); ret = 0; } - if (i915_modparams.enable_guc_submission) { - i915_modparams.enable_guc_submission = 0; - DRM_NOTE("Falling back from GuC submission to execlist mode\n"); - } - - i915_modparams.enable_guc_loading = 0; - return ret; } @@ -273,7 +278,7 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv) { guc_free_load_err_log(&dev_priv->guc); - if (!i915_modparams.enable_guc_loading) + if (!NEEDS_GUC_LOADING(dev_priv)) return; if (i915_modparams.enable_guc_submission) diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 20e3c65c..c631b0e 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -1803,8 +1803,7 @@ int intel_guc_reset(struct drm_i915_private *dev_priv) { int ret; - if (!HAS_GUC(dev_priv)) - return -EINVAL; + GEM_BUG_ON(!HAS_GUC(dev_priv)); intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); ret = gen6_hw_domain_reset(dev_priv, GEN9_GRDOM_GUC);
We currently have two module parameters that control GuC: "enable_guc_loading" and "enable_guc_submission". Whenever we need i915_modparams.enable_guc_submission=1, we also need enable_guc_loading=1. We also need enable_guc_loading=1 when we want to verify the HuC, which is every time we have a HuC (but all platforms with HuC have a GuC and viceversa). v2: Clarifying the commit message (Anusha) v3: Unify seq_puts messages, Re-factoring code as per review (Michal) v4: Rebase v5: Separating message unification into a separate patch v6: Re-factoring code (Sagar, Michal) Rebase v7: Applying review comments (Sagar) Rebase Suggested by: Oscar Mateo <oscar.mateo@intel.com> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> Cc: Oscar Mateo <oscar.mateo@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 6 +-- drivers/gpu/drm/i915/i915_drv.h | 9 +++-- drivers/gpu/drm/i915/i915_gem_context.c | 2 +- drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +- drivers/gpu/drm/i915/i915_irq.c | 2 +- drivers/gpu/drm/i915/i915_params.c | 4 -- drivers/gpu/drm/i915/i915_params.h | 1 - drivers/gpu/drm/i915/intel_uc.c | 69 ++++++++++++++++++--------------- drivers/gpu/drm/i915/intel_uncore.c | 3 +- 9 files changed, 50 insertions(+), 48 deletions(-)