[PATCH 1/3] drm/i915/guc: Release GuC interrupts in i915_guc_submission_disable

[PATCH 1/3] drm/i915/guc: Release GuC interrupts in i915_guc_submission_disable

[Sagar Arun Kamble]

Like capture of GuC interrupts while enabling GuC submission, release
them while disabling GuC submission.

Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 23 +++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_guc_loader.c    | 21 ---------------------
 2 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 41f2dd8..01b611c 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -995,10 +995,33 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
 	return 0;
 }
 
+static void guc_interrupts_release(struct drm_i915_private *dev_priv)
+{
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+	int irqs;
+
+	/*
+	 * tell all command streamers NOT to forward interrupts or vblank
+	 * to GuC.
+	 */
+	irqs = _MASKED_FIELD(GFX_FORWARD_VBLANK_MASK, GFX_FORWARD_VBLANK_NEVER);
+	irqs |= _MASKED_BIT_DISABLE(GFX_INTERRUPT_STEERING);
+	for_each_engine(engine, dev_priv, id)
+		I915_WRITE(RING_MODE_GEN7(engine), irqs);
+
+	/* route all GT interrupts to the host */
+	I915_WRITE(GUC_BCS_RCS_IER, 0);
+	I915_WRITE(GUC_VCS2_VCS1_IER, 0);
+	I915_WRITE(GUC_WD_VECS_IER, 0);
+}
+
 void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
 {
 	struct intel_guc *guc = &dev_priv->guc;
 
+	guc_interrupts_release(dev_priv);
+
 	if (!guc->execbuf_client)
 		return;
 
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 2e24712..759ab34 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -91,24 +91,6 @@ const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
 	}
 };
 
-static void guc_interrupts_release(struct drm_i915_private *dev_priv)
-{
-	struct intel_engine_cs *engine;
-	enum intel_engine_id id;
-	int irqs;
-
-	/* tell all command streamers NOT to forward interrupts or vblank to GuC */
-	irqs = _MASKED_FIELD(GFX_FORWARD_VBLANK_MASK, GFX_FORWARD_VBLANK_NEVER);
-	irqs |= _MASKED_BIT_DISABLE(GFX_INTERRUPT_STEERING);
-	for_each_engine(engine, dev_priv, id)
-		I915_WRITE(RING_MODE_GEN7(engine), irqs);
-
-	/* route all GT interrupts to the host */
-	I915_WRITE(GUC_BCS_RCS_IER, 0);
-	I915_WRITE(GUC_VCS2_VCS1_IER, 0);
-	I915_WRITE(GUC_WD_VECS_IER, 0);
-}
-
 static u32 get_gttype(struct drm_i915_private *dev_priv)
 {
 	/* XXX: GT type based on PCI device ID? field seems unused by fw */
@@ -429,7 +411,6 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
 		goto fail;
 	}
 
-	guc_interrupts_release(dev_priv);
 	gen9_reset_guc_interrupts(dev_priv);
 
 	/* We need to notify the guc whenever we change the GGTT */
@@ -496,7 +477,6 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
 	if (guc_fw->load_status == INTEL_UC_FIRMWARE_PENDING)
 		guc_fw->load_status = INTEL_UC_FIRMWARE_FAIL;
 
-	guc_interrupts_release(dev_priv);
 	i915_guc_submission_disable(dev_priv);
 	i915_guc_submission_fini(dev_priv);
 	i915_ggtt_disable_guc(dev_priv);
@@ -746,7 +726,6 @@ void intel_guc_fini(struct drm_i915_private *dev_priv)
 	struct drm_i915_gem_object *obj;
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
-	guc_interrupts_release(dev_priv);
 	i915_guc_submission_disable(dev_priv);
 	i915_guc_submission_fini(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

[PATCH 2/3] drm/i915: s/pm_intr_keep/pm_intrmsk_mbz

[Sagar Arun Kamble]

"pm_intr_keep" is not conveying the intent that it is bitmask
of interrupts that must be zero(mbz) in GEN6_PMINTRMSK.
Name it "pm_intrmsk_mbz".

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c |  3 ++-
 drivers/gpu/drm/i915/i915_drv.h     |  2 +-
 drivers/gpu/drm/i915/i915_irq.c     | 20 ++++++++++----------
 3 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 56674df..82fb005 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1189,7 +1189,8 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
 		}
 		seq_printf(m, "PM IER=0x%08x IMR=0x%08x ISR=0x%08x IIR=0x%08x, MASK=0x%08x\n",
 			   pm_ier, pm_imr, pm_isr, pm_iir, pm_mask);
