[PATCH v2 1/2] HAX drm/i915: Enable guc submission

[PATCH v2 1/2] HAX drm/i915: Enable guc submission

[Chris Wilson]

---
 drivers/gpu/drm/i915/i915_params.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 0e280fbd52f1..ef1e9921a2af 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -56,8 +56,8 @@ struct i915_params i915 __read_mostly = {
 	.verbose_state_checks = 1,
 	.nuclear_pageflip = 0,
 	.edp_vswing = 0,
-	.enable_guc_loading = 0,
-	.enable_guc_submission = 0,
+	.enable_guc_loading = -1,
+	.enable_guc_submission = -1,
 	.guc_log_level = -1,
 	.enable_dp_mst = true,
 	.inject_load_failure = 0,
-- 
2.10.2

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

[PATCH v2 2/2] drm/i915: Invalidate the guc ggtt TLB upon insertion

[Chris Wilson]

Move the GuC invalidation of its ggtt TLB to where we perform the ggtt
modification rather than proliferate it into all the callers of the
insert (which may or may not in fact have to do the insertion).

v2: Just do the guc invalidate unconditionally, (afaict) it has no impact
without the guc loaded on gen8+

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch> # v1
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> # v1
---
 drivers/gpu/drm/i915/i915_gem_gtt.c        | 12 ++++++++----
 drivers/gpu/drm/i915/i915_guc_submission.c |  3 ---
 drivers/gpu/drm/i915/intel_guc_loader.c    |  3 ---
 drivers/gpu/drm/i915/intel_lrc.c           |  6 ------
 4 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 02fb063302bf..dccc4d0eef88 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2387,6 +2387,12 @@ static void gen8_set_pte(void __iomem *addr, gen8_pte_t pte)
 	writeq(pte, addr);
 }
 
+static void gen8_ggtt_invalidate(struct drm_i915_private *dev_priv)
+{
+	I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
+	I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
+}
+
 static void gen8_ggtt_insert_page(struct i915_address_space *vm,
 				  dma_addr_t addr,
 				  uint64_t offset,
@@ -2400,8 +2406,7 @@ static void gen8_ggtt_insert_page(struct i915_address_space *vm,
 
 	gen8_set_pte(pte, gen8_pte_encode(addr, level));
 
-	I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
-	POSTING_READ(GFX_FLSH_CNTL_GEN6);
+	gen8_ggtt_invalidate(dev_priv);
 }
 
 static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
@@ -2438,8 +2443,7 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
 	 * want to flush the TLBs only after we're certain all the PTE updates
 	 * have finished.
 	 */
-	I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
-	POSTING_READ(GFX_FLSH_CNTL_GEN6);
+	gen8_ggtt_invalidate(dev_priv);
 }
 
 struct insert_entries {
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 58413803ba3c..ef8c18d61a0f 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -589,9 +589,6 @@ static struct i915_vma *guc_allocate_vma(struct intel_guc *guc, u32 size)
 		goto err;
 	}
 
-	/* Invalidate GuC TLB to let GuC take the latest updates to GTT. */
-	I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
-
 	return vma;
 
 err:
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index a330fa499384..4fb1037faad1 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -366,9 +366,6 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
 		return PTR_ERR(vma);
 	}
 
