[PATCH] drm/i915/guc: Make sure vma containing firmware is GuC mappable

[PATCH] drm/i915/guc: Make sure vma containing firmware is GuC mappable

[Michał Winiarski]

Since commit 4741da925fa3 ("drm/i915/guc: Assert that all GGTT offsets used
by the GuC are mappable"), we're asserting that GuC firmware is in the
GuC mappable range.
Except we're not pinning the object with bias, which means it's possible
to trigger this assert. Let's add a proper bias.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_loader.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index aa2b866..5a6ab87 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -360,7 +360,8 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
 		return ret;
 	}
 
-	vma = i915_gem_object_ggtt_pin(guc_fw->guc_fw_obj, NULL, 0, 0, 0);
+	vma = i915_gem_object_ggtt_pin(guc_fw->guc_fw_obj, NULL, 0, 0,
+				       PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
 	if (IS_ERR(vma)) {
 		DRM_DEBUG_DRIVER("pin failed %d\n", (int)PTR_ERR(vma));
 		return PTR_ERR(vma);
-- 
2.9.3

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

Re: [PATCH] drm/i915/guc: Make sure vma containing firmware is GuC mappable

[Chris Wilson]

On Wed, Jan 11, 2017 at 04:17:39PM +0100, Michał Winiarski wrote:
> Since commit 4741da925fa3 ("drm/i915/guc: Assert that all GGTT offsets used
> by the GuC are mappable"), we're asserting that GuC firmware is in the
> GuC mappable range.
> Except we're not pinning the object with bias, which means it's possible
> to trigger this assert. Let's add a proper bias.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>

Fits in with the checks we added. If they are correct, so is this fix ;)
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-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

✗ Fi.CI.BAT: warning for drm/i915/guc: Make sure vma containing firmware is GuC mappable

[Patchwork]

== Series Details ==

Series: drm/i915/guc: Make sure vma containing firmware is GuC mappable
URL   : https://patchwork.freedesktop.org/series/17836/
State : warning

== Summary ==

Series 17836v1 drm/i915/guc: Make sure vma containing firmware is GuC mappable
https://patchwork.freedesktop.org/api/1.0/series/17836/revisions/1/mbox/

Test kms_force_connector_basic:
        Subgroup force-edid:
                pass       -> DMESG-WARN (fi-snb-2520m)

fi-bdw-5557u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:246  pass:207  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-t5700     total:82   pass:69   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-ivb-3520m     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:246  pass:226  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:246  pass:222  dwarn:3   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:246  pass:214  dwarn:1   dfail:0   fail:0   skip:31 
fi-snb-2600      total:246  pass:214  dwarn:0   dfail:0   fail:0   skip:32 

b69fc4c941bef6d10750ce3f07daedfffc7017d1 drm-tip: 2017y-01m-11d-17h-30m-02s UTC integration manifest
e43f2d8 drm/i915/guc: Make sure vma containing firmware is GuC mappable

== Logs ==

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

Re: [PATCH] drm/i915/guc: Make sure vma containing firmware is GuC mappable

[Daniele Ceraolo Spurio]

On 11/01/17 07:17, Michał Winiarski wrote:
> Since commit 4741da925fa3 ("drm/i915/guc: Assert that all GGTT offsets used
> by the GuC are mappable"), we're asserting that GuC firmware is in the
> GuC mappable range.
> Except we're not pinning the object with bias, which means it's possible
> to trigger this assert. Let's add a proper bias.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_loader.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index aa2b866..5a6ab87 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -360,7 +360,8 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
>  		return ret;
>  	}
>
> -	vma = i915_gem_object_ggtt_pin(guc_fw->guc_fw_obj, NULL, 0, 0, 0);
> +	vma = i915_gem_object_ggtt_pin(guc_fw->guc_fw_obj, NULL, 0, 0,
> +				       PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
>  	if (IS_ERR(vma)) {
>  		DRM_DEBUG_DRIVER("pin failed %d\n", (int)PTR_ERR(vma));
>  		return PTR_ERR(vma);
>

This patch made me think about this again and actually I'm not sure 
anymore that there is an offset requirement for the firmware object. 
With the way we load the firmware the GuC should never access it in GGTT 
because it is first copied in WOPCM via DMA, which should be able to 
access the whole address range. I've asked a GuC dev but he has not been 
able to confirm if there are any offset limitation with the DMA transfer 
or not and unfortunately I don't have a platform to test this on at the 
moment. I'll try to get my hands on a new SKL and double check.
Anyway, I'm happy to merge this while we clarify the requirement because 
the firmware vma is immediately unpinned after the transfer so there 
should be no risk of unneeded ggtt fragmentation; it also looks 
generally cleaner to me to handle all guc-related objects the same way.

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

Re: [PATCH] drm/i915/guc: Make sure vma containing firmware is GuC mappable

[Chris Wilson]

On Wed, Jan 11, 2017 at 04:17:39PM +0100, Michał Winiarski wrote:
> Since commit 4741da925fa3 ("drm/i915/guc: Assert that all GGTT offsets used
> by the GuC are mappable"), we're asserting that GuC firmware is in the
> GuC mappable range.
> Except we're not pinning the object with bias, which means it's possible
> to trigger this assert. Let's add a proper bias.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>

My spider sense is tingling....

CI has been reporting a suspend freeze with guc enabled. Tomi?
-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] drm/i915/guc: Make sure vma containing firmware is GuC mappable

[Tomi Sarvela]

On Thursday, 12 January 2017 10:33:47 EET Chris Wilson wrote:
> On Wed, Jan 11, 2017 at 04:17:39PM +0100, Michał Winiarski wrote:
> > Since commit 4741da925fa3 ("drm/i915/guc: Assert that all GGTT
> > offsets used by the GuC are mappable"), we're asserting that GuC
> > firmware is in the GuC mappable range.
> > Except we're not pinning the object with bias, which means it's
> > possible to trigger this assert. Let's add a proper bias.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> 
> My spider sense is tingling....
> 
> CI has been reporting a suspend freeze with guc enabled. Tomi?

DRM-Tip kernels on Skylake from commit 8e6dfee and onwards
(drm-tip: 2017y-01m-05d-16h-48m-32s UTC integration manifest)
get into infinite suspend with following options turned on:
i915.enable_guc_submission=1 i915.enable_guc_loading=1

With this patch the testhost i6700K survived dozen rounds of
igt@gem_exec_suspend@basic-s3, without it the first run hangs the 
machine.

Tested-by: Tomi Sarvela <tomi.p.sarvela@intel.com>

Best regards,

Tomi Sarvela

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

Re: [PATCH] drm/i915/guc: Make sure vma containing firmware is GuC mappable

[Chris Wilson]

On Thu, Jan 12, 2017 at 12:44:52PM +0200, Tomi Sarvela wrote:
> On Thursday, 12 January 2017 10:33:47 EET Chris Wilson wrote:
> > On Wed, Jan 11, 2017 at 04:17:39PM +0100, Michał Winiarski wrote:
> > > Since commit 4741da925fa3 ("drm/i915/guc: Assert that all GGTT
> > > offsets used by the GuC are mappable"), we're asserting that GuC
> > > firmware is in the GuC mappable range.
> > > Except we're not pinning the object with bias, which means it's
> > > possible to trigger this assert. Let's add a proper bias.
> > > 
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> > 
> > My spider sense is tingling....
> > 
> > CI has been reporting a suspend freeze with guc enabled. Tomi?
> 
> DRM-Tip kernels on Skylake from commit 8e6dfee and onwards
> (drm-tip: 2017y-01m-05d-16h-48m-32s UTC integration manifest)
> get into infinite suspend with following options turned on:
> i915.enable_guc_submission=1 i915.enable_guc_loading=1
> 
> With this patch the testhost i6700K survived dozen rounds of
> igt@gem_exec_suspend@basic-s3, without it the first run hangs the 
> machine.
> 
> Tested-by: Tomi Sarvela <tomi.p.sarvela@intel.com>

Thanks for the patch Michał, and for Tomi patiently proving how badly I
broke everything. Pushed,
-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