| Message ID | 20171205163844.41264-3-michal.wajdeczko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Michal Wajdeczko (2017-12-05 16:38:39) > In the upcoming patch we will change the way how to recognize > when GuC is in use. Using helper macros will minimize scope > of that changes. While here, update dev_info message. > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> ... but can we please stop it with dev_priv :) We don't use it now or later, we are just making it more complicated for no imminent benefit, right? -Chris
On Tue, 05 Dec 2017 23:43:19 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Michal Wajdeczko (2017-12-05 16:38:39) >> In the upcoming patch we will change the way how to recognize >> when GuC is in use. Using helper macros will minimize scope >> of that changes. While here, update dev_info message. >> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > ... but can we please stop it with dev_priv :) We don't use it now or > later, we are just making it more complicated for no imminent benefit, > right? Note that there are few benefits for keeping dev_priv: a) it nicely matches all other existing HAS_|USES_ macros (even if dev_priv is not used/required: see USES_PPGTT or HAS_AUX_IRQ) (yes I know that you want to kill former and later is not widely used ;) b) as "uses" suggests that it reflects state of the driver, I hope one day we will stop relying on modparam and read actual state of driver from dev_priv (or struct guc or struct uc or something else that can be reached out from this dev_priv) Michal
Quoting Michal Wajdeczko (2017-12-06 11:24:33) > On Tue, 05 Dec 2017 23:43:19 +0100, Chris Wilson > <chris@chris-wilson.co.uk> wrote: > > > Quoting Michal Wajdeczko (2017-12-05 16:38:39) > >> In the upcoming patch we will change the way how to recognize > >> when GuC is in use. Using helper macros will minimize scope > >> of that changes. While here, update dev_info message. > >> > >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > >> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > >> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > ... but can we please stop it with dev_priv :) We don't use it now or > > later, we are just making it more complicated for no imminent benefit, > > right? > > Note that there are few benefits for keeping dev_priv: > > a) it nicely matches all other existing HAS_|USES_ macros > (even if dev_priv is not used/required: see USES_PPGTT or HAS_AUX_IRQ) > (yes I know that you want to kill former and later is not widely used ;) > b) as "uses" suggests that it reflects state of the driver, I hope one day > we will stop relying on modparam and read actual state of driver from > dev_priv (or struct guc or struct uc or something else that can be > reached out from this dev_priv) But we don't and we are adding it to places where we didn't need, if I remember the patches correctly. My concern is that we are creating work and not saving it, as we^W I don't have a design for the future derived state checks. -Chris
Quoting Chris Wilson (2017-12-06 11:32:01) > Quoting Michal Wajdeczko (2017-12-06 11:24:33) > > On Tue, 05 Dec 2017 23:43:19 +0100, Chris Wilson > > <chris@chris-wilson.co.uk> wrote: > > > > > Quoting Michal Wajdeczko (2017-12-05 16:38:39) > > >> In the upcoming patch we will change the way how to recognize > > >> when GuC is in use. Using helper macros will minimize scope > > >> of that changes. While here, update dev_info message. > > >> > > >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > > >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > > >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > >> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > > >> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > > > ... but can we please stop it with dev_priv :) We don't use it now or > > > later, we are just making it more complicated for no imminent benefit, > > > right? > > > > Note that there are few benefits for keeping dev_priv: > > > > a) it nicely matches all other existing HAS_|USES_ macros > > (even if dev_priv is not used/required: see USES_PPGTT or HAS_AUX_IRQ) > > (yes I know that you want to kill former and later is not widely used ;) > > b) as "uses" suggests that it reflects state of the driver, I hope one day > > we will stop relying on modparam and read actual state of driver from > > dev_priv (or struct guc or struct uc or something else that can be > > reached out from this dev_priv) > > But we don't and we are adding it to places where we didn't need, if I > remember the patches correctly. My concern is that we are creating work > and not saving it, as we^W I don't have a design for the future derived > state checks. To be clear, I'm happy if you see value in keeping dev_priv. Just whinging, because first its dev_priv! and I could see no reason why we need it. (So whether it is a "stitch in time" is debatable, it may equally just cause more work later.) -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index bd4eea5..937fa02 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3239,6 +3239,10 @@ intel_info(const struct drm_i915_private *dev_priv) #define HAS_HUC(dev_priv) (HAS_GUC(dev_priv)) #define HAS_HUC_UCODE(dev_priv) (HAS_GUC(dev_priv)) +/* Having a GuC is not the same as using a GuC */ +#define USES_GUC(dev_priv) (i915_modparams.enable_guc_loading) +#define USES_GUC_SUBMISSION(dev_priv) (i915_modparams.enable_guc_submission) + #define HAS_RESOURCE_STREAMER(dev_priv) ((dev_priv)->info.has_resource_streamer) #define HAS_POOLED_EU(dev_priv) ((dev_priv)->info.has_pooled_eu) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index ce3139e..21ce374 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -316,7 +316,7 @@ __create_hw_context(struct drm_i915_private *dev_priv, * 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 (USES_GUC(dev_priv)) ctx->ggtt_offset_bias = GUC_WOPCM_TOP; else ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE; @@ -409,7 +409,7 @@ i915_gem_context_create_gvt(struct drm_device *dev) i915_gem_context_set_closed(ctx); /* not user accessible */ i915_gem_context_clear_bannable(ctx); i915_gem_context_set_force_single_submission(ctx); - if (!i915_modparams.enable_guc_submission) + if (!USES_GUC_SUBMISSION(to_i915(dev))) ctx->ring_size = 512 * PAGE_SIZE; /* Max ring buffer size */ GEM_BUG_ON(i915_gem_context_is_kernel(ctx)); diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index f3c35e8..209bb11 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3503,7 +3503,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 (USES_GUC(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 7cac07d..3517c65 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -1400,7 +1400,7 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 iir, int test_shift) if (iir & (GT_RENDER_USER_INTERRUPT << test_shift)) { notify_ring(engine); - tasklet |= i915_modparams.enable_guc_submission; + tasklet |= USES_GUC_SUBMISSION(engine->i915); } if (tasklet) diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c index df86907..177ee69 100644 --- a/drivers/gpu/drm/i915/intel_guc.c +++ b/drivers/gpu/drm/i915/intel_guc.c @@ -129,7 +129,7 @@ void intel_guc_init_params(struct intel_guc *guc) } /* If GuC submission is enabled, set up additional parameters here */ - if (i915_modparams.enable_guc_submission) { + if (USES_GUC_SUBMISSION(dev_priv)) { u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT; u32 pgs = guc_ggtt_offset(dev_priv->guc.stage_desc_pool); u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16; diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c index 76d3eb1..1a2c5ee 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.c +++ b/drivers/gpu/drm/i915/intel_guc_log.c @@ -505,7 +505,7 @@ static void guc_flush_logs(struct intel_guc *guc) { struct drm_i915_private *dev_priv = guc_to_i915(guc); - if (!i915_modparams.enable_guc_submission || + if (!USES_GUC_SUBMISSION(dev_priv) || (i915_modparams.guc_log_level < 0)) return; @@ -646,7 +646,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val) void i915_guc_log_register(struct drm_i915_private *dev_priv) { - if (!i915_modparams.enable_guc_submission || + if (!USES_GUC_SUBMISSION(dev_priv) || (i915_modparams.guc_log_level < 0)) return; @@ -657,7 +657,7 @@ void i915_guc_log_register(struct drm_i915_private *dev_priv) void i915_guc_log_unregister(struct drm_i915_private *dev_priv) { - if (!i915_modparams.enable_guc_submission) + if (!USES_GUC_SUBMISSION(dev_priv)) return; mutex_lock(&dev_priv->drm.struct_mutex); diff --git a/drivers/gpu/drm/i915/intel_gvt.c b/drivers/gpu/drm/i915/intel_gvt.c index 126f7c7..a2fe7c8 100644 --- a/drivers/gpu/drm/i915/intel_gvt.c +++ b/drivers/gpu/drm/i915/intel_gvt.c @@ -95,7 +95,7 @@ int intel_gvt_init(struct drm_i915_private *dev_priv) return 0; } - if (i915_modparams.enable_guc_submission) { + if (USES_GUC_SUBMISSION(dev_priv)) { DRM_ERROR("i915 GVT-g loading failed due to Graphics virtualization is not yet supported with GuC submission\n"); return -EIO; } diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 4b7f2a7..ed2dd76 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -152,7 +152,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 (!USES_GUC(dev_priv)) return 0; guc_disable_communication(guc); @@ -161,7 +161,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) /* We need to notify the guc whenever we change the GGTT */ i915_ggtt_enable_guc(dev_priv); - if (i915_modparams.enable_guc_submission) { + if (USES_GUC_SUBMISSION(dev_priv)) { /* * This is stuff we need to have available at fw load time * if we are planning to enable submission later @@ -211,7 +211,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) goto err_log_capture; intel_huc_auth(&dev_priv->huc); - if (i915_modparams.enable_guc_submission) { + if (USES_GUC_SUBMISSION(dev_priv)) { if (i915_modparams.guc_log_level >= 0) gen9_enable_guc_interrupts(dev_priv); @@ -220,11 +220,10 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) goto err_interrupts; } - dev_info(dev_priv->drm.dev, "GuC %s (firmware %s [version %u.%u])\n", - i915_modparams.enable_guc_submission ? "submission enabled" : - "loaded", - guc->fw.path, + dev_info(dev_priv->drm.dev, "GuC firmware version %u.%u\n", guc->fw.major_ver_found, guc->fw.minor_ver_found); + dev_info(dev_priv->drm.dev, "GuC submission %s\n", + enableddisabled(USES_GUC_SUBMISSION(dev_priv))); return 0; @@ -243,7 +242,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) err_log_capture: guc_capture_load_err_log(guc); err_submission: - if (i915_modparams.enable_guc_submission) + if (USES_GUC_SUBMISSION(dev_priv)) intel_guc_submission_fini(guc); err_guc: i915_ggtt_disable_guc(dev_priv); @@ -257,7 +256,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) ret = 0; } - if (i915_modparams.enable_guc_submission) { + if (USES_GUC_SUBMISSION(dev_priv)) { i915_modparams.enable_guc_submission = 0; DRM_NOTE("Falling back from GuC submission to execlist mode\n"); } @@ -273,15 +272,15 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv) guc_free_load_err_log(guc); - if (!i915_modparams.enable_guc_loading) + if (!USES_GUC(dev_priv)) return; - if (i915_modparams.enable_guc_submission) + if (USES_GUC_SUBMISSION(dev_priv)) intel_guc_submission_disable(guc); guc_disable_communication(guc); - if (i915_modparams.enable_guc_submission) { + if (USES_GUC_SUBMISSION(dev_priv)) { gen9_disable_guc_interrupts(dev_priv); intel_guc_submission_fini(guc); } diff --git a/drivers/gpu/drm/i915/selftests/intel_guc.c b/drivers/gpu/drm/i915/selftests/intel_guc.c index 7b23597..68d6a69 100644 --- a/drivers/gpu/drm/i915/selftests/intel_guc.c +++ b/drivers/gpu/drm/i915/selftests/intel_guc.c @@ -362,7 +362,7 @@ int intel_guc_live_selftest(struct drm_i915_private *dev_priv) SUBTEST(igt_guc_doorbells), }; - if (!i915_modparams.enable_guc_submission) + if (!USES_GUC_SUBMISSION(dev_priv)) return 0; return i915_subtests(tests, dev_priv);