* [PATCH v13 01/21] drm/i915/guc: Add GuC submission initialization/enable state variables
2017-10-11 8:53 [PATCH v13 00/21] drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring Sagar Arun Kamble
@ 2017-10-11 8:53 ` Sagar Arun Kamble
2017-10-11 8:53 ` [PATCH v13 02/21] drm/i915/guc: Sanitize module parameter guc_log_level Sagar Arun Kamble
` (20 subsequent siblings)
21 siblings, 0 replies; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-11 8:53 UTC (permalink / raw)
To: intel-gfx
In order to reduce the dependency on enable_guc_submission parameter
we are adding GuC submission status variables, submission_initialized
and submission_enabled to track the initialization/enable status.
i915_guc_submission_initialized and i915_guc_submission_enabled need
to be used at places needing the check.
v2: Added status variables instead of depending on stage_desc_pool
and execbuf_client. (Joonas, Chris) Replaced other usages of
enable_guc_submission with i915_guc_submission_enabled check. It is
important to note that this status based enable/disable helps identify
paths that are not sanitizing properly like in reset/unload currently.
Future patches in the series will fix that sanitization.
Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_drv.c | 2 +-
drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
drivers/gpu/drm/i915/i915_guc_submission.c | 19 ++++++++++++++++++-
drivers/gpu/drm/i915/i915_irq.c | 2 +-
drivers/gpu/drm/i915/intel_guc.h | 14 ++++++++++++++
drivers/gpu/drm/i915/intel_guc_loader.c | 10 +++++-----
drivers/gpu/drm/i915/intel_guc_log.c | 10 +++++-----
drivers/gpu/drm/i915/intel_lrc.c | 2 +-
drivers/gpu/drm/i915/intel_uc.c | 23 +++++++++++++----------
9 files changed, 59 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 66fc156..8e25846 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -374,7 +374,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
if (INTEL_INFO(dev_priv)->has_logical_ring_preemption &&
i915_modparams.enable_execlists &&
- !i915_modparams.enable_guc_submission)
+ !i915_guc_submission_enabled(&dev_priv->guc))
value |= I915_SCHEDULER_CAP_PREEMPTION;
}
break;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 5bf96a2..f7c24c0 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -407,7 +407,7 @@ struct i915_gem_context *
i915_gem_context_set_closed(ctx); /* not user accessible */
i915_gem_context_clear_bannable(ctx);
i915_gem_context_set_force_single_submission(ctx);
- if (!i915_modparams.enable_guc_submission)
+ if (!i915_guc_submission_enabled(&ctx->i915->guc))
ctx->ring_size = 512 * PAGE_SIZE; /* Max ring buffer size */
GEM_BUG_ON(i915_gem_context_is_kernel(ctx));
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index f15de4d..3da1346 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -975,7 +975,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
void *vaddr;
int ret;
- if (guc->stage_desc_pool)
+ if (i915_guc_submission_initialized(guc))
return 0;
vma = intel_guc_allocate_vma(guc,
@@ -1004,6 +1004,8 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
ida_init(&guc->stage_ids);
+ guc->submission_initialized = true;
+
return 0;
err_log:
@@ -1019,11 +1021,16 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
{
struct intel_guc *guc = &dev_priv->guc;
+ if (!i915_guc_submission_initialized(guc))
+ return;
+
ida_destroy(&guc->stage_ids);
guc_ads_destroy(guc);
intel_guc_log_destroy(guc);
i915_gem_object_unpin_map(guc->stage_desc_pool->obj);
i915_vma_unpin_and_release(&guc->stage_desc_pool);
+
+ guc->submission_initialized = false;
}
static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
@@ -1100,6 +1107,9 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
enum intel_engine_id id;
int err;
+ if (i915_guc_submission_enabled(guc))
+ return 0;
+
/*
* We're using GuC work items for submitting work through GuC. Since
* we're coalescing multiple requests from a single context into a
@@ -1151,6 +1161,8 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
tasklet_schedule(&execlists->irq_tasklet);
}
+ guc->submission_enabled = true;
+
return 0;
err_execbuf_client:
@@ -1163,6 +1175,9 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
{
struct intel_guc *guc = &dev_priv->guc;
+ if (!i915_guc_submission_enabled(guc))
+ return;
+
guc_interrupts_release(dev_priv);
/* Revert back to manual ELSP submission */
@@ -1170,4 +1185,6 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
guc_client_free(guc->execbuf_client);
guc->execbuf_client = NULL;
+
+ guc->submission_enabled = false;
}
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 3736290..a3de408 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1388,7 +1388,7 @@ static void snb_gt_irq_handler(struct drm_i915_private *dev_priv,
if (iir & (GT_RENDER_USER_INTERRUPT << test_shift)) {
notify_ring(engine);
- tasklet |= i915_modparams.enable_guc_submission;
+ tasklet |= i915_guc_submission_enabled(&engine->i915->guc);
}
if (tasklet)
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index aa9a7b5..5046264f 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -44,6 +44,10 @@ struct intel_guc {
/* intel_guc_recv interrupt related state */
bool interrupts_enabled;
+ /* GuC submission related state */
+ bool submission_initialized;
+ bool submission_enabled;
+
struct i915_vma *ads_vma;
struct i915_vma *stage_desc_pool;
void *stage_desc_pool_vaddr;
@@ -93,6 +97,16 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
return offset;
}
+static inline bool i915_guc_submission_initialized(struct intel_guc *guc)
+{
+ return guc->submission_initialized;
+}
+
+static inline bool i915_guc_submission_enabled(struct intel_guc *guc)
+{
+ return guc->submission_enabled;
+}
+
void intel_guc_init_early(struct intel_guc *guc);
void intel_guc_init_send_regs(struct intel_guc *guc);
int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len);
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index c7a800a..5449e5e 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -137,8 +137,10 @@ static void guc_params_init(struct drm_i915_private *dev_priv)
} else
params[GUC_CTL_DEBUG] = GUC_LOG_DISABLED;
- /* If GuC submission is enabled, set up additional parameters here */
- if (i915_modparams.enable_guc_submission) {
+ /*
+ * If GuC submission is initialized, set up additional parameters here.
+ */
+ if (i915_guc_submission_initialized(guc)) {
u32 ads = guc_ggtt_offset(guc->ads_vma) >> PAGE_SHIFT;
u32 pgs = guc_ggtt_offset(dev_priv->guc.stage_desc_pool);
u32 ctx_in_16 = GUC_MAX_STAGE_DESCRIPTORS / 16;
@@ -367,9 +369,7 @@ int intel_guc_init_hw(struct intel_guc *guc)
guc->fw.load_status = INTEL_UC_FIRMWARE_SUCCESS;
- DRM_INFO("GuC %s (firmware %s [version %u.%u])\n",
- i915_modparams.enable_guc_submission ? "submission enabled" :
- "loaded",
+ DRM_INFO("GuC loaded (firmware %s [version %u.%u])\n",
guc->fw.path,
guc->fw.major_ver_found, guc->fw.minor_ver_found);
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 76d3eb1..0f201c0 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -505,8 +505,8 @@ static void guc_flush_logs(struct intel_guc *guc)
{
struct drm_i915_private *dev_priv = guc_to_i915(guc);
- if (!i915_modparams.enable_guc_submission ||
- (i915_modparams.guc_log_level < 0))
+ if (!i915_guc_submission_enabled(guc) ||
+ i915_modparams.guc_log_level < 0)
return;
/* First disable the interrupts, will be renabled afterwards */
@@ -646,8 +646,8 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
void i915_guc_log_register(struct drm_i915_private *dev_priv)
{
- if (!i915_modparams.enable_guc_submission ||
- (i915_modparams.guc_log_level < 0))
+ if (!i915_guc_submission_enabled(&dev_priv->guc) ||
+ i915_modparams.guc_log_level < 0)
return;
mutex_lock(&dev_priv->drm.struct_mutex);
@@ -657,7 +657,7 @@ void i915_guc_log_register(struct drm_i915_private *dev_priv)
void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
{
- if (!i915_modparams.enable_guc_submission)
+ if (!i915_guc_submission_enabled(&dev_priv->guc))
return;
mutex_lock(&dev_priv->drm.struct_mutex);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index fbfcf88..e9c3454 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1462,7 +1462,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
execlists->preempt = false;
/* After a GPU reset, we may have requests to replay */
- if (!i915_modparams.enable_guc_submission && execlists->first)
+ if (!i915_guc_submission_enabled(&dev_priv->guc) && execlists->first)
tasklet_schedule(&execlists->irq_tasklet);
return 0;
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 7b938e8..b33d469 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -212,11 +212,15 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
goto err_log_capture;
intel_huc_auth(&dev_priv->huc);
- if (i915_modparams.enable_guc_submission) {
+ if (i915_guc_submission_initialized(guc)) {
if (i915_modparams.guc_log_level >= 0)
gen9_enable_guc_interrupts(dev_priv);
ret = i915_guc_submission_enable(dev_priv);
+
+ DRM_INFO("GuC submission %s\n",
+ i915_guc_submission_enabled(guc) ?
+ "enabled" : "disabled");
if (ret)
goto err_interrupts;
}
@@ -238,8 +242,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
err_log_capture:
guc_capture_load_err_log(guc);
err_submission:
- if (i915_modparams.enable_guc_submission)
- i915_guc_submission_fini(dev_priv);
+ i915_guc_submission_fini(dev_priv);
err_guc:
i915_ggtt_disable_guc(dev_priv);
@@ -263,20 +266,20 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
{
- guc_free_load_err_log(&dev_priv->guc);
+ struct intel_guc *guc = &dev_priv->guc;
+
+ guc_free_load_err_log(guc);
if (!i915_modparams.enable_guc_loading)
return;
- if (i915_modparams.enable_guc_submission)
- i915_guc_submission_disable(dev_priv);
+ i915_guc_submission_disable(dev_priv);
- guc_disable_communication(&dev_priv->guc);
+ guc_disable_communication(guc);
- if (i915_modparams.enable_guc_submission) {
+ if (i915_guc_submission_enabled(guc))
gen9_disable_guc_interrupts(dev_priv);
- i915_guc_submission_fini(dev_priv);
- }
+ i915_guc_submission_fini(dev_priv);
i915_ggtt_disable_guc(dev_priv);
}
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 50+ messages in thread* [PATCH v13 02/21] drm/i915/guc: Sanitize module parameter guc_log_level
2017-10-11 8:53 [PATCH v13 00/21] drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring Sagar Arun Kamble
2017-10-11 8:53 ` [PATCH v13 01/21] drm/i915/guc: Add GuC submission initialization/enable state variables Sagar Arun Kamble
@ 2017-10-11 8:53 ` Sagar Arun Kamble
2017-10-11 14:51 ` Michal Wajdeczko
2017-10-11 8:53 ` [PATCH v13 03/21] drm/i915/guc: Add status checks to enable/disable_guc_interrupts Sagar Arun Kamble
` (19 subsequent siblings)
21 siblings, 1 reply; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-11 8:53 UTC (permalink / raw)
To: intel-gfx
Parameter guc_log_level needs to be sanitized based on GuC support and
enable_guc_loading parameter since it depends on them like
enable_guc_submission. This will make GuC logging paths independent of
enable_guc_submission parameter in further patches.
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/intel_uc.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index b33d469..3cf3cbd 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -56,6 +56,7 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
i915_modparams.enable_guc_loading = 0;
i915_modparams.enable_guc_submission = 0;
+ i915_modparams.guc_log_level = -1;
return;
}
@@ -72,9 +73,11 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
i915_modparams.enable_guc_loading = 0;
}
- /* Can't enable guc submission without guc loaded */
- if (!i915_modparams.enable_guc_loading)
+ /* Can't enable guc submission and logging without guc loaded */
+ if (!i915_modparams.enable_guc_loading) {
i915_modparams.enable_guc_submission = 0;
+ i915_modparams.guc_log_level = -1;
+ }
/* A negative value means "use platform default" */
if (i915_modparams.enable_guc_submission < 0)
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH v13 02/21] drm/i915/guc: Sanitize module parameter guc_log_level
2017-10-11 8:53 ` [PATCH v13 02/21] drm/i915/guc: Sanitize module parameter guc_log_level Sagar Arun Kamble
@ 2017-10-11 14:51 ` Michal Wajdeczko
2017-10-12 5:48 ` Sagar Arun Kamble
0 siblings, 1 reply; 50+ messages in thread
From: Michal Wajdeczko @ 2017-10-11 14:51 UTC (permalink / raw)
To: intel-gfx, Sagar Arun Kamble
On Wed, 11 Oct 2017 10:53:57 +0200, Sagar Arun Kamble
<sagar.a.kamble@intel.com> wrote:
> Parameter guc_log_level needs to be sanitized based on GuC support and
> enable_guc_loading parameter since it depends on them like
> enable_guc_submission. This will make GuC logging paths independent of
> enable_guc_submission parameter in further patches.
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_uc.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uc.c
> b/drivers/gpu/drm/i915/intel_uc.c
> index b33d469..3cf3cbd 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -56,6 +56,7 @@ void intel_uc_sanitize_options(struct drm_i915_private
> *dev_priv)
Btw, I think the message "Ignoring GuC options, no hardware\n" is also
applicable when user specified guc_log_level>=0. Please update condition
that controls that message.
> i915_modparams.enable_guc_loading = 0;
> i915_modparams.enable_guc_submission = 0;
> + i915_modparams.guc_log_level = -1;
> return;
> }
> @@ -72,9 +73,11 @@ void intel_uc_sanitize_options(struct
> drm_i915_private *dev_priv)
> i915_modparams.enable_guc_loading = 0;
> }
> - /* Can't enable guc submission without guc loaded */
> - if (!i915_modparams.enable_guc_loading)
> + /* Can't enable guc submission and logging without guc loaded */
> + if (!i915_modparams.enable_guc_loading) {
> i915_modparams.enable_guc_submission = 0;
> + i915_modparams.guc_log_level = -1;
> + }
> /* A negative value means "use platform default" */
> if (i915_modparams.enable_guc_submission < 0)
Looks good, but please also update condition in i915_guc_log_register()
as we may now rely only on sanitized guc_log_level:
void i915_guc_log_register(struct drm_i915_private *dev_priv)
{
if (!i915_modparams.enable_guc_submission ||
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
(i915_modparams.guc_log_level < 0))
return;
Also, maybe it is worth to update DOC in intel_guc_log.c and param
description in i915_params.c to indicate (obvious) dependency on guc.
Michal
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v13 02/21] drm/i915/guc: Sanitize module parameter guc_log_level
2017-10-11 14:51 ` Michal Wajdeczko
@ 2017-10-12 5:48 ` Sagar Arun Kamble
0 siblings, 0 replies; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-12 5:48 UTC (permalink / raw)
To: Michal Wajdeczko, intel-gfx
On 10/11/2017 8:21 PM, Michal Wajdeczko wrote:
> On Wed, 11 Oct 2017 10:53:57 +0200, Sagar Arun Kamble
> <sagar.a.kamble@intel.com> wrote:
>
>> Parameter guc_log_level needs to be sanitized based on GuC support and
>> enable_guc_loading parameter since it depends on them like
>> enable_guc_submission. This will make GuC logging paths independent of
>> enable_guc_submission parameter in further patches.
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_uc.c | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index b33d469..3cf3cbd 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -56,6 +56,7 @@ void intel_uc_sanitize_options(struct
>> drm_i915_private *dev_priv)
>
> Btw, I think the message "Ignoring GuC options, no hardware\n" is also
> applicable when user specified guc_log_level>=0. Please update condition
> that controls that message.
>
Yes. Will update that condition.
>> i915_modparams.enable_guc_loading = 0;
>> i915_modparams.enable_guc_submission = 0;
>> + i915_modparams.guc_log_level = -1;
>> return;
>> }
>> @@ -72,9 +73,11 @@ void intel_uc_sanitize_options(struct
>> drm_i915_private *dev_priv)
>> i915_modparams.enable_guc_loading = 0;
>> }
>> - /* Can't enable guc submission without guc loaded */
>> - if (!i915_modparams.enable_guc_loading)
>> + /* Can't enable guc submission and logging without guc loaded */
>> + if (!i915_modparams.enable_guc_loading) {
>> i915_modparams.enable_guc_submission = 0;
>> + i915_modparams.guc_log_level = -1;
>> + }
>> /* A negative value means "use platform default" */
>> if (i915_modparams.enable_guc_submission < 0)
>
> Looks good, but please also update condition in i915_guc_log_register()
> as we may now rely only on sanitized guc_log_level:
>
> void i915_guc_log_register(struct drm_i915_private *dev_priv)
> {
> if (!i915_modparams.enable_guc_submission ||
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> (i915_modparams.guc_log_level < 0))
> return;
>
This is updated in the later patches in the series.
> Also, maybe it is worth to update DOC in intel_guc_log.c and param
> description in i915_params.c to indicate (obvious) dependency on guc.
>
Sure. Will add this.
> Michal
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v13 03/21] drm/i915/guc: Add status checks to enable/disable_guc_interrupts
2017-10-11 8:53 [PATCH v13 00/21] drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring Sagar Arun Kamble
2017-10-11 8:53 ` [PATCH v13 01/21] drm/i915/guc: Add GuC submission initialization/enable state variables Sagar Arun Kamble
2017-10-11 8:53 ` [PATCH v13 02/21] drm/i915/guc: Sanitize module parameter guc_log_level Sagar Arun Kamble
@ 2017-10-11 8:53 ` Sagar Arun Kamble
2017-10-11 15:20 ` Michal Wajdeczko
2017-10-11 8:53 ` [PATCH v13 04/21] drm/i915/guc: Remove enable_guc_submission dependency for invoking GuC log functions Sagar Arun Kamble
` (18 subsequent siblings)
21 siblings, 1 reply; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-11 8:53 UTC (permalink / raw)
To: intel-gfx
GuC interrupts are currently enabled by Logging and disabled in different
scenarios. Make disabling check whether interrupts were already disabled
and similar for enable path. This will remove the state tracking for the
callers of these functions based on kernel parameters.
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_irq.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a3de408..6cf417c 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -455,18 +455,22 @@ void gen9_reset_guc_interrupts(struct drm_i915_private *dev_priv)
void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv)
{
+ if (READ_ONCE(dev_priv->guc.interrupts_enabled))
+ return;
+
spin_lock_irq(&dev_priv->irq_lock);
- if (!dev_priv->guc.interrupts_enabled) {
- WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
- dev_priv->pm_guc_events);
- dev_priv->guc.interrupts_enabled = true;
- gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
- }
+ WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
+ dev_priv->pm_guc_events);
+ dev_priv->guc.interrupts_enabled = true;
+ gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
spin_unlock_irq(&dev_priv->irq_lock);
}
void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv)
{
+ if (!READ_ONCE(dev_priv->guc.interrupts_enabled))
+ return;
+
spin_lock_irq(&dev_priv->irq_lock);
dev_priv->guc.interrupts_enabled = false;
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH v13 03/21] drm/i915/guc: Add status checks to enable/disable_guc_interrupts
2017-10-11 8:53 ` [PATCH v13 03/21] drm/i915/guc: Add status checks to enable/disable_guc_interrupts Sagar Arun Kamble
@ 2017-10-11 15:20 ` Michal Wajdeczko
2017-10-12 5:50 ` Sagar Arun Kamble
0 siblings, 1 reply; 50+ messages in thread
From: Michal Wajdeczko @ 2017-10-11 15:20 UTC (permalink / raw)
To: intel-gfx, Sagar Arun Kamble
On Wed, 11 Oct 2017 10:53:58 +0200, Sagar Arun Kamble
<sagar.a.kamble@intel.com> wrote:
> GuC interrupts are currently enabled by Logging and disabled in different
> scenarios. Make disabling check whether interrupts were already disabled
> and similar for enable path. This will remove the state tracking for the
> callers of these functions based on kernel parameters.
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_irq.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c
> b/drivers/gpu/drm/i915/i915_irq.c
> index a3de408..6cf417c 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -455,18 +455,22 @@ void gen9_reset_guc_interrupts(struct
> drm_i915_private *dev_priv)
> void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv)
> {
> + if (READ_ONCE(dev_priv->guc.interrupts_enabled))
Hmm, I don't like that functions from irq.c read and modify guc internal
members directly. I would expect that functions here just do their job
and any state is maintained by the helper function(s) in guc.c.
Also note that this change will not help scenario where one client will
try to disable irqs while other client still depends on them.
Michal
> + return;
> +
> spin_lock_irq(&dev_priv->irq_lock);
> - if (!dev_priv->guc.interrupts_enabled) {
> - WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
> - dev_priv->pm_guc_events);
> - dev_priv->guc.interrupts_enabled = true;
> - gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
> - }
> + WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
> + dev_priv->pm_guc_events);
> + dev_priv->guc.interrupts_enabled = true;
> + gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
> spin_unlock_irq(&dev_priv->irq_lock);
> }
> void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv)
> {
> + if (!READ_ONCE(dev_priv->guc.interrupts_enabled))
> + return;
> +
> spin_lock_irq(&dev_priv->irq_lock);
> dev_priv->guc.interrupts_enabled = false;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v13 03/21] drm/i915/guc: Add status checks to enable/disable_guc_interrupts
2017-10-11 15:20 ` Michal Wajdeczko
@ 2017-10-12 5:50 ` Sagar Arun Kamble
2017-10-12 6:17 ` Sagar Arun Kamble
0 siblings, 1 reply; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-12 5:50 UTC (permalink / raw)
To: Michal Wajdeczko, intel-gfx
On 10/11/2017 8:50 PM, Michal Wajdeczko wrote:
> On Wed, 11 Oct 2017 10:53:58 +0200, Sagar Arun Kamble
> <sagar.a.kamble@intel.com> wrote:
>
>> GuC interrupts are currently enabled by Logging and disabled in
>> different
>> scenarios. Make disabling check whether interrupts were already disabled
>> and similar for enable path. This will remove the state tracking for the
>> callers of these functions based on kernel parameters.
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_irq.c | 16 ++++++++++------
>> 1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index a3de408..6cf417c 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -455,18 +455,22 @@ void gen9_reset_guc_interrupts(struct
>> drm_i915_private *dev_priv)
>> void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv)
>> {
>> + if (READ_ONCE(dev_priv->guc.interrupts_enabled))
>
> Hmm, I don't like that functions from irq.c read and modify guc internal
> members directly. I would expect that functions here just do their job
> and any state is maintained by the helper function(s) in guc.c.
Sure will move to guc.c.
>
> Also note that this change will not help scenario where one client will
> try to disable irqs while other client still depends on them.
>
Will add refcounting then.
> Michal
>
>> + return;
>> +
>> spin_lock_irq(&dev_priv->irq_lock);
>> - if (!dev_priv->guc.interrupts_enabled) {
>> - WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
>> - dev_priv->pm_guc_events);
>> - dev_priv->guc.interrupts_enabled = true;
>> - gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
>> - }
>> + WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
>> + dev_priv->pm_guc_events);
>> + dev_priv->guc.interrupts_enabled = true;
>> + gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
>> spin_unlock_irq(&dev_priv->irq_lock);
>> }
>> void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv)
>> {
>> + if (!READ_ONCE(dev_priv->guc.interrupts_enabled))
>> + return;
>> +
>> spin_lock_irq(&dev_priv->irq_lock);
>> dev_priv->guc.interrupts_enabled = false;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v13 03/21] drm/i915/guc: Add status checks to enable/disable_guc_interrupts
2017-10-12 5:50 ` Sagar Arun Kamble
@ 2017-10-12 6:17 ` Sagar Arun Kamble
2017-10-13 8:09 ` Sagar Arun Kamble
0 siblings, 1 reply; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-12 6:17 UTC (permalink / raw)
To: Michal Wajdeczko, intel-gfx
On 10/12/2017 11:20 AM, Sagar Arun Kamble wrote:
>
>
> On 10/11/2017 8:50 PM, Michal Wajdeczko wrote:
>> On Wed, 11 Oct 2017 10:53:58 +0200, Sagar Arun Kamble
>> <sagar.a.kamble@intel.com> wrote:
>>
>>> GuC interrupts are currently enabled by Logging and disabled in
>>> different
>>> scenarios. Make disabling check whether interrupts were already
>>> disabled
>>> and similar for enable path. This will remove the state tracking for
>>> the
>>> callers of these functions based on kernel parameters.
>>>
>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_irq.c | 16 ++++++++++------
>>> 1 file changed, 10 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_irq.c
>>> b/drivers/gpu/drm/i915/i915_irq.c
>>> index a3de408..6cf417c 100644
>>> --- a/drivers/gpu/drm/i915/i915_irq.c
>>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>>> @@ -455,18 +455,22 @@ void gen9_reset_guc_interrupts(struct
>>> drm_i915_private *dev_priv)
>>> void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv)
>>> {
>>> + if (READ_ONCE(dev_priv->guc.interrupts_enabled))
>>
>> Hmm, I don't like that functions from irq.c read and modify guc internal
>> members directly. I would expect that functions here just do their job
>> and any state is maintained by the helper function(s) in guc.c.
> Sure will move to guc.c.
>>
>> Also note that this change will not help scenario where one client will
>> try to disable irqs while other client still depends on them.
>>
> Will add refcounting then.
realized that disable_guc_interrupts is currently happening twice during
unload and that can make
refcounting asymmetrical. So will stay with bool state for now and will
revisit during interrupts related
changes may be as precursor to GuC CT series.
>> Michal
>>
>>> + return;
>>> +
>>> spin_lock_irq(&dev_priv->irq_lock);
>>> - if (!dev_priv->guc.interrupts_enabled) {
>>> - WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
>>> - dev_priv->pm_guc_events);
>>> - dev_priv->guc.interrupts_enabled = true;
>>> - gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
>>> - }
>>> + WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
>>> + dev_priv->pm_guc_events);
>>> + dev_priv->guc.interrupts_enabled = true;
>>> + gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
>>> spin_unlock_irq(&dev_priv->irq_lock);
>>> }
>>> void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv)
>>> {
>>> + if (!READ_ONCE(dev_priv->guc.interrupts_enabled))
>>> + return;
>>> +
>>> spin_lock_irq(&dev_priv->irq_lock);
>>> dev_priv->guc.interrupts_enabled = false;
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v13 03/21] drm/i915/guc: Add status checks to enable/disable_guc_interrupts
2017-10-12 6:17 ` Sagar Arun Kamble
@ 2017-10-13 8:09 ` Sagar Arun Kamble
0 siblings, 0 replies; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-13 8:09 UTC (permalink / raw)
To: Michal Wajdeczko, intel-gfx
On 10/12/2017 11:47 AM, Sagar Arun Kamble wrote:
>
>
> On 10/12/2017 11:20 AM, Sagar Arun Kamble wrote:
>>
>>
>> On 10/11/2017 8:50 PM, Michal Wajdeczko wrote:
>>> On Wed, 11 Oct 2017 10:53:58 +0200, Sagar Arun Kamble
>>> <sagar.a.kamble@intel.com> wrote:
>>>
>>>> GuC interrupts are currently enabled by Logging and disabled in
>>>> different
>>>> scenarios. Make disabling check whether interrupts were already
>>>> disabled
>>>> and similar for enable path. This will remove the state tracking
>>>> for the
>>>> callers of these functions based on kernel parameters.
>>>>
>>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>>> ---
>>>> drivers/gpu/drm/i915/i915_irq.c | 16 ++++++++++------
>>>> 1 file changed, 10 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_irq.c
>>>> b/drivers/gpu/drm/i915/i915_irq.c
>>>> index a3de408..6cf417c 100644
>>>> --- a/drivers/gpu/drm/i915/i915_irq.c
>>>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>>>> @@ -455,18 +455,22 @@ void gen9_reset_guc_interrupts(struct
>>>> drm_i915_private *dev_priv)
>>>> void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv)
>>>> {
>>>> + if (READ_ONCE(dev_priv->guc.interrupts_enabled))
>>>
>>> Hmm, I don't like that functions from irq.c read and modify guc
>>> internal
>>> members directly. I would expect that functions here just do their job
>>> and any state is maintained by the helper function(s) in guc.c.
>> Sure will move to guc.c.
>>>
>>> Also note that this change will not help scenario where one client will
>>> try to disable irqs while other client still depends on them.
>>>
>> Will add refcounting then.
> realized that disable_guc_interrupts is currently happening twice
> during unload and that can make
> refcounting asymmetrical. So will stay with bool state for now and
> will revisit during interrupts related
> changes may be as precursor to GuC CT series.
Based on current interrupt mgmt - pm_ier/pm_imr are already handled by
intel_runtime_pm_*_interrupts.
And these are being done across suspend/reset properly. Just need to
order w.r.t GuC suspend/resume.
So gen9_*_guc_interrupts should not be called during fini. They will be
taken care of based on interrupt clients like
Logging, CT Buffer.
>>> Michal
>>>
>>>> + return;
>>>> +
>>>> spin_lock_irq(&dev_priv->irq_lock);
>>>> - if (!dev_priv->guc.interrupts_enabled) {
>>>> - WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
>>>> - dev_priv->pm_guc_events);
>>>> - dev_priv->guc.interrupts_enabled = true;
>>>> - gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
>>>> - }
>>>> + WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) &
>>>> + dev_priv->pm_guc_events);
>>>> + dev_priv->guc.interrupts_enabled = true;
>>>> + gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events);
>>>> spin_unlock_irq(&dev_priv->irq_lock);
>>>> }
>>>> void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv)
>>>> {
>>>> + if (!READ_ONCE(dev_priv->guc.interrupts_enabled))
>>>> + return;
>>>> +
>>>> spin_lock_irq(&dev_priv->irq_lock);
>>>> dev_priv->guc.interrupts_enabled = false;
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v13 04/21] drm/i915/guc: Remove enable_guc_submission dependency for invoking GuC log functions
2017-10-11 8:53 [PATCH v13 00/21] drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring Sagar Arun Kamble
` (2 preceding siblings ...)
2017-10-11 8:53 ` [PATCH v13 03/21] drm/i915/guc: Add status checks to enable/disable_guc_interrupts Sagar Arun Kamble
@ 2017-10-11 8:53 ` Sagar Arun Kamble
2017-10-11 15:40 ` Michal Wajdeczko
2017-10-11 8:54 ` [PATCH v13 05/21] drm/i915/guc: Update enable_guc_loading check in intel_uc_fini_hw Sagar Arun Kamble
` (17 subsequent siblings)
21 siblings, 1 reply; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-11 8:53 UTC (permalink / raw)
To: intel-gfx
With guc_log_level parameter sanitized and GuC interrupts control
functions made self sufficient w.r.t interrupts state, we can remove
the enable_guc_submission checks from flush_guc_logs and
i915_guc_log_register/unregister and intel_uc_fini_hw.
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/intel_guc_log.c | 9 ++-------
drivers/gpu/drm/i915/intel_uc.c | 3 +--
2 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 0f201c0..fb5eb2b9 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -505,8 +505,7 @@ static void guc_flush_logs(struct intel_guc *guc)
{
struct drm_i915_private *dev_priv = guc_to_i915(guc);
- if (!i915_guc_submission_enabled(guc) ||
- i915_modparams.guc_log_level < 0)
+ if (i915_modparams.guc_log_level < 0)
return;
/* First disable the interrupts, will be renabled afterwards */
@@ -646,8 +645,7 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
void i915_guc_log_register(struct drm_i915_private *dev_priv)
{
- if (!i915_guc_submission_enabled(&dev_priv->guc) ||
- i915_modparams.guc_log_level < 0)
+ if (i915_modparams.guc_log_level < 0)
return;
mutex_lock(&dev_priv->drm.struct_mutex);
@@ -657,9 +655,6 @@ void i915_guc_log_register(struct drm_i915_private *dev_priv)
void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
{
- if (!i915_guc_submission_enabled(&dev_priv->guc))
- return;
-
mutex_lock(&dev_priv->drm.struct_mutex);
/* GuC logging is currently the only user of Guc2Host interrupts */
gen9_disable_guc_interrupts(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 3cf3cbd..974434e 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -280,8 +280,7 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
guc_disable_communication(guc);
- if (i915_guc_submission_enabled(guc))
- gen9_disable_guc_interrupts(dev_priv);
+ gen9_disable_guc_interrupts(dev_priv);
i915_guc_submission_fini(dev_priv);
i915_ggtt_disable_guc(dev_priv);
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH v13 04/21] drm/i915/guc: Remove enable_guc_submission dependency for invoking GuC log functions
2017-10-11 8:53 ` [PATCH v13 04/21] drm/i915/guc: Remove enable_guc_submission dependency for invoking GuC log functions Sagar Arun Kamble
@ 2017-10-11 15:40 ` Michal Wajdeczko
2017-10-12 5:58 ` Sagar Arun Kamble
0 siblings, 1 reply; 50+ messages in thread
From: Michal Wajdeczko @ 2017-10-11 15:40 UTC (permalink / raw)
To: intel-gfx, Sagar Arun Kamble
On Wed, 11 Oct 2017 10:53:59 +0200, Sagar Arun Kamble
<sagar.a.kamble@intel.com> wrote:
> With guc_log_level parameter sanitized and GuC interrupts control
> functions made self sufficient w.r.t interrupts state, we can remove
> the enable_guc_submission checks from flush_guc_logs and
> i915_guc_log_register/unregister and intel_uc_fini_hw.
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_guc_log.c | 9 ++-------
> drivers/gpu/drm/i915/intel_uc.c | 3 +--
> 2 files changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c
> b/drivers/gpu/drm/i915/intel_guc_log.c
> index 0f201c0..fb5eb2b9 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -505,8 +505,7 @@ static void guc_flush_logs(struct intel_guc *guc)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> - if (!i915_guc_submission_enabled(guc) ||
> - i915_modparams.guc_log_level < 0)
> + if (i915_modparams.guc_log_level < 0)
> return;
> /* First disable the interrupts, will be renabled afterwards */
> @@ -646,8 +645,7 @@ int i915_guc_log_control(struct drm_i915_private
> *dev_priv, u64 control_val)
> void i915_guc_log_register(struct drm_i915_private *dev_priv)
> {
> - if (!i915_guc_submission_enabled(&dev_priv->guc) ||
> - i915_modparams.guc_log_level < 0)
> + if (i915_modparams.guc_log_level < 0)
I would expect above two fixes in patch 2/21
> return;
> mutex_lock(&dev_priv->drm.struct_mutex);
> @@ -657,9 +655,6 @@ void i915_guc_log_register(struct drm_i915_private
> *dev_priv)
> void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
> {
> - if (!i915_guc_submission_enabled(&dev_priv->guc))
> - return;
Hmm, as i915_guc_log_unregister() is called unconditionally by
i915_driver_unregister so maybe here we should have at least
if (i915_modparams.guc_log_level < 0)
return;
> -
> mutex_lock(&dev_priv->drm.struct_mutex);
> /* GuC logging is currently the only user of Guc2Host interrupts */
> gen9_disable_guc_interrupts(dev_priv);
What about dropping this gen9_disable_guc_interrupts from here
and rely on the intel_uc_fini_hw() that will do the same?
> diff --git a/drivers/gpu/drm/i915/intel_uc.c
> b/drivers/gpu/drm/i915/intel_uc.c
> index 3cf3cbd..974434e 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -280,8 +280,7 @@ void intel_uc_fini_hw(struct drm_i915_private
> *dev_priv)
> guc_disable_communication(guc);
> - if (i915_guc_submission_enabled(guc))
> - gen9_disable_guc_interrupts(dev_priv);
> + gen9_disable_guc_interrupts(dev_priv);
> i915_guc_submission_fini(dev_priv);
> i915_ggtt_disable_guc(dev_priv);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v13 04/21] drm/i915/guc: Remove enable_guc_submission dependency for invoking GuC log functions
2017-10-11 15:40 ` Michal Wajdeczko
@ 2017-10-12 5:58 ` Sagar Arun Kamble
0 siblings, 0 replies; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-12 5:58 UTC (permalink / raw)
To: Michal Wajdeczko, intel-gfx
On 10/11/2017 9:10 PM, Michal Wajdeczko wrote:
> On Wed, 11 Oct 2017 10:53:59 +0200, Sagar Arun Kamble
> <sagar.a.kamble@intel.com> wrote:
>
>> With guc_log_level parameter sanitized and GuC interrupts control
>> functions made self sufficient w.r.t interrupts state, we can remove
>> the enable_guc_submission checks from flush_guc_logs and
>> i915_guc_log_register/unregister and intel_uc_fini_hw.
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_guc_log.c | 9 ++-------
>> drivers/gpu/drm/i915/intel_uc.c | 3 +--
>> 2 files changed, 3 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c
>> b/drivers/gpu/drm/i915/intel_guc_log.c
>> index 0f201c0..fb5eb2b9 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_log.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
>> @@ -505,8 +505,7 @@ static void guc_flush_logs(struct intel_guc *guc)
>> {
>> struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> - if (!i915_guc_submission_enabled(guc) ||
>> - i915_modparams.guc_log_level < 0)
>> + if (i915_modparams.guc_log_level < 0)
>> return;
>> /* First disable the interrupts, will be renabled afterwards */
>> @@ -646,8 +645,7 @@ int i915_guc_log_control(struct drm_i915_private
>> *dev_priv, u64 control_val)
>> void i915_guc_log_register(struct drm_i915_private *dev_priv)
>> {
>> - if (!i915_guc_submission_enabled(&dev_priv->guc) ||
>> - i915_modparams.guc_log_level < 0)
>> + if (i915_modparams.guc_log_level < 0)
>
> I would expect above two fixes in patch 2/21
Since there are multiple places needing the update and for better
division I had created separate patches.
Will keep same for now.
>
>> return;
>> mutex_lock(&dev_priv->drm.struct_mutex);
>> @@ -657,9 +655,6 @@ void i915_guc_log_register(struct
>> drm_i915_private *dev_priv)
>> void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
>> {
>> - if (!i915_guc_submission_enabled(&dev_priv->guc))
>> - return;
>
> Hmm, as i915_guc_log_unregister() is called unconditionally by
> i915_driver_unregister so maybe here we should have at least
>
> if (i915_modparams.guc_log_level < 0)
> return;
Yes. Will add this.
>
>> -
>> mutex_lock(&dev_priv->drm.struct_mutex);
>> /* GuC logging is currently the only user of Guc2Host interrupts */
>> gen9_disable_guc_interrupts(dev_priv);
>
> What about dropping this gen9_disable_guc_interrupts from here
> and rely on the intel_uc_fini_hw() that will do the same?
>
This needs to be handled separately as if we remove it from here we will
have to synchronize the logging irq
with relay runtime destruction. Will defer this for now.
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 3cf3cbd..974434e 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -280,8 +280,7 @@ void intel_uc_fini_hw(struct drm_i915_private
>> *dev_priv)
>> guc_disable_communication(guc);
>> - if (i915_guc_submission_enabled(guc))
>> - gen9_disable_guc_interrupts(dev_priv);
>> + gen9_disable_guc_interrupts(dev_priv);
>> i915_guc_submission_fini(dev_priv);
>> i915_ggtt_disable_guc(dev_priv);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v13 05/21] drm/i915/guc: Update enable_guc_loading check in intel_uc_fini_hw
2017-10-11 8:53 [PATCH v13 00/21] drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring Sagar Arun Kamble
` (3 preceding siblings ...)
2017-10-11 8:53 ` [PATCH v13 04/21] drm/i915/guc: Remove enable_guc_submission dependency for invoking GuC log functions Sagar Arun Kamble
@ 2017-10-11 8:54 ` Sagar Arun Kamble
2017-10-11 8:54 ` [PATCH v13 06/21] drm/i915/guc: Pass intel_guc struct parameter to intel_guc_suspend/resume Sagar Arun Kamble
` (16 subsequent siblings)
21 siblings, 0 replies; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-11 8:54 UTC (permalink / raw)
To: intel-gfx
With most of the GuC related disabling/cleanup now separated from GuC
parameters, only function that needs GuC parameter enable_guc_loading
check is i915_disable_guc_ggtt as that is enabled based it. So call only
i915_disable_guc_ggtt when enable_guc_loading is set during uc_fini_hw.
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/intel_uc.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 974434e..7305486 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -273,9 +273,6 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
guc_free_load_err_log(guc);
- if (!i915_modparams.enable_guc_loading)
- return;
-
i915_guc_submission_disable(dev_priv);
guc_disable_communication(guc);
@@ -283,5 +280,6 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
gen9_disable_guc_interrupts(dev_priv);
i915_guc_submission_fini(dev_priv);
- i915_ggtt_disable_guc(dev_priv);
+ if (i915_modparams.enable_guc_loading)
+ i915_ggtt_disable_guc(dev_priv);
}
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 50+ messages in thread* [PATCH v13 06/21] drm/i915/guc: Pass intel_guc struct parameter to intel_guc_suspend/resume
2017-10-11 8:53 [PATCH v13 00/21] drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring Sagar Arun Kamble
` (4 preceding siblings ...)
2017-10-11 8:54 ` [PATCH v13 05/21] drm/i915/guc: Update enable_guc_loading check in intel_uc_fini_hw Sagar Arun Kamble
@ 2017-10-11 8:54 ` Sagar Arun Kamble
2017-10-11 15:50 ` Michal Wajdeczko
2017-10-11 8:54 ` [PATCH v13 07/21] drm/i915: Create GEM runtime resume helper and handle GEM runtime suspend error Sagar Arun Kamble
` (15 subsequent siblings)
21 siblings, 1 reply; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-11 8:54 UTC (permalink / raw)
To: intel-gfx
intel_guc_suspend and intel_guc_resume are GuC specific functions hence
update the parameter from dev_priv to intel_guc struct. While at it do
s/dev_priv/i915 in suspend/resume functions.
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_drv.c | 6 +++---
drivers/gpu/drm/i915/i915_gem.c | 2 +-
drivers/gpu/drm/i915/intel_guc.c | 20 ++++++++++----------
drivers/gpu/drm/i915/intel_guc.h | 4 ++--
4 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 8e25846..a500a81 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1710,7 +1710,7 @@ static int i915_drm_resume(struct drm_device *dev)
}
mutex_unlock(&dev->struct_mutex);
- intel_guc_resume(dev_priv);
+ intel_guc_resume(&dev_priv->guc);
intel_modeset_init_hw(dev);
@@ -2518,7 +2518,7 @@ static int intel_runtime_suspend(struct device *kdev)
*/
i915_gem_runtime_suspend(dev_priv);
- intel_guc_suspend(dev_priv);
+ intel_guc_suspend(&dev_priv->guc);
intel_runtime_pm_disable_interrupts(dev_priv);
@@ -2603,7 +2603,7 @@ static int intel_runtime_resume(struct device *kdev)
if (intel_uncore_unclaimed_mmio(dev_priv))
DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n");
- intel_guc_resume(dev_priv);
+ intel_guc_resume(&dev_priv->guc);
if (IS_GEN9_LP(dev_priv)) {
bxt_disable_dc9(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e829e8c..276166d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4645,7 +4645,7 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
i915_gem_contexts_lost(dev_priv);
mutex_unlock(&dev->struct_mutex);
- intel_guc_suspend(dev_priv);
+ intel_guc_suspend(&dev_priv->guc);
cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
cancel_delayed_work_sync(&dev_priv->gt.retire_work);
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index bbe4c32..9a2df69 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -169,20 +169,20 @@ int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset)
/**
* intel_guc_suspend() - notify GuC entering suspend state
- * @dev_priv: i915 device private
+ * @guc: intel_guc struct
*/
-int intel_guc_suspend(struct drm_i915_private *dev_priv)
+int intel_guc_suspend(struct intel_guc *guc)
{
- struct intel_guc *guc = &dev_priv->guc;
+ struct drm_i915_private *i915 = guc_to_i915(guc);
struct i915_gem_context *ctx;
u32 data[3];
if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
return 0;
- gen9_disable_guc_interrupts(dev_priv);
+ gen9_disable_guc_interrupts(i915);
- ctx = dev_priv->kernel_context;
+ ctx = i915->kernel_context;
data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
/* any value greater than GUC_POWER_D0 */
@@ -196,11 +196,11 @@ int intel_guc_suspend(struct drm_i915_private *dev_priv)
/**
* intel_guc_resume() - notify GuC resuming from suspend state
- * @dev_priv: i915 device private
+ * @guc: intel_guc struct
*/
-int intel_guc_resume(struct drm_i915_private *dev_priv)
+int intel_guc_resume(struct intel_guc *guc)
{
- struct intel_guc *guc = &dev_priv->guc;
+ struct drm_i915_private *i915 = guc_to_i915(guc);
struct i915_gem_context *ctx;
u32 data[3];
@@ -208,9 +208,9 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
return 0;
if (i915_modparams.guc_log_level >= 0)
- gen9_enable_guc_interrupts(dev_priv);
+ gen9_enable_guc_interrupts(i915);
- ctx = dev_priv->kernel_context;
+ ctx = i915->kernel_context;
data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
data[1] = GUC_POWER_D0;
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 5046264f..a587210 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -113,8 +113,8 @@ static inline bool i915_guc_submission_enabled(struct intel_guc *guc)
int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len);
int intel_guc_sample_forcewake(struct intel_guc *guc);
int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
-int intel_guc_suspend(struct drm_i915_private *dev_priv);
-int intel_guc_resume(struct drm_i915_private *dev_priv);
+int intel_guc_suspend(struct intel_guc *guc);
+int intel_guc_resume(struct intel_guc *guc);
struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
int intel_guc_select_fw(struct intel_guc *guc);
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH v13 06/21] drm/i915/guc: Pass intel_guc struct parameter to intel_guc_suspend/resume
2017-10-11 8:54 ` [PATCH v13 06/21] drm/i915/guc: Pass intel_guc struct parameter to intel_guc_suspend/resume Sagar Arun Kamble
@ 2017-10-11 15:50 ` Michal Wajdeczko
2017-10-12 6:18 ` Sagar Arun Kamble
0 siblings, 1 reply; 50+ messages in thread
From: Michal Wajdeczko @ 2017-10-11 15:50 UTC (permalink / raw)
To: intel-gfx, Sagar Arun Kamble
On Wed, 11 Oct 2017 10:54:01 +0200, Sagar Arun Kamble
<sagar.a.kamble@intel.com> wrote:
> intel_guc_suspend and intel_guc_resume are GuC specific functions hence
> update the parameter from dev_priv to intel_guc struct. While at it do
> s/dev_priv/i915 in suspend/resume functions.
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 6 +++---
> drivers/gpu/drm/i915/i915_gem.c | 2 +-
> drivers/gpu/drm/i915/intel_guc.c | 20 ++++++++++----------
> drivers/gpu/drm/i915/intel_guc.h | 4 ++--
> 4 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> index 8e25846..a500a81 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1710,7 +1710,7 @@ static int i915_drm_resume(struct drm_device *dev)
> }
> mutex_unlock(&dev->struct_mutex);
> - intel_guc_resume(dev_priv);
> + intel_guc_resume(&dev_priv->guc);
> intel_modeset_init_hw(dev);
> @@ -2518,7 +2518,7 @@ static int intel_runtime_suspend(struct device
> *kdev)
> */
> i915_gem_runtime_suspend(dev_priv);
> - intel_guc_suspend(dev_priv);
> + intel_guc_suspend(&dev_priv->guc);
> intel_runtime_pm_disable_interrupts(dev_priv);
> @@ -2603,7 +2603,7 @@ static int intel_runtime_resume(struct device
> *kdev)
> if (intel_uncore_unclaimed_mmio(dev_priv))
> DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n");
> - intel_guc_resume(dev_priv);
> + intel_guc_resume(&dev_priv->guc);
> if (IS_GEN9_LP(dev_priv)) {
> bxt_disable_dc9(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c
> index e829e8c..276166d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4645,7 +4645,7 @@ int i915_gem_suspend(struct drm_i915_private
> *dev_priv)
> i915_gem_contexts_lost(dev_priv);
> mutex_unlock(&dev->struct_mutex);
> - intel_guc_suspend(dev_priv);
> + intel_guc_suspend(&dev_priv->guc);
> cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
> cancel_delayed_work_sync(&dev_priv->gt.retire_work);
> diff --git a/drivers/gpu/drm/i915/intel_guc.c
> b/drivers/gpu/drm/i915/intel_guc.c
> index bbe4c32..9a2df69 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -169,20 +169,20 @@ int intel_guc_auth_huc(struct intel_guc *guc, u32
> rsa_offset)
> /**
> * intel_guc_suspend() - notify GuC entering suspend state
> - * @dev_priv: i915 device private
> + * @guc: intel_guc struct
> */
> -int intel_guc_suspend(struct drm_i915_private *dev_priv)
> +int intel_guc_suspend(struct intel_guc *guc)
> {
> - struct intel_guc *guc = &dev_priv->guc;
> + struct drm_i915_private *i915 = guc_to_i915(guc);
> struct i915_gem_context *ctx;
> u32 data[3];
> if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
> return 0;
> - gen9_disable_guc_interrupts(dev_priv);
> + gen9_disable_guc_interrupts(i915);
> - ctx = dev_priv->kernel_context;
> + ctx = i915->kernel_context;
> data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
> /* any value greater than GUC_POWER_D0 */
> @@ -196,11 +196,11 @@ int intel_guc_suspend(struct drm_i915_private
> *dev_priv)
> /**
> * intel_guc_resume() - notify GuC resuming from suspend state
> - * @dev_priv: i915 device private
> + * @guc: intel_guc struct
> */
> -int intel_guc_resume(struct drm_i915_private *dev_priv)
> +int intel_guc_resume(struct intel_guc *guc)
> {
> - struct intel_guc *guc = &dev_priv->guc;
> + struct drm_i915_private *i915 = guc_to_i915(guc);
> struct i915_gem_context *ctx;
> u32 data[3];
> @@ -208,9 +208,9 @@ int intel_guc_resume(struct drm_i915_private
> *dev_priv)
> return 0;
> if (i915_modparams.guc_log_level >= 0)
> - gen9_enable_guc_interrupts(dev_priv);
> + gen9_enable_guc_interrupts(i915);
> - ctx = dev_priv->kernel_context;
> + ctx = i915->kernel_context;
> data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
> data[1] = GUC_POWER_D0;
> diff --git a/drivers/gpu/drm/i915/intel_guc.h
> b/drivers/gpu/drm/i915/intel_guc.h
> index 5046264f..a587210 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -113,8 +113,8 @@ static inline bool
> i915_guc_submission_enabled(struct intel_guc *guc)
> int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32
> len);
> int intel_guc_sample_forcewake(struct intel_guc *guc);
> int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
> -int intel_guc_suspend(struct drm_i915_private *dev_priv);
> -int intel_guc_resume(struct drm_i915_private *dev_priv);
> +int intel_guc_suspend(struct intel_guc *guc);
> +int intel_guc_resume(struct intel_guc *guc);
> struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32
> size);
> int intel_guc_select_fw(struct intel_guc *guc);
I'm afraid some may prefer to do s/dev_priv/i915 in separate step,
but anyway
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v13 06/21] drm/i915/guc: Pass intel_guc struct parameter to intel_guc_suspend/resume
2017-10-11 15:50 ` Michal Wajdeczko
@ 2017-10-12 6:18 ` Sagar Arun Kamble
0 siblings, 0 replies; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-12 6:18 UTC (permalink / raw)
To: Michal Wajdeczko, intel-gfx
On 10/11/2017 9:20 PM, Michal Wajdeczko wrote:
> On Wed, 11 Oct 2017 10:54:01 +0200, Sagar Arun Kamble
> <sagar.a.kamble@intel.com> wrote:
>
>> intel_guc_suspend and intel_guc_resume are GuC specific functions hence
>> update the parameter from dev_priv to intel_guc struct. While at it do
>> s/dev_priv/i915 in suspend/resume functions.
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_drv.c | 6 +++---
>> drivers/gpu/drm/i915/i915_gem.c | 2 +-
>> drivers/gpu/drm/i915/intel_guc.c | 20 ++++++++++----------
>> drivers/gpu/drm/i915/intel_guc.h | 4 ++--
>> 4 files changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index 8e25846..a500a81 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1710,7 +1710,7 @@ static int i915_drm_resume(struct drm_device *dev)
>> }
>> mutex_unlock(&dev->struct_mutex);
>> - intel_guc_resume(dev_priv);
>> + intel_guc_resume(&dev_priv->guc);
>> intel_modeset_init_hw(dev);
>> @@ -2518,7 +2518,7 @@ static int intel_runtime_suspend(struct device
>> *kdev)
>> */
>> i915_gem_runtime_suspend(dev_priv);
>> - intel_guc_suspend(dev_priv);
>> + intel_guc_suspend(&dev_priv->guc);
>> intel_runtime_pm_disable_interrupts(dev_priv);
>> @@ -2603,7 +2603,7 @@ static int intel_runtime_resume(struct device
>> *kdev)
>> if (intel_uncore_unclaimed_mmio(dev_priv))
>> DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n");
>> - intel_guc_resume(dev_priv);
>> + intel_guc_resume(&dev_priv->guc);
>> if (IS_GEN9_LP(dev_priv)) {
>> bxt_disable_dc9(dev_priv);
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index e829e8c..276166d 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -4645,7 +4645,7 @@ int i915_gem_suspend(struct drm_i915_private
>> *dev_priv)
>> i915_gem_contexts_lost(dev_priv);
>> mutex_unlock(&dev->struct_mutex);
>> - intel_guc_suspend(dev_priv);
>> + intel_guc_suspend(&dev_priv->guc);
>> cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
>> cancel_delayed_work_sync(&dev_priv->gt.retire_work);
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index bbe4c32..9a2df69 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -169,20 +169,20 @@ int intel_guc_auth_huc(struct intel_guc *guc,
>> u32 rsa_offset)
>> /**
>> * intel_guc_suspend() - notify GuC entering suspend state
>> - * @dev_priv: i915 device private
>> + * @guc: intel_guc struct
>> */
>> -int intel_guc_suspend(struct drm_i915_private *dev_priv)
>> +int intel_guc_suspend(struct intel_guc *guc)
>> {
>> - struct intel_guc *guc = &dev_priv->guc;
>> + struct drm_i915_private *i915 = guc_to_i915(guc);
>> struct i915_gem_context *ctx;
>> u32 data[3];
>> if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>> return 0;
>> - gen9_disable_guc_interrupts(dev_priv);
>> + gen9_disable_guc_interrupts(i915);
>> - ctx = dev_priv->kernel_context;
>> + ctx = i915->kernel_context;
>> data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
>> /* any value greater than GUC_POWER_D0 */
>> @@ -196,11 +196,11 @@ int intel_guc_suspend(struct drm_i915_private
>> *dev_priv)
>> /**
>> * intel_guc_resume() - notify GuC resuming from suspend state
>> - * @dev_priv: i915 device private
>> + * @guc: intel_guc struct
>> */
>> -int intel_guc_resume(struct drm_i915_private *dev_priv)
>> +int intel_guc_resume(struct intel_guc *guc)
>> {
>> - struct intel_guc *guc = &dev_priv->guc;
>> + struct drm_i915_private *i915 = guc_to_i915(guc);
>> struct i915_gem_context *ctx;
>> u32 data[3];
>> @@ -208,9 +208,9 @@ int intel_guc_resume(struct drm_i915_private
>> *dev_priv)
>> return 0;
>> if (i915_modparams.guc_log_level >= 0)
>> - gen9_enable_guc_interrupts(dev_priv);
>> + gen9_enable_guc_interrupts(i915);
>> - ctx = dev_priv->kernel_context;
>> + ctx = i915->kernel_context;
>> data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
>> data[1] = GUC_POWER_D0;
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index 5046264f..a587210 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -113,8 +113,8 @@ static inline bool
>> i915_guc_submission_enabled(struct intel_guc *guc)
>> int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action,
>> u32 len);
>> int intel_guc_sample_forcewake(struct intel_guc *guc);
>> int intel_guc_auth_huc(struct intel_guc *guc, u32 rsa_offset);
>> -int intel_guc_suspend(struct drm_i915_private *dev_priv);
>> -int intel_guc_resume(struct drm_i915_private *dev_priv);
>> +int intel_guc_suspend(struct intel_guc *guc);
>> +int intel_guc_resume(struct intel_guc *guc);
>> struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32
>> size);
>> int intel_guc_select_fw(struct intel_guc *guc);
>
> I'm afraid some may prefer to do s/dev_priv/i915 in separate step,
> but anyway
>
> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Thanks Michal. Will remove the substitution. Can be taken up later.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v13 07/21] drm/i915: Create GEM runtime resume helper and handle GEM runtime suspend error
2017-10-11 8:53 [PATCH v13 00/21] drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring Sagar Arun Kamble
` (5 preceding siblings ...)
2017-10-11 8:54 ` [PATCH v13 06/21] drm/i915/guc: Pass intel_guc struct parameter to intel_guc_suspend/resume Sagar Arun Kamble
@ 2017-10-11 8:54 ` Sagar Arun Kamble
2017-10-11 8:54 ` [PATCH v13 08/21] drm/i915/guc: Update GEM suspend/resume flows with GuC suspend/resume functions Sagar Arun Kamble
` (14 subsequent siblings)
21 siblings, 0 replies; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-11 8:54 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni, Rodrigo Vivi
These changes are preparation to handle GuC suspend/resume. Prepared
helper i915_gem_runtime_resume to reinitialize suspended gem setup.
Returning status from i915_gem_runtime_suspend.
This will be placeholder for handling any errors from uC suspend/resume
in upcoming patches. Restructured the suspend/resume routines w.r.t setup
creation and rollback order.
This also fixes issue of ordering of i915_gem_runtime_resume with
intel_runtime_pm_enable_interrupts.
v2: Fixed return from intel_runtime_resume. (Michał Winiarski)
v3: Not returning status from gem_runtime_resume. (Chris)
v4: Refined return from i915_gem_runtime_suspend. (Michal Wajdeczko)
v5: Updated jump labels. (Chris, Joonas) Removed invalid comments about
gem_runtime_resume and removed updates to i915_gem_resume. (Chris)
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_drv.c | 28 +++++++++++++++-------------
drivers/gpu/drm/i915/i915_drv.h | 3 ++-
drivers/gpu/drm/i915/i915_gem.c | 10 +++++++++-
3 files changed, 26 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a500a81..7b043d7 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2516,7 +2516,9 @@ static int intel_runtime_suspend(struct device *kdev)
* We are safe here against re-faults, since the fault handler takes
* an RPM reference.
*/
- i915_gem_runtime_suspend(dev_priv);
+ ret = i915_gem_runtime_suspend(dev_priv);
+ if (ret)
+ goto err_gem;
intel_guc_suspend(&dev_priv->guc);
@@ -2534,11 +2536,7 @@ static int intel_runtime_suspend(struct device *kdev)
if (ret) {
DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
- intel_runtime_pm_enable_interrupts(dev_priv);
-
- enable_rpm_wakeref_asserts(dev_priv);
-
- return ret;
+ goto err_disable;
}
intel_uncore_suspend(dev_priv);
@@ -2581,6 +2579,15 @@ static int intel_runtime_suspend(struct device *kdev)
DRM_DEBUG_KMS("Device suspended\n");
return 0;
+
+err_disable:
+ intel_runtime_pm_enable_interrupts(dev_priv);
+ intel_guc_resume(&dev_priv->guc);
+ i915_gem_runtime_resume(dev_priv);
+
+err_gem:
+ enable_rpm_wakeref_asserts(dev_priv);
+ return ret;
}
static int intel_runtime_resume(struct device *kdev)
@@ -2617,13 +2624,6 @@ static int intel_runtime_resume(struct device *kdev)
ret = vlv_resume_prepare(dev_priv, true);
}
- /*
- * No point of rolling back things in case of an error, as the best
- * we can do is to hope that things will still work (and disable RPM).
- */
- i915_gem_init_swizzling(dev_priv);
- i915_gem_restore_fences(dev_priv);
-
intel_runtime_pm_enable_interrupts(dev_priv);
/*
@@ -2636,6 +2636,8 @@ static int intel_runtime_resume(struct device *kdev)
intel_enable_ipc(dev_priv);
+ i915_gem_runtime_resume(dev_priv);
+
enable_rpm_wakeref_asserts(dev_priv);
if (ret)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 770305b..bc21da2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3512,7 +3512,8 @@ struct i915_vma * __must_check
int i915_gem_object_unbind(struct drm_i915_gem_object *obj);
void i915_gem_release_mmap(struct drm_i915_gem_object *obj);
-void i915_gem_runtime_suspend(struct drm_i915_private *dev_priv);
+int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv);
+void i915_gem_runtime_resume(struct drm_i915_private *dev_priv);
static inline int __sg_page_count(const struct scatterlist *sg)
{
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 276166d..facf681 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2053,7 +2053,7 @@ static void __i915_gem_object_release_mmap(struct drm_i915_gem_object *obj)
intel_runtime_pm_put(i915);
}
-void i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
+int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
{
struct drm_i915_gem_object *obj, *on;
int i;
@@ -2093,6 +2093,14 @@ void i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
GEM_BUG_ON(i915_vma_has_userfault(reg->vma));
reg->dirty = true;
}
+
+ return 0;
+}
+
+void i915_gem_runtime_resume(struct drm_i915_private *dev_priv)
+{
+ i915_gem_init_swizzling(dev_priv);
+ i915_gem_restore_fences(dev_priv);
}
static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj)
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 50+ messages in thread* [PATCH v13 08/21] drm/i915/guc: Update GEM suspend/resume flows with GuC suspend/resume functions
2017-10-11 8:53 [PATCH v13 00/21] drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring Sagar Arun Kamble
` (6 preceding siblings ...)
2017-10-11 8:54 ` [PATCH v13 07/21] drm/i915: Create GEM runtime resume helper and handle GEM runtime suspend error Sagar Arun Kamble
@ 2017-10-11 8:54 ` Sagar Arun Kamble
2017-10-11 8:54 ` [PATCH v13 09/21] drm/i915/uc: Create uC suspend and resume functions Sagar Arun Kamble
` (13 subsequent siblings)
21 siblings, 0 replies; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-11 8:54 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni, Rodrigo Vivi
This patch moves GuC suspend/resume handlers to corresponding GEM handlers
and orders them properly in the runtime and system suspend/resume flows.
Also ensure all GEM suspend/resume paths are protected by struct_mutex.
v2: Removed documentation of suspend/resume handlers as those are not
interfaces and are just hooks. (Chris)
v3: Rebase. Removed i915_gem_restore_fences change from this patch.
(Michal Wajdeczko)
v4: Rebase. Added struct_mutex protection to i915_gem_runtime_suspend,
i915_gem_runtime_resume and intel_guc_suspend in i915_gem_suspend. Pulled
intel_guc_resume under struct_mutex in i915_gem_resume.
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
---
drivers/gpu/drm/i915/i915_drv.c | 7 -------
drivers/gpu/drm/i915/i915_gem.c | 20 ++++++++++++++++++--
2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 7b043d7..df9a561 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1710,8 +1710,6 @@ static int i915_drm_resume(struct drm_device *dev)
}
mutex_unlock(&dev->struct_mutex);
- intel_guc_resume(&dev_priv->guc);
-
intel_modeset_init_hw(dev);
spin_lock_irq(&dev_priv->irq_lock);
@@ -2520,8 +2518,6 @@ static int intel_runtime_suspend(struct device *kdev)
if (ret)
goto err_gem;
- intel_guc_suspend(&dev_priv->guc);
-
intel_runtime_pm_disable_interrupts(dev_priv);
ret = 0;
@@ -2582,7 +2578,6 @@ static int intel_runtime_suspend(struct device *kdev)
err_disable:
intel_runtime_pm_enable_interrupts(dev_priv);
- intel_guc_resume(&dev_priv->guc);
i915_gem_runtime_resume(dev_priv);
err_gem:
@@ -2610,8 +2605,6 @@ static int intel_runtime_resume(struct device *kdev)
if (intel_uncore_unclaimed_mmio(dev_priv))
DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n");
- intel_guc_resume(&dev_priv->guc);
-
if (IS_GEN9_LP(dev_priv)) {
bxt_disable_dc9(dev_priv);
bxt_display_core_init(dev_priv, true);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index facf681..38447ae 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2058,6 +2058,10 @@ int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
struct drm_i915_gem_object *obj, *on;
int i;
+ mutex_lock(&dev_priv->drm.struct_mutex);
+
+ intel_guc_suspend(&dev_priv->guc);
+
/*
* Only called during RPM suspend. All users of the userfault_list
* must be holding an RPM wakeref to ensure that this can not
@@ -2094,13 +2098,21 @@ int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
reg->dirty = true;
}
+ mutex_unlock(&dev_priv->drm.struct_mutex);
+
return 0;
}
void i915_gem_runtime_resume(struct drm_i915_private *dev_priv)
{
+ mutex_lock(&dev_priv->drm.struct_mutex);
+
i915_gem_init_swizzling(dev_priv);
i915_gem_restore_fences(dev_priv);
+
+ intel_guc_resume(&dev_priv->guc);
+
+ mutex_unlock(&dev_priv->drm.struct_mutex);
}
static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj)
@@ -4653,8 +4665,6 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
i915_gem_contexts_lost(dev_priv);
mutex_unlock(&dev->struct_mutex);
- intel_guc_suspend(&dev_priv->guc);
-
cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
cancel_delayed_work_sync(&dev_priv->gt.retire_work);
@@ -4670,6 +4680,10 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
if (WARN_ON(!intel_engines_are_idle(dev_priv)))
i915_gem_set_wedged(dev_priv); /* no hope, discard everything */
+ mutex_lock(&dev->struct_mutex);
+ intel_guc_suspend(&dev_priv->guc);
+ mutex_unlock(&dev->struct_mutex);
+
/*
* Neither the BIOS, ourselves or any other kernel
* expects the system to be in execlists mode on startup,
@@ -4716,6 +4730,8 @@ void i915_gem_resume(struct drm_i915_private *dev_priv)
*/
dev_priv->gt.resume(dev_priv);
+ intel_guc_resume(&dev_priv->guc);
+
mutex_unlock(&dev->struct_mutex);
}
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 50+ messages in thread* [PATCH v13 09/21] drm/i915/uc: Create uC suspend and resume functions
2017-10-11 8:53 [PATCH v13 00/21] drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring Sagar Arun Kamble
` (7 preceding siblings ...)
2017-10-11 8:54 ` [PATCH v13 08/21] drm/i915/guc: Update GEM suspend/resume flows with GuC suspend/resume functions Sagar Arun Kamble
@ 2017-10-11 8:54 ` Sagar Arun Kamble
2017-10-11 15:57 ` Michal Wajdeczko
2017-10-11 8:54 ` [PATCH v13 10/21] drm/i915/guc: Update uC suspend/resume function separating Host/GuC tasks Sagar Arun Kamble
` (12 subsequent siblings)
21 siblings, 1 reply; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-11 8:54 UTC (permalink / raw)
To: intel-gfx
Prepared generic helpers intel_uc_suspend, intel_uc_resume. These are
called from respective GEM functions. Only exception is intel_uc_resume
that needs to be called w/ or w/o GuC loaded in i915_drm_resume path.
Changes to add WOPCM condition check to load GuC during resume will be
added in later patches.
v2: Rebase w.r.t removal of GuC code restructuring.
v3: Calling intel_uc_resume from i915_gem_resume post resuming
i915 gem setup. This is symmetrical with i915_gem_suspend.
Removed error messages from i915 suspend/resume routines as
uC suspend/resume routines will have those. (Michal Wajdeczko)
Declare wedged on uc_suspend failure and uc_resume failure.
(Michał Winiarski)
Keeping the uC suspend/resume function definitions close to other
uC functions.
v4: Added implementation to intel_uc_resume as GuC resume is
needed to be triggered post reloading the firmware as well. Added
comments about semantics of GuC resume with the firmware reload.
v5: Updated return from i915_gem_runtime_suspend. Moved the comment
about GuC reload optimization to intel_uc_init_hw. (Michal Wajdeczko)
Updated comments as FIXME.
v6: Kept error handling for failure from i915_gem_runtime_suspend only.
We don't want GEM/GuC resume failure to impact intel_runtime_resume or
i915_drm_resume. GEM suspend failure along i915_drm_suspend can also
be ignored as we reset GPU post that. Updated comments. (Chris, Joonas)
v7: Removed intel_uc_resume from i915_drm_resume as it will be done as
part of intel_uc_init_hw in further patches. Removed TODO comments about
handling GuC load skip on resume. This is to be addressed in further
patches. Added error return from intel_uc_suspend as we plan to add
functionality to resume submission in case of suspend failure in further
patches. Removed runtime uC suspend/resume functions as functionality
will be similar in both paths.
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: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_gem.c | 17 +++++++++++------
drivers/gpu/drm/i915/intel_uc.c | 16 ++++++++++++++++
drivers/gpu/drm/i915/intel_uc.h | 2 ++
3 files changed, 29 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 38447ae..7d1b7e1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2056,11 +2056,13 @@ static void __i915_gem_object_release_mmap(struct drm_i915_gem_object *obj)
int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
{
struct drm_i915_gem_object *obj, *on;
- int i;
+ int i, ret;
mutex_lock(&dev_priv->drm.struct_mutex);
- intel_guc_suspend(&dev_priv->guc);
+ ret = intel_uc_suspend(dev_priv);
+ if (ret)
+ goto out_unlock;
/*
* Only called during RPM suspend. All users of the userfault_list
@@ -2098,9 +2100,10 @@ int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
reg->dirty = true;
}
+out_unlock:
mutex_unlock(&dev_priv->drm.struct_mutex);
- return 0;
+ return ret;
}
void i915_gem_runtime_resume(struct drm_i915_private *dev_priv)
@@ -2110,7 +2113,7 @@ void i915_gem_runtime_resume(struct drm_i915_private *dev_priv)
i915_gem_init_swizzling(dev_priv);
i915_gem_restore_fences(dev_priv);
- intel_guc_resume(&dev_priv->guc);
+ intel_uc_resume(dev_priv);
mutex_unlock(&dev_priv->drm.struct_mutex);
}
@@ -4681,7 +4684,9 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
i915_gem_set_wedged(dev_priv); /* no hope, discard everything */
mutex_lock(&dev->struct_mutex);
- intel_guc_suspend(&dev_priv->guc);
+ ret = intel_uc_suspend(dev_priv);
+ if (ret)
+ goto err_unlock;
mutex_unlock(&dev->struct_mutex);
/*
@@ -4730,7 +4735,7 @@ void i915_gem_resume(struct drm_i915_private *dev_priv)
*/
dev_priv->gt.resume(dev_priv);
- intel_guc_resume(&dev_priv->guc);
+ intel_uc_resume(dev_priv);
mutex_unlock(&dev->struct_mutex);
}
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 7305486..b5c132c 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -283,3 +283,19 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
if (i915_modparams.enable_guc_loading)
i915_ggtt_disable_guc(dev_priv);
}
+
+int intel_uc_suspend(struct drm_i915_private *dev_priv)
+{
+ int ret;
+
+ ret = intel_guc_suspend(&dev_priv->guc);
+ if (ret)
+ DRM_ERROR("Failed to suspend GuC\n");
+
+ return ret;
+}
+
+void intel_uc_resume(struct drm_i915_private *dev_priv)
+{
+ intel_guc_resume(&dev_priv->guc);
+}
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index e18d3bb..e20bc72 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -34,5 +34,7 @@
void intel_uc_fini_fw(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_suspend(struct drm_i915_private *dev_priv);
+void intel_uc_resume(struct drm_i915_private *dev_priv);
#endif
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH v13 09/21] drm/i915/uc: Create uC suspend and resume functions
2017-10-11 8:54 ` [PATCH v13 09/21] drm/i915/uc: Create uC suspend and resume functions Sagar Arun Kamble
@ 2017-10-11 15:57 ` Michal Wajdeczko
2017-10-12 6:25 ` Sagar Arun Kamble
0 siblings, 1 reply; 50+ messages in thread
From: Michal Wajdeczko @ 2017-10-11 15:57 UTC (permalink / raw)
To: intel-gfx, Sagar Arun Kamble
On Wed, 11 Oct 2017 10:54:04 +0200, Sagar Arun Kamble
<sagar.a.kamble@intel.com> wrote:
> Prepared generic helpers intel_uc_suspend, intel_uc_resume. These are
> called from respective GEM functions. Only exception is intel_uc_resume
> that needs to be called w/ or w/o GuC loaded in i915_drm_resume path.
> Changes to add WOPCM condition check to load GuC during resume will be
> added in later patches.
>
> v2: Rebase w.r.t removal of GuC code restructuring.
>
> v3: Calling intel_uc_resume from i915_gem_resume post resuming
> i915 gem setup. This is symmetrical with i915_gem_suspend.
> Removed error messages from i915 suspend/resume routines as
> uC suspend/resume routines will have those. (Michal Wajdeczko)
> Declare wedged on uc_suspend failure and uc_resume failure.
> (Michał Winiarski)
> Keeping the uC suspend/resume function definitions close to other
> uC functions.
>
> v4: Added implementation to intel_uc_resume as GuC resume is
> needed to be triggered post reloading the firmware as well. Added
> comments about semantics of GuC resume with the firmware reload.
>
> v5: Updated return from i915_gem_runtime_suspend. Moved the comment
> about GuC reload optimization to intel_uc_init_hw. (Michal Wajdeczko)
> Updated comments as FIXME.
>
> v6: Kept error handling for failure from i915_gem_runtime_suspend only.
> We don't want GEM/GuC resume failure to impact intel_runtime_resume or
> i915_drm_resume. GEM suspend failure along i915_drm_suspend can also
> be ignored as we reset GPU post that. Updated comments. (Chris, Joonas)
>
> v7: Removed intel_uc_resume from i915_drm_resume as it will be done as
> part of intel_uc_init_hw in further patches. Removed TODO comments about
> handling GuC load skip on resume. This is to be addressed in further
> patches. Added error return from intel_uc_suspend as we plan to add
> functionality to resume submission in case of suspend failure in further
> patches. Removed runtime uC suspend/resume functions as functionality
> will be similar in both paths.
>
> 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: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 17 +++++++++++------
> drivers/gpu/drm/i915/intel_uc.c | 16 ++++++++++++++++
> drivers/gpu/drm/i915/intel_uc.h | 2 ++
> 3 files changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c
> index 38447ae..7d1b7e1 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2056,11 +2056,13 @@ static void
> __i915_gem_object_release_mmap(struct drm_i915_gem_object *obj)
> int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
> {
> struct drm_i915_gem_object *obj, *on;
> - int i;
> + int i, ret;
> mutex_lock(&dev_priv->drm.struct_mutex);
> - intel_guc_suspend(&dev_priv->guc);
> + ret = intel_uc_suspend(dev_priv);
> + if (ret)
> + goto out_unlock;
> /*
> * Only called during RPM suspend. All users of the userfault_list
> @@ -2098,9 +2100,10 @@ int i915_gem_runtime_suspend(struct
> drm_i915_private *dev_priv)
> reg->dirty = true;
> }
> +out_unlock:
> mutex_unlock(&dev_priv->drm.struct_mutex);
> - return 0;
> + return ret;
> }
> void i915_gem_runtime_resume(struct drm_i915_private *dev_priv)
> @@ -2110,7 +2113,7 @@ void i915_gem_runtime_resume(struct
> drm_i915_private *dev_priv)
> i915_gem_init_swizzling(dev_priv);
> i915_gem_restore_fences(dev_priv);
> - intel_guc_resume(&dev_priv->guc);
> + intel_uc_resume(dev_priv);
> mutex_unlock(&dev_priv->drm.struct_mutex);
> }
> @@ -4681,7 +4684,9 @@ int i915_gem_suspend(struct drm_i915_private
> *dev_priv)
> i915_gem_set_wedged(dev_priv); /* no hope, discard everything */
> mutex_lock(&dev->struct_mutex);
> - intel_guc_suspend(&dev_priv->guc);
> + ret = intel_uc_suspend(dev_priv);
> + if (ret)
> + goto err_unlock;
> mutex_unlock(&dev->struct_mutex);
> /*
> @@ -4730,7 +4735,7 @@ void i915_gem_resume(struct drm_i915_private
> *dev_priv)
> */
> dev_priv->gt.resume(dev_priv);
> - intel_guc_resume(&dev_priv->guc);
> + intel_uc_resume(dev_priv);
> mutex_unlock(&dev->struct_mutex);
> }
> diff --git a/drivers/gpu/drm/i915/intel_uc.c
> b/drivers/gpu/drm/i915/intel_uc.c
> index 7305486..b5c132c 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -283,3 +283,19 @@ void intel_uc_fini_hw(struct drm_i915_private
> *dev_priv)
> if (i915_modparams.enable_guc_loading)
> i915_ggtt_disable_guc(dev_priv);
> }
> +
> +int intel_uc_suspend(struct drm_i915_private *dev_priv)
> +{
> + int ret;
> +
> + ret = intel_guc_suspend(&dev_priv->guc);
> + if (ret)
> + DRM_ERROR("Failed to suspend GuC\n");
> +
> + return ret;
> +}
> +
> +void intel_uc_resume(struct drm_i915_private *dev_priv)
> +{
> + intel_guc_resume(&dev_priv->guc);
> +}
Can we add kerneldoc for above new functions?
With doc added
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> diff --git a/drivers/gpu/drm/i915/intel_uc.h
> b/drivers/gpu/drm/i915/intel_uc.h
> index e18d3bb..e20bc72 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -34,5 +34,7 @@
> void intel_uc_fini_fw(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_suspend(struct drm_i915_private *dev_priv);
> +void intel_uc_resume(struct drm_i915_private *dev_priv);
> #endif
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v13 09/21] drm/i915/uc: Create uC suspend and resume functions
2017-10-11 15:57 ` Michal Wajdeczko
@ 2017-10-12 6:25 ` Sagar Arun Kamble
0 siblings, 0 replies; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-12 6:25 UTC (permalink / raw)
To: Michal Wajdeczko, intel-gfx
On 10/11/2017 9:27 PM, Michal Wajdeczko wrote:
> On Wed, 11 Oct 2017 10:54:04 +0200, Sagar Arun Kamble
> <sagar.a.kamble@intel.com> wrote:
>
>> Prepared generic helpers intel_uc_suspend, intel_uc_resume. These are
>> called from respective GEM functions. Only exception is intel_uc_resume
>> that needs to be called w/ or w/o GuC loaded in i915_drm_resume path.
>> Changes to add WOPCM condition check to load GuC during resume will be
>> added in later patches.
>>
>> v2: Rebase w.r.t removal of GuC code restructuring.
>>
>> v3: Calling intel_uc_resume from i915_gem_resume post resuming
>> i915 gem setup. This is symmetrical with i915_gem_suspend.
>> Removed error messages from i915 suspend/resume routines as
>> uC suspend/resume routines will have those. (Michal Wajdeczko)
>> Declare wedged on uc_suspend failure and uc_resume failure.
>> (Michał Winiarski)
>> Keeping the uC suspend/resume function definitions close to other
>> uC functions.
>>
>> v4: Added implementation to intel_uc_resume as GuC resume is
>> needed to be triggered post reloading the firmware as well. Added
>> comments about semantics of GuC resume with the firmware reload.
>>
>> v5: Updated return from i915_gem_runtime_suspend. Moved the comment
>> about GuC reload optimization to intel_uc_init_hw. (Michal Wajdeczko)
>> Updated comments as FIXME.
>>
>> v6: Kept error handling for failure from i915_gem_runtime_suspend only.
>> We don't want GEM/GuC resume failure to impact intel_runtime_resume or
>> i915_drm_resume. GEM suspend failure along i915_drm_suspend can also
>> be ignored as we reset GPU post that. Updated comments. (Chris, Joonas)
>>
>> v7: Removed intel_uc_resume from i915_drm_resume as it will be done as
>> part of intel_uc_init_hw in further patches. Removed TODO comments about
>> handling GuC load skip on resume. This is to be addressed in further
>> patches. Added error return from intel_uc_suspend as we plan to add
>> functionality to resume submission in case of suspend failure in further
>> patches. Removed runtime uC suspend/resume functions as functionality
>> will be similar in both paths.
>>
>> 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: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_gem.c | 17 +++++++++++------
>> drivers/gpu/drm/i915/intel_uc.c | 16 ++++++++++++++++
>> drivers/gpu/drm/i915/intel_uc.h | 2 ++
>> 3 files changed, 29 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index 38447ae..7d1b7e1 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2056,11 +2056,13 @@ static void
>> __i915_gem_object_release_mmap(struct drm_i915_gem_object *obj)
>> int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
>> {
>> struct drm_i915_gem_object *obj, *on;
>> - int i;
>> + int i, ret;
>> mutex_lock(&dev_priv->drm.struct_mutex);
>> - intel_guc_suspend(&dev_priv->guc);
>> + ret = intel_uc_suspend(dev_priv);
>> + if (ret)
>> + goto out_unlock;
>> /*
>> * Only called during RPM suspend. All users of the userfault_list
>> @@ -2098,9 +2100,10 @@ int i915_gem_runtime_suspend(struct
>> drm_i915_private *dev_priv)
>> reg->dirty = true;
>> }
>> +out_unlock:
>> mutex_unlock(&dev_priv->drm.struct_mutex);
>> - return 0;
>> + return ret;
>> }
>> void i915_gem_runtime_resume(struct drm_i915_private *dev_priv)
>> @@ -2110,7 +2113,7 @@ void i915_gem_runtime_resume(struct
>> drm_i915_private *dev_priv)
>> i915_gem_init_swizzling(dev_priv);
>> i915_gem_restore_fences(dev_priv);
>> - intel_guc_resume(&dev_priv->guc);
>> + intel_uc_resume(dev_priv);
>> mutex_unlock(&dev_priv->drm.struct_mutex);
>> }
>> @@ -4681,7 +4684,9 @@ int i915_gem_suspend(struct drm_i915_private
>> *dev_priv)
>> i915_gem_set_wedged(dev_priv); /* no hope, discard
>> everything */
>> mutex_lock(&dev->struct_mutex);
>> - intel_guc_suspend(&dev_priv->guc);
>> + ret = intel_uc_suspend(dev_priv);
>> + if (ret)
>> + goto err_unlock;
>> mutex_unlock(&dev->struct_mutex);
>> /*
>> @@ -4730,7 +4735,7 @@ void i915_gem_resume(struct drm_i915_private
>> *dev_priv)
>> */
>> dev_priv->gt.resume(dev_priv);
>> - intel_guc_resume(&dev_priv->guc);
>> + intel_uc_resume(dev_priv);
>> mutex_unlock(&dev->struct_mutex);
>> }
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 7305486..b5c132c 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -283,3 +283,19 @@ void intel_uc_fini_hw(struct drm_i915_private
>> *dev_priv)
>> if (i915_modparams.enable_guc_loading)
>> i915_ggtt_disable_guc(dev_priv);
>> }
>> +
>> +int intel_uc_suspend(struct drm_i915_private *dev_priv)
>> +{
>> + int ret;
>> +
>> + ret = intel_guc_suspend(&dev_priv->guc);
>> + if (ret)
>> + DRM_ERROR("Failed to suspend GuC\n");
>> +
>> + return ret;
>> +}
>> +
>> +void intel_uc_resume(struct drm_i915_private *dev_priv)
>> +{
>> + intel_guc_resume(&dev_priv->guc);
>> +}
>
> Can we add kerneldoc for above new functions?
> With doc added
Since full flow was established in the later patches hence deferred doc
addition.
Will add partially here. Thanks.
>
> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>
>> diff --git a/drivers/gpu/drm/i915/intel_uc.h
>> b/drivers/gpu/drm/i915/intel_uc.h
>> index e18d3bb..e20bc72 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.h
>> +++ b/drivers/gpu/drm/i915/intel_uc.h
>> @@ -34,5 +34,7 @@
>> void intel_uc_fini_fw(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_suspend(struct drm_i915_private *dev_priv);
>> +void intel_uc_resume(struct drm_i915_private *dev_priv);
>> #endif
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v13 10/21] drm/i915/guc: Update uC suspend/resume function separating Host/GuC tasks
2017-10-11 8:53 [PATCH v13 00/21] drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring Sagar Arun Kamble
` (8 preceding siblings ...)
2017-10-11 8:54 ` [PATCH v13 09/21] drm/i915/uc: Create uC suspend and resume functions Sagar Arun Kamble
@ 2017-10-11 8:54 ` Sagar Arun Kamble
2017-10-11 16:19 ` Michal Wajdeczko
2017-10-11 8:54 ` [PATCH v13 11/21] drm/i915/guc: Remove GuC submission disable from i915_driver_unload Sagar Arun Kamble
` (11 subsequent siblings)
21 siblings, 1 reply; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-11 8:54 UTC (permalink / raw)
To: intel-gfx
Suspending GuC involves bunch of tasks controlled by GuC OS and some
controlled by Host OS.
Host needs to disable submission to GuC and any other GuC functions. Then,
GuC's task is initiated by Host sending action to GuC to enter sleep
state. On this action, GuC preempts engines to idle context and then saves
internal state to a buffer. It also disables internal interrupts/timers to
avoid any wake-ups.
After this, Host should disable GuC interrupts, communication with GuC
(intel_guc_send/notify). GGTT invalidate update will have to be done in
conjunction with GTT related suspend/resume tasks.
v2: Rebase w.r.t removal of GuC code restructuring.
v3: Removed GuC specific helpers as tasks other than send H2G for
sleep/resume are to be done from uc generic functions. (Michal Wajdeczko)
v4: Simplified/Unified the error messaging in uc_runtime_suspend/resume.
(Michal Wajdeczko). Rebase w.r.t i915_modparams change.
Added documentation to intel_uc_runtime_suspend/resume.
v5: Removed enable_guc_loading based check from intel_uc_runtime_suspend
and intel_uc_runtime_resume and pulled FW load_status based checks from
intel_guc_suspend/resume into these functions. (Michal Wajdeczko)
v6: Adjusted intel_uc_runtime_resume with prototype change to not return
value.
v7: Rebase.
v8: Updated commit description and added submission enable/disable in
GuC suspend/resume paths. Removed GGTT invalidate update functions.
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: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> #6
---
drivers/gpu/drm/i915/intel_guc.c | 11 -------
drivers/gpu/drm/i915/intel_uc.c | 65 +++++++++++++++++++++++++++++++++++++---
2 files changed, 61 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 9a2df69..55a0158 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -177,11 +177,6 @@ int intel_guc_suspend(struct intel_guc *guc)
struct i915_gem_context *ctx;
u32 data[3];
- if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
- return 0;
-
- gen9_disable_guc_interrupts(i915);
-
ctx = i915->kernel_context;
data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
@@ -204,12 +199,6 @@ int intel_guc_resume(struct intel_guc *guc)
struct i915_gem_context *ctx;
u32 data[3];
- if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
- return 0;
-
- if (i915_modparams.guc_log_level >= 0)
- gen9_enable_guc_interrupts(i915);
-
ctx = i915->kernel_context;
data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index b5c132c..297a321 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -284,18 +284,75 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
i915_ggtt_disable_guc(dev_priv);
}
+/**
+ * intel_uc_suspend() - Suspend uC operation.
+ * @dev_priv: i915 device private
+ *
+ * This function disables GuC submission, invokes GuC OS suspension,
+ * disables GuC interrupts and disable communication with GuC.
+ *
+ * Return: non-zero code on error
+ */
int intel_uc_suspend(struct drm_i915_private *dev_priv)
{
- int ret;
+ struct intel_guc *guc = &dev_priv->guc;
+ int ret = 0;
+
+ if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
+ goto out;
+
+ i915_guc_submission_disable(dev_priv);
- ret = intel_guc_suspend(&dev_priv->guc);
+ ret = intel_guc_suspend(guc);
if (ret)
- DRM_ERROR("Failed to suspend GuC\n");
+ goto out_suspend;
+
+ gen9_disable_guc_interrupts(dev_priv);
+ guc_disable_communication(guc);
+
+ goto out;
+
+out_suspend:
+ i915_guc_submission_enable(dev_priv);
+out:
+ if (ret)
+ DRM_ERROR("uC Suspend failed (%d)\n", ret);
return ret;
}
+/**
+ * intel_uc_resume() - Resume uC operation.
+ * @dev_priv: i915 device private
+ *
+ * This function enables communication with GuC, enables GuC interrupts,
+ * invokes GuC OS resumption and enables GuC submission.
+ */
void intel_uc_resume(struct drm_i915_private *dev_priv)
{
- intel_guc_resume(&dev_priv->guc);
+ struct intel_guc *guc = &dev_priv->guc;
+ int ret;
+
+ if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
+ return;
+
+ ret = guc_enable_communication(guc);
+ if (ret) {
+ DRM_DEBUG_DRIVER("GuC communication enable failed (%d)\n", ret);
+ return;
+ }
+
+ if (i915_modparams.guc_log_level >= 0)
+ gen9_enable_guc_interrupts(dev_priv);
+
+ ret = intel_guc_resume(guc);
+ if (ret)
+ DRM_ERROR("GuC resume failed (%d)."
+ "GuC functions may not work\n", ret);
+
+ i915_guc_submission_enable(dev_priv);
+
+ DRM_DEBUG_DRIVER("GuC submission %s\n",
+ i915_guc_submission_enabled(guc) ?
+ "enabled" : "disabled");
}
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH v13 10/21] drm/i915/guc: Update uC suspend/resume function separating Host/GuC tasks
2017-10-11 8:54 ` [PATCH v13 10/21] drm/i915/guc: Update uC suspend/resume function separating Host/GuC tasks Sagar Arun Kamble
@ 2017-10-11 16:19 ` Michal Wajdeczko
2017-10-12 6:38 ` Sagar Arun Kamble
0 siblings, 1 reply; 50+ messages in thread
From: Michal Wajdeczko @ 2017-10-11 16:19 UTC (permalink / raw)
To: intel-gfx, Sagar Arun Kamble
On Wed, 11 Oct 2017 10:54:05 +0200, Sagar Arun Kamble
<sagar.a.kamble@intel.com> wrote:
> Suspending GuC involves bunch of tasks controlled by GuC OS and some
> controlled by Host OS.
>
> Host needs to disable submission to GuC and any other GuC functions.
> Then,
> GuC's task is initiated by Host sending action to GuC to enter sleep
> state. On this action, GuC preempts engines to idle context and then
> saves
> internal state to a buffer. It also disables internal interrupts/timers
> to
> avoid any wake-ups.
> After this, Host should disable GuC interrupts, communication with GuC
> (intel_guc_send/notify). GGTT invalidate update will have to be done in
> conjunction with GTT related suspend/resume tasks.
>
> v2: Rebase w.r.t removal of GuC code restructuring.
>
> v3: Removed GuC specific helpers as tasks other than send H2G for
> sleep/resume are to be done from uc generic functions. (Michal Wajdeczko)
>
> v4: Simplified/Unified the error messaging in uc_runtime_suspend/resume.
> (Michal Wajdeczko). Rebase w.r.t i915_modparams change.
> Added documentation to intel_uc_runtime_suspend/resume.
>
> v5: Removed enable_guc_loading based check from intel_uc_runtime_suspend
> and intel_uc_runtime_resume and pulled FW load_status based checks from
> intel_guc_suspend/resume into these functions. (Michal Wajdeczko)
>
> v6: Adjusted intel_uc_runtime_resume with prototype change to not return
> value.
>
> v7: Rebase.
>
> v8: Updated commit description and added submission enable/disable in
> GuC suspend/resume paths. Removed GGTT invalidate update functions.
>
> 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: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> #6
> ---
> drivers/gpu/drm/i915/intel_guc.c | 11 -------
> drivers/gpu/drm/i915/intel_uc.c | 65
> +++++++++++++++++++++++++++++++++++++---
> 2 files changed, 61 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc.c
> b/drivers/gpu/drm/i915/intel_guc.c
> index 9a2df69..55a0158 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -177,11 +177,6 @@ int intel_guc_suspend(struct intel_guc *guc)
> struct i915_gem_context *ctx;
> u32 data[3];
> - if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
> - return 0;
> -
> - gen9_disable_guc_interrupts(i915);
> -
> ctx = i915->kernel_context;
> data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
> @@ -204,12 +199,6 @@ int intel_guc_resume(struct intel_guc *guc)
> struct i915_gem_context *ctx;
> u32 data[3];
> - if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
> - return 0;
> -
> - if (i915_modparams.guc_log_level >= 0)
> - gen9_enable_guc_interrupts(i915);
> -
> ctx = i915->kernel_context;
> data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
> diff --git a/drivers/gpu/drm/i915/intel_uc.c
> b/drivers/gpu/drm/i915/intel_uc.c
> index b5c132c..297a321 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -284,18 +284,75 @@ void intel_uc_fini_hw(struct drm_i915_private
> *dev_priv)
> i915_ggtt_disable_guc(dev_priv);
> }
> +/**
> + * intel_uc_suspend() - Suspend uC operation.
> + * @dev_priv: i915 device private
> + *
Ha! found missing kerneldoc ... maybe it can be partially moved to
previous patch ?
> + * This function disables GuC submission, invokes GuC OS suspension,
> + * disables GuC interrupts and disable communication with GuC.
> + *
> + * Return: non-zero code on error
> + */
> int intel_uc_suspend(struct drm_i915_private *dev_priv)
> {
> - int ret;
> + struct intel_guc *guc = &dev_priv->guc;
> + int ret = 0;
> +
> + if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
> + goto out;
Hmm, is it ok to report DRM_ERROR if Guc was not started/loaded ?
Return 0 seems to be still the best option here.
> +
> + i915_guc_submission_disable(dev_priv);
> - ret = intel_guc_suspend(&dev_priv->guc);
> + ret = intel_guc_suspend(guc);
> if (ret)
> - DRM_ERROR("Failed to suspend GuC\n");
> + goto out_suspend;
> +
> + gen9_disable_guc_interrupts(dev_priv);
> + guc_disable_communication(guc);
> +
> + goto out;
> +
> +out_suspend:
> + i915_guc_submission_enable(dev_priv);
> +out:
> + if (ret)
> + DRM_ERROR("uC Suspend failed (%d)\n", ret);
Unless I read wrong, we are re-enabling guc submission on failure,
so maybe error should say something like:
DRM_ERROR("Failed to suspend uC, aborting suspend\n");
> return ret;
> }
> +/**
> + * intel_uc_resume() - Resume uC operation.
> + * @dev_priv: i915 device private
> + *
> + * This function enables communication with GuC, enables GuC interrupts,
> + * invokes GuC OS resumption and enables GuC submission.
> + */
> void intel_uc_resume(struct drm_i915_private *dev_priv)
> {
> - intel_guc_resume(&dev_priv->guc);
> + struct intel_guc *guc = &dev_priv->guc;
> + int ret;
> +
> + if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
> + return;
> +
> + ret = guc_enable_communication(guc);
Hmm, not too early ? CT will try to talk with Guc.
> + if (ret) {
> + DRM_DEBUG_DRIVER("GuC communication enable failed (%d)\n", ret);
> + return;
> + }
> +
> + if (i915_modparams.guc_log_level >= 0)
> + gen9_enable_guc_interrupts(dev_priv);
> +
> + ret = intel_guc_resume(guc);
> + if (ret)
> + DRM_ERROR("GuC resume failed (%d)."
> + "GuC functions may not work\n", ret);
> +
> + i915_guc_submission_enable(dev_priv);
> +
> + DRM_DEBUG_DRIVER("GuC submission %s\n",
> + i915_guc_submission_enabled(guc) ?
> + "enabled" : "disabled");
Hmm, this message can be part of i915_guc_submission_enable
> }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v13 10/21] drm/i915/guc: Update uC suspend/resume function separating Host/GuC tasks
2017-10-11 16:19 ` Michal Wajdeczko
@ 2017-10-12 6:38 ` Sagar Arun Kamble
0 siblings, 0 replies; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-12 6:38 UTC (permalink / raw)
To: Michal Wajdeczko, intel-gfx
On 10/11/2017 9:49 PM, Michal Wajdeczko wrote:
> On Wed, 11 Oct 2017 10:54:05 +0200, Sagar Arun Kamble
> <sagar.a.kamble@intel.com> wrote:
>
>> Suspending GuC involves bunch of tasks controlled by GuC OS and some
>> controlled by Host OS.
>>
>> Host needs to disable submission to GuC and any other GuC functions.
>> Then,
>> GuC's task is initiated by Host sending action to GuC to enter sleep
>> state. On this action, GuC preempts engines to idle context and then
>> saves
>> internal state to a buffer. It also disables internal
>> interrupts/timers to
>> avoid any wake-ups.
>> After this, Host should disable GuC interrupts, communication with GuC
>> (intel_guc_send/notify). GGTT invalidate update will have to be done in
>> conjunction with GTT related suspend/resume tasks.
>>
>> v2: Rebase w.r.t removal of GuC code restructuring.
>>
>> v3: Removed GuC specific helpers as tasks other than send H2G for
>> sleep/resume are to be done from uc generic functions. (Michal
>> Wajdeczko)
>>
>> v4: Simplified/Unified the error messaging in uc_runtime_suspend/resume.
>> (Michal Wajdeczko). Rebase w.r.t i915_modparams change.
>> Added documentation to intel_uc_runtime_suspend/resume.
>>
>> v5: Removed enable_guc_loading based check from intel_uc_runtime_suspend
>> and intel_uc_runtime_resume and pulled FW load_status based checks from
>> intel_guc_suspend/resume into these functions. (Michal Wajdeczko)
>>
>> v6: Adjusted intel_uc_runtime_resume with prototype change to not return
>> value.
>>
>> v7: Rebase.
>>
>> v8: Updated commit description and added submission enable/disable in
>> GuC suspend/resume paths. Removed GGTT invalidate update functions.
>>
>> 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: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> #6
>> ---
>> drivers/gpu/drm/i915/intel_guc.c | 11 -------
>> drivers/gpu/drm/i915/intel_uc.c | 65
>> +++++++++++++++++++++++++++++++++++++---
>> 2 files changed, 61 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index 9a2df69..55a0158 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -177,11 +177,6 @@ int intel_guc_suspend(struct intel_guc *guc)
>> struct i915_gem_context *ctx;
>> u32 data[3];
>> - if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>> - return 0;
>> -
>> - gen9_disable_guc_interrupts(i915);
>> -
>> ctx = i915->kernel_context;
>> data[0] = INTEL_GUC_ACTION_ENTER_S_STATE;
>> @@ -204,12 +199,6 @@ int intel_guc_resume(struct intel_guc *guc)
>> struct i915_gem_context *ctx;
>> u32 data[3];
>> - if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>> - return 0;
>> -
>> - if (i915_modparams.guc_log_level >= 0)
>> - gen9_enable_guc_interrupts(i915);
>> -
>> ctx = i915->kernel_context;
>> data[0] = INTEL_GUC_ACTION_EXIT_S_STATE;
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index b5c132c..297a321 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -284,18 +284,75 @@ void intel_uc_fini_hw(struct drm_i915_private
>> *dev_priv)
>> i915_ggtt_disable_guc(dev_priv);
>> }
>> +/**
>> + * intel_uc_suspend() - Suspend uC operation.
>> + * @dev_priv: i915 device private
>> + *
>
> Ha! found missing kerneldoc ... maybe it can be partially moved to
> previous patch ?
Yes.
>
>> + * This function disables GuC submission, invokes GuC OS suspension,
>> + * disables GuC interrupts and disable communication with GuC.
>> + *
>> + * Return: non-zero code on error
>> + */
>> int intel_uc_suspend(struct drm_i915_private *dev_priv)
>> {
>> - int ret;
>> + struct intel_guc *guc = &dev_priv->guc;
>> + int ret = 0;
>> +
>> + if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>> + goto out;
>
> Hmm, is it ok to report DRM_ERROR if Guc was not started/loaded ?
> Return 0 seems to be still the best option here.
>
This actually handles case for platforms without GuC support and I think
we should replace this with enable_guc_loading.
and then we can do DRM_ERROR if GuC was not loaded.
>> +
>> + i915_guc_submission_disable(dev_priv);
>> - ret = intel_guc_suspend(&dev_priv->guc);
>> + ret = intel_guc_suspend(guc);
>> if (ret)
>> - DRM_ERROR("Failed to suspend GuC\n");
>> + goto out_suspend;
>> +
>> + gen9_disable_guc_interrupts(dev_priv);
>> + guc_disable_communication(guc);
>> +
>> + goto out;
>> +
>> +out_suspend:
>> + i915_guc_submission_enable(dev_priv);
>> +out:
>> + if (ret)
>> + DRM_ERROR("uC Suspend failed (%d)\n", ret);
>
> Unless I read wrong, we are re-enabling guc submission on failure,
> so maybe error should say something like:
>
> DRM_ERROR("Failed to suspend uC, aborting suspend\n");
Sure.
>
>> return ret;
>> }
>> +/**
>> + * intel_uc_resume() - Resume uC operation.
>> + * @dev_priv: i915 device private
>> + *
>> + * This function enables communication with GuC, enables GuC
>> interrupts,
>> + * invokes GuC OS resumption and enables GuC submission.
>> + */
>> void intel_uc_resume(struct drm_i915_private *dev_priv)
>> {
>> - intel_guc_resume(&dev_priv->guc);
>> + struct intel_guc *guc = &dev_priv->guc;
>> + int ret;
>> +
>> + if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>> + return;
>> +
>> + ret = guc_enable_communication(guc);
>
> Hmm, not too early ? CT will try to talk with Guc.
intel_guc_resume will depend on CT mechanism to resume to GuC so CT
enabling should happen first.
Will keep this as it is v14 and revisit on the need to reorder.
>
>> + if (ret) {
>> + DRM_DEBUG_DRIVER("GuC communication enable failed (%d)\n",
>> ret);
>> + return;
>> + }
>> +
>> + if (i915_modparams.guc_log_level >= 0)
>> + gen9_enable_guc_interrupts(dev_priv);
>> +
>> + ret = intel_guc_resume(guc);
>> + if (ret)
>> + DRM_ERROR("GuC resume failed (%d)."
>> + "GuC functions may not work\n", ret);
>> +
>> + i915_guc_submission_enable(dev_priv);
>> +
>> + DRM_DEBUG_DRIVER("GuC submission %s\n",
>> + i915_guc_submission_enabled(guc) ?
>> + "enabled" : "disabled");
>
> Hmm, this message can be part of i915_guc_submission_enable
Ok. will move this message inside.
>
>> }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v13 11/21] drm/i915/guc: Remove GuC submission disable from i915_driver_unload
2017-10-11 8:53 [PATCH v13 00/21] drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring Sagar Arun Kamble
` (9 preceding siblings ...)
2017-10-11 8:54 ` [PATCH v13 10/21] drm/i915/guc: Update uC suspend/resume function separating Host/GuC tasks Sagar Arun Kamble
@ 2017-10-11 8:54 ` Sagar Arun Kamble
2017-10-11 8:54 ` [PATCH v13 12/21] drm/i915/guc: Fix GuC related state cleanup in unload path Sagar Arun Kamble
` (10 subsequent siblings)
21 siblings, 0 replies; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-11 8:54 UTC (permalink / raw)
To: intel-gfx
Earlier patch disabled GuC submission in i915_gem_suspend path.
This will destroy the client which will be setup back again during
resume. Unload relies on gem suspend hence we don't need to disable GuC
submission again during intel_uc_fini_hw (although it is not harm
calling at this point since we have added status checks.)
Also this location of disabling GuC submission was issue since it was
being done after GPU reset happening in i915_gem_suspend during
i915_driver_unload.
v2: Rebase w.r.t removal of GuC code restructuring. Added struct_mutex
protection for i915_guc_submission_disable.
v3: Rebase w.r.t updated GuC suspend function name.
v4: Added lockdep assert in i915_guc_submission_enable/disable.
Refined intel_uc_suspend to remove unnecessary locals and simplify
return. (Michal Winiarski)
Removed comment in guc_client_free about ignoring failure for
destroy_doorbell. (Oscar)
Rebase w.r.t i915_modparams change.
v5: Removed lockdep assert as mutex is needed by internal functions
which already have the asserts. (Chris)
Removed enable_guc_submission check for disabling GuC submission. (Chris)
v6: Rebase with enable_guc_submission related change done in earlier
newly introduced patches.
v7: Fixed intel_uc_resume to call intel_uc_runtime_resume and added
comment about need to enable submission later if needed. Commit message
updated. (Sagar)
v8. Rebase.
v9: Rebase. Updated comment.
v10: With uC suspend/resume functionality updated in the earlier patch
this patch merely fixes the unload path.
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: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Michał Winiarski <michal.winiarski@intel.com> #9
---
drivers/gpu/drm/i915/i915_guc_submission.c | 3 ---
drivers/gpu/drm/i915/intel_uc.c | 2 --
2 files changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 3da1346..fa81d3a 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -844,9 +844,6 @@ static void guc_client_free(struct i915_guc_client *client)
* Be sure to drop any locks
*/
- /* FIXME: in many cases, by the time we get here the GuC has been
- * reset, so we cannot destroy the doorbell properly. Ignore the
- * error message for now */
destroy_doorbell(client);
guc_stage_desc_fini(client->guc, client);
i915_gem_object_unpin_map(client->vma->obj);
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 297a321..716f581 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -273,8 +273,6 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
guc_free_load_err_log(guc);
- i915_guc_submission_disable(dev_priv);
-
guc_disable_communication(guc);
gen9_disable_guc_interrupts(dev_priv);
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 50+ messages in thread* [PATCH v13 12/21] drm/i915/guc: Fix GuC related state cleanup in unload path
2017-10-11 8:53 [PATCH v13 00/21] drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring Sagar Arun Kamble
` (10 preceding siblings ...)
2017-10-11 8:54 ` [PATCH v13 11/21] drm/i915/guc: Remove GuC submission disable from i915_driver_unload Sagar Arun Kamble
@ 2017-10-11 8:54 ` Sagar Arun Kamble
2017-10-11 8:54 ` [PATCH v13 13/21] drm/i915/uc: Support resume from sleep w/ and w/o GuC/HuC reload Sagar Arun Kamble
` (9 subsequent siblings)
21 siblings, 0 replies; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-11 8:54 UTC (permalink / raw)
To: intel-gfx
We ensure that GuC is completely suspended and client is destroyed
in i915_gem_suspend during i915_driver_unload. So now intel_uc_fini_hw
should just take care of cleanup, hence rename intel_uc_fini_hw as
intel_uc_cleanup. Correspondingly we also renamed i915_guc_submission_fini
as i915_guc_submission_cleanup. Other functionality to disable
communication, disable interrupts is taken care by intel_uc_suspend.
v2: Rebase w.r.t removal of GuC code restructuring.
v3: Removed intel_guc_cleanup. (Michal Wajdeczko)
v4: guc_free_load_err_log() needs to be called without checking
i915.enable_guc_loading as this param is cleared on GuC load failure.
(Michal Wajdeczko)
v5. Rebase.
v6: Rebase.
v7: Rebase.
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: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> #6
---
drivers/gpu/drm/i915/i915_drv.c | 2 +-
drivers/gpu/drm/i915/i915_guc_submission.c | 2 +-
drivers/gpu/drm/i915/i915_guc_submission.h | 2 +-
drivers/gpu/drm/i915/intel_uc.c | 9 +++------
drivers/gpu/drm/i915/intel_uc.h | 2 +-
5 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index df9a561..78aa90c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -612,7 +612,7 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv)
i915_gem_drain_workqueue(dev_priv);
mutex_lock(&dev_priv->drm.struct_mutex);
- intel_uc_fini_hw(dev_priv);
+ intel_uc_cleanup(dev_priv);
i915_gem_cleanup_engines(dev_priv);
i915_gem_contexts_fini(dev_priv);
i915_gem_cleanup_userptr(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index fa81d3a..ec8bed0 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -1014,7 +1014,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
return ret;
}
-void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
+void i915_guc_submission_cleanup(struct drm_i915_private *dev_priv)
{
struct intel_guc *guc = &dev_priv->guc;
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.h b/drivers/gpu/drm/i915/i915_guc_submission.h
index cb4353b..24b1ad8 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.h
+++ b/drivers/gpu/drm/i915/i915_guc_submission.h
@@ -75,6 +75,6 @@ struct i915_guc_client {
int i915_guc_submission_init(struct drm_i915_private *dev_priv);
int i915_guc_submission_enable(struct drm_i915_private *dev_priv);
void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
-void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
+void i915_guc_submission_cleanup(struct drm_i915_private *dev_priv);
#endif
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 716f581..1365724 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -245,7 +245,7 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
err_log_capture:
guc_capture_load_err_log(guc);
err_submission:
- i915_guc_submission_fini(dev_priv);
+ i915_guc_submission_cleanup(dev_priv);
err_guc:
i915_ggtt_disable_guc(dev_priv);
@@ -267,16 +267,13 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
return ret;
}
-void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
+void intel_uc_cleanup(struct drm_i915_private *dev_priv)
{
struct intel_guc *guc = &dev_priv->guc;
guc_free_load_err_log(guc);
- guc_disable_communication(guc);
-
- gen9_disable_guc_interrupts(dev_priv);
- i915_guc_submission_fini(dev_priv);
+ i915_guc_submission_cleanup(dev_priv);
if (i915_modparams.enable_guc_loading)
i915_ggtt_disable_guc(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index e20bc72..7d9dd9c 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -33,7 +33,7 @@
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_hw(struct drm_i915_private *dev_priv);
-void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
+void intel_uc_cleanup(struct drm_i915_private *dev_priv);
int intel_uc_suspend(struct drm_i915_private *dev_priv);
void intel_uc_resume(struct drm_i915_private *dev_priv);
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 50+ messages in thread* [PATCH v13 13/21] drm/i915/uc: Support resume from sleep w/ and w/o GuC/HuC reload
2017-10-11 8:53 [PATCH v13 00/21] drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring Sagar Arun Kamble
` (11 preceding siblings ...)
2017-10-11 8:54 ` [PATCH v13 12/21] drm/i915/guc: Fix GuC related state cleanup in unload path Sagar Arun Kamble
@ 2017-10-11 8:54 ` Sagar Arun Kamble
2017-10-11 17:06 ` Michal Wajdeczko
2017-10-11 8:54 ` [PATCH v13 14/21] drm/i915/uc: Update GEM runtime resume with need for reload of GuC/HuC Sagar Arun Kamble
` (8 subsequent siblings)
21 siblings, 1 reply; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-11 8:54 UTC (permalink / raw)
To: intel-gfx
GuC/HuC resume operation depends on whether firmwares are available
in the WOPCM region. This is known through register WOPCM_SIZE BIT(0).
If it indicates WOPCM is locked (bit is set) we just need to send action
to GuC to resume and enable other related GuC functionality such as
communication, interrupts and submission.
If it indicates WOPCM is not locked then we need to first reload the
GuC/HuC and then do all resume tasks. Currently on resume from sleep,
GuC/HuC are not loaded as GPU is reset at the end of suspend/early resume.
So we will have to reload the firmware and send action to resume.
Resume will be done through uc_init_hw from gem_init_hw based on newly
introduced state "guc->suspended". During gem_init_hw firmware load will
be skipped based on resume status during intel_uc_resume.
Also updated the accesses to dev_priv->guc and dev_priv->huc structure by
reusing initial declared pointer.
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: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_guc_reg.h | 1 +
drivers/gpu/drm/i915/intel_guc.c | 2 +
drivers/gpu/drm/i915/intel_guc.h | 3 ++
drivers/gpu/drm/i915/intel_uc.c | 99 +++++++++++++++++++++++++++++--------
4 files changed, 85 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h
index 35cf991..532296b 100644
--- a/drivers/gpu/drm/i915/i915_guc_reg.h
+++ b/drivers/gpu/drm/i915/i915_guc_reg.h
@@ -75,6 +75,7 @@
/* Defines WOPCM space available to GuC firmware */
#define GUC_WOPCM_SIZE _MMIO(0xc050)
+#define GUC_WOPCM_LOCKED BIT(0)
/* GuC addresses below GUC_WOPCM_TOP don't map through the GTT */
#define GUC_WOPCM_TOP (0x80 << 12) /* 512KB */
#define BXT_GUC_WOPCM_RC6_RESERVED (0x10 << 12) /* 64KB */
diff --git a/drivers/gpu/drm/i915/intel_guc.c b/drivers/gpu/drm/i915/intel_guc.c
index 55a0158..73be382 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -65,6 +65,8 @@ void intel_guc_init_early(struct intel_guc *guc)
mutex_init(&guc->send_mutex);
guc->send = intel_guc_send_nop;
guc->notify = gen8_guc_raise_irq;
+ guc->suspended = false;
+ guc->skip_load_on_resume = false;
}
int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len)
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index a587210..9f84033 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -38,6 +38,9 @@ struct intel_guc {
struct intel_guc_log log;
struct intel_guc_ct ct;
+ bool suspended;
+ bool skip_load_on_resume;
+
/* Log snapshot if GuC errors during load */
struct drm_i915_gem_object *load_err_log;
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 1365724..f641872 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -152,14 +152,35 @@ static void guc_disable_communication(struct intel_guc *guc)
guc->send = intel_guc_send_nop;
}
+static inline bool guc_wopcm_locked(struct intel_guc *guc)
+{
+ struct drm_i915_private *dev_priv = guc_to_i915(guc);
+
+ return I915_READ(GUC_WOPCM_SIZE) & GUC_WOPCM_LOCKED;
+}
+
int intel_uc_init_hw(struct drm_i915_private *dev_priv)
{
struct intel_guc *guc = &dev_priv->guc;
+ struct intel_huc *huc = &dev_priv->huc;
int ret, attempts;
if (!i915_modparams.enable_guc_loading)
return 0;
+ /*
+ * If on resume from sleep GuC was available we resumed GuC during
+ * i915_gem_resume. We need to skip load here. Reset skip_load_on_resume
+ * to allow load during module reload/reset/next resume behavior.
+ */
+ if (guc->skip_load_on_resume) {
+ guc->skip_load_on_resume = false;
+ return 0;
+ }
+
+ WARN_ON_ONCE(guc->fw.load_status == INTEL_UC_FIRMWARE_SUCCESS);
+ WARN_ON_ONCE(huc->fw.load_status == INTEL_UC_FIRMWARE_SUCCESS);
+
guc_disable_communication(guc);
gen9_reset_guc_interrupts(dev_priv);
@@ -197,8 +218,8 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
if (ret)
goto err_submission;
- intel_huc_init_hw(&dev_priv->huc);
- ret = intel_guc_init_hw(&dev_priv->guc);
+ intel_huc_init_hw(huc);
+ ret = intel_guc_init_hw(guc);
if (ret == 0 || ret != -EAGAIN)
break;
@@ -214,7 +235,21 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
if (ret)
goto err_log_capture;
- intel_huc_auth(&dev_priv->huc);
+ /*
+ * If WOPCM was not locked during resume from sleep, GuC/HuC need to
+ * be reloaded. For this we are using intel_uc_init_hw path as we want
+ * to handle HuC/GuC reload, GuC resume, HuC authentication and
+ * submission enabling etc. all here.
+ */
+ if (guc->suspended) {
+ ret = intel_guc_resume(guc);
+ if (ret)
+ DRM_ERROR("GuC resume failed (%d)."
+ " GuC functions may not work\n", ret);
+ guc->suspended = false;
+ }
+
+ intel_huc_auth(huc);
if (i915_guc_submission_initialized(guc)) {
if (i915_modparams.guc_log_level >= 0)
gen9_enable_guc_interrupts(dev_priv);
@@ -305,6 +340,8 @@ int intel_uc_suspend(struct drm_i915_private *dev_priv)
gen9_disable_guc_interrupts(dev_priv);
guc_disable_communication(guc);
+ guc->suspended = true;
+
goto out;
out_suspend:
@@ -326,28 +363,50 @@ int intel_uc_suspend(struct drm_i915_private *dev_priv)
void intel_uc_resume(struct drm_i915_private *dev_priv)
{
struct intel_guc *guc = &dev_priv->guc;
+ struct intel_huc *huc = &dev_priv->huc;
int ret;
- if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
- return;
-
- ret = guc_enable_communication(guc);
- if (ret) {
- DRM_DEBUG_DRIVER("GuC communication enable failed (%d)\n", ret);
+ if (!guc->suspended)
return;
- }
- if (i915_modparams.guc_log_level >= 0)
- gen9_enable_guc_interrupts(dev_priv);
+ /*
+ * If WOPCM is locked then GuC and HuC are still loaded. We just
+ * need to enable communication with GuC, enable interrupts,
+ * invoke GuC action to resume from sleep and enable submission.
+ * If WOPCM is not locked it is similar to fresh boot and we need
+ * reload the GuC/HuC firmwares and enable other GuC related
+ * mechanisms. Post reloading GuC we need to send action to resume
+ * from sleep for GuC to restore its state prior to suspend.
+ */
+ if (guc_wopcm_locked(guc)) {
+ huc->fw.load_status = INTEL_UC_FIRMWARE_SUCCESS;
+ guc->fw.load_status = INTEL_UC_FIRMWARE_SUCCESS;
- ret = intel_guc_resume(guc);
- if (ret)
- DRM_ERROR("GuC resume failed (%d)."
- "GuC functions may not work\n", ret);
+ ret = guc_enable_communication(guc);
+ if (ret) {
+ DRM_DEBUG_DRIVER("GuC communication enable failed"
+ " (%d)\n", ret);
+ return;
+ }
- i915_guc_submission_enable(dev_priv);
+ if (i915_modparams.guc_log_level >= 0)
+ gen9_enable_guc_interrupts(dev_priv);
- DRM_DEBUG_DRIVER("GuC submission %s\n",
- i915_guc_submission_enabled(guc) ?
- "enabled" : "disabled");
+ ret = intel_guc_resume(guc);
+ if (ret)
+ DRM_ERROR("GuC resume failed (%d)."
+ " GuC functions may not work\n", ret);
+
+ i915_guc_submission_enable(dev_priv);
+
+ DRM_DEBUG_DRIVER("GuC submission %s\n",
+ i915_guc_submission_enabled(guc) ?
+ "enabled" : "disabled");
+ guc->suspended = false;
+ guc->skip_load_on_resume = true;
+ } else {
+ DRM_DEBUG_DRIVER("GuC not available. Resume will be done"
+ " during i915_gem_init_hw\n");
+ guc->skip_load_on_resume = false;
+ }
}
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH v13 13/21] drm/i915/uc: Support resume from sleep w/ and w/o GuC/HuC reload
2017-10-11 8:54 ` [PATCH v13 13/21] drm/i915/uc: Support resume from sleep w/ and w/o GuC/HuC reload Sagar Arun Kamble
@ 2017-10-11 17:06 ` Michal Wajdeczko
2017-10-12 6:48 ` Sagar Arun Kamble
0 siblings, 1 reply; 50+ messages in thread
From: Michal Wajdeczko @ 2017-10-11 17:06 UTC (permalink / raw)
To: intel-gfx, Sagar Arun Kamble
On Wed, 11 Oct 2017 10:54:08 +0200, Sagar Arun Kamble
<sagar.a.kamble@intel.com> wrote:
> GuC/HuC resume operation depends on whether firmwares are available
> in the WOPCM region. This is known through register WOPCM_SIZE BIT(0).
>
> If it indicates WOPCM is locked (bit is set) we just need to send action
> to GuC to resume and enable other related GuC functionality such as
> communication, interrupts and submission.
>
> If it indicates WOPCM is not locked then we need to first reload the
> GuC/HuC and then do all resume tasks. Currently on resume from sleep,
> GuC/HuC are not loaded as GPU is reset at the end of suspend/early
> resume.
> So we will have to reload the firmware and send action to resume.
> Resume will be done through uc_init_hw from gem_init_hw based on newly
> introduced state "guc->suspended". During gem_init_hw firmware load will
> be skipped based on resume status during intel_uc_resume.
>
> Also updated the accesses to dev_priv->guc and dev_priv->huc structure by
> reusing initial declared pointer.
>
> 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: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_guc_reg.h | 1 +
> drivers/gpu/drm/i915/intel_guc.c | 2 +
> drivers/gpu/drm/i915/intel_guc.h | 3 ++
> drivers/gpu/drm/i915/intel_uc.c | 99
> +++++++++++++++++++++++++++++--------
> 4 files changed, 85 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h
> b/drivers/gpu/drm/i915/i915_guc_reg.h
> index 35cf991..532296b 100644
> --- a/drivers/gpu/drm/i915/i915_guc_reg.h
> +++ b/drivers/gpu/drm/i915/i915_guc_reg.h
> @@ -75,6 +75,7 @@
> /* Defines WOPCM space available to GuC firmware */
> #define GUC_WOPCM_SIZE _MMIO(0xc050)
> +#define GUC_WOPCM_LOCKED BIT(0)
> /* GuC addresses below GUC_WOPCM_TOP don't map through the GTT */
> #define GUC_WOPCM_TOP (0x80 << 12) /* 512KB */
> #define BXT_GUC_WOPCM_RC6_RESERVED (0x10 << 12) /* 64KB */
> diff --git a/drivers/gpu/drm/i915/intel_guc.c
> b/drivers/gpu/drm/i915/intel_guc.c
> index 55a0158..73be382 100644
> --- a/drivers/gpu/drm/i915/intel_guc.c
> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -65,6 +65,8 @@ void intel_guc_init_early(struct intel_guc *guc)
> mutex_init(&guc->send_mutex);
> guc->send = intel_guc_send_nop;
> guc->notify = gen8_guc_raise_irq;
> + guc->suspended = false;
> + guc->skip_load_on_resume = false;
> }
> int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len)
> diff --git a/drivers/gpu/drm/i915/intel_guc.h
> b/drivers/gpu/drm/i915/intel_guc.h
> index a587210..9f84033 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -38,6 +38,9 @@ struct intel_guc {
> struct intel_guc_log log;
> struct intel_guc_ct ct;
> + bool suspended;
> + bool skip_load_on_resume;
maybe bool xxx:1 to save space.
> +
> /* Log snapshot if GuC errors during load */
> struct drm_i915_gem_object *load_err_log;
> diff --git a/drivers/gpu/drm/i915/intel_uc.c
> b/drivers/gpu/drm/i915/intel_uc.c
> index 1365724..f641872 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -152,14 +152,35 @@ static void guc_disable_communication(struct
> intel_guc *guc)
> guc->send = intel_guc_send_nop;
> }
> +static inline bool guc_wopcm_locked(struct intel_guc *guc)
> +{
> + struct drm_i915_private *dev_priv = guc_to_i915(guc);
> +
> + return I915_READ(GUC_WOPCM_SIZE) & GUC_WOPCM_LOCKED;
what about adding !! trick?
> +}
> +
> int intel_uc_init_hw(struct drm_i915_private *dev_priv)
> {
> struct intel_guc *guc = &dev_priv->guc;
> + struct intel_huc *huc = &dev_priv->huc;
> int ret, attempts;
> if (!i915_modparams.enable_guc_loading)
> return 0;
> + /*
> + * If on resume from sleep GuC was available we resumed GuC during
> + * i915_gem_resume. We need to skip load here. Reset
> skip_load_on_resume
> + * to allow load during module reload/reset/next resume behavior.
> + */
> + if (guc->skip_load_on_resume) {
> + guc->skip_load_on_resume = false;
> + return 0;
> + }
> +
> + WARN_ON_ONCE(guc->fw.load_status == INTEL_UC_FIRMWARE_SUCCESS);
> + WARN_ON_ONCE(huc->fw.load_status == INTEL_UC_FIRMWARE_SUCCESS);
> +
> guc_disable_communication(guc);
> gen9_reset_guc_interrupts(dev_priv);
> @@ -197,8 +218,8 @@ int intel_uc_init_hw(struct drm_i915_private
> *dev_priv)
> if (ret)
> goto err_submission;
> - intel_huc_init_hw(&dev_priv->huc);
> - ret = intel_guc_init_hw(&dev_priv->guc);
> + intel_huc_init_hw(huc);
> + ret = intel_guc_init_hw(guc);
> if (ret == 0 || ret != -EAGAIN)
> break;
> @@ -214,7 +235,21 @@ int intel_uc_init_hw(struct drm_i915_private
> *dev_priv)
> if (ret)
> goto err_log_capture;
> - intel_huc_auth(&dev_priv->huc);
> + /*
> + * If WOPCM was not locked during resume from sleep, GuC/HuC need to
> + * be reloaded. For this we are using intel_uc_init_hw path as we want
> + * to handle HuC/GuC reload, GuC resume, HuC authentication and
> + * submission enabling etc. all here.
> + */
> + if (guc->suspended) {
> + ret = intel_guc_resume(guc);
> + if (ret)
> + DRM_ERROR("GuC resume failed (%d)."
> + " GuC functions may not work\n", ret);
> + guc->suspended = false;
> + }
> +
> + intel_huc_auth(huc);
> if (i915_guc_submission_initialized(guc)) {
> if (i915_modparams.guc_log_level >= 0)
> gen9_enable_guc_interrupts(dev_priv);
> @@ -305,6 +340,8 @@ int intel_uc_suspend(struct drm_i915_private
> *dev_priv)
> gen9_disable_guc_interrupts(dev_priv);
> guc_disable_communication(guc);
> + guc->suspended = true;
> +
> goto out;
> out_suspend:
> @@ -326,28 +363,50 @@ int intel_uc_suspend(struct drm_i915_private
> *dev_priv)
> void intel_uc_resume(struct drm_i915_private *dev_priv)
> {
> struct intel_guc *guc = &dev_priv->guc;
> + struct intel_huc *huc = &dev_priv->huc;
> int ret;
> - if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
> - return;
> -
> - ret = guc_enable_communication(guc);
> - if (ret) {
> - DRM_DEBUG_DRIVER("GuC communication enable failed (%d)\n", ret);
> + if (!guc->suspended)
> return;
> - }
> - if (i915_modparams.guc_log_level >= 0)
> - gen9_enable_guc_interrupts(dev_priv);
> + /*
> + * If WOPCM is locked then GuC and HuC are still loaded. We just
> + * need to enable communication with GuC, enable interrupts,
> + * invoke GuC action to resume from sleep and enable submission.
> + * If WOPCM is not locked it is similar to fresh boot and we need
> + * reload the GuC/HuC firmwares and enable other GuC related
> + * mechanisms. Post reloading GuC we need to send action to resume
> + * from sleep for GuC to restore its state prior to suspend.
> + */
> + if (guc_wopcm_locked(guc)) {
> + huc->fw.load_status = INTEL_UC_FIRMWARE_SUCCESS;
> + guc->fw.load_status = INTEL_UC_FIRMWARE_SUCCESS;
Hmm, as we didn't clear load_status (or at least I can't find it) then
load_status should still be INTEL_UC_FIRMWARE_SUCCESS here.
Maybe instead of introducing new skip_load_on_resume flag we can
just rely on load_status that is correctly maintained across suspend
resume.
> - ret = intel_guc_resume(guc);
> - if (ret)
> - DRM_ERROR("GuC resume failed (%d)."
> - "GuC functions may not work\n", ret);
> + ret = guc_enable_communication(guc);
> + if (ret) {
> + DRM_DEBUG_DRIVER("GuC communication enable failed"
> + " (%d)\n", ret);
Btw, the only path in guc_enable_communication that can fail reports
error so maybe this extra debug is not necessary ?
Maybe better option will be to reorg this code to include note about
forced GuC reload during init_hw ?
> + return;
> + }
> - i915_guc_submission_enable(dev_priv);
> + if (i915_modparams.guc_log_level >= 0)
> + gen9_enable_guc_interrupts(dev_priv);
> - DRM_DEBUG_DRIVER("GuC submission %s\n",
> - i915_guc_submission_enabled(guc) ?
> - "enabled" : "disabled");
> + ret = intel_guc_resume(guc);
> + if (ret)
> + DRM_ERROR("GuC resume failed (%d)."
> + " GuC functions may not work\n", ret);
> +
Hmm, is it safe to continue with submission after failing resume ?
> + i915_guc_submission_enable(dev_priv);
> +
> + DRM_DEBUG_DRIVER("GuC submission %s\n",
> + i915_guc_submission_enabled(guc) ?
> + "enabled" : "disabled");
> + guc->suspended = false;
> + guc->skip_load_on_resume = true;
> + } else {
> + DRM_DEBUG_DRIVER("GuC not available. Resume will be done"
> + " during i915_gem_init_hw\n");
> + guc->skip_load_on_resume = false;
> + }
> }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v13 13/21] drm/i915/uc: Support resume from sleep w/ and w/o GuC/HuC reload
2017-10-11 17:06 ` Michal Wajdeczko
@ 2017-10-12 6:48 ` Sagar Arun Kamble
0 siblings, 0 replies; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-12 6:48 UTC (permalink / raw)
To: Michal Wajdeczko, intel-gfx
On 10/11/2017 10:36 PM, Michal Wajdeczko wrote:
> On Wed, 11 Oct 2017 10:54:08 +0200, Sagar Arun Kamble
> <sagar.a.kamble@intel.com> wrote:
>
>> GuC/HuC resume operation depends on whether firmwares are available
>> in the WOPCM region. This is known through register WOPCM_SIZE BIT(0).
>>
>> If it indicates WOPCM is locked (bit is set) we just need to send action
>> to GuC to resume and enable other related GuC functionality such as
>> communication, interrupts and submission.
>>
>> If it indicates WOPCM is not locked then we need to first reload the
>> GuC/HuC and then do all resume tasks. Currently on resume from sleep,
>> GuC/HuC are not loaded as GPU is reset at the end of suspend/early
>> resume.
>> So we will have to reload the firmware and send action to resume.
>> Resume will be done through uc_init_hw from gem_init_hw based on newly
>> introduced state "guc->suspended". During gem_init_hw firmware load will
>> be skipped based on resume status during intel_uc_resume.
>>
>> Also updated the accesses to dev_priv->guc and dev_priv->huc
>> structure by
>> reusing initial declared pointer.
>>
>> 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: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_guc_reg.h | 1 +
>> drivers/gpu/drm/i915/intel_guc.c | 2 +
>> drivers/gpu/drm/i915/intel_guc.h | 3 ++
>> drivers/gpu/drm/i915/intel_uc.c | 99
>> +++++++++++++++++++++++++++++--------
>> 4 files changed, 85 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h
>> b/drivers/gpu/drm/i915/i915_guc_reg.h
>> index 35cf991..532296b 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_guc_reg.h
>> @@ -75,6 +75,7 @@
>> /* Defines WOPCM space available to GuC firmware */
>> #define GUC_WOPCM_SIZE _MMIO(0xc050)
>> +#define GUC_WOPCM_LOCKED BIT(0)
>> /* GuC addresses below GUC_WOPCM_TOP don't map through the GTT */
>> #define GUC_WOPCM_TOP (0x80 << 12) /* 512KB */
>> #define BXT_GUC_WOPCM_RC6_RESERVED (0x10 << 12) /* 64KB */
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index 55a0158..73be382 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -65,6 +65,8 @@ void intel_guc_init_early(struct intel_guc *guc)
>> mutex_init(&guc->send_mutex);
>> guc->send = intel_guc_send_nop;
>> guc->notify = gen8_guc_raise_irq;
>> + guc->suspended = false;
>> + guc->skip_load_on_resume = false;
>> }
>> int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32
>> len)
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index a587210..9f84033 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -38,6 +38,9 @@ struct intel_guc {
>> struct intel_guc_log log;
>> struct intel_guc_ct ct;
>> + bool suspended;
>> + bool skip_load_on_resume;
>
> maybe bool xxx:1 to save space.
Ok. Will update.
>
>> +
>> /* Log snapshot if GuC errors during load */
>> struct drm_i915_gem_object *load_err_log;
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 1365724..f641872 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -152,14 +152,35 @@ static void guc_disable_communication(struct
>> intel_guc *guc)
>> guc->send = intel_guc_send_nop;
>> }
>> +static inline bool guc_wopcm_locked(struct intel_guc *guc)
>> +{
>> + struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +
>> + return I915_READ(GUC_WOPCM_SIZE) & GUC_WOPCM_LOCKED;
>
> what about adding !! trick?
Sure. Will add.
>
>> +}
>> +
>> int intel_uc_init_hw(struct drm_i915_private *dev_priv)
>> {
>> struct intel_guc *guc = &dev_priv->guc;
>> + struct intel_huc *huc = &dev_priv->huc;
>> int ret, attempts;
>> if (!i915_modparams.enable_guc_loading)
>> return 0;
>> + /*
>> + * If on resume from sleep GuC was available we resumed GuC during
>> + * i915_gem_resume. We need to skip load here. Reset
>> skip_load_on_resume
>> + * to allow load during module reload/reset/next resume behavior.
>> + */
>> + if (guc->skip_load_on_resume) {
>> + guc->skip_load_on_resume = false;
>> + return 0;
>> + }
>> +
>> + WARN_ON_ONCE(guc->fw.load_status == INTEL_UC_FIRMWARE_SUCCESS);
>> + WARN_ON_ONCE(huc->fw.load_status == INTEL_UC_FIRMWARE_SUCCESS);
>> +
>> guc_disable_communication(guc);
>> gen9_reset_guc_interrupts(dev_priv);
>> @@ -197,8 +218,8 @@ int intel_uc_init_hw(struct drm_i915_private
>> *dev_priv)
>> if (ret)
>> goto err_submission;
>> - intel_huc_init_hw(&dev_priv->huc);
>> - ret = intel_guc_init_hw(&dev_priv->guc);
>> + intel_huc_init_hw(huc);
>> + ret = intel_guc_init_hw(guc);
>> if (ret == 0 || ret != -EAGAIN)
>> break;
>> @@ -214,7 +235,21 @@ int intel_uc_init_hw(struct drm_i915_private
>> *dev_priv)
>> if (ret)
>> goto err_log_capture;
>> - intel_huc_auth(&dev_priv->huc);
>> + /*
>> + * If WOPCM was not locked during resume from sleep, GuC/HuC
>> need to
>> + * be reloaded. For this we are using intel_uc_init_hw path as
>> we want
>> + * to handle HuC/GuC reload, GuC resume, HuC authentication and
>> + * submission enabling etc. all here.
>> + */
>> + if (guc->suspended) {
>> + ret = intel_guc_resume(guc);
>> + if (ret)
>> + DRM_ERROR("GuC resume failed (%d)."
>> + " GuC functions may not work\n", ret);
>> + guc->suspended = false;
>> + }
>> +
>> + intel_huc_auth(huc);
>> if (i915_guc_submission_initialized(guc)) {
>> if (i915_modparams.guc_log_level >= 0)
>> gen9_enable_guc_interrupts(dev_priv);
>> @@ -305,6 +340,8 @@ int intel_uc_suspend(struct drm_i915_private
>> *dev_priv)
>> gen9_disable_guc_interrupts(dev_priv);
>> guc_disable_communication(guc);
>> + guc->suspended = true;
>> +
>> goto out;
>> out_suspend:
>> @@ -326,28 +363,50 @@ int intel_uc_suspend(struct drm_i915_private
>> *dev_priv)
>> void intel_uc_resume(struct drm_i915_private *dev_priv)
>> {
>> struct intel_guc *guc = &dev_priv->guc;
>> + struct intel_huc *huc = &dev_priv->huc;
>> int ret;
>> - if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>> - return;
>> -
>> - ret = guc_enable_communication(guc);
>> - if (ret) {
>> - DRM_DEBUG_DRIVER("GuC communication enable failed (%d)\n",
>> ret);
>> + if (!guc->suspended)
>> return;
>> - }
>> - if (i915_modparams.guc_log_level >= 0)
>> - gen9_enable_guc_interrupts(dev_priv);
>> + /*
>> + * If WOPCM is locked then GuC and HuC are still loaded. We just
>> + * need to enable communication with GuC, enable interrupts,
>> + * invoke GuC action to resume from sleep and enable submission.
>> + * If WOPCM is not locked it is similar to fresh boot and we need
>> + * reload the GuC/HuC firmwares and enable other GuC related
>> + * mechanisms. Post reloading GuC we need to send action to resume
>> + * from sleep for GuC to restore its state prior to suspend.
>> + */
>> + if (guc_wopcm_locked(guc)) {
>> + huc->fw.load_status = INTEL_UC_FIRMWARE_SUCCESS;
>> + guc->fw.load_status = INTEL_UC_FIRMWARE_SUCCESS;
>
> Hmm, as we didn't clear load_status (or at least I can't find it) then
> load_status should still be INTEL_UC_FIRMWARE_SUCCESS here.
>
> Maybe instead of introducing new skip_load_on_resume flag we can
> just rely on load_status that is correctly maintained across suspend
> resume.
load status will be cleared in the uc_sanitize and that patch was later
in the sequence.
Have pulled earlier in the v14.
We can drop the skip_load_on_resume as you suggested. will update.
>
>> - ret = intel_guc_resume(guc);
>> - if (ret)
>> - DRM_ERROR("GuC resume failed (%d)."
>> - "GuC functions may not work\n", ret);
>> + ret = guc_enable_communication(guc);
>> + if (ret) {
>> + DRM_DEBUG_DRIVER("GuC communication enable failed"
>> + " (%d)\n", ret);
>
> Btw, the only path in guc_enable_communication that can fail reports
> error so maybe this extra debug is not necessary ?
ok will remove.
>
> Maybe better option will be to reorg this code to include note about
> forced GuC reload during init_hw ?
ok.
>
>> + return;
>> + }
>> - i915_guc_submission_enable(dev_priv);
>> + if (i915_modparams.guc_log_level >= 0)
>> + gen9_enable_guc_interrupts(dev_priv);
>> - DRM_DEBUG_DRIVER("GuC submission %s\n",
>> - i915_guc_submission_enabled(guc) ?
>> - "enabled" : "disabled");
>> + ret = intel_guc_resume(guc);
>> + if (ret)
>> + DRM_ERROR("GuC resume failed (%d)."
>> + " GuC functions may not work\n", ret);
>> +
>
> Hmm, is it safe to continue with submission after failing resume ?
It may not so will revert all earlier steps and fall back to execlists.
>
>> + i915_guc_submission_enable(dev_priv);
>> +
>> + DRM_DEBUG_DRIVER("GuC submission %s\n",
>> + i915_guc_submission_enabled(guc) ?
>> + "enabled" : "disabled");
>> + guc->suspended = false;
>> + guc->skip_load_on_resume = true;
>> + } else {
>> + DRM_DEBUG_DRIVER("GuC not available. Resume will be done"
>> + " during i915_gem_init_hw\n");
>> + guc->skip_load_on_resume = false;
>> + }
>> }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v13 14/21] drm/i915/uc: Update GEM runtime resume with need for reload of GuC/HuC
2017-10-11 8:53 [PATCH v13 00/21] drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring Sagar Arun Kamble
` (12 preceding siblings ...)
2017-10-11 8:54 ` [PATCH v13 13/21] drm/i915/uc: Support resume from sleep w/ and w/o GuC/HuC reload Sagar Arun Kamble
@ 2017-10-11 8:54 ` Sagar Arun Kamble
2017-10-11 17:19 ` Michal Wajdeczko
2017-10-11 8:54 ` [PATCH v13 15/21] drm/i915/guc: Add comment about update needed in GuC submission enable/disable for RPM Sagar Arun Kamble
` (7 subsequent siblings)
21 siblings, 1 reply; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-11 8:54 UTC (permalink / raw)
To: intel-gfx
On resume from drm sleep/suspend, we have gem_init_hw path to reload
the GuC/HuC firmware. However, on resume from runtime suspend we needed
to add support to reload the GuC/HuC firmware and resume.
We can leverage intel_uc_init_hw for this based on skip_load_on_resume.
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: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_gem.c | 2 +-
drivers/gpu/drm/i915/intel_uc.c | 28 ++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_uc.h | 1 +
3 files changed, 30 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7d1b7e1..9e257e2 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2113,7 +2113,7 @@ void i915_gem_runtime_resume(struct drm_i915_private *dev_priv)
i915_gem_init_swizzling(dev_priv);
i915_gem_restore_fences(dev_priv);
- intel_uc_resume(dev_priv);
+ intel_uc_runtime_resume(dev_priv);
mutex_unlock(&dev_priv->drm.struct_mutex);
}
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index f641872..25acf8f 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -410,3 +410,31 @@ void intel_uc_resume(struct drm_i915_private *dev_priv)
guc->skip_load_on_resume = false;
}
}
+
+/**
+ * intel_uc_runtime_resume() - Resume uC operation.
+ * @dev_priv: i915 device private
+ *
+ * This function invokes intel_uc_suspend that will if GuC is loaded
+ * enable communication with GuC, enable GuC interrupts, invoke GuC OS
+ * resumption and enable GuC submission.
+ * If GuC is not loaded, GuC needs to be loaded and do the entire setup
+ * by leveraging intel_uc_init_hw.
+ *
+ */
+void intel_uc_runtime_resume(struct drm_i915_private *dev_priv)
+{
+ struct intel_guc *guc = &dev_priv->guc;
+
+ if (!guc->suspended)
+ return;
+
+ intel_uc_resume(dev_priv);
+
+ if (guc->skip_load_on_resume)
+ return;
+
+ WARN_ON(guc_wopcm_locked(guc));
+
+ intel_uc_init_hw(dev_priv);
+}
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 7d9dd9c..f741ccc 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -36,5 +36,6 @@
void intel_uc_cleanup(struct drm_i915_private *dev_priv);
int intel_uc_suspend(struct drm_i915_private *dev_priv);
void intel_uc_resume(struct drm_i915_private *dev_priv);
+void intel_uc_runtime_resume(struct drm_i915_private *dev_priv);
#endif
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH v13 14/21] drm/i915/uc: Update GEM runtime resume with need for reload of GuC/HuC
2017-10-11 8:54 ` [PATCH v13 14/21] drm/i915/uc: Update GEM runtime resume with need for reload of GuC/HuC Sagar Arun Kamble
@ 2017-10-11 17:19 ` Michal Wajdeczko
2017-10-12 6:50 ` Sagar Arun Kamble
0 siblings, 1 reply; 50+ messages in thread
From: Michal Wajdeczko @ 2017-10-11 17:19 UTC (permalink / raw)
To: intel-gfx, Sagar Arun Kamble
On Wed, 11 Oct 2017 10:54:09 +0200, Sagar Arun Kamble
<sagar.a.kamble@intel.com> wrote:
> On resume from drm sleep/suspend, we have gem_init_hw path to reload
> the GuC/HuC firmware. However, on resume from runtime suspend we needed
> to add support to reload the GuC/HuC firmware and resume.
> We can leverage intel_uc_init_hw for this based on skip_load_on_resume.
>
> 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: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 2 +-
> drivers/gpu/drm/i915/intel_uc.c | 28 ++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_uc.h | 1 +
> 3 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c
> index 7d1b7e1..9e257e2 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2113,7 +2113,7 @@ void i915_gem_runtime_resume(struct
> drm_i915_private *dev_priv)
> i915_gem_init_swizzling(dev_priv);
> i915_gem_restore_fences(dev_priv);
> - intel_uc_resume(dev_priv);
> + intel_uc_runtime_resume(dev_priv);
> mutex_unlock(&dev_priv->drm.struct_mutex);
> }
> diff --git a/drivers/gpu/drm/i915/intel_uc.c
> b/drivers/gpu/drm/i915/intel_uc.c
> index f641872..25acf8f 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -410,3 +410,31 @@ void intel_uc_resume(struct drm_i915_private
> *dev_priv)
> guc->skip_load_on_resume = false;
> }
> }
> +
> +/**
> + * intel_uc_runtime_resume() - Resume uC operation.
> + * @dev_priv: i915 device private
> + *
> + * This function invokes intel_uc_suspend that will if GuC is loaded
^^^^^^^^^^^^^^^^
Please focus on tasks rather than function names.
> + * enable communication with GuC, enable GuC interrupts, invoke GuC OS
> + * resumption and enable GuC submission.
> + * If GuC is not loaded, GuC needs to be loaded and do the entire setup
> + * by leveraging intel_uc_init_hw.
> + *
> + */
> +void intel_uc_runtime_resume(struct drm_i915_private *dev_priv)
> +{
> + struct intel_guc *guc = &dev_priv->guc;
> +
> + if (!guc->suspended)
> + return;
> +
> + intel_uc_resume(dev_priv);
> +
> + if (guc->skip_load_on_resume)
Hmm, I may be lost, but I feel that some changes from 13/21 done
in intel_uc_resume() looks like good candidate for this function.
What I'm missing is clear distinction what each function will do,
due to lot of conditions and cross calls.
> + return;
> +
> + WARN_ON(guc_wopcm_locked(guc));
Why here?
> +
> + intel_uc_init_hw(dev_priv);
> +}
> diff --git a/drivers/gpu/drm/i915/intel_uc.h
> b/drivers/gpu/drm/i915/intel_uc.h
> index 7d9dd9c..f741ccc 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -36,5 +36,6 @@
> void intel_uc_cleanup(struct drm_i915_private *dev_priv);
> int intel_uc_suspend(struct drm_i915_private *dev_priv);
> void intel_uc_resume(struct drm_i915_private *dev_priv);
> +void intel_uc_runtime_resume(struct drm_i915_private *dev_priv);
> #endif
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v13 14/21] drm/i915/uc: Update GEM runtime resume with need for reload of GuC/HuC
2017-10-11 17:19 ` Michal Wajdeczko
@ 2017-10-12 6:50 ` Sagar Arun Kamble
0 siblings, 0 replies; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-12 6:50 UTC (permalink / raw)
To: Michal Wajdeczko, intel-gfx
On 10/11/2017 10:49 PM, Michal Wajdeczko wrote:
> On Wed, 11 Oct 2017 10:54:09 +0200, Sagar Arun Kamble
> <sagar.a.kamble@intel.com> wrote:
>
>> On resume from drm sleep/suspend, we have gem_init_hw path to reload
>> the GuC/HuC firmware. However, on resume from runtime suspend we needed
>> to add support to reload the GuC/HuC firmware and resume.
>> We can leverage intel_uc_init_hw for this based on skip_load_on_resume.
>>
>> 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: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_gem.c | 2 +-
>> drivers/gpu/drm/i915/intel_uc.c | 28 ++++++++++++++++++++++++++++
>> drivers/gpu/drm/i915/intel_uc.h | 1 +
>> 3 files changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index 7d1b7e1..9e257e2 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2113,7 +2113,7 @@ void i915_gem_runtime_resume(struct
>> drm_i915_private *dev_priv)
>> i915_gem_init_swizzling(dev_priv);
>> i915_gem_restore_fences(dev_priv);
>> - intel_uc_resume(dev_priv);
>> + intel_uc_runtime_resume(dev_priv);
>> mutex_unlock(&dev_priv->drm.struct_mutex);
>> }
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index f641872..25acf8f 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -410,3 +410,31 @@ void intel_uc_resume(struct drm_i915_private
>> *dev_priv)
>> guc->skip_load_on_resume = false;
>> }
>> }
>> +
>> +/**
>> + * intel_uc_runtime_resume() - Resume uC operation.
>> + * @dev_priv: i915 device private
>> + *
>> + * This function invokes intel_uc_suspend that will if GuC is loaded
> ^^^^^^^^^^^^^^^^
> Please focus on tasks rather than function names.
Sure.
>
>> + * enable communication with GuC, enable GuC interrupts, invoke GuC OS
>> + * resumption and enable GuC submission.
>> + * If GuC is not loaded, GuC needs to be loaded and do the entire setup
>> + * by leveraging intel_uc_init_hw.
>> + *
>> + */
>> +void intel_uc_runtime_resume(struct drm_i915_private *dev_priv)
>> +{
>> + struct intel_guc *guc = &dev_priv->guc;
>> +
>> + if (!guc->suspended)
>> + return;
>> +
>> + intel_uc_resume(dev_priv);
>> +
>> + if (guc->skip_load_on_resume)
>
> Hmm, I may be lost, but I feel that some changes from 13/21 done
> in intel_uc_resume() looks like good candidate for this function.
>
> What I'm missing is clear distinction what each function will do,
> due to lot of conditions and cross calls.
Have introduced separate runtime and drm uc_suspend/resume functions in
v14 and will help understand this better.
>
>> + return;
>> +
>> + WARN_ON(guc_wopcm_locked(guc));
>
> Why here?
will remove.
>
>> +
>> + intel_uc_init_hw(dev_priv);
>> +}
>> diff --git a/drivers/gpu/drm/i915/intel_uc.h
>> b/drivers/gpu/drm/i915/intel_uc.h
>> index 7d9dd9c..f741ccc 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.h
>> +++ b/drivers/gpu/drm/i915/intel_uc.h
>> @@ -36,5 +36,6 @@
>> void intel_uc_cleanup(struct drm_i915_private *dev_priv);
>> int intel_uc_suspend(struct drm_i915_private *dev_priv);
>> void intel_uc_resume(struct drm_i915_private *dev_priv);
>> +void intel_uc_runtime_resume(struct drm_i915_private *dev_priv);
>> #endif
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v13 15/21] drm/i915/guc: Add comment about update needed in GuC submission enable/disable for RPM
2017-10-11 8:53 [PATCH v13 00/21] drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring Sagar Arun Kamble
` (13 preceding siblings ...)
2017-10-11 8:54 ` [PATCH v13 14/21] drm/i915/uc: Update GEM runtime resume with need for reload of GuC/HuC Sagar Arun Kamble
@ 2017-10-11 8:54 ` Sagar Arun Kamble
2017-10-11 8:54 ` [PATCH v13 16/21] drm/i915: Enable interrupts prior to GEM resume during i915_drm_resume Sagar Arun Kamble
` (6 subsequent siblings)
21 siblings, 0 replies; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-11 8:54 UTC (permalink / raw)
To: intel-gfx
During Runtime suspend/resume, GuC is allocating/destroying client vmas
that needs RPM reference and it leads to recursion. So RPM suspend/resume
will not work currently with GuC enabled. Clarify this TODO through
comment in i915_gem_runtime_suspend/resume while invoking uC
suspend/resume functions.
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: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_gem.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 9e257e2..bcf3ca5 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2060,6 +2060,13 @@ int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
mutex_lock(&dev_priv->drm.struct_mutex);
+ /*
+ * TODO: Note that currently GuC submission enable/disable will
+ * lead to GuC execbuf client vma allocation/free that
+ * in turn involves RPM get/put hence this path will hang.
+ * Once we update guc_submission_disable to only destroy doorbell
+ * and not free/unmap client vmas, this will be fixed.
+ */
ret = intel_uc_suspend(dev_priv);
if (ret)
goto out_unlock;
@@ -2113,6 +2120,13 @@ void i915_gem_runtime_resume(struct drm_i915_private *dev_priv)
i915_gem_init_swizzling(dev_priv);
i915_gem_restore_fences(dev_priv);
+ /*
+ * TODO: Note that currently GuC submission enable/disable will
+ * lead to GuC execbuf client vma allocation/free that
+ * in turn involves RPM get/put hence this path will hang.
+ * Once we update guc_submission_enable to only acquire doorbell
+ * and not allocate/map client vmas, this will be fixed.
+ */
intel_uc_runtime_resume(dev_priv);
mutex_unlock(&dev_priv->drm.struct_mutex);
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 50+ messages in thread* [PATCH v13 16/21] drm/i915: Enable interrupts prior to GEM resume during i915_drm_resume
2017-10-11 8:53 [PATCH v13 00/21] drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring Sagar Arun Kamble
` (14 preceding siblings ...)
2017-10-11 8:54 ` [PATCH v13 15/21] drm/i915/guc: Add comment about update needed in GuC submission enable/disable for RPM Sagar Arun Kamble
@ 2017-10-11 8:54 ` Sagar Arun Kamble
2017-10-11 8:54 ` [PATCH v13 17/21] drm/i915: Split i915_gem_suspend into gem quiescing and HW suspend Sagar Arun Kamble
` (5 subsequent siblings)
21 siblings, 0 replies; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-11 8:54 UTC (permalink / raw)
To: intel-gfx
With uC resume now happening in i915_gem_resume we need to enable/install
the i915 interrupts first as we were enabling GuC interrupts earlier.
Interrupt configuration update by enable_guc_interrupts will take effect
with this patch.
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: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_drv.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 78aa90c..8ed3e57 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1681,14 +1681,6 @@ static int i915_drm_resume(struct drm_device *dev)
intel_csr_ucode_resume(dev_priv);
- i915_gem_resume(dev_priv);
-
- i915_restore_state(dev_priv);
- intel_pps_unlock_regs_wa(dev_priv);
- intel_opregion_setup(dev_priv);
-
- intel_init_pch_refclk(dev_priv);
-
/*
* Interrupts have to be enabled before any batches are run. If not the
* GPU will hang. i915_gem_init_hw() will initiate batches to
@@ -1701,6 +1693,14 @@ static int i915_drm_resume(struct drm_device *dev)
*/
intel_runtime_pm_enable_interrupts(dev_priv);
+ i915_gem_resume(dev_priv);
+
+ i915_restore_state(dev_priv);
+ intel_pps_unlock_regs_wa(dev_priv);
+ intel_opregion_setup(dev_priv);
+
+ intel_init_pch_refclk(dev_priv);
+
drm_mode_config_reset(dev);
mutex_lock(&dev->struct_mutex);
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 50+ messages in thread* [PATCH v13 17/21] drm/i915: Split i915_gem_suspend into gem quiescing and HW suspend
2017-10-11 8:53 [PATCH v13 00/21] drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring Sagar Arun Kamble
` (15 preceding siblings ...)
2017-10-11 8:54 ` [PATCH v13 16/21] drm/i915: Enable interrupts prior to GEM resume during i915_drm_resume Sagar Arun Kamble
@ 2017-10-11 8:54 ` Sagar Arun Kamble
2017-10-11 8:54 ` [PATCH v13 18/21] drm/i915/uc: Introduce intel_uc_sanitize to initialize GuC/HuC reset state Sagar Arun Kamble
` (4 subsequent siblings)
21 siblings, 0 replies; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-11 8:54 UTC (permalink / raw)
To: intel-gfx
GTT mappings are to be suspended post idling GPU and display suspend.
Currently GPU was reset in i915_gem_suspend and then GTT mappings were
suspended in i915_drm_suspend. Also part of i915_gem_suspend was about
suspending HW operations. This separated the HW/GEM suspend across reset.
To achieve the complete GEM/HW suspend prior to GPU reset we need to
separate i915_gem_suspend functionality into gem quiescing and HW
suspend. With this patch new function i915_gem_quiesce is created that
will ensure GEM is idle. i915_gem_hw_suspend will ensure all GPU HW
operations are suspended. These currently are RPS, GTT mappings
suspension.
With this patch drm suspend sequence is gem_quiesce followed by
gem_hw_suspend. Unload sequence is gem_quiesce->gem_hw_suspend->gem_fini.
v2. Split i915_gem_suspend to accommodate GTT mappings suspend as that is
not to be done till display suspend. (Chris)
v3: s/i915_gem_quiescent/i915_gem_quiesce and s/i915_gem_suspend/
i915_gem_hw_suspend. gem_hw_suspend has to be done prior to gem_fini as
engine state/GuC software state would be needed to suspend HW.
s/dev_priv/i915 in i915_gem_quiesce and i915_gem_hw_suspend.
Doing GuC suspend before suspending GTT mappings as GuC needs to access
the shared data. Limiting struct_mutex lock to only gem_sanitize.
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_drv.c | 11 +++++++----
drivers/gpu/drm/i915/i915_drv.h | 3 ++-
drivers/gpu/drm/i915/i915_gem.c | 29 ++++++++++++++++++++++-------
3 files changed, 31 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 8ed3e57..198e774e 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -693,8 +693,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
return 0;
cleanup_gem:
- if (i915_gem_suspend(dev_priv))
+ if (i915_gem_quiesce(dev_priv))
DRM_ERROR("failed to idle hardware; continuing to unload!\n");
+ i915_gem_hw_suspend(dev_priv);
i915_gem_fini(dev_priv);
cleanup_uc:
intel_uc_fini_fw(dev_priv);
@@ -1395,9 +1396,11 @@ void i915_driver_unload(struct drm_device *dev)
i915_driver_unregister(dev_priv);
- if (i915_gem_suspend(dev_priv))
+ if (i915_gem_quiesce(dev_priv))
DRM_ERROR("failed to idle hardware; continuing to unload!\n");
+ i915_gem_hw_suspend(dev_priv);
+
intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
drm_atomic_helper_shutdown(dev);
@@ -1540,7 +1543,7 @@ static int i915_drm_suspend(struct drm_device *dev)
pci_save_state(pdev);
- error = i915_gem_suspend(dev_priv);
+ error = i915_gem_quiesce(dev_priv);
if (error) {
dev_err(&pdev->dev,
"GEM idle failed, resume might fail\n");
@@ -1558,7 +1561,7 @@ static int i915_drm_suspend(struct drm_device *dev)
intel_suspend_hw(dev_priv);
- i915_gem_suspend_gtt_mappings(dev_priv);
+ i915_gem_hw_suspend(dev_priv);
i915_save_state(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bc21da2..f51abc6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3716,7 +3716,8 @@ void i915_gem_reset_engine(struct intel_engine_cs *engine,
void i915_gem_cleanup_engines(struct drm_i915_private *dev_priv);
int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv,
unsigned int flags);
-int __must_check i915_gem_suspend(struct drm_i915_private *dev_priv);
+int __must_check i915_gem_quiesce(struct drm_i915_private *dev_priv);
+void i915_gem_hw_suspend(struct drm_i915_private *dev_priv);
void i915_gem_resume(struct drm_i915_private *dev_priv);
int i915_gem_fault(struct vm_fault *vmf);
int i915_gem_object_wait(struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index bcf3ca5..f1a785a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4650,13 +4650,12 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
}
}
-int i915_gem_suspend(struct drm_i915_private *dev_priv)
+int i915_gem_quiesce(struct drm_i915_private *dev_priv)
{
struct drm_device *dev = &dev_priv->drm;
int ret;
intel_runtime_pm_get(dev_priv);
- intel_suspend_gt_powersave(dev_priv);
mutex_lock(&dev->struct_mutex);
@@ -4703,6 +4702,26 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
goto err_unlock;
mutex_unlock(&dev->struct_mutex);
+ intel_runtime_pm_put(dev_priv);
+ return 0;
+
+err_unlock:
+ mutex_unlock(&dev->struct_mutex);
+ intel_runtime_pm_put(dev_priv);
+ return ret;
+}
+
+void i915_gem_hw_suspend(struct drm_i915_private *dev_priv)
+{
+ struct drm_device *dev = &dev_priv->drm;
+
+ intel_runtime_pm_get(dev_priv);
+ intel_suspend_gt_powersave(dev_priv);
+
+ mutex_lock(&dev->struct_mutex);
+
+ i915_gem_suspend_gtt_mappings(dev_priv);
+
/*
* Neither the BIOS, ourselves or any other kernel
* expects the system to be in execlists mode on startup,
@@ -4724,13 +4743,9 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
*/
i915_gem_sanitize(dev_priv);
- intel_runtime_pm_put(dev_priv);
- return 0;
-
-err_unlock:
mutex_unlock(&dev->struct_mutex);
+
intel_runtime_pm_put(dev_priv);
- return ret;
}
void i915_gem_resume(struct drm_i915_private *dev_priv)
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 50+ messages in thread* [PATCH v13 18/21] drm/i915/uc: Introduce intel_uc_sanitize to initialize GuC/HuC reset state
2017-10-11 8:53 [PATCH v13 00/21] drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring Sagar Arun Kamble
` (16 preceding siblings ...)
2017-10-11 8:54 ` [PATCH v13 17/21] drm/i915: Split i915_gem_suspend into gem quiescing and HW suspend Sagar Arun Kamble
@ 2017-10-11 8:54 ` Sagar Arun Kamble
2017-10-11 17:30 ` Michal Wajdeczko
2017-10-11 8:54 ` [PATCH v13 19/21] drm/i915/guc: Fix enable/disable of GuC GGTT invalidate functions Sagar Arun Kamble
` (3 subsequent siblings)
21 siblings, 1 reply; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-11 8:54 UTC (permalink / raw)
To: intel-gfx
In i915_reset/gem_sanitize, GPU will be reset and driver state about
GuC/HuC load status will be invalid. Hence, we mark both GuC/HuC as not
loaded/NONE.
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_gem.c | 4 ++++
drivers/gpu/drm/i915/intel_uc.c | 18 ++++++++++++++++++
drivers/gpu/drm/i915/intel_uc.h | 1 +
3 files changed, 23 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f1a785a..a4bbf6c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2954,6 +2954,8 @@ int i915_gem_reset_prepare(struct drm_i915_private *dev_priv)
i915_gem_revoke_fences(dev_priv);
+ intel_uc_sanitize(dev_priv);
+
return err;
}
@@ -4636,6 +4638,8 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
mutex_unlock(&i915->drm.struct_mutex);
}
+ intel_uc_sanitize(i915);
+
/*
* If we inherit context state from the BIOS or earlier occupants
* of the GPU, the GPU may be in an inconsistent state when we
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 25acf8f..9010ab5 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -438,3 +438,21 @@ void intel_uc_runtime_resume(struct drm_i915_private *dev_priv)
intel_uc_init_hw(dev_priv);
}
+
+/**
+ * intel_uc_sanitize() - Sanitize uC state.
+ * @dev_priv: i915 device private
+ *
+ * This function marks load_status as FIRMWARE_NONE and sanitizes state of
+ * other GuC tasks.
+ */
+void intel_uc_sanitize(struct drm_i915_private *dev_priv)
+{
+ struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
+ struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
+
+ if (i915_modparams.enable_guc_loading) {
+ guc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
+ huc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
+ }
+}
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index f741ccc..fbae5d8 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -37,5 +37,6 @@
int intel_uc_suspend(struct drm_i915_private *dev_priv);
void intel_uc_resume(struct drm_i915_private *dev_priv);
void intel_uc_runtime_resume(struct drm_i915_private *dev_priv);
+void intel_uc_sanitize(struct drm_i915_private *dev_priv);
#endif
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH v13 18/21] drm/i915/uc: Introduce intel_uc_sanitize to initialize GuC/HuC reset state
2017-10-11 8:54 ` [PATCH v13 18/21] drm/i915/uc: Introduce intel_uc_sanitize to initialize GuC/HuC reset state Sagar Arun Kamble
@ 2017-10-11 17:30 ` Michal Wajdeczko
2017-10-11 17:46 ` Sagar Arun Kamble
0 siblings, 1 reply; 50+ messages in thread
From: Michal Wajdeczko @ 2017-10-11 17:30 UTC (permalink / raw)
To: intel-gfx, Sagar Arun Kamble
On Wed, 11 Oct 2017 10:54:13 +0200, Sagar Arun Kamble
<sagar.a.kamble@intel.com> wrote:
> In i915_reset/gem_sanitize, GPU will be reset and driver state about
> GuC/HuC load status will be invalid. Hence, we mark both GuC/HuC as not
> loaded/NONE.
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 4 ++++
> drivers/gpu/drm/i915/intel_uc.c | 18 ++++++++++++++++++
> drivers/gpu/drm/i915/intel_uc.h | 1 +
> 3 files changed, 23 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c
> index f1a785a..a4bbf6c 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2954,6 +2954,8 @@ int i915_gem_reset_prepare(struct drm_i915_private
> *dev_priv)
> i915_gem_revoke_fences(dev_priv);
> + intel_uc_sanitize(dev_priv);
> +
> return err;
> }
> @@ -4636,6 +4638,8 @@ void i915_gem_sanitize(struct drm_i915_private
> *i915)
> mutex_unlock(&i915->drm.struct_mutex);
> }
> + intel_uc_sanitize(i915);
> +
> /*
> * If we inherit context state from the BIOS or earlier occupants
> * of the GPU, the GPU may be in an inconsistent state when we
> diff --git a/drivers/gpu/drm/i915/intel_uc.c
> b/drivers/gpu/drm/i915/intel_uc.c
> index 25acf8f..9010ab5 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -438,3 +438,21 @@ void intel_uc_runtime_resume(struct
> drm_i915_private *dev_priv)
> intel_uc_init_hw(dev_priv);
> }
> +
> +/**
> + * intel_uc_sanitize() - Sanitize uC state.
> + * @dev_priv: i915 device private
> + *
> + * This function marks load_status as FIRMWARE_NONE and sanitizes state
> of
> + * other GuC tasks.
> + */
> +void intel_uc_sanitize(struct drm_i915_private *dev_priv)
> +{
> + struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
> + struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
nitpick:
struct intel_guc *guc = &dev_priv->guc;
struct intel_huc *huc = &dev_priv->huc;
> +
> + if (i915_modparams.enable_guc_loading) {
> + guc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
> + huc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
> + }
Maybe we should move this patch before 13/21 to better handle
"skip_load_on_resume" case ?
> +}
> diff --git a/drivers/gpu/drm/i915/intel_uc.h
> b/drivers/gpu/drm/i915/intel_uc.h
> index f741ccc..fbae5d8 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -37,5 +37,6 @@
> int intel_uc_suspend(struct drm_i915_private *dev_priv);
> void intel_uc_resume(struct drm_i915_private *dev_priv);
> void intel_uc_runtime_resume(struct drm_i915_private *dev_priv);
> +void intel_uc_sanitize(struct drm_i915_private *dev_priv);
> #endif
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v13 18/21] drm/i915/uc: Introduce intel_uc_sanitize to initialize GuC/HuC reset state
2017-10-11 17:30 ` Michal Wajdeczko
@ 2017-10-11 17:46 ` Sagar Arun Kamble
0 siblings, 0 replies; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-11 17:46 UTC (permalink / raw)
To: Michal Wajdeczko, intel-gfx
On 10/11/2017 11:00 PM, Michal Wajdeczko wrote:
> On Wed, 11 Oct 2017 10:54:13 +0200, Sagar Arun Kamble
> <sagar.a.kamble@intel.com> wrote:
>
>> In i915_reset/gem_sanitize, GPU will be reset and driver state about
>> GuC/HuC load status will be invalid. Hence, we mark both GuC/HuC as not
>> loaded/NONE.
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_gem.c | 4 ++++
>> drivers/gpu/drm/i915/intel_uc.c | 18 ++++++++++++++++++
>> drivers/gpu/drm/i915/intel_uc.h | 1 +
>> 3 files changed, 23 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index f1a785a..a4bbf6c 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2954,6 +2954,8 @@ int i915_gem_reset_prepare(struct
>> drm_i915_private *dev_priv)
>> i915_gem_revoke_fences(dev_priv);
>> + intel_uc_sanitize(dev_priv);
>> +
>> return err;
>> }
>> @@ -4636,6 +4638,8 @@ void i915_gem_sanitize(struct drm_i915_private
>> *i915)
>> mutex_unlock(&i915->drm.struct_mutex);
>> }
>> + intel_uc_sanitize(i915);
>> +
>> /*
>> * If we inherit context state from the BIOS or earlier occupants
>> * of the GPU, the GPU may be in an inconsistent state when we
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 25acf8f..9010ab5 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -438,3 +438,21 @@ void intel_uc_runtime_resume(struct
>> drm_i915_private *dev_priv)
>> intel_uc_init_hw(dev_priv);
>> }
>> +
>> +/**
>> + * intel_uc_sanitize() - Sanitize uC state.
>> + * @dev_priv: i915 device private
>> + *
>> + * This function marks load_status as FIRMWARE_NONE and sanitizes
>> state of
>> + * other GuC tasks.
>> + */
>> +void intel_uc_sanitize(struct drm_i915_private *dev_priv)
>> +{
>> + struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
>> + struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
>
> nitpick:
>
> struct intel_guc *guc = &dev_priv->guc;
> struct intel_huc *huc = &dev_priv->huc;
Sure. will update uc_fw through these struct pointers.
>
>> +
>> + if (i915_modparams.enable_guc_loading) {
>> + guc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
>> + huc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
>> + }
>
> Maybe we should move this patch before 13/21 to better handle
> "skip_load_on_resume" case ?
yes. in v14 i have moved this patch earlier.
>
>> +}
>> diff --git a/drivers/gpu/drm/i915/intel_uc.h
>> b/drivers/gpu/drm/i915/intel_uc.h
>> index f741ccc..fbae5d8 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.h
>> +++ b/drivers/gpu/drm/i915/intel_uc.h
>> @@ -37,5 +37,6 @@
>> int intel_uc_suspend(struct drm_i915_private *dev_priv);
>> void intel_uc_resume(struct drm_i915_private *dev_priv);
>> void intel_uc_runtime_resume(struct drm_i915_private *dev_priv);
>> +void intel_uc_sanitize(struct drm_i915_private *dev_priv);
>> #endif
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v13 19/21] drm/i915/guc: Fix enable/disable of GuC GGTT invalidate functions
2017-10-11 8:53 [PATCH v13 00/21] drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring Sagar Arun Kamble
` (17 preceding siblings ...)
2017-10-11 8:54 ` [PATCH v13 18/21] drm/i915/uc: Introduce intel_uc_sanitize to initialize GuC/HuC reset state Sagar Arun Kamble
@ 2017-10-11 8:54 ` Sagar Arun Kamble
2017-10-11 17:35 ` Michal Wajdeczko
2017-10-11 8:54 ` [PATCH v13 20/21] drm/i915/guc: Disable GuC submission/interrupts/communication in intel_uc_sanitize Sagar Arun Kamble
` (2 subsequent siblings)
21 siblings, 1 reply; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-11 8:54 UTC (permalink / raw)
To: intel-gfx
i915_ggtt_enable_guc has to happen first during i915_gem_resume
if GuC loading is enabled before GTT restore. In case GuC is not
loaded this enabling happening during intel_uc_init_hw need to
skipped. (avoid the GEM_BUG_ON)
i915_ggtt_disable_guc at the end of reset/suspend/unload is needed
post GGTT suspend operations. Calling it during uc_sanitize covers
all scenarios. Hence, it is removed from intel_uc_fini_hw. Also these
needto be protected by struct_mutex. Hence struct_mutex locking is
added in i915_gem_sanitize while sanitizing uC. struct_mutex is already
held during i915_gem_reset_prepare.
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/i915_gem.c | 4 ++++
drivers/gpu/drm/i915/intel_uc.c | 16 +++++++++++-----
2 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a4bbf6c..77a0746 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4759,6 +4759,10 @@ void i915_gem_resume(struct drm_i915_private *dev_priv)
WARN_ON(dev_priv->gt.awake);
mutex_lock(&dev->struct_mutex);
+ /* We need to notify the guc whenever we change the GGTT */
+ if (i915_modparams.enable_guc_loading)
+ i915_ggtt_enable_guc(dev_priv);
+
i915_gem_restore_gtt_mappings(dev_priv);
i915_gem_restore_fences(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 9010ab5..0b799fe 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -184,8 +184,14 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
guc_disable_communication(guc);
gen9_reset_guc_interrupts(dev_priv);
- /* We need to notify the guc whenever we change the GGTT */
- i915_ggtt_enable_guc(dev_priv);
+ /*
+ * We need to notify the guc whenever we change the GGTT.
+ * During resume from sleep we would have already updated the
+ * GGTT invalidate function for GuC during i915_gem_resume so
+ * we need to skip here. Will enable here on driver load/reset.
+ */
+ if (!guc->suspended)
+ i915_ggtt_enable_guc(dev_priv);
if (i915_modparams.enable_guc_submission) {
/*
@@ -309,9 +315,6 @@ void intel_uc_cleanup(struct drm_i915_private *dev_priv)
guc_free_load_err_log(guc);
i915_guc_submission_cleanup(dev_priv);
-
- if (i915_modparams.enable_guc_loading)
- i915_ggtt_disable_guc(dev_priv);
}
/**
@@ -452,6 +455,9 @@ void intel_uc_sanitize(struct drm_i915_private *dev_priv)
struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
if (i915_modparams.enable_guc_loading) {
+ if (guc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS)
+ i915_ggtt_disable_guc(dev_priv);
+
guc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
huc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
}
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH v13 19/21] drm/i915/guc: Fix enable/disable of GuC GGTT invalidate functions
2017-10-11 8:54 ` [PATCH v13 19/21] drm/i915/guc: Fix enable/disable of GuC GGTT invalidate functions Sagar Arun Kamble
@ 2017-10-11 17:35 ` Michal Wajdeczko
2017-10-11 17:44 ` Sagar Arun Kamble
0 siblings, 1 reply; 50+ messages in thread
From: Michal Wajdeczko @ 2017-10-11 17:35 UTC (permalink / raw)
To: intel-gfx, Sagar Arun Kamble
On Wed, 11 Oct 2017 10:54:14 +0200, Sagar Arun Kamble
<sagar.a.kamble@intel.com> wrote:
> i915_ggtt_enable_guc has to happen first during i915_gem_resume
> if GuC loading is enabled before GTT restore. In case GuC is not
> loaded this enabling happening during intel_uc_init_hw need to
> skipped. (avoid the GEM_BUG_ON)
> i915_ggtt_disable_guc at the end of reset/suspend/unload is needed
> post GGTT suspend operations. Calling it during uc_sanitize covers
> all scenarios. Hence, it is removed from intel_uc_fini_hw. Also these
> needto be protected by struct_mutex. Hence struct_mutex locking is
> added in i915_gem_sanitize while sanitizing uC. struct_mutex is already
> held during i915_gem_reset_prepare.
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 4 ++++
> drivers/gpu/drm/i915/intel_uc.c | 16 +++++++++++-----
> 2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c
> index a4bbf6c..77a0746 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4759,6 +4759,10 @@ void i915_gem_resume(struct drm_i915_private
> *dev_priv)
> WARN_ON(dev_priv->gt.awake);
> mutex_lock(&dev->struct_mutex);
> + /* We need to notify the guc whenever we change the GGTT */
> + if (i915_modparams.enable_guc_loading)
> + i915_ggtt_enable_guc(dev_priv);
> +
> i915_gem_restore_gtt_mappings(dev_priv);
> i915_gem_restore_fences(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_uc.c
> b/drivers/gpu/drm/i915/intel_uc.c
> index 9010ab5..0b799fe 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -184,8 +184,14 @@ int intel_uc_init_hw(struct drm_i915_private
> *dev_priv)
> guc_disable_communication(guc);
> gen9_reset_guc_interrupts(dev_priv);
> - /* We need to notify the guc whenever we change the GGTT */
> - i915_ggtt_enable_guc(dev_priv);
> + /*
> + * We need to notify the guc whenever we change the GGTT.
> + * During resume from sleep we would have already updated the
> + * GGTT invalidate function for GuC during i915_gem_resume so
> + * we need to skip here. Will enable here on driver load/reset.
> + */
> + if (!guc->suspended)
> + i915_ggtt_enable_guc(dev_priv);
> if (i915_modparams.enable_guc_submission) {
> /*
> @@ -309,9 +315,6 @@ void intel_uc_cleanup(struct drm_i915_private
> *dev_priv)
> guc_free_load_err_log(guc);
> i915_guc_submission_cleanup(dev_priv);
> -
> - if (i915_modparams.enable_guc_loading)
> - i915_ggtt_disable_guc(dev_priv);
> }
> /**
> @@ -452,6 +455,9 @@ void intel_uc_sanitize(struct drm_i915_private
> *dev_priv)
> struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
> if (i915_modparams.enable_guc_loading) {
> + if (guc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS)
Hmm, isn't that check redundant ?
> + i915_ggtt_disable_guc(dev_priv);
> +
> guc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
> huc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
> }
Btw, what should we do with "suspended" flag during sanitize ?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v13 19/21] drm/i915/guc: Fix enable/disable of GuC GGTT invalidate functions
2017-10-11 17:35 ` Michal Wajdeczko
@ 2017-10-11 17:44 ` Sagar Arun Kamble
2017-10-11 17:58 ` Michal Wajdeczko
0 siblings, 1 reply; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-11 17:44 UTC (permalink / raw)
To: Michal Wajdeczko, intel-gfx
On 10/11/2017 11:05 PM, Michal Wajdeczko wrote:
> On Wed, 11 Oct 2017 10:54:14 +0200, Sagar Arun Kamble
> <sagar.a.kamble@intel.com> wrote:
>
>> i915_ggtt_enable_guc has to happen first during i915_gem_resume
>> if GuC loading is enabled before GTT restore. In case GuC is not
>> loaded this enabling happening during intel_uc_init_hw need to
>> skipped. (avoid the GEM_BUG_ON)
>> i915_ggtt_disable_guc at the end of reset/suspend/unload is needed
>> post GGTT suspend operations. Calling it during uc_sanitize covers
>> all scenarios. Hence, it is removed from intel_uc_fini_hw. Also these
>> needto be protected by struct_mutex. Hence struct_mutex locking is
>> added in i915_gem_sanitize while sanitizing uC. struct_mutex is already
>> held during i915_gem_reset_prepare.
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_gem.c | 4 ++++
>> drivers/gpu/drm/i915/intel_uc.c | 16 +++++++++++-----
>> 2 files changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index a4bbf6c..77a0746 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -4759,6 +4759,10 @@ void i915_gem_resume(struct drm_i915_private
>> *dev_priv)
>> WARN_ON(dev_priv->gt.awake);
>> mutex_lock(&dev->struct_mutex);
>> + /* We need to notify the guc whenever we change the GGTT */
>> + if (i915_modparams.enable_guc_loading)
>> + i915_ggtt_enable_guc(dev_priv);
>> +
>> i915_gem_restore_gtt_mappings(dev_priv);
>> i915_gem_restore_fences(dev_priv);
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 9010ab5..0b799fe 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -184,8 +184,14 @@ int intel_uc_init_hw(struct drm_i915_private
>> *dev_priv)
>> guc_disable_communication(guc);
>> gen9_reset_guc_interrupts(dev_priv);
>> - /* We need to notify the guc whenever we change the GGTT */
>> - i915_ggtt_enable_guc(dev_priv);
>> + /*
>> + * We need to notify the guc whenever we change the GGTT.
>> + * During resume from sleep we would have already updated the
>> + * GGTT invalidate function for GuC during i915_gem_resume so
>> + * we need to skip here. Will enable here on driver load/reset.
>> + */
>> + if (!guc->suspended)
>> + i915_ggtt_enable_guc(dev_priv);
>> if (i915_modparams.enable_guc_submission) {
>> /*
>> @@ -309,9 +315,6 @@ void intel_uc_cleanup(struct drm_i915_private
>> *dev_priv)
>> guc_free_load_err_log(guc);
>> i915_guc_submission_cleanup(dev_priv);
>> -
>> - if (i915_modparams.enable_guc_loading)
>> - i915_ggtt_disable_guc(dev_priv);
>> }
>> /**
>> @@ -452,6 +455,9 @@ void intel_uc_sanitize(struct drm_i915_private
>> *dev_priv)
>> struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
>> if (i915_modparams.enable_guc_loading) {
>> + if (guc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS)
>
> Hmm, isn't that check redundant ?
uc_sanitize can happen without firmware loaded too in which case we
don't want to ggtt_disable_guc.
if we want to ggtt_disable_guc then we should remove the GEM_BUG_ON in it.
>
>> + i915_ggtt_disable_guc(dev_priv);
>> +
>> guc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
>> huc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
>> }
>
> Btw, what should we do with "suspended" flag during sanitize ?
suspended flag is set to true on suspend and false on resume.
sanitize is done post suspend and before resume so we should not touch it.
initializing it to false during guc_init_early should take care of
reload (during unload we are suspending gem but we wont resume)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v13 19/21] drm/i915/guc: Fix enable/disable of GuC GGTT invalidate functions
2017-10-11 17:44 ` Sagar Arun Kamble
@ 2017-10-11 17:58 ` Michal Wajdeczko
2017-10-11 18:09 ` Sagar Arun Kamble
0 siblings, 1 reply; 50+ messages in thread
From: Michal Wajdeczko @ 2017-10-11 17:58 UTC (permalink / raw)
To: intel-gfx, Sagar Arun Kamble
On Wed, 11 Oct 2017 19:44:31 +0200, Sagar Arun Kamble
<sagar.a.kamble@intel.com> wrote:
>
>
> On 10/11/2017 11:05 PM, Michal Wajdeczko wrote:
>> On Wed, 11 Oct 2017 10:54:14 +0200, Sagar Arun Kamble
>> <sagar.a.kamble@intel.com> wrote:
>>
>>> i915_ggtt_enable_guc has to happen first during i915_gem_resume
>>> if GuC loading is enabled before GTT restore. In case GuC is not
>>> loaded this enabling happening during intel_uc_init_hw need to
>>> skipped. (avoid the GEM_BUG_ON)
>>> i915_ggtt_disable_guc at the end of reset/suspend/unload is needed
>>> post GGTT suspend operations. Calling it during uc_sanitize covers
>>> all scenarios. Hence, it is removed from intel_uc_fini_hw. Also these
>>> needto be protected by struct_mutex. Hence struct_mutex locking is
>>> added in i915_gem_sanitize while sanitizing uC. struct_mutex is already
>>> held during i915_gem_reset_prepare.
>>>
>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_gem.c | 4 ++++
>>> drivers/gpu/drm/i915/intel_uc.c | 16 +++++++++++-----
>>> 2 files changed, 15 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>>> b/drivers/gpu/drm/i915/i915_gem.c
>>> index a4bbf6c..77a0746 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>> @@ -4759,6 +4759,10 @@ void i915_gem_resume(struct drm_i915_private
>>> *dev_priv)
>>> WARN_ON(dev_priv->gt.awake);
>>> mutex_lock(&dev->struct_mutex);
>>> + /* We need to notify the guc whenever we change the GGTT */
>>> + if (i915_modparams.enable_guc_loading)
>>> + i915_ggtt_enable_guc(dev_priv);
>>> +
>>> i915_gem_restore_gtt_mappings(dev_priv);
>>> i915_gem_restore_fences(dev_priv);
>>> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>>> b/drivers/gpu/drm/i915/intel_uc.c
>>> index 9010ab5..0b799fe 100644
>>> --- a/drivers/gpu/drm/i915/intel_uc.c
>>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>>> @@ -184,8 +184,14 @@ int intel_uc_init_hw(struct drm_i915_private
>>> *dev_priv)
>>> guc_disable_communication(guc);
>>> gen9_reset_guc_interrupts(dev_priv);
>>> - /* We need to notify the guc whenever we change the GGTT */
>>> - i915_ggtt_enable_guc(dev_priv);
>>> + /*
>>> + * We need to notify the guc whenever we change the GGTT.
>>> + * During resume from sleep we would have already updated the
>>> + * GGTT invalidate function for GuC during i915_gem_resume so
>>> + * we need to skip here. Will enable here on driver load/reset.
>>> + */
>>> + if (!guc->suspended)
>>> + i915_ggtt_enable_guc(dev_priv);
>>> if (i915_modparams.enable_guc_submission) {
>>> /*
>>> @@ -309,9 +315,6 @@ void intel_uc_cleanup(struct drm_i915_private
>>> *dev_priv)
>>> guc_free_load_err_log(guc);
>>> i915_guc_submission_cleanup(dev_priv);
>>> -
>>> - if (i915_modparams.enable_guc_loading)
>>> - i915_ggtt_disable_guc(dev_priv);
>>> }
>>> /**
>>> @@ -452,6 +455,9 @@ void intel_uc_sanitize(struct drm_i915_private
>>> *dev_priv)
>>> struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
>>> if (i915_modparams.enable_guc_loading) {
>>> + if (guc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS)
>>
>> Hmm, isn't that check redundant ?
> uc_sanitize can happen without firmware loaded too in which case we
If uc_sanitize can be loaded without firmware loaded, then I assume
i915_modparams.enable_guc_loading will be cleared too, right ?
I'm just wondering if we need to check both modparam and fw status.
> don't want to ggtt_disable_guc.
> if we want to ggtt_disable_guc then we should remove the GEM_BUG_ON in
> it.
Hmm, this is the way how we run CI tests ;)
>>
>>> + i915_ggtt_disable_guc(dev_priv);
>>> +
>>> guc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
>>> huc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
>>> }
>>
>> Btw, what should we do with "suspended" flag during sanitize ?
> suspended flag is set to true on suspend and false on resume.
> sanitize is done post suspend and before resume so we should not touch
> it.
> initializing it to false during guc_init_early should take care of
> reload (during unload we are suspending gem but we wont resume)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v13 19/21] drm/i915/guc: Fix enable/disable of GuC GGTT invalidate functions
2017-10-11 17:58 ` Michal Wajdeczko
@ 2017-10-11 18:09 ` Sagar Arun Kamble
2017-10-11 18:20 ` Michal Wajdeczko
0 siblings, 1 reply; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-11 18:09 UTC (permalink / raw)
To: Michal Wajdeczko, intel-gfx
On 10/11/2017 11:28 PM, Michal Wajdeczko wrote:
> On Wed, 11 Oct 2017 19:44:31 +0200, Sagar Arun Kamble
> <sagar.a.kamble@intel.com> wrote:
>
>>
>>
>> On 10/11/2017 11:05 PM, Michal Wajdeczko wrote:
>>> On Wed, 11 Oct 2017 10:54:14 +0200, Sagar Arun Kamble
>>> <sagar.a.kamble@intel.com> wrote:
>>>
>>>> i915_ggtt_enable_guc has to happen first during i915_gem_resume
>>>> if GuC loading is enabled before GTT restore. In case GuC is not
>>>> loaded this enabling happening during intel_uc_init_hw need to
>>>> skipped. (avoid the GEM_BUG_ON)
>>>> i915_ggtt_disable_guc at the end of reset/suspend/unload is needed
>>>> post GGTT suspend operations. Calling it during uc_sanitize covers
>>>> all scenarios. Hence, it is removed from intel_uc_fini_hw. Also these
>>>> needto be protected by struct_mutex. Hence struct_mutex locking is
>>>> added in i915_gem_sanitize while sanitizing uC. struct_mutex is
>>>> already
>>>> held during i915_gem_reset_prepare.
>>>>
>>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>>> ---
>>>> drivers/gpu/drm/i915/i915_gem.c | 4 ++++
>>>> drivers/gpu/drm/i915/intel_uc.c | 16 +++++++++++-----
>>>> 2 files changed, 15 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>>>> b/drivers/gpu/drm/i915/i915_gem.c
>>>> index a4bbf6c..77a0746 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>>> @@ -4759,6 +4759,10 @@ void i915_gem_resume(struct drm_i915_private
>>>> *dev_priv)
>>>> WARN_ON(dev_priv->gt.awake);
>>>> mutex_lock(&dev->struct_mutex);
>>>> + /* We need to notify the guc whenever we change the GGTT */
>>>> + if (i915_modparams.enable_guc_loading)
>>>> + i915_ggtt_enable_guc(dev_priv);
>>>> +
>>>> i915_gem_restore_gtt_mappings(dev_priv);
>>>> i915_gem_restore_fences(dev_priv);
>>>> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>>>> b/drivers/gpu/drm/i915/intel_uc.c
>>>> index 9010ab5..0b799fe 100644
>>>> --- a/drivers/gpu/drm/i915/intel_uc.c
>>>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>>>> @@ -184,8 +184,14 @@ int intel_uc_init_hw(struct drm_i915_private
>>>> *dev_priv)
>>>> guc_disable_communication(guc);
>>>> gen9_reset_guc_interrupts(dev_priv);
>>>> - /* We need to notify the guc whenever we change the GGTT */
>>>> - i915_ggtt_enable_guc(dev_priv);
>>>> + /*
>>>> + * We need to notify the guc whenever we change the GGTT.
>>>> + * During resume from sleep we would have already updated the
>>>> + * GGTT invalidate function for GuC during i915_gem_resume so
>>>> + * we need to skip here. Will enable here on driver load/reset.
>>>> + */
>>>> + if (!guc->suspended)
>>>> + i915_ggtt_enable_guc(dev_priv);
>>>> if (i915_modparams.enable_guc_submission) {
>>>> /*
>>>> @@ -309,9 +315,6 @@ void intel_uc_cleanup(struct drm_i915_private
>>>> *dev_priv)
>>>> guc_free_load_err_log(guc);
>>>> i915_guc_submission_cleanup(dev_priv);
>>>> -
>>>> - if (i915_modparams.enable_guc_loading)
>>>> - i915_ggtt_disable_guc(dev_priv);
>>>> }
>>>> /**
>>>> @@ -452,6 +455,9 @@ void intel_uc_sanitize(struct drm_i915_private
>>>> *dev_priv)
>>>> struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
>>>> if (i915_modparams.enable_guc_loading) {
>>>> + if (guc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS)
>>>
>>> Hmm, isn't that check redundant ?
>> uc_sanitize can happen without firmware loaded too in which case we
>
> If uc_sanitize can be loaded without firmware loaded, then I assume
> i915_modparams.enable_guc_loading will be cleared too, right ?
>
> I'm just wondering if we need to check both modparam and fw status.
actually load time uc_sanitize is happening before uc_sanitize_options
so enable_guc_loading will have
non-zero value for some platforms. So I think it makes sense to limit
this to only load_status based.
>
>> don't want to ggtt_disable_guc.
>> if we want to ggtt_disable_guc then we should remove the GEM_BUG_ON
>> in it.
>
> Hmm, this is the way how we run CI tests ;)
yes ... from long time :)
>
>>>
>>>> + i915_ggtt_disable_guc(dev_priv);
>>>> +
>>>> guc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
>>>> huc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
>>>> }
>>>
>>> Btw, what should we do with "suspended" flag during sanitize ?
>> suspended flag is set to true on suspend and false on resume.
>> sanitize is done post suspend and before resume so we should not
>> touch it.
>> initializing it to false during guc_init_early should take care of
>> reload (during unload we are suspending gem but we wont resume)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v13 19/21] drm/i915/guc: Fix enable/disable of GuC GGTT invalidate functions
2017-10-11 18:09 ` Sagar Arun Kamble
@ 2017-10-11 18:20 ` Michal Wajdeczko
2017-10-12 9:08 ` Joonas Lahtinen
0 siblings, 1 reply; 50+ messages in thread
From: Michal Wajdeczko @ 2017-10-11 18:20 UTC (permalink / raw)
To: intel-gfx, Sagar Arun Kamble
On Wed, 11 Oct 2017 20:09:10 +0200, Sagar Arun Kamble
<sagar.a.kamble@intel.com> wrote:
>
>
> On 10/11/2017 11:28 PM, Michal Wajdeczko wrote:
>> On Wed, 11 Oct 2017 19:44:31 +0200, Sagar Arun Kamble
>> <sagar.a.kamble@intel.com> wrote:
>>
>>>
>>>
>>> On 10/11/2017 11:05 PM, Michal Wajdeczko wrote:
>>>> On Wed, 11 Oct 2017 10:54:14 +0200, Sagar Arun Kamble
>>>> <sagar.a.kamble@intel.com> wrote:
>>>>
>>>>> i915_ggtt_enable_guc has to happen first during i915_gem_resume
>>>>> if GuC loading is enabled before GTT restore. In case GuC is not
>>>>> loaded this enabling happening during intel_uc_init_hw need to
>>>>> skipped. (avoid the GEM_BUG_ON)
>>>>> i915_ggtt_disable_guc at the end of reset/suspend/unload is needed
>>>>> post GGTT suspend operations. Calling it during uc_sanitize covers
>>>>> all scenarios. Hence, it is removed from intel_uc_fini_hw. Also these
>>>>> needto be protected by struct_mutex. Hence struct_mutex locking is
>>>>> added in i915_gem_sanitize while sanitizing uC. struct_mutex is
>>>>> already
>>>>> held during i915_gem_reset_prepare.
>>>>>
>>>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>>>> ---
>>>>> drivers/gpu/drm/i915/i915_gem.c | 4 ++++
>>>>> drivers/gpu/drm/i915/intel_uc.c | 16 +++++++++++-----
>>>>> 2 files changed, 15 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>>>>> b/drivers/gpu/drm/i915/i915_gem.c
>>>>> index a4bbf6c..77a0746 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>>>> @@ -4759,6 +4759,10 @@ void i915_gem_resume(struct drm_i915_private
>>>>> *dev_priv)
>>>>> WARN_ON(dev_priv->gt.awake);
>>>>> mutex_lock(&dev->struct_mutex);
>>>>> + /* We need to notify the guc whenever we change the GGTT */
>>>>> + if (i915_modparams.enable_guc_loading)
>>>>> + i915_ggtt_enable_guc(dev_priv);
>>>>> +
>>>>> i915_gem_restore_gtt_mappings(dev_priv);
>>>>> i915_gem_restore_fences(dev_priv);
>>>>> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>>>>> b/drivers/gpu/drm/i915/intel_uc.c
>>>>> index 9010ab5..0b799fe 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_uc.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>>>>> @@ -184,8 +184,14 @@ int intel_uc_init_hw(struct drm_i915_private
>>>>> *dev_priv)
>>>>> guc_disable_communication(guc);
>>>>> gen9_reset_guc_interrupts(dev_priv);
>>>>> - /* We need to notify the guc whenever we change the GGTT */
>>>>> - i915_ggtt_enable_guc(dev_priv);
>>>>> + /*
>>>>> + * We need to notify the guc whenever we change the GGTT.
>>>>> + * During resume from sleep we would have already updated the
>>>>> + * GGTT invalidate function for GuC during i915_gem_resume so
>>>>> + * we need to skip here. Will enable here on driver load/reset.
>>>>> + */
>>>>> + if (!guc->suspended)
>>>>> + i915_ggtt_enable_guc(dev_priv);
>>>>> if (i915_modparams.enable_guc_submission) {
>>>>> /*
>>>>> @@ -309,9 +315,6 @@ void intel_uc_cleanup(struct drm_i915_private
>>>>> *dev_priv)
>>>>> guc_free_load_err_log(guc);
>>>>> i915_guc_submission_cleanup(dev_priv);
>>>>> -
>>>>> - if (i915_modparams.enable_guc_loading)
>>>>> - i915_ggtt_disable_guc(dev_priv);
>>>>> }
>>>>> /**
>>>>> @@ -452,6 +455,9 @@ void intel_uc_sanitize(struct drm_i915_private
>>>>> *dev_priv)
>>>>> struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
>>>>> if (i915_modparams.enable_guc_loading) {
>>>>> + if (guc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS)
>>>>
>>>> Hmm, isn't that check redundant ?
>>> uc_sanitize can happen without firmware loaded too in which case we
>>
>> If uc_sanitize can be loaded without firmware loaded, then I assume
>> i915_modparams.enable_guc_loading will be cleared too, right ?
>>
>> I'm just wondering if we need to check both modparam and fw status.
> actually load time uc_sanitize is happening before uc_sanitize_options
Hmm, so maybe we should call intel_sanitize_options() from or right after
i915_driver_init_early() ? It looks that all 'sanitize-options' are using
only device info flags, there is no MMIO access. Chris/Joonas?
> so enable_guc_loading will have
> non-zero value for some platforms. So I think it makes sense to limit
> this to only load_status based.
>>
>>> don't want to ggtt_disable_guc.
>>> if we want to ggtt_disable_guc then we should remove the GEM_BUG_ON in
>>> it.
>>
>> Hmm, this is the way how we run CI tests ;)
> yes ... from long time :)
>>
>>>>
>>>>> + i915_ggtt_disable_guc(dev_priv);
>>>>> +
>>>>> guc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
>>>>> huc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
>>>>> }
>>>>
>>>> Btw, what should we do with "suspended" flag during sanitize ?
>>> suspended flag is set to true on suspend and false on resume.
>>> sanitize is done post suspend and before resume so we should not touch
>>> it.
>>> initializing it to false during guc_init_early should take care of
>>> reload (during unload we are suspending gem but we wont resume)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v13 19/21] drm/i915/guc: Fix enable/disable of GuC GGTT invalidate functions
2017-10-11 18:20 ` Michal Wajdeczko
@ 2017-10-12 9:08 ` Joonas Lahtinen
2017-10-12 12:08 ` Sagar Arun Kamble
0 siblings, 1 reply; 50+ messages in thread
From: Joonas Lahtinen @ 2017-10-12 9:08 UTC (permalink / raw)
To: Michal Wajdeczko, intel-gfx, Sagar Arun Kamble
On Wed, 2017-10-11 at 20:20 +0200, Michal Wajdeczko wrote:
> On Wed, 11 Oct 2017 20:09:10 +0200, Sagar Arun Kamble
> <sagar.a.kamble@intel.com> wrote:
>
> >
> >
> > On 10/11/2017 11:28 PM, Michal Wajdeczko wrote:
> > > On Wed, 11 Oct 2017 19:44:31 +0200, Sagar Arun Kamble
> > > <sagar.a.kamble@intel.com> wrote:
> > >
> > > >
> > > >
> > > > On 10/11/2017 11:05 PM, Michal Wajdeczko wrote:
> > > > > On Wed, 11 Oct 2017 10:54:14 +0200, Sagar Arun Kamble
> > > > > <sagar.a.kamble@intel.com> wrote:
> > > > >
> > > > > > i915_ggtt_enable_guc has to happen first during i915_gem_resume
> > > > > > if GuC loading is enabled before GTT restore. In case GuC is not
> > > > > > loaded this enabling happening during intel_uc_init_hw need to
> > > > > > skipped. (avoid the GEM_BUG_ON)
> > > > > > i915_ggtt_disable_guc at the end of reset/suspend/unload is needed
> > > > > > post GGTT suspend operations. Calling it during uc_sanitize covers
> > > > > > all scenarios. Hence, it is removed from intel_uc_fini_hw. Also these
> > > > > > needto be protected by struct_mutex. Hence struct_mutex locking is
> > > > > > added in i915_gem_sanitize while sanitizing uC. struct_mutex is
> > > > > > already
> > > > > > held during i915_gem_reset_prepare.
> > > > > >
> > > > > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> > > > > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > > > > Cc: Michał Winiarski <michal.winiarski@intel.com>
> > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/i915_gem.c | 4 ++++
> > > > > > drivers/gpu/drm/i915/intel_uc.c | 16 +++++++++++-----
> > > > > > 2 files changed, 15 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > > > > > b/drivers/gpu/drm/i915/i915_gem.c
> > > > > > index a4bbf6c..77a0746 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > > > > @@ -4759,6 +4759,10 @@ void i915_gem_resume(struct drm_i915_private
> > > > > > *dev_priv)
> > > > > > WARN_ON(dev_priv->gt.awake);
> > > > > > mutex_lock(&dev->struct_mutex);
> > > > > > + /* We need to notify the guc whenever we change the GGTT */
> > > > > > + if (i915_modparams.enable_guc_loading)
> > > > > > + i915_ggtt_enable_guc(dev_priv);
> > > > > > +
> > > > > > i915_gem_restore_gtt_mappings(dev_priv);
> > > > > > i915_gem_restore_fences(dev_priv);
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_uc.c
> > > > > > b/drivers/gpu/drm/i915/intel_uc.c
> > > > > > index 9010ab5..0b799fe 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_uc.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > > > > > @@ -184,8 +184,14 @@ int intel_uc_init_hw(struct drm_i915_private
> > > > > > *dev_priv)
> > > > > > guc_disable_communication(guc);
> > > > > > gen9_reset_guc_interrupts(dev_priv);
> > > > > > - /* We need to notify the guc whenever we change the GGTT */
> > > > > > - i915_ggtt_enable_guc(dev_priv);
> > > > > > + /*
> > > > > > + * We need to notify the guc whenever we change the GGTT.
> > > > > > + * During resume from sleep we would have already updated the
> > > > > > + * GGTT invalidate function for GuC during i915_gem_resume so
> > > > > > + * we need to skip here. Will enable here on driver load/reset.
> > > > > > + */
> > > > > > + if (!guc->suspended)
> > > > > > + i915_ggtt_enable_guc(dev_priv);
> > > > > > if (i915_modparams.enable_guc_submission) {
> > > > > > /*
> > > > > > @@ -309,9 +315,6 @@ void intel_uc_cleanup(struct drm_i915_private
> > > > > > *dev_priv)
> > > > > > guc_free_load_err_log(guc);
> > > > > > i915_guc_submission_cleanup(dev_priv);
> > > > > > -
> > > > > > - if (i915_modparams.enable_guc_loading)
> > > > > > - i915_ggtt_disable_guc(dev_priv);
> > > > > > }
> > > > > > /**
> > > > > > @@ -452,6 +455,9 @@ void intel_uc_sanitize(struct drm_i915_private
> > > > > > *dev_priv)
> > > > > > struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
> > > > > > if (i915_modparams.enable_guc_loading) {
> > > > > > + if (guc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS)
> > > > >
> > > > > Hmm, isn't that check redundant ?
> > > >
> > > > uc_sanitize can happen without firmware loaded too in which case we
> > >
> > > If uc_sanitize can be loaded without firmware loaded, then I assume
> > > i915_modparams.enable_guc_loading will be cleared too, right ?
> > >
> > > I'm just wondering if we need to check both modparam and fw status.
> >
> > actually load time uc_sanitize is happening before uc_sanitize_options
>
> Hmm, so maybe we should call intel_sanitize_options() from or right after
> i915_driver_init_early() ? It looks that all 'sanitize-options' are using
> only device info flags, there is no MMIO access. Chris/Joonas?
Will be hard to answer any question on patch 19/21, when the preceding
patches are still under debate.
What I meant with focusing on single series at a time is that we first
discuss on any changes to get the first patches merged in reasonable
sized chunks of few patches per merge. During that discussion, lets
hold the dependent series from the mailing list for a while, because
the existing patches will cause changes and invalidate the reviews.
Now I have multiple dozens of e-mails in my inbox, and we're not going
to make progress that way. My understanding is that Michal's patches on
better organizing the GuC code are the first ones, so lets focus on
those first. So lets get back to this series after we're done with the
code organization sanitization.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH v13 19/21] drm/i915/guc: Fix enable/disable of GuC GGTT invalidate functions
2017-10-12 9:08 ` Joonas Lahtinen
@ 2017-10-12 12:08 ` Sagar Arun Kamble
0 siblings, 0 replies; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-12 12:08 UTC (permalink / raw)
To: Joonas Lahtinen, Michal Wajdeczko, intel-gfx
On 10/12/2017 2:38 PM, Joonas Lahtinen wrote:
> On Wed, 2017-10-11 at 20:20 +0200, Michal Wajdeczko wrote:
>> On Wed, 11 Oct 2017 20:09:10 +0200, Sagar Arun Kamble
>> <sagar.a.kamble@intel.com> wrote:
>>
>>>
>>> On 10/11/2017 11:28 PM, Michal Wajdeczko wrote:
>>>> On Wed, 11 Oct 2017 19:44:31 +0200, Sagar Arun Kamble
>>>> <sagar.a.kamble@intel.com> wrote:
>>>>
>>>>>
>>>>> On 10/11/2017 11:05 PM, Michal Wajdeczko wrote:
>>>>>> On Wed, 11 Oct 2017 10:54:14 +0200, Sagar Arun Kamble
>>>>>> <sagar.a.kamble@intel.com> wrote:
>>>>>>
>>>>>>> i915_ggtt_enable_guc has to happen first during i915_gem_resume
>>>>>>> if GuC loading is enabled before GTT restore. In case GuC is not
>>>>>>> loaded this enabling happening during intel_uc_init_hw need to
>>>>>>> skipped. (avoid the GEM_BUG_ON)
>>>>>>> i915_ggtt_disable_guc at the end of reset/suspend/unload is needed
>>>>>>> post GGTT suspend operations. Calling it during uc_sanitize covers
>>>>>>> all scenarios. Hence, it is removed from intel_uc_fini_hw. Also these
>>>>>>> needto be protected by struct_mutex. Hence struct_mutex locking is
>>>>>>> added in i915_gem_sanitize while sanitizing uC. struct_mutex is
>>>>>>> already
>>>>>>> held during i915_gem_reset_prepare.
>>>>>>>
>>>>>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>>>>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>>>>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>>>>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>>>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>>>>>> ---
>>>>>>> drivers/gpu/drm/i915/i915_gem.c | 4 ++++
>>>>>>> drivers/gpu/drm/i915/intel_uc.c | 16 +++++++++++-----
>>>>>>> 2 files changed, 15 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>>>>>>> b/drivers/gpu/drm/i915/i915_gem.c
>>>>>>> index a4bbf6c..77a0746 100644
>>>>>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>>>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>>>>>> @@ -4759,6 +4759,10 @@ void i915_gem_resume(struct drm_i915_private
>>>>>>> *dev_priv)
>>>>>>> WARN_ON(dev_priv->gt.awake);
>>>>>>> mutex_lock(&dev->struct_mutex);
>>>>>>> + /* We need to notify the guc whenever we change the GGTT */
>>>>>>> + if (i915_modparams.enable_guc_loading)
>>>>>>> + i915_ggtt_enable_guc(dev_priv);
>>>>>>> +
>>>>>>> i915_gem_restore_gtt_mappings(dev_priv);
>>>>>>> i915_gem_restore_fences(dev_priv);
>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_uc.c
>>>>>>> b/drivers/gpu/drm/i915/intel_uc.c
>>>>>>> index 9010ab5..0b799fe 100644
>>>>>>> --- a/drivers/gpu/drm/i915/intel_uc.c
>>>>>>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>>>>>>> @@ -184,8 +184,14 @@ int intel_uc_init_hw(struct drm_i915_private
>>>>>>> *dev_priv)
>>>>>>> guc_disable_communication(guc);
>>>>>>> gen9_reset_guc_interrupts(dev_priv);
>>>>>>> - /* We need to notify the guc whenever we change the GGTT */
>>>>>>> - i915_ggtt_enable_guc(dev_priv);
>>>>>>> + /*
>>>>>>> + * We need to notify the guc whenever we change the GGTT.
>>>>>>> + * During resume from sleep we would have already updated the
>>>>>>> + * GGTT invalidate function for GuC during i915_gem_resume so
>>>>>>> + * we need to skip here. Will enable here on driver load/reset.
>>>>>>> + */
>>>>>>> + if (!guc->suspended)
>>>>>>> + i915_ggtt_enable_guc(dev_priv);
>>>>>>> if (i915_modparams.enable_guc_submission) {
>>>>>>> /*
>>>>>>> @@ -309,9 +315,6 @@ void intel_uc_cleanup(struct drm_i915_private
>>>>>>> *dev_priv)
>>>>>>> guc_free_load_err_log(guc);
>>>>>>> i915_guc_submission_cleanup(dev_priv);
>>>>>>> -
>>>>>>> - if (i915_modparams.enable_guc_loading)
>>>>>>> - i915_ggtt_disable_guc(dev_priv);
>>>>>>> }
>>>>>>> /**
>>>>>>> @@ -452,6 +455,9 @@ void intel_uc_sanitize(struct drm_i915_private
>>>>>>> *dev_priv)
>>>>>>> struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
>>>>>>> if (i915_modparams.enable_guc_loading) {
>>>>>>> + if (guc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS)
>>>>>> Hmm, isn't that check redundant ?
>>>>> uc_sanitize can happen without firmware loaded too in which case we
>>>> If uc_sanitize can be loaded without firmware loaded, then I assume
>>>> i915_modparams.enable_guc_loading will be cleared too, right ?
>>>>
>>>> I'm just wondering if we need to check both modparam and fw status.
>>> actually load time uc_sanitize is happening before uc_sanitize_options
>> Hmm, so maybe we should call intel_sanitize_options() from or right after
>> i915_driver_init_early() ? It looks that all 'sanitize-options' are using
>> only device info flags, there is no MMIO access. Chris/Joonas?
> Will be hard to answer any question on patch 19/21, when the preceding
> patches are still under debate.
>
> What I meant with focusing on single series at a time is that we first
> discuss on any changes to get the first patches merged in reasonable
> sized chunks of few patches per merge. During that discussion, lets
> hold the dependent series from the mailing list for a while, because
> the existing patches will cause changes and invalidate the reviews.
>
> Now I have multiple dozens of e-mails in my inbox, and we're not going
> to make progress that way.
Sorry. I thought the first reorg series that was merged last time was
the only gate for GEM/GuC fixes series.
There are not a lot conflicts with current reorg series. But I now
understand that I should have held onto the new revision.
Will wait for these changes to go in.
> My understanding is that Michal's patches on
> better organizing the GuC code are the first ones, so lets focus on
> those first. So lets get back to this series after we're done with the
> code organization sanitization.
>
> Regards, Joonas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH v13 20/21] drm/i915/guc: Disable GuC submission/interrupts/communication in intel_uc_sanitize
2017-10-11 8:53 [PATCH v13 00/21] drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring Sagar Arun Kamble
` (18 preceding siblings ...)
2017-10-11 8:54 ` [PATCH v13 19/21] drm/i915/guc: Fix enable/disable of GuC GGTT invalidate functions Sagar Arun Kamble
@ 2017-10-11 8:54 ` Sagar Arun Kamble
2017-10-11 8:54 ` [PATCH v13 21/21] HAX enable GuC submission for CI Sagar Arun Kamble
2017-10-11 9:44 ` ✗ Fi.CI.BAT: failure for drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring Patchwork
21 siblings, 0 replies; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-11 8:54 UTC (permalink / raw)
To: intel-gfx
GuC submission, Interrupts and GuC communication are set up during
intel_uc_init_hw. Keeping it ON during GPU reset might cause issues.
To achieve uC sanitization w.r.t these functions prior to reset,
disable these during intel_uc_sanitize.
Also submission/interrupts are to be enabled only after sanitizing/
disabling with the state based checks protecting double enable/disable.
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
drivers/gpu/drm/i915/intel_uc.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 0b799fe..00b54d0 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -453,10 +453,16 @@ void intel_uc_sanitize(struct drm_i915_private *dev_priv)
{
struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
+ struct intel_guc *guc = &dev_priv->guc;
if (i915_modparams.enable_guc_loading) {
- if (guc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS)
+ if (guc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS) {
+ i915_guc_submission_disable(dev_priv);
+ gen9_disable_guc_interrupts(dev_priv);
+ guc_disable_communication(guc);
+
i915_ggtt_disable_guc(dev_priv);
+ }
guc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
huc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 50+ messages in thread* [PATCH v13 21/21] HAX enable GuC submission for CI
2017-10-11 8:53 [PATCH v13 00/21] drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring Sagar Arun Kamble
` (19 preceding siblings ...)
2017-10-11 8:54 ` [PATCH v13 20/21] drm/i915/guc: Disable GuC submission/interrupts/communication in intel_uc_sanitize Sagar Arun Kamble
@ 2017-10-11 8:54 ` Sagar Arun Kamble
2017-10-11 9:44 ` ✗ Fi.CI.BAT: failure for drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring Patchwork
21 siblings, 0 replies; 50+ messages in thread
From: Sagar Arun Kamble @ 2017-10-11 8:54 UTC (permalink / raw)
To: intel-gfx
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
drivers/gpu/drm/i915/i915_params.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index c729226..c38cef0 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -44,8 +44,8 @@
param(int, disable_power_well, -1) \
param(int, enable_ips, 1) \
param(int, invert_brightness, 0) \
- param(int, enable_guc_loading, 0) \
- param(int, enable_guc_submission, 0) \
+ param(int, enable_guc_loading, 1) \
+ param(int, enable_guc_submission, 1) \
param(int, guc_log_level, -1) \
param(char *, guc_firmware_path, NULL) \
param(char *, huc_firmware_path, NULL) \
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 50+ messages in thread* ✗ Fi.CI.BAT: failure for drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring
2017-10-11 8:53 [PATCH v13 00/21] drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring Sagar Arun Kamble
` (20 preceding siblings ...)
2017-10-11 8:54 ` [PATCH v13 21/21] HAX enable GuC submission for CI Sagar Arun Kamble
@ 2017-10-11 9:44 ` Patchwork
21 siblings, 0 replies; 50+ messages in thread
From: Patchwork @ 2017-10-11 9:44 UTC (permalink / raw)
To: Sagar Arun Kamble; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring
URL : https://patchwork.freedesktop.org/series/31721/
State : failure
== Summary ==
Series 31721v1 drm/i915: GEM/GuC Suspend/Resume/Reset fixes and restructuring
https://patchwork.freedesktop.org/api/1.0/series/31721/revisions/1/mbox/
Test gem_exec_suspend:
Subgroup basic-s3:
pass -> DMESG-WARN (fi-kbl-7560u) fdo#103039
pass -> DMESG-WARN (fi-kbl-r) fdo#102846 +6
dmesg-warn -> PASS (fi-cfl-s) fdo#103186
Subgroup basic-s4-devices:
pass -> DMESG-WARN (fi-skl-6260u)
pass -> DMESG-WARN (fi-skl-6700hq)
pass -> DMESG-WARN (fi-skl-6700k)
pass -> DMESG-WARN (fi-skl-6770hq)
pass -> DMESG-WARN (fi-skl-gvtdvm)
pass -> DMESG-WARN (fi-bxt-dsi)
pass -> DMESG-WARN (fi-bxt-j4205)
pass -> DMESG-WARN (fi-kbl-7500u)
pass -> DMESG-WARN (fi-cfl-s)
Test kms_pipe_crc_basic:
Subgroup nonblocking-crc-pipe-a-frame-sequence:
dmesg-fail -> PASS (fi-cfl-s) fdo#102996
Subgroup nonblocking-crc-pipe-b:
incomplete -> PASS (fi-cfl-s) fdo#103022
Subgroup suspend-read-crc-pipe-a:
pass -> DMESG-WARN (fi-kbl-7560u)
pass -> DMESG-WARN (fi-kbl-r)
Test pm_rpm:
Subgroup basic-pci-d3-state:
pass -> INCOMPLETE (fi-byt-j1900)
pass -> INCOMPLETE (fi-byt-n2820)
pass -> INCOMPLETE (fi-hsw-4770)
pass -> INCOMPLETE (fi-bdw-5557u)
pass -> INCOMPLETE (fi-bsw-n3050)
pass -> INCOMPLETE (fi-skl-6260u)
pass -> INCOMPLETE (fi-skl-6700hq)
pass -> INCOMPLETE (fi-skl-6700k)
pass -> INCOMPLETE (fi-skl-6770hq)
pass -> INCOMPLETE (fi-bxt-dsi)
pass -> INCOMPLETE (fi-bxt-j4205)
pass -> INCOMPLETE (fi-kbl-7500u)
pass -> FAIL (fi-kbl-7560u)
pass -> INCOMPLETE (fi-kbl-7567u)
pass -> FAIL (fi-kbl-r)
pass -> INCOMPLETE (fi-glk-1)
notrun -> INCOMPLETE (fi-cfl-s) fdo#103169
Subgroup basic-rte:
pass -> FAIL (fi-kbl-7560u)
pass -> FAIL (fi-kbl-r)
Test drv_module_reload:
Subgroup basic-reload:
pass -> DMESG-WARN (fi-kbl-7560u)
pass -> DMESG-WARN (fi-kbl-r)
fdo#103039 https://bugs.freedesktop.org/show_bug.cgi?id=103039
fdo#102846 https://bugs.freedesktop.org/show_bug.cgi?id=102846
fdo#103186 https://bugs.freedesktop.org/show_bug.cgi?id=103186
fdo#102996 https://bugs.freedesktop.org/show_bug.cgi?id=102996
fdo#103022 https://bugs.freedesktop.org/show_bug.cgi?id=103022
fdo#103169 https://bugs.freedesktop.org/show_bug.cgi?id=103169
fi-bdw-5557u total:252 pass:231 dwarn:0 dfail:0 fail:0 skip:20
fi-bdw-gvtdvm total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:469s
fi-blb-e6850 total:289 pass:223 dwarn:1 dfail:0 fail:0 skip:65 time:392s
fi-bsw-n3050 total:252 pass:207 dwarn:0 dfail:0 fail:0 skip:44
fi-bwr-2160 total:289 pass:183 dwarn:0 dfail:0 fail:0 skip:106 time:284s
fi-bxt-dsi total:252 pass:222 dwarn:1 dfail:0 fail:0 skip:28
fi-bxt-j4205 total:252 pass:222 dwarn:1 dfail:0 fail:0 skip:28
fi-byt-j1900 total:252 pass:216 dwarn:1 dfail:0 fail:0 skip:34
fi-byt-n2820 total:252 pass:212 dwarn:1 dfail:0 fail:0 skip:38
fi-cfl-s total:252 pass:219 dwarn:1 dfail:0 fail:0 skip:31
fi-elk-e7500 total:289 pass:229 dwarn:0 dfail:0 fail:0 skip:60 time:427s
fi-glk-1 total:252 pass:225 dwarn:0 dfail:0 fail:0 skip:26
fi-hsw-4770 total:252 pass:225 dwarn:0 dfail:0 fail:0 skip:26
fi-ilk-650 total:289 pass:228 dwarn:0 dfail:0 fail:0 skip:61 time:459s
fi-ivb-3520m total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:506s
fi-ivb-3770 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:478s
fi-kbl-7500u total:252 pass:226 dwarn:2 dfail:0 fail:0 skip:23
fi-kbl-7560u total:289 pass:262 dwarn:3 dfail:3 fail:2 skip:19 time:548s
fi-kbl-7567u total:252 pass:231 dwarn:1 dfail:0 fail:0 skip:19
fi-kbl-r total:289 pass:254 dwarn:3 dfail:3 fail:2 skip:27 time:561s
fi-pnv-d510 total:289 pass:222 dwarn:1 dfail:0 fail:0 skip:66 time:654s
fi-skl-6260u total:252 pass:231 dwarn:1 dfail:0 fail:0 skip:19
fi-skl-6700hq total:252 pass:226 dwarn:1 dfail:0 fail:0 skip:24
fi-skl-6700k total:252 pass:227 dwarn:1 dfail:0 fail:0 skip:23
fi-skl-6770hq total:252 pass:231 dwarn:1 dfail:0 fail:0 skip:19
fi-skl-gvtdvm total:289 pass:265 dwarn:1 dfail:0 fail:0 skip:23 time:475s
fi-snb-2520m total:289 pass:250 dwarn:0 dfail:0 fail:0 skip:39 time:584s
fi-snb-2600 total:289 pass:249 dwarn:0 dfail:0 fail:0 skip:40 time:431s
fi-cnl-y failed to connect after reboot
2dcc40d169e600cfa63887260901c74f943e9bf5 drm-tip: 2017y-10m-11d-08h-54m-27s UTC integration manifest
9558f74f1aae HAX enable GuC submission for CI
b83fefe84f11 drm/i915/guc: Disable GuC submission/interrupts/communication in intel_uc_sanitize
8cfc9223e8cf drm/i915/guc: Fix enable/disable of GuC GGTT invalidate functions
053b6fea9f25 drm/i915/uc: Introduce intel_uc_sanitize to initialize GuC/HuC reset state
62baa53c0d2e drm/i915: Split i915_gem_suspend into gem quiescing and HW suspend
916bd8a4de72 drm/i915: Enable interrupts prior to GEM resume during i915_drm_resume
d7dd130dfdb0 drm/i915/guc: Add comment about update needed in GuC submission enable/disable for RPM
ff4574a2b01c drm/i915/uc: Update GEM runtime resume with need for reload of GuC/HuC
894d5d2ab3db drm/i915/uc: Support resume from sleep w/ and w/o GuC/HuC reload
9c69e194a8ee drm/i915/guc: Fix GuC related state cleanup in unload path
87cf0d35b5fa drm/i915/guc: Remove GuC submission disable from i915_driver_unload
74e5627a24db drm/i915/guc: Update uC suspend/resume function separating Host/GuC tasks
456e6ea249da drm/i915/uc: Create uC suspend and resume functions
a0b39f68f615 drm/i915/guc: Update GEM suspend/resume flows with GuC suspend/resume functions
02d800d1551f drm/i915: Create GEM runtime resume helper and handle GEM runtime suspend error
f7a7a8ef9243 drm/i915/guc: Pass intel_guc struct parameter to intel_guc_suspend/resume
f6965ffcdb7f drm/i915/guc: Update enable_guc_loading check in intel_uc_fini_hw
adfa9a549f78 drm/i915/guc: Remove enable_guc_submission dependency for invoking GuC log functions
ac69f96a19b1 drm/i915/guc: Add status checks to enable/disable_guc_interrupts
bfe499b7c49a drm/i915/guc: Sanitize module parameter guc_log_level
fb636ee7370d drm/i915/guc: Add GuC submission initialization/enable state variables
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5986/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 50+ messages in thread