| Message ID | 1517379279-12967-2-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Sagar Arun Kamble (2018-01-31 06:14:38) > On some systems like skl-gvtdvm, SSE4.1 movntdqa might not be available. > movntdqa is needed for efficient capture of the logs from uncached GuC > log buffer. GuC init was tied with this support and other setup needed > for interrupt based GuC log capture like relay channel/file support and > uncached mapping support. With this patch, GuC init is now unblocked from > lack of this support. > SSE and relay support init/fini is now being done by new functions > intel_guc_log_init|fini_runtime() which makes relay functions static. > We have introduced two states "supported" and "enabled". Supported is set > when we have SSE4.1 support and have relay, GuC log, WC mapping available. > Enabled is set when support is present and user has requested logging > through i915_modparams.guc_log_level. > While at this change, fixed unwind order in intel_uc_fini_misc. Downside would appear to be the loss of feedback in i915.guc_log_level when it fail? Otherwise, looks tidy enough. -Chris
On 1/31/2018 3:08 PM, Chris Wilson wrote: > Quoting Sagar Arun Kamble (2018-01-31 06:14:38) >> On some systems like skl-gvtdvm, SSE4.1 movntdqa might not be available. >> movntdqa is needed for efficient capture of the logs from uncached GuC >> log buffer. GuC init was tied with this support and other setup needed >> for interrupt based GuC log capture like relay channel/file support and >> uncached mapping support. With this patch, GuC init is now unblocked from >> lack of this support. >> SSE and relay support init/fini is now being done by new functions >> intel_guc_log_init|fini_runtime() which makes relay functions static. >> We have introduced two states "supported" and "enabled". Supported is set >> when we have SSE4.1 support and have relay, GuC log, WC mapping available. >> Enabled is set when support is present and user has requested logging >> through i915_modparams.guc_log_level. >> While at this change, fixed unwind order in intel_uc_fini_misc. > Downside would appear to be the loss of feedback in i915.guc_log_level > when it fail? This patch decouples the guc_log_level from only interrupt based log support. We can continue to use/know guc_log_level to send the verbosity to GuC still. Have to rely only on static dumps like i915_guc_log_dump or error_uc->guc_log wherever runtime logging is off. > Otherwise, looks tidy enough. > -Chris
On Wed, Jan 31, 2018 at 11:44:38AM +0530, Sagar Arun Kamble wrote: > On some systems like skl-gvtdvm, SSE4.1 movntdqa might not be available. > movntdqa is needed for efficient capture of the logs from uncached GuC > log buffer. GuC init was tied with this support and other setup needed > for interrupt based GuC log capture like relay channel/file support and > uncached mapping support. With this patch, GuC init is now unblocked from > lack of this support. > SSE and relay support init/fini is now being done by new functions > intel_guc_log_init|fini_runtime() which makes relay functions static. > We have introduced two states "supported" and "enabled". Supported is set > when we have SSE4.1 support and have relay, GuC log, WC mapping available. > Enabled is set when support is present and user has requested logging > through i915_modparams.guc_log_level. > While at this change, fixed unwind order in intel_uc_fini_misc. Or, we could rework GuC log a bit. We could change the meaning of guc_log_level - it would only affect the "internal" GuC state (verbosity - in other words, the content of buffer used by GuC). This allows userspace to inspect the content of GuC buffer (i915_guc_log_dump), but i915 is not copying the data to the relay (we're ignoring the interrupts recieved from GuC). This means that we need to introduce alternative way of controling the relay, let's say another file called "i915_guc_log_relay". Opening this file causes the relay to be created, we can throw an error if we don't have SSE4.1 there. (well - the whole runtime is created actually, things are quite badly named today, runtime means mapping of buffer shared with GuC, relay is the buffer "shared" with userspace, functions operating on "runtime" should probably setup both of those rather than just the mapping). We're also solving the problem of overflows if GuC is loaded with "guc_log_level > 0" while nobody is consuming the data from the relay. And we don't really need to handle the "runtime" at module load time at all, which simplifies things. I'm working on a series that does all of the above. -Michał > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Michal Winiarski <michal.winiarski@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/intel_guc.c | 2 +- > drivers/gpu/drm/i915/intel_guc_log.c | 97 ++++++++++++++++++++++++++---------- > drivers/gpu/drm/i915/intel_guc_log.h | 16 +++++- > drivers/gpu/drm/i915/intel_uc.c | 14 ++---- > 4 files changed, 90 insertions(+), 39 deletions(-)
Quoting Michał Winiarski (2018-01-31 10:37:20) > On Wed, Jan 31, 2018 at 11:44:38AM +0530, Sagar Arun Kamble wrote: > > On some systems like skl-gvtdvm, SSE4.1 movntdqa might not be available. > > movntdqa is needed for efficient capture of the logs from uncached GuC > > log buffer. GuC init was tied with this support and other setup needed > > for interrupt based GuC log capture like relay channel/file support and > > uncached mapping support. With this patch, GuC init is now unblocked from > > lack of this support. > > SSE and relay support init/fini is now being done by new functions > > intel_guc_log_init|fini_runtime() which makes relay functions static. > > We have introduced two states "supported" and "enabled". Supported is set > > when we have SSE4.1 support and have relay, GuC log, WC mapping available. > > Enabled is set when support is present and user has requested logging > > through i915_modparams.guc_log_level. > > While at this change, fixed unwind order in intel_uc_fini_misc. > > Or, we could rework GuC log a bit. > We could change the meaning of guc_log_level - it would only affect the > "internal" GuC state (verbosity - in other words, the content of buffer used by > GuC). This allows userspace to inspect the content of GuC buffer > (i915_guc_log_dump), but i915 is not copying the data to the relay (we're > ignoring the interrupts recieved from GuC). > > This means that we need to introduce alternative way of controling the relay, > let's say another file called "i915_guc_log_relay". Opening this file causes the > relay to be created, we can throw an error if we don't have SSE4.1 there. > (well - the whole runtime is created actually, things are quite badly named > today, runtime means mapping of buffer shared with GuC, relay is the buffer > "shared" with userspace, functions operating on "runtime" should probably setup > both of those rather than just the mapping). > > We're also solving the problem of overflows if GuC is loaded with > "guc_log_level > 0" while nobody is consuming the data from the relay. > And we don't really need to handle the "runtime" at module load time at all, > which simplifies things. > > I'm working on a series that does all of the above. I'll push the 1st patch (the -EEXISTS fix) and we can come back for the rest. -Chris
On 1/31/2018 4:07 PM, Michał Winiarski wrote: > On Wed, Jan 31, 2018 at 11:44:38AM +0530, Sagar Arun Kamble wrote: >> On some systems like skl-gvtdvm, SSE4.1 movntdqa might not be available. >> movntdqa is needed for efficient capture of the logs from uncached GuC >> log buffer. GuC init was tied with this support and other setup needed >> for interrupt based GuC log capture like relay channel/file support and >> uncached mapping support. With this patch, GuC init is now unblocked from >> lack of this support. >> SSE and relay support init/fini is now being done by new functions >> intel_guc_log_init|fini_runtime() which makes relay functions static. >> We have introduced two states "supported" and "enabled". Supported is set >> when we have SSE4.1 support and have relay, GuC log, WC mapping available. >> Enabled is set when support is present and user has requested logging >> through i915_modparams.guc_log_level. >> While at this change, fixed unwind order in intel_uc_fini_misc. > Or, we could rework GuC log a bit. > We could change the meaning of guc_log_level - it would only affect the > "internal" GuC state (verbosity - in other words, the content of buffer used by > GuC). This allows userspace to inspect the content of GuC buffer > (i915_guc_log_dump), but i915 is not copying the data to the relay (we're > ignoring the interrupts recieved from GuC). > > This means that we need to introduce alternative way of controling the relay, > let's say another file called "i915_guc_log_relay". Opening this file causes the > relay to be created, we can throw an error if we don't have SSE4.1 there. > (well - the whole runtime is created actually, things are quite badly named > today, runtime means mapping of buffer shared with GuC, relay is the buffer > "shared" with userspace, functions operating on "runtime" should probably setup > both of those rather than just the mapping). True. Setup spread over different stages of load made it little complicated to change and your change will help. > We're also solving the problem of overflows if GuC is loaded with > "guc_log_level > 0" while nobody is consuming the data from the relay. > And we don't really need to handle the "runtime" at module load time at all, > which simplifies things. Yes. Agree on this approach. Thanks. > I'm working on a series that does all of the above. > > -Michał > >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Michal Winiarski <michal.winiarski@intel.com> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> --- >> drivers/gpu/drm/i915/intel_guc.c | 2 +- >> drivers/gpu/drm/i915/intel_guc_log.c | 97 ++++++++++++++++++++++++++---------- >> drivers/gpu/drm/i915/intel_guc_log.h | 16 +++++- >> drivers/gpu/drm/i915/intel_uc.c | 14 ++---- >> 4 files changed, 90 insertions(+), 39 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c index 21140cc..a9e5848 100644 --- a/drivers/gpu/drm/i915/intel_guc.c +++ b/drivers/gpu/drm/i915/intel_guc.c @@ -458,7 +458,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv) if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS) return 0; - if (i915_modparams.guc_log_level) + if (guc->log.runtime.enabled) gen9_enable_guc_interrupts(dev_priv); data[0] = INTEL_GUC_ACTION_EXIT_S_STATE; diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c index 3fbe93a..3454e0c 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.c +++ b/drivers/gpu/drm/i915/intel_guc_log.c @@ -438,7 +438,7 @@ void intel_guc_log_init_early(struct intel_guc *guc) INIT_WORK(&guc->log.runtime.flush_work, capture_logs_work); } -int intel_guc_log_relay_create(struct intel_guc *guc) +static int guc_log_relay_create(struct intel_guc *guc) { struct drm_i915_private *dev_priv = guc_to_i915(guc); struct rchan *guc_log_relay_chan; @@ -486,12 +486,10 @@ int intel_guc_log_relay_create(struct intel_guc *guc) err: mutex_unlock(&guc->log.runtime.relay_lock); - /* logging will be off */ - i915_modparams.guc_log_level = 0; return ret; } -void intel_guc_log_relay_destroy(struct intel_guc *guc) +static void guc_log_relay_destroy(struct intel_guc *guc) { mutex_lock(&guc->log.runtime.relay_lock); @@ -509,18 +507,51 @@ void intel_guc_log_relay_destroy(struct intel_guc *guc) mutex_unlock(&guc->log.runtime.relay_lock); } +void intel_guc_log_init_runtime(struct intel_guc *guc) +{ + /* + * We require SSE 4.1 for fast reads from the GuC log buffer and + * it should likely be present on the chipsets supporting GuC based + * submisssions. Runtime logging is to be supported if SSE 4.1 support + * is present and relay & uncached buffer can be setup. + */ + guc->log.runtime.supported = i915_has_memcpy_from_wc(); + if (!guc->log.runtime.supported) { + DRM_DEBUG_DRIVER("SSE4.1 support not present. Runtime logging " + "disabled\n"); + return; + } + + if (guc_log_relay_create(guc)) { + DRM_ERROR("Couldn't allocate relay for GuC log. Runtime logging" + " disabled\n"); + guc->log.runtime.supported = false; + } +} + +void intel_guc_log_fini_runtime(struct intel_guc *guc) +{ + guc_log_relay_destroy(guc); + guc->log.runtime.supported = false; +} + static int guc_log_late_setup(struct intel_guc *guc) { struct drm_i915_private *dev_priv = guc_to_i915(guc); int ret; + if (!guc->log.runtime.supported) { + ret = -ENODEV; + goto err; + } + if (!guc_log_has_runtime(guc)) { /* * If log was disabled at boot time, then setup needed to handle * log buffer flush interrupts would not have been done yet, so * do that now. */ - ret = intel_guc_log_relay_create(guc); + ret = guc_log_relay_create(guc); if (ret) goto err; @@ -545,10 +576,18 @@ static int guc_log_late_setup(struct intel_guc *guc) guc_log_runtime_destroy(guc); mutex_unlock(&dev_priv->drm.struct_mutex); err_relay: - intel_guc_log_relay_destroy(guc); + guc_log_relay_destroy(guc); err: - /* logging will remain off */ - i915_modparams.guc_log_level = 0; + mutex_lock(&dev_priv->drm.struct_mutex); + intel_runtime_pm_get(dev_priv); + gen9_disable_guc_interrupts(dev_priv); + guc->log.runtime.enabled = false; + intel_runtime_pm_put(dev_priv); + mutex_unlock(&dev_priv->drm.struct_mutex); + + /* runtime logging will remain off */ + guc->log.runtime.supported = false; + return ret; } @@ -571,7 +610,7 @@ static void guc_flush_logs(struct intel_guc *guc) { struct drm_i915_private *dev_priv = guc_to_i915(guc); - if (!USES_GUC_SUBMISSION(dev_priv) || !i915_modparams.guc_log_level) + if (!USES_GUC_SUBMISSION(dev_priv) || !guc->log.runtime.enabled) return; /* First disable the interrupts, will be renabled afterwards */ @@ -605,16 +644,6 @@ int intel_guc_log_create(struct intel_guc *guc) GEM_BUG_ON(guc->log.vma); - /* - * We require SSE 4.1 for fast reads from the GuC log buffer and - * it should be present on the chipsets supporting GuC based - * submisssions. - */ - if (WARN_ON(!i915_has_memcpy_from_wc())) { - ret = -EINVAL; - goto err; - } - vma = intel_guc_allocate_vma(guc, GUC_LOG_SIZE); if (IS_ERR(vma)) { ret = PTR_ERR(vma); @@ -623,28 +652,41 @@ int intel_guc_log_create(struct intel_guc *guc) guc->log.vma = vma; - if (i915_modparams.guc_log_level) { + /* + * We are making runtime creation optional based on support as + * we can continue to use GuC and base log support. + */ + if (i915_modparams.guc_log_level && guc->log.runtime.supported) { ret = guc_log_runtime_create(guc); - if (ret < 0) - goto err_vma; + if (ret < 0) { + DRM_ERROR("GuC log runtime create failed. Runtime " + "logging disabled\n"); + guc->log.runtime.supported = false; + } } /* each allocated unit is a page */ - flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL | + flags = GUC_LOG_VALID | (GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) | (GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) | (GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT); + if (guc->log.runtime.supported) { + flags |= GUC_LOG_NOTIFY_ON_HALF_FULL; + guc->log.runtime.enabled = true; + } + offset = guc_ggtt_offset(vma) >> PAGE_SHIFT; /* in pages */ guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags; return 0; -err_vma: - i915_vma_unpin_and_release(&guc->log.vma); err: /* logging will be off */ i915_modparams.guc_log_level = 0; + /* runtime logging will remain off */ + guc->log.runtime.supported = false; + return ret; } @@ -720,6 +762,8 @@ int intel_guc_log_control(struct intel_guc *guc, u64 control_val) i915_modparams.guc_log_level = 0; } + guc->log.runtime.enabled = enable_logging; + return ret; } @@ -742,10 +786,11 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv) /* GuC logging is currently the only user of Guc2Host interrupts */ intel_runtime_pm_get(dev_priv); gen9_disable_guc_interrupts(dev_priv); + guc->log.runtime.enabled = false; intel_runtime_pm_put(dev_priv); guc_log_runtime_destroy(guc); mutex_unlock(&dev_priv->drm.struct_mutex); - intel_guc_log_relay_destroy(guc); + guc_log_relay_destroy(guc); } diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h index dab0e94..c89c331 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.h +++ b/drivers/gpu/drm/i915/intel_guc_log.h @@ -44,6 +44,18 @@ struct intel_guc_log { struct i915_vma *vma; /* The runtime stuff gets created only when GuC logging gets enabled */ struct { + /* + * Support based on movntdqa availability, GuC log, relay and + * runtime creation status. + */ + bool supported; + + /* + * Status based on "supported" and user request for logging + * through i915_modparams.guc_log_level. + */ + bool enabled; + void *buf_addr; struct workqueue_struct *flush_wq; struct work_struct flush_work; @@ -62,8 +74,8 @@ struct intel_guc_log { int intel_guc_log_create(struct intel_guc *guc); void intel_guc_log_destroy(struct intel_guc *guc); void intel_guc_log_init_early(struct intel_guc *guc); -int intel_guc_log_relay_create(struct intel_guc *guc); -void intel_guc_log_relay_destroy(struct intel_guc *guc); +void intel_guc_log_init_runtime(struct intel_guc *guc); +void intel_guc_log_fini_runtime(struct intel_guc *guc); int intel_guc_log_control(struct intel_guc *guc, u64 control_val); void i915_guc_log_register(struct drm_i915_private *dev_priv); void i915_guc_log_unregister(struct drm_i915_private *dev_priv); diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index e3f3509..1cc0d86 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -250,16 +250,10 @@ int intel_uc_init_misc(struct drm_i915_private *dev_priv) goto err; } - ret = intel_guc_log_relay_create(guc); - if (ret) { - DRM_ERROR("Couldn't allocate relay for GuC log\n"); - goto err_relay; - } + intel_guc_log_init_runtime(guc); return 0; -err_relay: - intel_guc_fini_wq(guc); err: return ret; } @@ -271,9 +265,9 @@ void intel_uc_fini_misc(struct drm_i915_private *dev_priv) if (!USES_GUC(dev_priv)) return; - intel_guc_fini_wq(guc); + intel_guc_log_fini_runtime(guc); - intel_guc_log_relay_destroy(guc); + intel_guc_fini_wq(guc); } int intel_uc_init(struct drm_i915_private *dev_priv) @@ -386,7 +380,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) } if (USES_GUC_SUBMISSION(dev_priv)) { - if (i915_modparams.guc_log_level) + if (guc->log.runtime.enabled) gen9_enable_guc_interrupts(dev_priv); ret = intel_guc_submission_enable(guc);
On some systems like skl-gvtdvm, SSE4.1 movntdqa might not be available. movntdqa is needed for efficient capture of the logs from uncached GuC log buffer. GuC init was tied with this support and other setup needed for interrupt based GuC log capture like relay channel/file support and uncached mapping support. With this patch, GuC init is now unblocked from lack of this support. SSE and relay support init/fini is now being done by new functions intel_guc_log_init|fini_runtime() which makes relay functions static. We have introduced two states "supported" and "enabled". Supported is set when we have SSE4.1 support and have relay, GuC log, WC mapping available. Enabled is set when support is present and user has requested logging through i915_modparams.guc_log_level. While at this change, fixed unwind order in intel_uc_fini_misc. Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Michal Winiarski <michal.winiarski@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/intel_guc.c | 2 +- drivers/gpu/drm/i915/intel_guc_log.c | 97 ++++++++++++++++++++++++++---------- drivers/gpu/drm/i915/intel_guc_log.h | 16 +++++- drivers/gpu/drm/i915/intel_uc.c | 14 ++---- 4 files changed, 90 insertions(+), 39 deletions(-)