-	/* Invalidate GuC TLB to let GuC take the latest updates to GTT. */
-	I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
-
 	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
 
 	/* init WOPCM */
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index b2c0d509e191..48fc93dadb40 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -874,12 +874,6 @@ static int intel_lr_context_pin(struct i915_gem_context *ctx,
 
 	ce->state->obj->mm.dirty = true;
 
-	/* Invalidate GuC TLB. */
-	if (i915.enable_guc_submission) {
-		struct drm_i915_private *dev_priv = ctx->i915;
-		I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
-	}
-
 	i915_gem_context_get(ctx);
 	return 0;
 
-- 
2.10.2

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

Re: [PATCH v2 2/2] drm/i915: Invalidate the guc ggtt TLB upon insertion

[Tvrtko Ursulin]

On 01/12/2016 09:46, Chris Wilson wrote:
> Move the GuC invalidation of its ggtt TLB to where we perform the ggtt
> modification rather than proliferate it into all the callers of the
> insert (which may or may not in fact have to do the insertion).
>
> v2: Just do the guc invalidate unconditionally, (afaict) it has no impact
> without the guc loaded on gen8+

Why do you find it tempting to do it unconditionally? I would rather not 
touch it on gen8 at all and would also prefer the conditional flush in gen9.

Regards,

Tvrtko

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> # v1
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> # v1
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c        | 12 ++++++++----
>  drivers/gpu/drm/i915/i915_guc_submission.c |  3 ---
>  drivers/gpu/drm/i915/intel_guc_loader.c    |  3 ---
>  drivers/gpu/drm/i915/intel_lrc.c           |  6 ------
>  4 files changed, 8 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 02fb063302bf..dccc4d0eef88 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2387,6 +2387,12 @@ static void gen8_set_pte(void __iomem *addr, gen8_pte_t pte)
>  	writeq(pte, addr);
>  }
>
> +static void gen8_ggtt_invalidate(struct drm_i915_private *dev_priv)
> +{
> +	I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
> +	I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
> +}
> +
>  static void gen8_ggtt_insert_page(struct i915_address_space *vm,
>  				  dma_addr_t addr,
>  				  uint64_t offset,
> @@ -2400,8 +2406,7 @@ static void gen8_ggtt_insert_page(struct i915_address_space *vm,
>
>  	gen8_set_pte(pte, gen8_pte_encode(addr, level));
>
> -	I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
> -	POSTING_READ(GFX_FLSH_CNTL_GEN6);
> +	gen8_ggtt_invalidate(dev_priv);
>  }
>
>  static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
> @@ -2438,8 +2443,7 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
>  	 * want to flush the TLBs only after we're certain all the PTE updates
>  	 * have finished.
>  	 */
> -	I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
> -	POSTING_READ(GFX_FLSH_CNTL_GEN6);
> +	gen8_ggtt_invalidate(dev_priv);
>  }
>
>  struct insert_entries {
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 58413803ba3c..ef8c18d61a0f 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -589,9 +589,6 @@ static struct i915_vma *guc_allocate_vma(struct intel_guc *guc, u32 size)
>  		goto err;
>  	}
>
> -	/* Invalidate GuC TLB to let GuC take the latest updates to GTT. */
> -	I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
> -
>  	return vma;
>
>  err:
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index a330fa499384..4fb1037faad1 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -366,9 +366,6 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
>  		return PTR_ERR(vma);
>  	}
>
> -	/* Invalidate GuC TLB to let GuC take the latest updates to GTT. */
> -	I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
> -
>  	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>
>  	/* init WOPCM */
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index b2c0d509e191..48fc93dadb40 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -874,12 +874,6 @@ static int intel_lr_context_pin(struct i915_gem_context *ctx,
>
>  	ce->state->obj->mm.dirty = true;
>
> -	/* Invalidate GuC TLB. */
> -	if (i915.enable_guc_submission) {
> -		struct drm_i915_private *dev_priv = ctx->i915;
> -		I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
> -	}
> -
>  	i915_gem_context_get(ctx);
>  	return 0;
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/2] drm/i915: Invalidate the guc ggtt TLB upon insertion

[Chris Wilson]

On Thu, Dec 01, 2016 at 10:06:25AM +0000, Tvrtko Ursulin wrote:
> 
> On 01/12/2016 09:46, Chris Wilson wrote:
> >Move the GuC invalidation of its ggtt TLB to where we perform the ggtt
> >modification rather than proliferate it into all the callers of the
> >insert (which may or may not in fact have to do the insertion).
> >
> >v2: Just do the guc invalidate unconditionally, (afaict) it has no impact
> >without the guc loaded on gen8+
> 
> Why do you find it tempting to do it unconditionally? I would rather
> not touch it on gen8 at all and would also prefer the conditional
> flush in gen9.

Because if I add a conditional here, I end up wanting writing a new vfunc
for invalidate (if I can coax the gmch / gen6 / guc usage into something
consistent). And I'm lazy. :)
-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: [PATCH v2 2/2] drm/i915: Invalidate the guc ggtt TLB upon insertion

[Tvrtko Ursulin]

On 01/12/2016 10:26, Chris Wilson wrote:
> On Thu, Dec 01, 2016 at 10:06:25AM +0000, Tvrtko Ursulin wrote:
>>
>> On 01/12/2016 09:46, Chris Wilson wrote:
>>> Move the GuC invalidation of its ggtt TLB to where we perform the ggtt
>>> modification rather than proliferate it into all the callers of the
>>> insert (which may or may not in fact have to do the insertion).
>>>
>>> v2: Just do the guc invalidate unconditionally, (afaict) it has no impact
>>> without the guc loaded on gen8+
>>
>> Why do you find it tempting to do it unconditionally? I would rather
>> not touch it on gen8 at all and would also prefer the conditional
>> flush in gen9.
>
> Because if I add a conditional here, I end up wanting writing a new vfunc
> for invalidate (if I can coax the gmch / gen6 / guc usage into something
> consistent). And I'm lazy. :)

To make sure I fully understand - just because you would not like to see 
the conditional in gen8_ggtt_invalidate? So you would add 
gen8_ggtt_invalidate and gen9_ggtt_invalidate with a GuC flush?

I would have thought conditional is less bothersome than making the 
unused piece of the GPU (on gen8) do stuff.

Regards,

Tvrtko

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

Re: [PATCH v2 2/2] drm/i915: Invalidate the guc ggtt TLB upon insertion

[Chris Wilson]

On Thu, Dec 01, 2016 at 10:49:31AM +0000, Tvrtko Ursulin wrote:
> 
> On 01/12/2016 10:26, Chris Wilson wrote:
> >On Thu, Dec 01, 2016 at 10:06:25AM +0000, Tvrtko Ursulin wrote:
> >>
> >>On 01/12/2016 09:46, Chris Wilson wrote:
> >>>Move the GuC invalidation of its ggtt TLB to where we perform the ggtt
> >>>modification rather than proliferate it into all the callers of the
> >>>insert (which may or may not in fact have to do the insertion).
> >>>
> >>>v2: Just do the guc invalidate unconditionally, (afaict) it has no impact
> >>>without the guc loaded on gen8+
> >>
> >>Why do you find it tempting to do it unconditionally? I would rather
> >>not touch it on gen8 at all and would also prefer the conditional
> >>flush in gen9.
> >
> >Because if I add a conditional here, I end up wanting writing a new vfunc
> >for invalidate (if I can coax the gmch / gen6 / guc usage into something
> >consistent). And I'm lazy. :)
> 
> To make sure I fully understand - just because you would not like to
> see the conditional in gen8_ggtt_invalidate? So you would add
> gen8_ggtt_invalidate and gen9_ggtt_invalidate with a GuC flush?

gen9_guc_ggtt_invalidate. Because I don't like the conditional in
i915_ggtt_flush() - and that shows we have another place missing the guc
invalidate. And also because at one point, I was toying with
the idea of pushing the flush out to the caller of ->insert_page() in
case they wanted to manipulate multiple pages. (However, that looks
like it will remain single pages only.) I
 
> I would have thought conditional is less bothersome than making the
> unused piece of the GPU (on gen8) do stuff.

I thought you wouldn't want to punch a bit on the register if it wasn't
being used at all.
-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: [PATCH v2 2/2] drm/i915: Invalidate the guc ggtt TLB upon insertion

[Tvrtko Ursulin]

On 01/12/2016 10:57, Chris Wilson wrote:
> On Thu, Dec 01, 2016 at 10:49:31AM +0000, Tvrtko Ursulin wrote:
>>
>> On 01/12/2016 10:26, Chris Wilson wrote:
>>> On Thu, Dec 01, 2016 at 10:06:25AM +0000, Tvrtko Ursulin wrote:
>>>>
>>>> On 01/12/2016 09:46, Chris Wilson wrote:
>>>>> Move the GuC invalidation of its ggtt TLB to where we perform the ggtt
>>>>> modification rather than proliferate it into all the callers of the
>>>>> insert (which may or may not in fact have to do the insertion).
>>>>>
>>>>> v2: Just do the guc invalidate unconditionally, (afaict) it has no impact
>>>>> without the guc loaded on gen8+
>>>>
>>>> Why do you find it tempting to do it unconditionally? I would rather
>>>> not touch it on gen8 at all and would also prefer the conditional
>>>> flush in gen9.
>>>
>>> Because if I add a conditional here, I end up wanting writing a new vfunc
>>> for invalidate (if I can coax the gmch / gen6 / guc usage into something
>>> consistent). And I'm lazy. :)
>>
>> To make sure I fully understand - just because you would not like to
>> see the conditional in gen8_ggtt_invalidate? So you would add
>> gen8_ggtt_invalidate and gen9_ggtt_invalidate with a GuC flush?
>
> gen9_guc_ggtt_invalidate. Because I don't like the conditional in
> i915_ggtt_flush() - and that shows we have another place missing the guc
> invalidate. And also because at one point, I was toying with
> the idea of pushing the flush out to the caller of ->insert_page() in
> case they wanted to manipulate multiple pages. (However, that looks
> like it will remain single pages only.) I
>
>> I would have thought conditional is less bothersome than making the
>> unused piece of the GPU (on gen8) do stuff.
>
> I thought you wouldn't want to punch a bit on the register if it wasn't
> being used at all.

Yes I wouldn't, and not only unused but not validated probably.

Regards,

Tvrtko
_______________________________________________
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 [v2,1/2] HAX drm/i915: Enable guc submission

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/2] HAX drm/i915: Enable guc submission
URL   : https://patchwork.freedesktop.org/series/16210/
State : success

== Summary ==

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


fi-bdw-5557u     total:245  pass:230  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:245  pass:205  dwarn:0   dfail:0   fail:0   skip:40 
fi-byt-j1900     total:245  pass:217  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:245  pass:213  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:245  pass:225  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:245  pass:225  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650       total:245  pass:192  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:245  pass:223  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:245  pass:223  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7500u     total:245  pass:223  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:245  pass:231  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:245  pass:224  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:245  pass:223  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:245  pass:231  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:245  pass:213  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:245  pass:212  dwarn:0   dfail:0   fail:0   skip:33 

ae97fd2e43ca3693dbd127f51b9bba1c2dd1d37d drm-tip: 2016y-12m-01d-10h-10m-46s UTC integration manifest
73d6e51 drm/i915: Invalidate the guc ggtt TLB upon insertion
c268ef0 HAX drm/i915: Enable guc submission

== Logs ==

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