-		seq_printf(m, "pm_intr_keep: 0x%08x\n", dev_priv->rps.pm_intr_keep);
+		seq_printf(m, "pm_intrmsk_mbz: 0x%08x\n",
+			   dev_priv->rps.pm_intrmsk_mbz);
 		seq_printf(m, "GT_PERF_STATUS: 0x%08x\n", gt_perf_status);
 		seq_printf(m, "Render p-state ratio: %d\n",
 			   (gt_perf_status & (IS_GEN9(dev_priv) ? 0x1ff00 : 0xff00)) >> 8);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3002996..16e0767 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1338,7 +1338,7 @@ struct intel_gen6_power_mgmt {
 	u32 pm_iir;
 
 	/* PM interrupt bits that should never be masked */
-	u32 pm_intr_keep;
+	u32 pm_intrmsk_mbz;
 
 	/* Frequencies are stored in potentially platform dependent multiples.
 	 * In other words, *_freq needs to be multiplied by X to be interesting.
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 22ac703..7a71d4e 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -391,7 +391,7 @@ void gen6_enable_rps_interrupts(struct drm_i915_private *dev_priv)
 
 u32 gen6_sanitize_rps_pm_mask(struct drm_i915_private *dev_priv, u32 mask)
 {
-	return (mask & ~dev_priv->rps.pm_intr_keep);
+	return (mask & ~dev_priv->rps.pm_intrmsk_mbz);
 }
 
 void gen6_disable_rps_interrupts(struct drm_i915_private *dev_priv)
@@ -4287,7 +4287,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 	else
 		dev_priv->pm_rps_events = GEN6_PM_RPS_EVENTS;
 
-	dev_priv->rps.pm_intr_keep = 0;
+	dev_priv->rps.pm_intrmsk_mbz = 0;
 
 	/*
 	 * SNB,IVB can while VLV,CHV may hard hang on looping batchbuffer
@@ -4296,33 +4296,33 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 	 * TODO: verify if this can be reproduced on VLV,CHV.
 	 */
 	if (INTEL_INFO(dev_priv)->gen <= 7 && !IS_HASWELL(dev_priv))
-		dev_priv->rps.pm_intr_keep |= GEN6_PM_RP_UP_EI_EXPIRED;
+		dev_priv->rps.pm_intrmsk_mbz |= GEN6_PM_RP_UP_EI_EXPIRED;
 
 	if (INTEL_INFO(dev_priv)->gen >= 8)
-		dev_priv->rps.pm_intr_keep |= GEN8_PMINTR_REDIRECT_TO_GUC;
+		dev_priv->rps.pm_intrmsk_mbz |= GEN8_PMINTR_REDIRECT_TO_GUC;
 
 	/*
 	 * The REDIRECT_TO_GUC bit of the PMINTRMSK register directs all
 	 * (unmasked) PM interrupts to the GuC. All other bits of this
 	 * register *disable* generation of a specific interrupt.
 	 *
-	 * 'pm_intr_keep' indicates bits that are NOT to be set when
+	 * 'pm_intrmsk_mbz' indicates bits that are NOT to be set when
 	 * writing to the PM interrupt mask register, i.e. interrupts
 	 * that must not be disabled.
 	 *
 	 * If the GuC is handling these interrupts, then we must not let
 	 * the PM code disable ANY interrupt that the GuC is expecting.
 	 * So for each ENABLED (0) bit in this register, we must SET the
-	 * bit in pm_intr_keep so that it's left enabled for the GuC.
+	 * bit in pm_intrmsk_mbz so that it's left enabled for the GuC.
 	 * GuC needs ARAT expired interrupt unmasked hence it is set in
-	 * pm_intr_keep.
+	 * pm_intrmsk_mbz.
 	 *
-	 * Here we CLEAR REDIRECT_TO_GUC bit in pm_intr_keep, which will
+	 * Here we CLEAR REDIRECT_TO_GUC bit in pm_intrmsk_mbz, which will
 	 * result in the register bit being left SET!
 	 */
 	if (HAS_GUC_SCHED(dev_priv)) {
-		dev_priv->rps.pm_intr_keep |= ARAT_EXPIRED_INTRMSK;
-		dev_priv->rps.pm_intr_keep &= ~GEN8_PMINTR_REDIRECT_TO_GUC;
+		dev_priv->rps.pm_intrmsk_mbz |= ARAT_EXPIRED_INTRMSK;
+		dev_priv->rps.pm_intrmsk_mbz &= ~GEN8_PMINTR_REDIRECT_TO_GUC;
 	}
 
 	if (IS_GEN2(dev_priv)) {
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915/guc: Update rps.pm_intrmsk_mbz in guc_interrupts_capture/release

[Sagar Arun Kamble]

Different state is to be maintained for rps.pm_intrmsk_mbz for GuC and
Execlists. Updating it inside guc_interrupts_* routines as in those
routines GuC load/submission params are sanitized and it should not be set
based on HAS_GUC_SCHED during intel_irq_init.

Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 26 ++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_irq.c            | 24 ------------------------
 2 files changed, 26 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 01b611c..ca7723f 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -954,6 +954,28 @@ static void guc_interrupts_capture(struct drm_i915_private *dev_priv)
 	I915_WRITE(GUC_BCS_RCS_IER, ~irqs);
 	I915_WRITE(GUC_VCS2_VCS1_IER, ~irqs);
 	I915_WRITE(GUC_WD_VECS_IER, ~irqs);
+
+	/*
+	 * The REDIRECT_TO_GUC bit of the PMINTRMSK register directs all
+	 * (unmasked) PM interrupts to the GuC. All other bits of this
+	 * register *disable* generation of a specific interrupt.
+	 *
+	 * 'pm_intrmsk_mbz' indicates bits that are NOT to be set when
+	 * writing to the PM interrupt mask register, i.e. interrupts
+	 * that must not be disabled.
+	 *
+	 * If the GuC is handling these interrupts, then we must not let
+	 * the PM code disable ANY interrupt that the GuC is expecting.
+	 * So for each ENABLED (0) bit in this register, we must SET the
+	 * bit in pm_intrmsk_mbz so that it's left enabled for the GuC.
+	 * GuC needs ARAT expired interrupt unmasked hence it is set in
+	 * pm_intrmsk_mbz.
+	 *
+	 * Here we CLEAR REDIRECT_TO_GUC bit in pm_intrmsk_mbz, which will
+	 * result in the register bit being left SET!
+	 */
+	dev_priv->rps.pm_intrmsk_mbz |= ARAT_EXPIRED_INTRMSK;
+	dev_priv->rps.pm_intrmsk_mbz &= ~GEN8_PMINTR_REDIRECT_TO_GUC;
 }
 
 int i915_guc_submission_enable(struct drm_i915_private *dev_priv)
@@ -1014,6 +1036,10 @@ static void guc_interrupts_release(struct drm_i915_private *dev_priv)
 	I915_WRITE(GUC_BCS_RCS_IER, 0);
 	I915_WRITE(GUC_VCS2_VCS1_IER, 0);
 	I915_WRITE(GUC_WD_VECS_IER, 0);
+
+	dev_priv->rps.pm_intrmsk_mbz |= GEN8_PMINTR_REDIRECT_TO_GUC;
+	dev_priv->rps.pm_intrmsk_mbz &= ~ARAT_EXPIRED_INTRMSK;
+
 }
 
 void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 7a71d4e..6577e51 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -4301,30 +4301,6 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 	if (INTEL_INFO(dev_priv)->gen >= 8)
 		dev_priv->rps.pm_intrmsk_mbz |= GEN8_PMINTR_REDIRECT_TO_GUC;
 
-	/*
-	 * The REDIRECT_TO_GUC bit of the PMINTRMSK register directs all
-	 * (unmasked) PM interrupts to the GuC. All other bits of this
-	 * register *disable* generation of a specific interrupt.
-	 *
-	 * 'pm_intrmsk_mbz' indicates bits that are NOT to be set when
-	 * writing to the PM interrupt mask register, i.e. interrupts
-	 * that must not be disabled.
-	 *
-	 * If the GuC is handling these interrupts, then we must not let
-	 * the PM code disable ANY interrupt that the GuC is expecting.
-	 * So for each ENABLED (0) bit in this register, we must SET the
-	 * bit in pm_intrmsk_mbz so that it's left enabled for the GuC.
-	 * GuC needs ARAT expired interrupt unmasked hence it is set in
-	 * pm_intrmsk_mbz.
-	 *
-	 * Here we CLEAR REDIRECT_TO_GUC bit in pm_intrmsk_mbz, which will
-	 * result in the register bit being left SET!
-	 */
-	if (HAS_GUC_SCHED(dev_priv)) {
-		dev_priv->rps.pm_intrmsk_mbz |= ARAT_EXPIRED_INTRMSK;
-		dev_priv->rps.pm_intrmsk_mbz &= ~GEN8_PMINTR_REDIRECT_TO_GUC;
-	}
-
 	if (IS_GEN2(dev_priv)) {
 		/* Gen2 doesn't have a hardware frame counter */
 		dev->max_vblank_count = 0;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/guc: Release GuC interrupts in i915_guc_submission_disable

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915/guc: Release GuC interrupts in i915_guc_submission_disable
URL   : https://patchwork.freedesktop.org/series/21090/
State : success

== Summary ==

Series 21090v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/21090/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-b:
                dmesg-warn -> PASS       (fi-byt-j1900)

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 463s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 608s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 527s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 575s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 503s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 502s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 436s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 430s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 444s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 511s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 492s
fi-kbl-7500u     total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18  time: 474s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 486s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 589s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 491s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 542s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 549s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 423s

2095bbc9d234d71fa44fd9181597431e2653058c drm-tip: 2017y-03m-10d-15h-03m-17s UTC integration manifest
a8a184e drm/i915/guc: Update rps.pm_intrmsk_mbz in guc_interrupts_capture/release
d9379c9 drm/i915: s/pm_intr_keep/pm_intrmsk_mbz
c417696 drm/i915/guc: Release GuC interrupts in i915_guc_submission_disable

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4142/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/guc: Release GuC interrupts in i915_guc_submission_disable

[Chris Wilson]

On Sat, Mar 11, 2017 at 04:17:34AM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [1/3] drm/i915/guc: Release GuC interrupts in i915_guc_submission_disable
> URL   : https://patchwork.freedesktop.org/series/21090/
> State : success
> 
> == Summary ==
> 
> Series 21090v1 Series without cover letter
> https://patchwork.freedesktop.org/api/1.0/series/21090/revisions/1/mbox/
> 
> Test kms_pipe_crc_basic:
>         Subgroup hang-read-crc-pipe-b:
>                 dmesg-warn -> PASS       (fi-byt-j1900)

Applied, thanks for the quick fix. It is looking much neater now as well
:)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/guc: Release GuC interrupts in i915_guc_submission_disable

[Kamble, Sagar A]

On 3/12/2017 6:29 PM, Chris Wilson wrote:
> On Sat, Mar 11, 2017 at 04:17:34AM -0000, Patchwork wrote:
>> == Series Details ==
>>
>> Series: series starting with [1/3] drm/i915/guc: Release GuC interrupts in i915_guc_submission_disable
>> URL   : https://patchwork.freedesktop.org/series/21090/
>> State : success
>>
>> == Summary ==
>>
>> Series 21090v1 Series without cover letter
>> https://patchwork.freedesktop.org/api/1.0/series/21090/revisions/1/mbox/
>>
>> Test kms_pipe_crc_basic:
>>          Subgroup hang-read-crc-pipe-b:
>>                  dmesg-warn -> PASS       (fi-byt-j1900)
> Applied, thanks for the quick fix. It is looking much neater now as well
> :)
> -Chris

Thanks Chris.
I feel unmasking of ARAT_EXPIRED is hard coding the behavior with GuC.
Ideally rps enabling should happen post GuC load in reset path like in load time flow.
That way instead of hard coding interrupts to be kept as ARAT_EXPIRED we will be able to derive from bits unmasked by GuC in PMINTRMSK.
So before GuC load, we should be resetting RPS interrupts (making PMINRMSK=~0u) and then derive interrupts to be kept unmasked by Host.
And then enable RPS. Current state is fine as we know GuC isn't using other PM interrupts. (might use some of those in SLPC)

>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/guc: Release GuC interrupts in i915_guc_submission_disable

[Chris Wilson]

On Mon, Mar 13, 2017 at 10:28:34AM +0530, Kamble, Sagar A wrote:
> 
> 
> On 3/12/2017 6:29 PM, Chris Wilson wrote:
> >On Sat, Mar 11, 2017 at 04:17:34AM -0000, Patchwork wrote:
> >>== Series Details ==
> >>
> >>Series: series starting with [1/3] drm/i915/guc: Release GuC interrupts in i915_guc_submission_disable
> >>URL   : https://patchwork.freedesktop.org/series/21090/
> >>State : success
> >>
> >>== Summary ==
> >>
> >>Series 21090v1 Series without cover letter
> >>https://patchwork.freedesktop.org/api/1.0/series/21090/revisions/1/mbox/
> >>
> >>Test kms_pipe_crc_basic:
> >>         Subgroup hang-read-crc-pipe-b:
> >>                 dmesg-warn -> PASS       (fi-byt-j1900)
> >Applied, thanks for the quick fix. It is looking much neater now as well
> >:)
> >-Chris
> 
> Thanks Chris.
> I feel unmasking of ARAT_EXPIRED is hard coding the behavior with GuC.
> Ideally rps enabling should happen post GuC load in reset path like in load time flow.

The catch though is that we don't go through a rps disable sequence
point across reset. We might be able to do an explict disable/enable
pair now.

> That way instead of hard coding interrupts to be kept as ARAT_EXPIRED we will be able to derive from bits unmasked by GuC in PMINTRMSK.
> So before GuC load, we should be resetting RPS interrupts (making PMINRMSK=~0u) and then derive interrupts to be kept unmasked by Host.
> And then enable RPS. Current state is fine as we know GuC isn't using other PM interrupts. (might use some of those in SLPC)

But the set of bits used by guc will be fixed depending on what mode we
are in, and should already be setup by time we reset. You just have a
slightly more elaborate guc interrupts enable/disable sequence, I don't
see that as making anything simpler or more elegant yet - but anticipate
enlightenment.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/guc: Release GuC interrupts in i915_guc_submission_disable

[Kamble, Sagar A]

On 3/13/2017 3:17 PM, Chris Wilson wrote:
> On Mon, Mar 13, 2017 at 10:28:34AM +0530, Kamble, Sagar A wrote:
>>
>> On 3/12/2017 6:29 PM, Chris Wilson wrote:
>>> On Sat, Mar 11, 2017 at 04:17:34AM -0000, Patchwork wrote:
>>>> == Series Details ==
>>>>
>>>> Series: series starting with [1/3] drm/i915/guc: Release GuC interrupts in i915_guc_submission_disable
>>>> URL   : https://patchwork.freedesktop.org/series/21090/
>>>> State : success
>>>>
>>>> == Summary ==
>>>>
>>>> Series 21090v1 Series without cover letter
>>>> https://patchwork.freedesktop.org/api/1.0/series/21090/revisions/1/mbox/
>>>>
>>>> Test kms_pipe_crc_basic:
>>>>          Subgroup hang-read-crc-pipe-b:
>>>>                  dmesg-warn -> PASS       (fi-byt-j1900)
>>> Applied, thanks for the quick fix. It is looking much neater now as well
>>> :)
>>> -Chris
>> Thanks Chris.
>> I feel unmasking of ARAT_EXPIRED is hard coding the behavior with GuC.
>> Ideally rps enabling should happen post GuC load in reset path like in load time flow.
> The catch though is that we don't go through a rps disable sequence
> point across reset. We might be able to do an explict disable/enable
> pair now.

Yes.

>
>> That way instead of hard coding interrupts to be kept as ARAT_EXPIRED we will be able to derive from bits unmasked by GuC in PMINTRMSK.
>> So before GuC load, we should be resetting RPS interrupts (making PMINRMSK=~0u) and then derive interrupts to be kept unmasked by Host.
>> And then enable RPS. Current state is fine as we know GuC isn't using other PM interrupts. (might use some of those in SLPC)
> But the set of bits used by guc will be fixed depending on what mode we
> are in, and should already be setup by time we reset. You just have a
> slightly more elaborate guc interrupts enable/disable sequence, I don't
> see that as making anything simpler or more elegant yet - but anticipate
> enlightenment.
> -Chris

Agree that bits used will be fixed and no need to dynamically determine.
Other bits if needed will be configured by GuC SLPC and in that case Host
RPS flows will not update the registers. So the current implementation looks fine.

Thanks
Sagar


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx