| Message ID | 20161221141126.2452-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 21, 2016 at 02:11:26PM +0000, Chris Wilson wrote: > The GuC would like to own the upper portion of the GTT for itself, so > exclude it from our drm_mm to prevent us using it. Fixes: 2947e40 ("drm/i915/execlists: Request the kernel context be pinned high") Reviewed-by: Michał Winiarski <michal.winiarski@intel.com> Thanks! -Michał > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 5 +++++ > drivers/gpu/drm/i915/intel_guc_fwif.h | 3 +++ > 2 files changed, 8 insertions(+)
On Wed, Dec 21, 2016 at 02:11:26PM +0000, Chris Wilson wrote: > The GuC would like to own the upper portion of the GTT for itself, so > exclude it from our drm_mm to prevent us using it. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> Tested-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> Fixes the issue with GuC loading with submission on.
On Wed, Dec 21, 2016 at 02:11:26PM +0000, Chris Wilson wrote: > The GuC would like to own the upper portion of the GTT for itself, so > exclude it from our drm_mm to prevent us using it. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 5 +++++ > drivers/gpu/drm/i915/intel_guc_fwif.h | 3 +++ > 2 files changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 6af9311f72f5..96bc0e83286a 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -3176,6 +3176,11 @@ int i915_ggtt_probe_hw(struct drm_i915_private *dev_priv) > if (ret) > return ret; > > + if (HAS_GUC_SCHED(dev_priv)) { Btw, shouldn't we also modify HAS_GUC_SCHED macro to look at i915.enable_guc_submission instead of .has_guc ? Note that Guc may be present but disabled... > + ggtt->base.total -= GUC_GGTT_RESERVED_TOP; Hmm, while unlikely, what if RESERVED_TOP is larger than detected base.total ? > + ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total); > + } > + > if ((ggtt->base.total - 1) >> 32) { > DRM_ERROR("We never expected a Global GTT with more than 32bits" > " of address space! Found %lldM!\n", > diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h > index 3202b32b5638..3361d38ed859 100644 > --- a/drivers/gpu/drm/i915/intel_guc_fwif.h > +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h > @@ -23,6 +23,9 @@ > #ifndef _INTEL_GUC_FWIF_H > #define _INTEL_GUC_FWIF_H > > +/* A small region at the top of the global GTT is reserved for use by the GuC */ > +#define GUC_GGTT_RESERVED_TOP 0x1200000 Is this region size fixed (part of Guc HW assumptions), or it can change per different FW? If former, then maybe this macro should be moved to i915_guc_reg.h? If latter, then maybe it is worth to explictly state that in the comment? > + > #define GFXCORE_FAMILY_GEN9 12 > #define GFXCORE_FAMILY_UNKNOWN 0x7fffffff > > -- > 2.11.0 > Thanks, Michal
On Wed, Dec 21, 2016 at 04:30:14PM +0100, Michal Wajdeczko wrote: > On Wed, Dec 21, 2016 at 02:11:26PM +0000, Chris Wilson wrote: > > The GuC would like to own the upper portion of the GTT for itself, so > > exclude it from our drm_mm to prevent us using it. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > --- > > drivers/gpu/drm/i915/i915_gem_gtt.c | 5 +++++ > > drivers/gpu/drm/i915/intel_guc_fwif.h | 3 +++ > > 2 files changed, 8 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > > index 6af9311f72f5..96bc0e83286a 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > @@ -3176,6 +3176,11 @@ int i915_ggtt_probe_hw(struct drm_i915_private *dev_priv) > > if (ret) > > return ret; > > > > + if (HAS_GUC_SCHED(dev_priv)) { > > Btw, shouldn't we also modify HAS_GUC_SCHED macro to look at i915.enable_guc_submission > instead of .has_guc ? Note that Guc may be present but disabled... And we don't really know at this point, since i915.enable_guc_submission is resolved later. if (HAS_GUC_SCHED(dev_priv) && i915.enable_guc_submission) is conservative enough. > > + ggtt->base.total -= GUC_GGTT_RESERVED_TOP; > > Hmm, while unlikely, what if RESERVED_TOP is larger than detected base.total ? Then we are screwed :) - if (HAS_GUC_SCHED(dev_priv)) { + if (HAS_GUC_SCHED(dev_priv) && i915.enable_guc_submission) { + if (WARN(ggtt->base.total < GUC_GGTT_RESERVED_TOP, + "Only found %lldMiB, but need %dMiB for the GuC", + ggtt->base.total >> 20, GUC_GGTT_RESERVED_TOP >> 20)) + return -ENODEV; + ggtt->base.total -= GUC_GGTT_RESERVED_TOP; ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total); } > > + ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total); > > + } > > + > > if ((ggtt->base.total - 1) >> 32) { > > DRM_ERROR("We never expected a Global GTT with more than 32bits" > > " of address space! Found %lldM!\n", > > diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h > > index 3202b32b5638..3361d38ed859 100644 > > --- a/drivers/gpu/drm/i915/intel_guc_fwif.h > > +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h > > @@ -23,6 +23,9 @@ > > #ifndef _INTEL_GUC_FWIF_H > > #define _INTEL_GUC_FWIF_H > > > > +/* A small region at the top of the global GTT is reserved for use by the GuC */ > > +#define GUC_GGTT_RESERVED_TOP 0x1200000 > > Is this region size fixed (part of Guc HW assumptions), or it can change per different FW? > If former, then maybe this macro should be moved to i915_guc_reg.h? > If latter, then maybe it is worth to explictly state that in the comment? This is purely guess work. Feel free to supply the mising details... -Chris
On Wed, Dec 21, 2016 at 04:03:30PM +0000, Chris Wilson wrote: > On Wed, Dec 21, 2016 at 04:30:14PM +0100, Michal Wajdeczko wrote: > > On Wed, Dec 21, 2016 at 02:11:26PM +0000, Chris Wilson wrote: > > > The GuC would like to own the upper portion of the GTT for itself, so > > > exclude it from our drm_mm to prevent us using it. > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_gem_gtt.c | 5 +++++ > > > drivers/gpu/drm/i915/intel_guc_fwif.h | 3 +++ > > > 2 files changed, 8 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > > > index 6af9311f72f5..96bc0e83286a 100644 > > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > > @@ -3176,6 +3176,11 @@ int i915_ggtt_probe_hw(struct drm_i915_private *dev_priv) > > > if (ret) > > > return ret; > > > > > > + if (HAS_GUC_SCHED(dev_priv)) { > > > > Btw, shouldn't we also modify HAS_GUC_SCHED macro to look at i915.enable_guc_submission > > instead of .has_guc ? Note that Guc may be present but disabled... > > And we don't really know at this point, since i915.enable_guc_submission > is resolved later. > > if (HAS_GUC_SCHED(dev_priv) && i915.enable_guc_submission) is > conservative enough. > > > > + ggtt->base.total -= GUC_GGTT_RESERVED_TOP; > > > > Hmm, while unlikely, what if RESERVED_TOP is larger than detected base.total ? > > Then we are screwed :) > > - if (HAS_GUC_SCHED(dev_priv)) { > + if (HAS_GUC_SCHED(dev_priv) && i915.enable_guc_submission) { Assuming that in the future HAS_GUC_SCHED will be corrected to take use sanitized value of .enable_guc_submission, to avoid any mislead, I would rather go with - if (HAS_GUC_SCHED(dev_priv) && i915.enable_guc_submission) { + if (HAS_GUC(dev_priv) && i915.enable_guc_submission) { > + if (WARN(ggtt->base.total < GUC_GGTT_RESERVED_TOP, > + "Only found %lldMiB, but need %dMiB for the GuC", Please update message with "GGTT" or similar to capture context what is missing ;) > + ggtt->base.total >> 20, GUC_GGTT_RESERVED_TOP >> 20)) > + return -ENODEV; > + > ggtt->base.total -= GUC_GGTT_RESERVED_TOP; > ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total); > } > > > > + ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total); > > > + } > > > + > > > if ((ggtt->base.total - 1) >> 32) { > > > DRM_ERROR("We never expected a Global GTT with more than 32bits" > > > " of address space! Found %lldM!\n", > > > diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h > > > index 3202b32b5638..3361d38ed859 100644 > > > --- a/drivers/gpu/drm/i915/intel_guc_fwif.h > > > +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h > > > @@ -23,6 +23,9 @@ > > > #ifndef _INTEL_GUC_FWIF_H > > > #define _INTEL_GUC_FWIF_H > > > > > > +/* A small region at the top of the global GTT is reserved for use by the GuC */ > > > +#define GUC_GGTT_RESERVED_TOP 0x1200000 > > > > Is this region size fixed (part of Guc HW assumptions), or it can change per different FW? > > If former, then maybe this macro should be moved to i915_guc_reg.h? > > If latter, then maybe it is worth to explictly state that in the comment? > > This is purely guess work. Feel free to supply the mising details... If only I knew ;) maybe another day. With the proposed changes, Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Thanks, Michal
With enable_guc_loading=2 and enable_guc_submission=0 I get HuC authentication failure and with enable_guc_loading and enable_guc_submisssion both set to 2 the device does not show any display. Anusha >-----Original Message----- >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of >Michal Wajdeczko >Sent: Wednesday, December 21, 2016 8:31 AM >To: Chris Wilson <chris@chris-wilson.co.uk>; intel-gfx@lists.freedesktop.org; >Hiler, Arkadiusz <arkadiusz.hiler@intel.com> >Subject: Re: [Intel-gfx] [PATCH] drm/i915/guc: Reserve the upper end of the >Global GTT for the GuC > >On Wed, Dec 21, 2016 at 04:03:30PM +0000, Chris Wilson wrote: >> On Wed, Dec 21, 2016 at 04:30:14PM +0100, Michal Wajdeczko wrote: >> > On Wed, Dec 21, 2016 at 02:11:26PM +0000, Chris Wilson wrote: >> > > The GuC would like to own the upper portion of the GTT for itself, >> > > so exclude it from our drm_mm to prevent us using it. >> > > >> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> > > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> >> > > --- >> > > drivers/gpu/drm/i915/i915_gem_gtt.c | 5 +++++ >> > > drivers/gpu/drm/i915/intel_guc_fwif.h | 3 +++ >> > > 2 files changed, 8 insertions(+) >> > > >> > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c >> > > b/drivers/gpu/drm/i915/i915_gem_gtt.c >> > > index 6af9311f72f5..96bc0e83286a 100644 >> > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c >> > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c >> > > @@ -3176,6 +3176,11 @@ int i915_ggtt_probe_hw(struct >drm_i915_private *dev_priv) >> > > if (ret) >> > > return ret; >> > > >> > > + if (HAS_GUC_SCHED(dev_priv)) { >> > >> > Btw, shouldn't we also modify HAS_GUC_SCHED macro to look at >> > i915.enable_guc_submission instead of .has_guc ? Note that Guc may be >present but disabled... >> >> And we don't really know at this point, since >> i915.enable_guc_submission is resolved later. >> >> if (HAS_GUC_SCHED(dev_priv) && i915.enable_guc_submission) is >> conservative enough. >> >> > > + ggtt->base.total -= GUC_GGTT_RESERVED_TOP; >> > >> > Hmm, while unlikely, what if RESERVED_TOP is larger than detected >base.total ? >> >> Then we are screwed :) >> >> - if (HAS_GUC_SCHED(dev_priv)) { >> + if (HAS_GUC_SCHED(dev_priv) && i915.enable_guc_submission) { > >Assuming that in the future HAS_GUC_SCHED will be corrected to take use >sanitized value of .enable_guc_submission, to avoid any mislead, I would rather >go with > >- if (HAS_GUC_SCHED(dev_priv) && i915.enable_guc_submission) { >+ if (HAS_GUC(dev_priv) && i915.enable_guc_submission) { > > >> + if (WARN(ggtt->base.total < GUC_GGTT_RESERVED_TOP, >> + "Only found %lldMiB, but need %dMiB for the >> + GuC", > >Please update message with "GGTT" or similar to capture context what is missing >;) > >> + ggtt->base.total >> 20, GUC_GGTT_RESERVED_TOP >> 20)) >> + return -ENODEV; >> + >> ggtt->base.total -= GUC_GGTT_RESERVED_TOP; >> ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total); >> } >> >> > > + ggtt->mappable_end = min(ggtt->mappable_end, ggtt- >>base.total); >> > > + } >> > > + >> > > if ((ggtt->base.total - 1) >> 32) { >> > > DRM_ERROR("We never expected a Global GTT with more than >32bits" >> > > " of address space! Found %lldM!\n", diff --git >> > > a/drivers/gpu/drm/i915/intel_guc_fwif.h >> > > b/drivers/gpu/drm/i915/intel_guc_fwif.h >> > > index 3202b32b5638..3361d38ed859 100644 >> > > --- a/drivers/gpu/drm/i915/intel_guc_fwif.h >> > > +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h >> > > @@ -23,6 +23,9 @@ >> > > #ifndef _INTEL_GUC_FWIF_H >> > > #define _INTEL_GUC_FWIF_H >> > > >> > > +/* A small region at the top of the global GTT is reserved for use by the >GuC */ >> > > +#define GUC_GGTT_RESERVED_TOP 0x1200000 >> > >> > Is this region size fixed (part of Guc HW assumptions), or it can change per >different FW? >> > If former, then maybe this macro should be moved to i915_guc_reg.h? >> > If latter, then maybe it is worth to explictly state that in the comment? >> >> This is purely guess work. Feel free to supply the mising details... > >If only I knew ;) maybe another day. > >With the proposed changes, > >Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > >Thanks, >Michal >_______________________________________________ >Intel-gfx mailing list >Intel-gfx@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 21/12/16 06:11, Chris Wilson wrote: > The GuC would like to own the upper portion of the GTT for itself, so > exclude it from our drm_mm to prevent us using it. I had a quick chat with a GuC dev and he is pretty sure that GuC can't reserve any GTT areas for itself (and that is why we do the allocation for the buffers the GuC uses). However, it looks like the offsets above 0xFEE00000 are reserved internally from the GuC point of view for some private resources, similarly to what happens for the offsets below GUC_WOPCM_TOP. This means that we should be able to actually use those offsets from the host side, but we can't pass any object placed there to GuC. The GuC FW will drop any operation on objects placed above 0xFEE00000, which may be what caused the issues with guc_submission (not sure of this since I haven't seen the report). The fix below might still be ok for us as reducing the GGTT size by a small amount shouldn't be a big issue, but I'd prefer the comment and the commit message to be updated. To also reply to Michal question, I was told that the region size is fixed and not FW dependent. Thanks, Daniele > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 5 +++++ > drivers/gpu/drm/i915/intel_guc_fwif.h | 3 +++ > 2 files changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 6af9311f72f5..96bc0e83286a 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -3176,6 +3176,11 @@ int i915_ggtt_probe_hw(struct drm_i915_private *dev_priv) > if (ret) > return ret; > > + if (HAS_GUC_SCHED(dev_priv)) { > + ggtt->base.total -= GUC_GGTT_RESERVED_TOP; > + ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total); > + } > + > if ((ggtt->base.total - 1) >> 32) { > DRM_ERROR("We never expected a Global GTT with more than 32bits" > " of address space! Found %lldM!\n", > diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h > index 3202b32b5638..3361d38ed859 100644 > --- a/drivers/gpu/drm/i915/intel_guc_fwif.h > +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h > @@ -23,6 +23,9 @@ > #ifndef _INTEL_GUC_FWIF_H > #define _INTEL_GUC_FWIF_H > > +/* A small region at the top of the global GTT is reserved for use by the GuC */ > +#define GUC_GGTT_RESERVED_TOP 0x1200000 > + > #define GFXCORE_FAMILY_GEN9 12 > #define GFXCORE_FAMILY_UNKNOWN 0x7fffffff > >
On 12/22/2016 2:40 AM, Daniele Ceraolo Spurio wrote: > > > On 21/12/16 06:11, Chris Wilson wrote: >> The GuC would like to own the upper portion of the GTT for itself, so >> exclude it from our drm_mm to prevent us using it. > > I had a quick chat with a GuC dev and he is pretty sure that GuC can't > reserve any GTT areas for itself (and that is why we do the allocation > for the buffers the GuC uses). However, it looks like the offsets > above 0xFEE00000 are reserved internally from the GuC point of view > for some private resources, similarly to what happens for the offsets > below GUC_WOPCM_TOP. This means that we should be able to actually use > those offsets from the host side, but we can't pass any object placed > there to GuC. The GuC FW will drop any operation on objects placed > above 0xFEE00000, which may be what caused the issues with > guc_submission (not sure of this since I haven't seen the report). golden_context_lrca, reg_state, reg_state_buffer addresses in ADS that makes GuC load fail: [ 4.955330] [drm:i915_guc_submission_init [i915]] fffe7000 8542c8 854b18 With this patch applied golden_context_lrca gets placed properly and GuC load succeeds. [ 4.953817] [drm:i915_guc_submission_init [i915]] fede7000 8542c8 854b18 > The fix below might still be ok for us as reducing the GGTT size by a > small amount shouldn't be a big issue, but I'd prefer the comment and > the commit message to be updated. > > To also reply to Michal question, I was told that the region size is > fixed and not FW dependent. > > Thanks, > Daniele > >> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> >> --- >> drivers/gpu/drm/i915/i915_gem_gtt.c | 5 +++++ >> drivers/gpu/drm/i915/intel_guc_fwif.h | 3 +++ >> 2 files changed, 8 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c >> b/drivers/gpu/drm/i915/i915_gem_gtt.c >> index 6af9311f72f5..96bc0e83286a 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c >> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c >> @@ -3176,6 +3176,11 @@ int i915_ggtt_probe_hw(struct drm_i915_private >> *dev_priv) >> if (ret) >> return ret; >> >> + if (HAS_GUC_SCHED(dev_priv)) { >> + ggtt->base.total -= GUC_GGTT_RESERVED_TOP; >> + ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total); >> + } >> + >> if ((ggtt->base.total - 1) >> 32) { >> DRM_ERROR("We never expected a Global GTT with more than >> 32bits" >> " of address space! Found %lldM!\n", >> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h >> b/drivers/gpu/drm/i915/intel_guc_fwif.h >> index 3202b32b5638..3361d38ed859 100644 >> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h >> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h >> @@ -23,6 +23,9 @@ >> #ifndef _INTEL_GUC_FWIF_H >> #define _INTEL_GUC_FWIF_H >> >> +/* A small region at the top of the global GTT is reserved for use >> by the GuC */ >> +#define GUC_GGTT_RESERVED_TOP 0x1200000 >> + >> #define GFXCORE_FAMILY_GEN9 12 >> #define GFXCORE_FAMILY_UNKNOWN 0x7fffffff >> >> > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Dec 21, 2016 at 07:35:04PM +0100, Srivatsa, Anusha wrote: > With enable_guc_loading=2 and enable_guc_submission=0 I get HuC > authentication failure and with enable_guc_loading and > enable_guc_submisssion both set to 2 the device does not show any > display. Sadly "the fix" fixes the issues we had with the loading - we can load GuC but... GuC Actions (INTEL_GUC_ACTION*, and intel_guc_send()) seem to care about kernel context pinned location as well (not directly though). And it's much more restritive. Experimentally I was able to determine that if we have ctx pinned above 0x10000000 (2 GB) mark, GuC Actions just fail. > >-----Original Message----- > >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of > >Michal Wajdeczko > >Sent: Wednesday, December 21, 2016 8:31 AM > >To: Chris Wilson <chris@chris-wilson.co.uk>; intel-gfx@lists.freedesktop.org; > >Hiler, Arkadiusz <arkadiusz.hiler@intel.com> > >Subject: Re: [Intel-gfx] [PATCH] drm/i915/guc: Reserve the upper end of the > >Global GTT for the GuC > > > >On Wed, Dec 21, 2016 at 04:03:30PM +0000, Chris Wilson wrote: > >> On Wed, Dec 21, 2016 at 04:30:14PM +0100, Michal Wajdeczko wrote: > >> > On Wed, Dec 21, 2016 at 02:11:26PM +0000, Chris Wilson wrote: > >> > > The GuC would like to own the upper portion of the GTT for itself, > >> > > so exclude it from our drm_mm to prevent us using it. > >> > > > >> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > >> > > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > >> > > --- > >> > > drivers/gpu/drm/i915/i915_gem_gtt.c | 5 +++++ > >> > > drivers/gpu/drm/i915/intel_guc_fwif.h | 3 +++ > >> > > 2 files changed, 8 insertions(+) > >> > > > >> > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > >> > > b/drivers/gpu/drm/i915/i915_gem_gtt.c > >> > > index 6af9311f72f5..96bc0e83286a 100644 > >> > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > >> > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > >> > > @@ -3176,6 +3176,11 @@ int i915_ggtt_probe_hw(struct > >drm_i915_private *dev_priv) > >> > > if (ret) > >> > > return ret; > >> > > > >> > > + if (HAS_GUC_SCHED(dev_priv)) { > >> > > >> > Btw, shouldn't we also modify HAS_GUC_SCHED macro to look at > >> > i915.enable_guc_submission instead of .has_guc ? Note that Guc may be > >present but disabled... > >> > >> And we don't really know at this point, since > >> i915.enable_guc_submission is resolved later. > >> > >> if (HAS_GUC_SCHED(dev_priv) && i915.enable_guc_submission) is > >> conservative enough. > >> > >> > > + ggtt->base.total -= GUC_GGTT_RESERVED_TOP; > >> > > >> > Hmm, while unlikely, what if RESERVED_TOP is larger than detected > >base.total ? > >> > >> Then we are screwed :) > >> > >> - if (HAS_GUC_SCHED(dev_priv)) { > >> + if (HAS_GUC_SCHED(dev_priv) && i915.enable_guc_submission) { > > > >Assuming that in the future HAS_GUC_SCHED will be corrected to take use > >sanitized value of .enable_guc_submission, to avoid any mislead, I would rather > >go with > > > >- if (HAS_GUC_SCHED(dev_priv) && i915.enable_guc_submission) { > >+ if (HAS_GUC(dev_priv) && i915.enable_guc_submission) { > > > > > >> + if (WARN(ggtt->base.total < GUC_GGTT_RESERVED_TOP, > >> + "Only found %lldMiB, but need %dMiB for the > >> + GuC", > > > >Please update message with "GGTT" or similar to capture context what is missing > >;) > > > >> + ggtt->base.total >> 20, GUC_GGTT_RESERVED_TOP >> 20)) > >> + return -ENODEV; > >> + > >> ggtt->base.total -= GUC_GGTT_RESERVED_TOP; > >> ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total); > >> } > >> > >> > > + ggtt->mappable_end = min(ggtt->mappable_end, ggtt- > >>base.total); > >> > > + } > >> > > + > >> > > if ((ggtt->base.total - 1) >> 32) { > >> > > DRM_ERROR("We never expected a Global GTT with more than > >32bits" > >> > > " of address space! Found %lldM!\n", diff --git > >> > > a/drivers/gpu/drm/i915/intel_guc_fwif.h > >> > > b/drivers/gpu/drm/i915/intel_guc_fwif.h > >> > > index 3202b32b5638..3361d38ed859 100644 > >> > > --- a/drivers/gpu/drm/i915/intel_guc_fwif.h > >> > > +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h > >> > > @@ -23,6 +23,9 @@ > >> > > #ifndef _INTEL_GUC_FWIF_H > >> > > #define _INTEL_GUC_FWIF_H > >> > > > >> > > +/* A small region at the top of the global GTT is reserved for use by the > >GuC */ > >> > > +#define GUC_GGTT_RESERVED_TOP 0x1200000 > >> > > >> > Is this region size fixed (part of Guc HW assumptions), or it can change per > >different FW? > >> > If former, then maybe this macro should be moved to i915_guc_reg.h? > >> > If latter, then maybe it is worth to explictly state that in the comment? > >> > >> This is purely guess work. Feel free to supply the mising details... > > > >If only I knew ;) maybe another day. > > > >With the proposed changes, > > > >Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > > > >Thanks, > >Michal > >_______________________________________________ > >Intel-gfx mailing list > >Intel-gfx@lists.freedesktop.org > >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Dec 22, 2016 at 03:53:15PM +0100, Arkadiusz Hiler wrote: > On Wed, Dec 21, 2016 at 07:35:04PM +0100, Srivatsa, Anusha wrote: > > With enable_guc_loading=2 and enable_guc_submission=0 I get HuC > > authentication failure and with enable_guc_loading and > > enable_guc_submisssion both set to 2 the device does not show any > > display. > > Sadly "the fix" fixes the issues we had with the loading - we can load > GuC but... > > GuC Actions (INTEL_GUC_ACTION*, and intel_guc_send()) seem to care about > kernel context pinned location as well (not directly though). And it's > much more restritive. > > Experimentally I was able to determine that if we have ctx pinned above > 0x10000000 (2 GB) mark, GuC Actions just fail. Are we certain this only applies to the kernel/golden context? No other objects accessed by the GuC are affected? -Chris
On Thu, Dec 22, 2016 at 03:18:08PM +0000, Chris Wilson wrote: > On Thu, Dec 22, 2016 at 03:53:15PM +0100, Arkadiusz Hiler wrote: > > On Wed, Dec 21, 2016 at 07:35:04PM +0100, Srivatsa, Anusha wrote: > > > With enable_guc_loading=2 and enable_guc_submission=0 I get HuC > > > authentication failure and with enable_guc_loading and > > > enable_guc_submisssion both set to 2 the device does not show any > > > display. > > > > Sadly "the fix" fixes the issues we had with the loading - we can load > > GuC but... > > > > GuC Actions (INTEL_GUC_ACTION*, and intel_guc_send()) seem to care about > > kernel context pinned location as well (not directly though). And it's > > much more restritive. > > > > Experimentally I was able to determine that if we have ctx pinned above > > 0x10000000 (2 GB) mark, GuC Actions just fail. > > Are we certain this only applies to the kernel/golden context? No other > objects accessed by the GuC are affected? > -Chris Not sure yet. > -- > Chris Wilson, Intel Open Source Technology Centre
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 6af9311f72f5..96bc0e83286a 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3176,6 +3176,11 @@ int i915_ggtt_probe_hw(struct drm_i915_private *dev_priv) if (ret) return ret; + if (HAS_GUC_SCHED(dev_priv)) { + ggtt->base.total -= GUC_GGTT_RESERVED_TOP; + ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total); + } + if ((ggtt->base.total - 1) >> 32) { DRM_ERROR("We never expected a Global GTT with more than 32bits" " of address space! Found %lldM!\n", diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h index 3202b32b5638..3361d38ed859 100644 --- a/drivers/gpu/drm/i915/intel_guc_fwif.h +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h @@ -23,6 +23,9 @@ #ifndef _INTEL_GUC_FWIF_H #define _INTEL_GUC_FWIF_H +/* A small region at the top of the global GTT is reserved for use by the GuC */ +#define GUC_GGTT_RESERVED_TOP 0x1200000 + #define GFXCORE_FAMILY_GEN9 12 #define GFXCORE_FAMILY_UNKNOWN 0x7fffffff
The GuC would like to own the upper portion of the GTT for itself, so exclude it from our drm_mm to prevent us using it. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> --- drivers/gpu/drm/i915/i915_gem_gtt.c | 5 +++++ drivers/gpu/drm/i915/intel_guc_fwif.h | 3 +++ 2 files changed, 8 insertions(+)