Re: [PATCH 02/15] drm/i915/guc: Create common entry points for log register/unregister

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Mar 05, 2018 at 12:39:58PM +0530, Sagar Arun Kamble wrote:
> Overall change looks good. Could you please clarify on below:
> 
> intel_uc_log_register|unregister are removed in patch later in the series.
> Should we just stay with inner functions then to minimize changes?

I've done this to move (USES_GUC/guc_log_level) checks to the callers. Otherwise
it would need to stay in intel_guc_log_register, which would cause us to do more
changes in intel_guc_log_control_set later in the series (when we're actually
doing the decoupling).

-Michał

> 
> 
> Thanks
> Sagar
> 
> On 2/27/2018 6:22 PM, Michał Winiarski wrote:
> > We have many functions responsible for allocating different parts of
> > runtime called from multiple places. Let's stick with keeping
> > everything in guc_log_register instead.
> > 
> > Signed-off-by: Michał Winiarski <michal.winiarski@xxxxxxxxx>
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > Cc: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/i915_drv.c      |   6 +-
> >   drivers/gpu/drm/i915/i915_gem.c      |   4 +-
> >   drivers/gpu/drm/i915/intel_guc_log.c | 148 ++++++++++++++---------------------
> >   drivers/gpu/drm/i915/intel_guc_log.h |   6 +-
> >   drivers/gpu/drm/i915/intel_uc.c      |  39 ++++-----
> >   drivers/gpu/drm/i915/intel_uc.h      |   6 +-
> >   6 files changed, 91 insertions(+), 118 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index aaa861b51024..719b2be73292 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -636,7 +636,7 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv)
> >   	i915_gem_contexts_fini(dev_priv);
> >   	mutex_unlock(&dev_priv->drm.struct_mutex);
> > -	intel_uc_fini_misc(dev_priv);
> > +	intel_uc_fini_wq(dev_priv);
> >   	i915_gem_cleanup_userptr(dev_priv);
> >   	i915_gem_drain_freed_objects(dev_priv);
> > @@ -1237,7 +1237,7 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
> >   	/* Reveal our presence to userspace */
> >   	if (drm_dev_register(dev, 0) == 0) {
> >   		i915_debugfs_register(dev_priv);
> > -		i915_guc_log_register(dev_priv);
> > +		intel_uc_log_register(dev_priv);
> >   		i915_setup_sysfs(dev_priv);
> >   		/* Depends on sysfs having been initialized */
> > @@ -1297,7 +1297,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
> >   	i915_pmu_unregister(dev_priv);
> >   	i915_teardown_sysfs(dev_priv);
> > -	i915_guc_log_unregister(dev_priv);
> > +	intel_uc_log_unregister(dev_priv);
> >   	drm_dev_unregister(&dev_priv->drm);
> >   	i915_gem_shrinker_unregister(dev_priv);
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 14c855b1a3a4..4bf5f25b29e2 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -5279,7 +5279,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
> >   	if (ret)
> >   		return ret;
> > -	ret = intel_uc_init_misc(dev_priv);
> > +	ret = intel_uc_init_wq(dev_priv);
> >   	if (ret)
> >   		return ret;
> > @@ -5375,7 +5375,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
> >   	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> >   	mutex_unlock(&dev_priv->drm.struct_mutex);
> > -	intel_uc_fini_misc(dev_priv);
> > +	intel_uc_fini_wq(dev_priv);
> >   	if (ret != -EIO)
> >   		i915_gem_cleanup_userptr(dev_priv);
> > diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> > index 22a05320817b..f1cab43d334e 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_log.c
> > +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> > @@ -441,7 +441,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)
> > +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;
> > @@ -494,7 +494,7 @@ int intel_guc_log_relay_create(struct intel_guc *guc)
> >   	return ret;
> >   }
> > -void intel_guc_log_relay_destroy(struct intel_guc *guc)
> > +void guc_log_relay_destroy(struct intel_guc *guc)
> >   {
> >   	mutex_lock(&guc->log.runtime.relay_lock);
> > @@ -512,49 +512,6 @@ void intel_guc_log_relay_destroy(struct intel_guc *guc)
> >   	mutex_unlock(&guc->log.runtime.relay_lock);
> >   }
> > -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_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);
> > -		if (ret)
> > -			goto err;
> > -
> > -		mutex_lock(&dev_priv->drm.struct_mutex);
> > -		intel_runtime_pm_get(dev_priv);
> > -		ret = guc_log_runtime_create(guc);
> > -		intel_runtime_pm_put(dev_priv);
> > -		mutex_unlock(&dev_priv->drm.struct_mutex);
> > -
> > -		if (ret)
> > -			goto err_relay;
> > -	}
> > -
> > -	ret = guc_log_relay_file_create(guc);
> > -	if (ret)
> > -		goto err_runtime;
> > -
> > -	return 0;
> > -
> > -err_runtime:
> > -	mutex_lock(&dev_priv->drm.struct_mutex);
> > -	guc_log_runtime_destroy(guc);
> > -	mutex_unlock(&dev_priv->drm.struct_mutex);
> > -err_relay:
> > -	intel_guc_log_relay_destroy(guc);
> > -err:
> > -	/* logging will remain off */
> > -	i915_modparams.guc_log_level = 0;
> > -	return ret;
> > -}
> > -
> >   static void guc_log_capture_logs(struct intel_guc *guc)
> >   {
> >   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> > @@ -574,16 +531,6 @@ 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)
> > -		return;
> > -
> > -	/* First disable the interrupts, will be renabled afterwards */
> > -	mutex_lock(&dev_priv->drm.struct_mutex);
> > -	intel_runtime_pm_get(dev_priv);
> > -	gen9_disable_guc_interrupts(dev_priv);
> > -	intel_runtime_pm_put(dev_priv);
> > -	mutex_unlock(&dev_priv->drm.struct_mutex);
> > -
> >   	/*
> >   	 * Before initiating the forceful flush, wait for any pending/ongoing
> >   	 * flush to complete otherwise forceful flush may not actually happen.
> > @@ -626,12 +573,6 @@ int intel_guc_log_create(struct intel_guc *guc)
> >   	guc->log.vma = vma;
> > -	if (i915_modparams.guc_log_level) {
> > -		ret = guc_log_runtime_create(guc);
> > -		if (ret < 0)
> > -			goto err_vma;
> > -	}
> > -
> >   	/* each allocated unit is a page */
> >   	flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL |
> >   		(GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) |
> > @@ -643,8 +584,6 @@ int intel_guc_log_create(struct intel_guc *guc)
> >   	return 0;
> > -err_vma:
> > -	i915_vma_unpin_and_release(&guc->log.vma);
> >   err:
> >   	/* logging will be off */
> >   	i915_modparams.guc_log_level = 0;
> > @@ -703,24 +642,14 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 val)
> >   	mutex_unlock(&dev_priv->drm.struct_mutex);
> >   	if (GUC_LOG_IS_ENABLED(val) && !guc_log_has_runtime(guc)) {
> > -		ret = guc_log_late_setup(guc);
> > -		if (ret)
> > +		ret = intel_guc_log_register(guc);
> > +		if (ret) {
> > +			/* logging will remain off */
> > +			i915_modparams.guc_log_level = 0;
> >   			goto out;
> > -
> > -		/* GuC logging is currently the only user of Guc2Host interrupts */
> > -		mutex_lock(&dev_priv->drm.struct_mutex);
> > -		intel_runtime_pm_get(dev_priv);
> > -		gen9_enable_guc_interrupts(dev_priv);
> > -		intel_runtime_pm_put(dev_priv);
> > -		mutex_unlock(&dev_priv->drm.struct_mutex);
> > +		}
> >   	} else if (!GUC_LOG_IS_ENABLED(val) && guc_log_has_runtime(guc)) {
> > -		/*
> > -		 * 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.
> > -		 */
> > -		guc_flush_logs(guc);
> > +		intel_guc_log_unregister(guc);
> >   	}
> >   	return 0;
> > @@ -731,20 +660,63 @@ int intel_guc_log_control_set(struct intel_guc *guc, u64 val)
> >   	return ret;
> >   }
> > -void i915_guc_log_register(struct drm_i915_private *dev_priv)
> > +int intel_guc_log_register(struct intel_guc *guc)
> >   {
> > -	if (!USES_GUC_SUBMISSION(dev_priv) || !i915_modparams.guc_log_level)
> > -		return;
> > +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> > +	int ret;
> > +
> > +	GEM_BUG_ON(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 = guc_log_relay_create(guc);
> > +	if (ret)
> > +		goto err;
> > +
> > +	mutex_lock(&dev_priv->drm.struct_mutex);
> > +	ret = guc_log_runtime_create(guc);
> > +	mutex_unlock(&dev_priv->drm.struct_mutex);
> > +
> > +	if (ret)
> > +		goto err_relay;
> > +
> > +	ret = guc_log_relay_file_create(guc);
> > +	if (ret)
> > +		goto err_runtime;
> > -	guc_log_late_setup(&dev_priv->guc);
> > +	/* GuC logging is currently the only user of Guc2Host interrupts */
> > +	mutex_lock(&dev_priv->drm.struct_mutex);
> > +	intel_runtime_pm_get(dev_priv);
> > +	gen9_enable_guc_interrupts(dev_priv);
> > +	intel_runtime_pm_put(dev_priv);
> > +	mutex_unlock(&dev_priv->drm.struct_mutex);
> > +
> > +	return 0;
> > +
> > +err_runtime:
> > +	mutex_lock(&dev_priv->drm.struct_mutex);
> > +	guc_log_runtime_destroy(guc);
> > +	mutex_unlock(&dev_priv->drm.struct_mutex);
> > +err_relay:
> > +	guc_log_relay_destroy(guc);
> > +err:
> > +	return ret;
> >   }
> > -void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
> > +void intel_guc_log_unregister(struct intel_guc *guc)
> >   {
> > -	struct intel_guc *guc = &dev_priv->guc;
> > +	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> > -	if (!USES_GUC_SUBMISSION(dev_priv))
> > -		return;
> > +	/*
> > +	 * 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.
> > +	 */
> > +	guc_flush_logs(guc);
> >   	mutex_lock(&dev_priv->drm.struct_mutex);
> >   	/* GuC logging is currently the only user of Guc2Host interrupts */
> > @@ -755,5 +727,5 @@ void i915_guc_log_unregister(struct drm_i915_private *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 141ce9ca22ce..09dd2ef1933d 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_log.h
> > +++ b/drivers/gpu/drm/i915/intel_guc_log.h
> > @@ -62,11 +62,9 @@ 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);
> >   int intel_guc_log_control_get(struct intel_guc *guc);
> >   int intel_guc_log_control_set(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);
> > +int intel_guc_log_register(struct intel_guc *guc);
> > +void intel_guc_log_unregister(struct intel_guc *guc);
> >   #endif
> > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> > index 9f1bac6398fb..3f41ef525e26 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.c
> > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > @@ -219,6 +219,22 @@ static void guc_free_load_err_log(struct intel_guc *guc)
> >   		i915_gem_object_put(guc->load_err_log);
> >   }
> > +int intel_uc_log_register(struct drm_i915_private *dev_priv)
> > +{
> > +	if (!USES_GUC(dev_priv) || !i915_modparams.guc_log_level)
> > +		return 0;
> > +
> > +	return intel_guc_log_register(&dev_priv->guc);
> > +}
> > +
> > +void intel_uc_log_unregister(struct drm_i915_private *dev_priv)
> > +{
> > +	if (!USES_GUC(dev_priv) || !i915_modparams.guc_log_level)
> > +		return;
> > +
> > +	intel_guc_log_unregister(&dev_priv->guc);
> > +}
> > +
> >   static int guc_enable_communication(struct intel_guc *guc)
> >   {
> >   	struct drm_i915_private *dev_priv = guc_to_i915(guc);
> > @@ -240,7 +256,7 @@ static void guc_disable_communication(struct intel_guc *guc)
> >   	guc->send = intel_guc_send_nop;
> >   }
> > -int intel_uc_init_misc(struct drm_i915_private *dev_priv)
> > +int intel_uc_init_wq(struct drm_i915_private *dev_priv)
> >   {
> >   	struct intel_guc *guc = &dev_priv->guc;
> >   	int ret;
> > @@ -249,26 +265,13 @@ int intel_uc_init_misc(struct drm_i915_private *dev_priv)
> >   		return 0;
> >   	ret = intel_guc_init_wq(guc);
> > -	if (ret) {
> > -		DRM_ERROR("Couldn't allocate workqueues for GuC\n");
> > -		goto err;
> > -	}
> > -
> > -	ret = intel_guc_log_relay_create(guc);
> > -	if (ret) {
> > -		DRM_ERROR("Couldn't allocate relay for GuC log\n");
> > -		goto err_relay;
> > -	}
> > +	if (ret)
> > +		return ret;
> >   	return 0;
> > -
> > -err_relay:
> > -	intel_guc_fini_wq(guc);
> > -err:
> > -	return ret;
> >   }
> > -void intel_uc_fini_misc(struct drm_i915_private *dev_priv)
> > +void intel_uc_fini_wq(struct drm_i915_private *dev_priv)
> >   {
> >   	struct intel_guc *guc = &dev_priv->guc;
> > @@ -276,8 +279,6 @@ void intel_uc_fini_misc(struct drm_i915_private *dev_priv)
> >   		return;
> >   	intel_guc_fini_wq(guc);
> > -
> > -	intel_guc_log_relay_destroy(guc);
> >   }
> >   int intel_uc_init(struct drm_i915_private *dev_priv)
> > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> > index f2984e01e257..9bc22103396d 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.h
> > +++ b/drivers/gpu/drm/i915/intel_uc.h
> > @@ -31,10 +31,12 @@
> >   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_log_register(struct drm_i915_private *dev_priv);
> > +void intel_uc_log_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);
> > -void intel_uc_fini_misc(struct drm_i915_private *dev_priv);
> > +int intel_uc_init_wq(struct drm_i915_private *dev_priv);
> > +void intel_uc_fini_wq(struct drm_i915_private *dev_priv);
> >   int intel_uc_init_hw(struct drm_i915_private *dev_priv);
> >   void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
> >   int intel_uc_init(struct drm_i915_private *dev_priv);
> 
> -- 
> Thanks,
> Sagar
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux