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@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of > >Michal Wajdeczko > >Sent: Wednesday, December 21, 2016 8:31 AM > >To: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; > >Hiler, Arkadiusz <arkadiusz.hiler@xxxxxxxxx> > >Subject: Re: [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@xxxxxxxxxxxxxxxxxx> > >> > > Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx> > >> > > Cc: Arkadiusz Hiler <arkadiusz.hiler@xxxxxxxxx> > >> > > --- > >> > > 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@xxxxxxxxx> > > > >Thanks, > >Michal > >_______________________________________________ > >Intel-gfx mailing list > >Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > >https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Cheers, Arek _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
