[Intel-gfx] [PATCH 3/3] drm/i915/uc: replace uc init/fini misc
Daniele Ceraolo Spurio
daniele.ceraolospurio at intel.com
Tue Jul 2 21:22:23 UTC 2019
On 7/2/19 1:52 PM, Michal Wajdeczko wrote:
> On Tue, 02 Jul 2019 22:09:47 +0200, Daniele Ceraolo Spurio
> <daniele.ceraolospurio at intel.com> wrote:
>
>> The "misc" terminology doesn't clearly explain what we intend to cover
>> in this phase. The only thing we do in there apart from FW fetch is
>> initializing the log workqueue. We can move the latter to the
>> init_early phase and rename the function to clarify that they only
>> fetch/release the blobs.
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_drv.c | 7 +++-
>> drivers/gpu/drm/i915/i915_gem.c | 12 +++---
>> drivers/gpu/drm/i915/intel_guc.c | 57 ++++------------------------
>> drivers/gpu/drm/i915/intel_guc.h | 5 +--
>> drivers/gpu/drm/i915/intel_guc_log.c | 32 +++++++++++++++-
>> drivers/gpu/drm/i915/intel_guc_log.h | 3 +-
>> drivers/gpu/drm/i915/intel_huc.c | 8 ----
>> drivers/gpu/drm/i915/intel_huc.h | 6 ---
>> drivers/gpu/drm/i915/intel_uc.c | 50 ++++++++++--------------
>> drivers/gpu/drm/i915/intel_uc.h | 6 +--
>> 10 files changed, 75 insertions(+), 111 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index 794c6814a6d0..20ca0208dd98 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -934,7 +934,11 @@ static int i915_driver_init_early(struct
>> drm_i915_private *dev_priv)
>> intel_detect_pch(dev_priv);
>> intel_wopcm_init_early(&dev_priv->wopcm);
>> - intel_uc_init_early(dev_priv);
>> +
>> + ret = intel_uc_init_early(dev_priv);
>> + if (ret)
>> + goto err_gem;
>> +
>> intel_pm_setup(dev_priv);
>> intel_init_dpio(dev_priv);
>> ret = intel_power_domains_init(dev_priv);
>> @@ -953,6 +957,7 @@ static int i915_driver_init_early(struct
>> drm_i915_private *dev_priv)
>> err_uc:
>> intel_uc_cleanup_early(dev_priv);
>> +err_gem:
>> i915_gem_cleanup_early(dev_priv);
>> err_workqueues:
>> i915_workqueues_cleanup(dev_priv);
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index b7f290b77f8f..46a75844f303 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -1429,13 +1429,11 @@ int i915_gem_init(struct drm_i915_private
>> *dev_priv)
>> if (ret)
>> return ret;
>> - ret = intel_uc_init_misc(dev_priv);
>> - if (ret)
>> - return ret;
>> + intel_uc_fetch_firmwares(dev_priv);
>> ret = intel_wopcm_init(&dev_priv->wopcm);
>> if (ret)
>> - goto err_uc_misc;
>> + goto err_uc_fw;
>> /* This is just a security blanket to placate dragons.
>> * On some systems, we very sporadically observe that the first TLBs
>> @@ -1561,8 +1559,8 @@ int i915_gem_init(struct drm_i915_private
>> *dev_priv)
>> intel_uncore_forcewake_put(&dev_priv->uncore, FORCEWAKE_ALL);
>> mutex_unlock(&dev_priv->drm.struct_mutex);
>> -err_uc_misc:
>> - intel_uc_fini_misc(dev_priv);
>> +err_uc_fw:
>> + intel_uc_cleanup_firmwares(dev_priv);
>> if (ret != -EIO) {
>> i915_gem_cleanup_userptr(dev_priv);
>> @@ -1628,7 +1626,7 @@ void i915_gem_fini(struct drm_i915_private
>> *dev_priv)
>> intel_cleanup_gt_powersave(dev_priv);
>> - intel_uc_fini_misc(dev_priv);
>> + intel_uc_cleanup_firmwares(dev_priv);
>> i915_gem_cleanup_userptr(dev_priv);
>> intel_timelines_fini(dev_priv);
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index 501b74f44374..03fad4fe3d9b 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -74,13 +74,16 @@ void intel_guc_init_send_regs(struct intel_guc *guc)
>> guc->send_regs.fw_domains = fw_domains;
>> }
>> -void intel_guc_init_early(struct intel_guc *guc)
>> +int intel_guc_init_early(struct intel_guc *guc)
>> {
>> struct drm_i915_private *i915 = guc_to_i915(guc);
>> + int ret;
>> intel_guc_fw_init_early(guc);
>> intel_guc_ct_init_early(&guc->ct);
>> - intel_guc_log_init_early(&guc->log);
>> + ret = intel_guc_log_init_early(&guc->log);
>> + if (ret)
>> + return ret;
>> mutex_init(&guc->send_mutex);
>> spin_lock_init(&guc->irq_lock);
>> @@ -97,59 +100,13 @@ void intel_guc_init_early(struct intel_guc *guc)
>> guc->interrupts.enable = gen9_enable_guc_interrupts;
>> guc->interrupts.disable = gen9_disable_guc_interrupts;
>> }
>> -}
>> -
>> -static int guc_init_wq(struct intel_guc *guc)
>> -{
>> - /*
>> - * GuC log buffer flush work item has to do register access to
>> - * send the ack to GuC and this work item, if not synced before
>> - * suspend, can potentially get executed after the GFX device is
>> - * suspended.
>> - * By marking the WQ as freezable, we don't have to bother about
>> - * flushing of this work item from the suspend hooks, the pending
>> - * work item if any will be either executed before the suspend
>> - * or scheduled later on resume. This way the handling of work
>> - * item can be kept same between system suspend & rpm suspend.
>> - */
>> - guc->log.relay.flush_wq =
>> - alloc_ordered_workqueue("i915-guc_log",
>> - WQ_HIGHPRI | WQ_FREEZABLE);
>> - if (!guc->log.relay.flush_wq) {
>> - DRM_ERROR("Couldn't allocate workqueue for GuC log\n");
>> - return -ENOMEM;
>> - }
>> return 0;
>> }
>> -static void guc_fini_wq(struct intel_guc *guc)
>> -{
>> - struct workqueue_struct *wq;
>> -
>> - wq = fetch_and_zero(&guc->log.relay.flush_wq);
>> - if (wq)
>> - destroy_workqueue(wq);
>> -}
>> -
>> -int intel_guc_init_misc(struct intel_guc *guc)
>> +void intel_guc_fini_early(struct intel_guc *guc)
>
> looks like this function can be defined as inline in .h
>
I thought we were moving away from inlines in .h files when it is not
required for performance.
>> {
>> - struct drm_i915_private *i915 = guc_to_i915(guc);
>> - int ret;
>> -
>> - ret = guc_init_wq(guc);
>> - if (ret)
>> - return ret;
>> -
>> - intel_uc_fw_fetch(i915, &guc->fw);
>> -
>> - return 0;
>> -}
>> -
>> -void intel_guc_fini_misc(struct intel_guc *guc)
>> -{
>> - intel_uc_fw_cleanup_fetch(&guc->fw);
>> - guc_fini_wq(guc);
>> + intel_guc_log_fini_early(&guc->log);
>> }
>> static int guc_shared_data_create(struct intel_guc *guc)
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index ec1038c1f50e..045a3608bade 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -150,13 +150,12 @@ static inline u32 intel_guc_ggtt_offset(struct
>> intel_guc *guc,
>> return offset;
>> }
>> -void intel_guc_init_early(struct intel_guc *guc);
>> +int intel_guc_init_early(struct intel_guc *guc);
>> +void intel_guc_fini_early(struct intel_guc *guc);
>> void intel_guc_init_send_regs(struct intel_guc *guc);
>> void intel_guc_init_params(struct intel_guc *guc);
>> -int intel_guc_init_misc(struct intel_guc *guc);
>> int intel_guc_init(struct intel_guc *guc);
>> void intel_guc_fini(struct intel_guc *guc);
>> -void intel_guc_fini_misc(struct intel_guc *guc);
>> int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32
>> len,
>> u32 *response_buf, u32 response_buf_size);
>> int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32
>> len,
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c
>> b/drivers/gpu/drm/i915/intel_guc_log.c
>> index e3b83ecb90b5..4b1c7a9f23e7 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -374,10 +374,40 @@ static void guc_log_unmap(struct intel_guc_log
>> *log)
>> log->relay.buf_addr = NULL;
>> }
>> -void intel_guc_log_init_early(struct intel_guc_log *log)
>> +int intel_guc_log_init_early(struct intel_guc_log *log)
>> {
>> mutex_init(&log->relay.lock);
>> INIT_WORK(&log->relay.flush_work, capture_logs_work);
>> +
>> + /*
>> + * GuC log buffer flush work item has to do register access to
>> + * send the ack to GuC and this work item, if not synced before
>> + * suspend, can potentially get executed after the GFX device is
>> + * suspended.
>> + * By marking the WQ as freezable, we don't have to bother about
>> + * flushing of this work item from the suspend hooks, the pending
>> + * work item if any will be either executed before the suspend
>> + * or scheduled later on resume. This way the handling of work
>> + * item can be kept same between system suspend & rpm suspend.
>> + */
>> + log->relay.flush_wq =
>> + alloc_ordered_workqueue("i915-guc_log",
>> + WQ_HIGHPRI | WQ_FREEZABLE);
>
> I'm wondering if this is ok that now we always create wq regardless GuC
> is used or not ... in now removed init_misc we checked for USES_GUC
>
> maybe we can use other already existing wq for flush work instead?
>
I don't think we have another suitable wq. I could reorder uc_init_early
instead to only allocate the wq if USES_GUC is true, e.g.:
int intel_uc_init_early(struct drm_i915_private *i915)
{
struct intel_guc *guc = &i915->guc;
struct intel_huc *huc = &i915->huc;
int ret;
intel_guc_fw_init_early(guc);
intel_huc_fw_init_early(huc);
/* option sanitizing requires fw selection */
sanitize_options_early(i915);
if (!USES_GUC(i915))
return 0;
ret = intel_guc_init_early(guc);
if (ret)
return ret;
intel_huc_init_early(huc);
return 0;
}
What do you think?
>> + if (!log->relay.flush_wq) {
>> + DRM_ERROR("Couldn't allocate workqueue for GuC log\n");
>> + return -ENOMEM;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +void intel_guc_log_fini_early(struct intel_guc_log *log)
>> +{
>> + struct workqueue_struct *wq;
>> +
>> + wq = fetch_and_zero(&log->relay.flush_wq);
>> + if (wq)
>
> do we need to check this? if we failed to allocate wq in init_early
> we should not be here
>
True, will update.
Daniele
>> + destroy_workqueue(wq);
>> }
>> static int guc_log_relay_create(struct intel_guc_log *log)
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.h
>> b/drivers/gpu/drm/i915/intel_guc_log.h
>> index 7bc763f10c03..8b5e2fc7df24 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.h
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.h
>> @@ -80,7 +80,8 @@ struct intel_guc_log {
>> } stats[GUC_MAX_LOG_BUFFER];
>> };
>> -void intel_guc_log_init_early(struct intel_guc_log *log);
>> +int intel_guc_log_init_early(struct intel_guc_log *log);
>> +void intel_guc_log_fini_early(struct intel_guc_log *log);
>> int intel_guc_log_create(struct intel_guc_log *log);
>> void intel_guc_log_destroy(struct intel_guc_log *log);
>> diff --git a/drivers/gpu/drm/i915/intel_huc.c
>> b/drivers/gpu/drm/i915/intel_huc.c
>> index fb6f693d3cac..2a41ee89a16d 100644
>> --- a/drivers/gpu/drm/i915/intel_huc.c
>> +++ b/drivers/gpu/drm/i915/intel_huc.c
>> @@ -44,14 +44,6 @@ void intel_huc_init_early(struct intel_huc *huc)
>> }
>> }
>> -int intel_huc_init_misc(struct intel_huc *huc)
>> -{
>> - struct drm_i915_private *i915 = huc_to_i915(huc);
>> -
>> - intel_uc_fw_fetch(i915, &huc->fw);
>> - return 0;
>> -}
>> -
>> static int intel_huc_rsa_data_create(struct intel_huc *huc)
>> {
>> struct drm_i915_private *i915 = huc_to_i915(huc);
>> diff --git a/drivers/gpu/drm/i915/intel_huc.h
>> b/drivers/gpu/drm/i915/intel_huc.h
>> index 2a6c94e79f17..9fa3d4629f2e 100644
>> --- a/drivers/gpu/drm/i915/intel_huc.h
>> +++ b/drivers/gpu/drm/i915/intel_huc.h
>> @@ -45,17 +45,11 @@ struct intel_huc {
>> };
>> void intel_huc_init_early(struct intel_huc *huc);
>> -int intel_huc_init_misc(struct intel_huc *huc);
>> int intel_huc_init(struct intel_huc *huc);
>> void intel_huc_fini(struct intel_huc *huc);
>> int intel_huc_auth(struct intel_huc *huc);
>> int intel_huc_check_status(struct intel_huc *huc);
>> -static inline void intel_huc_fini_misc(struct intel_huc *huc)
>> -{
>> - intel_uc_fw_cleanup_fetch(&huc->fw);
>> -}
>> -
>> static inline int intel_huc_sanitize(struct intel_huc *huc)
>> {
>> intel_uc_fw_sanitize(&huc->fw);
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index fdf00f1ebb57..c3e65236cfba 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -171,15 +171,19 @@ static void sanitize_options_early(struct
>> drm_i915_private *i915)
>> GEM_BUG_ON(i915_modparams.guc_log_level < 0);
>> }
>> -void intel_uc_init_early(struct drm_i915_private *i915)
>> +int intel_uc_init_early(struct drm_i915_private *i915)
>> {
>> - struct intel_guc *guc = &i915->guc;
>> - struct intel_huc *huc = &i915->huc;
>> + int ret;
>> +
>> + ret = intel_guc_init_early(&i915->guc);
>> + if (ret)
>> + return ret;
>> - intel_guc_init_early(guc);
>> - intel_huc_init_early(huc);
>> + intel_huc_init_early(&i915->huc);
>> sanitize_options_early(i915);
>> +
>> + return 0;
>> }
>> void intel_uc_cleanup_early(struct drm_i915_private *i915)
>> @@ -187,6 +191,8 @@ void intel_uc_cleanup_early(struct
>> drm_i915_private *i915)
>> struct intel_guc *guc = &i915->guc;
>> guc_free_load_err_log(guc);
>> +
>> + intel_guc_fini_early(guc);
>> }
>> /**
>> @@ -345,44 +351,26 @@ static void guc_disable_communication(struct
>> intel_guc *guc)
>> DRM_INFO("GuC communication disabled\n");
>> }
>> -int intel_uc_init_misc(struct drm_i915_private *i915)
>> +void intel_uc_fetch_firmwares(struct drm_i915_private *i915)
>> {
>> - struct intel_guc *guc = &i915->guc;
>> - struct intel_huc *huc = &i915->huc;
>> - int ret;
>> -
>> if (!USES_GUC(i915))
>> - return 0;
>> -
>> - ret = intel_guc_init_misc(guc);
>> - if (ret)
>> - return ret;
>> + return;
>> - if (USES_HUC(i915)) {
>> - ret = intel_huc_init_misc(huc);
>> - if (ret)
>> - goto err_guc;
>> - }
>> + intel_uc_fw_fetch(i915, &i915->guc.fw);
>> - return 0;
>> -
>> -err_guc:
>> - intel_guc_fini_misc(guc);
>> - return ret;
>> + if (USES_HUC(i915))
>> + intel_uc_fw_fetch(i915, &i915->huc.fw);
>> }
>> -void intel_uc_fini_misc(struct drm_i915_private *i915)
>> +void intel_uc_cleanup_firmwares(struct drm_i915_private *i915)
>> {
>> - struct intel_guc *guc = &i915->guc;
>> - struct intel_huc *huc = &i915->huc;
>> -
>> if (!USES_GUC(i915))
>> return;
>> if (USES_HUC(i915))
>> - intel_huc_fini_misc(huc);
>> + intel_uc_fw_cleanup_fetch(&i915->huc.fw);
>> - intel_guc_fini_misc(guc);
>> + intel_uc_fw_cleanup_fetch(&i915->guc.fw);
>> }
>> int intel_uc_init(struct drm_i915_private *i915)
>> diff --git a/drivers/gpu/drm/i915/intel_uc.h
>> b/drivers/gpu/drm/i915/intel_uc.h
>> index 3ea06c87dfcd..c3022890e604 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.h
>> +++ b/drivers/gpu/drm/i915/intel_uc.h
>> @@ -28,11 +28,11 @@
>> #include "intel_huc.h"
>> #include "i915_params.h"
>> -void intel_uc_init_early(struct drm_i915_private *dev_priv);
>> +int intel_uc_init_early(struct drm_i915_private *dev_priv);
>> void intel_uc_cleanup_early(struct drm_i915_private *dev_priv);
>> void intel_uc_init_mmio(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);
>> +void intel_uc_fetch_firmwares(struct drm_i915_private *dev_priv);
>> +void intel_uc_cleanup_firmwares(struct drm_i915_private *dev_priv);
>> void intel_uc_sanitize(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);
More information about the Intel-gfx
mailing list