[Intel-gfx] [PATCH 02/15] drm/i915/guc: Create common entry points for log register/unregister
Sagar Arun Kamble
sagar.a.kamble at intel.com
Tue Mar 6 05:24:54 UTC 2018
On 3/5/2018 7:08 PM, Michał Winiarski wrote:
> 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).
But AFAIU change in that patch (8/15) will be decoupling
guc_log_register from USES_GUC/guc_log_level which would be fine I guess.
Your call :)
> -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 at intel.com>
>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>>> ---
>>> 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
>>
--
Thanks,
Sagar
More information about the Intel-gfx
mailing list