| Message ID | 20180308154707.21716-8-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 3/8/2018 9:17 PM, Michał Winiarski wrote: > Those two concepts are really separate. Since GuC is writing data into > its own buffer and we even provide a way for userspace to read directly > from it using i915_guc_log_dump debugfs, there's no real reason to tie > log level with relay creation. > Let's create a separate debugfs, giving userspace a way to create a > relay on demand, when it wants to read a continuous log rather than a > snapshot. > > v2: Don't touch guc_log_level on relay creation error, adjust locking > after rebase, s/dev_priv/i915, pass guc to file->private_data (Sagar) > Use struct_mutex rather than runtime.lock for set_log_level > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> <snip> > diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h > index 8c26cce77a98..df91f12a36ed 100644 > --- a/drivers/gpu/drm/i915/intel_guc_log.h > +++ b/drivers/gpu/drm/i915/intel_guc_log.h > @@ -61,9 +61,10 @@ 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_control_get(struct intel_guc *guc); > -int intel_guc_log_control_set(struct intel_guc *guc, u64 control_val); > -int intel_guc_log_register(struct intel_guc *guc); > -void intel_guc_log_unregister(struct intel_guc *guc); > +int intel_guc_log_level_get(struct intel_guc *guc); > +int intel_guc_log_level_set(struct intel_guc *guc, u64 control_val); > +int intel_guc_log_relay_open(struct intel_guc *guc); > +void intel_guc_log_relay_close(struct intel_guc *guc); > +void intel_guc_log_relay_flush(struct intel_guc *guc); Need to maintain order of definition. init_early is also not in order. With that: Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > > #endif > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index 90d2f38e22c9..abce0e38528a 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -219,28 +219,6 @@ static void guc_free_load_err_log(struct intel_guc *guc) > i915_gem_object_put(guc->load_err_log); > } > > -int intel_uc_register(struct drm_i915_private *i915) > -{ > - int ret = 0; > - > - if (!USES_GUC(i915)) > - return 0; > - > - if (i915_modparams.guc_log_level) > - ret = intel_guc_log_register(&i915->guc); > - > - return ret; > -} > - > -void intel_uc_unregister(struct drm_i915_private *i915) > -{ > - if (!USES_GUC(i915)) > - return; > - > - if (i915_modparams.guc_log_level) > - intel_guc_log_unregister(&i915->guc); > -} > - > static int guc_enable_communication(struct intel_guc *guc) > { > struct drm_i915_private *dev_priv = guc_to_i915(guc); > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h > index d6af984cd789..f76d51d1ce70 100644 > --- a/drivers/gpu/drm/i915/intel_uc.h > +++ b/drivers/gpu/drm/i915/intel_uc.h > @@ -31,8 +31,6 @@ > void intel_uc_sanitize_options(struct drm_i915_private *dev_priv); > void intel_uc_init_early(struct drm_i915_private *dev_priv); > void intel_uc_init_mmio(struct drm_i915_private *dev_priv); > -int intel_uc_register(struct drm_i915_private *dev_priv); > -void intel_uc_unregister(struct drm_i915_private *dev_priv); > void intel_uc_init_fw(struct drm_i915_private *dev_priv); > void intel_uc_fini_fw(struct drm_i915_private *dev_priv); > int intel_uc_init_misc(struct drm_i915_private *dev_priv);
On Thu, 08 Mar 2018 16:47:00 +0100, Michał Winiarski <michal.winiarski@intel.com> wrote: > Those two concepts are really separate. Since GuC is writing data into > its own buffer and we even provide a way for userspace to read directly > from it using i915_guc_log_dump debugfs, there's no real reason to tie > log level with relay creation. > Let's create a separate debugfs, giving userspace a way to create a > relay on demand, when it wants to read a continuous log rather than a > snapshot. > > v2: Don't touch guc_log_level on relay creation error, adjust locking > after rebase, s/dev_priv/i915, pass guc to file->private_data (Sagar) > Use struct_mutex rather than runtime.lock for set_log_level > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > --- /snip/ > diff --git a/drivers/gpu/drm/i915/intel_uc.c > b/drivers/gpu/drm/i915/intel_uc.c > index 90d2f38e22c9..abce0e38528a 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -219,28 +219,6 @@ static void guc_free_load_err_log(struct intel_guc > *guc) > i915_gem_object_put(guc->load_err_log); > } > -int intel_uc_register(struct drm_i915_private *i915) > -{ > - int ret = 0; > - > - if (!USES_GUC(i915)) > - return 0; > - > - if (i915_modparams.guc_log_level) > - ret = intel_guc_log_register(&i915->guc); > - > - return ret; > -} > - > -void intel_uc_unregister(struct drm_i915_private *i915) > -{ > - if (!USES_GUC(i915)) > - return; > - > - if (i915_modparams.guc_log_level) > - intel_guc_log_unregister(&i915->guc); > -} > - > static int guc_enable_communication(struct intel_guc *guc) > { > struct drm_i915_private *dev_priv = guc_to_i915(guc); > diff --git a/drivers/gpu/drm/i915/intel_uc.h > b/drivers/gpu/drm/i915/intel_uc.h > index d6af984cd789..f76d51d1ce70 100644 > --- a/drivers/gpu/drm/i915/intel_uc.h > +++ b/drivers/gpu/drm/i915/intel_uc.h > @@ -31,8 +31,6 @@ > void intel_uc_sanitize_options(struct drm_i915_private *dev_priv); > void intel_uc_init_early(struct drm_i915_private *dev_priv); > void intel_uc_init_mmio(struct drm_i915_private *dev_priv); > -int intel_uc_register(struct drm_i915_private *dev_priv); > -void intel_uc_unregister(struct drm_i915_private *dev_priv); hmm, it looks that timelines of these two functions were very short (from patch 2 till patch 8) - so maybe by doing some patch re-order can we fully skip them ? > void intel_uc_init_fw(struct drm_i915_private *dev_priv); > void intel_uc_fini_fw(struct drm_i915_private *dev_priv); > int intel_uc_init_misc(struct drm_i915_private *dev_priv);
On Fri, Mar 09, 2018 at 12:00:06PM +0100, Michal Wajdeczko wrote: > On Thu, 08 Mar 2018 16:47:00 +0100, Michał Winiarski > <michal.winiarski@intel.com> wrote: > > > Those two concepts are really separate. Since GuC is writing data into > > its own buffer and we even provide a way for userspace to read directly > > from it using i915_guc_log_dump debugfs, there's no real reason to tie > > log level with relay creation. > > Let's create a separate debugfs, giving userspace a way to create a > > relay on demand, when it wants to read a continuous log rather than a > > snapshot. > > > > v2: Don't touch guc_log_level on relay creation error, adjust locking > > after rebase, s/dev_priv/i915, pass guc to file->private_data (Sagar) > > Use struct_mutex rather than runtime.lock for set_log_level > > > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > --- > > /snip/ > > > diff --git a/drivers/gpu/drm/i915/intel_uc.c > > b/drivers/gpu/drm/i915/intel_uc.c > > index 90d2f38e22c9..abce0e38528a 100644 > > --- a/drivers/gpu/drm/i915/intel_uc.c > > +++ b/drivers/gpu/drm/i915/intel_uc.c > > @@ -219,28 +219,6 @@ static void guc_free_load_err_log(struct intel_guc > > *guc) > > i915_gem_object_put(guc->load_err_log); > > } > > -int intel_uc_register(struct drm_i915_private *i915) > > -{ > > - int ret = 0; > > - > > - if (!USES_GUC(i915)) > > - return 0; > > - > > - if (i915_modparams.guc_log_level) > > - ret = intel_guc_log_register(&i915->guc); > > - > > - return ret; > > -} > > - > > -void intel_uc_unregister(struct drm_i915_private *i915) > > -{ > > - if (!USES_GUC(i915)) > > - return; > > - > > - if (i915_modparams.guc_log_level) > > - intel_guc_log_unregister(&i915->guc); > > -} > > - > > static int guc_enable_communication(struct intel_guc *guc) > > { > > struct drm_i915_private *dev_priv = guc_to_i915(guc); > > diff --git a/drivers/gpu/drm/i915/intel_uc.h > > b/drivers/gpu/drm/i915/intel_uc.h > > index d6af984cd789..f76d51d1ce70 100644 > > --- a/drivers/gpu/drm/i915/intel_uc.h > > +++ b/drivers/gpu/drm/i915/intel_uc.h > > @@ -31,8 +31,6 @@ > > void intel_uc_sanitize_options(struct drm_i915_private *dev_priv); > > void intel_uc_init_early(struct drm_i915_private *dev_priv); > > void intel_uc_init_mmio(struct drm_i915_private *dev_priv); > > -int intel_uc_register(struct drm_i915_private *dev_priv); > > -void intel_uc_unregister(struct drm_i915_private *dev_priv); > > hmm, it looks that timelines of these two functions were very short > (from patch 2 till patch 8) - so maybe by doing some patch re-order > can we fully skip them ? I could drop patch 2 entirely and go straight to the decoupling - but that means more code movement overall and way more content in this one. I don't really see a better split (but since I'm the author I'm biased :) ). I'm open for suggestions. -Michał > > > void intel_uc_init_fw(struct drm_i915_private *dev_priv); > > void intel_uc_fini_fw(struct drm_i915_private *dev_priv); > > int intel_uc_init_misc(struct drm_i915_private *dev_priv);
Quoting Michał Winiarski (2018-03-09 16:30:42) > On Fri, Mar 09, 2018 at 12:00:06PM +0100, Michal Wajdeczko wrote: > > On Thu, 08 Mar 2018 16:47:00 +0100, Michał Winiarski > > <michal.winiarski@intel.com> wrote: > > > > > Those two concepts are really separate. Since GuC is writing data into > > > its own buffer and we even provide a way for userspace to read directly > > > from it using i915_guc_log_dump debugfs, there's no real reason to tie > > > log level with relay creation. > > > Let's create a separate debugfs, giving userspace a way to create a > > > relay on demand, when it wants to read a continuous log rather than a > > > snapshot. > > > > > > v2: Don't touch guc_log_level on relay creation error, adjust locking > > > after rebase, s/dev_priv/i915, pass guc to file->private_data (Sagar) > > > Use struct_mutex rather than runtime.lock for set_log_level > > > > > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > > --- > > > > /snip/ > > > > > diff --git a/drivers/gpu/drm/i915/intel_uc.c > > > b/drivers/gpu/drm/i915/intel_uc.c > > > index 90d2f38e22c9..abce0e38528a 100644 > > > --- a/drivers/gpu/drm/i915/intel_uc.c > > > +++ b/drivers/gpu/drm/i915/intel_uc.c > > > @@ -219,28 +219,6 @@ static void guc_free_load_err_log(struct intel_guc > > > *guc) > > > i915_gem_object_put(guc->load_err_log); > > > } > > > -int intel_uc_register(struct drm_i915_private *i915) > > > -{ > > > - int ret = 0; > > > - > > > - if (!USES_GUC(i915)) > > > - return 0; > > > - > > > - if (i915_modparams.guc_log_level) > > > - ret = intel_guc_log_register(&i915->guc); > > > - > > > - return ret; > > > -} > > > - > > > -void intel_uc_unregister(struct drm_i915_private *i915) > > > -{ > > > - if (!USES_GUC(i915)) > > > - return; > > > - > > > - if (i915_modparams.guc_log_level) > > > - intel_guc_log_unregister(&i915->guc); > > > -} > > > - > > > static int guc_enable_communication(struct intel_guc *guc) > > > { > > > struct drm_i915_private *dev_priv = guc_to_i915(guc); > > > diff --git a/drivers/gpu/drm/i915/intel_uc.h > > > b/drivers/gpu/drm/i915/intel_uc.h > > > index d6af984cd789..f76d51d1ce70 100644 > > > --- a/drivers/gpu/drm/i915/intel_uc.h > > > +++ b/drivers/gpu/drm/i915/intel_uc.h > > > @@ -31,8 +31,6 @@ > > > void intel_uc_sanitize_options(struct drm_i915_private *dev_priv); > > > void intel_uc_init_early(struct drm_i915_private *dev_priv); > > > void intel_uc_init_mmio(struct drm_i915_private *dev_priv); > > > -int intel_uc_register(struct drm_i915_private *dev_priv); > > > -void intel_uc_unregister(struct drm_i915_private *dev_priv); > > > > hmm, it looks that timelines of these two functions were very short > > (from patch 2 till patch 8) - so maybe by doing some patch re-order > > can we fully skip them ? > > I could drop patch 2 entirely and go straight to the decoupling - but that means > more code movement overall and way more content in this one. > > I don't really see a better split (but since I'm the author I'm biased :) ). > I'm open for suggestions. I can settle the argument by grabbing the first 3 patches as code movement, before we get into the details. Ok? -Chris
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index fa0755fe10d0..f99fe9910634 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2496,32 +2496,73 @@ static int i915_guc_log_dump(struct seq_file *m, void *data) return 0; } -static int i915_guc_log_control_get(void *data, u64 *val) +static int i915_guc_log_level_get(void *data, u64 *val) { struct drm_i915_private *dev_priv = data; if (!USES_GUC(dev_priv)) return -ENODEV; - *val = intel_guc_log_control_get(&dev_priv->guc); + *val = intel_guc_log_level_get(&dev_priv->guc); return 0; } -static int i915_guc_log_control_set(void *data, u64 val) +static int i915_guc_log_level_set(void *data, u64 val) { struct drm_i915_private *dev_priv = data; if (!USES_GUC(dev_priv)) return -ENODEV; - return intel_guc_log_control_set(&dev_priv->guc, val); + return intel_guc_log_level_set(&dev_priv->guc, val); } -DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_control_fops, - i915_guc_log_control_get, i915_guc_log_control_set, +DEFINE_SIMPLE_ATTRIBUTE(i915_guc_log_level_fops, + i915_guc_log_level_get, i915_guc_log_level_set, "%lld\n"); +static int i915_guc_log_relay_open(struct inode *inode, struct file *file) +{ + struct drm_i915_private *dev_priv = inode->i_private; + + if (!USES_GUC(dev_priv)) + return -ENODEV; + + file->private_data = &dev_priv->guc; + + return intel_guc_log_relay_open(&dev_priv->guc); +} + +static ssize_t +i915_guc_log_relay_write(struct file *filp, + const char __user *ubuf, + size_t cnt, + loff_t *ppos) +{ + struct intel_guc *guc = filp->private_data; + + intel_guc_log_relay_flush(guc); + + return cnt; +} + +static int i915_guc_log_relay_release(struct inode *inode, struct file *file) +{ + struct drm_i915_private *dev_priv = inode->i_private; + + intel_guc_log_relay_close(&dev_priv->guc); + + return 0; +} + +static const struct file_operations i915_guc_log_relay_fops = { + .owner = THIS_MODULE, + .open = i915_guc_log_relay_open, + .write = i915_guc_log_relay_write, + .release = i915_guc_log_relay_release, +}; + static const char *psr2_live_status(u32 val) { static const char * const live_status[] = { @@ -4776,7 +4817,8 @@ static const struct i915_debugfs_files { {"i915_dp_test_data", &i915_displayport_test_data_fops}, {"i915_dp_test_type", &i915_displayport_test_type_fops}, {"i915_dp_test_active", &i915_displayport_test_active_fops}, - {"i915_guc_log_control", &i915_guc_log_control_fops}, + {"i915_guc_log_level", &i915_guc_log_level_fops}, + {"i915_guc_log_relay", &i915_guc_log_relay_fops}, {"i915_hpd_storm_ctl", &i915_hpd_storm_ctl_fops}, {"i915_ipc_status", &i915_ipc_status_fops}, {"i915_drrs_ctl", &i915_drrs_ctl_fops} diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 987c6770d1a6..0a238e3ed5b6 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1240,9 +1240,6 @@ static void i915_driver_register(struct drm_i915_private *dev_priv) i915_debugfs_register(dev_priv); i915_setup_sysfs(dev_priv); - /* Depends on debugfs having been initialized */ - intel_uc_register(dev_priv); - /* Depends on sysfs having been initialized */ i915_perf_register(dev_priv); } else @@ -1300,7 +1297,6 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv) i915_pmu_unregister(dev_priv); i915_teardown_sysfs(dev_priv); - intel_uc_unregister(dev_priv); drm_dev_unregister(&dev_priv->drm); i915_gem_shrinker_unregister(dev_priv); diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c index 5ff4b510569a..62bf8da14b13 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.c +++ b/drivers/gpu/drm/i915/intel_guc_log.c @@ -406,18 +406,13 @@ static int guc_log_relay_create(struct intel_guc *guc) DRM_ERROR("Couldn't create relay chan for GuC logging\n"); ret = -ENOMEM; - goto err; + return ret; } GEM_BUG_ON(guc_log_relay_chan->subbuf_size < subbuf_size); guc->log.runtime.relay_chan = guc_log_relay_chan; return 0; - -err: - /* logging will be off */ - i915_modparams.guc_log_level = 0; - return ret; } static void guc_log_relay_destroy(struct intel_guc *guc) @@ -508,7 +503,7 @@ void intel_guc_log_destroy(struct intel_guc *guc) i915_vma_unpin_and_release(&guc->log.vma); } -int intel_guc_log_control_get(struct intel_guc *guc) +int intel_guc_log_level_get(struct intel_guc *guc) { GEM_BUG_ON(!guc->log.vma); GEM_BUG_ON(i915_modparams.guc_log_level < 0); @@ -523,10 +518,9 @@ int intel_guc_log_control_get(struct intel_guc *guc) LOG_LEVEL_TO_ENABLED(_x) ? _x - 1 : 0; \ }) #define VERBOSITY_TO_LOG_LEVEL(x) ((x) + 1) -int intel_guc_log_control_set(struct intel_guc *guc, u64 val) +int intel_guc_log_level_set(struct intel_guc *guc, u64 val) { struct drm_i915_private *dev_priv = guc_to_i915(guc); - bool enabled = LOG_LEVEL_TO_ENABLED(val); int ret; BUILD_BUG_ON(GUC_LOG_VERBOSITY_MIN != 0); @@ -549,7 +543,8 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 val) } intel_runtime_pm_get(dev_priv); - ret = guc_log_control(guc, enabled, LOG_LEVEL_TO_VERBOSITY(val)); + ret = guc_log_control(guc, LOG_LEVEL_TO_ENABLED(val), + LOG_LEVEL_TO_VERBOSITY(val)); intel_runtime_pm_put(dev_priv); if (ret) { DRM_DEBUG_DRIVER("guc_log_control action failed %d\n", ret); @@ -558,88 +553,78 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 val) i915_modparams.guc_log_level = val; - mutex_unlock(&dev_priv->drm.struct_mutex); - - if (enabled && !guc_log_has_runtime(guc)) { - ret = intel_guc_log_register(guc); - if (ret) { - /* logging will remain off */ - i915_modparams.guc_log_level = 0; - goto out; - } - } else if (!enabled && guc_log_has_runtime(guc)) { - intel_guc_log_unregister(guc); - } - - return 0; - out_unlock: mutex_unlock(&dev_priv->drm.struct_mutex); -out: + return ret; } -int intel_guc_log_register(struct intel_guc *guc) +int intel_guc_log_relay_open(struct intel_guc *guc) { int ret; mutex_lock(&guc->log.runtime.lock); - GEM_BUG_ON(guc_log_has_runtime(guc)); + if (guc_log_has_runtime(guc)) { + ret = -EEXIST; + goto out_unlock; + } ret = guc_log_relay_create(guc); if (ret) - goto err; + goto out_unlock; ret = guc_log_map(guc); if (ret) - goto err_relay; + goto out_relay; + + mutex_unlock(&guc->log.runtime.lock); guc_log_flush_irq_enable(guc); - mutex_unlock(&guc->log.runtime.lock); + /* + * When GuC is logging without us relaying to userspace, we're ignoring + * the flush notification. This means that we need to unconditionally + * flush on relay enabling, since GuC only notifies us once. + */ + queue_work(guc->log.runtime.flush_wq, &guc->log.runtime.flush_work); return 0; -err_relay: +out_relay: guc_log_relay_destroy(guc); -err: +out_unlock: mutex_unlock(&guc->log.runtime.lock); return ret; } -void intel_guc_log_unregister(struct intel_guc *guc) +void intel_guc_log_relay_flush(struct intel_guc *guc) { struct drm_i915_private *i915 = guc_to_i915(guc); - guc_log_flush_irq_disable(guc); - /* * Before initiating the forceful flush, wait for any pending/ongoing * flush to complete otherwise forceful flush may not actually happen. */ flush_work(&guc->log.runtime.flush_work); - /* - * Once logging is disabled, GuC won't generate logs & send an - * interrupt. But there could be some data in the log buffer - * which is yet to be captured. So request GuC to update the log - * buffer state and then collect the left over logs. - */ intel_runtime_pm_get(i915); guc_log_flush(guc); intel_runtime_pm_put(i915); /* GuC would have updated log buffer by now, so capture it */ guc_log_capture_logs(guc); +} - mutex_lock(&guc->log.runtime.lock); +void intel_guc_log_relay_close(struct intel_guc *guc) +{ + guc_log_flush_irq_disable(guc); + flush_work(&guc->log.runtime.flush_work); + mutex_lock(&guc->log.runtime.lock); GEM_BUG_ON(!guc_log_has_runtime(guc)); - - guc_log_unmap(guc); + guc_log_unmap(guc); guc_log_relay_destroy(guc); - mutex_unlock(&guc->log.runtime.lock); } diff --git a/drivers/gpu/drm/i915/intel_guc_log.h b/drivers/gpu/drm/i915/intel_guc_log.h index 8c26cce77a98..df91f12a36ed 100644 --- a/drivers/gpu/drm/i915/intel_guc_log.h +++ b/drivers/gpu/drm/i915/intel_guc_log.h @@ -61,9 +61,10 @@ 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_control_get(struct intel_guc *guc); -int intel_guc_log_control_set(struct intel_guc *guc, u64 control_val); -int intel_guc_log_register(struct intel_guc *guc); -void intel_guc_log_unregister(struct intel_guc *guc); +int intel_guc_log_level_get(struct intel_guc *guc); +int intel_guc_log_level_set(struct intel_guc *guc, u64 control_val); +int intel_guc_log_relay_open(struct intel_guc *guc); +void intel_guc_log_relay_close(struct intel_guc *guc); +void intel_guc_log_relay_flush(struct intel_guc *guc); #endif diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 90d2f38e22c9..abce0e38528a 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -219,28 +219,6 @@ static void guc_free_load_err_log(struct intel_guc *guc) i915_gem_object_put(guc->load_err_log); } -int intel_uc_register(struct drm_i915_private *i915) -{ - int ret = 0; - - if (!USES_GUC(i915)) - return 0; - - if (i915_modparams.guc_log_level) - ret = intel_guc_log_register(&i915->guc); - - return ret; -} - -void intel_uc_unregister(struct drm_i915_private *i915) -{ - if (!USES_GUC(i915)) - return; - - if (i915_modparams.guc_log_level) - intel_guc_log_unregister(&i915->guc); -} - static int guc_enable_communication(struct intel_guc *guc) { struct drm_i915_private *dev_priv = guc_to_i915(guc); diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h index d6af984cd789..f76d51d1ce70 100644 --- a/drivers/gpu/drm/i915/intel_uc.h +++ b/drivers/gpu/drm/i915/intel_uc.h @@ -31,8 +31,6 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv); void intel_uc_init_early(struct drm_i915_private *dev_priv); void intel_uc_init_mmio(struct drm_i915_private *dev_priv); -int intel_uc_register(struct drm_i915_private *dev_priv); -void intel_uc_unregister(struct drm_i915_private *dev_priv); void intel_uc_init_fw(struct drm_i915_private *dev_priv); void intel_uc_fini_fw(struct drm_i915_private *dev_priv); int intel_uc_init_misc(struct drm_i915_private *dev_priv);
Those two concepts are really separate. Since GuC is writing data into its own buffer and we even provide a way for userspace to read directly from it using i915_guc_log_dump debugfs, there's no real reason to tie log level with relay creation. Let's create a separate debugfs, giving userspace a way to create a relay on demand, when it wants to read a continuous log rather than a snapshot. v2: Don't touch guc_log_level on relay creation error, adjust locking after rebase, s/dev_priv/i915, pass guc to file->private_data (Sagar) Use struct_mutex rather than runtime.lock for set_log_level Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> --- drivers/gpu/drm/i915/i915_debugfs.c | 56 ++++++++++++++++++++++---- drivers/gpu/drm/i915/i915_drv.c | 4 -- drivers/gpu/drm/i915/intel_guc_log.c | 77 +++++++++++++++--------------------- drivers/gpu/drm/i915/intel_guc_log.h | 9 +++-- drivers/gpu/drm/i915/intel_uc.c | 22 ----------- drivers/gpu/drm/i915/intel_uc.h | 2 - 6 files changed, 85 insertions(+), 85 deletions(-)