| Message ID | 20170111151739.28965-1-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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);
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(-)