| Message ID | 20230825163343.e2fc7eb70db106edc6df3c4c@otheo.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gt: Fix reservation address in ggtt_reserve_guc_top | expand |
On 8/25/2023 7:33 AM, Javier Pello wrote: > There is an assertion in ggtt_reserve_guc_top that the global GTT > is of size at least GUC_GGTT_TOP, which is not the case on a 32-bit > platform; see commit 562d55d991b39ce376c492df2f7890fd6a541ffc > ("drm/i915/bdw: Only use 2g GGTT for 32b platforms"). If GEM_BUG_ON > is enabled, this triggers a BUG(); if GEM_BUG_ON is disabled, the > subsequent reservation fails and the driver fails to initialise > the device: > > i915 0000:00:02.0: [drm:i915_init_ggtt [i915]] Failed to reserve top of GGTT for GuC > i915 0000:00:02.0: Device initialization failed (-28) > i915 0000:00:02.0: Please file a bug on drm/i915; see https://gitlab.freedesktop.org/drm/intel/-/wikis/How-to-file-i915-bugs for details. > i915: probe of 0000:00:02.0 failed with error -28 > > Make the reservation at the top of the available space, whatever > that is, instead of assuming that the top will be GUC_GGTT_TOP. > > Fixes: 911800765ef6 ("drm/i915/uc: Reserve upper range of GGTT") For tracking, it might be good to also add: Link: https://gitlab.freedesktop.org/drm/intel/-/issues/9080 > Signed-off-by: Javier Pello <devel@otheo.eu> > Cc: intel-gfx@lists.freedesktop.org > Cc: stable@vger.kernel.org # v5.3+ Need the full CC list here, so that when the patch gets back-ported the relevant developers get automatically added. > --- > drivers/gpu/drm/i915/gt/intel_ggtt.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c > index e9328e1a..0157bebb 100644 > --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c > +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c > @@ -511,20 +511,29 @@ void intel_ggtt_unbind_vma(struct i915_address_space *vm, > vm->clear_range(vm, vma_res->start, vma_res->vma_size); > } > > +/* Reserve the top of the GuC address space for firmware images. Addresses > + * beyond GUC_GGTT_TOP in the GuC address space are inaccessible by GuC, > + * which makes for a suitable range to hold GuC/HuC firmware images if the > + * size of the GGTT is 4G. However, on a 32-bit platform the size of the GGTT > + * is limited to 2G, which is less than GUC_GGTT_TOP, but we reserve a chunk > + * of the same size anyway, which is far more than needed, to keep the logic > + * in uc_fw_ggtt_offset() simple. */ Style: multi-line comment should be formatted as: /* * Text * more text */ > +#define GUC_TOP_RESERVE_SIZE (SZ_4G - GUC_GGTT_TOP) > + > static int ggtt_reserve_guc_top(struct i915_ggtt *ggtt) > { > - u64 size; > + u64 offset; > int ret; > > if (!intel_uc_uses_guc(&ggtt->vm.gt->uc)) > return 0; > > - GEM_BUG_ON(ggtt->vm.total <= GUC_GGTT_TOP); > - size = ggtt->vm.total - GUC_GGTT_TOP; > + GEM_BUG_ON(ggtt->vm.total <= GUC_TOP_RESERVE_SIZE); > + offset = ggtt->vm.total - GUC_TOP_RESERVE_SIZE; > > - ret = i915_gem_gtt_reserve(&ggtt->vm, NULL, &ggtt->uc_fw, size, > - GUC_GGTT_TOP, I915_COLOR_UNEVICTABLE, > - PIN_NOEVICT); > + ret = i915_gem_gtt_reserve(&ggtt->vm, NULL, &ggtt->uc_fw, > + GUC_TOP_RESERVE_SIZE, offset, > + I915_COLOR_UNEVICTABLE, PIN_NOEVICT); The code change looks good to me, so with the style fix and the additions to the commit message this is: Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Daniele > if (ret) > drm_dbg(&ggtt->vm.i915->drm, > "Failed to reserve top of GGTT for GuC\n");
On Thu, 31 Aug 2023 15:49:28 -0700 "Ceraolo Spurio, Daniele" <daniele.ceraolospurio@intel.com> wrote: > On 8/25/2023 7:33 AM, Javier Pello wrote: > > There is an assertion in ggtt_reserve_guc_top that the global GTT > > is of size at least GUC_GGTT_TOP, which is not the case on a 32-bit > > platform; see commit 562d55d991b39ce376c492df2f7890fd6a541ffc > > ("drm/i915/bdw: Only use 2g GGTT for 32b platforms"). If GEM_BUG_ON > > is enabled, this triggers a BUG(); if GEM_BUG_ON is disabled, the > > subsequent reservation fails and the driver fails to initialise > > the device: > > > > i915 0000:00:02.0: [drm:i915_init_ggtt [i915]] Failed to reserve top of GGTT for GuC > > i915 0000:00:02.0: Device initialization failed (-28) > > i915 0000:00:02.0: Please file a bug on drm/i915; see https://gitlab.freedesktop.org/drm/intel/-/wikis/How-to-file-i915-bugs for details. > > i915: probe of 0000:00:02.0 failed with error -28 > > > > Make the reservation at the top of the available space, whatever > > that is, instead of assuming that the top will be GUC_GGTT_TOP. > > > > Fixes: 911800765ef6 ("drm/i915/uc: Reserve upper range of GGTT") > > For tracking, it might be good to also add: > > Link: https://gitlab.freedesktop.org/drm/intel/-/issues/9080 Sure. > > Signed-off-by: Javier Pello <devel@otheo.eu> > > Cc: intel-gfx@lists.freedesktop.org > > Cc: stable@vger.kernel.org # v5.3+ > > Need the full CC list here, so that when the patch gets back-ported the > relevant developers get automatically added. I was unsure if I should add them from the start or wait for a review. > > --- > > drivers/gpu/drm/i915/gt/intel_ggtt.c | 21 +++++++++++++++------ > > 1 file changed, 15 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c > > index e9328e1a..0157bebb 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c > > +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c > > @@ -511,20 +511,29 @@ void intel_ggtt_unbind_vma(struct i915_address_space *vm, > > vm->clear_range(vm, vma_res->start, vma_res->vma_size); > > } > > > > +/* Reserve the top of the GuC address space for firmware images. Addresses > > + * beyond GUC_GGTT_TOP in the GuC address space are inaccessible by GuC, > > + * which makes for a suitable range to hold GuC/HuC firmware images if the > > + * size of the GGTT is 4G. However, on a 32-bit platform the size of the GGTT > > + * is limited to 2G, which is less than GUC_GGTT_TOP, but we reserve a chunk > > + * of the same size anyway, which is far more than needed, to keep the logic > > + * in uc_fw_ggtt_offset() simple. */ > > Style: multi-line comment should be formatted as: > > /* > * Text > * more text > */ Yes, I realised that after I sent the patch, but I thought I would wait for replies rather than resending with only a style change. > > +#define GUC_TOP_RESERVE_SIZE (SZ_4G - GUC_GGTT_TOP) > > + > > static int ggtt_reserve_guc_top(struct i915_ggtt *ggtt) > > { > > - u64 size; > > + u64 offset; > > int ret; > > > > if (!intel_uc_uses_guc(&ggtt->vm.gt->uc)) > > return 0; > > > > - GEM_BUG_ON(ggtt->vm.total <= GUC_GGTT_TOP); > > - size = ggtt->vm.total - GUC_GGTT_TOP; > > + GEM_BUG_ON(ggtt->vm.total <= GUC_TOP_RESERVE_SIZE); > > + offset = ggtt->vm.total - GUC_TOP_RESERVE_SIZE; > > > > - ret = i915_gem_gtt_reserve(&ggtt->vm, NULL, &ggtt->uc_fw, size, > > - GUC_GGTT_TOP, I915_COLOR_UNEVICTABLE, > > - PIN_NOEVICT); > > + ret = i915_gem_gtt_reserve(&ggtt->vm, NULL, &ggtt->uc_fw, > > + GUC_TOP_RESERVE_SIZE, offset, > > + I915_COLOR_UNEVICTABLE, PIN_NOEVICT); > > The code change looks good to me, so with the style fix and the > additions to the commit message this is: > > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Thank you. Javier
diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c index e9328e1a..0157bebb 100644 --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c @@ -511,20 +511,29 @@ void intel_ggtt_unbind_vma(struct i915_address_space *vm, vm->clear_range(vm, vma_res->start, vma_res->vma_size); } +/* Reserve the top of the GuC address space for firmware images. Addresses + * beyond GUC_GGTT_TOP in the GuC address space are inaccessible by GuC, + * which makes for a suitable range to hold GuC/HuC firmware images if the + * size of the GGTT is 4G. However, on a 32-bit platform the size of the GGTT + * is limited to 2G, which is less than GUC_GGTT_TOP, but we reserve a chunk + * of the same size anyway, which is far more than needed, to keep the logic + * in uc_fw_ggtt_offset() simple. */ +#define GUC_TOP_RESERVE_SIZE (SZ_4G - GUC_GGTT_TOP) + static int ggtt_reserve_guc_top(struct i915_ggtt *ggtt) { - u64 size; + u64 offset; int ret; if (!intel_uc_uses_guc(&ggtt->vm.gt->uc)) return 0; - GEM_BUG_ON(ggtt->vm.total <= GUC_GGTT_TOP); - size = ggtt->vm.total - GUC_GGTT_TOP; + GEM_BUG_ON(ggtt->vm.total <= GUC_TOP_RESERVE_SIZE); + offset = ggtt->vm.total - GUC_TOP_RESERVE_SIZE; - ret = i915_gem_gtt_reserve(&ggtt->vm, NULL, &ggtt->uc_fw, size, - GUC_GGTT_TOP, I915_COLOR_UNEVICTABLE, - PIN_NOEVICT); + ret = i915_gem_gtt_reserve(&ggtt->vm, NULL, &ggtt->uc_fw, + GUC_TOP_RESERVE_SIZE, offset, + I915_COLOR_UNEVICTABLE, PIN_NOEVICT); if (ret) drm_dbg(&ggtt->vm.i915->drm, "Failed to reserve top of GGTT for GuC\n");
There is an assertion in ggtt_reserve_guc_top that the global GTT is of size at least GUC_GGTT_TOP, which is not the case on a 32-bit platform; see commit 562d55d991b39ce376c492df2f7890fd6a541ffc ("drm/i915/bdw: Only use 2g GGTT for 32b platforms"). If GEM_BUG_ON is enabled, this triggers a BUG(); if GEM_BUG_ON is disabled, the subsequent reservation fails and the driver fails to initialise the device: i915 0000:00:02.0: [drm:i915_init_ggtt [i915]] Failed to reserve top of GGTT for GuC i915 0000:00:02.0: Device initialization failed (-28) i915 0000:00:02.0: Please file a bug on drm/i915; see https://gitlab.freedesktop.org/drm/intel/-/wikis/How-to-file-i915-bugs for details. i915: probe of 0000:00:02.0 failed with error -28 Make the reservation at the top of the available space, whatever that is, instead of assuming that the top will be GUC_GGTT_TOP. Fixes: 911800765ef6 ("drm/i915/uc: Reserve upper range of GGTT") Signed-off-by: Javier Pello <devel@otheo.eu> Cc: intel-gfx@lists.freedesktop.org Cc: stable@vger.kernel.org # v5.3+ --- drivers/gpu/drm/i915/gt/intel_ggtt.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)