| Message ID | 572B4D76.9040708@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
The attached version still does not explain that the WOPCM_TOP is to tell the GuC not to use that space. The extra information does not aid anybody as the information is used internally within the GuC. But, I have not actual objection to the patch. Peter. -----Original Message----- From: Gordon, David S Sent: Thursday, May 5, 2016 2:41 PM To: Antoine, Peter <peter.antoine@intel.com>; intel-gfx@lists.freedesktop.org Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com> Subject: Re: [PATCH v3] drm/i915: resize the GuC WOPCM for rc6 On 26/04/2016 10:11, Peter Antoine wrote: > This patch resizes the GuC WOPCM to so that the GuC and the RC6 memory > spaces do not overlap. > > Issue: https://jira01.devtools.intel.com/browse/VIZ-6638 > Signed-off-by: Peter Antoine <peter.antoine@intel.com> > --- > drivers/gpu/drm/i915/i915_guc_reg.h | 5 +++-- > drivers/gpu/drm/i915/intel_guc_loader.c | 6 +++++- > 2 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h > b/drivers/gpu/drm/i915/i915_guc_reg.h > index 80786d9..6e01238 100644 > --- a/drivers/gpu/drm/i915/i915_guc_reg.h > +++ b/drivers/gpu/drm/i915/i915_guc_reg.h > @@ -68,10 +68,11 @@ > #define GUC_MAX_IDLE_COUNT _MMIO(0xC3E4) > > #define GUC_WOPCM_SIZE _MMIO(0xc050) > -#define GUC_WOPCM_SIZE_VALUE (0x80 << 12) /* 512KB */ > +#define GUC_WOPCM_SIZE_VALUE (0x80 << 12) /* 512KB */ > +#define BXT_GUC_WOPCM_SIZE_VALUE (0x70 << 12) /* 448KB */ > > /* GuC addresses below GUC_WOPCM_TOP don't map through the GTT */ > -#define GUC_WOPCM_TOP (GUC_WOPCM_SIZE_VALUE) > +#define GUC_WOPCM_TOP (0x80 << 12) /* 512KB */ > > #define GEN8_GT_PM_CONFIG _MMIO(0x138140) > #define GEN9LP_GT_PM_CONFIG _MMIO(0x138140) > diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c > b/drivers/gpu/drm/i915/intel_guc_loader.c > index fc3ff68..38fb321 100644 > --- a/drivers/gpu/drm/i915/intel_guc_loader.c > +++ b/drivers/gpu/drm/i915/intel_guc_loader.c > @@ -312,7 +312,11 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv) > intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); > > /* init WOPCM */ > - I915_WRITE(GUC_WOPCM_SIZE, GUC_WOPCM_SIZE_VALUE); > + if (IS_BROXTON(dev)) > + I915_WRITE(GUC_WOPCM_SIZE, BXT_GUC_WOPCM_SIZE_VALUE); > + else > + I915_WRITE(GUC_WOPCM_SIZE, GUC_WOPCM_SIZE_VALUE); > + > I915_WRITE(DMA_GUC_WOPCM_OFFSET, GUC_WOPCM_OFFSET_VALUE); > > /* Enable MIA caching. GuC clock gating is disabled. */ So, this gives the right result, but doesn't really show or explain why we have different values, or how the values are arrived at; they're just more magic numbers. Also, in the loader there's a check on the firmware size that uses different values. So I'd rather prefer the unified approach in the attached version ... .Dave.
On 05/05/2016 15:02, Antoine, Peter wrote: > The attached version still does not explain that the WOPCM_TOP is to tell the GuC not to use that space. That's NOT what WOPCM_TOP means. The GuC is allowed to use the space up to the value stored in the GUC_WOPCM_SIZE register (as the comment above the #define says). Architecturally, this is allowed to be any value greater than (16K+sizeof internal SRAM (64, 128, or 256K)) and less than or equal to GUC_WOPCM_TOP (which is a platform-independent constant), so we normally choose the maximm allowed. Howver on BXT, we need to leave some space at the top for the RC6 image, hence the logic (and comments!) in guc_wopcm_size(). > The extra information does not aid anybody as the information is used internally within the GuC. It may help the next person who has to figure out what's gone wrong on some future chip that needs more than 64K for RC6! .Dave. > > But, I have not actual objection to the patch. > > Peter. >
On Thu, 5 May 2016, Dave Gordon wrote: > On 05/05/2016 15:02, Antoine, Peter wrote: > > The attached version still does not explain that the WOPCM_TOP is to tell the GuC not to use that space. > > > That's NOT what WOPCM_TOP means. The GuC is allowed to use the space up to the value stored in the GUC_WOPCM_SIZE register (as the comment above the #define says). Architecturally, this is allowed to be any value greater than > (16K+sizeof internal SRAM (64, 128, or 256K)) and less than or equal to GUC_WOPCM_TOP (which is a platform-independent constant), so we normally choose the maximm allowed. Howver on BXT, we need to leave some space at the top for the > RC6 image, hence the logic (and comments!) in guc_wopcm_size(). Yes, the firmware can use upto GUC_WOPCM_TOP and to leave the rest alone. > > The extra information does not aid anybody as the information is used internally within the GuC. > > It may help the next person who has to figure out what's gone wrong on some future chip that needs more than 64K for RC6! You hid a if statement in a function (making the code harder to read and more prone to error). Where maybe a slightly clearer comment was required. And this patch has been held up two weeks just for a better comment. Peter. > > .Dave. And what if the next reserved space is not for RC6? > > > But, I have not actual objection to the patch. > > Peter. > > > > -- Peter Antoine (Android Graphics Driver Software Engineer) --------------------------------------------------------------------- Intel Corporation (UK) Limited Registered No. 1134945 (England) Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47
On 06/05/2016 08:01, Peter Antoine wrote: > On Thu, 5 May 2016, Dave Gordon wrote: > >> On 05/05/2016 15:02, Antoine, Peter wrote: >> >> The attached version still does not explain that the WOPCM_TOP is to >> tell the GuC not to use that space. >> >> >> That's NOT what WOPCM_TOP means. The GuC is allowed to use the space >> up to the value stored in the GUC_WOPCM_SIZE register (as the comment >> above the #define says). Architecturally, this is allowed to be any >> value greater than >> (16K+sizeof internal SRAM (64, 128, or 256K)) and less than or equal >> to GUC_WOPCM_TOP (which is a platform-independent constant), so we >> normally choose the maximm allowed. Howver on BXT, we need to leave >> some space at the top for the >> RC6 image, hence the logic (and comments!) in guc_wopcm_size(). > Yes, the firmware can use upto GUC_WOPCM_TOP and to leave the rest alone. >> >> The extra information does not aid anybody as the information is used >> internally within the GuC. >> >> It may help the next person who has to figure out what's gone wrong on >> some future chip that needs more than 64K for RC6! > > You hid a if statement in a function (making the code harder to read and > more prone to error). Where maybe a slightly clearer comment was required. > > And this patch has been held up two weeks just for a better comment. > > Peter. >> >> .Dave. > > And what if the next reserved space is not for RC6? > >> >> >> But, I have not actual objection to the patch. >> >> Peter. >> >> >> >> Tested-by: Nick Hoath <nicholas.hoath@intel.com> Reviewed-by: Nick Hoath <nicholas.hoath@intel.com> > > -- > Peter Antoine (Android Graphics Driver Software Engineer) > --------------------------------------------------------------------- > Intel Corporation (UK) Limited > Registered No. 1134945 (England) > Registered Office: Pipers Way, Swindon SN3 1RJ > VAT No: 860 2173 47 > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 05/05/2016 16:04, Dave Gordon wrote: > On 05/05/2016 15:02, Antoine, Peter wrote: >> The attached version still does not explain that the WOPCM_TOP is to tell the GuC not to use that space. > > That's NOT what WOPCM_TOP means. The GuC is allowed to use the space up > to the value stored in the GUC_WOPCM_SIZE register (as the comment above > the #define says). Architecturally, this is allowed to be any value > greater than (16K+sizeof internal SRAM (64, 128, or 256K)) and less than > or equal to GUC_WOPCM_TOP (which is a platform-independent constant), so > we normally choose the maximm allowed. Howver on BXT, we need to leave > some space at the top for the RC6 image, hence the logic (and comments!) > in guc_wopcm_size(). > >> The extra information does not aid anybody as the information is used internally within the GuC. > It may help the next person who has to figure out what's gone wrong on > some future chip that needs more than 64K for RC6! > > .Dave. >> >> But, I have not actual objection to the patch. >> >> Peter. >> > Unfortunately Dave's patch locked my test system on bootup, so I've t-b & r-b'd Peter's.
On 06/05/16 10:37, Nick Hoath wrote: > On 05/05/2016 16:04, Dave Gordon wrote: >> On 05/05/2016 15:02, Antoine, Peter wrote: >>> The attached version still does not explain that the WOPCM_TOP is to >>> tell the GuC not to use that space. >> >> That's NOT what WOPCM_TOP means. The GuC is allowed to use the space up >> to the value stored in the GUC_WOPCM_SIZE register (as the comment above >> the #define says). Architecturally, this is allowed to be any value >> greater than (16K+sizeof internal SRAM (64, 128, or 256K)) and less than >> or equal to GUC_WOPCM_TOP (which is a platform-independent constant), so >> we normally choose the maximm allowed. Howver on BXT, we need to leave >> some space at the top for the RC6 image, hence the logic (and comments!) >> in guc_wopcm_size(). >> >>> The extra information does not aid anybody as the information is used >>> internally within the GuC. >> It may help the next person who has to figure out what's gone wrong on >> some future chip that needs more than 64K for RC6! >> >> .Dave. >>> >>> But, I have not actual objection to the patch. >>> >>> Peter. >>> >> > Unfortunately Dave's patch locked my test system on bootup, so I've t-b > & r-b'd Peter's. They're equivalent, unless your firmware happens to be between 458752 and 491520 bytes in size (in which case you have a problem anyway). To check, I've run both versions, with debug printing the value chosen (on SKL) and the value that would have been chosen on BXT, and they're identical (and both work). So I think your build had some other problem unrelated to the specific patch. I've no problem with using Peter's patch for now, but it's not just a matter of the comments; there's also the other use(s) of GUC_WOP_(TOP,SIZE_VALUE), with ad-hoc additions or subtractions. So it still needs fixing properly. .Dave.
On 06/05/2016 13:18, Gordon, David S wrote: > On 06/05/16 10:37, Nick Hoath wrote: >> On 05/05/2016 16:04, Dave Gordon wrote: >>> On 05/05/2016 15:02, Antoine, Peter wrote: >>>> The attached version still does not explain that the WOPCM_TOP is to >>>> tell the GuC not to use that space. >>> >>> That's NOT what WOPCM_TOP means. The GuC is allowed to use the space up >>> to the value stored in the GUC_WOPCM_SIZE register (as the comment above >>> the #define says). Architecturally, this is allowed to be any value >>> greater than (16K+sizeof internal SRAM (64, 128, or 256K)) and less than >>> or equal to GUC_WOPCM_TOP (which is a platform-independent constant), so >>> we normally choose the maximm allowed. Howver on BXT, we need to leave >>> some space at the top for the RC6 image, hence the logic (and comments!) >>> in guc_wopcm_size(). >>> >>>> The extra information does not aid anybody as the information is used >>>> internally within the GuC. >>> It may help the next person who has to figure out what's gone wrong on >>> some future chip that needs more than 64K for RC6! >>> >>> .Dave. >>>> >>>> But, I have not actual objection to the patch. >>>> >>>> Peter. >>>> >>> >> Unfortunately Dave's patch locked my test system on bootup, so I've t-b >> & r-b'd Peter's. > > They're equivalent, unless your firmware happens to be between 458752 > and 491520 bytes in size (in which case you have a problem anyway). > > To check, I've run both versions, with debug printing the value chosen > (on SKL) and the value that would have been chosen on BXT, and they're > identical (and both work). So I think your build had some other problem > unrelated to the specific patch. > > I've no problem with using Peter's patch for now, but it's not just a > matter of the comments; there's also the other use(s) of > GUC_WOP_(TOP,SIZE_VALUE), with ad-hoc additions or subtractions. So it > still needs fixing properly. > > .Dave. > After a rebuild & a retest, Dave's patch works fine. Therefore for "drm/i915/bxt: reserve space for RC6 in the the GuC WOPCM": Tested-by: Nick Hoath <nicholas.hoath@intel.com> Reviewed-by: Nick Hoath <nicholas.hoath@intel.com>
diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h index 80786d9..cf5a65b 100644 --- a/drivers/gpu/drm/i915/i915_guc_reg.h +++ b/drivers/gpu/drm/i915/i915_guc_reg.h @@ -67,11 +67,11 @@ #define GUC_WOPCM_OFFSET_VALUE 0x80000 /* 512KB */ #define GUC_MAX_IDLE_COUNT _MMIO(0xC3E4) +/* Defines WOPCM space available to GuC firmware */ #define GUC_WOPCM_SIZE _MMIO(0xc050) -#define GUC_WOPCM_SIZE_VALUE (0x80 << 12) /* 512KB */ - /* GuC addresses below GUC_WOPCM_TOP don't map through the GTT */ -#define GUC_WOPCM_TOP (GUC_WOPCM_SIZE_VALUE) +#define GUC_WOPCM_TOP (0x80 << 12) /* 512KB */ +#define BXT_GUC_WOPCM_RC6_RESERVED (0x10 << 12) /* 64KB */ #define GEN8_GT_PM_CONFIG _MMIO(0x138140) #define GEN9LP_GT_PM_CONFIG _MMIO(0x138140) diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c index 876e5da..80fc1e0 100644 --- a/drivers/gpu/drm/i915/intel_guc_loader.c +++ b/drivers/gpu/drm/i915/intel_guc_loader.c @@ -281,6 +281,17 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv) return ret; } +static u32 guc_wopcm_size(struct drm_i915_private *dev_priv) +{ + u32 wopcm_size = GUC_WOPCM_TOP; + + /* On BXT, the top of WOPCM is reserved for RC6 context */ + if (IS_BROXTON(dev_priv)) + wopcm_size -= BXT_GUC_WOPCM_RC6_RESERVED; + + return wopcm_size; +} + /* * Load the GuC firmware blob into the MinuteIA. */ @@ -308,7 +319,7 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv) intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL); /* init WOPCM */ - I915_WRITE(GUC_WOPCM_SIZE, GUC_WOPCM_SIZE_VALUE); + I915_WRITE(GUC_WOPCM_SIZE, guc_wopcm_size(dev_priv)); I915_WRITE(DMA_GUC_WOPCM_OFFSET, GUC_WOPCM_OFFSET_VALUE); /* Enable MIA caching. GuC clock gating is disabled. */ @@ -552,9 +563,7 @@ static void guc_fw_fetch(struct drm_device *dev, struct intel_guc_fw *guc_fw) /* Header and uCode will be loaded to WOPCM. Size of the two. */ size = guc_fw->header_size + guc_fw->ucode_size; - - /* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */ - if (size > GUC_WOPCM_SIZE_VALUE - 0x8000) { + if (size > guc_wopcm_size(dev->dev_private)) { DRM_ERROR("Firmware is too large to fit in WOPCM\n"); goto fail; }